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

[Peter Krempa]

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 at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20180702/7d35c00f/attachment-0001.sig>