From: Alex Rousskov Date: Sat, 8 May 2021 07:51:32 +0000 (+0000) Subject: API to separate Config.x users/parsers from Config.y details (#811) X-Git-Tag: 4.15-20210522-snapshot~5 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=2e6535ab844258c37966957924d74144eb62a5bf;p=thirdparty%2Fsquid.git API to separate Config.x users/parsers from Config.y details (#811) 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). --- diff --git a/src/ConfigOption.h b/src/ConfigOption.h index df58ae2318..bfcee22ab5 100644 --- a/src/ConfigOption.h +++ b/src/ConfigOption.h @@ -9,11 +9,50 @@ #ifndef SQUID_CONFIGOPTION_H #define SQUID_CONFIGOPTION_H +#include #include 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 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, 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 { diff --git a/src/ConfigParser.cc b/src/ConfigParser.cc index d451fb2a64..d9f69996a9 100644 --- a/src/ConfigParser.cc +++ b/src/ConfigParser.cc @@ -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) { diff --git a/src/ConfigParser.h b/src/ConfigParser.h index 424d2bb939..cafcfa9e1e 100644 --- a/src/ConfigParser.h +++ b/src/ConfigParser.h @@ -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); diff --git a/src/cache_cf.cc b/src/cache_cf.cc index 0c53e0f899..847cc74098 100644 --- a/src/cache_cf.cc +++ b/src/cache_cf.cc @@ -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 +static bool +SawDirective(const T &raw) +{ + return bool(raw); +} + +/// Sets the given raw SquidConfig data member. +/// Extracts and interprets parser's configuration tokens. +template +static void +ParseDirective(T &raw, ConfigParser &parser) +{ + if (SawDirective(raw)) + parser.rejectDuplicateDirective(); + + // TODO: parser.openDirective(directiveName); + Must(!raw); + raw = Configuration::Component::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 +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::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 +static void +FreeDirective(T &raw) +{ + Configuration::Component::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::value, "SquidConfig member is trivial"); + memset(&raw, 0, sizeof(raw)); +} + static void configDoConfigure(void) { diff --git a/src/cf_gen.cc b/src/cf_gen.cc index 1ed2085242..9e1cab3f93 100644 --- a/src/cf_gen.cc +++ b/src/cf_gen.cc @@ -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;