Re: [libvirt] [PATCH 1/2] qemu: Add capability flag for usb-storage

On Tue, Aug 13, 2013 at 01:52:33PM +0200, Fred A. Kemp wrote: > From: "Fred A. Kemp" <anonym(a)riseup.net&gt; > > 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.

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