measurement-factory · eduard-bagdasaryan · Nov 21, 2023 · Nov 21, 2023 · Nov 22, 2023 · Nov 24, 2023
diff --git a/src/ConfigParser.cc b/src/ConfigParser.cc
@@ -38,6 +38,8 @@ void
 ConfigParser::destruct()
 {
     shutting_down = 1;
+    // do not duplicate context in destruct() messages
+    CodeContext::Reset();
     if (!CfgFiles.empty()) {
         std::ostringstream message;
         CfgFile *f = CfgFiles.top();

diff --git a/src/acl/Acl.cc b/src/acl/Acl.cc
@@ -102,6 +102,18 @@ Acl::SetKey(SBuf &keyStorage, const char *keyParameterName, const char *newKey)
                         Here());
 }
 
+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();
+}
+
 void *
 ACL::operator new (size_t)
 {
@@ -213,6 +225,10 @@ ACL::ParseAclLine(ConfigParser &parser, ACL ** head)
     }
 
     xstrncpy(aclname, t, ACL_NAME_SZ);
+
+    const auto parseContext = Acl::ParsingContext::Pointer::Make(aclname);
+    CodeContext::Reset(parseContext);
+
     /* snarf the ACL type */
     const char *theType;
 
@@ -269,22 +285,11 @@ ACL::ParseAclLine(ConfigParser &parser, ACL ** head)
         new_acl = 0;
     }
 
-    /*
-     * Here we set AclMatchedName in case we need to use it in a
-     * warning message in aclDomainCompare().
-     */
-    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/Acl.h b/src/acl/Acl.h
@@ -11,6 +11,7 @@
 
 #include "acl/forward.h"
 #include "acl/Options.h"
+#include "base/CodeContext.h"
 #include "cbdata.h"
 #include "defines.h"
 #include "dlink.h"
@@ -36,6 +37,22 @@ void RegisterMaker(TypeName typeName, Maker maker);
 /// Key comparison is case-insensitive.
 void SetKey(SBuf &keyStorage, const char *keyParameterName, const char *newKey);
 
+/// CodeContext of the ACL being parsed
+class ParsingContext : public CodeContext
+{
+public:
+    using Pointer = RefCount<ParsingContext>;
+
+    ParsingContext(const char *name) : name_(name) {}
+
+    /* CodeContext API */
+    ScopedId codeContextGist() const override;
+    std::ostream &detailCodeContext(std::ostream &os) const override;
+
+private:
+    const char *name_; ///< the parsed ACL name
+};
+
 } // namespace Acl
 
 /// A configurable condition. A node in the ACL expression tree.

diff --git a/src/acl/DomainData.cc b/src/acl/DomainData.cc
@@ -12,9 +12,11 @@
 #include "acl/Checklist.h"
 #include "acl/DomainData.h"
 #include "anyp/Uri.h"
+#include "base/TextException.h"
 #include "cache_cf.h"
 #include "ConfigParser.h"
 #include "debug/Stream.h"
+#include "sbuf/Stream.h"
 #include "util.h"
 
 template<class T>
@@ -78,24 +80,23 @@ aclDomainCompare(T const &a, T const &b)
             // When a.example.com comes after .example.com in an ACL
             // sub-domain is ignored. That is okay. Just important
             bool d3big = (strlen(d3) > strlen(d4)); // Always suggest removing the longer one.
