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

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(a)redhat.com&gt; > --- > > 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