bubblewrap: add --as-pid-1#196
Conversation
cgwalters
left a comment
cgwalters
left a comment
There was a problem hiding this comment.
Only minor nits, though it should be pretty easy to at least add a use of this in tests/test-run.sh, something like run bash -c 'echo $$' and assert the output is 1?
| " --info-fd FD Write information about the running container to FD\n" | ||
| " --new-session Create a new terminal session\n" | ||
| " --die-with-parent Kills with SIGKILL child process (COMMAND) when bwrap or bwrap's parent dies.\n" | ||
| " --no-reaper Do not install a reaper process with PID=1\n" |
There was a problem hiding this comment.
How about --as-pid1, like the inverse of systemd-nspawn --as-pid2?
There was a problem hiding this comment.
I am fine with it. @alexlarsson do you like --as-pid1 more?
There was a problem hiding this comment.
In general, this option only makes sense with unshare-pid. Maybe we should verify that and print a warning if not. Maybe it could be --unshare-pid-noinit?
There was a problem hiding this comment.
I dunno...it feels like we're optimizing for the wrong thing here. We can easily error out if the user doesn't pass --unshare-pid too.
But if I was looking at the --help output I would be a lot less likely to find --unshare-pid-noinit if I was looking for this functionality.
--as-pid1 The executed program will be PID 1 (suppress builtin init)
vs
--unshare-pid-noinit Do not install a reaper process with PID=1
?
There was a problem hiding this comment.
(Sorry to bikeshed, but OTOH the command line is the bwrap "API" and it makes sense to consider it carefully I think)
There was a problem hiding this comment.
I also like --as-pid-1 more, easier to find it the --help output. I am fine to change the patch if consensus is reached
There was a problem hiding this comment.
--as-pid-1 sounds good to me!
| __debug__ (("forking for child\n")); | ||
|
|
||
| if (opt_unshare_pid || lock_files != NULL || opt_sync_fd != -1) | ||
| if (!no_reaper && (opt_unshare_pid || lock_files != NULL || opt_sync_fd != -1)) |
There was a problem hiding this comment.
!as_pid1 as a variable would also read better than the double-negative !no
There was a problem hiding this comment.
I think we should also check if the user specified any lock files or a sync-fd and error out, because those require support from our own pid1.
There was a problem hiding this comment.
Also, opt_die_with_parent needs pid1 too.
There was a problem hiding this comment.
Also, if opt_unshare_pid is set *and no_reaper, then we should not allocate event_fd (because it needs the init process).
There was a problem hiding this comment.
hm.. doesn't opt_die_with_parent still work without pid1? The handle_die_with_parent() just before the execvp will still be valid no?
I've done a quick test and the process in the sandbox exits when bwrap is killed.
There was a problem hiding this comment.
True, i was just looking at the handle_die_with_parent call in do_init(). It should work.
|
|
||
| if (opt_sync_fd != -1) | ||
| close (opt_sync_fd); | ||
| if (!no_reaper) |
There was a problem hiding this comment.
We don't need this hunk, since we can rely on sync_fd = -1 in this case right?
There was a problem hiding this comment.
I thought of leaking opt_sync_fd into the container process so that sync_fd would still work with --no-reaper (unless the container process doesn't close it), so that the two options are not mutually exclusive.
I'd say it should be fine to leak opt_sync_fd, or do we change to an explicit error?
There was a problem hiding this comment.
I guess that leaking the fd should work. However, please add a more detailed comment about this in the code.
| static int proc_fd = -1; | ||
| static char *opt_exec_label = NULL; | ||
| static char *opt_file_label = NULL; | ||
| static bool no_reaper; |
There was a problem hiding this comment.
Call this opt_no_reaper to match the other options.
giuseppe
force-pushed
the
no-reaper
branch
2 times, most recently
from
a4d3cdd to
a86d621
Compare
|
pushed a new version with all the comments addressed |
giuseppe
force-pushed
the
no-reaper
branch
2 times, most recently
from
06a606e to
d6f611c
Compare
It allows to run a process with PID=1 in the new pid namespace. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
|
I changed the option name to |
|
@alexlarsson This repo uses homu, now we have a merge commit 😞 |
|
@cgwalters :( too many different repos... |
|
I understand, it bites me too. Things should now be set up on this repo though so that only homu can push. |
3 participants
Useful to run a process with PID=1
Signed-off-by: Giuseppe Scrivano gscrivan@redhat.com