utils: improve error message if path not executable #1672

giuseppe · 2025-02-14T10:54:24Z

improve the error returned when the specified path is not executable.

Return the reason why the lookup failed when the path is not a regular file, or has no executable bit set.

Closes: #1671

packit-as-a-service · 2025-02-14T11:03:09Z

podman system tests failed. @containers/packit-build please check.

src/libcrun/utils.c

Luap99 · 2025-02-14T11:16:39Z

src/libcrun/utils.c

+        {
+          *out = xstrdup (executable_path);
+          return 0;
+        }


There seems to be no return here anymore which means even in the absolute path you fall back to looking up PATH. That seems wrong? should this set last_error = ret and then have en else branch for the PATH lookup?

src/libcrun/utils.c

Luap99

2025-02-14T12:04:23.5782745Z Summarizing 8 Failures:
2025-02-14T12:04:23.5783188Z   [FAIL] Podman exec [It] podman exec cannot be invoked
2025-02-14T12:04:23.5783867Z   /root/go/src/github.com/containers/podman/test/e2e/exec_test.go:436
2025-02-14T12:04:23.5784630Z   [FAIL] Podman run [It] podman run --rm failed container should delete itself
2025-02-14T12:04:23.5785117Z   /root/go/src/github.com/containers/podman/test/e2e/run_test.go:1453
2025-02-14T12:04:23.5785606Z   [FAIL] Podman run [It] podman run failed container should NOT delete itself
2025-02-14T12:04:23.5786107Z   /root/go/src/github.com/containers/podman/test/e2e/run_test.go:1465
2025-02-14T12:04:23.5786731Z   [FAIL] Podman run exit [It] podman run exit ExecErrorCodeCannotInvoke
2025-02-14T12:04:23.5787542Z   /root/go/src/github.com/containers/podman/test/e2e/run_exit_test.go:25
2025-02-14T12:04:23.5789852Z   [FAIL] Podman run exit [It] podman run exit ExecErrorCodeNotFound
2025-02-14T12:04:23.5790572Z   /root/go/src/github.com/containers/podman/test/e2e/run_exit_test.go:31
2025-02-14T12:04:23.5791262Z   [FAIL] Podman start [It] podman start --rm removed on failure
2025-02-14T12:04:23.5791928Z   /root/go/src/github.com/containers/podman/test/e2e/start_test.go:41
2025-02-14T12:04:23.5792629Z   [FAIL] Podman start [It] podman start --rm --attach removed on failure
2025-02-14T12:04:23.5793331Z   /root/go/src/github.com/containers/podman/test/e2e/start_test.go:54
2025-02-14T12:04:23.5794023Z   [FAIL] Podman run entrypoint [It] podman run no command, entrypoint, or cmd
2025-02-14T12:04:23.5794747Z   /root/go/src/github.com/containers/podman/test/e2e/run_entrypoint_test.go:21

That is not blocker of course but something we should fix ahead of time in podman I think, though one thing I noticed in "podman run failed container should NOT delete itself"

$ bin/podman --runtime /home/pholzing/CODE/crun/crun run --rm quay.io/libpod/testimage:20241011 blah
Error: /home/pholzing/CODE/crun/crun: cannot find executable `blah`: No such file or directory: OCI runtime attempted to invoke a command that was not found

# with current crun
$ bin/podman --runtime crun run --rm quay.io/libpod/testimage:20241011 blah
Error: crun: executable file `blah` not found in $PATH: No such file or directory: OCI runtime attempted to invoke a command that was not found

The not found in $PATH is missing now and I think that has some value to know that crun checks PATH so maybe consider keeping it?
Aside from that LGTM, I agree that the new message is overall better

src/libcrun/utils.c

eriksjolund · 2025-02-14T15:45:18Z

$ podman --runtime  ~/cruninstall/bin/crun   run --rm docker.io/library/alpine blah/blah
Error: /var/home/core/cruninstall/bin/crun: executable file `//blah/blah` not found: No such file or directory: OCI runtime attempted to invoke a command that was not found

not important but a small improvement would be changing // to /

update

or show the error in runc style:

$ podman --runtime runc  run --rm docker.io/library/alpine blah/blah
Error: runc: runc create failed: unable to start container process: error during container init: exec: "blah/blah": stat blah/blah: no such file or directory: OCI runtime attempted to invoke a command that was not found

giuseppe · 2025-02-14T16:02:10Z

$ podman --runtime  ~/cruninstall/bin/crun   run --rm docker.io/library/alpine blah/blah
Error: /var/home/core/cruninstall/bin/crun: executable file `//blah/blah` not found: No such file or directory: OCI runtime attempted to invoke a command that was not found

not important but a small improvement would be changing // to /

what do you think of the new version?

eriksjolund · 2025-02-14T16:06:17Z

what do you think of the new version?

