[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCHv3 6/8] Implement virDomain{G, S}etNumaParameters for the qemu driver



On 12/15/2011 03:50 AM, Hu Tao wrote:
> ---
>  src/qemu/qemu_driver.c |  334 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 334 insertions(+), 0 deletions(-)
> 

> +
> +    isActive = virDomainObjIsActive(vm);
> +
> +    if (flags == VIR_DOMAIN_AFFECT_CURRENT) {
> +        if (isActive)
> +            flags = VIR_DOMAIN_AFFECT_LIVE;
> +        else
> +            flags = VIR_DOMAIN_AFFECT_CONFIG;
> +    }

This can be simplified by the code in commit ae523427.

> +
> +            if (flags & VIR_DOMAIN_AFFECT_LIVE) {
> +                if (vm->def->numatune.memory.mode != VIR_DOMAIN_NUMATUNE_MEM_STRICT) {
> +                    qemuReportError(VIR_ERR_INVALID_ARG, "%s",
> +                                    _("change of nodeset for running domain is supported only when numa mode is strict"));
> +                    ret = -1;

I think OPERATION_INVALID fits better here than INVALID_ARG, since once
the domain goes offline, the same argument will then start working.
Also, the message was long.

> +                    continue;
> +                }
> +                rc = virCgroupSetCpusetMems(group, params[i].value.s);
> +                if (rc != 0) {
> +                    virReportSystemError(-rc, "%s",
> +                                         _("unable to set memory hard_limit tunable"));

Too much copy and paste.

> +                    ret = -1;
> +                    continue;
> +                }
> +            }
> +
> +            if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
> +                char *oldnodemask = strdup(persistentDef->numatune.memory.nodemask);

strdup is wrong; the CPU mask can contain NUL bytes in the middle.
Also, you have a memory leak.  Using stack allocation and memcpy solves
both issues.

> +                if (!oldnodemask) {
> +                    virReportOOMError();
> +                    ret = -1;
> +                    continue;
> +                }
> +                if (virDomainCpuSetParse(params[i].value.s,
> +                                     0,

Indentation.

> +        } else if (STREQ(param->field, VIR_DOMAIN_NUMA_MODE)) {
> +            if (param->type != VIR_TYPED_PARAM_ULLONG) {
> +                qemuReportError(VIR_ERR_INVALID_ARG, "%s",
> +                                _("invalid type for numa strict tunable, expected a 'ullong'"));

Why a ullong?  That's awfully wide, when domain_conf only stores
numatune.memory.mode as an int, and that in turn only maps to an enum
valued between 0 and 2 inclusive (see my tweaks to 3/8).  Not to mention
you used value.i later on, instead of value.ul.

> +                ret = -1;
> +                continue;
> +            }
> +
> +            if (flags & VIR_DOMAIN_AFFECT_LIVE) {
> +                qemuReportError(VIR_ERR_INVALID_ARG, _("can't change numa mode for running domain"));

Missing "%s".  Again, OPERATION_INVALID fits better here.

> +
> +static int qemuDomainGetNumaParameters(virDomainPtr dom,
> +                                       virTypedParameterPtr params,
> +                                       int *nparams,
> +                                       unsigned int flags)

Style-wise, we aren't yet consistent in libvirt (let alone in the qemu
driver), but we are starting to converge on a style where new code lists
the return type on one line and function name on column 1 of the next.

> +{
> +    struct qemud_driver *driver = dom->conn->privateData;
> +    int i;
> +    virCgroupPtr group = NULL;
> +    virDomainObjPtr vm = NULL;
> +    virDomainDefPtr persistentDef = NULL;
> +    char *nodeset = NULL;
> +    int ret = -1;
> +    int rc;
> +    bool isActive;

Again, you don't need this.

> +
> +    virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
> +                  VIR_DOMAIN_AFFECT_CONFIG |
> +                  VIR_TYPED_PARAM_STRING_OKAY, -1);
> +
> +    qemuDriverLock(driver);
> +
> +    flags &= ~VIR_TYPED_PARAM_STRING_OKAY;

A comment helps here.

> +
> +        if (virCgroupForDomain(driver->cgroup, vm->def->name, &group, 0) != 0) {
> +            qemuReportError(VIR_ERR_INTERNAL_ERROR,
> +                            _("cannot find cgroup for domain %s"), vm->def->name);
> +            goto cleanup;
> +        }
> +    }
> +
> +    if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
> +        if (!vm->persistent) {
> +            qemuReportError(VIR_ERR_OPERATION_INVALID, "%s",
> +                            _("cannot change persistent config of a transient domain"));
> +            goto cleanup;
> +        }
> +        if (!(persistentDef = virDomainObjGetPersistentDef(driver->caps, vm)))
> +            goto cleanup;
> +    }
> +
> +    if ((*nparams) == 0) {
> +        *nparams = QEMU_NB_NUMA_PARAM;
> +        ret = 0;
> +        goto cleanup;
> +    }

