By default clone() will create a child that does not send SIGCHLD when
the child exits. The caller has to specifiy the SIGNAL it should get in
the flag bitmask.
see clone(2) under "The child termination signal"
This fixes the problem where pasta would not exit when the execvp()
call failed, i.e. when the command does not exists.
Signed-off-by: Paul Holzinger <pholzing(a)redhat.com>
---
pasta.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/pasta.c b/pasta.c
index 528f02a..3f6477c 100644
--- a/pasta.c
+++ b/pasta.c
@@ -229,7 +229,7 @@ void pasta_start_ns(struct ctx *c, uid_t uid, gid_t gid,
pasta_child_pid = do_clone(pasta_spawn_cmd, ns_fn_stack,
sizeof(ns_fn_stack),
CLONE_NEWIPC | CLONE_NEWPID | CLONE_NEWNET |
- CLONE_NEWUTS,
+ CLONE_NEWUTS | SIGCHLD,
(void *)&arg);
if (pasta_child_pid == -1) {
--
2.39.1
Exits codes are very useful for scripts, when the pasta child execvp()
call fails with ENOENT that parent should also exit with > 0. In short
the parent should always exit with the code from the child to make it
useful in scripts.
It is easy to test with: `pasta -- bash -c "exit 3"; echo $?`
Signed-off-by: Paul Holzinger <pholzing(a)redhat.com>
---
pasta.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/pasta.c b/pasta.c
index 3f6477c..4b18d7e 100644
--- a/pasta.c
+++ b/pasta.c
@@ -64,9 +64,14 @@ void pasta_child_handler(int signal)
if (pasta_child_pid &&
!waitid(P_PID, pasta_child_pid, &infop, WEXITED | WNOHANG)) {
- if (infop.si_pid == pasta_child_pid)
- exit(EXIT_SUCCESS);
- /* Nothing to do, detached PID namespace going away */
+ if (infop.si_pid == pasta_child_pid) {
+ if (infop.si_code == CLD_EXITED)
+ exit(infop.si_status);
+
+ /* else killed by signal, si_status == SIGNUM in this case */
+ exit(infop.si_status + 128);
+ }
+ /* Nothing to do, detached PID namespace going away */
}
waitid(P_ALL, 0, NULL, WEXITED | WNOHANG);
--
2.39.1
On Wed, 8 Feb 2023 16:54:41 +0100 Paul Holzinger <pholzing(a)redhat.com> wrote:Exits codes are very useful for scripts, when the pasta child execvp() call fails with ENOENT that parent should also exit with > 0. In short the parent should always exit with the code from the child to make it useful in scripts. It is easy to test with: `pasta -- bash -c "exit 3"; echo $?` Signed-off-by: Paul Holzinger <pholzing(a)redhat.com> --- pasta.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/pasta.c b/pasta.c index 3f6477c..4b18d7e 100644 --- a/pasta.c +++ b/pasta.c @@ -64,9 +64,14 @@ void pasta_child_handler(int signal) if (pasta_child_pid && !waitid(P_PID, pasta_child_pid, &infop, WEXITED | WNOHANG)) { - if (infop.si_pid == pasta_child_pid) - exit(EXIT_SUCCESS); - /* Nothing to do, detached PID namespace going away */ + if (infop.si_pid == pasta_child_pid) { + if (infop.si_code == CLD_EXITED) + exit(infop.si_status); + + /* else killed by signal, si_status == SIGNUM in this case */It took me a while to figure out that SIGNUM is not a constant I wasn't aware of, just the signal number -- I guess you meant 'signum' from sigaction() or suchlike? Maybe you could go with: /* If killed by a signal, si_status is its number */+ exit(infop.si_status + 128);This is a bit arbitrary -- returning n + 128 is what Bash, zsh, and a few other shells do, and probably a reasonable convention given that POSIX says we need to return a value greater than 128. Still I would mention it: /* Arbitrary: return signal number + 128 */ or: /* If killed by a signal, si_status is the number. * Follow common shell convention of returning it + 128. */+ } + /* Nothing to do, detached PID namespace going away */This should be moved back into the conditional block (swapping the two lines above, or moving this comment at the beginning of the block). If si_pid is not the child, we need to do other stuff (that is, reap clones).} waitid(P_ALL, 0, NULL, WEXITED | WNOHANG);-- Stefano
Thanks for the feedback, I will send a v2 with the comments fixed. On 08/02/2023 19:14, Stefano Brivio wrote:On Wed, 8 Feb 2023 16:54:41 +0100 Paul Holzinger <pholzing(a)redhat.com> wrote:The second suggestion sounds good to me.Exits codes are very useful for scripts, when the pasta child execvp() call fails with ENOENT that parent should also exit with > 0. In short the parent should always exit with the code from the child to make it useful in scripts. It is easy to test with: `pasta -- bash -c "exit 3"; echo $?` Signed-off-by: Paul Holzinger <pholzing(a)redhat.com> --- pasta.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/pasta.c b/pasta.c index 3f6477c..4b18d7e 100644 --- a/pasta.c +++ b/pasta.c @@ -64,9 +64,14 @@ void pasta_child_handler(int signal) if (pasta_child_pid && !waitid(P_PID, pasta_child_pid, &infop, WEXITED | WNOHANG)) { - if (infop.si_pid == pasta_child_pid) - exit(EXIT_SUCCESS); - /* Nothing to do, detached PID namespace going away */ + if (infop.si_pid == pasta_child_pid) { + if (infop.si_code == CLD_EXITED) + exit(infop.si_status); + + /* else killed by signal, si_status == SIGNUM in this case */It took me a while to figure out that SIGNUM is not a constant I wasn't aware of, just the signal number -- I guess you meant 'signum' from sigaction() or suchlike? Maybe you could go with: /* If killed by a signal, si_status is its number */+ exit(infop.si_status + 128);This is a bit arbitrary -- returning n + 128 is what Bash, zsh, and a few other shells do, and probably a reasonable convention given that POSIX says we need to return a value greater than 128. Still I would mention it: /* Arbitrary: return signal number + 128 */ or: /* If killed by a signal, si_status is the number. * Follow common shell convention of returning it + 128. */Whoops, this was by accident.+ } + /* Nothing to do, detached PID namespace going away */This should be moved back into the conditional block (swapping the two lines above, or moving this comment at the beginning of the block).If si_pid is not the child, we need to do other stuff (that is, reap clones). > } > > waitid(P_ALL, 0, NULL, WEXITED | WNOHANG);
On Wed, 8 Feb 2023 16:54:40 +0100 Paul Holzinger <pholzing(a)redhat.com> wrote:By default clone() will create a child that does not send SIGCHLD when the child exits. The caller has to specifiy the SIGNAL it should get in the flag bitmask. see clone(2) under "The child termination signal" This fixes the problem where pasta would not exit when the execvp() call failed, i.e. when the command does not exists. Signed-off-by: Paul Holzinger <pholzing(a)redhat.com>Thanks, this looks good to me, pushing later. -- Stefano
On Wed, 8 Feb 2023 16:54:40 +0100 Paul Holzinger <pholzing(a)redhat.com> wrote:By default clone() will create a child that does not send SIGCHLD when the child exits. The caller has to specifiy the SIGNAL it should get in the flag bitmask. see clone(2) under "The child termination signal" This fixes the problem where pasta would not exit when the execvp() call failed, i.e. when the command does not exists. Signed-off-by: Paul Holzinger <pholzing(a)redhat.com>Applied. -- Stefano