[PATCH 0/2] qemu: Match firmware with fully resolved and canonicalized paths
From: James Le Cuirot <jlecuirot@microsoft.com> Distros may provide compatibility symlinks after moving firmware files around, but they won't work for existing VMs when doing a straight string comparison. virFileComparePaths has been used to do this, but it was previously only resolving the last path component, despite what the description says. James Le Cuirot (2): util: Fully resolve paths with virFileComparePaths qemu: Match firmware with fully resolved and canonicalized paths src/qemu/qemu_firmware.c | 11 ++++++----- src/util/virfile.c | 4 ++-- 2 files changed, 8 insertions(+), 7 deletions(-) -- 2.49.0
From: James Le Cuirot <jlecuirot@microsoft.com> The description says it "resolve all symlinks", but it was only resolving the last component. Signed-off-by: James Le Cuirot <jlecuirot@microsoft.com> --- src/util/virfile.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/util/virfile.c b/src/util/virfile.c index 3b7a795..a5c9fbe 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -4108,11 +4108,11 @@ virFileComparePaths(const char *p1, const char *p2) * 'sysfs', since they're no real paths so fallback to plain string * comparison. */ - ignore_value(virFileResolveLink(p1, &res1)); + ignore_value(virFileResolveAllLinks(p1, &res1)); if (!res1) res1 = g_strdup(p1); - ignore_value(virFileResolveLink(p2, &res2)); + ignore_value(virFileResolveAllLinks(p2, &res2)); if (!res2) res2 = g_strdup(p2); -- 2.49.0
From: James Le Cuirot <jlecuirot@microsoft.com> Distros may provide compatibility symlinks after moving firmware files around, but they won't work for existing VMs when doing a straight string comparison. I tried to compare inodes instead, but even glib doesn't provide a straightforward cross-platform method to do this. Resolves: https://bugs.gentoo.org/960591 Signed-off-by: James Le Cuirot <jlecuirot@microsoft.com> --- src/qemu/qemu_firmware.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_firmware.c b/src/qemu/qemu_firmware.c index 2d0ec0b..b13b4f9 100644 --- a/src/qemu/qemu_firmware.c +++ b/src/qemu/qemu_firmware.c @@ -33,6 +33,7 @@ #include "viralloc.h" #include "virenum.h" #include "virstring.h" +#include "virfile.h" #define VIR_FROM_THIS VIR_FROM_QEMU @@ -937,23 +938,23 @@ qemuFirmwareMatchesPaths(const qemuFirmware *fw, switch (fw->mapping.device) { case QEMU_FIRMWARE_DEVICE_FLASH: if (loader && loader->path && - STRNEQ(loader->path, flash->executable.filename)) + !virFileComparePaths(loader->path, flash->executable.filename)) return false; if (loader && loader->nvramTemplate) { if (flash->mode != QEMU_FIRMWARE_FLASH_MODE_SPLIT) return false; - if (STRNEQ(loader->nvramTemplate, flash->nvram_template.filename)) + if (!virFileComparePaths(loader->nvramTemplate, flash->nvram_template.filename)) return false; } break; case QEMU_FIRMWARE_DEVICE_MEMORY: if (loader && loader->path && - STRNEQ(loader->path, memory->filename)) + !virFileComparePaths(loader->path, memory->filename)) return false; break; case QEMU_FIRMWARE_DEVICE_KERNEL: if (kernelPath && - STRNEQ(kernelPath, kernel->filename)) + !virFileComparePaths(kernelPath, kernel->filename)) return false; break; case QEMU_FIRMWARE_DEVICE_NONE: @@ -1657,7 +1658,7 @@ qemuFirmwareFillDomainLegacy(virQEMUDriver *driver, for (i = 0; i < cfg->nfirmwares; i++) { virFirmware *fw = cfg->firmwares[i]; - if (STRNEQ(fw->name, loader->path)) { + if (!virFileComparePaths(fw->name, loader->path)) { VIR_DEBUG("Not matching loader path '%s' for user provided path '%s'", fw->name, loader->path); continue; -- 2.49.0
On 7/24/25 15:49, James Le Cuirot wrote:
From: James Le Cuirot <jlecuirot@microsoft.com>
Distros may provide compatibility symlinks after moving firmware files around, but they won't work for existing VMs when doing a straight string comparison.
virFileComparePaths has been used to do this, but it was previously only resolving the last path component, despite what the description says.
James Le Cuirot (2): util: Fully resolve paths with virFileComparePaths qemu: Match firmware with fully resolved and canonicalized paths
src/qemu/qemu_firmware.c | 11 ++++++----- src/util/virfile.c | 4 ++-- 2 files changed, 8 insertions(+), 7 deletions(-)
-- 2.49.0
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> and merged. Congratulations on your first libvirt contribution! Michal
On Tue, Jul 29, 2025 at 10:42:09 +0200, Michal Prívozník via Devel wrote:
On 7/24/25 15:49, James Le Cuirot wrote:
From: James Le Cuirot <jlecuirot@microsoft.com>
Distros may provide compatibility symlinks after moving firmware files around, but they won't work for existing VMs when doing a straight string comparison.
virFileComparePaths has been used to do this, but it was previously only resolving the last path component, despite what the description says.
James Le Cuirot (2): util: Fully resolve paths with virFileComparePaths qemu: Match firmware with fully resolved and canonicalized paths
src/qemu/qemu_firmware.c | 11 ++++++----- src/util/virfile.c | 4 ++-- 2 files changed, 8 insertions(+), 7 deletions(-)
-- 2.49.0
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
and merged. Congratulations on your first libvirt contribution!
Note that this patch made the tests depend on host. While the pipelines did pass the local build on Fedora caused differences in test output which I attempted to fix, but ultimately that broke the pipelines in turn. Since we're in freeze we agreed to just revert the patches. Next attempt should happen after the freeze. I'll go ahead and post the revert.
On Tue, Jul 29, 2025 at 01:02:18PM +0200, Peter Krempa via Devel wrote:
On Tue, Jul 29, 2025 at 10:42:09 +0200, Michal Prívozník via Devel wrote:
On 7/24/25 15:49, James Le Cuirot wrote:
From: James Le Cuirot <jlecuirot@microsoft.com>
Distros may provide compatibility symlinks after moving firmware files around, but they won't work for existing VMs when doing a straight string comparison.
virFileComparePaths has been used to do this, but it was previously only resolving the last path component, despite what the description says.
James Le Cuirot (2): util: Fully resolve paths with virFileComparePaths qemu: Match firmware with fully resolved and canonicalized paths
src/qemu/qemu_firmware.c | 11 ++++++----- src/util/virfile.c | 4 ++-- 2 files changed, 8 insertions(+), 7 deletions(-)
-- 2.49.0
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
and merged. Congratulations on your first libvirt contribution!
Note that this patch made the tests depend on host. While the pipelines did pass the local build on Fedora caused differences in test output which I attempted to fix, but ultimately that broke the pipelines in turn.
More details is that qemuxmlconftest.c has installed mock content for /usr/share/edk2: tests/qemuxmlconftest.c: virFileWrapperAddPrefix("/sys/devices/system", tests/qemuxmlconftest.c: virFileWrapperAddPrefix(SYSCONFDIR "/qemu/firmware", tests/qemuxmlconftest.c: virFileWrapperAddPrefix(PREFIX "/share/qemu/firmware", tests/qemuxmlconftest.c: virFileWrapperAddPrefix("/home/user/.config/qemu/firmware", tests/qemuxmlconftest.c: virFileWrapperAddPrefix(SYSCONFDIR "/qemu/vhost-user", tests/qemuxmlconftest.c: virFileWrapperAddPrefix(PREFIX "/share/qemu/vhost-user", tests/qemuxmlconftest.c: virFileWrapperAddPrefix("/home/user/.config/qemu/vhost-user", tests/qemuxmlconftest.c: virFileWrapperAddPrefix("/usr/libexec/qemu/vhost-user", tests/qemuxmlconftest.c: virFileWrapperAddPrefix("/usr/libexec/virtiofsd", Somewhere the new file canonicalization, however, it bypassing the rewritten paths, and working off the real /usr/share/edk2 that contains symlinks, which get rewritten. Thus we can the non-determinstic build failures depending on the build environment/OS. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 7/29/25 13:08, Daniel P. Berrangé wrote:
On Tue, Jul 29, 2025 at 01:02:18PM +0200, Peter Krempa via Devel wrote:
On Tue, Jul 29, 2025 at 10:42:09 +0200, Michal Prívozník via Devel wrote:
On 7/24/25 15:49, James Le Cuirot wrote:
From: James Le Cuirot <jlecuirot@microsoft.com>
Distros may provide compatibility symlinks after moving firmware files around, but they won't work for existing VMs when doing a straight string comparison.
virFileComparePaths has been used to do this, but it was previously only resolving the last path component, despite what the description says.
James Le Cuirot (2): util: Fully resolve paths with virFileComparePaths qemu: Match firmware with fully resolved and canonicalized paths
src/qemu/qemu_firmware.c | 11 ++++++----- src/util/virfile.c | 4 ++-- 2 files changed, 8 insertions(+), 7 deletions(-)
-- 2.49.0
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
and merged. Congratulations on your first libvirt contribution!
Note that this patch made the tests depend on host. While the pipelines did pass the local build on Fedora caused differences in test output which I attempted to fix, but ultimately that broke the pipelines in turn.
More details is that qemuxmlconftest.c has installed mock content for /usr/share/edk2:
tests/qemuxmlconftest.c: virFileWrapperAddPrefix("/sys/devices/system", tests/qemuxmlconftest.c: virFileWrapperAddPrefix(SYSCONFDIR "/qemu/firmware", tests/qemuxmlconftest.c: virFileWrapperAddPrefix(PREFIX "/share/qemu/firmware", tests/qemuxmlconftest.c: virFileWrapperAddPrefix("/home/user/.config/qemu/firmware", tests/qemuxmlconftest.c: virFileWrapperAddPrefix(SYSCONFDIR "/qemu/vhost-user", tests/qemuxmlconftest.c: virFileWrapperAddPrefix(PREFIX "/share/qemu/vhost-user", tests/qemuxmlconftest.c: virFileWrapperAddPrefix("/home/user/.config/qemu/vhost-user", tests/qemuxmlconftest.c: virFileWrapperAddPrefix("/usr/libexec/qemu/vhost-user", tests/qemuxmlconftest.c: virFileWrapperAddPrefix("/usr/libexec/virtiofsd",
Somewhere the new file canonicalization, however, it bypassing the rewritten paths, and working off the real /usr/share/edk2 that contains symlinks, which get rewritten. Thus we can the non-determinstic build failures depending on the build environment/OS.
But our vifilewrapper.c doesn't mock realpath() nor virFileCanonicalizePath(). And even if it did we don't store those files in our tree, so there's nowhere for us to redirect realpah()/virFileCanonicalizePath() to. I'll try to come up with a fix. But given how close we are to the release, let me revert these for the time being and repost them with proper test suite fixes for merge after the release. Michal
participants (4)
-
Daniel P. Berrangé -
James Le Cuirot -
Michal Prívozník -
Peter Krempa