qemu: Fix qemu startup check for QEMU_CAPS_OBJECT_IOTHREAD

https://bugzilla.redhat.com/show_bug.cgi?id=1249981

When qemuDomainPinIOThread was added in commit id 'fb562614', a check
for the IOThread capability was not needed since a check for iothreadpids
covered the condition where the support for IOThreads was not present.
The iothreadpids array was only created if qemuProcessDetectIOThreadPIDs
was able to query the monitor for IOThreads. It would only do that if
the QEMU_CAPS_OBJECT_IOTHREAD capability was set.

However, when iothreadids were added in commit id '8d4614a5' and the
check for iothreadpids was replaced by a search through the iothreadids[]
array for the matching iothread_id that left open the possibility that
an iothreadids[] array was defined, but the entries essentially pointed
to elements with only the 'iothread_id' defined leaving the 'thread_id'
value of 0 and eventually the cpumap entry of NULL.

This was because, the original IOThreads commit id '72edaae7' only
checked if IOThreads were defined and if the emulator had the IOThreads
capability, then IOThread objects were added at startup. The "capability
failure" check was only done when a disk was assigned to an IOThread in
qemuCheckIOThreads. This was because the initial implementation had no way
to dynamically add IOThreads, but it was possible to dynamically add a
disk to the domain. So the decision was if the domain supported it, then
add the IOThread objects. Then if a disk with an IOThread defined was
added, it could check the capability and fail to add if not there. This
just meant the 'iothreads' value was essentially ignored.

Eventually commit id 'a27ed6e7' allowed for the dynamic addition and
deletion of IOThread objects. So it was no longer necessary to generate
IOThread objects to dynamically attach a disk to. However, the startup
and disk check code was not modified to reflect this.

This patch will move the capability failure check to when IOThread
objects are being added to the command line. Thus a domain that has
IOThreads defined will not be started if the emulator doesn't support
the capability. This means when qemuCheckIOThreads is called to add
a disk, it's no longer necessary to check the capability. Instead the
code can use the IOThreadFind call to indicate that the IOThread
doesn't exist.

Finally because it could be possible to have a domain running with the
iothreadids[] defined prior to this change if libvirtd is restarted each
having mostly empty elements, qemuProcessDetectIOThreadPIDs will check
if there are niothreadids when the QEMU_CAPS_OBJECT_IOTHREAD capability
check fails and remove the elements and array if it exists.

With these changes in place, it turns out the cputune-numatune test
was failing because the right bit wasn't set in the test. So used the
opportunity to fix that and create a test that would expect to fail
with some sort of iothreads defined and used, but not having the
correct capability.

