cleanups in Storage-related signals

diff --git a/include/simgrid/s4u/Storage.hpp b/include/simgrid/s4u/Storage.hpp
index 3204e04..13ae9be 100644 (file)
--- a/include/simgrid/s4u/Storage.hpp
+++ b/include/simgrid/s4u/Storage.hpp
@@ -57,11 +57,12 @@ public:
   surf::StorageImpl* getImpl() { return pimpl_; }
 
   /* The signals */
-  /** @brief Callback signal fired when a new Link is created */
-  static simgrid::xbt::signal<void(s4u::Storage&)> onCreation;
-
-  /** @brief Callback signal fired when a Link is destroyed */
-  static simgrid::xbt::signal<void(s4u::Storage&)> onDestruction;
+  /** @brief Callback signal fired when a new Storage is created */
+  static simgrid::xbt::signal<void(s4u::Storage&)> on_creation;
+  /** @brief Callback signal fired when a Storage is destroyed */
+  static simgrid::xbt::signal<void(s4u::Storage&)> on_destruction;
+  /** @brief Callback signal fired when a Storage's state changes */
+  static simgrid::xbt::signal<void(s4u::Storage&)> on_state_change;
 
   Host* attached_to_              = nullptr;
 

diff --git a/src/kernel/EngineImpl.cpp b/src/kernel/EngineImpl.cpp
index dcda57d..ad6799f 100644 (file)
--- a/src/kernel/EngineImpl.cpp
+++ b/src/kernel/EngineImpl.cpp
@@ -40,7 +40,7 @@ EngineImpl::~EngineImpl()
 
   for (auto const& kv : storages_)
     if (kv.second)
-      delete kv.second->getImpl();
+      kv.second->getImpl()->destroy();
 }
 }
 }

diff --git a/src/plugins/file_system/s4u_FileSystem.cpp b/src/plugins/file_system/s4u_FileSystem.cpp
index d89ecd0..5957459 100644 (file)
--- a/src/plugins/file_system/s4u_FileSystem.cpp
+++ b/src/plugins/file_system/s4u_FileSystem.cpp
@@ -335,11 +335,6 @@ static void on_storage_creation(simgrid::s4u::Storage& st)
   st.extension_set(new FileSystemStorageExt(&st));
 }
 
-static void on_storage_destruction(simgrid::s4u::Storage& st)
-{
-  delete st.extension<FileSystemStorageExt>();
-}
-
 static void on_host_creation(simgrid::s4u::Host& host)
 {
   host.extension_set<FileDescriptorHostExt>(new FileDescriptorHostExt());
@@ -354,8 +349,7 @@ void sg_storage_file_system_init()
 
   if (not FileSystemStorageExt::EXTENSION_ID.valid()) {
     FileSystemStorageExt::EXTENSION_ID = simgrid::s4u::Storage::extension_create<FileSystemStorageExt>();
-    simgrid::s4u::Storage::onCreation.connect(&on_storage_creation);
-    simgrid::s4u::Storage::onDestruction.connect(&on_storage_destruction);
+    simgrid::s4u::Storage::on_creation.connect(&on_storage_creation);
   }
 
   if (not FileDescriptorHostExt::EXTENSION_ID.valid()) {

diff --git a/src/s4u/s4u_Storage.cpp b/src/s4u/s4u_Storage.cpp
index 95e250c..a0e7699 100644 (file)
--- a/src/s4u/s4u_Storage.cpp
+++ b/src/s4u/s4u_Storage.cpp
@@ -83,8 +83,9 @@ sg_size_t Storage::write(sg_size_t size)
 /*************
  * Callbacks *
  *************/
-simgrid::xbt::signal<void(s4u::Storage&)> Storage::onCreation;
-simgrid::xbt::signal<void(s4u::Storage&)> Storage::onDestruction;
+simgrid::xbt::signal<void(s4u::Storage&)> Storage::on_creation;
+simgrid::xbt::signal<void(s4u::Storage&)> Storage::on_destruction;
+simgrid::xbt::signal<void(s4u::Storage&)> Storage::on_state_change;
 
 } /* namespace s4u */
 } /* namespace simgrid */

