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

Sorry for being so slow on this reply.

On 30/04/2013 7:14 a.m., Alex Rousskov wrote:
> Hello,
>
> The attached r12779 patch contains all current cumulative changes
> for bug 3389. Please see the patch preamble for technical details. They
> have not changed since the last submission except I added (iii) to
> clarify parsing assumption. The changes correspond to the following lp
> branch (at revision 12779):
> https://code.launchpad.net/~squid/squid/bug3389
>
> This snapshot implements the following Amos request:
>
> On 04/29/2013 08:59 AM, Amos Jeffries wrote:
>
>> * 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.
> The changes to implement the above are limited to src/cache_cf.cc and
> src/cf.data.pre.
>
> I believe this snapshot addresses all review issues raised so far.

The rest that I can find is mostly just polishing bits.

in cf.data.pre:
* text about module"none" has "]]" at the end of the optional ACL list.
Please take the opportunity to remove one of the brackets.

in src/cache_cf.cc:
* doxygen comment before parse_access_log() is missing the " * " prefix
to each line signalling that it is a doxygen comment text and not a
verbatim code example.

in src/log/TcpLogger.h
* please remove the squid.h include. It must be first in the .cc and
_not_ present in any .h.
* please convert the #ifdef to #if for HAVE_LIST
* please remove the XXX commenat about Open().
* please add whitespace line between logRecord() and start of
documentation for flush(). Same in .cc between the global-static
variables near the top.
* TYPO: "unpexted"
* please shrink the double-empty lines at the end of file to one (same
in the .cc file above CBDATA_CLASS_INIT)

in src/log/TcpLogger.cc:
* in Log::TcpLogger::connectDone() please use prefix ++ instead of
suffix in "connectFailures++ % 100 == 0"
* in Log::TcpLogger::Open() please use xfree(strAddr) instead of
safe_free. safe_free() is only useful if the variable needs to be set
NULL after free (eg, for members or for later logics).

I am happy for the above to go in during merge, without another audit
cycle. +1.

Amos
Received on Sat May 11 2013 - 06:36:53 MDT