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@xxxxxxxxxx>
> ---
>
> 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
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
