Dev/shm free obsolete (#1274)

* freeing obsolete shm withoua a 'isValid' should access raw pointers. Need to move this all into the shm class

* fixed obsolete shm free issue

* minor

* ensuring the test works platform independent for size of int

slsDetectorSoftware/src/CtbConfig.h

@@ -3,7 +3,16 @@
 #include <vector>
 namespace sls {
 
+#define CTB_SHMAPIVERSION 0x250820
+#define CTB_SHMVERSION    0x250820
+
 class CtbConfig {
+  public:
+    /** fixed pattern */
+    int shmversion{CTB_SHMVERSION};
+    bool isValid{true}; // false if freed to block access from python or c++ api
+                        /** end of fixed pattern */
+  private:
     static constexpr size_t name_length = 20;
     static constexpr size_t num_dacs = 18;
     static constexpr size_t num_adcs = 32;
@@ -56,7 +65,6 @@ class CtbConfig {
     std::string getSlowADCName(size_t index) const;
     std::vector<std::string> getSlowADCNames() const;
     static const char *shm_tag();
-    bool isValid{true}; // false if freed to block access from python or c++ api
 };
 
 } // namespace sls

slsDetectorSoftware/src/Detector.cpp

@@ -30,7 +30,6 @@ void freeSharedMemory(const int detectorIndex, const int moduleIndex) {
         SharedMemory<sharedModule> moduleShm(detectorIndex, moduleIndex);
         if (moduleShm.exists()) {
             moduleShm.openSharedMemory(false);
-            moduleShm()->isValid = false;
             moduleShm.removeSharedMemory();
         }
         return;
@@ -43,7 +42,6 @@ void freeSharedMemory(const int detectorIndex, const int moduleIndex) {
     if (detectorShm.exists()) {
         detectorShm.openSharedMemory(false);
         numDetectors = detectorShm()->totalNumberOfModules;
-        detectorShm()->isValid = false;
         detectorShm.removeSharedMemory();
     }
 
@@ -51,16 +49,14 @@ void freeSharedMemory(const int detectorIndex, const int moduleIndex) {
         SharedMemory<sharedModule> moduleShm(detectorIndex, i);
         if (moduleShm.exists()) {
             moduleShm.openSharedMemory(false);
-            moduleShm()->isValid = false;
+            moduleShm.removeSharedMemory();
         }
-        moduleShm.removeSharedMemory();
     }
 
     // Ctb configuration
     SharedMemory<CtbConfig> ctbShm(detectorIndex, -1, CtbConfig::shm_tag());
     if (ctbShm.exists()) {
         ctbShm.openSharedMemory(false);
-        ctbShm()->isValid = false;
         ctbShm.removeSharedMemory();
     }
 }

slsDetectorSoftware/src/DetectorImpl.h

@@ -24,7 +24,7 @@ class detectorData;
 class Module;
 
 #define DETECTOR_SHMAPIVERSION 0x190809
-#define DETECTOR_SHMVERSION    0x250729
+#define DETECTOR_SHMVERSION    0x250820
 #define SHORT_STRING_LENGTH    50
 
 /**
@@ -51,17 +51,17 @@ struct sharedDetector {
     int totalNumberOfModules;
     slsDetectorDefs::detectorType detType;
 
-    bool isValid{true}; // false if freed to block access from python or c++ api
-
-    /** END OF FIXED PATTERN
-     * -----------------------------------------------*/
-
     /** Number of modules operated at once */
     slsDetectorDefs::xy numberOfModules;
 
     /**  max number of channels for complete detector*/
     slsDetectorDefs::xy numberOfChannels;
 
+    bool isValid{true}; // false if freed to block access from python or c++ api
+
+    /** END OF FIXED PATTERN
+     * -----------------------------------------------*/
+
     bool acquiringFlag;
     bool initialChecks;
     bool gapPixels;

slsDetectorSoftware/src/Module.h

@@ -19,7 +19,7 @@ namespace sls {
 class ServerInterface;
 
 #define MODULE_SHMAPIVERSION 0x190726
-#define MODULE_SHMVERSION    0x250729
+#define MODULE_SHMVERSION    0x250820
 
 /**
  * @short structure allocated in shared memory to store Module settings for

slsDetectorSoftware/src/SharedMemory.h

@@ -27,17 +27,25 @@
 
 namespace sls {
 
-struct sharedDetector;
+struct CtbConfig;
+// struct sharedDetector;
 
-#define SHM_DETECTOR_PREFIX "/slsDetectorPackage_detector_"
+#define SHM_IS_VALID_CHECK_VERSION 0x250820
-#define SHM_MODULE_PREFIX   "_module_"
+#define SHM_DETECTOR_PREFIX        "/slsDetectorPackage_detector_"
-#define SHM_ENV_NAME        "SLSDETNAME"
+#define SHM_MODULE_PREFIX          "_module_"
+#define SHM_ENV_NAME               "SLSDETNAME"
+
+template <typename T, typename U> constexpr bool is_type() {
+    return std::is_same_v<std::decay_t<U>, T>;
+}
 
 template <typename T> class SharedMemory {
 
+#ifndef DISABLE_STATIC_ASSERT
     static_assert(has_bool_isValid<T>::value,
                   "SharedMemory requires the struct to have a bool member "
                   "named 'isValid'");
+#endif
 
     static constexpr int NAME_MAX_LENGTH = 255;
     std::string name;
@@ -72,21 +80,49 @@ template <typename T> class SharedMemory {
             unmapSharedMemory();
     }
 
+    bool memoryHasValidFlag() const {
+        if constexpr (is_type<CtbConfig, T>()) {
+            // CtbConfig did not have shmversion before, so exact value check
+            if (shared_struct->shmversion == SHM_IS_VALID_CHECK_VERSION) {
+                return true;
+            }
+        } else if (shared_struct->shmversion >= SHM_IS_VALID_CHECK_VERSION) {
+            return true;
+        }
+        return false;
+    }
+
+    bool checkValidity() const {
+        if (memoryHasValidFlag() && !shared_struct->isValid)
+            return false;
+        return true;
+    }
+
+    void invalidate() {
+        // also called by obsolete shm test structure (so check added)
+        if constexpr (has_bool_isValid<T>::value) {
+            if (memoryHasValidFlag())
+                shared_struct->isValid = false;
+        }
+    }
+
     T *operator()() {
         if (!shared_struct)
             throw SharedMemoryError(getNoShmAccessMessage());
-        if (!shared_struct->isValid) {
+
+        if (!checkValidity())
             throw SharedMemoryError(getInvalidShmMessage());
-        }
+
         return shared_struct;
     }
 
     const T *operator()() const {
         if (!shared_struct)
             throw SharedMemoryError(getNoShmAccessMessage());
-        if (!shared_struct->isValid) {
+
+        if (!checkValidity())
             throw SharedMemoryError(getInvalidShmMessage());
-        }
+
         return shared_struct;
     }
 
@@ -146,6 +182,7 @@ template <typename T> class SharedMemory {
     }
 
     void removeSharedMemory() {
+        invalidate();
         unmapSharedMemory();
         if (shm_unlink(name.c_str()) < 0) {
             // silent exit if shm did not exist anyway

slsDetectorSoftware/tests/test-CtbConfig.cpp

@@ -10,8 +10,10 @@
 namespace sls {
 
 TEST_CASE("Default construction") {
-    static_assert(sizeof(CtbConfig) == ((18 + 32 + 64 + 5 + 8) * 20 + 1),
+    std::cout << "size of int:" << sizeof(int) << std::endl;
-                  "Size of CtbConfig does not match");
+    static_assert(sizeof(CtbConfig) ==
+                      (2 * sizeof(int) + (18 + 32 + 64 + 5 + 8) * 20),
+                  "Size of CtbConfig does not match ");
 
     CtbConfig c;
     auto dacnames = c.getDacNames();

slsDetectorSoftware/tests/test-SharedMemory.cpp

@@ -1,31 +1,96 @@
 // SPDX-License-Identifier: LGPL-3.0-or-other
 // Copyright (C) 2021 Contributors to the SLS Detector Package
 
+#define DISABLE_STATIC_ASSERT // to be able to test obsolete shm without isValid
 #include "SharedMemory.h"
 #include "catch.hpp"
 #include "sls/string_utils.h"
+#include <filesystem>
 
 namespace sls {
 
 struct Data {
+    int shmversion{SHM_IS_VALID_CHECK_VERSION};
     int x;
     double y;
     char mess[50];
     bool isValid{true};
 };
 
+struct ObsoleteData {
+    int shmversion{SHM_IS_VALID_CHECK_VERSION - 10};
+    int x;
+    double y;
+    char mess[50];
+};
+
+struct ObsoleteCtbData {
+    int x;
+    double y;
+    char mess[50];
+};
+
 void freeShm(const int dindex, const int mIndex) {
     SharedMemory<Data> shm(dindex, mIndex);
     if (shm.exists()) {
         shm.openSharedMemory(false);
-        shm()->isValid = false;
         shm.removeSharedMemory();
     }
 }
 
 constexpr int shm_id = 10;
+const std::string file_path =
+    std::string("/dev/shm/slsDetectorPackage_detector_") +
+    std::to_string(shm_id);
 
-TEST_CASE("Create SharedMemory read and write", "[detector]") {
+TEST_CASE("Free obsolete (without isValid)", "[detector][shm]") {
+
+    // ensure its clean to start
+    freeShm(shm_id, -1);
+
+    {
+        // create actual shm and free
+        SharedMemory<Data> shm(shm_id, -1);
+        shm.createSharedMemory();
+        REQUIRE(std::filesystem::exists(file_path) == true);
+        REQUIRE(shm.memoryHasValidFlag() == true);
+        REQUIRE_NOTHROW(freeShm(shm_id, -1));
+        REQUIRE(std::filesystem::exists(file_path) == false);
+    }
+
+    {
+        // create obsolete and free
+        SharedMemory<ObsoleteData> shm(shm_id, -1);
+        shm.createSharedMemory();
+        REQUIRE(std::filesystem::exists(file_path) == true);
+        shm.unmapSharedMemory();
+    }
+
+    {
+        SharedMemory<Data> shm(shm_id, -1);
+        shm.openSharedMemory(false);
+        REQUIRE(shm.memoryHasValidFlag() == false);
+        REQUIRE_NOTHROW(freeShm(shm_id, -1));
+        REQUIRE(std::filesystem::exists(file_path) == false);
+    }
+
+    {
+        // create obsolete ctb (without shmversion) and free
+        SharedMemory<ObsoleteCtbData> shm(shm_id, -1);
+        shm.createSharedMemory();
+        REQUIRE(std::filesystem::exists(file_path) == true);
+        shm.unmapSharedMemory();
+    }
+    {
+        SharedMemory<Data> shm(shm_id, -1);
+        shm.openSharedMemory(false);
+        REQUIRE(shm.memoryHasValidFlag() == false);
+        REQUIRE_NOTHROW(freeShm(shm_id, -1));
+        REQUIRE(std::filesystem::exists(file_path) == false);
+    }
+}
+
+TEST_CASE("Create SharedMemory read and write", "[detector][shm]") {
     SharedMemory<Data> shm(shm_id, -1);
     if (shm.exists()) {
         shm.removeSharedMemory();
@@ -36,7 +101,6 @@ TEST_CASE("Create SharedMemory read and write", "[detector]") {
     std::string env_name = env_p ? ("_" + std::string(env_p)) : "";
     CHECK(shm.getName() == std::string("/slsDetectorPackage_detector_") +
                                std::to_string(shm_id) + env_name);
-
     shm()->x = 3;
     shm()->y = 5.7;
     strcpy_safe(shm()->mess, "Some string");
@@ -45,13 +109,12 @@ TEST_CASE("Create SharedMemory read and write", "[detector]") {
     CHECK(shm()->y == 5.7);
     CHECK(std::string(shm()->mess) == "Some string");
 
-    shm.unmapSharedMemory();
     shm.removeSharedMemory();
 
     CHECK(shm.exists() == false);
 }
 
-TEST_CASE("Open existing SharedMemory and read", "[detector]") {
+TEST_CASE("Open existing SharedMemory and read", "[detector][shm]") {
     {
         SharedMemory<Data> shm(shm_id, -1);
         if (shm.exists()) {
@@ -70,7 +133,7 @@ TEST_CASE("Open existing SharedMemory and read", "[detector]") {
 }
 
 TEST_CASE("Creating a second shared memory with the same name throws",
-          "[detector]") {
+          "[detector][shm]") {
 
     SharedMemory<Data> shm0(shm_id, -1);
     SharedMemory<Data> shm1(shm_id, -1);
@@ -80,7 +143,7 @@ TEST_CASE("Creating a second shared memory with the same name throws",
     shm0.removeSharedMemory();
 }
 
-TEST_CASE("Open two shared memories to the same place", "[detector]") {
+TEST_CASE("Open two shared memories to the same place", "[detector][shm]") {
 
     // Create the first shared memory
     SharedMemory<Data> shm(shm_id, -1);
@@ -105,7 +168,7 @@ TEST_CASE("Open two shared memories to the same place", "[detector]") {
     CHECK(shm2.exists() == false);
 }
 
-TEST_CASE("Move SharedMemory", "[detector]") {
+TEST_CASE("Move SharedMemory", "[detector][shm]") {
     const char *env_p = std::getenv("SLSDETNAME");
     std::string env_name = env_p ? ("_" + std::string(env_p)) : "";
 
@@ -126,7 +189,7 @@ TEST_CASE("Move SharedMemory", "[detector]") {
     shm2.removeSharedMemory();
 }
 
-TEST_CASE("Create several shared memories", "[detector]") {
+TEST_CASE("Create several shared memories", "[detector][shm]") {
     const char *env_p = std::getenv("SLSDETNAME");
     std::string env_name = env_p ? ("_" + std::string(env_p)) : "";
 
@@ -182,7 +245,7 @@ TEST_CASE("Create create a shared memory with a tag when SLSDETNAME is set") {
         setenv(SHM_ENV_NAME, old_slsdetname.c_str(), 1);
 }
 
-TEST_CASE("Access to already freed shm object", "[detector]") {
+TEST_CASE("Access to already freed shm object", "[detector][shm]") {
     SharedMemory<Data> shm(shm_id, -1);
     shm.createSharedMemory();
     shm()->x = 10;