On 19/04/2013 2:38 a.m., Alex Rousskov wrote:
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?
Er, yes I did miss that earlier.
Okay, as it stands:
* With the flexible parser we can make format=X an option and still
handle ACLs. I think we are both happy with that situation syntax-wise
in the current patch.
* I object to adding the "logger=" tag on the front of the module:place
field. It seems pointless and a waste of letters.
If the first token after module:place is not an option it must be teh
format name old syntax) or an ACL name (new syntax).
This should be easy enough to fix without adding a surprise logger=
token to the directive.
Amos
