virDomainNumatuneGetMode: Report if numatune was defined

So far, we are not reporting if numatune was even defined. The
value of zero is blindly returned (which maps onto
VIR_DOMAIN_NUMATUNE_MEM_STRICT). Unfortunately, we are making
decisions based on this value. Instead, we should not only return
the correct value, but report to the caller if the value is valid
at all.

For better viewing of this patch use '-w'.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>

src/conf/numa_conf.c

@@ -351,20 +351,40 @@ virDomainNumaFree(virDomainNumaPtr numa)
     VIR_FREE(numa);
 }
 
-virDomainNumatuneMemMode
+/**
-virDomainNumatuneGetMode(virDomainNumaPtr numatune,
+ * virDomainNumatuneGetMode:
-                         int cellid)
+ * @numatune: pointer to numatune definition
+ * @cellid: cell selector
+ * @mode: where to store the result
+ *
+ * Get the defined mode for domain's memory. It's safe to pass
+ * NULL to @mode if the return value is the only info needed.
+ *
+ * Returns: 0 on success (with @mode updated)
+ *         -1 if no mode was defined in XML
+ */
+int virDomainNumatuneGetMode(virDomainNumaPtr numatune,
+                             int cellid,
+                             virDomainNumatuneMemMode *mode)
 {
+    int ret = -1;
+    virDomainNumatuneMemMode tmp_mode;
+
     if (!numatune)
-        return 0;
+        return ret;
 
     if (virDomainNumatuneNodeSpecified(numatune, cellid))
-        return numatune->mem_nodes[cellid].mode;
+        tmp_mode = numatune->mem_nodes[cellid].mode;
+    else if (numatune->memory.specified)
+        tmp_mode = numatune->memory.mode;
+    else
+        goto cleanup;
 
-    if (numatune->memory.specified)
+    if (mode)
-        return numatune->memory.mode;
+        *mode = tmp_mode;
-
+    ret = 0;
-    return 0;
+ cleanup:
+    return ret;
 }
 
 virBitmapPtr

src/conf/numa_conf.h

@@ -72,8 +72,9 @@ int virDomainNumatuneFormatXML(virBufferPtr buf, virDomainNumaPtr numatune)
 /*
  * Getters
  */
