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

[Martin Kletzander <mkletzan@xxxxxxxxxx>]

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.

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.

> Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
> ---
> 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 */
--

Attachment: signature.asc
Description: PGP signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list