[squid-dev] [PATCH] external_acl_type logformat tokens

[Alex Rousskov]

On 10/03/2015 02:35 AM, Amos Jeffries wrote:
> Update the external_acl_type helper interface to use libformat and thus
> make any logformat token valid in its format parameter field.
> 
> As a result much of the logic surrounding format code parsing, display
> and helper query generation has been completely dropped. What remains is
> a basic parse loop handling backward compatibility for the unusual
> %CERT_* token syntax, space delimiter and field default encodings.
> 
> 
> Extensions to logformat resulting from the merger:
> 
> * adds \-escape encoding of output fields
> 
> * allows {arg} field to be placed before or after the format code.
> 
> * extended to accept the old external_acl_type %macros. But not
> documented, these are deprecated and only for backward compatibility.
> 
> * extended to support outputting formats without a format-name prefix.

Please rephrase/clarify the last bullet in the commit message. I do not
know what that last bullet means.



> The major side effect of this change is that these ACLs now require
> AccessLogEntry to be filled out with state data, rather than just the
> ACLChecklist object members.
> 
> The requires*() mechanism of ACLChecklist has been extended to catch
> some cases resulting from missing the ALE entirely. But it cannot catch
> the more subtle problem of data members inside the ALE being unset.


Agreed. I expect lots of small but time-wasting and
deployment-preventing bugs because of that change.

Would it be possible to avoid most of those problems by using the
following approach?

1. If both hasAleXXX() and requiresAleXXX() are true in
   ACL::matches(), call a new virtual checklist->syncAle() method.

2. Add FilledChecklist::syncAle() method that fills unset ALE data
   members with data from the checklist object, where possible. This
   can be limited to external_acl_type-relevant fields if needed.

3. If any ALE data member is filled in #2, print a level-1 warning.
   Do not print more than a few such warnings per worker lifetime.

The above plan is meant to alert us of the bugs introduced by this
change without actually exposing admins to those bugs (except for the
warning and a small performance penalty).

Please let me know if the above sketch is not detailed enough to follow.


> +    virtual bool requiresAleXXX() const {return true;}

I do not think this really warrants an XXX because I see nothing
seriously wrong with this method. Same for hasAleXXX().


>      // Why is this a sub-class and not a set of real "private:" fields?
>      // TODO: shuffle this to the relevant ICP/HTCP protocol section
>      class Private
>      {
>  
>      public:
> -        Private() : method_str(NULL) {}
> +        Private() : method_str(NULL), lastAclName(NULL), lastAclData(NULL) {}
> +        ~Private() {
> +            safe_free(lastAclName);
> +            safe_free(lastAclData);
> +        }
>  
>          const char *method_str;
> +        const char *lastAclName; ///< string for external_acl_type %ACL format code
> +        const char *lastAclData; ///< string for external_acl_type %DATA format code
> +
>      } _private;


Why exacerbate the existing problem by adding more fields to _private
instead of just adding those fields to the "public:" section? Just give
them names specific to external_acl_type to emphasize that these fields
are not generally available (which is true for many ALE fields). You can
even create a dedicated sub-class for them, like we already do for
protocols!

Eventually, we may decide to support lastAclName for access_log (and
other contexts?) as well.



> +            safe_free(lastAclData);
...
> +            ch->al->_private.lastAclData = sb.c_str();

The combination looks wrong to me. c_str() does not allocate a new
c-string AFAICT.


Please also check the patch for out-of-scope or needless whitespace changes.

This is not a full/detailed audit, but I do not expect to be able to do
more in the foreseeable future. Consider pinging Christos if you need a
second opinion -- he knows this code well.


Thank you,

Alex.