diff --git a/lib/base/function.hpp b/lib/base/function.hpp index 780dae2c5..d52a230a1 100644 --- a/lib/base/function.hpp +++ b/lib/base/function.hpp @@ -60,28 +60,28 @@ private: INITIALIZE_ONCE_WITH_PRIORITY([]() { \ Function::Ptr sf = new icinga::Function(#ns "#" #name, callback, String(args).Split(":"), false); \ Namespace::Ptr nsp = ScriptGlobal::Get(#ns); \ - nsp->Set(#name, sf, true, true); \ + nsp->Set(#name, sf, true); \ }, InitializePriority::RegisterFunctions) #define REGISTER_SAFE_FUNCTION(ns, name, callback, args) \ INITIALIZE_ONCE_WITH_PRIORITY([]() { \ Function::Ptr sf = new icinga::Function(#ns "#" #name, callback, String(args).Split(":"), true); \ Namespace::Ptr nsp = ScriptGlobal::Get(#ns); \ - nsp->Set(#name, sf, true, true); \ + nsp->Set(#name, sf, true); \ }, InitializePriority::RegisterFunctions) #define REGISTER_FUNCTION_NONCONST(ns, name, callback, args) \ INITIALIZE_ONCE_WITH_PRIORITY([]() { \ Function::Ptr sf = new icinga::Function(#ns "#" #name, callback, String(args).Split(":"), false); \ Namespace::Ptr nsp = ScriptGlobal::Get(#ns); \ - nsp->Set(#name, sf, false, true); \ + nsp->Set(#name, sf, false); \ }, InitializePriority::RegisterFunctions) #define REGISTER_SAFE_FUNCTION_NONCONST(ns, name, callback, args) \ INITIALIZE_ONCE_WITH_PRIORITY([]() { \ Function::Ptr sf = new icinga::Function(#ns "#" #name, callback, String(args).Split(":"), true); \ Namespace::Ptr nsp = ScriptGlobal::Get(#ns); \ - nsp->SetAttribute(#name, sf, false, true); \ + nsp->SetAttribute(#name, sf, false); \ }, InitializePriority::RegisterFunctions) } diff --git a/lib/base/json-script.cpp b/lib/base/json-script.cpp index 5738a33fa..90595c88f 100644 --- a/lib/base/json-script.cpp +++ b/lib/base/json-script.cpp @@ -24,5 +24,5 @@ INITIALIZE_ONCE([]() { jsonNS->Freeze(); Namespace::Ptr systemNS = ScriptGlobal::Get("System"); - systemNS->Set("Json", jsonNS, true, true); + systemNS->Set("Json", jsonNS, true); }); diff --git a/lib/base/math-script.cpp b/lib/base/math-script.cpp index 7d96611a4..6cd7b0eb4 100644 --- a/lib/base/math-script.cpp +++ b/lib/base/math-script.cpp @@ -180,5 +180,5 @@ INITIALIZE_ONCE([]() { mathNS->Freeze(); Namespace::Ptr systemNS = ScriptGlobal::Get("System"); - systemNS->Set("Math", mathNS, true, true); + systemNS->Set("Math", mathNS, true); }); diff --git a/lib/base/namespace.cpp b/lib/base/namespace.cpp index 729841cac..cb7c872bd 100644 --- a/lib/base/namespace.cpp +++ b/lib/base/namespace.cpp @@ -33,7 +33,7 @@ Value Namespace::Get(const String& field) const bool Namespace::Get(const String& field, Value *value) const { - std::shared_lock lock(m_DataMutex); + auto lock(ReadLockUnlessFrozen()); auto nsVal = m_Data.find(field); @@ -45,21 +45,22 @@ bool Namespace::Get(const String& field, Value *value) const return true; } -void Namespace::Set(const String& field, const Value& value, bool isConst, bool overrideFrozen, const DebugInfo& debugInfo) +void Namespace::Set(const String& field, const Value& value, bool isConst, const DebugInfo& debugInfo) { ObjectLock olock(this); + + if (m_Frozen) { + BOOST_THROW_EXCEPTION(ScriptError("Namespace is read-only and must not be modified.", debugInfo)); + } + std::unique_lock dlock (m_DataMutex); auto nsVal = m_Data.find(field); if (nsVal == m_Data.end()) { - if (m_Frozen && !overrideFrozen) { - BOOST_THROW_EXCEPTION(ScriptError("Namespace is read-only and must not be modified.", debugInfo)); - } - m_Data[field] = NamespaceValue{value, isConst}; } else { - if (nsVal->second.Const && !overrideFrozen) { + if (nsVal->second.Const) { BOOST_THROW_EXCEPTION(ScriptError("Constant must not be modified.", debugInfo)); } @@ -74,46 +75,43 @@ void Namespace::Set(const String& field, const Value& value, bool isConst, bool */ size_t Namespace::GetLength() const { - std::shared_lock lock(m_DataMutex); + auto lock(ReadLockUnlessFrozen()); return m_Data.size(); } bool Namespace::Contains(const String& field) const { - std::shared_lock lock(m_DataMutex); + auto lock (ReadLockUnlessFrozen()); return m_Data.find(field) != m_Data.end(); } -void Namespace::Remove(const String& field, bool overrideFrozen) +void Namespace::Remove(const String& field) { ObjectLock olock(this); - if (m_Frozen && !overrideFrozen) { + if (m_Frozen) { BOOST_THROW_EXCEPTION(ScriptError("Namespace is read-only and must not be modified.")); } std::unique_lock dlock (m_DataMutex); - if (!overrideFrozen) { - auto attr = m_Data.find(field); - - if (attr != m_Data.end() && attr->second.Const) { - BOOST_THROW_EXCEPTION(ScriptError("Constants must not be removed.")); - } - } - auto it = m_Data.find(field); - if (it == m_Data.end()) + if (it == m_Data.end()) { return; + } + + if (it->second.Const) { + BOOST_THROW_EXCEPTION(ScriptError("Constants must not be removed.")); + } m_Data.erase(it); } /** - * Freeze the namespace, preventing further updates unless overrideFrozen is set. + * Freeze the namespace, preventing further updates. * * This only prevents inserting, replacing or deleting values from the namespace. This operation has no effect on * objects referenced by the values, these remain mutable if they were before. @@ -124,9 +122,18 @@ void Namespace::Freeze() { m_Frozen = true; } +std::shared_lock Namespace::ReadLockUnlessFrozen() const +{ + if (m_Frozen.load(std::memory_order_relaxed)) { + return std::shared_lock(); + } else { + return std::shared_lock(m_DataMutex); + } +} + Value Namespace::GetFieldByName(const String& field, bool, const DebugInfo& debugInfo) const { - std::shared_lock lock(m_DataMutex); + auto lock (ReadLockUnlessFrozen()); auto nsVal = m_Data.find(field); @@ -138,7 +145,12 @@ Value Namespace::GetFieldByName(const String& field, bool, const DebugInfo& debu void Namespace::SetFieldByName(const String& field, const Value& value, bool overrideFrozen, const DebugInfo& debugInfo) { - Set(field, value, false, overrideFrozen, debugInfo); + // The override frozen parameter is mandated by the interface but ignored here. If the namespace is frozen, this + // disables locking for read operations, so it must not be modified again to ensure the consistency of the internal + // data structures. + (void) overrideFrozen; + + Set(field, value, false, debugInfo); } bool Namespace::HasOwnField(const String& field) const diff --git a/lib/base/namespace.hpp b/lib/base/namespace.hpp index f643d9661..94f2055d3 100644 --- a/lib/base/namespace.hpp +++ b/lib/base/namespace.hpp @@ -8,6 +8,7 @@ #include "base/shared-object.hpp" #include "base/value.hpp" #include "base/debuginfo.hpp" +#include #include #include #include @@ -68,9 +69,9 @@ public: Value Get(const String& field) const; bool Get(const String& field, Value *value) const; - void Set(const String& field, const Value& value, bool isConst = false, bool overrideFrozen = false, const DebugInfo& debugInfo = DebugInfo()); + void Set(const String& field, const Value& value, bool isConst = false, const DebugInfo& debugInfo = DebugInfo()); bool Contains(const String& field) const; - void Remove(const String& field, bool overrideFrozen = false); + void Remove(const String& field); void Freeze(); Iterator Begin(); @@ -86,10 +87,12 @@ public: static Object::Ptr GetPrototype(); private: + std::shared_lock ReadLockUnlessFrozen() const; + std::map m_Data; mutable std::shared_timed_mutex m_DataMutex; bool m_ConstValues; - bool m_Frozen; + std::atomic m_Frozen; }; Namespace::Iterator begin(const Namespace::Ptr& x); diff --git a/lib/base/scriptframe.cpp b/lib/base/scriptframe.cpp index 4a67520ff..b9725cf60 100644 --- a/lib/base/scriptframe.cpp +++ b/lib/base/scriptframe.cpp @@ -29,18 +29,18 @@ INITIALIZE_ONCE_WITH_PRIORITY([]() { l_SystemNS->Set("BuildHostName", ICINGA_BUILD_HOST_NAME); l_SystemNS->Set("BuildCompilerName", ICINGA_BUILD_COMPILER_NAME); l_SystemNS->Set("BuildCompilerVersion", ICINGA_BUILD_COMPILER_VERSION); - globalNS->Set("System", l_SystemNS, false, true); + globalNS->Set("System", l_SystemNS, true); - l_SystemNS->Set("Configuration", new Configuration(), false, true); + l_SystemNS->Set("Configuration", new Configuration()); l_TypesNS = new Namespace(true); - globalNS->Set("Types", l_TypesNS, false, true); + globalNS->Set("Types", l_TypesNS, true); l_StatsNS = new Namespace(true); - globalNS->Set("StatsFunctions", l_StatsNS, false, true); + globalNS->Set("StatsFunctions", l_StatsNS, true); l_InternalNS = new Namespace(true); - globalNS->Set("Internal", l_InternalNS, false, true); + globalNS->Set("Internal", l_InternalNS, true); }, InitializePriority::CreateNamespaces); INITIALIZE_ONCE_WITH_PRIORITY([]() { diff --git a/lib/icinga/icingaapplication.cpp b/lib/icinga/icingaapplication.cpp index 1e554f3ff..c20ee4cfb 100644 --- a/lib/icinga/icingaapplication.cpp +++ b/lib/icinga/icingaapplication.cpp @@ -54,8 +54,8 @@ void IcingaApplication::StaticInitialize() /* Ensure that the System namespace is already initialized. Otherwise this is a programming error. */ VERIFY(systemNS); - systemNS->Set("ApplicationType", "IcingaApplication", false, true); - systemNS->Set("ApplicationVersion", Application::GetAppVersion(), false, true); + systemNS->Set("ApplicationType", "IcingaApplication", true); + systemNS->Set("ApplicationVersion", Application::GetAppVersion(), true); Namespace::Ptr globalNS = ScriptGlobal::GetGlobals(); VERIFY(globalNS);