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
