Re: [PATCH] Support client connection annotation by helpers via clt_conn_id=ID

On 06/24/2014 08:21 AM, Amos Jeffries wrote:
> On 24/06/2014 1:44 a.m., Tsantilas Christos wrote:
>> On 06/23/2014 09:50 AM, Amos Jeffries wrote:
>>> On 23/06/2014 5:43 a.m., Tsantilas Christos wrote:
>>>
>>> in src/Notes.cc:
>>> * replaceOrAdd iterates over the notes list twice in the event of
>>> removals (due to remove() iterating over it from scratch). Please fix
>>> this to iterate over the list only once and do an in-place replace if
>>> the key is found already there, add if end() is reached.
>>>
>>> * remove() appears to be unnecessary after the above correction.
>>
>> I can fix a litle replaceOrAdd, but the remove() method still is needed.
>>
>
> By what code? I see it only being used by replaceOrAdd(), and only to
> perform the unnecessary loop. It is fine as a remove() implementation,
> but to me looks needless at this time.

Which loop is unnecessary?
You need one loop to iterate over src NotePairs and one loop to iterate
over this->entries to find and remove the existing item. The second loop
done by remove() method.
Am I missing something?

>> The replaceOrAdd() method method will be:
>> void
>> NotePairs::replaceOrAdd(const NotePairs *src)
>> {
>> for (std::vector<NotePairs::Entry *>::const_iterator i =
>> src->entries.begin(); i != src->entries.end(); ++i) {
>> remove((*i)->name.termedBuf());
>> entries.push_back(new NotePairs::Entry((*i)->name.termedBuf(),
>> (*i)->value.termedBuf()));
>> }
>> }
>>
>>>
>>>
>>> in Notes.*:
>>> * findFirst() no longer makes sense. There being no "second" now.
>>> - Please rename to find().
>>
>> This is not true. We can still support multiple values per key name.
>> I am suggesting to not touch this one.
>>
>
> Every helper callback is now using Update*() which uses replaceOrAddd()
> to ensure uniqueness.

Not exactly uniqueness, just that the new key/value pairs will replace
any existing.

>
> I see that the list output directly from the helper may contain
> duplicates and might be used by the handler instead of the HttpRequest
> merged list. We may want to change that to make handler behaviour match
> behaviour of the ACL testing the HttpRequest list.

My understanding from discussion is that in future we may want to
support multiple values per key note. Maybe using a configuration
parameter, or other mechanism.

Moreover the helper may return for the same key multiple values:
    ... key=value1 key=value2 key=value3

Just if already exist the key 'key' in notes list will be replaced by
the new key/values pairs.

We can change the above and support only one value per key. My opinion
is that there is not any strong reason for denying multiple values per
key. It may be useful feature.
But I have not strong opinion on this.

>>>
>>> in src/client_side_request.cc:
>>> * ClientRequestContext::clientRedirectDone was previously calling
>>> SyncNotes to ensure the AccessLogEntry and HttpRequest shared a notes
>>> list before updating the request. Order is important here because Sync
>>> will kill Squid with an assert if the objects have different notes list
>>> objects.
>>> Until that is fixed please retain the order of:
>>> - sync first, then update the request.
>>>
>>> * same in ClientRequestContext::clientStoreIdDone
>>
>> It is the same, it is not make difference.
>> UpdateRequestNotes will just copy helper notes to the request notes. The
>> SyncNotes will check if AccessLogEntry and HttpRequest object points to
>> the same NotePairs object.
>
> If you are going to retain it the way it is please add a note to the
> AccessLogEntry::notes member indicating that it must NEVER be set other
> than by calling SyncNotes() with the current transactions HttpRequest
> object. Doing so will guarantee crashes after this patch goes in.

The AccessLogEntry::notes must never set. This is does not have to do
with the order the UpdataRequestNotes and SyncNotes called. The squid
will crash even if we call SyncNotes before UpdateRequestNotes.

However I have not any problem to change the order. Maybe make more
sense and also I can remove the "if (!request.notes)" block from
UpdateRequestNotes.

>
> The burden will also be placed on auditors to check this with every
> notes related patch. (I would rather avoid that extra work TBH).

OK, if I will avoid the extra work that way I will call SyncNotes before
UpdateRequestNotes :-)

Regards,
   Christos

>
> Amos
>
Received on Tue Jun 24 2014 - 14:40:37 MDT