[libvirt] [PATCH 3/5] conf: Don't format cputune element when not needed

[Martin Kletzander]

On 12/18/2012 12:39 PM, Osier Yang wrote:
> On 2012年12月18日 19:20, Martin Kletzander wrote:
>> On 12/18/2012 04:01 AM, Osier Yang wrote:
>>> On 2012年12月17日 23:17, Martin Kletzander wrote:
>>>> Commit 60b176c3d0f0d5037acfa5e27c7753f657833a0b introduced a bug that
>>>> when editing an XML with cputune similar to this:
>>>
>>> Thanks for fixing this.
>>>
>>>>
>>>> ...
>>>>     <vcpu placement='static' current='1'>2</vcpu>
>>>>     <cputune>
>>>>       <vcpupin vcpu="1" cpuset="0"/>
>>>>     </cputune>
>>>> ...
>>>>
>>>> results in formatted XML that looks like this:
>>>>
>>>> ...
>>>>     <vcpu placement='static' current='1'>2</vcpu>
>>>>     <cputune>
>>>>     </cputune>
>>>> ...
>>>>
>>>> That is caused by a condition depending on def->cputune.vcpupin being
>>>> set rather than checking def->cputune.nvcpupin.  Notice that nvcpupin
>>>> can be 0 and vcpupin can still be allocated since it's a pointer to an
>>>> array, so no harm done there.
>>>> ---
>>>>    src/conf/domain_conf.c | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>>>> index cba910a..329ada3 100644
>>>> --- a/src/conf/domain_conf.c
>>>> +++ b/src/conf/domain_conf.c
>>>> @@ -13896,7 +13896,7 @@ virDomainDefFormatInternal(virDomainDefPtr def,
>>>>        virBufferAsprintf(buf, ">%u</vcpu>\n", def->maxvcpus);
>>>>
>>>>        if (def->cputune.shares ||
>>>> -        (def->cputune.vcpupin&&
>>>> !virDomainIsAllVcpupinInherited(def)) ||
>>>> +        (def->cputune.nvcpupin&&
>>>> !virDomainIsAllVcpupinInherited(def)) ||
>>>
>>> This is a good fix, but I don't think it fixes the problem what commit
>>> message describes. As both def->cputune.vcpupin and
>>> def->cputune.nvcpupin should be true here for the testing case you
>>> wrote in the commit message.
>>>
>>
>> I checked this fixes it (the vcpupin is allocated, but nvcpupin is 0,
>> which means no values are in vcpupin),
> 
> It sounds like a bug, as nvcpupin should not be 0, per there is one
> specified in the domain config.
> 

Actually, no.  It is in the way we agreed to parse those parameters. For
each pin the code does:

 if (vcpupin->vcpuid >= def->vcpus)
     /* To avoid the regression when daemon loading
      * domain confs, we can't simply error out if
      * <vcpupin> nodes greater than current vcpus,
      * ignoring them instead.
      */
     VIR_WARN("Ignore vcpupin for not onlined vcpus");
 else
     def->cputune.vcpupin[def->cputune.nvcpupin++] = vcpupin;

and that's why it's not "1".  It is expected behavior and the problem
lies in the check for the vcpupin where we should check nvcpupin instead.

>> but I'll check the other places
>> where we format it as well.
>>

As I said, I'll have a look where else it is used incorrectly.  Will
send a v2 for that.

>>> And as long as we try to use nvcpupin here, we should use nvcpupin
>>> when formating "</cputune>" too.
>>>
>>> Osier
>>>
>>>
>>>
>>
>