]> git.ipfire.org Git - thirdparty/squid.git/commitdiff
API to separate Config.x users/parsers from Config.y details (#811)
authorAlex Rousskov <rousskov@measurement-factory.com>
Sat, 8 May 2021 07:51:32 +0000 (07:51 +0000)
committerSquid Anubis <squid-anubis@squid-cache.org>
Sun, 9 May 2021 08:05:03 +0000 (08:05 +0000)
The new API avoids several kinds of unwanted code dependencies:

* Config.x users do not need to know the type of any Config.y component.
  Forward declarations of Config.y types are sufficient.

* Config.x parsers do not need to know about Config.y parsers. Config.x
  parsers can also be hidden from Config.x users.

* The centralized parsing code does not need to contain Config.x parsing
  code (for any x) and does not maintain "generic" parser registration.
  The compiler/linker do that for us _without_ losing C++ type safety.

Correct API implementation may also help separate the active Config
object from the being-parsed configuration, to eventually support safe
"hot" reconfiguration.

This change does not convert any existing Config fields and does not
imply a policy that existing fields must be converted when touched.

The API was tested on a typical new Config.x component (to be merged
separately). It is likely to evolve, especially when support for
multi-directive components is added.

Activated by using TYPE names with two colons (e.g., TYPE: Security::X).

src/ConfigOption.h
src/ConfigParser.cc
src/ConfigParser.h
src/cache_cf.cc
src/cf_gen.cc

index df58ae231871afbf6f96d635cd65f3210bc503f5..bfcee22ab5a2a82c5f2eb1b0edf4d442f41eaf6e 100644 (file)
@@ -9,11 +9,50 @@
 #ifndef SQUID_CONFIGOPTION_H
 #define SQUID_CONFIGOPTION_H
 
+#include <iosfwd>
 #include <vector>
 
 class StoreEntry;
+class ConfigParser;
+
+namespace Configuration {
+
+/// Interface for basic/low-level manipulation of a squid.conf directive value.
+/// Hides T's declarations from squid.conf parsing/reconfiguring/reporting code.
+///
+/// Implementations/specializations must not modify the current configuration
+/// (i.e. the Config objects and similar/related global state). To facilitate
+/// reuse, implementations/specializations should also be independent from any
+/// specific configuration directive name and its squid.conf location.
+///
+/// TODO: Support multi-directive components of various kinds.
+template <class T>
+class Component
+{
+public:
+    /* the code adding "TYPE: T" to cf.data.pre must specialize these */
+
+    /// creates a new T instance using the given parser; never returns nil
+    static T Parse(ConfigParser &);
 
-/* cache option parsers */
+    /// reports the current T instance configuration in squid.conf format
+    static void Print(std::ostream &, const T &);
+
+    /// destroys Parse() result
+    static void Free(T);
+};
+
+} // namespace Configuration
+
+/*
+ * Deprecated squid.conf option wrappers used by cache_dir handling code. These
+ * classes are similar to Configuration::Component<T>, but they merge T with T
+ * parsing API, making them ill-suited for handling SquidConfig data members
+ * with built-in C++ types and, more importantly, forcing SquidConfig users to
+ * know about parsing/dumping/freeing capabilities of each SquidConfig
+ * component. They also do not hide T details from the generic squid.conf
+ * parsing code -- one has to provide a type-specific parse_T() for each T.
+ */
 
 class ConfigOption
 {
index d451fb2a644c515d4315dd86b1ebb844a916a65e..d9f69996a967f56e62e71495e261c90ca890f19e 100644 (file)
@@ -536,6 +536,22 @@ ConfigParser::QuoteString(const String &var)
     return quotedStr.termedBuf();
 }
 
+void
+ConfigParser::rejectDuplicateDirective()
+{
+    assert(cfg_directive);
+    throw TextException("duplicate configuration directive", Here());
+}
+
+void
+ConfigParser::closeDirective()
+{
+    assert(cfg_directive);
+    if (const auto garbage = PeekAtToken())
+        throw TextException(ToSBuf("trailing garbage at the end of a configuration directive: ", garbage), Here());
+    // TODO: cfg_directive = nullptr; // currently in generated code
+}
+
 bool
 ConfigParser::CfgFile::startParse(char *path)
 {
index 424d2bb939dd9f54eba605acb4bc017149c87002..cafcfa9e1e771e284848810510e26c81c31f9693 100644 (file)
@@ -49,6 +49,13 @@ public:
     enum TokenType {SimpleToken, QuotedToken, FunctionParameters};
 
     void destruct();
+
+    /// stops parsing the current configuration directive
+    void closeDirective();
+
+    /// rejects configuration due to a repeated directive
+    void rejectDuplicateDirective();
+
     static void ParseUShort(unsigned short *var);
     static void ParseBool(bool *var);
     static const char *QuoteString(const String &var);
index 0c53e0f89994fac1bab4d3bd6f37bf48576221cc..847cc74098ef10058db9bb6767087b699be8da2e 100644 (file)
@@ -24,6 +24,7 @@
 #include "base/RunnersRegistry.h"
 #include "cache_cf.h"
 #include "CachePeer.h"
+#include "ConfigOption.h"
 #include "ConfigParser.h"
 #include "CpuAffinityMap.h"
 #include "DiskIO/DiskIOModule.h"
@@ -633,6 +634,75 @@ parseConfigFile(const char *file_name)
     }
 }
 
