Re: [PATCH] StoreID latest implementation in sync with rev 12552. stage 2-3 from 3.

On 01/02/2013 03:38 PM, Eliezer Croitoru wrote:
> Thanks Alex,
>
> On 1/2/2013 6:13 PM, Alex Rousskov wrote:
>> On 12/29/2012 12:16 AM, Eliezer Croitoru wrote:
>>> Thanks for all the help until now.
>>>
>>> This PATCH is in sync with trunk which comes after the last preview.
>>> I tried my best until now in the small windows of time I have for the
>>> project with hopes for the best.
>>>
>>> This no longer a PREVIEW since most of the code was reviewed couple
>>> times and was tested in a small production testing environment for
>>> couple weeks with a great success.
>>> I left couple FIXME and TODO in the code.
>>
>> PREVIEW == submitter will make more changes (and post a PATCH)
>> PATCH == everything else
>>
>> Do you expect to fix the already identified problems in the latest
>> patch? If yes, then this is PREVIEW.

> I fixed the problem in the latest patch...(maybe I missed a thing like
> anyone else can)

You said there are a "couple FIXME and TODO in the code"

>>
>>> + // HttpRequest initializes with null_string. So we must check
>>> both defined() and size()
>>> + if (!r->client_ident && http->request->extacl_user.defined() &&
>>> http->request->extacl_user.size()) {
>>
>> Calling .size() without calling .defined() ought to be sufficient. The
>> defined() method is essentially an API bug and should not be used unless
>> absolutely needed. The size() method returns zero for "undefined"
>> Strings.
>>
>> Also, you need to decide whether empty store IDs are allowed (I do not
>> think they should be) and check all changes for consistency. For
>> example, the code below assumes that empty store IDs are allowed:
>>
>>> + StoreEntry *e = storeCreateEntry(http->store_id.defined() ?
>>> http->store_id.termedBuf(): http->uri, http->log_uri, reqFlags, m);
>>
> An empty return is not allowed and this is one of the bugs in the
> related helper code which dosn't relate in any way to the StoreID
> code.(also answers your later question about committing broken code)
> It causes an unfulfilled assertion for all the cases I have checked and
> I wont have time to debug this specific problem now which is not related
> in anyway to the actual StoreID function but to the helper interface.
>
> Since we do not care for the size of the string else then it's more then
> what 0 size then I think size will be ok.
>
> Any size enforcement should be done in the helper surrounding code
> before any other code which suppose to know if one exists or not.

If your code works correctly when using .size() instead of .defined(),
it should be using .size() instead of .defined(). There is no good
reason to use both .size() and .define() inconsistently, regardless of
what some other code may or may not be doing (and .defined() should be
avoided whenever possible for reasons discussed earlier).

>> Please move redirectStateData initialization code shared between
>> storeIdStart() and redirectStart() into a separate function or method.
>> Perhaps it should become a redirectStateData constructor?
>>
> Well you are right about it but I first wrote a production working code.
> I will need more time for that.
> I understand it duplicate code and should be eventually be done.
> I know we are against code duplication but if I will do it later after
> committing the working code into squid, will it be bad?

We already have a lot of code that folks promised to polish later but
then disappeared. Thus, I certainly prefer that you polish the code you
want to commit, especially when the changes are relatively small.
However, you may, of course, refuse to do so. In that case, your patch
may still be committed if others polish it for you or if others decide
it is worth committing your code with this known flaw.

In general, while we are all for the "working code", many of us also
spend days, weeks, and sometimes months on fixing or working around what
used to be "working code". IMO, "working" is necessary but not always
sufficient condition. There are certainly exceptions where committing
poor quality working code is the right choice, but we should do our best
to minimize the number of such exceptions.

In this particular case, the issue is minor. If not addressed, it is
likely to cause a minor bug or two later (somebody will update one copy
of the code but not the other). We will probably spend an extra day or
two triaging and fixing those bugs, but it is not a big deal. I am just
trying to explain how things should be done in general using this minor
issue as an imperfect context/example...

>>
>>> + ConnStateData * conn = http->getConn();
>>> + redirectStateData *r = NULL;
>>> + const char *fqdn;
>>> + char buf[MAX_REDIRECTOR_REQUEST_STRLEN];
>>> + int sz;
>>> + http_status status;
>>> + char claddr[MAX_IPSTRLEN];
>>> + char myaddr[MAX_IPSTRLEN];
>>> + assert(http);
>>> + assert(handler);
>>
>> Please do not declare local variables until they are needed. And declare
>> them constant whenever possible.
> It's a copy from redirect helper code so.. I dont know why exactly it's
> in this exact shape.

The code you are copying is in poor shape. The reasons are not really
important, but it probably used to be C code years ago and nobody
polished it since then. After your changes, the amount of such code will
increase. On the other hand, if you unify initialization code as I
suggested above, most of those problems will be removed in both places.

> Once this feature will get into squid I think that many admins will just
> replace 2.7 with 3.3 which will be a good benefit and bless to the
> branch (and of-course to the WWW).

Sure, but even more good things (or fewer bad ones) will happen if we
commit higher quality code. It is my responsibility to point out the
flaws in (and suggest improvements for) the patch you posted for review.
It is your decision when to stop improving the feature.

HTH,

Alex.
Received on Thu Jan 03 2013 - 16:13:23 MST