[libvirt] [PATCH] conf: check rombar against VIR_DOMAIN_TRISTATE_SWITCH_ABSENT

[Michal Privoznik]

On 08/08/2017 10:49 AM, Pavel Hrdina wrote:
> On Mon, Aug 07, 2017 at 05:50:51PM +0200, Michal Privoznik wrote:
>> On 08/07/2017 05:30 PM, Pavel Hrdina wrote:
>>> On Mon, Aug 07, 2017 at 05:06:49PM +0200, Michal Privoznik wrote:
>>>> On 08/07/2017 04:56 PM, Ján Tomko wrote:
>>>>> Make the comparison explicit.
>>>>> ---
>>>>>  src/conf/domain_conf.c | 4 ++--
>>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>>>>> index 3cdb5e348..b5ce2ecd9 100644
>>>>> --- a/src/conf/domain_conf.c
>>>>> +++ b/src/conf/domain_conf.c
>>>>> @@ -5359,10 +5359,10 @@ virDomainDeviceInfoFormat(virBufferPtr buf,
>>>>>      }
>>>>>  
>>>>>      if ((flags & VIR_DOMAIN_DEF_FORMAT_ALLOW_ROM) &&
>>>>> -        (info->rombar || info->romfile)) {
>>>>> +        (info->rombar != VIR_TRISTATE_SWITCH_ABSENT || info->romfile)) {
>>>>>  
>>>>>          virBufferAddLit(buf, "<rom");
>>>>> -        if (info->rombar) {
>>>>> +        if (info->rombar != VIR_TRISTATE_SWITCH_ABSENT) {
>>>>>              const char *rombar = virTristateSwitchTypeToString(info->rombar);
>>>>>  
>>>>>              if (rombar)
>>>>>
>>>>
>>>> I'm not against this patch, it's just that we set ABSENT explicitly to
>>>> zero value so that we can do shortcuts like this. If we don't want to
>>>> have them, we ought to remove the explicit value assignment.
>>>
>>> The shortcut is nice, but I don't like it personally.  If the variable
>>> can contain more than two states I'd rather check it explicitly. 
>>
>> So what's the point of assigning _ABSENT zero value then?
>>
>>> That's
>>> why I prefer (int == 0) over (!int).
>>
>> Well, if this is a part of bigger statement then yes, for instance:
>>
>> if (x == 0) {
>> } else if (x == 1) {
>> } else {
>> }
>>
>> (although, sometimes we might prefer switch() for that). But if it's
>> just a simple check whether a value was set or is equal to some default
>> (= if the check is interested in distinguishing just two states anyway),
>> !var works for me too:
>>
>> if (x)
>>    formatToXML(x)
> 
> IMHO we should use the same form for all checks.
> 
>>
>> But sure, var == 0 vs !var is a personal preference. The important part
>> is my first question. If we dislike these shortcuts (in either of their
>> form), shouldn't we just drop explicit value assignment in the enum?
> 
> The point is that the value 0 is still named as _ABSENT.  The benefit
> of not using the shortcut for the VIR_TRISTATE_*_ABSENT is that you know
> right away what the variable stores and also you now right away that
> this part of code is executed only if the tristate was set to something.

This is argument for being explicit about checks (which I'm not
objecting to, I said at the beginning that I'm up for it). The thing I'm
interested in is: if we are explicit in all the places, what's the point
in having explicit assignment _ABSENT = 0? We have some enums without
explicit value assignment to any member. The only reason I can think of
is that when we alloc new struct, the memory chunk is prefilled with
zeroes so we don't have to do explicit setting to _ABSENT.

Michal