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