From d2d631117d57451f3a6a4cde40f7584ea5e37b75 Mon Sep 17 00:00:00 2001 From: Viorel Munteanu Date: Sat, 5 Sep 2026 08:43:58 +0000 Subject: [PATCH] Drop -m/-u/-c short options for --mount/--umount/--cleanup Now that -r/--run and -x/--exec cover normal use, --mount/--umount/--cleanup are debug-only escape hatches not worth a short letter. Reassigned their long_options codes to long-option-only constants (options::mount/umount/ cleanup) and dropped m:/u:/c: from getopt_long's own short-options string. Updated the fixture smoke test (tests/run_test.py) and docs, which invoked -m/-u/-c directly. --- CLAUDE.md | 9 +++--- README.md | 16 +++++----- src/cli_args.cpp | 59 +++++++++++++++++++++--------------- tests/run_test.py | 4 +-- tests/unit/test_cli_args.cpp | 12 ++++++-- 5 files changed, 59 insertions(+), 41 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 21b7070..b5660ed 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -329,8 +329,9 @@ Source layout (all under `src/`): compiled into this build" message and returns nonzero instead of failing to link. `-t`'s own leftover-args capture requires a literal `--` before any Catch2 option that looks like one of slocker-lite's own - (`-r/--reporter` collides with `-r/--run`, `-c/--section` with - `-c/--cleanup`) — a bare tag expression like `-t -- "[unit]"` needs it + (`-r/--reporter` collides with `-r/--run` — `-c/--section` no longer + collides with anything now that `--cleanup` dropped its own `-c` short + form) — a bare tag expression like `-t -- "[unit]"` needs it too by convention, though `getopt_long`'s own permutation happens to let a `-t "[unit]"` without `--` work anyway, since `"[unit]"` doesn't start with `-`. Distinct from the Meson-driven fixture smoke test under @@ -2277,8 +2278,8 @@ Build directory is `buildDir/` (already configured). - Build: `meson compile -C buildDir` (or `ninja -C buildDir`) — also builds `buildDir/slocker-lite-priv-drop`, the statically-linked helper `-r --user` needs (see `priv_drop_helper.cpp` in "Project state") -- Run the executable: `./buildDir/slocker-lite -m ` (see `--help` for the - full flag list: `-m/--mount`, `-r/--run`, `-u/--umount`, `-c/--cleanup`, +- Run the executable: `./buildDir/slocker-lite --mount ` (see `--help` for the + full flag list: `--mount`, `-r/--run`, `--umount`, `--cleanup`, `-l/--list-images`, `-i/--inspect`, `-x/--exec`, `--kill`, `--no-nsenter`, `-D/--daemonize`, `--user`, `--group`, `--hostname`, `--env`, `--env-file`, `-v/--volume`, `--list-volumes`, `--delete-volume`, `--delete-volume-full`, `-n/--network`, `--extern`, `--intern`, `--subnet`, `--with-ipv6`, `--subnet6`, diff --git a/README.md b/README.md index d23b550..d799bfd 100644 --- a/README.md +++ b/README.md @@ -59,8 +59,8 @@ two categories are for a developer to run by hand on a real machine. `-t`/`--test` runs this project's own built-in [Catch2](https://github.com/catchorg/Catch2) test suite directly: `slocker-lite -t [-- ]`. A bare `-t` runs everything Catch2 finds; select a category with a tag expression (note the `--` -before it, needed since Catch2's own `-r/--reporter` and `-c/--section` would otherwise -collide with slocker-lite's own `-r/--run` and `-c/--cleanup`): +before it, needed since Catch2's own `-r/--reporter` would otherwise collide with +slocker-lite's own `-r/--run`): | Category | What it needs | Invocation | |---|---|---| @@ -85,10 +85,10 @@ busybox-like image there directly. ## Usage ``` -slocker-lite -m|--mount +slocker-lite --mount slocker-lite -r|--run [-v ]... [-- [args...]] -slocker-lite -u|--umount -slocker-lite -c|--cleanup +slocker-lite --umount +slocker-lite --cleanup slocker-lite -l|--list-images slocker-lite -i|--inspect slocker-lite -x|--exec [-- [args...]] @@ -106,10 +106,10 @@ slocker-lite -V|--version | Flag | Description | | --- | --- | -| `-m, --mount ` | Validate and mount an OCI Image Layout tar. | +| `--mount ` | Validate and mount an OCI Image Layout tar. Debug-only — see `-r/--run`/`-x/--exec` for the normal way to actually use an image. | | `-r, --run ` | Mount, run `bwrap` in the foreground, then unmount and clean up on exit. Defaults to the image's own `Entrypoint`/`Cmd` (or `/bin/sh` if neither is set); pass `-- [args...]` to override. | -| `-u, --umount ` | Unmount a previously mounted layer (the ID printed by `--mount`/`--run`, or from `containers-storage layers`). | -| `-c, --cleanup ` | Delete a layer and its ancestor chain from local storage (unmount it first). | +| `--umount ` | Unmount a previously mounted layer (the ID printed by `--mount`/`--run`, or from `containers-storage layers`). Debug-only — `-r/--run` already unmounts on its own when done. | +| `--cleanup ` | Delete a layer and its ancestor chain from local storage (unmount it first). Debug-only, same reason as `--umount` above. | | `--no-nsenter` | With `--run`, bind the mount directly instead of `nsenter`-ing into `fuse-overlayfs`'s namespace. Automatic when running as root; use this to force it off otherwise. | | `-D, --daemonize` | With `--run`, fork into the background: detaches from the controlling terminal (`setsid()`), ignores `SIGHUP`, and redirects stdin from `/dev/null` and stdout/stderr to a log file under `$XDG_STATE_HOME/slocker-lite/logs/`. Prints the session's pid and log path, then returns — the same pid `--list-processes`/`-x/--exec` use. | | `--user ` | With `--run`, run the command as this user (name or numeric uid) instead of the image's own declared user (or root, if it declares none). Resolved against the image's own `/etc/passwd`. Only takes effect when `--run` executes as root. | diff --git a/src/cli_args.cpp b/src/cli_args.cpp index 2b63cf4..caf24c4 100644 --- a/src/cli_args.cpp +++ b/src/cli_args.cpp @@ -30,12 +30,16 @@ namespace { // --log-level/--user/--group/--list-volumes/--delete-volume[-full]/--hostname/ // --list-processes/--clean-processes/--env/--env-file have no short form -// (--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. --no-nsenter used to be -n/--no-nsenter; +// (--log-level's was freed up so -l could become --list-images; 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. --mount/--umount/--cleanup used to have +// -m/-u/-c short forms too; dropped (long-option-only now) once -r/--run and +// -x/--exec became the normal way to actually use an image, leaving these +// three as debug-only escape hatches not worth a memorable single letter. +// --no-nsenter used to be -n/--no-nsenter; // 'n' was reassigned to the much more heavily-used -n/--network (see below), // so --no-nsenter moved here too -- it's a rare debugging override, long-only // is no real loss. --extern/--intern/--with-ipv6/--with-veth (booleans, @@ -69,6 +73,9 @@ constexpr int delete_network = 275; constexpr int network_with_veth = 276; constexpr int delete_network_full = 277; constexpr int no_dns = 278; +constexpr int mount = 279; +constexpr int umount = 280; +constexpr int cleanup = 281; } // namespace options constexpr std::array long_options = {{ @@ -76,10 +83,10 @@ constexpr std::array long_options = {{ {"version", no_argument, nullptr, 'V'}, {"test", no_argument, nullptr, 't'}, {"log-level", required_argument, nullptr, options::log_level}, - {"mount", required_argument, nullptr, 'm'}, - {"umount", required_argument, nullptr, 'u'}, + {"mount", required_argument, nullptr, options::mount}, + {"umount", required_argument, nullptr, options::umount}, {"run", required_argument, nullptr, 'r'}, - {"cleanup", required_argument, nullptr, 'c'}, + {"cleanup", required_argument, nullptr, options::cleanup}, {"no-nsenter", no_argument, nullptr, options::no_nsenter}, {"daemonize", no_argument, nullptr, 'D'}, {"list-images", required_argument, nullptr, 'l'}, @@ -115,12 +122,12 @@ constexpr std::array long_options = {{ void print_usage(const char* prog) { fmt::print( - "usage: {0} -m|--mount \n" + "usage: {0} --mount \n" " {0} -r|--run [-v ]...\n" " [-n ]... [-p [:]:]...\n" " [--no-dns] [-- [args...]]\n" - " {0} -u|--umount \n" - " {0} -c|--cleanup \n" + " {0} --umount \n" + " {0} --cleanup \n" " {0} -l|--list-images \n" " {0} -i|--inspect \n" " {0} -x|--exec [-- [args...]]\n" @@ -142,17 +149,21 @@ void print_usage(const char* prog) { " {0} -V|--version\n" "\n" "options:\n" - " -m, --mount validate and mount an OCI Image Layout tar\n" + " --mount validate and mount an OCI Image Layout tar\n" + " (debug-only -- see -r/--run and -x/--exec for\n" + " the normal way to actually use an image)\n" " -r, --run mount, run bwrap in the foreground (default\n" " command: the image's own Entrypoint/Cmd if set,\n" " else /bin/sh; pass -- [args...] to\n" " override), then unmount and clean up when it\n" " exits\n" - " -u, --umount unmount a previously mounted image layer (the\n" + " --umount unmount a previously mounted image layer (the\n" " ID printed by --mount/--run, or from\n" - " `containers-storage layers`)\n" - " -c, --cleanup delete a layer and its ancestor chain from\n" + " `containers-storage layers`) -- debug-only,\n" + " -r/--run already unmounts on its own when done\n" + " --cleanup delete a layer and its ancestor chain from\n" " local storage (unmount it first with --umount)\n" + " -- debug-only, same reason as --umount above\n" " --no-nsenter with --run, bind the mount directly instead of\n" " nsenter-ing into fuse-overlayfs's namespace\n" " (this is automatic when running as root, where\n" @@ -362,7 +373,7 @@ std::optional parse_args(int argc, char* argv[], ParsedArgs& out) { optind = 0; opterr = 0; int opt; - while ((opt = getopt_long(argc, argv, ":hVtm:u:r:c:l:v:i:x:Dwn:p:", long_options.data(), nullptr)) != -1) { + while ((opt = getopt_long(argc, argv, ":hVtr:l:v:i:x:Dwn:p:", long_options.data(), nullptr)) != -1) { switch (opt) { case 'h': print_usage(argv[0]); @@ -371,10 +382,10 @@ std::optional parse_args(int argc, char* argv[], ParsedArgs& out) { print_version(); return 0; case 't': - case 'm': - case 'u': + case options::mount: + case options::umount: case 'r': - case 'c': + case options::cleanup: case 'l': case 'i': case 'x': @@ -396,16 +407,16 @@ std::optional parse_args(int argc, char* argv[], ParsedArgs& out) { case 'w': requested = Mode::write_config; break; - case 'm': + case options::mount: requested = Mode::mount; break; - case 'u': + case options::umount: requested = Mode::unmount; break; case 'r': requested = Mode::run; break; - case 'c': + case options::cleanup: requested = Mode::cleanup; break; case 'l': @@ -475,7 +486,7 @@ std::optional parse_args(int argc, char* argv[], ParsedArgs& out) { // -n/--network takes a single token (the name), always via // getopt's own required_argument -- repeatable, so it's // accumulated here rather than set as a one-shot Mode like - // 't'/'m'/etc. above. Whether occurrences mean "join" (with + // 't'/`--mount`/etc. above. Whether occurrences mean "join" (with // -r) or "create" (alone, exactly one) is resolved after the // loop, same as -v/--volume's own standalone-vs-with-run split. out.network_specs.push_back(optarg); diff --git a/tests/run_test.py b/tests/run_test.py index 5c5c1bb..1bb2980 100644 --- a/tests/run_test.py +++ b/tests/run_test.py @@ -15,7 +15,7 @@ def run(slocker_lite: str, *args: str) -> subprocess.CompletedProcess: def main() -> int: slocker_lite, fixture_tar = sys.argv[1], sys.argv[2] - mount = run(slocker_lite, "-m", fixture_tar) + mount = run(slocker_lite, "--mount", fixture_tar) if mount.returncode != 0: print(mount.stderr, file=sys.stderr) return mount.returncode @@ -26,7 +26,7 @@ def main() -> int: return 1 layer_id = match.group(1) - for args in (("-u", layer_id), ("-c", layer_id)): + for args in (("--umount", layer_id), ("--cleanup", layer_id)): result = run(slocker_lite, *args) if result.returncode != 0: print(result.stderr, file=sys.stderr) diff --git a/tests/unit/test_cli_args.cpp b/tests/unit/test_cli_args.cpp index 865a84c..7591acb 100644 --- a/tests/unit/test_cli_args.cpp +++ b/tests/unit/test_cli_args.cpp @@ -56,13 +56,19 @@ ParseResult run_parse(const std::vector& args) { } // namespace -TEST_CASE("parse_args: -m selects Mode::mount", "[unit]") { - auto result = run_parse({"-m", "image.tar"}); +TEST_CASE("parse_args: --mount selects Mode::mount", "[unit]") { + auto result = run_parse({"--mount", "image.tar"}); REQUIRE_FALSE(result.exit_code.has_value()); CHECK(result.args.mode == Mode::mount); CHECK(result.args.mode_arg == "image.tar"); } +TEST_CASE("parse_args: -m is no longer a recognized short option", "[unit]") { + auto result = run_parse({"-m", "image.tar"}); + REQUIRE(result.exit_code.has_value()); + CHECK(*result.exit_code == 1); +} + TEST_CASE("parse_args: -h returns exit code 0 immediately", "[unit]") { auto result = run_parse({"-h"}); REQUIRE(result.exit_code.has_value()); @@ -82,7 +88,7 @@ TEST_CASE("parse_args: no mode at all is a parse error", "[unit]") { } TEST_CASE("parse_args: two actions on the same command line is a parse error", "[unit]") { - auto result = run_parse({"-m", "a.tar", "-l", "dir"}); + auto result = run_parse({"--mount", "a.tar", "-l", "dir"}); REQUIRE(result.exit_code.has_value()); CHECK(*result.exit_code == 1); }