From 488e6f1b0de0da0c8a568003ab926025cb46ec15 Mon Sep 17 00:00:00 2001
From: Tobin Richard <tobin@tobin.id.au>
Date: Sun, 25 Jul 2021 22:08:44 +1000
Subject: [PATCH] Reduce the number of times the config file is opened in
 SimpleStack (#555)

Fixes  #398.

* Do not permit repeated calls to ConfigUpdateFlow::open_file() as this was ignoring the path argument.

* Add ConfigUpdateFlow::get_fd() for callers which need access to the shared config file descriptor.

* Reuse the file descriptor for the config file that was opened by the ConfigUpdateFlow when creating FileMemorySpace instances in SimpleStackBase.

* Add missing #ifdef GTEST and fix #endif comments.
---
 src/openlcb/ConfigUpdateFlow.cxx | 14 ++++----------
 src/openlcb/ConfigUpdateFlow.hxx | 13 +++++++++++--
 src/openlcb/SimpleStack.cxx      | 26 +++++++++++++++++++-------
 3 files changed, 34 insertions(+), 19 deletions(-)

diff --git a/src/openlcb/ConfigUpdateFlow.cxx b/src/openlcb/ConfigUpdateFlow.cxx
index 13a4ae6a4..f46784f3c 100644
--- a/src/openlcb/ConfigUpdateFlow.cxx
+++ b/src/openlcb/ConfigUpdateFlow.cxx
@@ -42,16 +42,10 @@ namespace openlcb
 
 int ConfigUpdateFlow::open_file(const char *path)
 {
-    if (fd_ >= 0) return fd_;
-    if (!path)
-    {
-        fd_ = -1;
-    }
-    else
-    {
-        fd_ = ::open(path, O_RDWR);
-        HASSERT(fd_ >= 0);
-    }
+    HASSERT(fd_ < 0);
+    HASSERT(path);
+    fd_ = ::open(path, O_RDWR);
+    HASSERT(fd_ >= 0);
     return fd_;
 }
 
diff --git a/src/openlcb/ConfigUpdateFlow.hxx b/src/openlcb/ConfigUpdateFlow.hxx
index f298c1e60..2d07ea943 100644
--- a/src/openlcb/ConfigUpdateFlow.hxx
+++ b/src/openlcb/ConfigUpdateFlow.hxx
@@ -71,14 +71,22 @@ public:
     {
     }
 
-    /// Must be called once before calling anything else. Returns the file
-    /// descriptor.
+    /// Must be called once (only) before calling anything else. Returns the
+    /// file descriptor.
     int open_file(const char *path);
     /// Asynchronously invokes all update listeners with the config FD.
     void init_flow();
     /// Synchronously invokes all update listeners to factory reset.
     void factory_reset();
 
+    /// @return the file descriptor of the configuration file, or -1 if the
+    /// configuration file has not yet been opened.
+    int get_fd()
+    {
+        return fd_;
+    }
+
+#ifdef GTEST
     void TEST_set_fd(int fd)
     {
         fd_ = fd;
@@ -86,6 +94,7 @@ public:
     bool TEST_is_terminated() {
         return is_terminated();
     }
+#endif // GTEST
 
     void trigger_update() override
     {
diff --git a/src/openlcb/SimpleStack.cxx b/src/openlcb/SimpleStack.cxx
index dd0214252..66eece97a 100644
--- a/src/openlcb/SimpleStack.cxx
+++ b/src/openlcb/SimpleStack.cxx
@@ -103,8 +103,13 @@ void SimpleStackBase::start_stack(bool delay_start)
 {
 #if OPENMRN_HAVE_POSIX_FD
     // Opens the eeprom file and sends configuration update commands to all
-    // listeners.
-    configUpdateFlow_.open_file(CONFIG_FILENAME);
+    // listeners. We must only call ConfigUpdateFlow::open_file() once and it
+    // may have been done by an earlier call to create_config_file_if_needed()
+    // or check_version_and_factory_reset().
+    if (configUpdateFlow_.get_fd() < 0)
+    {
+        configUpdateFlow_.open_file(CONFIG_FILENAME);
+    }
     configUpdateFlow_.init_flow();
 #endif // have posix fd
 
@@ -139,12 +144,12 @@ void SimpleStackBase::default_start_node()
 #if OPENMRN_HAVE_POSIX_FD 
     {
         auto *space = new FileMemorySpace(
-            SNIP_DYNAMIC_FILENAME, sizeof(SimpleNodeDynamicValues));
+            configUpdateFlow_.get_fd(), sizeof(SimpleNodeDynamicValues));
         memoryConfigHandler_.registry()->insert(
             node(), MemoryConfigDefs::SPACE_ACDI_USR, space);
         additionalComponents_.emplace_back(space);
     }
-#endif // NOT ARDUINO, YES ESP32
+#endif // OPENMRN_HAVE_POSIX_FD
     size_t cdi_size = strlen(CDI_DATA);
     if (cdi_size > 0)
     {
@@ -157,12 +162,13 @@ void SimpleStackBase::default_start_node()
 #if OPENMRN_HAVE_POSIX_FD
     if (CONFIG_FILENAME != nullptr)
     {
-        auto *space = new FileMemorySpace(CONFIG_FILENAME, CONFIG_FILE_SIZE);
+        auto *space =
+            new FileMemorySpace(configUpdateFlow_.get_fd(), CONFIG_FILE_SIZE);
         memory_config_handler()->registry()->insert(
             node(), openlcb::MemoryConfigDefs::SPACE_CONFIG, space);
         additionalComponents_.emplace_back(space);
     }
-#endif // NOT ARDUINO, YES ESP32
+#endif // OPENMRN_HAVE_POSIX_FD
 }
 
 SimpleTrainCanStack::SimpleTrainCanStack(
@@ -220,6 +226,7 @@ int SimpleStackBase::create_config_file_if_needed(const InternalConfigData &cfg,
     uint16_t expected_version, unsigned file_size)
 {
     HASSERT(CONFIG_FILENAME);
+    HASSERT(configUpdateFlow_.get_fd() < 0);
     struct stat statbuf;
     bool reset = false;
     bool extend = false;
@@ -318,7 +325,12 @@ int SimpleStackBase::check_version_and_factory_reset(
     const InternalConfigData &cfg, uint16_t expected_version, bool force)
 {
     HASSERT(CONFIG_FILENAME);
-    int fd = configUpdateFlow_.open_file(CONFIG_FILENAME);
+    int fd = configUpdateFlow_.get_fd();
+    if (fd < 0)
+    {
+        fd = configUpdateFlow_.open_file(CONFIG_FILENAME);
+    }
+
     if (cfg.version().read(fd) != expected_version)
     {
         /// @todo (balazs.racz): We need to clear the eeprom. Best would be if