Re: [PATCH] qemu: Validate memory hotplug in domainValidateCallback instead of cmd line generator

[Daniel Henrique Barboza]

On 8/31/20 10:44 AM, Michal Privoznik wrote:

When editing a domain with hotplug enabled, I removed the only
NUMA node it had and got no error. I got the error later though,
when starting the domain. This is not as user friendly as it can
be. Move the validation call out from command line generator and
into domain validator (which is called prior to starting cmd line
generation anyway).

When doing this, I had to remove memory-hotplug-nonuma xml2xml
test case because there is no way the test case can succeed,
obviously.

I wonder why x86 doesn't just turn on auto_enable_numa in QEMU to
create an empty NUMA node for hotplug. If the user set everything else
to enable mem hotplug (ram_slots, maxram_size, etc) then might as well
go the distance ...

(ps: pSeries has auto_enable_numa, which makes this comment above completely
biased :P )

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

Reviewed-by: Daniel Henrique Barboza <danielhb...@gmail.com>

  src/qemu/qemu_command.c                       |  3 --
  src/qemu/qemu_domain.c                        |  2 ++
  src/qemu/qemu_validate.c                      |  3 ++
  tests/qemuxml2argvtest.c                      |  2 +-
  .../memory-hotplug-nonuma.xml                 | 28 -------------------
  tests/qemuxml2xmltest.c                       | 14 ++++++----
  6 files changed, 14 insertions(+), 38 deletions(-)
  delete mode 100644 tests/qemuxml2xmloutdata/memory-hotplug-nonuma.xml

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 6798febf8d..bd98b0a97c 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -7123,9 +7123,6 @@ qemuBuildMemCommandLine(virCommandPtr cmd,
                          virQEMUCapsPtr qemuCaps,
                          qemuDomainObjPrivatePtr priv)
  {
-    if (qemuDomainDefValidateMemoryHotplug(def, qemuCaps, NULL) < 0)
-        return -1;
-
      virCommandAddArg(cmd, "-m");

if (virDomainDefHasMemoryHotplug(def)) {

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 21f24fceed..aa0f5c1a2d 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -8468,6 +8468,8 @@ qemuCheckMemoryDimmConflict(const virDomainDef *def,

return false;

  }
+
+
  static int
  qemuDomainDefValidateMemoryHotplugDevice(const virDomainMemoryDef *mem,
                                           const virDomainDef *def)
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
index c4b86326ad..6f3fee5427 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
@@ -916,6 +916,9 @@ qemuValidateDomainDef(const virDomainDef *def,
          }
      }

+ if (qemuDomainDefValidateMemoryHotplug(def, qemuCaps, NULL) < 0)

+        return -1;
+
      if (qemuValidateDomainDefClockTimers(def, qemuCaps) < 0)
          return -1;

diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c

index 01839cb88c..e93948e3fc 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -2931,7 +2931,7 @@ mymain(void)
      DO_TEST("cpu-host-passthrough-features", QEMU_CAPS_KVM);

DO_TEST_FAILURE("memory-align-fail", NONE);

-    DO_TEST_FAILURE("memory-hotplug-nonuma", QEMU_CAPS_DEVICE_PC_DIMM);
+    DO_TEST_PARSE_ERROR("memory-hotplug-nonuma", QEMU_CAPS_DEVICE_PC_DIMM);
      DO_TEST("memory-hotplug", NONE);
      DO_TEST("memory-hotplug", QEMU_CAPS_DEVICE_PC_DIMM, QEMU_CAPS_NUMA);
      DO_TEST("memory-hotplug-dimm", QEMU_CAPS_DEVICE_PC_DIMM, QEMU_CAPS_NUMA,
diff --git a/tests/qemuxml2xmloutdata/memory-hotplug-nonuma.xml 
b/tests/qemuxml2xmloutdata/memory-hotplug-nonuma.xml
deleted file mode 100644
index 7c277f01a3..0000000000
--- a/tests/qemuxml2xmloutdata/memory-hotplug-nonuma.xml
+++ /dev/null
@@ -1,28 +0,0 @@
-<domain type='qemu'>
-  <name>QEMUGuest1</name>
-  <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
-  <maxMemory slots='9' unit='KiB'>1233456789</maxMemory>
-  <memory unit='KiB'>219136</memory>
-  <currentMemory unit='KiB'>219136</currentMemory>
-  <vcpu placement='static'>1</vcpu>
-  <os>
-    <type arch='i686' machine='pc'>hvm</type>
-    <boot dev='hd'/>
-  </os>
-  <clock offset='utc'/>
-  <on_poweroff>destroy</on_poweroff>
-  <on_reboot>restart</on_reboot>
-  <on_crash>destroy</on_crash>
-  <devices>
-    <emulator>/usr/bin/qemu-system-i386</emulator>
-    <controller type='usb' index='0'>
-      <address type='pci' domain='0x0000' bus='0x00' slot='0x01' 
function='0x2'/>
-    </controller>
-    <controller type='pci' index='0' model='pci-root'/>
-    <input type='mouse' bus='ps2'/>
-    <input type='keyboard' bus='ps2'/>
-    <memballoon model='virtio'>
-      <address type='pci' domain='0x0000' bus='0x00' slot='0x03' 
function='0x0'/>
-    </memballoon>
-  </devices>
-</domain>
diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
index a07e2b7553..6eb008c8d2 100644
--- a/tests/qemuxml2xmltest.c
+++ b/tests/qemuxml2xmltest.c
@@ -303,18 +303,21 @@ mymain(void)

DO_TEST("pages-discard", NONE);

      DO_TEST("pages-discard-hugepages", QEMU_CAPS_OBJECT_MEMORY_FILE);
-    DO_TEST("pages-dimm-discard", NONE);
+    DO_TEST("pages-dimm-discard", QEMU_CAPS_DEVICE_PC_DIMM);
      DO_TEST("hugepages-default", QEMU_CAPS_OBJECT_MEMORY_FILE);
      DO_TEST("hugepages-default-2M", QEMU_CAPS_OBJECT_MEMORY_FILE);
      DO_TEST("hugepages-default-system-size", QEMU_CAPS_OBJECT_MEMORY_FILE);
      DO_TEST("hugepages-nodeset", QEMU_CAPS_OBJECT_MEMORY_FILE);
      DO_TEST("hugepages-numa-default-2M", QEMU_CAPS_OBJECT_MEMORY_FILE);
-    DO_TEST("hugepages-numa-default-dimm", QEMU_CAPS_OBJECT_MEMORY_FILE);
+    DO_TEST("hugepages-numa-default-dimm", QEMU_CAPS_DEVICE_PC_DIMM,
+            QEMU_CAPS_OBJECT_MEMORY_FILE);
      DO_TEST("hugepages-numa-nodeset", QEMU_CAPS_OBJECT_MEMORY_FILE);
      DO_TEST("hugepages-numa-nodeset-part", QEMU_CAPS_OBJECT_MEMORY_FILE);
      DO_TEST("hugepages-shared", QEMU_CAPS_OBJECT_MEMORY_FILE);
-    DO_TEST("hugepages-memaccess", QEMU_CAPS_OBJECT_MEMORY_FILE);
-    DO_TEST("hugepages-memaccess2", QEMU_CAPS_OBJECT_MEMORY_FILE);
+    DO_TEST("hugepages-memaccess", QEMU_CAPS_DEVICE_PC_DIMM,
+            QEMU_CAPS_OBJECT_MEMORY_FILE);
+    DO_TEST("hugepages-memaccess2", QEMU_CAPS_DEVICE_PC_DIMM,
+            QEMU_CAPS_OBJECT_MEMORY_FILE);
      DO_TEST("hugepages-nvdimm", QEMU_CAPS_DEVICE_NVDIMM,
              QEMU_CAPS_OBJECT_MEMORY_FILE);
      DO_TEST("nosharepages", NONE);
@@ -1260,8 +1263,7 @@ mymain(void)
      DO_TEST_CAPS_ARCH_LATEST("aarch64-features-sve", "aarch64");

DO_TEST("memory-hotplug", NONE);

-    DO_TEST("memory-hotplug-nonuma", NONE);
-    DO_TEST("memory-hotplug-dimm", NONE);
+    DO_TEST("memory-hotplug-dimm", QEMU_CAPS_DEVICE_PC_DIMM);
      DO_TEST("memory-hotplug-nvdimm", QEMU_CAPS_DEVICE_NVDIMM);
      DO_TEST("memory-hotplug-nvdimm-access", QEMU_CAPS_DEVICE_NVDIMM);
      DO_TEST("memory-hotplug-nvdimm-label", QEMU_CAPS_DEVICE_NVDIMM);