Do this check earlier, to avoid time doing a wasted cgroup lookup.

> +
> +    if ((*nparams) < QEMU_NB_NUMA_PARAM) {
> +        qemuReportError(VIR_ERR_INVALID_ARG,
> +                        "%s", _("Invalid parameter count"));
> +        goto cleanup;
> +    }

Drop this check.  It needlessly limits the user.

> +
> +    if (flags & VIR_DOMAIN_AFFECT_CONFIG) {

Rather than making this the outer condition, and repeating the loop
twice, I find it simpler to make it the inner condition only in the
places where it matters within a single loop.

> +        for (i = 0; i < *nparams; i++) {

Cap this loop to the max number of parameters we know about.

> +            virMemoryParameterPtr param = &params[i];
> +            param->value.ul = 0;
> +            param->type = VIR_TYPED_PARAM_ULLONG;

Wrong.  One param is string, and the other int (given my earlier tweaks).

> +
> +            switch (i) {
> +            case 0: /* fill numa nodeset here */
> +                if (virStrcpyStatic(param->field, VIR_DOMAIN_NUMA_NODESET) == NULL) {
> +                    qemuReportError(VIR_ERR_INTERNAL_ERROR,
> +                                    "%s", _("Field numa nodeset too long for destination"));
> +                    goto cleanup;
> +                }
> +                if (persistentDef->numatune.memory.nodemask) {
> +                    nodeset = virDomainCpuSetFormat(persistentDef->numatune.memory.nodemask,
> +                                                    VIR_DOMAIN_CPUMASK_LEN);
> +                    if (!nodeset) {
> +                        qemuReportError(VIR_ERR_INTERNAL_ERROR,
> +                                        "%s", _("failed to format nodeset for NUMA memory tuning"));
> +                        goto cleanup;
> +                    }
> +                    param->value.s = nodeset;
> +                    nodeset = NULL;
> +                } else {
> +                    param->value.s = strdup("");

Check for allocation failure.

> +                }
> +                param->type = VIR_TYPED_PARAM_STRING;
> +                break;
> +
> +            case 1: /* fill numa mode here */

I find it confusing listing nodeset before mode here, when the XML lists
it in the reverse direction.  For consistency, I also swapped the order
in the setter routine (but there you still have the issue that the user
can pass in parameters out of order, and result in changing the numaset
prior to incorrectly requesting a mode change, and we don't roll back
the numaset change in that case - oh well).

> +                if (virStrcpyStatic(param->field, VIR_DOMAIN_NUMA_MODE) == NULL) {
> +                    qemuReportError(VIR_ERR_INTERNAL_ERROR,
> +                                    "%s", _("Field numa mode too long for destination"));
> +                    goto cleanup;
> +                }
> +                param->value.i = persistentDef->numatune.memory.mode;
> +                break;
> +
> +            default:
> +                break;
> +                /* should not hit here */
> +            }
> +        }
> +        goto out;
> +    }
> +
> +    for (i = 0; i < QEMU_NB_NUMA_PARAM; i++) {

Wrong - you just did a buffer overflow if the user passed *nparams = 1
(well once you take in my earlier comment that requires a minimum *nparams).

> +        virTypedParameterPtr param = &params[i];
> +        param->value.ul = 0;
> +        param->type = VIR_TYPED_PARAM_ULLONG;
> +
> +        /* Coverity does not realize that if we get here, group is set.  */
> +        sa_assert(group);

I'd rather omit this until we know for sure what Coverity complains about.

> +
> +        switch (i) {
> +        case 0: /* fill numa nodeset here */
> +            rc = virCgroupGetCpusetMems(group, &nodeset);

Hmm - here, we are reading cgroups in preference to trusting what is in
domain_conf.  But earlier, in the set routine, we didn't update vm->def
(just cgroups).  That means if we alter the nodeset at runtim, then run
'virsh dumpxml', the XML will be wrong, even though the virsh query of
the numatune parameters will be correct.  The fix is either to make
setting _also_ update vm->def in addition to cgroups (but if we do that,
then why query cgroups - just read off vm->def); or to make sure that
virsh dumpxml _always_ reads cgroups rather than trusting vm->def.

I'm not sure which approach is better, so for now, I added a FIXME and
am hoping we get it resolved soon (I think you're not the first person
to fall prey to this design bug; my recollection is that the per-device
blkiotune parameters added in 0.9.8 have the same bug, so cleaning all
instances up in a separate patch later makes sense anyways).;

> +            if (rc != 0) {
> +                virReportSystemError(-rc, "%s",
> +                                     _("unable to get numa nodeset"));
> +                goto cleanup;
> +            }

> +out:
> +    *nparams = QEMU_NB_NUMA_PARAM;
> +    ret = 0;

If the user passed *nparams as 1, then this causes the user to buffer
overrun.

> +
> +cleanup:
> +    if (group)
> +        virCgroupFree(&group);

Useless if before free.

Wow - I had to really touch this up.

 src/qemu/qemu_driver.c |  282
++++++++++++++++++------------------------------
 1 files changed, 105 insertions(+), 177 deletions(-)
diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c
index 24f3047..12235ef 100644
--- i/src/qemu/qemu_driver.c
+++ w/src/qemu/qemu_driver.c
@@ -6606,10 +6606,11 @@ cleanup:
     return ret;
 }

-static int qemuDomainSetNumaParameters(virDomainPtr dom,
-                                       virTypedParameterPtr params,
-                                       int nparams,
-                                       unsigned int flags)
+static int
+qemuDomainSetNumaParameters(virDomainPtr dom,
+                            virTypedParameterPtr params,
+                            int nparams,
+                            unsigned int flags)
 {
     struct qemud_driver *driver = dom->conn->privateData;
     int i;
@@ -6617,7 +6618,6 @@ static int
qemuDomainSetNumaParameters(virDomainPtr dom,
     virCgroupPtr group = NULL;
     virDomainObjPtr vm = NULL;
     int ret = -1;
-    bool isActive;

     virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
                   VIR_DOMAIN_AFFECT_CONFIG, -1);
@@ -6632,22 +6632,11 @@ static int
qemuDomainSetNumaParameters(virDomainPtr dom,
         goto cleanup;
     }

-    isActive = virDomainObjIsActive(vm);
-
-    if (flags == VIR_DOMAIN_AFFECT_CURRENT) {
-        if (isActive)
-            flags = VIR_DOMAIN_AFFECT_LIVE;
-        else
-            flags = VIR_DOMAIN_AFFECT_CONFIG;
-    }
+    if (virDomainLiveConfigHelperMethod(driver->caps, vm, &flags,
+                                        &persistentDef) < 0)
+        goto cleanup;

     if (flags & VIR_DOMAIN_AFFECT_LIVE) {
-        if (!isActive) {
-            qemuReportError(VIR_ERR_OPERATION_INVALID,
-                            "%s", _("domain is not running"));
-            goto cleanup;
-        }
-
         if (!qemuCgroupControllerActive(driver,
VIR_CGROUP_CONTROLLER_CPUSET)) {
             qemuReportError(VIR_ERR_OPERATION_INVALID,
                             "%s", _("cgroup cpuset controller is not
mounted"));
@@ -6656,84 +6645,80 @@ static int
qemuDomainSetNumaParameters(virDomainPtr dom,

         if (virCgroupForDomain(driver->cgroup, vm->def->name, &group,
0) != 0) {
             qemuReportError(VIR_ERR_INTERNAL_ERROR,
-                            _("cannot find cgroup for domain %s"),
vm->def->name);
-            goto cleanup;
-        }
-    }
-
-    if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
-        if (!vm->persistent) {
-            qemuReportError(VIR_ERR_OPERATION_INVALID, "%s",
-                            _("cannot change persistent config of a
transient domain"));
+                            _("cannot find cgroup for domain %s"),
+                            vm->def->name);
             goto cleanup;
         }
-        if (!(persistentDef =
virDomainObjGetPersistentDef(driver->caps, vm)))
-            goto cleanup;
     }

     ret = 0;
     for (i = 0; i < nparams; i++) {
         virTypedParameterPtr param = &params[i];

-        if (STREQ(param->field, VIR_DOMAIN_NUMA_NODESET)) {
+        if (STREQ(param->field, VIR_DOMAIN_NUMA_MODE)) {
+            if (param->type != VIR_TYPED_PARAM_INT) {
+                qemuReportError(VIR_ERR_INVALID_ARG, "%s",
+                                _("invalid type for numa strict tunable, "
+                                  "expected an 'int'"));
+                ret = -1;
+                continue;
+            }
+
+            if (flags & VIR_DOMAIN_AFFECT_LIVE) {
+                qemuReportError(VIR_ERR_OPERATION_INVALID, "%s",
+                                _("can't change numa mode for running
domain"));
+                ret = -1;
+                goto cleanup;
+            }
+
+            if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
+                persistentDef->numatune.memory.mode = params[i].value.i;
+            }
+        } else if (STREQ(param->field, VIR_DOMAIN_NUMA_NODESET)) {
             int rc;
             if (param->type != VIR_TYPED_PARAM_STRING) {
                 qemuReportError(VIR_ERR_INVALID_ARG, "%s",
-                                _("invalid type for numa nodeset
tunable, expected a 'string'"));
+                                _("invalid type for numa nodeset tunable, "
+                                  "expected a 'string'"));
                 ret = -1;
                 continue;
             }

             if (flags & VIR_DOMAIN_AFFECT_LIVE) {
-                if (vm->def->numatune.memory.mode !=
VIR_DOMAIN_NUMATUNE_MEM_STRICT) {
-                    qemuReportError(VIR_ERR_INVALID_ARG, "%s",
-                                    _("change of nodeset for running
domain is supported only when numa mode is strict"));
+                if (vm->def->numatune.memory.mode !=
+                    VIR_DOMAIN_NUMATUNE_MEM_STRICT) {
+                    qemuReportError(VIR_ERR_OPERATION_INVALID, "%s",
+                                    _("change of nodeset for running
domain "
+                                      "requires strict numa mode"));
                     ret = -1;
                     continue;
                 }
                 rc = virCgroupSetCpusetMems(group, params[i].value.s);
                 if (rc != 0) {
                     virReportSystemError(-rc, "%s",
-                                         _("unable to set memory
hard_limit tunable"));
+                                         _("unable to set numa tunable"));
                     ret = -1;
                     continue;
                 }
+                /* XXX FIXME - should we also be updating vm->def
+                 * here? If not, then we need to fix dumpxml to always
+                 * read from cgroups rather than trusting vm->def.  */
             }

             if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
-                char *oldnodemask =
strdup(persistentDef->numatune.memory.nodemask);
-                if (!oldnodemask) {
-                    virReportOOMError();
-                    ret = -1;
-                    continue;
-                }
+                char oldnodemask[VIR_CPU_MAPLEN(VIR_DOMAIN_CPUMASK_LEN)];
+                memcpy(oldnodemask,
persistentDef->numatune.memory.nodemask,
+                       VIR_DOMAIN_CPUMASK_LEN);
                 if (virDomainCpuSetParse(params[i].value.s,
-                                     0,
-
persistentDef->numatune.memory.nodemask,
-                                     VIR_DOMAIN_CPUMASK_LEN) < 0) {
-                    VIR_FREE(persistentDef->numatune.memory.nodemask);
-                    persistentDef->numatune.memory.nodemask = oldnodemask;
+                                         0,
+
persistentDef->numatune.memory.nodemask,
+                                         VIR_DOMAIN_CPUMASK_LEN) < 0) {
+                    memcpy(persistentDef->numatune.memory.nodemask,
+                           oldnodemask, VIR_DOMAIN_CPUMASK_LEN);
                     ret = -1;
                     continue;
                 }
             }
-        } else if (STREQ(param->field, VIR_DOMAIN_NUMA_MODE)) {
-            if (param->type != VIR_TYPED_PARAM_ULLONG) {
-                qemuReportError(VIR_ERR_INVALID_ARG, "%s",
-                                _("invalid type for numa strict
tunable, expected a 'ullong'"));
-                ret = -1;
-                continue;
-            }
-
-            if (flags & VIR_DOMAIN_AFFECT_LIVE) {
-                qemuReportError(VIR_ERR_INVALID_ARG, _("can't change
numa mode for running domain"));
-                ret = -1;
-                goto cleanup;
-            }
-
-            if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
-                persistentDef->numatune.memory.mode = params[i].value.i;
-            }
         } else {
             qemuReportError(VIR_ERR_INVALID_ARG,
                             _("Parameter `%s' not supported"),
param->field);
@@ -6754,10 +6739,11 @@ cleanup:
     return ret;
 }

-static int qemuDomainGetNumaParameters(virDomainPtr dom,
-                                       virTypedParameterPtr params,
-                                       int *nparams,
-                                       unsigned int flags)
+static int
+qemuDomainGetNumaParameters(virDomainPtr dom,
+                            virTypedParameterPtr params,
+                            int *nparams,
+                            unsigned int flags)
 {
     struct qemud_driver *driver = dom->conn->privateData;
     int i;
@@ -6767,7 +6753,6 @@ static int
qemuDomainGetNumaParameters(virDomainPtr dom,
     char *nodeset = NULL;
     int ret = -1;
     int rc;
-    bool isActive;

     virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
                   VIR_DOMAIN_AFFECT_CONFIG |
@@ -6775,6 +6760,9 @@ static int
qemuDomainGetNumaParameters(virDomainPtr dom,

     qemuDriverLock(driver);

+    /* We blindly return a string, and let libvirt.c and
+     * remote_driver.c do the filtering on behalf of older clients
+     * that can't parse it.  */
     flags &= ~VIR_TYPED_PARAM_STRING_OKAY;

     vm = virDomainFindByUUID(&driver->domains, dom->uuid);
@@ -6785,22 +6773,17 @@ static int
qemuDomainGetNumaParameters(virDomainPtr dom,
         goto cleanup;
     }

-    isActive = virDomainObjIsActive(vm);
+    if (virDomainLiveConfigHelperMethod(driver->caps, vm, &flags,
+                                        &persistentDef) < 0)
+        goto cleanup;

-    if (flags == VIR_DOMAIN_AFFECT_CURRENT) {
-        if (isActive)
-            flags = VIR_DOMAIN_AFFECT_LIVE;
-        else
-            flags = VIR_DOMAIN_AFFECT_CONFIG;
+    if ((*nparams) == 0) {
+        *nparams = QEMU_NB_NUMA_PARAM;
+        ret = 0;
+        goto cleanup;
     }

     if (flags & VIR_DOMAIN_AFFECT_LIVE) {
-        if (!isActive) {
-            qemuReportError(VIR_ERR_OPERATION_INVALID,
-                            "%s", _("domain is not running"));
-            goto cleanup;
-        }
-
         if (!qemuCgroupControllerActive(driver,
VIR_CGROUP_CONTROLLER_MEMORY)) {
             qemuReportError(VIR_ERR_OPERATION_INVALID,
                             "%s", _("cgroup memory controller is not
mounted"));
@@ -6809,112 +6792,58 @@ static int
qemuDomainGetNumaParameters(virDomainPtr dom,

         if (virCgroupForDomain(driver->cgroup, vm->def->name, &group,
0) != 0) {
             qemuReportError(VIR_ERR_INTERNAL_ERROR,
-                            _("cannot find cgroup for domain %s"),
vm->def->name);
-            goto cleanup;
-        }
-    }
-
-    if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
-        if (!vm->persistent) {
-            qemuReportError(VIR_ERR_OPERATION_INVALID, "%s",
-                            _("cannot change persistent config of a
transient domain"));
+                            _("cannot find cgroup for domain %s"),
+                            vm->def->name);
             goto cleanup;
         }
-        if (!(persistentDef =
virDomainObjGetPersistentDef(driver->caps, vm)))
-            goto cleanup;
     }

-    if ((*nparams) == 0) {
-        *nparams = QEMU_NB_NUMA_PARAM;
-        ret = 0;
-        goto cleanup;
-    }
-
-    if ((*nparams) < QEMU_NB_NUMA_PARAM) {
-        qemuReportError(VIR_ERR_INVALID_ARG,
-                        "%s", _("Invalid parameter count"));
-        goto cleanup;
-    }
-
-    if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
-        for (i = 0; i < *nparams; i++) {
-            virMemoryParameterPtr param = &params[i];
-            param->value.ul = 0;
-            param->type = VIR_TYPED_PARAM_ULLONG;
-
-            switch (i) {
-            case 0: /* fill numa nodeset here */
-                if (virStrcpyStatic(param->field,
VIR_DOMAIN_NUMA_NODESET) == NULL) {
-                    qemuReportError(VIR_ERR_INTERNAL_ERROR,
-                                    "%s", _("Field numa nodeset too
long for destination"));
-                    goto cleanup;
-                }
-                if (persistentDef->numatune.memory.nodemask) {
-                    nodeset =
virDomainCpuSetFormat(persistentDef->numatune.memory.nodemask,
-
VIR_DOMAIN_CPUMASK_LEN);
-                    if (!nodeset) {
-                        qemuReportError(VIR_ERR_INTERNAL_ERROR,
-                                        "%s", _("failed to format
nodeset for NUMA memory tuning"));
-                        goto cleanup;
-                    }
-                    param->value.s = nodeset;
-                    nodeset = NULL;
-                } else {
-                    param->value.s = strdup("");
-                }
-                param->type = VIR_TYPED_PARAM_STRING;
-                break;
-
-            case 1: /* fill numa mode here */
-                if (virStrcpyStatic(param->field, VIR_DOMAIN_NUMA_MODE)
== NULL) {
-                    qemuReportError(VIR_ERR_INTERNAL_ERROR,
-                                    "%s", _("Field numa mode too long
for destination"));
-                    goto cleanup;
-                }
-                param->value.i = persistentDef->numatune.memory.mode;
-                break;
-
-            default:
-                break;
-                /* should not hit here */
-            }
-        }
-        goto out;
-    }
-
-    for (i = 0; i < QEMU_NB_NUMA_PARAM; i++) {
-        virTypedParameterPtr param = &params[i];
-        param->value.ul = 0;
-        param->type = VIR_TYPED_PARAM_ULLONG;
-
-        /* Coverity does not realize that if we get here, group is set.  */
-        sa_assert(group);
+    for (i = 0; i < QEMU_NB_NUMA_PARAM && i < *nparams; i++) {
+        virMemoryParameterPtr param = &params[i];

         switch (i) {
-        case 0: /* fill numa nodeset here */
-            rc = virCgroupGetCpusetMems(group, &nodeset);
-            if (rc != 0) {
-                virReportSystemError(-rc, "%s",
-                                     _("unable to get numa nodeset"));
-                goto cleanup;
-            }
-            if (virStrcpyStatic(param->field, VIR_DOMAIN_NUMA_NODESET)
== NULL) {
+        case 0: /* fill numa mode here */
+            if (!virStrcpyStatic(param->field, VIR_DOMAIN_NUMA_MODE)) {
                 qemuReportError(VIR_ERR_INTERNAL_ERROR,
-                                "%s", _("Field numa nodeset too long
for destination"));
-                VIR_FREE(nodeset);
+                                _("Field '%s' too long for destination"),
+                                VIR_DOMAIN_NUMA_MODE);
                 goto cleanup;
             }
-            param->value.s = nodeset;
-            param->type = VIR_TYPED_PARAM_STRING;
+            param->type = VIR_TYPED_PARAM_INT;
+            if (flags & VIR_DOMAIN_AFFECT_CONFIG)
+                param->value.i = persistentDef->numatune.memory.mode;
+            else
+                param->value.i = vm->def->numatune.memory.mode;
             break;

-        case 1: /* file numa mode here */
-            if (virStrcpyStatic(param->field, VIR_DOMAIN_NUMA_MODE) ==
NULL) {
+        case 1: /* fill numa nodeset here */
+            if (!virStrcpyStatic(param->field, VIR_DOMAIN_NUMA_NODESET)) {
                 qemuReportError(VIR_ERR_INTERNAL_ERROR,
-                                "%s", _("Field numa exclusive too long
for destination"));
+                                _("Field '%s' too long for destination"),
+                                VIR_DOMAIN_NUMA_NODESET);
+                goto cleanup;
+            }
+            if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
+                char *mask = persistentDef->numatune.memory.nodemask;
+                if (mask)
+                    nodeset = virDomainCpuSetFormat(mask,
+
VIR_DOMAIN_CPUMASK_LEN);
+                else
+                    nodeset = strdup("");
+            } else {
+                rc = virCgroupGetCpusetMems(group, &nodeset);
+                if (rc != 0) {
+                    virReportSystemError(-rc, "%s",
+                                         _("unable to get numa nodeset"));
+                    goto cleanup;
+                }
+            }
+            if (!nodeset) {
+                virReportOOMError();
                 goto cleanup;
             }
-            param->value.ul = vm->def->numatune.memory.mode;
+            param->type = VIR_TYPED_PARAM_STRING;
+            param->value.s = nodeset;
             break;

         default:
@@ -6923,13 +6852,12 @@ static int
qemuDomainGetNumaParameters(virDomainPtr dom,
         }
     }

-out:
-    *nparams = QEMU_NB_NUMA_PARAM;
+    if (*nparams > QEMU_NB_NUMA_PARAM)
+        *nparams = QEMU_NB_NUMA_PARAM;
     ret = 0;

 cleanup:
-    if (group)
-        virCgroupFree(&group);
+    virCgroupFree(&group);
     if (vm)
         virDomainObjUnlock(vm);
     qemuDriverUnlock(driver);



-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]