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

On 12/1/2012 9:27 PM, 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.
I understand it but it's not true.
I did verification before writing the code.
I'm not the bad-ass reviewer but I am sure about most of the
mem_obj::url usage.

If you can take a look at the patch again and see some place that the
store_url can affect other then debug sections I will be happy to hear
about it so we can thing again.

One of my problems is that it will be hard for me to test changes if I
wont have a solid ground in the branch.

If you have a good way to help me test it while not having the need to
make huge changes before a diff\patch I will be happy to learn.

>
>
>> + 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:
Changed.

>
> * Please do not add urlStoreUrl(request). Add a constant HttpRequest
> method instead.
Done.

>
> * Please do not dupe the url inside urlStoreUrl().
OK.

> * 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).
Ok, got it.

>
>> +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.
>
Done.

>
>> + if (http->request->store_url){
>> + assert(http->request->store_url);
>
> The assert is not needed.
>
OK.

>
>> - 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.
Ok. Hope it's what you meant

>
> 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).
Well the question here is how the http->uri is computed:
I am not sure that always the uri and canonical uri that I am using on
the request are the same.
I will check it but if someone knows I will wait for the word.

>
>> + 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?

Amos gave me some notes about it last time and I hope it's fine now.
Anyway this is out of the scope of my knowledge\capabilities right now.
Maybe Amos?

>
>> + // 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?
A note Amos left for me in the surrounding code(he did me a favor).
Fixed.

>
> 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.
It should be the HttpRequest thing since I need this value to be
available later as long the request exists.
Done.

>
>> + 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.
>
Will do.
>
> 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).
It's fine by me but I am almost sure that backwards compatibility is wanted.
There was a change about the redirector and url_rewrite in the past if
I'm right.
This kind of a change will remind the admins that the feature works
differently from the old store_url_rewrite.

Thanks,
Eliezer