-            debugs(28, DBG_IMPORTANT, "WARNING: '" << (d3big?d3:d4) << "' is a subdomain of '" << (d3big?d4:d3) << "'");
-            debugs(28, DBG_IMPORTANT, "WARNING: You should remove '" << (d3big?d3:d4) << "' from the ACL named '" << AclMatchedName << "'");
+            debugs(28, DBG_IMPORTANT, "WARNING: '" << (d3big?d3:d4) << "' is a subdomain of '" << (d3big?d4:d3) << "'. " <<
+                    "You should remove '" << (d3big?d3:d4) << "' from the ACL.");
             debugs(28, 2, "Ignore '" << d3 << "' to keep splay tree searching predictable");
         }
     } else if (ret == 0) {
         // It may be an exact duplicate. No problem. Just drop.
         if (strcmp(d1,d2)==0) {
-            debugs(28, 2, "WARNING: '" << d2 << "' is duplicated in the list.");
-            debugs(28, 2, "WARNING: You should remove one '" << d2 << "' from the ACL named '" << AclMatchedName << "'");
+            debugs(28, DBG_PARSE_NOTE(2), "WARNING: '" << d2 << "' is duplicated in the list. " <<
+                    "You should remove one '" << d2 << "' from the ACL.");
             return ret;
         }
         // When a.example.com comes before .example.com in an ACL
         // discarding the wildcard is critically bad.
         // or Maybe even both are wildcards. Things are very weird in those cases.
         bool d1big = (strlen(d1) > strlen(d2)); // Always suggest removing the longer one.
-        debugs(28, DBG_CRITICAL, "ERROR: '" << (d1big?d1:d2) << "' is a subdomain of '" << (d1big?d2:d1) << "'");
-        debugs(28, DBG_CRITICAL, "ERROR: You need to remove '" << (d1big?d1:d2) << "' from the ACL named '" << AclMatchedName << "'");
-        self_destruct();
+        throw TextException(ToSBuf("'", (d1big?d1:d2), "' is a subdomain of '", (d1big?d2:d1), "'. ",
+                    "You need to remove '", (d1big?d1:d2), "' from the ACL."), Here());
     }
 
     return ret;

diff --git a/src/acl/HttpStatus.cc b/src/acl/HttpStatus.cc
@@ -48,9 +48,9 @@ int acl_httpstatus_data::compare(acl_httpstatus_data* const& a, acl_httpstatus_d
     if (ret == 0) {
         const SBuf sa = a->toStr();
         const SBuf sb = b->toStr();
-        debugs(28, DBG_CRITICAL, "WARNING: '" << sa << "' is a subrange of '" << sb << "'");
-        debugs(28, DBG_CRITICAL, "WARNING: because of this '" << sa << "' is ignored to keep splay tree searching predictable");
-        debugs(28, DBG_CRITICAL, "WARNING: You should probably remove '" << sb << "' from the ACL named '" << AclMatchedName << "'");
+        debugs(28, DBG_CRITICAL, "WARNING: '" << sa << "' is a subrange of '" << sb << "'. " <<
+                "Because of this '" << sa << "' is ignored to keep splay tree searching predictable. " <<
+                "You should probably remove '" << sb << "' from the ACL.");
     }
 
     return ret;

diff --git a/src/acl/Ip.cc b/src/acl/Ip.cc
@@ -139,9 +139,9 @@ acl_ip_data::NetworkCompare(acl_ip_data * const & a, acl_ip_data * const &b)
             a->toStr(buf_n1, 3*(MAX_IPSTRLEN+1));
             b->toStr(buf_n2, 3*(MAX_IPSTRLEN+1));
         }
-        debugs(28, DBG_CRITICAL, "WARNING: (" << (bina?'B':'A') << ") '" << buf_n1 << "' is a subnetwork of (" << (bina?'A':'B') << ") '" << buf_n2 << "'");
-        debugs(28, DBG_CRITICAL, "WARNING: because of this '" << (bina?buf_n2:buf_n1) << "' is ignored to keep splay tree searching predictable");
-        debugs(28, DBG_CRITICAL, "WARNING: You should probably remove '" << buf_n1 << "' from the ACL named '" << AclMatchedName << "'");
+        debugs(28, DBG_CRITICAL, "WARNING: (" << (bina?'B':'A') << ") '" << buf_n1 << "' is a subnetwork of (" << (bina?'A':'B') << ") '" << buf_n2 << "'. " <<
+                "Because of this '" << (bina?buf_n2:buf_n1) << "' is ignored to keep splay tree searching predictable. " <<
+                "You should probably remove '" << buf_n1 << "' from the ACL.");
     }
 
     return ret;

