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

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