+/*
+ * The templated functions below are essentially ConfigParser methods. They are
+ * not implemented as such because our generated code calling them is the only
+ * code that can instantiate implementations for each T -- we cannot place these
+ * definitions into ConfigParser.cc unless cf_parser.cci is moved there.
+ */
+
+// TODO: When adding Ts incompatible with this trivial API and implementation,
+// replace both with a ConfigParser-maintained table of seen directives.
+/// whether we have seen (and, hence, configured) the given directive
+template <typename T>
+static bool
+SawDirective(const T &raw)
+{
+    return bool(raw);
+}
+
+/// Sets the given raw SquidConfig data member.
+/// Extracts and interprets parser's configuration tokens.
+template <typename T>
+static void
+ParseDirective(T &raw, ConfigParser &parser)
+{
+    if (SawDirective(raw))
+        parser.rejectDuplicateDirective();
+
+    // TODO: parser.openDirective(directiveName);
+    Must(!raw);
+    raw = Configuration::Component<T>::Parse(parser);
+    Must(raw);
+    parser.closeDirective();
+}
+
+/// reports raw SquidConfig data member configuration using squid.conf syntax
+/// \param name the name of the configuration directive being dumped
+template <typename T>
+static void
+DumpDirective(const T &raw, StoreEntry *entry, const char *name)
+{
+    if (!SawDirective(raw))
+        return; // not configured
+
+    entry->append(name, strlen(name));
+    SBufStream os;
+    Configuration::Component<T>::Print(os, raw);
+    const auto buf = os.buf();
+    if (buf.length()) {
+        entry->append(" ", 1);
+        entry->append(buf.rawContent(), buf.length());
+    }
+    entry->append("\n", 1);
+}
+
+/// frees any resources associated with the given raw SquidConfig data member
+template <typename T>
+static void
+FreeDirective(T &raw)
+{
+    Configuration::Component<T>::Free(raw);
+
+    // While the implementation may change, there is no way to avoid zeroing.
+    // Even migration to a proper SquidConfig class would not help: While
+    // ordinary destructors do not need to zero data members, a SquidConfig
+    // destructor would have to zero to protect any SquidConfig::x destruction
+    // code from accidentally dereferencing an already destroyed Config.y.
+    static_assert(std::is_trivial<T>::value, "SquidConfig member is trivial");
+    memset(&raw, 0, sizeof(raw));
+}
+
 static void
 configDoConfigure(void)
 {
index 1ed2085242b8df9612ed560af345065101b18558..9e1cab3f93b60202fe46fc9820db7a7cf49fd10b 100644 (file)
@@ -613,6 +613,8 @@ Entry::genParseAlias(const std::string &aName, std::ostream &fout) const
         fout << "        parse_obsolete(token);";
     } else if (!loc.size() || loc.compare("none") == 0) {
         fout << "parse_" << type << "();";
+    } else if (type.find("::") != std::string::npos) {
+        fout << "ParseDirective<" << type << ">(" << loc << ", LegacyParser);";
     } else {
         fout << "parse_" << type << "(&" << loc << (array_flag ? "[0]" : "") << ");";
     }
@@ -683,7 +685,10 @@ gen_dump(const EntryList &head, std::ostream &fout)
         if (e.ifdef.size())
             fout << "#if " << e.ifdef << std::endl;
 
-        fout << "    dump_" << e.type << "(entry, \"" << e.name << "\", " << e.loc << ");" << std::endl;
+        if (e.type.find("::") != std::string::npos)
+            fout << "    DumpDirective<" << e.type << ">(" << e.loc << ", entry, \"" << e.name << "\");\n";
+        else
+            fout << "    dump_" << e.type << "(entry, \"" << e.name << "\", " << e.loc << ");" << std::endl;
 
         if (e.ifdef.size())
             fout << "#endif" << std::endl;
@@ -711,7 +716,10 @@ gen_free(const EntryList &head, std::ostream &fout)
         if (e.ifdef.size())
             fout << "#if " << e.ifdef << std::endl;
 
-        fout << "    free_" << e.type << "(&" << e.loc << (e.array_flag ? "[0]" : "") << ");" << std::endl;
+        if (e.type.find("::") != std::string::npos)
+            fout << "    FreeDirective<" << e.type << ">(" << e.loc << ");\n";
+        else
+            fout << "    free_" << e.type << "(&" << e.loc << (e.array_flag ? "[0]" : "") << ");" << std::endl;
 
         if (e.ifdef.size())
             fout << "#endif" << std::endl;