[libvirt] [PATCH v3] qemu: format serial and geometry on frontend disk device
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@redhat.com> --- 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; + } + 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; + } + 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; + } } - /* 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; diff --git a/tests/qemuxml2argvdata/disk-drive-network-tlsx509.args b/tests/qemuxml2argvdata/disk-drive-network-tlsx509.args index e25f45742c..b5e73064c0 100644 --- a/tests/qemuxml2argvdata/disk-drive-network-tlsx509.args +++ b/tests/qemuxml2argvdata/disk-drive-network-tlsx509.args @@ -28,22 +28,22 @@ server,nowait \ -drive file.driver=vxhs,file.tls-creds=objvirtio-disk0_tls0,\ file.vdisk-id=eb90327c-8302-4725-9e1b-4e85ed4dc251,\ file.server.host=192.168.0.1,file.server.port=9999,format=raw,if=none,\ -id=drive-virtio-disk0,serial=eb90327c-8302-4725-9e1b-4e85ed4dc251,cache=none \ +id=drive-virtio-disk0,cache=none \ -device virtio-blk-pci,bus=pci.0,addr=0x4,drive=drive-virtio-disk0,\ -id=virtio-disk0 \ +id=virtio-disk0,serial=eb90327c-8302-4725-9e1b-4e85ed4dc251 \ -object tls-creds-x509,id=objvirtio-disk1_tls0,dir=/etc/pki/libvirt-vxhs/dummy,\ ,path,endpoint=client,verify-peer=yes \ -drive file.driver=vxhs,file.tls-creds=objvirtio-disk1_tls0,\ file.vdisk-id=eb90327c-8302-4725-9e1b-4e85ed4dc252,\ file.server.host=192.168.0.2,file.server.port=9999,format=raw,if=none,\ -id=drive-virtio-disk1,serial=eb90327c-8302-4725-9e1b-4e85ed4dc252,cache=none \ +id=drive-virtio-disk1,cache=none \ -device virtio-blk-pci,bus=pci.0,addr=0x5,drive=drive-virtio-disk1,\ -id=virtio-disk1 \ +id=virtio-disk1,serial=eb90327c-8302-4725-9e1b-4e85ed4dc252 \ -drive file.driver=vxhs,file.vdisk-id=eb90327c-8302-4725-9e1b-4e85ed4dc253,\ file.server.host=192.168.0.3,file.server.port=9999,format=raw,if=none,\ -id=drive-virtio-disk2,serial=eb90327c-8302-4725-9e1b-4e85ed4dc252,cache=none \ +id=drive-virtio-disk2,cache=none \ -device virtio-blk-pci,bus=pci.0,addr=0x6,drive=drive-virtio-disk2,\ -id=virtio-disk2 \ +id=virtio-disk2,serial=eb90327c-8302-4725-9e1b-4e85ed4dc252 \ -object tls-creds-x509,id=objvirtio-disk3_tls0,dir=/etc/pki/libvirt-nbd/dummy,,\ path,endpoint=client,verify-peer=yes \ -drive file.driver=nbd,file.server.type=inet,file.server.host=example.com,\ diff --git a/tests/qemuxml2argvdata/disk-drive-network-vxhs.args b/tests/qemuxml2argvdata/disk-drive-network-vxhs.args index c2a1d4681f..fad4dfd942 100644 --- a/tests/qemuxml2argvdata/disk-drive-network-vxhs.args +++ b/tests/qemuxml2argvdata/disk-drive-network-vxhs.args @@ -25,6 +25,6 @@ server,nowait \ -usb \ -drive file.driver=vxhs,file.vdisk-id=eb90327c-8302-4725-9e1b-4e85ed4dc251,\ file.server.host=192.168.0.1,file.server.port=9999,format=raw,if=none,\ -id=drive-virtio-disk0,serial=eb90327c-8302-4725-9e1b-4e85ed4dc251,cache=none \ +id=drive-virtio-disk0,cache=none \ -device virtio-blk-pci,bus=pci.0,addr=0x4,drive=drive-virtio-disk0,\ -id=virtio-disk0 +id=virtio-disk0,serial=eb90327c-8302-4725-9e1b-4e85ed4dc251 diff --git a/tests/qemuxml2argvdata/disk-drive-shared.args b/tests/qemuxml2argvdata/disk-drive-shared.args index 5306fdf750..032e86e291 100644 --- a/tests/qemuxml2argvdata/disk-drive-shared.args +++ b/tests/qemuxml2argvdata/disk-drive-shared.args @@ -23,8 +23,9 @@ server,nowait \ -boot c \ -usb \ -drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0,\ -serial=XYZXYZXYZYXXYZYZYXYZY,cache=none \ --device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \ +cache=none \ +-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,\ +serial=XYZXYZXYZYXXYZYZYXYZY \ -drive file=/dev/HostVG/QEMUGuest2,format=raw,if=none,id=drive-ide0-1-0,\ media=cdrom,readonly=on \ -device ide-drive,bus=ide.1,unit=0,drive=drive-ide0-1-0,id=ide0-1-0 \ diff --git a/tests/qemuxml2argvdata/disk-geometry.args b/tests/qemuxml2argvdata/disk-geometry.args index b173ab6407..f3e35b5f9e 100644 --- a/tests/qemuxml2argvdata/disk-geometry.args +++ b/tests/qemuxml2argvdata/disk-geometry.args @@ -22,7 +22,7 @@ server,nowait \ -no-acpi \ -boot c \ -usb \ --drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0,\ -cyls=16383,heads=16,secs=63,trans=lba \ --device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \ +-drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \ +-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,cyls=16383,\ +heads=16,secs=63,bios-chs-trans=lba \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/disk-ide-wwn.args b/tests/qemuxml2argvdata/disk-ide-wwn.args index e6d2758ec3..a0e3f06b68 100644 --- a/tests/qemuxml2argvdata/disk-ide-wwn.args +++ b/tests/qemuxml2argvdata/disk-ide-wwn.args @@ -22,8 +22,7 @@ server,nowait \ -no-acpi \ -boot c \ -usb \ --drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-1,\ -serial=WD-WMAP9A966149 \ +-drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-1 \ -device ide-hd,bus=ide.0,unit=1,drive=drive-ide0-0-1,id=ide0-0-1,\ -wwn=0x5000c50015ea71ad \ +wwn=0x5000c50015ea71ad,serial=WD-WMAP9A966149 \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/disk-scsi-disk-wwn.args b/tests/qemuxml2argvdata/disk-scsi-disk-wwn.args index 29607e8927..6407fd002d 100644 --- a/tests/qemuxml2argvdata/disk-scsi-disk-wwn.args +++ b/tests/qemuxml2argvdata/disk-scsi-disk-wwn.args @@ -25,9 +25,9 @@ server,nowait \ -device lsi,id=scsi1,bus=pci.0,addr=0x4 \ -usb \ -drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-scsi0-0-1-0,\ -serial=WD-WMAP9A966149,readonly=on \ +readonly=on \ -device scsi-cd,bus=scsi0.0,channel=0,scsi-id=1,lun=0,drive=drive-scsi0-0-1-0,\ -id=scsi0-0-1-0,wwn=0x5000c50015ea71ac \ +id=scsi0-0-1-0,wwn=0x5000c50015ea71ac,serial=WD-WMAP9A966149 \ -drive file=/dev/HostVG/QEMUGuest2,format=raw,if=none,id=drive-scsi0-0-0-0 \ -device scsi-hd,bus=scsi0.0,channel=0,scsi-id=0,lun=0,drive=drive-scsi0-0-0-0,\ id=scsi0-0-0-0,wwn=0x5000c50015ea71ad \ diff --git a/tests/qemuxml2argvdata/disk-serial.args b/tests/qemuxml2argvdata/disk-serial.args index 88310b009f..5892f64c29 100644 --- a/tests/qemuxml2argvdata/disk-serial.args +++ b/tests/qemuxml2argvdata/disk-serial.args @@ -22,11 +22,11 @@ server,nowait \ -no-acpi \ -boot c \ -usb \ --drive 'file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-1,\ +-drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-1 \ +-device 'ide-drive,bus=ide.0,unit=1,drive=drive-ide0-0-1,id=ide0-0-1,\ serial=\ \ WD-WMAP9A966149' \ --device ide-drive,bus=ide.0,unit=1,drive=drive-ide0-0-1,id=ide0-0-1 \ --drive 'file=/dev/HostVG/AllSerialChars,format=raw,if=none,id=drive-ide0-0-2,\ +-drive file=/dev/HostVG/AllSerialChars,format=raw,if=none,id=drive-ide0-0-2 \ +-device 'ide-drive,bus=ide.0,unit=2,drive=drive-ide0-0-2,id=ide0-0-2,\ serial=abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789-_\ .+' \ --device ide-drive,bus=ide.0,unit=2,drive=drive-ide0-0-2,id=ide0-0-2 \ --drive file=/some/file,format=raw,if=sd,index=0,serial=sdserial \ +-drive file=/some/file,format=raw,if=sd,index=0 \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/disk-serial.xml b/tests/qemuxml2argvdata/disk-serial.xml index ea71f2c339..18e72bb4ba 100644 --- a/tests/qemuxml2argvdata/disk-serial.xml +++ b/tests/qemuxml2argvdata/disk-serial.xml @@ -29,7 +29,6 @@ <disk type='file' device='disk'> <source file='/some/file'/> <target dev='sda' bus='sd'/> - <serial>sdserial</serial> </disk> <controller type='usb' index='0'/> <controller type='ide' index='0'/> diff --git a/tests/qemuxml2xmloutdata/disk-serial.xml b/tests/qemuxml2xmloutdata/disk-serial.xml index 9313c699b6..a803cd959c 100644 --- a/tests/qemuxml2xmloutdata/disk-serial.xml +++ b/tests/qemuxml2xmloutdata/disk-serial.xml @@ -32,7 +32,6 @@ <driver name='qemu' type='raw'/> <source file='/some/file'/> <target dev='sda' bus='sd'/> - <serial>sdserial</serial> </disk> <controller type='usb' index='0'> <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> -- 2.17.1
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@redhat.com> ---
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
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@redhat.com> ---
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 :|
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@redhat.com> ---
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.
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@redhat.com> ---
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 :|
participants (2)
-
Daniel P. Berrangé -
Peter Krempa