diff --git a/src/simix/smx_global.cpp b/src/simix/smx_global.cpp
index f01bd8f..a4fe1c1 100644 (file)
--- a/src/simix/smx_global.cpp
+++ b/src/simix/smx_global.cpp
@@ -214,9 +214,9 @@ void SIMIX_global_init(int *argc, char **argv)
         host.extension_set<simgrid::simix::Host>(new simgrid::simix::Host());
     });
 
-    simgrid::surf::storageCreatedCallbacks.connect([](simgrid::surf::StorageImpl* storage) {
-      sg_storage_t s = simgrid::s4u::Storage::byName(storage->get_cname());
-      xbt_assert(s != nullptr, "Storage not found for name %s", storage->get_cname());
+    simgrid::s4u::Storage::on_creation.connect([](simgrid::s4u::Storage& storage) {
+      sg_storage_t s = simgrid::s4u::Storage::byName(storage.get_cname());
+      xbt_assert(s != nullptr, "Storage not found for name %s", storage.get_cname());
     });
   }
 

diff --git a/src/surf/StorageImpl.cpp b/src/surf/StorageImpl.cpp
index 999ec2a..5df10ec 100644 (file)
--- a/src/surf/StorageImpl.cpp
+++ b/src/surf/StorageImpl.cpp
@@ -16,16 +16,6 @@ simgrid::surf::StorageModel* surf_storage_model = nullptr;
 namespace simgrid {
 namespace surf {
 
-/*************
- * Callbacks *
- *************/
-
-simgrid::xbt::signal<void(StorageImpl*)> storageCreatedCallbacks;
-simgrid::xbt::signal<void(StorageImpl*)> storageDestructedCallbacks;
-simgrid::xbt::signal<void(StorageImpl*, int, int)> storageStateChangedCallbacks; // signature: wasOn, isOn
-simgrid::xbt::signal<void(StorageAction*, kernel::resource::Action::State, kernel::resource::Action::State)>
-    storageActionStateChangedCallbacks;
-
 /*********
  * Model *
  *********/
@@ -62,7 +52,20 @@ StorageImpl::StorageImpl(kernel::resource::Model* model, std::string name, kerne
 
 StorageImpl::~StorageImpl()
 {
-  storageDestructedCallbacks(this);
+  xbt_assert(currentlyDestroying_, "Don't delete Storages directly. Call destroy() instead.");
+}
+
+/** @brief Fire the required callbacks and destroy the object
+ *
+ * Don't delete directly a Storage, call s->destroy() instead.
+ */
+void StorageImpl::destroy()
+{
+  if (not currentlyDestroying_) {
+    currentlyDestroying_ = true;
+    s4u::Storage::on_destruction(this->piface_);
+    delete this;
+  }
 }
 
 bool StorageImpl::is_used()
@@ -80,16 +83,18 @@ void StorageImpl::turn_on()
 {
   if (is_off()) {
     Resource::turn_on();
-    storageStateChangedCallbacks(this, 0, 1);
+    s4u::Storage::on_state_change(this->piface_);
   }
 }
 void StorageImpl::turn_off()
 {
   if (is_on()) {
     Resource::turn_off();
-    storageStateChangedCallbacks(this, 1, 0);
+    s4u::Storage::on_state_change(this->piface_);
   }
 }
+xbt::signal<void(StorageAction*, kernel::resource::Action::State, kernel::resource::Action::State)>
+    StorageAction::on_state_change;
 
 /**********
  * Action *
@@ -98,7 +103,7 @@ void StorageAction::set_state(Action::State state)
 {
   Action::State old = get_state();
   Action::set_state(state);
-  storageActionStateChangedCallbacks(this, old, state);
+  on_state_change(this, old, state);
 }
 }
 }

diff --git a/src/surf/StorageImpl.hpp b/src/surf/StorageImpl.hpp
index 29b98f8..530c728 100644 (file)
--- a/src/surf/StorageImpl.hpp
+++ b/src/surf/StorageImpl.hpp
@@ -29,25 +29,6 @@ class StorageAction;
  * Callbacks *
  *************/
 
-/** @ingroup SURF_callbacks
- * @brief Callbacks handler which emit the callbacks after Storage creation *
- * @details Callback functions have the following signature: `void(Storage*)`
- */
-XBT_PUBLIC_DATA simgrid::xbt::signal<void(StorageImpl*)> storageCreatedCallbacks;
-
-/** @ingroup SURF_callbacks
- * @brief Callbacks handler which emit the callbacks after Storage destruction *
- * @details Callback functions have the following signature: `void(StoragePtr)`
- */
-XBT_PUBLIC_DATA simgrid::xbt::signal<void(StorageImpl*)> storageDestructedCallbacks;
-
-/** @ingroup SURF_callbacks
- * @brief Callbacks handler which emit the callbacks after Storage State changed *
- * @details Callback functions have the following signature: `void(StorageAction *action, int previouslyOn, int
- * currentlyOn)`
- */
-XBT_PUBLIC_DATA simgrid::xbt::signal<void(StorageImpl*, int, int)> storageStateChangedCallbacks;
-
 /** @ingroup SURF_callbacks
  * @brief Callbacks handler which emit the callbacks after StorageAction State changed *
  * @details Callback functions have the following signature: `void(StorageAction *action,
@@ -55,7 +36,7 @@ XBT_PUBLIC_DATA simgrid::xbt::signal<void(StorageImpl*, int, int)> storageStateC
  */
 XBT_PUBLIC_DATA
 simgrid::xbt::signal<void(StorageAction*, kernel::resource::Action::State, kernel::resource::Action::State)>
-    storageActionStateChangedCallbacks;
+    on_state_change;
 
 /*********
  * Model *
@@ -99,6 +80,8 @@ public:
   void turn_on() override;
   void turn_off() override;
 
+  void destroy(); // Must be called instead of the destructor
+
   /**
    * @brief Read a file
    *
@@ -124,6 +107,7 @@ public:
   sg_size_t size_;          // Only used at parsing time then goes to the FileSystemExtension
 
 private:
+  bool currentlyDestroying_ = false;
   // Name of the host to which this storage is attached. Only used at platform parsing time, then the interface stores
   // the Host directly.
   std::string attach_;
@@ -146,6 +130,9 @@ enum e_surf_action_storage_type_t {
  */
 class StorageAction : public kernel::resource::Action {
 public:
+  static xbt::signal<void(StorageAction*, kernel::resource::Action::State, kernel::resource::Action::State)>
+      on_state_change;
+
   /**
    * @brief StorageAction constructor
    *

diff --git a/src/surf/storage_n11.cpp b/src/surf/storage_n11.cpp
index e1a0cd2..a36ca2e 100644 (file)
--- a/src/surf/storage_n11.cpp
+++ b/src/surf/storage_n11.cpp
@@ -54,7 +54,6 @@ StorageImpl* StorageN11Model::createStorage(std::string id, std::string type_id,
 
   StorageImpl* storage =
       new StorageN11(this, id, get_maxmin_system(), Bread, Bwrite, type_id, content_name, storage_type->size, attach);
-  storageCreatedCallbacks(storage);
 
   XBT_DEBUG("SURF storage create resource\n\t\tid '%s'\n\t\ttype '%s'\n\t\tBread '%f'\n", id.c_str(), type_id.c_str(),
             Bread);
@@ -93,7 +92,7 @@ StorageN11::StorageN11(StorageModel* model, std::string name, kernel::lmm::Syste
     : StorageImpl(model, name, maxminSystem, bread, bwrite, type_id, content_name, size, attach)
 {
   XBT_DEBUG("Create resource with Bread '%f' Bwrite '%f' and Size '%llu'", bread, bwrite, size);
-  simgrid::s4u::Storage::onCreation(this->piface_);
+  simgrid::s4u::Storage::on_creation(this->piface_);
 }
 
 StorageAction* StorageN11::read(sg_size_t size)