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>
Allow use of the usb-storage device only if the new capability flag QEMU_CAPS_DEVICE_USB_STORAGE is set, which it is for qemu(-kvm) versions >= 0.12.1.2-rhel62-beta. --- src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 6 +++--- tests/qemuhelptest.c | 18 ++++++++++++------ tests/qemuxml2argvtest.c | 3 ++- 5 files changed, 20 insertions(+), 10 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 47cc07a..5c566c1 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -235,6 +235,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, "vnc-share-policy", /* 150 */ "device-del-event", "dmi-to-pci-bridge", + "usb-storage", );
struct _virQEMUCaps { @@ -1383,6 +1384,7 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = { { "vfio-pci", QEMU_CAPS_DEVICE_VFIO_PCI }, { "scsi-generic", QEMU_CAPS_DEVICE_SCSI_GENERIC }, { "i82801b11-bridge", QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE }, + { "usb-storage", QEMU_CAPS_DEVICE_USB_STORAGE }, };
static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsVirtioBlk[] = { diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 074e55d..4a8b14b 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -191,6 +191,7 @@ enum virQEMUCapsFlags { QEMU_CAPS_VNC_SHARE_POLICY = 150, /* set display sharing policy */ QEMU_CAPS_DEVICE_DEL_EVENT = 151, /* DEVICE_DELETED event */ QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE = 152, /* -device i82801b11-bridge */ + QEMU_CAPS_DEVICE_USB_STORAGE = 153, /* -device usb-storage */
QEMU_CAPS_LAST, /* this must always be the last item */ }; 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. If the 'usb-storage' device does not exist, then '-usbdevice disk' is not going to work either since it uses the same code internally.
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
This doesn't make sense to me either, but I guess it will after clearing up my previous confusion.
Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|