[PATCH] 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. Regression introduced in commit bca731d0f562f0842f56ec2206fdbd721a468f5b. Signed-off-by: Fima Shevrin <efim.shevrin@virtuozzo.com> --- 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. */ + if (virStorageSourceIsBacking(src->backingStore)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("backingStore is not supported with NVRAM")); return -1; -- 2.47.1
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 <efim.shevrin@virtuozzo.com> --- 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 <pkrempa@redhat.com> on the code. I'll hold off for a bit to discuss the commit message.
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 <efim.shevrin@virtuozzo.com> 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 <efim.shevrin@virtuozzo.com> --- 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 <pkrempa@redhat.com> Sent: Wednesday, May 27, 2026 15:44 To: Efim Shevrin <efim.shevrin@virtuozzo.com> Cc: devel@lists.libvirt.org <devel@lists.libvirt.org>; Den Lunev <den@virtuozzo.com> 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 <efim.shevrin@virtuozzo.com> --- 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 <pkrempa@redhat.com> on the code. I'll hold off for a bit to discuss the commit message.
On Fri, Jun 05, 2026 at 10:14:49 +0000, Efim Shevrin wrote:
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 <efim.shevrin@virtuozzo.com> 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 <efim.shevrin@virtuozzo.com> --- 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)) {
I have 'if (virStorageSourceHasBacking(src)) {' in my private branch where I've fixed v1 of this patch ...
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("backingStore is not supported with NVRAM")); return -1; -- 2.47.1
Looks good, but the patch is un-applicable because you've pasted it into your mail client. Please use 'git publish' or 'git send-email' per our docs to send patches so that they are not chewed up the next time. I'll apply it manually this time, based on the v1 I already have and I already fixed so that I don't have to do it again. Reviewed-by: Peter Krempa <pkrempa@redhat.com>
participants (2)
-
Efim Shevrin -
Peter Krempa