From a8835c1190ca944ea0fa5f541ee1933d55188b9a Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 26 Aug 2022 16:10:40 +0200 Subject: [PATCH 01/17] condition: split out order operator enum Let's move the operator enum into its own .c/.h file, so that we can reuse it elsewhere, in particular systemd-analyze's compare-versions logic. Let's rename the concept CompareOperator, since it is nowadays genericlaly about both order *and* fnmatch comparisons, hence just naming it "order" is misleading. --- src/shared/compare-operator.c | 38 ++++++++++++ src/shared/compare-operator.h | 39 ++++++++++++ src/shared/condition.c | 110 ++++++++++------------------------ src/shared/meson.build | 2 + 4 files changed, 112 insertions(+), 77 deletions(-) create mode 100644 src/shared/compare-operator.c create mode 100644 src/shared/compare-operator.h diff --git a/src/shared/compare-operator.c b/src/shared/compare-operator.c new file mode 100644 index 0000000000..f9796ab80c --- /dev/null +++ b/src/shared/compare-operator.c @@ -0,0 +1,38 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ + +#include "compare-operator.h" +#include "string-util.h" + +CompareOperator parse_compare_operator(const char **s, bool allow_fnmatch) { + static const char *const prefix[_COMPARE_OPERATOR_MAX] = { + [COMPARE_FNMATCH_EQUAL] = "=$", + [COMPARE_FNMATCH_UNEQUAL] = "!=$", + + [COMPARE_LOWER_OR_EQUAL] = "<=", + [COMPARE_GREATER_OR_EQUAL] = ">=", + [COMPARE_LOWER] = "<", + [COMPARE_GREATER] = ">", + [COMPARE_EQUAL] = "=", + [COMPARE_UNEQUAL] = "!=", + }; + + assert(s); + + if (!*s) /* Hmm, we already reached the end, for example because extract_first_word() and + * parse_compare_operator() are use on the same string? */ + return _COMPARE_OPERATOR_INVALID; + + for (CompareOperator i = 0; i < _COMPARE_OPERATOR_MAX; i++) { + const char *e; + + e = startswith(*s, prefix[i]); + if (e) { + if (!allow_fnmatch && COMPARE_OPERATOR_IS_FNMATCH(i)) + break; + *s = e; + return i; + } + } + + return _COMPARE_OPERATOR_INVALID; +} diff --git a/src/shared/compare-operator.h b/src/shared/compare-operator.h new file mode 100644 index 0000000000..2efd4cae68 --- /dev/null +++ b/src/shared/compare-operator.h @@ -0,0 +1,39 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ +#pragma once + +#include +#include + +typedef enum CompareOperator { + /* Listed in order of checking. Note that some comparators are prefixes of others, hence the longest + * should be listed first. */ + + /* fnmatch() compare operators */ + _COMPARE_OPERATOR_FNMATCH_FIRST, + COMPARE_FNMATCH_EQUAL = _COMPARE_OPERATOR_FNMATCH_FIRST, + COMPARE_FNMATCH_UNEQUAL, + _COMPARE_OPERATOR_FNMATCH_LAST = COMPARE_FNMATCH_UNEQUAL, + + /* Order compare operators */ + _COMPARE_OPERATOR_ORDER_FIRST, + COMPARE_LOWER_OR_EQUAL = _COMPARE_OPERATOR_ORDER_FIRST, + COMPARE_GREATER_OR_EQUAL, + COMPARE_LOWER, + COMPARE_GREATER, + COMPARE_EQUAL, + COMPARE_UNEQUAL, + _COMPARE_OPERATOR_ORDER_LAST = COMPARE_UNEQUAL, + + _COMPARE_OPERATOR_MAX, + _COMPARE_OPERATOR_INVALID = -EINVAL, +} CompareOperator; + +static inline bool COMPARE_OPERATOR_IS_FNMATCH(CompareOperator c) { + return c >= _COMPARE_OPERATOR_FNMATCH_FIRST && c <= _COMPARE_OPERATOR_FNMATCH_LAST; +} + +static inline bool COMPARE_OPERATOR_IS_ORDER(CompareOperator c) { + return c >= _COMPARE_OPERATOR_ORDER_FIRST && c <= _COMPARE_OPERATOR_ORDER_LAST; +} + +CompareOperator parse_compare_operator(const char **s, bool allow_fnmatch); diff --git a/src/shared/condition.c b/src/shared/condition.c index 00d732e6ef..dc0d6d8eb5 100644 --- a/src/shared/condition.c +++ b/src/shared/condition.c @@ -21,6 +21,7 @@ #include "blockdev-util.h" #include "cap-list.h" #include "cgroup-util.h" +#include "compare-operator.h" #include "condition.h" #include "cpu-set-util.h" #include "creds-util.h" @@ -182,70 +183,25 @@ static int condition_test_credential(Condition *c, char **env) { return false; } -typedef enum { - /* Listed in order of checking. Note that some comparators are prefixes of others, hence the longest - * should be listed first. */ - _ORDER_FNMATCH_FIRST, - ORDER_FNMATCH_EQUAL = _ORDER_FNMATCH_FIRST, - ORDER_FNMATCH_UNEQUAL, - _ORDER_FNMATCH_LAST = ORDER_FNMATCH_UNEQUAL, - ORDER_LOWER_OR_EQUAL, - ORDER_GREATER_OR_EQUAL, - ORDER_LOWER, - ORDER_GREATER, - ORDER_EQUAL, - ORDER_UNEQUAL, - _ORDER_MAX, - _ORDER_INVALID = -EINVAL, -} OrderOperator; - -static OrderOperator parse_order(const char **s, bool allow_fnmatch) { - static const char *const prefix[_ORDER_MAX] = { - [ORDER_FNMATCH_EQUAL] = "=$", - [ORDER_FNMATCH_UNEQUAL] = "!=$", - [ORDER_LOWER_OR_EQUAL] = "<=", - [ORDER_GREATER_OR_EQUAL] = ">=", - [ORDER_LOWER] = "<", - [ORDER_GREATER] = ">", - [ORDER_EQUAL] = "=", - [ORDER_UNEQUAL] = "!=", - }; - - for (OrderOperator i = 0; i < _ORDER_MAX; i++) { - const char *e; - - e = startswith(*s, prefix[i]); - if (e) { - if (!allow_fnmatch && (i >= _ORDER_FNMATCH_FIRST && i <= _ORDER_FNMATCH_LAST)) - break; - *s = e; - return i; - } - } - - return _ORDER_INVALID; -} - -static bool test_order(int k, OrderOperator p) { - +static bool test_order(int k, CompareOperator p) { switch (p) { - case ORDER_LOWER: + case COMPARE_LOWER: return k < 0; - case ORDER_LOWER_OR_EQUAL: + case COMPARE_LOWER_OR_EQUAL: return k <= 0; - case ORDER_EQUAL: + case COMPARE_EQUAL: return k == 0; - case ORDER_UNEQUAL: + case COMPARE_UNEQUAL: return k != 0; - case ORDER_GREATER_OR_EQUAL: + case COMPARE_GREATER_OR_EQUAL: return k >= 0; - case ORDER_GREATER: + case COMPARE_GREATER: return k > 0; default: @@ -255,7 +211,7 @@ static bool test_order(int k, OrderOperator p) { } static int condition_test_kernel_version(Condition *c, char **env) { - OrderOperator order; + CompareOperator operator; struct utsname u; bool first = true; @@ -277,8 +233,8 @@ static int condition_test_kernel_version(Condition *c, char **env) { break; s = strstrip(word); - order = parse_order(&s, /* allow_fnmatch= */ false); - if (order >= 0) { + operator = parse_compare_operator(&s, /* allow_fnmatch= */ false); + if (operator >= 0) { s += strspn(s, WHITESPACE); if (isempty(s)) { if (first) { @@ -295,7 +251,7 @@ static int condition_test_kernel_version(Condition *c, char **env) { return log_debug_errno(SYNTHETIC_ERRNO(EINVAL), "Unexpected end of expression: %s", p); } - r = test_order(strverscmp_improved(u.release, s), order); + r = test_order(strverscmp_improved(u.release, s), operator); } else /* No prefix? Then treat as glob string */ r = fnmatch(s, u.release, 0) == 0; @@ -317,7 +273,7 @@ static int condition_test_osrelease(Condition *c, char **env) { for (const char *parameter = ASSERT_PTR(c->parameter);;) { _cleanup_free_ char *key = NULL, *condition = NULL, *actual_value = NULL; - OrderOperator order; + CompareOperator operator; const char *word; bool matches; @@ -327,7 +283,7 @@ static int condition_test_osrelease(Condition *c, char **env) { if (r == 0) break; - /* parse_order() needs the string to start with the comparators */ + /* parse_compare_operator() needs the string to start with the comparators */ word = condition; r = extract_first_word(&word, &key, "!<=>", EXTRACT_RETAIN_SEPARATORS); if (r < 0) @@ -338,8 +294,8 @@ static int condition_test_osrelease(Condition *c, char **env) { "Failed to parse parameter, key/value format expected: %m"); /* Do not allow whitespace after the separator, as that's not a valid os-release format */ - order = parse_order(&word, /* allow_fnmatch= */ false); - if (order < 0 || isempty(word) || strchr(WHITESPACE, *word) != NULL) + operator = parse_compare_operator(&word, /* allow_fnmatch= */ false); + if (operator < 0 || isempty(word) || strchr(WHITESPACE, *word) != NULL) return log_debug_errno(SYNTHETIC_ERRNO(EINVAL), "Failed to parse parameter, key/value format expected: %m"); @@ -348,12 +304,12 @@ static int condition_test_osrelease(Condition *c, char **env) { return log_debug_errno(r, "Failed to parse os-release: %m"); /* Might not be comparing versions, so do exact string matching */ - if (order == ORDER_EQUAL) + if (operator == COMPARE_EQUAL) matches = streq_ptr(actual_value, word); - else if (order == ORDER_UNEQUAL) + else if (operator == COMPARE_UNEQUAL) matches = !streq_ptr(actual_value, word); else - matches = test_order(strverscmp_improved(actual_value, word), order); + matches = test_order(strverscmp_improved(actual_value, word), operator); if (!matches) return false; @@ -363,7 +319,7 @@ static int condition_test_osrelease(Condition *c, char **env) { } static int condition_test_memory(Condition *c, char **env) { - OrderOperator order; + CompareOperator operator; uint64_t m, k; const char *p; int r; @@ -375,19 +331,19 @@ static int condition_test_memory(Condition *c, char **env) { m = physical_memory(); p = c->parameter; - order = parse_order(&p, /* allow_fnmatch= */ false); - if (order < 0) - order = ORDER_GREATER_OR_EQUAL; /* default to >= check, if nothing is specified. */ + operator = parse_compare_operator(&p, /* allow_fnmatch= */ false); + if (operator < 0) + operator = COMPARE_GREATER_OR_EQUAL; /* default to >= check, if nothing is specified. */ r = parse_size(p, 1024, &k); if (r < 0) return log_debug_errno(r, "Failed to parse size '%s': %m", p); - return test_order(CMP(m, k), order); + return test_order(CMP(m, k), operator); } static int condition_test_cpus(Condition *c, char **env) { - OrderOperator order; + CompareOperator operator; const char *p; unsigned k; int r, n; @@ -401,15 +357,15 @@ static int condition_test_cpus(Condition *c, char **env) { return log_debug_errno(n, "Failed to determine CPUs in affinity mask: %m"); p = c->parameter; - order = parse_order(&p, /* allow_fnmatch= */ false); - if (order < 0) - order = ORDER_GREATER_OR_EQUAL; /* default to >= check, if nothing is specified. */ + operator = parse_compare_operator(&p, /* allow_fnmatch= */ false); + if (operator < 0) + operator = COMPARE_GREATER_OR_EQUAL; /* default to >= check, if nothing is specified. */ r = safe_atou(p, &k); if (r < 0) return log_debug_errno(r, "Failed to parse number of CPUs: %m"); - return test_order(CMP((unsigned) n, k), order); + return test_order(CMP((unsigned) n, k), operator); } static int condition_test_user(Condition *c, char **env) { @@ -589,7 +545,7 @@ static int condition_test_firmware_devicetree_compatible(const char *dtcarg) { static int condition_test_firmware_smbios_field(const char *expression) { _cleanup_free_ char *field = NULL, *expected_value = NULL, *actual_value = NULL; - OrderOperator operator; + CompareOperator operator; int r; assert(expression); @@ -605,7 +561,7 @@ static int condition_test_firmware_smbios_field(const char *expression) { delete_trailing_chars(field, WHITESPACE); /* Parse operator */ - operator = parse_order(&expression, /* allow_fnmatch= */ true); + operator = parse_compare_operator(&expression, /* allow_fnmatch= */ true); if (operator < 0) return operator; @@ -633,9 +589,9 @@ static int condition_test_firmware_smbios_field(const char *expression) { delete_trailing_chars(actual_value, WHITESPACE); /* Finally compare actual and expected value */ - if (operator == ORDER_FNMATCH_EQUAL) + if (operator == COMPARE_FNMATCH_EQUAL) return fnmatch(expected_value, actual_value, FNM_EXTMATCH) != FNM_NOMATCH; - if (operator == ORDER_FNMATCH_UNEQUAL) + if (operator == COMPARE_FNMATCH_UNEQUAL) return fnmatch(expected_value, actual_value, FNM_EXTMATCH) == FNM_NOMATCH; return test_order(strverscmp_improved(actual_value, expected_value), operator); } diff --git a/src/shared/meson.build b/src/shared/meson.build index 426a87b70c..abac8a7eb8 100644 --- a/src/shared/meson.build +++ b/src/shared/meson.build @@ -71,6 +71,8 @@ shared_sources = files( 'clean-ipc.h', 'clock-util.c', 'clock-util.h', + 'compare-operator.c', + 'compare-operator.h', 'condition.c', 'condition.h', 'conf-parser.c', From 650c4c870795a6cbd9869ad2892242f5dc7c547a Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 26 Aug 2022 16:27:55 +0200 Subject: [PATCH 02/17] =?UTF-8?q?compare:=20move=20test=5Forder()=20?= =?UTF-8?q?=E2=86=92=20compare-operator.[ch]?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/shared/compare-operator.c | 27 +++++++++++++++++++++++++++ src/shared/compare-operator.h | 2 ++ src/shared/condition.c | 27 --------------------------- 3 files changed, 29 insertions(+), 27 deletions(-) diff --git a/src/shared/compare-operator.c b/src/shared/compare-operator.c index f9796ab80c..385f41894b 100644 --- a/src/shared/compare-operator.c +++ b/src/shared/compare-operator.c @@ -36,3 +36,30 @@ CompareOperator parse_compare_operator(const char **s, bool allow_fnmatch) { return _COMPARE_OPERATOR_INVALID; } + +int test_order(int k, CompareOperator op) { + + switch (op) { + + case COMPARE_LOWER: + return k < 0; + + case COMPARE_LOWER_OR_EQUAL: + return k <= 0; + + case COMPARE_EQUAL: + return k == 0; + + case COMPARE_UNEQUAL: + return k != 0; + + case COMPARE_GREATER_OR_EQUAL: + return k >= 0; + + case COMPARE_GREATER: + return k > 0; + + default: + return -EINVAL; + } +} diff --git a/src/shared/compare-operator.h b/src/shared/compare-operator.h index 2efd4cae68..9d1b71ed99 100644 --- a/src/shared/compare-operator.h +++ b/src/shared/compare-operator.h @@ -37,3 +37,5 @@ static inline bool COMPARE_OPERATOR_IS_ORDER(CompareOperator c) { } CompareOperator parse_compare_operator(const char **s, bool allow_fnmatch); + +int test_order(int k, CompareOperator op); diff --git a/src/shared/condition.c b/src/shared/condition.c index dc0d6d8eb5..d5aee1d793 100644 --- a/src/shared/condition.c +++ b/src/shared/condition.c @@ -183,33 +183,6 @@ static int condition_test_credential(Condition *c, char **env) { return false; } -static bool test_order(int k, CompareOperator p) { - switch (p) { - - case COMPARE_LOWER: - return k < 0; - - case COMPARE_LOWER_OR_EQUAL: - return k <= 0; - - case COMPARE_EQUAL: - return k == 0; - - case COMPARE_UNEQUAL: - return k != 0; - - case COMPARE_GREATER_OR_EQUAL: - return k >= 0; - - case COMPARE_GREATER: - return k > 0; - - default: - assert_not_reached(); - - } -} - static int condition_test_kernel_version(Condition *c, char **env) { CompareOperator operator; struct utsname u; From 8bd2cf6e11a1145a01c648663e45cc946f0495d9 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 26 Aug 2022 16:28:06 +0200 Subject: [PATCH 03/17] comapre: add generic implementation for comapring with verscmp+fnmatch --- src/shared/compare-operator.c | 23 +++++++++++++++++++++++ src/shared/compare-operator.h | 2 ++ src/shared/condition.c | 6 +----- 3 files changed, 26 insertions(+), 5 deletions(-) diff --git a/src/shared/compare-operator.c b/src/shared/compare-operator.c index 385f41894b..4baf57a35d 100644 --- a/src/shared/compare-operator.c +++ b/src/shared/compare-operator.c @@ -1,5 +1,7 @@ /* SPDX-License-Identifier: LGPL-2.1-or-later */ +#include + #include "compare-operator.h" #include "string-util.h" @@ -63,3 +65,24 @@ int test_order(int k, CompareOperator op) { return -EINVAL; } } + +int version_or_fnmatch_compare( + CompareOperator op, + const char *a, + const char *b) { + + switch (op) { + + case COMPARE_FNMATCH_EQUAL: + return fnmatch(b, a, FNM_EXTMATCH) != FNM_NOMATCH; + + case COMPARE_FNMATCH_UNEQUAL: + return fnmatch(b, a, FNM_EXTMATCH) == FNM_NOMATCH; + + case _COMPARE_OPERATOR_ORDER_FIRST..._COMPARE_OPERATOR_ORDER_LAST: + return test_order(strverscmp_improved(a, b), op); + + default: + return -EINVAL; + } +} diff --git a/src/shared/compare-operator.h b/src/shared/compare-operator.h index 9d1b71ed99..ceb534f268 100644 --- a/src/shared/compare-operator.h +++ b/src/shared/compare-operator.h @@ -39,3 +39,5 @@ static inline bool COMPARE_OPERATOR_IS_ORDER(CompareOperator c) { CompareOperator parse_compare_operator(const char **s, bool allow_fnmatch); int test_order(int k, CompareOperator op); + +int version_or_fnmatch_compare(CompareOperator op, const char *a, const char *b); diff --git a/src/shared/condition.c b/src/shared/condition.c index d5aee1d793..31b3db1182 100644 --- a/src/shared/condition.c +++ b/src/shared/condition.c @@ -562,11 +562,7 @@ static int condition_test_firmware_smbios_field(const char *expression) { delete_trailing_chars(actual_value, WHITESPACE); /* Finally compare actual and expected value */ - if (operator == COMPARE_FNMATCH_EQUAL) - return fnmatch(expected_value, actual_value, FNM_EXTMATCH) != FNM_NOMATCH; - if (operator == COMPARE_FNMATCH_UNEQUAL) - return fnmatch(expected_value, actual_value, FNM_EXTMATCH) == FNM_NOMATCH; - return test_order(strverscmp_improved(actual_value, expected_value), operator); + return version_or_fnmatch_compare(operator, actual_value, expected_value); } static int condition_test_firmware(Condition *c, char **env) { From 97c7bed90a4083184fadd3c6d24cc38fc487161d Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 26 Aug 2022 16:31:30 +0200 Subject: [PATCH 04/17] compare: drop use of FNM_EXTMATCH for now None of our other fnmatch() calls make use of this, and the concept was new to me at least. Given that this is only used for the recently added SMBIOS field matches (and is not included in any release) let's disable "extended" matches for now. We can certainly revisit this, and enable it later if there is real demand, but if we do, we should probably add that all over the place, not just for smbios matches. --- src/shared/compare-operator.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/shared/compare-operator.c b/src/shared/compare-operator.c index 4baf57a35d..a5b6c1ac6b 100644 --- a/src/shared/compare-operator.c +++ b/src/shared/compare-operator.c @@ -74,10 +74,10 @@ int version_or_fnmatch_compare( switch (op) { case COMPARE_FNMATCH_EQUAL: - return fnmatch(b, a, FNM_EXTMATCH) != FNM_NOMATCH; + return fnmatch(b, a, 0) != FNM_NOMATCH; case COMPARE_FNMATCH_UNEQUAL: - return fnmatch(b, a, FNM_EXTMATCH) == FNM_NOMATCH; + return fnmatch(b, a, 0) == FNM_NOMATCH; case _COMPARE_OPERATOR_ORDER_FIRST..._COMPARE_OPERATOR_ORDER_LAST: return test_order(strverscmp_improved(a, b), op); From 69f0a6091b8004cba50f49accb73843d25527618 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 26 Aug 2022 16:51:12 +0200 Subject: [PATCH 05/17] compare: add a proper flags field for parse_compare_operator() --- src/shared/compare-operator.c | 7 ++++--- src/shared/compare-operator.h | 6 +++++- src/shared/condition.c | 10 +++++----- 3 files changed, 14 insertions(+), 9 deletions(-) diff --git a/src/shared/compare-operator.c b/src/shared/compare-operator.c index a5b6c1ac6b..1a19e4219d 100644 --- a/src/shared/compare-operator.c +++ b/src/shared/compare-operator.c @@ -5,7 +5,7 @@ #include "compare-operator.h" #include "string-util.h" -CompareOperator parse_compare_operator(const char **s, bool allow_fnmatch) { +CompareOperator parse_compare_operator(const char **s, CompareOperatorParseFlags flags) { static const char *const prefix[_COMPARE_OPERATOR_MAX] = { [COMPARE_FNMATCH_EQUAL] = "=$", [COMPARE_FNMATCH_UNEQUAL] = "!=$", @@ -29,8 +29,9 @@ CompareOperator parse_compare_operator(const char **s, bool allow_fnmatch) { e = startswith(*s, prefix[i]); if (e) { - if (!allow_fnmatch && COMPARE_OPERATOR_IS_FNMATCH(i)) - break; + if (!FLAGS_SET(flags, COMPARE_ALLOW_FNMATCH) && COMPARE_OPERATOR_IS_FNMATCH(i)) + return _COMPARE_OPERATOR_INVALID; + *s = e; return i; } diff --git a/src/shared/compare-operator.h b/src/shared/compare-operator.h index ceb534f268..f7137bba1a 100644 --- a/src/shared/compare-operator.h +++ b/src/shared/compare-operator.h @@ -36,7 +36,11 @@ static inline bool COMPARE_OPERATOR_IS_ORDER(CompareOperator c) { return c >= _COMPARE_OPERATOR_ORDER_FIRST && c <= _COMPARE_OPERATOR_ORDER_LAST; } -CompareOperator parse_compare_operator(const char **s, bool allow_fnmatch); +typedef enum CompareOperatorParseFlags { + COMPARE_ALLOW_FNMATCH = 1 << 0, +} CompareOperatorParseFlags; + +CompareOperator parse_compare_operator(const char **s, CompareOperatorParseFlags flags); int test_order(int k, CompareOperator op); diff --git a/src/shared/condition.c b/src/shared/condition.c index 31b3db1182..1f9e6d221f 100644 --- a/src/shared/condition.c +++ b/src/shared/condition.c @@ -206,7 +206,7 @@ static int condition_test_kernel_version(Condition *c, char **env) { break; s = strstrip(word); - operator = parse_compare_operator(&s, /* allow_fnmatch= */ false); + operator = parse_compare_operator(&s, 0); if (operator >= 0) { s += strspn(s, WHITESPACE); if (isempty(s)) { @@ -267,7 +267,7 @@ static int condition_test_osrelease(Condition *c, char **env) { "Failed to parse parameter, key/value format expected: %m"); /* Do not allow whitespace after the separator, as that's not a valid os-release format */ - operator = parse_compare_operator(&word, /* allow_fnmatch= */ false); + operator = parse_compare_operator(&word, 0); if (operator < 0 || isempty(word) || strchr(WHITESPACE, *word) != NULL) return log_debug_errno(SYNTHETIC_ERRNO(EINVAL), "Failed to parse parameter, key/value format expected: %m"); @@ -304,7 +304,7 @@ static int condition_test_memory(Condition *c, char **env) { m = physical_memory(); p = c->parameter; - operator = parse_compare_operator(&p, /* allow_fnmatch= */ false); + operator = parse_compare_operator(&p, 0); if (operator < 0) operator = COMPARE_GREATER_OR_EQUAL; /* default to >= check, if nothing is specified. */ @@ -330,7 +330,7 @@ static int condition_test_cpus(Condition *c, char **env) { return log_debug_errno(n, "Failed to determine CPUs in affinity mask: %m"); p = c->parameter; - operator = parse_compare_operator(&p, /* allow_fnmatch= */ false); + operator = parse_compare_operator(&p, 0); if (operator < 0) operator = COMPARE_GREATER_OR_EQUAL; /* default to >= check, if nothing is specified. */ @@ -534,7 +534,7 @@ static int condition_test_firmware_smbios_field(const char *expression) { delete_trailing_chars(field, WHITESPACE); /* Parse operator */ - operator = parse_compare_operator(&expression, /* allow_fnmatch= */ true); + operator = parse_compare_operator(&expression, COMPARE_ALLOW_FNMATCH); if (operator < 0) return operator; From 57610982f73162ec37b1de03734250bb5a3222da Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 26 Aug 2022 16:56:04 +0200 Subject: [PATCH 06/17] compare: add flag for parse_compare_operator() to do equality/inequality comparison via simple string compares This allows us to switch condition_test_osrelease() to use generic version_or_fnmatch_compare() for executing the comparison. --- src/shared/compare-operator.c | 17 +++++++++++++++++ src/shared/compare-operator.h | 11 +++++++++++ src/shared/condition.c | 16 +++++----------- 3 files changed, 33 insertions(+), 11 deletions(-) diff --git a/src/shared/compare-operator.c b/src/shared/compare-operator.c index 1a19e4219d..2bd64add7f 100644 --- a/src/shared/compare-operator.c +++ b/src/shared/compare-operator.c @@ -27,12 +27,23 @@ CompareOperator parse_compare_operator(const char **s, CompareOperatorParseFlags for (CompareOperator i = 0; i < _COMPARE_OPERATOR_MAX; i++) { const char *e; + if (!prefix[i]) + continue; + e = startswith(*s, prefix[i]); if (e) { if (!FLAGS_SET(flags, COMPARE_ALLOW_FNMATCH) && COMPARE_OPERATOR_IS_FNMATCH(i)) return _COMPARE_OPERATOR_INVALID; *s = e; + + if (FLAGS_SET(flags, COMPARE_EQUAL_BY_STRING)) { + if (i == COMPARE_EQUAL) + return COMPARE_STRING_EQUAL; + if (i == COMPARE_UNEQUAL) + return COMPARE_STRING_UNEQUAL; + } + return i; } } @@ -74,6 +85,12 @@ int version_or_fnmatch_compare( switch (op) { + case COMPARE_STRING_EQUAL: + return streq_ptr(a, b); + + case COMPARE_STRING_UNEQUAL: + return !streq_ptr(a, b); + case COMPARE_FNMATCH_EQUAL: return fnmatch(b, a, 0) != FNM_NOMATCH; diff --git a/src/shared/compare-operator.h b/src/shared/compare-operator.h index f7137bba1a..363b4bdc75 100644 --- a/src/shared/compare-operator.h +++ b/src/shared/compare-operator.h @@ -8,6 +8,12 @@ typedef enum CompareOperator { /* Listed in order of checking. Note that some comparators are prefixes of others, hence the longest * should be listed first. */ + /* Simple string compare operators */ + _COMPARE_OPERATOR_STRING_FIRST, + COMPARE_STRING_EQUAL = _COMPARE_OPERATOR_STRING_FIRST, + COMPARE_STRING_UNEQUAL, + _COMPARE_OPERATOR_STRING_LAST = COMPARE_STRING_UNEQUAL, + /* fnmatch() compare operators */ _COMPARE_OPERATOR_FNMATCH_FIRST, COMPARE_FNMATCH_EQUAL = _COMPARE_OPERATOR_FNMATCH_FIRST, @@ -28,6 +34,10 @@ typedef enum CompareOperator { _COMPARE_OPERATOR_INVALID = -EINVAL, } CompareOperator; +static inline bool COMPARE_OPERATOR_IS_STRING(CompareOperator c) { + return c >= _COMPARE_OPERATOR_STRING_FIRST && c <= _COMPARE_OPERATOR_STRING_LAST; +} + static inline bool COMPARE_OPERATOR_IS_FNMATCH(CompareOperator c) { return c >= _COMPARE_OPERATOR_FNMATCH_FIRST && c <= _COMPARE_OPERATOR_FNMATCH_LAST; } @@ -38,6 +48,7 @@ static inline bool COMPARE_OPERATOR_IS_ORDER(CompareOperator c) { typedef enum CompareOperatorParseFlags { COMPARE_ALLOW_FNMATCH = 1 << 0, + COMPARE_EQUAL_BY_STRING = 1 << 1, } CompareOperatorParseFlags; CompareOperator parse_compare_operator(const char **s, CompareOperatorParseFlags flags); diff --git a/src/shared/condition.c b/src/shared/condition.c index 1f9e6d221f..db7a02db7d 100644 --- a/src/shared/condition.c +++ b/src/shared/condition.c @@ -248,7 +248,6 @@ static int condition_test_osrelease(Condition *c, char **env) { _cleanup_free_ char *key = NULL, *condition = NULL, *actual_value = NULL; CompareOperator operator; const char *word; - bool matches; r = extract_first_word(¶meter, &condition, NULL, EXTRACT_UNQUOTE); if (r < 0) @@ -267,7 +266,7 @@ static int condition_test_osrelease(Condition *c, char **env) { "Failed to parse parameter, key/value format expected: %m"); /* Do not allow whitespace after the separator, as that's not a valid os-release format */ - operator = parse_compare_operator(&word, 0); + operator = parse_compare_operator(&word, COMPARE_EQUAL_BY_STRING); if (operator < 0 || isempty(word) || strchr(WHITESPACE, *word) != NULL) return log_debug_errno(SYNTHETIC_ERRNO(EINVAL), "Failed to parse parameter, key/value format expected: %m"); @@ -276,15 +275,10 @@ static int condition_test_osrelease(Condition *c, char **env) { if (r < 0) return log_debug_errno(r, "Failed to parse os-release: %m"); - /* Might not be comparing versions, so do exact string matching */ - if (operator == COMPARE_EQUAL) - matches = streq_ptr(actual_value, word); - else if (operator == COMPARE_UNEQUAL) - matches = !streq_ptr(actual_value, word); - else - matches = test_order(strverscmp_improved(actual_value, word), operator); - - if (!matches) + r = version_or_fnmatch_compare(operator, actual_value, word); + if (r < 0) + return r; + if (!r) return false; } From 8daa674090d47320cd7ed6abb72dfdc0c4aa60f3 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 26 Aug 2022 16:59:47 +0200 Subject: [PATCH 07/17] condition: allow fnmatch compares for ConditionOSRelease= We support this for smbios matches, hence do so for /etc/os-release matches too. --- man/systemd.unit.xml | 11 +++++++---- src/shared/condition.c | 4 ++-- src/test/test-condition.c | 4 ++-- 3 files changed, 11 insertions(+), 8 deletions(-) diff --git a/man/systemd.unit.xml b/man/systemd.unit.xml index 7c1f7186e2..78a8db2e60 100644 --- a/man/systemd.unit.xml +++ b/man/systemd.unit.xml @@ -1694,10 +1694,13 @@ Verify that a specific key=value pair is set in the host's os-release5. - Other than exact matching with =, and !=, relative - comparisons are supported for versioned parameters (e.g. VERSION_ID). The - comparator can be one of <, <=, =, - !=, >= and >. + Other than exact string matching with =, and !=, + relative comparisons are supported for versioned parameters (e.g. VERSION_ID), + and shell-style wildcard comparisons (*, ?, + []) are supported with the =$ (match) and + !=$ (non-match). The comparator can be one of <, + <=, =, !=, >=, + >, =$ and !=$. diff --git a/src/shared/condition.c b/src/shared/condition.c index db7a02db7d..4e5702bc41 100644 --- a/src/shared/condition.c +++ b/src/shared/condition.c @@ -257,7 +257,7 @@ static int condition_test_osrelease(Condition *c, char **env) { /* parse_compare_operator() needs the string to start with the comparators */ word = condition; - r = extract_first_word(&word, &key, "!<=>", EXTRACT_RETAIN_SEPARATORS); + r = extract_first_word(&word, &key, "!<=>$", EXTRACT_RETAIN_SEPARATORS); if (r < 0) return log_debug_errno(r, "Failed to parse parameter: %m"); /* The os-release spec mandates env-var-like key names */ @@ -266,7 +266,7 @@ static int condition_test_osrelease(Condition *c, char **env) { "Failed to parse parameter, key/value format expected: %m"); /* Do not allow whitespace after the separator, as that's not a valid os-release format */ - operator = parse_compare_operator(&word, COMPARE_EQUAL_BY_STRING); + operator = parse_compare_operator(&word, COMPARE_ALLOW_FNMATCH|COMPARE_EQUAL_BY_STRING); if (operator < 0 || isempty(word) || strchr(WHITESPACE, *word) != NULL) return log_debug_errno(SYNTHETIC_ERRNO(EINVAL), "Failed to parse parameter, key/value format expected: %m"); diff --git a/src/test/test-condition.c b/src/test/test-condition.c index c430110acd..f93619293f 100644 --- a/src/test/test-condition.c +++ b/src/test/test-condition.c @@ -1177,13 +1177,13 @@ TEST(condition_test_os_release) { key_value_pair = strjoina(os_release_pairs[0], "=$", quote, os_release_pairs[1], quote); condition = condition_new(CONDITION_OS_RELEASE, key_value_pair, false, false); assert_se(condition); - assert_se(condition_test(condition, environ) == -EINVAL); + assert_se(condition_test(condition, environ) > 0); condition_free(condition); key_value_pair = strjoina(os_release_pairs[0], "!=$", quote, os_release_pairs[1], quote); condition = condition_new(CONDITION_OS_RELEASE, key_value_pair, false, false); assert_se(condition); - assert_se(condition_test(condition, environ) == -EINVAL); + assert_se(condition_test(condition, environ) == 0); condition_free(condition); /* Some distros (eg: Arch) do not set VERSION_ID */ From 38c09fa008b6c02fb01eb6b5b2d9931f04ff890c Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 26 Aug 2022 17:08:37 +0200 Subject: [PATCH 08/17] compare: rework table in parse_compare_operator() to be array of structs Let's change the lookup table to contain pairs of operator/strings, instead of being indexed by operator. The table isn't dense anymore, and this allows us to add alias strings sooner or later. --- src/shared/compare-operator.c | 43 +++++++++++++++++------------------ 1 file changed, 21 insertions(+), 22 deletions(-) diff --git a/src/shared/compare-operator.c b/src/shared/compare-operator.c index 2bd64add7f..43f876cabe 100644 --- a/src/shared/compare-operator.c +++ b/src/shared/compare-operator.c @@ -6,16 +6,23 @@ #include "string-util.h" CompareOperator parse_compare_operator(const char **s, CompareOperatorParseFlags flags) { - static const char *const prefix[_COMPARE_OPERATOR_MAX] = { - [COMPARE_FNMATCH_EQUAL] = "=$", - [COMPARE_FNMATCH_UNEQUAL] = "!=$", + static const struct { + CompareOperator op; + const char *str; + CompareOperatorParseFlags valid_mask; /* If this operator appears when flags in mask not set, fail */ + CompareOperatorParseFlags need_mask; /* Skip over this operattor when flags in mask not set */ + } table[] = { + { COMPARE_FNMATCH_EQUAL, "=$", .valid_mask = COMPARE_ALLOW_FNMATCH }, + { COMPARE_FNMATCH_UNEQUAL, "!=$", .valid_mask = COMPARE_ALLOW_FNMATCH }, - [COMPARE_LOWER_OR_EQUAL] = "<=", - [COMPARE_GREATER_OR_EQUAL] = ">=", - [COMPARE_LOWER] = "<", - [COMPARE_GREATER] = ">", - [COMPARE_EQUAL] = "=", - [COMPARE_UNEQUAL] = "!=", + { COMPARE_LOWER_OR_EQUAL, "<=" }, + { COMPARE_GREATER_OR_EQUAL, ">=" }, + { COMPARE_LOWER, "<" }, + { COMPARE_GREATER, ">" }, + { COMPARE_STRING_EQUAL, "=", .need_mask = COMPARE_EQUAL_BY_STRING }, + { COMPARE_EQUAL, "=" }, + { COMPARE_STRING_UNEQUAL, "!=", .need_mask = COMPARE_EQUAL_BY_STRING }, + { COMPARE_UNEQUAL, "!=" }, }; assert(s); @@ -24,27 +31,19 @@ CompareOperator parse_compare_operator(const char **s, CompareOperatorParseFlags * parse_compare_operator() are use on the same string? */ return _COMPARE_OPERATOR_INVALID; - for (CompareOperator i = 0; i < _COMPARE_OPERATOR_MAX; i++) { + for (size_t i = 0; i < ELEMENTSOF(table); i ++) { const char *e; - if (!prefix[i]) + if (table[i].need_mask != 0 && !FLAGS_SET(flags, table[i].need_mask)) continue; - e = startswith(*s, prefix[i]); + e = startswith(*s, table[i].str); if (e) { - if (!FLAGS_SET(flags, COMPARE_ALLOW_FNMATCH) && COMPARE_OPERATOR_IS_FNMATCH(i)) + if (table[i].valid_mask != 0 && !FLAGS_SET(flags, table[i].valid_mask)) return _COMPARE_OPERATOR_INVALID; *s = e; - - if (FLAGS_SET(flags, COMPARE_EQUAL_BY_STRING)) { - if (i == COMPARE_EQUAL) - return COMPARE_STRING_EQUAL; - if (i == COMPARE_UNEQUAL) - return COMPARE_STRING_UNEQUAL; - } - - return i; + return table[i].op; } } From 6061c86693453162395c46eda5e6fac34dea8985 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 26 Aug 2022 17:20:56 +0200 Subject: [PATCH 09/17] compare: add two new operators "==" and "<>" These two operators always indicate ordering comparisons, as opposed to "=" and "!=" which depending on context mean literal string compares. This is useful for ConditionOSRelease= for example, as this means there's now always a way to do version compares. --- man/systemd.unit.xml | 62 +++++++++++++++++++---------------- src/shared/compare-operator.c | 2 ++ 2 files changed, 35 insertions(+), 29 deletions(-) diff --git a/man/systemd.unit.xml b/man/systemd.unit.xml index 78a8db2e60..c5ba817579 100644 --- a/man/systemd.unit.xml +++ b/man/systemd.unit.xml @@ -1244,15 +1244,17 @@ device-tree-compatible(value) for systems with a device tree that is compatible to value. - smbios-field(field operator value) - for systems with a SMBIOS field containing a certain value. - field is the name of the SMBIOS field exposed as sysfs attribute file - below /sys/class/dmi/id/. + smbios-field(field operator + value) for systems with a SMBIOS field containing a certain + value. field is the name of the SMBIOS field exposed as + sysfs attribute file below /sys/class/dmi/id/. operator is one of <, <=, - >=, >, =, != for version - comparison, or =$, !=$ for string comparison. - value is the expected value of the SMBIOS field (shell-style globs are possible if - =$ or!=$ is used). + >=, >, ==, + <> for version comparison, = and != + for literal string comparison, or =$, !=$ for shell-style + glob comparison. value is the expected value of the SMBIOS field value + (possibly containing shell style globs in case =$/!=$ is + used). @@ -1333,9 +1335,10 @@ version (as reported by uname -r) matches a certain expression (or if prefixed with the exclamation mark does not match it). The argument must be a list of (potentially quoted) expressions. For each of the expressions, if it starts with one of <, - <=, =, !=, >=, - > a relative version comparison is done, otherwise the specified string is - matched with shell-style globs. + <=, = (or ==), != + (or <>), >=, > a relative + version comparison is done, otherwise the specified string is matched with shell-style + globs. Note that using the kernel version string is an unreliable way to determine which features are supported by a kernel, because of the widespread practice of backporting drivers, features, and @@ -1605,10 +1608,11 @@ Verify that the specified amount of system memory is available to the current system. Takes a memory size in bytes as argument, optionally prefixed with a comparison operator - <, <=, =, !=, - >=, >. On bare-metal systems compares the amount of - physical memory in the system with the specified size, adhering to the specified comparison - operator. In containers compares the amount of memory assigned to the container instead. + <, <=, = (or ==), + != (or <>), >=, + >. On bare-metal systems compares the amount of physical memory in the system + with the specified size, adhering to the specified comparison operator. In containers compares the + amount of memory assigned to the container instead. @@ -1617,13 +1621,14 @@ Verify that the specified number of CPUs is available to the current system. Takes a number of CPUs as argument, optionally prefixed with a comparison operator - <, <=, =, !=, - >=, >. Compares the number of CPUs in the CPU affinity - mask configured of the service manager itself with the specified number, adhering to the specified - 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. + <, <=, = (or ==), + != (or <>), >=, + >. Compares the number of CPUs in the CPU affinity mask configured of the + service manager itself with the specified number, adhering to the specified 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. @@ -1694,13 +1699,12 @@ Verify that a specific key=value pair is set in the host's os-release5. - Other than exact string matching with =, and !=, - relative comparisons are supported for versioned parameters (e.g. VERSION_ID), - and shell-style wildcard comparisons (*, ?, - []) are supported with the =$ (match) and - !=$ (non-match). The comparator can be one of <, - <=, =, !=, >=, - >, =$ and !=$. + Other than exact string matching (with = and !=), + relative comparisons are supported for versioned parameters (e.g. VERSION_ID; + with <, <=, ==, + <>, >=, >), and shell-style + wildcard comparisons (*, ?, []) are + supported with the =$ (match) and !=$ (non-match). diff --git a/src/shared/compare-operator.c b/src/shared/compare-operator.c index 43f876cabe..d7117f0771 100644 --- a/src/shared/compare-operator.c +++ b/src/shared/compare-operator.c @@ -15,10 +15,12 @@ CompareOperator parse_compare_operator(const char **s, CompareOperatorParseFlags { COMPARE_FNMATCH_EQUAL, "=$", .valid_mask = COMPARE_ALLOW_FNMATCH }, { COMPARE_FNMATCH_UNEQUAL, "!=$", .valid_mask = COMPARE_ALLOW_FNMATCH }, + { COMPARE_UNEQUAL, "<>" }, { COMPARE_LOWER_OR_EQUAL, "<=" }, { COMPARE_GREATER_OR_EQUAL, ">=" }, { COMPARE_LOWER, "<" }, { COMPARE_GREATER, ">" }, + { COMPARE_EQUAL, "==" }, { COMPARE_STRING_EQUAL, "=", .need_mask = COMPARE_EQUAL_BY_STRING }, { COMPARE_EQUAL, "=" }, { COMPARE_STRING_UNEQUAL, "!=", .need_mask = COMPARE_EQUAL_BY_STRING }, From 2e8fa6274d5c0b5ca540bb1786d57a9491c79f1c Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 26 Aug 2022 17:31:34 +0200 Subject: [PATCH 10/17] compare: support textual operators, and port analyze over to it --- src/analyze/analyze-compare-versions.c | 25 ++++++++++--------------- src/shared/compare-operator.c | 7 +++++++ src/shared/compare-operator.h | 1 + 3 files changed, 18 insertions(+), 15 deletions(-) diff --git a/src/analyze/analyze-compare-versions.c b/src/analyze/analyze-compare-versions.c index 202b8a7d09..07b52a55f2 100644 --- a/src/analyze/analyze-compare-versions.c +++ b/src/analyze/analyze-compare-versions.c @@ -3,6 +3,7 @@ #include #include "analyze-compare-versions.h" +#include "compare-operator.h" #include "macro.h" #include "string-util.h" #include "strv.h" @@ -26,22 +27,16 @@ int verb_compare_versions(int argc, char *argv[], void *userdata) { } else { const char *op = ASSERT_PTR(argv[2]); + CompareOperator operator; - r = strverscmp_improved(ASSERT_PTR(argv[1]), ASSERT_PTR(argv[3])); + operator = parse_compare_operator(&op, COMPARE_ALLOW_TEXTUAL); + if (operator < 0 || !isempty(op)) + return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "Unknown operator \"%s\".", op); - if (STR_IN_SET(op, "lt", "<")) - return r < 0 ? EXIT_SUCCESS : EXIT_FAILURE; - if (STR_IN_SET(op, "le", "<=")) - return r <= 0 ? EXIT_SUCCESS : EXIT_FAILURE; - if (STR_IN_SET(op, "eq", "==")) - return r == 0 ? EXIT_SUCCESS : EXIT_FAILURE; - if (STR_IN_SET(op, "ne", "!=")) - return r != 0 ? EXIT_SUCCESS : EXIT_FAILURE; - if (STR_IN_SET(op, "ge", ">=")) - return r >= 0 ? EXIT_SUCCESS : EXIT_FAILURE; - if (STR_IN_SET(op, "gt", ">")) - return r > 0 ? EXIT_SUCCESS : EXIT_FAILURE; - return log_error_errno(SYNTHETIC_ERRNO(EINVAL), - "Unknown operator \"%s\".", op); + r = version_or_fnmatch_compare(operator, ASSERT_PTR(argv[1]), ASSERT_PTR(argv[3])); + if (r < 0) + return log_error_errno(r, "Failed to compare versions: %m"); + + return r ? EXIT_SUCCESS : EXIT_FAILURE; } } diff --git a/src/shared/compare-operator.c b/src/shared/compare-operator.c index d7117f0771..694c778a27 100644 --- a/src/shared/compare-operator.c +++ b/src/shared/compare-operator.c @@ -25,6 +25,13 @@ CompareOperator parse_compare_operator(const char **s, CompareOperatorParseFlags { COMPARE_EQUAL, "=" }, { COMPARE_STRING_UNEQUAL, "!=", .need_mask = COMPARE_EQUAL_BY_STRING }, { COMPARE_UNEQUAL, "!=" }, + + { COMPARE_LOWER, "lt", .valid_mask = COMPARE_ALLOW_TEXTUAL }, + { COMPARE_LOWER_OR_EQUAL, "le", .valid_mask = COMPARE_ALLOW_TEXTUAL }, + { COMPARE_EQUAL, "eq", .valid_mask = COMPARE_ALLOW_TEXTUAL }, + { COMPARE_UNEQUAL, "ne", .valid_mask = COMPARE_ALLOW_TEXTUAL }, + { COMPARE_GREATER_OR_EQUAL, "ge", .valid_mask = COMPARE_ALLOW_TEXTUAL }, + { COMPARE_GREATER, "gt", .valid_mask = COMPARE_ALLOW_TEXTUAL }, }; assert(s); diff --git a/src/shared/compare-operator.h b/src/shared/compare-operator.h index 363b4bdc75..db5c2f0cfc 100644 --- a/src/shared/compare-operator.h +++ b/src/shared/compare-operator.h @@ -49,6 +49,7 @@ static inline bool COMPARE_OPERATOR_IS_ORDER(CompareOperator c) { typedef enum CompareOperatorParseFlags { COMPARE_ALLOW_FNMATCH = 1 << 0, COMPARE_EQUAL_BY_STRING = 1 << 1, + COMPARE_ALLOW_TEXTUAL = 1 << 2, } CompareOperatorParseFlags; CompareOperator parse_compare_operator(const char **s, CompareOperatorParseFlags flags); From 5f6b2d394b4e2d9dbad16e386d6bd9083ea40393 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 29 Aug 2022 11:08:18 +0200 Subject: [PATCH 11/17] compare: propagate errors of fnmatch() as errors --- src/shared/compare-operator.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/shared/compare-operator.c b/src/shared/compare-operator.c index 694c778a27..e9c7cc9b2b 100644 --- a/src/shared/compare-operator.c +++ b/src/shared/compare-operator.c @@ -90,6 +90,7 @@ int version_or_fnmatch_compare( CompareOperator op, const char *a, const char *b) { + int r; switch (op) { @@ -100,10 +101,14 @@ int version_or_fnmatch_compare( return !streq_ptr(a, b); case COMPARE_FNMATCH_EQUAL: - return fnmatch(b, a, 0) != FNM_NOMATCH; + r = fnmatch(b, a, 0); + return r == 0 ? true : + r == FNM_NOMATCH ? false : -EINVAL; case COMPARE_FNMATCH_UNEQUAL: - return fnmatch(b, a, 0) == FNM_NOMATCH; + r = fnmatch(b, a, 0); + return r == FNM_NOMATCH ? true: + r == 0 ? false : -EINVAL; case _COMPARE_OPERATOR_ORDER_FIRST..._COMPARE_OPERATOR_ORDER_LAST: return test_order(strverscmp_improved(a, b), op); From 4803b0bcaa21616d2feb8e38998a95e28aa43d54 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 29 Aug 2022 11:09:48 +0200 Subject: [PATCH 12/17] compare: add macro for operator charset --- src/shared/compare-operator.h | 3 +++ src/shared/condition.c | 4 ++-- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/src/shared/compare-operator.h b/src/shared/compare-operator.h index db5c2f0cfc..900f3e54b2 100644 --- a/src/shared/compare-operator.h +++ b/src/shared/compare-operator.h @@ -4,6 +4,9 @@ #include #include +#define COMPARE_OPERATOR_CHARS "!<=>" +#define COMPARE_OPERATOR_WITH_FNMATCH_CHARS COMPARE_OPERATOR_CHARS "$" + typedef enum CompareOperator { /* Listed in order of checking. Note that some comparators are prefixes of others, hence the longest * should be listed first. */ diff --git a/src/shared/condition.c b/src/shared/condition.c index 4e5702bc41..2d9979b9c4 100644 --- a/src/shared/condition.c +++ b/src/shared/condition.c @@ -257,7 +257,7 @@ static int condition_test_osrelease(Condition *c, char **env) { /* parse_compare_operator() needs the string to start with the comparators */ word = condition; - r = extract_first_word(&word, &key, "!<=>$", EXTRACT_RETAIN_SEPARATORS); + r = extract_first_word(&word, &key, COMPARE_OPERATOR_WITH_FNMATCH_CHARS, EXTRACT_RETAIN_SEPARATORS); if (r < 0) return log_debug_errno(r, "Failed to parse parameter: %m"); /* The os-release spec mandates env-var-like key names */ @@ -518,7 +518,7 @@ static int condition_test_firmware_smbios_field(const char *expression) { assert(expression); /* Parse SMBIOS field */ - r = extract_first_word(&expression, &field, "!<=>$", EXTRACT_RETAIN_SEPARATORS); + r = extract_first_word(&expression, &field, COMPARE_OPERATOR_WITH_FNMATCH_CHARS, EXTRACT_RETAIN_SEPARATORS); if (r < 0) return r; if (r == 0 || isempty(expression)) From c9907425238bdfd07c0a054debb3232ea2a944ef Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 29 Aug 2022 11:10:18 +0200 Subject: [PATCH 13/17] condition: allow fnmatch() matches in ConditionKernelVersion= This is mostly to make things systematic, and brings no new functionality, as not specifying any operator is identical to prefixing with =$ anyway. --- man/systemd.unit.xml | 10 +++++----- src/shared/condition.c | 44 +++++++++++++++++++++--------------------- 2 files changed, 27 insertions(+), 27 deletions(-) diff --git a/man/systemd.unit.xml b/man/systemd.unit.xml index c5ba817579..9dd02c3060 100644 --- a/man/systemd.unit.xml +++ b/man/systemd.unit.xml @@ -1334,11 +1334,11 @@ ConditionKernelVersion= may be used to check whether the kernel version (as reported by uname -r) matches a certain expression (or if prefixed with the exclamation mark does not match it). The argument must be a list of (potentially quoted) - expressions. For each of the expressions, if it starts with one of <, - <=, = (or ==), != - (or <>), >=, > a relative - version comparison is done, otherwise the specified string is matched with shell-style - globs. + expressions. Each expression starts with one of <, <=, + = (or ==), != (or + <>), >=, > for a relative + version comparison, or =$, !=$ for a shell-style glob + match. If no operator is specified =$ is implied. Note that using the kernel version string is an unreliable way to determine which features are supported by a kernel, because of the widespread practice of backporting drivers, features, and diff --git a/src/shared/condition.c b/src/shared/condition.c index 2d9979b9c4..f893937156 100644 --- a/src/shared/condition.c +++ b/src/shared/condition.c @@ -206,30 +206,30 @@ static int condition_test_kernel_version(Condition *c, char **env) { break; s = strstrip(word); - operator = parse_compare_operator(&s, 0); - if (operator >= 0) { - s += strspn(s, WHITESPACE); - if (isempty(s)) { - if (first) { - /* For backwards compatibility, allow whitespace between the operator and - * value, without quoting, but only in the first expression. */ - word = mfree(word); - r = extract_first_word(&p, &word, NULL, 0); - if (r < 0) - return log_debug_errno(r, "Failed to parse condition string \"%s\": %m", p); - if (r == 0) - return log_debug_errno(SYNTHETIC_ERRNO(EINVAL), "Unexpected end of expression: %s", p); - s = word; - } else + operator = parse_compare_operator(&s, COMPARE_ALLOW_FNMATCH); + if (operator < 0) /* No prefix? Then treat as glob string */ + operator = COMPARE_FNMATCH_EQUAL; + + s += strspn(s, WHITESPACE); + if (isempty(s)) { + if (first) { + /* For backwards compatibility, allow whitespace between the operator and + * value, without quoting, but only in the first expression. */ + word = mfree(word); + r = extract_first_word(&p, &word, NULL, 0); + if (r < 0) + return log_debug_errno(r, "Failed to parse condition string \"%s\": %m", p); + if (r == 0) return log_debug_errno(SYNTHETIC_ERRNO(EINVAL), "Unexpected end of expression: %s", p); - } + s = word; + } else + return log_debug_errno(SYNTHETIC_ERRNO(EINVAL), "Unexpected end of expression: %s", p); + } - r = test_order(strverscmp_improved(u.release, s), operator); - } else - /* No prefix? Then treat as glob string */ - r = fnmatch(s, u.release, 0) == 0; - - if (r == 0) + r = version_or_fnmatch_compare(operator, u.release, s); + if (r < 0) + return r; + if (!r) return false; first = false; From 666d314a743a59005b4ccef5df5811bd135a3d9d Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 29 Aug 2022 11:10:38 +0200 Subject: [PATCH 14/17] condition: allow literal string compares in SMBIOS condition This ensures that "=" and "!=" are now interpreted as literal string compares, and "==" and "<>" are for version compares. This is not a compat break, since the SMBIOS stuff has not been included in any release yet. Main reason to do this, is to be systematic with the other conditions that check for text stuff. --- src/shared/condition.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/shared/condition.c b/src/shared/condition.c index f893937156..6c8023143b 100644 --- a/src/shared/condition.c +++ b/src/shared/condition.c @@ -528,7 +528,7 @@ static int condition_test_firmware_smbios_field(const char *expression) { delete_trailing_chars(field, WHITESPACE); /* Parse operator */ - operator = parse_compare_operator(&expression, COMPARE_ALLOW_FNMATCH); + operator = parse_compare_operator(&expression, COMPARE_ALLOW_FNMATCH|COMPARE_EQUAL_BY_STRING); if (operator < 0) return operator; From 06219747f5368909e8006c7b6138aca671a3b9d7 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 29 Aug 2022 13:27:11 +0200 Subject: [PATCH 15/17] condition: change ConditionKernelVersion= so that =/!= mean literal string comparison, and ==/<> version comparison The only reason to do this is to ensure uniformity with the other options, that work like this, i.e. ConditionOSRelease= or ConditionSecurity=. This is a compatibility break, but a minor one, given that string comparison and version comparison is mostly the same for equality and inequality. --- man/systemd.unit.xml | 6 +++--- src/shared/condition.c | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/man/systemd.unit.xml b/man/systemd.unit.xml index 9dd02c3060..16aa8303e7 100644 --- a/man/systemd.unit.xml +++ b/man/systemd.unit.xml @@ -1334,9 +1334,9 @@ ConditionKernelVersion= may be used to check whether the kernel version (as reported by uname -r) matches a certain expression (or if prefixed with the exclamation mark does not match it). The argument must be a list of (potentially quoted) - expressions. Each expression starts with one of <, <=, - = (or ==), != (or - <>), >=, > for a relative + expressions. Each expression starts with one of = or != for + string comparisons, <, <=, ==, + <>, >=, > for a relative version comparison, or =$, !=$ for a shell-style glob match. If no operator is specified =$ is implied. diff --git a/src/shared/condition.c b/src/shared/condition.c index 6c8023143b..ffca2006c0 100644 --- a/src/shared/condition.c +++ b/src/shared/condition.c @@ -206,7 +206,7 @@ static int condition_test_kernel_version(Condition *c, char **env) { break; s = strstrip(word); - operator = parse_compare_operator(&s, COMPARE_ALLOW_FNMATCH); + operator = parse_compare_operator(&s, COMPARE_ALLOW_FNMATCH|COMPARE_EQUAL_BY_STRING); if (operator < 0) /* No prefix? Then treat as glob string */ operator = COMPARE_FNMATCH_EQUAL; From 71a3ff036be64cd9c4f66c02598be58666a1d132 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 29 Aug 2022 13:42:44 +0200 Subject: [PATCH 16/17] condition: change operator logic to use $= instead of =$ for glob comparisons So this is a bit of a bikeshedding thing. But I think we should do this nonetheless, before this is released. Playing around with the glob matches I realized that "=$" is really hard to grep for, since in shell code it's an often seen construct. Also, when reading code I often found myself thinking first that the "$" belongs to the rvalue instead of the operator, in a variable expansion scheme. If we move the $ character to the left hand, I think we are on the safer side, since usually lvalues are much more restricted in character sets than rvalues (at least most programming languages do enforce limits on the character set for identifiers). It makes it much easier to grep for the new operator, and easier to read too. Example: before: ConditionOSRelease=ID=$fedora-* after: ConditionOSRelease=ID$=fedora-* --- man/systemd.unit.xml | 10 +++++----- src/shared/compare-operator.c | 4 ++-- src/test/test-condition.c | 14 +++++++------- 3 files changed, 14 insertions(+), 14 deletions(-) diff --git a/man/systemd.unit.xml b/man/systemd.unit.xml index 16aa8303e7..02d5e72aee 100644 --- a/man/systemd.unit.xml +++ b/man/systemd.unit.xml @@ -1251,9 +1251,9 @@ operator is one of <, <=, >=, >, ==, <> for version comparison, = and != - for literal string comparison, or =$, !=$ for shell-style + for literal string comparison, or $=, !$= for shell-style glob comparison. value is the expected value of the SMBIOS field value - (possibly containing shell style globs in case =$/!=$ is + (possibly containing shell style globs in case $=/!$= is used). @@ -1337,8 +1337,8 @@ expressions. Each expression starts with one of = or != for string comparisons, <, <=, ==, <>, >=, > for a relative - version comparison, or =$, !=$ for a shell-style glob - match. If no operator is specified =$ is implied. + version comparison, or $=, !$= for a shell-style glob + match. If no operator is specified $= is implied. Note that using the kernel version string is an unreliable way to determine which features are supported by a kernel, because of the widespread practice of backporting drivers, features, and @@ -1704,7 +1704,7 @@ with <, <=, ==, <>, >=, >), and shell-style wildcard comparisons (*, ?, []) are - supported with the =$ (match) and !=$ (non-match). + supported with the $= (match) and !$= (non-match). diff --git a/src/shared/compare-operator.c b/src/shared/compare-operator.c index e9c7cc9b2b..ad931b778f 100644 --- a/src/shared/compare-operator.c +++ b/src/shared/compare-operator.c @@ -12,8 +12,8 @@ CompareOperator parse_compare_operator(const char **s, CompareOperatorParseFlags CompareOperatorParseFlags valid_mask; /* If this operator appears when flags in mask not set, fail */ CompareOperatorParseFlags need_mask; /* Skip over this operattor when flags in mask not set */ } table[] = { - { COMPARE_FNMATCH_EQUAL, "=$", .valid_mask = COMPARE_ALLOW_FNMATCH }, - { COMPARE_FNMATCH_UNEQUAL, "!=$", .valid_mask = COMPARE_ALLOW_FNMATCH }, + { COMPARE_FNMATCH_EQUAL, "$=", .valid_mask = COMPARE_ALLOW_FNMATCH }, + { COMPARE_FNMATCH_UNEQUAL, "!$=", .valid_mask = COMPARE_ALLOW_FNMATCH }, { COMPARE_UNEQUAL, "<>" }, { COMPARE_LOWER_OR_EQUAL, "<=" }, diff --git a/src/test/test-condition.c b/src/test/test-condition.c index f93619293f..10494b010c 100644 --- a/src/test/test-condition.c +++ b/src/test/test-condition.c @@ -359,31 +359,31 @@ TEST(condition_test_firmware_smbios_field) { const char *quote = strchr(bios_vendor, ' ') ? "\"" : ""; /* Test equality / inequality using fnmatch() */ - expression = strjoina("smbios-field(bios_vendor =$ ", quote, bios_vendor, quote, ")"); + expression = strjoina("smbios-field(bios_vendor $= ", quote, bios_vendor, quote, ")"); condition = condition_new(CONDITION_FIRMWARE, expression, false, false); assert_se(condition); assert_se(condition_test(condition, environ) > 0); condition_free(condition); - expression = strjoina("smbios-field(bios_vendor=$", quote, bios_vendor, quote, ")"); + expression = strjoina("smbios-field(bios_vendor$=", quote, bios_vendor, quote, ")"); condition = condition_new(CONDITION_FIRMWARE, expression, false, false); assert_se(condition); assert_se(condition_test(condition, environ) > 0); condition_free(condition); - expression = strjoina("smbios-field(bios_vendor !=$ ", quote, bios_vendor, quote, ")"); + expression = strjoina("smbios-field(bios_vendor !$= ", quote, bios_vendor, quote, ")"); condition = condition_new(CONDITION_FIRMWARE, expression, false, false); assert_se(condition); assert_se(condition_test(condition, environ) == 0); condition_free(condition); - expression = strjoina("smbios-field(bios_vendor!=$", quote, bios_vendor, quote, ")"); + expression = strjoina("smbios-field(bios_vendor!$=", quote, bios_vendor, quote, ")"); condition = condition_new(CONDITION_FIRMWARE, expression, false, false); assert_se(condition); assert_se(condition_test(condition, environ) == 0); condition_free(condition); - expression = strjoina("smbios-field(bios_vendor =$ ", quote, bios_vendor, "*", quote, ")"); + expression = strjoina("smbios-field(bios_vendor $= ", quote, bios_vendor, "*", quote, ")"); condition = condition_new(CONDITION_FIRMWARE, expression, false, false); assert_se(condition); assert_se(condition_test(condition, environ) > 0); @@ -1174,13 +1174,13 @@ TEST(condition_test_os_release) { condition_free(condition); /* Test fnmatch() operators */ - key_value_pair = strjoina(os_release_pairs[0], "=$", quote, os_release_pairs[1], quote); + key_value_pair = strjoina(os_release_pairs[0], "$=", quote, os_release_pairs[1], quote); condition = condition_new(CONDITION_OS_RELEASE, key_value_pair, false, false); assert_se(condition); assert_se(condition_test(condition, environ) > 0); condition_free(condition); - key_value_pair = strjoina(os_release_pairs[0], "!=$", quote, os_release_pairs[1], quote); + key_value_pair = strjoina(os_release_pairs[0], "!$=", quote, os_release_pairs[1], quote); condition = condition_new(CONDITION_OS_RELEASE, key_value_pair, false, false); assert_se(condition); assert_se(condition_test(condition, environ) == 0); From 10736074b492b8a1d2583862c0b0e08077f59ab2 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 1 Sep 2022 23:20:11 +0200 Subject: [PATCH 17/17] mention ConditionKernelVersion= compat break in NEWS --- NEWS | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/NEWS b/NEWS index f6f49cb902..fe91391627 100644 --- a/NEWS +++ b/NEWS @@ -2,7 +2,7 @@ systemd System and Service Manager CHANGES WITH 252 in spe: - Announcement of Future Feature Removal + Announcement of Future Feature Removal: * Please note that we intend to remove cgroupsv1 support from systemd release after EOY 2023. If you run services that make explicit use of @@ -10,7 +10,20 @@ CHANGES WITH 252 in spe: sooner rather than later, if you haven't done so yet. Most of Linux userspace has been ported over already. - New features: + Compatibility Breaks: + + * ConditionKernelVersion= checks that use the = or != operator will now + do simple string compares (as opposed to version compare – á la + stverscmp() — as before, which is still done for the ordering + operators <, >, <=, >=). Moreover, if no operator is specified a + shell-style glob match is now done. This creates a minor + incompatibility compared to older systemd versions, in case the *, ?, + [, ], characters have been used in such condition expressions before, + as these will now match per shell glob rules instead of + literally. Given that kernel version strings typically do not include + these characters we expect little breakage through this change. + + New Features: * systemd-measure is a new helper to precalculate PCR measurements to make it easier to set TPM2 policies.