> Huh, this is a 4 year old commit, are you sure it's a regression at that
> point?
> Looking a bit further I've found my more recent commit d57630c282a which adds
> the terminator.
You're right, sorry, bca731d0f5 only added the validation. The actual change
that
makes src->backingStore non-NULL for pflash/nvram is your later commit
d57630c282a ("qemu: Install backing store terminators for 'pflash' blockdevs",
2024-11-04). I'll fix the attribution in v2.
> IMO such an extensive commit is not needed. All code handling virStorageSource
> needs to do the same. I'll likely drop it before pushing.
Agreed, the comment is more general guidance than something specific to this
hunk.
Dropped the inline comment in v2.
>From 1327b8ce4844fe91a6a62cc6702551c52e6813e7 Mon Sep 17 00:00:00 2001
From: Fima Shevrin <[email protected]>
Date: Sun, 3 May 2026 22:22:44 +0300
Subject: [PATCH v2] qemu: validate: treat only real backing chains as NVRAM
backingStore
qemuDomainInitializePflashStorageSource() always attaches a non-NULL
src->backingStore used as an empty virStorageSource chain terminator
(type VIR_STORAGE_TYPE_NONE). qemuValidateDomainDefNvram() incorrectly
interpreted every non-NULL backingStore as a genuine backing overlay and
reported VIR_ERR_CONFIG_UNSUPPORTED, so legitimate UEFI/NVRAM setups were
rejected.
Check virStorageSourceIsBacking(src->backingStore) instead of a plain
pointer test so only a real backing node is rejected.
Fixes: d57630c282ae7220d8998f74b7e4fec21841797f
Signed-off-by: Fima Shevrin <[email protected]>
---
v2:
- dropped the inline comment per review
- fixed attribution: Fixes: d57630c282a instead of bca731d0f5
src/qemu/qemu_validate.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
index 642244b62e..2b5a2535c5 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
@@ -740,7 +740,7 @@ qemuValidateDomainDefNvram(const virDomainDef *def,
return -1;
}
- if (src->backingStore) {
+ if (virStorageSourceIsBacking(src->backingStore)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("backingStore is not supported with NVRAM"));
return -1;
--
2.47.1
________________________________
From: Peter Krempa <[email protected]>
Sent: Wednesday, May 27, 2026 15:44
To: Efim Shevrin <[email protected]>
Cc: [email protected] <[email protected]>; Den Lunev
<[email protected]>
Subject: Re: [PATCH] qemu: validate: treat only real backing chains as NVRAM
backingStore
On Fri, May 22, 2026 at 15:49:27 +0000, Efim Shevrin via Devel wrote:
> qemuDomainInitializePflashStorageSource() always attaches a non-NULL
> src->backingStore used as an empty virStorageSource chain terminator
> (type VIR_STORAGE_TYPE_NONE). qemuValidateDomainDefNvram() incorrectly
> interpreted every non-NULL backingStore as a genuine backing overlay and
> reported VIR_ERR_CONFIG_UNSUPPORTED, so legitimate UEFI/NVRAM setups were
> rejected.
> Check virStorageSourceIsBacking(src->backingStore) instead of a plain
> pointer test so only a real backing node is rejected.
>
> Regression introduced in commit bca731d0f562f0842f56ec2206fdbd721a468f5b.
Huh, this is a 4 year old commit, are you sure it's a regression at that
point?
Looking a bit further I've found my more recent commit d57630c282a which
adds the terminator.
> Signed-off-by: Fima Shevrin <[email protected]>
> ---
> src/qemu/qemu_validate.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
> index 642244b62e..d8347d1964 100644
> --- a/src/qemu/qemu_validate.c
> +++ b/src/qemu/qemu_validate.c
> @@ -740,7 +740,14 @@ qemuValidateDomainDefNvram(const virDomainDef *def,
> return -1;
> }
>
> - if (src->backingStore) {
> + /* qemuDomainInitializePflashStorageSource() always sets
> + * src->backingStore to a fresh empty virStorageSource as a
> + * chain terminator, so a plain `if (src->backingStore)` check
> + * is always true and rejects every UEFI/NVRAM domain after
> + * upstream commit that introduced the terminator. Use
> + * virStorageSourceIsBacking() (type != VIR_STORAGE_TYPE_NONE)
> + * so we only reject genuine backing chains. */
IMO such an extensive commit is not needed. All code handling
virStorageSource needs to do the same.
I'll likely drop it before pushing.
> + if (virStorageSourceIsBacking(src->backingStore)) {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> _("backingStore is not supported with NVRAM"));
> return -1;
Reviewed-by: Peter Krempa <[email protected]>
on the code. I'll hold off for a bit to discuss the commit message.
