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

[anonym]

15/08/13 12:58, Daniel P. Berrange wrote:
> 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>
>>>>
>>>> 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.

Without knowing the exact development history of qemu, I assumed it
could be the case the we have '-device' but 'usb-storage' hadn't been
implemented yet (so '-usbdevice' was still the way to go).

> If the 'usb-storage' device does not exist, then '-usbdevice disk' is
> not going to work either since it uses the same code internally.

Note that the QEMU_CAPS_DEVICE_USB_STORAGE capability I add only checks
if '-device usb-storage' is supported (by parsing 'qemu -device ?' like
other, similar caps) and has nothing to do with '-usbdevice'.

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

So, what if I drop the above chunk, and do the following instead:

--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -8046,18 +8046,26 @@ qemuBuildCommandLine(virConnectPtr conn,

             /* Unless we have -device, then USB disks need special
                handling */
-            if ((disk->bus == VIR_DOMAIN_DISK_BUS_USB) &&
-                !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) {
-                if (disk->device == VIR_DOMAIN_DISK_DEVICE_DISK) {
-                    virCommandAddArg(cmd, "-usbdevice");
-                    virCommandAddArgFormat(cmd, "disk:%s", disk->src);
+            if (disk->bus == VIR_DOMAIN_DISK_BUS_USB) {
+                if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) {
+                    if (!virQEMUCapsGet(qemuCaps,
QEMU_CAPS_DEVICE_USB_STORAGE)) {
+                        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                                       _("This QEMU doesn't support
'-device "
+                                         "usb-storage'"));
+                        goto error;
+                    }
                 } else {
-                    virReportError(VIR_ERR_INTERNAL_ERROR,
-                                   _("unsupported usb disk type for '%s'"),
-                                   disk->src);
-                    goto error;
+                    if (disk->device == VIR_DOMAIN_DISK_DEVICE_DISK) {
+                        virCommandAddArg(cmd, "-usbdevice");
+                        virCommandAddArgFormat(cmd, "disk:%s", disk->src);
+                    } else {
+                        virReportError(VIR_ERR_INTERNAL_ERROR,
+                                       _("unsupported usb disk type for
'%s'"),
+                                       disk->src);
+                        goto error;
+                    }
+                    continue;
                 }
-                continue;
             }

             switch (disk->device) {


Alternatively, I could do the following instead, which is more concise,
but doesn't happen in the same if() and thus spread the capability
checking over a larger code area:

--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -4311,13 +4311,6 @@ qemuBuildDriveDevStr(virDomainDefPtr def,
             goto error;
         break;
     case VIR_DOMAIN_DISK_BUS_USB:
+        if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_USB_STORAGE)) {
+            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                           _("This QEMU doesn't support '-device "
+                             "usb-storage'"));
+            goto error;
+
+        }
         virBufferAddLit(&opt, "usb-storage");

         if (qemuBuildDeviceAddressStr(&opt, def, &disk->info, qemuCaps)
< 0)


Once I have an opinion (or further explanation why I'm still confused
:)) I'll re-submit a new patchset with the preferred fix, rebased on the
then-current master.

Cheers!