[libvirt] [PATCH 0/9] qemu: command: -drive formatting cleanups (blockdev-add saga)
Kill some ugly code. Peter Krempa (9): util: Fix condition check in virDiskNameToIndex qemu: command: Remove dead code when formatting -drive qemu: command: Move disk index validation closer to usage qemu: command: Directly report bus type in qemuBuildDriveStrValidate qemu: command: Refactor qemuBuildDriveStrValidate to make qemuCaps optional qemu: command: Merge checks from qemuBuildDriveStrValidate to qemuCheckDiskConfig qemu: command: Refactor blkiotune checks to tolerate NULL qemuCaps qemu: command: Move blkiotune checks to qemuCheckDiskConfig qemu: command: Move disk serial validation to checker function src/qemu/qemu_command.c | 485 +++++++++++++++++++++++------------------------- src/qemu/qemu_command.h | 3 +- src/qemu/qemu_driver.c | 2 +- src/util/virutil.c | 2 +- 4 files changed, 236 insertions(+), 256 deletions(-) -- 2.14.3
Use the more common '< 0' rather than the non-zero check. --- src/util/virutil.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/virutil.c b/src/util/virutil.c index 170e92192..8bdcb02fd 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -562,7 +562,7 @@ int virDiskNameToIndex(const char *name) { int idx; - if (virDiskNameParse(name, &idx, NULL)) + if (virDiskNameParse(name, &idx, NULL) < 0) idx = -1; return idx; -- 2.14.3
busid and unitid are ever used only if the device is an SD card due to the check in qemuDiskBusNeedsDeviceArg. Since the SD card does not have an bus or unit number, most of the code and command line formatter can be removed since it will never be used. --- src/qemu/qemu_command.c | 37 +------------------------------------ 1 file changed, 1 insertion(+), 36 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index b1cfafa79..a77a0a1fa 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1639,38 +1639,11 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk, const char *trans = virDomainDiskGeometryTransTypeToString(disk->geometry.trans); int idx = virDiskNameToIndex(disk->dst); - int busid = -1, unitid = -1; bool emitDeviceSyntax = qemuDiskBusNeedsDeviceArg(disk->bus); if (qemuBuildDriveStrValidate(disk, qemuCaps, bus, idx) < 0) goto error; - switch (disk->bus) { - case VIR_DOMAIN_DISK_BUS_SCSI: - busid = disk->info.addr.drive.controller; - unitid = disk->info.addr.drive.unit; - break; - - case VIR_DOMAIN_DISK_BUS_IDE: - busid = disk->info.addr.drive.bus; - unitid = disk->info.addr.drive.unit; - break; - - case VIR_DOMAIN_DISK_BUS_FDC: - unitid = disk->info.addr.drive.unit; - break; - - case VIR_DOMAIN_DISK_BUS_VIRTIO: - idx = -1; - break; - - case VIR_DOMAIN_DISK_BUS_XEN: - case VIR_DOMAIN_DISK_BUS_SD: - /* Xen and SD have no address type currently, so assign - * based on index */ - break; - } - if (qemuBuildDriveSourceStr(disk, cfg, &opt, qemuCaps) < 0) goto error; @@ -1698,15 +1671,7 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk, virBufferAsprintf(&opt, ",id=%s", drivealias); VIR_FREE(drivealias); } else { - if (busid == -1 && unitid == -1) { - if (idx != -1) - virBufferAsprintf(&opt, ",index=%d", idx); - } else { - if (busid != -1) - virBufferAsprintf(&opt, ",bus=%d", busid); - if (unitid != -1) - virBufferAsprintf(&opt, ",unit=%d", unitid); - } + virBufferAsprintf(&opt, ",index=%d", idx); } if (bootable && virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_BOOT) && -- 2.14.3
The disk index validation is used only in very specific cases and does not need to be performed otherwise. Move it out of the global check into the usage place. --- src/qemu/qemu_command.c | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index a77a0a1fa..e663bc357 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1468,15 +1468,8 @@ qemuBuildDriveSourceStr(virDomainDiskDefPtr disk, static int qemuBuildDriveStrValidate(virDomainDiskDefPtr disk, virQEMUCapsPtr qemuCaps, - const char *bus, - int idx) + const char *bus) { - if (idx < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unsupported disk type '%s'"), disk->dst); - return -1; - } - switch (disk->bus) { case VIR_DOMAIN_DISK_BUS_SCSI: if (disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE) { @@ -1638,10 +1631,9 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk, const char *bus = virDomainDiskQEMUBusTypeToString(disk->bus); const char *trans = virDomainDiskGeometryTransTypeToString(disk->geometry.trans); - int idx = virDiskNameToIndex(disk->dst); bool emitDeviceSyntax = qemuDiskBusNeedsDeviceArg(disk->bus); - if (qemuBuildDriveStrValidate(disk, qemuCaps, bus, idx) < 0) + if (qemuBuildDriveStrValidate(disk, qemuCaps, bus) < 0) goto error; if (qemuBuildDriveSourceStr(disk, cfg, &opt, qemuCaps) < 0) @@ -1671,6 +1663,13 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk, virBufferAsprintf(&opt, ",id=%s", drivealias); VIR_FREE(drivealias); } else { + int idx = virDiskNameToIndex(disk->dst); + + if (idx < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unsupported disk type '%s'"), disk->dst); + goto error; + } virBufferAsprintf(&opt, ",index=%d", idx); } if (bootable && -- 2.14.3
All of the error message are already in a conditional block with known bus type. Inline the bus type rather than formatting it from a separate variable. --- src/qemu/qemu_command.c | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index e663bc357..0d38fc3f2 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1467,8 +1467,7 @@ qemuBuildDriveSourceStr(virDomainDiskDefPtr disk, static int qemuBuildDriveStrValidate(virDomainDiskDefPtr disk, - virQEMUCapsPtr qemuCaps, - const char *bus) + virQEMUCapsPtr qemuCaps) { switch (disk->bus) { case VIR_DOMAIN_DISK_BUS_SCSI: @@ -1496,8 +1495,8 @@ qemuBuildDriveStrValidate(virDomainDiskDefPtr disk, } /* We can only have 1 IDE controller (currently) */ if (disk->info.addr.drive.controller != 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Only 1 %s controller is supported"), bus); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Only 1 IDE controller is supported")); return -1; } break; @@ -1510,14 +1509,14 @@ qemuBuildDriveStrValidate(virDomainDiskDefPtr disk, } /* We can only have 1 FDC controller (currently) */ if (disk->info.addr.drive.controller != 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Only 1 %s controller is supported"), bus); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Only 1 fdc controller is supported")); return -1; } /* We can only have 1 FDC bus (currently) */ if (disk->info.addr.drive.bus != 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Only 1 %s bus is supported"), bus); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Only 1 fdc bus is supported")); return -1; } if (disk->info.addr.drive.target != 0) { @@ -1628,12 +1627,11 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk, virQEMUCapsPtr qemuCaps) { virBuffer opt = VIR_BUFFER_INITIALIZER; - const char *bus = virDomainDiskQEMUBusTypeToString(disk->bus); const char *trans = virDomainDiskGeometryTransTypeToString(disk->geometry.trans); bool emitDeviceSyntax = qemuDiskBusNeedsDeviceArg(disk->bus); - if (qemuBuildDriveStrValidate(disk, qemuCaps, bus) < 0) + if (qemuBuildDriveStrValidate(disk, qemuCaps) < 0) goto error; if (qemuBuildDriveSourceStr(disk, cfg, &opt, qemuCaps) < 0) @@ -1642,7 +1640,8 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk, if (emitDeviceSyntax) virBufferAddLit(&opt, "if=none"); else - virBufferAsprintf(&opt, "if=%s", bus); + virBufferAsprintf(&opt, "if=%s", + virDomainDiskQEMUBusTypeToString(disk->bus)); if (disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM) { if (disk->bus == VIR_DOMAIN_DISK_BUS_SCSI) { -- 2.14.3
On Fri, Nov 03, 2017 at 01:03:32PM +0100, Peter Krempa wrote:
All of the error message are already in a conditional block with known bus type. Inline the bus type rather than formatting it from a separate variable. --- src/qemu/qemu_command.c | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index e663bc357..0d38fc3f2 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1510,14 +1509,14 @@ qemuBuildDriveStrValidate(virDomainDiskDefPtr disk, } /* We can only have 1 FDC controller (currently) */ if (disk->info.addr.drive.controller != 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Only 1 %s controller is supported"), bus); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Only 1 fdc controller is supported")); return -1; } /* We can only have 1 FDC bus (currently) */ if (disk->info.addr.drive.bus != 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Only 1 %s bus is supported"), bus); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Only 1 fdc bus is supported"));
s/c /c /
return -1; } if (disk->info.addr.drive.target != 0) {
Jan
To allow merging this with other disk type checks we need to check qemuCaps only when available, since some of the checks are executed on disk cold-plug and thus capabilities should not be checked. Make the checks optional by making them conditional on qemuCaps not being NULL. --- src/qemu/qemu_command.c | 98 +++++++++++++++++++++++++------------------------ 1 file changed, 50 insertions(+), 48 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 0d38fc3f2..f7e9c0fa4 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1553,31 +1553,6 @@ qemuBuildDriveStrValidate(virDomainDiskDefPtr disk, return -1; } - if (disk->serial && - virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_SERIAL)) { - if (disk->bus == VIR_DOMAIN_DISK_BUS_SCSI && - disk->device == VIR_DOMAIN_DISK_DEVICE_LUN) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("scsi-block 'lun' devices do not support the " - "serial property")); - return -1; - } - } - - if (disk->cachemode == VIR_DOMAIN_DISK_CACHE_DIRECTSYNC && - !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_CACHE_DIRECTSYNC)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("disk cache mode 'directsync' is not supported by this QEMU")); - return -1; - } - - if (disk->cachemode == VIR_DOMAIN_DISK_CACHE_UNSAFE && - !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_CACHE_UNSAFE)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("disk cache mode 'unsafe' is not supported by this QEMU")); - return -1; - } - if (disk->iomode == VIR_DOMAIN_DISK_IO_NATIVE && disk->cachemode != VIR_DOMAIN_DISK_CACHE_DIRECTSYNC && disk->cachemode != VIR_DOMAIN_DISK_CACHE_DISABLE) { @@ -1588,32 +1563,59 @@ qemuBuildDriveStrValidate(virDomainDiskDefPtr disk, return -1; } - if (disk->copy_on_read && - !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_COPY_ON_READ)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("copy_on_read is not supported by this QEMU binary")); - return -1; - } + if (qemuCaps) { + if (disk->serial && + virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_SERIAL)) { + if (disk->bus == VIR_DOMAIN_DISK_BUS_SCSI && + disk->device == VIR_DOMAIN_DISK_DEVICE_LUN) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("scsi-block 'lun' devices do not support the " + "serial property")); + return -1; + } + } - if (disk->discard && - !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_DISCARD)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("discard is not supported by this QEMU binary")); - return -1; - } + if (disk->cachemode == VIR_DOMAIN_DISK_CACHE_DIRECTSYNC && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_CACHE_DIRECTSYNC)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("disk cache mode 'directsync' is not supported by this QEMU")); + return -1; + } - if (disk->detect_zeroes && - !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_DETECT_ZEROES)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("detect_zeroes is not supported by this QEMU binary")); - return -1; - } + if (disk->cachemode == VIR_DOMAIN_DISK_CACHE_UNSAFE && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_CACHE_UNSAFE)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("disk cache mode 'unsafe' is not supported by this QEMU")); + return -1; + } - if (disk->iomode && - !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_AIO)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("disk aio mode not supported with this QEMU binary")); - return -1; + if (disk->copy_on_read && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_COPY_ON_READ)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("copy_on_read is not supported by this QEMU binary")); + return -1; + } + + if (disk->discard && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_DISCARD)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("discard is not supported by this QEMU binary")); + return -1; + } + + if (disk->detect_zeroes && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_DETECT_ZEROES)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("detect_zeroes is not supported by this QEMU binary")); + return -1; + } + + if (disk->iomode && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_AIO)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("disk aio mode not supported with this QEMU binary")); + return -1; + } } return 0; -- 2.14.3
Stash all the disk definition and capability checks into one function. --- src/qemu/qemu_command.c | 324 ++++++++++++++++++++++++------------------------ src/qemu/qemu_command.h | 3 +- src/qemu/qemu_driver.c | 2 +- 3 files changed, 166 insertions(+), 163 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index f7e9c0fa4..fa02a3895 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1199,9 +1199,16 @@ qemuCheckDiskConfigBlkdeviotune(virDomainDiskDefPtr disk, } -/* Perform disk definition config validity checks */ +/** + * qemuCheckDiskConfig: + * @disk: disk definition + * @qemuCaps: qemu capabilities, may be NULL for cold-plug check + * + * Perform disk definition config validity checks. Returns -1 on error with + * error reported */ int -qemuCheckDiskConfig(virDomainDiskDefPtr disk) +qemuCheckDiskConfig(virDomainDiskDefPtr disk, + virQEMUCapsPtr qemuCaps) { if (virDiskNameToIndex(disk->dst) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -1258,6 +1265,156 @@ qemuCheckDiskConfig(virDomainDiskDefPtr disk) return -1; } } + + switch (disk->bus) { + case VIR_DOMAIN_DISK_BUS_SCSI: + if (disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("unexpected address type for scsi disk")); + return -1; + } + + /* Setting bus= attr for SCSI drives, causes a controller + * to be created. Yes this is slightly odd. It is not possible + * to have > 1 bus on a SCSI controller (yet). */ + if (disk->info.addr.drive.bus != 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("SCSI controller only supports 1 bus")); + return -1; + } + break; + + case VIR_DOMAIN_DISK_BUS_IDE: + if (disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("unexpected address type for ide disk")); + return -1; + } + /* We can only have 1 IDE controller (currently) */ + if (disk->info.addr.drive.controller != 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Only 1 IDE controller is supported")); + return -1; + } + break; + + case VIR_DOMAIN_DISK_BUS_FDC: + if (disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("unexpected address type for fdc disk")); + return -1; + } + /* We can only have 1 FDC controller (currently) */ + if (disk->info.addr.drive.controller != 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Only 1 fdc controller is supported")); + return -1; + } + /* We can only have 1 FDC bus (currently) */ + if (disk->info.addr.drive.bus != 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Only 1 fdc bus is supported")); + return -1; + } + if (disk->info.addr.drive.target != 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("target must be 0 for controller fdc")); + return -1; + } + break; + + case VIR_DOMAIN_DISK_BUS_VIRTIO: + case VIR_DOMAIN_DISK_BUS_XEN: + case VIR_DOMAIN_DISK_BUS_SD: + break; + } + + if (disk->src->readonly && + disk->device == VIR_DOMAIN_DISK_DEVICE_DISK) { + if (disk->bus == VIR_DOMAIN_DISK_BUS_IDE) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("readonly ide disks are not supported")); + return -1; + } + + if (disk->bus == VIR_DOMAIN_DISK_BUS_SATA) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("readonly sata disks are not supported")); + return -1; + } + } + + if (disk->transient) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("transient disks not supported yet")); + return -1; + } + + if (disk->iomode == VIR_DOMAIN_DISK_IO_NATIVE && + disk->cachemode != VIR_DOMAIN_DISK_CACHE_DIRECTSYNC && + disk->cachemode != VIR_DOMAIN_DISK_CACHE_DISABLE) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("native I/O needs either no disk cache " + "or directsync cache mode, QEMU will fallback " + "to aio=threads")); + return -1; + } + + if (qemuCaps) { + if (disk->serial && + virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_SERIAL)) { + if (disk->bus == VIR_DOMAIN_DISK_BUS_SCSI && + disk->device == VIR_DOMAIN_DISK_DEVICE_LUN) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("scsi-block 'lun' devices do not support the " + "serial property")); + return -1; + } + } + + if (disk->cachemode == VIR_DOMAIN_DISK_CACHE_DIRECTSYNC && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_CACHE_DIRECTSYNC)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("disk cache mode 'directsync' is not supported by this QEMU")); + return -1; + } + + if (disk->cachemode == VIR_DOMAIN_DISK_CACHE_UNSAFE && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_CACHE_UNSAFE)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("disk cache mode 'unsafe' is not supported by this QEMU")); + return -1; + } + + if (disk->copy_on_read && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_COPY_ON_READ)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("copy_on_read is not supported by this QEMU binary")); + return -1; + } + + if (disk->discard && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_DISCARD)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("discard is not supported by this QEMU binary")); + return -1; + } + + if (disk->detect_zeroes && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_DETECT_ZEROES)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("detect_zeroes is not supported by this QEMU binary")); + return -1; + } + + if (disk->iomode && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_AIO)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("disk aio mode not supported with this QEMU binary")); + return -1; + } + } + return 0; } @@ -1465,163 +1622,6 @@ qemuBuildDriveSourceStr(virDomainDiskDefPtr disk, } -static int -qemuBuildDriveStrValidate(virDomainDiskDefPtr disk, - virQEMUCapsPtr qemuCaps) -{ - switch (disk->bus) { - case VIR_DOMAIN_DISK_BUS_SCSI: - if (disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("unexpected address type for scsi disk")); - return -1; - } - - /* Setting bus= attr for SCSI drives, causes a controller - * to be created. Yes this is slightly odd. It is not possible - * to have > 1 bus on a SCSI controller (yet). */ - if (disk->info.addr.drive.bus != 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("SCSI controller only supports 1 bus")); - return -1; - } - break; - - case VIR_DOMAIN_DISK_BUS_IDE: - if (disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("unexpected address type for ide disk")); - return -1; - } - /* We can only have 1 IDE controller (currently) */ - if (disk->info.addr.drive.controller != 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Only 1 IDE controller is supported")); - return -1; - } - break; - - case VIR_DOMAIN_DISK_BUS_FDC: - if (disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("unexpected address type for fdc disk")); - return -1; - } - /* We can only have 1 FDC controller (currently) */ - if (disk->info.addr.drive.controller != 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Only 1 fdc controller is supported")); - return -1; - } - /* We can only have 1 FDC bus (currently) */ - if (disk->info.addr.drive.bus != 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Only 1 fdc bus is supported")); - return -1; - } - if (disk->info.addr.drive.target != 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("target must be 0 for controller fdc")); - return -1; - } - break; - - case VIR_DOMAIN_DISK_BUS_VIRTIO: - case VIR_DOMAIN_DISK_BUS_XEN: - case VIR_DOMAIN_DISK_BUS_SD: - break; - } - - if (disk->src->readonly && - disk->device == VIR_DOMAIN_DISK_DEVICE_DISK) { - if (disk->bus == VIR_DOMAIN_DISK_BUS_IDE) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("readonly ide disks are not supported")); - return -1; - } - - if (disk->bus == VIR_DOMAIN_DISK_BUS_SATA) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("readonly sata disks are not supported")); - return -1; - } - } - - if (disk->transient) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("transient disks not supported yet")); - return -1; - } - - if (disk->iomode == VIR_DOMAIN_DISK_IO_NATIVE && - disk->cachemode != VIR_DOMAIN_DISK_CACHE_DIRECTSYNC && - disk->cachemode != VIR_DOMAIN_DISK_CACHE_DISABLE) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("native I/O needs either no disk cache " - "or directsync cache mode, QEMU will fallback " - "to aio=threads")); - return -1; - } - - if (qemuCaps) { - if (disk->serial && - virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_SERIAL)) { - if (disk->bus == VIR_DOMAIN_DISK_BUS_SCSI && - disk->device == VIR_DOMAIN_DISK_DEVICE_LUN) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("scsi-block 'lun' devices do not support the " - "serial property")); - return -1; - } - } - - if (disk->cachemode == VIR_DOMAIN_DISK_CACHE_DIRECTSYNC && - !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_CACHE_DIRECTSYNC)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("disk cache mode 'directsync' is not supported by this QEMU")); - return -1; - } - - if (disk->cachemode == VIR_DOMAIN_DISK_CACHE_UNSAFE && - !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_CACHE_UNSAFE)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("disk cache mode 'unsafe' is not supported by this QEMU")); - return -1; - } - - if (disk->copy_on_read && - !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_COPY_ON_READ)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("copy_on_read is not supported by this QEMU binary")); - return -1; - } - - if (disk->discard && - !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_DISCARD)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("discard is not supported by this QEMU binary")); - return -1; - } - - if (disk->detect_zeroes && - !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_DETECT_ZEROES)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("detect_zeroes is not supported by this QEMU binary")); - return -1; - } - - if (disk->iomode && - !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_AIO)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("disk aio mode not supported with this QEMU binary")); - return -1; - } - } - - return 0; -} - - char * qemuBuildDriveStr(virDomainDiskDefPtr disk, virQEMUDriverConfigPtr cfg, @@ -1633,7 +1633,9 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk, virDomainDiskGeometryTransTypeToString(disk->geometry.trans); bool emitDeviceSyntax = qemuDiskBusNeedsDeviceArg(disk->bus); - if (qemuBuildDriveStrValidate(disk, qemuCaps) < 0) + /* if we are using -device this was already chekced elsewhere */ + if (!emitDeviceSyntax && + qemuCheckDiskConfig(disk, qemuCaps) < 0) goto error; if (qemuBuildDriveSourceStr(disk, cfg, &opt, qemuCaps) < 0) @@ -1873,7 +1875,7 @@ qemuBuildDriveDevStr(const virDomainDef *def, char *drivealias; int controllerModel; - if (qemuCheckDiskConfig(disk) < 0) + if (qemuCheckDiskConfig(disk, qemuCaps) < 0) goto error; if (!qemuDomainCheckCCWS390AddressSupport(def, disk->info, qemuCaps, disk->dst)) diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index dd01a42a4..2bcfc6c70 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -177,7 +177,8 @@ int qemuGetDriveSourceString(virStorageSourcePtr src, qemuDomainSecretInfoPtr secinfo, char **source); -int qemuCheckDiskConfig(virDomainDiskDefPtr disk); +int qemuCheckDiskConfig(virDomainDiskDefPtr disk, + virQEMUCapsPtr qemuCaps); bool qemuCheckFips(void); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 6c5ec5f55..545c2dc68 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7936,7 +7936,7 @@ qemuDomainAttachDeviceConfig(virDomainDefPtr vmdef, } if (virStorageTranslateDiskSourcePool(conn, disk) < 0) return -1; - if (qemuCheckDiskConfig(disk) < 0) + if (qemuCheckDiskConfig(disk, NULL) < 0) return -1; if (virDomainDiskInsert(vmdef, disk)) return -1; -- 2.14.3
On Fri, Nov 03, 2017 at 01:03:34PM +0100, Peter Krempa wrote:
Stash all the disk definition and capability checks into one function. --- src/qemu/qemu_command.c | 324 ++++++++++++++++++++++++------------------------ src/qemu/qemu_command.h | 3 +- src/qemu/qemu_driver.c | 2 +- 3 files changed, 166 insertions(+), 163 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index f7e9c0fa4..fa02a3895 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1199,9 +1199,16 @@ qemuCheckDiskConfigBlkdeviotune(virDomainDiskDefPtr disk, }
-/* Perform disk definition config validity checks */ +/** + * qemuCheckDiskConfig: + * @disk: disk definition + * @qemuCaps: qemu capabilities, may be NULL for cold-plug check + * + * Perform disk definition config validity checks. Returns -1 on error with + * error reported */
Please put the ending */ on a separate line. [...]
+ } + /* We can only have 1 FDC bus (currently) */ + if (disk->info.addr.drive.bus != 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Only 1 fdc bus is supported"));
Extra space.
+ return -1; + }
@@ -1633,7 +1633,9 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk, virDomainDiskGeometryTransTypeToString(disk->geometry.trans); bool emitDeviceSyntax = qemuDiskBusNeedsDeviceArg(disk->bus);
- if (qemuBuildDriveStrValidate(disk, qemuCaps) < 0) + /* if we are using -device this was already chekced elsewhere */
s/chekced/checked/ s/was/will be/ The device string is built at a later time than the drive string.
+ if (!emitDeviceSyntax && + qemuCheckDiskConfig(disk, qemuCaps) < 0) goto error;
if (qemuBuildDriveSourceStr(disk, cfg, &opt, qemuCaps) < 0)
Jan
To allow agregating the checks, refactor the code to check capabilities only if they were provided. --- src/qemu/qemu_command.c | 96 +++++++++++++++++++++++++++---------------------- 1 file changed, 53 insertions(+), 43 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index fa02a3895..cfd9ef9e2 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1125,54 +1125,26 @@ qemuDiskConfigBlkdeviotuneHasMaxLength(virDomainDiskDefPtr disk) } +/** + * qemuCheckDiskConfigBlkdeviotune: + * @disk: disk configuration + * @qemuCaps: qemu capabilities, NULL if checking cold-configuration + * + * Checks whether block io tuning settings make sense. Returns -1 on error and + * reports a proper libvirt error. + */ static int qemuCheckDiskConfigBlkdeviotune(virDomainDiskDefPtr disk, virQEMUCapsPtr qemuCaps) { - /* block I/O throttling */ - if (qemuDiskConfigBlkdeviotuneHasBasic(disk) && - !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_IOTUNE)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("block I/O throttling not supported with this " - "QEMU binary")); - return -1; - } - - /* block I/O throttling 1.7 */ - if (qemuDiskConfigBlkdeviotuneHasMax(disk) && - !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_MAX)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("there are some block I/O throttling parameters " - "that are not supported with this QEMU binary")); - return -1; - } - - /* block I/O group 2.4 */ - if (disk->blkdeviotune.group_name) { - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_GROUP)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("the block I/O throttling group parameter is " - "not supported with this QEMU binary")); - return -1; - } - - /* group_name by itself is ignored by qemu */ - if (!qemuDiskConfigBlkdeviotuneHasBasic(disk) && - !qemuDiskConfigBlkdeviotuneHasMax(disk) && - !qemuDiskConfigBlkdeviotuneHasMaxLength(disk)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("group_name can be configured only together with " - "settings")); - return -1; - } - } - - /* block I/O throttling length 2.6 */ - if (qemuDiskConfigBlkdeviotuneHasMaxLength(disk) && - !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_MAX_LENGTH)) { + /* group_name by itself is ignored by qemu */ + if (disk->blkdeviotune.group_name && + !qemuDiskConfigBlkdeviotuneHasBasic(disk) && + !qemuDiskConfigBlkdeviotuneHasMax(disk) && + !qemuDiskConfigBlkdeviotuneHasMaxLength(disk)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("there are some block I/O throttling length parameters " - "that are not supported with this QEMU binary")); + _("group_name can be configured only together with " + "settings")); return -1; } @@ -1195,6 +1167,44 @@ qemuCheckDiskConfigBlkdeviotune(virDomainDiskDefPtr disk, return -1; } + if (qemuCaps) { + /* block I/O throttling */ + if (qemuDiskConfigBlkdeviotuneHasBasic(disk) && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_IOTUNE)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("block I/O throttling not supported with this " + "QEMU binary")); + return -1; + } + + /* block I/O throttling 1.7 */ + if (qemuDiskConfigBlkdeviotuneHasMax(disk) && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_MAX)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("there are some block I/O throttling parameters " + "that are not supported with this QEMU binary")); + return -1; + } + + /* block I/O group 2.4 */ + if (disk->blkdeviotune.group_name && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_GROUP)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("the block I/O throttling group parameter is " + "not supported with this QEMU binary")); + return -1; + } + + /* block I/O throttling length 2.6 */ + if (qemuDiskConfigBlkdeviotuneHasMaxLength(disk) && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_MAX_LENGTH)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("there are some block I/O throttling length parameters " + "that are not supported with this QEMU binary")); + return -1; + } + } + return 0; } -- 2.14.3
On Fri, Nov 03, 2017 at 01:03:35PM +0100, Peter Krempa wrote:
To allow agregating the checks, refactor the code to check capabilities
aggregating
only if they were provided. --- src/qemu/qemu_command.c | 96 +++++++++++++++++++++++++++---------------------- 1 file changed, 53 insertions(+), 43 deletions(-)
Jan
--- src/qemu/qemu_command.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index cfd9ef9e2..507401d3f 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1220,6 +1220,9 @@ int qemuCheckDiskConfig(virDomainDiskDefPtr disk, virQEMUCapsPtr qemuCaps) { + if (qemuCheckDiskConfigBlkdeviotune(disk, qemuCaps) < 0) + return -1; + if (virDiskNameToIndex(disk->dst) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("unsupported disk type '%s'"), disk->dst); @@ -1781,9 +1784,6 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk, virDomainDiskIoTypeToString(disk->iomode)); } - if (qemuCheckDiskConfigBlkdeviotune(disk, qemuCaps) < 0) - goto error; - #define IOTUNE_ADD(_field, _label) \ if (disk->blkdeviotune._field) { \ virBufferAsprintf(&opt, ",throttling." _label "=%llu", \ -- 2.14.3
--- src/qemu/qemu_command.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 507401d3f..1913bbf67 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1428,6 +1428,10 @@ qemuCheckDiskConfig(virDomainDiskDefPtr disk, } } + if (disk->serial && + qemuSafeSerialParamValue(disk->serial) < 0) + return -1; + return 0; } @@ -1713,8 +1717,6 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk, if (disk->serial && virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_SERIAL)) { - if (qemuSafeSerialParamValue(disk->serial) < 0) - goto error; virBufferAddLit(&opt, ",serial="); virBufferEscape(&opt, '\\', " ", "%s", disk->serial); } -- 2.14.3
On Fri, Nov 03, 2017 at 01:03:28PM +0100, Peter Krempa wrote:
Kill some ugly code.
Peter Krempa (9): util: Fix condition check in virDiskNameToIndex qemu: command: Remove dead code when formatting -drive qemu: command: Move disk index validation closer to usage qemu: command: Directly report bus type in qemuBuildDriveStrValidate qemu: command: Refactor qemuBuildDriveStrValidate to make qemuCaps optional qemu: command: Merge checks from qemuBuildDriveStrValidate to qemuCheckDiskConfig qemu: command: Refactor blkiotune checks to tolerate NULL qemuCaps qemu: command: Move blkiotune checks to qemuCheckDiskConfig qemu: command: Move disk serial validation to checker function
src/qemu/qemu_command.c | 485 +++++++++++++++++++++++------------------------- src/qemu/qemu_command.h | 3 +- src/qemu/qemu_driver.c | 2 +- src/util/virutil.c | 2 +- 4 files changed, 236 insertions(+), 256 deletions(-)
ACK series Jan
participants (2)
-
Ján Tomko -
Peter Krempa