1. Patchew
  2. Libvirt
  3. View series

[libvirt] [PATCH v3] qemu: format serial and geometry on frontend disk device

Daniel P. Berrangé posted 1 patch 6 years, 10 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20180702091707.1083-1-berrange@redhat.com
Test syntax-check passed
There is a newer version of this series
src/qemu/qemu_command.c                       | 46 ++++++++++++++++---
.../disk-drive-network-tlsx509.args           | 12 ++---
.../disk-drive-network-vxhs.args              |  4 +-
tests/qemuxml2argvdata/disk-drive-shared.args |  5 +-
tests/qemuxml2argvdata/disk-geometry.args     |  6 +--
tests/qemuxml2argvdata/disk-ide-wwn.args      |  5 +-
.../qemuxml2argvdata/disk-scsi-disk-wwn.args  |  4 +-
tests/qemuxml2argvdata/disk-serial.args       | 10 ++--
tests/qemuxml2argvdata/disk-serial.xml        |  1 -
tests/qemuxml2xmloutdata/disk-serial.xml      |  1 -
10 files changed, 62 insertions(+), 32 deletions(-)
[libvirt] [PATCH v3] qemu: format serial and geometry on frontend disk device
Posted by Daniel P. Berrangé 6 years, 10 months ago 
Currently we format the serial, geometry and error policy on the -drive
backend argument.

QEMU added the ability to set serial and geometry on the frontend in
the 1.2 release deprecating use of -drive, with support being deleted
from -drive in 3.0.

We keep formatting error policy on -drive for now, because we don't
ahve support for that with -device for usb-storage just yet.

Note that some disk buses (sd) still don't support -device. Although
QEMU allowed these properties to be set on -drive for if=sd, they
have been ignored so we now report an error in this case.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---

Changed in v3:

 - Report error for setting CHS on USB
 - Report error for setting CHS translation mode for bus != IDE
 - Use 'bios-chs-trans' property not 'trans'

 src/qemu/qemu_command.c                       | 46 ++++++++++++++++---
 .../disk-drive-network-tlsx509.args           | 12 ++---
 .../disk-drive-network-vxhs.args              |  4 +-
 tests/qemuxml2argvdata/disk-drive-shared.args |  5 +-
 tests/qemuxml2argvdata/disk-geometry.args     |  6 +--
 tests/qemuxml2argvdata/disk-ide-wwn.args      |  5 +-
 .../qemuxml2argvdata/disk-scsi-disk-wwn.args  |  4 +-
 tests/qemuxml2argvdata/disk-serial.args       | 10 ++--
 tests/qemuxml2argvdata/disk-serial.xml        |  1 -
 tests/qemuxml2xmloutdata/disk-serial.xml      |  1 -
 10 files changed, 62 insertions(+), 32 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 4fc3176ad3..46726b055e 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1599,7 +1599,7 @@ qemuBuildDiskFrontendAttributeErrorPolicy(virDomainDiskDefPtr disk,
 }
 
 
-static void
+static int
 qemuBuildDiskFrontendAttributes(virDomainDiskDefPtr disk,
                                 virBufferPtr buf)
 {
@@ -1607,14 +1607,27 @@ qemuBuildDiskFrontendAttributes(virDomainDiskDefPtr disk,
     if (disk->geometry.cylinders > 0 &&
         disk->geometry.heads > 0 &&
         disk->geometry.sectors > 0) {
+        if (disk->bus == VIR_DOMAIN_DISK_BUS_USB) {
+            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                           _("CHS geometry can not be set for 'usb' bus"));
+            return -1;
+        }
+
         virBufferAsprintf(buf, ",cyls=%u,heads=%u,secs=%u",
                           disk->geometry.cylinders,
                           disk->geometry.heads,
                           disk->geometry.sectors);
 
-        if (disk->geometry.trans != VIR_DOMAIN_DISK_TRANS_DEFAULT)
-            virBufferAsprintf(buf, ",trans=%s",
+        if (disk->geometry.trans != VIR_DOMAIN_DISK_TRANS_DEFAULT) {
+            if (disk->bus != VIR_DOMAIN_DISK_BUS_IDE) {
+                virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                               _("CHS translation mode can only be set for 'ide' bus not '%s'"),
+                               virDomainDiskBusTypeToString(disk->bus));
+                return -1;
+            }
+            virBufferAsprintf(buf, ",bios-chs-trans=%s",
                               virDomainDiskGeometryTransTypeToString(disk->geometry.trans));
+        }
     }
 
     if (disk->serial) {
@@ -1622,7 +1635,7 @@ qemuBuildDiskFrontendAttributes(virDomainDiskDefPtr disk,
         virBufferEscape(buf, '\\', " ", "%s", disk->serial);
     }
 
-    qemuBuildDiskFrontendAttributeErrorPolicy(disk, buf);
+    return 0;
 }
 
 
@@ -1662,11 +1675,27 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk,
         virBufferAsprintf(&opt, "if=%s",
                           virDomainDiskQEMUBusTypeToString(disk->bus));
         virBufferAsprintf(&opt, ",index=%d", idx);
+
+        if (disk->serial) {
+            virReportError(VIR_ERR_INTERNAL_ERROR,
+                           _("Serial property not supported for drive bus '%s'"),
+                           virDomainDiskBusTypeToString(disk->bus));
+            goto error;
+        }
+        if (disk->geometry.cylinders > 0 &&
+            disk->geometry.heads > 0 &&
+            disk->geometry.sectors > 0) {
+            virReportError(VIR_ERR_INTERNAL_ERROR,
+                           _("Geometry not supported for drive bus '%s'"),
+                           virDomainDiskBusTypeToString(disk->bus));
+            goto error;
+        }
     }
 
