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

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(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. 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?

> 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.