On 04/18/2013 07:32 AM, Amos Jeffries wrote:
> On 17/04/2013 3:41 a.m., Alex Rousskov wrote:
>> On 04/16/2013 01:39 AM, Amos Jeffries wrote:
>>> On 16/04/2013 5:30 p.m., Alex Rousskov wrote:
>>>> On 04/15/2013 09:33 PM, Amos Jeffries wrote:
>>>>> On 13/04/2013 4:43 a.m., Alex Rousskov wrote:
>>>>>> The code and detailed development history are also available at
>>>>>> https://code.launchpad.net/~squid/squid/bug3389
>>>>> access_log logger=<module>:<place> [option ...] [acl acl ...]
>>>>> access_log <module>:<place> [<logformat name> [acl acl ...]]
>>>>> access_log none [acl acl ...]
>>>>> I propose keeping the access_log syntax largely unchanged by simply
>>>>> inserting [options] before the format field.
>>>>> access_log module:place [options] [ format [acls ...] ]
>>>> We can do that, but it requires an explicit format name to use ACLs
>>> The existing config does as well.
>> Yes, but it is a weakness of the current syntax, not its strength.
> I know. It is a weakness you are not solving in scope of the TCP module
> upgrade, so needs to be taken advantage of rather than adding more
> complexity to workaround it. Minimal impact changes remember.
I do not think minimal impact is the goal. As long as the impact is in
the feature scope, it should be maximized, not minimized! Since proper
support for TCP loggers requires adding configuration parameters, and
since old logger configuration styles do not support new configuration
parameters, adding such support is in scope (Your proposal adds such
support as well so hopefully we do not need to argue about the need for
that support). And since our code added options support in a backward
compatible way, I do not understand why you are objecting that our
changes make an optional parameter optional.
We could split the patch in two: TCP logger changes with hard-coded
configuration parameters plus configuration changes. If you insist on
that, we will do it. Is that what you want? Personally, I see no reason
for such a split and the extra work in requires.
> If you have some way to alter the parser to remove that weakness without
> breaking ACLs or adding a lot of complexity toa simple parser, please
> propose it as a separate patch.
I already posted a fix for parsing ACLs in the new format. It was just a
coding bug, not a design or scope issue. And the fix changes just a few
lines of code. The fix works in my tests, but it is possible that I
missed something, of course. Did I?
> If we have a system to undo strtok() today then please do use that
That is what the posted fix does. I am attaching the same fix to this
email for your reference. Perhaps you missed it earlier?
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)
