On 12/06/2013 1:36 a.m., Tsantilas Christos wrote:
On 06/11/2013 02:14 PM, Amos Jeffries wrote:
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.
Are we OK with this?
I am okay with the change. It just needs documenting so we don't
overlook it in the release documentation.
+1 from me.
+ this also goes for the src/client_side.cc checklist.
well this is not possible here...
Hmm. Okay. We will need to get to that eventually. For now the skip is okay.
- is it really necessary to send dash_str as the IDENT username? doing
so completely breaks 'ident' ACLs on icap_log.
Yes. This is true. My sense is that using the dash_str as IDENT
username, just used to speedup the fast acl checking.
In client_side.cc code inside clientAclChecklistCreate function it is
used as follows:
- If it is already exist in ConnStateData::clientConnection::rfc931
then use it else do not spend time to find it (from cache or lookup).
Probably this logic should be moved inside ident acl, or let ident acl
to do its job. I had this dilemma when started this patch, but at the
end I let it untouched.
The ident ACL already operates like that if the ConnStateData is
available. That is usually setup from the HttpRequest on checklist creation.
The problem is that ident ACL uses the explicit value from checklist
creation to override the ConnStateData value lookup, so using dash_str
makes it always non-match.
Amos
