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

On 13/04/2013 4:43 a.m., Alex Rousskov wrote:
> Hello,
>
> 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
>

Firstly, thank you guys for working on this.

I have a few architectural / design issues with this code in its current
form before a full audit is done...

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.

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.

2) splitting the active code from ModTcp.* files into TcpLogger.*
AsyncJob but leaving ModTcp.* as a wrapper is a very nasty way to do it.
  The log/File.h API definition uses static function pointers, which *do
not* need to be the ones currently in ModTcp.* - that was simply for
consistency in the C-style code.
  Please either call the AsyncJob Log::ModTcp and leave it in the
original files, OR remove the ModTcp.* wrappers entirely - replacing
with TcpLogger static members.
  Either way new objects need to be in the Log:: namespace now.

Amos
Received on Tue Apr 16 2013 - 03:34:02 MDT