qemu: log error when domain has an unsupported IDE controller

We have previously effectively ignored all <controller type='ide'>
elements in a domain definition.

On the i440fx-based machinetypes there is an IDE controller that is
included in the chipset and can't be removed (which is the ide
controller with index='0'>), so it makes sense to ignore that one
controller. However, if an i440fx domain definition has a 2nd
controller, nothing catches this error (unless you also have a disk
attached to it, in which case qemu will complain that you're trying to
use the ide controller named "ide1", which doesn't exist), and if any
other type of domain has even a single controller defined, it will be
incorrectly ignored.

Ignoring a bogus controller definition isn't such a big problem, as
long as an error is logged when any disk is attached to that
non-existent controller. But in the case of q35-based machinetypes,
the hardcoded id ("alias" in libvirt terms) of its builtin SATA
controller is "ide", which happens to be the same id as the builtin
IDE controller on i440fx machinetypes. So libvirt creates a
commandline believing that it is connecting the disk to the builtin
(but actually nonexistent) IDE controller, qemu thinks that libvirt
wanted that disk connected to the builtin SATA controller, and
everybody is happy.

Until you try to connect a 2nd disk to the IDE controller. Then qemu
will complain that you're trying to set unit=1 on a controller that
requires unit=0 (SATA controllers are organized differently than IDE
controllers).

After this patch, if a domain has an IDE controller defined for a
machinetype that has no IDE controllers, libvirt will log an error
about the controller itself as it is building the qemu commandline
(rather than a (possible) error from qemu about disks attached to that
controller). This is done by adding IDE to the list of controller
types that are handled in the loop that creates controller command
strings in qemuBuildCommandline() (previously it would *always* skip
IDE controllers). Then qemuBuildControllerDevStr() is modified to log
an appropriate error in the case of IDE controllers.

In the future, if we add support for extra IDE controllers (piix3-ide
and/or piix4-ide) we can just add it into the IDE case in
qemuBuildControllerDevStr(). For now, nobody seems anxious to add
extra support for an aging and very slow controller, when there are so
many better options available.

Resolves:

https://bugzilla.redhat.com/show_bug.cgi?id=1176071 (Fedora)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index c6962d5d95f60ad60eb2830431bcb0eb3fc3af3a..b231d65b2471e610da3db9f31acf2b84fadcacb9 100644 (file)
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -4599,11 +4599,25 @@ qemuBuildControllerDevStr(virDomainDefPtr domainDef,
         }
         break;
 
-    /* We always get an IDE controller, whether we want it or not. */
     case VIR_DOMAIN_CONTROLLER_TYPE_IDE:
+        /* Since we currently only support the integrated IDE controller
+         * on 440fx, if we ever get to here, it's because some other
+         * machinetype had an IDE controller specified, or a 440fx had
+         * multiple ide controllers.
+         */
+        if (qemuDomainMachineIsI440FX(domainDef))
+            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                           _("Only a single IDE controller is unsupported "
+                             "for this machine type"));
+        else
+            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                           _("IDE controllers are unsupported for "
+                             "this QEMU binary or machine type"));
+        goto error;
+
     default:
         virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-                       _("Unknown controller type: %s"),
+                       _("Unsupported controller type: %s"),
                        virDomainControllerTypeToString(def->type));
         goto error;
     }
@@ -8629,20 +8643,21 @@ qemuBuildCommandLine(virConnectPtr conn,
          * List of controller types that we add commandline args for,
          * *in the order we want to add them*.
          *
-         * We don't add an explicit IDE or FD controller because the
+         * We don't add an explicit FD controller because the
          * provided PIIX4 device already includes one. It isn't possible to
          * remove the PIIX4.
          *
          * We don't add PCI/PCIe root controller either, because it's
          * implicit, but we do add PCI bridges and other PCI
          * controllers, so we leave that in to check each
-         * one. Likewise, we don't do anything for the primary SATA
-         * controller on q35, but we do add those beyond this one
-         * exception.
+         * one. Likewise, we don't do anything for the primary IDE
+         * controller on an i440fx machine or primary SATA on q35, but
+         * we do add those beyond these two exceptions.
          */
         VIR_DOMAIN_CONTROLLER_TYPE_PCI,
         VIR_DOMAIN_CONTROLLER_TYPE_USB,
         VIR_DOMAIN_CONTROLLER_TYPE_SCSI,
+        VIR_DOMAIN_CONTROLLER_TYPE_IDE,
         VIR_DOMAIN_CONTROLLER_TYPE_SATA,
         VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL,
         VIR_DOMAIN_CONTROLLER_TYPE_CCID,
@@ -9439,6 +9454,11 @@ qemuBuildCommandLine(virConnectPtr conn,
                     cont->idx == 0 && qemuDomainMachineIsQ35(def))
                         continue;
 
+                /* first IDE controller on i440fx machines is implicit */
+                if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE &&
+                    cont->idx == 0 && qemuDomainMachineIsI440FX(def))
+                        continue;
+
                 if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_USB &&
                     cont->model == -1 &&
                     !qemuDomainMachineIsQ35(def) &&

diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-blockio.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-blockio.args
index 33f87142f3ea6e42a1318062805b35bac88a84b0..79af23d1e3d119196052ce9fc5e7a215ddc3ca2d 100644 (file)
--- a/tests/qemuxml2argvdata/qemuxml2argv-disk-blockio.args
+++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-blockio.args
@@ -6,4 +6,4 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \
 -drive file=/tmp/idedisk.img,if=none,id=drive-ide0-0-2 \
 -device ide-hd,bus=ide.0,unit=2,drive=drive-ide0-0-2,id=ide0-0-2,\
 logical_block_size=512,physical_block_size=512 \
--device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x4
+-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3

diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-blockio.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-blockio.xml
index 52c9704cd2ba560b911c33e7e9d2cce8e94d7c5f..2b400c3d61fe7f15787c6cace40702e5fbef32c3 100644 (file)
--- a/tests/qemuxml2argvdata/qemuxml2argv-disk-blockio.xml
+++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-blockio.xml
@@ -27,7 +27,6 @@
     </disk>
     <controller type='usb' index='0'/>
     <controller type='ide' index='0'/>
-    <controller type='ide' index='1'/>
     <memballoon model='virtio'/>
   </devices>
 </domain>

diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-ide-drive-split.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-ide-drive-split.args
index 9ccdd5e2fd40ee60dda29a1ad1162aaa01eca564..4fe04b3233b86b4fd2190ea4c49869f2100e6720 100644 (file)
--- a/tests/qemuxml2argvdata/qemuxml2argv-disk-ide-drive-split.args
+++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-ide-drive-split.args
@@ -5,4 +5,4 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \
 -device ide-cd,bus=ide.0,unit=1,drive=drive-ide0-0-1,id=ide0-0-1 \
 -drive file=/tmp/idedisk.img,if=none,id=drive-ide0-0-2 \
 -device ide-hd,bus=ide.0,unit=2,drive=drive-ide0-0-2,id=ide0-0-2 \
--device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x4
+-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3

diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-ide-drive-split.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-ide-drive-split.xml
index 21c285b803315cc96510b1c40385ec5fe045b281..65c438bcd735c57435877071342c5a0c77468e6d 100644 (file)
--- a/tests/qemuxml2argvdata/qemuxml2argv-disk-ide-drive-split.xml
+++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-ide-drive-split.xml
@@ -26,7 +26,6 @@
     </disk>
     <controller type='usb' index='0'/>
     <controller type='ide' index='0'/>
-    <controller type='ide' index='1'/>
     <memballoon model='virtio'/>
   </devices>
 </domain>

diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.args
index 8f6a3dd9f6749df4aa4ab63f937ca7cfc9a22d21..75562946b6ad4fee409a1bcea91ffe1d9b64e7dc 100644 (file)
--- a/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.args
+++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.args
@@ -7,4 +7,4 @@ file=iscsi://iscsi.example.com:3260/demo-target/2,if=none,media=cdrom,id=drive-i
 -device ide-drive,bus=ide.0,unit=2,drive=drive-ide0-0-2,id=ide0-0-2 -drive \
 file=/tmp/idedisk.img,if=none,id=drive-ide0-0-3 -device \
 ide-drive,bus=ide.0,unit=3,drive=drive-ide0-0-3,id=ide0-0-3 -device \
-virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x4
+virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3

diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.xml
index c791717232b398f6b64ea4a08b59dd60bfcbe814..dcab1e9aa037f637aaebf7449508d050f98574b8 100644 (file)
--- a/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.xml
+++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.xml
@@ -41,7 +41,6 @@
     </disk>
     <controller type='usb' index='0'/>
     <controller type='ide' index='0'/>
-    <controller type='ide' index='1'/>
     <controller type='pci' index='0' model='pci-root'/>
     <memballoon model='virtio'/>
   </devices>

diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool.args
index 6b409b7a95534c0c3cbea4979fadda5f3113a4f2..f930e467ca1ca3904b397bc949f9bca6b8caf0f3 100644 (file)
--- a/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool.args
+++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool.args
@@ -7,4 +7,4 @@ if=none,media=cdrom,id=drive-ide0-1-0 -device \
 ide-drive,bus=ide.1,unit=0,drive=drive-ide0-1-0,id=ide0-1-0 -drive \
 file=/tmp/idedisk.img,if=none,id=drive-ide0-0-2 -device \
 ide-drive,bus=ide.0,unit=2,drive=drive-ide0-0-2,id=ide0-0-2 -device \
-virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x4
+virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3

diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool.xml
index ef095a0920766f5d19e70a52ae939784ed8df77f..19255c98fe4a442cfc93c5ab4f9fd8b4cdfd3f62 100644 (file)
--- a/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool.xml
+++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool.xml
@@ -39,7 +39,6 @@
     </disk>
     <controller type='usb' index='0'/>
     <controller type='ide' index='0'/>
-    <controller type='ide' index='1'/>
     <controller type='pci' index='0' model='pci-root'/>
     <memballoon model='virtio'/>
   </devices>

diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-source-pool.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-source-pool.xml
index 31e4928caf686b481735f6c41111b0eea4cfc39c..d3c8b692b1d794e267b8e2d4e4d682d829028756 100644 (file)
--- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-source-pool.xml
+++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-source-pool.xml
@@ -37,7 +37,6 @@
     </disk>
     <controller type='usb' index='0'/>
     <controller type='ide' index='0'/>
-    <controller type='ide' index='1'/>
     <controller type='pci' index='0' model='pci-root'/>
     <memballoon model='virtio'/>
   </devices>