Resolve symlinks in config.FindHelperBinary

[n8henrie]

Change FindHelperBinary to resolve symlinks if possible, if error then fallback
to current behavior.

Fixes containers/podman#17026
Fixes #1302

See conversation at:

• Resolve symlink path for qemu directory if possible podman#17027
• nixpkgs podman and qemu on darwin don't play well together NixOS/nixpkgs#169118
• podman: includes missing edk2-aarch64-code.fd on aarch64-darwin NixOS/nixpkgs#163015

[vrothberg]

Thanks for contributing! The changes look good to me.

In order to pass CI, you need to sign your commit via git commit -s.

@rhatdan PTAL

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: n8henrie, vrothberg

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [vrothberg]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[n8henrie]

No problem, signed.

Please note, I've expressed some concerns at NixOS/nixpkgs#163015 that this will change the current behavior (which is kind of the point I guess) in some cases. It looks like it should be helpful for the nix issues I've linked to (the very simple test I used locally is below), but I do worry that there will be some edge cases in which things break for some users (which is why I made the original PR to podman specifically for the case of finding qemu-system-aarch64).

Maybe I'm just being overcautious here, having dealt with the consequences of eager symlink resolution, as I certainly don't know of any specific problems this is likely to cause. ¯\_(ツ)_/¯

package config

import (
	"strings"
	"testing"

	"github.com/containers/common/pkg/config"
)

func TestGetDivisorSum(t *testing.T) {
	cfg, err := config.Default()
	if err != nil {
		panic(err)
	}
	execPath, err := cfg.FindHelperBinary("qemu-system-aarch64", true)
	if err != nil {
		panic(err)
	}
	if strings.HasPrefix(execPath, "/run/current-system/sw/bin") {
		t.Error("Didn't resolve the symlink")
	}
}

[n8henrie]

Other potential paths to make this less surprising would obviously be an API change to add a resolveSymlinks parameter, or to add a separate function FindHelperBinaryResolvingSymlinks (which sounds very Swift to me).

[Luap99]

I don't understand why this function should resolve the symlinks at all. I don't see how any caller will benefit from that.
Looking at containers/podman#17027 I would argue that getEdk2CodeFdPathFromQemuBinaryPath() is just broken by assuming a hardcoded path without any way to customize it. It just assumes that the path is always relative (../share/qemu ) to the qemu binary. I would argue that this is wrong and resolving any symlink will not fix the underlying issue of using hardcoded paths.

I much rather fix this in podman for this one function. Anyway why does it use full paths in the first place. I thought qemu will already find the correct path based on the file, the search paths are defined in /usr/share/qemu/firmware (at least on linux).
So my proposal would be to test it with only the file name in the qemu command line. I don't own a mac so I cannot test if it works there but I can test later if this works on linux. That would just completely remove this problem from podman and the qemu installation would need to provide the correct paths instead.

[rhatdan]

@n8henrie Please respond to @Luap99

[n8henrie]

Sorry, missed the prior comment.

@Luap99:

I don't see how any caller will benefit from that.

Well, in this case, podman users on aarch64-darwin users would benefit it they have qemu installed via nix instead of e.g. homebrew.

I would argue that getEdk2CodeFdPathFromQemuBinaryPath() is just broken by assuming a hardcoded path without any way to customize it. ... I much rather fix this in podman for this one function.

I made my initial PR there, I think that's where the fix belongs. @rhatdan disagrees -- perhaps a discussion between you two would help move the ball down the field?

So my proposal would be to test it with only the file name in the qemu command line.

I'm sorry, I don't understand what you're suggesting, but if you can explain I'd be happy to test.

[Luap99]

I'm sorry, I don't understand what you're suggesting, but if you can explain I'd be happy to test.

Remove the full path to the edk2 file from the qemu commands line. Just use the file name, qemu should already know where to look for it. So just remove the getEdk2CodeFd() function and test if it works with your qemu install.

[n8henrie]

Remove the full path to the edk2 file from the qemu commands line.

I still don't know what you mean, sorry. I am new to podman, trying to move away from docker for some cross-compilation with cross-rs, and this has been my introductory experience:

$ type -a podman
podman is /run/current-system/sw/bin/podman
$ podman --version
podman version 4.3.1
$ type -a qemu-system-aarch64
qemu-system-aarch64 is /run/current-system/sw/bin/qemu-system-aarch64
$ realpath $(command -v qemu-system-aarch64)
/nix/store/2ww0lrgz8ixscijqs0n51hhb0yxpj76z-qemu-7.1.0/bin/qemu-system-aarch64
$ ls /nix/store/2ww0lrgz8ixscijqs0n51hhb0yxpj76z-qemu-7.1.0/share/qemu/edk2-aarch64-code.fd
/nix/store/2ww0lrgz8ixscijqs0n51hhb0yxpj76z-qemu-7.1.0/share/qemu/edk2-aarch64-code.fd
$ podman machine init
Downloading VM image: fedora-coreos-37.20230122.2.0-qemu.aarch64.qcow2.xz: done
Extracting compressed file
Image resized.
Machine init complete
To start your machine run:

        podman machine start
$ podman machine start
Starting machine "podman-machine-default"
Waiting for VM ...
Error: qemu exited unexpectedly with exit code 1, stderr: qemu-system-aarch64: -drive file=edk2-aarch64-code.fd,if=pflash,format=raw,readonly=on: Could not open 'edk2-aarch64-code.fd': No such file or directory

Remove the full path to the edk2 file from the qemu commands line

I'm not passing anything (directly) to qemu.

[n8henrie]

I attempted to follow your instructions, no difference.

I changed the line from:

"-drive", "file=" + getEdk2CodeFd("edk2-aarch64-code.fd") + ",if=pflash,format=raw,readonly=on",  

to:

"-drive", "file=edk2-aarch64-code.fd" + ",if=pflash,format=raw,readonly=on",

and commented out the getEdk2CodeFd function completely.

Compiled a new podman binary:

$ /Users/n8henrie/git/podman/bin/darwin/podman machine start
Starting machine "podman-machine-default"
Waiting for VM ...
Error: qemu exited unexpectedly with exit code 1, stderr: qemu-system-aarch64: -drive file=edk2-aarch64-code.fd,if=pflash,format=raw,readonly=on: Could not open 'edk2-aarch64-code.fd': No such file or directory

[n8henrie]

In contrast, with my patch (applied to podman):

$ /Users/n8henrie/git/podman/bin/darwin/podman machine init && /Users/n8henrie/git/podman/bin/darwin/podman machine start
Extracting compressed file
Image resized.
Machine init complete
To start your machine run:

        podman machine start

Starting machine "podman-machine-default"
Waiting for VM ...
Mounting volume... /Users:/Users
Mounting volume... /private:/private
Mounting volume... /var/folders:/var/folders

This machine is currently configured in rootless mode. If your containers
require root permissions (e.g. ports < 1024), or if you run into compatibility
issues with non-podman clients, you can switch using the following command:

        podman machine set --rootful

API forwarding listening on: /Users/n8henrie/.local/share/containers/podman/machine/podman-machine-default/podman.sock

The system helper service is not installed; the default Docker API socket
address can't be used by podman. If you would like to install it run the
following commands:

        sudo /Users/n8henrie/git/podman/bin/darwin/podman-mac-helper install
        podman machine stop; podman machine start

You can still connect Docker API clients by setting DOCKER_HOST using the
following command in your terminal session:

        export DOCKER_HOST='unix:///Users/n8henrie/.local/share/containers/podman/machine/podman-machine-default/podman.sock'

Machine "podman-machine-default" started successfully

[Luap99]

Yeah I guess qemu always uses the current dir in this case. I don't understand why it would not lookup that in /usr/share/qemu/firmware/ because this already has all the necessary information. For some reason it expects the caller to always specify the full path which means we need to hardcore it somehow.

I personally prefer your podman PR to fix this. It is the only caller that needs this change so I do not see why all other should resolve the symlink first when the kernel will do this for us.

[arixmkii]

I personally prefer your podman PR to fix this.

Would say the same. Not a member of Podman team, but recently worked on a patch for FindHelperBinary and to me it looks like allowing it to resolve symlinks would change the behavior (making it very different from how LookPath and similar family of methods work).

Also, there is no problem having a binary with symlink. The problem is to resolve the path relative to the binary and it doesn't look like a responsibility of the FindHelperBinary function. In some situation the caller might really want the symlink, so, that actual resolution is postponed to the call time and if it always resolves there will be no such option, but if the old behavior is kept, then that logic could reside on the caller side.

As for PR in Podman. May be it is worth to allow additional QEMU share path target using env var. Because adjusting it for other possible package managers will require a new release.

Also. what if there is actually 2 different shares - one relative to symlink and one relative to resolved binary path. Which one should win?

[n8henrie]

In some situation the caller might really want the symlink

I agree (see also: the Quicksilver issue I linked above). I've bumped the issue at podman and it looks like with this context @rhatdan is amenable, so I'll close and fingers crossed we can come up with a solution there.

Thanks to all for your time and feedback!