[libvirt] [PATCH] qemu: Support setting the 'removable' flag for USB disks
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 diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 8a3c3b7..384da4f 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1498,9 +1498,13 @@ removable disks (i.e. CDROM or Floppy disk), the value can be either "open" or "closed", defaults to "closed". NB, the value of <code>tray</code> could be updated while the domain is running. - <span class="since">Since 0.0.3; <code>bus</code> attribute since 0.4.3; + The optional attribute <code>removable</code> sets the + removable flag for USB disks, and its value can be either "on" + or "off", defaulting to "off". <span class="since">Since + 0.0.3; <code>bus</code> attribute since 0.4.3; <code>tray</code> attribute since 0.9.11; "usb" attribute value since - after 0.4.4; "sata" attribute value since 0.9.7</span> + after 0.4.4; "sata" attribute value since 0.9.7; "removable" attribute + value since X.Y.Z</span> </dd> <dt><code>iotune</code></dt> <dd>The optional <code>iotune</code> element provides the diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 9792065..eab6aa3 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1164,6 +1164,14 @@ </choice> </attribute> </optional> + <optional> + <attribute name="removable"> + <choice> + <value>on</value> + <value>off</value> + </choice> + </attribute> + </optional> </element> </define> <define name="geometry"> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3278e9c..551bac3 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -709,6 +709,10 @@ VIR_ENUM_IMPL(virDomainDiskTray, VIR_DOMAIN_DISK_TRAY_LAST, "closed", "open"); +VIR_ENUM_IMPL(virDomainDiskRemovable, VIR_DOMAIN_DISK_REMOVABLE_LAST, + "on", + "off"); + VIR_ENUM_IMPL(virDomainNumatuneMemPlacementMode, VIR_DOMAIN_NUMATUNE_MEM_PLACEMENT_MODE_LAST, "default", @@ -3996,6 +4000,7 @@ virDomainDiskDefParseXML(virCapsPtr caps, char *authUUID = NULL; char *usageType = NULL; char *tray = NULL; + char *removable = NULL; char *logical_block_size = NULL; char *physical_block_size = NULL; char *wwn = NULL; @@ -4149,6 +4154,7 @@ virDomainDiskDefParseXML(virCapsPtr caps, target = virXMLPropString(cur, "dev"); bus = virXMLPropString(cur, "bus"); tray = virXMLPropString(cur, "tray"); + removable = virXMLPropString(cur, "removable"); /* HACK: Work around for compat with Xen * driver in previous libvirt releases */ @@ -4556,6 +4562,24 @@ virDomainDiskDefParseXML(virCapsPtr caps, def->tray_status = VIR_DOMAIN_DISK_TRAY_CLOSED; } + if (removable) { + if ((def->removable = virDomainDiskRemovableTypeFromString(removable)) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("unknown disk removable status '%s'"), removable); + goto error; + } + + if (def->bus != VIR_DOMAIN_DISK_BUS_USB) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("removable is only valid for usb disks")); + goto error; + } + } else { + if (def->bus == VIR_DOMAIN_DISK_BUS_USB) { + def->removable = VIR_DOMAIN_DISK_REMOVABLE_OFF; + } + } + if (def->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY && def->bus != VIR_DOMAIN_DISK_BUS_FDC) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -4754,6 +4778,7 @@ cleanup: VIR_FREE(target); VIR_FREE(source); VIR_FREE(tray); + VIR_FREE(removable); VIR_FREE(trans); while (nhosts > 0) { virDomainDiskHostDefFree(&hosts[nhosts - 1]); @@ -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) { + 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 { + VIR_DOMAIN_DISK_REMOVABLE_ON, + VIR_DOMAIN_DISK_REMOVABLE_OFF, + + VIR_DOMAIN_DISK_REMOVABLE_LAST +}; + enum virDomainDiskGeometryTrans { VIR_DOMAIN_DISK_TRANS_DEFAULT = 0, VIR_DOMAIN_DISK_TRANS_NONE, @@ -612,6 +619,7 @@ struct _virDomainDiskDef { char *src; char *dst; int tray_status; + int removable; int protocol; size_t nhosts; virDomainDiskHostDefPtr hosts; @@ -2346,6 +2354,7 @@ VIR_ENUM_DECL(virDomainDiskIo) VIR_ENUM_DECL(virDomainDiskSecretType) VIR_ENUM_DECL(virDomainDiskSGIO) VIR_ENUM_DECL(virDomainDiskTray) +VIR_ENUM_DECL(virDomainDiskRemovable) VIR_ENUM_DECL(virDomainIoEventFd) VIR_ENUM_DECL(virDomainVirtioEventIdx) VIR_ENUM_DECL(virDomainDiskCopyOnRead) 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; virDomainDiskRemove; virDomainDiskRemoveByName; virDomainDiskTypeFromString; 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)); + } if (virBufferError(&opt)) { virReportOOMError(); goto error; @@ -9391,6 +9396,7 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr qemuCaps, disk->type = VIR_DOMAIN_DISK_TYPE_FILE; disk->device = VIR_DOMAIN_DISK_DEVICE_DISK; disk->bus = VIR_DOMAIN_DISK_BUS_USB; + disk->removable = VIR_DOMAIN_DISK_REMOVABLE_OFF; if (!(disk->dst = strdup("sda")) || VIR_REALLOC_N(def->disks, def->ndisks+1) < 0) goto no_memory; diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-usb-device-removable.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-usb-device-removable.args new file mode 100644 index 0000000..36be080 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-usb-device-removable.args @@ -0,0 +1,8 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M \ +pc -m 214 -smp 1 -nographic -nodefconfig -nodefaults -monitor \ +unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -usb -drive \ +file=/dev/HostVG/QEMUGuest1,if=none,id=drive-ide0-0-0 -device ide-drive,\ +bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 -drive file=/tmp/usbdisk.img,\ +if=none,id=drive-usb-disk0 -device usb-storage,drive=drive-usb-disk0,\ +id=usb-disk0,removable=on -device virtio-balloon-pci,id=balloon0,bus=pci.0,\ +addr=0x3 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-usb-device-removable.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-usb-device-removable.xml new file mode 100644 index 0000000..6ee3e9b --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-usb-device-removable.xml @@ -0,0 +1,27 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + </disk> + <disk type='file' device='disk'> + <source file='/tmp/usbdisk.img'/> + <target dev='sda' bus='usb' removable='on'/> + </disk> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index e76d844..2eca7ac 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -516,6 +516,8 @@ mymain(void) DO_TEST("disk-usb", NONE); DO_TEST("disk-usb-device", QEMU_CAPS_DRIVE, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG); + DO_TEST("disk-usb-device-removable", + QEMU_CAPS_DRIVE, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG); DO_TEST("disk-scsi-device", QEMU_CAPS_DRIVE, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG, QEMU_CAPS_SCSI_LSI); -- 1.7.10.4
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 :|
On 03/19/13 12:59, Daniel P. Berrange wrote:
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
...
/*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
Or use VIR_DOMAIN_FEATURE_STATE_[DEFAULT|ON|OFF|LAST] that I introduced specifically to avoid adding enums like this for every feature.
+}; +
Peter
19/03/13 12:59, Daniel P. Berrange wrote:
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.
Sure. Will do in my next patch proposal.
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.
Being rather new to this, I modeled the 'removable' attribute after the 'tray' attribute, which has that behaviour. Hence you may want to reconsider if that's what you want for 'tray' too.
+ 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.
Ok. To reduce bloat I'll use VIR_DOMAIN_FEATURE_STATE_* as suggested by Peter Krempa.
+ 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;
Not an issue once I move to VIR_DOMAIN_FEATURE_STATE_*.
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.
Ack. I guess it may be a good idea to not use any *TypeToString() functions like that either since that implies that the qemu value names have to be the same as those in libvirt's domain XML, both which could change in the future. I'll explicitly write "=on" or "=off" instead. Ok?
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
As I understand it we want QEMU_CAPS_USB_STORAGE_REMOVABLE for usb-storage.removable, but does that make sense without a capability QEMU_CAPS_DEVICE_USB_STORAGE for the actual device, i.e. `-device usb-storage`? OTOH: * there currently is no separate capability for usb-storage (i.e. QEMU_CAPS_DEVICE_USB_STORAGE), and * the current code assumes that if we have QEMU_CAPS_DEVICE, then we can use `-device usb-storage`, so I get the impression that `-device` and `-device usb-storage` were introduced into qemu in tandem, so a separate capability is perhaps not needed. Or am I missing something? Also, if you have any remarks about how the capability (or capabilities) you want me to add should be named, please do tell. Another issue this introduces for me is with the libvirt test suite. To test QEMU_CAPS_USB_STORAGE_REMOVABLE in qemuhelptest.c, the respective outputs of `-device usb-storage,?` of the various qemu/qemu-kvm versions must be in the corresponding tests/qemuhelpdata/*-device files. They currently are not and I certainly don't have all those versions (easily) available to extract them from. How do you developers deal with this? I assume you'd want those help outputs present in a patch in order to accept it (?). Otherwise I could just skip testing the QEMU_CAPS_USB_STORAGE_REMOVABLE capability by not adding it to the corresponding tests in qemuhelptest.c (that's how I got my current WIP patch to pass). Thanks for the feedback, Daniel and Peter! Cheers!
participants (3)
-
anonym -
Daniel P. Berrange -
Peter Krempa