On 06/23/2014 09:50 AM, Amos Jeffries wrote:
On 23/06/2014 5:43 a.m., Tsantilas Christos wrote:
Hi all,
The attached patch replaces existin annotation values with the new one
received from helpers.
Just one question. We are documenting key-value pairs in cf.data.pre
only for url-rewriter helpers, but annotations works for all squid helpers.
Should we move the related url-rewriter section to a global section? If
yes where?
For example something like the following in a global section should be
enough:
The interface for all helpers has been extended to support arbitrary
lists of key=value pairs, with the syntax key=value . Some keys have
special meaning to Squid, as documented here.
Currently Squid understands the following optional key=value pairs
received from URL rewriters:
clt_conn_id=ID
Associates the received ID with the client TCP connection.
The clt_conn_id=ID pair is treated as a regular annotation but
it persists across all transactions on the client connection
rather than disappearing after the current request. A helper
may update the client connection ID value during subsequent
requests by returning a new ID value. To send the connection
ID to the URL rewriter, use url_rewrite_extras:
url_rewrite_extras clt_conn_id=%{clt_conn_id}note ...
Design issue:
* can we call this "client-tag=" or "connection-tag=" or
"connection-label=". We have ID used internally via InstanceId<>
template members, which are very different to this "ID" being user-assigned.
The clt_conn_id is not a client tag, but it can be a connection-tag or
connection-label.
Lets wait Alex to answer on this.
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.
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.
in src/cf.data.pre:
* "kv-pairs" is a format syntax definition used throughout the
documentation. Please retain it. If the term needs re-defining that is
an issue outside this patch scope.
* Please reword "Squid understands the following optional key=value
pairs received from URL rewriters:". Since there are optional kv-pairs
already documented in the rewrite vs redirect vs no-change syntax we
need to specify that these are optional extensions.
Perhapse this:
"In addition to the above kv-pairs Squid also understands the following
optional kv-pairs received from URL rewriters:"
* Please update the kv-pair description like so (where FOO is the term
tag or label selected to replace ID by the above mentioned design issue)
"
Associates a FOO with the client TCP connection.
The FOO is treated as a regular annotation but persists
across future requests on the client connection
rather than just the current request.
A helper may update the FOO during subsequent
requests by returning a new kv-pair.
"
* Reference to url_rewrite_extras should be directy after the helper
input syntax line. Like so:
"
[channel-ID <SP>] URL [<SP> extras]<NL>
see url_rewrite_extras on how to send "extras" with
optional values to the helper.
"
Ok for these changes
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.
* in ClientHttpRequest::doCallouts() please use SBuf::isEmpty() instead
of length() to check for existence.
OK.
Lets wait others comments before I post a new patch.
Amos