$ podman --runtime  ~/cruninstall/bin/crun run --rm docker.io/library/alpine blah/blah
Error: /var/home/core/cruninstall/bin/crun: executable file `/blah/blah` not found: No such file or directory: OCI runtime attempted to invoke a command that was not found

lgtm

giuseppe · 2025-02-14T19:57:35Z

how do we move forward with the Podman tests?

I think we should just drop that checks, there is no guarantee about the error messages and makes testing of different versions more difficult than it should be.

Luap99 · 2025-02-14T20:09:56Z

I think we should just drop that checks, there is no guarantee about the error messages and makes testing of different versions more difficult than it should be.

We should not drop error message checks, the reason we must do that is because people keep adding tests for errors that caused a totally different error and if you don't check the message you will never find out.
containers/podman#18188

The way to move forward is to change the string we match, if there is a partial overlap between the old and new error we can use that (assuming it is still useful and not just "error")
If there is no way to do a meaningful match we need to match both new or old message, given these are only a few test cases I don't think it is a big deal. If you don't want to deal with that part I can do that on Monday.

giuseppe · 2025-02-14T20:18:42Z

We should not drop error message checks, the reason we must do that is because people keep adding tests for errors that caused a totally different error and if you don't check the message you will never find out. containers/podman#18188

The way to move forward is to change the string we match, if there is a partial overlap between the old and new error we can use that (assuming it is still useful and not just "error") If there is no way to do a meaningful match we need to match both new or old message, given these are only a few test cases I don't think it is a big deal. If you don't want to deal with that part I can do that on Monday.

sorry for bothering you so late on a Friday night.

I know the rationale behind these checks, but I still disagree with that because it is not possible to test with different OCI runtimes.

In any case, I'll take care of these incompatibilities, since I am introducing them :-)

Needed-by: containers/crun#1672 Signed-off-by: Giuseppe Scrivano <[email protected]>

giuseppe · 2025-02-17T08:41:34Z

@Luap99 opened a PR for Podman: containers/podman#25340

Needed-by: containers/crun#1672 Signed-off-by: Giuseppe Scrivano <[email protected]>

unify the error codes returned by runc and crun. Fix the tests to work with both runtimes, as well as the containers/crun#1672 changes in progress for crun. Follow-up for containers#25340 Signed-off-by: Giuseppe Scrivano <[email protected]>

Signed-off-by: Giuseppe Scrivano <[email protected]>

if a handler is specified, do not return an early error if the executable path could not be found. Signed-off-by: Giuseppe Scrivano <[email protected]>

Signed-off-by: Giuseppe Scrivano <[email protected]>

improve the error returned when the specified path is not executable. Return the reason why the lookup failed when the path is not a regular file, or has no executable bit set. Closes: containers#1671 Signed-off-by: Giuseppe Scrivano <[email protected]>

unify the error codes returned by runc and crun. Fix the tests to work with both runtimes, as well as the containers/crun#1672 changes in progress for crun. Follow-up for containers#25340 Signed-off-by: Giuseppe Scrivano <[email protected]>

Luap99

LGTM

giuseppe mentioned this pull request Feb 14, 2025

Pod containers are instantly terminated without errors #1671

Closed

Luap99 reviewed Feb 14, 2025

View reviewed changes

giuseppe force-pushed the improve-error-message-path-not-executable branch from 39abc01 to 8f3e370 Compare February 14, 2025 11:43

Luap99 reviewed Feb 14, 2025

View reviewed changes

giuseppe force-pushed the improve-error-message-path-not-executable branch 2 times, most recently from 072dba0 to 3bf019e Compare February 14, 2025 12:54

Luap99 reviewed Feb 14, 2025

View reviewed changes

src/libcrun/utils.c Outdated Show resolved Hide resolved

giuseppe force-pushed the improve-error-message-path-not-executable branch from 3bf019e to 2cd3915 Compare February 14, 2025 15:34

giuseppe force-pushed the improve-error-message-path-not-executable branch from 2cd3915 to ec6e413 Compare February 14, 2025 16:01

giuseppe force-pushed the improve-error-message-path-not-executable branch 2 times, most recently from 0dfca41 to 9ada14f Compare February 14, 2025 16:46

giuseppe force-pushed the improve-error-message-path-not-executable branch from 9ada14f to 12bd6a8 Compare February 14, 2025 20:19

giuseppe marked this pull request as draft February 15, 2025 12:04

giuseppe added a commit to giuseppe/libpod that referenced this pull request Feb 17, 2025

tests: adapt e2e tests new crun error messages

a19d9c9

Needed-by: containers/crun#1672 Signed-off-by: Giuseppe Scrivano <[email protected]>

giuseppe force-pushed the improve-error-message-path-not-executable branch from 12bd6a8 to 1437baa Compare February 17, 2025 08:15

giuseppe mentioned this pull request Feb 17, 2025

