2:36 p.m.
V1 was here:
http://www.redhat.com/archives/libvir-list/2015-May/msg00124.html
This started out with the intent to generate an error/failure on
request for an IDE controller on a machinetype that doesn't support
IDE (currently anything except 440fx-based machinetypes), or a 2nd IDE
controller on a machinetype that only supports one (i.e. 440fx). This
led to a few other related fixes, and some "fixes related to the
related fixes".
I've pushed some of the ACKed patches, and dropped the two patches
that changed qemuBuildDeviceAddress and qemuAssignDeviceControllerAlias
to use switches instead of "if .. else if .." (03/13 and 04/13) and
dropped the two SCSI patches (12/13 and 13/13), but left a couple of
ACKed patches in for clarity.
Detailed differences from V1 in each patch.
Laine Stump (9):
conf: utility to return alias of a controller based on type/index
qemu: fix exceptions in qemuAssignDeviceControllerAlias
qemu: use controller alias when constructing device/controller args
qemu: use alias to refer to non-multibus PCI controller
qemu: use controller's alias in commandline for scsi-generic
qemu: use USB controller's alias instead of qemuUSBId()
qemu: remove test for allowing ide controller in s390, rename usb
tests
qemu: clean up qemuBuildCommandline loop that builds controller args
qemu: log error when domain has an unsupported IDE controller
src/conf/domain_conf.c | 34 +++
src/conf/domain_conf.h | 3 +
src/libvirt_private.syms | 1 +
src/qemu/qemu_command.c | 317 +++++++++++----------
src/qemu/qemu_command.h | 6 +-
src/qemu/qemu_hotplug.c | 4 +-
.../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-sata-device.args | 4 +-
.../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 -
...uxml2argv-s390-allow-bogus-usb-controller.args} | 0
...muxml2argv-s390-allow-bogus-usb-controller.xml} | 3 -
...=> qemuxml2argv-s390-allow-bogus-usb-none.args} | 0
... => qemuxml2argv-s390-allow-bogus-usb-none.xml} | 0
tests/qemuxml2argvtest.c | 4 +-
.../qemuxml2xmlout-disk-source-pool.xml | 1 -
21 files changed, 226 insertions(+), 163 deletions(-)
rename tests/qemuxml2argvdata/{qemuxml2argv-s390-piix-controllers.args =>
qemuxml2argv-s390-allow-bogus-usb-controller.args} (100%)
rename tests/qemuxml2argvdata/{qemuxml2argv-s390-piix-controllers.xml =>
qemuxml2argv-s390-allow-bogus-usb-controller.xml} (86%)
rename tests/qemuxml2argvdata/{qemuxml2argv-s390-usb-none.args =>
qemuxml2argv-s390-allow-bogus-usb-none.args} (100%)
rename tests/qemuxml2argvdata/{qemuxml2argv-s390-usb-none.xml =>
qemuxml2argv-s390-allow-bogus-usb-none.xml} (100%)
--
2.1.0
2:36 p.m.
New subject: [libvirt] [PATCHv2 1/9] conf: utility to return alias of a controller based on type/index
Because there are multiple potential reasons for an error, this
function logs any errors before returning NULL (since the caller won't
have the information needed to determine which was the reason for
failure).
---
(this was 05/13 in V1)
No diff from V1, already ACKed, included for continuity/clarity.
src/conf/domain_conf.c | 34 ++++++++++++++++++++++++++++++++++
src/conf/domain_conf.h | 3 +++
src/libvirt_private.syms | 1 +
3 files changed, 38 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index d150582..4d3620e 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -12535,6 +12535,40 @@ virDomainControllerFind(virDomainDefPtr def,
return -1;
}
+
+const char *
+virDomainControllerAliasFind(virDomainDefPtr def,
+ int type, int idx)
+{
+ int contIndex;
+ const char *contTypeStr = virDomainControllerTypeToString(type);
+
+ if (!contTypeStr) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Unknown controller type %d"),
+ type);
+ return NULL;
+ }
+
+ contIndex = virDomainControllerFind(def, type, idx);
+ if (contIndex < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not find %s controller with index %d "
+ "required for device"),
+ contTypeStr, idx);
+ return NULL;
+ }
+ if (!def->controllers[contIndex]->info.alias) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Device alias was not set for %s controller "
+ "with index %d "),
+ contTypeStr, idx);
+ return NULL;
+ }
+ return def->controllers[contIndex]->info.alias;
+}
+
+
int
virDomainControllerFindByType(virDomainDefPtr def,
int type)
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 2cd105a7..2527ff4 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2754,6 +2754,9 @@ int virDomainControllerFindByType(virDomainDefPtr def, int type);
int virDomainControllerFindByPCIAddress(virDomainDefPtr def,
virDevicePCIAddressPtr addr);
virDomainControllerDefPtr virDomainControllerRemove(virDomainDefPtr def, size_t i);
+const char *virDomainControllerAliasFind(virDomainDefPtr def,
+ int type, int idx)
+ ATTRIBUTE_NONNULL(1);
int virDomainLeaseIndex(virDomainDefPtr def,
virDomainLeaseDefPtr lease);
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 6e33f84..74847d7 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -181,6 +181,7 @@ virDomainClockBasisTypeToString;
virDomainClockOffsetTypeFromString;
virDomainClockOffsetTypeToString;
virDomainConfigFile;
+virDomainControllerAliasFind;
virDomainControllerDefFree;
virDomainControllerFind;
virDomainControllerFindByType;
--
2.1.0
2:36 p.m.
New subject: [libvirt] [PATCHv2 2/9] qemu: fix exceptions in qemuAssignDeviceControllerAlias
There are a few extra exceptions that weren't being accounted for when
creating the alias for a controller. This resulted in 1) incorrect
status XML, and 2) exceptions/printfs of what *should* have been
directly available in the controller alias when constructing device
commandline arguments:
1) The primary (and only) IDE controller on a 440FX machinetype is
hardcoded to be "ide" in qemu.
2) The primary SATA controller on a 440FX machinetype is also
hardcoded to be "ide" in qemu.
3) On machinetypes that don't support multiple PCI buses, the PCI bus
is hardcoded in qemu to have the name "pci".
4) The first usb master controller is "usb", all others are the normal
"usb%d". (note that usb controllers that are not a "master" will have
the same index, and thus alias, as the master).
We needed to pass in the full domainDef and qemuCaps in order to
properly make the decisions about these exceptions.
---
Changes from V1: (this was 06/13 in V1)
* no longer changed into switch statement, since so few of the types
actually need an exception.
* Added exception for USB controller aliases. (thanks for pointing
that one out, John!)
src/qemu/qemu_command.c | 53 +++++++++++++++++++++++++++++++++++++++----------
src/qemu/qemu_command.h | 5 ++++-
src/qemu/qemu_hotplug.c | 4 ++--
3 files changed, 48 insertions(+), 14 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 5d0a167..a75664c 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1031,21 +1031,52 @@ qemuAssignDeviceRedirdevAlias(virDomainDefPtr def,
virDomainRedirdevDefPtr redir
int
-qemuAssignDeviceControllerAlias(virDomainControllerDefPtr controller)
+qemuAssignDeviceControllerAlias(virDomainDefPtr domainDef,
+ virQEMUCapsPtr qemuCaps,
+ virDomainControllerDefPtr controller)
{
const char *prefix = virDomainControllerTypeToString(controller->type);
if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) {
- /* only pcie-root uses a different naming convention
- * ("pcie.0"), because it is hardcoded that way in qemu. All
- * other buses use the consistent "pci.%u".
- */
- if (controller->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT)
+ if (!virQEMUCapsHasPCIMultiBus(qemuCaps, domainDef)) {
+ /* qemus that don't support multiple PCI buses have
+ * hardcoded the name of their single PCI controller as
+ * "pci".
+ */
+ return VIR_STRDUP(controller->info.alias, "pci");
+ } else if (controller->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) {
+ /* The pcie-root controller on Q35 machinetypes uses a
+ * different naming convention ("pcie.0"), because it is
+ * hardcoded that way in qemu.
+ */
return virAsprintf(&controller->info.alias, "pcie.%d",
controller->idx);
- else
- return virAsprintf(&controller->info.alias, "pci.%d",
controller->idx);
- }
-
+ }
+ /* All other PCI controllers use the consistent "pci.%u"
+ * (including the hardcoded pci-root controller on
+ * multibus-capable qemus).
+ */
+ return virAsprintf(&controller->info.alias, "pci.%d",
controller->idx);
+ } else if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE) {
+ /* for any machine based on I440FX, the first (and currently
+ * only) IDE controller is an integrated controller hardcoded
+ * with id "ide"
+ */
+ if (qemuDomainMachineIsI440FX(domainDef) && controller->idx == 0)
+ return VIR_STRDUP(controller->info.alias, "ide");
+ } else if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_SATA) {
+ /* for any Q35 machine, the first SATA controller is the
+ * integrated one, and it too is hardcoded with id "ide"
+ */
+ if (qemuDomainMachineIsQ35(domainDef) && controller->idx == 0)
+ return VIR_STRDUP(controller->info.alias, "ide");
+ } else if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_USB) {
+ /* first USB device is "usb", others are normal "usb%d" */
+ if (controller->idx == 0)
+ return VIR_STRDUP(controller->info.alias, "usb");
+ }
+ /* all other controllers use the default ${type}${index} naming
+ * scheme for alias/id.
+ */
return virAsprintf(&controller->info.alias, "%s%d", prefix,
controller->idx);
}
@@ -1174,7 +1205,7 @@ qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr
qemuCaps)
return -1;
}
for (i = 0; i < def->ncontrollers; i++) {
- if (qemuAssignDeviceControllerAlias(def->controllers[i]) < 0)
+ if (qemuAssignDeviceControllerAlias(def, qemuCaps, def->controllers[i]) <
0)
return -1;
}
for (i = 0; i < def->ninputs; i++) {
diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h
index 675eb62..9ef7046 100644
--- a/src/qemu/qemu_command.h
+++ b/src/qemu/qemu_command.h
@@ -287,7 +287,10 @@ int qemuAssignDeviceDiskAlias(virDomainDefPtr vmdef,
virDomainDiskDefPtr def,
virQEMUCapsPtr qemuCaps);
int qemuAssignDeviceHostdevAlias(virDomainDefPtr def, virDomainHostdevDefPtr hostdev, int
idx);
-int qemuAssignDeviceControllerAlias(virDomainControllerDefPtr controller);
+int
+qemuAssignDeviceControllerAlias(virDomainDefPtr domainDef,
+ virQEMUCapsPtr qemuCaps,
+ virDomainControllerDefPtr controller);
int qemuAssignDeviceRedirdevAlias(virDomainDefPtr def, virDomainRedirdevDefPtr redirdev,
int idx);
int qemuAssignDeviceChrAlias(virDomainDefPtr def,
virDomainChrDefPtr chr,
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index fc45de1..4c743bf 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -465,7 +465,7 @@ int qemuDomainAttachControllerDevice(virQEMUDriverPtr driver,
goto cleanup;
}
releaseaddr = true;
- if (qemuAssignDeviceControllerAlias(controller) < 0)
+ if (qemuAssignDeviceControllerAlias(vm->def, priv->qemuCaps, controller)
< 0)
goto cleanup;
if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_USB &&
@@ -3639,7 +3639,7 @@ int qemuDomainDetachControllerDevice(virQEMUDriverPtr driver,
if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE) &&
!detach->info.alias) {
- if (qemuAssignDeviceControllerAlias(detach) < 0)
+ if (qemuAssignDeviceControllerAlias(vm->def, priv->qemuCaps, detach) <
0)
goto cleanup;
}
--
2.1.0
2:36 p.m.
New subject: [libvirt] [PATCHv2 3/9] qemu: use controller alias when constructing device/controller args
This makes sure that that the commandlines generated for devices and
controller devices are all using the alias that has been set in the
controller's object as the id of the controller, rather than
hardcoding a printf (or worse, encoding exceptions to the standard
${controller}${index} into the logic)
Since this "fixes" the controller name used for the sata controller,
the commandline arg for the sata controller in the sata test case had
to be adjusted to be "sata0" instead of "ahci0".
Because the function that finds a controller alias based on a device
def requires a pointer to the full domainDef in order to get the list
of controllers, the arglist of a few functions had to have this added.
---
Changes from V1:
This is the Patches 07/13 08/13 and 09/13 of V1 merged together on
John's suggestion.
src/qemu/qemu_command.c | 85 +++++++++++-----------
src/qemu/qemu_command.h | 1 -
.../qemuxml2argv-disk-sata-device.args | 4 +-
3 files changed, 46 insertions(+), 44 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index a75664c..a3413ee 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -4014,6 +4014,7 @@ qemuBuildDriveDevStr(virDomainDefPtr def,
{
virBuffer opt = VIR_BUFFER_INITIALIZER;
const char *bus = virDomainDiskQEMUBusTypeToString(disk->bus);
+ const char *contAlias;
int controllerModel;
if (virDomainDiskDefDstDuplicates(def))
@@ -4061,7 +4062,11 @@ qemuBuildDriveDevStr(virDomainDefPtr def,
virBufferAddLit(&opt, "ide-drive");
}
- virBufferAsprintf(&opt, ",bus=ide.%d,unit=%d",
+ if (!(contAlias = virDomainControllerAliasFind(def,
VIR_DOMAIN_CONTROLLER_TYPE_IDE,
+
disk->info.addr.drive.controller)))
+ goto error;
+ virBufferAsprintf(&opt, ",bus=%s.%d,unit=%d",
+ contAlias,
disk->info.addr.drive.bus,
disk->info.addr.drive.unit);
break;
@@ -4114,6 +4119,10 @@ qemuBuildDriveDevStr(virDomainDefPtr def,
}
}
+ if (!(contAlias = virDomainControllerAliasFind(def,
VIR_DOMAIN_CONTROLLER_TYPE_SCSI,
+
disk->info.addr.drive.controller)))
+ goto error;
+
if (controllerModel == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSILOGIC) {
if (disk->info.addr.drive.target != 0) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
@@ -4122,8 +4131,8 @@ qemuBuildDriveDevStr(virDomainDefPtr def,
goto error;
}
- virBufferAsprintf(&opt, ",bus=scsi%d.%d,scsi-id=%d",
- disk->info.addr.drive.controller,
+ virBufferAsprintf(&opt, ",bus=%s.%d,scsi-id=%d",
+ contAlias,
disk->info.addr.drive.bus,
disk->info.addr.drive.unit);
} else {
@@ -4144,8 +4153,8 @@ qemuBuildDriveDevStr(virDomainDefPtr def,
}
}
- virBufferAsprintf(&opt,
",bus=scsi%d.0,channel=%d,scsi-id=%d,lun=%d",
- disk->info.addr.drive.controller,
+ virBufferAsprintf(&opt,
",bus=%s.0,channel=%d,scsi-id=%d,lun=%d",
+ contAlias,
disk->info.addr.drive.bus,
disk->info.addr.drive.target,
disk->info.addr.drive.unit);
@@ -4173,23 +4182,12 @@ qemuBuildDriveDevStr(virDomainDefPtr def,
virBufferAddLit(&opt, "ide-drive");
}
- if (qemuDomainMachineIsQ35(def) &&
- disk->info.addr.drive.controller == 0) {
- /* Q35 machines have an implicit ahci (sata) controller at
- * 00:1F.2 which for some reason is hardcoded with the id
- * "ide" instead of the seemingly more reasonable
"ahci0"
- * or "sata0".
- */
- virBufferAsprintf(&opt, ",bus=ide.%d",
disk->info.addr.drive.unit);
- } else {
- /* All other ahci controllers have been created by
- * libvirt, and we gave them the id "ahci${n}" where ${n}
- * is the controller number. So we identify them that way.
- */
- virBufferAsprintf(&opt, ",bus=ahci%d.%d",
- disk->info.addr.drive.controller,
- disk->info.addr.drive.unit);
- }
+ if (!(contAlias = virDomainControllerAliasFind(def,
VIR_DOMAIN_CONTROLLER_TYPE_SATA,
+
disk->info.addr.drive.controller)))
+ goto error;
+ virBufferAsprintf(&opt, ",bus=%s.%d",
+ contAlias,
+ disk->info.addr.drive.unit);
break;
case VIR_DOMAIN_DISK_BUS_VIRTIO:
@@ -4541,7 +4539,7 @@ qemuBuildControllerDevStr(virDomainDefPtr domainDef,
_("Unsupported controller model: %s"),
virDomainControllerModelSCSITypeToString(def->model));
}
- virBufferAsprintf(&buf, ",id=scsi%d", def->idx);
+ virBufferAsprintf(&buf, ",id=%s", def->info.alias);
break;
case VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL:
@@ -4559,8 +4557,7 @@ qemuBuildControllerDevStr(virDomainDefPtr domainDef,
} else {
virBufferAddLit(&buf, "virtio-serial");
}
- virBufferAsprintf(&buf, ",id=" QEMU_VIRTIO_SERIAL_PREFIX
"%d",
- def->idx);
+ virBufferAsprintf(&buf, ",id=%s", def->info.alias);
if (def->opts.vioserial.ports != -1) {
virBufferAsprintf(&buf, ",max_ports=%d",
def->opts.vioserial.ports);
@@ -4572,11 +4569,11 @@ qemuBuildControllerDevStr(virDomainDefPtr domainDef,
break;
case VIR_DOMAIN_CONTROLLER_TYPE_CCID:
- virBufferAsprintf(&buf, "usb-ccid,id=ccid%d", def->idx);
+ virBufferAsprintf(&buf, "usb-ccid,id=%s", def->info.alias);
break;
case VIR_DOMAIN_CONTROLLER_TYPE_SATA:
- virBufferAsprintf(&buf, "ahci,id=ahci%d", def->idx);
+ virBufferAsprintf(&buf, "ahci,id=%s", def->info.alias);
break;
case VIR_DOMAIN_CONTROLLER_TYPE_USB:
@@ -4596,8 +4593,8 @@ qemuBuildControllerDevStr(virDomainDefPtr domainDef,
_("PCI bridge index should be > 0"));
goto error;
}
- virBufferAsprintf(&buf, "pci-bridge,chassis_nr=%d,id=pci.%d",
- def->idx, def->idx);
+ virBufferAsprintf(&buf, "pci-bridge,chassis_nr=%d,id=%s",
+ def->idx, def->info.alias);
break;
case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE:
if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE)) {
@@ -4611,7 +4608,7 @@ qemuBuildControllerDevStr(virDomainDefPtr domainDef,
_("dmi-to-pci-bridge index should be > 0"));
goto error;
}
- virBufferAsprintf(&buf, "i82801b11-bridge,id=pci.%d",
def->idx);
+ virBufferAsprintf(&buf, "i82801b11-bridge,id=%s",
def->info.alias);
break;
case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT:
case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT:
@@ -6311,10 +6308,13 @@ qemuBuildChrArgStr(virDomainChrSourceDefPtr dev, const char
*prefix)
static char *
-qemuBuildVirtioSerialPortDevStr(virDomainChrDefPtr dev,
+qemuBuildVirtioSerialPortDevStr(virDomainDefPtr def,
+ virDomainChrDefPtr dev,
virQEMUCapsPtr qemuCaps)
{
virBuffer buf = VIR_BUFFER_INITIALIZER;
+ const char *contAlias;
+
switch (dev->deviceType) {
case VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE:
virBufferAddLit(&buf, "virtconsole");
@@ -6346,12 +6346,13 @@ qemuBuildVirtioSerialPortDevStr(virDomainChrDefPtr dev,
goto error;
}
- virBufferAsprintf(&buf,
- ",bus=" QEMU_VIRTIO_SERIAL_PREFIX "%d.%d",
- dev->info.addr.vioserial.controller,
- dev->info.addr.vioserial.bus);
- virBufferAsprintf(&buf,
- ",nr=%d",
+ contAlias = virDomainControllerAliasFind(def,
VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL,
+
dev->info.addr.vioserial.controller);
+ if (!contAlias)
+ goto error;
+
+ virBufferAsprintf(&buf, ",bus=%s.%d,nr=%d", contAlias,
+ dev->info.addr.vioserial.bus,
dev->info.addr.vioserial.port);
}
@@ -10912,6 +10913,7 @@ qemuBuildParallelChrDeviceStr(char **deviceStr,
static int
qemuBuildChannelChrDeviceStr(char **deviceStr,
+ virDomainDefPtr def,
virDomainChrDefPtr chr,
virQEMUCapsPtr qemuCaps)
{
@@ -10934,7 +10936,7 @@ qemuBuildChannelChrDeviceStr(char **deviceStr,
break;
case VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO:
- if (!(*deviceStr = qemuBuildVirtioSerialPortDevStr(chr, qemuCaps)))
+ if (!(*deviceStr = qemuBuildVirtioSerialPortDevStr(def, chr, qemuCaps)))
goto cleanup;
break;
@@ -10951,6 +10953,7 @@ qemuBuildChannelChrDeviceStr(char **deviceStr,
static int
qemuBuildConsoleChrDeviceStr(char **deviceStr,
+ virDomainDefPtr def,
virDomainChrDefPtr chr,
virQEMUCapsPtr qemuCaps)
{
@@ -10964,7 +10967,7 @@ qemuBuildConsoleChrDeviceStr(char **deviceStr,
break;
case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_VIRTIO:
- if (!(*deviceStr = qemuBuildVirtioSerialPortDevStr(chr, qemuCaps)))
+ if (!(*deviceStr = qemuBuildVirtioSerialPortDevStr(def, chr, qemuCaps)))
goto cleanup;
break;
@@ -11008,11 +11011,11 @@ qemuBuildChrDeviceStr(char **deviceStr,
break;
case VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL:
- ret = qemuBuildChannelChrDeviceStr(deviceStr, chr, qemuCaps);
+ ret = qemuBuildChannelChrDeviceStr(deviceStr, vmdef, chr, qemuCaps);
break;
case VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE:
- ret = qemuBuildConsoleChrDeviceStr(deviceStr, chr, qemuCaps);
+ ret = qemuBuildConsoleChrDeviceStr(deviceStr, vmdef, chr, qemuCaps);
break;
case VIR_DOMAIN_CHR_DEVICE_TYPE_LAST:
diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h
index 9ef7046..0fc59a8 100644
--- a/src/qemu/qemu_command.h
+++ b/src/qemu/qemu_command.h
@@ -36,7 +36,6 @@
# define QEMU_CONFIG_FORMAT_ARGV "qemu-argv"
# define QEMU_DRIVE_HOST_PREFIX "drive-"
-# define QEMU_VIRTIO_SERIAL_PREFIX "virtio-serial"
# define QEMU_FSDEV_HOST_PREFIX "fsdev-"
/* These are only defaults, they can be changed now in qemu.conf and
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-sata-device.args
b/tests/qemuxml2argvdata/qemuxml2argv-disk-sata-device.args
index 475a0b1..7ae610f 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-disk-sata-device.args
+++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-sata-device.args
@@ -1,7 +1,7 @@
LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \
/usr/bin/qemu -S -M \
pc -m 214 -smp 1 -nographic -nodefconfig -nodefaults -monitor \
-unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -device ahci,id=ahci0,\
+unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -device ahci,id=sata0,\
bus=pci.0,addr=0x3 -usb -drive file=/dev/HostVG/QEMUGuest1,if=none,\
-id=drive-sata0-0-0 -device ide-drive,bus=ahci0.0,drive=drive-sata0-0-0,\
+id=drive-sata0-0-0 -device ide-drive,bus=sata0.0,drive=drive-sata0-0-0,\
id=sata0-0-0 -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x4
--
2.1.0
2:36 p.m.
New subject: [libvirt] [PATCHv2 4/9] qemu: use alias to refer to non-multibus PCI controller
Another example of using a literal rather than the controller's alias
in qemuBuildDeviceAddressStr(). In the past this was necessary because
the pci controllers on non-multibus capable machinetypes had incorrect
aliases; that problem is now fixed in
qemuAssignDeviceControllerAlias() so we can eliminate the extra code
here.
---
This is new in V2. It will be merged into 3/9 before pushing, but is
split out here for easy review.
src/qemu/qemu_command.c | 31 ++++++++-----------------------
1 file changed, 8 insertions(+), 23 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index a3413ee..4c7c2b2 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -2664,30 +2664,15 @@ qemuBuildDeviceAddressStr(virBufferPtr buf,
}
}
- /*
- * PCI bridge support is required for multiple buses
- * 'pci.%u' is the ID of the bridge as specified in
- * qemuBuildControllerDevStr
- *
- * PCI_MULTIBUS capability indicates that the implicit
- * PCI bus is named 'pci.0' instead of 'pci'.
- */
- if (info->addr.pci.bus != 0) {
- if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PCI_BRIDGE)) {
- virBufferAsprintf(buf, ",bus=%s", contAlias);
- } else {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("Multiple PCI buses are not supported "
- "with this QEMU binary"));
- goto cleanup;
- }
- } else {
- if (virQEMUCapsHasPCIMultiBus(qemuCaps, domainDef)) {
- virBufferAsprintf(buf, ",bus=%s", contAlias);
- } else {
- virBufferAddLit(buf, ",bus=pci");
- }
+ if (info->addr.pci.bus != 0 &&
+ !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PCI_BRIDGE)) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Multiple PCI buses are not supported "
+ "with this QEMU binary"));
+ goto cleanup;
}
+ virBufferAsprintf(buf, ",bus=%s", contAlias);
+
if (info->addr.pci.multi == VIR_TRISTATE_SWITCH_ON)
virBufferAddLit(buf, ",multifunction=on");
else if (info->addr.pci.multi == VIR_TRISTATE_SWITCH_OFF)
--
2.1.0
2:36 p.m.
New subject: [libvirt] [PATCHv2 5/9] qemu: use controller's alias in commandline for scsi-generic
---
Pointed out by John during review.
This is new in V2. It will be merged into 3/9 before pushing, but is
split out here for easy review.
src/qemu/qemu_command.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 4c7c2b2..81305c1 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -6024,6 +6024,7 @@ qemuBuildSCSIHostdevDevStr(virDomainDefPtr def,
{
virBuffer buf = VIR_BUFFER_INITIALIZER;
int model = -1;
+ const char *contAlias;
model = virDomainDeviceFindControllerModel(def, dev->info,
VIR_DOMAIN_CONTROLLER_TYPE_SCSI);
@@ -6049,14 +6050,18 @@ qemuBuildSCSIHostdevDevStr(virDomainDefPtr def,
virBufferAddLit(&buf, "scsi-generic");
+ if (!(contAlias = virDomainControllerAliasFind(def, VIR_DOMAIN_CONTROLLER_TYPE_SCSI,
+
dev->info->addr.drive.controller)))
+ goto error;
+
if (model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSILOGIC) {
- virBufferAsprintf(&buf, ",bus=scsi%d.%d,scsi-id=%d",
- dev->info->addr.drive.controller,
+ virBufferAsprintf(&buf, ",bus=%s.%d,scsi-id=%d",
+ contAlias,
dev->info->addr.drive.bus,
dev->info->addr.drive.unit);
} else {
- virBufferAsprintf(&buf,
",bus=scsi%d.0,channel=%d,scsi-id=%d,lun=%d",
- dev->info->addr.drive.controller,
+ virBufferAsprintf(&buf, ",bus=%s.0,channel=%d,scsi-id=%d,lun=%d",
+ contAlias,
dev->info->addr.drive.bus,
dev->info->addr.drive.target,
dev->info->addr.drive.unit);
--
2.1.0
2:36 p.m.
New subject: [libvirt] [PATCHv2 6/9] qemu: use USB controller's alias instead of qemuUSBId()
qemuUSBId() is just constructing what should already be in the alias
for the USB controller that has the same index as the bus number
listed for the device being connected, so replace calls to qemuUSBId()
with the controller's alias.
---
Pointed out by John during review.
This is new in V2. It will be merged into 3/9 before pushing, but is
split out here for easy review.
src/qemu/qemu_command.c | 32 +++++++++++---------------------
1 file changed, 11 insertions(+), 21 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 81305c1..eead482 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -2599,15 +2599,6 @@ qemuAssignDevicePCISlots(virDomainDefPtr def,
return -1;
}
-static void
-qemuUSBId(virBufferPtr buf, int idx)
-{
- if (idx == 0)
- virBufferAddLit(buf, "usb");
- else
- virBufferAsprintf(buf, "usb%d", idx);
-}
-
static int
qemuBuildDeviceAddressStr(virBufferPtr buf,
virDomainDefPtr domainDef,
@@ -2616,9 +2607,9 @@ qemuBuildDeviceAddressStr(virBufferPtr buf,
{
int ret = -1;
char *devStr = NULL;
+ const char *contAlias = NULL;
if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) {
- const char *contAlias = NULL;
size_t i;
if (!(devStr = virDomainPCIAddressAsString(&info->addr.pci)))
@@ -2681,9 +2672,11 @@ qemuBuildDeviceAddressStr(virBufferPtr buf,
if (info->addr.pci.function != 0)
virBufferAsprintf(buf, ".0x%x", info->addr.pci.function);
} else if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB) {
- virBufferAddLit(buf, ",bus=");
- qemuUSBId(buf, info->addr.usb.bus);
- virBufferAsprintf(buf, ".0,port=%s", info->addr.usb.port);
+ if (!(contAlias = virDomainControllerAliasFind(domainDef,
+ VIR_DOMAIN_CONTROLLER_TYPE_USB,
+ info->addr.usb.bus)))
+ goto cleanup;
+ virBufferAsprintf(buf, ",bus=%s.0,port=%s", contAlias,
info->addr.usb.port);
} else if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO) {
if (info->addr.spaprvio.has_reg)
virBufferAsprintf(buf, ",reg=0x%llx", info->addr.spaprvio.reg);
@@ -4450,14 +4443,11 @@ qemuBuildUSBControllerDevStr(virDomainDefPtr domainDef,
virBufferAsprintf(buf, "%s", smodel);
- if (def->info.mastertype == VIR_DOMAIN_CONTROLLER_MASTER_USB) {
- virBufferAddLit(buf, ",masterbus=");
- qemuUSBId(buf, def->idx);
- virBufferAsprintf(buf, ".0,firstport=%d",
def->info.master.usb.startport);
- } else {
- virBufferAddLit(buf, ",id=");
- qemuUSBId(buf, def->idx);
- }
+ if (def->info.mastertype == VIR_DOMAIN_CONTROLLER_MASTER_USB)
+ virBufferAsprintf(buf, ",masterbus=%s.0,firstport=%d",
+ def->info.alias, def->info.master.usb.startport);
+ else
+ virBufferAsprintf(buf, ",id=%s", def->info.alias);
return 0;
}
--
2.1.0
2:36 p.m.
New subject: [libvirt] [PATCHv2 7/9] qemu: remove test for allowing ide controller in s390, rename usb tests
Back in 2013, commit 877bc089 added in some tests that made sure no
error was generated on a domain definition that had an automatically
added usb controller if that domain didn't have a PCI bus to attach
the usb controller to. This was done because, at that time, libvirt
was automatically adding a usb controller to *any* domain definition
that didn't have one. Along with permitting the controller, two
s390-specific tests were added to ensure this behavior was maintained:
one with <controller type='usb' model='none'/> and another (called
"s390-piix-controllers") that had both usb and ide controllers, but
nothing attached to them.
Then in February of this year, commit 09ab9dcc eliminated the annoying
auto-adding of a usb device for s390 and s390x machines, stating:
"Since s390 does not support usb the default creation of a usb
controller for a domain should not occur."
Although, as verified here, the s390 doesn't support usb, and usb
controllers aren't currently added to s390 domain definitions
automatically, there are likely still some domain definitions in the
wild that have a usb controller (which was added *by libvirt*, not by
the user), so we will keep the tests verifying that behavior for
now. But this patch changes the names of the tests to reflect that
they don't actually contain a valid s390 config; this way future
developers won't propagate the incorrect idea that an s390 virtual
machine can have a USB (or IDE) bus.
In the case of the IDE controller, though, libvirt has never
automatically added an IDE controller unless a user added an IDE disk
(which itself would have caused an error), and we specifically *do*
want to begin generating an error when someone tries to add an IDE
controller to a domain that can't support one. For that reason, while
renaming the sz390-piix-controllers patch, this patch removes the
<controller type='ide'...> from it (otherwise the upcoming patch would
break make check)
---
Change from V1: (this was patch 10/13 in V1)
None - already ACKed (technically, this is V3, since I previously
posted the patch by itself and modified it based on feedback from
jtomko). Included here because it's needed for the next patch.
...rollers.args => qemuxml2argv-s390-allow-bogus-usb-controller.args} | 0
...ntrollers.xml => qemuxml2argv-s390-allow-bogus-usb-controller.xml} | 3 ---
...s390-usb-none.args => qemuxml2argv-s390-allow-bogus-usb-none.args} | 0
...v-s390-usb-none.xml => qemuxml2argv-s390-allow-bogus-usb-none.xml} | 0
tests/qemuxml2argvtest.c | 4 ++--
5 files changed, 2 insertions(+), 5 deletions(-)
rename tests/qemuxml2argvdata/{qemuxml2argv-s390-piix-controllers.args =>
qemuxml2argv-s390-allow-bogus-usb-controller.args} (100%)
rename tests/qemuxml2argvdata/{qemuxml2argv-s390-piix-controllers.xml =>
qemuxml2argv-s390-allow-bogus-usb-controller.xml} (86%)
rename tests/qemuxml2argvdata/{qemuxml2argv-s390-usb-none.args =>
qemuxml2argv-s390-allow-bogus-usb-none.args} (100%)
rename tests/qemuxml2argvdata/{qemuxml2argv-s390-usb-none.xml =>
qemuxml2argv-s390-allow-bogus-usb-none.xml} (100%)
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-s390-piix-controllers.args
b/tests/qemuxml2argvdata/qemuxml2argv-s390-allow-bogus-usb-controller.args
similarity index 100%
rename from tests/qemuxml2argvdata/qemuxml2argv-s390-piix-controllers.args
rename to tests/qemuxml2argvdata/qemuxml2argv-s390-allow-bogus-usb-controller.args
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-s390-piix-controllers.xml
b/tests/qemuxml2argvdata/qemuxml2argv-s390-allow-bogus-usb-controller.xml
similarity index 86%
rename from tests/qemuxml2argvdata/qemuxml2argv-s390-piix-controllers.xml
rename to tests/qemuxml2argvdata/qemuxml2argv-s390-allow-bogus-usb-controller.xml
index a8b72d7..ba0e6f7 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-s390-piix-controllers.xml
+++ b/tests/qemuxml2argvdata/qemuxml2argv-s390-allow-bogus-usb-controller.xml
@@ -22,9 +22,6 @@
<controller type='usb' index='0'>
<address type='pci' domain='0x0000' bus='0x00'
slot='0x01' function='0x2'/>
</controller>
- <controller type='ide' index='0'>
- <address type='pci' domain='0x0000' bus='0x00'
slot='0x01' function='0x1'/>
- </controller>
<memballoon model='virtio'>
</memballoon>
<rng model='virtio'>
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-s390-usb-none.args
b/tests/qemuxml2argvdata/qemuxml2argv-s390-allow-bogus-usb-none.args
similarity index 100%
rename from tests/qemuxml2argvdata/qemuxml2argv-s390-usb-none.args
rename to tests/qemuxml2argvdata/qemuxml2argv-s390-allow-bogus-usb-none.args
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-s390-usb-none.xml
b/tests/qemuxml2argvdata/qemuxml2argv-s390-allow-bogus-usb-none.xml
similarity index 100%
rename from tests/qemuxml2argvdata/qemuxml2argv-s390-usb-none.xml
rename to tests/qemuxml2argvdata/qemuxml2argv-s390-allow-bogus-usb-none.xml
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index e67d909..c08b3e0 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -1415,12 +1415,12 @@ mymain(void)
QEMU_CAPS_VIRTIO_S390, QEMU_CAPS_DEVICE_VIRTIO_RNG,
QEMU_CAPS_OBJECT_RNG_RANDOM);
- DO_TEST("s390-usb-none",
+ DO_TEST("s390-allow-bogus-usb-none",
QEMU_CAPS_DEVICE, QEMU_CAPS_CHARDEV, QEMU_CAPS_NODEFCONFIG,
QEMU_CAPS_DRIVE, QEMU_CAPS_BOOTINDEX, QEMU_CAPS_VIRTIO_S390,
QEMU_CAPS_DEVICE_VIRTIO_RNG, QEMU_CAPS_OBJECT_RNG_RANDOM);
- DO_TEST("s390-piix-controllers",
+ DO_TEST("s390-allow-bogus-usb-controller",
QEMU_CAPS_DEVICE, QEMU_CAPS_CHARDEV, QEMU_CAPS_NODEFCONFIG,
QEMU_CAPS_DRIVE, QEMU_CAPS_BOOTINDEX, QEMU_CAPS_VIRTIO_S390,
QEMU_CAPS_DEVICE_VIRTIO_RNG, QEMU_CAPS_OBJECT_RNG_RANDOM);
--
2.1.0
2:36 p.m.
New subject: [libvirt] [PATCHv2 8/9] qemu: clean up qemuBuildCommandline loop that builds controller args
Reorganize the loop that builds controller args to remove unnecessary
duplicated code and superfluous else clauses. No functional change
(this was split out from the following patch to make review easier).
---
New in V2 - this was a part of patch 11/13 in V1. jtomko requested
that I separate out the cleanup from the bugfix, so this is the
cleanup of existing code, and the bugfix will be in the next patch.
src/qemu/qemu_command.c | 81 ++++++++++++++++++++++++-------------------------
1 file changed, 39 insertions(+), 42 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index eead482..fdd6a2e 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -4548,6 +4548,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;
@@ -8619,12 +8625,21 @@ 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 IDE or 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 SATA
+ * controller on q35, but we do add those beyond this one
+ * exception.
+ */
VIR_DOMAIN_CONTROLLER_TYPE_PCI,
VIR_DOMAIN_CONTROLLER_TYPE_USB,
VIR_DOMAIN_CONTROLLER_TYPE_SCSI,
@@ -9401,51 +9416,35 @@ 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 */
continue;
}
- /* Skip pci-root/pcie-root */
+ /* 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;
- 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 +9452,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);
}
}
}
--
2.1.0
10:26 a.m.
New subject: [libvirt] [PATCHv2 8/9] qemu: clean up qemuBuildCommandline loop that builds controller args
On Thu, May 14, 2015 at 03:36:28PM -0400, Laine Stump wrote:
Reorganize the loop that builds controller args to remove
unnecessary
duplicated code and superfluous else clauses. No functional change
(this was split out from the following patch to make review easier).
This note can be dropped from git history.
---
New in V2 - this was a part of patch 11/13 in V1. jtomko requested
that I separate out the cleanup from the bugfix, so this is the
cleanup of existing code, and the bugfix will be in the next patch.
Thanks, it's much easier to follow now.
src/qemu/qemu_command.c | 81 ++++++++++++++++++++++++-------------------------
1 file changed, 39 insertions(+), 42 deletions(-)
- /* 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;
Unless intended, this is indented one level too many.
- } else {
- char *devstr;
-
- virCommandAddArg(cmd, "-device");
- if (!(devstr = qemuBuildControllerDevStr(def, cont,
- qemuCaps, NULL)))
- goto error;
- virCommandAddArg(cmd, devstr);
- VIR_FREE(devstr);
- }
Jan
2:36 p.m.
New subject: [libvirt] [PATCHv2 9/9] 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.
---
Change from V1: (this is the bugfix part of 11/13 in V1)
* put the cleanup that had no functional change into a separate patch
* remove some of the text from the commit log (although not too much,
because I think the history is important).
src/qemu/qemu_command.c | 32 ++++++++++++++++++----
.../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, 30 insertions(+), 15 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index fdd6a2e..2fa4c0d 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 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
10:28 a.m.
On Thu, May 14, 2015 at 03:36:20PM -0400, Laine Stump wrote:
V1 was here:
http://www.redhat.com/archives/libvir-list/2015-May/msg00124.html
This started out with the intent to generate an error/failure on
request for an IDE controller on a machinetype that doesn't support
IDE (currently anything except 440fx-based machinetypes), or a 2nd IDE
controller on a machinetype that only supports one (i.e. 440fx). This
led to a few other related fixes, and some "fixes related to the
related fixes".
I've pushed some of the ACKed patches, and dropped the two patches
that changed qemuBuildDeviceAddress and qemuAssignDeviceControllerAlias
to use switches instead of "if .. else if .." (03/13 and 04/13) and
dropped the two SCSI patches (12/13 and 13/13), but left a couple of
ACKed patches in for clarity.
Detailed differences from V1 in each patch.
Laine Stump (9):
conf: utility to return alias of a controller based on type/index
qemu: fix exceptions in qemuAssignDeviceControllerAlias
qemu: use controller alias when constructing device/controller args
qemu: use alias to refer to non-multibus PCI controller
qemu: use controller's alias in commandline for scsi-generic
qemu: use USB controller's alias instead of qemuUSBId()
qemu: remove test for allowing ide controller in s390, rename usb
tests
qemu: clean up qemuBuildCommandline loop that builds controller args
qemu: log error when domain has an unsupported IDE controller
...
21 files changed, 226 insertions(+), 163 deletions(-)
ACK series.
Jan
3480
days inactive
3481
days old
11 comments
2 participants
participants (2)
-
Ján Tomko -
Laine Stump