15/08/13 12:58, Daniel P. Berrange wrote:
On Thu, Aug 15, 2013 at 12:55:26PM +0200, anonym wrote:
13/08/13 16:15, Daniel P. Berrange wrote:
On Tue, Aug 13, 2013 at 01:52:33PM +0200, Fred A. Kemp wrote:
From: "Fred A. Kemp" <anonym@riseup.net>
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index b811e1d..03fb798 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -8044,10 +8044,10 @@ qemuBuildCommandLine(virConnectPtr conn, bool withDeviceArg = false; bool deviceFlagMasked = false;
- /* Unless we have -device, then USB disks need special - handling */ + /* Unless we have `-device usb-storage`, then USB disks + need special handling */ if ((disk->bus == VIR_DOMAIN_DISK_BUS_USB) && - !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) { + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_USB_STORAGE)) { if (disk->device == VIR_DOMAIN_DISK_DEVICE_DISK) { virCommandAddArg(cmd, "-usbdevice"); virCommandAddArgFormat(cmd, "disk:%s", disk->src);
Hmm, I'm not sure this logic change is right.
Previously if you have -device support, but 'usb-storage' was not available, libvirt would try to start the guest with -device & then QEMU would report an error.
With this change, if you have -device support, but 'usb-storage' was not available, then libvirt is going to fallback to using the legacy '-usbdevice' support. This is not good.
I fail to see why this is not good. I thought '-usbdevice' was the correct way do handle USB disks if 'usb-storage' is missing. Whether we have '-device' or seems beside the point; if we have 'usb-storage', then it's implied we have '-device', and if we don't, '-device' is worthless and '-usbdevice' is the way to go. Letting QEMU fail and die when we have '-device' but not 'usb-storage' seems worse than just falling back to '-usbdevice'.
What am I missing?
If -device exists, we must *never* use the -usbdevice syntax. This is a legacy syntax that is only there for compatibility with apps that predate the introduction of -device syntax.
Without knowing the exact development history of qemu, I assumed it could be the case the we have '-device' but 'usb-storage' hadn't been implemented yet (so '-usbdevice' was still the way to go).
If the 'usb-storage' device does not exist, then '-usbdevice disk' is not going to work either since it uses the same code internally.
Note that the QEMU_CAPS_DEVICE_USB_STORAGE capability I add only checks if '-device usb-storage' is supported (by parsing 'qemu -device ?' like other, similar caps) and has nothing to do with '-usbdevice'.
Adding an explicit check for 'usb-storage' is a fine goal, but we should be doing that check in the branch of this if() that handles '-device usb-storage', so we don't exercise the -usbdevice branch
So, what if I drop the above chunk, and do the following instead: --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -8046,18 +8046,26 @@ qemuBuildCommandLine(virConnectPtr conn, /* Unless we have -device, then USB disks need special handling */ - if ((disk->bus == VIR_DOMAIN_DISK_BUS_USB) && - !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) { - if (disk->device == VIR_DOMAIN_DISK_DEVICE_DISK) { - virCommandAddArg(cmd, "-usbdevice"); - virCommandAddArgFormat(cmd, "disk:%s", disk->src); + if (disk->bus == VIR_DOMAIN_DISK_BUS_USB) { + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) { + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_USB_STORAGE)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("This QEMU doesn't support '-device " + "usb-storage'")); + goto error; + } } else { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unsupported usb disk type for '%s'"), - disk->src); - goto error; + if (disk->device == VIR_DOMAIN_DISK_DEVICE_DISK) { + virCommandAddArg(cmd, "-usbdevice"); + virCommandAddArgFormat(cmd, "disk:%s", disk->src); + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unsupported usb disk type for '%s'"), + disk->src); + goto error; + } + continue; } - continue; } switch (disk->device) { Alternatively, I could do the following instead, which is more concise, but doesn't happen in the same if() and thus spread the capability checking over a larger code area: --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4311,13 +4311,6 @@ qemuBuildDriveDevStr(virDomainDefPtr def, goto error; break; case VIR_DOMAIN_DISK_BUS_USB: + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_USB_STORAGE)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("This QEMU doesn't support '-device " + "usb-storage'")); + goto error; + + } virBufferAddLit(&opt, "usb-storage"); if (qemuBuildDeviceAddressStr(&opt, def, &disk->info, qemuCaps) < 0) Once I have an opinion (or further explanation why I'm still confused :)) I'll re-submit a new patchset with the preferred fix, rebased on the then-current master. Cheers!