Re: [libvirt] [PATCHv2 0/2] Support setting the 'removable' flag for USB disks
27/08/13 14:40, Peter Krempa wrote: On 08/23/13 12:38, Fred A. Kemp wrote: From: Fred A. Kemp ano...@riseup.net The commit message of patch #2 explains the purpose of this patch set. A review would be greatly appreciated! Note that I've only added the new capability for usb-storage.removable to the qemu help tests of qemu(-kvm) version 1.2.0, since that's what I had easily available to get the output of `-device usb-storage,?` from. I hope that's not an issue, otherwise, is there a way to obtain these outputs without having to hunt down and install all supported versions? Previous submissions of this patch set to this list: http://www.redhat.com/archives/libvir-list/2013-March/msg01051.html http://www.redhat.com/archives/libvir-list/2013-May/msg02039.html https://www.redhat.com/archives/libvir-list/2013-July/msg01635.html https://www.redhat.com/archives/libvir-list/2013-August/msg00581.html Fred A. Kemp (2): qemu: Add capability flag for usb-storage qemu: Support setting the 'removable' flag for USB disks This patchset unfortunately breaks the recently added qemuhotplugtest: 14) hotplug-base ATTACH disk-usb ... libvirt: QEMU Driver error : unsupported configuration: This QEMU doesn't support '-device usb-storage' FAILED 15) hotplug-base DETACH disk-usb ... libvirt: QEMU Driver error : operation failed: disk sdq not found FAILED 16) hotplug-base ATTACH disk-usb ... libvirt: QEMU Driver error : unsupported configuration: This QEMU doesn't support '-device usb-storage' FAILED 17) hotplug-base DETACH disk-usb ... domain XML should not match the expected result libvirt: QEMU Driver error : operation failed: disk sdq not found FAILED 18) hotplug-base DETACH disk-usb ... libvirt: QEMU Driver error : operation failed: disk sdq not found FAILED After a two minute investigation of this new test, I threw in the following fix which *seemingly* does the trick (i.e. I see no test failures any more): --- a/tests/qemuhotplugtest.c +++ b/tests/qemuhotplugtest.c @@ -78,6 +78,7 @@ qemuHotplugCreateObjects(virDomainXMLOptionPtr xmlopt, /* for attach detach qemu must support -device */ virQEMUCapsSet(priv-qemuCaps, QEMU_CAPS_DRIVE); virQEMUCapsSet(priv-qemuCaps, QEMU_CAPS_DEVICE); +virQEMUCapsSet(priv-qemuCaps, QEMU_CAPS_DEVICE_USB_STORAGE); virQEMUCapsSet(priv-qemuCaps, QEMU_CAPS_NET_NAME); virQEMUCapsSet(priv-qemuCaps, QEMU_CAPS_VIRTIO_SCSI); if (event) I'm very time constrained at the moment so I didn't have time to read the sources in detail, so the above fix is based on pattern matching only. If the fix looks good any way, it should be fixup'ed into my patch #1. I'm a bit confused with the process now, as my previous patches were ACKed but not pushed. Should send a new patchset? Cheers! -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] qemu: Add capability flag for usb-storage
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 ano...@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
Re: [libvirt] [PATCH 1/2] qemu: Add capability flag for usb-storage
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 ano...@riseup.net
Allow use of the usb-storage device only if the new capability flag
QEMU_CAPS_DEVICE_USB_STORAGE is set, which it is for qemu(-kvm)
versions = 0.12.1.2-rhel62-beta.
---
src/qemu/qemu_capabilities.c |2 ++
src/qemu/qemu_capabilities.h |1 +
src/qemu/qemu_command.c |6 +++---
tests/qemuhelptest.c | 18 --
tests/qemuxml2argvtest.c |3 ++-
5 files changed, 20 insertions(+), 10 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 47cc07a..5c566c1 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -235,6 +235,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
vnc-share-policy, /* 150 */
device-del-event,
dmi-to-pci-bridge,
+ usb-storage,
);
struct _virQEMUCaps {
@@ -1383,6 +1384,7 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[]
= {
{ vfio-pci, QEMU_CAPS_DEVICE_VFIO_PCI },
{ scsi-generic, QEMU_CAPS_DEVICE_SCSI_GENERIC },
{ i82801b11-bridge, QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE },
+{ usb-storage, QEMU_CAPS_DEVICE_USB_STORAGE },
};
static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsVirtioBlk[] = {
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index 074e55d..4a8b14b 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -191,6 +191,7 @@ enum virQEMUCapsFlags {
QEMU_CAPS_VNC_SHARE_POLICY = 150, /* set display sharing policy */
QEMU_CAPS_DEVICE_DEL_EVENT = 151, /* DEVICE_DELETED event */
QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE = 152, /* -device i82801b11-bridge
*/
+QEMU_CAPS_DEVICE_USB_STORAGE = 153, /* -device usb-storage */
QEMU_CAPS_LAST, /* this must always be the last item
*/
};
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?
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
This doesn't make sense to me either, but I guess it will after clearing
up my previous confusion.
Cheers!
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/2] Support settings the 'removable' flag for USB disks
Eric Blake wrote: Does this patch still apply as is? No, so I just re-submitted a rebased patch-set: http://www.redhat.com/archives/libvir-list/2013-August/msg00581.html It should be noted that my patches constantly gets in a conflicting state versus your master since they add capabilities, and that seems to be the most rapidly changing place in the code base. Luckily such conflicts are trivial to resolve. In fact, since my second patch submission in May [1], in which I (hope I) fixed all issues found in the review [2] of my first submission in March [3], resolving these types of trivial conflicts is the only thing I've done. [1] http://www.redhat.com/archives/libvir-list/2013-May/msg02039.html [2] http://www.redhat.com/archives/libvir-list/2013-March/msg01065.html [3] http://www.redhat.com/archives/libvir-list/2013-March/msg01051.html I apologize on behalf of a busy team that sometimes patches don't get reviewed, and it is okay to send pings every week or so as needed to spur someone on to looking at it. Ok. Please let me know if there's anything, process-wise, I can do too smoothen this further. Otherwise I'll just continue sending rebased patches as pings every week or two. Cheers! -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/2] Support setting the 'removable' flag for USB disks
Bump! Also: 31/05/13 16:23, Fred A. Kemp wrote: For reference, my first attempt at a patch for this feature was: 1363682454-32012-1-git-send-email-ano...@lavabit.com On second thought, an archive link might be a better reference for people that doesn't keep old emails in the inbox for very long: https://www.redhat.com/archives/libvir-list/2013-March/msg01051.html Cheers! -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Support setting the 'removable' flag for USB disks
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
[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
codetray/code could be updated while the domain is running.
-span class=sinceSince 0.0.3; codebus/code attribute since
0.4.3;
+The optional attribute coderemovable/code sets the
+removable flag for USB disks, and its value can be either on
+or off, defaulting to off. span class=sinceSince
+0.0.3; codebus/code attribute since 0.4.3;
codetray/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
dtcodeiotune/code/dt
ddThe optional codeiotune/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
+valueon/value
+valueoff/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;
+}
+
