From 4d3510d00fb978c101912b6873746bf3f43ca5e1 Mon Sep 17 00:00:00 2001 From: Frantisek Sumsal Date: Tue, 26 Dec 2023 00:41:40 +0100 Subject: [PATCH 01/10] coccinelle: explicitly undefine SD_BOOT So Coccinelle doesn't pull in includes guarded by #if SD_BOOT. For example: $ head -n5 main.c #if FOO #include "foo.h" #else #include "bar.h" #endif $ spatch --verbose-includes --recursive-includes --sp-file zz-drop-braces.cocci main.c init_defs_builtins: /usr/lib64/coccinelle/standard.h HANDLING: main.c including ./foo.h including ./bar.h $ spatch --verbose-includes --recursive-includes --sp-file zz-drop-braces.cocci main.c --undefined FOO init_defs_builtins: /usr/lib64/coccinelle/standard.h HANDLING: main.c including ./bar.h --- coccinelle/run-coccinelle.sh | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/coccinelle/run-coccinelle.sh b/coccinelle/run-coccinelle.sh index bb72a493f08..360f9268e5a 100755 --- a/coccinelle/run-coccinelle.sh +++ b/coccinelle/run-coccinelle.sh @@ -7,7 +7,6 @@ set -e # that TEST(xsetxattr) yields test_xsetxattr() and uses just xsetxattr() in this case, which then conflicts # with the tested xsetxattr() function, leading up to the whole test case getting skipped due to # conflicting typedefs -# - something keeps pulling in src/boot/efi/*.h stuff, even though it's excluded # - Coccinelle has issues with some of our more complex macros # Exclude following paths from the Coccinelle transformations @@ -73,7 +72,9 @@ for script in "${SCRIPTS[@]}"; do # definitions (--include-headers-for-types) - otherwise we'd start formating them as well, which might be # unwanted, especially for includes we fetch verbatim from third-parties # - # 4) Use cache, since generating the full AST is _very_ expensive, i.e. the uncached run takes 15 - 30 + # 4) Explicitly undefine the SD_BOOT symbol, so Coccinelle ignores includes guarded by #if SD_BOOT + # + # 5) Use cache, since generating the full AST is _very_ expensive, i.e. the uncached run takes 15 - 30 # minutes (for one rule(!)), vs 30 - 90 seconds when the cache is populated. One major downside of the # cache is that it's quite big - ATTOW the cache takes around 15 GiB, but the performance boost is # definitely worth it @@ -82,6 +83,7 @@ for script in "${SCRIPTS[@]}"; do -I src \ --recursive-includes \ --include-headers-for-types \ + --undefined SD_BOOT \ --smpl-spacing \ --sp-file "$script" \ "${ARGS[@]}" ::: "${FILES[@]}" \ From 11959eb201bdd478a8dc247b78a10fc8ffd611b9 Mon Sep 17 00:00:00 2001 From: Frantisek Sumsal Date: Tue, 26 Dec 2023 18:44:21 +0100 Subject: [PATCH 02/10] coccinelle: search the system include path for header files as well Since Coccinelle is originally a kernel tool, it doesn't search the system include path by default for header files. Without this we're missing a lot of types provides by stdlib (and other libraries we make use of). --- coccinelle/run-coccinelle.sh | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/coccinelle/run-coccinelle.sh b/coccinelle/run-coccinelle.sh index 360f9268e5a..9edfaa83838 100755 --- a/coccinelle/run-coccinelle.sh +++ b/coccinelle/run-coccinelle.sh @@ -66,7 +66,9 @@ for script in "${SCRIPTS[@]}"; do # at once one spatch process can take around 2.5 GiB of RAM, which can easily eat up all available RAM # when paired together with parallel # - # 2) Make sure spatch can find our includes via -I , similarly as we do when compiling stuff + # 2) Make sure spatch can find our includes via -I , similarly as we do when compiling stuff. + # Also, include the system include path as well, since we're not kernel and we make use of the stdlib + # (and other libraries). # # 3) Make sure to include includes from includes (--recursive-includes), but use them only to get type # definitions (--include-headers-for-types) - otherwise we'd start formating them as well, which might be @@ -81,6 +83,7 @@ for script in "${SCRIPTS[@]}"; do parallel --halt now,fail=1 --keep-order --noswap --max-args=10 \ spatch --cache-prefix "$CACHE_DIR" \ -I src \ + -I /usr/include \ --recursive-includes \ --include-headers-for-types \ --undefined SD_BOOT \ From b25d3b36a21cd7a68039b85f887cc849ff7664a1 Mon Sep 17 00:00:00 2001 From: Frantisek Sumsal Date: Tue, 26 Dec 2023 18:55:31 +0100 Subject: [PATCH 03/10] coccinelle: help Coccinelle with some more complex macros Drop the original macro file, since it's not needed anymore thanks to resolving includes properly, but introduce a similar file - parsing_hacks.h - that helps Coccinelle in some specific corner cases. This eliminates most of the outstanding parsing errors in source files. The remaining ones are limitations of the parsing engine (see the FIXMEs in pasing_hacks.h) and need further investigation. --- coccinelle/macros.h | 231 ----------------------------------- coccinelle/parsing_hacks.h | 80 ++++++++++++ coccinelle/run-coccinelle.sh | 8 +- 3 files changed, 81 insertions(+), 238 deletions(-) delete mode 100644 coccinelle/macros.h create mode 100644 coccinelle/parsing_hacks.h diff --git a/coccinelle/macros.h b/coccinelle/macros.h deleted file mode 100644 index adfea5fbec4..00000000000 --- a/coccinelle/macros.h +++ /dev/null @@ -1,231 +0,0 @@ -/* SPDX-License-Identifier: LGPL-2.1-or-later */ -/* - * Collected macros from our systemd codebase to make the cocci semantic - * parser happy. Inspired by the original cocci macros file - * /usr/lib64/coccinelle/standard.h (including the YACFE_* symbols) - */ - -// General -#define PTR_TO_PID(x) - -// src/basic/macro.h -#define _printf_(a, b) __attribute__((__format__(printf, a, b))) -#define _alloc_(...) __attribute__((__alloc_size__(__VA_ARGS__))) -#define _sentinel_ __attribute__((__sentinel__)) -#define _section_(x) __attribute__((__section__(x))) -#define _used_ __attribute__((__used__)) -#define _unused_ __attribute__((__unused__)) -#define _destructor_ __attribute__((__destructor__)) -#define _pure_ __attribute__((__pure__)) -#define _const_ __attribute__((__const__)) -#define _deprecated_ __attribute__((__deprecated__)) -#define _packed_ __attribute__((__packed__)) -#define _malloc_ __attribute__((__malloc__)) -#define _weak_ __attribute__((__weak__)) -#define _likely_(x) (__builtin_expect(!!(x), 1)) -#define _unlikely_(x) (__builtin_expect(!!(x), 0)) -#define _public_ __attribute__((__visibility__("default"))) -#define _hidden_ __attribute__((__visibility__("hidden"))) -#define _weakref_(x) __attribute__((__weakref__(#x))) -#define _align_(x) __attribute__((__aligned__(x))) -#define _alignas_(x) __attribute__((__aligned__(__alignof(x)))) -#define _alignptr_ __attribute__((__aligned__(sizeof(void*)))) -#define _cleanup_(x) __attribute__((__cleanup__(x))) -#define _fallthrough_ -#define _noreturn_ __attribute__((__noreturn__)) -#define thread_local __thread - -#define ELEMENTSOF(x) \ - (__builtin_choose_expr( \ - !__builtin_types_compatible_p(typeof(x), typeof(&*(x))), \ - sizeof(x)/sizeof((x)[0]), \ - VOID_0)) - -// src/basic/umask-util.h -#define _cleanup_umask_ -#define WITH_UMASK(mask) \ - for (_cleanup_umask_ mode_t _saved_umask_ = umask(mask) | S_IFMT; \ - FLAGS_SET(_saved_umask_, S_IFMT); \ - _saved_umask_ &= 0777) - -// src/basic/hashmap.h -#define _IDX_ITERATOR_FIRST (UINT_MAX - 1) -#define HASHMAP_FOREACH(e, h) YACFE_ITERATOR -#define ORDERED_HASHMAP_FOREACH(e, h) YACFE_ITERATOR -#define HASHMAP_FOREACH_KEY(e, k, h) YACFE_ITERATOR -#define ORDERED_HASHMAP_FOREACH_KEY(e, k, h) YACFE_ITERATOR - -// src/basic/list.h -#define LIST_HEAD(t,name) \ - t *name -#define LIST_FIELDS(t,name) \ - t *name##_next, *name##_prev -#define LIST_HEAD_INIT(head) \ - do { \ - (head) = NULL; \ - } while (false) -#define LIST_INIT(name,item) \ - do { \ - typeof(*(item)) *_item = (item); \ - assert(_item); \ - _item->name##_prev = _item->name##_next = NULL; \ - } while (false) -#define LIST_PREPEND(name,head,item) \ - do { \ - typeof(*(head)) **_head = &(head), *_item = (item); \ - assert(_item); \ - if ((_item->name##_next = *_head)) \ - _item->name##_next->name##_prev = _item; \ - _item->name##_prev = NULL; \ - *_head = _item; \ - } while (false) -#define LIST_APPEND(name,head,item) \ - do { \ - typeof(*(head)) **_hhead = &(head), *_tail; \ - LIST_FIND_TAIL(name, *_hhead, _tail); \ - LIST_INSERT_AFTER(name, *_hhead, _tail, item); \ - } while (false) -#define LIST_REMOVE(name,head,item) \ - do { \ - typeof(*(head)) **_head = &(head), *_item = (item); \ - assert(_item); \ - if (_item->name##_next) \ - _item->name##_next->name##_prev = _item->name##_prev; \ - if (_item->name##_prev) \ - _item->name##_prev->name##_next = _item->name##_next; \ - else { \ - assert(*_head == _item); \ - *_head = _item->name##_next; \ - } \ - _item->name##_next = _item->name##_prev = NULL; \ - } while (false) -#define LIST_FIND_HEAD(name,item,head) \ - do { \ - typeof(*(item)) *_item = (item); \ - if (!_item) \ - (head) = NULL; \ - else { \ - while (_item->name##_prev) \ - _item = _item->name##_prev; \ - (head) = _item; \ - } \ - } while (false) -#define LIST_FIND_TAIL(name,item,tail) \ - do { \ - typeof(*(item)) *_item = (item); \ - if (!_item) \ - (tail) = NULL; \ - else { \ - while (_item->name##_next) \ - _item = _item->name##_next; \ - (tail) = _item; \ - } \ - } while (false) -#define LIST_INSERT_AFTER(name,head,a,b) \ - do { \ - typeof(*(head)) **_head = &(head), *_a = (a), *_b = (b); \ - assert(_b); \ - if (!_a) { \ - if ((_b->name##_next = *_head)) \ - _b->name##_next->name##_prev = _b; \ - _b->name##_prev = NULL; \ - *_head = _b; \ - } else { \ - if ((_b->name##_next = _a->name##_next)) \ - _b->name##_next->name##_prev = _b; \ - _b->name##_prev = _a; \ - _a->name##_next = _b; \ - } \ - } while (false) -#define LIST_INSERT_BEFORE(name,head,a,b) \ - do { \ - typeof(*(head)) **_head = &(head), *_a = (a), *_b = (b); \ - assert(_b); \ - if (!_a) { \ - if (!*_head) { \ - _b->name##_next = NULL; \ - _b->name##_prev = NULL; \ - *_head = _b; \ - } else { \ - typeof(*(head)) *_tail = (head); \ - while (_tail->name##_next) \ - _tail = _tail->name##_next; \ - _b->name##_next = NULL; \ - _b->name##_prev = _tail; \ - _tail->name##_next = _b; \ - } \ - } else { \ - if ((_b->name##_prev = _a->name##_prev)) \ - _b->name##_prev->name##_next = _b; \ - else \ - *_head = _b; \ - _b->name##_next = _a; \ - _a->name##_prev = _b; \ - } \ - } while (false) - -#define LIST_JUST_US(name,item) \ - (!(item)->name##_prev && !(item)->name##_next) -#define LIST_FOREACH(name,i,head) \ - for ((i) = (head); (i); (i) = (i)->name##_next) -#define LIST_FOREACH_SAFE(name,i,n,head) \ - for ((i) = (head); (i) && (((n) = (i)->name##_next), 1); (i) = (n)) -#define LIST_FOREACH_BEFORE(name,i,p) \ - for ((i) = (p)->name##_prev; (i); (i) = (i)->name##_prev) -#define LIST_FOREACH_AFTER(name,i,p) \ - for ((i) = (p)->name##_next; (i); (i) = (i)->name##_next) -#define LIST_FOREACH_OTHERS(name,i,p) \ - for (({ \ - (i) = (p); \ - while ((i) && (i)->name##_prev) \ - (i) = (i)->name##_prev; \ - if ((i) == (p)) \ - (i) = (p)->name##_next; \ - }); \ - (i); \ - (i) = (i)->name##_next == (p) ? (p)->name##_next : (i)->name##_next) -#define LIST_LOOP_BUT_ONE(name,i,head,p) \ - for ((i) = (p)->name##_next ? (p)->name##_next : (head); \ - (i) != (p); \ - (i) = (i)->name##_next ? (i)->name##_next : (head)) - -#define LIST_JOIN(name,a,b) \ - do { \ - assert(b); \ - if (!(a)) \ - (a) = (b); \ - else { \ - typeof(*(a)) *_head = (b), *_tail; \ - LIST_FIND_TAIL(name, (a), _tail); \ - _tail->name##_next = _head; \ - _head->name##_prev = _tail; \ - } \ - (b) = NULL; \ - } while (false) - -// src/basic/strv.h -#define STRV_FOREACH(s, l) YACFE_ITERATOR -#define STRV_FOREACH_BACKWARDS(s, l) YACFE_ITERATOR -#define STRV_FOREACH_PAIR(x, y, l) YACFE_ITERATOR - -// src/basic/socket-util.h -#define CMSG_BUFFER_TYPE(size) \ - union { \ - struct cmsghdr cmsghdr; \ - uint8_t buf[size]; \ - uint8_t align_check[(size) >= CMSG_SPACE(0) && \ - (size) == CMSG_ALIGN(size) ? 1 : -1]; \ - } - -// src/libsystemd/sd-device/device-util.h -#define FOREACH_DEVICE_PROPERTY(device, key, value) YACFE_ITERATOR -#define FOREACH_DEVICE_TAG(device, tag) YACFE_ITERATOR -#define FOREACH_DEVICE_CURRENT_TAG(device, tag) YACFE_ITERATOR -#define FOREACH_DEVICE_SYSATTR(device, attr) YACFE_ITERATOR -#define FOREACH_DEVICE_DEVLINK(device, devlink) YACFE_ITERATOR -#define FOREACH_DEVICE(enumerator, device) YACFE_ITERATOR -#define FOREACH_SUBSYSTEM(enumerator, device) YACFE_ITERATOR - -// src/basic/dirent-util.h -#define FOREACH_DIRENT(de, d, on_error) YACFE_ITERATOR -#define FOREACH_DIRENT_ALL(de, d, on_error) YACFE_ITERATOR diff --git a/coccinelle/parsing_hacks.h b/coccinelle/parsing_hacks.h new file mode 100644 index 00000000000..e16aa9fdaed --- /dev/null +++ b/coccinelle/parsing_hacks.h @@ -0,0 +1,80 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ + +/* FIXME + * - issues with parsing stuff like + * * int foo[ELEMENTSOF(bar)] = {}; + * * validchars = UPPERCASE_LETTERS DIGITS; + * * multiline compound literals (some instances) + * * compound literals in function calls (some instances) + * * keywords in macro invocations like FOREACH_DIRENT_ALL(de, d, return -errno) + * (also, see FIXME in the TEST stuff below) + */ + +/* This file contains parsing hacks for Coccinelle (spatch), to make it happy with some of our more complex + * macros - it is intended to be used with the --macro-file-builtins option for spatch. + * + * Coccinelle's macro support is somewhat limited and the parser trips over some of our more complex macros. + * In most cases this doesn't really matter, as the parsing errors are silently ignored, but there are + * special cases in which the parser incorrectly infers information that then causes issues in valid code + * later down the line. + * + * Inspired by a similarly named file [0] from the Coccinelle sources, and the original bultin macros [1]. + * + * [0] https://github.com/coccinelle/coccinelle/blob/master/parsing_c/parsing_hacks.ml + * [1] https://github.com/coccinelle/coccinelle/blob/master/standard.h + * + */ + +/* Coccinelle really doesn't like our way of registering unit test cases, and incorrectly assumes that "id" + * from TEST(id) is the actual function identifier. This then causes name conflicts, since the unit tests + * are usually named after the functions they test. + * + * For example, a unit test for xsetxattr() is defined using TEST(xsetxattr), which eventually yields a + * procedure with following declaration: + * + * static const void test_xsetxattr(void); + * + * However, Coccinelle fails to parse the chain of macros behind TEST(x) and assumes the test function is + * named "xsetxattr", which then causes a name conflict when the actual "xsetxattr" function is called: + * + * (ONCE) SEMANTIC:parameter name omitted, but I continue + * Warning: PARSING: src/test/test-xattr-util.c:57: type defaults to 'int'; ... + * ERROR-RECOV: found sync '}' at line 127 + * Parsing pass2: try again + * ERROR-RECOV: found sync '}' at line 127 + * Parsing pass3: try again + * ERROR-RECOV: found sync '}' at line 127 + * Parse error + * = File "src/test/test-xattr-util.c", line 101, column 12, charpos = 3152 + * around = 'xsetxattr', + * whole content = r = xsetxattr(AT_FDCWD, x, "user.foo", "fullpath", SIZE_MAX, 0); + * Badcount: 40 + * + * The easy way out here is to just provide a simplified version of the TEST(x) macro that pinpoints the most + * important detail - that the actual function name is prefixed with test_. + * + * FIXME: even with this Coccinelle still fails to process TEST(x) instances where x is a keyword, e.g. + * TEST(float), TEST(default), ... + */ +#define TEST(x, ...) static void test_##x(void) +#define TEST_RET(x, ...) static int test_##x(void) + +/* Coccinelle doesn't know this keyword, so just drop it, since it's not important for any of our rules. */ +#define thread_local + +/* Coccinelle fails to get this one from the included headers, so let's just drop it. */ +#define PAM_EXTERN + +/* Mark a couple of iterator explicitly as iterators, otherwise Coccinelle gets a bit confused. Coccinelle + * can usually infer this information automagically, but in these specific cases it needs a bit of help. */ +#define FOREACH_ARRAY(i, array, num) YACFE_ITERATOR +#define FOREACH_DIRENT_ALL(de, d, on_error) YACFE_ITERATOR +#define FOREACH_STRING(x, y, ...) YACFE_ITERATOR +#define HASHMAP_FOREACH(e, h) YACFE_ITERATOR +#define LIST_FOREACH(name, i, head) YACFE_ITERATOR +#define ORDERED_HASHMAP_FOREACH(e, h) YACFE_ITERATOR +#define SET_FOREACH(e, s) YACFE_ITERATOR + +/* Coccinelle really doesn't like multiline macros that are not in the "usual" do { ... } while(0) format, so + * let's help it a little here by providing simplified one-line versions. */ +#define CMSG_BUFFER_TYPE(x) union { uint8_t align_check[(size) >= CMSG_SPACE(0) && (size) == CMSG_ALIGN(size) ? 1 : -1]; } diff --git a/coccinelle/run-coccinelle.sh b/coccinelle/run-coccinelle.sh index 9edfaa83838..cc2bfc14c5d 100755 --- a/coccinelle/run-coccinelle.sh +++ b/coccinelle/run-coccinelle.sh @@ -2,13 +2,6 @@ # SPDX-License-Identifier: LGPL-2.1-or-later set -e -# FIXME: -# - Coccinelle doesn't like our TEST() macros, which then causes name conflicts; i.e. Cocci can't process -# that TEST(xsetxattr) yields test_xsetxattr() and uses just xsetxattr() in this case, which then conflicts -# with the tested xsetxattr() function, leading up to the whole test case getting skipped due to -# conflicting typedefs -# - Coccinelle has issues with some of our more complex macros - # Exclude following paths from the Coccinelle transformations EXCLUDED_PATHS=( "src/boot/efi/*" @@ -87,6 +80,7 @@ for script in "${SCRIPTS[@]}"; do --recursive-includes \ --include-headers-for-types \ --undefined SD_BOOT \ + --macro-file-builtins "coccinelle/parsing_hacks.h" \ --smpl-spacing \ --sp-file "$script" \ "${ARGS[@]}" ::: "${FILES[@]}" \ From c633361f063b6ee2675b2075ff327ec2530fa21a Mon Sep 17 00:00:00 2001 From: Frantisek Sumsal Date: Tue, 26 Dec 2023 19:39:12 +0100 Subject: [PATCH 04/10] coccinelle: dial back warnings about performance Turns out I _really_ underestimated the impact of --include-headers-for-types, as it significantly reduces both runtime and storage penalties. For example, on my machine the runtime of uncached run goes down from ~15 minutes to ~2 minutes, and similarly the total storage needed by the cache goes from ~15 GiB down to ~3 GiB. --- coccinelle/run-coccinelle.sh | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/coccinelle/run-coccinelle.sh b/coccinelle/run-coccinelle.sh index cc2bfc14c5d..79536b38954 100755 --- a/coccinelle/run-coccinelle.sh +++ b/coccinelle/run-coccinelle.sh @@ -43,10 +43,6 @@ fi mkdir -p "$CACHE_DIR" echo "--x-- Using Coccinelle cache directory: $CACHE_DIR" -echo "--x--" -echo "--x-- Note: running spatch for the first time without populated cache takes" -echo "--x-- a _long_ time (15-30 minutes). Also, the cache is quite large" -echo "--x-- (~15 GiB), so make sure you have enough free space." echo for script in "${SCRIPTS[@]}"; do @@ -69,10 +65,8 @@ for script in "${SCRIPTS[@]}"; do # # 4) Explicitly undefine the SD_BOOT symbol, so Coccinelle ignores includes guarded by #if SD_BOOT # - # 5) Use cache, since generating the full AST is _very_ expensive, i.e. the uncached run takes 15 - 30 - # minutes (for one rule(!)), vs 30 - 90 seconds when the cache is populated. One major downside of the - # cache is that it's quite big - ATTOW the cache takes around 15 GiB, but the performance boost is - # definitely worth it + # 5) Use cache, since generating the full AST is expensive. With cache we can do that only once and then + # reuse the cached ASTs for other rules. This cuts down the time needed to run each rule by ~60%. parallel --halt now,fail=1 --keep-order --noswap --max-args=10 \ spatch --cache-prefix "$CACHE_DIR" \ -I src \ From c83f4220a1a8cd471ed3258214cd571af2e61eb8 Mon Sep 17 00:00:00 2001 From: Frantisek Sumsal Date: Tue, 26 Dec 2023 20:20:51 +0100 Subject: [PATCH 05/10] tree-wide: use IN_SET() more --- src/basic/locale-util.c | 2 +- src/shared/elf-util.c | 2 +- src/shared/pkcs11-util.c | 8 ++++---- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/basic/locale-util.c b/src/basic/locale-util.c index d3fef01febf..9e70c3f01fb 100644 --- a/src/basic/locale-util.c +++ b/src/basic/locale-util.c @@ -221,7 +221,7 @@ int get_locales(char ***ret) { locales = set_free(locales); r = getenv_bool("SYSTEMD_LIST_NON_UTF8_LOCALES"); - if (r == -ENXIO || r == 0) { + if (IN_SET(r, -ENXIO, 0)) { char **a, **b; /* Filter out non-UTF-8 locales, because it's 2019, by default */ diff --git a/src/shared/elf-util.c b/src/shared/elf-util.c index 24ed16e56b5..b3b5037a7c6 100644 --- a/src/shared/elf-util.c +++ b/src/shared/elf-util.c @@ -348,7 +348,7 @@ static int parse_package_metadata(const char *name, JsonVariant *id_json, Elf *e /* Package metadata is in PT_NOTE headers. */ program_header = sym_gelf_getphdr(elf, i, &mem); - if (!program_header || (program_header->p_type != PT_NOTE && program_header->p_type != PT_INTERP)) + if (!program_header || !IN_SET(program_header->p_type, PT_NOTE, PT_INTERP)) continue; if (program_header->p_type == PT_INTERP) { diff --git a/src/shared/pkcs11-util.c b/src/shared/pkcs11-util.c index edf4fb01b92..9792f70d422 100644 --- a/src/shared/pkcs11-util.c +++ b/src/shared/pkcs11-util.c @@ -665,7 +665,7 @@ int pkcs11_token_find_private_key( optional_attributes[1].ulValueLen = sizeof(derive_value); rv = m->C_GetAttributeValue(session, candidate, optional_attributes, ELEMENTSOF(optional_attributes)); - if (rv != CKR_OK && rv != CKR_ATTRIBUTE_TYPE_INVALID) + if (!IN_SET(rv, CKR_OK, CKR_ATTRIBUTE_TYPE_INVALID)) return log_error_errno(SYNTHETIC_ERRNO(EIO), "Failed to get attributes of a selected private key: %s", sym_p11_kit_strerror(rv)); @@ -737,7 +737,7 @@ int pkcs11_token_find_related_object( CK_RV rv; rv = m->C_GetAttributeValue(session, prototype, attributes, ELEMENTSOF(attributes)); - if (rv != CKR_OK && rv != CKR_ATTRIBUTE_TYPE_INVALID) + if (!IN_SET(rv, CKR_OK, CKR_ATTRIBUTE_TYPE_INVALID)) return log_debug_errno(SYNTHETIC_ERRNO(EIO), "Failed to retrieve length of attributes: %s", sym_p11_kit_strerror(rv)); if (attributes[0].ulValueLen != CK_UNAVAILABLE_INFORMATION) { @@ -812,7 +812,7 @@ static int ecc_convert_to_compressed( int r; rv = m->C_GetAttributeValue(session, object, &ec_params_attr, 1); - if (rv != CKR_OK && rv != CKR_ATTRIBUTE_TYPE_INVALID) + if (!IN_SET(rv, CKR_OK, CKR_ATTRIBUTE_TYPE_INVALID)) return log_error_errno(SYNTHETIC_ERRNO(EIO), "Failed to retrieve length of CKA_EC_PARAMS: %s", sym_p11_kit_strerror(rv)); @@ -834,7 +834,7 @@ static int ecc_convert_to_compressed( ec_params_attr.ulValueLen = 0; rv = m->C_GetAttributeValue(session, public_key, &ec_params_attr, 1); - if (rv != CKR_OK && rv != CKR_ATTRIBUTE_TYPE_INVALID) + if (!IN_SET(rv, CKR_OK, CKR_ATTRIBUTE_TYPE_INVALID)) return log_error_errno(SYNTHETIC_ERRNO(EIO), "Failed to retrieve length of CKA_EC_PARAMS: %s", sym_p11_kit_strerror(rv)); From 6f8b3838c9fcb644dca47d56b64058a6678069ff Mon Sep 17 00:00:00 2001 From: Frantisek Sumsal Date: Tue, 26 Dec 2023 20:21:48 +0100 Subject: [PATCH 06/10] test: use IN_SET()/ERRNO_IS_NEG_*() more --- src/test/test-mountpoint-util.c | 10 +++++----- src/test/test-xattr-util.c | 4 ++-- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/test/test-mountpoint-util.c b/src/test/test-mountpoint-util.c index 0c1b849c897..1f71a258598 100644 --- a/src/test/test-mountpoint-util.c +++ b/src/test/test-mountpoint-util.c @@ -328,23 +328,23 @@ TEST(mount_option_supported) { r = mount_option_supported("tmpfs", "size", "64M"); log_info("tmpfs supports size=64M: %s (%i)", r < 0 ? "don't know" : yes_no(r), r); - assert_se(r > 0 || r == -EAGAIN || (r < 0 && ERRNO_IS_PRIVILEGE(r))); + assert_se(r > 0 || r == -EAGAIN || ERRNO_IS_NEG_PRIVILEGE(r)); r = mount_option_supported("ext4", "discard", NULL); log_info("ext4 supports discard: %s (%i)", r < 0 ? "don't know" : yes_no(r), r); - assert_se(r > 0 || r == -EAGAIN || (r < 0 && ERRNO_IS_PRIVILEGE(r))); + assert_se(r > 0 || r == -EAGAIN || ERRNO_IS_NEG_PRIVILEGE(r)); r = mount_option_supported("tmpfs", "idontexist", "64M"); log_info("tmpfs supports idontexist: %s (%i)", r < 0 ? "don't know" : yes_no(r), r); - assert_se(r == 0 || r == -EAGAIN || (r < 0 && ERRNO_IS_PRIVILEGE(r))); + assert_se(IN_SET(r, 0, -EAGAIN) || ERRNO_IS_NEG_PRIVILEGE(r)); r = mount_option_supported("tmpfs", "ialsodontexist", NULL); log_info("tmpfs supports ialsodontexist: %s (%i)", r < 0 ? "don't know" : yes_no(r), r); - assert_se(r == 0 || r == -EAGAIN || (r < 0 && ERRNO_IS_PRIVILEGE(r))); + assert_se(IN_SET(r, 0, -EAGAIN) || ERRNO_IS_NEG_PRIVILEGE(r)); r = mount_option_supported("proc", "hidepid", "1"); log_info("proc supports hidepid=1: %s (%i)", r < 0 ? "don't know" : yes_no(r), r); - assert_se(r >= 0 || r == -EAGAIN || (r < 0 && ERRNO_IS_PRIVILEGE(r))); + assert_se(r >= 0 || r == -EAGAIN || ERRNO_IS_NEG_PRIVILEGE(r)); } TEST(fstype_can_discard) { diff --git a/src/test/test-xattr-util.c b/src/test/test-xattr-util.c index 85901c9a49b..b473165a3e0 100644 --- a/src/test/test-xattr-util.c +++ b/src/test/test-xattr-util.c @@ -45,7 +45,7 @@ TEST(getxattr_at_malloc) { fd = open("/", O_RDONLY|O_DIRECTORY|O_CLOEXEC|O_NOCTTY); assert_se(fd >= 0); r = getxattr_at_malloc(fd, "usr", "user.idontexist", 0, &value); - assert_se(r < 0 && ERRNO_IS_XATTR_ABSENT(r)); + assert_se(ERRNO_IS_NEG_XATTR_ABSENT(r)); safe_close(fd); fd = open(x, O_PATH|O_CLOEXEC); @@ -99,7 +99,7 @@ TEST(xsetxattr) { /* by full path */ r = xsetxattr(AT_FDCWD, x, "user.foo", "fullpath", SIZE_MAX, 0); - if (r < 0 && ERRNO_IS_NOT_SUPPORTED(r)) + if (ERRNO_IS_NEG_NOT_SUPPORTED(r)) return (void) log_tests_skipped_errno(r, "no xattrs supported on /var/tmp"); assert_se(r >= 0); verify_xattr(dfd, "fullpath"); From cfae9ec203f62d84cf81d0c69561fb661422d0b4 Mon Sep 17 00:00:00 2001 From: Frantisek Sumsal Date: Tue, 26 Dec 2023 20:22:53 +0100 Subject: [PATCH 07/10] test: use set_isempty() in one more place --- src/test/test-set.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/test-set.c b/src/test/test-set.c index 0d5a6a18560..af5b5553e06 100644 --- a/src/test/test-set.c +++ b/src/test/test-set.c @@ -141,7 +141,7 @@ TEST(set_ensure_allocated) { assert_se(set_ensure_allocated(&m, &string_hash_ops) == 1); assert_se(set_ensure_allocated(&m, &string_hash_ops) == 0); assert_se(set_ensure_allocated(&m, NULL) == 0); - assert_se(set_size(m) == 0); + assert_se(set_isempty(m)); } TEST(set_copy) { From b8e0dd39393ca041cea50c7f1d54faaaf6d4e834 Mon Sep 17 00:00:00 2001 From: Frantisek Sumsal Date: Tue, 26 Dec 2023 20:23:19 +0100 Subject: [PATCH 08/10] test: use timestamp_is_set() --- src/test/test-time-util.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/test/test-time-util.c b/src/test/test-time-util.c index 76931ce0ab3..d29afa38dd2 100644 --- a/src/test/test-time-util.c +++ b/src/test/test-time-util.c @@ -1089,9 +1089,9 @@ TEST(map_clock_usec) { assert_se(nowr < USEC_INFINITY - USEC_PER_DAY*7); /* overflow check */ x = nowr + USEC_PER_DAY*7; /* 1 week from now */ y = map_clock_usec(x, CLOCK_REALTIME, CLOCK_MONOTONIC); - assert_se(y > 0 && y < USEC_INFINITY); + assert_se(timestamp_is_set(y)); z = map_clock_usec(y, CLOCK_MONOTONIC, CLOCK_REALTIME); - assert_se(z > 0 && z < USEC_INFINITY); + assert_se(timestamp_is_set(z)); assert_se((z > x ? z - x : x - z) < USEC_PER_HOUR); assert_se(nowr > USEC_PER_DAY * 7); /* underflow check */ @@ -1100,7 +1100,7 @@ TEST(map_clock_usec) { if (y != 0) { /* might underflow if machine is not up long enough for the monotonic clock to be beyond 1w */ assert_se(y < USEC_INFINITY); z = map_clock_usec(y, CLOCK_MONOTONIC, CLOCK_REALTIME); - assert_se(z > 0 && z < USEC_INFINITY); + assert_se(timestamp_is_set(z)); assert_se((z > x ? z - x : x - z) < USEC_PER_HOUR); } } From 8f07111aa866e1cafc551e7432a8d91c40a2ad0f Mon Sep 17 00:00:00 2001 From: Frantisek Sumsal Date: Tue, 26 Dec 2023 21:23:55 +0100 Subject: [PATCH 09/10] bootctl: add a missing space --- src/boot/bootctl-install.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/boot/bootctl-install.c b/src/boot/bootctl-install.c index d1bd3c681c7..58470385df2 100644 --- a/src/boot/bootctl-install.c +++ b/src/boot/bootctl-install.c @@ -1018,7 +1018,7 @@ static int remove_loader_variables(void) { EFI_LOADER_VARIABLE(LoaderEntryDefault), EFI_LOADER_VARIABLE(LoaderEntryLastBooted), EFI_LOADER_VARIABLE(LoaderEntryOneShot), - EFI_LOADER_VARIABLE(LoaderSystemToken)){ + EFI_LOADER_VARIABLE(LoaderSystemToken)) { int q; From 321e64dc670699f5bbf6afe57678da795a674f7d Mon Sep 17 00:00:00 2001 From: Frantisek Sumsal Date: Tue, 26 Dec 2023 21:24:14 +0100 Subject: [PATCH 10/10] tpm2-util: declare the cleanup attribute first As we do everywhere else (apart from it being incosistent, the switched order also makes Coccinelle unhappy). --- src/shared/tpm2-util.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/shared/tpm2-util.c b/src/shared/tpm2-util.c index 2a75db572dd..80a0d5f2dc6 100644 --- a/src/shared/tpm2-util.c +++ b/src/shared/tpm2-util.c @@ -7569,7 +7569,7 @@ int tpm2_util_pbkdf2_hmac_sha256(const void *pass, size_t saltlen, uint8_t ret_key[static SHA256_DIGEST_SIZE]) { - uint8_t _cleanup_(erase_and_freep) *buffer = NULL; + _cleanup_(erase_and_freep) uint8_t *buffer = NULL; uint8_t u[SHA256_DIGEST_SIZE]; /* To keep this simple, since derived KeyLen (dkLen in docs)