Re: [PATCH] Bug 3389: Auto-reconnect for tcp access_log

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.