-virDomainNumatuneMemMode virDomainNumatuneGetMode(virDomainNumaPtr numatune,
+int virDomainNumatuneGetMode(virDomainNumaPtr numatune,
-                                                  int cellid);
+                             int cellid,
+                             virDomainNumatuneMemMode *mode);
 
 virBitmapPtr virDomainNumatuneGetNodeset(virDomainNumaPtr numatune,
                                          virBitmapPtr auto_nodeset,

src/lxc/lxc_cgroup.c

@@ -69,6 +69,7 @@ static int virLXCCgroupSetupCpusetTune(virDomainDefPtr def,
 {
     int ret = -1;
     char *mask = NULL;
+    virDomainNumatuneMemMode mode;
 
     if (def->placement_mode != VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO &&
         def->cpumask) {
@@ -81,8 +82,8 @@ static int virLXCCgroupSetupCpusetTune(virDomainDefPtr def,
         VIR_FREE(mask);
     }
 
-    if (virDomainNumatuneGetMode(def->numa, -1) !=
+    if (virDomainNumatuneGetMode(def->numa, -1, &mode) < 0 ||
-        VIR_DOMAIN_NUMATUNE_MEM_STRICT) {
+        mode == VIR_DOMAIN_NUMATUNE_MEM_STRICT) {
         ret = 0;
         goto cleanup;
     }

src/lxc/lxc_controller.c

@@ -741,25 +741,25 @@ static int virLXCControllerSetupResourceLimits(virLXCControllerPtr ctrl)
     virBitmapPtr nodeset = NULL;
     virDomainNumatuneMemMode mode;
 
-    mode = virDomainNumatuneGetMode(ctrl->def->numa, -1);
+    if (virDomainNumatuneGetMode(ctrl->def->numa, -1, &mode) == 0) {
+        if (mode == VIR_DOMAIN_NUMATUNE_MEM_STRICT &&
+            virCgroupControllerAvailable(VIR_CGROUP_CONTROLLER_CPUSET)) {
+            /* Use virNuma* API iff necessary. Once set and child is exec()-ed,
+             * there's no way for us to change it. Rely on cgroups (if available
+             * and enabled in the config) rather than virNuma*. */
+            VIR_DEBUG("Relying on CGroups for memory binding");
+        } else {
 
-    if (mode == VIR_DOMAIN_NUMATUNE_MEM_STRICT &&
+            VIR_DEBUG("Setting up process resource limits");
-        virCgroupControllerAvailable(VIR_CGROUP_CONTROLLER_CPUSET)) {
-        /* Use virNuma* API iff necessary. Once set and child is exec()-ed,
-         * there's no way for us to change it. Rely on cgroups (if available
-         * and enabled in the config) rather than virNuma*. */
-        VIR_DEBUG("Relying on CGroups for memory binding");
-    } else {
 
-        VIR_DEBUG("Setting up process resource limits");
+            if (virLXCControllerGetNumadAdvice(ctrl, &auto_nodeset) < 0)
+                goto cleanup;
 
-        if (virLXCControllerGetNumadAdvice(ctrl, &auto_nodeset) < 0)
+            nodeset = virDomainNumatuneGetNodeset(ctrl->def->numa, auto_nodeset, -1);
-            goto cleanup;
 
-        nodeset = virDomainNumatuneGetNodeset(ctrl->def->numa, auto_nodeset, -1);
+            if (virNumaSetupMemoryPolicy(mode, nodeset) < 0)
-
+                goto cleanup;
-        if (virNumaSetupMemoryPolicy(mode, nodeset) < 0)
+        }
-            goto cleanup;
     }
 
     if (virLXCControllerSetupCpuAffinity(ctrl) < 0)

src/parallels/parallels_sdk.c

@@ -1845,6 +1845,7 @@ prlsdkCheckUnsupportedParams(PRL_HANDLE sdkdom, virDomainDefPtr def)
     size_t i;
     PRL_VM_TYPE vmType;
     PRL_RESULT pret;
+    virDomainNumatuneMemMode memMode;
 
     if (def->title) {
         virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
@@ -1924,8 +1925,8 @@ prlsdkCheckUnsupportedParams(PRL_HANDLE sdkdom, virDomainDefPtr def)
      * virDomainDefPtr always contain non zero NUMA configuration
      * So, just make sure this configuration does't differ from auto generated.
      */
-    if ((virDomainNumatuneGetMode(def->numa, -1) !=
+    if ((virDomainNumatuneGetMode(def->numa, -1, &memMode) == 0 &&
-         VIR_DOMAIN_NUMATUNE_MEM_STRICT) ||
+         memMode == VIR_DOMAIN_NUMATUNE_MEM_STRICT) ||
          virDomainNumatuneHasPerNodeBinding(def->numa)) {
         virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
                         _("numa parameters are not supported "

src/qemu/qemu_cgroup.c

@@ -613,14 +613,15 @@ qemuSetupCpusetMems(virDomainObjPtr vm)
 {
     virCgroupPtr cgroup_temp = NULL;
     qemuDomainObjPrivatePtr priv = vm->privateData;
+    virDomainNumatuneMemMode mode;
     char *mem_mask = NULL;
     int ret = -1;
 
     if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPUSET))
         return 0;
 
-    if (virDomainNumatuneGetMode(vm->def->numa, -1) !=
+    if (virDomainNumatuneGetMode(vm->def->numa, -1, &mode) < 0 ||
-        VIR_DOMAIN_NUMATUNE_MEM_STRICT)
+        mode != VIR_DOMAIN_NUMATUNE_MEM_STRICT)
         return 0;
 
     if (virDomainNumatuneMaybeFormatNodeset(vm->def->numa,
@@ -979,6 +980,7 @@ qemuSetupCgroupForVcpu(virDomainObjPtr vm)
     unsigned long long period = vm->def->cputune.period;
     long long quota = vm->def->cputune.quota;
     char *mem_mask = NULL;
+    virDomainNumatuneMemMode mem_mode;
 
     if ((period || quota) &&
         !virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPU)) {
@@ -1009,8 +1011,8 @@ qemuSetupCgroupForVcpu(virDomainObjPtr vm)
         return 0;
     }
 
-    if (virDomainNumatuneGetMode(vm->def->numa, -1) ==
+    if (virDomainNumatuneGetMode(vm->def->numa, -1, &mem_mode) == 0 &&
-        VIR_DOMAIN_NUMATUNE_MEM_STRICT &&
+        mem_mode == VIR_DOMAIN_NUMATUNE_MEM_STRICT &&
         virDomainNumatuneMaybeFormatNodeset(vm->def->numa,
                                             priv->autoNodeset,
                                             &mem_mask, -1) < 0)
@@ -1153,6 +1155,7 @@ qemuSetupCgroupForIOThreads(virDomainObjPtr vm)
     unsigned long long period = vm->def->cputune.period;
     long long quota = vm->def->cputune.quota;
     char *mem_mask = NULL;
+    virDomainNumatuneMemMode mem_mode;
 
     if ((period || quota) &&
         !virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPU)) {
@@ -1176,8 +1179,8 @@ qemuSetupCgroupForIOThreads(virDomainObjPtr vm)
     if (priv->cgroup == NULL)
         return 0;
 
-    if (virDomainNumatuneGetMode(vm->def->numa, -1) ==
+    if (virDomainNumatuneGetMode(vm->def->numa, -1, &mem_mode) == 0 &&
-        VIR_DOMAIN_NUMATUNE_MEM_STRICT &&
+        mem_mode == VIR_DOMAIN_NUMATUNE_MEM_STRICT &&
         virDomainNumatuneMaybeFormatNodeset(vm->def->numa,
                                             priv->autoNodeset,
                                             &mem_mask, -1) < 0)

src/qemu/qemu_command.c

@@ -4707,7 +4707,9 @@ qemuBuildMemoryBackendStr(unsigned long long size,
         return -1;
 
     memAccess = virDomainNumaGetNodeMemoryAccessMode(def->numa, guestNode);
-    mode = virDomainNumatuneGetMode(def->numa, guestNode);
+    if (virDomainNumatuneGetMode(def->numa, guestNode, &mode) < 0 &&
+        virDomainNumatuneGetMode(def->numa, -1, &mode) < 0)
+        mode = VIR_DOMAIN_NUMATUNE_MEM_STRICT;
 
     if (pagesize == 0 || pagesize != system_page_size) {
         /* Find the huge page size we want to use */

src/qemu/qemu_driver.c

@@ -4737,6 +4737,7 @@ qemuDomainHotplugVcpus(virQEMUDriverPtr driver,
     int ncpupids;
     virCgroupPtr cgroup_vcpu = NULL;
     char *mem_mask = NULL;
+    virDomainNumatuneMemMode mem_mode;
 
     qemuDomainObjEnterMonitor(driver, vm);
 
@@ -4804,8 +4805,8 @@ qemuDomainHotplugVcpus(virQEMUDriverPtr driver,
         goto cleanup;
     }
 
-    if (virDomainNumatuneGetMode(vm->def->numa, -1) ==
+    if (virDomainNumatuneGetMode(vm->def->numa, -1, &mem_mode) == 0 &&
-        VIR_DOMAIN_NUMATUNE_MEM_STRICT &&
+        mem_mode == VIR_DOMAIN_NUMATUNE_MEM_STRICT &&
         virDomainNumatuneMaybeFormatNodeset(vm->def->numa,
                                             priv->autoNodeset,
                                             &mem_mask, -1) < 0)
@@ -6113,6 +6114,7 @@ qemuDomainHotplugAddIOThread(virQEMUDriverPtr driver,
     qemuMonitorIOThreadInfoPtr *new_iothreads = NULL;
     virCgroupPtr cgroup_iothread = NULL;
     char *mem_mask = NULL;
+    virDomainNumatuneMemMode mode;
     virDomainIOThreadIDDefPtr iothrid;
     virBitmapPtr cpumask;
 
@@ -6154,8 +6156,8 @@ qemuDomainHotplugAddIOThread(virQEMUDriverPtr driver,
     }
     vm->def->iothreads = exp_niothreads;
 
-    if (virDomainNumatuneGetMode(vm->def->numa, -1) ==
+    if (virDomainNumatuneGetMode(vm->def->numa, -1, &mode) == 0 &&
-        VIR_DOMAIN_NUMATUNE_MEM_STRICT &&
+        mode == VIR_DOMAIN_NUMATUNE_MEM_STRICT &&
         virDomainNumatuneMaybeFormatNodeset(vm->def->numa,
                                             priv->autoNodeset,
                                             &mem_mask, -1) < 0)
@@ -10331,11 +10333,12 @@ qemuDomainSetNumaParamsLive(virDomainObjPtr vm,
     virCgroupPtr cgroup_temp = NULL;
     qemuDomainObjPrivatePtr priv = vm->privateData;
     char *nodeset_str = NULL;
+    virDomainNumatuneMemMode mode;
     size_t i = 0;
     int ret = -1;
 
-    if (virDomainNumatuneGetMode(vm->def->numa, -1) !=
+    if (virDomainNumatuneGetMode(vm->def->numa, -1, &mode) < 0 ||
-        VIR_DOMAIN_NUMATUNE_MEM_STRICT) {
+        mode != VIR_DOMAIN_NUMATUNE_MEM_STRICT) {
         virReportError(VIR_ERR_OPERATION_INVALID, "%s",
                        _("change of nodeset for running domain "
                          "requires strict numa mode"));
@@ -10392,6 +10395,7 @@ qemuDomainSetNumaParameters(virDomainPtr dom,
     virCapsPtr caps = NULL;
     qemuDomainObjPrivatePtr priv;
     virBitmapPtr nodeset = NULL;
+    virDomainNumatuneMemMode config_mode;
     int mode = -1;
 
     virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
@@ -10467,7 +10471,8 @@ qemuDomainSetNumaParameters(virDomainPtr dom,
 
     if (flags & VIR_DOMAIN_AFFECT_LIVE) {
         if (mode != -1 &&
-            virDomainNumatuneGetMode(vm->def->numa, -1) != mode) {
+            virDomainNumatuneGetMode(vm->def->numa, -1, &config_mode) == 0 &&
+            config_mode != mode) {
             virReportError(VIR_ERR_OPERATION_INVALID, "%s",
                            _("can't change numatune mode for running domain"));
             goto endjob;
@@ -10568,7 +10573,8 @@ qemuDomainGetNumaParameters(virDomainPtr dom,
                                         VIR_TYPED_PARAM_INT, 0) < 0)
                 goto cleanup;
 
-            param->value.i = virDomainNumatuneGetMode(def->numa, -1);
+            virDomainNumatuneGetMode(def->numa, -1,
+                                     (virDomainNumatuneMemMode *) &param->value.i);
             break;
 
         case 1: /* fill numa nodeset here */

src/qemu/qemu_process.c

@@ -3135,21 +3135,21 @@ static int qemuProcessHook(void *data)
     if (virSecurityManagerClearSocketLabel(h->driver->securityManager, h->vm->def) < 0)
         goto cleanup;
 
-    mode = virDomainNumatuneGetMode(h->vm->def->numa, -1);
+    if (virDomainNumatuneGetMode(h->vm->def->numa, -1, &mode) == 0) {
+        if (mode == VIR_DOMAIN_NUMATUNE_MEM_STRICT &&
+            h->cfg->cgroupControllers & (1 << VIR_CGROUP_CONTROLLER_CPUSET) &&
+            virCgroupControllerAvailable(VIR_CGROUP_CONTROLLER_CPUSET)) {
+            /* Use virNuma* API iff necessary. Once set and child is exec()-ed,
+             * there's no way for us to change it. Rely on cgroups (if available
+             * and enabled in the config) rather than virNuma*. */
+            VIR_DEBUG("Relying on CGroups for memory binding");
+        } else {
+            nodeset = virDomainNumatuneGetNodeset(h->vm->def->numa,
+                                                  priv->autoNodeset, -1);
 
-    if (mode == VIR_DOMAIN_NUMATUNE_MEM_STRICT &&
+            if (virNumaSetupMemoryPolicy(mode, nodeset) < 0)
-        h->cfg->cgroupControllers & (1 << VIR_CGROUP_CONTROLLER_CPUSET) &&
+                goto cleanup;
-        virCgroupControllerAvailable(VIR_CGROUP_CONTROLLER_CPUSET)) {
+        }
-        /* Use virNuma* API iff necessary. Once set and child is exec()-ed,
-         * there's no way for us to change it. Rely on cgroups (if available
-         * and enabled in the config) rather than virNuma*. */
-        VIR_DEBUG("Relying on CGroups for memory binding");
-    } else {
-        nodeset = virDomainNumatuneGetNodeset(h->vm->def->numa,
-                                              priv->autoNodeset, -1);
-
-        if (virNumaSetupMemoryPolicy(mode, nodeset) < 0)
-            goto cleanup;
     }
 
     ret = 0;