5 files changed, 73 insertions, 12 deletions

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index f99e363507..7b0879d5cc 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -4145,16 +4145,8 @@ qemuBuildDriveStr(virConnectPtr conn,
 
 static bool
 qemuCheckIOThreads(virDomainDefPtr def,
-                   virQEMUCapsPtr qemuCaps,
                    virDomainDiskDefPtr disk)
 {
-    /* Have capability */
-    if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_IOTHREAD)) {
-        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-                       _("IOThreads not supported for this QEMU"));
-        return false;
-    }
-
     /* Right "type" of disk" */
     if (disk->bus != VIR_DOMAIN_DISK_BUS_VIRTIO ||
         (disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI &&
@@ -4208,7 +4200,7 @@ qemuBuildDriveDevStr(virDomainDefPtr def,
     if (!qemuCheckCCWS390AddressSupport(def, disk->info, qemuCaps, disk->dst))
         goto error;
 
-    if (disk->iothread && !qemuCheckIOThreads(def, qemuCaps, disk))
+    if (disk->iothread && !qemuCheckIOThreads(def, disk))
         goto error;
 
     switch (disk->bus) {
@@ -9462,8 +9454,13 @@ qemuBuildCommandLine(virConnectPtr conn,
     virCommandAddArg(cmd, smp);
     VIR_FREE(smp);
 
-    if (def->niothreadids > 0 &&
-        virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_IOTHREAD)) {
+    if (def->niothreadids) {
+        if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_IOTHREAD)) {
+            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                           _("IOThreads not supported for this QEMU"));
+            goto error;
+        }
+
         /* Create iothread objects using the defined iothreadids list
          * and the defined id and name from the list. These may be used
          * by a disk definition which will associate to an iothread by
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index e31885b37e..07e41f0cda 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -2296,8 +2296,32 @@ qemuProcessDetectIOThreadPIDs(virQEMUDriverPtr driver,
     int ret = -1;
     size_t i;
 
-    if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_IOTHREAD))
+    if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_IOTHREAD)) {
+        /* The following check is because at one time a domain could
+         * define iothreadids and start the domain - only failing the
+         * capability check when attempting to add a disk. Because the
+         * iothreads and [n]iothreadids were left untouched other code
+         * assumed it could use the ->thread_id value to make thread_id
+         * based adjustments (e.g. pinning, scheduling) which while
+         * succeeding would execute on the calling thread.
+         */
+        if (vm->def->niothreadids) {
+            for (i = 0; i < vm->def->niothreadids; i++) {
+                /* Check if the domain had defined any iothreadid elements
+                 * and supply a VIR_INFO indicating that it's being removed.
+                 */
+                if (!vm->def->iothreadids[i]->autofill)
+                    VIR_INFO("IOThreads not supported, remove iothread id '%u'",
+                             vm->def->iothreadids[i]->iothread_id);
+                virDomainIOThreadIDDefFree(vm->def->iothreadids[i]);
+            }
+            /* Remove any trace */
+            VIR_FREE(vm->def->iothreadids);
+            vm->def->niothreadids = 0;
+            vm->def->iothreads = 0;
+        }
         return 0;
+    }
 
     /* Get the list of IOThreads from qemu */
     if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-cputune-numatune.args b/tests/qemuxml2argvdata/qemuxml2argv-cputune-numatune.args
index 481f72ffd2..affe648089 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-cputune-numatune.args
+++ b/tests/qemuxml2argvdata/qemuxml2argv-cputune-numatune.args
@@ -1,5 +1,6 @@
 LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \
 /usr/bin/qemu-system-x86_64 -S -M pc-q35-2.3 -m 128 \
 -smp 2,maxcpus=6,sockets=6,cores=1,threads=1 \
+-object iothread,id=iothread1 -object iothread,id=iothread2 \
 -nographic -monitor unix:/tmp/test-monitor,server,nowait -no-acpi \
 -boot c -net none -serial none -parallel none
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-iothreads-nocap.xml b/tests/qemuxml2argvdata/qemuxml2argv-iothreads-nocap.xml
new file mode 100644
index 0000000000..61871e7b6c
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-iothreads-nocap.xml
@@ -0,0 +1,37 @@
+<domain type='qemu'>
+  <name>QEMUGuest1</name>
+  <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+  <memory unit='KiB'>219136</memory>
+  <currentMemory unit='KiB'>219136</currentMemory>
+  <vcpu placement='static'>2</vcpu>
+  <iothreads>4</iothreads>
+  <iothreadids>
+    <iothread id='5'/>
+    <iothread id='6'/>
+  </iothreadids>
+  <cputune>
+    <iothreadpin iothread='5' cpuset='2'/>
+    <iothreadpin iothread='6' cpuset='1'/>
+  </cputune>
+  <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</emulator>
+    <disk type='block' device='disk'>
+      <driver name='qemu' type='raw'/>
+      <source dev='/dev/HostVG/QEMUGuest1'/>
+      <target dev='hda' bus='ide'/>
+      <address type='drive' controller='0' bus='0' target='0' unit='0'/>
+    </disk>
+    <controller type='usb' index='0'/>
+    <controller type='ide' index='0'/>
+    <controller type='pci' index='0' model='pci-root'/>
+    <memballoon model='none'/>
+  </devices>
+</domain>
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 80cb55ec3e..53580e3d68 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -1285,6 +1285,7 @@ mymain(void)
     DO_TEST("iothreads", QEMU_CAPS_OBJECT_IOTHREAD);
     DO_TEST("iothreads-ids", QEMU_CAPS_OBJECT_IOTHREAD);
     DO_TEST("iothreads-ids-partial", QEMU_CAPS_OBJECT_IOTHREAD);
+    DO_TEST_FAILURE("iothreads-nocap", NONE);
     DO_TEST("iothreads-disk", QEMU_CAPS_OBJECT_IOTHREAD, QEMU_CAPS_DEVICE,
             QEMU_CAPS_DRIVE);
     DO_TEST("iothreads-disk-virtio-ccw", QEMU_CAPS_OBJECT_IOTHREAD, QEMU_CAPS_DEVICE,
@@ -1338,6 +1339,7 @@ mymain(void)
     DO_TEST_PARSE_ERROR("cputune-vcpusched-overlap", QEMU_CAPS_NAME);
     DO_TEST("cputune-numatune", QEMU_CAPS_SMP_TOPOLOGY,
             QEMU_CAPS_KVM,
+            QEMU_CAPS_OBJECT_IOTHREAD,
             QEMU_CAPS_OBJECT_MEMORY_RAM,
             QEMU_CAPS_OBJECT_MEMORY_FILE);