On 04/15/2013 09:33 PM, Amos Jeffries wrote:
> On 13/04/2013 4:43 a.m., Alex Rousskov wrote:
>> The attached trunk patch fixes several problems with TCP module for
>> access_log. The module is unusable in production without these changes
>> if there is a chance the TCP logger becomes unavailable or TCP bandwidth
>> becomes temporary restricted . Nathan Hoad is the primary author of
>> these improvements. Factory helped Nathan during last stages of the
>> project. The code passed initial lab tests.
>>
>> The code and detailed development history are also available at
>> https://code.launchpad.net/~squid/squid/bug3389
> 1) The implementation of the access_log config parser does not match the
> documentation or this description. It currently implements the syntax:
>
> access_log logger=<module>:<place> [option ...]
> access_log <module>:<place> [<logformat name> [acl acl ...]]
> access_log none [acl acl ...]
>
>
> ... the new options code will halt with self_destruct() and "Unknown
> access_log option" if anything like ACLs occur.
My bug. Documentation was correct, but I missed testing this common
case. Very embarrassing. Implementation fixed in the attached patch.
> I propose keeping the access_log syntax largely unchanged by simply
> inserting [options] before the format field.
> Making the directive syntax:
> access_log module:place [options] [ format [acls ...] ]
>
> The parser can exit the options parse loop when either EOL or the log
> format name field is identified. There is also no problems introduced
> about multiple logger= or format= options being listed. This is fully
> backward-compatible and does not loose any popular features.
We can do that, but it requires an explicit format name to use ACLs (for
no good reason that an admin may see) and makes the whole syntax less
uniform because some options have names and some do not (again, for no
good reason that an admin may see). I believe our proposed format is
better in those aspects, and the implementation should be
backwards-compatible without loosing any popular features as well (with
the attached fix).
> 2) splitting the active code from ModTcp.* files into TcpLogger.*
> AsyncJob but leaving ModTcp.* as a wrapper is a very nasty way to do it.
> The log/File.h API definition uses static function pointers, which *do
> not* need to be the ones currently in ModTcp.* - that was simply for
> consistency in the C-style code.
> Please either call the AsyncJob Log::ModTcp and leave it in the
> original files, OR remove the ModTcp.* wrappers entirely - replacing
> with TcpLogger static members.
Sounds good. I think the first option (move the new code back into the
original files and continue to use wrappers) is better because it
minimizes changes while keeping code consistent with other log wrappers.
It also does not try to hide the fact that we are communicating with an
async job directly -- something that I think we should polish as well.
> Either way new objects need to be in the Log:: namespace now.
Will add.
Thank you,
Alex.
Fixed parsing optional ACL names following the new acess_log config style.
ACLs were treated as name=value options because the options loop lacked
the termination condition (other than the end of all tokens on the line).
The correct termination condition is a non-option token (the one that does
not contain '=').
As a side-effect of this change, ACL parts of http_access, access_log and
other ACL-driven rules support inclusion of ACL names from files using
standard quoted "filename" syntax.
=== modified file 'src/acl/Gadgets.cc'
--- src/acl/Gadgets.cc 2012-09-05 14:49:29 +0000
+++ src/acl/Gadgets.cc 2013-04-16 04:46:27 +0000
@@ -192,41 +192,41 @@
A->cfgline = xstrdup(config_input_line);
/* Append to the end of this list */
for (B = *head, T = head; B; T = &B->next, B = B->next);
*T = A;
/* We lock _acl_access structures in ACLChecklist::matchNonBlocking() */
}
void
aclParseAclList(ConfigParser &parser, ACLList ** head)
{
ACLList **Tail = head; /* sane name in the use below */
ACL *a = NULL;
char *t;
/* next expect a list of ACL names, possibly preceeded
* by '!' for negation */
- while ((t = strtok(NULL, w_space))) {
+ while ((t = parser.strtokFile())) {
ACLList *L = new ACLList;
if (*t == '!') {
L->negated (true);
++t;
}
debugs(28, 3, "aclParseAclList: looking for ACL name '" << t << "'");
a = ACL::FindByName(t);
if (a == NULL) {
debugs(28, DBG_CRITICAL, "aclParseAclList: ACL name '" << t << "' not found.");
delete L;
parser.destruct();
continue;
}
L->_acl = a;
*Tail = L;
Tail = &L->next;
=== modified file 'src/cache_cf.cc'
--- src/cache_cf.cc 2013-04-12 16:29:21 +0000
+++ src/cache_cf.cc 2013-04-16 04:46:27 +0000
@@ -4052,63 +4052,66 @@
const char *token = strtok(NULL, w_space);
if (!token) {
self_destruct();
return;
}
if (strcmp(token, "none") == 0) {
cl->type = Log::Format::CLF_NONE;
aclParseAclList(LegacyParser, &cl->aclList);
while (*logs)
logs = &(*logs)->next;
*logs = cl;
return;
}
const char *logdef_name = NULL;
// new style must start with a logger=... option
if (strncasecmp(token, "logger=", 7) == 0) {
cl->filename = xstrdup(token+7);
- while ((token = strtok(NULL, w_space)) != NULL) {
+ while ((token = ConfigParser::strtokFile()) != NULL) {
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'");
self_destruct();
}
} else if (strncasecmp(token, "buffer-size=", 12) == 0) {
parseBytesOptionValue(&cl->bufferSize, B_BYTES_STR, token+12);
} else if (strncasecmp(token, "logger=", 7) == 0) {
debugs(3, DBG_CRITICAL, "duplicate access_log logger=... " <<
"option near " << token);
self_destruct();
} else if (strncasecmp(token, "logformat=", 10) == 0) {
logdef_name = token+10;
- } else {
+ } else if (strchr(token, '=') != NULL) {
debugs(3, DBG_CRITICAL, "Unknown access_log option " << token);
self_destruct();
- return;
+ } else {
+ // put back the token; it must be an ACL
+ ConfigParser::strtokFileUndo();
+ break; // done with name=value options, now to ACLs
}
}
} else {
// old or ancient style
cl->filename = xstrdup(token);
logdef_name = strtok(NULL, w_space); // may be nil
}
if (logdef_name == NULL)
logdef_name = "squid";
debugs(3, 9, "Log definition name '" << logdef_name << "' file '" << cl->filename << "'");
/* 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)
