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

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(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. > > 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.