From 48f48b8c7cfbbea44eca91eb21e408dfeb9e22a9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 26 Jun 2019 11:52:57 +0200 Subject: core: move assert before actual use of the variable No point in using u->id first, and doing assert(u) later. -std=c89 strikes again. --- src/core/unit-printf.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/core/unit-printf.c b/src/core/unit-printf.c index 72391ace3a..ffa12d8a92 100644 --- a/src/core/unit-printf.c +++ b/src/core/unit-printf.c @@ -244,6 +244,10 @@ int unit_full_printf(Unit *u, const char *format, char **ret) { * before or after the relevant configuration setting. Hence: don't add them. */ + assert(u); + assert(format); + assert(ret); + const Specifier table[] = { { 'n', specifier_string, u->id }, { 'N', specifier_prefix_and_instance, NULL }, @@ -281,9 +285,5 @@ int unit_full_printf(Unit *u, const char *format, char **ret) { {} }; - assert(u); - assert(format); - assert(ret); - return specifier_printf(format, table, u, ret); } -- cgit v1.2.3 From bbd199c438b172034e40ecdfa47ecad5b3f94c1f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 26 Jun 2019 14:56:04 +0200 Subject: man: move description of how conditions are combined to the beginning Originally the description of conditions was brief, so it was acceptable to put this part at the end. But now we have a myriad conditions, and this crucial bit of information is easy to miss. --- man/systemd.unit.xml | 32 ++++++++++++-------------------- 1 file changed, 12 insertions(+), 20 deletions(-) diff --git a/man/systemd.unit.xml b/man/systemd.unit.xml index 38a239a0e8..81bce696bd 100644 --- a/man/systemd.unit.xml +++ b/man/systemd.unit.xml @@ -1007,9 +1007,7 @@ ConditionMemory= ConditionCPUs= - Before starting a unit, verify that the specified condition is true. If it is not true, the @@ -1024,6 +1022,16 @@ conditions are considered to be in a clean state and will be garbage collected if they are not referenced. This means, that when queried, the condition failure may or may not show up in the state of the unit. + If multiple conditions are specified, the unit will be executed if all of them apply (i.e. a + logical AND is applied). Condition checks can be prefixed with a pipe symbol (|) + in which case a condition becomes a triggering condition. If at least one triggering condition is + defined for a unit, then the unit will be executed if at least one of the triggering conditions apply + and all of the non-triggering conditions. If you prefix an argument with the pipe symbol and an + exclamation mark, the pipe symbol must be passed first, the exclamation second. Except for + ConditionPathIsSymbolicLink=, all path checks follow symlinks. If any of these + options is assigned the empty string, the list of conditions is reset completely, all previous + condition settings (of any kind) will have no effect. + ConditionArchitecture= may be used to check whether the system is running on a specific architecture. Takes one of @@ -1279,23 +1287,7 @@ comparison operator. On physical systems the number of CPUs in the affinity mask of the service manager usually matches the number of physical CPUs, but in special and virtual environments might differ. In particular, in containers the affinity mask usually matches the number of CPUs assigned to - the container and not the physically available ones. - - If multiple conditions are specified, the unit will be - executed if all of them apply (i.e. a logical AND is applied). - Condition checks can be prefixed with a pipe symbol (|) in - which case a condition becomes a triggering condition. If at - least one triggering condition is defined for a unit, then the - unit will be executed if at least one of the triggering - conditions apply and all of the non-triggering conditions. If - you prefix an argument with the pipe symbol and an exclamation - mark, the pipe symbol must be passed first, the exclamation - second. Except for - ConditionPathIsSymbolicLink=, all path - checks follow symlinks. If any of these options is assigned - the empty string, the list of conditions is reset completely, - all previous condition settings (of any kind) will have no - effect. + the container and not the physically available ones. -- cgit v1.2.3 From b1d5246d2953415cb6ef7c229cf93e3151740725 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 26 Jun 2019 14:56:59 +0200 Subject: core: do not enumerate units in MANAGER_TEST_RUN_MINIMAL mode In this mode we are not supposed to "interact with the environment", so loading all units and printing warnings about syntax errors and /var/run usage seems inappropriate. --- src/core/manager.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/core/manager.c b/src/core/manager.c index 78f03a824c..0c1adf2850 100644 --- a/src/core/manager.c +++ b/src/core/manager.c @@ -1378,6 +1378,9 @@ static void manager_enumerate_perpetual(Manager *m) { assert(m); + if (m->test_run_flags == MANAGER_TEST_RUN_MINIMAL) + return; + /* Let's ask every type to load all units from disk/kernel that it might know */ for (c = 0; c < _UNIT_TYPE_MAX; c++) { if (!unit_type_supported(c)) { @@ -1395,6 +1398,9 @@ static void manager_enumerate(Manager *m) { assert(m); + if (m->test_run_flags == MANAGER_TEST_RUN_MINIMAL) + return; + /* Let's ask every type to load all units from disk/kernel that it might know */ for (c = 0; c < _UNIT_TYPE_MAX; c++) { if (!unit_type_supported(c)) { -- cgit v1.2.3 From edfea9fe0db025d8b90f07d969b48a1017399265 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 26 Jun 2019 14:58:45 +0200 Subject: analyze: add 'condition' verb We didn't have a straightforward way to parse and evaluate those strings. Prompted by #12881. --- man/systemd-analyze.xml | 33 ++++++++ man/systemd.unit.xml | 10 ++- src/analyze/analyze-condition.c | 155 ++++++++++++++++++++++++++++++++++ src/analyze/analyze-condition.h | 6 ++ src/analyze/analyze.c | 7 ++ src/analyze/meson.build | 2 + src/core/load-fragment-gperf.gperf.m4 | 1 + 7 files changed, 212 insertions(+), 2 deletions(-) create mode 100644 src/analyze/analyze-condition.c create mode 100644 src/analyze/analyze-condition.h diff --git a/man/systemd-analyze.xml b/man/systemd-analyze.xml index 651a73848e..5dce2ae8fb 100644 --- a/man/systemd-analyze.xml +++ b/man/systemd-analyze.xml @@ -83,6 +83,12 @@ OPTIONS unit-paths + + systemd-analyze + OPTIONS + condition + CONDITION… + systemd-analyze OPTIONS @@ -348,6 +354,33 @@ $ eog targets.svg to retrieve the actual list that the manager uses, with any empty directories omitted. + + <command>systemd-analyze condition <replaceable>CONDITION</replaceable>...</command> + + This command will evaluate Condition*=... and + Assert*=... assignments, and print their values, and + the resulting value of the combined condition set. See + systemd.unit5 + for a list of available conditions and asserts. + + + Evaluate conditions that check kernel versions + + $ systemd-analyze condition 'ConditionKernelVersion = ! <4.0' \ + 'ConditionKernelVersion = >=5.1' \ + 'ConditionACPower=|false' \ + 'ConditionArchitecture=|!arm' \ + 'AssertPathExists=/etc/os-release' +test.service: AssertPathExists=/etc/os-release succeeded. +Asserts succeeded. +test.service: ConditionArchitecture=|!arm succeeded. +test.service: ConditionACPower=|false failed. +test.service: ConditionKernelVersion=>=5.1 succeeded. +test.service: ConditionKernelVersion=!<4.0 succeeded. +Conditions succeeded. + + + <command>systemd-analyze syscall-filter <optional><replaceable>SET</replaceable>...</optional></command> diff --git a/man/systemd.unit.xml b/man/systemd.unit.xml index 81bce696bd..045931038b 100644 --- a/man/systemd.unit.xml +++ b/man/systemd.unit.xml @@ -1030,7 +1030,9 @@ exclamation mark, the pipe symbol must be passed first, the exclamation second. Except for ConditionPathIsSymbolicLink=, all path checks follow symlinks. If any of these options is assigned the empty string, the list of conditions is reset completely, all previous - condition settings (of any kind) will have no effect. + condition settings (of any kind) will have no effect. The condition verb of + systemd-analyze1 + can be used to test condition and assert expressions. ConditionArchitecture= may be used to check whether the system is running on a specific @@ -1326,7 +1328,11 @@ Note that neither assertion nor condition expressions result in unit state changes. Also note that both are checked at the time the job is to be executed, i.e. long after depending jobs and it itself were queued. Thus, neither condition nor assertion expressions are suitable for conditionalizing unit - dependencies. + dependencies. + + The condition verb of + systemd-analyze1 + can be used to test condition and assert expressions. diff --git a/src/analyze/analyze-condition.c b/src/analyze/analyze-condition.c new file mode 100644 index 0000000000..d0cefa0992 --- /dev/null +++ b/src/analyze/analyze-condition.c @@ -0,0 +1,155 @@ +/* SPDX-License-Identifier: LGPL-2.1+ */ + +#include + +#include "analyze-condition.h" +#include "condition.h" +#include "conf-parser.h" +#include "load-fragment.h" +#include "service.h" + +typedef struct condition_definition { + const char *name; + ConfigParserCallback parser; + ConditionType type; +} condition_definition; + +static const condition_definition condition_definitions[] = { + { "ConditionPathExists", config_parse_unit_condition_path, CONDITION_PATH_EXISTS }, + { "ConditionPathExistsGlob", config_parse_unit_condition_path, CONDITION_PATH_EXISTS_GLOB }, + { "ConditionPathIsDirectory", config_parse_unit_condition_path, CONDITION_PATH_IS_DIRECTORY }, + { "ConditionPathIsSymbolicLink", config_parse_unit_condition_path, CONDITION_PATH_IS_SYMBOLIC_LINK }, + { "ConditionPathIsMountPoint", config_parse_unit_condition_path, CONDITION_PATH_IS_MOUNT_POINT }, + { "ConditionPathIsReadWrite", config_parse_unit_condition_path, CONDITION_PATH_IS_READ_WRITE }, + { "ConditionDirectoryNotEmpty", config_parse_unit_condition_path, CONDITION_DIRECTORY_NOT_EMPTY }, + { "ConditionFileNotEmpty", config_parse_unit_condition_path, CONDITION_FILE_NOT_EMPTY }, + { "ConditionFileIsExecutable", config_parse_unit_condition_path, CONDITION_FILE_IS_EXECUTABLE }, + { "ConditionNeedsUpdate", config_parse_unit_condition_path, CONDITION_NEEDS_UPDATE }, + { "ConditionFirstBoot", config_parse_unit_condition_string, CONDITION_FIRST_BOOT }, + { "ConditionKernelCommandLine", config_parse_unit_condition_string, CONDITION_KERNEL_COMMAND_LINE }, + { "ConditionKernelVersion", config_parse_unit_condition_string, CONDITION_KERNEL_VERSION }, + { "ConditionArchitecture", config_parse_unit_condition_string, CONDITION_ARCHITECTURE }, + { "ConditionVirtualization", config_parse_unit_condition_string, CONDITION_VIRTUALIZATION }, + { "ConditionSecurity", config_parse_unit_condition_string, CONDITION_SECURITY }, + { "ConditionCapability", config_parse_unit_condition_string, CONDITION_CAPABILITY }, + { "ConditionHost", config_parse_unit_condition_string, CONDITION_HOST }, + { "ConditionACPower", config_parse_unit_condition_string, CONDITION_AC_POWER }, + { "ConditionUser", config_parse_unit_condition_string, CONDITION_USER }, + { "ConditionGroup", config_parse_unit_condition_string, CONDITION_GROUP }, + { "ConditionControlGroupController", config_parse_unit_condition_string, CONDITION_CONTROL_GROUP_CONTROLLER }, + + { "AssertPathExists", config_parse_unit_condition_path, CONDITION_PATH_EXISTS }, + { "AssertPathExistsGlob", config_parse_unit_condition_path, CONDITION_PATH_EXISTS_GLOB }, + { "AssertPathIsDirectory", config_parse_unit_condition_path, CONDITION_PATH_IS_DIRECTORY }, + { "AssertPathIsSymbolicLink", config_parse_unit_condition_path, CONDITION_PATH_IS_SYMBOLIC_LINK }, + { "AssertPathIsMountPoint", config_parse_unit_condition_path, CONDITION_PATH_IS_MOUNT_POINT }, + { "AssertPathIsReadWrite", config_parse_unit_condition_path, CONDITION_PATH_IS_READ_WRITE }, + { "AssertDirectoryNotEmpty", config_parse_unit_condition_path, CONDITION_DIRECTORY_NOT_EMPTY }, + { "AssertFileNotEmpty", config_parse_unit_condition_path, CONDITION_FILE_NOT_EMPTY }, + { "AssertFileIsExecutable", config_parse_unit_condition_path, CONDITION_FILE_IS_EXECUTABLE }, + { "AssertNeedsUpdate", config_parse_unit_condition_path, CONDITION_NEEDS_UPDATE }, + { "AssertFirstBoot", config_parse_unit_condition_string, CONDITION_FIRST_BOOT }, + { "AssertKernelCommandLine", config_parse_unit_condition_string, CONDITION_KERNEL_COMMAND_LINE }, + { "AssertKernelVersion", config_parse_unit_condition_string, CONDITION_KERNEL_VERSION }, + { "AssertArchitecture", config_parse_unit_condition_string, CONDITION_ARCHITECTURE }, + { "AssertVirtualization", config_parse_unit_condition_string, CONDITION_VIRTUALIZATION }, + { "AssertSecurity", config_parse_unit_condition_string, CONDITION_SECURITY }, + { "AssertCapability", config_parse_unit_condition_string, CONDITION_CAPABILITY }, + { "AssertHost", config_parse_unit_condition_string, CONDITION_HOST }, + { "AssertACPower", config_parse_unit_condition_string, CONDITION_AC_POWER }, + { "AssertUser", config_parse_unit_condition_string, CONDITION_USER }, + { "AssertGroup", config_parse_unit_condition_string, CONDITION_GROUP }, + { "AssertControlGroupController", config_parse_unit_condition_string, CONDITION_CONTROL_GROUP_CONTROLLER }, + + /* deprecated, but we should still parse them */ + { "ConditionNull", config_parse_unit_condition_null, 0 }, + { "AssertNull", config_parse_unit_condition_null, 0 }, +}; + +static int parse_condition(Unit *u, const char *line) { + const char *p; + Condition **target; + + if ((p = startswith(line, "Condition"))) + target = &u->conditions; + else if ((p = startswith(line, "Assert"))) + target = &u->asserts; + else + return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "Cannot parse \"%s\".", line); + + for (size_t i = 0; i < ELEMENTSOF(condition_definitions); i++) { + const condition_definition *c = &condition_definitions[i]; + + p = startswith(line, c->name); + if (!p) + continue; + p += strspn(p, WHITESPACE); + if (*p != '=') + return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "Expected \"=\" in \"%s\".", line); + + p += 1 + strspn(p + 1, WHITESPACE); + + return c->parser(NULL, "(stdin)", 0, NULL, 0, c->name, c->type, p, target, u); + } + + return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "Cannot parse \"%s\".", line); +} + +_printf_(7, 8) +static int log_helper(void *userdata, int level, int error, const char *file, int line, const char *func, const char *format, ...) { + Unit *u = userdata; + va_list ap; + int r; + + assert(u); + + /* "upgrade" debug messages */ + level = MIN(LOG_INFO, level); + + va_start(ap, format); + r = log_object_internalv(level, error, file, line, func, + NULL, + u->id, + NULL, + NULL, + format, ap); + va_end(ap); + + return r; +} + +int verify_conditions(char **lines, UnitFileScope scope) { + _cleanup_(manager_freep) Manager *m = NULL; + Unit *u; + char **line; + int r, q = 1; + + r = manager_new(scope, MANAGER_TEST_RUN_MINIMAL, &m); + if (r < 0) + return log_error_errno(r, "Failed to initialize manager: %m"); + + log_debug("Starting manager..."); + r = manager_startup(m, NULL, NULL); + if (r < 0) + return r; + + r = unit_new_for_name(m, sizeof(Service), "test.service", &u); + if (r < 0) + return log_error_errno(r, "Failed to create test.service: %m"); + + STRV_FOREACH(line, lines) { + r = parse_condition(u, *line); + if (r < 0) + return r; + } + + r = condition_test_list(u->asserts, assert_type_to_string, log_helper, u); + if (u->asserts) + log_notice("Asserts %s.", r > 0 ? "succeeded" : "failed"); + + q = condition_test_list(u->conditions, condition_type_to_string, log_helper, u); + if (u->conditions) + log_notice("Conditions %s.", q > 0 ? "succeeded" : "failed"); + + return r > 0 && q > 0 ? 0 : -EIO; +} diff --git a/src/analyze/analyze-condition.h b/src/analyze/analyze-condition.h new file mode 100644 index 0000000000..2ef278eb5c --- /dev/null +++ b/src/analyze/analyze-condition.h @@ -0,0 +1,6 @@ +/* SPDX-License-Identifier: LGPL-2.1+ */ +#pragma once + +#include "install.h" + +int verify_conditions(char **lines, UnitFileScope scope); diff --git a/src/analyze/analyze.c b/src/analyze/analyze.c index 5217a92b43..40f54f9d46 100644 --- a/src/analyze/analyze.c +++ b/src/analyze/analyze.c @@ -13,6 +13,7 @@ #include "sd-bus.h" #include "alloc-util.h" +#include "analyze-condition.h" #include "analyze-security.h" #include "analyze-verify.h" #include "build.h" @@ -1897,6 +1898,10 @@ static int service_watchdogs(int argc, char *argv[], void *userdata) { return 0; } +static int do_condition(int argc, char *argv[], void *userdata) { + return verify_conditions(strv_skip(argv, 1), arg_scope); +} + static int do_verify(int argc, char *argv[], void *userdata) { return verify_units(strv_skip(argv, 1), arg_scope, arg_man, arg_generators); } @@ -1955,6 +1960,7 @@ static int help(int argc, char *argv[], void *userdata) { " cat-config Show configuration file and drop-ins\n" " unit-paths List load directories for units\n" " syscall-filter [NAME...] Print list of syscalls in seccomp filter\n" + " condition CONDITION... Evaluate conditions and asserts\n" " verify FILE... Check unit files for correctness\n" " service-watchdogs [BOOL] Get/set service watchdog state\n" " calendar SPEC... Validate repetitive calendar time events\n" @@ -2157,6 +2163,7 @@ static int run(int argc, char *argv[]) { { "cat-config", 2, VERB_ANY, 0, cat_config }, { "unit-paths", 1, 1, 0, dump_unit_paths }, { "syscall-filter", VERB_ANY, VERB_ANY, 0, dump_syscall_filters }, + { "condition", 2, VERB_ANY, 0, do_condition }, { "verify", 2, VERB_ANY, 0, do_verify }, { "calendar", 2, VERB_ANY, 0, test_calendar }, { "timestamp", 2, VERB_ANY, 0, test_timestamp }, diff --git a/src/analyze/meson.build b/src/analyze/meson.build index 4db4dfa552..58760d609b 100644 --- a/src/analyze/meson.build +++ b/src/analyze/meson.build @@ -2,6 +2,8 @@ systemd_analyze_sources = files(''' analyze.c + analyze-condition.c + analyze-condition.h analyze-verify.c analyze-verify.h analyze-security.c diff --git a/src/core/load-fragment-gperf.gperf.m4 b/src/core/load-fragment-gperf.gperf.m4 index f7906b374a..19ee56662c 100644 --- a/src/core/load-fragment-gperf.gperf.m4 +++ b/src/core/load-fragment-gperf.gperf.m4 @@ -254,6 +254,7 @@ Unit.SuccessAction, config_parse_emergency_action, 0, Unit.FailureActionExitStatus, config_parse_exit_status, 0, offsetof(Unit, failure_action_exit_status) Unit.SuccessActionExitStatus, config_parse_exit_status, 0, offsetof(Unit, success_action_exit_status) Unit.RebootArgument, config_parse_unit_string_printf, 0, offsetof(Unit, reboot_arg) +m4_dnl Also add any conditions to condition_definitions[] in src/analyze/analyze-condition.c. Unit.ConditionPathExists, config_parse_unit_condition_path, CONDITION_PATH_EXISTS, offsetof(Unit, conditions) Unit.ConditionPathExistsGlob, config_parse_unit_condition_path, CONDITION_PATH_EXISTS_GLOB, offsetof(Unit, conditions) Unit.ConditionPathIsDirectory, config_parse_unit_condition_path, CONDITION_PATH_IS_DIRECTORY, offsetof(Unit, conditions) -- cgit v1.2.3 From e3b52014e2b3e5cfa1ddc23828b14a9fce3507b3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 26 Jun 2019 16:21:34 +0200 Subject: shared/condition: fix printing of ConditionNull= ConditionNull= is the only condition where parameter==NULL is allowed, and we'd print ConditionNull=(null) or ConditionNull=!(null). --- src/shared/condition.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/shared/condition.c b/src/shared/condition.c index 2d521bc8c6..70ede533c0 100644 --- a/src/shared/condition.c +++ b/src/shared/condition.c @@ -747,20 +747,23 @@ bool condition_test_list(Condition *first, const char *(*to_string)(ConditionTyp r = condition_test(c); if (logger) { + const char *p = c->type == CONDITION_NULL ? "true" : c->parameter; + assert(p); + if (r < 0) logger(userdata, LOG_WARNING, r, __FILE__, __LINE__, __func__, "Couldn't determine result for %s=%s%s%s, assuming failed: %m", to_string(c->type), c->trigger ? "|" : "", c->negate ? "!" : "", - c->parameter); + p); else logger(userdata, LOG_DEBUG, 0, __FILE__, __LINE__, __func__, "%s=%s%s%s %s.", to_string(c->type), c->trigger ? "|" : "", c->negate ? "!" : "", - c->parameter, + p, condition_result_to_string(c->result)); } -- cgit v1.2.3 From 9266f31e61b6b6bda7929911e8e3d2fc87587911 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 26 Jun 2019 16:23:18 +0200 Subject: core: skip whitespace after "|" and "!" in the condition parser We'd skip any whitespace immediately after "=", but then we'd treat whitespace that is between "|" or "!" and the value as significant. This is rather confusing, let's ignore it too. --- src/core/load-fragment.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/core/load-fragment.c b/src/core/load-fragment.c index 8e8f26b636..d88b9317e9 100644 --- a/src/core/load-fragment.c +++ b/src/core/load-fragment.c @@ -2587,13 +2587,13 @@ int config_parse_unit_condition_string( return 0; } - trigger = rvalue[0] == '|'; + trigger = *rvalue == '|'; if (trigger) - rvalue++; + rvalue += 1 + strspn(rvalue + 1, WHITESPACE); - negate = rvalue[0] == '!'; + negate = *rvalue == '!'; if (negate) - rvalue++; + rvalue += 1 + strspn(rvalue + 1, WHITESPACE); r = unit_full_printf(u, rvalue, &s); if (r < 0) { -- cgit v1.2.3