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@xxxxxxxxxx>
>>
>> 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.
Cheers!
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list