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

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.

Please don't add multiple empty lines in a row. This is happening in
several places.

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()

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

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

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 << "{...

* in redirectInit() you are using new on the redirectors variable when
it should be the storeurl_rewriters variable.

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.

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

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

Amos
Received on Sun Sep 09 2012 - 11:54:31 MDT