Re: [PATCH] note acl

On 05/21/2013 09:14 AM, Amos Jeffries wrote:
> On 17/05/2013 4:41 a.m., Tsantilas Christos wrote:
>> Hi all,
>
> Sorry for the slow reply..
>
>> I am attaching two patches here:
>>
>> 1) Merge AccessLogEntry 'helperNotes' and 'configNotes' members to
>> 'notes' member (trunk-merge-cfg-helper-notes-v2.patch)
>>
>> There is not any need to store notes added using Note cfg option and
>> notes added from helper to separated member. This patch merge them to
>> the same AccessLogEntry::note member.
>
> +1. Looks fine as-is.

I will apply this patch to trunk if there are no objections.

>
> PS. When you apply this please prefix the commit subject with "Cleanup: "

OK.

I am attaching a new version for the note ACL patch.

>
>> 2) note acl ( trunk-note-ACL-v5.patch )
>> Syntax:
>> acl aclname note name [value ...]
>>
>> Without values, matches any annotation with a given name. With value(s),
>> matches any annotation with a given name that also has one of the given
>> values. Annotation sources include note and adaptation_meta directives
>> as well as helper and eCAP responses.
>
> audit results for the second patch:
>
> * please do not cut-n-paste the (partially wrong) copyright from other
> files into new ones.
> NOTE: a default Squid copyright blurb is planned to be automatically
> pre-pended somehow. So for now only add one if you want one different
> from the main Squid COPYRIGHT file. To mark the copyright as yours for
> this file under the default, add your name in an AUTHOR: line instead
> for now.

I removed any copyright comment from these files.

>
> * please also add a "note_regex" ACL which takes regex patterns as the
> value (and/or key) details.
> NP: that will allow removing "Names and values are compared using a
> string equality test." statement in cf.data.pre

I prefer to avoid it for now. We spent a lot of time for this project,
so even the few hours required to add this feature looks long time.
It is easy for someone to add it if needed.

>
> in: src/acl/NoteData.h:
> * please be consistent about doxygen comments:
> + "///" for one-liners before the member definition
> + "///<" for one-liners after the member definition.
> + "/**" for multi-liners before the member definition
> + _no_ multi-liners after the member definition.

OK. I hope it is OK now.

>
> * you don't need to document each #include.
> We are only using that to document why the files like
> globals.h/enums.h/defines.h/structs.h which are scheduled for removal
> are still being included.
OK, this comments removed.

Regards,
   Christos

>
> Amos
>