[squid-dev] [PATCH] YesNoNone class updates

[Alex Rousskov]

On 01/13/2016 03:56 PM, Amos Jeffries wrote:
> On 13/01/2016 5:11 a.m., Alex Rousskov wrote:
>> On 01/11/2016 06:36 PM, Amos Jeffries wrote:
>>> Distinction is made between implicit and explicit configuration.
>>
>> Why do we need this increased complexity?

> This was written in context of using it to complete the final audit
> request for the bug 4005 patch instead of multiple bools.

Noted, thank you.


>>> +bool
>>> +YesNoNone::operator !() const
>>> +{
>>> +    assert(option != YesNoNone::optUnspecified);
>>> +    return option == YesNoNone::optDisabled;
>>>  }
>>
>> Please never reverse the existing operator bool code. Inline the
>> negation of that operator call instead.

[ excuses snipped without review ]

If you cannot simply negate the existing "bool" conversion operator, for
any reason, then you must not add the "bool" operator in the first place!


After further consideration, I think I can make an even stronger and
simpler rule for the logical not operator in new code:

There is one and only one reason to add a logical not operator to a
C++11 class: Optimization of a boolean conversion operator (for rare
cases where "!foo" is much faster to compute than "foo").

Please correct me if I missed any other cases where an explicit logical
not operator should be added to a C++11 class.


> +    /** forbidden operation.
> +     * Use x.configure(bool) when the value is configured.
> +     * Use x = YesNoNone(bool) to assign defaults.
> +     */
> +    YesNoNone &operator =(bool) = delete;

Why do we need to delete something that does not exist? If you do not
add the above code, do you really gain the ability to assign booleans to
YesNoNone objects? If not, please remove.


> +    uint8_t setWith; ///< initialized instead of configured

Stale comment. Consider using "how the value was set" description instead.

Stale type. Should be SetWith AFAICT.


> +    enum SetWith : uint8_t { optUnspecified = 0, optImplicit = 1, optConfigured = 2 };


Why do we need to specify the exact enum storage size in this case? If
there is a good reason, please add a source code comment to explain. If
there is not, remove storage type specification.

BTW, if you need to minimize storage overhead at the expense of
value-testing performance, you can encode all the information (both
setWith and option members) in 8 bits, of course.

Please consider renaming SetWith to something that matches the enum
values better: "set with configured" does not work well. I suggest
"SetHow" and optUnspecified, optImplicitly, and optConfigured values.


> +    bool option; ///< configured value or false

Or an implicitly set value. I would rephrase the description to
something like:

bool option; /// specified yes/no value; meaningless if optUnspecified


> +    bool option; ///< configured value or false
> +    uint8_t setWith; ///< initialized instead of configured

Please reorder to minimize padding, especially if setWith type changes.



> +    Config.memShared = YesNoNone(false);
> -        writeableCfg().connectionEncryption.configure(true);
> +        writeableCfg().connectionEncryption = YesNoNone(true);

The new code that configures "implicit" values looks unnecessary
awkward/complex. I suggest adding a method to change the value (in
addition to the constructor):

    Config.memShared.defaultTo(false);
    writeableCfg().connectionEncryption.defaultTo(true);

As an added benefit, the method can throw if optConfigured values are
reset using defaultTo() calls!


> -YesNoNone::operator void*() const
> -{
> -    assert(option != 0); // must call configure() first
> -    return option > 0 ? (void*)this : NULL;
> -}

> +    explicit operator bool() const {
> +        assert(setWith != optUnspecified);
> +        return option;
> +    }

To reduce chances for getting another CVE for a reconfiguration crash, I
suggest that new configuration-related code does not assert() but throws
(e.g., via Must()). Whether that implies moving the above code back to
.cc file is your call.


> If there are no objections I will apply this patch and the bug 4005 fix
> that depends on it tomorrow.

I object to the current YesNoNone patch, but I hope that the above
comments give you enough info to polish it without another review round.

FWIW, I have not seen a bug 4005 patch that depends on this. The old mk2
does not seem to use the changes introduced here. On the other hand, I
cannot promise to review the updated patch so others should decide
whether they have to see it before it goes in.


Thank you,

Alex.