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

On 12/2/2012 8:20 AM, Amos Jeffries wrote:
> On 2/12/2012 8:27 a.m., Alex Rousskov wrote:
>> On 12/01/2012 06:34 AM, Eliezer Croitoru wrote:
<SNIP>
>>> +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.
>
Oops :\

>> 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.
OK, So If there is no objection from anyone else in the next few days I
will change the whole naming convention to StoreID.

Eliezer
Received on Sun Dec 02 2012 - 06:28:00 MST