[cpp-sp] branch main updated: Rework ReloadableFile back to align to existing design.
Scott Cantor
cantor.2 at osu.edu
Wed Dec 4 21:02:11 UTC 2024
This is an automated email from the git hooks/post-receive script.
scantor pushed a commit to branch main
in repository cpp-sp.
View the commit online:
http://git.shibboleth.net/view/?p=cpp-sp.git;a=commit;h=32626982ba942c666f6cd09af625102f1da9b995
The following commit(s) were added to refs/heads/main by this push:
new 32626982 Rework ReloadableFile back to align to existing design.
32626982 is described below
commit 32626982ba942c666f6cd09af625102f1da9b995
Author: Scott Cantor <cantor.2 at osu.edu>
AuthorDate: Wed Dec 4 16:02:03 2024 -0500
Rework ReloadableFile back to align to existing design.
---
shibsp/Makefile.am | 1 +
shibsp/util/Lockable.h | 74 ++++++++++++++++++
shibsp/util/ReloadableFile.cpp | 91 +++++++++++++++++-----
shibsp/util/ReloadableFile.h | 65 ++++++++++------
.../util/reloadablefile/console-shibboleth.ini | 1 +
tests/data/util/reloadablefile/external.xml | 1 +
tests/data/util/reloadablefile/inline.xml | 8 ++
.../{requestmap1.xml => requestmap.xml} | 0
tests/util/ReloadableFileTests.cpp | 79 +++++++++----------
9 files changed, 231 insertions(+), 89 deletions(-)
diff --git a/shibsp/Makefile.am b/shibsp/Makefile.am
index 1355b6e3..a531ea03 100644
--- a/shibsp/Makefile.am
+++ b/shibsp/Makefile.am
@@ -71,6 +71,7 @@ utilinclude_HEADERS = \
util/Date.h \
util/DOMPropertySet.h \
util/IPRange.h \
+ util/Lockable.h \
util/PathResolver.h \
util/PropertySet.h \
util/ReloadableFile.h \
diff --git a/shibsp/util/Lockable.h b/shibsp/util/Lockable.h
new file mode 100644
index 00000000..5f9930eb
--- /dev/null
+++ b/shibsp/util/Lockable.h
@@ -0,0 +1,74 @@
+/**
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+/**
+ * @file shibsp/util/Lockable.h
+ *
+ * Interfaces for C++ locking template compatibility.
+ */
+
+#ifndef __shibsp_lockable_h__
+#define __shibsp_lockable_h__
+
+#include <shibsp/base.h>
+
+namespace shibsp {
+
+ /**
+ * BasicLockable semantics for exclusive locking.
+ */
+ class SHIBSP_API BasicLockable
+ {
+ public:
+ virtual void lock()=0;
+ virtual bool try_lock()=0;
+ virtual void unlock()=0;
+ };
+
+ /**
+ * A class supplying a BasicLockable implementation as a no-op.
+ */
+ class SHIBSP_API NoOpBasicLockable : public virtual BasicLockable
+ {
+ public:
+ void lock() {}
+ bool try_lock() { return true; }
+ void unlock() {}
+ };
+
+ /**
+ * SharedLockable semantics for shared locking.
+ */
+ class SHIBSP_API SharedLockable
+ {
+ public:
+ virtual void lock_shared()=0;
+ virtual bool try_lock_shared()=0;
+ virtual void unlock_shared()=0;
+ };
+
+ /**
+ * A class supplying a SharedLockable implementation as a no-op.
+ */
+ class SHIBSP_API NoOpSharedLockable : public virtual SharedLockable
+ {
+ public:
+ void lock_shared() {}
+ bool try_lock_shared() { return true; }
+ void unlock_shared() {}
+ };
+
+};
+
+#endif /* __shibsp_lockable_h__ */
diff --git a/shibsp/util/ReloadableFile.cpp b/shibsp/util/ReloadableFile.cpp
index 7a845e6a..5e4f9a45 100644
--- a/shibsp/util/ReloadableFile.cpp
+++ b/shibsp/util/ReloadableFile.cpp
@@ -33,29 +33,55 @@
#include <sys/types.h>
#include <sys/stat.h>
+#include <boost/property_tree/xml_parser.hpp>
using namespace boost::property_tree;
using namespace shibsp;
using namespace std;
-ReloadableFile::ReloadableFile(const std::string& path, Category& log, bool reloadChanges)
- : m_log(log), m_source(path), m_filestamp(0)
+namespace {
+ // More an experiment than anything but it does encapsulate the conversion.
+ struct string_to_bool_translator {
+ typedef std::string internal_type;
+ typedef bool external_type;
+
+ boost::optional<bool> get_value(const string &s) {
+ if (s == "true" || s == "1") {
+ return boost::make_optional(true);
+ } else if (s == "false" || s == "0") {
+ return boost::make_optional(false);
+ } else {
+ return boost::none;
+ }
+ }
+ };
+};
+
+const char ReloadableFile::PATH_PROP_NAME[] = "path";
+const char ReloadableFile::RELOAD_CHANGES_PROP_NAME[] = "reloadChanges";
+
+ReloadableFile::ReloadableFile(const ptree& pt, Category& log) : m_root(pt), m_log(log), m_filestamp(0)
#ifdef HAVE_CXX17
, m_lock(nullptr)
#elif HAVE_CXX14
, m_lock(nullptr)
#endif
{
- AgentConfig::getConfig().getPathResolver().resolve(m_source, PathResolver::SHIBSP_CFG_FILE);
-
- log.info("using path (%s), will %smonitor for changes", m_source.c_str(), reloadChanges ? "" : "not ");
-
- if (reloadChanges) {
+ boost::optional<string> path = pt.get_optional<string>(PATH_PROP_NAME);
+ if (path) {
+ m_source = path.get();
+ AgentConfig::getConfig().getPathResolver().resolve(m_source, PathResolver::SHIBSP_CFG_FILE);
+
+ string_to_bool_translator tr;
+ bool reloadChanges = pt.get(RELOAD_CHANGES_PROP_NAME, false, tr);
+ log.info("using path (%s), will %smonitor for changes", m_source.c_str(), reloadChanges ? "" : "not ");
+ if (reloadChanges) {
#ifdef HAVE_CXX17
- m_lock.reset(new shared_mutex());
+ m_lock.reset(new shared_mutex());
#elif HAVE_CXX14
- m_lock.reset(new shared_timed_mutex());
+ m_lock.reset(new shared_timed_mutex());
#endif
+ }
}
}
@@ -63,11 +89,6 @@ ReloadableFile::~ReloadableFile()
{
}
-const std::string& ReloadableFile::getSource() const
-{
- return m_source;
-}
-
time_t ReloadableFile::getLastModified() const
{
return m_filestamp;
@@ -75,6 +96,10 @@ time_t ReloadableFile::getLastModified() const
bool ReloadableFile::isUpdated() const
{
+ if (m_source.empty()) {
+ return false;
+ }
+
#ifdef WIN32
struct _stat stat_buf;
if (_stat(m_source.c_str(), &stat_buf) != 0) {
@@ -107,10 +132,29 @@ void ReloadableFile::updateModificationTime(time_t t)
m_filestamp = t;
}
-bool ReloadableFile::load()
+pair<bool,ptree*> ReloadableFile::load()
{
- updateModificationTime();
- return true;
+ if (m_source.empty()) {
+ m_log.debug("loading inline configuration...");
+ // Data comes from the tree we were handed.
+ // Because property trees work differently from an XML DOM,
+ // we return the actual root, and not the first child as before
+ // so the caller can interrogate the name of the child tree to
+ // ensure it's as expected.
+ // The const_cast is safe because the flag is false,
+ // preventing the caller from retaining ownership.
+ return make_pair(false, const_cast<ptree*>(&m_root));
+ }
+
+ try {
+ unique_ptr<ptree> newtree = unique_ptr<ptree>(new ptree());
+ xml_parser::read_xml(m_source, *newtree, xml_parser::no_comments|xml_parser::trim_whitespace);
+ return make_pair(true, newtree.release());
+ } catch (const bad_alloc& e) {
+ m_log.crit("out of memory parsing XML configuration (%s)", m_source.c_str());
+ } catch (const xml_parser_error& e) {
+ m_log.error("failed to process XML configuration (%s): %s", m_source.c_str(), e.what());
+ }
}
void ReloadableFile::lock()
@@ -149,10 +193,15 @@ void ReloadableFile::lock_shared()
m_lock->unlock();
m_log.info("change detected, attempting reload...");
- if (load()) {
- m_log.info("swapped in new configuration");
- } else {
- m_log.info("new configuration was invalid");
+ // TODO: maybe a second mutex could guard the reload operation so > 1 thread doesn't try it?
+
+ // The result is handled entirely by the subclass so is ignored here.
+ // The original root tree is purely a means of communicating the object
+ // from the c'tor over to the load method for the inline case, at first load.
+ try {
+ load();
+ } catch (...) {
+ // Shouldn't happen but ensures we generally will acquire the lock before returning.
}
m_lock->lock_shared();
diff --git a/shibsp/util/ReloadableFile.h b/shibsp/util/ReloadableFile.h
index 1551dd94..394bbc34 100644
--- a/shibsp/util/ReloadableFile.h
+++ b/shibsp/util/ReloadableFile.h
@@ -21,12 +21,14 @@
#ifndef __shibsp_reloadablefile_h__
#define __shibsp_reloadablefile_h__
-#include <shibsp/base.h>
+#include <shibsp/util/Lockable.h>
#include <ctime>
#include <memory>
#include <string>
+#include <boost/property_tree/ptree_fwd.hpp>
+
#ifdef HAVE_CXX14
# include <shared_mutex>
#endif
@@ -37,19 +39,35 @@ namespace shibsp {
/**
* Base class for file-based configuration, provides locking and reload semantics.
+ *
+ * <p>Also supports "inliine" configuration that short-circuits most of this logic
+ * allowing for unified handling of the two cases by implementing classes and the
+ * consumers of a configuration interface.</p>
*/
- class SHIBSP_API ReloadableFile
+ class SHIBSP_API ReloadableFile : public virtual BasicLockable, public virtual SharedLockable
{
- MAKE_NONCOPYABLE(ReloadableFile);
+ MAKE_NONCOPYABLE(ReloadableFile);
+
+ public:
+ static const char PATH_PROP_NAME[];
+ static const char RELOAD_CHANGES_PROP_NAME[];
+
protected:
/**
* Base class constructor.
*
- * @param path path to file to use
+ * <p>The supported property keys for an "out of band" file-backed instance
+ * of whatever the underlying configuration is are "path" and "reloadChanges"
+ * (the latter a boolean flag)</p>
+ *
+ * <p>In the absence of a "path" key, the configuration is assumed to be
+ * inline as the content of the supplied tree and the base class essentially
+ * performs no activity, stubs out locking, etc.</p>
+ *
+ * @param pt root of property tree defining resource
* @param log logging object to use
- * @param reloadChanges whether to monitor for changes
*/
- ReloadableFile(const std::string& path, Category& log, bool reloadChanges=false);
+ ReloadableFile(const boost::property_tree::ptree& pt, Category& log);
virtual ~ReloadableFile();
@@ -57,31 +75,27 @@ namespace shibsp {
* Loads (or reloads) configuration material.
*
* <p>This method is called to load configuration material
- * initially and any time a change is detected. The base class version
- * assumes success and calls the updateModificationTime method.</p>
+ * initially and any time a change is detected but is not called
+ * initially unless by a subclass.</p>
*
* <p>This method is not called with the object locked, so actual
- * modification of implementation state requires explicit locking within
- * the method override, and the method should return with the object
- * unlocked.</p>
+ * modification of configuration state requires explicit locking
+ * within the method.</p>
*
* <p>This method should NOT throw exceptions.</p>
- */
- virtual bool load();
-
- /**
- * Gets the source path for the configuration.
*
- * @return source path
+ * @return a pair containing a pointer to the property tree loaded
+ * and a flag indicating whether the subclass should retain ownership
+ * of the tree and free it when done with it
*/
- const std::string& getSource() const;
+ virtual std::pair<bool,boost::property_tree::ptree*> load();
/**
- * Returns the last successful load of this configuration resource.
+ * Gets the last time the configuration was updated.
*
* <p>This method must be called with the object locked, shared or exclusively.</p>
*
- * @return last successful load time
+ * @return the last configuration update
*/
time_t getLastModified() const;
@@ -120,6 +134,9 @@ namespace shibsp {
void updateModificationTime(time_t t);
private:
+ /** Root of configuration or of the pointer to the configuration. */
+ const boost::property_tree::ptree& m_root;
+
/** Logging object. */
Category& m_log;
@@ -137,14 +154,14 @@ namespace shibsp {
#endif
public:
- // SharedLockable
- void lock_shared();
- bool try_lock_shared();
- void unlock_shared();
// BasicLockable
void lock();
bool try_lock();
void unlock();
+ // SharedLockable
+ void lock_shared();
+ bool try_lock_shared();
+ void unlock_shared();
};
};
diff --git a/tests/data/util/reloadablefile/console-shibboleth.ini b/tests/data/util/reloadablefile/console-shibboleth.ini
index dbc985df..dd4ff8f9 100644
--- a/tests/data/util/reloadablefile/console-shibboleth.ini
+++ b/tests/data/util/reloadablefile/console-shibboleth.ini
@@ -4,3 +4,4 @@ default-level = INFO
[logging-categories]
Shibboleth.AgentConfig = DEBUG
+DummyXMLFile = DEBUG
\ No newline at end of file
diff --git a/tests/data/util/reloadablefile/external.xml b/tests/data/util/reloadablefile/external.xml
new file mode 100644
index 00000000..c2e362a9
--- /dev/null
+++ b/tests/data/util/reloadablefile/external.xml
@@ -0,0 +1 @@
+<RequestMapper type="XML" path="./data/util/reloadablefile/external.xml" reloadChanges="true" />
diff --git a/tests/data/util/reloadablefile/inline.xml b/tests/data/util/reloadablefile/inline.xml
new file mode 100644
index 00000000..977fc86f
--- /dev/null
+++ b/tests/data/util/reloadablefile/inline.xml
@@ -0,0 +1,8 @@
+<RequestMapper type="XML">
+ <RequestMap>
+ <Host name="sp.example.org">
+ <Path name="secure" requireSession="true" />
+ </Host>
+ <Host name="admin.example.org" applicationId="admin" requireSession="true" />
+ </RequestMap>
+</RequestMapper>
diff --git a/tests/data/util/reloadablefile/requestmap1.xml b/tests/data/util/reloadablefile/requestmap.xml
similarity index 100%
rename from tests/data/util/reloadablefile/requestmap1.xml
rename to tests/data/util/reloadablefile/requestmap.xml
diff --git a/tests/util/ReloadableFileTests.cpp b/tests/util/ReloadableFileTests.cpp
index 6a1402f7..e4660964 100644
--- a/tests/util/ReloadableFileTests.cpp
+++ b/tests/util/ReloadableFileTests.cpp
@@ -31,26 +31,27 @@ using namespace std;
#define DATA_PATH "./data/util/reloadablefile/"
-struct RF_Fixture {
- RF_Fixture() : data_path(DATA_PATH) {
+struct Inline_Fixture {
+ Inline_Fixture() : data_path(DATA_PATH) {
AgentConfig::getConfig().init(nullptr, (data_path + "console-shibboleth.ini").c_str(), true);
+ xml_parser::read_xml(data_path + "inline.xml", tree, xml_parser::no_comments|xml_parser::trim_whitespace);
}
- ~RF_Fixture() {
+ ~Inline_Fixture() {
AgentConfig::getConfig().term();
}
string data_path;
+ ptree tree;
};
class DummyXMLFile : virtual public ReloadableFile
{
public:
- DummyXMLFile(const string& source, bool reloadable)
- : ReloadableFile(source, Category::getInstance("DummyXMLFile"), reloadable),
+ DummyXMLFile(const ptree& pt)
+ : ReloadableFile(pt, Category::getInstance("DummyXMLFile")),
m_log(Category::getInstance("DummyXMLFile")), m_tree(nullptr), m_forceReload(false) {
- if (!load()) {
- m_log.error("initial configuration was invalid");
- }
+
+ load();
}
~DummyXMLFile() {}
@@ -67,7 +68,7 @@ public:
}
protected:
- bool load();
+ pair<bool,ptree*> load();
private:
Category& m_log;
@@ -75,58 +76,48 @@ private:
bool m_forceReload;
};
-bool DummyXMLFile::load()
+pair<bool,ptree*> DummyXMLFile::load()
{
-#ifdef HAVE_CXX14
- unique_lock<ReloadableFile> locker(*this);
-#endif
- try {
- unique_ptr<ptree> newtree = unique_ptr<ptree>(new ptree());
- xml_parser::read_xml(getSource(), *newtree, xml_parser::no_comments|xml_parser::trim_whitespace);
- m_tree.swap(newtree);
- m_forceReload = false;
- updateModificationTime(time(nullptr));
- return true;
- } catch (const bad_alloc& e) {
- m_log.crit("out of memory parsing XML configuration (%s)", getSource().c_str());
- } catch (const xml_parser_error& e) {
- m_log.error("failed to process XML configuration (%s): %s", getSource().c_str(), e.what());
+ pair<bool,ptree*> ret = ReloadableFile::load();
+ if (ret.second) {
+ if (ret.first) {
+ m_log.debug("external config is valid");
+ } else {
+ m_log.debug("inline config is valid");
+ return ret;
+ }
+ } else {
+ m_log.error("initial configuration was invalid");
+ return ret;
}
- return false;
-}
-BOOST_FIXTURE_TEST_CASE(ReloadableFileTest_no_reload, RF_Fixture)
-{
- DummyXMLFile dummy(data_path + "requestmap1.xml", false);
-
- dummy.lock_shared();
- time_t ts1 = dummy.getLastModified();
- BOOST_CHECK_GT(ts1, 0);
- dummy.unlock();
+ // Swap in external config and update timestamp.
- dummy.forceReload();
- sleep(2);
+#ifdef HAVE_CXX14
+ unique_lock<ReloadableFile> locker(*this);
+#endif
+ unique_ptr<ptree> newtree(ret.second);
+ m_tree.swap(newtree);
+ updateModificationTime(time(nullptr));
- dummy.lock_shared();
- time_t ts2 = dummy.getLastModified();
- BOOST_CHECK_EQUAL(ts2, ts1);
- dummy.unlock();
+ return ret;
}
-BOOST_FIXTURE_TEST_CASE(ReloadableFileTest_no_load, RF_Fixture)
+BOOST_FIXTURE_TEST_CASE(ReloadableFileTest_no_reload, Inline_Fixture)
{
- DummyXMLFile dummy(data_path + "requestmap1.xml", true);
+ DummyXMLFile dummy(tree);
dummy.lock_shared();
time_t ts1 = dummy.getLastModified();
- BOOST_CHECK_GT(ts1, 0);
+ BOOST_CHECK_EQUAL(ts1, 0);
dummy.unlock();
+ // No-op since there's no locking internally.
dummy.forceReload();
sleep(2);
dummy.lock_shared();
time_t ts2 = dummy.getLastModified();
- BOOST_CHECK_GT(ts2, ts1);
+ BOOST_CHECK_EQUAL(ts2, 0);
dummy.unlock();
}
--
To stop receiving notification emails like this one, please contact
the administrator of this repository.
More information about the commits
mailing list