[libvirt] [PATCH 3/7] conf: Reintroduce virDomainDef::hpt_resizing

[Michal Privoznik]

On 06/26/2018 10:19 AM, Andrea Bolognani wrote:
> On Tue, 2018-06-26 at 09:18 +0200, Michal Privoznik wrote:
>> On 06/25/2018 07:11 PM, Andrea Bolognani wrote:
>>> @@ -19803,9 +19803,12 @@ virDomainDefParseXML(xmlDocPtr xml,
>>>                                     tmp);
>>>                      goto error;
>>>                  }
>>> -                def->features[val] = value;
>>> +                def->hpt_resizing = (virDomainHPTResizing) value;
>>
>> This typecast seems useless. Also, pre-existing, the if() statement
>> above is more verbose than it needs to be since
>> VIR_DOMAIN_HPT_RESIZING_NONE is defined to be zero.
> 
> I agree the cast is mostly unnecessary here, but it doesn't cause
> any issues either and as a personal preference I like being explicit
> about this kind of conversion :)
> 
> As for the if() statement above, which looks like
> 
>   int value = virDomainHPTResizingTypeFromString(tmp);
>   if (value < 0 || value == VIR_DOMAIN_HPT_RESIZING_NONE) {
>       ...
> 
> you could definitely rewrite the condition as 'value <= 0' and get
> the same behavior, but you would make the code more opaque as a
> result.
> 
> Right now it doesn't take much effort to figure out what the code
> above does: it very obviouly tries to convert a string to an enum,
> and errors out if either the conversion fails entirely or the
> returned value is one that we don't want the user to specify.
> 
> If rewritten, the two error conditions would be smushed together
> and the developer would need to unpack them in their head, possibly
> after looking up the virDomainHPTResizing enum and confirming 0
> corresponds to a value that it would probably make sense to
> blacklist when parsing the configuration.
> 
> The trade-off is very much not worth it just to save a few dozen
> characters, in my opinion.
> 

The thing is, we are not consistent. On many places we use short version
and in fewer places (my feeling, haven't counted them) we are using this
explicit longer version.
Also, usually the enums are designed that way so the first element (if
set to zero) means 'nada', unset value so that we can do checks like
this later in the code:

    if (def->enum == 0) { /* unset */ }

So by looking at the code I can usually tell if the first element is
supposed to be accepted or not:

    if (value < 0) ...
    if (value <= 0) ...

Either works, it's just that we ought to be consistent. But since you're
not touching that line anyway, it's okay to leave it as is and say
'pre-existing :-P'.

Michal