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

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

Yes, but it is a weakness of the current syntax, not its strength.

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

I think our primary motivation should be "good configuration
design"(succinct, easy to understand, easy to extend, etc.). Temporary
coding difficulties ought to be secondary factors in deliberations about
that design. Yesterday, we did not have the ability to undo strtok().
Today we have that ability. Tomorrow, we may have a grammar-based parser
that does not use strtok() and does not need [external/explicit] undo.

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

The instincts (intuition?) should tell a good admin to use options she
wants to change and only those options. Adding default log format name
in order to add ACLs is far from intuitive IMO! Furthermore, not being
able to specify log format using a named option when other log
configuration aspects are specified using a named option, is not intuitive.

If I missed some reasons why the log format should be required (despite
having a commonly used default) with ACLs and why it should not be
specified using the same syntax as other options, please let me know.

Thank you,

Alex.
Received on Tue Apr 16 2013 - 15:41:09 MDT