-    /* Format attributes for the drive itself (not the storage backing it) which
-     * we've formatted historically with -drive */
-    qemuBuildDiskFrontendAttributes(disk, &opt);
+    /* werror/rerror are really frontend attributes, but older
+     * qemu requires them on -drive instead of -device */
+    qemuBuildDiskFrontendAttributeErrorPolicy(disk, &opt);
+
 
     /* While this is a frontend attribute, it only makes sense to be used when
      * legacy -drive is used. In modern qemu the 'ide-cd' or 'scsi-cd' are used.
@@ -2125,6 +2154,9 @@ qemuBuildDriveDevStr(const virDomainDef *def,
     if (qemuBuildDriveDevCacheStr(disk, &opt, qemuCaps) < 0)
         goto error;
 
+    if (qemuBuildDiskFrontendAttributes(disk, &opt) < 0)
+        goto error;
+
     if (virBufferCheckError(&opt) < 0)
         goto error;
 
diff --git a/tests/qemuxml2argvdata/disk-drive-network-tlsx509.args b/tests/qemuxml2argvdata/disk-drive-network-tlsx509.args
index e25f45742c..b5e73064c0 100644
--- a/tests/qemuxml2argvdata/disk-drive-network-tlsx509.args
+++ b/tests/qemuxml2argvdata/disk-drive-network-tlsx509.args
@@ -28,22 +28,22 @@ server,nowait \
 -drive file.driver=vxhs,file.tls-creds=objvirtio-disk0_tls0,\
 file.vdisk-id=eb90327c-8302-4725-9e1b-4e85ed4dc251,\
 file.server.host=192.168.0.1,file.server.port=9999,format=raw,if=none,\
-id=drive-virtio-disk0,serial=eb90327c-8302-4725-9e1b-4e85ed4dc251,cache=none \
+id=drive-virtio-disk0,cache=none \
 -device virtio-blk-pci,bus=pci.0,addr=0x4,drive=drive-virtio-disk0,\
-id=virtio-disk0 \
+id=virtio-disk0,serial=eb90327c-8302-4725-9e1b-4e85ed4dc251 \
 -object tls-creds-x509,id=objvirtio-disk1_tls0,dir=/etc/pki/libvirt-vxhs/dummy,\
 ,path,endpoint=client,verify-peer=yes \
 -drive file.driver=vxhs,file.tls-creds=objvirtio-disk1_tls0,\
 file.vdisk-id=eb90327c-8302-4725-9e1b-4e85ed4dc252,\
 file.server.host=192.168.0.2,file.server.port=9999,format=raw,if=none,\
-id=drive-virtio-disk1,serial=eb90327c-8302-4725-9e1b-4e85ed4dc252,cache=none \
+id=drive-virtio-disk1,cache=none \
 -device virtio-blk-pci,bus=pci.0,addr=0x5,drive=drive-virtio-disk1,\
-id=virtio-disk1 \
+id=virtio-disk1,serial=eb90327c-8302-4725-9e1b-4e85ed4dc252 \
 -drive file.driver=vxhs,file.vdisk-id=eb90327c-8302-4725-9e1b-4e85ed4dc253,\
 file.server.host=192.168.0.3,file.server.port=9999,format=raw,if=none,\
-id=drive-virtio-disk2,serial=eb90327c-8302-4725-9e1b-4e85ed4dc252,cache=none \
+id=drive-virtio-disk2,cache=none \
 -device virtio-blk-pci,bus=pci.0,addr=0x6,drive=drive-virtio-disk2,\
-id=virtio-disk2 \
+id=virtio-disk2,serial=eb90327c-8302-4725-9e1b-4e85ed4dc252 \
 -object tls-creds-x509,id=objvirtio-disk3_tls0,dir=/etc/pki/libvirt-nbd/dummy,,\
 path,endpoint=client,verify-peer=yes \
 -drive file.driver=nbd,file.server.type=inet,file.server.host=example.com,\
diff --git a/tests/qemuxml2argvdata/disk-drive-network-vxhs.args b/tests/qemuxml2argvdata/disk-drive-network-vxhs.args
index c2a1d4681f..fad4dfd942 100644
--- a/tests/qemuxml2argvdata/disk-drive-network-vxhs.args
+++ b/tests/qemuxml2argvdata/disk-drive-network-vxhs.args
@@ -25,6 +25,6 @@ server,nowait \
 -usb \
 -drive file.driver=vxhs,file.vdisk-id=eb90327c-8302-4725-9e1b-4e85ed4dc251,\
 file.server.host=192.168.0.1,file.server.port=9999,format=raw,if=none,\
-id=drive-virtio-disk0,serial=eb90327c-8302-4725-9e1b-4e85ed4dc251,cache=none \
+id=drive-virtio-disk0,cache=none \
 -device virtio-blk-pci,bus=pci.0,addr=0x4,drive=drive-virtio-disk0,\
-id=virtio-disk0
+id=virtio-disk0,serial=eb90327c-8302-4725-9e1b-4e85ed4dc251
diff --git a/tests/qemuxml2argvdata/disk-drive-shared.args b/tests/qemuxml2argvdata/disk-drive-shared.args
index 5306fdf750..032e86e291 100644
--- a/tests/qemuxml2argvdata/disk-drive-shared.args
+++ b/tests/qemuxml2argvdata/disk-drive-shared.args
@@ -23,8 +23,9 @@ server,nowait \
 -boot c \
 -usb \
 -drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0,\
-serial=XYZXYZXYZYXXYZYZYXYZY,cache=none \
--device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \
+cache=none \
+-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,\
+serial=XYZXYZXYZYXXYZYZYXYZY \
 -drive file=/dev/HostVG/QEMUGuest2,format=raw,if=none,id=drive-ide0-1-0,\
 media=cdrom,readonly=on \
 -device ide-drive,bus=ide.1,unit=0,drive=drive-ide0-1-0,id=ide0-1-0 \
diff --git a/tests/qemuxml2argvdata/disk-geometry.args b/tests/qemuxml2argvdata/disk-geometry.args
index b173ab6407..f3e35b5f9e 100644
--- a/tests/qemuxml2argvdata/disk-geometry.args
+++ b/tests/qemuxml2argvdata/disk-geometry.args
@@ -22,7 +22,7 @@ server,nowait \
 -no-acpi \
 -boot c \
 -usb \
--drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0,\
-cyls=16383,heads=16,secs=63,trans=lba \
--device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \
+-drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \
+-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,cyls=16383,\
+heads=16,secs=63,bios-chs-trans=lba \
 -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3
diff --git a/tests/qemuxml2argvdata/disk-ide-wwn.args b/tests/qemuxml2argvdata/disk-ide-wwn.args
index e6d2758ec3..a0e3f06b68 100644
--- a/tests/qemuxml2argvdata/disk-ide-wwn.args
+++ b/tests/qemuxml2argvdata/disk-ide-wwn.args
@@ -22,8 +22,7 @@ server,nowait \
 -no-acpi \
 -boot c \
 -usb \
--drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-1,\
-serial=WD-WMAP9A966149 \
+-drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-1 \
 -device ide-hd,bus=ide.0,unit=1,drive=drive-ide0-0-1,id=ide0-0-1,\
-wwn=0x5000c50015ea71ad \
+wwn=0x5000c50015ea71ad,serial=WD-WMAP9A966149 \
 -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3
diff --git a/tests/qemuxml2argvdata/disk-scsi-disk-wwn.args b/tests/qemuxml2argvdata/disk-scsi-disk-wwn.args
index 29607e8927..6407fd002d 100644
--- a/tests/qemuxml2argvdata/disk-scsi-disk-wwn.args
+++ b/tests/qemuxml2argvdata/disk-scsi-disk-wwn.args
@@ -25,9 +25,9 @@ server,nowait \
 -device lsi,id=scsi1,bus=pci.0,addr=0x4 \
 -usb \
 -drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-scsi0-0-1-0,\
-serial=WD-WMAP9A966149,readonly=on \
+readonly=on \
 -device scsi-cd,bus=scsi0.0,channel=0,scsi-id=1,lun=0,drive=drive-scsi0-0-1-0,\
-id=scsi0-0-1-0,wwn=0x5000c50015ea71ac \
+id=scsi0-0-1-0,wwn=0x5000c50015ea71ac,serial=WD-WMAP9A966149 \
 -drive file=/dev/HostVG/QEMUGuest2,format=raw,if=none,id=drive-scsi0-0-0-0 \
 -device scsi-hd,bus=scsi0.0,channel=0,scsi-id=0,lun=0,drive=drive-scsi0-0-0-0,\
 id=scsi0-0-0-0,wwn=0x5000c50015ea71ad \
diff --git a/tests/qemuxml2argvdata/disk-serial.args b/tests/qemuxml2argvdata/disk-serial.args
index 88310b009f..5892f64c29 100644
--- a/tests/qemuxml2argvdata/disk-serial.args
+++ b/tests/qemuxml2argvdata/disk-serial.args
@@ -22,11 +22,11 @@ server,nowait \
 -no-acpi \
 -boot c \
 -usb \
--drive 'file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-1,\
+-drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-1 \
+-device 'ide-drive,bus=ide.0,unit=1,drive=drive-ide0-0-1,id=ide0-0-1,\
 serial=\ \ WD-WMAP9A966149' \
--device ide-drive,bus=ide.0,unit=1,drive=drive-ide0-0-1,id=ide0-0-1 \
--drive 'file=/dev/HostVG/AllSerialChars,format=raw,if=none,id=drive-ide0-0-2,\
+-drive file=/dev/HostVG/AllSerialChars,format=raw,if=none,id=drive-ide0-0-2 \
+-device 'ide-drive,bus=ide.0,unit=2,drive=drive-ide0-0-2,id=ide0-0-2,\
 serial=abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789-_\ .+' \
--device ide-drive,bus=ide.0,unit=2,drive=drive-ide0-0-2,id=ide0-0-2 \
--drive file=/some/file,format=raw,if=sd,index=0,serial=sdserial \
+-drive file=/some/file,format=raw,if=sd,index=0 \
 -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3
diff --git a/tests/qemuxml2argvdata/disk-serial.xml b/tests/qemuxml2argvdata/disk-serial.xml
index ea71f2c339..18e72bb4ba 100644
--- a/tests/qemuxml2argvdata/disk-serial.xml
+++ b/tests/qemuxml2argvdata/disk-serial.xml
@@ -29,7 +29,6 @@
     <disk type='file' device='disk'>
       <source file='/some/file'/>
       <target dev='sda' bus='sd'/>
-      <serial>sdserial</serial>
     </disk>
     <controller type='usb' index='0'/>
     <controller type='ide' index='0'/>
diff --git a/tests/qemuxml2xmloutdata/disk-serial.xml b/tests/qemuxml2xmloutdata/disk-serial.xml
index 9313c699b6..a803cd959c 100644
--- a/tests/qemuxml2xmloutdata/disk-serial.xml
+++ b/tests/qemuxml2xmloutdata/disk-serial.xml
@@ -32,7 +32,6 @@
       <driver name='qemu' type='raw'/>
       <source file='/some/file'/>
       <target dev='sda' bus='sd'/>
-      <serial>sdserial</serial>
     </disk>
     <controller type='usb' index='0'>
       <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/>
-- 
2.17.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3] qemu: format serial and geometry on frontend disk device
Posted by Peter Krempa 6 years, 10 months ago 
On Mon, Jul 02, 2018 at 10:17:07 +0100, Daniel Berrange wrote:
> Currently we format the serial, geometry and error policy on the -drive
> backend argument.
> 
> QEMU added the ability to set serial and geometry on the frontend in
> the 1.2 release deprecating use of -drive, with support being deleted
> from -drive in 3.0.
> 
> We keep formatting error policy on -drive for now, because we don't
> ahve support for that with -device for usb-storage just yet.
> 
> Note that some disk buses (sd) still don't support -device. Although
> QEMU allowed these properties to be set on -drive for if=sd, they
> have been ignored so we now report an error in this case.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
> 
> Changed in v3:
> 
>  - Report error for setting CHS on USB
>  - Report error for setting CHS translation mode for bus != IDE
>  - Use 'bios-chs-trans' property not 'trans'
> 
>  src/qemu/qemu_command.c                       | 46 ++++++++++++++++---
>  .../disk-drive-network-tlsx509.args           | 12 ++---
>  .../disk-drive-network-vxhs.args              |  4 +-
>  tests/qemuxml2argvdata/disk-drive-shared.args |  5 +-
>  tests/qemuxml2argvdata/disk-geometry.args     |  6 +--
>  tests/qemuxml2argvdata/disk-ide-wwn.args      |  5 +-
>  .../qemuxml2argvdata/disk-scsi-disk-wwn.args  |  4 +-
>  tests/qemuxml2argvdata/disk-serial.args       | 10 ++--
>  tests/qemuxml2argvdata/disk-serial.xml        |  1 -
>  tests/qemuxml2xmloutdata/disk-serial.xml      |  1 -
>  10 files changed, 62 insertions(+), 32 deletions(-)
> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 4fc3176ad3..46726b055e 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -1599,7 +1599,7 @@ qemuBuildDiskFrontendAttributeErrorPolicy(virDomainDiskDefPtr disk,
>  }
>  
>  
> -static void
> +static int
>  qemuBuildDiskFrontendAttributes(virDomainDiskDefPtr disk,
>                                  virBufferPtr buf)
>  {
> @@ -1607,14 +1607,27 @@ qemuBuildDiskFrontendAttributes(virDomainDiskDefPtr disk,
>      if (disk->geometry.cylinders > 0 &&
>          disk->geometry.heads > 0 &&
>          disk->geometry.sectors > 0) {
> +        if (disk->bus == VIR_DOMAIN_DISK_BUS_USB) {
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                           _("CHS geometry can not be set for 'usb' bus"));
> +            return -1;
> +        }

I'm not a fan of validation in the command line generator. I'd prefer if
you could move this to qemuDomainDeviceDefValidateDisk so that it gets
rejected at define time.

> +
>          virBufferAsprintf(buf, ",cyls=%u,heads=%u,secs=%u",
>                            disk->geometry.cylinders,
>                            disk->geometry.heads,
>                            disk->geometry.sectors);
>  
> -        if (disk->geometry.trans != VIR_DOMAIN_DISK_TRANS_DEFAULT)
> -            virBufferAsprintf(buf, ",trans=%s",
> +        if (disk->geometry.trans != VIR_DOMAIN_DISK_TRANS_DEFAULT) {
> +            if (disk->bus != VIR_DOMAIN_DISK_BUS_IDE) {
> +                virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                               _("CHS translation mode can only be set for 'ide' bus not '%s'"),
> +                               virDomainDiskBusTypeToString(disk->bus));
> +                return -1;

This would be a good candidate too.


> +            }
> +            virBufferAsprintf(buf, ",bios-chs-trans=%s",
>                                virDomainDiskGeometryTransTypeToString(disk->geometry.trans));
> +        }
>      }
>  
>      if (disk->serial) {
> @@ -1622,7 +1635,7 @@ qemuBuildDiskFrontendAttributes(virDomainDiskDefPtr disk,
>          virBufferEscape(buf, '\\', " ", "%s", disk->serial);
>      }
>  
> -    qemuBuildDiskFrontendAttributeErrorPolicy(disk, buf);
> +    return 0;
>  }
>  
>  
> @@ -1662,11 +1675,27 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk,
>          virBufferAsprintf(&opt, "if=%s",
>                            virDomainDiskQEMUBusTypeToString(disk->bus));
>          virBufferAsprintf(&opt, ",index=%d", idx);
> +
> +        if (disk->serial) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("Serial property not supported for drive bus '%s'"),
> +                           virDomainDiskBusTypeToString(disk->bus));
> +            goto error;
> +        }
> +        if (disk->geometry.cylinders > 0 &&
> +            disk->geometry.heads > 0 &&
> +            disk->geometry.sectors > 0) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("Geometry not supported for drive bus '%s'"),
> +                           virDomainDiskBusTypeToString(disk->bus));
> +            goto error;
> +        }

I don't care that much about the -drive part though since that will
become almost unused.

>      }
>  
> -    /* Format attributes for the drive itself (not the storage backing it) which
> -     * we've formatted historically with -drive */
> -    qemuBuildDiskFrontendAttributes(disk, &opt);
> +    /* werror/rerror are really frontend attributes, but older
> +     * qemu requires them on -drive instead of -device */
> +    qemuBuildDiskFrontendAttributeErrorPolicy(disk, &opt);
> +
>  
>      /* While this is a frontend attribute, it only makes sense to be used when
>       * legacy -drive is used. In modern qemu the 'ide-cd' or 'scsi-cd' are used.
> @@ -2125,6 +2154,9 @@ qemuBuildDriveDevStr(const virDomainDef *def,
>      if (qemuBuildDriveDevCacheStr(disk, &opt, qemuCaps) < 0)
>          goto error;
>  
> +    if (qemuBuildDiskFrontendAttributes(disk, &opt) < 0)
> +        goto error;
> +
>      if (virBufferCheckError(&opt) < 0)
>          goto error;

ACK
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3] qemu: format serial and geometry on frontend disk device
Posted by Daniel P. Berrangé 6 years, 10 months ago 
On Mon, Jul 02, 2018 at 02:31:31PM +0200, Peter Krempa wrote:
> On Mon, Jul 02, 2018 at 10:17:07 +0100, Daniel Berrange wrote:
> > Currently we format the serial, geometry and error policy on the -drive
> > backend argument.
> > 
> > QEMU added the ability to set serial and geometry on the frontend in
> > the 1.2 release deprecating use of -drive, with support being deleted
> > from -drive in 3.0.
> > 
> > We keep formatting error policy on -drive for now, because we don't
> > ahve support for that with -device for usb-storage just yet.
> > 
> > Note that some disk buses (sd) still don't support -device. Although
> > QEMU allowed these properties to be set on -drive for if=sd, they
> > have been ignored so we now report an error in this case.
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> > 
> > Changed in v3:
> > 
> >  - Report error for setting CHS on USB
> >  - Report error for setting CHS translation mode for bus != IDE
> >  - Use 'bios-chs-trans' property not 'trans'
> > 
> >  src/qemu/qemu_command.c                       | 46 ++++++++++++++++---
> >  .../disk-drive-network-tlsx509.args           | 12 ++---
> >  .../disk-drive-network-vxhs.args              |  4 +-
> >  tests/qemuxml2argvdata/disk-drive-shared.args |  5 +-
> >  tests/qemuxml2argvdata/disk-geometry.args     |  6 +--
> >  tests/qemuxml2argvdata/disk-ide-wwn.args      |  5 +-
> >  .../qemuxml2argvdata/disk-scsi-disk-wwn.args  |  4 +-
> >  tests/qemuxml2argvdata/disk-serial.args       | 10 ++--
> >  tests/qemuxml2argvdata/disk-serial.xml        |  1 -
> >  tests/qemuxml2xmloutdata/disk-serial.xml      |  1 -
> >  10 files changed, 62 insertions(+), 32 deletions(-)
> > 
> > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> > index 4fc3176ad3..46726b055e 100644
> > --- a/src/qemu/qemu_command.c
> > +++ b/src/qemu/qemu_command.c
> > @@ -1599,7 +1599,7 @@ qemuBuildDiskFrontendAttributeErrorPolicy(virDomainDiskDefPtr disk,
> >  }
> >  
> >  
> > -static void
> > +static int
> >  qemuBuildDiskFrontendAttributes(virDomainDiskDefPtr disk,
> >                                  virBufferPtr buf)
> >  {
> > @@ -1607,14 +1607,27 @@ qemuBuildDiskFrontendAttributes(virDomainDiskDefPtr disk,
> >      if (disk->geometry.cylinders > 0 &&
> >          disk->geometry.heads > 0 &&
> >          disk->geometry.sectors > 0) {
> > +        if (disk->bus == VIR_DOMAIN_DISK_BUS_USB) {
> > +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> > +                           _("CHS geometry can not be set for 'usb' bus"));
> > +            return -1;
> > +        }
> 
> I'm not a fan of validation in the command line generator. I'd prefer if
> you could move this to qemuDomainDeviceDefValidateDisk so that it gets
> rejected at define time.

Won't that cause us to fail to load the VM definition when upgrading existing
libvirt.

> 
> > +
> >          virBufferAsprintf(buf, ",cyls=%u,heads=%u,secs=%u",
> >                            disk->geometry.cylinders,
> >                            disk->geometry.heads,
> >                            disk->geometry.sectors);
> >  
> > -        if (disk->geometry.trans != VIR_DOMAIN_DISK_TRANS_DEFAULT)
> > -            virBufferAsprintf(buf, ",trans=%s",
> > +        if (disk->geometry.trans != VIR_DOMAIN_DISK_TRANS_DEFAULT) {
> > +            if (disk->bus != VIR_DOMAIN_DISK_BUS_IDE) {
> > +                virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > +                               _("CHS translation mode can only be set for 'ide' bus not '%s'"),
> > +                               virDomainDiskBusTypeToString(disk->bus));
> > +                return -1;
> 
> This would be a good candidate too.
> 
> 
> > +            }
> > +            virBufferAsprintf(buf, ",bios-chs-trans=%s",
> >                                virDomainDiskGeometryTransTypeToString(disk->geometry.trans));
> > +        }
> >      }
> >  
> >      if (disk->serial) {
> > @@ -1622,7 +1635,7 @@ qemuBuildDiskFrontendAttributes(virDomainDiskDefPtr disk,
> >          virBufferEscape(buf, '\\', " ", "%s", disk->serial);
> >      }
> >  
> > -    qemuBuildDiskFrontendAttributeErrorPolicy(disk, buf);
> > +    return 0;
> >  }
> >  
> >  
> > @@ -1662,11 +1675,27 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk,
> >          virBufferAsprintf(&opt, "if=%s",
> >                            virDomainDiskQEMUBusTypeToString(disk->bus));
> >          virBufferAsprintf(&opt, ",index=%d", idx);
> > +
> > +        if (disk->serial) {
> > +            virReportError(VIR_ERR_INTERNAL_ERROR,
> > +                           _("Serial property not supported for drive bus '%s'"),
> > +                           virDomainDiskBusTypeToString(disk->bus));
> > +            goto error;
> > +        }
> > +        if (disk->geometry.cylinders > 0 &&
> > +            disk->geometry.heads > 0 &&
> > +            disk->geometry.sectors > 0) {
> > +            virReportError(VIR_ERR_INTERNAL_ERROR,
> > +                           _("Geometry not supported for drive bus '%s'"),
> > +                           virDomainDiskBusTypeToString(disk->bus));
> > +            goto error;
> > +        }
> 
> I don't care that much about the -drive part though since that will
> become almost unused.
> 
> >      }
> >  
> > -    /* Format attributes for the drive itself (not the storage backing it) which
> > -     * we've formatted historically with -drive */
> > -    qemuBuildDiskFrontendAttributes(disk, &opt);
> > +    /* werror/rerror are really frontend attributes, but older
> > +     * qemu requires them on -drive instead of -device */
> > +    qemuBuildDiskFrontendAttributeErrorPolicy(disk, &opt);
> > +
> >  
> >      /* While this is a frontend attribute, it only makes sense to be used when
> >       * legacy -drive is used. In modern qemu the 'ide-cd' or 'scsi-cd' are used.
> > @@ -2125,6 +2154,9 @@ qemuBuildDriveDevStr(const virDomainDef *def,
> >      if (qemuBuildDriveDevCacheStr(disk, &opt, qemuCaps) < 0)
> >          goto error;
> >  
> > +    if (qemuBuildDiskFrontendAttributes(disk, &opt) < 0)
> > +        goto error;
> > +
> >      if (virBufferCheckError(&opt) < 0)
> >          goto error;
> 
> ACK



Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3] qemu: format serial and geometry on frontend disk device
Posted by Peter Krempa 6 years, 10 months ago 
On Mon, Jul 02, 2018 at 13:35:49 +0100, Daniel Berrange wrote:
> On Mon, Jul 02, 2018 at 02:31:31PM +0200, Peter Krempa wrote:
> > On Mon, Jul 02, 2018 at 10:17:07 +0100, Daniel Berrange wrote:
> > > Currently we format the serial, geometry and error policy on the -drive
> > > backend argument.
> > > 
> > > QEMU added the ability to set serial and geometry on the frontend in
> > > the 1.2 release deprecating use of -drive, with support being deleted
> > > from -drive in 3.0.
> > > 
> > > We keep formatting error policy on -drive for now, because we don't
> > > ahve support for that with -device for usb-storage just yet.
> > > 
> > > Note that some disk buses (sd) still don't support -device. Although
> > > QEMU allowed these properties to be set on -drive for if=sd, they
> > > have been ignored so we now report an error in this case.
> > > 
> > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > > ---
> > > 
> > > Changed in v3:
> > > 
> > >  - Report error for setting CHS on USB
> > >  - Report error for setting CHS translation mode for bus != IDE
> > >  - Use 'bios-chs-trans' property not 'trans'
> > > 
> > >  src/qemu/qemu_command.c                       | 46 ++++++++++++++++---
> > >  .../disk-drive-network-tlsx509.args           | 12 ++---
> > >  .../disk-drive-network-vxhs.args              |  4 +-
> > >  tests/qemuxml2argvdata/disk-drive-shared.args |  5 +-
> > >  tests/qemuxml2argvdata/disk-geometry.args     |  6 +--
> > >  tests/qemuxml2argvdata/disk-ide-wwn.args      |  5 +-
> > >  .../qemuxml2argvdata/disk-scsi-disk-wwn.args  |  4 +-
> > >  tests/qemuxml2argvdata/disk-serial.args       | 10 ++--
> > >  tests/qemuxml2argvdata/disk-serial.xml        |  1 -
> > >  tests/qemuxml2xmloutdata/disk-serial.xml      |  1 -
> > >  10 files changed, 62 insertions(+), 32 deletions(-)
> > > 
> > > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> > > index 4fc3176ad3..46726b055e 100644
> > > --- a/src/qemu/qemu_command.c
> > > +++ b/src/qemu/qemu_command.c
> > > @@ -1599,7 +1599,7 @@ qemuBuildDiskFrontendAttributeErrorPolicy(virDomainDiskDefPtr disk,
> > >  }
> > >  
> > >  
> > > -static void
> > > +static int
> > >  qemuBuildDiskFrontendAttributes(virDomainDiskDefPtr disk,
> > >                                  virBufferPtr buf)
> > >  {
> > > @@ -1607,14 +1607,27 @@ qemuBuildDiskFrontendAttributes(virDomainDiskDefPtr disk,
> > >      if (disk->geometry.cylinders > 0 &&
> > >          disk->geometry.heads > 0 &&
> > >          disk->geometry.sectors > 0) {
> > > +        if (disk->bus == VIR_DOMAIN_DISK_BUS_USB) {
> > > +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> > > +                           _("CHS geometry can not be set for 'usb' bus"));
> > > +            return -1;
> > > +        }
> > 
> > I'm not a fan of validation in the command line generator. I'd prefer if
> > you could move this to qemuDomainDeviceDefValidateDisk so that it gets
> > rejected at define time.
> 
> Won't that cause us to fail to load the VM definition when upgrading existing
> libvirt.

No. The validation callbacks are not called for existing configs on
disk. They are re-called on vm startup though so that any existing
config gets surely validated.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3] qemu: format serial and geometry on frontend disk device
Posted by Daniel P. Berrangé 6 years, 10 months ago 
On Mon, Jul 02, 2018 at 02:38:52PM +0200, Peter Krempa wrote:
> On Mon, Jul 02, 2018 at 13:35:49 +0100, Daniel Berrange wrote:
> > On Mon, Jul 02, 2018 at 02:31:31PM +0200, Peter Krempa wrote:
> > > On Mon, Jul 02, 2018 at 10:17:07 +0100, Daniel Berrange wrote:
> > > > Currently we format the serial, geometry and error policy on the -drive
> > > > backend argument.
> > > > 
> > > > QEMU added the ability to set serial and geometry on the frontend in
> > > > the 1.2 release deprecating use of -drive, with support being deleted
> > > > from -drive in 3.0.
> > > > 
> > > > We keep formatting error policy on -drive for now, because we don't
> > > > ahve support for that with -device for usb-storage just yet.
> > > > 
> > > > Note that some disk buses (sd) still don't support -device. Although
> > > > QEMU allowed these properties to be set on -drive for if=sd, they
> > > > have been ignored so we now report an error in this case.
> > > > 
> > > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > > > ---
> > > > 
> > > > Changed in v3:
> > > > 
> > > >  - Report error for setting CHS on USB
> > > >  - Report error for setting CHS translation mode for bus != IDE
> > > >  - Use 'bios-chs-trans' property not 'trans'
> > > > 
> > > >  src/qemu/qemu_command.c                       | 46 ++++++++++++++++---
> > > >  .../disk-drive-network-tlsx509.args           | 12 ++---
> > > >  .../disk-drive-network-vxhs.args              |  4 +-
> > > >  tests/qemuxml2argvdata/disk-drive-shared.args |  5 +-
> > > >  tests/qemuxml2argvdata/disk-geometry.args     |  6 +--
> > > >  tests/qemuxml2argvdata/disk-ide-wwn.args      |  5 +-
> > > >  .../qemuxml2argvdata/disk-scsi-disk-wwn.args  |  4 +-
> > > >  tests/qemuxml2argvdata/disk-serial.args       | 10 ++--
> > > >  tests/qemuxml2argvdata/disk-serial.xml        |  1 -
> > > >  tests/qemuxml2xmloutdata/disk-serial.xml      |  1 -
> > > >  10 files changed, 62 insertions(+), 32 deletions(-)
> > > > 
> > > > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> > > > index 4fc3176ad3..46726b055e 100644
> > > > --- a/src/qemu/qemu_command.c
> > > > +++ b/src/qemu/qemu_command.c
> > > > @@ -1599,7 +1599,7 @@ qemuBuildDiskFrontendAttributeErrorPolicy(virDomainDiskDefPtr disk,
> > > >  }
> > > >  
> > > >  
> > > > -static void
> > > > +static int
> > > >  qemuBuildDiskFrontendAttributes(virDomainDiskDefPtr disk,
> > > >                                  virBufferPtr buf)
> > > >  {
> > > > @@ -1607,14 +1607,27 @@ qemuBuildDiskFrontendAttributes(virDomainDiskDefPtr disk,
> > > >      if (disk->geometry.cylinders > 0 &&
> > > >          disk->geometry.heads > 0 &&
> > > >          disk->geometry.sectors > 0) {
> > > > +        if (disk->bus == VIR_DOMAIN_DISK_BUS_USB) {
> > > > +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> > > > +                           _("CHS geometry can not be set for 'usb' bus"));
> > > > +            return -1;
> > > > +        }
> > > 
> > > I'm not a fan of validation in the command line generator. I'd prefer if
> > > you could move this to qemuDomainDeviceDefValidateDisk so that it gets
> > > rejected at define time.
> > 
> > Won't that cause us to fail to load the VM definition when upgrading existing
> > libvirt.
> 
> No. The validation callbacks are not called for existing configs on
> disk. They are re-called on vm startup though so that any existing
> config gets surely validated.

Ok, that's great, I'll make such a change.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patchew 2.1-devel-8c64711

© 2016 - 2025 Red Hat, Inc.