From 71a3ff036be64cd9c4f66c02598be58666a1d132 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 29 Aug 2022 13:42:44 +0200 Subject: [PATCH] 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 16aa8303e79..02d5e72aeeb 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 e9c7cc9b2b2..ad931b778f6 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 f93619293fa..10494b010c3 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);