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
>>>
>>>
>>>
>>
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list