[libvirt] [PATCH 11/13] qemu: log error when domain has an upsupported IDE controller
Laine Stump
laine at laine.org
Tue May 5 18:03:16 UTC 2015
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 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).
libvirt should really be saying what it means, and meaning what it
says, and that's what this patch does - 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
rearranging the loop that creates controller command strings in
qemuBuildCommandline() so that it also calls
qemuBuildControllerDevStr() for IDE controllers unless it is the
primary controller on an i440fx machine (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.
(note that the body of the controller loop in qemuBuildCommandline()
was cleaned up in the process to eliminate duplicated code, but other
than the addition of calling qemuBuildControllerDevStr() for IDE, it
is functionally unchanged).
---
src/qemu/qemu_command.c | 101 ++++++++++++---------
.../qemuxml2argv-disk-blockio.args | 2 +-
.../qemuxml2argvdata/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, 63 insertions(+), 51 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 7f7f3f4..72d425f 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -4573,6 +4573,12 @@ qemuBuildControllerDevStr(virDomainDefPtr domainDef,
break;
case VIR_DOMAIN_CONTROLLER_TYPE_SATA:
+ if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_ICH9_AHCI)) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("SATA is not supported with this "
+ "QEMU binary"));
+ goto error;
+ }
virBufferAsprintf(&buf, "ahci,id=%s", def->info.alias);
break;
@@ -4618,11 +4624,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;
}
@@ -8619,15 +8639,25 @@ qemuBuildCommandLine(virConnectPtr conn,
bool usblegacy = false;
bool mlock = false;
int contOrder[] = {
- /* We don't add an explicit IDE or FD controller because the
+ /*
+ * List of controller types that we add commandline args for,
+ * *in the order we want to add them*.
+ *
+ * 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 root controller either, because it's implicit,
- * but we do add PCI bridges. */
+ * 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 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,
@@ -9401,11 +9431,12 @@ qemuBuildCommandLine(virConnectPtr conn,
for (j = 0; j < ARRAY_CARDINALITY(contOrder); j++) {
for (i = 0; i < def->ncontrollers; i++) {
virDomainControllerDefPtr cont = def->controllers[i];
+ char *devstr;
if (cont->type != contOrder[j])
continue;
- /* Also, skip USB controllers with type none.*/
+ /* skip USB controllers with type none.*/
if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_USB &&
cont->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE) {
usbcontroller = -1; /* mark we don't want a controller */
@@ -9415,37 +9446,25 @@ qemuBuildCommandLine(virConnectPtr conn,
/* Skip pci-root/pcie-root */
if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI &&
(cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT ||
- cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT)) {
+ cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT))
continue;
- }
- /* Only recent QEMU implements a SATA (AHCI) controller */
- if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_SATA) {
- if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_ICH9_AHCI)) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("SATA is not supported with this "
- "QEMU binary"));
- goto error;
- } else if (cont->idx == 0 && qemuDomainMachineIsQ35(def)) {
- /* first SATA controller on Q35 machines is implicit */
+ /* first SATA controller on Q35 machines is implicit */
+ if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_SATA &&
+ cont->idx == 0 && qemuDomainMachineIsQ35(def))
continue;
- } else {
- char *devstr;
- virCommandAddArg(cmd, "-device");
- if (!(devstr = qemuBuildControllerDevStr(def, cont,
- qemuCaps, NULL)))
- goto error;
+ /* first IDE controller on i440fx machines is implicit */
+ if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE &&
+ cont->idx == 0 && qemuDomainMachineIsI440FX(def))
+ continue;
- virCommandAddArg(cmd, devstr);
- VIR_FREE(devstr);
- }
- } else if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_USB &&
- cont->model == -1 &&
- !qemuDomainMachineIsQ35(def) &&
- (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_PIIX3_USB_UHCI) ||
- (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_PCI_OHCI) &&
- ARCH_IS_PPC64(def->os.arch)))) {
+ if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_USB &&
+ cont->model == -1 &&
+ !qemuDomainMachineIsQ35(def) &&
+ (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_PIIX3_USB_UHCI) ||
+ (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_PCI_OHCI) &&
+ ARCH_IS_PPC64(def->os.arch)))) {
if (usblegacy) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("Multiple legacy USB controllers are "
@@ -9453,17 +9472,15 @@ qemuBuildCommandLine(virConnectPtr conn,
goto error;
}
usblegacy = true;
- } else {
- virCommandAddArg(cmd, "-device");
-
- char *devstr;
- if (!(devstr = qemuBuildControllerDevStr(def, cont, qemuCaps,
- &usbcontroller)))
- goto error;
-
- virCommandAddArg(cmd, devstr);
- VIR_FREE(devstr);
+ continue;
}
+
+ virCommandAddArg(cmd, "-device");
+ if (!(devstr = qemuBuildControllerDevStr(def, cont, qemuCaps,
+ &usbcontroller)))
+ goto error;
+ virCommandAddArg(cmd, devstr);
+ VIR_FREE(devstr);
}
}
}
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-blockio.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-blockio.args
index 33f8714..79af23d 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 52c9704..2b400c3 100644
--- 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 9ccdd5e..4fe04b3 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 21c285b..65c438b 100644
--- 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 8f6a3dd..7556294 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 c791717..dcab1e9 100644
--- 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 6b409b7..f930e46 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 ef095a0..19255c9 100644
--- 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 31e4928..d3c8b69 100644
--- 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>
--
2.1.0
More information about the libvir-list
mailing list