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

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?

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

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

Thank you,

Alex.
Received on Sat Dec 01 2012 - 19:27:49 MST