From 15cbaa509e80ca74a5894af5b635d50cdd39abdb Mon Sep 17 00:00:00 2001 From: Dhanya Thattil Date: Thu, 21 Aug 2025 14:32:15 +0200 Subject: [PATCH] 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 | 10 ++- slsDetectorSoftware/src/Detector.cpp | 6 +- slsDetectorSoftware/src/DetectorImpl.h | 12 +-- slsDetectorSoftware/src/Module.h | 2 +- slsDetectorSoftware/src/SharedMemory.h | 53 ++++++++++-- slsDetectorSoftware/tests/test-CtbConfig.cpp | 6 +- .../tests/test-SharedMemory.cpp | 83 ++++++++++++++++--- 7 files changed, 139 insertions(+), 33 deletions(-) diff --git a/slsDetectorSoftware/src/CtbConfig.h b/slsDetectorSoftware/src/CtbConfig.h index ed8fc5ee6..0e2fe68d7 100644 --- a/slsDetectorSoftware/src/CtbConfig.h +++ b/slsDetectorSoftware/src/CtbConfig.h @@ -3,7 +3,16 @@ #include 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 getSlowADCNames() const; static const char *shm_tag(); - bool isValid{true}; // false if freed to block access from python or c++ api }; } // namespace sls diff --git a/slsDetectorSoftware/src/Detector.cpp b/slsDetectorSoftware/src/Detector.cpp index 3887403bc..a1e9eb159 100644 --- a/slsDetectorSoftware/src/Detector.cpp +++ b/slsDetectorSoftware/src/Detector.cpp @@ -30,7 +30,6 @@ void freeSharedMemory(const int detectorIndex, const int moduleIndex) { SharedMemory 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 moduleShm(detectorIndex, i); if (moduleShm.exists()) { moduleShm.openSharedMemory(false); - moduleShm()->isValid = false; + moduleShm.removeSharedMemory(); } - moduleShm.removeSharedMemory(); } // Ctb configuration SharedMemory ctbShm(detectorIndex, -1, CtbConfig::shm_tag()); if (ctbShm.exists()) { ctbShm.openSharedMemory(false); - ctbShm()->isValid = false; ctbShm.removeSharedMemory(); } } diff --git a/slsDetectorSoftware/src/DetectorImpl.h b/slsDetectorSoftware/src/DetectorImpl.h index bcc1b53ef..7358e3eab 100644 --- a/slsDetectorSoftware/src/DetectorImpl.h +++ b/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; diff --git a/slsDetectorSoftware/src/Module.h b/slsDetectorSoftware/src/Module.h index 040a29ee8..d00f4ca4c 100644 --- a/slsDetectorSoftware/src/Module.h +++ b/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 diff --git a/slsDetectorSoftware/src/SharedMemory.h b/slsDetectorSoftware/src/SharedMemory.h index 4631e1dc0..43b5cde52 100644 --- a/slsDetectorSoftware/src/SharedMemory.h +++ b/slsDetectorSoftware/src/SharedMemory.h @@ -27,17 +27,25 @@ namespace sls { -struct sharedDetector; +struct CtbConfig; +// struct sharedDetector; -#define SHM_DETECTOR_PREFIX "/slsDetectorPackage_detector_" -#define SHM_MODULE_PREFIX "_module_" -#define SHM_ENV_NAME "SLSDETNAME" +#define SHM_IS_VALID_CHECK_VERSION 0x250820 +#define SHM_DETECTOR_PREFIX "/slsDetectorPackage_detector_" +#define SHM_MODULE_PREFIX "_module_" +#define SHM_ENV_NAME "SLSDETNAME" + +template constexpr bool is_type() { + return std::is_same_v, T>; +} template class SharedMemory { +#ifndef DISABLE_STATIC_ASSERT static_assert(has_bool_isValid::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 class SharedMemory { unmapSharedMemory(); } + bool memoryHasValidFlag() const { + if constexpr (is_type()) { + // 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::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 class SharedMemory { } void removeSharedMemory() { + invalidate(); unmapSharedMemory(); if (shm_unlink(name.c_str()) < 0) { // silent exit if shm did not exist anyway diff --git a/slsDetectorSoftware/tests/test-CtbConfig.cpp b/slsDetectorSoftware/tests/test-CtbConfig.cpp index eb3429795..167741292 100644 --- a/slsDetectorSoftware/tests/test-CtbConfig.cpp +++ b/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), - "Size of CtbConfig does not match"); + std::cout << "size of int:" << sizeof(int) << std::endl; + 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(); diff --git a/slsDetectorSoftware/tests/test-SharedMemory.cpp b/slsDetectorSoftware/tests/test-SharedMemory.cpp index fce86b05d..89865bf2f 100644 --- a/slsDetectorSoftware/tests/test-SharedMemory.cpp +++ b/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 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 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 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 shm(shm_id, -1); + shm.createSharedMemory(); + REQUIRE(std::filesystem::exists(file_path) == true); + shm.unmapSharedMemory(); + } + + { + SharedMemory 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 shm(shm_id, -1); + shm.createSharedMemory(); + REQUIRE(std::filesystem::exists(file_path) == true); + shm.unmapSharedMemory(); + } + { + SharedMemory 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 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 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 shm0(shm_id, -1); SharedMemory 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 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 shm(shm_id, -1); shm.createSharedMemory(); shm()->x = 10;