From: Alex Rousskov <rousskov@measurement-factory.com>
Date: Thu, 20 May 2021 03:11:17 +0000 (+0000)
Subject: Make CustomLog more reusable (#824)
X-Git-Tag: 4.15-20210525-snapshot~1
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=39d7714ae03e9d9b27dfb0ffdfb1a6a5de01ccf4;p=thirdparty%2Fsquid.git

Make CustomLog more reusable (#824)

The future tls_key_log directive supports most access_log features but
should not inherit deprecated configuration styles, multi-directive
support complications, and legacy top-level configuration parsing code.
The shared/reusable bits were extracted from CustomLog and cache_cf.cc
functions into the new FormattedLog class. Some of them were polished.

All existing access_log/etc. directives continue to use CustomLog, so
nothing in the code justifies FormattedLog existence right now.

Fixed at least these old bugs:

* access_log config dumps were missing the destination parameter
* "access_log none" config dump included a bogus "logformat=none" option
* "access_log buffer-size=..." config dump were missing size units
* built-in logformat re-definition check missed the "icap_squid" format
* "access_log rotate=N" accepted (but then ignored) negative N values
* "access_log rotate=N" was stored as short but dumped as full integer

Admin-visible changes:

* access_log key=value strings are now case sensitive. This change
  improves consistency with squid.conf directives that are all case
  sensitive and most other directive options. The logformat name was
  already case sensitive. I see no good reason to keep this undocumented
  exception.
---

diff --git a/src/ConfigParser.cc b/src/ConfigParser.cc
index d9f69996a9..4fd966de08 100644
--- a/src/ConfigParser.cc
+++ b/src/ConfigParser.cc
@@ -454,6 +454,32 @@ ConfigParser::NextQuotedOrToEol()
     return token;
 }
 
+bool
+ConfigParser::optionalKvPair(char * &key, char * &value)
+{
+    key = nullptr;
+    value = nullptr;
+
+    if (const char *currentToken = PeekAtToken()) {
+        // NextKvPair() accepts "a = b" and skips "=" or "a=". To avoid
+        // misinterpreting the admin intent, we use strict checks.
+        if (const auto middle = strchr(currentToken, '=')) {
+            if (middle == currentToken)
+                throw TextException(ToSBuf("missing key in a key=value option: ", currentToken), Here());
+            if (middle + 1 == currentToken + strlen(currentToken))
+                throw TextException(ToSBuf("missing value in a key=value option: ", currentToken), Here());
+        } else
+            return false; // not a key=value token
+
+        if (!NextKvPair(key, value)) // may still fail (e.g., bad value quoting)
+            throw TextException(ToSBuf("invalid key=value option: ", currentToken), Here());
+
+        return true;
+    }
+
+    return false; // end of directive or input
+}
+
 bool
 ConfigParser::NextKvPair(char * &key, char * &value)
 {
diff --git a/src/ConfigParser.h b/src/ConfigParser.h
index cafcfa9e1e..b53722cf5c 100644
--- a/src/ConfigParser.h
+++ b/src/ConfigParser.h
@@ -56,6 +56,11 @@ public:
     /// rejects configuration due to a repeated directive
     void rejectDuplicateDirective();
 
+    /// extracts an optional key=value token or returns false
+    /// rejects configurations with empty keys or empty values
+    /// key and value have lifetime of the current line/directive
+    bool optionalKvPair(char * &key, char * &value);
+
     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 847cc74098..8413a006ff 100644
--- a/src/cache_cf.cc
+++ b/src/cache_cf.cc
@@ -21,6 +21,7 @@
 #include "auth/Config.h"
 #include "auth/Scheme.h"
 #include "AuthReg.h"
+#include "base/PackableStream.h"
 #include "base/RunnersRegistry.h"
 #include "cache_cf.h"
 #include "CachePeer.h"
@@ -165,7 +166,6 @@ static void parse_access_log(CustomLog ** customlog_definitions);
 static int check_null_access_log(CustomLog *customlog_definitions);
 static void dump_access_log(StoreEntry * entry, const char *name, CustomLog * definitions);
 static void free_access_log(CustomLog ** definitions);
-static bool setLogformat(CustomLog *cl, const char *name, const bool dieWhenMissing);
 
 static void configDoConfigure(void);
 static void parse_refreshpattern(RefreshPattern **);
@@ -4064,6 +4064,8 @@ requirePathnameExists(const char *name, const char *path)
  *
  * #4: Configurable logging module with name=value options such as logformat=x:
  * The first ACL name may not contain '='.
+ * Without any optional parts, directives using this style are indistinguishable
+ * from directives using style #1 until we start requiring the "module:" prefix.
  * access_log module:place [option ...] [acl ...]
  *
  */
@@ -4076,12 +4078,9 @@ parse_access_log(CustomLog ** logs)
         return;
     }
 
-    CustomLog *cl = (CustomLog *)xcalloc(1, sizeof(*cl));
+    const auto cl = new CustomLog();
 
     cl->filename = xstrdup(filename);
-    // default buffer size and fatal settings
-    cl->bufferSize = 8*MAX_URL;
-    cl->fatal = true;
 
     if (strcmp(filename, "none") == 0) {
         cl->type = Log::Format::CLF_NONE;
@@ -4092,57 +4091,22 @@ parse_access_log(CustomLog ** logs)
         return;
     }
 
-    cl->type = Log::Format::CLF_UNKNOWN;
-    cl->rotateCount = -1; // default: use global logfile_rotate setting.
-
     const char *token = ConfigParser::PeekAtToken();
-    if (!token) { // style #1
-        // no options to deal with
-    } else if (!strchr(token, '=')) { // style #3
-        // if logformat name is recognized,
-        // pop the previewed token; Else it must be an ACL name
-        if (setLogformat(cl, token, false))
-            (void)ConfigParser::NextToken();
-    } else { // style #4
-        do {
-            if (strncasecmp(token, "on-error=", 9) == 0) {
-                if (strncasecmp(token+9, "die", 3) == 0) {
-                    cl->fatal = true;
-                } else if (strncasecmp(token+9, "drop", 4) == 0) {
-                    cl->fatal = false;
-                } else {
-                    debugs(3, DBG_CRITICAL, "Unknown value for on-error '" <<
-                           token << "' expected 'drop' or 'die'");
-                    xfree(cl->filename);
-                    xfree(cl);
-                    self_destruct();
-                    return;
-                }
-            } else if (strncasecmp(token, "buffer-size=", 12) == 0) {
-                parseBytesOptionValue(&cl->bufferSize, B_BYTES_STR, token+12);
-            } else if (strncasecmp(token, "rotate=", 7) == 0) {
-                cl->rotateCount = xatoi(token + 7);
-            } else if (strncasecmp(token, "logformat=", 10) == 0) {
-                setLogformat(cl, token+10, true);
-            } else if (!strchr(token, '=')) {
-                // Do not pop the token; it must be an ACL name
-                break; // done with name=value options, now to ACLs
-            } else {
-                debugs(3, DBG_CRITICAL, "Unknown access_log option " << token);
-                xfree(cl->filename);
-                xfree(cl);
-                self_destruct();
-                return;
-            }
-            // Pop the token, it was a valid "name=value" option
-            (void)ConfigParser::NextToken();
-            // Get next with preview ConfigParser::NextToken call.
-        } while ((token = ConfigParser::PeekAtToken()) != NULL);
+    if (token && !strchr(token, '=')) { // style #3
+        // TODO: Deprecate this style to avoid this dangerous guessing.
+        if (Log::TheConfig.knownFormat(token)) {
+            cl->setLogformat(token);
+            (void)ConfigParser::NextToken(); // consume the token used above
+        } else {
+            // assume there is no explicit logformat name and use the default
+            cl->setLogformat("squid");
+        }
+    } else { // style #1 or style #4
+        // TODO: Drop deprecated style #1 support. We already warn about it, and
+        // its exceptional treatment makes detecting "module" typos impractical!
+        cl->parseOptions(LegacyParser, "squid");
     }
-
-    // set format if it has not been specified explicitly
-    if (cl->type == Log::Format::CLF_UNKNOWN)
-        setLogformat(cl, "squid", true);
+    assert(cl->type); // setLogformat() was called
 
     aclParseAclList(LegacyParser, &cl->aclList, cl->filename);
 
@@ -4152,66 +4116,6 @@ parse_access_log(CustomLog ** logs)
     *logs = cl;
 }
 
-/// sets CustomLog::type and, if needed, CustomLog::lf
-/// returns false iff there is no named log format
-static bool
-setLogformat(CustomLog *cl, const char *logdef_name, const bool dieWhenMissing)
-{
-    assert(cl);
-    assert(logdef_name);
-
-    debugs(3, 9, "possible " << cl->filename << " logformat: " << logdef_name);
-
-    if (cl->type != Log::Format::CLF_UNKNOWN) {
-        debugs(3, DBG_CRITICAL, "FATAL: Second logformat name in one access_log: " <<
-               logdef_name << " " << cl->type << " ? " << Log::Format::CLF_NONE);
-        self_destruct();
-        return false;
-    }
-
-    /* look for the definition pointer corresponding to this name */
-    Format::Format *lf = Log::TheConfig.logformats;
-
-    while (lf != NULL) {
-        debugs(3, 9, "Comparing against '" << lf->name << "'");
-
-        if (strcmp(lf->name, logdef_name) == 0)
-            break;
-
-        lf = lf->next;
-    }
-
-    if (lf != NULL) {
-        cl->type = Log::Format::CLF_CUSTOM;
-        cl->logFormat = lf;
-    } else if (strcmp(logdef_name, "auto") == 0) {
-        debugs(0, DBG_CRITICAL, "WARNING: Log format 'auto' no longer exists. Using 'squid' instead.");
-        cl->type = Log::Format::CLF_SQUID;
-    } else if (strcmp(logdef_name, "squid") == 0) {
-        cl->type = Log::Format::CLF_SQUID;
-    } else if (strcmp(logdef_name, "common") == 0) {
-        cl->type = Log::Format::CLF_COMMON;
-    } else if (strcmp(logdef_name, "combined") == 0) {
-        cl->type = Log::Format::CLF_COMBINED;
-#if ICAP_CLIENT
-    } else if (strcmp(logdef_name, "icap_squid") == 0) {
-        cl->type = Log::Format::CLF_ICAP_SQUID;
-#endif
-    } else if (strcmp(logdef_name, "useragent") == 0) {
-        cl->type = Log::Format::CLF_USERAGENT;
-    } else if (strcmp(logdef_name, "referrer") == 0) {
-        cl->type = Log::Format::CLF_REFERER;
-    } else if (dieWhenMissing) {
-        debugs(3, DBG_CRITICAL, "FATAL: Log format '" << logdef_name << "' is not defined");
-        self_destruct();
-        return false;
-    } else {
-        return false;
-    }
-
-    return true;
-}
-
 static int
 check_null_access_log(CustomLog *customlog_definitions)
 {
@@ -4221,62 +4125,15 @@ check_null_access_log(CustomLog *customlog_definitions)
 static void
 dump_access_log(StoreEntry * entry, const char *name, CustomLog * logs)
 {
-    CustomLog *log;
-
-    for (log = logs; log; log = log->next) {
-        storeAppendPrintf(entry, "%s ", name);
-
-        switch (log->type) {
-
-        case Log::Format::CLF_CUSTOM:
-            storeAppendPrintf(entry, "%s logformat=%s", log->filename, log->logFormat->name);
-            break;
-
-        case Log::Format::CLF_NONE:
-            storeAppendPrintf(entry, "logformat=none");
-            break;
-
-        case Log::Format::CLF_SQUID:
-            // this is the default, no need to add to the dump
-            //storeAppendPrintf(entry, "%s logformat=squid", log->filename);
-            break;
-
-        case Log::Format::CLF_COMBINED:
-            storeAppendPrintf(entry, "%s logformat=combined", log->filename);
-            break;
-
-        case Log::Format::CLF_COMMON:
-            storeAppendPrintf(entry, "%s logformat=common", log->filename);
-            break;
-
-#if ICAP_CLIENT
-        case Log::Format::CLF_ICAP_SQUID:
-            storeAppendPrintf(entry, "%s logformat=icap_squid", log->filename);
-            break;
-#endif
-        case Log::Format::CLF_USERAGENT:
-            storeAppendPrintf(entry, "%s logformat=useragent", log->filename);
-            break;
-
-        case Log::Format::CLF_REFERER:
-            storeAppendPrintf(entry, "%s logformat=referrer", log->filename);
-            break;
-
-        case Log::Format::CLF_UNKNOWN:
-            break;
+    assert(entry);
+    for (auto log = logs; log; log = log->next) {
+        {
+            PackableStream os(*entry);
+            os << name; // directive name
+            os << ' ' << log->filename; // including "none"
+            log->dumpOptions(os);
         }
 
-        // default is on-error=die
-        if (!log->fatal)
-            storeAppendPrintf(entry, " on-error=drop");
-
-        // default: 64KB
-        if (log->bufferSize != 64*1024)
-            storeAppendPrintf(entry, " buffer-size=%" PRIuSIZE, log->bufferSize);
-
-        if (log->rotateCount >= 0)
-            storeAppendPrintf(entry, " rotate=%d", log->rotateCount);
-
         if (log->aclList)
             dump_acl_list(entry, log->aclList);
 
@@ -4290,16 +4147,7 @@ free_access_log(CustomLog ** definitions)
     while (*definitions) {
         CustomLog *log = *definitions;
         *definitions = log->next;
-
-        log->logFormat = NULL;
-        log->type = Log::Format::CLF_UNKNOWN;
-
-        if (log->aclList)
-            aclDestroyAclList(&log->aclList);
-
-        safe_free(log->filename);
-
-        xfree(log);
+        delete log;
     }
 }
 
diff --git a/src/log/Config.cc b/src/log/Config.cc
index 24b93ad2bc..f24c8b4472 100644
--- a/src/log/Config.cc
+++ b/src/log/Config.cc
@@ -14,6 +14,92 @@
 
 Log::LogConfig Log::TheConfig;
 
+const char *
+Log::LogConfig::BuiltInFormatName(const Format::log_type logformatType)
+{
+    switch (logformatType) {
+    case Format::CLF_UNKNOWN:
+    case Format::CLF_NONE:
+    case Format::CLF_CUSTOM:
+        return nullptr; // the above types are not built-in
+
+    case Format::CLF_SQUID:
+        return "squid";
+
+    case Format::CLF_COMBINED:
+        return "combined";
+
+    case Format::CLF_COMMON:
+        return "common";
+
+#if ICAP_CLIENT
+    case Format::CLF_ICAP_SQUID:
+        return "icap_squid";
+#endif
+
+    case Format::CLF_USERAGENT:
+        return "useragent";
+
+    case Log::Format::CLF_REFERER:
+        return "referrer";
+    }
+
+    // forgotten (by developers) type, invalid type, or unreachable code
+    return nullptr;
+}
+
+Log::Format::log_type
+Log::LogConfig::FindBuiltInFormat(const char *logformatName)
+{
+    assert(logformatName);
+
+    if (strcmp(logformatName, "auto") == 0) {
+        debugs(0, DBG_CRITICAL, "WARNING: Log format 'auto' no longer exists. Using 'squid' instead.");
+        return Format::CLF_SQUID;
+    }
+
+    if (strcmp(logformatName, "squid") == 0)
+        return Format::CLF_SQUID;
+
+    if (strcmp(logformatName, "common") == 0)
+        return Format::CLF_COMMON;
+
+    if (strcmp(logformatName, "combined") == 0)
+        return Format::CLF_COMBINED;
+
+#if ICAP_CLIENT
+    if (strcmp(logformatName, "icap_squid") == 0)
+        return Format::CLF_ICAP_SQUID;
+#endif
+
+    if (strcmp(logformatName, "useragent") == 0)
+        return Format::CLF_USERAGENT;
+
+    if (strcmp(logformatName, "referrer") == 0)
+        return Format::CLF_REFERER;
+
+    // CLF_NONE, CLF_UNKNOWN, CLF_CUSTOM types cannot be specified explicitly.
+    // TODO: Ban "none" and "unknown" custom logformat names to avoid confusion.
+    return Format::CLF_UNKNOWN;
+}
+
+Format::Format *
+Log::LogConfig::findCustomFormat(const char *logformatName) const
+{
+    assert(logformatName);
+    for (auto i = logformats; i ; i = i->next) {
+        if (strcmp(i->name, logformatName) == 0)
+            return i;
+    }
+    return nullptr;
+}
+
+bool
+Log::LogConfig::knownFormat(const char *logformatName) const
+{
+    return FindBuiltInFormat(logformatName) || findCustomFormat(logformatName);
+}
+
 void
 Log::LogConfig::parseFormats()
 {
@@ -25,22 +111,14 @@ Log::LogConfig::parseFormats()
         return;
     }
 
-    // check for re-definition of built-in formats
-    if (strcmp(name, "squid") == 0 ||
-            strcmp(name, "common") == 0 ||
-            strcmp(name, "combined") == 0 ||
-            strcmp(name, "useragent") == 0 ||
-            strcmp(name, "referrer") == 0) {
-        debugs(3, DBG_PARSE_NOTE(DBG_IMPORTANT), "ERROR: logformat " << name << " is already defined. Ignoring.");
+    if (FindBuiltInFormat(name)) {
+        debugs(3, DBG_PARSE_NOTE(DBG_IMPORTANT), "ERROR: logformat " << name << " is a built-in format. Ignoring redefinition attempt.");
         return;
     }
 
-    // check for re-definition of custom formats
-    for (auto i = logformats; i ; i = i->next) {
-        if (strcmp(i->name, name) == 0) {
-            debugs(3, DBG_PARSE_NOTE(DBG_IMPORTANT), "ERROR: logformat " << name << " is already defined. Ignoring.");
-            return;
-        }
+    if (findCustomFormat(name)) {
+        debugs(3, DBG_PARSE_NOTE(DBG_IMPORTANT), "ERROR: logformat " << name << " is already defined. Ignoring redefinition attempt.");
+        return;
     }
 
     ::Format::Format *nlf = new ::Format::Format(name);
diff --git a/src/log/Config.h b/src/log/Config.h
index fd96a71f6b..87a055488a 100644
--- a/src/log/Config.h
+++ b/src/log/Config.h
@@ -10,6 +10,7 @@
 #define SQUID_SRC_LOG_CONFIG_H
 
 #include "format/Format.h"
+#include "log/Formats.h"
 
 class StoreEntry;
 
@@ -19,6 +20,20 @@ namespace Log
 class LogConfig
 {
 public:
+    /// \returns the name of the given built-in logformat type (or nil)
+    static const char *BuiltInFormatName(Format::log_type type);
+
+    /// \returns either a named built-in logformat ID or CLF_UNKNOWN
+    static Format::log_type FindBuiltInFormat(const char *logformatName);
+    static_assert(!Log::Format::CLF_UNKNOWN, "FindBuiltInFormat(unknown) is falsy");
+
+    /// \returns a named (previously configured) custom log format object or nil
+    ::Format::Format *findCustomFormat(const char *logformatName) const;
+
+    /// whether the given format name is supported, either as a built-in or a
+    /// (previously configured) custom logformat
+    bool knownFormat(const char *logformatName) const;
+
     void parseFormats();
     void dumpFormats(StoreEntry *e, const char *name) {
         logformats->dump(e, name);
diff --git a/src/log/CustomLog.cc b/src/log/CustomLog.cc
deleted file mode 100644
index 61fd0d5106..0000000000
--- a/src/log/CustomLog.cc
+++ /dev/null
@@ -1,18 +0,0 @@
-/*
- * Copyright (C) 1996-2021 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.
- */
-
-#include "squid.h"
-#include "log/CustomLog.h"
-#include "log/File.h"
-
-bool
-CustomLog::usesDaemon() const
-{
-    return (filename && strncmp(filename, "daemon:", 7) == 0);
-}
-
diff --git a/src/log/CustomLog.h b/src/log/CustomLog.h
index a34c6db98a..62a46225f4 100644
--- a/src/log/CustomLog.h
+++ b/src/log/CustomLog.h
@@ -9,34 +9,16 @@
 #ifndef SQUID_CUSTOMLOG_H_
 #define SQUID_CUSTOMLOG_H_
 
-#include "acl/forward.h"
-#include "log/Formats.h"
+#include "log/FormattedLog.h"
 
-class Logfile;
-namespace Format
-{
-class Format;
-}
-
-/// representation of a custom log directive.
-class CustomLog
+// TODO: Replace with std::list<FormattedLog> or its wrapper.
+/// all same-directive transaction logging rules
+/// (e.g., all access_log rules or all icap_log rules)
+class CustomLog: public FormattedLog
 {
 public:
-    /// \returns whether the daemon module is used for this log
-    bool usesDaemon() const;
-
-    char *filename;
-    ACLList *aclList;
-    Format::Format *logFormat;
-    Logfile *logfile;
-    CustomLog *next;
-    Log::Format::log_type type;
-    /// how much to buffer before dropping or dying (access_log buffer-size)
-    size_t bufferSize;
-    /// whether unrecoverable errors (e.g., dropping a log record) kill worker
-    bool fatal;
-    /// How many log files to retain when rotating. Default: obey logfile_rotate
-    int16_t rotateCount;
+    /// next _log line (if any); maintained by cache_cf.cc
+    CustomLog *next = nullptr;
 };
 
 #endif /* SQUID_CUSTOMLOG_H_ */
diff --git a/src/log/FormattedLog.cc b/src/log/FormattedLog.cc
new file mode 100644
index 0000000000..da680e7802
--- /dev/null
+++ b/src/log/FormattedLog.cc
@@ -0,0 +1,173 @@
+/*
+ * Copyright (C) 1996-2021 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.
+ */
+
+#include "squid.h"
+#include "acl/Gadgets.h"
+#include "base/TextException.h"
+#include "cache_cf.h"
+#include "Debug.h"
+#include "log/Config.h"
+#include "log/File.h"
+#include "log/FormattedLog.h"
+#include "Parsing.h"
+#include "sbuf/Stream.h"
+#include "SquidConfig.h"
+
+FormattedLog::~FormattedLog()
+{
+    close(); // TODO: destructing a Logfile object should be enough
+    aclDestroyAclList(&aclList);
+    safe_free(filename);
+    // leave logFormat alone -- we do not own that object
+}
+
+bool
+FormattedLog::usesDaemon() const
+{
+    return (filename && strncmp(filename, "daemon:", 7) == 0);
+}
+
+void
+FormattedLog::parseOptions(ConfigParser &parser, const char *defaultFormatName)
+{
+    const char *explicitFormatName = nullptr;
+    char *key = nullptr;
+    char *value = nullptr;
+    while (parser.optionalKvPair(key, value)) {
+
+        if (strcmp(key, "on-error") == 0) {
+            if (strcmp(value, "die") == 0) {
+                fatal = true;
+            } else if (strcmp(value, "drop") == 0) {
+                fatal = false;
+            } else {
+                throw TextException(ToSBuf("unsupported ", cfg_directive, " on-error value: ", value,
+                                           Debug::Extra, "expected 'drop' or 'die'"), Here());
+            }
+            continue;
+        }
+
+        if (strcmp(key, "buffer-size") == 0) {
+            parseBytesOptionValue(&bufferSize, "bytes", value);
+            continue;
+        }
+
+        if (strcmp(key, "rotate") == 0) {
+            rotationsToKeep = Optional<unsigned int>(xatoui(value));
+            continue;
+        }
+
+        if (strcmp(key, "logformat") == 0 && defaultFormatName) {
+            if (explicitFormatName)
+                throw TextException(ToSBuf("duplicated ", cfg_directive, " option: ", key), Here());
+
+            explicitFormatName = value;
+            continue;
+        }
+
+        throw TextException(ToSBuf("unsupported ", cfg_directive, " option: ", key, "=", value), Here());
+    }
+
+    if (const auto formatName = explicitFormatName ? explicitFormatName : defaultFormatName) {
+        assert(defaultFormatName); // this log supports logformat=name
+        setLogformat(formatName);
+    } // else OK: this log does not support logformat=name and none was given
+}
+
+void
+FormattedLog::dumpOptions(std::ostream &os) const
+{
+    /* do not report defaults */
+
+    // TODO: Here and elsewhere, report both explicitly configured settings and
+    // various defaults. Properly excluding defaults requires wrapping most
+    // non-pointer members in Optional _and_ adding methods to compute the final
+    // option value after accounting for defaults (and those may change with
+    // reconfiguration!). And all that effort may still not result in a faithful
+    // reproduction of the original squid.conf because of size unit changes,
+    // order changes, duplicates removal, etc. More importantly, these reports
+    // are much more useful for determining complete Squid state (especially
+    // when triaging older Squids with some difficult-to-figure-out defaults).
+
+    switch (type) {
+    case Log::Format::CLF_UNKNOWN:
+        break; // do not report a format when it was not configured
+
+    case Log::Format::CLF_NONE:
+        break; // the special "none" case has no format to report
+
+    case Log::Format::CLF_SQUID:
+        break; // do not report default format (XXX: icap_log default differs)
+
+    case Log::Format::CLF_CUSTOM:
+        if (logFormat) // paranoid; the format should be set
+            os << " logformat=" << logFormat->name;
+        break;
+
+    default:
+        os << " logformat=" << Log::LogConfig::BuiltInFormatName(type);
+    }
+
+    if (!fatal)
+        os << " on-error=drop";
+
+    if (bufferSize != 8*MAX_URL)
+        os << " buffer-size=" << bufferSize << "bytes";
+
+    if (rotationsToKeep)
+        os << " rotate=" << rotationsToKeep.value();
+}
+
+void
+FormattedLog::setLogformat(const char *logformatName)
+{
+    assert(logformatName);
+    assert(type == Log::Format::CLF_UNKNOWN); // set only once
+    assert(!logFormat); // set only once
+
+    debugs(3, 7, "possible " << filename << " logformat: " << logformatName);
+
+    if (const auto lf = Log::TheConfig.findCustomFormat(logformatName)) {
+        type = Log::Format::CLF_CUSTOM;
+        logFormat = lf;
+        return;
+    }
+
+    if (const auto id = Log::LogConfig::FindBuiltInFormat(logformatName)) {
+        type = id;
+        return;
+    }
+
+    throw TextException(ToSBuf("unknown logformat name in ", cfg_directive, ": ", logformatName), Here());
+}
+
+void
+FormattedLog::open()
+{
+    Must(!logfile);
+    Must(filename);
+    logfile = logfileOpen(filename, bufferSize, fatal);
+    // the opening code reports failures and returns nil if they are non-fatal
+}
+
+void
+FormattedLog::rotate()
+{
+    if (logfile)
+        logfileRotate(logfile, rotationsToKeep.value_or(Config.Log.rotateNumber));
+}
+
+void
+FormattedLog::close()
+{
+    if (logfile) {
+        logfileClose(logfile);
+        logfile = nullptr; // deleted by the closing code
+    }
+}
+
diff --git a/src/log/FormattedLog.h b/src/log/FormattedLog.h
new file mode 100644
index 0000000000..88920d2fb5
--- /dev/null
+++ b/src/log/FormattedLog.h
@@ -0,0 +1,80 @@
+/*
+ * Copyright (C) 1996-2021 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.
+ */
+
+#ifndef SQUID_LOG_FORMATTEDLOG_H_
+#define SQUID_LOG_FORMATTEDLOG_H_
+
+#include "acl/forward.h"
+#include "base/Optional.h"
+#include "log/Formats.h"
+#include "log/forward.h"
+
+#include <iosfwd>
+
+class ConfigParser;
+
+/// A single-destination, single-record-format log.
+/// The customizable destination is based on Logfile "logging modules" API.
+/// Some logs allow the admin to select or specify the record format.
+class FormattedLog
+{
+public:
+    FormattedLog() = default;
+    ~FormattedLog();
+
+    FormattedLog(FormattedLog &&) = delete; // no need to support copying of any kind
+
+    /// \returns whether the daemon module is used for this log
+    bool usesDaemon() const;
+
+    /// handles the [key=value...] part of the log configuration
+    /// \param defaultFormat default logformat or, to force built-in format, nil
+    void parseOptions(ConfigParser&, const char *defaultFormat);
+
+    /// reports explicitly-configured key=value options, in squid.conf format
+    void dumpOptions(std::ostream &os) const;
+
+    /// configures formatting-related settings for the given logformat name
+    void setLogformat(const char *logformatName);
+
+    /// prepare for recording entries
+    void open();
+
+    /// handle the log rotation request
+    void rotate();
+
+    /// stop recording entries
+    void close();
+
+    /// records writer
+    Logfile *logfile = nullptr;
+
+    /// logging destination
+    char *filename = nullptr;
+
+    /// restrict logging to matching transactions
+    ACLList *aclList = nullptr;
+
+    /// custom log record template for type == Log::Format::CLF_CUSTOM
+    Format::Format *logFormat = nullptr;
+
+    /// log record template ID
+    Log::Format::log_type type = Log::Format::CLF_UNKNOWN;
+
+    /// how much to buffer before dropping or dying (buffer-size=N)
+    size_t bufferSize = 8*MAX_URL;
+
+    /// how many log files to retain when rotating. Default: obey logfile_rotate
+    Optional<unsigned int> rotationsToKeep;
+
+    /// whether unrecoverable errors (e.g., dropping a log record) kill worker
+    bool fatal = true;
+};
+
+#endif /* SQUID_LOG_FORMATTEDLOG_H_ */
+
diff --git a/src/log/Makefile.am b/src/log/Makefile.am
index efa85a45a3..20604ff3bf 100644
--- a/src/log/Makefile.am
+++ b/src/log/Makefile.am
@@ -17,7 +17,6 @@ noinst_LTLIBRARIES = liblog.la
 liblog_la_SOURCES = \
 	Config.cc \
 	Config.h \
-	CustomLog.cc \
 	CustomLog.h \
 	File.cc \
 	File.h \
@@ -29,6 +28,8 @@ liblog_la_SOURCES = \
 	FormatSquidReferer.cc \
 	FormatSquidUseragent.cc \
 	Formats.h \
+	FormattedLog.cc \
+	FormattedLog.h \
 	ModDaemon.cc \
 	ModDaemon.h \
 	ModStdio.cc \
diff --git a/src/log/access_log.cc b/src/log/access_log.cc
index a35bdee646..6a1621a8af 100644
--- a/src/log/access_log.cc
+++ b/src/log/access_log.cc
@@ -185,10 +185,7 @@ accessLogRotate(void)
 #endif
 
     for (log = Config.Log.accesslogs; log; log = log->next) {
-        if (log->logfile) {
-            int16_t rc = (log->rotateCount >= 0 ? log->rotateCount : Config.Log.rotateNumber);
-            logfileRotate(log->logfile, rc);
-        }
+        log->rotate();
     }
 
 #if HEADERS_LOG
diff --git a/src/log/forward.h b/src/log/forward.h
index 9ff0d43f66..b45e0204c8 100644
--- a/src/log/forward.h
+++ b/src/log/forward.h
@@ -14,6 +14,7 @@
 class AccessLogEntry;
 typedef RefCount<AccessLogEntry> AccessLogEntryPointer;
 
+class Logfile;
 class LogTags;
 class LogTagsErrors;
 
diff --git a/src/tests/stub_cache_cf.cc b/src/tests/stub_cache_cf.cc
index f9eb70fbbe..90d744ced1 100644
--- a/src/tests/stub_cache_cf.cc
+++ b/src/tests/stub_cache_cf.cc
@@ -26,6 +26,7 @@ void requirePathnameExists(const char *name, const char *path) STUB_NOP
 void parse_time_t(time_t * var) STUB
 void ConfigParser::ParseUShort(unsigned short *var) STUB
 void ConfigParser::ParseWordList(wordlist **) STUB
+void parseBytesOptionValue(size_t *, const char *, char const * value) STUB
 void dump_acl_access(StoreEntry * entry, const char *name, acl_access * head) STUB
 void dump_acl_list(StoreEntry*, ACLList*) STUB
 
diff --git a/src/tests/stub_liblog.cc b/src/tests/stub_liblog.cc
index a313f9c3e0..a07b333484 100644
--- a/src/tests/stub_liblog.cc
+++ b/src/tests/stub_liblog.cc
@@ -43,8 +43,8 @@ void LogConfig::parseFormats() STUB
 LogConfig TheConfig;
 }
 
-#include "log/CustomLog.h"
-bool CustomLog::usesDaemon() const STUB_RETVAL(false)
+#include "log/FormattedLog.h"
+bool FormattedLog::usesDaemon() const STUB_RETVAL(false)
 
 #include "log/File.h"
 CBDATA_CLASS_INIT(Logfile);