Re: [PATCH] external_acl_type: add %EXT_LOG

On 23/03/11 16:03, James Bowe wrote:
> On Wed, Mar 23, 2011 at 9:43 AM, Amos Jeffries wrote:
>> On Wed, 23 Mar 2011 09:30:26 +1100, James Bowe wrote:
>>>
>>> On Wed, Mar 23, 2011 at 9:12 AM, Amos Jeffrieswrote:
>>>>
>>>> On Wed, 23 Mar 2011 00:38:08 +1100, James Bowe wrote:
>>>>>
>>>>> Attached is a patch for adding %EXT_LOG as a format specifier to
>>>>> external_acl_type statements.
>>>>> This is filled with the extacl_log field (log message set from
>>>>> previous external acls).
>>>>>
>>>>> The patch only adds EXT_LOG, and not EXT_MESSAGE, since EXT_MESSAGE is
>>>>> likely a longer user-oriented string.
>>>>>
>>>>> Cheers,
>>>>> James Bowe
>>>>
>>>> +1. Looks fine.
>>>> Can you explain the need for this though please? we already re-pass the
>>>> tag= from one ACL test to the next. This log= is defined as being a
>>>> message
>>>> for the log file.
>>>>
>>>> Amos
>>>>
>>>>
>>>
>>> We use the message for the log file to tie a request with our own
>>> record keeping and data structures. Creating these structures can be
>>> an expensive process, and the data is needed in multiple external acls
>>> (as well as when processing access log lines).
>>>
>>> I suppose a similar thing could be achieved if the unique sequence
>>> number was available for external acls, but EXT_LOG is more flexible
>>> as we have full control over that value.
>>>
>>> Cheers
>>> James Bowe
>>
>>
>> Thanks.
>>
>> Have you tried using tag= for this scenario?
>> once set tag persists, whereas log= is replaced by each external ACL test
>> (helpers which do not return one set it to empty).
>>
>> Amos
>>
>>
>
> Yes tag= could also work for this purpose, although I don't see a
> format specifier for tag in the external_acl_type docs either.
>
> Although looking at the code in external_acl.cc:
> if (entry->log.size())
> ch->request->extacl_log = entry->log;
> shows that the log= is only replaced if the external_acl returns a log message.
>

Doh. right.

> So
> -for a string that never changes after it is set, tag= is suitable.
> -for a string that may need updating or overwriting by a later
> external_acl, log= is suitable.
>
> Under both circumstances it is conceivable that later external_acls
> may need access to the tag= or log= values after they have been set
> (e.g. for external_acl debugging, merging log messages, etc). I have
> updated the patch to add %TAG format specifier too.

Thanks.

Now that I've woken up, I notice the dump_externalAclHelper() function
in external_acl.cc also needs a display entry for these new tags.

Amos

-- 
Please be using
   Current Stable Squid 2.7.STABLE9 or 3.1.11
   Beta testers wanted for 3.2.0.5