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

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 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

The existing config does as well.

My first thought was to make format an option too and detect first
unknown option as an ACL. But the ACL parsing is a separate function
that does not support un-doing a strtok(). Leaving the format field
untouched for now makes it useful as a delimiter token which can be used
to switch processing paths without having to un-do anything.

> (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).
>

I'm not sure what type of argument that is. Sounds like such admin need
to give in to their instincts and add the format names.

Amos
Received on Tue Apr 16 2013 - 07:39:44 MDT