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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux