[libvirt] [PATCH] Fix parsing of bond interface XML

[Jim Fehlig]

Eric Blake wrote:
> On 03/21/2013 03:50 PM, Jim Fehlig wrote:
>   
>> Noticed that parsing bond interface XML containing the miimon element
>> fails
>>
>>   <interface type="bond" name="bond0">
>>     ...
>>     <bond mode="active-backup">
>>       <miimon freq="100" carrier="netif"/>
>>       ...
>>     </bond>
>>   </interface>
>>
>> This configuration does not contain the optional updelay and downdelay
>> attributes, but parsing will fail due to returning the result of
>> virXPathULong (a -1 when the attribute doesn't exist) from
>> virInterfaceDefParseBond after examining the updelay attribute.
>>
>> I considered just adding a ret = 0; near the bottom of
>> virInterfaceDefParseBond, but see there is no cleanup in the error
>> label.  Instead, just return failure where failure occurs and
>> return success if the end of the function is reached.
>> ---
>>     
>
>   
>>  virInterfaceDefParseBond(virInterfaceDefPtr def,
>>                           xmlXPathContextPtr ctxt) {
>> -    int ret = -1;
>> +    int res;
>>      unsigned long tmp;
>>     
>
> Pre-existing, but this is a long...
>
>   
>>      if (virXPathNode("./miimon[1]", ctxt) != NULL) {
>>          def->data.bond.monit = VIR_INTERFACE_BOND_MONIT_MII;
>>  
>> -        ret = virXPathULong("string(./miimon/@freq)", ctxt, &tmp);
>> -        if ((ret == -2) || (ret == -1)) {
>> +        res = virXPathULong("string(./miimon/@freq)", ctxt, &tmp);
>> +        if ((res == -2) || (res == -1)) {
>>              virReportError(VIR_ERR_XML_ERROR,
>>                             "%s", _("bond interface miimon freq missing or invalid"));
>> -            goto error;
>> +            return -1;
>>          }
>>          def->data.bond.frequency = (int) tmp;
>>     
>
> yet here we want it to be an int.  Why not use virXPathInt() on an int
> in the first place, so that we don't risk silent truncation?  In fact,
> why even have tmp, when we can:
>
> ret = virXPathInt("string(./miimon/@freq)", ctxt,
>                   &def->data.bond.frequency)
>
> in the first place?
>   

The current behavior is to store the value in def depending on the
return value of virXPathULong.  But looking at the function, this is
only done when the return value is 0.  And it seems the virXPath
functions only store the value when returning 0, so IMO your proposal is
correct.  I've sent a V2.

Thanks,
Jim