[libvirt] [PATCH 1/3] conf/domain_conf: use virStringParseYesNohelper

[maozy]

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.

> 
>>       }
>>       /* 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'.


Thank you very much for your review. :)
Mao

> 
>> +        def->data.vnc.websocketGenerated = false;
>>       if (sharePolicy) {
>>           int policy =
>> @@ -15479,8 +15477,8 @@ 
>> virDomainVideoDefParseXML(virDomainXMLOptionPtr xmlopt,
>>                   heads = virXMLPropString(cur, "heads");
>>                   if ((primary = virXMLPropString(cur, "primary")) != 
>> NULL) {
>> -                    if (STREQ(primary, "yes"))
>> -                        def->primary = true;
>> +                    if (virStringParseYesNo(primary, &def->primary) < 0)
>> +                        def->primary = false;
> 
> Again, I'd rather see an error reported here.
> 
>>                       VIR_FREE(primary);
>>                   }
>>
> 
> Michal
>