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

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
Received on Mon Apr 29 2013 - 15:05:00 MDT