From: Alex Rousskov Date: Sun, 28 Jan 2024 09:51:37 +0000 (+0000) Subject: Remove AclMatchedName from ACL::ParseAclLine() (#1642) X-Git-Tag: SQUID_7_0_1~231 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=b58181364a8fe1bdc0be705476bb9dfab2192979;p=thirdparty%2Fsquid.git Remove AclMatchedName from ACL::ParseAclLine() (#1642) ACL parsing code needs to know the aclname parameter of the being-parsed acl directive to report various errors. Most admins recognize their ACLs by these names so reporting aclnames improves UX. Since before 1999 commit b6a2f15, Squid used a "temporary" and "ugly" trick that supplied aclname via the unrelated global variable called AclMatchedName (which has its own set of problems). Some ACL parsing code used AclMatchedName in cache.log messages, but most ACL-related problems were still reported without that information. Passing ACL::name to each parsing-related function via an extra parameter is not just ugly but impractical because some the low-level parsing functions should not really know about ACLs. Instead, we reuse existing CodeContext mechanism to report parsing context information (in this case -- aclname). We plan to enhance parsing context to cover directives other than "acl" (without modifying every directive parser, of course), but this first small step significantly reduces configuration code exposure to AclMatchedName, unblocking ACL-related smooth reconfiguration improvements. --- diff --git a/src/acl/Acl.cc b/src/acl/Acl.cc index 8cc5c5b394..d2e9c5faec 100644 --- a/src/acl/Acl.cc +++ b/src/acl/Acl.cc @@ -66,6 +66,22 @@ Make(TypeName typeName) return result; } +/// CodeContext of the being-parsed acl directive +class ParsingContext: public CodeContext +{ +public: + using Pointer = RefCount; + + explicit ParsingContext(const SBuf &name): name_(name) {} + + /* CodeContext API */ + ScopedId codeContextGist() const override; + std::ostream &detailCodeContext(std::ostream &os) const override; + +private: + SBuf name_; ///< the aclname parameter of the being-parsed acl directive +}; + } // namespace Acl void @@ -80,9 +96,7 @@ void Acl::SetKey(SBuf &keyStorage, const char *keyParameterName, const char *newKey) { if (!newKey) { - throw TextException(ToSBuf("An acl declaration is missing a ", keyParameterName, - Debug::Extra, "ACL name: ", AclMatchedName), - Here()); + throw TextException(ToSBuf("An acl declaration is missing a ", keyParameterName), Here()); } if (keyStorage.isEmpty()) { @@ -96,12 +110,27 @@ Acl::SetKey(SBuf &keyStorage, const char *keyParameterName, const char *newKey) throw TextException(ToSBuf("Attempt to change the value of the ", keyParameterName, " argument in a subsequent acl declaration:", Debug::Extra, "previously seen value: ", keyStorage, Debug::Extra, "new/conflicting value: ", newKey, - Debug::Extra, "ACL name: ", AclMatchedName, Debug::Extra, "advice: Use a dedicated ACL name for each distinct ", keyParameterName, " (and group those ACLs together using an 'any-of' ACL)."), Here()); } +/* Acl::ParsingContext */ + +ScopedId +Acl::ParsingContext::codeContextGist() const { + return ScopedId("acl"); +} + +std::ostream & +Acl::ParsingContext::detailCodeContext(std::ostream &os) const +{ + return os << Debug::Extra << "acl name: " << name_ << + Debug::Extra << "configuration context: " << ConfigParser::CurrentLocation(); +} + +/* Acl::Node */ + void * Acl::Node::operator new (size_t) { @@ -192,9 +221,6 @@ Acl::Node::ParseAclLine(ConfigParser &parser, Node ** head) { /* we're already using strtok() to grok the line */ char *t = nullptr; - Node *A = nullptr; - LOCAL_ARRAY(char, aclname, ACL_NAME_SZ); - int new_acl = 0; /* snarf the ACL name */ @@ -211,7 +237,16 @@ Acl::Node::ParseAclLine(ConfigParser &parser, Node ** head) return; } - xstrncpy(aclname, t, ACL_NAME_SZ); + SBuf aclname(t); + CallParser(ParsingContext::Pointer::Make(aclname), [&] { + ParseNamed(parser, head, aclname.c_str()); // TODO: Convert Node::name to SBuf + }); +} + +/// parses acl directive parts that follow aclname +void +Acl::Node::ParseNamed(ConfigParser &parser, Node ** const head, const char * const aclname) +{ /* snarf the ACL type */ const char *theType; @@ -252,6 +287,8 @@ Acl::Node::ParseAclLine(ConfigParser &parser, Node ** head) theType = "client_connection_mark"; } + Node *A = nullptr; + int new_acl = 0; if ((A = FindByName(aclname)) == nullptr) { debugs(28, 3, "aclParseAclLine: Creating ACL '" << aclname << "'"); A = Acl::Make(theType); @@ -268,22 +305,11 @@ Acl::Node::ParseAclLine(ConfigParser &parser, Node ** head) new_acl = 0; } - /* - * Here we set AclMatchedName in case we need to use it in a - * warning message in Acl::SplayInserter::Merge(). - */ - AclMatchedName = A->name; /* ugly */ - A->parseFlags(); /*split the function here */ A->parse(); - /* - * Clear AclMatchedName from our temporary hack - */ - AclMatchedName = nullptr; /* ugly */ - if (!new_acl) return; diff --git a/src/acl/Node.h b/src/acl/Node.h index 224a81e868..747f76a5cf 100644 --- a/src/acl/Node.h +++ b/src/acl/Node.h @@ -90,6 +90,8 @@ private: /// \returns (linked) "line" Options supported by this Acl::Node /// \see Acl::Node::options() virtual const Acl::Options &lineOptions() { return Acl::NoOptions(); } + + static void ParseNamed(ConfigParser &, Node **head, const char *name); }; } // namespace Acl diff --git a/src/acl/ProtocolData.cc b/src/acl/ProtocolData.cc index e1ffbf1a5c..ca340cac49 100644 --- a/src/acl/ProtocolData.cc +++ b/src/acl/ProtocolData.cc @@ -57,7 +57,7 @@ ACLProtocolData::parse() } } if (p == AnyP::PROTO_UNKNOWN) { - debugs(28, DBG_IMPORTANT, "WARNING: Ignoring unknown protocol '" << t << "' in the ACL named '" << AclMatchedName << "'"); + debugs(28, DBG_IMPORTANT, "WARNING: Ignoring unknown protocol '" << t << "' in the ACL"); // XXX: store the text pattern of this protocol name for live comparisons } } diff --git a/src/acl/SplayInserter.h b/src/acl/SplayInserter.h index 2871044ed8..bcfd8a8e95 100644 --- a/src/acl/SplayInserter.h +++ b/src/acl/SplayInserter.h @@ -73,14 +73,14 @@ Acl::SplayInserter::Merge(Splay &storage, Value &&newItem) if (IsSubset(newItem, oldItem)) { debugs(28, DBG_PARSE_NOTE(DBG_IMPORTANT), "WARNING: Ignoring " << newItem << " because it is already covered by " << oldItem << - Debug::Extra << "advice: Remove value " << newItem << " from the ACL named " << AclMatchedName); + Debug::Extra << "advice: Remove value " << newItem << " from the ACL"); DestroyValue(newItem); return; } if (IsSubset(oldItem, newItem)) { debugs(28, DBG_PARSE_NOTE(DBG_IMPORTANT), "WARNING: Ignoring earlier " << oldItem << " because it is covered by " << newItem << - Debug::Extra << "advice: Remove value " << oldItem << " from the ACL named " << AclMatchedName); + Debug::Extra << "advice: Remove value " << oldItem << " from the ACL"); storage.remove(oldItem, comparator); DestroyValue(oldItem); continue; // still need to insert newItem (and it may conflict with other old items) @@ -88,7 +88,7 @@ Acl::SplayInserter::Merge(Splay &storage, Value &&newItem) const auto combinedItem = MakeCombinedValue(oldItem, newItem); debugs(28, DBG_PARSE_NOTE(DBG_IMPORTANT), "WARNING: Merging overlapping " << newItem << " and " << oldItem << " into " << combinedItem << - Debug::Extra << "advice: Replace values " << newItem << " and " << oldItem << " with " << combinedItem << " in the ACL named " << AclMatchedName); + Debug::Extra << "advice: Replace values " << newItem << " and " << oldItem << " with " << combinedItem << " in the ACL"); DestroyValue(newItem); newItem = combinedItem; storage.remove(oldItem, comparator); diff --git a/src/base/CodeContext.h b/src/base/CodeContext.h index ee9094bf40..cfda2a77a4 100644 --- a/src/base/CodeContext.h +++ b/src/base/CodeContext.h @@ -106,20 +106,38 @@ public: CodeContext::Pointer savedCodeContext; }; -/// Executes service `callback` in `callbackContext`. If an exception occurs, -/// the callback context is preserved, so that the exception is associated with -/// the callback that triggered them (rather than with the service). -/// +/// A helper that calls the given function in the given call context. If the +/// function throws, the call context is preserved, so that the exception is +/// associated with the context that triggered it. +template +inline void +CallAndRestore_(const CodeContext::Pointer &context, Fun &&fun) +{ + const auto savedCodeContext(CodeContext::Current()); + CodeContext::Reset(context); + fun(); + CodeContext::Reset(savedCodeContext); +} + /// Service code running in its own service context should use this function. +/// \sa CallAndRestore_() template inline void CallBack(const CodeContext::Pointer &callbackContext, Fun &&callback) { // TODO: Consider catching exceptions and letting CodeContext handle them. - const auto savedCodeContext(CodeContext::Current()); - CodeContext::Reset(callbackContext); - callback(); - CodeContext::Reset(savedCodeContext); + CallAndRestore_(callbackContext, callback); +} + +/// To supply error-reporting code with parsing context X (where the error +/// occurred), parsing code should use this function when initiating parsing +/// inside that context X. +/// \sa CallAndRestore_() +template +inline void +CallParser(const CodeContext::Pointer &parsingContext, Fun &&parse) +{ + CallAndRestore_(parsingContext, parse); } /// Executes `service` in `serviceContext` but due to automatic caller context diff --git a/src/external_acl.cc b/src/external_acl.cc index 7238adbc8d..a64155baf4 100644 --- a/src/external_acl.cc +++ b/src/external_acl.cc @@ -528,7 +528,7 @@ ACLExternal::parse() // def->name is the name of the external_acl_type. // this is the name of the 'acl' directive being tested - data->name = xstrdup(AclMatchedName); + data->name = xstrdup(name); while ((token = ConfigParser::strtokFile())) { wordlistAdd(&data->arguments, token); diff --git a/test-suite/squidconf/bad-acl-dstdomain-dupe.conf b/test-suite/squidconf/bad-acl-dstdomain-dupe.conf index 11ead73a69..c7506f8c71 100644 --- a/test-suite/squidconf/bad-acl-dstdomain-dupe.conf +++ b/test-suite/squidconf/bad-acl-dstdomain-dupe.conf @@ -6,7 +6,7 @@ ## acl test11 dstdomain .example.com example.com -acl test12 dstdomain example.com .example.com +acl test12 dstdomain example.org .example.org acl test21 dstdomain .example.com a.example.com acl test22 dstdomain b.example.com .example.com diff --git a/test-suite/squidconf/bad-acl-dstdomain-dupe.conf.instructions b/test-suite/squidconf/bad-acl-dstdomain-dupe.conf.instructions index 868234ee65..ff18d9c66a 100644 --- a/test-suite/squidconf/bad-acl-dstdomain-dupe.conf.instructions +++ b/test-suite/squidconf/bad-acl-dstdomain-dupe.conf.instructions @@ -1,25 +1,31 @@ expect-messages <