[v1] qemu: Match firmware with fully resolved and canonicalized paths

[PATCH 0/2] qemu: Match firmware with fully resolved and canonicalized paths

Posted by James Le Cuirot 1 month, 2 weeks ago

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

Re: [PATCH 0/2] qemu: Match firmware with fully resolved and canonicalized paths

Posted by Michal Prívozník via Devel 1 month, 1 week ago

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

Re: [PATCH 0/2] qemu: Match firmware with fully resolved and canonicalized paths

Posted by Peter Krempa via Devel 1 month, 1 week ago

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.

Re: [PATCH 0/2] qemu: Match firmware with fully resolved and canonicalized paths

Posted by Daniel P. Berrangé via Devel 1 month, 1 week ago

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 :|

Re: [PATCH 0/2] qemu: Match firmware with fully resolved and canonicalized paths

Posted by Michal Prívozník via Devel 1 month, 1 week ago

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