tests: adapt e2e tests new crun error messages containers/podman#25340

Merged

giuseppe force-pushed the improve-error-message-path-not-executable branch from 1437baa to 9fdb879 Compare February 17, 2025 08:41

giuseppe added a commit to giuseppe/libpod that referenced this pull request Feb 17, 2025

tests: adapt e2e tests new crun error messages

db07d8a

Needed-by: containers/crun#1672 Signed-off-by: Giuseppe Scrivano <[email protected]>

giuseppe added a commit to giuseppe/libpod that referenced this pull request Feb 17, 2025

tests: adapt tests new crun error messages

be5b4a3

Needed-by: containers/crun#1672 Signed-off-by: Giuseppe Scrivano <[email protected]>

giuseppe added a commit to giuseppe/libpod that referenced this pull request Feb 17, 2025

test: adapt tests new crun error messages

1caa9ec

Needed-by: containers/crun#1672 Signed-off-by: Giuseppe Scrivano <[email protected]>

giuseppe added a commit to giuseppe/libpod that referenced this pull request Feb 17, 2025

test: adapt tests new crun error messages

c65bb90

Needed-by: containers/crun#1672 Signed-off-by: Giuseppe Scrivano <[email protected]>

giuseppe force-pushed the improve-error-message-path-not-executable branch from 9fdb879 to 8f9d583 Compare February 18, 2025 07:16

giuseppe mentioned this pull request Feb 18, 2025

oci: report empty exec path as ENOENT containers/podman#25350

Merged

giuseppe force-pushed the improve-error-message-path-not-executable branch from 8f9d583 to 4f0333b Compare February 18, 2025 10:20

giuseppe force-pushed the improve-error-message-path-not-executable branch from 4f0333b to 5c7dca7 Compare February 18, 2025 11:53

giuseppe marked this pull request as ready for review February 18, 2025 12:24

giuseppe marked this pull request as draft February 18, 2025 12:24

giuseppe force-pushed the improve-error-message-path-not-executable branch from 5c7dca7 to 013f69c Compare February 18, 2025 12:38

giuseppe added 7 commits February 18, 2025 15:10

error: silence compiler warning

f941be4

Signed-off-by: Giuseppe Scrivano <[email protected]>

tests: do a shallow git clone for podman

a77702c

Signed-off-by: Giuseppe Scrivano <[email protected]>

utils: move error handling inside find_executable()

bb56343

Signed-off-by: Giuseppe Scrivano <[email protected]>

container: pass down executable path to custom handler

410f0d5

if a handler is specified, do not return an early error if the executable path could not be found. Signed-off-by: Giuseppe Scrivano <[email protected]>

utils: check for eaccess existence

4948e45

Signed-off-by: Giuseppe Scrivano <[email protected]>

utils: do not use hardcoded path buffer

5f5454b

Signed-off-by: Giuseppe Scrivano <[email protected]>

giuseppe force-pushed the improve-error-message-path-not-executable branch from 013f69c to 5393867 Compare February 18, 2025 14:10

giuseppe force-pushed the improve-error-message-path-not-executable branch from 5393867 to d92b070 Compare February 18, 2025 20:54

giuseppe force-pushed the improve-error-message-path-not-executable branch from d92b070 to 3721355 Compare February 19, 2025 15:03

giuseppe marked this pull request as ready for review February 19, 2025 15:03

Luap99 reviewed Feb 19, 2025

View reviewed changes

giuseppe merged commit 6da2b1b into containers:main Feb 19, 2025
48 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

utils: improve error message if path not executable #1672

utils: improve error message if path not executable #1672

giuseppe commented Feb 14, 2025

packit-as-a-service bot commented Feb 14, 2025

Luap99 Feb 14, 2025

Luap99 left a comment

eriksjolund commented Feb 14, 2025 •

edited

Loading

giuseppe commented Feb 14, 2025

eriksjolund commented Feb 14, 2025

giuseppe commented Feb 14, 2025

Luap99 commented Feb 14, 2025

giuseppe commented Feb 14, 2025

giuseppe commented Feb 17, 2025

Luap99 left a comment

utils: improve error message if path not executable #1672

utils: improve error message if path not executable #1672

Conversation

giuseppe commented Feb 14, 2025

packit-as-a-service bot commented Feb 14, 2025

Luap99 Feb 14, 2025

Choose a reason for hiding this comment

Luap99 left a comment

Choose a reason for hiding this comment

eriksjolund commented Feb 14, 2025 • edited Loading

update

giuseppe commented Feb 14, 2025

eriksjolund commented Feb 14, 2025

giuseppe commented Feb 14, 2025

Luap99 commented Feb 14, 2025

giuseppe commented Feb 14, 2025

giuseppe commented Feb 17, 2025

Luap99 left a comment

Choose a reason for hiding this comment

eriksjolund commented Feb 14, 2025 •

edited

Loading