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@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.
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@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
