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

On 4/01/2013 7:10 a.m., Eliezer Croitoru wrote:
> Thanks Alex,
>
> On 1/3/2013 6:23 PM, Alex Rousskov wrote:
>> One of the reasons is that sooner or later the current or updated code
>> will crash while "accessing the request in these situations". Just
>> because you have carefully identified when the request should or should
>> not be used now, does not mean that your identification will remain
>> valid a year from now or that nobody will copy your code to a different
>> location where it will break next week.
>
> Well I can't be responsible for the wind in Taiwan while I'm here.
> Sorry.(there is more to come..)
> If there will be a change with such a huge impact a basic runtime test
> should find that so allow me to doubt a bit.
>
>>
>> This has nothing to do with object orientedness. This is about giving a
>> correct way of accessing information that may be present in two
>> different places, one of which may be invalid. If testing request for
>> being nil is sufficient, this is a simple change. If it is not
>> sufficient because request may be non-nil but invalid, then it is
>> probably too dangerous to commit this code and more work is needed to
>> make it reliable (which may include changing other code, of course).
>
> OK got you now.
> I need to rephrase something:
> There are places in Squid code (not related to my code) which the code
> scope allows access to an invalid request.(it's like saying a kid poops)
> Leave these aside and while researching the code my main effort was to
> back-trace couple functions from runtime back to the request creation.
> This research provided me with enough knowledge about the code which
> makes this patch code Stable in many aspects.
> One of these aspects is not accessing(r\w) what and when not needed at
> all.
>
> I choose by hand with a needle the specific points of code which sets
> and gets data in the main components which affect the identifiers of
> the requests while considering the scopes of the code carefully.

Sure, but you coded in that patch a whole bunch of fetch rules which all
said: if a request was present, use its storeId, if none was present
use the alternative variable storeId.

All I was asking for was to take that piece of logic decision which was
repeated in a handful of places (with slightly different boolean tests)
and make a inline accessor function which anybody can call without
needing to reasearch teh diferentce between the HttpReqeust:storeId and
the alternative one, or to re-code the test logic themselves. We should
be able to just call storeId() accessor from the state object and get
whatever the current storeId is. No complex conditionals.

Amos
Received on Fri Jan 11 2013 - 03:43:30 MST