1:03 p.m.
These are intertwined a bit, hence they are all posted in the same series.
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":
* 1 and 2 just eliminate redundant code that I found while
making the changes.
* 3 & 4 change two sets of if else if else if, into switches,
so that it's cleaner and simpler to add more cases.
* 5 is a utility function that will be used later to make it easier to
get the alias of the controller required by a particular device.
* 6 adds exceptions for the primary IDE on 440fx and primary SATA on
q35 to the function that creates controller alias names (since those
are hardcoded in qemu)
* 7-9 switch all remaining uses of BufferAsprintf(buf,
"blahcontroller%d") with calls to the function that returns the
*real* alias of the controller device.
* 10 removes a test that verifies we allow an IDE controller in an
s390 machine definition, and renames the tests to indicate that
although we're checking to make sure we allow usb controllers on
s390, that this isn't actually a valid config. (I would add a
warning in the appropriate place when this happens, but at the
moment I'm sick of looking at the problem :-/) - This is v2 of the
following patch that I posted by itself earlier:
https://www.redhat.com/archives/libvir-list/2015-April/msg01596.html
https://www.redhat.com/archives/libvir-list/2015-May/msg00031.html
* 11 makes it an error to define an IDE controller that qemu won't be
able to actually create.
* 12 and 13 fix two additional prolems I noticed with the creation of
device and drive strings for SCSI disks - the first is that we add a
"channel=" option even when qemu has told us it doesn't support it,
and the 2nd is that we have carried forward a restriction that bus
must == 0 for SCSI - this check was added in 2009, and it seems that
it is only valid when *not* using -device to define the disk
parameters.
Laine Stump (13):
qemu: use qemuDomainMachineIsI440FX() in appropriate place
qemu: eliminate duplicated code in qemuBuildDriveDevStr()
qemu: change if to switch in qemuBuildDeviceAddressStr
qemu: restructure qemuAssignDeviceControllerAlias
conf: utility to return alias of a controller based on type/index
qemu: add exceptions for alias names of primary sata/ide controllers
qemu: use controller alias when constructing disk/controller args
qemu: use alias for all controller ids in qemuBuildControllerDevStr
qemu: use controller object alias in commandline for virtio-serial
device
qemu: remove test for allowing ide controller in s390, rename usb
tests
qemu: log error when domain has an upsupported IDE controller
qemu: only add channel arg to scsi-disk if qemu binary supports it
qemu: allow bus != 0 for scsi-disk when -device is used
src/conf/domain_conf.c | 34 +++
src/conf/domain_conf.h | 3 +
src/libvirt_private.syms | 1 +
src/qemu/qemu_command.c | 314 ++++++++++++---------
src/qemu/qemu_command.h | 3 +-
src/qemu/qemu_hotplug.c | 4 +-
.../qemuxml2argv-disk-blockio.args | 2 +-
.../qemuxml2argvdata/qemuxml2argv-disk-blockio.xml | 1 -
.../qemuxml2argv-disk-drive-network-iscsi-lun.args | 2 +-
.../qemuxml2argv-disk-ide-drive-split.args | 2 +-
.../qemuxml2argv-disk-ide-drive-split.xml | 1 -
.../qemuxml2argv-disk-sata-device.args | 4 +-
.../qemuxml2argv-disk-scsi-disk-split.args | 8 +-
.../qemuxml2argv-disk-scsi-disk-vpd.args | 2 +-
.../qemuxml2argv-disk-scsi-disk-wwn.args | 4 +-
.../qemuxml2argv-disk-scsi-lun-passthrough.args | 12 +-
.../qemuxml2argv-disk-scsi-lun-passthrough.xml | 4 +-
.../qemuxml2argv-disk-scsi-megasas.args | 2 +-
.../qemuxml2argv-disk-scsi-virtio-scsi.args | 2 +-
.../qemuxml2argv-disk-scsi-vscsi.args | 2 +-
.../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 -
.../qemuxml2argv-disk-virtio-scsi-ccw.args | 2 +-
.../qemuxml2argv-disk-virtio-scsi-cmd_per_lun.args | 2 +-
.../qemuxml2argv-disk-virtio-scsi-max_sectors.args | 2 +-
.../qemuxml2argv-disk-virtio-scsi-num_queues.args | 2 +-
.../qemuxml2argv-pseries-vio-user-assigned.args | 2 +-
.../qemuxml2argvdata/qemuxml2argv-pseries-vio.args | 2 +-
...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 | 29 +-
.../qemuxml2xmlout-disk-source-pool.xml | 1 -
36 files changed, 266 insertions(+), 192 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
1:03 p.m.
New subject: [libvirt] [PATCH 01/13] qemu: use qemuDomainMachineIsI440FX() in appropriate place
This patch makes qemuValideDevicePCISlotsChipsets() more consistent in
appearance by replacing several clauses of an if with the equivalent
call to qemuDomainMachineIsI440FX. The if was checking exactly the
same items, just in a slightly different order.
---
src/qemu/qemu_command.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index c32d8c6..67214e3 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -2110,11 +2110,7 @@ qemuValidateDevicePCISlotsChipsets(virDomainDefPtr def,
virQEMUCapsPtr qemuCaps,
virDomainPCIAddressSetPtr addrs)
{
- if ((STRPREFIX(def->os.machine, "pc-0.") ||
- STRPREFIX(def->os.machine, "pc-1.") ||
- STRPREFIX(def->os.machine, "pc-i440") ||
- STREQ(def->os.machine, "pc") ||
- STRPREFIX(def->os.machine, "rhel")) &&
+ if (qemuDomainMachineIsI440FX(def) &&
qemuValidateDevicePCISlotsPIIX3(def, qemuCaps, addrs) < 0) {
return -1;
}
--
2.1.0
11:38 a.m.
New subject: [libvirt] [PATCH 01/13] qemu: use qemuDomainMachineIsI440FX() in appropriate place
On 05/05/2015 02:03 PM, Laine Stump wrote:
This patch makes qemuValideDevicePCISlotsChipsets() more consistent
in
appearance by replacing several clauses of an if with the equivalent
call to qemuDomainMachineIsI440FX. The if was checking exactly the
same items, just in a slightly different order.
---
src/qemu/qemu_command.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
ACK
John
1:03 p.m.
New subject: [libvirt] [PATCH 02/13] qemu: eliminate duplicated code in qemuBuildDriveDevStr()
The code to add device type to the commandline was identical for lsi
and other models of SCSI controllers, but was duplicated (with the
exception of a minor ordering difference of the if-else clauses) for
the two cases. This patch replaces those two with a single instance of
the code just before the if().
---
src/qemu/qemu_command.c | 39 +++++++++++++--------------------------
1 file changed, 13 insertions(+), 26 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 67214e3..620a2a0 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -4070,6 +4070,19 @@ qemuBuildDriveDevStr(virDomainDefPtr def,
if ((qemuSetSCSIControllerModel(def, qemuCaps, &controllerModel)) < 0)
goto error;
+ if (disk->device == VIR_DOMAIN_DISK_DEVICE_LUN) {
+ virBufferAddLit(&opt, "scsi-block");
+ } else {
+ if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SCSI_CD)) {
+ if (disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM)
+ virBufferAddLit(&opt, "scsi-cd");
+ else
+ virBufferAddLit(&opt, "scsi-hd");
+ } else {
+ virBufferAddLit(&opt, "scsi-disk");
+ }
+ }
+
if (controllerModel == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSILOGIC) {
if (disk->info.addr.drive.target != 0) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
@@ -4078,19 +4091,6 @@ qemuBuildDriveDevStr(virDomainDefPtr def,
goto error;
}
- if (disk->device == VIR_DOMAIN_DISK_DEVICE_LUN) {
- virBufferAddLit(&opt, "scsi-block");
- } else {
- if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SCSI_CD)) {
- if (disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM)
- virBufferAddLit(&opt, "scsi-cd");
- else
- virBufferAddLit(&opt, "scsi-hd");
- } else {
- virBufferAddLit(&opt, "scsi-disk");
- }
- }
-
virBufferAsprintf(&opt, ",bus=scsi%d.%d,scsi-id=%d",
disk->info.addr.drive.controller,
disk->info.addr.drive.bus,
@@ -4113,19 +4113,6 @@ qemuBuildDriveDevStr(virDomainDefPtr def,
}
}
- if (disk->device != VIR_DOMAIN_DISK_DEVICE_LUN) {
- if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SCSI_CD)) {
- if (disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM)
- virBufferAddLit(&opt, "scsi-cd");
- else
- virBufferAddLit(&opt, "scsi-hd");
- } else {
- virBufferAddLit(&opt, "scsi-disk");
- }
- } else {
- virBufferAddLit(&opt, "scsi-block");
- }
-
virBufferAsprintf(&opt,
",bus=scsi%d.0,channel=%d,scsi-id=%d,lun=%d",
disk->info.addr.drive.controller,
disk->info.addr.drive.bus,
--
2.1.0
11:39 a.m.
New subject: [libvirt] [PATCH 02/13] qemu: eliminate duplicated code in qemuBuildDriveDevStr()
On 05/05/2015 02:03 PM, Laine Stump wrote:
The code to add device type to the commandline was identical for lsi
and other models of SCSI controllers, but was duplicated (with the
exception of a minor ordering difference of the if-else clauses) for
the two cases. This patch replaces those two with a single instance of
the code just before the if().
---
src/qemu/qemu_command.c | 39 +++++++++++++--------------------------
1 file changed, 13 insertions(+), 26 deletions(-)
ACK,
John
1:03 p.m.
New subject: [libvirt] [PATCH 03/13] qemu: change if to switch in qemuBuildDeviceAddressStr
---
src/qemu/qemu_command.c | 18 +++++++++++-------
1 file changed, 11 insertions(+), 7 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 620a2a0..b76bd98 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -2585,11 +2585,11 @@ qemuBuildDeviceAddressStr(virBufferPtr buf,
{
int ret = -1;
char *devStr = NULL;
+ const char *contAlias = NULL;
+ size_t i;
- if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) {
- const char *contAlias = NULL;
- size_t i;
-
+ switch (info->type) {
+ case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI:
if (!(devStr = virDomainPCIAddressAsString(&info->addr.pci)))
goto cleanup;
for (i = 0; i < domainDef->ncontrollers; i++) {
@@ -2664,19 +2664,23 @@ qemuBuildDeviceAddressStr(virBufferPtr buf,
virBufferAsprintf(buf, ",addr=0x%x", info->addr.pci.slot);
if (info->addr.pci.function != 0)
virBufferAsprintf(buf, ".0x%x", info->addr.pci.function);
- } else if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB) {
+ break;
+ case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB:
virBufferAddLit(buf, ",bus=");
qemuUSBId(buf, info->addr.usb.bus);
virBufferAsprintf(buf, ".0,port=%s", info->addr.usb.port);
- } else if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO) {
+ break;
+ case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO:
if (info->addr.spaprvio.has_reg)
virBufferAsprintf(buf, ",reg=0x%llx", info->addr.spaprvio.reg);
- } else if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) {
+ break;
+ case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW:
if (info->addr.ccw.assigned)
virBufferAsprintf(buf, ",devno=%x.%x.%04x",
info->addr.ccw.cssid,
info->addr.ccw.ssid,
info->addr.ccw.devno);
+ break;
}
ret = 0;
--
2.1.0
11:39 a.m.
New subject: [libvirt] [PATCH 03/13] qemu: change if to switch in qemuBuildDeviceAddressStr
On 05/05/2015 02:03 PM, Laine Stump wrote:
---
src/qemu/qemu_command.c | 18 +++++++++++-------
1 file changed, 11 insertions(+), 7 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 620a2a0..b76bd98 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -2585,11 +2585,11 @@ qemuBuildDeviceAddressStr(virBufferPtr buf,
{
int ret = -1;
char *devStr = NULL;
+ const char *contAlias = NULL;
+ size_t i;
- if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) {
- const char *contAlias = NULL;
- size_t i;
-
+ switch (info->type) {
s/(/((virDomainDeviceAddressType)/
+ case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI:
if (!(devStr = virDomainPCIAddressAsString(&info->addr.pci)))
goto cleanup;
for (i = 0; i < domainDef->ncontrollers; i++) {
@@ -2664,19 +2664,23 @@ qemuBuildDeviceAddressStr(virBufferPtr buf,
virBufferAsprintf(buf, ",addr=0x%x", info->addr.pci.slot);
if (info->addr.pci.function != 0)
virBufferAsprintf(buf, ".0x%x", info->addr.pci.function);
- } else if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB) {
+ break;
+ case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB:
virBufferAddLit(buf, ",bus=");
qemuUSBId(buf, info->addr.usb.bus);
virBufferAsprintf(buf, ".0,port=%s", info->addr.usb.port);
- } else if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO) {
+ break;
+ case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO:
if (info->addr.spaprvio.has_reg)
virBufferAsprintf(buf, ",reg=0x%llx",
info->addr.spaprvio.reg);
- } else if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) {
+ break;
+ case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW:
if (info->addr.ccw.assigned)
virBufferAsprintf(buf, ",devno=%x.%x.%04x",
info->addr.ccw.cssid,
info->addr.ccw.ssid,
info->addr.ccw.devno);
+ break;
and the obligatory:
case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE:
case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE:
case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL:
case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCID:
case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390:
case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO:
case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_ISA:
case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM:
case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_LAST:
break;
ACK with that,
John
}
ret = 0;
1:03 p.m.
New subject: [libvirt] [PATCH 04/13] qemu: restructure qemuAssignDeviceControllerAlias
No functional change, just rearrange this function and pass in full
domain definition to make it simpler to add exceptions.
---
src/qemu/qemu_command.c | 32 ++++++++++++++++++++++----------
src/qemu/qemu_command.h | 2 +-
src/qemu/qemu_hotplug.c | 4 ++--
3 files changed, 25 insertions(+), 13 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index b76bd98..340478c 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1031,22 +1031,34 @@ qemuAssignDeviceRedirdevAlias(virDomainDefPtr def,
virDomainRedirdevDefPtr redir
int
-qemuAssignDeviceControllerAlias(virDomainControllerDefPtr controller)
+qemuAssignDeviceControllerAlias(ATTRIBUTE_UNUSED virDomainDefPtr def,
+ virDomainControllerDefPtr controller)
{
- const char *prefix = virDomainControllerTypeToString(controller->type);
+ int ret = 0;
+
+ VIR_FREE(controller->info.alias);
- 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".
+ switch (controller->type) {
+ case VIR_DOMAIN_CONTROLLER_TYPE_PCI:
+ /* pcie-root uses a different naming convention ("pcie.0"),
+ * because it is hardcoded that way in qemu. All other PCI
+ * buses use the consistent "pci.%u".
*/
if (controller->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT)
- return virAsprintf(&controller->info.alias, "pcie.%d",
controller->idx);
+ ret = virAsprintf(&controller->info.alias, "pcie.%d",
controller->idx);
else
- return virAsprintf(&controller->info.alias, "pci.%d",
controller->idx);
+ ret = virAsprintf(&controller->info.alias, "pci.%d",
controller->idx);
+ break;
+ default:
+ break;
}
- return virAsprintf(&controller->info.alias, "%s%d", prefix,
controller->idx);
+ /* catchall for anything that wasn't an exception */
+ if (ret == 0 && !controller->info.alias)
+ ret = virAsprintf(&controller->info.alias, "%s%d",
+ virDomainControllerTypeToString(controller->type),
+ controller->idx);
+ return ret;
}
static ssize_t
@@ -1174,7 +1186,7 @@ qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr
qemuCaps)
return -1;
}
for (i = 0; i < def->ncontrollers; i++) {
- if (qemuAssignDeviceControllerAlias(def->controllers[i]) < 0)
+ if (qemuAssignDeviceControllerAlias(def, 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..d066953 100644
--- a/src/qemu/qemu_command.h
+++ b/src/qemu/qemu_command.h
@@ -287,7 +287,7 @@ int qemuAssignDeviceDiskAlias(virDomainDefPtr vmdef,
virDomainDiskDefPtr def,
virQEMUCapsPtr qemuCaps);
int qemuAssignDeviceHostdevAlias(virDomainDefPtr def, virDomainHostdevDefPtr hostdev, int
idx);
-int qemuAssignDeviceControllerAlias(virDomainControllerDefPtr controller);
+int qemuAssignDeviceControllerAlias(virDomainDefPtr def, 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 613b728..24a5f51 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, controller) < 0)
goto cleanup;
if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_USB &&
@@ -3631,7 +3631,7 @@ int qemuDomainDetachControllerDevice(virQEMUDriverPtr driver,
if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE) &&
!detach->info.alias) {
- if (qemuAssignDeviceControllerAlias(detach) < 0)
+ if (qemuAssignDeviceControllerAlias(vm->def, detach) < 0)
goto cleanup;
}
--
2.1.0
11:51 a.m.
New subject: [libvirt] [PATCH 04/13] qemu: restructure qemuAssignDeviceControllerAlias
On 05/05/2015 02:03 PM, Laine Stump wrote:
No functional change, just rearrange this function and pass in full
domain definition to make it simpler to add exceptions.
---
src/qemu/qemu_command.c | 32 ++++++++++++++++++++++----------
src/qemu/qemu_command.h | 2 +-
src/qemu/qemu_hotplug.c | 4 ++--
3 files changed, 25 insertions(+), 13 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index b76bd98..340478c 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1031,22 +1031,34 @@ qemuAssignDeviceRedirdevAlias(virDomainDefPtr def,
virDomainRedirdevDefPtr redir
int
-qemuAssignDeviceControllerAlias(virDomainControllerDefPtr controller)
+qemuAssignDeviceControllerAlias(ATTRIBUTE_UNUSED virDomainDefPtr def,
?? Not using it now or in this series to add exceptions, so is there
really a need for it?
+ virDomainControllerDefPtr
controller)
{
- const char *prefix = virDomainControllerTypeToString(controller->type);
+ int ret = 0;
+
+ VIR_FREE(controller->info.alias);
- 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".
+ switch (controller->type) {
Similar to 3/13
s/(/((virDomainControllerType)/
+ case VIR_DOMAIN_CONTROLLER_TYPE_PCI:
+ /* pcie-root uses a different naming convention ("pcie.0"),
+ * because it is hardcoded that way in qemu. All other PCI
+ * buses use the consistent "pci.%u".
*/
if (controller->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT)
- return virAsprintf(&controller->info.alias, "pcie.%d",
controller->idx);
+ ret = virAsprintf(&controller->info.alias, "pcie.%d",
controller->idx);
else
- return virAsprintf(&controller->info.alias, "pci.%d",
controller->idx);
+ ret = virAsprintf(&controller->info.alias, "pci.%d",
controller->idx);
+ break;
+ default:
Change default to:
case VIR_DOMAIN_CONTROLLER_TYPE_IDE:
case VIR_DOMAIN_CONTROLLER_TYPE_FDC:
case VIR_DOMAIN_CONTROLLER_TYPE_SCSI:
case VIR_DOMAIN_CONTROLLER_TYPE_SATA:
case VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL:
case VIR_DOMAIN_CONTROLLER_TYPE_CCID:
case VIR_DOMAIN_CONTROLLER_TYPE_USB:
ret = virAsprintf(&controller->info.alias, "%s%d",
virDomainControllerTypeToString(controller->type),
controller->idx);
break;
case VIR_DOMAIN_CONTROLLER_TYPE_LAST:
Some sort of virReportError...
ret = -1;
+ break;
}
- return virAsprintf(&controller->info.alias, "%s%d", prefix,
controller->idx);
+ /* catchall for anything that wasn't an exception */
+ if (ret == 0 && !controller->info.alias)
+ ret = virAsprintf(&controller->info.alias, "%s%d",
+ virDomainControllerTypeToString(controller->type),
+ controller->idx);
Thus removing the need for this ^^^^
ACK w/ adjustments... Although I'm still unsure why we want to pass
the whole domain def...
John
+ return ret;
}
static ssize_t
@@ -1174,7 +1186,7 @@ qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr
qemuCaps)
return -1;
}
for (i = 0; i < def->ncontrollers; i++) {
- if (qemuAssignDeviceControllerAlias(def->controllers[i]) < 0)
+ if (qemuAssignDeviceControllerAlias(def, 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..d066953 100644
--- a/src/qemu/qemu_command.h
+++ b/src/qemu/qemu_command.h
@@ -287,7 +287,7 @@ int qemuAssignDeviceDiskAlias(virDomainDefPtr vmdef,
virDomainDiskDefPtr def,
virQEMUCapsPtr qemuCaps);
int qemuAssignDeviceHostdevAlias(virDomainDefPtr def, virDomainHostdevDefPtr hostdev,
int idx);
-int qemuAssignDeviceControllerAlias(virDomainControllerDefPtr controller);
+int qemuAssignDeviceControllerAlias(virDomainDefPtr def, 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 613b728..24a5f51 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, controller) < 0)
goto cleanup;
if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_USB &&
@@ -3631,7 +3631,7 @@ int qemuDomainDetachControllerDevice(virQEMUDriverPtr driver,
if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE) &&
!detach->info.alias) {
- if (qemuAssignDeviceControllerAlias(detach) < 0)
+ if (qemuAssignDeviceControllerAlias(vm->def, detach) < 0)
goto cleanup;
}
12:17 p.m.
New subject: [libvirt] [PATCH 04/13] qemu: restructure qemuAssignDeviceControllerAlias
On 05/08/2015 12:51 PM, John Ferlan wrote:
On 05/05/2015 02:03 PM, Laine Stump wrote:
> No functional change, just rearrange this function and pass in full
> domain definition to make it simpler to add exceptions.
> ---
> src/qemu/qemu_command.c | 32 ++++++++++++++++++++++----------
> src/qemu/qemu_command.h | 2 +-
> src/qemu/qemu_hotplug.c | 4 ++--
> 3 files changed, 25 insertions(+), 13 deletions(-)
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index b76bd98..340478c 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -1031,22 +1031,34 @@ qemuAssignDeviceRedirdevAlias(virDomainDefPtr def,
virDomainRedirdevDefPtr redir
>
>
> int
> -qemuAssignDeviceControllerAlias(virDomainControllerDefPtr controller)
> +qemuAssignDeviceControllerAlias(ATTRIBUTE_UNUSED virDomainDefPtr def,
?? Not using it now or in this series to add exceptions, so is there
really a need for it?
Patch 6 - add exceptions for primate ide/sata controllers.
> + virDomainControllerDefPtr controller)
> {
> - const char *prefix = virDomainControllerTypeToString(controller->type);
> + int ret = 0;
> +
> + VIR_FREE(controller->info.alias);
>
> - 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".
> + switch (controller->type) {
Similar to 3/13
s/(/((virDomainControllerType)/
> + case VIR_DOMAIN_CONTROLLER_TYPE_PCI:
> + /* pcie-root uses a different naming convention ("pcie.0"),
> + * because it is hardcoded that way in qemu. All other PCI
> + * buses use the consistent "pci.%u".
> */
> if (controller->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT)
> - return virAsprintf(&controller->info.alias, "pcie.%d",
controller->idx);
> + ret = virAsprintf(&controller->info.alias, "pcie.%d",
controller->idx);
> else
> - return virAsprintf(&controller->info.alias, "pci.%d",
controller->idx);
> + ret = virAsprintf(&controller->info.alias, "pci.%d",
controller->idx);
> + break;
> + default:
Change default to:
case VIR_DOMAIN_CONTROLLER_TYPE_IDE:
case VIR_DOMAIN_CONTROLLER_TYPE_FDC:
case VIR_DOMAIN_CONTROLLER_TYPE_SCSI:
case VIR_DOMAIN_CONTROLLER_TYPE_SATA:
case VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL:
case VIR_DOMAIN_CONTROLLER_TYPE_CCID:
case VIR_DOMAIN_CONTROLLER_TYPE_USB:
ret = virAsprintf(&controller->info.alias, "%s%d",
virDomainControllerTypeToString(controller->type),
controller->idx);
break;
case VIR_DOMAIN_CONTROLLER_TYPE_LAST:
Some sort of virReportError...
ret = -1;
> + break;
> }
>
> - return virAsprintf(&controller->info.alias, "%s%d", prefix,
controller->idx);
> + /* catchall for anything that wasn't an exception */
> + if (ret == 0 && !controller->info.alias)
> + ret = virAsprintf(&controller->info.alias, "%s%d",
> + virDomainControllerTypeToString(controller->type),
> + controller->idx);
Thus removing the need for this ^^^^
That won't work, because the cases for IDE and SATA don't always set the
alias (they only set it or machinetypes and controller indexes where it
needs to be an exception).
ACK w/ adjustments... Although I'm still unsure why we want to
pass
the whole domain def...
You need the DomainDefPtr so you check the machinetype with
qemuDomainMachineIs*()
12:42 p.m.
New subject: [libvirt] [PATCH 04/13] qemu: restructure qemuAssignDeviceControllerAlias
On 05/08/2015 01:17 PM, Laine Stump wrote:
On 05/08/2015 12:51 PM, John Ferlan wrote:
>
> On 05/05/2015 02:03 PM, Laine Stump wrote:
>> No functional change, just rearrange this function and pass in full
>> domain definition to make it simpler to add exceptions.
>> ---
>> src/qemu/qemu_command.c | 32 ++++++++++++++++++++++----------
>> src/qemu/qemu_command.h | 2 +-
>> src/qemu/qemu_hotplug.c | 4 ++--
>> 3 files changed, 25 insertions(+), 13 deletions(-)
>>
>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>> index b76bd98..340478c 100644
>> --- a/src/qemu/qemu_command.c
>> +++ b/src/qemu/qemu_command.c
>> @@ -1031,22 +1031,34 @@ qemuAssignDeviceRedirdevAlias(virDomainDefPtr def,
virDomainRedirdevDefPtr redir
>>
>>
>> int
>> -qemuAssignDeviceControllerAlias(virDomainControllerDefPtr controller)
>> +qemuAssignDeviceControllerAlias(ATTRIBUTE_UNUSED virDomainDefPtr def,
> ?? Not using it now or in this series to add exceptions, so is there
> really a need for it?
Patch 6 - add exceptions for primate ide/sata controllers.
oh - right... but I was wondering why no compiler complaint on the
ATTRIBUTE_UNUSED... Perhaps it's "ordering related"...
That is should it be:
virDomainDefPtr def ATTRIBUTE_UNUSED,
here... then in patch 6 it gets removed...
That's why my eyes locked onto...
>
>> + virDomainControllerDefPtr controller)
>> {
>> - const char *prefix = virDomainControllerTypeToString(controller->type);
>> + int ret = 0;
>> +
>> + VIR_FREE(controller->info.alias);
>>
>> - 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".
>> + switch (controller->type) {
> Similar to 3/13
>
> s/(/((virDomainControllerType)/
>
>> + case VIR_DOMAIN_CONTROLLER_TYPE_PCI:
>> + /* pcie-root uses a different naming convention ("pcie.0"),
>> + * because it is hardcoded that way in qemu. All other PCI
>> + * buses use the consistent "pci.%u".
>> */
>> if (controller->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT)
>> - return virAsprintf(&controller->info.alias,
"pcie.%d", controller->idx);
>> + ret = virAsprintf(&controller->info.alias,
"pcie.%d", controller->idx);
>> else
>> - return virAsprintf(&controller->info.alias,
"pci.%d", controller->idx);
>> + ret = virAsprintf(&controller->info.alias,
"pci.%d", controller->idx);
>> + break;
>> + default:
> Change default to:
>
> case VIR_DOMAIN_CONTROLLER_TYPE_IDE:
> case VIR_DOMAIN_CONTROLLER_TYPE_FDC:
> case VIR_DOMAIN_CONTROLLER_TYPE_SCSI:
> case VIR_DOMAIN_CONTROLLER_TYPE_SATA:
> case VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL:
> case VIR_DOMAIN_CONTROLLER_TYPE_CCID:
> case VIR_DOMAIN_CONTROLLER_TYPE_USB:
> ret = virAsprintf(&controller->info.alias, "%s%d",
> virDomainControllerTypeToString(controller->type),
> controller->idx);
> break;
>
> case VIR_DOMAIN_CONTROLLER_TYPE_LAST:
> Some sort of virReportError...
> ret = -1;
>> + break;
>> }
>>
>> - return virAsprintf(&controller->info.alias, "%s%d", prefix,
controller->idx);
>> + /* catchall for anything that wasn't an exception */
>> + if (ret == 0 && !controller->info.alias)
>> + ret = virAsprintf(&controller->info.alias, "%s%d",
>> + virDomainControllerTypeToString(controller->type),
>> + controller->idx);
> Thus removing the need for this ^^^^
That won't work, because the cases for IDE and SATA don't always set the
alias (they only set it or machinetypes and controller indexes where it
needs to be an exception).
Huh? Patch 6 has those exceptions, but the way I'm reading how you
wrote the code an alias would be assigned (either "ide.%d" or
"sata.%d")
because you'd fall into the catchall code as ret would be 0 and
info.alias would be NULL.
> ACK w/ adjustments... Although I'm still unsure why we want
to pass
> the whole domain def...
You need the DomainDefPtr so you check the machinetype with
qemuDomainMachineIs*()
Yes, now I see - again, locked onto the ATTRIBUTE_UNUSED
John
4:42 p.m.
New subject: [libvirt] [PATCH 04/13] qemu: restructure qemuAssignDeviceControllerAlias
On 05/08/2015 01:42 PM, John Ferlan wrote:
On 05/08/2015 01:17 PM, Laine Stump wrote:
> On 05/08/2015 12:51 PM, John Ferlan wrote:
>> On 05/05/2015 02:03 PM, Laine Stump wrote:
>>> No functional change, just rearrange this function and pass in full
>>> domain definition to make it simpler to add exceptions.
>>> ---
>>> src/qemu/qemu_command.c | 32 ++++++++++++++++++++++----------
>>> src/qemu/qemu_command.h | 2 +-
>>> src/qemu/qemu_hotplug.c | 4 ++--
>>> 3 files changed, 25 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>>> index b76bd98..340478c 100644
>>> --- a/src/qemu/qemu_command.c
>>> +++ b/src/qemu/qemu_command.c
>>> @@ -1031,22 +1031,34 @@ qemuAssignDeviceRedirdevAlias(virDomainDefPtr def,
virDomainRedirdevDefPtr redir
>>>
>>>
>>> int
>>> -qemuAssignDeviceControllerAlias(virDomainControllerDefPtr controller)
>>> +qemuAssignDeviceControllerAlias(ATTRIBUTE_UNUSED virDomainDefPtr def,
>> ?? Not using it now or in this series to add exceptions, so is there
>> really a need for it?
> Patch 6 - add exceptions for primate ide/sata controllers.
>
oh - right... but I was wondering why no compiler complaint on the
ATTRIBUTE_UNUSED... Perhaps it's "ordering related"...
That is should it be:
virDomainDefPtr def ATTRIBUTE_UNUSED,
Okay, right. Not sure why I put it in that order.
here... then in patch 6 it gets removed...
That's why my eyes locked onto...
>>> + virDomainControllerDefPtr controller)
>>> {
>>> - const char *prefix =
virDomainControllerTypeToString(controller->type);
>>> + int ret = 0;
>>> +
>>> + VIR_FREE(controller->info.alias);
>>>
>>> - 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".
>>> + switch (controller->type) {
>> Similar to 3/13
>>
>> s/(/((virDomainControllerType)/
>>
>>> + case VIR_DOMAIN_CONTROLLER_TYPE_PCI:
>>> + /* pcie-root uses a different naming convention
("pcie.0"),
>>> + * because it is hardcoded that way in qemu. All other PCI
>>> + * buses use the consistent "pci.%u".
>>> */
>>> if (controller->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT)
>>> - return virAsprintf(&controller->info.alias,
"pcie.%d", controller->idx);
>>> + ret = virAsprintf(&controller->info.alias,
"pcie.%d", controller->idx);
>>> else
>>> - return virAsprintf(&controller->info.alias,
"pci.%d", controller->idx);
>>> + ret = virAsprintf(&controller->info.alias,
"pci.%d", controller->idx);
>>> + break;
>>> + default:
>> Change default to:
>>
>> case VIR_DOMAIN_CONTROLLER_TYPE_IDE:
>> case VIR_DOMAIN_CONTROLLER_TYPE_FDC:
>> case VIR_DOMAIN_CONTROLLER_TYPE_SCSI:
>> case VIR_DOMAIN_CONTROLLER_TYPE_SATA:
>> case VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL:
>> case VIR_DOMAIN_CONTROLLER_TYPE_CCID:
>> case VIR_DOMAIN_CONTROLLER_TYPE_USB:
>> ret = virAsprintf(&controller->info.alias, "%s%d",
>> virDomainControllerTypeToString(controller->type),
>> controller->idx);
>> break;
>>
>> case VIR_DOMAIN_CONTROLLER_TYPE_LAST:
>> Some sort of virReportError...
>> ret = -1;
>>> + break;
>>> }
>>>
>>> - return virAsprintf(&controller->info.alias, "%s%d",
prefix, controller->idx);
>>> + /* catchall for anything that wasn't an exception */
>>> + if (ret == 0 && !controller->info.alias)
>>> + ret = virAsprintf(&controller->info.alias, "%s%d",
>>> +
virDomainControllerTypeToString(controller->type),
>>> + controller->idx);
>> Thus removing the need for this ^^^^
> That won't work, because the cases for IDE and SATA don't always set the
> alias (they only set it or machinetypes and controller indexes where it
> needs to be an exception).
>
Huh? Patch 6 has those exceptions, but the way I'm reading how you
wrote the code an alias would be assigned (either "ide.%d" or
"sata.%d")
because you'd fall into the catchall code as ret would be 0 and
info.alias would be NULL.
Correct. I was saying that removing the catchall bit wouldn't work
because it would be needed when it was IDE/SATA but not primary.
Anyway, I've decided that I agree with your later asessment that, since
this is for exceptions, I should just retain the if .... else if ...
else structure of the original rather than change it to a switch. And
since I'm doing that, I'm merging the addition of argument(s) (turns out
I need qemuCaps in addition to DomainDef) into a single patch that adds
the exceptions for primary sata and ide controllers.
1:03 p.m.
New subject: [libvirt] [PATCH 05/13] 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).
---
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 4cd36a1..22f49c9 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -12554,6 +12554,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 087d282..7cc655b 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 c8e6fb4..0eb5f75 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
12:44 p.m.
New subject: [libvirt] [PATCH 05/13] conf: utility to return alias of a controller based on type/index
On 05/05/2015 02:03 PM, Laine Stump wrote:
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).
---
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 4cd36a1..22f49c9 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -12554,6 +12554,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;
+ }
Unless this is LAST ... in which case there's a lot more places in the
code that are going to fail using the *ToString calls.
+
+ 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;
So if the alias wasn't set for some reason we're going to start seeing
errors. Is there a reason the alias wouldn't be set... (just trying to
think/type outloud later in the day at the end of the week as my brain
is beginning to check out).
ACK - although I'm still trying to read ahead a bit and may come back to
this...
John
+ }
+ 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 087d282..7cc655b 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 c8e6fb4..0eb5f75 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -181,6 +181,7 @@ virDomainClockBasisTypeToString;
virDomainClockOffsetTypeFromString;
virDomainClockOffsetTypeToString;
virDomainConfigFile;
+virDomainControllerAliasFind;
virDomainControllerDefFree;
virDomainControllerFind;
virDomainControllerFindByType;
1:04 p.m.
New subject: [libvirt] [PATCH 05/13] conf: utility to return alias of a controller based on type/index
On 05/08/2015 01:44 PM, John Ferlan wrote:
On 05/05/2015 02:03 PM, Laine Stump wrote:
> +
> + 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;
So if the alias wasn't set for some reason we're going to start seeing
errors. Is there a reason the alias wouldn't be set... (just trying to
think/type outloud later in the day at the end of the week as my brain
is beginning to check out).
If the alias hasn't been set at the time that we're calling this
function, that would be an error that should be remedied. Currently the
only place it is going to be called is during commandline generation,
after all devices have had their aliases assigned, and during hotplug
(also by definition after all other devices have had their aliases
assigned). So for current usage we are fine. In the future if someone
tries to use this function when they shouldn't, they will get the
internal error and learn their lesson.
1:03 p.m.
New subject: [libvirt] [PATCH 06/13] qemu: add exceptions for alias names of primary sata/ide controllers
If a machine is Q35, the primary sata controller is hardcoded in qemu
to have the id "ide" (with no index in the name). Likewise on
440fx-based machinetypes, the primary ide controller is called
"ide". All other SATA controllers will have the standard name
"sata%d", where %d is the index of the controller, and if any
additional IDE controllers are ever supported, they will have the name
"ide%d".
---
src/qemu/qemu_command.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 340478c..89beeb3 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1049,6 +1049,21 @@ qemuAssignDeviceControllerAlias(ATTRIBUTE_UNUSED virDomainDefPtr
def,
else
ret = virAsprintf(&controller->info.alias, "pci.%d",
controller->idx);
break;
+ case VIR_DOMAIN_CONTROLLER_TYPE_IDE:
+ /* for any machine based on I440FX, the first (currently only)
+ * IDE controller is an integrated controller hardcoded with
+ * id "ide"
+ */
+ if (qemuDomainMachineIsI440FX(def) && controller->idx == 0)
+ ret = VIR_STRDUP(controller->info.alias, "ide");
+ break;
+ case 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(def) && controller->idx == 0)
+ ret = VIR_STRDUP(controller->info.alias, "ide");
+ break;
default:
break;
}
--
2.1.0
12:47 p.m.
New subject: [libvirt] [PATCH 06/13] qemu: add exceptions for alias names of primary sata/ide controllers
On 05/05/2015 02:03 PM, Laine Stump wrote:
If a machine is Q35, the primary sata controller is hardcoded in
qemu
to have the id "ide" (with no index in the name). Likewise on
440fx-based machinetypes, the primary ide controller is called
"ide". All other SATA controllers will have the standard name
"sata%d", where %d is the index of the controller, and if any
additional IDE controllers are ever supported, they will have the name
"ide%d".
---
src/qemu/qemu_command.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 340478c..89beeb3 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1049,6 +1049,21 @@ qemuAssignDeviceControllerAlias(ATTRIBUTE_UNUSED virDomainDefPtr
def,
else
ret = virAsprintf(&controller->info.alias, "pci.%d",
controller->idx);
break;
+ case VIR_DOMAIN_CONTROLLER_TYPE_IDE:
+ /* for any machine based on I440FX, the first (currently only)
+ * IDE controller is an integrated controller hardcoded with
+ * id "ide"
+ */
+ if (qemuDomainMachineIsI440FX(def) && controller->idx == 0)
+ ret = VIR_STRDUP(controller->info.alias, "ide");
Based on my review of 4/13 and my followup comment, there'd need to be
an "else" here with does the :
ret = virAsprintf(&controller->info.alias, "%s%d",
virDomainControllerTypeToString(controller->type),
controller->idx);
+ break;
+ case 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(def) && controller->idx == 0)
+ ret = VIR_STRDUP(controller->info.alias, "ide");
similarly, else
ret = virAsprintf(&controller->info.alias, "%s%d",
virDomainControllerTypeToString(controller->type),
controller->idx);
John
+ break;
default:
break;
}
1:12 p.m.
New subject: [libvirt] [PATCH 06/13] qemu: add exceptions for alias names of primary sata/ide controllers
On 05/08/2015 01:47 PM, John Ferlan wrote:
On 05/05/2015 02:03 PM, Laine Stump wrote:
> If a machine is Q35, the primary sata controller is hardcoded in qemu
> to have the id "ide" (with no index in the name). Likewise on
> 440fx-based machinetypes, the primary ide controller is called
> "ide". All other SATA controllers will have the standard name
> "sata%d", where %d is the index of the controller, and if any
> additional IDE controllers are ever supported, they will have the name
> "ide%d".
> ---
> src/qemu/qemu_command.c | 15 +++++++++++++++
> 1 file changed, 15 insertions(+)
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 340478c..89beeb3 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -1049,6 +1049,21 @@ qemuAssignDeviceControllerAlias(ATTRIBUTE_UNUSED
virDomainDefPtr def,
> else
> ret = virAsprintf(&controller->info.alias, "pci.%d",
controller->idx);
> break;
> + case VIR_DOMAIN_CONTROLLER_TYPE_IDE:
> + /* for any machine based on I440FX, the first (currently only)
> + * IDE controller is an integrated controller hardcoded with
> + * id "ide"
> + */
> + if (qemuDomainMachineIsI440FX(def) && controller->idx == 0)
> + ret = VIR_STRDUP(controller->info.alias, "ide");
Based on my review of 4/13 and my followup comment, there'd need to be
an "else" here with does the :
ret = virAsprintf(&controller->info.alias, "%s%d",
virDomainControllerTypeToString(controller->type),
controller->idx);
Right, that's why I put that extra "catchall" at the bottom of the
function. I wanted to avoid duplicating code.
> + break;
> + case 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(def) && controller->idx == 0)
> + ret = VIR_STRDUP(controller->info.alias, "ide");
similarly, else
ret = virAsprintf(&controller->info.alias, "%s%d",
virDomainControllerTypeToString(controller->type),
controller->idx);
... like this :-)
5:54 p.m.
New subject: [libvirt] [PATCH 06/13] qemu: add exceptions for alias names of primary sata/ide controllers
On 05/08/2015 02:12 PM, Laine Stump wrote:
On 05/08/2015 01:47 PM, John Ferlan wrote:
>
> On 05/05/2015 02:03 PM, Laine Stump wrote:
>> If a machine is Q35, the primary sata controller is hardcoded in qemu
>> to have the id "ide" (with no index in the name). Likewise on
>> 440fx-based machinetypes, the primary ide controller is called
>> "ide". All other SATA controllers will have the standard name
>> "sata%d", where %d is the index of the controller, and if any
>> additional IDE controllers are ever supported, they will have the name
>> "ide%d".
>> ---
>> src/qemu/qemu_command.c | 15 +++++++++++++++
>> 1 file changed, 15 insertions(+)
>>
>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>> index 340478c..89beeb3 100644
>> --- a/src/qemu/qemu_command.c
>> +++ b/src/qemu/qemu_command.c
>> @@ -1049,6 +1049,21 @@ qemuAssignDeviceControllerAlias(ATTRIBUTE_UNUSED
virDomainDefPtr def,
>> else
>> ret = virAsprintf(&controller->info.alias,
"pci.%d", controller->idx);
>> break;
>> + case VIR_DOMAIN_CONTROLLER_TYPE_IDE:
>> + /* for any machine based on I440FX, the first (currently only)
>> + * IDE controller is an integrated controller hardcoded with
>> + * id "ide"
>> + */
>> + if (qemuDomainMachineIsI440FX(def) && controller->idx == 0)
>> + ret = VIR_STRDUP(controller->info.alias, "ide");
> Based on my review of 4/13 and my followup comment, there'd need to be
> an "else" here with does the :
>
> ret = virAsprintf(&controller->info.alias, "%s%d",
> virDomainControllerTypeToString(controller->type),
> controller->idx);
Right, that's why I put that extra "catchall" at the bottom of the
function. I wanted to avoid duplicating code.
>
>
>> + break;
>> + case 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(def) && controller->idx == 0)
>> + ret = VIR_STRDUP(controller->info.alias, "ide");
> similarly, else
> ret = virAsprintf(&controller->info.alias, "%s%d",
> virDomainControllerTypeToString(controller->type),
> controller->idx);
... like this :-)
Right - not that I want to rewrite the code; however, I think the
purpose of using a switch is to catch all the options, especially with
the typecasting of the switch... While perhaps an if ... else if ...
would be more suitable for the exceptions such as this.
Maybe someone else has a strong enough opinion to warrant a fully
populated switch.
What you have works, but may not fit everyone's style/taste.
I'm OK with it though
John
2 a.m.
New subject: [libvirt] [PATCH 06/13] qemu: add exceptions for alias names of primary sata/ide controllers
On Tue, May 05, 2015 at 02:03:11PM -0400, Laine Stump wrote:
If a machine is Q35, the primary sata controller is hardcoded in
qemu
to have the id "ide" (with no index in the name). Likewise on
440fx-based machinetypes, the primary ide controller is called
"ide". All other SATA controllers will have the standard name
"sata%d", where %d is the index of the controller, and if any
additional IDE controllers are ever supported, they will have the name
"ide%d".
---
src/qemu/qemu_command.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 340478c..89beeb3 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1049,6 +1049,21 @@ qemuAssignDeviceControllerAlias(ATTRIBUTE_UNUSED virDomainDefPtr
def,
The attribute is now used.
Jan
else
ret = virAsprintf(&controller->info.alias, "pci.%d",
controller->idx);
break;
+ case VIR_DOMAIN_CONTROLLER_TYPE_IDE:
+ /* for any machine based on I440FX, the first (currently only)
+ * IDE controller is an integrated controller hardcoded with
+ * id "ide"
+ */
+ if (qemuDomainMachineIsI440FX(def) && controller->idx == 0)
+ ret = VIR_STRDUP(controller->info.alias, "ide");
+ break;
+ case 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(def) && controller->idx == 0)
+ ret = VIR_STRDUP(controller->info.alias, "ide");
+ break;
default:
break;
}
--
2.1.0
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
1:03 p.m.
New subject: [libvirt] [PATCH 07/13] qemu: use controller alias when constructing disk/controller args
This makes sure that that the commandlines generated for disk devices
and disk 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.
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".
---
src/qemu/qemu_command.c | 46 +++++++++++-----------
.../qemuxml2argv-disk-sata-device.args | 4 +-
2 files changed, 24 insertions(+), 26 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 89beeb3..b9f4c51 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:
@@ -4576,7 +4574,7 @@ qemuBuildControllerDevStr(virDomainDefPtr domainDef,
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:
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
7:01 p.m.
New subject: [libvirt] [PATCH 07/13] qemu: use controller alias when constructing disk/controller args
On 05/05/2015 02:03 PM, Laine Stump wrote:
This makes sure that that the commandlines generated for disk
devices
and disk 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.
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".
---
src/qemu/qemu_command.c | 46 +++++++++++-----------
.../qemuxml2argv-disk-sata-device.args | 4 +-
2 files changed, 24 insertions(+), 26 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 89beeb3..b9f4c51 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;
through here - no issues...
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:
@@ -4576,7 +4574,7 @@ qemuBuildControllerDevStr(virDomainDefPtr domainDef,
break;
case VIR_DOMAIN_CONTROLLER_TYPE_SATA:
- virBufferAsprintf(&buf, "ahci,id=ahci%d", def->idx);
+ virBufferAsprintf(&buf, "ahci,id=%s", def->info.alias);
break;
But these two feel like they should go with patch 8 since that deals
with qemuBuildControllerDevStr
I guess the difference for me is these two go direct to def->info.alias
while the others use virDomainControllerAliasFind
ACK - either way to handle this.
John
case VIR_DOMAIN_CONTROLLER_TYPE_USB:
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
1:03 p.m.
New subject: [libvirt] [PATCH 08/13] qemu: use alias for all controller ids in qemuBuildControllerDevStr
Some of the other controllers in this function still had their id's
hardcoded with a printf rather than getting it from alias in the
controller object.
---
src/qemu/qemu_command.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index b9f4c51..7adce0a 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -4557,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);
@@ -4570,7 +4569,7 @@ 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:
@@ -4594,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)) {
@@ -4609,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:
--
2.1.0
7:02 p.m.
New subject: [libvirt] [PATCH 08/13] qemu: use alias for all controller ids in qemuBuildControllerDevStr
On 05/05/2015 02:03 PM, Laine Stump wrote:
Some of the other controllers in this function still had their
id's
hardcoded with a printf rather than getting it from alias in the
controller object.
---
src/qemu/qemu_command.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
As w/ 7/13 - either merge these there or use his for all
qemuBuildControllerDevStr changes.
ACK - either way you do this.
John
1:03 p.m.
New subject: [libvirt] [PATCH 09/13] qemu: use controller object alias in commandline for virtio-serial device
The commandline generator for a virtio-serial device had a hardcoded
printf of "virtio-serial%d" as the id of the virtio-serial
controller. This patch changes it to use the alias of the controller
instead. Because the function that finds a controller alias requires a
pointer to the domainDef in order to get the list of controllers, the
arglist of a few functions had to have this added.
Once this was done, the literal string QEMU_VIRTIO_SERIAL_PREFIX was
no longer needed, so it has been removed.
---
src/qemu/qemu_command.c | 28 +++++++++++++++++-----------
src/qemu/qemu_command.h | 1 -
2 files changed, 17 insertions(+), 12 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 7adce0a..7f7f3f4 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -6300,10 +6300,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");
@@ -6335,12 +6338,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);
}
@@ -10889,6 +10893,7 @@ qemuBuildParallelChrDeviceStr(char **deviceStr,
static int
qemuBuildChannelChrDeviceStr(char **deviceStr,
+ virDomainDefPtr def,
virDomainChrDefPtr chr,
virQEMUCapsPtr qemuCaps)
{
@@ -10911,7 +10916,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;
@@ -10928,6 +10933,7 @@ qemuBuildChannelChrDeviceStr(char **deviceStr,
static int
qemuBuildConsoleChrDeviceStr(char **deviceStr,
+ virDomainDefPtr def,
virDomainChrDefPtr chr,
virQEMUCapsPtr qemuCaps)
{
@@ -10941,7 +10947,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;
@@ -10985,11 +10991,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 d066953..51e7f1f 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
--
2.1.0
7:05 p.m.
New subject: [libvirt] [PATCH 09/13] qemu: use controller object alias in commandline for virtio-serial device
On 05/05/2015 02:03 PM, Laine Stump wrote:
The commandline generator for a virtio-serial device had a hardcoded
printf of "virtio-serial%d" as the id of the virtio-serial
controller. This patch changes it to use the alias of the controller
instead. Because the function that finds a controller alias requires a
pointer to the domainDef in order to get the list of controllers, the
arglist of a few functions had to have this added.
Once this was done, the literal string QEMU_VIRTIO_SERIAL_PREFIX was
no longer needed, so it has been removed.
---
src/qemu/qemu_command.c | 28 +++++++++++++++++-----------
src/qemu/qemu_command.h | 1 -
2 files changed, 17 insertions(+), 12 deletions(-)
ACK - although a couple of notes (a cscope search on "bus=" finds :
qemuUSBId
- where there are decision points to use usb or usb%d
qemuBuildSCSIHostdevDevStr
- where bus=scsi%d is used
Can/Should either or both of those be changed as well?
John
10:16 a.m.
New subject: [libvirt] [PATCH 09/13] qemu: use controller object alias in commandline for virtio-serial device
On 05/08/2015 08:05 PM, John Ferlan wrote:
On 05/05/2015 02:03 PM, Laine Stump wrote:
> The commandline generator for a virtio-serial device had a hardcoded
> printf of "virtio-serial%d" as the id of the virtio-serial
> controller. This patch changes it to use the alias of the controller
> instead. Because the function that finds a controller alias requires a
> pointer to the domainDef in order to get the list of controllers, the
> arglist of a few functions had to have this added.
>
> Once this was done, the literal string QEMU_VIRTIO_SERIAL_PREFIX was
> no longer needed, so it has been removed.
> ---
> src/qemu/qemu_command.c | 28 +++++++++++++++++-----------
> src/qemu/qemu_command.h | 1 -
> 2 files changed, 17 insertions(+), 12 deletions(-)
>
ACK - although a couple of notes (a cscope search on "bus=" finds :
qemuUSBId
- where there are decision points to use usb or usb%d
Yeah. It looks like that is another exception (so another place where
the status XML isn't correctly reflecting the id used in qemu).
qemuBuildSCSIHostdevDevStr
- where bus=scsi%d is used
Yes, this one should be changed too.
Also, there is a place in qemuBuildDeviceAddressStr() where it is making
decisions about the name to use for the PCI bus based on whether or not
PCI_MULTIBUS is true. Instead, the function that builds the alias for
controllers should create the correct name for the one-and-only PCI
controller in this case, and qemuBuildDeviceAddressStr() should just
unconditionally use that name.
Should I merge all of these into a single patch? Or a separate patch for
each?
1:25 p.m.
New subject: [libvirt] [PATCH 09/13] qemu: use controller object alias in commandline for virtio-serial device
On 05/12/2015 11:16 AM, Laine Stump wrote:
On 05/08/2015 08:05 PM, John Ferlan wrote:
>
> On 05/05/2015 02:03 PM, Laine Stump wrote:
>> The commandline generator for a virtio-serial device had a hardcoded
>> printf of "virtio-serial%d" as the id of the virtio-serial
>> controller. This patch changes it to use the alias of the controller
>> instead. Because the function that finds a controller alias requires a
>> pointer to the domainDef in order to get the list of controllers, the
>> arglist of a few functions had to have this added.
>>
>> Once this was done, the literal string QEMU_VIRTIO_SERIAL_PREFIX was
>> no longer needed, so it has been removed.
>> ---
>> src/qemu/qemu_command.c | 28 +++++++++++++++++-----------
>> src/qemu/qemu_command.h | 1 -
>> 2 files changed, 17 insertions(+), 12 deletions(-)
>>
> ACK - although a couple of notes (a cscope search on "bus=" finds :
>
> qemuUSBId
>
> - where there are decision points to use usb or usb%d
Yeah. It looks like that is another exception (so another place where
the status XML isn't correctly reflecting the id used in qemu).
> qemuBuildSCSIHostdevDevStr
>
> - where bus=scsi%d is used
>
Yes, this one should be changed too.
Also, there is a place in qemuBuildDeviceAddressStr() where it is making
decisions about the name to use for the PCI bus based on whether or not
PCI_MULTIBUS is true. Instead, the function that builds the alias for
controllers should create the correct name for the one-and-only PCI
controller in this case, and qemuBuildDeviceAddressStr() should just
unconditionally use that name.
Should I merge all of these into a single patch? Or a separate patch for
each?
I'm fine with a single patch being submitted - although perhaps "for
review purposes" - combine 7-9 as one... then post the new areas with
the mindset to merge the patches before pushing. That way it makes it
easier to review/explain the new changes.
John
1:03 p.m.
New subject: [libvirt] [PATCH 10/13] 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)
---
...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 97c7fba..4712819 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -1413,12 +1413,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
7:15 p.m.
New subject: [libvirt] [PATCH 10/13] qemu: remove test for allowing ide controller in s390, rename usb tests
On 05/05/2015 02:03 PM, Laine Stump wrote:
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)
---
...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%)
ACK - hopefully anyone who looks to change them afterwards reads the
commit message ;-)
John
1:03 p.m.
New subject: [libvirt] [PATCH 11/13] qemu: log error when domain has an upsupported 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 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
8:15 p.m.
New subject: [libvirt] [PATCH 11/13] qemu: log error when domain has an upsupported IDE controller
$SUBJ: s/upsupported/unsupported
On 05/05/2015 02:03 PM, Laine Stump wrote:
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(-)
I'd say a "soft" ACK - things seem OK to me... Rules are defined, the
commit message is very descriptive, code motion for QEMU_CAPS_ICH9_AHCI,
and enforcing IDE rules What scares me most is the unknown of what
havoc or misconfigurations are "out there"... TBH: I close my eyes and
hope this code works ;-)...
Hopefully another set of eyes can look as well!
John
2:35 a.m.
New subject: [libvirt] [PATCH 11/13] qemu: log error when domain has an upsupported IDE controller
On Tue, May 05, 2015 at 02:03:16PM -0400, Laine Stump wrote:
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.
The error was just mentioned a few lines above, there's no need to
mention it twice in the commit message.
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
Dropping this sentence would make the commit message shorter.
- 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).
The cleanup should be in a separate patch to make review possible.
A shorter commit message would be helpful as well.
Jan
1:03 p.m.
New subject: [libvirt] [PATCH 12/13] qemu: only add channel arg to scsi-disk if qemu binary supports it
libvirt enforces bus (channel to qemu) == 0 for those qemu binaries
that don't support the channel argument to scsi disk devices, but
still adds "channel=0" in those cases. Apparently nobody with a qemu
old enough to not support channel ever uses these devices, because
they otherwise should get an error.
This patch only adds channel when the scsi-disk.channel capability is
set for the qemu binary. Most of the change in the patch is updating
patches to either 1) remove channel=0 from the .args file of the test,
or 2) reposition channel=0 to precede the bus arg (because this makes
the code cleaner/simpler) and change the DO_TEST() invocation for the
test to add QEMU_CAPS_SCSI_DISK_CHANNEL.
I'm uncommitted about whether or not it is worthwhile to push this
patch. On one hand, I'm fairly certain this is what is correct; on the
other hand nobody has ever complained about it, and at this point
almost everyone is using new enough qemu that it supports channel.
---
src/qemu/qemu_command.c | 7 +++---
.../qemuxml2argv-disk-drive-network-iscsi-lun.args | 2 +-
.../qemuxml2argv-disk-scsi-disk-split.args | 8 +++----
.../qemuxml2argv-disk-scsi-disk-vpd.args | 2 +-
.../qemuxml2argv-disk-scsi-disk-wwn.args | 4 ++--
.../qemuxml2argv-disk-scsi-lun-passthrough.args | 4 ++--
.../qemuxml2argv-disk-scsi-megasas.args | 2 +-
.../qemuxml2argv-disk-scsi-virtio-scsi.args | 2 +-
.../qemuxml2argv-disk-scsi-vscsi.args | 2 +-
.../qemuxml2argv-disk-virtio-scsi-ccw.args | 2 +-
.../qemuxml2argv-disk-virtio-scsi-cmd_per_lun.args | 2 +-
.../qemuxml2argv-disk-virtio-scsi-max_sectors.args | 2 +-
.../qemuxml2argv-disk-virtio-scsi-num_queues.args | 2 +-
.../qemuxml2argv-pseries-vio-user-assigned.args | 2 +-
.../qemuxml2argvdata/qemuxml2argv-pseries-vio.args | 2 +-
tests/qemuxml2argvtest.c | 25 +++++++++++++---------
16 files changed, 38 insertions(+), 32 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 72d425f..12de4ca 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -4136,7 +4136,9 @@ qemuBuildDriveDevStr(virDomainDefPtr def,
disk->info.addr.drive.bus,
disk->info.addr.drive.unit);
} else {
- if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SCSI_DISK_CHANNEL)) {
+ if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SCSI_DISK_CHANNEL)) {
+ virBufferAsprintf(&opt, ",channel=%d",
disk->info.addr.drive.bus);
+ } else {
if (disk->info.addr.drive.target > 7) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("This QEMU doesn't support target "
@@ -4153,9 +4155,8 @@ qemuBuildDriveDevStr(virDomainDefPtr def,
}
}
- virBufferAsprintf(&opt,
",bus=%s.0,channel=%d,scsi-id=%d,lun=%d",
+ virBufferAsprintf(&opt, ",bus=%s.0,scsi-id=%d,lun=%d",
contAlias,
- disk->info.addr.drive.bus,
disk->info.addr.drive.target,
disk->info.addr.drive.unit);
}
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-lun.args
b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-lun.args
index 109f2f8..4e4f8b1 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-lun.args
+++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-lun.args
@@ -3,5 +3,5 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test
QEMU_AUDIO_DRV=none \
-monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c \
-device virtio-scsi-pci,id=scsi0,bus=pci.0,addr=0x3 -usb \
-drive file=iscsi://example.org:3260/iqn.1992-01.com.example,if=none,\
-id=drive-scsi0-0-0-0,format=raw -device scsi-block,bus=scsi0.0,channel=0,\
+id=drive-scsi0-0-0-0,format=raw -device scsi-block,channel=0,bus=scsi0.0,\
scsi-id=0,lun=0,drive=drive-scsi0-0-0-0,id=scsi0-0-0-0
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-disk-split.args
b/tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-disk-split.args
index 87799b2..f0bc952 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-disk-split.args
+++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-disk-split.args
@@ -7,15 +7,15 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test
QEMU_AUDIO_DRV=none \
-device virtio-scsi-pci,id=scsi3,bus=pci.0,addr=0x6 \
-usb \
-drive file=/dev/HostVG/QEMUGuest1,if=none,id=drive-scsi0-0-1-0 \
--device scsi-cd,bus=scsi0.0,channel=0,scsi-id=1,lun=0,drive=drive-scsi0-0-1-0,\
+-device scsi-cd,channel=0,bus=scsi0.0,scsi-id=1,lun=0,drive=drive-scsi0-0-1-0,\
id=scsi0-0-1-0 \
-drive file=/dev/HostVG/QEMUGuest2,if=none,id=drive-scsi0-0-0-0 \
--device scsi-cd,bus=scsi0.0,channel=0,scsi-id=0,lun=0,drive=drive-scsi0-0-0-0,\
+-device scsi-cd,channel=0,bus=scsi0.0,scsi-id=0,lun=0,drive=drive-scsi0-0-0-0,\
id=scsi0-0-0-0 \
-drive file=/tmp/scsidisk.img,if=none,id=drive-scsi0-0-0-1 \
--device scsi-hd,bus=scsi0.0,channel=0,scsi-id=0,lun=1,drive=drive-scsi0-0-0-1,\
+-device scsi-hd,channel=0,bus=scsi0.0,scsi-id=0,lun=1,drive=drive-scsi0-0-0-1,\
id=scsi0-0-0-1 \
-drive file=/tmp/scsidisk.img,if=none,id=drive-scsi0-0-1-1 \
--device scsi-hd,bus=scsi0.0,channel=0,scsi-id=1,lun=1,drive=drive-scsi0-0-1-1,\
+-device scsi-hd,channel=0,bus=scsi0.0,scsi-id=1,lun=1,drive=drive-scsi0-0-1-1,\
id=scsi0-0-1-1 \
-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x7
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-disk-vpd.args
b/tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-disk-vpd.args
index aa6e639..23e4830 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-disk-vpd.args
+++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-disk-vpd.args
@@ -5,7 +5,7 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test
QEMU_AUDIO_DRV=none \
-device lsi,id=scsi1,bus=pci.0,addr=0x4 \
-usb \
-drive file=/dev/HostVG/QEMUGuest1,if=none,id=drive-scsi0-0-0-0 \
--device scsi-cd,bus=scsi0.0,channel=0,scsi-id=0,lun=0,drive=drive-scsi0-0-0-0,\
+-device scsi-cd,bus=scsi0.0,scsi-id=0,lun=0,drive=drive-scsi0-0-0-0,\
id=scsi0-0-0-0,vendor=SEAGATE,product=ST3146707LC \
-drive file=/dev/HostVG/QEMUGuest2,if=none,id=drive-scsi1-0-0 \
-device scsi-hd,bus=scsi1.0,scsi-id=0,drive=drive-scsi1-0-0,\
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-disk-wwn.args
b/tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-disk-wwn.args
index f2e1a95..28b9d02 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-disk-wwn.args
+++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-disk-wwn.args
@@ -5,9 +5,9 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test
QEMU_AUDIO_DRV=none \
-device lsi,id=scsi1,bus=pci.0,addr=0x4 \
-usb \
-drive file=/dev/HostVG/QEMUGuest1,if=none,id=drive-scsi0-0-1-0 \
--device scsi-cd,bus=scsi0.0,channel=0,scsi-id=1,lun=0,drive=drive-scsi0-0-1-0,\
+-device scsi-cd,channel=0,bus=scsi0.0,scsi-id=1,lun=0,drive=drive-scsi0-0-1-0,\
id=scsi0-0-1-0,wwn=0x5000c50015ea71ac \
-drive file=/dev/HostVG/QEMUGuest2,if=none,id=drive-scsi0-0-0-0 \
--device scsi-hd,bus=scsi0.0,channel=0,scsi-id=0,lun=0,drive=drive-scsi0-0-0-0,\
+-device scsi-hd,channel=0,bus=scsi0.0,scsi-id=0,lun=0,drive=drive-scsi0-0-0-0,\
id=scsi0-0-0-0,wwn=0x5000c50015ea71ad \
-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x5
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-lun-passthrough.args
b/tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-lun-passthrough.args
index 163b91d..e1dfc1a 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-lun-passthrough.args
+++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-lun-passthrough.args
@@ -6,9 +6,9 @@ pc -m 214 -smp 1 -nographic -nodefaults \
-device lsi,id=scsi1,bus=pci.0,addr=0x4 \
-usb \
-drive file=/dev/HostVG/QEMUGuest1,if=none,id=drive-scsi0-0-0-0 \
--device scsi-block,bus=scsi0.0,channel=0,scsi-id=0,lun=0,\
+-device scsi-block,channel=0,bus=scsi0.0,scsi-id=0,lun=0,\
drive=drive-scsi0-0-0-0,id=scsi0-0-0-0 \
-drive file=/dev/HostVG/QEMUGuest2,if=none,id=drive-scsi0-0-1-1 \
--device scsi-block,bus=scsi0.0,channel=0,scsi-id=1,lun=1,\
+-device scsi-block,channel=0,bus=scsi0.0,scsi-id=1,lun=1,\
drive=drive-scsi0-0-1-1,id=scsi0-0-1-1 \
-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x5
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-megasas.args
b/tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-megasas.args
index f20f25e..ebd1218 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-megasas.args
+++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-megasas.args
@@ -5,6 +5,6 @@ unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -device \
megasas,id=scsi0,bus=pci.0,addr=0x3 -usb -drive file=/dev/HostVG/QEMUGuest1,\
if=none,id=drive-ide0-0-0 -device ide-drive,bus=ide.0,unit=0,\
drive=drive-ide0-0-0,id=ide0-0-0 -drive file=/tmp/scsidisk.img,if=none,\
-id=drive-scsi0-0-4-0 -device scsi-disk,bus=scsi0.0,channel=0,scsi-id=4,lun=0,\
+id=drive-scsi0-0-4-0 -device scsi-disk,channel=0,bus=scsi0.0,scsi-id=4,lun=0,\
drive=drive-scsi0-0-4-0,id=scsi0-0-4-0 -device virtio-balloon-pci,\
id=balloon0,bus=pci.0,addr=0x4
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-virtio-scsi.args
b/tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-virtio-scsi.args
index de53ece..1063a1c 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-virtio-scsi.args
+++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-virtio-scsi.args
@@ -5,6 +5,6 @@ unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -device \
virtio-scsi-pci,id=scsi0,bus=pci.0,addr=0x3 -usb -drive file=/dev/HostVG/QEMUGuest1,\
if=none,id=drive-ide0-0-0 -device ide-drive,bus=ide.0,unit=0,\
drive=drive-ide0-0-0,id=ide0-0-0 -drive file=/tmp/scsidisk.img,if=none,\
-id=drive-scsi0-0-4-0 -device scsi-disk,bus=scsi0.0,channel=0,scsi-id=4,lun=0,\
+id=drive-scsi0-0-4-0 -device scsi-disk,channel=0,bus=scsi0.0,scsi-id=4,lun=0,\
drive=drive-scsi0-0-4-0,id=scsi0-0-4-0 -device virtio-balloon-pci,\
id=balloon0,bus=pci.0,addr=0x4
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-vscsi.args
b/tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-vscsi.args
index a353b06..32d6e71 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-vscsi.args
+++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-vscsi.args
@@ -6,6 +6,6 @@ id=scsi0,reg=0x2000 -usb \
-drive file=/dev/HostVG/QEMUGuest1,if=none,id=drive-ide0-0-0 \
-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \
-drive file=/tmp/scsidisk.img,if=none,id=drive-scsi0-0-3-0 \
--device scsi-disk,bus=scsi0.0,channel=0,scsi-id=3,lun=0,\
+-device scsi-disk,bus=scsi0.0,scsi-id=3,lun=0,\
drive=drive-scsi0-0-3-0,id=scsi0-0-3-0 \
-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-scsi-ccw.args
b/tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-scsi-ccw.args
index 7b4d24b..8bfa847 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-scsi-ccw.args
+++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-scsi-ccw.args
@@ -7,6 +7,6 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test
QEMU_AUDIO_DRV=none \
-device virtio-blk-ccw,devno=fe.0.0000,drive=drive-virtio-disk0,\
id=virtio-disk0 \
-drive file=/dev/HostVG/QEMUGuest2,if=none,id=drive-scsi0-0-4-0 \
--device scsi-disk,bus=scsi0.0,channel=0,scsi-id=4,lun=0,\
+-device scsi-disk,channel=0,bus=scsi0.0,scsi-id=4,lun=0,\
drive=drive-scsi0-0-4-0,id=scsi0-0-4-0 \
-device virtio-balloon-ccw,id=balloon0,devno=fe.0.000a
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-scsi-cmd_per_lun.args
b/tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-scsi-cmd_per_lun.args
index 2c75790..3b8450c 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-scsi-cmd_per_lun.args
+++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-scsi-cmd_per_lun.args
@@ -4,6 +4,6 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test
QEMU_AUDIO_DRV=none \
-device virtio-scsi-pci,id=scsi0,cmd_per_lun=50,bus=pci.0,addr=0x3 \
-usb \
-drive file=/dev/HostVG/QEMUGuest1,if=none,id=drive-scsi0-0-0-0 \
--device scsi-disk,bus=scsi0.0,channel=0,scsi-id=0,lun=0,\
+-device scsi-disk,channel=0,bus=scsi0.0,scsi-id=0,lun=0,\
drive=drive-scsi0-0-0-0,id=scsi0-0-0-0 \
-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x4
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-scsi-max_sectors.args
b/tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-scsi-max_sectors.args
index 895f379..913a7a2 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-scsi-max_sectors.args
+++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-scsi-max_sectors.args
@@ -4,6 +4,6 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test
QEMU_AUDIO_DRV=none \
-device virtio-scsi-pci,id=scsi0,max_sectors=512,bus=pci.0,addr=0x3 \
-usb \
-drive file=/dev/HostVG/QEMUGuest1,if=none,id=drive-scsi0-0-0-0 \
--device scsi-disk,bus=scsi0.0,channel=0,scsi-id=0,lun=0,\
+-device scsi-disk,channel=0,bus=scsi0.0,scsi-id=0,lun=0,\
drive=drive-scsi0-0-0-0,id=scsi0-0-0-0 \
-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x4
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-scsi-num_queues.args
b/tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-scsi-num_queues.args
index 4f03a79..fe14d23 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-scsi-num_queues.args
+++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-scsi-num_queues.args
@@ -4,6 +4,6 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test
QEMU_AUDIO_DRV=none \
-device virtio-scsi-pci,id=scsi0,num_queues=8,bus=pci.0,addr=0x3 \
-usb \
-drive file=/dev/HostVG/QEMUGuest1,if=none,id=drive-scsi0-0-0-0 \
--device scsi-disk,bus=scsi0.0,channel=0,scsi-id=0,lun=0,\
+-device scsi-disk,channel=0,bus=scsi0.0,scsi-id=0,lun=0,\
drive=drive-scsi0-0-0-0,id=scsi0-0-0-0 \
-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x4
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pseries-vio-user-assigned.args
b/tests/qemuxml2argvdata/qemuxml2argv-pseries-vio-user-assigned.args
index c62cc9a..c544e71 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-pseries-vio-user-assigned.args
+++ b/tests/qemuxml2argvdata/qemuxml2argv-pseries-vio-user-assigned.args
@@ -6,7 +6,7 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test
QEMU_AUDIO_DRV=none \
-device spapr-vscsi,id=scsi0,reg=0x2000 \
-device spapr-vscsi,id=scsi1,reg=0x30000000 \
-usb -drive file=/tmp/scsidisk.img,if=none,id=drive-scsi1-0-0-0 \
--device scsi-disk,bus=scsi1.0,channel=0,scsi-id=0,lun=0,\
+-device scsi-disk,bus=scsi1.0,scsi-id=0,lun=0,\
drive=drive-scsi1-0-0-0,id=scsi1-0-0-0 \
-chardev pty,id=charserial0 \
-device spapr-vty,chardev=charserial0,reg=0x20000000 \
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pseries-vio.args
b/tests/qemuxml2argvdata/qemuxml2argv-pseries-vio.args
index f7c3af0..aff243e 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-pseries-vio.args
+++ b/tests/qemuxml2argvdata/qemuxml2argv-pseries-vio.args
@@ -6,7 +6,7 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test
QEMU_AUDIO_DRV=none \
-device spapr-vscsi,id=scsi0,reg=0x2000 \
-device spapr-vscsi,id=scsi1,reg=0x3000 \
-usb -drive file=/tmp/scsidisk.img,if=none,id=drive-scsi1-0-0-0 \
--device scsi-disk,bus=scsi1.0,channel=0,scsi-id=0,lun=0,\
+-device scsi-disk,bus=scsi1.0,scsi-id=0,lun=0,\
drive=drive-scsi1-0-0-0,id=scsi1-0-0-0 \
-chardev pty,id=charserial0 \
-device spapr-vty,chardev=charserial0,reg=0x30000000 \
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 4712819..1787eb7 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -726,7 +726,8 @@ mymain(void)
DO_TEST("disk-virtio-ccw-many", QEMU_CAPS_DRIVE,
QEMU_CAPS_DEVICE, QEMU_CAPS_VIRTIO_CCW, QEMU_CAPS_VIRTIO_S390);
DO_TEST("disk-virtio-scsi-ccw", QEMU_CAPS_DRIVE, QEMU_CAPS_VIRTIO_SCSI,
- QEMU_CAPS_DEVICE, QEMU_CAPS_VIRTIO_CCW, QEMU_CAPS_VIRTIO_S390);
+ QEMU_CAPS_DEVICE, QEMU_CAPS_VIRTIO_CCW, QEMU_CAPS_VIRTIO_S390,
+ QEMU_CAPS_SCSI_DISK_CHANNEL);
DO_TEST("disk-order",
QEMU_CAPS_DRIVE, QEMU_CAPS_DEVICE, QEMU_CAPS_DRIVE_BOOT,
QEMU_CAPS_VIRTIO_BLK_SCSI, QEMU_CAPS_VIRTIO_BLK_SG_IO);
@@ -792,7 +793,8 @@ mymain(void)
DO_TEST("disk-drive-network-iscsi-lun",
QEMU_CAPS_DRIVE, QEMU_CAPS_DEVICE, QEMU_CAPS_DRIVE_FORMAT,
QEMU_CAPS_NODEFCONFIG, QEMU_CAPS_VIRTIO_SCSI,
- QEMU_CAPS_VIRTIO_BLK_SG_IO, QEMU_CAPS_SCSI_BLOCK);
+ QEMU_CAPS_VIRTIO_BLK_SG_IO, QEMU_CAPS_SCSI_BLOCK,
+ QEMU_CAPS_SCSI_DISK_CHANNEL);
DO_TEST("disk-drive-network-gluster",
QEMU_CAPS_DRIVE, QEMU_CAPS_DRIVE_FORMAT);
DO_TEST("disk-drive-network-rbd",
@@ -827,11 +829,13 @@ mymain(void)
QEMU_CAPS_SCSI_LSI);
DO_TEST("disk-scsi-disk-split",
QEMU_CAPS_DRIVE, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG,
- QEMU_CAPS_SCSI_CD, QEMU_CAPS_SCSI_LSI, QEMU_CAPS_VIRTIO_SCSI);
+ QEMU_CAPS_SCSI_CD, QEMU_CAPS_SCSI_LSI, QEMU_CAPS_VIRTIO_SCSI,
+ QEMU_CAPS_SCSI_DISK_CHANNEL);
DO_TEST("disk-scsi-disk-wwn",
QEMU_CAPS_DRIVE, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG,
QEMU_CAPS_SCSI_CD, QEMU_CAPS_SCSI_LSI, QEMU_CAPS_VIRTIO_SCSI,
- QEMU_CAPS_SCSI_DISK_WWN);
+ QEMU_CAPS_SCSI_DISK_WWN, QEMU_CAPS_SCSI_DISK_CHANNEL);
+
DO_TEST("disk-scsi-disk-vpd",
QEMU_CAPS_DRIVE, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG,
QEMU_CAPS_SCSI_CD, QEMU_CAPS_SCSI_LSI, QEMU_CAPS_VIRTIO_SCSI,
@@ -844,19 +848,19 @@ mymain(void)
QEMU_CAPS_DRIVE, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG);
DO_TEST("disk-scsi-virtio-scsi",
QEMU_CAPS_DRIVE, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG,
- QEMU_CAPS_VIRTIO_SCSI);
+ QEMU_CAPS_VIRTIO_SCSI, QEMU_CAPS_SCSI_DISK_CHANNEL);
DO_TEST("disk-virtio-scsi-num_queues",
QEMU_CAPS_DRIVE, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG,
- QEMU_CAPS_VIRTIO_SCSI);
+ QEMU_CAPS_VIRTIO_SCSI, QEMU_CAPS_SCSI_DISK_CHANNEL);
DO_TEST("disk-virtio-scsi-cmd_per_lun",
QEMU_CAPS_DRIVE, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG,
- QEMU_CAPS_VIRTIO_SCSI);
+ QEMU_CAPS_VIRTIO_SCSI, QEMU_CAPS_SCSI_DISK_CHANNEL);
DO_TEST("disk-virtio-scsi-max_sectors",
QEMU_CAPS_DRIVE, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG,
- QEMU_CAPS_VIRTIO_SCSI);
+ QEMU_CAPS_VIRTIO_SCSI, QEMU_CAPS_SCSI_DISK_CHANNEL);
DO_TEST("disk-scsi-megasas",
QEMU_CAPS_DRIVE, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG,
- QEMU_CAPS_SCSI_MEGASAS);
+ QEMU_CAPS_SCSI_MEGASAS, QEMU_CAPS_SCSI_DISK_CHANNEL);
DO_TEST("disk-sata-device",
QEMU_CAPS_DRIVE, QEMU_CAPS_DEVICE,
QEMU_CAPS_NODEFCONFIG, QEMU_CAPS_ICH9_AHCI);
@@ -894,7 +898,8 @@ mymain(void)
QEMU_CAPS_DRIVE,
QEMU_CAPS_DEVICE,
QEMU_CAPS_SCSI_BLOCK, QEMU_CAPS_VIRTIO_BLK_SG_IO,
- QEMU_CAPS_SCSI_LSI, QEMU_CAPS_VIRTIO_SCSI);
+ QEMU_CAPS_SCSI_LSI, QEMU_CAPS_VIRTIO_SCSI,
+ QEMU_CAPS_SCSI_DISK_CHANNEL);
DO_TEST("disk-serial",
QEMU_CAPS_KVM,
QEMU_CAPS_DEVICE,
--
2.1.0
8:44 p.m.
New subject: [libvirt] [PATCH 12/13] qemu: only add channel arg to scsi-disk if qemu binary supports it
On 05/05/2015 02:03 PM, Laine Stump wrote:
libvirt enforces bus (channel to qemu) == 0 for those qemu binaries
that don't support the channel argument to scsi disk devices, but
still adds "channel=0" in those cases. Apparently nobody with a qemu
old enough to not support channel ever uses these devices, because
they otherwise should get an error.
This patch only adds channel when the scsi-disk.channel capability is
set for the qemu binary. Most of the change in the patch is updating
patches to either 1) remove channel=0 from the .args file of the test,
or 2) reposition channel=0 to precede the bus arg (because this makes
the code cleaner/simpler) and change the DO_TEST() invocation for the
test to add QEMU_CAPS_SCSI_DISK_CHANNEL.
I'm uncommitted about whether or not it is worthwhile to push this
patch. On one hand, I'm fairly certain this is what is correct; on the
other hand nobody has ever complained about it, and at this point
almost everyone is using new enough qemu that it supports channel.
---
src/qemu/qemu_command.c | 7 +++---
.../qemuxml2argv-disk-drive-network-iscsi-lun.args | 2 +-
.../qemuxml2argv-disk-scsi-disk-split.args | 8 +++----
.../qemuxml2argv-disk-scsi-disk-vpd.args | 2 +-
.../qemuxml2argv-disk-scsi-disk-wwn.args | 4 ++--
.../qemuxml2argv-disk-scsi-lun-passthrough.args | 4 ++--
.../qemuxml2argv-disk-scsi-megasas.args | 2 +-
.../qemuxml2argv-disk-scsi-virtio-scsi.args | 2 +-
.../qemuxml2argv-disk-scsi-vscsi.args | 2 +-
.../qemuxml2argv-disk-virtio-scsi-ccw.args | 2 +-
.../qemuxml2argv-disk-virtio-scsi-cmd_per_lun.args | 2 +-
.../qemuxml2argv-disk-virtio-scsi-max_sectors.args | 2 +-
.../qemuxml2argv-disk-virtio-scsi-num_queues.args | 2 +-
.../qemuxml2argv-pseries-vio-user-assigned.args | 2 +-
.../qemuxml2argvdata/qemuxml2argv-pseries-vio.args | 2 +-
tests/qemuxml2argvtest.c | 25 +++++++++++++---------
16 files changed, 38 insertions(+), 32 deletions(-)
I'm in agreement with you vis-a-vis whether it's worthwhile. Nothing's
broken, so the axiom may apply. However, we don't know if it's broken
"under the covers" since we don't know the affect of adding channel=# to
a qemu that doesn't support it since we don't seem to have one of those
today.
OTOH since you added the QEMU_CAPS_SCSI_DISK_CHANNEL bit in and that
alone didn't break anything, then it would appear this change should be
OK. Other than changing the order/placement of channel (which seems to
be a non-issue), it seems these changes are fine.
Should I assume you've started guests with all these changes? And
perhaps restart libvirtd to make sure noone disappears?
quasi-ACK, your call depending on your comfort level.
John
11:25 a.m.
New subject: [libvirt] [PATCH 12/13] qemu: only add channel arg to scsi-disk if qemu binary supports it
On 05/08/2015 09:44 PM, John Ferlan wrote:
On 05/05/2015 02:03 PM, Laine Stump wrote:
> libvirt enforces bus (channel to qemu) == 0 for those qemu binaries
> that don't support the channel argument to scsi disk devices, but
> still adds "channel=0" in those cases. Apparently nobody with a qemu
> old enough to not support channel ever uses these devices, because
> they otherwise should get an error.
>
> This patch only adds channel when the scsi-disk.channel capability is
> set for the qemu binary. Most of the change in the patch is updating
> patches to either 1) remove channel=0 from the .args file of the test,
> or 2) reposition channel=0 to precede the bus arg (because this makes
> the code cleaner/simpler) and change the DO_TEST() invocation for the
> test to add QEMU_CAPS_SCSI_DISK_CHANNEL.
>
> I'm uncommitted about whether or not it is worthwhile to push this
> patch. On one hand, I'm fairly certain this is what is correct; on the
> other hand nobody has ever complained about it, and at this point
> almost everyone is using new enough qemu that it supports channel.
> ---
> src/qemu/qemu_command.c | 7 +++---
> .../qemuxml2argv-disk-drive-network-iscsi-lun.args | 2 +-
> .../qemuxml2argv-disk-scsi-disk-split.args | 8 +++----
> .../qemuxml2argv-disk-scsi-disk-vpd.args | 2 +-
> .../qemuxml2argv-disk-scsi-disk-wwn.args | 4 ++--
> .../qemuxml2argv-disk-scsi-lun-passthrough.args | 4 ++--
> .../qemuxml2argv-disk-scsi-megasas.args | 2 +-
> .../qemuxml2argv-disk-scsi-virtio-scsi.args | 2 +-
> .../qemuxml2argv-disk-scsi-vscsi.args | 2 +-
> .../qemuxml2argv-disk-virtio-scsi-ccw.args | 2 +-
> .../qemuxml2argv-disk-virtio-scsi-cmd_per_lun.args | 2 +-
> .../qemuxml2argv-disk-virtio-scsi-max_sectors.args | 2 +-
> .../qemuxml2argv-disk-virtio-scsi-num_queues.args | 2 +-
> .../qemuxml2argv-pseries-vio-user-assigned.args | 2 +-
> .../qemuxml2argvdata/qemuxml2argv-pseries-vio.args | 2 +-
> tests/qemuxml2argvtest.c | 25 +++++++++++++---------
> 16 files changed, 38 insertions(+), 32 deletions(-)
>
I'm in agreement with you vis-a-vis whether it's worthwhile. Nothing's
broken, so the axiom may apply. However, we don't know if it's broken
"under the covers" since we don't know the affect of adding channel=# to
a qemu that doesn't support it since we don't seem to have one of those
today.
OTOH since you added the QEMU_CAPS_SCSI_DISK_CHANNEL bit in and that
alone didn't break anything, then it would appear this change should be
OK. Other than changing the order/placement of channel (which seems to
be a non-issue), it seems these changes are fine.
Should I assume you've started guests with all these changes? And
perhaps restart libvirtd to make sure noone disappears?
I did restart libvirtd with a running guest with SCSI disk, no problems,
and then destroyed and restart the guest (so it uses the different
argument ordering), no problems. I don't have a reasonable way to test
the case when channel= isn't supported, since that has even been
backported to RHEL6.
quasi-ACK, your call depending on your comfort level.
I'm planning on pushing this later today, if there are no objections.
1:37 p.m.
New subject: [libvirt] [PATCH 12/13] qemu: only add channel arg to scsi-disk if qemu binary supports it
On 05/14/2015 12:25 PM, Laine Stump wrote:
On 05/08/2015 09:44 PM, John Ferlan wrote:
> On 05/05/2015 02:03 PM, Laine Stump wrote:
>> libvirt enforces bus (channel to qemu) == 0 for those qemu binaries
>> that don't support the channel argument to scsi disk devices, but
>> still adds "channel=0" in those cases. Apparently nobody with a qemu
>> old enough to not support channel ever uses these devices, because
>> they otherwise should get an error.
>>
>> This patch only adds channel when the scsi-disk.channel capability is
>> set for the qemu binary. Most of the change in the patch is updating
>> patches to either 1) remove channel=0 from the .args file of the test,
>> or 2) reposition channel=0 to precede the bus arg (because this makes
>> the code cleaner/simpler) and change the DO_TEST() invocation for the
>> test to add QEMU_CAPS_SCSI_DISK_CHANNEL.
>>
>> I'm uncommitted about whether or not it is worthwhile to push this
>> patch. On one hand, I'm fairly certain this is what is correct; on the
>> other hand nobody has ever complained about it, and at this point
>> almost everyone is using new enough qemu that it supports channel.
>> ---
>> src/qemu/qemu_command.c | 7 +++---
>> .../qemuxml2argv-disk-drive-network-iscsi-lun.args | 2 +-
>> .../qemuxml2argv-disk-scsi-disk-split.args | 8 +++----
>> .../qemuxml2argv-disk-scsi-disk-vpd.args | 2 +-
>> .../qemuxml2argv-disk-scsi-disk-wwn.args | 4 ++--
>> .../qemuxml2argv-disk-scsi-lun-passthrough.args | 4 ++--
>> .../qemuxml2argv-disk-scsi-megasas.args | 2 +-
>> .../qemuxml2argv-disk-scsi-virtio-scsi.args | 2 +-
>> .../qemuxml2argv-disk-scsi-vscsi.args | 2 +-
>> .../qemuxml2argv-disk-virtio-scsi-ccw.args | 2 +-
>> .../qemuxml2argv-disk-virtio-scsi-cmd_per_lun.args | 2 +-
>> .../qemuxml2argv-disk-virtio-scsi-max_sectors.args | 2 +-
>> .../qemuxml2argv-disk-virtio-scsi-num_queues.args | 2 +-
>> .../qemuxml2argv-pseries-vio-user-assigned.args | 2 +-
>> .../qemuxml2argvdata/qemuxml2argv-pseries-vio.args | 2 +-
>> tests/qemuxml2argvtest.c | 25 +++++++++++++---------
>> 16 files changed, 38 insertions(+), 32 deletions(-)
>>
> I'm in agreement with you vis-a-vis whether it's worthwhile. Nothing's
> broken, so the axiom may apply. However, we don't know if it's broken
> "under the covers" since we don't know the affect of adding channel=#
to
> a qemu that doesn't support it since we don't seem to have one of those
> today.
>
> OTOH since you added the QEMU_CAPS_SCSI_DISK_CHANNEL bit in and that
> alone didn't break anything, then it would appear this change should be
> OK. Other than changing the order/placement of channel (which seems to
> be a non-issue), it seems these changes are fine.
>
> Should I assume you've started guests with all these changes? And
> perhaps restart libvirtd to make sure noone disappears?
I did restart libvirtd with a running guest with SCSI disk, no problems,
and then destroyed and restart the guest (so it uses the different
argument ordering), no problems. I don't have a reasonable way to test
the case when channel= isn't supported, since that has even been
backported to RHEL6.
> quasi-ACK, your call depending on your comfort level.
I'm planning on pushing this later today, if there are no objections.
Along with RHEL6/CentOS6 supporting channel, RHEL5 is old enough that it
doesn't even support -device, so I have nothing of the proper vintage to
test for non-support of channel, and this makes me doubt that anybody
anywhere has a system that would be affected by this patch, so I'm
dropping it too.
3:26 a.m.
New subject: [libvirt] [PATCH 12/13] qemu: only add channel arg to scsi-disk if qemu binary supports it
On Tue, May 05, 2015 at 02:03:17PM -0400, Laine Stump wrote:
libvirt enforces bus (channel to qemu) == 0 for those qemu binaries
that don't support the channel argument to scsi disk devices, but
still adds "channel=0" in those cases. Apparently nobody with a qemu
old enough to not support channel ever uses these devices, because
they otherwise should get an error.
This patch only adds channel when the scsi-disk.channel capability is
set for the qemu binary. Most of the change in the patch is updating
patches to either 1) remove channel=0 from the .args file of the test,
or 2) reposition channel=0 to precede the bus arg (because this makes
the code cleaner/simpler) and change the DO_TEST() invocation for the
test to add QEMU_CAPS_SCSI_DISK_CHANNEL.
I'm uncommitted about whether or not it is worthwhile to push this
patch. On one hand, I'm fairly certain this is what is correct; on the
other hand nobody has ever complained about it, and at this point
almost everyone is using new enough qemu that it supports channel.
---
Can we introduce a new rule that allows us to delete functionality that
was broken for three years and nobody complained? :)
Similarly, the last patch adding an error message when QEMU_CAPS_DEVICE
is missing is practically dead code - I doubt someone is running qemu
that old with the latest libvirt.
Jan
11:29 a.m.
New subject: [libvirt] [PATCH 12/13] qemu: only add channel arg to scsi-disk if qemu binary supports it
On 05/11/2015 04:26 AM, Ján Tomko wrote:
On Tue, May 05, 2015 at 02:03:17PM -0400, Laine Stump wrote:
> libvirt enforces bus (channel to qemu) == 0 for those qemu binaries
> that don't support the channel argument to scsi disk devices, but
> still adds "channel=0" in those cases. Apparently nobody with a qemu
> old enough to not support channel ever uses these devices, because
> they otherwise should get an error.
>
> This patch only adds channel when the scsi-disk.channel capability is
> set for the qemu binary. Most of the change in the patch is updating
> patches to either 1) remove channel=0 from the .args file of the test,
> or 2) reposition channel=0 to precede the bus arg (because this makes
> the code cleaner/simpler) and change the DO_TEST() invocation for the
> test to add QEMU_CAPS_SCSI_DISK_CHANNEL.
>
> I'm uncommitted about whether or not it is worthwhile to push this
> patch. On one hand, I'm fairly certain this is what is correct; on the
> other hand nobody has ever complained about it, and at this point
> almost everyone is using new enough qemu that it supports channel.
> ---
Can we introduce a new rule that allows us to delete functionality that
was broken for three years and nobody complained? :)
Similarly, the last patch adding an error message when QEMU_CAPS_DEVICE
is missing is practically dead code - I doubt someone is running qemu
that old with the latest libvirt.
If you're referring to patch 13 of this series, it actually *removes* an
error condition when -device *is* supported. We had been limiting scsi
devices to only bus='0' due to a limitation that only applied when not
using -device. So that patch adds extra functionality to hosts with
newer qemu.
1:03 p.m.
New subject: [libvirt] [PATCH 13/13] qemu: allow bus != 0 for scsi-disk when -device is used
All the way back at the end of 2009, commit d78554d8 added a check
that prevented scsi disks from having a bus != 0 due to problems that
caused (noted in the comments). At that time, -device wasn't supported
by qemu, so the -drive parameter had to identify each disk by bus
type (the "if" option), bus#, and unit#.
Since that time qemu has added the ability to provide such details
about a disk in a -device parameter, which will contain an id, and
that id can be referenced in -drive rather than giving
type/bus/unit. Since no bus= option is present in the -drive string in
this case, the problem with a non-0 bus is presumably no longer
present, so this patch makes the restriction active only when -device
isn't used.
qemuxml2argv-disk-scsi-lun-passthrough has been modified to set non-0
buses to assure that the restriction is no longer valid.
---
src/qemu/qemu_command.c | 13 +++++++++----
.../qemuxml2argv-disk-scsi-lun-passthrough.args | 12 ++++++------
.../qemuxml2argv-disk-scsi-lun-passthrough.xml | 4 ++--
3 files changed, 17 insertions(+), 12 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 12de4ca..260a468 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -3546,10 +3546,15 @@ qemuBuildDriveStr(virConnectPtr conn,
goto error;
}
- /* Setting bus= attr for SCSI drives, causes a controller
- * to be created. Yes this is slightly odd. It is not possible
- * to have > 1 bus on a SCSI controller (yet). */
- if (disk->info.addr.drive.bus != 0) {
+ /* Setting bus= attr (only happens when *not* using -device to
+ * specify the other half of the disk device) for SCSI drives,
+ * causes a controller to be created. Yes this is slightly
+ * odd. And it means that is not possible to have > 1 bus on a
+ * SCSI controller unless -device is supported/used (at this
+ * point only very old qemu binaries don't support
+ * -device). */
+ if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE) &&
+ disk->info.addr.drive.bus != 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
"%s", _("SCSI controller only supports 1
bus"));
goto error;
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-lun-passthrough.args
b/tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-lun-passthrough.args
index e1dfc1a..739ceb4 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-lun-passthrough.args
+++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-lun-passthrough.args
@@ -5,10 +5,10 @@ pc -m 214 -smp 1 -nographic -nodefaults \
-device virtio-scsi-pci,id=scsi0,bus=pci.0,addr=0x3 \
-device lsi,id=scsi1,bus=pci.0,addr=0x4 \
-usb \
--drive file=/dev/HostVG/QEMUGuest1,if=none,id=drive-scsi0-0-0-0 \
--device scsi-block,channel=0,bus=scsi0.0,scsi-id=0,lun=0,\
-drive=drive-scsi0-0-0-0,id=scsi0-0-0-0 \
--drive file=/dev/HostVG/QEMUGuest2,if=none,id=drive-scsi0-0-1-1 \
--device scsi-block,channel=0,bus=scsi0.0,scsi-id=1,lun=1,\
-drive=drive-scsi0-0-1-1,id=scsi0-0-1-1 \
+-drive file=/dev/HostVG/QEMUGuest1,if=none,id=drive-scsi0-1-0-0 \
+-device scsi-block,channel=1,bus=scsi0.0,scsi-id=0,lun=0,\
+drive=drive-scsi0-1-0-0,id=scsi0-1-0-0 \
+-drive file=/dev/HostVG/QEMUGuest2,if=none,id=drive-scsi0-2-1-1 \
+-device scsi-block,channel=2,bus=scsi0.0,scsi-id=1,lun=1,\
+drive=drive-scsi0-2-1-1,id=scsi0-2-1-1 \
-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x5
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-lun-passthrough.xml
b/tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-lun-passthrough.xml
index 3858ede..84dd12e 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-lun-passthrough.xml
+++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-lun-passthrough.xml
@@ -17,12 +17,12 @@
<disk type='block' device='lun'>
<source dev='/dev/HostVG/QEMUGuest1'/>
<target dev='hda' bus='scsi'/>
- <address type='drive' controller='0' bus='0'
target='0' unit='0'/>
+ <address type='drive' controller='0' bus='1'
target='0' unit='0'/>
</disk>
<disk type='block' device='lun'>
<source dev='/dev/HostVG/QEMUGuest2'/>
<target dev='hdb' bus='scsi'/>
- <address type='drive' controller='0' bus='0'
target='1' unit='1'/>
+ <address type='drive' controller='0' bus='2'
target='1' unit='1'/>
</disk>
<controller type='scsi' index='0'
model='virtio-scsi'/>
<controller type='scsi' index='1' model='lsilogic'/>
--
2.1.0
8:50 p.m.
New subject: [libvirt] [PATCH 13/13] qemu: allow bus != 0 for scsi-disk when -device is used
On 05/05/2015 02:03 PM, Laine Stump wrote:
All the way back at the end of 2009, commit d78554d8 added a check
that prevented scsi disks from having a bus != 0 due to problems that
caused (noted in the comments). At that time, -device wasn't supported
by qemu, so the -drive parameter had to identify each disk by bus
type (the "if" option), bus#, and unit#.
Since that time qemu has added the ability to provide such details
about a disk in a -device parameter, which will contain an id, and
that id can be referenced in -drive rather than giving
type/bus/unit. Since no bus= option is present in the -drive string in
this case, the problem with a non-0 bus is presumably no longer
present, so this patch makes the restriction active only when -device
isn't used.
qemuxml2argv-disk-scsi-lun-passthrough has been modified to set non-0
buses to assure that the restriction is no longer valid.
---
src/qemu/qemu_command.c | 13 +++++++++----
.../qemuxml2argv-disk-scsi-lun-passthrough.args | 12 ++++++------
.../qemuxml2argv-disk-scsi-lun-passthrough.xml | 4 ++--
3 files changed, 17 insertions(+), 12 deletions(-)
Seems reasonable to me... ACK
John
1:08 p.m.
New subject: [libvirt] [PATCH 13/13] qemu: allow bus != 0 for scsi-disk when -device is used
On 05/08/2015 09:50 PM, John Ferlan wrote:
On 05/05/2015 02:03 PM, Laine Stump wrote:
> All the way back at the end of 2009, commit d78554d8 added a check
> that prevented scsi disks from having a bus != 0 due to problems that
> caused (noted in the comments). At that time, -device wasn't supported
> by qemu, so the -drive parameter had to identify each disk by bus
> type (the "if" option), bus#, and unit#.
>
> Since that time qemu has added the ability to provide such details
> about a disk in a -device parameter, which will contain an id, and
> that id can be referenced in -drive rather than giving
> type/bus/unit. Since no bus= option is present in the -drive string in
> this case, the problem with a non-0 bus is presumably no longer
> present, so this patch makes the restriction active only when -device
> isn't used.
>
> qemuxml2argv-disk-scsi-lun-passthrough has been modified to set non-0
> buses to assure that the restriction is no longer valid.
> ---
> src/qemu/qemu_command.c | 13 +++++++++----
> .../qemuxml2argv-disk-scsi-lun-passthrough.args | 12 ++++++------
> .../qemuxml2argv-disk-scsi-lun-passthrough.xml | 4 ++--
> 3 files changed, 17 insertions(+), 12 deletions(-)
>
Seems reasonable to me... ACK
I think the comment in the code was unintentionally misleading (at least
to me). After testing this and looking into it further, I think I was
wrong, and that bus *should* always be limited to 0 (see the comments in
virDomainDiskDefAssignAddress()), so I'm dropping this patch.
3480
days inactive
3489
days old
41 comments
3 participants
participants (3)
-
John Ferlan -
Ján Tomko -
Laine Stump