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-*

diff --git a/man/systemd.unit.xml b/man/systemd.unit.xml
index 16aa8303e799dcf27b68d091eb4c2450ebd31c17..02d5e72aeebf717244afa03b690d9cda567d4165 100644 (file)
--- a/man/systemd.unit.xml
+++ b/man/systemd.unit.xml
@@ -1251,9 +1251,9 @@
           <literal>operator</literal> is one of <literal>&lt;</literal>, <literal>&lt;=</literal>,
           <literal>&gt;=</literal>, <literal>&gt;</literal>, <literal>==</literal>,
           <literal>&lt;&gt;</literal> for version comparison, <literal>=</literal> and <literal>!=</literal>
-          for literal string comparison, or <literal>=$</literal>, <literal>!=$</literal> for shell-style
+          for literal string comparison, or <literal>$=</literal>, <literal>!$=</literal> for shell-style
           glob comparison.  <literal>value</literal> is the expected value of the SMBIOS field value
-          (possibly containing shell style globs in case <literal>=$</literal>/<literal>!=$</literal> is
+          (possibly containing shell style globs in case <literal>$=</literal>/<literal>!$=</literal> is
           used).</para>
           </listitem>
         </varlistentry>
@@ -1337,8 +1337,8 @@
           expressions.  Each expression starts with one of <literal>=</literal> or <literal>!=</literal> for
           string comparisons, <literal>&lt;</literal>, <literal>&lt;=</literal>, <literal>==</literal>,
           <literal>&lt;&gt;</literal>, <literal>&gt;=</literal>, <literal>&gt;</literal> for a relative
-          version comparison, or <literal>=$</literal>, <literal>!=$</literal> for a shell-style glob
-          match. If no operator is specified <literal>=$</literal> is implied.</para>
+          version comparison, or <literal>$=</literal>, <literal>!$=</literal> for a shell-style glob
+          match. If no operator is specified <literal>$=</literal> is implied.</para>
 
           <para>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 <literal>&lt;</literal>, <literal>&lt;=</literal>, <literal>==</literal>,
           <literal>&lt;&gt;</literal>, <literal>&gt;=</literal>, <literal>&gt;</literal>), and shell-style
           wildcard comparisons (<literal>*</literal>, <literal>?</literal>, <literal>[]</literal>) are
-          supported with the <literal>=$</literal> (match) and <literal>!=$</literal> (non-match).</para>
+          supported with the <literal>$=</literal> (match) and <literal>!$=</literal> (non-match).</para>
           </listitem>
         </varlistentry>
 

diff --git a/src/shared/compare-operator.c b/src/shared/compare-operator.c
index e9c7cc9b2b20335c20f39e6d2639dfe0242f3031..ad931b778f6805a3a2f41607ec921c4f2da00901 100644 (file)
--- 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 f93619293fa8c32d285c6e47fe8d95a8e0d3bb9a..10494b010c3e37cd7bd887f64df61dc7b8ef9cd7 100644 (file)
--- 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);