[libvirt] [PATCH] qemu: Support setting the 'removable' flag for USB disks

[Daniel P. Berrange]

On Tue, Mar 19, 2013 at 09:40:54AM +0100, anonym wrote:

Can you use a real name instead of an anonymous psuedonym for patches.

> This adds an attribute named 'removable' to the 'target' element of
> disks, which controls the removable flag. For instance, on a Linux
> guest it controls the value of /sys/block/$dev/removable. This option
> is only valid for USB disks (i.e. bus='usb'), and its default value is
> 'off', which is the same behaviour as before.
> 
> To achieve this, 'removable=on' is appended to the '-device
> usb-storage' parameter sent to qemu when adding a USB disk via
> '-disk'. For versions of qemu only supporting '-usbdevice disk:' for
> adding USB disks this feature always remains 'off' since there's no
> support for passing such an option.
> 
> Bug: https://bugzilla.redhat.com/show_bug.cgi?id=922495
> ---
>  docs/formatdomain.html.in                          |    8 +++--
>  docs/schemas/domaincommon.rng                      |    8 +++++
>  src/conf/domain_conf.c                             |   35 ++++++++++++++++++--
>  src/conf/domain_conf.h                             |    9 +++++
>  src/libvirt_private.syms                           |    1 +
>  src/qemu/qemu_command.c                            |    6 ++++
>  .../qemuxml2argv-disk-usb-device-removable.args    |    8 +++++
>  .../qemuxml2argv-disk-usb-device-removable.xml     |   27 +++++++++++++++
>  tests/qemuxml2argvtest.c                           |    2 ++
>  9 files changed, 99 insertions(+), 5 deletions(-)
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-usb-device-removable.args
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-usb-device-removable.xml
> 
> @@ -12915,10 +12940,14 @@ virDomainDiskDefFormat(virBufferPtr buf,
>      if ((def->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY ||
>           def->device == VIR_DOMAIN_DISK_DEVICE_CDROM) &&
>          def->tray_status != VIR_DOMAIN_DISK_TRAY_CLOSED)
> -        virBufferAsprintf(buf, " tray='%s'/>\n",
> +        virBufferAsprintf(buf, " tray='%s'",
>                            virDomainDiskTrayTypeToString(def->tray_status));
> -    else
> -        virBufferAddLit(buf, "/>\n");
> +    if (def->bus == VIR_DOMAIN_DISK_BUS_USB &&
> +        def->removable != VIR_DOMAIN_DISK_REMOVABLE_OFF) {

This means that if the user explicitly  added   removeable='off' to their
XML, we'll be dropping it.

> +        virBufferAsprintf(buf, " removable='%s'",
> +                          virDomainDiskRemovableTypeToString(def->removable));
> +    }
> +    virBufferAddLit(buf, "/>\n");
>  
>      /*disk I/O throttling*/
>      if (def->blkdeviotune.total_bytes_sec ||
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 96f11ba..0f4f0d7 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -518,6 +518,13 @@ enum virDomainDiskTray {
>      VIR_DOMAIN_DISK_TRAY_LAST
>  };
>  
> +enum virDomainDiskRemovable {

If you add in

  VIR_DOMAIN_DISK_REMOVABLE_DEFAULT

then you can distinguish explicit on/off settings from the
default setting to address my earlier comment.

> +    VIR_DOMAIN_DISK_REMOVABLE_ON,
> +    VIR_DOMAIN_DISK_REMOVABLE_OFF,
> +
> +    VIR_DOMAIN_DISK_REMOVABLE_LAST
> +};
> +


> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 5cad990..0d1a9d6 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -156,6 +156,7 @@ virDomainDiskIoTypeToString;
>  virDomainDiskPathByName;
>  virDomainDiskProtocolTransportTypeFromString;
>  virDomainDiskProtocolTransportTypeToString;
> +virDomainDiskRemovableTypeToString;

The VIR_ENUM macro generates 2 methods, so also add in

  virDomainDiskRemovableTypeFromString;


> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 4891b65..c04cecf 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -3219,6 +3219,11 @@ qemuBuildDriveDevStr(virDomainDefPtr def,
>      if (disk->product)
>          virBufferAsprintf(&opt, ",product=%s", disk->product);
>  
> +    if (disk->bus == VIR_DOMAIN_DISK_BUS_USB &&
> +        disk->removable != VIR_DOMAIN_DISK_REMOVABLE_OFF) {
> +        virBufferAsprintf(&opt, ",removable=%s",
> +                          virDomainDiskRemovableTypeToString(disk->removable));
> +    }

We should should not on the QEMU default setting - so make sure
you explicitly set both removeable=on or removeable=off.

Also, not all versions of QEMU support this property, so you'll
need to add a new capability flag in qemu_capabilities.h and
then update qemu_capabilities.c to detect whether it exists or
not.

Then in this code, if you find a QEMU which does not support the
flag, you should do  a virRaiseError(VIR_ERR_CONFIG_UNSUPPORTED, ....)
to tell the user we can't do what they asked

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