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

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

2013-03-21 Thread anonym

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

2013-03-19 Thread anonym

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

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

2013-03-19 Thread 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 :|

--

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

2013-03-19 Thread Peter Krempa


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

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

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

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

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

4 matches

Site Navigation

Mail list logo

Footer information