From be28436c2d1c57f92d97ad6308997c7a3ca5952a Mon Sep 17 00:00:00 2001 From: Joachim Wiberg Date: Wed, 29 Jul 2026 10:27:47 +0200 Subject: [PATCH] conf: drop the angle brackets from if, infer from the value The block format spells conditions as bare strings everywhere else, so requiring `if = ""` left one sigil behind, carried over from the line-based `if:` token. A namespace separator already tells the two apart: a value with a '/' is a condition, anything else is a service name. svc_ifthen() picks its mode from the start of the statement and applies it to the whole, so a statement naming both kinds cannot be evaluated. That is now an error, as are the old angle brackets, and either one skips the block: /etc/finit.conf: mixed: if: cannot mix a service name with a condition in 'anchor,usr/enable-me', a statement must be all of one kind, skipping Signed-off-by: Joachim Wiberg --- doc/config/services.md | 24 ++++++++--- src/conf.c | 68 +++++++++++++++++++++++++++++- test/Makefile.am | 2 + test/conf-if.sh | 95 ++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 182 insertions(+), 7 deletions(-) create mode 100755 test/conf-if.sh diff --git a/doc/config/services.md b/doc/config/services.md index 857a335e..281106ee 100644 --- a/doc/config/services.md +++ b/doc/config/services.md @@ -296,7 +296,7 @@ Consider this (simplified) example from the Infix operating system: run failure { runlevel = "S" - if = "" + if = "usr/fail-startup" conditions = { "pid/sysrepo" } command = "confd --load failure-config" } @@ -312,9 +312,23 @@ check whether a run/task/service is qualified to even be considered. `if` has a negation of its own, `!`, which is unrelated to anything in the `conditions` list. -What `if` compares against depends on the angle brackets: `if = "udevd"` -asks whether a service by that name is known, decided when the .conf is -read, while `if = ""` tests a condition at runtime. +What `if` compares against depends on the value. A namespace +separator makes it a condition, anything else is a service name: + +| `if` | Asks | +|---|---| +| `"udevd"` | is a service by this name known? | +| `"usr/foo"` | was this condition set? | + +Both are questions about whether the block belongs in the running +configuration at all, usually answered from what bootstrap established. +A statement is all of one kind or the other, so the block is rejected +if you mix them. + +> [!NOTE] +> `if` qualifies, it does not track. A condition asserted or cleared +> later does not start or stop the service by itself -- that is what +> the `conditions` list is for. Conditional execution can also be negated, so provided the file loaded did the opposite, i.e., set a condition on success, the previous block @@ -322,7 +336,7 @@ can be written as: run failure { runlevel = "S" - if = "" + if = "!usr/startup-ok" conditions = { "pid/sysrepo" } command = "confd ..." } diff --git a/src/conf.c b/src/conf.c index aef89a85..752d84bc 100644 --- a/src/conf.c +++ b/src/conf.c @@ -1230,6 +1230,65 @@ static void addlog(char *line, size_t len, cfg_t *sec, char *file) * Translate service/task/run/sysv section to the canonical legacy * one-liner and register through the same path as legacy files. */ +/* + * if = "udevd" asks whether a service by that name is known, answered + * when the .conf is read, while if = "usr/foo" tests a condition, + * answered at runtime. The namespace separator tells the two apart, + * so the legacy angle brackets are not needed. A statement must be + * all of one kind; the two are not checked at the same time, so they + * cannot be combined. + */ +static int if_translate(const char *str, char *buf, size_t len, char *file, const char *ident) +{ + char tmp[MAX_IDENT_LEN]; + char *op, *save; + int cond = -1; + + if (strpbrk(str, "<>")) { + logit(LOG_ERR, "%s: %s: if: angle brackets are not used in this" + " format, got '%s', skipping", file, ident, str); + return -1; + } + + if (strlcpy(tmp, str, sizeof(tmp)) >= sizeof(tmp)) { + logit(LOG_ERR, "%s: %s: if: '%s' is too long, skipping", + file, ident, str); + return -1; + } + + for (op = strtok_r(tmp, ",", &save); op; op = strtok_r(NULL, ",", &save)) { + int is_cond; + + if (op[0] == '!') + op++; + if (!op[0]) + continue; + + is_cond = strchr(op, '/') != NULL; + if (cond == -1) + cond = is_cond; + else if (cond != is_cond) { + logit(LOG_ERR, "%s: %s: if: cannot mix a service name with a" + " condition in '%s', a statement must be all of one" + " kind, skipping", file, ident, str); + return -1; + } + } + + if (cond == -1) { + logit(LOG_ERR, "%s: %s: if: no condition or service name in '%s'," + " skipping", file, ident, str); + return -1; + } + + if (cond) + snprintf(buf, len, "<%s>", str); + else + strlcpy(buf, str, len); + + return 0; +} + static void svc_translate(cfg_t *sec, int type, struct rlimit rlimit[], char *file) { struct rlimit local_rlimit[RLIMIT_NLIMITS]; @@ -1401,8 +1460,13 @@ static void svc_translate(cfg_t *sec, int type, struct rlimit rlimit[], char *fi if (sec_getlist(sec, "conflicts", NULL, buf, sizeof(buf))) addtok(line, sizeof(line), "conflict:%s", buf); - if ((str = sec_getstr(sec, "if", NULL))) - addopt(line, sizeof(line), "if:", str); + if ((str = sec_getstr(sec, "if", NULL))) { + char ifbuf[MAX_IDENT_LEN + 2]; + + if (if_translate(str, ifbuf, sizeof(ifbuf), file, cfg_title(sec))) + return; + addopt(line, sizeof(line), "if:", ifbuf); + } if ((str = sec_getstr(sec, "tty", NULL))) addtok(line, sizeof(line), "tty:%s", str); diff --git a/test/Makefile.am b/test/Makefile.am index 0860d904..392f845e 100644 --- a/test/Makefile.am +++ b/test/Makefile.am @@ -32,6 +32,7 @@ EXTRA_DIST += add-remove-dynamic-service-sub-config.sh EXTRA_DIST += bootstrap-crash.sh EXTRA_DIST += cond-start-task.sh EXTRA_DIST += conf-format.sh +EXTRA_DIST += conf-if.sh EXTRA_DIST += conf-template.sh EXTRA_DIST += script-timeout.sh EXTRA_DIST += crashing.sh @@ -79,6 +80,7 @@ TESTS += add-remove-dynamic-service-sub-config.sh TESTS += bootstrap-crash.sh TESTS += cond-start-task.sh TESTS += conf-format.sh +TESTS += conf-if.sh TESTS += conf-template.sh TESTS += script-timeout.sh TESTS += crashing.sh diff --git a/test/conf-if.sh b/test/conf-if.sh new file mode 100755 index 00000000..3f9b08be --- /dev/null +++ b/test/conf-if.sh @@ -0,0 +1,95 @@ +#!/bin/sh +# Verify the block format 'if' setting: a value with a namespace +# separator is a condition, checked at runtime, anything else is a +# service name, checked when the .conf is read. The two cannot be +# combined, and the legacy angle brackets are rejected. +set -eu + +TEST_DIR=$(dirname "$0") + +# shellcheck disable=SC2034 +BOOTSTRAP="service anchor { + runlevel = \"S12345\" + command = \"serv -np -i anchor\" +} +service byname { + runlevel = \"S12345\" + if = \"anchor\" + command = \"serv -np -i byname\" +} +service noname { + runlevel = \"S12345\" + if = \"nosuchservice\" + command = \"serv -np -i noname\" +} +service negated { + runlevel = \"S12345\" + if = \"!anchor\" + command = \"serv -np -i negated\" +} +service bycond { + runlevel = \"S12345\" + if = \"usr/enable-me\" + command = \"serv -np -i bycond\" +} +service brackets { + runlevel = \"S12345\" + if = \"\" + command = \"serv -np -i brackets\" +} +service mixed { + runlevel = \"S12345\" + if = \"anchor,usr/enable-me\" + command = \"serv -np -i mixed\" +}" + +# initctl status prints a detail block for a single match and a table +# only for several, so count lines in the full listing instead. +assert_loaded() +{ + assert "Service $1 loaded: $2" \ + "$(texec initctl -t status | awk -v n="$1" '$2 == n' | wc -l)" -eq "$2" +} + +test_teardown() +{ + say "Running test teardown." + run "initctl cond clear enable-me" || true +} + +# shellcheck source=/dev/null +. "$TEST_DIR/lib/setup.sh" + +say 'A bare value names a service, resolved when the .conf is read' +retry 'assert_loaded byname 1' + +say 'A service that is not known keeps the block out of the config' +assert_loaded noname 0 + +say 'Negation of a known service keeps the block out too' +assert_loaded negated 0 + +say 'A value with a namespace separator is a condition, so the block is' +say 'loaded, but held until the condition is asserted' +assert_loaded bycond 1 +assert_status bycond halted + +run "initctl cond set enable-me" +retry 'assert_status bycond running' + +# 'if' qualifies the block, it does not track the condition, so +# clearing it does not stop a running service. The qualification is +# re-read when the configuration is, i.e. on reload. +run "initctl cond clear enable-me" +run "initctl reload" +retry 'assert_status bycond halted' + +say 'Angle brackets belong to the line-based format, the block is rejected' +assert_loaded brackets 0 + +say 'A service name and a condition are checked at different times, so' +say 'mixing them in one statement is rejected' +assert_loaded mixed 0 + +say 'Rejecting a block leaves the rest of the file alone' +assert_loaded anchor 1