[PATCH 0/4] Fix regression in 'startupPolicy' validation for block disks
Patch 4/4 fixes the regression. Patch 3/4 fixes missing cases in the validation. Rest of the series is preparation/cleanup. Peter Krempa (4): virDomainDiskDefValidate: Improve error messages for 'startupPolicy' checks domain_validate: Split out validation of disk startup policy virDomainDiskDefValidateStartupPolicy: Validate disk type better virDomainDiskTranslateSourcePool: Fix check of 'startupPolicy' definition src/conf/domain_conf.c | 12 +++++----- src/conf/domain_validate.c | 49 ++++++++++++++++++++++++-------------- src/conf/domain_validate.h | 2 ++ src/libvirt_private.syms | 1 + 4 files changed, 40 insertions(+), 24 deletions(-) -- 2.36.1
Remove linebreak and mention the attribute name. Also prepare the error messages for future by substituting the type of offending access. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_validate.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c index 1e18826bc1..3e11e00ee4 100644 --- a/src/conf/domain_validate.c +++ b/src/conf/domain_validate.c @@ -855,9 +855,9 @@ virDomainDiskDefValidate(const virDomainDef *def, if (disk->startupPolicy != VIR_DOMAIN_STARTUP_POLICY_DEFAULT) { if (disk->src->type == VIR_STORAGE_TYPE_NETWORK) { virReportError(VIR_ERR_XML_ERROR, - _("Setting disk %s is not allowed for " - "disk of network type"), - virDomainStartupPolicyTypeToString(disk->startupPolicy)); + _("disk startupPolicy '%s' is not allowed for disk of '%s' type"), + virDomainStartupPolicyTypeToString(disk->startupPolicy), + virStorageTypeToString(disk->src->type)); return -1; } @@ -865,8 +865,7 @@ virDomainDiskDefValidate(const virDomainDef *def, disk->device != VIR_DOMAIN_DISK_DEVICE_FLOPPY && disk->startupPolicy == VIR_DOMAIN_STARTUP_POLICY_REQUISITE) { virReportError(VIR_ERR_XML_ERROR, "%s", - _("Setting disk 'requisite' is allowed only for " - "cdrom or floppy")); + _("disk startupPolicy 'requisite' is allowed only for cdrom or floppy")); return -1; } } -- 2.36.1
Move the code into 'virDomainDiskDefValidateStartupPolicy' which will be later reused in the qemu driver. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_validate.c | 45 ++++++++++++++++++++++++-------------- src/conf/domain_validate.h | 2 ++ src/libvirt_private.syms | 1 + 3 files changed, 31 insertions(+), 17 deletions(-) diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c index 3e11e00ee4..70e9167eac 100644 --- a/src/conf/domain_validate.c +++ b/src/conf/domain_validate.c @@ -632,6 +632,32 @@ virDomainDiskDefSourceLUNValidate(const virStorageSource *src) } +int +virDomainDiskDefValidateStartupPolicy(const virDomainDiskDef *disk) +{ + if (disk->startupPolicy == VIR_DOMAIN_STARTUP_POLICY_DEFAULT) + return 0; + + if (disk->src->type == VIR_STORAGE_TYPE_NETWORK) { + virReportError(VIR_ERR_XML_ERROR, + _("disk startupPolicy '%s' is not allowed for disk of '%s' type"), + virDomainStartupPolicyTypeToString(disk->startupPolicy), + virStorageTypeToString(disk->src->type)); + return -1; + } + + if (disk->device != VIR_DOMAIN_DISK_DEVICE_CDROM && + disk->device != VIR_DOMAIN_DISK_DEVICE_FLOPPY && + disk->startupPolicy == VIR_DOMAIN_STARTUP_POLICY_REQUISITE) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("disk startupPolicy 'requisite' is allowed only for cdrom or floppy")); + return -1; + } + + return 0; +} + + static int virDomainDiskDefValidate(const virDomainDef *def, const virDomainDiskDef *disk) @@ -852,23 +878,8 @@ virDomainDiskDefValidate(const virDomainDef *def, return -1; } - if (disk->startupPolicy != VIR_DOMAIN_STARTUP_POLICY_DEFAULT) { - if (disk->src->type == VIR_STORAGE_TYPE_NETWORK) { - virReportError(VIR_ERR_XML_ERROR, - _("disk startupPolicy '%s' is not allowed for disk of '%s' type"), - virDomainStartupPolicyTypeToString(disk->startupPolicy), - virStorageTypeToString(disk->src->type)); - return -1; - } - - if (disk->device != VIR_DOMAIN_DISK_DEVICE_CDROM && - disk->device != VIR_DOMAIN_DISK_DEVICE_FLOPPY && - disk->startupPolicy == VIR_DOMAIN_STARTUP_POLICY_REQUISITE) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("disk startupPolicy 'requisite' is allowed only for cdrom or floppy")); - return -1; - } - } + if (virDomainDiskDefValidateStartupPolicy(disk) < 0) + return -1; if (disk->wwn && !virValidateWWN(disk->wwn)) return -1; diff --git a/src/conf/domain_validate.h b/src/conf/domain_validate.h index 430d61fd3c..07b99195e3 100644 --- a/src/conf/domain_validate.h +++ b/src/conf/domain_validate.h @@ -41,4 +41,6 @@ int virDomainDeviceDefValidate(const virDomainDeviceDef *dev, int virDomainDiskDefValidateSource(const virStorageSource *src); +int virDomainDiskDefValidateStartupPolicy(const virDomainDiskDef *disk); + int virDomainDiskDefSourceLUNValidate(const virStorageSource *src); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 770dfe459a..76bcc64eb0 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -779,6 +779,7 @@ virDomainActualNetDefValidate; virDomainDefValidate; virDomainDeviceValidateAliasForHotplug; virDomainDiskDefSourceLUNValidate; +virDomainDiskDefValidateStartupPolicy; # conf/interface_conf.h -- 2.36.1
Our startup policy checkers work only for local paths, so disk sources such as NVMe, or vhost-user can't be used with startup policy. Unfortunately the validation did not catch these cases. Fix it. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_validate.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c index 70e9167eac..9c55d05cac 100644 --- a/src/conf/domain_validate.c +++ b/src/conf/domain_validate.c @@ -638,7 +638,10 @@ virDomainDiskDefValidateStartupPolicy(const virDomainDiskDef *disk) if (disk->startupPolicy == VIR_DOMAIN_STARTUP_POLICY_DEFAULT) return 0; - if (disk->src->type == VIR_STORAGE_TYPE_NETWORK) { + /* We want to allow any startup policy for un-translated _TYPE_VOLUME disks. + * virStorageSourceGetActualType returns _TYPE_VOLUME in such case */ + if (virStorageSourceGetActualType(disk->src) != VIR_STORAGE_TYPE_VOLUME && + !virStorageSourceIsLocalStorage(disk->src)) { virReportError(VIR_ERR_XML_ERROR, _("disk startupPolicy '%s' is not allowed for disk of '%s' type"), virDomainStartupPolicyTypeToString(disk->startupPolicy), -- 2.36.1
The check was historically done only for _TYPE_VOLUME disks, but refactors to allow _TYPE_VOLUME disks in the backing chain caused a regression where we'd reject startupPolicy also for _TYPE_BLOCK disks which historically worked well. Fix it by using the 'virDomainDiskDefValidateStartupPolicy' helper and use it only when the top level image is a _TYPE_VOLUME as in other cases it was already validated. This also allows _TYPE_BLOCK volumes to use startup policy. Fixes: 37f01262eed9f37dd5eb7de8b83edd2fea741054 Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2095758 Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 761c3f4d87..d5c5a10a8c 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -31433,13 +31433,13 @@ virDomainDiskTranslateSourcePool(virDomainDiskDef *def) if (virDomainStorageSourceTranslateSourcePool(n, conn) < 0) return -1; - } - if (def->startupPolicy != 0 && - virStorageSourceGetActualType(def->src) != VIR_STORAGE_TYPE_FILE) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("'startupPolicy' is only valid for 'file' type volume")); - return -1; + /* The validity of 'startupPolicy' setting is checked only for the top + * level image. For any other subsequent images we honour it only if + * possible */ + if (n == def->src && + virDomainDiskDefValidateStartupPolicy(def) < 0) + return -1; } return 0; -- 2.36.1
On a Tuesday in June Peter Krempa <pkrempa@redhat.com> wrote:
Patch 4/4 fixes the regression. Patch 3/4 fixes missing cases in the validation. Rest of the series is preparation/cleanup.
Peter Krempa (4): virDomainDiskDefValidate: Improve error messages for 'startupPolicy' checks domain_validate: Split out validation of disk startup policy virDomainDiskDefValidateStartupPolicy: Validate disk type better virDomainDiskTranslateSourcePool: Fix check of 'startupPolicy' definition
src/conf/domain_conf.c | 12 +++++----- src/conf/domain_validate.c | 49 ++++++++++++++++++++++++-------------- src/conf/domain_validate.h | 2 ++ src/libvirt_private.syms | 1 + 4 files changed, 40 insertions(+), 24 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com>
-- 2.36.1
participants (2)
-
Jano Tomko -
Peter Krempa