Re: [PATCH] stage 2 of store url as fake store_url helper

On 2/12/2012 8:27 a.m., Alex Rousskov wrote:
> On 12/01/2012 06:34 AM, Eliezer Croitoru wrote:
>> - For now I am not changing the mem-obj->url and leave the branch as it
>> is since there is no need for it now.
>> Once the feature will be fine The change will be placed.
> That is fine, but please note that the feature cannot be committed until
> that renaming change is implemented so that you can verify that all
> other uses of mem_obj::url are still correct even though that member is
> no longer a URL but a store entry ID.
>
>
>> + const char *store_url = urlStoreUrl(request);
>> SquidMD5_CTX M;
>> SquidMD5Init(&M);
>> SquidMD5Update(&M, &m, sizeof(m));
>> - SquidMD5Update(&M, (unsigned char *) url, strlen(url));
>> + if (store_url)
>> + SquidMD5Update(&M, (unsigned char *) store_url, strlen(store_url));
>> + else
>> + SquidMD5Update(&M, (unsigned char *) url, strlen(url));
>
>> +const char *
>> +urlStoreUrl(HttpRequest * request)
>> +{
>> + if (request->store_url)
>> + return xstrdup(request->store_url);
>> + else
>> + return NULL;
>> +}
> The combination leaks urlStoreUrl() result as far as I can tell. Besides
> that bug, there are several design issues with this:
>
> * Please do not add urlStoreUrl(request). Add a constant HttpRequest
> method instead.
>
> * Please do not dupe the url inside urlStoreUrl().
>
> * Please always return a URL for Store from urlStoreUrl(). There is
> always such a url, regardless of configuration (it is either the
> requested URL or the rewritten one).

>> +static helper *storeurls = NULL;
> Please use camelCase for variable names unless underline_based_names are
> needed for consistency reasons.
>
> Please document all new variables, functions, and methods, including
> this one.
>
>
>
>> + if (http->request->store_url){
>> + assert(http->request->store_url);
> The assert is not needed.
>
>
>> - StoreEntry *e = storeCreateEntry(http->uri, http->log_uri, reqFlags, m);
>> + StoreEntry *e;
>> + if (http->request->store_url){
>> + assert(http->request->store_url);
>> + e = storeCreateEntry(http->request->store_url, http->log_uri, reqFlags, m);
>> + }
>> + else{
>> + e = storeCreateEntry(http->uri, http->log_uri, reqFlags, m);
>> + }
> To avoid code duplication, compute the URL (the first parameter) and
> then call storeCreateEntry with the right first parameter.
>
> BTW, to compute the first parameter, you may be able to just call
> HttpRequest::storeUrl() method discussed above (if it always returns a
> URL for Store).
>
>
>> + int storeurl_state;
>> + uint8_t storeurl_fail_count;
> The use of underline_based_names is correct here, but you need to add an
> underline between the store and url words as well.
>
>> + // reset state flag to try redirector again from scratch.
>> + storeurl_rewrite_done = false;
>> + case HelperReply::Okay: {
>> + Note::Pointer urlNote = reply.notes.find("rewrite-url");
> Can we unify these parts of redirector and store URL rewriting code to
> avoid code duplication and bugs that it brings, like the ones above? Is
> ClientRequestContext::clientStoreUrlDone() that much different from
> ClientRequestContext::clientRedirectDone() that you cannot merge their
> common parts?

They are. The whole of the OK result handling is completely different,
as well as the context handling conditions after the switch case. Also
it is important that the context members altered for BH case error
handling are different to prevent a BH from one helper affecting the
other helper.

The old->new format conversion is moved to redirect.cc and shared
already via my helper upgrade stage 3 patch.

>
>> + // XXX: put the the storeURL value somewhere.
>> + http->request->store_url= xstrdup(urlNote->firstValue());
> What does this XXX mean? Does not this code put the storeURL value
> somewhere already?

Was a note from me to Elizeir which should have been removed.

> You may want to either assert that http->request->store_url is not NULL
> when this code runs or free the old value of the store_url. This should
> probably be done by an HttpRequest setter method though.
>
>> + Note::Pointer urlNote = reply.notes.find("rewrite-url");
> Make urlNote const if you can.
>
>
> Please search your patch for tabulation characters in sources and
> replace the ones you added with spaces.
>
>
> Finally, I suggest using "store ID" instead of "store URL" in all
> internal names and code comments. It is a good idea to remind us that
> there is nothing "URL" about this opaque string.
>
> I would even argue that squid.conf directive should be called
> differently to emphasize that this is a URL:ID mapping feature rather
> than URL rewriting feature (I do not think we need to maintain backward
> compatibility with Squid2 unfortunate naming here).

Correct we do not.

Amos
Received on Sun Dec 02 2012 - 06:21:12 MST