From 68df2df66857b597a2adbb29d27d709a5d59a5a3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 17 May 2022 13:26:47 +0200 Subject: [PATCH 01/11] Clean up the text in description of strverscmp_improved() --- docs/DISCOVERABLE_PARTITIONS.md | 10 +++++----- src/fundamental/string-util-fundamental.c | 18 ++++++++++-------- 2 files changed, 15 insertions(+), 13 deletions(-) diff --git a/docs/DISCOVERABLE_PARTITIONS.md b/docs/DISCOVERABLE_PARTITIONS.md index 85e858921fb..b7c9c1e2ebf 100644 --- a/docs/DISCOVERABLE_PARTITIONS.md +++ b/docs/DISCOVERABLE_PARTITIONS.md @@ -333,11 +333,11 @@ directory is found to be populated already in the root partition, the automatic discovery _must not_ mount any discovered file system over it. Optionally, in case of the root, `/usr/` and their Verity partitions instead of strictly mounting the first suitable partition an OS might choose to mount the partition -whose label compares the highest according to `strverscmp()` or a similar -logic, in order to implement a simple partition-based A/B versioning -scheme. The precise rules are left for the implementation to decide, but when -in doubt earlier partitions (by their index) should always win over later -partitions if the label comparison is inconclusive. +whose label compares the highest according to `strverscmp()` or similar logic, +in order to implement a simple partition-based A/B versioning scheme. The +precise rules are left for the implementation to decide, but when in doubt +earlier partitions (by their index) should always win over later partitions if +the label comparison is inconclusive. A *container* *manager* should automatically discover and mount the root, `/usr/`, `/home/`, `/srv/`, `/var/`, `/var/tmp/` partitions inside a container diff --git a/src/fundamental/string-util-fundamental.c b/src/fundamental/string-util-fundamental.c index 101d3f71962..c9b1b89b3e9 100644 --- a/src/fundamental/string-util-fundamental.c +++ b/src/fundamental/string-util-fundamental.c @@ -93,20 +93,22 @@ static sd_bool is_valid_version_char(sd_char a) { } sd_int strverscmp_improved(const sd_char *a, const sd_char *b) { - - /* This is based on RPM's rpmvercmp(). But this explicitly handles '-' and '.', as we usually - * want to directly compare strings which contain both version and release; e.g. - * '247.2-3.1.fc33.x86_64' or '5.11.0-0.rc5.20210128git76c057c84d28.137.fc34'. - * Unlike rpmvercmp(), this distiguishes e.g. 123a and 123.a, and 123a is newer. + /* This function is similar to strverscmp(3), but it treats '-' and '.' as separators. * - * This splits the input strings into segments. Each segment is numeric or alpha, and may be + * The logic is based on rpm's rpmvercmp(), but unlike rpmvercmp(), it distiguishes e.g. + * '123a' and '123.a', with '123a' being newer. + * + * It allows direct comparison of strings which contain both a version and a release; e.g. + * '247.2-3.1.fc33.x86_64' or '5.11.0-0.rc5.20210128git76c057c84d28.137.fc34'. + * + * The input string is split into segments. Each segment is numeric or alphabetic, and may be * prefixed with the following: * '~' : used for pre-releases, a segment prefixed with this is the oldest, * '-' : used for the separator between version and release, * '^' : used for patched releases, a segment with this is newer than one with '-'. * '.' : used for point releases. - * Note, no prefix segment is the newest. All non-supported characters are dropped, and - * handled as a separator of segments, e.g., 123_a is equivalent to 123a. + * Note that no prefix segment is the newest. All non-supported characters are dropped, and + * handled as a separator of segments, e.g., '123_a' is equivalent to '123a'. * * By using this, version strings can be sorted like following: * (older) 122.1 From c3e4cbe0c52ce6d0d986e6b650ca051f6eeffc68 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 17 May 2022 14:15:44 +0200 Subject: [PATCH 02/11] basic: make macro-fundamental.h self-contained When !SD_BOOT, it used size_t without including the appropriate header. --- src/fundamental/macro-fundamental.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/fundamental/macro-fundamental.h b/src/fundamental/macro-fundamental.h index 1c198f6ad9a..0e3e22d4352 100644 --- a/src/fundamental/macro-fundamental.h +++ b/src/fundamental/macro-fundamental.h @@ -2,7 +2,8 @@ #pragma once #ifndef SD_BOOT -#include +# include +# include #endif #include From e77a55c599824df0f1dbbda0d2e4b920760c26ca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 17 May 2022 14:17:03 +0200 Subject: [PATCH 03/11] fundamental/string-util-fundamental: include appropriate headers We were using CMP() without pulling the definition in directly. --- src/fundamental/string-util-fundamental.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/fundamental/string-util-fundamental.c b/src/fundamental/string-util-fundamental.c index c9b1b89b3e9..0d8a820bacb 100644 --- a/src/fundamental/string-util-fundamental.c +++ b/src/fundamental/string-util-fundamental.c @@ -1,10 +1,10 @@ /* SPDX-License-Identifier: LGPL-2.1-or-later */ #ifndef SD_BOOT -#include - -#include "macro.h" +# include #endif + +#include "macro-fundamental.h" #include "string-util-fundamental.h" sd_char *startswith(const sd_char *s, const sd_char *prefix) { From dbf43adce2863e4362d6fa286bf77fd2dc47cdc1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 17 May 2022 14:19:19 +0200 Subject: [PATCH 04/11] fundamental: make strverscmp_improved() return -1/0/+1 in all cases We would return the result of strcmp(), i.e. some positive/negative value. Now that we want to make this a documented interface for other people to implement, let's make the implementation more contstrained, even if we ourselves don't care about whether the specific values. --- src/fundamental/string-util-fundamental.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/fundamental/string-util-fundamental.c b/src/fundamental/string-util-fundamental.c index 0d8a820bacb..57b4d535f3a 100644 --- a/src/fundamental/string-util-fundamental.c +++ b/src/fundamental/string-util-fundamental.c @@ -126,7 +126,7 @@ sd_int strverscmp_improved(const sd_char *a, const sd_char *b) { */ if (isempty(a) || isempty(b)) - return strcmp_ptr(a, b); + return CMP(strcmp_ptr(a, b), 0); for (;;) { const sd_char *aa, *bb; @@ -208,7 +208,7 @@ sd_int strverscmp_improved(const sd_char *a, const sd_char *b) { return r; /* Then, compare them as strings. */ - r = strncmp(a, b, aa - a); + r = CMP(strncmp(a, b, aa - a), 0); if (r != 0) return r; } else { @@ -219,7 +219,7 @@ sd_int strverscmp_improved(const sd_char *a, const sd_char *b) { ; /* Note that the segments are usually not NUL-terminated. */ - r = strncmp(a, b, MIN(aa - a, bb - b)); + r = CMP(strncmp(a, b, MIN(aa - a, bb - b)), 0); if (r != 0) return r; From d970092fa5eb41ae095408b3692f1bb997802869 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 17 May 2022 14:33:08 +0200 Subject: [PATCH 05/11] test-string-util: include a copy of rpm's version comparison tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We said that strverscmp_improved() is similar to rpm, so it's nice to include their tests too so we can pin down the differences. Our test is changed to print olderolder. (I know the computer doesn't care, but I find it much harder to think about when newer is on the left…) The rpm test strings are copied from https://github.com/rpm-software-management/rpm/blob/master/tests/rpmvercmp.at. rpmio is licensed GPL OR LGPL, so we can do that without any issue. (I think it could be argued as "fair use" anyway, but that's not necessary in this case.) I kept the original form as much as possible so it'll be easy to copy things back and forth in the future. --- src/test/test-string-util.c | 213 +++++++++++++++++++++++++++++++----- 1 file changed, 185 insertions(+), 28 deletions(-) diff --git a/src/test/test-string-util.c b/src/test/test-string-util.c index 00163d501ec..d0a4eca3906 100644 --- a/src/test/test-string-util.c +++ b/src/test/test-string-util.c @@ -828,13 +828,24 @@ TEST(string_contains_word) { assert_se(!string_contains_word("a:b:cc", ":#", ":cc")); } -static void test_strverscmp_improved_one(const char *newer, const char *older) { - log_info("/* %s(%s, %s) */", __func__, strnull(newer), strnull(older)); +static void test_strverscmp_improved_one(const char* a, const char *b, int expected) { + int r = strverscmp_improved(a, b); + + log_info("'%s' %s '%s'%s", + strnull(a), + r > 0 ? ">" : r < 0 ? "<" : "==", + strnull(b), + r == expected ? "" : " !!!!!!!!!!!!!"); + assert_se(r == expected); +} + +static void test_strverscmp_improved_newer(const char *older, const char *newer) { + test_strverscmp_improved_one(older, newer, -1); - assert_se(strverscmp_improved(newer, newer) == 0); - assert_se(strverscmp_improved(newer, older) > 0); - assert_se(strverscmp_improved(older, newer) < 0); assert_se(strverscmp_improved(older, older) == 0); + assert_se(strverscmp_improved(older, newer) < 0); + assert_se(strverscmp_improved(newer, older) > 0); + assert_se(strverscmp_improved(newer, newer) == 0); } TEST(strverscmp_improved) { @@ -868,43 +879,189 @@ TEST(strverscmp_improved) { STRV_FOREACH(p, versions) STRV_FOREACH(q, p + 1) - test_strverscmp_improved_one(*q, *p); + test_strverscmp_improved_newer(*p, *q); - test_strverscmp_improved_one("123.45-67.89", "123.45-67.88"); - test_strverscmp_improved_one("123.45-67.89a", "123.45-67.89"); - test_strverscmp_improved_one("123.45-67.89", "123.45-67.ab"); - test_strverscmp_improved_one("123.45-67.89", "123.45-67.9"); - test_strverscmp_improved_one("123.45-67.89", "123.45-67"); - test_strverscmp_improved_one("123.45-67.89", "123.45-66.89"); - test_strverscmp_improved_one("123.45-67.89", "123.45-9.99"); - test_strverscmp_improved_one("123.45-67.89", "123.42-99.99"); - test_strverscmp_improved_one("123.45-67.89", "123-99.99"); + test_strverscmp_improved_newer("123.45-67.88", "123.45-67.89"); + test_strverscmp_improved_newer("123.45-67.89", "123.45-67.89a"); + test_strverscmp_improved_newer("123.45-67.ab", "123.45-67.89"); + test_strverscmp_improved_newer("123.45-67.9", "123.45-67.89"); + test_strverscmp_improved_newer("123.45-67", "123.45-67.89"); + test_strverscmp_improved_newer("123.45-66.89", "123.45-67.89"); + test_strverscmp_improved_newer("123.45-9.99", "123.45-67.89"); + test_strverscmp_improved_newer("123.42-99.99", "123.45-67.89"); + test_strverscmp_improved_newer("123-99.99", "123.45-67.89"); /* '~' : pre-releases */ - test_strverscmp_improved_one("123.45-67.89", "123~rc1-99.99"); - test_strverscmp_improved_one("123-45.67.89", "123~rc1-99.99"); - test_strverscmp_improved_one("123~rc2-67.89", "123~rc1-99.99"); - test_strverscmp_improved_one("123^aa2-67.89", "123~rc1-99.99"); - test_strverscmp_improved_one("123aa2-67.89", "123~rc1-99.99"); + test_strverscmp_improved_newer("123~rc1-99.99", "123.45-67.89"); + test_strverscmp_improved_newer("123~rc1-99.99", "123-45.67.89"); + test_strverscmp_improved_newer("123~rc1-99.99", "123~rc2-67.89"); + test_strverscmp_improved_newer("123~rc1-99.99", "123^aa2-67.89"); + test_strverscmp_improved_newer("123~rc1-99.99", "123aa2-67.89"); /* '-' : separator between version and release. */ - test_strverscmp_improved_one("123.45-67.89", "123-99.99"); - test_strverscmp_improved_one("123^aa2-67.89", "123-99.99"); - test_strverscmp_improved_one("123aa2-67.89", "123-99.99"); + test_strverscmp_improved_newer("123-99.99", "123.45-67.89"); + test_strverscmp_improved_newer("123-99.99", "123^aa2-67.89"); + test_strverscmp_improved_newer("123-99.99", "123aa2-67.89"); /* '^' : patch releases */ - test_strverscmp_improved_one("123.45-67.89", "123^45-67.89"); - test_strverscmp_improved_one("123^aa2-67.89", "123^aa1-99.99"); - test_strverscmp_improved_one("123aa2-67.89", "123^aa2-67.89"); + test_strverscmp_improved_newer("123^45-67.89", "123.45-67.89"); + test_strverscmp_improved_newer("123^aa1-99.99", "123^aa2-67.89"); + test_strverscmp_improved_newer("123^aa2-67.89", "123aa2-67.89"); /* '.' : point release */ - test_strverscmp_improved_one("123aa2-67.89", "123.aa2-67.89"); - test_strverscmp_improved_one("123.ab2-67.89", "123.aa2-67.89"); + test_strverscmp_improved_newer("123.aa2-67.89", "123aa2-67.89"); + test_strverscmp_improved_newer("123.aa2-67.89", "123.ab2-67.89"); /* invalid characters */ assert_se(strverscmp_improved("123_aa2-67.89", "123aa+2-67.89") == 0); } +#define RPMVERCMP(a, b, c) \ + test_strverscmp_improved_one(STRINGIFY(a), STRINGIFY(b), (c)) + +TEST(strverscmp_improved_rpm) { + /* Tests copied from rmp's rpmio test suite, under the LGPL license: + * https://github.com/rpm-software-management/rpm/blob/master/tests/rpmvercmp.at. + * The original form is retained for easy comparisons and updates. + */ + + RPMVERCMP(1.0, 1.0, 0); + RPMVERCMP(1.0, 2.0, -1); + RPMVERCMP(2.0, 1.0, 1); + + RPMVERCMP(2.0.1, 2.0.1, 0); + RPMVERCMP(2.0, 2.0.1, -1); + RPMVERCMP(2.0.1, 2.0, 1); + + RPMVERCMP(2.0.1a, 2.0.1a, 0); + RPMVERCMP(2.0.1a, 2.0.1, 1); + RPMVERCMP(2.0.1, 2.0.1a, -1); + + RPMVERCMP(5.5p1, 5.5p1, 0); + RPMVERCMP(5.5p1, 5.5p2, -1); + RPMVERCMP(5.5p2, 5.5p1, 1); + + RPMVERCMP(5.5p10, 5.5p10, 0); + RPMVERCMP(5.5p1, 5.5p10, -1); + RPMVERCMP(5.5p10, 5.5p1, 1); + + RPMVERCMP(10xyz, 10.1xyz, 1); /* Note: this is reversed from rpm's vercmp */ + RPMVERCMP(10.1xyz, 10xyz, -1); /* Note: this is reversed from rpm's vercmp */ + + RPMVERCMP(xyz10, xyz10, 0); + RPMVERCMP(xyz10, xyz10.1, -1); + RPMVERCMP(xyz10.1, xyz10, 1); + + RPMVERCMP(xyz.4, xyz.4, 0); + RPMVERCMP(xyz.4, 8, -1); + RPMVERCMP(8, xyz.4, 1); + RPMVERCMP(xyz.4, 2, -1); + RPMVERCMP(2, xyz.4, 1); + + RPMVERCMP(5.5p2, 5.6p1, -1); + RPMVERCMP(5.6p1, 5.5p2, 1); + + RPMVERCMP(5.6p1, 6.5p1, -1); + RPMVERCMP(6.5p1, 5.6p1, 1); + + RPMVERCMP(6.0.rc1, 6.0, 1); + RPMVERCMP(6.0, 6.0.rc1, -1); + + RPMVERCMP(10b2, 10a1, 1); + RPMVERCMP(10a2, 10b2, -1); + + RPMVERCMP(1.0aa, 1.0aa, 0); + RPMVERCMP(1.0a, 1.0aa, -1); + RPMVERCMP(1.0aa, 1.0a, 1); + + RPMVERCMP(10.0001, 10.0001, 0); + RPMVERCMP(10.0001, 10.1, 0); + RPMVERCMP(10.1, 10.0001, 0); + RPMVERCMP(10.0001, 10.0039, -1); + RPMVERCMP(10.0039, 10.0001, 1); + + RPMVERCMP(4.999.9, 5.0, -1); + RPMVERCMP(5.0, 4.999.9, 1); + + RPMVERCMP(20101121, 20101121, 0); + RPMVERCMP(20101121, 20101122, -1); + RPMVERCMP(20101122, 20101121, 1); + + RPMVERCMP(2_0, 2_0, 0); + RPMVERCMP(2.0, 2_0, -1); /* Note: in rpm those compare equal */ + RPMVERCMP(2_0, 2.0, 1); /* Note: in rpm those compare equal */ + + /* RhBug:178798 case */ + RPMVERCMP(a, a, 0); + RPMVERCMP(a+, a+, 0); + RPMVERCMP(a+, a_, 0); + RPMVERCMP(a_, a+, 0); + RPMVERCMP(+a, +a, 0); + RPMVERCMP(+a, _a, 0); + RPMVERCMP(_a, +a, 0); + RPMVERCMP(+_, +_, 0); + RPMVERCMP(_+, +_, 0); + RPMVERCMP(_+, _+, 0); + RPMVERCMP(+, _, 0); + RPMVERCMP(_, +, 0); + + /* Basic testcases for tilde sorting */ + RPMVERCMP(1.0~rc1, 1.0~rc1, 0); + RPMVERCMP(1.0~rc1, 1.0, -1); + RPMVERCMP(1.0, 1.0~rc1, 1); + RPMVERCMP(1.0~rc1, 1.0~rc2, -1); + RPMVERCMP(1.0~rc2, 1.0~rc1, 1); + RPMVERCMP(1.0~rc1~git123, 1.0~rc1~git123, 0); + RPMVERCMP(1.0~rc1~git123, 1.0~rc1, -1); + RPMVERCMP(1.0~rc1, 1.0~rc1~git123, 1); + + /* Basic testcases for caret sorting */ + RPMVERCMP(1.0^, 1.0^, 0); + RPMVERCMP(1.0^, 1.0, 1); + RPMVERCMP(1.0, 1.0^, -1); + RPMVERCMP(1.0^git1, 1.0^git1, 0); + RPMVERCMP(1.0^git1, 1.0, 1); + RPMVERCMP(1.0, 1.0^git1, -1); + RPMVERCMP(1.0^git1, 1.0^git2, -1); + RPMVERCMP(1.0^git2, 1.0^git1, 1); + RPMVERCMP(1.0^git1, 1.01, -1); + RPMVERCMP(1.01, 1.0^git1, 1); + RPMVERCMP(1.0^20160101, 1.0^20160101, 0); + RPMVERCMP(1.0^20160101, 1.0.1, -1); + RPMVERCMP(1.0.1, 1.0^20160101, 1); + RPMVERCMP(1.0^20160101^git1, 1.0^20160101^git1, 0); + RPMVERCMP(1.0^20160102, 1.0^20160101^git1, 1); + RPMVERCMP(1.0^20160101^git1, 1.0^20160102, -1); + + /* Basic testcases for tilde and caret sorting */ + RPMVERCMP(1.0~rc1^git1, 1.0~rc1^git1, 0); + RPMVERCMP(1.0~rc1^git1, 1.0~rc1, 1); + RPMVERCMP(1.0~rc1, 1.0~rc1^git1, -1); + RPMVERCMP(1.0^git1~pre, 1.0^git1~pre, 0); + RPMVERCMP(1.0^git1, 1.0^git1~pre, 1); + RPMVERCMP(1.0^git1~pre, 1.0^git1, -1); + + /* These are included here to document current, arguably buggy behaviors + * for reference purposes and for easy checking against unintended + * behavior changes. */ + log_info("/* RPM version comparison oddities */"); + /* RhBug:811992 case */ + RPMVERCMP(1b.fc17, 1b.fc17, 0); + RPMVERCMP(1b.fc17, 1.fc17, 1); /* Note: this is reversed from rpm's vercmp, WAT! */ + RPMVERCMP(1.fc17, 1b.fc17, -1); + RPMVERCMP(1g.fc17, 1g.fc17, 0); + RPMVERCMP(1g.fc17, 1.fc17, 1); + RPMVERCMP(1.fc17, 1g.fc17, -1); + + /* Non-ascii characters are considered equal so these are all the same, eh… */ + RPMVERCMP(1.1.α, 1.1.α, 0); + RPMVERCMP(1.1.α, 1.1.β, 0); + RPMVERCMP(1.1.β, 1.1.α, 0); + RPMVERCMP(1.1.αα, 1.1.α, 0); + RPMVERCMP(1.1.α, 1.1.ββ, 0); + RPMVERCMP(1.1.ββ, 1.1.αα, 0); +} + TEST(strextendf) { _cleanup_free_ char *p = NULL; From e2d999345e3e95b1badbe4cca9989865d8ebcb38 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 18 May 2022 18:29:56 +0200 Subject: [PATCH 06/11] analyze: sort/fix header includes in one place --- src/analyze/analyze-critical-chain.c | 8 ++++---- src/analyze/analyze-time-data.h | 2 ++ 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/src/analyze/analyze-critical-chain.c b/src/analyze/analyze-critical-chain.c index 3a6b7770531..4084f15db5a 100644 --- a/src/analyze/analyze-critical-chain.c +++ b/src/analyze/analyze-critical-chain.c @@ -1,15 +1,15 @@ /* SPDX-License-Identifier: LGPL-2.1-or-later */ -#include "analyze.h" #include "analyze-critical-chain.h" #include "analyze-time-data.h" -#include "strv.h" +#include "analyze.h" +#include "bus-error.h" #include "copy.h" #include "path-util.h" -#include "terminal-util.h" #include "sort-util.h" #include "special.h" -#include "bus-error.h" +#include "strv.h" +#include "terminal-util.h" static int list_dependencies_print( const char *name, diff --git a/src/analyze/analyze-time-data.h b/src/analyze/analyze-time-data.h index 0082de213bb..02ee16a7141 100644 --- a/src/analyze/analyze-time-data.h +++ b/src/analyze/analyze-time-data.h @@ -1,6 +1,8 @@ /* SPDX-License-Identifier: LGPL-2.1-or-later */ #pragma once +#include + #include "time-util.h" typedef struct BootTimes { From d5dcd00ba23e13b29bf9b8ad296f164a962825b5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 18 May 2022 18:30:36 +0200 Subject: [PATCH 07/11] analyze: use automatic cleanup in one more place --- src/analyze/analyze-critical-chain.c | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/src/analyze/analyze-critical-chain.c b/src/analyze/analyze-critical-chain.c index 4084f15db5a..a3d993a3735 100644 --- a/src/analyze/analyze-critical-chain.c +++ b/src/analyze/analyze-critical-chain.c @@ -8,9 +8,13 @@ #include "path-util.h" #include "sort-util.h" #include "special.h" +#include "static-destruct.h" #include "strv.h" #include "terminal-util.h" +static Hashmap *unit_times_hashmap = NULL; +STATIC_DESTRUCTOR_REGISTER(unit_times_hashmap, hashmap_freep); + static int list_dependencies_print( const char *name, unsigned level, @@ -54,8 +58,6 @@ static int list_dependencies_get_dependencies(sd_bus *bus, const char *name, cha return bus_get_unit_property_strv(bus, path, "After", deps); } -static Hashmap *unit_times_hashmap; - static int list_dependencies_compare(char *const *a, char *const *b) { usec_t usa = 0, usb = 0; UnitTimes *times; @@ -197,7 +199,6 @@ static int list_dependencies(sd_bus *bus, const char *name) { int verb_critical_chain(int argc, char *argv[], void *userdata) { _cleanup_(sd_bus_flush_close_unrefp) sd_bus *bus = NULL; _cleanup_(unit_times_free_arrayp) UnitTimes *times = NULL; - Hashmap *h; int n, r; r = acquire_bus(&bus, NULL); @@ -208,16 +209,11 @@ int verb_critical_chain(int argc, char *argv[], void *userdata) { if (n <= 0) return n; - h = hashmap_new(&string_hash_ops); - if (!h) - return log_oom(); - for (UnitTimes *u = times; u->has_data; u++) { - r = hashmap_put(h, u->name, u); + r = hashmap_ensure_put(&unit_times_hashmap, &string_hash_ops, u->name, u); if (r < 0) return log_error_errno(r, "Failed to add entry to hashmap: %m"); } - unit_times_hashmap = h; pager_open(arg_pager_flags); @@ -230,6 +226,5 @@ int verb_critical_chain(int argc, char *argv[], void *userdata) { else list_dependencies(bus, SPECIAL_DEFAULT_TARGET); - h = hashmap_free(h); return 0; } From fddad5f4a66a68682892e3fa7f22ec2689786d33 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 17 May 2022 16:25:06 +0200 Subject: [PATCH 08/11] analyze: allow verbs to return positive failure No functional change is intended. The verbs where it wasn't immediately clear if the success exit status is 0 or >= 0 are changed to explicitly return 0. (I think it's better to be explicit than to rely on some call stack always returning 0 on success.) Some other functions are cleaned up to be more idiomatic. --- src/analyze/analyze-blame.c | 6 +++++- src/analyze/analyze-calendar.c | 14 ++++++++------ src/analyze/analyze-capability.c | 6 +++++- src/analyze/analyze-cat-config.c | 2 +- src/analyze/analyze-condition.c | 8 +++++++- src/analyze/analyze-critical-chain.c | 2 +- src/analyze/analyze-dump.c | 6 +++++- src/analyze/analyze-exit-status.c | 6 +++++- src/analyze/analyze-filesystems.c | 2 +- src/analyze/analyze-log-control.c | 6 +++++- src/analyze/analyze-plot.c | 2 +- src/analyze/analyze-service-watchdogs.c | 2 +- src/analyze/analyze-syscall-filter.c | 2 +- src/analyze/analyze-time.c | 2 +- src/analyze/analyze-timestamp.c | 14 ++++++++------ src/analyze/analyze-unit-files.c | 2 +- src/analyze/analyze-unit-paths.c | 2 +- src/analyze/analyze.c | 2 +- src/shared/verb-log-control.c | 25 ++++++++++--------------- 19 files changed, 68 insertions(+), 43 deletions(-) diff --git a/src/analyze/analyze-blame.c b/src/analyze/analyze-blame.c index 6cd6846956d..c9112685f83 100644 --- a/src/analyze/analyze-blame.c +++ b/src/analyze/analyze-blame.c @@ -61,5 +61,9 @@ int verb_blame(int argc, char *argv[], void *userdata) { pager_open(arg_pager_flags); - return table_print(table, NULL); + r = table_print(table, NULL); + if (r < 0) + return r; + + return EXIT_SUCCESS; } diff --git a/src/analyze/analyze-calendar.c b/src/analyze/analyze-calendar.c index 8f6517fa8e5..952fe95c2cf 100644 --- a/src/analyze/analyze-calendar.c +++ b/src/analyze/analyze-calendar.c @@ -123,7 +123,7 @@ static int test_calendar_one(usec_t n, const char *p) { } int verb_calendar(int argc, char *argv[], void *userdata) { - int ret = 0, r; + int r = 0; usec_t n; if (arg_base_time != USEC_INFINITY) @@ -132,13 +132,15 @@ int verb_calendar(int argc, char *argv[], void *userdata) { n = now(CLOCK_REALTIME); /* We want to use the same "base" for all expressions */ STRV_FOREACH(p, strv_skip(argv, 1)) { - r = test_calendar_one(n, *p); - if (ret == 0 && r < 0) - ret = r; + int k; - if (*(p + 1)) + k = test_calendar_one(n, *p); + if (r == 0 && k < 0) + r = k; + + if (p[1]) putchar('\n'); } - return ret; + return r; } diff --git a/src/analyze/analyze-capability.c b/src/analyze/analyze-capability.c index ebb205e7f2c..8072175a847 100644 --- a/src/analyze/analyze-capability.c +++ b/src/analyze/analyze-capability.c @@ -48,5 +48,9 @@ int verb_capabilities(int argc, char *argv[], void *userdata) { pager_open(arg_pager_flags); - return table_print(table, NULL); + r = table_print(table, NULL); + if (r < 0) + return r; + + return EXIT_SUCCESS; } diff --git a/src/analyze/analyze-cat-config.c b/src/analyze/analyze-cat-config.c index 85ed8b01e0d..d214ceaf424 100644 --- a/src/analyze/analyze-cat-config.c +++ b/src/analyze/analyze-cat-config.c @@ -42,5 +42,5 @@ int verb_cat_config(int argc, char *argv[], void *userdata) { return r; } - return 0; + return EXIT_SUCCESS; } diff --git a/src/analyze/analyze-condition.c b/src/analyze/analyze-condition.c index 011066f148b..d1643b61802 100644 --- a/src/analyze/analyze-condition.c +++ b/src/analyze/analyze-condition.c @@ -137,5 +137,11 @@ static int verify_conditions(char **lines, LookupScope scope, const char *unit, } int verb_condition(int argc, char *argv[], void *userdata) { - return verify_conditions(strv_skip(argv, 1), arg_scope, arg_unit, arg_root); + int r; + + r = verify_conditions(strv_skip(argv, 1), arg_scope, arg_unit, arg_root); + if (r < 0) + return r; + + return EXIT_SUCCESS; } diff --git a/src/analyze/analyze-critical-chain.c b/src/analyze/analyze-critical-chain.c index a3d993a3735..bb077e2293f 100644 --- a/src/analyze/analyze-critical-chain.c +++ b/src/analyze/analyze-critical-chain.c @@ -226,5 +226,5 @@ int verb_critical_chain(int argc, char *argv[], void *userdata) { else list_dependencies(bus, SPECIAL_DEFAULT_TARGET); - return 0; + return EXIT_SUCCESS; } diff --git a/src/analyze/analyze-dump.c b/src/analyze/analyze-dump.c index 24094ce6149..448ce09bd78 100644 --- a/src/analyze/analyze-dump.c +++ b/src/analyze/analyze-dump.c @@ -60,5 +60,9 @@ int verb_dump(int argc, char *argv[], void *userdata) { return bus_log_parse_error(r); fflush(stdout); - return copy_bytes(fd, STDOUT_FILENO, UINT64_MAX, 0); + r = copy_bytes(fd, STDOUT_FILENO, UINT64_MAX, 0); + if (r < 0) + return r; + + return EXIT_SUCCESS; } diff --git a/src/analyze/analyze-exit-status.c b/src/analyze/analyze-exit-status.c index a3f21c60eaf..3a8d3f4b2a9 100644 --- a/src/analyze/analyze-exit-status.c +++ b/src/analyze/analyze-exit-status.c @@ -48,5 +48,9 @@ int verb_exit_status(int argc, char *argv[], void *userdata) { pager_open(arg_pager_flags); - return table_print(table, NULL); + r = table_print(table, NULL); + if (r < 0) + return r; + + return EXIT_SUCCESS; } diff --git a/src/analyze/analyze-filesystems.c b/src/analyze/analyze-filesystems.c index 66d8397e312..e30b3a6ac6d 100644 --- a/src/analyze/analyze-filesystems.c +++ b/src/analyze/analyze-filesystems.c @@ -221,5 +221,5 @@ int verb_filesystems(int argc, char *argv[], void *userdata) { first = false; } - return 0; + return EXIT_SUCCESS; } diff --git a/src/analyze/analyze-log-control.c b/src/analyze/analyze-log-control.c index d31081b6a0b..cead0e833f5 100644 --- a/src/analyze/analyze-log-control.c +++ b/src/analyze/analyze-log-control.c @@ -14,5 +14,9 @@ int verb_log_control(int argc, char *argv[], void *userdata) { if (r < 0) return bus_log_connect_error(r, arg_transport); - return verb_log_control_common(bus, "org.freedesktop.systemd1", argv[0], argc == 2 ? argv[1] : NULL); + r = verb_log_control_common(bus, "org.freedesktop.systemd1", argv[0], argc == 2 ? argv[1] : NULL); + if (r < 0) + return r; + + return EXIT_SUCCESS; } diff --git a/src/analyze/analyze-plot.c b/src/analyze/analyze-plot.c index edfdfba1449..100bdc3787c 100644 --- a/src/analyze/analyze-plot.c +++ b/src/analyze/analyze-plot.c @@ -391,5 +391,5 @@ int verb_plot(int argc, char *argv[], void *userdata) { svg("\n"); - return 0; + return EXIT_SUCCESS; } diff --git a/src/analyze/analyze-service-watchdogs.c b/src/analyze/analyze-service-watchdogs.c index 96aed32b55f..6535eb1a894 100644 --- a/src/analyze/analyze-service-watchdogs.c +++ b/src/analyze/analyze-service-watchdogs.c @@ -37,5 +37,5 @@ int verb_service_watchdogs(int argc, char *argv[], void *userdata) { return log_error_errno(r, "Failed to set service-watchdog state: %s", bus_error_message(&error, r)); } - return 0; + return EXIT_SUCCESS; } diff --git a/src/analyze/analyze-syscall-filter.c b/src/analyze/analyze-syscall-filter.c index 582a043088f..308b1724e5d 100644 --- a/src/analyze/analyze-syscall-filter.c +++ b/src/analyze/analyze-syscall-filter.c @@ -176,7 +176,7 @@ int verb_syscall_filters(int argc, char *argv[], void *userdata) { first = false; } - return 0; + return EXIT_SUCCESS; } #else diff --git a/src/analyze/analyze-time.c b/src/analyze/analyze-time.c index e5744a218ef..c233b1f0855 100644 --- a/src/analyze/analyze-time.c +++ b/src/analyze/analyze-time.c @@ -18,5 +18,5 @@ int verb_time(int argc, char *argv[], void *userdata) { return r; puts(buf); - return 0; + return EXIT_SUCCESS; } diff --git a/src/analyze/analyze-timestamp.c b/src/analyze/analyze-timestamp.c index ddf34ab75fa..79764e014e5 100644 --- a/src/analyze/analyze-timestamp.c +++ b/src/analyze/analyze-timestamp.c @@ -79,16 +79,18 @@ static int test_timestamp_one(const char *p) { } int verb_timestamp(int argc, char *argv[], void *userdata) { - int ret = 0, r; + int r = 0; STRV_FOREACH(p, strv_skip(argv, 1)) { - r = test_timestamp_one(*p); - if (ret == 0 && r < 0) - ret = r; + int k; - if (*(p + 1)) + k = test_timestamp_one(*p); + if (r == 0 && k < 0) + r = k; + + if (p[1]) putchar('\n'); } - return ret; + return r; } diff --git a/src/analyze/analyze-unit-files.c b/src/analyze/analyze-unit-files.c index f07ec3e56cd..ec9be336e1a 100644 --- a/src/analyze/analyze-unit-files.c +++ b/src/analyze/analyze-unit-files.c @@ -46,5 +46,5 @@ int verb_unit_files(int argc, char *argv[], void *userdata) { printf("aliases: %s ← %s\n", k, j); } - return 0; + return EXIT_SUCCESS; } diff --git a/src/analyze/analyze-unit-paths.c b/src/analyze/analyze-unit-paths.c index 6fa1527dd6f..c2254619bca 100644 --- a/src/analyze/analyze-unit-paths.c +++ b/src/analyze/analyze-unit-paths.c @@ -16,5 +16,5 @@ int verb_unit_paths(int argc, char *argv[], void *userdata) { STRV_FOREACH(p, paths.search_path) puts(*p); - return 0; + return EXIT_SUCCESS; } diff --git a/src/analyze/analyze.c b/src/analyze/analyze.c index cb83e738770..2935ecea7af 100644 --- a/src/analyze/analyze.c +++ b/src/analyze/analyze.c @@ -607,4 +607,4 @@ static int run(int argc, char *argv[]) { return dispatch_verb(argc, argv, verbs, NULL); } -DEFINE_MAIN_FUNCTION(run); +DEFINE_MAIN_FUNCTION_WITH_POSITIVE_FAILURE(run); diff --git a/src/shared/verb-log-control.c b/src/shared/verb-log-control.c index eea0e8151ab..555fb9ff7cd 100644 --- a/src/shared/verb-log-control.c +++ b/src/shared/verb-log-control.c @@ -30,27 +30,22 @@ int verb_log_control_common(sd_bus *bus, const char *destination, const char *ve r = bus_set_property(bus, &bloc, level ? "LogLevel" : "LogTarget", &error, "s", value); - if (r >= 0) - return 0; - - log_error_errno(r, "Failed to set log %s of %s to %s: %s", - level ? "level" : "target", - bloc.destination, value, bus_error_message(&error, r)); + if (r < 0) + return log_error_errno(r, "Failed to set log %s of %s to %s: %s", + level ? "level" : "target", + bloc.destination, value, bus_error_message(&error, r)); } else { _cleanup_free_ char *t = NULL; r = bus_get_property_string(bus, &bloc, level ? "LogLevel" : "LogTarget", &error, &t); - if (r >= 0) { - puts(t); - return 0; - } - - log_error_errno(r, "Failed to get log %s of %s: %s", - level ? "level" : "target", - bloc.destination, bus_error_message(&error, r)); + if (r < 0) + return log_error_errno(r, "Failed to get log %s of %s: %s", + level ? "level" : "target", + bloc.destination, bus_error_message(&error, r)); + puts(t); } - return r; + return 0; } From bc012a3e913075d14ac39d33a090c5dcddce2e09 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 17 May 2022 16:28:45 +0200 Subject: [PATCH 09/11] analyze: add compare-versions The interface, output, and exit status convention are all taken directly from rpmdev-vercmp and dpkg --compare-versions. The implementation is different though. See test-string-util for a list of known cases where we compare strings incompatibly. The idea is that this string comparison function will be declared as "the" method to use for boot entry ordering in the specification and similar uses. Thus it's nice to allow users to compare strings. --- man/systemd-analyze.xml | 62 +++++++++++++++++++++++++- src/analyze/analyze-compare-versions.c | 47 +++++++++++++++++++ src/analyze/analyze-compare-versions.h | 3 ++ src/analyze/analyze.c | 4 ++ src/analyze/meson.build | 2 + 5 files changed, 116 insertions(+), 2 deletions(-) create mode 100644 src/analyze/analyze-compare-versions.c create mode 100644 src/analyze/analyze-compare-versions.h diff --git a/man/systemd-analyze.xml b/man/systemd-analyze.xml index 97290d479b4..8061743c567 100644 --- a/man/systemd-analyze.xml +++ b/man/systemd-analyze.xml @@ -118,6 +118,14 @@ cat-config NAME|PATH + + systemd-analyze + OPTIONS + compare-versions + VERSION1 + OP + VERSION2 + systemd-analyze OPTIONS @@ -547,6 +555,52 @@ NAutoVTs=8 + + <command>systemd-analyze compare-versions + <replaceable>VERSION1</replaceable> + <optional><replaceable>OP</replaceable></optional> + <replaceable>VERSION2</replaceable></command> + + This command has two distinct modes of operation, depending on whether the operator + OP is specified. + + In the first mode — when OP is not specified — it will compare the two + version strings and print either VERSION1 < + VERSION2, or VERSION1 == + VERSION2, or VERSION1 > + VERSION2 as appropriate. + + The exit status is 0 if the versions are equal, 11 if + the version of the right is smaller, and 12 if the version of the left is + smaller. (This matches the convention used by rpmdev-vercmp.) + + In the second mode — when OP is specified — it will compare the two + version strings using the operation OP and return 0 + (success) if they condition is satisfied, and 1 (failure) + otherwise. OP may be lt, le, + eq, ne, ge, gt. In this + mode, no output is printed. + (This matches the convention used by + dpkg1 + .) + + + Compare versions of a package + + +$ systemd-analyze compare-versions systemd-250~rc1.fc36.aarch64 systemd-251.fc36.aarch64 +systemd-250~rc1.fc36.aarch64 < systemd-251.fc36.aarch64 +$ echo $? +12 + +$ systemd-analyze compare-versions 1 lt 2; echo $? +0 +$ systemd-analyze compare-versions 1 ge 2; echo $? +1 + + + + <command>systemd-analyze verify <replaceable>FILE</replaceable>...</command> @@ -1197,8 +1251,12 @@ $ systemd-analyze verify /tmp/source:alias.service Exit status - On success, 0 is returned, a non-zero failure code - otherwise. + For most commands, 0 is returned on success, and a non-zero failure code otherwise. + + With the verb compare-versions, in the two-argument form, + 12, 0, 11 is returned if the second + version string is respectively larger, equal, or smaller to the first. In the three-argument form, + 0 or 1 if the condition is respectively true or false. diff --git a/src/analyze/analyze-compare-versions.c b/src/analyze/analyze-compare-versions.c new file mode 100644 index 00000000000..9545326fa9c --- /dev/null +++ b/src/analyze/analyze-compare-versions.c @@ -0,0 +1,47 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ + +#include + +#include "analyze-compare-versions.h" +#include "macro.h" +#include "string-util.h" +#include "strv.h" + +int verb_compare_versions(int argc, char *argv[], void *userdata) { + int r; + + assert(argc == 3 || argc == 4); + assert(argv); + + if (argc == 3) { + r = strverscmp_improved(ASSERT_PTR(argv[1]), ASSERT_PTR(argv[2])); + printf("%s %s %s\n", + argv[1], + r < 0 ? "<" : r > 0 ? ">" : "==", + argv[2]); + + /* This matches the exit convention used by rpmdev-vercmp. + * We don't use named values because 11 and 12 don't have names. */ + return r < 0 ? 12 : r > 0 ? 11 : 0; + + } else { + const char *op = ASSERT_PTR(argv[2]); + + r = strverscmp_improved(ASSERT_PTR(argv[1]), ASSERT_PTR(argv[3])); + + 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); + } +} diff --git a/src/analyze/analyze-compare-versions.h b/src/analyze/analyze-compare-versions.h new file mode 100644 index 00000000000..ac90ede2f05 --- /dev/null +++ b/src/analyze/analyze-compare-versions.h @@ -0,0 +1,3 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ + +int verb_compare_versions(int argc, char *argv[], void *userdata); diff --git a/src/analyze/analyze.c b/src/analyze/analyze.c index 2935ecea7af..4968946963a 100644 --- a/src/analyze/analyze.c +++ b/src/analyze/analyze.c @@ -35,6 +35,7 @@ #include "analyze-timestamp.h" #include "analyze-unit-files.h" #include "analyze-unit-paths.h" +#include "analyze-compare-versions.h" #include "analyze-verify.h" #include "bus-error.h" #include "bus-locator.h" @@ -199,6 +200,8 @@ static int help(int argc, char *argv[], void *userdata) { " syscall-filter [NAME...] List syscalls in seccomp filters\n" " filesystems [NAME...] List known filesystems\n" " condition CONDITION... Evaluate conditions and asserts\n" + " compare-versions VERSION1 [OP] VERSION2\n" + " Compare two version strings\n" " verify FILE... Check unit files for correctness\n" " calendar SPEC... Validate repetitive calendar time\n" " events\n" @@ -564,6 +567,7 @@ static int run(int argc, char *argv[]) { { "capability", VERB_ANY, VERB_ANY, 0, verb_capabilities }, { "filesystems", VERB_ANY, VERB_ANY, 0, verb_filesystems }, { "condition", VERB_ANY, VERB_ANY, 0, verb_condition }, + { "compare-versions", 3, 4, 0, verb_compare_versions }, { "verify", 2, VERB_ANY, 0, verb_verify }, { "calendar", 2, VERB_ANY, 0, verb_calendar }, { "timestamp", 2, VERB_ANY, 0, verb_timestamp }, diff --git a/src/analyze/meson.build b/src/analyze/meson.build index f0cfbb195ea..24ef94149c9 100644 --- a/src/analyze/meson.build +++ b/src/analyze/meson.build @@ -9,6 +9,8 @@ systemd_analyze_sources = files( 'analyze-capability.h', 'analyze-cat-config.c', 'analyze-cat-config.h', + 'analyze-compare-versions.c', + 'analyze-compare-versions.h', 'analyze-condition.c', 'analyze-condition.h', 'analyze-critical-chain.c', From 4e8295f42dc7b7e001f620db64abfcd407b4e36c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 18 May 2022 10:40:54 +0200 Subject: [PATCH 10/11] test-compare-versions: basic test for systemd-analyze compare-versions --- meson.build | 9 ++++++++- test/meson.build | 8 ++++++++ test/test-compare-versions.sh | 34 ++++++++++++++++++++++++++++++++++ 3 files changed, 50 insertions(+), 1 deletion(-) create mode 100755 test/test-compare-versions.sh diff --git a/meson.build b/meson.build index 36cbfa48938..42e746558b4 100644 --- a/meson.build +++ b/meson.build @@ -2162,7 +2162,7 @@ meson.add_install_script(meson_make_symlink, rootlibexecdir / 'systemd', rootsbindir / 'init') -public_programs += executable( +exe = executable( 'systemd-analyze', systemd_analyze_sources, include_directories : core_includes, @@ -2172,6 +2172,13 @@ public_programs += executable( libseccomp], install_rpath : rootlibexecdir, install : conf.get('ENABLE_ANALYZE')) +public_programs += exe + +if want_tests != 'false' + test('test-compare-versions', + test_compare_versions_sh, + args : exe.full_path()) +endif executable( 'systemd-journald', diff --git a/test/meson.build b/test/meson.build index d4e1e3088d1..65cb52cd213 100644 --- a/test/meson.build +++ b/test/meson.build @@ -108,6 +108,14 @@ endif ############################################################ +test_compare_versions_sh = files('test-compare-versions.sh') +if install_tests + install_data(test_compare_versions_sh, + install_dir : testsdir) +endif + +############################################################ + rule_syntax_check_py = find_program('rule-syntax-check.py') if want_tests != 'false' test('rule-syntax-check', diff --git a/test/test-compare-versions.sh b/test/test-compare-versions.sh new file mode 100755 index 00000000000..6cfcc158e6f --- /dev/null +++ b/test/test-compare-versions.sh @@ -0,0 +1,34 @@ +#!/usr/bin/env bash +# SPDX-License-Identifier: LGPL-2.1-or-later +set -e + +ANALYZE="${1:-systemd-analyze}" + +$ANALYZE compare-versions 1 lt 2 +$ANALYZE compare-versions 1 '<' 2 +$ANALYZE compare-versions 1 le 2 +$ANALYZE compare-versions 1 '<=' 2 +$ANALYZE compare-versions 1 ne 2 +$ANALYZE compare-versions 1 '!=' 2 +( ! $ANALYZE compare-versions 1 ge 2 ) +( ! $ANALYZE compare-versions 1 '>=' 2 ) +( ! $ANALYZE compare-versions 1 eq 2 ) +( ! $ANALYZE compare-versions 1 '==' 2 ) +( ! $ANALYZE compare-versions 1 gt 2 ) +( ! $ANALYZE compare-versions 1 '>' 2 ) + +$ANALYZE compare-versions 1 2 | grep ' < ' +$ANALYZE compare-versions 2 2 | grep ' == ' +$ANALYZE compare-versions 2 1 | grep ' > ' + +set +e + +$ANALYZE compare-versions 1 2; ret1=$? +$ANALYZE compare-versions 2 2; ret2=$? +$ANALYZE compare-versions 2 1; ret3=$? + +set -e + +test $ret1 == 12 +test $ret2 == 0 +test $ret3 == 11 From 8d9156660d6958c8d63b1d44692968f1b5d33920 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 19 May 2022 09:05:48 +0200 Subject: [PATCH 11/11] version comparisons: stop using locale-dependent isdigit() The docs are not entirely clear what glyphs qualify as digits. The function is supposed to be locale-dependent, but I couldn't get it to return true on any non-ascii digits I tried. But it's better to be safe than sorry, let's use our trivial replacement instead. --- src/fundamental/string-util-fundamental.c | 15 +++++++-------- src/test/test-string-util.c | 9 +++++++++ 2 files changed, 16 insertions(+), 8 deletions(-) diff --git a/src/fundamental/string-util-fundamental.c b/src/fundamental/string-util-fundamental.c index 57b4d535f3a..feccb822ff7 100644 --- a/src/fundamental/string-util-fundamental.c +++ b/src/fundamental/string-util-fundamental.c @@ -77,19 +77,18 @@ sd_char* endswith_no_case(const sd_char *s, const sd_char *postfix) { return (sd_char*) s + sl - pl; } -#ifdef SD_BOOT -static sd_bool isdigit(sd_char a) { +static sd_bool is_digit(sd_char a) { + /* Locale-independent version of isdigit(). */ return a >= '0' && a <= '9'; } -#endif static sd_bool is_alpha(sd_char a) { - /* Locale independent version of isalpha(). */ + /* Locale-independent version of isalpha(). */ return (a >= 'a' && a <= 'z') || (a >= 'A' && a <= 'Z'); } static sd_bool is_valid_version_char(sd_char a) { - return isdigit(a) || is_alpha(a) || IN_SET(a, '~', '-', '^', '.'); + return is_digit(a) || is_alpha(a) || IN_SET(a, '~', '-', '^', '.'); } sd_int strverscmp_improved(const sd_char *a, const sd_char *b) { @@ -187,7 +186,7 @@ sd_int strverscmp_improved(const sd_char *a, const sd_char *b) { b++; } - if (isdigit(*a) || isdigit(*b)) { + if (is_digit(*a) || is_digit(*b)) { /* Skip leading '0', to make 00123 equivalent to 123. */ while (*a == '0') a++; @@ -196,9 +195,9 @@ sd_int strverscmp_improved(const sd_char *a, const sd_char *b) { /* Find the leading numeric segments. One may be an empty string. So, * numeric segments are always newer than alpha segments. */ - for (aa = a; isdigit(*aa); aa++) + for (aa = a; is_digit(*aa); aa++) ; - for (bb = b; isdigit(*bb); bb++) + for (bb = b; is_digit(*bb); bb++) ; /* To compare numeric segments without parsing their values, first compare the diff --git a/src/test/test-string-util.c b/src/test/test-string-util.c index d0a4eca3906..1054f9ea31d 100644 --- a/src/test/test-string-util.c +++ b/src/test/test-string-util.c @@ -1,5 +1,7 @@ /* SPDX-License-Identifier: LGPL-2.1-or-later */ +#include + #include "alloc-util.h" #include "locale-util.h" #include "macro.h" @@ -914,6 +916,13 @@ TEST(strverscmp_improved) { /* invalid characters */ assert_se(strverscmp_improved("123_aa2-67.89", "123aa+2-67.89") == 0); + + /* non-ASCII digits */ + (void) setlocale(LC_NUMERIC, "ar_YE.utf8"); + assert_se(strverscmp_improved("1٠١٢٣٤٥٦٧٨٩", "1") == 0); + + (void) setlocale(LC_NUMERIC, "th_TH.utf8"); + assert_se(strverscmp_improved("1๐๑๒๓๔๕๖๗๘๙", "1") == 0); } #define RPMVERCMP(a, b, c) \