From eadd757ccee0e72156a5345e6c25477aa057a9f1 Mon Sep 17 00:00:00 2001 From: Laine Stump Date: Thu, 30 Apr 2015 16:59:04 -0400 Subject: [PATCH] qemu: log error when domain has an unsupported IDE controller We have previously effectively ignored all 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) --- src/qemu/qemu_command.c | 32 +++++++++++++++---- .../qemuxml2argv-disk-blockio.args | 2 +- .../qemuxml2argv-disk-blockio.xml | 1 - .../qemuxml2argv-disk-ide-drive-split.args | 2 +- .../qemuxml2argv-disk-ide-drive-split.xml | 1 - .../qemuxml2argv-disk-source-pool-mode.args | 2 +- .../qemuxml2argv-disk-source-pool-mode.xml | 1 - .../qemuxml2argv-disk-source-pool.args | 2 +- .../qemuxml2argv-disk-source-pool.xml | 1 - .../qemuxml2xmlout-disk-source-pool.xml | 1 - 10 files changed, 30 insertions(+), 15 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index c6962d5d9..b231d65b2 100644 --- 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 33f87142f..79af23d1e 100644 --- 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 52c9704cd..2b400c3d6 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-disk-blockio.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-blockio.xml @@ -27,7 +27,6 @@ - diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-ide-drive-split.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-ide-drive-split.args index 9ccdd5e2f..4fe04b323 100644 --- 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 21c285b80..65c438bcd 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-disk-ide-drive-split.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-ide-drive-split.xml @@ -26,7 +26,6 @@ - diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.args index 8f6a3dd9f..75562946b 100644 --- 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 c79171723..dcab1e9aa 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.xml @@ -41,7 +41,6 @@ - diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool.args index 6b409b7a9..f930e467c 100644 --- 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 ef095a092..19255c98f 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool.xml @@ -39,7 +39,6 @@ - diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-source-pool.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-source-pool.xml index 31e4928ca..d3c8b692b 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-source-pool.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-source-pool.xml @@ -37,7 +37,6 @@ - -- 2.39.5