[libvirt] [PATCH 2/2] qemuDomainGetNumaParameters: Don't report spurious info

[Martin Kletzander]

On Wed, May 20, 2015 at 01:53:14PM +0200, Michal Privoznik wrote:
>On 19.05.2015 19:15, Martin Kletzander wrote:
>> On Tue, May 19, 2015 at 01:33:11PM +0200, Michal Privoznik wrote:
>>> This API does not work well on domains without <numatune/>. It blindly
>>> reports misleading info on a shutoff domain:
>>>
>>>  # virsh numatune rhel7
>>>  numa_mode      : strict
>>>  numa_nodeset   :
>>>
>>> This is obviously wrong as long as there's no numatune for rhel7
>>> domain, which isn't. What we should do, is report only those NUMA
>>> parameters, that domain has defined.
>>>
>>
>> I'm not sure, though, whether we can change the behaviour this way as
>> clients may depend on the fact that not setting anything in the XML
>> will cause this API to return 'strict' with nodeset "".  Empty nodeset
>> is something that was used for that purpose many times.  I don't like
>> the fact, but it is a fact.
>
>Depending on a buggy behaviour is itself a bug.
>

Yes, definitely, the thing is that I'm not sure whether to consider a
bug as returning "" was taken as not having any numatune for a long
time.

But let's say this is OK.

>>
>> Having said that, I haven't found anything like that mentioned in the
>> documentation, so I'm not totally against that.
>>
>> The thing is that the documentation says:
>>
>>  "As a special case, calling with @params as NULL and @nparams as 0
>>  on input will cause @nparams on output to contain the number of
>>  parameters supported by the hypervisor."
>>
>> But we don't check that params == NULL, only that *nparams == 0.  That
>> itself is not a problem, but let's continue...  Calling
>> virDomainGetNumaParameters() with *nparams == 0 returns 0 in nparams.
>> Calling that API again will again return 0 etc.  *Unless* someone
>> changes the numatune parameters in the meantime, which will cause the
>> second call to return QEMU_NB_NUMA_PARAM without filing up the values.
>> And that's a problem.  And I don't think we can return an error
>> either.
>
>I don't see why this is an error. To me it's the same as old object
>listing APIs we have. You call an API to get count of objects, so that
>you can allocate an array, then you call (in general) different API to
>fill up the array. If, however, there's no object to list, the count API
>returns zero so you can both continue to 2nd step without getting any
>error or just skip it as well. This is the same: the first call will
>tell you how much fields can we report. None. You can allocate an array
>to hold up zero items, and call the API again to fill up that array.
>Again, without any error. Correct, you'll get the value from a different
>code path, but that does not matter. As soon as someone adds another
>param to report, we're back in the track.
>

Yes, we should be reporting the tuning for each node.  That might be
reported as multiple TypedParameters with the same name, each one
representing a guest node.  That way, if there is a node without any
tuning, you need to return it, but show that it has no tuning, let's
say by saying it is bound to empty nodeset (just "").  Then we could
document this behaviour, and skip such nodesets in virsh.  You
probably see where I'm heading now, right?  The thing is that I'd
rather consider this weird (but really old) behaviour valid and
document it solving your problem by fixing it in virsh.

>>
>>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>>> ---
>>> src/qemu/qemu_driver.c | 32 +++++++++++++++++++++-----------
>>> 1 file changed, 21 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>>> index e7f235b..9b3bc68 100644
>>> --- a/src/qemu/qemu_driver.c
>>> +++ b/src/qemu/qemu_driver.c
>>> @@ -10522,13 +10522,14 @@ qemuDomainGetNumaParameters(virDomainPtr dom,
>>>                             unsigned int flags)
>>> {
>>>     virQEMUDriverPtr driver = dom->conn->privateData;
>>> -    size_t i;
>>> +    size_t i, j;
>>>     virDomainObjPtr vm = NULL;
>>>     virDomainDefPtr persistentDef = NULL;
>>>     char *nodeset = NULL;
>>>     int ret = -1;
>>>     virCapsPtr caps = NULL;
>>>     virDomainDefPtr def = NULL;
>>> +    bool hasNumatune;
>>>
>>>     virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
>>>                   VIR_DOMAIN_AFFECT_CONFIG |
>>> @@ -10552,31 +10553,40 @@ qemuDomainGetNumaParameters(virDomainPtr dom,
>>>                                         &persistentDef) < 0)
>>>         goto cleanup;
>>>
>>> -    if ((*nparams) == 0) {
>>> -        *nparams = QEMU_NB_NUMA_PARAM;
>>> -        ret = 0;
>>> -        goto cleanup;
>>> -    }
>>> -
>>>     if (flags & VIR_DOMAIN_AFFECT_CONFIG)
>>>         def = persistentDef;
>>>     else
>>>         def = vm->def;
>>>
>>> -    for (i = 0; i < QEMU_NB_NUMA_PARAM && i < *nparams; i++) {
>>> -        virMemoryParameterPtr param = &params[i];
>>> +    hasNumatune = virDomainNumatuneGetMode(def->numa, -1, NULL) == 0;
>>> +
>>> +    if ((*nparams) == 0) {
>>> +        *nparams = QEMU_NB_NUMA_PARAM - (hasNumatune ? 0 : 2);
>>
>> This is... Well, looking at it I'm wondering why didn't you do just
>> something like the following, which I think is way more readable.
>>
>> diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c
>> index e7f235b..72e735a 100644
>> --- i/src/qemu/qemu_driver.c
>> +++ w/src/qemu/qemu_driver.c
>> @@ -10529,6 +10529,8 @@ qemuDomainGetNumaParameters(virDomainPtr dom,
>>     int ret = -1;
>>     virCapsPtr caps = NULL;
>>     virDomainDefPtr def = NULL;
>> +    virDomainNumatuneMemMode mode;
>> +    bool hasNumatune = virDomainNumatuneGetMode(def->numa, -1, &mode)
>> == 0;
>>
>>     virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
>>                   VIR_DOMAIN_AFFECT_CONFIG |
>> @@ -10552,8 +10554,11 @@ qemuDomainGetNumaParameters(virDomainPtr dom,
>>                                         &persistentDef) < 0)
>>         goto cleanup;
>>
>> -    if ((*nparams) == 0) {
>> -        *nparams = QEMU_NB_NUMA_PARAM;
>> +    if (!hasNumatune || (*nparams) == 0) {
>> +        if (!hasNumatune)
>> +            *nparams == 0;
>> +        else
>> +            *nparams = QEMU_NB_NUMA_PARAM;
>>         ret = 0;
>>         goto cleanup;
>>     }
>> @@ -10572,8 +10577,7 @@ qemuDomainGetNumaParameters(virDomainPtr dom,
>>                                         VIR_TYPED_PARAM_INT, 0) < 0)
>>                 goto cleanup;
>>
>> -            virDomainNumatuneGetMode(def->numa, -1,
>> -                                     (virDomainNumatuneMemMode *)
>> &param->value.i);
>> +            param->value.i = mode;
>>             break;
>>
>>         case 1: /* fill numa nodeset here */
>> --
>
>This may be more readable, but it's far less future proof.
>
>Michal
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150520/39826285/attachment-0001.sig>