diff --git 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/cache_cf.cc b/src/cache_cf.cc
@@ -559,10 +559,12 @@ parseOneConfigFile(const char *file_name, unsigned int depth)
                 err_count += parseManyConfigFiles(tmp_line + 8, depth + 1);
             } else {
                 try {
-                    if (!parse_line(tmp_line)) {
-                        debugs(3, DBG_CRITICAL, ConfigParser::CurrentLocation() << ": unrecognized: '" << tmp_line << "'");
-                        ++err_count;
-                    }
+                    CallContextCreator([&] {
+                        if (!parse_line(tmp_line)) {
+                            debugs(3, DBG_CRITICAL, ConfigParser::CurrentLocation() << ": unrecognized: '" << tmp_line << "'");
+                            ++err_count;
+                        }
+                    });
                 } catch (...) {
                     // fatal for now
                     debugs(3, DBG_CRITICAL, "ERROR: configuration failure: " << CurrentException);
@@ -605,7 +607,11 @@ parseConfigFileOrThrow(const char *file_name)
     configFreeMemory();
 
     ACLMethodData::ThePurgeCount = 0;
-    default_all();
+
+    // TODO: wrap each parse_line() instead
+    CallContextCreator([] {
+        default_all();
+    });
 
     err_count = parseOneConfigFile(file_name, 0);
 

diff --git 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/dstdomain-domain-dup.conf b/test-suite/squidconf/dstdomain-domain-dup.conf
@@ -0,0 +1,8 @@
+## Copyright (C) 1996-2023 The Squid Software Foundation and contributors
+##
+## Squid software is distributed under GPLv2+ license and includes
+## contributions from numerous individuals and organizations.
+## Please see the COPYING and CONTRIBUTORS files for details.
+##
+
+acl foo dstdomain .example.com .example.com
diff --git a/test-suite/squidconf/dstdomain-domain-dup.conf.instructions b/test-suite/squidconf/dstdomain-domain-dup.conf.instructions
@@ -0,0 +1 @@
+expect-message WARNING:.*is.duplicated.in.the.list.*You.should.remove.one
diff --git a/test-suite/squidconf/dstdomain-subdomain-after.conf b/test-suite/squidconf/dstdomain-subdomain-after.conf
@@ -0,0 +1,8 @@
+## Copyright (C) 1996-2023 The Squid Software Foundation and contributors
+##
+## Squid software is distributed under GPLv2+ license and includes
+## contributions from numerous individuals and organizations.
+## Please see the COPYING and CONTRIBUTORS files for details.
+##
+
+acl foo dstdomain .example.com a.example.com
diff --git a/test-suite/squidconf/dstdomain-subdomain-after.conf.instructions b/test-suite/squidconf/dstdomain-subdomain-after.conf.instructions
@@ -0,0 +1 @@
+expect-message WARNING:.*is.a.subdomain.of.*You.should.remove
diff --git a/test-suite/squidconf/dstdomain-subdomain-before.conf b/test-suite/squidconf/dstdomain-subdomain-before.conf
@@ -0,0 +1,8 @@
+## Copyright (C) 1996-2023 The Squid Software Foundation and contributors
+##
+## Squid software is distributed under GPLv2+ license and includes
+## contributions from numerous individuals and organizations.
+## Please see the COPYING and CONTRIBUTORS files for details.
+##
+
+acl foo dstdomain a.example.com .example.com
diff --git a/test-suite/squidconf/dstdomain-subdomain-before.conf.instructions b/test-suite/squidconf/dstdomain-subdomain-before.conf.instructions
@@ -0,0 +1 @@
+expect-failure ERROR:.configuration.failure:.*is.a.subdomain.of.*You.need.to.remove
diff --git a/test-suite/squidconf/status-dub.conf.instructions b/test-suite/squidconf/status-dub.conf.instructions
@@ -0,0 +1 @@
+expect-message WARNING:.*is.a.subrange.of.*Because.of.this.*isignored
diff --git a/test-suite/squidconf/status-dup.conf b/test-suite/squidconf/status-dup.conf
@@ -0,0 +1,8 @@
+## Copyright (C) 1996-2023 The Squid Software Foundation and contributors
+##
+## Squid software is distributed under GPLv2+ license and includes
+## contributions from numerous individuals and organizations.
+## Please see the COPYING and CONTRIBUTORS files for details.
+##
+
+acl foo http_status 200 200
diff --git a/test-suite/squidconf/subnetwork.conf b/test-suite/squidconf/subnetwork.conf
@@ -0,0 +1,8 @@
+## Copyright (C) 1996-2023 The Squid Software Foundation and contributors
+##
+## Squid software is distributed under GPLv2+ license and includes
+## contributions from numerous individuals and organizations.
+## Please see the COPYING and CONTRIBUTORS files for details.
+##
+
+acl foo src 192.168.0.0/16 192.168.1.0/24
diff --git a/test-suite/squidconf/subnetwork.conf.instructions b/test-suite/squidconf/subnetwork.conf.instructions
@@ -0,0 +1 @@
+expect-message WARNING:.*is.a.subnetwork.of.*Because.of.this.*is.ignored
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1 @@
		expect-message WARNING:.is.duplicated.in.the.list.You.should.remove.one
rousskov marked this conversation as resolved. Show resolved Hide resolved
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1 @@
		expect-message WARNING:.is.a.subdomain.of.You.should.remove
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1 @@
		expect-failure ERROR:.configuration.failure:.is.a.subdomain.of.You.need.to.remove
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1 @@
		expect-message WARNING:.is.a.subrange.of.Because.of.this.*isignored
Copy link rousskov Nov 24, 2023 Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. AFAICT, there is a typo in this expect-message value (missing space), and the instruction file name is misspelled. BTW, if you can think of a simple way to catch mismatching instruction files, we should post the corresponding improvement in a dedicated PR. Otherwise, such twin errors may go unnoticed for a long time! Copy link Author eduard-bagdasaryan Nov 29, 2023 Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. a simple way to catch mismatching instruction files If we are going to keep to the "bad-" for negative tests, we can require that each "bad-" had an instruction file - that should be easy to adjust, I think. If positive tests (i.e., all other ones) do not need instruction files, then we can also prohibit such files for them. Copy link rousskov Nov 29, 2023 Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. The "bad" prefix is for test cases that contain problematic configurations that should not be used as a starting point for deployed configurations. I expect all of them to have instruction files because Squid should warn about problems (at least). If this is easy to check, we probably should, but it is not a high priority, and I am afraid that it may trigger wasteful discussions. Let's not do that just yet. The lack of a "bad" prefix does not imply the lack of instructions file. Some instructions are purely "positive". Ideally, we should catch instruction files that do not have the corresponding configuration file. That check, AFAICT, will catch most of the problems that are very difficult for a human to spot during review (and that do not result in "make check" failures today). Perhaps there is some simple Makefile.am (dependency) trick that can require a configuration file for each instruction file? Copy link rousskov Jan 17, 2024 Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. Ideally, we should catch instruction files that do not have the corresponding configuration file. That check, AFAICT, will catch most of the problems that are very difficult for a human to spot during review (and that do not result in "make check" failures today). Perhaps there is some simple Makefile.am (dependency) trick that can require a configuration file for each instruction file? If you know of such a trick, let's discuss adding it (in a dedicated minimal PR). There is no rush with this, of course. I just did not want this accidentally discovered problem to be forgotten if it has a simple solution. Copy link Author eduard-bagdasaryan Jan 30, 2024 Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. Unfortunately I could not find a simple (one-liner?) solution. However, we have already some logic in test-suite/Makefile.am for squid-conf-tests target. What if we enhance it with an additional check: squid-conf-tests: $(srcdir)/test-squid-conf.sh $(top_builddir)/src/squid.conf.default $(srcdir)/squidconf/* - @failed=0; cfglist="$(top_builddir)/src/squid.conf.default $(srcdir)/squidconf/.conf"; rm -f $@ \|\| $(TRUE); \ + @failed=0; instrlist="$(srcdir)/squidconf/.conf.instructions"; cfglist="$(top_builddir)/src/squid.conf.default $(srcdir)/squidconf/.conf"; rm -f $@ \|\| $(TRUE); \ + for instr in $$instrlist; do \ + cfg=$(srcdir)/squidconf/$$(basename $$instr .instructions); \ + if ! test -f $$cfg; then \ + echo "FAIL: $$cfg is missing"; \ + failed=1; \ + break; \ + fi; \ + done; \ + if test "$$failed" -eq 1; then exit 1; fi; \ for cfg in $$cfglist ; do \ We can either abort the test (as in this snippet) upon spotting the first missing file or just warn about all missing .conf files. Copy link rousskov* Jan 31, 2024 Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. LGTM. Please post a dedicated official "Maintenance: Detect mismatching .instructions files" PR with the following adjustments: Rename instrlist to instructionFIles and instr to instructionFile If possible, use backticks for basenane invocation. This is for consistency sake: `git grep basename '.am'`. If you prefer, save the basename result into a local cfgBasename variable and use that variable to set `cfg`. If possible, start with the new instrlist assignment/loop, keeping the old `@failed=...rm...` line (below the new loop) intact. Quit on the first error to simplify and* avoid producing misleading results when running tests with missing/misspelled instruction files.
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1 @@
		expect-message WARNING:.is.a.subnetwork.of.Because.of.this.*is.ignored