On 12/29/2012 12:16 AM, Eliezer Croitoru wrote:
> Thanks for all the help until now.
>
> This PATCH is in sync with trunk which comes after the last preview.
> I tried my best until now in the small windows of time I have for the
> project with hopes for the best.
>
> This no longer a PREVIEW since most of the code was reviewed couple
> times and was tested in a small production testing environment for
> couple weeks with a great success.
> I left couple FIXME and TODO in the code.
PREVIEW == submitter will make more changes (and post a PATCH)
PATCH == everything else
Do you expect to fix the already identified problems in the latest
patch? If yes, then this is PREVIEW.
> + // HttpRequest initializes with null_string. So we must check both defined() and size()
> + if (!r->client_ident && http->request->extacl_user.defined() && http->request->extacl_user.size()) {
Calling .size() without calling .defined() ought to be sufficient. The
defined() method is essentially an API bug and should not be used unless
absolutely needed. The size() method returns zero for "undefined" Strings.
Also, you need to decide whether empty store IDs are allowed (I do not
think they should be) and check all changes for consistency. For
example, the code below assumes that empty store IDs are allowed:
> + StoreEntry *e = storeCreateEntry(http->store_id.defined() ? http->store_id.termedBuf(): http->uri, http->log_uri, reqFlags, m);
Please move redirectStateData initialization code shared between
storeIdStart() and redirectStart() into a separate function or method.
Perhaps it should become a redirectStateData constructor?
> + debugs(20, 3, "storeSwapMetaBuild URL: " << url ); /* important to debug StoreID */
> + * instead of the store_id we will see it here hence the Debug section is
> + * very important.s
Please remove these and other comments regarding the perceived
importance of debugging. Every debugging statement is or ought to be
important in some context or it should not be there at all.
BTW, in general, put the debugging of the newly set value into the
setter rather than the caller code if possible -- more callers will
benefit from the same debugging line that way, without code repetition.
> - debugs(33, 5, HERE << "'" << http->uri << "'");
> + debugs(33, 5,"'" << http->uri << "'");
Please do not change debugging lines just to remove HERE, especially
when the surrounding code does not change. If you do change, remove
single quotes around URLs.
> HttpRequest *request; /* Parsed URL ... */
...
> + /**
> + * a copy of request->store_id which needed in couple places where the request
> + * cannot be accessed or dosnt exists.
> + */
> + String store_id;
This description does not make sense to me. If request does not exist
(i.e., the request member is nil), there cannot be a copy of
request->store_id. And if it exists, then it can be accessed without
making a copy.
> + const char *uri = request->storeId(); /* using StoreID to create a storeEntry by the StoreID or canonicalUrl*/
Please do not repeat storeId() description when using storeId(). Just
calling storeId() ought to be sufficient.
> + // XXX: This function is now kept only to check for and display the garbage use-case
> + // and to map the old helper response format(s) into new format result code and key=value pairs
> + // it can be removed when the helpers are all updated to the normalized "OK/ERR kv-pairs" format
Do we need to add this function when we are adding support for a new
helper? Perhaps it is OK to assume that all Store ID helpers will use
new key=value format? Is this to support Squid2 Store ID helpers? Will
those still work with the proposed Squid3 changes?
> + ConnStateData * conn = http->getConn();
> + redirectStateData *r = NULL;
> + const char *fqdn;
> + char buf[MAX_REDIRECTOR_REQUEST_STRLEN];
> + int sz;
> + http_status status;
> + char claddr[MAX_IPSTRLEN];
> + char myaddr[MAX_IPSTRLEN];
> + assert(http);
> + assert(handler);
Please do not declare local variables until they are needed. And declare
them constant whenever possible.
> + * cannot be accessed or dosnt exists.
Please spellcheck comments.
> if (mem_obj->request)
> newkey = storeKeyPublicByRequest(mem_obj->request);
> - else
> - newkey = storeKeyPublic(mem_obj->url, mem_obj->method);
> + else{
> + newkey = storeKeyPublic(mem_obj->url, mem_obj->method);
> + }
> +
Please remove non-changes from the patch.
> + /* A new handling for all helpers Shutdown */
This comment will not make sense in the final code (there will be
nothing "old" in that code). It is also incorrect because the code does
not handle all helpers, just two of them. Please remove.
> While testing some helpers I have seen a bug when using concurrency
> which I will debug later after committing the working StoreID to trunk.
I am not sure I understand why these bugs should not be fixed before
commit. Any particular reason we should commit broken code?
> The next thing after this patch is adding ICAP the option to respond
> with some StoreID for a request.
> How do I even start on that?
> and a case I had in mind is what to do when Store-ID is used in ICAP? do
> we allow second helper?
There is already an ACL that controls whether the Store ID helper is
used. We should not need another piece of code to control that decision
IMO. If we do not have enough ACLs to detect ICAP/eCAP decision in this
context, we should add more ACLs.
> if so what and how do we send to the helper since it comes after the
> ICAP REQMOD service ?
The helper, if allowed, will overwrite the ICAP-derived Store ID
setting. You will want to make sure that current Store ID can be sent to
the helper as a key=value pair, along with the original URL.
However, let's finish this project before switching to ICAP/eCAP.
HTH,
Alex.
Received on Wed Jan 02 2013 - 16:14:00 MST
This archive was generated by hypermail 2.2.0 : Thu Jan 03 2013 - 12:00:06 MST