Re: [PATCH] Deprecate log_icap and log_access configuration directives

On 11/06/2013 10:16 p.m., Tsantilas Christos wrote:
> The log_icap and log_access are not really needed.
> The users have acls control for access and icap logging using the
> access_log and icap_log configuration directives.
>
> This patch removes these options from configuration file.
>
>
> This is a Measurement Factory project

Overall I like it. Removing code and a frequent source of user confusion
in one patch.

Please also note in the descriptive message that this alters the
documented behaviour of "Requests denied for logging will also not be
accounted for in performance counters.", and now makes all traffic get
performane counters accounting.

Also, this is not exactly backward-compatible. We *do* have a number of
installations using log_access to restrict their logging for limited
storage spaces which will suddenly get much more log data and face DoS
effects sometime after an upgrade. This will need to have a small code
snippet in src/parse_cf.cc function parse_obsolete() which detects at
minimum log_access, maybe log_icap, and uses self_destruct() to
highlight the change.

in src/adaptation/icap/icap_log.cc:
* including HttpReply.h pulls in HttpMsg.h. No need to #include both.
* FilledChecklist can be created as local variables. Please remove the
unnecessary new/delete in icapLogLog()
   + this also goes for the src/client_side.cc checklist.

- is it really necessary to send dash_str as the IDENT username? doing
so completely breaks 'ident' ACLs on icap_log.
   This does appear to have been a problem inherited from maybeLog(),
but now is a good time to fix it.

in src/cf.data.pre:
* directives of type "obsolete" have their DOC text turned into debugs() .
   Please replace the description with a short sentence informing the
admin what to do to their config file. (one-liner if possible)
* please remove the DEFAULT_DOC, DEFAULT, COMMENT, IFDEF and LOC lines
which are now useless.

Amos
Received on Tue Jun 11 2013 - 11:15:00 MDT