[squid-dev] [PATCH] YesNoNone class updates

[Alex Rousskov]

On 01/14/2016 12:31 PM, Amos Jeffries wrote:
> On 15/01/2016 6:26 a.m., Alex Rousskov wrote:
>> 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:
>>> +    /** 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.


> We have a bool constructor and implicit copy-assignment. Which tempts
> the compiler to try bool constructing then copying. 

Fortunately, we do not have just any "bool constructor". We have an
_explicit_ bool constructor which prohibits implicit "bool constructing
then copying", no matter how tempting it might seem to the compiler.

What would be the purpose of marking the constructor "explicit" if the
imaginary conversion sequence you have described would still be
possible, depending on the compiler or other factors? [Rhetorical]

Please either remove the bool assignment operator (instead of deleting
what does not exist) or provide a use case where its deleted existence
changes something other than the compiler error message text.

FWIW, in my tests, without the operator, the errors look like this:

error: conversion from ‘bool’ to non-scalar type ‘YesNoNone’ requested


In this particular case, deleting what does not exist is just a minor
annoyance because the class is very simple and efficient. I am insisting
on this change because I do not want to create a precedent where folks
starting to =delete more and more stuff elsewhere, simply out of
irrational phobias, complicating an already complex code.

C++11 is supposed to make our life easier and our code safer, not force
us fear more black magic. It is not like we suddenly need to go through
all of our classes and =delete stuff because we now can or because we
now fear some new implicitly generated methods. A correctly written
C++09 class should continue to work correctly without =delete.


>>> +    bool option; ///< configured value or false
>>> +    uint8_t setWith; ///< initialized instead of configured
>>
>> Please reorder to minimize padding, especially if setWith type changes.


> Do you have a good way to test for that?

Since we are talking about reducing padding and not eliminating or
prohibiting it, I do not think there is such a way. You can test
manually, of course, but that is not a good way.

A good rule of thumb is to put boolean data members last.


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


> Sure. Done.

s/optImplicit/optImplicitly/:

 - Set how?
 - Implicitly.


>>> -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.
> 
> I am a little wary that this could lead to mysterious uncaught
> exceptions when these config values are tested by code outside the
> AsyncCall protections.


>From a CVE point of view, mysterious uncaught exceptions are no worse
than assertions. And if the exception is caught, then there are more
chances to avoid a CVE.

Forcing one of these exceptions by temporary modifying configuration
code and testing what actually happens is a good idea, but not a
prerequisite.

We do need code/instrumentation to convert admin-selected exception(s)
into assertions for debugging/triage (without recompiling Squid), but
that is a different topic and not a precondition for using exceptions in
new code IMO.


I do not think another round of reviews is needed.


HTH,

Alex.