[squid-dev] [PATCH] Older response must not update

[Alex Rousskov]

On 08/27/2016 08:23 AM, Amos Jeffries wrote:
> On 26/08/2016 5:05 a.m., Alex Rousskov wrote:
>> I recommend renaming and re-documenting that subclass:
>>
>> /// Things that may happen to a transaction while it is being
>> /// processed according to its LOG_* category. Logged as _SUFFIX(es).
>> /// Unlike LOG_* categories, these flags may not be mutually exclusive.
>> class Flags
>> {
>> ...
>>    bool ignored; ///< _IGNORED: the response was not used for anything
>>    bool timedout; ///< _TIMEDOUT: terminated due to a lifetime or I/O
>> timeout
>>    bool aborted; ///< _ABORTED: other abnormal termination (e.g., I/O error)
>> } flags;
>>
>> This does not fix all the problems, but reduces the confusion
>> considerably IMHO.


> Well, yes and no. 'Flags' is too generic. The sub-struct is called
> 'Errs' because it stores the data for the fail 'errors' that occured
> during processing. 

I am not sure we want to restrict flags to errors, and it is often
difficult to say whether something is an error. For example, a response
with a smaller Date value is not really an error from the
sender/protocol point of view but can be seen as an error by the end
user that wasted all that time waiting for it. Squid does not need to
take sides. It just needs to tell the admin what has happened.


> We will eventually have several groups of flags in
> similar sub-struct/class - grouped as per the wiki meaning groupings and
> documented along the lines of what the wiki says already.
> http://wiki.squid-cache.org/SquidFaq/SquidLogs#Squid_result_codes

FWIW, I doubt that wiki page is ready to represent consensus regarding
future implementation -- I see a few problems with the way tags are
classified right now, and it is not even clear whether most of the
groups contain mutually exclusive values (like enums) or flags (like
bitmaps or a bunch of boolean fields). If all but one group contains
enums, then having that special Flags "bitmap" group would make a lot of
sense.


> I'm fine with a name change if we can preserve the fail/error/abnormal
> event meaning in it.

If you insist, the committer can rename Flags back to Errors. I doubt
Errors is better or that it is the right direction, but I am not going
to argue about it.


>> * The explicitly-defined LogTags_ot conversion assignment operator is
>> already buggy or dangerous. It should be removed. Whether it was meant
>> to clear the flags, I do not know, unfortunately. I recommend leaving it
>> as is because fixing it is out of scope and may not be trivial.

> It is a setter for (only) the old "mutually exclusive" LOG_* category
> detail - it still seems like a realatively (but not perfect) way to set
> the category. The other flags are not erased by category having changed
> since the event that caused them to be set has still previously occured
> in the transaction. The only reason we would have for clearing those
> would be a retry/reforward which is not something the LogTags itself is
> aware of. The caller changing the category is expected to be aware of
> the old category value and preserve anything that needs preserving
> (which is part of what is wrong with the old categories way).

Thank you for explaining this. Based on the above, the TODO comment in
the patch can be made more specific to just say "replace with a
category-only setter". A comitter can do that.

The new XXX comment remains correct as is: Using an assignment operator
to _partially_ reset an object is just plain wrong because it violates
the expected assignment semantics.


In summary, if you are going to commit the patch, it is your call
whether to rename Flags. If you want me to commit, please let me know
what name to use for that class. And the new TODO comment should be
adjusted to say "replace with a category-only setter".


Thank you,

Alex.