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

[Amos Jeffries]

On 26/08/2016 5:05 a.m., Alex Rousskov wrote:
> On 08/25/2016 08:18 AM, Eduard Bagdasaryan wrote:
>> 2016-08-24 18:20 GMT+03:00 Amos Jeffries <squid3 at treenet.co.nz>:
>>
>>> in src/LogTags.cc:
>>> * instead of adding new enum entry please extend LogTags with a new bool
>>> flag and the c_str() to append the "IGNORED" when that flag is true.
> 
>> Added a new LogTags::ignored for this.
> 
> The naming of things in logging code is exceptionally poor so please do
> not feel upset that you got this wrong. The "_IGNORED" flag Amos is
> talking about should go into LogTags::Errors.

Yes. Sorry for not being more clear on that.

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

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

> 
>> I wonder whether the
>> LogTags assignment operator should copy this member too.
> 
> Which assignment operator?
> 
> * The compiler-generated assignment operator will copy all members
> automatically, of course.
> 
> * 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. Adding
> an "XXX: this operator does not reset flags" and "TODO: either replace
> with a category-only setter or remove" comments would be appropriate.

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

Amos