From cbec986e781bb31a5a9dfa1db233272baaff7d49 Mon Sep 17 00:00:00 2001 From: Viorel Munteanu Date: Sat, 5 Sep 2026 09:32:36 +0000 Subject: [PATCH] Split config.yaml into global+persistent files; add -c/--config-file config.yaml now holds only the global section (log-level, unshare-*, with-veth, with-ipv6); a new persistent.yaml holds volumes/networks. load_config_file()/write_config_file() are replaced by load_global_config()/load_persistent_config()/write_global_config()/ write_persistent_config(), each touching only their own file. -c/--config-file lets one invocation use an alternate file for the global section only -- persistent.yaml is always the one fixed path, regardless of -c, so an experiment can never affect real volumes/networks (a -c file's own volumes/networks, if any, are simply never read either). A -c path that doesn't exist is a hard error, unlike the default path's existing missing-file leniency. migrate_legacy_config_if_needed() moves volumes/networks out of an old-format config.yaml into persistent.yaml on first run after upgrading, always against the fixed default paths regardless of -c. A name collision aborts the migration for that run (touching neither file) rather than risking data loss. Required reordering main() to parse CLI args before loading config (so -c's value is known first) -- ParsedArgs::log_level_flag_given tracks whether --log-level was already given so the config file's own log-level doesn't clobber it despite the reversed call order. --- CLAUDE.md | 204 +++++++++++---- README.md | 101 ++++++-- src/cli_args.cpp | 42 ++- src/cli_args.h | 23 +- src/commands.cpp | 49 ++-- src/commands.h | 4 + src/config_file.cpp | 240 ++++++++++++++---- src/config_file.h | 71 +++++- src/main.cpp | 52 +++- tests/integration/test_config_bwrap_chain.cpp | 134 +++++++++- tests/unit/test_cli_args.cpp | 26 ++ 11 files changed, 738 insertions(+), 208 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index b5660ed..749f22d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -15,14 +15,30 @@ kernel actually supports. See `README.md` for the human-facing overview (build/u status); this file stays the dense, file-by-file reference. Still early-stage. Source layout (all under `src/`): -- `main.cpp` — the CLI entry point only, and deliberately tiny (~30 lines): - loads the config file, applies its `global.log-level` (`apply_log_level()`, - `cli_args.h`) before CLI parsing so an explicit `--log-level` can still - override it afterward, calls `parse_args()` (`cli_args.h`) and returns its - exit code immediately if it gives one (covers `-h`/`-V` and every parse - error), otherwise calls `dispatch_command()` (`commands.h`) and returns its - result. All of the actual option-parsing and command logic that used to live - here moved out into `cli_args.{h,cpp}`/`commands.{h,cpp}`/`self_test.{h,cpp}` +- `main.cpp` — the CLI entry point only, and deliberately tiny (~45 lines): + calls `parse_args()` (`cli_args.h`) first and returns its exit code + immediately if it gives one (covers `-h`/`-V` and every parse error); + otherwise calls `migrate_legacy_config_if_needed()` (`config_file.h`, see + below) unconditionally, resolves the effective *global* config path + (`args.config_file_flag` if `-c/--config-file` was given, else + `config_file_path()` — a `-c` path that doesn't exist is a hard error here, + unlike the default path's own missing-file leniency), loads that via + `load_global_config()` and the separate, always-fixed + `persistent_file_path()` via `load_persistent_config()`, and merges both + into one `AppConfig`. **Config loading had to move to *after* `parse_args()`** + (it used to run first, specifically so an explicit `--log-level` could + still override the config file's own value by being applied later) since + which file even supplies the global section now depends on `-c`, itself a + CLI flag — this only works without breaking that precedence because + `ParsedArgs::log_level_flag_given` (`cli_args.h`) tracks whether the CLI + already gave `--log-level` (which still applies immediately, in + `parse_args()`, unchanged); `main()` then only applies `config.log_level` + via `apply_log_level()` when that's false, preserving the exact same final + SPDLOG_LEVEL-env → config-file → `--log-level` precedence as before, just + with the config load itself now happening later. Finally calls + `dispatch_command()` (`commands.h`) and returns its result. All of the + actual option-parsing and command logic that used to live here moved out + into `cli_args.{h,cpp}`/`commands.{h,cpp}`/`self_test.{h,cpp}` (see below) specifically to keep this file from re-growing into a dumping ground as more commands (docker-compose support, etc.) get added. - `cli_args.{h,cpp}` — command-line parsing only, nothing else. `Mode` (the @@ -113,6 +129,22 @@ Source layout (all under `src/`): `port_forward.h`, since resolving which network a spec refers to needs runtime join state that doesn't exist yet at parse time), but has no standalone use at all: rejected post-loop unless combined with `-r`. + `-c/--config-file ` reuses `'c'` (freed up when `--cleanup` dropped + its own short form) into `ParsedArgs::config_file_flag` — a plain + `required_argument` flag with no mode interaction at all, always available + regardless of what command is being run, same as `--hostname`/`--user`. + It only ever affects which file `main()` (`main.cpp`, see below) loads for + the *global* section (`config_file.h`'s `load_global_config()`); resolving + it, and deciding whether the resulting `AppConfig`'s own `log_level` + should still be applied, both had to move out of this file and into + `main()`, since they need to know about the loaded config, which + `cli_args.cpp` itself has no dependency on otherwise. What `parse_args()` + *does* still do, unchanged, is apply `--log-level` immediately in its own + case (`apply_log_level()`) — but now also sets a new + `ParsedArgs::log_level_flag_given` bool alongside it, purely so `main()` + can tell afterward whether the CLI already provided one before deciding + whether to also apply the config's own (see `main.cpp`'s own entry for why + this two-flag dance is needed at all). - `commands.{h,cpp}` — every command's implementation, plus the dispatcher. `dispatch_command(args, config_path, config)` (the only externally-linked function; everything else in this file is `.cpp`-local) is a `switch @@ -211,22 +243,29 @@ Source layout (all under `src/`): expected, not a reason to leave a network the user explicitly asked to delete sitting in the config. `write_config_command()` implements `-w/--write-config`: unlike - `create_volume_command()`/`delete_volume_command()`'s use of - `write_config_file()` (which only ever persists `AppConfig` fields that are - already set), this fills in *every* field before writing — the six + `create_volume_command()`/`delete_volume_command()`'s own use of + `write_persistent_config()` (which only ever persists `AppConfig` fields + that are already set, into the separate `persistent.yaml`), this fills in + *every global* field before writing via `write_global_config()` — the six `unshare-*` bools plus `with-veth`/`with-ipv6`, all via `.value_or(true)`, and `log_level` from the actually active `spdlog::get_level()` (not merely a default for when unset — this also captures an explicit `--log-level` passed alongside `-w` on the same - command line, overriding whatever an existing config file's own - `log-level` already was, since `main()`/`parse_args()` already applied it - in that precedence order by the time this runs) — so a bare `-w` bootstraps - a complete, fully-populated config file for hand-editing, and `-w` combined - with other flags captures their effective values into it. `volumes` is left - exactly as loaded — an open-ended list with no "default" entry to - materialize. Prints the config file's full path (`write_config_file()` - already creates the parent directory and the file itself if missing, so no - separate existence check is needed here). + command line, overriding whatever the effective config's own `log-level` + already was, since `main()`/`parse_args()` already applied it in that + precedence order by the time this runs — see `main.cpp`'s own entry above + for the full precedence chain, including how `-c/--config-file` fits in) + — so a bare `-w` bootstraps a complete, fully-populated *global* config + file for hand-editing, and `-w` combined with other flags captures their + effective values into it. `volumes`/`networks` aren't part of `AppConfig`'s + global-only concern from this command's point of view at all anymore — + it never reads or writes `persistent.yaml`. `config_path` (the parameter + this function takes) is `main()`'s already-resolved effective global path + — the default `config.yaml`, or wherever `-c/--config-file` pointed + instead — so `-w` combined with `-c` bootstraps a global config file at + that custom location rather than the default. Prints the file's full path + (`write_global_config()` already creates the parent directory and the + file itself if missing, so no separate existence check is needed here). `run_container()` (the `Mode::run` dispatch case) resolves each `-v` spec (erroring out, `ok = false`, same as a failed `--user` resolution — `bwrap` is skipped but unmount/cleanup still runs) into a `ResolvedVolumeMount`, @@ -2065,28 +2104,53 @@ Source layout (all under `src/`): `EINTR`-retry loop — matching this project's existing direct-POSIX style (`process.cpp` already retries `waitpid()` the same way) — rather than ``/`` (unused anywhere else in this project). -- `config_file.{h,cpp}` — `load_config_file()` reads and parses (via libyaml's - document API, ``) the `global`, `volumes`, and `networks` sections of - the local YAML config file located by `config_file_path()` - (`$XDG_CONFIG_HOME/slocker-lite/config.yaml`, - falling back to `$HOME/.config/slocker-lite/config.yaml`). Supported `global` keys: - `log-level`; six `unshare-` keys (`unshare-user`/`unshare-ipc`/ - `unshare-pid`/`unshare-net`/`unshare-uts`/`unshare-cgroup`, one per - `bwrap.cpp`'s own `namespace_probes` entry) controlling whether `-r/--run` - requests each of bwrap's `--unshare-xxx` flags; and two `with-` keys - (`with-veth`/`with-ipv6`, `AppConfig::with_veth`/`with_ipv6`) giving - `-n/--network`'s own creation-time `veth`/`ipv6` policy (`NetworkEntry`, - above) a persistent default, used whenever the corresponding - `--with-veth`/`--with-ipv6` CLI flag (`cli_args.{h,cpp}`, see above) isn't - given — every other long option is a one-shot flag, not a setting, so it - doesn't belong in a persistent config file. All eight of these boolean keys - share one small `.cpp`-local `BoolGlobalKey {key, field}` pairing table (two - arrays, `unshare_keys` and `network_default_keys`, both consumed by shared - `load_bool_keys()`/`write_bool_keys()` helpers rather than repeating the same - find-parse-or-warn / emit-if-set loop body per group) and accept +- `config_file.{h,cpp}` — reads/writes (via libyaml's document API, + ``) two separate local YAML files, not one: `config_file_path()` + (`$XDG_CONFIG_HOME/slocker-lite/config.yaml`, falling back to + `$HOME/.config/slocker-lite/config.yaml`) holds only the `global` section; + `persistent_file_path()` (same directory, `persistent.yaml`) holds + `volumes`/`networks` — both resolved through one shared `.cpp`-local + `config_dir()` so the two paths can never drift relative to each other. + **Split specifically so `-c/--config-file` (`cli_args.{h,cpp}`) can safely + redirect just the global section for one invocation**: `volumes`/`networks` + are real, provisioned host state (named volumes, live bridges/namespaces), + and letting `-c` touch them too would risk a mistake shadowing or + corrupting real persistent state. `load_global_config(path)` parses only + `path`'s `global` mapping (any `volumes`/`networks` physically present are + never read at all — this is what makes `-c` safe even against an + old-format file that still has them); `load_persistent_config(path)` is + the mirror image, reading only `volumes`/`networks` and ignoring `global`. + Both share one `.cpp`-local `parse_yaml_file(path)` (open + `yaml_parser_load()`; + a missing file yields an empty, root-less document rather than `nullopt`, + so "doesn't exist" and "exists but empty" are indistinguishable to callers + — both already read back as "nothing set"; `nullopt` only for a genuine + parse failure) instead of duplicating that scaffolding twice. + `write_global_config(path, config)`/`write_persistent_config(path, config)` + are the write-side mirror, sharing a `.cpp`-local `write_yaml_document(path, + document)` for the create-directory/open-file/emitter dance — carefully + preserving the exact document-ownership rule libyaml requires + (`yaml_emitter_dump()` consumes/deletes the document itself once + `yaml_emitter_open()` succeeds; a failure before that point means this + function must delete it explicitly instead, exactly as the original single + combined write function already had to). + Supported `global` keys: `log-level`; six `unshare-` keys + (`unshare-user`/`unshare-ipc`/`unshare-pid`/`unshare-net`/`unshare-uts`/ + `unshare-cgroup`, one per `bwrap.cpp`'s own `namespace_probes` entry) + controlling whether `-r/--run` requests each of bwrap's `--unshare-xxx` + flags; and two `with-` keys (`with-veth`/`with-ipv6`, + `AppConfig::with_veth`/`with_ipv6`) giving `-n/--network`'s own + creation-time `veth`/`ipv6` policy (`NetworkEntry`, above) a persistent + default, used whenever the corresponding `--with-veth`/`--with-ipv6` CLI + flag (`cli_args.{h,cpp}`, see above) isn't given — every other long option + is a one-shot flag, not a setting, so it doesn't belong in a persistent + config file. All eight of these boolean keys share one small `.cpp`-local + `BoolGlobalKey {key, field}` pairing table (two arrays, `unshare_keys` and + `network_default_keys`, both consumed by shared + `load_bool_keys()`/`write_bool_keys()` helpers rather than repeating the + same find-parse-or-warn / emit-if-set loop body per group) and accept `"1"`/`"on"`/`"yes"`/`"true"` (enabled) or `"0"`/`"off"`/`"no"`/`"false"` - (disabled), case-insensitively — `parse_bool_flag()`, exported (not just this - file's own internal helper) specifically so `cli_args.cpp`'s own + (disabled), case-insensitively — `parse_bool_flag()`, exported (not just + this file's own internal helper) specifically so `cli_args.cpp`'s own `--with-ipv6`/`--with-veth` value parsing accepts exactly the same forms as the config file itself, rather than a second, drifting copy. An unset key defaults to enabled, and an unrecognized value logs a `spdlog::warn` and is @@ -2096,16 +2160,17 @@ Source layout (all under `src/`): returns a default-constructed (empty) `AppConfig`, not an error; unknown sections/keys (and malformed individual volume entries) are likewise ignored for forward-compatibility. - `main()` applies `config->log_level` (via the existing `apply_log_level()`) - right after `spdlog::cfg::load_env_levels()` and before parsing CLI options, - so an explicit `--log-level` on the command line always overwrites it - afterward — same precedence pattern already used for `SPDLOG_LEVEL`. - `write_config_file()` writes the whole file back out (via libyaml's - document-building/emitter API, symmetric to the read side) — used by - `-v/--volume` (`create_volume_command()`, `commands.cpp`) to persist a new - `VolumeEntry {name, directory}` into the `volumes` section, preserving - `global` (including any set `unshare-*`/`with-veth`/`with-ipv6` keys, - re-serialized as canonical `"true"`/`"false"`) untouched. **`VolumeEntry`/the `volumes` section is a + `main()` (see above) applies the merged `AppConfig`'s own `log_level` (via + the existing `apply_log_level()`) once loaded, but only when + `!args.log_level_flag_given` — see `main.cpp`'s own entry for why config + loading had to move to *after* `parse_args()` (to know `-c`'s value first) + and what that meant for preserving log-level precedence. + `write_persistent_config()` writes `volumes`/`networks` back out — used by + `-v/--volume` (`create_volume_command()`, `commands.cpp`, which now calls + `persistent_file_path()` directly rather than taking a `config_path` + parameter it no longer needs) to persist a new `VolumeEntry {name, + directory}` into the `volumes` section, leaving `config.yaml` completely + untouched. **`VolumeEntry`/the `volumes` section is a distinct concept from `OciImageConfig::volumes`**: this is a user-defined `name -> host directory` mapping created via `-v/--volume`, not an image's own declared mount points (still unconsumed, see `oci_image.{h,cpp}` above). @@ -2128,15 +2193,42 @@ Source layout (all under `src/`): on load, same forward-compatible policy as everything else here. `ipv6` reuses `parse_bool_flag()`, defaulting to `true` (enabled) if absent or unparseable; `subnet6` is only read/written when `ipv6` is true. - `write_config_file()` writes each network as its own nested mapping under - `networks`, `ipv6` re-serialized as canonical `"true"`/`"false"` like the - `unshare-*` keys. `veth` (default `true`) round-trips the same way as + `write_persistent_config()` writes each network as its own nested mapping + under `networks`, `ipv6` re-serialized as canonical `"true"`/`"false"` like + the `unshare-*` keys. `veth` (default `true`) round-trips the same way as `ipv6` (`parse_bool_flag()`, written as canonical `"true"`/`"false"`, always written regardless of value — unlike `subnet6`, there's no companion field whose presence depends on it) — see `network_bridge.h`'s `probe_veth_support()`/`should_use_veth()` above and `--with-veth`/ `global.with-veth` (`cli_args.{h,cpp}`/`config_file.{h,cpp}`) below for - what it controls. + what it controls. `create_network_command()`/`delete_network_command()` + (`commands.cpp`) likewise call `persistent_file_path()` directly now, + dropping the `config_path` parameter they used to take. + + **`migrate_legacy_config_if_needed()`** — the one-time-per-upgrade bridge + from the old single-file format to the split above: reuses + `load_persistent_config()` *against `config_file_path()`* (the global + file's own path) to detect legacy `volumes`/`networks` still sitting + there — that loader only ever reads those two sections, so pointing it at + an old-format `config.yaml` naturally surfaces whatever's left, with no + separate detection logic needed. If found, merges them into + `persistent_file_path()` (a name collision against an already-existing + entry there aborts the *entire* migration for this run, touching neither + file, with a warning identifying the conflict — never silently drops or + overwrites data; expected to be exceedingly rare, since `persistent.yaml` + doesn't exist at all the first time this runs for a given install), then + rewrites `config.yaml` via `write_global_config()` — which, by + construction, never writes `volumes`/`networks` at all, completing the + strip. Logs an info-level summary of what moved. A cheap no-op (single + read, no writes) when `config.yaml` has no legacy data, which is the + common case on every run after the first. **Always operates on the fixed + default paths, regardless of `-c/--config-file`** — migration only ever + concerns the *default* `config.yaml`, never whatever a given invocation's + `-c` points at instead, so an alternate global-only file can never be + mistaken for — or have its own `volumes`/`networks`, if any, migrated + from — the real persistent config. Called unconditionally, early in + `main()`, before resolving which file supplies that invocation's own + global section (see `main.cpp`'s own entry above). - `network_subnet.{h,cpp}` — pure CIDR arithmetic backing `-n/--network`'s subnet allocation and `network_bridge.{h,cpp}`'s (see below) gateway-address computation; no kernel/`ip`/`iptables` calls of its own. `is_valid_network_name()` @@ -2284,7 +2376,7 @@ Build directory is `buildDir/` (already configured). `--user`, `--group`, `--hostname`, `--env`, `--env-file`, `-v/--volume`, `--list-volumes`, `--delete-volume`, `--delete-volume-full`, `-n/--network`, `--extern`, `--intern`, `--subnet`, `--with-ipv6`, `--subnet6`, `--with-veth`, `--list-networks`, `--delete-network`, `-p/--port-forward`, `--no-dns`, `--list-processes`, `--clean-processes`, - `-w/--write-config`, `-t/--test [-- ]`, `--log-level`, + `-c/--config-file`, `-w/--write-config`, `-t/--test [-- ]`, `--log-level`, `-h/--help`, `-V/--version`) - Run tests: `meson test -C buildDir` (the `[unit]` + safe `[integration]` categories only — see `self_test.{h,cpp}`'s own entry above and `README.md`'s "Testing" section diff --git a/README.md b/README.md index d799bfd..0efdab5 100644 --- a/README.md +++ b/README.md @@ -99,7 +99,8 @@ slocker-lite --delete-volume slocker-lite --delete-volume-full slocker-lite --list-processes slocker-lite --clean-processes -slocker-lite -t|--test +slocker-lite [-c|--config-file ] -w|--write-config +slocker-lite -t|--test [-- ] slocker-lite -h|--help slocker-lite -V|--version ``` @@ -132,8 +133,9 @@ slocker-lite -V|--version | `-p, --port-forward [:]:[/tcp\|udp]` | With `--run`, forward a port from the host into the container -- TCP by default, or UDP with an explicit `/udp` suffix. `` is optional, defaulting to the container's sole `--extern` network (an error if it joined more than one without specifying). Repeatable, including the same port pair once per protocol. Reachable via the host's real, externally-facing IP; `localhost`/loopback access has a known NAT-hairpinning limitation, for both protocols (see [`docs/networking-design.md`](docs/networking-design.md)). | | `--list-processes` | List running `--run` sessions found by their pid files under `$XDG_STATE_HOME/slocker-lite/run/`, with their pid, container name, and status (`running` or `exited`). | | `--clean-processes` | Remove stale pid files (see `--list-processes`), and any `-p`/`--port-forward` iptables rules, left behind by sessions that are no longer running (e.g. after a crash). | -| `-w, --write-config` | Write a complete config file (creating it, and its parent directory, if missing), filling in every option's current or default value. Useful to bootstrap one for hand-editing. Prints the config file's full path. | -| `-t, --test` | Run the (currently empty) self-test placeholder. | +| `-c, --config-file ` | Use `` instead of the default `config.yaml` for the *global* section only (`log-level`, `unshare-*`, `with-veth`, `with-ipv6`) — volumes/networks always come from the separate, always-fixed `persistent.yaml` (see Configuration below), never affected by this. Errors if `` doesn't exist (no silent fall-back to defaults, unlike the default path). Combine with `-w` to bootstrap a config file at a custom location. Always available, regardless of command. | +| `-w, --write-config` | Write a complete *global* config file (creating it, and its parent directory, if missing), filling in every global option's current or default value. Useful to bootstrap one for hand-editing. Never touches volumes/networks. Prints the file's full path. | +| `-t, --test [-- ]` | Run the built-in [Catch2](https://github.com/catchorg/Catch2) test suite (see Testing above). | | `--log-level ` | Set log verbosity (`trace`, `debug`, `info`, `warn`, `error`, `critical`, `off`). | | `-h, --help` | Print usage and exit. | | `-V, --version` | Print version information and exit. | @@ -194,16 +196,32 @@ sudo ./buildDir/slocker-lite -r myimage.tar --user git # Remove any leftover, stale pid files ./buildDir/slocker-lite --clean-processes + +# Bootstrap a config file at a custom location (global section only -- +# volumes/networks are unaffected, see Configuration below) +./buildDir/slocker-lite -c ~/alt-slocker.yaml -w + +# Use that alternate file's global settings for one run +./buildDir/slocker-lite -c ~/alt-slocker.yaml -r myimage.tar ``` ## Configuration -Persistent settings can be kept in a local YAML config file at -`$XDG_CONFIG_HOME/slocker-lite/config.yaml` (falling back to -`$HOME/.config/slocker-lite/config.yaml` if `XDG_CONFIG_HOME` isn't set). The file is -organized into sections: +Persistent settings are split across **two** local YAML files, both under +`$XDG_CONFIG_HOME/slocker-lite/` (falling back to +`$HOME/.config/slocker-lite/` if `XDG_CONFIG_HOME` isn't set): + +- **`config.yaml`** — the `global` section only (log verbosity, bwrap + namespace policy, network-creation defaults). This is the file + `-c/--config-file` can point elsewhere for a single invocation. +- **`persistent.yaml`** — the `volumes` and `networks` sections: real, + provisioned host state (named volumes, bridges/namespaces). Always this + one fixed file — `-c/--config-file` never affects it, so an experiment + with an alternate global config can never corrupt or shadow real + volumes/networks. ```yaml +# config.yaml global: log-level: debug unshare-user: on @@ -214,6 +232,10 @@ global: unshare-cgroup: on with-veth: on with-ipv6: on +``` + +```yaml +# persistent.yaml volumes: mydata: /home/user/slocker-volumes/mydata networks: @@ -226,7 +248,8 @@ networks: ``` `global.log-level` sets the default log verbosity (an explicit `--log-level` on -the command line always overrides it). The six `global.unshare-` keys +the command line always overrides it, whether the config came from the default +`config.yaml` or a `-c` override). The six `global.unshare-` keys control whether `-r/--run` requests the matching bwrap `--unshare-xxx` flag (only namespace types the running kernel actually supports are ever affected either way) — each accepts `1`/`on`/`yes`/`true` or `0`/`off`/`no`/`false`, @@ -241,27 +264,49 @@ same "on unless set otherwise" default) give `-n/--network`'s own creation-time always used to exercise the tap+relay fallback can set `with-veth: off` once instead of passing `--with-veth=false` on every network creation — an explicit `--with-veth`/`--with-ipv6` on the command line still overrides it for that one -call. No other long options belong in a config file (one-shot -commands like `--mount`/`--run`/`--user` don't). The `volumes` section is -managed by `-v/--volume` (see above) rather than hand-edited — it's what -`-r/--run`'s own `-v` usage looks named volumes up in. The `networks` section -is likewise managed by `-n/--network` rather than hand-edited — see -[`docs/networking-design.md`](docs/networking-design.md) for the full -persistent-network feature design. `-n/--network` both creates a network -(standing up its real bridge/iptables state, root-only) and, combined with -`-r/--run`, joins a container to one or more of them with a real veth -interface and address on each; `-p/--port-forward` then forwards a host port -into a container on one of its `--extern` networks. A missing config file is -fine either way (nothing is overridden, and one gets created the first time -`-v/--volume`/`-n/--network` is used). +call. No other long options belong in `config.yaml` (one-shot +commands like `--mount`/`--run`/`--user` don't). -Run `-w/--write-config` to bootstrap a config file: it writes out every -supported option explicitly (filling in the current or default value for -anything not already set — so the block above is exactly what a fresh `-w` -produces), creating the file and its parent directory if they don't exist -yet, and prints the file's full path. Combine it with other flags to seed -specific values, e.g. `slocker-lite --log-level debug -w` writes -`log-level: debug`. +`persistent.yaml`'s `volumes` section is managed by `-v/--volume` (see above) +rather than hand-edited — it's what `-r/--run`'s own `-v` usage looks named +volumes up in. Its `networks` section is likewise managed by `-n/--network` +rather than hand-edited — see [`docs/networking-design.md`](docs/networking-design.md) +for the full persistent-network feature design. `-n/--network` both creates a +network (standing up its real bridge/iptables state, root-only) and, combined +with `-r/--run`, joins a container to one or more of them with a real veth +interface and address on each; `-p/--port-forward` then forwards a host port +into a container on one of its `--extern` networks. Missing config files are +fine either way (nothing is overridden, and each file gets created the first +time something needs to write to it). + +**`-c/--config-file `** uses `` instead of the default +`config.yaml` for the *global* section only, for that one invocation — +`persistent.yaml` is always read from its one fixed location regardless, and +if `` itself happens to contain `volumes`/`networks` sections (e.g. an +old-format file reused by mistake), those are simply ignored, never read or +migrated. Unlike the default `config.yaml` (missing = defaults, not an +error), a `-c`-given path that doesn't exist is a hard error, since pointing +at a specific file is a deliberate choice. + +**Migrating from an older single-file `config.yaml`**: versions before this +split kept `volumes`/`networks` directly in `config.yaml` alongside `global`. +The first time a newer build runs against such a file, it automatically +moves any `volumes`/`networks` it finds there into `persistent.yaml` and +rewrites `config.yaml` down to just its `global` section — logged at info +level (`migrated N volume(s) and M network(s) from ... to ...`). This only +ever inspects the *default* `config.yaml`, never a `-c`-given file. A name +collision against an already-existing `persistent.yaml` entry aborts the +migration for that run (neither file is touched) with a warning identifying +the conflict, rather than silently dropping or overwriting anything. + +Run `-w/--write-config` to bootstrap a `config.yaml` (or, combined with `-c`, +an alternate global file at a custom location): it writes out every +supported *global* option explicitly (filling in the current or default +value for anything not already set — so the first block above is exactly +what a fresh `-w` produces), creating the file and its parent directory if +they don't exist yet, and prints the file's full path. It never touches +`persistent.yaml`. Combine it with other flags to seed specific values, e.g. +`slocker-lite --log-level debug -w` writes `log-level: debug`. ## How it works diff --git a/src/cli_args.cpp b/src/cli_args.cpp index caf24c4..909dac3 100644 --- a/src/cli_args.cpp +++ b/src/cli_args.cpp @@ -42,7 +42,11 @@ namespace { // --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, +// is no real loss. 'c', freed up by --cleanup dropping its own short form +// above, is reused here for -c/--config-file (config_file.h's own +// config_file_path()/persistent_file_path() split) -- always available, +// regardless of mode, same as --hostname/--user below. +// --extern/--intern/--with-ipv6/--with-veth (booleans, // each taking an explicit true/false value -- e.g. --with-veth=false -- so // they double as each one's own config-file-default override, see // config_file.h's global.with-ipv6/global.with-veth) and --subnet/--subnet6 @@ -78,10 +82,11 @@ constexpr int umount = 280; constexpr int cleanup = 281; } // namespace options -constexpr std::array long_options = {{ +constexpr std::array long_options = {{ {"help", no_argument, nullptr, 'h'}, {"version", no_argument, nullptr, 'V'}, {"test", no_argument, nullptr, 't'}, + {"config-file", required_argument, nullptr, 'c'}, {"log-level", required_argument, nullptr, options::log_level}, {"mount", required_argument, nullptr, options::mount}, {"umount", required_argument, nullptr, options::umount}, @@ -143,7 +148,7 @@ void print_usage(const char* prog) { " {0} --delete-network-full \n" " {0} --list-processes\n" " {0} --clean-processes\n" - " {0} -w|--write-config\n" + " {0} [-c|--config-file ] -w|--write-config\n" " {0} -t|--test [-- ]\n" " {0} -h|--help\n" " {0} -V|--version\n" @@ -292,11 +297,15 @@ void print_usage(const char* prog) { " --clean-processes remove stale pid files (see --list-processes)\n" " left behind by sessions that are no longer\n" " running\n" - " -w, --write-config write a complete config file (creating it, and\n" - " its parent directory, if missing), filling in\n" - " every option's current or default value --\n" - " useful to bootstrap one for hand-editing.\n" - " Prints the config file's full path\n" + " -w, --write-config write a complete *global* config file (creating\n" + " it, and its parent directory, if missing),\n" + " filling in every global option's current or\n" + " default value -- useful to bootstrap one for\n" + " hand-editing. Combine with -c/--config-file to\n" + " write it somewhere other than the default\n" + " config.yaml. Never touches volumes/networks\n" + " (a separate file -- see -v/--volume,\n" + " -n/--network). Prints the file's full path\n" " -t, --test [-- ]\n" " run the built-in Catch2 test suite. Bare -t\n" " runs everything; select a category with a tag\n" @@ -311,6 +320,17 @@ void print_usage(const char* prog) { " -t -- --help for Catch2's own full option list\n" " (not compiled in if built with\n" " -Denable_tests=false)\n" + " -c, --config-file \n" + " use instead of the default config.yaml\n" + " for the *global* section only (log-level,\n" + " unshare-*, with-veth, with-ipv6) -- volumes/\n" + " networks always come from the separate,\n" + " always-fixed persistent.yaml, never affected\n" + " by this. Errors if doesn't exist, unlike\n" + " the default path's own silent fall-back to\n" + " defaults when missing. Combine with -w to\n" + " bootstrap a config file at a custom location.\n" + " Always available, regardless of command\n" " --log-level set log verbosity (trace, debug, info, warn,\n" " error, critical, off)\n" " -h, --help print this help and exit\n" @@ -373,7 +393,7 @@ std::optional parse_args(int argc, char* argv[], ParsedArgs& out) { optind = 0; opterr = 0; int opt; - while ((opt = getopt_long(argc, argv, ":hVtr:l:v:i:x:Dwn:p:", long_options.data(), nullptr)) != -1) { + while ((opt = getopt_long(argc, argv, ":hVtc:r:l:v:i:x:Dwn:p:", long_options.data(), nullptr)) != -1) { switch (opt) { case 'h': print_usage(argv[0]); @@ -542,6 +562,10 @@ std::optional parse_args(int argc, char* argv[], ParsedArgs& out) { if (!apply_log_level(optarg)) { return 1; } + out.log_level_flag_given = true; + break; + case 'c': + out.config_file_flag = optarg; break; case options::user: out.user_flag = optarg; diff --git a/src/cli_args.h b/src/cli_args.h index 8b654f5..eff1946 100644 --- a/src/cli_args.h +++ b/src/cli_args.h @@ -99,7 +99,7 @@ struct ParsedArgs { // self_test.h), not a command to run, hence its own field rather than // reusing `command` above. A literal '--' before any Catch2 flag that // looks like one of slocker-lite's own short options (Catch2's own - // -r/--reporter collides with -r/--run, -c/--section with -c/--cleanup) + // -r/--reporter collides with -r/--run, -c/--section with -c/--config-file) // is required for the same reason -x/--exec's own trailing command // needs one for a command starting with '-'. std::vector test_args; @@ -107,13 +107,30 @@ struct ParsedArgs { std::optional exec_pid; // Parsed and range-validated from mode_arg when mode == Mode::kill. std::optional kill_pid; + // -c/--config-file : use this file instead of the default + // config_file_path() (config_file.h) as the source of the *global* + // section only for this invocation -- volumes/networks always come from + // the separate, always-fixed persistent_file_path(), regardless of this + // flag, so an experiment with -c can never affect real persistent state. + // Always available, regardless of mode (like --hostname/--user). + std::optional config_file_flag; + // Set alongside apply_log_level()'s own immediate call in --log-level's + // case, purely so main() can tell whether the CLI already gave an + // explicit log level before deciding whether to also apply the + // (possibly -c-sourced) global config's own log-level -- see + // apply_log_level()'s own doc comment below for why this matters. + bool log_level_flag_given = false; }; // Validates and applies a log-level name (trace/debug/info/warn/error/critical/off) // via spdlog::set_level(). Returns false (logging an error) if `name` isn't // recognized. Exposed (not parse_args()-internal) because main() also applies it -// directly for the config file's global.log-level, before parse_args() runs, so -// an explicit --log-level on the command line can still override it afterward. +// directly for the effective global config's own log-level, once loaded -- +// which happens *after* parse_args() runs (since -c/--config-file, above, +// decides which file that even is), unlike --log-level's own immediate +// in-loop apply below. To keep the CLI's own precedence over the config file +// despite that reversed call order, main() only applies the config's +// log-level when `!log_level_flag_given` -- see that field's own doc comment. bool apply_log_level(std::string_view name); // Parses argv via getopt_long() into `out`, including all of the post-loop diff --git a/src/commands.cpp b/src/commands.cpp index 758f379..f4e4c55 100644 --- a/src/commands.cpp +++ b/src/commands.cpp @@ -245,8 +245,7 @@ int inspect_image_command(const std::filesystem::path& image_tar) { return 0; } -int create_volume_command(const std::string& name, const std::string& directory, - const std::filesystem::path& config_path, AppConfig& config) { +int create_volume_command(const std::string& name, const std::string& directory, AppConfig& config) { if (!is_valid_volume_name(name)) { spdlog::error("volume name '{}' must not contain '/'", name); return 1; @@ -281,7 +280,7 @@ int create_volume_command(const std::string& name, const std::string& directory, } config.volumes.push_back({name, resolved.string()}); - if (!write_config_file(config_path, config)) { + if (!write_persistent_config(persistent_file_path(), config)) { return 1; } @@ -352,8 +351,7 @@ int clean_processes_command() { return 0; } -int delete_volume_command(const std::string& name, const std::filesystem::path& config_path, - AppConfig& config, bool delete_directory) { +int delete_volume_command(const std::string& name, AppConfig& config, bool delete_directory) { auto it = std::find_if(config.volumes.begin(), config.volumes.end(), [&](const VolumeEntry& volume) { return volume.name == name; }); if (it == config.volumes.end()) { @@ -374,7 +372,7 @@ int delete_volume_command(const std::string& name, const std::filesystem::path& } config.volumes.erase(it); - if (!write_config_file(config_path, config)) { + if (!write_persistent_config(persistent_file_path(), config)) { return 1; } @@ -391,8 +389,7 @@ int delete_volume_command(const std::string& name, const std::filesystem::path& // docs/networking-design.md's commit sequence; that lands in a later commit). int create_network_command(const std::string& name, NetworkKind kind, const std::optional& subnet_override, bool ipv6, - const std::optional& subnet6_override, bool veth, - const std::filesystem::path& config_path, AppConfig& config) { + const std::optional& subnet6_override, bool veth, AppConfig& config) { if (!is_valid_network_name(name)) { spdlog::error("network name '{}' must not contain ':'", name); return 1; @@ -459,7 +456,7 @@ int create_network_command(const std::string& name, NetworkKind kind, } config.networks.push_back(entry); - if (!write_config_file(config_path, config)) { + if (!write_persistent_config(persistent_file_path(), config)) { return 1; } @@ -515,8 +512,7 @@ int list_networks_command(const AppConfig& config) { return 0; } -int delete_network_command(const std::string& name, const std::filesystem::path& config_path, AppConfig& config, - bool delete_full) { +int delete_network_command(const std::string& name, AppConfig& config, bool delete_full) { auto it = std::find_if(config.networks.begin(), config.networks.end(), [&](const NetworkEntry& network) { return network.name == name; }); if (it == config.networks.end()) { @@ -533,7 +529,7 @@ int delete_network_command(const std::string& name, const std::filesystem::path& } config.networks.erase(it); - if (!write_config_file(config_path, config)) { + if (!write_persistent_config(persistent_file_path(), config)) { return 1; } @@ -541,12 +537,15 @@ int delete_network_command(const std::string& name, const std::filesystem::path& return 0; } -// Writes out every supported config option explicitly, defaulting anything +// Writes out every supported *global* option explicitly, defaulting anything // currently unset to its effective value, creating the file (and its parent // directory) if it doesn't exist yet -- unlike create_volume_command()/ -// delete_volume_command()'s use of write_config_file(), which only ever -// persists fields the user actually set. Meant to bootstrap a complete config -// file for hand-editing. +// delete_volume_command()'s own use of write_persistent_config(), which only +// ever persists fields the user actually set. `config_path` is the effective +// global path for this invocation -- the default config.yaml, or whatever +// -c/--config-file (cli_args.h) pointed at instead; volumes/networks are a +// separate, always-fixed file this command never touches at all. Meant to +// bootstrap a complete global config file for hand-editing. int write_config_command(const std::filesystem::path& config_path, const AppConfig& config) { AppConfig full = config; // Always the actually active spdlog level -- not merely a default for when @@ -569,11 +568,8 @@ int write_config_command(const std::filesystem::path& config_path, const AppConf full.unshare_cgroup = full.unshare_cgroup.value_or(true); full.with_veth = full.with_veth.value_or(true); full.with_ipv6 = full.with_ipv6.value_or(true); - // `volumes` is left exactly as loaded -- an open-ended list with no - // "default" entry to materialize, unlike the eight fixed boolean flags - // above. - if (!write_config_file(config_path, full)) { + if (!write_global_config(config_path, full)) { return 1; } fmt::print("{}\n", config_path.string()); @@ -881,14 +877,13 @@ int dispatch_command(const ParsedArgs& args, const std::filesystem::path& config case Mode::inspect: return inspect_image_command(args.mode_arg); case Mode::volume: - return create_volume_command(args.volume_specs.front().first, args.volume_specs.front().second, - config_path, config); + return create_volume_command(args.volume_specs.front().first, args.volume_specs.front().second, config); case Mode::list_volumes: return list_volumes_command(config); case Mode::delete_volume: - return delete_volume_command(args.mode_arg, config_path, config, false); + return delete_volume_command(args.mode_arg, config, false); case Mode::delete_volume_full: - return delete_volume_command(args.mode_arg, config_path, config, true); + return delete_volume_command(args.mode_arg, config, true); case Mode::list_processes: return list_processes_command(); case Mode::clean_processes: @@ -910,14 +905,14 @@ int dispatch_command(const ParsedArgs& args, const std::filesystem::path& config bool ipv6 = args.network_with_ipv6_flag.value_or(config.with_ipv6.value_or(true)); bool veth = args.network_with_veth_flag.value_or(config.with_veth.value_or(true)); return create_network_command(args.network_specs.front(), kind, args.network_subnet_flag, ipv6, - args.network_subnet6_flag, veth, config_path, config); + args.network_subnet6_flag, veth, config); } case Mode::list_networks: return list_networks_command(config); case Mode::delete_network: - return delete_network_command(args.mode_arg, config_path, config, false); + return delete_network_command(args.mode_arg, config, false); case Mode::delete_network_full: - return delete_network_command(args.mode_arg, config_path, config, true); + return delete_network_command(args.mode_arg, config, true); case Mode::run: { // As root, containers-storage mount doesn't need to reexec into a private // user namespace to gain privilege, so the mount is already directly diff --git a/src/commands.h b/src/commands.h index 531d095..2675882 100644 --- a/src/commands.h +++ b/src/commands.h @@ -24,4 +24,8 @@ // Runs whichever command args.mode selects (an exhaustive switch over Mode -- // see commands.cpp -- so a Mode value ever added without a matching case // triggers a -Wswitch warning/error rather than silently falling through). +// `config_path` is the effective *global* config path for this invocation -- +// the default config.yaml, or wherever -c/--config-file (cli_args.h) +// pointed instead; volumes/networks always live at the separate, always-fixed +// persistent_file_path() (config_file.h), never affected by config_path. int dispatch_command(const ParsedArgs& args, const std::filesystem::path& config_path, AppConfig& config); diff --git a/src/config_file.cpp b/src/config_file.cpp index 0727c64..0108d04 100644 --- a/src/config_file.cpp +++ b/src/config_file.cpp @@ -122,6 +122,97 @@ void write_bool_keys(yaml_document_t& document, int global_mapping, const AppCon } } +// Shared directory resolution for config_file_path()/persistent_file_path() +// -- factored out so the two paths can never drift relative to each other +// (both are just a different filename under the same directory). +std::filesystem::path config_dir() { + const char* xdg_config_home = std::getenv("XDG_CONFIG_HOME"); + std::filesystem::path config_home; + if (xdg_config_home && *xdg_config_home) { + config_home = xdg_config_home; + } else { + config_home = resolve_home_dir() / ".config"; + } + return config_home / "slocker-lite"; +} + +// Opens and parses `path` into a fresh yaml_document_t -- shared scaffolding +// for load_global_config()/load_persistent_config(), which each then only +// look at their own section(s) of the resulting document. A missing file +// yields an empty (root-less) document rather than nullopt, so callers can +// treat "file doesn't exist" and "file exists but is empty" identically -- +// both simply have no root node, the same condition a section lookup below +// already treats as "nothing set". nullopt only for a genuine YAML parse +// failure (logged here). +std::optional parse_yaml_file(const std::filesystem::path& path) { + FILE* file = std::fopen(path.c_str(), "r"); + if (!file) { + yaml_document_t empty; + yaml_document_initialize(&empty, nullptr, nullptr, nullptr, 1, 1); + return empty; + } + + yaml_parser_t parser; + yaml_parser_initialize(&parser); + yaml_parser_set_input_file(&parser, file); + + yaml_document_t document; + bool loaded = yaml_parser_load(&parser, &document) != 0; + yaml_parser_delete(&parser); + std::fclose(file); + + if (!loaded) { + spdlog::error("failed to parse config file {}", path.string()); + return std::nullopt; + } + return document; +} + +// Shared write-side scaffolding for write_global_config()/ +// write_persistent_config(): creates path's parent directory, dumps +// `document` to it via libyaml's emitter, and reports success/failure -- +// mirrors the exact same document-ownership dance the single combined +// write function used to do (yaml_emitter_dump() consumes/deletes +// `document` itself once yaml_emitter_open() succeeds; a failure before +// that point means this function must delete it explicitly instead). +bool write_yaml_document(const std::filesystem::path& path, yaml_document_t& document) { + std::error_code ec; + std::filesystem::create_directories(path.parent_path(), ec); + if (ec) { + spdlog::error("failed to create directory {}: {}", path.parent_path().string(), ec.message()); + yaml_document_delete(&document); + return false; + } + + FILE* file = std::fopen(path.c_str(), "w"); + if (!file) { + spdlog::error("failed to open {} for writing", path.string()); + yaml_document_delete(&document); + return false; + } + + yaml_emitter_t emitter; + yaml_emitter_initialize(&emitter); + yaml_emitter_set_output_file(&emitter, file); + + bool ok = yaml_emitter_open(&emitter) != 0; + if (!ok) { + yaml_document_delete(&document); + } else { + ok = yaml_emitter_dump(&emitter, &document) != 0; + ok = yaml_emitter_close(&emitter) != 0 && ok; + } + + yaml_emitter_delete(&emitter); + std::fclose(file); + + if (!ok) { + spdlog::error("failed to write config file {}", path.string()); + return false; + } + return true; +} + } // namespace // Accepts the usual truthy/falsy string forms, case-insensitively. Returns @@ -143,44 +234,27 @@ std::optional parse_bool_flag(std::string_view value) { } std::filesystem::path config_file_path() { - const char* xdg_config_home = std::getenv("XDG_CONFIG_HOME"); - std::filesystem::path config_home; - if (xdg_config_home && *xdg_config_home) { - config_home = xdg_config_home; - } else { - config_home = resolve_home_dir() / ".config"; - } // See xdg_state_dir()'s own comment (pid_file.cpp) for why this is // defensively made absolute regardless of which piece might still be // relative -- the same real bug (two slocker-lite invocations from // different working directories getting two completely disjoint // config.yaml files, silently) applies here identically. - return std::filesystem::absolute(config_home / "slocker-lite" / "config.yaml"); + return std::filesystem::absolute(config_dir() / "config.yaml"); } -std::optional load_config_file(const std::filesystem::path& path) { - FILE* file = std::fopen(path.c_str(), "r"); - if (!file) { - return AppConfig{}; - } +std::filesystem::path persistent_file_path() { + return std::filesystem::absolute(config_dir() / "persistent.yaml"); +} - yaml_parser_t parser; - yaml_parser_initialize(&parser); - yaml_parser_set_input_file(&parser, file); - - yaml_document_t document; - bool loaded = yaml_parser_load(&parser, &document) != 0; - yaml_parser_delete(&parser); - std::fclose(file); - - if (!loaded) { - spdlog::error("failed to parse config file {}", path.string()); +std::optional load_global_config(const std::filesystem::path& path) { + auto parsed = parse_yaml_file(path); + if (!parsed) { return std::nullopt; } + yaml_document_t& document = *parsed; AppConfig config; - yaml_node_t* root = yaml_document_get_root_node(&document); - if (root) { + if (yaml_node_t* root = yaml_document_get_root_node(&document)) { if (const yaml_node_t* global = find_in_mapping(document, *root, "global")) { if (const yaml_node_t* log_level = find_in_mapping(document, *global, "log-level")) { if (log_level->type == YAML_SCALAR_NODE) { @@ -190,6 +264,21 @@ std::optional load_config_file(const std::filesystem::path& path) { load_bool_keys(document, *global, config, unshare_keys); load_bool_keys(document, *global, config, network_default_keys); } + } + + yaml_document_delete(&document); + return config; +} + +std::optional load_persistent_config(const std::filesystem::path& path) { + auto parsed = parse_yaml_file(path); + if (!parsed) { + return std::nullopt; + } + yaml_document_t& document = *parsed; + + AppConfig config; + if (yaml_node_t* root = yaml_document_get_root_node(&document)) { if (const yaml_node_t* volumes = find_in_mapping(document, *root, "volumes")) { if (volumes->type == YAML_MAPPING_NODE) { for (auto* pair = volumes->data.mapping.pairs.start; @@ -271,17 +360,9 @@ std::optional load_config_file(const std::filesystem::path& path) { return config; } -bool write_config_file(const std::filesystem::path& path, const AppConfig& config) { - std::error_code ec; - std::filesystem::create_directories(path.parent_path(), ec); - if (ec) { - spdlog::error("failed to create directory {}: {}", path.parent_path().string(), ec.message()); - return false; - } - +bool write_global_config(const std::filesystem::path& path, const AppConfig& config) { yaml_document_t document; yaml_document_initialize(&document, nullptr, nullptr, nullptr, 1, 1); - int root = add_mapping(document); auto has_bool_override = [&](const auto& keys) { @@ -299,6 +380,14 @@ bool write_config_file(const std::filesystem::path& path, const AppConfig& confi yaml_document_append_mapping_pair(&document, root, add_scalar(document, "global"), global); } + return write_yaml_document(path, document); +} + +bool write_persistent_config(const std::filesystem::path& path, const AppConfig& config) { + yaml_document_t document; + yaml_document_initialize(&document, nullptr, nullptr, nullptr, 1, 1); + int root = add_mapping(document); + if (!config.volumes.empty()) { int volumes = add_mapping(document); for (const auto& volume : config.volumes) { @@ -330,33 +419,74 @@ bool write_config_file(const std::filesystem::path& path, const AppConfig& confi yaml_document_append_mapping_pair(&document, root, add_scalar(document, "networks"), networks); } - FILE* file = std::fopen(path.c_str(), "w"); - if (!file) { - spdlog::error("failed to open {} for writing", path.string()); - yaml_document_delete(&document); + return write_yaml_document(path, document); +} + +bool migrate_legacy_config_if_needed() { + // Reusing the persistent-section loader *against the global file's own + // path* is exactly how this detects legacy data: it only ever reads + // volumes/networks, so pointing it at an old-format config.yaml + // naturally surfaces whatever's still there. + auto legacy = load_persistent_config(config_file_path()); + if (!legacy) { + return false; + } + if (legacy->volumes.empty() && legacy->networks.empty()) { + return true; // nothing to migrate -- the common case on every run + } + + auto persistent = load_persistent_config(persistent_file_path()); + if (!persistent) { return false; } - yaml_emitter_t emitter; - yaml_emitter_initialize(&emitter); - yaml_emitter_set_output_file(&emitter, file); - - bool ok = yaml_emitter_open(&emitter) != 0; - if (!ok) { - // yaml_emitter_dump() below is what normally consumes/destroys `document` -- - // since open failed and dump never runs, it must be deleted explicitly here. - yaml_document_delete(&document); - } else { - ok = yaml_emitter_dump(&emitter, &document) != 0; - ok = yaml_emitter_close(&emitter) != 0 && ok; + // Never silently drop or overwrite data: a name collision aborts the + // whole migration for this run (neither file is touched), rather than + // skipping just the colliding entry -- expected to be vanishingly rare, + // since persistent.yaml doesn't exist at all the first time this runs + // for a given install. + for (const auto& volume : legacy->volumes) { + if (std::any_of(persistent->volumes.begin(), persistent->volumes.end(), + [&](const VolumeEntry& v) { return v.name == volume.name; })) { + spdlog::warn( + "config file migration: volume '{}' already exists in {}; leaving the legacy copy in {} " + "untouched -- resolve the name collision by hand", + volume.name, persistent_file_path().string(), config_file_path().string()); + return true; + } + } + for (const auto& network : legacy->networks) { + if (std::any_of(persistent->networks.begin(), persistent->networks.end(), + [&](const NetworkEntry& n) { return n.name == network.name; })) { + spdlog::warn( + "config file migration: network '{}' already exists in {}; leaving the legacy copy in {} " + "untouched -- resolve the name collision by hand", + network.name, persistent_file_path().string(), config_file_path().string()); + return true; + } } - yaml_emitter_delete(&emitter); - std::fclose(file); + size_t migrated_volumes = legacy->volumes.size(); + size_t migrated_networks = legacy->networks.size(); + persistent->volumes.insert(persistent->volumes.end(), legacy->volumes.begin(), legacy->volumes.end()); + persistent->networks.insert(persistent->networks.end(), legacy->networks.begin(), legacy->networks.end()); - if (!ok) { - spdlog::error("failed to write config file {}", path.string()); + if (!write_persistent_config(persistent_file_path(), *persistent)) { return false; } + + // Rewriting via write_global_config() is what actually strips + // volumes/networks out of the old-format file -- it never writes them + // regardless of what's still in memory here. + auto global_only = load_global_config(config_file_path()); + if (!global_only) { + return false; + } + if (!write_global_config(config_file_path(), *global_only)) { + return false; + } + + spdlog::info("migrated {} volume(s) and {} network(s) from {} to {}", migrated_volumes, migrated_networks, + config_file_path().string(), persistent_file_path().string()); return true; } diff --git a/src/config_file.h b/src/config_file.h index b6c84d7..ee78901 100644 --- a/src/config_file.h +++ b/src/config_file.h @@ -100,18 +100,67 @@ struct AppConfig { }; // $XDG_CONFIG_HOME/slocker-lite/config.yaml, or $HOME/.config/slocker-lite/config.yaml -// if XDG_CONFIG_HOME is unset/empty. +// if XDG_CONFIG_HOME is unset/empty. Holds only the "global" section -- see +// persistent_file_path() for "volumes"/"networks". This is the file +// -c/--config-file (cli_args.h) can point elsewhere for a single invocation; +// persistent_file_path() never can, precisely so a -c experiment can't ever +// affect real volumes/networks. std::filesystem::path config_file_path(); -// Loads and parses `path`'s "global", "volumes", and "networks" sections. A -// missing file is not an error -- returns a default-constructed AppConfig -// (nothing set). Unknown sections/keys (and malformed individual volume/network -// entries -- e.g. a network with an unrecognized `kind`) are ignored, so the -// format stays forward-compatible. Malformed YAML syntax logs a specific error and -// returns nullopt. -std::optional load_config_file(const std::filesystem::path& path); +// $XDG_CONFIG_HOME/slocker-lite/persistent.yaml (same directory/fallback +// resolution as config_file_path(), factored into one shared .cpp-local +// helper so the two paths can never drift relative to each other). Holds the +// "volumes"/"networks" sections that used to live in config.yaml before it +// was split in two -- always this fixed path, with no override of any kind. +std::filesystem::path persistent_file_path(); -// Writes `config` back to `path` as YAML (global + volumes + networks sections), -// creating `path`'s parent directory if needed. Rewrites the whole file. Logs a +// Loads and parses only `path`'s "global" section into a fresh AppConfig -- +// any "volumes"/"networks" section physically present in `path` is not read +// at all. This is what makes -c/--config-file (cli_args.h) safe to point at +// an arbitrary file: even an old-format file with volumes/networks embedded +// has that content completely invisible to this loader, never mistaken for +// -- or migrated from -- the real persistent config. A missing file is not +// an error -- returns a default-constructed AppConfig. Malformed YAML syntax +// logs a specific error and returns nullopt. +std::optional load_global_config(const std::filesystem::path& path); + +// Loads and parses only `path`'s "volumes"/"networks" sections into a fresh +// AppConfig -- any "global" section present is ignored. Same missing-file/ +// malformed-YAML semantics as load_global_config(). Unknown keys and +// malformed individual volume/network entries (e.g. a network with an +// unrecognized `kind`) are ignored, so the format stays forward-compatible. +std::optional load_persistent_config(const std::filesystem::path& path); + +// Writes only `config`'s global fields to `path` as YAML, creating `path`'s +// parent directory if needed. Rewrites the whole file -- any +// "volumes"/"networks" section physically present in an existing file at +// `path` is dropped, which is also the mechanism +// migrate_legacy_config_if_needed() uses to strip them out of an old-format +// config.yaml once their content has been copied to persistent.yaml. Logs a // specific error and returns false on failure. -bool write_config_file(const std::filesystem::path& path, const AppConfig& config); +bool write_global_config(const std::filesystem::path& path, const AppConfig& config); + +// Writes only `config`'s "volumes"/"networks" fields to `path` as YAML, +// creating `path`'s parent directory if needed. Rewrites the whole file. +// Logs a specific error and returns false on failure. +bool write_persistent_config(const std::filesystem::path& path, const AppConfig& config); + +// One-time-per-upgrade maintenance step: if the *default* config.yaml +// (config_file_path() -- never a -c/--config-file override; migration only +// ever concerns the fixed default paths, regardless of what a given +// invocation's -c points at) still has a legacy "volumes" or "networks" +// section (from before this file was split in two), moves those entries +// into persistent_file_path(), then rewrites config.yaml with only its +// global section. A name collision against an already-existing +// persistent.yaml entry aborts the whole migration for this run (neither +// file is touched) with a clear warning identifying the conflict, rather +// than silently dropping or overwriting data -- expected to be exceedingly +// rare in practice, since persistent.yaml doesn't exist at all the first +// time this runs for a given install. A cheap no-op when config.yaml has no +// legacy volumes/networks at all. Meant to be called once, unconditionally, +// early in main() -- before resolving which file actually supplies the +// current invocation's own global section, so a first run after upgrading +// already sees the migrated persistent.yaml. Returns false only on a +// genuine read/parse/write failure; logs an info-level summary of what was +// migrated, if anything. +bool migrate_legacy_config_if_needed(); diff --git a/src/main.cpp b/src/main.cpp index 3207985..9bc4fa5 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -26,19 +26,51 @@ int main(int argc, char* argv[]) { spdlog::cfg::load_env_levels(); - std::filesystem::path config_path = config_file_path(); - auto config = load_config_file(config_path); - if (!config) { - return 1; - } - if (config->log_level) { - apply_log_level(*config->log_level); - } - ParsedArgs args; if (auto exit_code = parse_args(argc, argv, args)) { return *exit_code; } - return dispatch_command(args, config_path, *config); + // Always operates on the fixed default paths, regardless of + // args.config_file_flag below -- see its own doc comment (config_file.h). + if (!migrate_legacy_config_if_needed()) { + return 1; + } + + // -c/--config-file only ever replaces the *global* section's source file + // -- volumes/networks always come from the separate, always-fixed + // persistent_file_path(), so a -c experiment can never affect them. + std::filesystem::path config_path = + args.config_file_flag ? std::filesystem::absolute(*args.config_file_flag) : config_file_path(); + if (args.config_file_flag && !std::filesystem::exists(config_path)) { + // Unlike the default config.yaml (missing == defaults, not an + // error), an explicit -c override is deliberate -- a typo in the + // path should surface clearly rather than silently falling back. + spdlog::error("config file not found: {}", config_path.string()); + return 1; + } + auto global = load_global_config(config_path); + if (!global) { + return 1; + } + + auto persistent = load_persistent_config(persistent_file_path()); + if (!persistent) { + return 1; + } + + AppConfig config = *global; + config.volumes = std::move(persistent->volumes); + config.networks = std::move(persistent->networks); + + // The CLI's own --log-level (applied immediately, in parse_args() above) + // must keep winning over the config file's -- since which file even + // supplies "the config file" (config_path, above) is only known after + // parse_args() has already run, this can't simply run before it the way + // it used to. See ParsedArgs::log_level_flag_given's own doc comment. + if (!args.log_level_flag_given && config.log_level) { + apply_log_level(*config.log_level); + } + + return dispatch_command(args, config_path, config); } diff --git a/tests/integration/test_config_bwrap_chain.cpp b/tests/integration/test_config_bwrap_chain.cpp index 94b00da..97ce256 100644 --- a/tests/integration/test_config_bwrap_chain.cpp +++ b/tests/integration/test_config_bwrap_chain.cpp @@ -23,6 +23,7 @@ // actually accessed. #include +#include #include #include @@ -47,9 +48,9 @@ TEST_CASE("config file -> NamespaceConfig -> bwrap argv: disabled namespaces are AppConfig written; written.unshare_net = false; written.unshare_uts = false; - REQUIRE(write_config_file(config_path, written)); + REQUIRE(write_global_config(config_path, written)); - auto loaded = load_config_file(config_path); + auto loaded = load_global_config(config_path); REQUIRE(loaded.has_value()); CHECK(loaded->unshare_net == std::optional(false)); CHECK(loaded->unshare_uts == std::optional(false)); @@ -79,10 +80,10 @@ TEST_CASE("config file -> NamespaceConfig -> bwrap argv: default (unset) config ScratchXdgDirs scratch; auto config_path = scratch.path() / "config.yaml"; - // Nothing set -- write_config_file()/load_config_file() round-trip an - // otherwise-empty AppConfig, so every unshare-* key comes back unset. - REQUIRE(write_config_file(config_path, AppConfig{})); - auto loaded = load_config_file(config_path); + // Nothing set -- write_global_config()/load_global_config() round-trip + // an otherwise-empty AppConfig, so every unshare-* key comes back unset. + REQUIRE(write_global_config(config_path, AppConfig{})); + auto loaded = load_global_config(config_path); REQUIRE(loaded.has_value()); CHECK_FALSE(loaded->unshare_net.has_value()); @@ -110,9 +111,9 @@ TEST_CASE("config file -> AppConfig: global.with-veth/with-ipv6 round-trip", "[i AppConfig written; written.with_veth = false; written.with_ipv6 = false; - REQUIRE(write_config_file(config_path, written)); + REQUIRE(write_global_config(config_path, written)); - auto loaded = load_config_file(config_path); + auto loaded = load_global_config(config_path); REQUIRE(loaded.has_value()); CHECK(loaded->with_veth == std::optional(false)); CHECK(loaded->with_ipv6 == std::optional(false)); @@ -120,8 +121,123 @@ TEST_CASE("config file -> AppConfig: global.with-veth/with-ipv6 round-trip", "[i // Same "unset means enabled" convention as the six unshare-* keys -- // create_network_command()'s own resolution (commands.cpp) is // `args.network_with_*_flag.value_or(config.with_*.value_or(true))`. - auto loaded_empty = load_config_file(scratch.path() / "nonexistent.yaml"); + auto loaded_empty = load_global_config(scratch.path() / "nonexistent.yaml"); REQUIRE(loaded_empty.has_value()); CHECK_FALSE(loaded_empty->with_veth.has_value()); CHECK_FALSE(loaded_empty->with_ipv6.has_value()); } + +TEST_CASE("persistent file -> AppConfig: volumes/networks round-trip, global section ignored", + "[integration]") { + ScratchXdgDirs scratch; + auto persistent_path = scratch.path() / "persistent.yaml"; + + AppConfig written; + written.log_level = "debug"; // global-only field -- must never reach persistent.yaml + written.volumes.push_back({"myvol", "/home/user/myvol"}); + written.networks.push_back({"mynet", NetworkKind::extern_, "10.168.0.0/24", true, "fdf0::/64", true}); + REQUIRE(write_persistent_config(persistent_path, written)); + + auto loaded = load_persistent_config(persistent_path); + REQUIRE(loaded.has_value()); + REQUIRE(loaded->volumes.size() == 1); + CHECK(loaded->volumes[0].name == "myvol"); + CHECK(loaded->volumes[0].directory == "/home/user/myvol"); + REQUIRE(loaded->networks.size() == 1); + CHECK(loaded->networks[0].name == "mynet"); + CHECK(loaded->networks[0].subnet == "10.168.0.0/24"); + // write_persistent_config() never writes a "global" mapping at all, so + // there's nothing for load_persistent_config() to (deliberately) ignore + // here -- confirmed via load_global_config() against the same file + // instead, below. + CHECK_FALSE(loaded->log_level.has_value()); + + auto global_view = load_global_config(persistent_path); + REQUIRE(global_view.has_value()); + CHECK_FALSE(global_view->log_level.has_value()); +} + +TEST_CASE("migrate_legacy_config_if_needed: moves volumes/networks out of an old-format config.yaml", + "[integration]") { + ScratchXdgDirs scratch; + + // Hand-write an old-format single-file config.yaml -- global + volumes + + // networks all combined, exactly the pre-split shape -- directly at the + // real default config_file_path() (ScratchXdgDirs already points + // XDG_CONFIG_HOME here for this test's lifetime, so this doesn't touch + // the real developer's own config). + auto config_path = config_file_path(); + std::filesystem::create_directories(config_path.parent_path()); + { + std::ofstream legacy(config_path); + legacy << "global:\n" + " log-level: debug\n" + "volumes:\n" + " myvol: /home/user/myvol\n" + "networks:\n" + " mynet:\n" + " kind: extern\n" + " subnet: 10.168.0.0/24\n" + " ipv6: true\n" + " subnet6: fdf0::/64\n" + " veth: true\n"; + } + + REQUIRE(migrate_legacy_config_if_needed()); + + auto persistent = load_persistent_config(persistent_file_path()); + REQUIRE(persistent.has_value()); + REQUIRE(persistent->volumes.size() == 1); + CHECK(persistent->volumes[0].name == "myvol"); + REQUIRE(persistent->networks.size() == 1); + CHECK(persistent->networks[0].name == "mynet"); + + // config.yaml itself is rewritten global-only -- the legacy + // volumes/networks are gone from it, but the global section survives. + auto remaining_global = load_global_config(config_path); + REQUIRE(remaining_global.has_value()); + CHECK(remaining_global->log_level == std::optional("debug")); + auto remaining_persistent_view = load_persistent_config(config_path); + REQUIRE(remaining_persistent_view.has_value()); + CHECK(remaining_persistent_view->volumes.empty()); + CHECK(remaining_persistent_view->networks.empty()); + + // A second run is a clean no-op -- nothing left to migrate. + REQUIRE(migrate_legacy_config_if_needed()); + auto persistent_again = load_persistent_config(persistent_file_path()); + REQUIRE(persistent_again.has_value()); + CHECK(persistent_again->volumes.size() == 1); + CHECK(persistent_again->networks.size() == 1); +} + +TEST_CASE("migrate_legacy_config_if_needed: a name collision aborts the migration, touching neither file", + "[integration]") { + ScratchXdgDirs scratch; + + AppConfig existing_persistent; + existing_persistent.volumes.push_back({"myvol", "/already/here"}); + REQUIRE(write_persistent_config(persistent_file_path(), existing_persistent)); + + AppConfig legacy; + legacy.volumes.push_back({"myvol", "/legacy/path"}); + REQUIRE(write_global_config(config_file_path(), AppConfig{})); // an existing global section + // write_persistent_config() targets persistent_file_path(), not + // config_file_path() -- to plant "legacy" volumes/networks directly in + // config.yaml the way an old-format file would have them, write there + // explicitly instead. + REQUIRE(write_persistent_config(config_file_path(), legacy)); + + REQUIRE(migrate_legacy_config_if_needed()); // false only on a genuine I/O error -- a collision just warns + + // Neither file was touched: the legacy copy is still in config.yaml, + // and persistent.yaml's own pre-existing entry is unchanged. + auto still_legacy = load_persistent_config(config_file_path()); + REQUIRE(still_legacy.has_value()); + REQUIRE(still_legacy->volumes.size() == 1); + CHECK(still_legacy->volumes[0].directory == "/legacy/path"); + + auto still_persistent = load_persistent_config(persistent_file_path()); + REQUIRE(still_persistent.has_value()); + REQUIRE(still_persistent->volumes.size() == 1); + CHECK(still_persistent->volumes[0].directory == "/already/here"); +} diff --git a/tests/unit/test_cli_args.cpp b/tests/unit/test_cli_args.cpp index 7591acb..fa799d2 100644 --- a/tests/unit/test_cli_args.cpp +++ b/tests/unit/test_cli_args.cpp @@ -223,3 +223,29 @@ TEST_CASE("parse_args: --kill requires a numeric pid", "[unit]") { REQUIRE(bad.exit_code.has_value()); CHECK(*bad.exit_code == 1); } + +TEST_CASE("parse_args: -c/--config-file populates config_file_flag, unset by default", "[unit]") { + auto without = run_parse({"--list-volumes"}); + REQUIRE_FALSE(without.exit_code.has_value()); + CHECK_FALSE(without.args.config_file_flag.has_value()); + + auto with_short = run_parse({"-c", "/tmp/alt.yaml", "--list-volumes"}); + REQUIRE_FALSE(with_short.exit_code.has_value()); + REQUIRE(with_short.args.config_file_flag.has_value()); + CHECK(*with_short.args.config_file_flag == "/tmp/alt.yaml"); + + auto with_long = run_parse({"--config-file", "/tmp/alt2.yaml", "-w"}); + REQUIRE_FALSE(with_long.exit_code.has_value()); + REQUIRE(with_long.args.config_file_flag.has_value()); + CHECK(*with_long.args.config_file_flag == "/tmp/alt2.yaml"); +} + +TEST_CASE("parse_args: --log-level sets log_level_flag_given", "[unit]") { + auto without = run_parse({"--list-volumes"}); + REQUIRE_FALSE(without.exit_code.has_value()); + CHECK_FALSE(without.args.log_level_flag_given); + + auto with_level = run_parse({"--log-level", "debug", "--list-volumes"}); + REQUIRE_FALSE(with_level.exit_code.has_value()); + CHECK(with_level.args.log_level_flag_given); +}