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

[Eric Blake]

On 08/19/2013 08:00 AM, anonym wrote:

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

No, if we have -device, the we MUST use it.  Fallback to older code is
possible only if -device is missing.

> 
> 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;
> +                    }

This looks okay...

>                  } 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);

...but this still looks fishy (-device is so old that we are probably
safer rejecting attempts to use a usb on a qemu that lacked -device than
we are trying to figure out if -usbdevice worked for something that old
- no one targetting a qemu that old will be trying to use new features
anyway).


> 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;
> +
> +        }

That actually looks a bit nicer, even if it does spread the capability
check across a wider set of code.

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

When resubmitting, send as a top-level patch (no in-reply-to) so it
isn't buried, and use 'git send-email --subject-prefix=PATCHv2' or
similar to make it obvious in the title that it is a rebased series.
Good luck.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 621 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20130821/fff437b8/attachment-0001.sig>