Make --kill long-option-only, no -k short form
Per the user's request: --kill is destructive enough (stops every process a session started) that typo-prone brevity isn't worth it -- dropping the short option makes it harder to invoke by mistake. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Gv3s5jckJKzh6JkMoi2Akz
This commit is contained in:
@@ -38,7 +38,7 @@ Source layout (all under `src/`):
|
||||
`Mode::exec` dispatch arm) `-x/--exec <pid>`'s own pid parsing/validation
|
||||
(`ParsedArgs::exec_pid`, a positive integer or a hard error) and its
|
||||
trailing-command requirement (`ParsedArgs::command`, required non-empty).
|
||||
`-k/--kill <pid>` (`ParsedArgs::kill_pid`) shares that same positive-integer
|
||||
`--kill <pid>` (`ParsedArgs::kill_pid`) shares that same positive-integer
|
||||
parsing/validation via a small extracted `parse_pid_arg()` helper (`.cpp`-local)
|
||||
rather than duplicating the `strtol` dance a second time — unlike `-x/--exec`,
|
||||
it takes no trailing command, so it's simply not added to the leftover-args
|
||||
@@ -264,7 +264,7 @@ Source layout (all under `src/`):
|
||||
same instant everything else that needs it does, rather than needing its own
|
||||
separate pid-discovery mechanism. That same `on_start` lambda also calls
|
||||
`create_session_cgroup()` (`session_cgroup.h`, see below), right alongside
|
||||
`create_session_lock()`, so `-k/--kill` can later find every process the
|
||||
`create_session_lock()`, so `--kill` can later find every process the
|
||||
session ever starts via its dedicated cgroup; `remove_session_cgroup()` is
|
||||
called from the same post-`run_process_foreground()` spot
|
||||
`release_session_lock()` already is.
|
||||
@@ -383,7 +383,7 @@ Source layout (all under `src/`):
|
||||
a check and a later removal. Only files it actually removes are reported back
|
||||
(as `SessionInfo`s with `running=false`); still-locked (running) files are
|
||||
left untouched and not reported.
|
||||
- `session_cgroup.{h,cpp}` — gives `-k/--kill` (`kill_session.{h,cpp}`, see
|
||||
- `session_cgroup.{h,cpp}` — gives `--kill` (`kill_session.{h,cpp}`, see
|
||||
below) a reliable way to find every process a session ever started, however
|
||||
deeply forked/daemonized/reparented, by putting it in a dedicated cgroup v2
|
||||
group from the moment it starts. `cgroup_v2_available()` checks for
|
||||
@@ -429,7 +429,7 @@ Source layout (all under `src/`):
|
||||
shared by `exec_session.{h,cpp}` and `kill_session.{h,cpp}` (see both
|
||||
below); pulled into their own file (rather than staying private to
|
||||
`exec_session.cpp`, where `resolve_namespace_pid()` originally lived) once
|
||||
`-k/--kill` needed the exact same "find the real sandboxed child" logic, to
|
||||
`--kill` needed the exact same "find the real sandboxed child" logic, to
|
||||
avoid a second, drifting copy. `resolve_namespace_pid()` is unchanged from
|
||||
its original `exec_session.cpp` form (see that entry for the full
|
||||
reasoning: bwrap's own outer/tracked pid never actually enters the
|
||||
@@ -517,7 +517,7 @@ Source layout (all under `src/`):
|
||||
itself never actually enters those namespaces — comparing
|
||||
`/proc/<outer_pid>/ns/{pid,uts,ipc,cgroup}` against this process's own showed
|
||||
them identical, while only `mnt`/`user` differed. `resolve_namespace_pid()`
|
||||
(`sandbox_process.{h,cpp}` — moved out of this file once `-k/--kill`
|
||||
(`sandbox_process.{h,cpp}` — moved out of this file once `--kill`
|
||||
needed the exact same logic, see that entry) finds that real inner process
|
||||
so this can join *its* namespaces instead. For each of `{mnt→--mount, uts→--uts, ipc→--ipc, pid→--pid,
|
||||
cgroup→--cgroup, user→--user}` (`net` deliberately excluded — this project
|
||||
@@ -584,7 +584,7 @@ Source layout (all under `src/`):
|
||||
real `250:250` via the fetched `/etc/passwd` and then fails clearly (helper not
|
||||
bind-mounted, since the session itself had no declared user) rather than
|
||||
silently running as the wrong identity.
|
||||
- `kill_session.{h,cpp}` — implements `-k/--kill <pid>`, stopping a tracked,
|
||||
- `kill_session.{h,cpp}` — implements `--kill <pid>`, stopping a tracked,
|
||||
running `-r/--run` session and everything it started. `kill_session()`
|
||||
validates `pid` the same way `exec_in_session()` does (via `list_sessions()`,
|
||||
`pid_file.h`). **Real bug reported by the user against their own actual
|
||||
@@ -642,7 +642,7 @@ Source layout (all under `src/`):
|
||||
pid namespaces, unlike the user's real target device): a daemonized
|
||||
busybox session running `sh -c 'sleep 300 & exec sleep 300'` (mirroring
|
||||
the daemonize-then-exec shape of the original bug) was fully cleaned up
|
||||
by `-k/--kill`, including the backgrounded child, with no leftover
|
||||
by `--kill`, including the backgrounded child, with no leftover
|
||||
processes, mounts, or layers; a second run using `sh -c 'trap "" TERM;
|
||||
sleep 300'` (ignoring `SIGTERM` entirely) confirmed the forced-`SIGKILL`
|
||||
escalation path too, taking the full 10s grace period before the pid
|
||||
@@ -778,7 +778,7 @@ Build directory is `buildDir/` (already configured).
|
||||
(see `priv_drop_helper.cpp` in "Project state")
|
||||
- Run the executable: `./buildDir/slocker-lite -m <image.tar>` (see `--help` for the
|
||||
full flag list: `-m/--mount`, `-r/--run`, `-u/--umount`, `-c/--cleanup`,
|
||||
`-l/--list-images`, `-i/--inspect`, `-x/--exec`, `-k/--kill`, `-n/--no-nsenter`, `-D/--daemonize`,
|
||||
`-l/--list-images`, `-i/--inspect`, `-x/--exec`, `--kill`, `-n/--no-nsenter`, `-D/--daemonize`,
|
||||
`--user`, `--group`, `--hostname`, `--env`, `--env-file`, `-v/--volume`, `--list-volumes`, `--delete-volume`,
|
||||
`--delete-volume-full`, `--list-processes`, `--clean-processes`, `-t/--test`, `--log-level`,
|
||||
`-h/--help`, `-V/--version`)
|
||||
|
||||
@@ -59,7 +59,7 @@ slocker-lite -c|--cleanup <layer-id>
|
||||
slocker-lite -l|--list-images <directory>
|
||||
slocker-lite -i|--inspect <image.tar>
|
||||
slocker-lite -x|--exec <pid> [-- <command> [args...]]
|
||||
slocker-lite -k|--kill <pid>
|
||||
slocker-lite --kill <pid>
|
||||
slocker-lite -v|--volume <name> <directory>
|
||||
slocker-lite --list-volumes
|
||||
slocker-lite --delete-volume <name>
|
||||
@@ -87,7 +87,7 @@ slocker-lite -V|--version
|
||||
| `-l, --list-images <dir>` | List OCI Image Layout tars (`*.tar`, `*.tar.*`) found directly in `<dir>`, with their `name:tag`. |
|
||||
| `-i, --inspect <image.tar>` | Print an image's declared user, exposed ports, env, volumes, and default command, without mounting or running it. |
|
||||
| `-x, --exec <pid>` | Join an already-running `--run` session (`<pid>` must be one `--list-processes` shows as `running`) and run a command inside its container. Pass `-- <command> [args...]` to specify it. |
|
||||
| `-k, --kill <pid>` | Stop a running `--run` session (`<pid>` must be one `--list-processes` shows as `running`): sends `SIGTERM`, waits up to 10s, then forces it with `SIGKILL`. Reaches every process the session started — including daemonized/reparented ones a plain `kill <pid>` would leave behind — via a dedicated cgroup when available, or the sandboxed pid namespace's own collapse-on-kill guarantee when not, falling back to signaling the tracked pid alone if neither applies. |
|
||||
| `--kill <pid>` | Stop a running `--run` session (`<pid>` must be one `--list-processes` shows as `running`): sends `SIGTERM`, waits up to 10s, then forces it with `SIGKILL`. Reaches every process the session started — including daemonized/reparented ones a plain `kill <pid>` would leave behind — via a dedicated cgroup when available, or the sandboxed pid namespace's own collapse-on-kill guarantee when not, falling back to signaling the tracked pid alone if neither applies. |
|
||||
| `-v, --volume <name> <dir>` | Create a named volume mapped to a host directory (created if missing), recorded in the config file's `volumes` section. Fails if the name or directory is already used by an existing volume. Volume names can't contain `/`. With `--run`, instead mounts a volume into the sandbox (repeatable): `<name>` is an existing named volume, or, if it contains `/`, a host directory path (created if missing); `<dir>` is the absolute path inside the container to mount it at. If the host directory is empty and the image already has content there, that content is copied in first, preserving numeric ownership/permissions/links and, where the host filesystem supports them, extended attributes/ACLs (skipped with a warning otherwise). |
|
||||
| `--list-volumes` | List all named volumes (see `-v/--volume`) with their host directory. |
|
||||
| `--delete-volume <name>` | Remove a named volume from the config. The host directory is left untouched. |
|
||||
@@ -214,13 +214,13 @@ rather than joining the outer `bwrap` process's own namespaces, so the joined
|
||||
command sees the container's process tree and hostname too, not just its
|
||||
filesystem.
|
||||
|
||||
`-k/--kill <pid>` stops a running session and everything it started — not just
|
||||
`--kill <pid>` stops a running session and everything it started — not just
|
||||
the tracked `bwrap` process. A plain `kill <pid>` can leave processes behind: a
|
||||
container whose entrypoint daemonizes a service (double-forks and detaches)
|
||||
before `exec`-ing its main command can end up with that service reparented
|
||||
somewhere `bwrap` dying never reaches, especially on a kernel without pid
|
||||
namespace support, where it reparents all the way to the *host's* own pid 1.
|
||||
`-k/--kill` picks between three mechanisms depending on what's actually
|
||||
`--kill` picks between three mechanisms depending on what's actually
|
||||
available for that session: a dedicated cgroup (set up at `-r/--run` time,
|
||||
reliably includes every process the session ever started regardless of
|
||||
daemonizing or pid namespace support — the most complete option, when the
|
||||
|
||||
+15
-9
@@ -32,6 +32,9 @@ namespace {
|
||||
// (--log-level's was freed up so -l could become --list-images; -u is already
|
||||
// --umount; the rest have no natural free letter left, or don't need one), so
|
||||
// they need long-option vals outside the printable-char range short options use.
|
||||
// --kill is different: 'k' was free, but long-option-only is deliberate here
|
||||
// (not an availability gap) -- stopping a session is destructive enough that
|
||||
// typo-prone brevity isn't worth it.
|
||||
namespace options {
|
||||
constexpr int log_level = 256;
|
||||
constexpr int user = 257;
|
||||
@@ -44,6 +47,7 @@ constexpr int list_processes = 263;
|
||||
constexpr int clean_processes = 264;
|
||||
constexpr int env = 265;
|
||||
constexpr int env_file = 266;
|
||||
constexpr int kill = 267;
|
||||
} // namespace options
|
||||
|
||||
constexpr std::array<struct option, 26> long_options = {{
|
||||
@@ -66,7 +70,7 @@ constexpr std::array<struct option, 26> long_options = {{
|
||||
{"delete-volume-full", required_argument, nullptr, options::delete_volume_full},
|
||||
{"inspect", required_argument, nullptr, 'i'},
|
||||
{"exec", required_argument, nullptr, 'x'},
|
||||
{"kill", required_argument, nullptr, 'k'},
|
||||
{"kill", required_argument, nullptr, options::kill},
|
||||
{"hostname", required_argument, nullptr, options::hostname},
|
||||
{"list-processes", no_argument, nullptr, options::list_processes},
|
||||
{"clean-processes", no_argument, nullptr, options::clean_processes},
|
||||
@@ -84,7 +88,7 @@ void print_usage(const char* prog) {
|
||||
" {0} -l|--list-images <directory>\n"
|
||||
" {0} -i|--inspect <image.tar>\n"
|
||||
" {0} -x|--exec <pid> [-- <command> [args...]]\n"
|
||||
" {0} -k|--kill <pid>\n"
|
||||
" {0} --kill <pid>\n"
|
||||
" {0} -v|--volume <name> <directory>\n"
|
||||
" {0} --list-volumes\n"
|
||||
" {0} --delete-volume <name>\n"
|
||||
@@ -153,7 +157,7 @@ void print_usage(const char* prog) {
|
||||
" shown by --list-processes as \"running\") and run\n"
|
||||
" a command inside its container; pass\n"
|
||||
" -- <command> [args...] to specify it\n"
|
||||
" -k, --kill <pid> stop a running --run session (pid must be one\n"
|
||||
" --kill <pid> stop a running --run session (pid must be one\n"
|
||||
" shown by --list-processes as \"running\"): sends\n"
|
||||
" SIGTERM, waits up to 10s, then forces it with\n"
|
||||
" SIGKILL. Reaches every process the session\n"
|
||||
@@ -162,7 +166,9 @@ void print_usage(const char* prog) {
|
||||
" dedicated cgroup when available, or the sandboxed\n"
|
||||
" pid namespace's own collapse-on-kill guarantee\n"
|
||||
" when not, falling back to signaling the tracked\n"
|
||||
" pid alone if neither applies\n"
|
||||
" pid alone if neither applies. Long-option only,\n"
|
||||
" deliberately: this is destructive enough that\n"
|
||||
" typo-prone brevity isn't worth it\n"
|
||||
" -v, --volume <name> <dir> create a named volume mapped to a host directory\n"
|
||||
" (created if missing), recorded in the config\n"
|
||||
" file's volumes section. With --run, instead\n"
|
||||
@@ -205,7 +211,7 @@ void print_version() {
|
||||
PACKAGE, VERSION);
|
||||
}
|
||||
|
||||
// Shared by -x/--exec's exec_pid and -k/--kill's kill_pid -- both are a bare
|
||||
// Shared by -x/--exec's exec_pid and --kill's kill_pid -- both are a bare
|
||||
// positive integer, nothing else.
|
||||
std::optional<pid_t> parse_pid_arg(const std::string& text) {
|
||||
char* end = nullptr;
|
||||
@@ -234,7 +240,7 @@ bool apply_log_level(std::string_view name) {
|
||||
std::optional<int> parse_args(int argc, char* argv[], ParsedArgs& out) {
|
||||
opterr = 0;
|
||||
int opt;
|
||||
while ((opt = getopt_long(argc, argv, ":hVtm:u:r:c:nl:v:i:x:Dk:", long_options.data(), nullptr)) != -1) {
|
||||
while ((opt = getopt_long(argc, argv, ":hVtm:u:r:c:nl:v:i:x:D", long_options.data(), nullptr)) != -1) {
|
||||
switch (opt) {
|
||||
case 'h':
|
||||
print_usage(argv[0]);
|
||||
@@ -250,12 +256,12 @@ std::optional<int> parse_args(int argc, char* argv[], ParsedArgs& out) {
|
||||
case 'l':
|
||||
case 'i':
|
||||
case 'x':
|
||||
case 'k':
|
||||
case options::list_volumes:
|
||||
case options::delete_volume:
|
||||
case options::delete_volume_full:
|
||||
case options::list_processes:
|
||||
case options::clean_processes: {
|
||||
case options::clean_processes:
|
||||
case options::kill: {
|
||||
Mode requested;
|
||||
switch (opt) {
|
||||
case 't':
|
||||
@@ -282,7 +288,7 @@ std::optional<int> parse_args(int argc, char* argv[], ParsedArgs& out) {
|
||||
case 'x':
|
||||
requested = Mode::exec;
|
||||
break;
|
||||
case 'k':
|
||||
case options::kill:
|
||||
requested = Mode::kill;
|
||||
break;
|
||||
case options::list_volumes:
|
||||
|
||||
@@ -41,7 +41,7 @@ std::filesystem::path session_cgroup_path(std::string_view container_name, pid_t
|
||||
std::optional<SessionCgroup> create_session_cgroup(std::string_view container_name, pid_t pid) {
|
||||
if (!cgroup_v2_available()) {
|
||||
spdlog::debug(
|
||||
"cgroup v2 not available; -k/--kill will fall back to a less complete method for this session");
|
||||
"cgroup v2 not available; --kill will fall back to a less complete method for this session");
|
||||
return std::nullopt;
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user