[libvirt] [PATCH 00/13] qemu: fix device alias usage, ide controllers, scsi options
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
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
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
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
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
--- 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
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;
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
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; }
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*()
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
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.
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
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;
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.
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
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; }
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 :-)
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
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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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
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
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
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
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
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
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?
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
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
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
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
$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
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
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
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
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.
On 05/14/2015 12:25 PM, Laine Stump 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
On 05/08/2015 09:44 PM, John Ferlan wrote: 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.
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
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.
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
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
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.
participants (3)
-
John Ferlan -
Ján Tomko -
Laine Stump