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
