[libvirt] [PATCH 1/3] conf/domain_conf: usevirStringParseYesNohelper

[maozy]

On 10/16/19 7:02 PM, Michal Privoznik wrote:
> On 10/16/19 12:10 PM, maozy wrote:
>> Hi, Michal
>>
>> On 10/16/19 4:38 PM, Michal Privoznik wrote:
>>> On 10/16/19 4:39 AM, Mao Zhongyi wrote:
>>>> This helper performs a conversion from a "yes|no" string
>>>> to a corresponding boolean, and several conversions were
>>>> already done, but there are still some omissions.
>>>>
>>>> For most of the remaining usages in domain_conf.c only
>>>> "yes" is explicitly checked for. This means all other
>>>> values are implicitly handled as 'false'. In this case,
>>>> use virStringParseYesNo, but if it returns -1, don't
>>>> raise an error but set the bool value to false.
>>>>
>>>> Cc: crobinso at redhat.com
>>>> Cc: berrange at redhat.com
>>>> Cc: abologna at redhat.com
>>>> Cc: laine at laine.org
>>>> Cc: phrdina at redhat.com
>>>> Cc: mkletzan at redhat.com
>>>> Cc: g.sho1500 at gmail.com
>>>>
>>>> Signed-off-by: Mao Zhongyi <maozhongyi at cmss.chinamobile.com>
>>>> Signed-off-by: Zhang Shengju <zhangshengju at cmss.chinamobile.com>
>>>> ---
>>>>   src/conf/domain_conf.c | 30 ++++++++++++++----------------
>>>>   1 file changed, 14 insertions(+), 16 deletions(-)
>>>>
>>>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>>>> index 10d6bf0eea..7420658726 100644
>>>> --- a/src/conf/domain_conf.c
>>>> +++ b/src/conf/domain_conf.c
>>>> @@ -7601,8 +7601,8 @@ 
>>>> virDomainHostdevSubsysUSBDefParseXML(xmlNodePtr node,
>>>>       }
>>>>       if ((autoAddress = virXMLPropString(node, "autoAddress"))) {
>>>> -        if (STREQ(autoAddress, "yes"))
>>>> -            usbsrc->autoAddress = true;
>>>> +        if (virStringParseYesNo(autoAddress, &usbsrc->autoAddress) 
>>>> < 0)
>>>> +            usbsrc->autoAddress = false;
>>>
>>> I think we should either report a proper error here or don't touch 
>>> this line (because this attribute is generated by libvirt itself, it 
>>> doesn't come from an user).
>>
>> This patch replaces all STREQs that explicitly handle 'yes' to true,
>> reserving the original implicitly handling of other values to false,
>> so not report an error, which is also the recommendation of Cole
>> Robinson in BiteSizedtasks. Therefor, I'm not very sure whether it
>> is better to report an error in this case? If indeed, will fix it in v2.
> 
> Alright then. But what we can use then is:
> 
> ignore_value(virStringParseYesNo(autoAddress, &usbsrc->autoAddress));
> 
> which is equivallent to existing code and also shorter.
> 
>>
>>>
>>>>       }
>>>>       /* Product can validly be 0, so we need some extra help to 
>>>> determine
>>>> @@ -8168,8 +8168,8 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr 
>>>> node,
>>>>        * (e.g. <interface>) with type='hostdev')
>>>>        */
>>>>       if ((managed = virXMLPropString(node, "managed")) != NULL) {
>>>> -        if (STREQ(managed, "yes"))
>>>> -            def->managed = true;
>>>> +        if (virStringParseYesNo(managed, &def->managed) < 0)
>>>> +            def->managed = false;
>>>
>>> Here we need to report a proper error because "managed" attribute 
>>> comes from user, therefore risk of an user providing invalid value is 
>>> high compared to the first hunk. But if we want to be consistent we 
>>> should report error in both cases.
>>>
>>>>       }
>>>>       sgio = virXMLPropString(node, "sgio");
>>>> @@ -13807,9 +13807,7 @@ 
>>>> virDomainGraphicsListenDefParseXML(virDomainGraphicsListenDefPtr def,
>>>>       if (autoGenerated &&
>>>>           flags & VIR_DOMAIN_DEF_PARSE_STATUS) {
>>>> -        if (STREQ(autoGenerated, "yes")) {
>>>> -            def->autoGenerated = true;
>>>> -        } else if (STRNEQ(autoGenerated, "no")) {
>>>> +        if (virStringParseYesNo(autoGenerated, &def->autoGenerated) 
>>>> < 0) {
>>>>               virReportError(VIR_ERR_XML_ERROR,
>>>>                              _("Invalid autoGenerated value: %s"),
>>>>                              autoGenerated);
>>>
>>> This one is correct.
>>>
>>>> @@ -13939,12 +13937,11 @@ 
>>>> virDomainGraphicsDefParseXMLVNC(virDomainGraphicsDefPtr def,
>>>>       }
>>>>       if (autoport) {
>>>> -        if (STREQ(autoport, "yes")) {
>>>> -            if (flags & VIR_DOMAIN_DEF_PARSE_INACTIVE)
>>>> -                def->data.vnc.port = 0;
>>>> -            def->data.vnc.autoport = true;
>>>> -        } else {
>>>> +        if (virStringParseYesNo(autoport, &def->data.vnc.autoport) 
>>>> < 0) {
>>>>               def->data.vnc.autoport = false;
>>>> +        } else {
>>>> +            if (def->data.vnc.autoport && flags & 
>>>> VIR_DOMAIN_DEF_PARSE_INACTIVE)
>>>> +                def->data.vnc.port = 0;
>>>
>>> This doesn't need to go under else branch really.
>>
>> right
>>>
>>>>           }
>>>>       }
>>>> @@ -13958,8 +13955,9 @@ 
>>>> virDomainGraphicsDefParseXMLVNC(virDomainGraphicsDefPtr def,
>>>>           }
>>>>       }
>>>> -    if (websocketGenerated && STREQ(websocketGenerated, "yes"))
>>>> -        def->data.vnc.websocketGenerated = true;
>>>> +    if (websocketGenerated &&
>>>> +        virStringParseYesNo(websocketGenerated, 
>>>> &def->data.vnc.websocketGenerated))
>>>
>>> Ooops, you've missed '< 0' comparison.
>>
>> virStringParseYesNo return 0 on succeed and -1 on error, and
>> here is && operation, so, no explicit comparison '< 0'.
> 
> The comparison is required because even though the function doesn't 
> return any positive value now it might at some point in the future. In 
> libvirt, there's the following semantics for integer return values:
> 
> < 0 : an error state,
>    0 : success,
>  > 0 : success, but also something is signalized to the caller
> 
> There's plenty of examples, but let me pick just one: The retvals for 
> virConfGetValueBool() are defined as follows:
> 
> -1 on error,
>   0 if missing,
>   1 if the value was present
> 
> And in this code you're adding, you're interested whether the function 
> failed or not. If we ever add another success value (e.g. 1), we don't 
> have to fix all the places where the function is called.

Well, very far-sighted. I got it.

Thanks for the detailed and kind reply.
Mao

> 
> Michal
>