default: look up init binary in helper_binaries_dir #1115
Conversation
Some Linux distributions does not install files into `/usr/libexec` and would fail to lookup the init binary. This change first checks if the file exists in the default location, else we look for the binary in helper_binaries_dir as they should encompass all the possible installation directories. Fixes containers#1110 Signed-off-by: Morten Linderud <morten@linderud.pw>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Foxboron The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| if _, err := os.Stat(DefaultInitPath); err == nil { | ||
| return DefaultInitPath | ||
| } | ||
| for _, dir := range c.HelperBinariesDir { |
There was a problem hiding this comment.
Can we use FindHelperBinary() instead?
There was a problem hiding this comment.
Oh, missed that function. Yes we can use it :)
There was a problem hiding this comment.
Okay, this is a bit complicated without some slight refactoring I think.
We pick the init binary in EngineConfig, but there is no Config struct in scope for us to access FindHelperBinary. We could refactor FindHelperBinary to be below EngineConfig, as we only have two uses of them to find the network binaries, and they have access to both structs.
Else we keep the code as-is. Opinions?
Is there something I don't see here?
There was a problem hiding this comment.
We can take a step back.
Why not create a public function InitPath() (string, error) that can be called by Podman? The logic would be similar to the current function but all data should be accessible at that point.
There was a problem hiding this comment.
It would need to be InitPath(conf *config.Config) (string, error) right? You still want it to use the FindHelperBinary code from what I can tell.
There was a problem hiding this comment.
@containers/podman-maintainers PTAL
There was a problem hiding this comment.
@vrothberg the PR does not look like it has your suggested change?
There was a problem hiding this comment.
Yes, needs change but I was looking for other opinions
There was a problem hiding this comment.
I haven't gotten around to taking a stab at the last suggestions sadly :/ I haven't forgotten about it, just need to find a little bit of time.
|
@Foxboron @vrothberg What do you want to do with this one? |
|
I'm not quite sure if this is the right idea, this pr seems like it would make it difficult for podman to use other container init |
|
Closing due to lack of followup. |
Some Linux distributions does not install files into
/usr/libexecandwould fail to lookup the init binary.
This change first checks if the file exists in the default location,
else we look for the binary in helper_binaries_dir as they should
encompass all the possible installation directories.
Fixes #1110
Signed-off-by: Morten Linderud morten@linderud.pw