Re: [PATCH]proposal for a fake_strore_url helper wrapping. +fixes

On 9/9/2012 2:54 PM, Amos Jeffries wrote:
> On 9/09/2012 10:08 p.m., Eliezer Croitoru wrote:
>> I took the url_rewrite helper and used what ever seems needed to
>> create a url_store_rewrite helper.
>> I also added the needed variables for the next step and freeing them
>> where and if needed.
>> this is a more of a basic sketch.
>>
>> Eliezer
>>
>
> You dont need all the "added for storeurl", "changed X" comments, they
> just bloat the code and patch.
OK,these was more for me.
Recommendations on how to manage myself in such huge code are more then
welcome.(using eclipse)
>
> Please don't add multiple empty lines in a row. This is happening in
> several places.
will be fixed.

>
> src/client_side_request.cc:
>
> * in ClientRequestContext::clientStoreurlStart the else condition is
> what gets done if there is no access list configured. This should be
> defaulting to *not* changing the store-URL. call
> clientStoreurlDone(NULL) instead of storeurlStart()
Done.

>
> * in doCallouts() please move the store-url bits to after adaptation and
> URL-redirect bits. Those checks may result in a non-storable response
> being generated by Squid straight to the client.
I wasn't sure about it, Thanks.

> ++ It is questionable whether the store-URL should also be below the
> no_cache_done bits as well. Since requests with"cache deny" are not
> going to be loaded from cache, but maybe they will be stored later by
> future Squid (which will need store-URL changes then) - in current squid
> they are useless passing to the storeurl helper.
When we will be there then...
Are there any visible plans?

>
> src/client_side_request.h:
> * please name the new variable "store_uri" in line with the other
> surrounding variable naming style.
Done

>
> src/redirect.cc:
> * why is storeurlHandleReply() being added? all it does is receive the
> reply and call a callback handler. It is that handler which is the only
> part store-url specific and set by storeurlStart. If it was just the
> debugs, they should be changed to debugs(61, 5, HERE << "{...
Thanks. applied
>
> * in redirectInit() you are using new on the redirectors variable when
> it should be the storeurl_rewriters variable.
oops. ~0-0~
Fixed

>
> To answer your query comment... CBDATA_INIT_TYPE does memory allocation
> and new/delete method definition setup for the redirectStateData
> objects. Since they are shared by both systems you can ignore that part
> of redirectInit().

> Although the static variable should be moved down
> next to the if(init) and changed to a bool type - that is irrelevant to
> the storeurl stuff though.
Well i'm already here so..

> * in storeUrlStart() the INTERNAL_SERVER_ERROR log message is still
> saying "redirector"
fixed

>
> Fix those and I think this is a good part-1 patch that can be applied to
> trunk.
>
> Amos

-- 
Eliezer Croitoru
https://www1.ngtech.co.il
IT consulting for Nonprofit organizations
eliezer <at> ngtech.co.il