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:38:52PM +0200, Peter Krempa wrote:
> On Mon, Jul 02, 2018 at 13:35:49 +0100, Daniel Berrange wrote:
> > 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.
> 
> No. The validation callbacks are not called for existing configs on
> disk. They are re-called on vm startup though so that any existing
> config gets surely validated.

Ok, that's great, I'll make such a change.


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