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

[Daniel P. Berrange]

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 at 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 :|