From 6ba1ee5c7d06baa2aab056c5a0add04175d8ce01 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 19 Nov 2024 14:57:32 +0100 Subject: [PATCH 01/10] vconsole-setup: reword error messages MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit I was looking at some logs, and — without context — "Setting source virtual console failed" doesn't make sense. It's a source internally in vconsole-setup because we configure it and then use it to copy the settings to other consoles. But at the point this message is emitted it's not yet a source of anything. Also, "setting" sounds like we'd like to assign it somewhere, which is even more confusing. Reword the messages to be more down-to-earth and meaningful without looking at the code. --- src/vconsole/vconsole-setup.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/vconsole/vconsole-setup.c b/src/vconsole/vconsole-setup.c index 7142405657..131bd41c72 100644 --- a/src/vconsole/vconsole-setup.c +++ b/src/vconsole/vconsole-setup.c @@ -607,7 +607,7 @@ static int run(int argc, char **argv) { } else { fd = find_source_vc(&vc, &idx); if (fd < 0 && fd != -EBUSY) - return log_error_errno(fd, "No usable source console found: %m"); + return log_error_errno(fd, "No virtual console that can be configured found: %m"); } utf8 = is_locale_utf8(); @@ -617,7 +617,7 @@ static int run(int argc, char **argv) { /* We found only busy VCs, which might happen during the boot process when the boot splash is * displayed on the only allocated VC. In this case we don't interfere and avoid initializing * the VC partially as some operations are likely to fail. */ - log_notice("All allocated VCs are currently busy, skipping initialization of font and keyboard settings."); + log_notice("All allocated virtual consoles are busy, will not configure key mapping and font."); return EXIT_SUCCESS; } @@ -641,7 +641,7 @@ static int run(int argc, char **argv) { setup_remaining_vcs(fd, idx, utf8); else log_full(r == EX_OSERR ? LOG_NOTICE : LOG_WARNING, - "Setting source virtual console failed, ignoring remaining ones."); + "Configuration of first virtual console failed, ignoring remaining ones."); } return IN_SET(r, 0, EX_OSERR) && keyboard_ok ? EXIT_SUCCESS : EXIT_FAILURE; From 872710c43794e040f27fb32551ec78c72417fd0f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 19 Nov 2024 16:50:44 +0100 Subject: [PATCH 02/10] homectl: split out inspect_home() and authenticate_home() mangle_user_list() was doing a microoptimization of avoiding of copying of a single string by constructing the strv object manually. This seems like more trouble than it's worth, considering that this is called once in the program's life. Rework the code to have inspect_home() and authenticate_home() that handle a single home name and call them in a loop from the outer function. --- src/home/homectl.c | 217 +++++++++++++++++++-------------------------- 1 file changed, 92 insertions(+), 125 deletions(-) diff --git a/src/home/homectl.c b/src/home/homectl.c index c4c1ad9761..acceadf176 100644 --- a/src/home/homectl.c +++ b/src/home/homectl.c @@ -710,114 +710,106 @@ static void dump_home_record(UserRecord *hr) { } } -static char **mangle_user_list(char **list, char ***ret_allocated) { - _cleanup_free_ char *myself = NULL; - char **l; +static int inspect_home(sd_bus *bus, const char *name) { + _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; + _cleanup_(sd_bus_message_unrefp) sd_bus_message *reply = NULL; + _cleanup_(sd_json_variant_unrefp) sd_json_variant *v = NULL; + _cleanup_(user_record_unrefp) UserRecord *hr = NULL; + const char *json; + int incomplete; + uid_t uid; + int r; - if (!strv_isempty(list)) { - *ret_allocated = NULL; - return list; - } + r = parse_uid(name, &uid); + if (r < 0) + r = bus_call_method(bus, bus_mgr, "GetUserRecordByName", &error, &reply, "s", name); + else + r = bus_call_method(bus, bus_mgr, "GetUserRecordByUID", &error, &reply, "u", (uint32_t) uid); + if (r < 0) + return log_error_errno(r, "Failed to inspect home: %s", bus_error_message(&error, r)); - myself = getusername_malloc(); - if (!myself) - return NULL; + r = sd_bus_message_read(reply, "sbo", &json, &incomplete, NULL); + if (r < 0) + return bus_log_parse_error(r); - l = new(char*, 2); - if (!l) - return NULL; + r = sd_json_parse(json, SD_JSON_PARSE_SENSITIVE, &v, NULL, NULL); + if (r < 0) + return log_error_errno(r, "Failed to parse JSON identity: %m"); - l[0] = TAKE_PTR(myself); - l[1] = NULL; + hr = user_record_new(); + if (!hr) + return log_oom(); - *ret_allocated = l; - return l; + r = user_record_load(hr, v, USER_RECORD_LOAD_REFUSE_SECRET|USER_RECORD_LOG|USER_RECORD_PERMISSIVE); + if (r < 0) + return r; + + hr->incomplete = incomplete; + dump_home_record(hr); + return 0; } -static int inspect_home(int argc, char *argv[], void *userdata) { +static int inspect_homes(int argc, char *argv[], void *userdata) { _cleanup_(sd_bus_flush_close_unrefp) sd_bus *bus = NULL; - _cleanup_strv_free_ char **mangled_list = NULL; - int r, ret = 0; - char **items; - - pager_open(arg_pager_flags); + int r; r = acquire_bus(&bus); if (r < 0) return r; - items = mangle_user_list(strv_skip(argv, 1), &mangled_list); - if (!items) - return log_oom(); + pager_open(arg_pager_flags); - STRV_FOREACH(i, items) { - _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; - _cleanup_(sd_bus_message_unrefp) sd_bus_message *reply = NULL; - _cleanup_(sd_json_variant_unrefp) sd_json_variant *v = NULL; - _cleanup_(user_record_unrefp) UserRecord *hr = NULL; - const char *json; - int incomplete; - uid_t uid; - - r = parse_uid(*i, &uid); - if (r < 0) - r = bus_call_method(bus, bus_mgr, "GetUserRecordByName", &error, &reply, "s", *i); - else - r = bus_call_method(bus, bus_mgr, "GetUserRecordByUID", &error, &reply, "u", (uint32_t) uid); - if (r < 0) { - log_error_errno(r, "Failed to inspect home: %s", bus_error_message(&error, r)); - if (ret == 0) - ret = r; - - continue; - } - - r = sd_bus_message_read(reply, "sbo", &json, &incomplete, NULL); - if (r < 0) { - bus_log_parse_error(r); - if (ret == 0) - ret = r; - - continue; - } - - r = sd_json_parse(json, SD_JSON_PARSE_SENSITIVE, &v, NULL, NULL); - if (r < 0) { - log_error_errno(r, "Failed to parse JSON identity: %m"); - if (ret == 0) - ret = r; - - continue; - } - - hr = user_record_new(); - if (!hr) + char **args = strv_skip(argv, 1); + if (strv_isempty(args)) { + _cleanup_free_ char *myself = getusername_malloc(); + if (!myself) return log_oom(); - r = user_record_load(hr, v, USER_RECORD_LOAD_REFUSE_SECRET|USER_RECORD_LOG|USER_RECORD_PERMISSIVE); - if (r < 0) { - if (ret == 0) - ret = r; - - continue; - } - - hr->incomplete = incomplete; - dump_home_record(hr); + return inspect_home(bus, myself); + } else { + STRV_FOREACH(arg, args) + RET_GATHER(r, inspect_home(bus, *arg)); + return r; } - - return ret; } -static int authenticate_home(int argc, char *argv[], void *userdata) { - _cleanup_(sd_bus_flush_close_unrefp) sd_bus *bus = NULL; - _cleanup_strv_free_ char **mangled_list = NULL; - int r, ret = 0; - char **items; +static int authenticate_home(sd_bus *bus, const char *name) { + _cleanup_(user_record_unrefp) UserRecord *secret = NULL; + int r; - items = mangle_user_list(strv_skip(argv, 1), &mangled_list); - if (!items) - return log_oom(); + r = acquire_passed_secrets(name, &secret); + if (r < 0) + return r; + + for (;;) { + _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; + _cleanup_(sd_bus_message_unrefp) sd_bus_message *m = NULL; + + r = bus_message_new_method_call(bus, &m, bus_mgr, "AuthenticateHome"); + if (r < 0) + return bus_log_create_error(r); + + r = sd_bus_message_append(m, "s", name); + if (r < 0) + return bus_log_create_error(r); + + r = bus_message_append_secret(m, secret); + if (r < 0) + return bus_log_create_error(r); + + r = sd_bus_call(bus, m, HOME_SLOW_BUS_CALL_TIMEOUT_USEC, &error, NULL); + if (r < 0) { + r = handle_generic_user_record_error(name, secret, &error, r, false); + if (r >= 0) + continue; + } + return r; + } +} + +static int authenticate_homes(int argc, char *argv[], void *userdata) { + _cleanup_(sd_bus_flush_close_unrefp) sd_bus *bus = NULL; + int r; r = acquire_bus(&bus); if (r < 0) @@ -825,44 +817,19 @@ static int authenticate_home(int argc, char *argv[], void *userdata) { (void) polkit_agent_open_if_enabled(arg_transport, arg_ask_password); - STRV_FOREACH(i, items) { - _cleanup_(user_record_unrefp) UserRecord *secret = NULL; + char **args = strv_skip(argv, 1); + if (strv_isempty(args)) { + _cleanup_free_ char *myself = getusername_malloc(); + if (!myself) + return log_oom(); - r = acquire_passed_secrets(*i, &secret); - if (r < 0) - return r; + return authenticate_home(bus, myself); + } else { + STRV_FOREACH(arg, args) + RET_GATHER(r, authenticate_home(bus, *arg)); - for (;;) { - _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; - _cleanup_(sd_bus_message_unrefp) sd_bus_message *m = NULL; - - r = bus_message_new_method_call(bus, &m, bus_mgr, "AuthenticateHome"); - if (r < 0) - return bus_log_create_error(r); - - r = sd_bus_message_append(m, "s", *i); - if (r < 0) - return bus_log_create_error(r); - - r = bus_message_append_secret(m, secret); - if (r < 0) - return bus_log_create_error(r); - - r = sd_bus_call(bus, m, HOME_SLOW_BUS_CALL_TIMEOUT_USEC, &error, NULL); - if (r < 0) { - r = handle_generic_user_record_error(*i, secret, &error, r, false); - if (r < 0) { - if (ret == 0) - ret = r; - - break; - } - } else - break; - } + return r; } - - return ret; } static int update_last_change(sd_json_variant **v, bool with_password, bool override) { @@ -5477,8 +5444,8 @@ static int run(int argc, char *argv[]) { { "list", VERB_ANY, 1, VERB_DEFAULT, list_homes }, { "activate", 2, VERB_ANY, 0, activate_home }, { "deactivate", 2, VERB_ANY, 0, deactivate_home }, - { "inspect", VERB_ANY, VERB_ANY, 0, inspect_home }, - { "authenticate", VERB_ANY, VERB_ANY, 0, authenticate_home }, + { "inspect", VERB_ANY, VERB_ANY, 0, inspect_homes }, + { "authenticate", VERB_ANY, VERB_ANY, 0, authenticate_homes }, { "create", VERB_ANY, 2, 0, create_home }, { "adopt", VERB_ANY, VERB_ANY, 0, verb_adopt_home }, { "register", VERB_ANY, VERB_ANY, 0, verb_register_home }, From 55a8d118d6e9e28fc889be88c275b396f39ee4a7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 19 Nov 2024 15:41:22 +0100 Subject: [PATCH 03/10] basic/strv: return NULL from strv_skip strv_skip was written to carefully return the original array, but this turns out to be an unnecessary complication. After the previous patch, no caller cares about the distinction between NULL and { NULL }, but various callers need to wrap the process the returned value with strv_isempty(), sometimes more than once. Let's always return NULL for an empty result to allow callers to be simplified. --- src/basic/strv.c | 6 ++++-- src/home/homectl.c | 22 +++++++++++----------- src/test/test-strv.c | 20 ++++++++++++-------- 3 files changed, 27 insertions(+), 21 deletions(-) diff --git a/src/basic/strv.c b/src/basic/strv.c index 184804cea0..a4ddaa5e78 100644 --- a/src/basic/strv.c +++ b/src/basic/strv.c @@ -976,14 +976,16 @@ bool strv_fnmatch_full( } char** strv_skip(char **l, size_t n) { - while (n > 0) { if (strv_isempty(l)) - return l; + return NULL; l++, n--; } + /* To simplify callers, always return NULL instead of a zero-item array. */ + if (strv_isempty(l)) + return NULL; return l; } diff --git a/src/home/homectl.c b/src/home/homectl.c index acceadf176..23570ca31b 100644 --- a/src/home/homectl.c +++ b/src/home/homectl.c @@ -760,16 +760,16 @@ static int inspect_homes(int argc, char *argv[], void *userdata) { pager_open(arg_pager_flags); char **args = strv_skip(argv, 1); - if (strv_isempty(args)) { + if (args) { + STRV_FOREACH(arg, args) + RET_GATHER(r, inspect_home(bus, *arg)); + return r; + } else { _cleanup_free_ char *myself = getusername_malloc(); if (!myself) return log_oom(); return inspect_home(bus, myself); - } else { - STRV_FOREACH(arg, args) - RET_GATHER(r, inspect_home(bus, *arg)); - return r; } } @@ -818,17 +818,17 @@ static int authenticate_homes(int argc, char *argv[], void *userdata) { (void) polkit_agent_open_if_enabled(arg_transport, arg_ask_password); char **args = strv_skip(argv, 1); - if (strv_isempty(args)) { + if (args) { + STRV_FOREACH(arg, args) + RET_GATHER(r, authenticate_home(bus, *arg)); + + return r; + } else { _cleanup_free_ char *myself = getusername_malloc(); if (!myself) return log_oom(); return authenticate_home(bus, myself); - } else { - STRV_FOREACH(arg, args) - RET_GATHER(r, authenticate_home(bus, *arg)); - - return r; } } diff --git a/src/test/test-strv.c b/src/test/test-strv.c index d64cc55c5b..028765665b 100644 --- a/src/test/test-strv.c +++ b/src/test/test-strv.c @@ -1004,17 +1004,21 @@ TEST(strv_skip) { test_strv_skip_one(STRV_MAKE("foo", "bar", "baz"), 0, STRV_MAKE("foo", "bar", "baz")); test_strv_skip_one(STRV_MAKE("foo", "bar", "baz"), 1, STRV_MAKE("bar", "baz")); test_strv_skip_one(STRV_MAKE("foo", "bar", "baz"), 2, STRV_MAKE("baz")); - test_strv_skip_one(STRV_MAKE("foo", "bar", "baz"), 3, STRV_MAKE(NULL)); - test_strv_skip_one(STRV_MAKE("foo", "bar", "baz"), 4, STRV_MAKE(NULL)); - test_strv_skip_one(STRV_MAKE("foo", "bar", "baz"), 55, STRV_MAKE(NULL)); + test_strv_skip_one(STRV_MAKE("foo", "bar", "baz"), 3, NULL); + test_strv_skip_one(STRV_MAKE("foo", "bar", "baz"), 4, NULL); + test_strv_skip_one(STRV_MAKE("foo", "bar", "baz"), 55, NULL); test_strv_skip_one(STRV_MAKE("quux"), 0, STRV_MAKE("quux")); - test_strv_skip_one(STRV_MAKE("quux"), 1, STRV_MAKE(NULL)); - test_strv_skip_one(STRV_MAKE("quux"), 55, STRV_MAKE(NULL)); + test_strv_skip_one(STRV_MAKE("quux"), 1, NULL); + test_strv_skip_one(STRV_MAKE("quux"), 55, NULL); - test_strv_skip_one(STRV_MAKE(NULL), 0, STRV_MAKE(NULL)); - test_strv_skip_one(STRV_MAKE(NULL), 1, STRV_MAKE(NULL)); - test_strv_skip_one(STRV_MAKE(NULL), 55, STRV_MAKE(NULL)); + test_strv_skip_one(STRV_MAKE(NULL), 0, NULL); + test_strv_skip_one(STRV_MAKE(NULL), 1, NULL); + test_strv_skip_one(STRV_MAKE(NULL), 55, NULL); + + test_strv_skip_one(NULL, 0, NULL); + test_strv_skip_one(NULL, 1, NULL); + test_strv_skip_one(NULL, 55, NULL); } TEST(strv_extend_n) { From bc602f32c978e042809b7b15f09ddc6ff4e15896 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 19 Nov 2024 15:48:31 +0100 Subject: [PATCH 04/10] analyze: use STRV_FOREACH in consistent fashion Also put positive condition first. --- src/analyze/analyze-architectures.c | 28 +++++++-------- src/analyze/analyze-cat-config.c | 3 +- src/analyze/analyze-exit-status.c | 35 +++++++++--------- src/analyze/analyze-filesystems.c | 48 +++++++++++-------------- src/analyze/analyze-malloc.c | 7 ++-- src/analyze/analyze-pcrs.c | 22 ++++++------ src/analyze/analyze-syscall-filter.c | 53 +++++++++++++--------------- src/analyze/analyze-timestamp.c | 9 ++--- 8 files changed, 98 insertions(+), 107 deletions(-) diff --git a/src/analyze/analyze-architectures.c b/src/analyze/analyze-architectures.c index 08c62e6285..832d50c5b7 100644 --- a/src/analyze/analyze-architectures.c +++ b/src/analyze/analyze-architectures.c @@ -47,30 +47,25 @@ int verb_architectures(int argc, char *argv[], void *userdata) { (void) table_hide_column_from_display(table, (size_t) 0); - if (strv_isempty(strv_skip(argv, 1))) - for (Architecture a = 0; a < _ARCHITECTURE_MAX; a++) { - r = add_arch(table, a); - if (r < 0) - return r; - } - else { - STRV_FOREACH(as, strv_skip(argv, 1)) { + char **args = strv_skip(argv, 1); + if (args) { + STRV_FOREACH(arg, args) { Architecture a; - if (streq(*as, "native")) + if (streq(*arg, "native")) a = native_architecture(); - else if (streq(*as, "uname")) + else if (streq(*arg, "uname")) a = uname_architecture(); - else if (streq(*as, "secondary")) { + else if (streq(*arg, "secondary")) { #ifdef ARCHITECTURE_SECONDARY a = ARCHITECTURE_SECONDARY; #else return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "No secondary architecture."); #endif } else - a = architecture_from_string(*as); + a = architecture_from_string(*arg); if (a < 0) - return log_error_errno(a, "Architecture \"%s\" not known.", *as); + return log_error_errno(a, "Architecture \"%s\" not known.", *arg); r = add_arch(table, a); if (r < 0) @@ -78,7 +73,12 @@ int verb_architectures(int argc, char *argv[], void *userdata) { } (void) table_set_sort(table, (size_t) 0); - } + } else + for (Architecture a = 0; a < _ARCHITECTURE_MAX; a++) { + r = add_arch(table, a); + if (r < 0) + return r; + } r = table_print_with_pager(table, arg_json_format_flags, arg_pager_flags, arg_legend); if (r < 0) diff --git a/src/analyze/analyze-cat-config.c b/src/analyze/analyze-cat-config.c index b480d4a009..8b86ef9328 100644 --- a/src/analyze/analyze-cat-config.c +++ b/src/analyze/analyze-cat-config.c @@ -9,12 +9,11 @@ #include "strv.h" int verb_cat_config(int argc, char *argv[], void *userdata) { - char **list; int r; pager_open(arg_pager_flags); - list = strv_skip(argv, 1); + char **list = strv_skip(argv, 1); STRV_FOREACH(arg, list) { const char *t = NULL; diff --git a/src/analyze/analyze-exit-status.c b/src/analyze/analyze-exit-status.c index 1032f1a4b7..eb60aa0051 100644 --- a/src/analyze/analyze-exit-status.c +++ b/src/analyze/analyze-exit-status.c @@ -17,7 +17,24 @@ int verb_exit_status(int argc, char *argv[], void *userdata) { if (r < 0) return log_error_errno(r, "Failed to right-align status: %m"); - if (strv_isempty(strv_skip(argv, 1))) + char **args = strv_skip(argv, 1); + if (args) + STRV_FOREACH(arg, args) { + int status; + + status = exit_status_from_string(*arg); + if (status < 0) + return log_error_errno(status, "Invalid exit status \"%s\".", *arg); + + assert(status >= 0 && (size_t) status < ELEMENTSOF(exit_status_mappings)); + r = table_add_many(table, + TABLE_STRING, exit_status_mappings[status].name ?: "-", + TABLE_INT, status, + TABLE_STRING, exit_status_class(status) ?: "-"); + if (r < 0) + return table_log_add_error(r); + } + else for (size_t i = 0; i < ELEMENTSOF(exit_status_mappings); i++) { if (!exit_status_mappings[i].name) continue; @@ -29,22 +46,6 @@ int verb_exit_status(int argc, char *argv[], void *userdata) { if (r < 0) return table_log_add_error(r); } - else - for (int i = 1; i < argc; i++) { - int status; - - status = exit_status_from_string(argv[i]); - if (status < 0) - return log_error_errno(status, "Invalid exit status \"%s\".", argv[i]); - - assert(status >= 0 && (size_t) status < ELEMENTSOF(exit_status_mappings)); - r = table_add_many(table, - TABLE_STRING, exit_status_mappings[status].name ?: "-", - TABLE_INT, status, - TABLE_STRING, exit_status_class(status) ?: "-"); - if (r < 0) - return table_log_add_error(r); - } r = table_print_with_pager(table, arg_json_format_flags, arg_pager_flags, arg_legend); if (r < 0) diff --git a/src/analyze/analyze-filesystems.c b/src/analyze/analyze-filesystems.c index b09cd9fa0e..8ec542ec7a 100644 --- a/src/analyze/analyze-filesystems.c +++ b/src/analyze/analyze-filesystems.c @@ -106,15 +106,30 @@ static void dump_filesystem_set(const FilesystemSet *set) { } int verb_filesystems(int argc, char *argv[], void *userdata) { - bool first = true; - #if ! HAVE_LIBBPF return log_error_errno(SYNTHETIC_ERRNO(EOPNOTSUPP), "Not compiled with libbpf support, sorry."); #endif pager_open(arg_pager_flags); - if (strv_isempty(strv_skip(argv, 1))) { + char **args = strv_skip(argv, 1); + if (args) + STRV_FOREACH(name, args) { + if (name != args) + puts(""); + + const FilesystemSet *set = filesystem_set_find(*name); + if (!set) { + /* make sure the error appears below normal output */ + fflush(stdout); + + return log_error_errno(SYNTHETIC_ERRNO(ENOENT), + "Filesystem set \"%s\" not found.", *name); + } + + dump_filesystem_set(set); + } + else { _cleanup_set_free_ Set *kernel = NULL, *known = NULL; int k; @@ -126,27 +141,24 @@ int verb_filesystems(int argc, char *argv[], void *userdata) { for (FilesystemGroups i = 0; i < _FILESYSTEM_SET_MAX; i++) { const FilesystemSet *set = filesystem_sets + i; - if (!first) + if (i > 0) puts(""); dump_filesystem_set(set); filesystem_set_remove(kernel, set); if (i != FILESYSTEM_SET_KNOWN) filesystem_set_remove(known, set); - first = false; } if (arg_quiet) /* Let's not show the extra stuff in quiet mode */ return 0; if (!set_isempty(known)) { - _cleanup_free_ char **l = NULL; - printf("\n" "# %sUngrouped filesystems%s (known but not included in any of the groups except @known):\n", ansi_highlight(), ansi_normal()); - l = set_get_strv(known); + _cleanup_free_ char **l = set_get_strv(known); if (!l) return log_oom(); @@ -197,25 +209,7 @@ int verb_filesystems(int argc, char *argv[], void *userdata) { STRV_FOREACH(filesystem, l) printf("# %s\n", *filesystem); } - } else - STRV_FOREACH(name, strv_skip(argv, 1)) { - const FilesystemSet *set; - - if (!first) - puts(""); - - set = filesystem_set_find(*name); - if (!set) { - /* make sure the error appears below normal output */ - fflush(stdout); - - return log_error_errno(SYNTHETIC_ERRNO(ENOENT), - "Filesystem set \"%s\" not found.", *name); - } - - dump_filesystem_set(set); - first = false; - } + } return EXIT_SUCCESS; } diff --git a/src/analyze/analyze-malloc.c b/src/analyze/analyze-malloc.c index 9eb234e669..0438d03ae8 100644 --- a/src/analyze/analyze-malloc.c +++ b/src/analyze/analyze-malloc.c @@ -35,11 +35,12 @@ int verb_malloc(int argc, char *argv[], void *userdata) { char **services = STRV_MAKE("org.freedesktop.systemd1"); int r; - if (!strv_isempty(strv_skip(argv, 1))) { - services = strv_skip(argv, 1); - STRV_FOREACH(service, services) + char **args = strv_skip(argv, 1); + if (args) { + STRV_FOREACH(service, args) if (!service_name_is_valid(*service)) return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "D-Bus service name '%s' is not valid.", *service); + services = args; } r = acquire_bus(&bus, NULL); diff --git a/src/analyze/analyze-pcrs.c b/src/analyze/analyze-pcrs.c index fb67a733dd..23f8f0773d 100644 --- a/src/analyze/analyze-pcrs.c +++ b/src/analyze/analyze-pcrs.c @@ -114,19 +114,14 @@ int verb_pcrs(int argc, char *argv[], void *userdata) { if (!alg) /* hide hash column if we couldn't acquire it */ (void) table_set_display(table, 0, 1); - if (strv_isempty(strv_skip(argv, 1))) - for (uint32_t pi = 0; pi < _TPM2_PCR_INDEX_MAX_DEFINED; pi++) { - r = add_pcr_to_table(table, alg, pi); - if (r < 0) - return r; - } - else { - for (int i = 1; i < argc; i++) { + char **args = strv_skip(argv, 1); + if (args) { + STRV_FOREACH(arg, args) { int pi; - pi = tpm2_pcr_index_from_string(argv[i]); + pi = tpm2_pcr_index_from_string(*arg); if (pi < 0) - return log_error_errno(pi, "PCR index \"%s\" not known.", argv[i]); + return log_error_errno(pi, "PCR index \"%s\" not known.", *arg); r = add_pcr_to_table(table, alg, pi); if (r < 0) @@ -134,7 +129,12 @@ int verb_pcrs(int argc, char *argv[], void *userdata) { } (void) table_set_sort(table, (size_t) 0); - } + } else + for (uint32_t pi = 0; pi < _TPM2_PCR_INDEX_MAX_DEFINED; pi++) { + r = add_pcr_to_table(table, alg, pi); + if (r < 0) + return r; + } r = table_print_with_pager(table, arg_json_format_flags, arg_pager_flags, /* show_header= */true); if (r < 0) diff --git a/src/analyze/analyze-syscall-filter.c b/src/analyze/analyze-syscall-filter.c index 66a52da897..fceeff8275 100644 --- a/src/analyze/analyze-syscall-filter.c +++ b/src/analyze/analyze-syscall-filter.c @@ -103,12 +103,30 @@ static void dump_syscall_filter(const SyscallFilterSet *set) { } int verb_syscall_filters(int argc, char *argv[], void *userdata) { - bool first = true; int r; pager_open(arg_pager_flags); - if (strv_isempty(strv_skip(argv, 1))) { + char **args = strv_skip(argv, 1); + if (args) + STRV_FOREACH(name, args) { + const SyscallFilterSet *set; + + if (name != args) + puts(""); + + set = syscall_filter_set_find(*name); + if (!set) { + /* make sure the error appears below normal output */ + fflush(stdout); + + return log_error_errno(SYNTHETIC_ERRNO(ENOENT), + "Filter set \"%s\" not found.", *name); + } + + dump_syscall_filter(set); + } + else { _cleanup_set_free_ Set *kernel = NULL, *known = NULL; int k = 0; /* explicit initialization to appease gcc */ @@ -121,27 +139,24 @@ int verb_syscall_filters(int argc, char *argv[], void *userdata) { for (int i = 0; i < _SYSCALL_FILTER_SET_MAX; i++) { const SyscallFilterSet *set = syscall_filter_sets + i; - if (!first) + if (i > 0) puts(""); dump_syscall_filter(set); syscall_set_remove(kernel, set); if (i != SYSCALL_FILTER_SET_KNOWN) syscall_set_remove(known, set); - first = false; } if (arg_quiet) /* Let's not show the extra stuff in quiet mode */ return 0; if (!set_isempty(known)) { - _cleanup_free_ char **l = NULL; - printf("\n" "# %sUngrouped System Calls%s (known but not included in any of the groups except @known):\n", ansi_highlight(), ansi_normal()); - l = set_get_strv(known); + _cleanup_free_ char **l = set_get_strv(known); if (!l) return log_oom(); @@ -157,13 +172,11 @@ int verb_syscall_filters(int argc, char *argv[], void *userdata) { if (!arg_quiet) log_notice_errno(k, "# Not showing unlisted system calls, couldn't retrieve kernel system call list: %m"); } else if (!set_isempty(kernel)) { - _cleanup_free_ char **l = NULL; - printf("\n" "# %sUnlisted System Calls%s (supported by the local kernel, but not included in any of the groups listed above):\n", ansi_highlight(), ansi_normal()); - l = set_get_strv(kernel); + _cleanup_free_ char **l = set_get_strv(kernel); if (!l) return log_oom(); @@ -172,25 +185,7 @@ int verb_syscall_filters(int argc, char *argv[], void *userdata) { STRV_FOREACH(syscall, l) printf("# %s\n", *syscall); } - } else - STRV_FOREACH(name, strv_skip(argv, 1)) { - const SyscallFilterSet *set; - - if (!first) - puts(""); - - set = syscall_filter_set_find(*name); - if (!set) { - /* make sure the error appears below normal output */ - fflush(stdout); - - return log_error_errno(SYNTHETIC_ERRNO(ENOENT), - "Filter set \"%s\" not found.", *name); - } - - dump_syscall_filter(set); - first = false; - } + } return EXIT_SUCCESS; } diff --git a/src/analyze/analyze-timestamp.c b/src/analyze/analyze-timestamp.c index 8c96a8ec55..791fa28b1f 100644 --- a/src/analyze/analyze-timestamp.c +++ b/src/analyze/analyze-timestamp.c @@ -75,11 +75,12 @@ static int test_timestamp_one(const char *p) { int verb_timestamp(int argc, char *argv[], void *userdata) { int r = 0; - STRV_FOREACH(p, strv_skip(argv, 1)) { - RET_GATHER(r, test_timestamp_one(*p)); + char **args = strv_skip(argv, 1); + STRV_FOREACH(arg, args) { + if (arg != args) + puts(""); - if (p[1]) - putchar('\n'); + RET_GATHER(r, test_timestamp_one(*arg)); } return r; From 635accef79c64f275e3d732bb093dbb8a7cd9da7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 19 Nov 2024 16:26:03 +0100 Subject: [PATCH 05/10] busctl: use STRV_FOREACH in the usual fashion Also put positive branch first, do not use 'i' as a char** variable name. --- src/busctl/busctl.c | 53 ++++++++++++++++++++++----------------------- 1 file changed, 26 insertions(+), 27 deletions(-) diff --git a/src/busctl/busctl.c b/src/busctl/busctl.c index 2e25e6a006..21f45df682 100644 --- a/src/busctl/busctl.c +++ b/src/busctl/busctl.c @@ -547,9 +547,25 @@ static int tree(int argc, char **argv, void *userdata) { if (r < 0) return r; - if (argc <= 1) { + char **args = strv_skip(argv, 1); + if (args) + STRV_FOREACH(arg, args) { + int q; + + if (arg != args) + puts(""); + + if (args[1]) { + pager_open(arg_pager_flags); + printf("Service %s%s%s:\n", ansi_highlight(), *arg, ansi_normal()); + } + + q = tree_one(bus, *arg); + if (q < 0 && r >= 0) + r = q; + } + else { _cleanup_strv_free_ char **names = NULL; - bool not_first = false; r = sd_bus_list_names(bus, &names, NULL); if (r < 0) @@ -557,42 +573,25 @@ static int tree(int argc, char **argv, void *userdata) { pager_open(arg_pager_flags); - STRV_FOREACH(i, names) { + STRV_FOREACH(name, names) { int q; - if (!arg_unique && (*i)[0] == ':') + if (!arg_unique && (*name)[0] == ':') continue; - if (!arg_acquired && (*i)[0] == ':') + if (!arg_acquired && (*name)[0] == ':') continue; - if (not_first) - printf("\n"); + if (name != names) + puts(""); - printf("Service %s%s%s:\n", ansi_highlight(), *i, ansi_normal()); + printf("Service %s%s%s:\n", ansi_highlight(), *name, ansi_normal()); - q = tree_one(bus, *i); - if (q < 0 && r >= 0) - r = q; - - not_first = true; - } - } else - STRV_FOREACH(i, strv_skip(argv, 1)) { - int q; - - if (i > argv+1) - printf("\n"); - - if (argv[2]) { - pager_open(arg_pager_flags); - printf("Service %s%s%s:\n", ansi_highlight(), *i, ansi_normal()); - } - - q = tree_one(bus, *i); + q = tree_one(bus, *name); if (q < 0 && r >= 0) r = q; } + } return r; } From 129446ddec0acad88097a008b28418a5efb76e19 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 19 Nov 2024 16:27:50 +0100 Subject: [PATCH 06/10] busctl: use RET_GATHER --- src/busctl/busctl.c | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/src/busctl/busctl.c b/src/busctl/busctl.c index 21f45df682..557a4571f2 100644 --- a/src/busctl/busctl.c +++ b/src/busctl/busctl.c @@ -550,8 +550,6 @@ static int tree(int argc, char **argv, void *userdata) { char **args = strv_skip(argv, 1); if (args) STRV_FOREACH(arg, args) { - int q; - if (arg != args) puts(""); @@ -560,9 +558,7 @@ static int tree(int argc, char **argv, void *userdata) { printf("Service %s%s%s:\n", ansi_highlight(), *arg, ansi_normal()); } - q = tree_one(bus, *arg); - if (q < 0 && r >= 0) - r = q; + RET_GATHER(r, tree_one(bus, *arg)); } else { _cleanup_strv_free_ char **names = NULL; @@ -574,8 +570,6 @@ static int tree(int argc, char **argv, void *userdata) { pager_open(arg_pager_flags); STRV_FOREACH(name, names) { - int q; - if (!arg_unique && (*name)[0] == ':') continue; @@ -587,9 +581,7 @@ static int tree(int argc, char **argv, void *userdata) { printf("Service %s%s%s:\n", ansi_highlight(), *name, ansi_normal()); - q = tree_one(bus, *name); - if (q < 0 && r >= 0) - r = q; + RET_GATHER(r, tree_one(bus, *name)); } } From fdfa9fd8abbc0e07738b1345ea1b15350ef158d2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 19 Nov 2024 16:51:27 +0100 Subject: [PATCH 07/10] various: handle the positive condition after strv_skip() first --- src/id128/id128.c | 23 +++++++++++---------- src/systemctl/systemctl-list-dependencies.c | 10 ++++----- src/udev/udev-builtin-kmod.c | 9 ++++---- 3 files changed, 22 insertions(+), 20 deletions(-) diff --git a/src/id128/id128.c b/src/id128/id128.c index ed444c8022..d3e2f28c1e 100644 --- a/src/id128/id128.c +++ b/src/id128/id128.c @@ -133,17 +133,7 @@ static int verb_show(int argc, char **argv, void *userdata) { int r; argv = strv_skip(argv, 1); - if (strv_isempty(argv)) { - if (!sd_id128_is_null(arg_app)) - return log_error_errno(SYNTHETIC_ERRNO(EINVAL), - "'show --app-specific=' can only be used with explicit UUID input."); - - for (const GptPartitionType *e = gpt_partition_type_table; e->name; e++) { - r = show_one(&table, e->name, e->uuid, e == gpt_partition_type_table); - if (r < 0) - return r; - } - } else + if (argv) STRV_FOREACH(p, argv) { sd_id128_t uuid; const char *id = NULL; @@ -171,6 +161,17 @@ static int verb_show(int argc, char **argv, void *userdata) { if (r < 0) return r; } + else { + if (!sd_id128_is_null(arg_app)) + return log_error_errno(SYNTHETIC_ERRNO(EINVAL), + "'show --app-specific=' can only be used with explicit UUID input."); + + for (const GptPartitionType *e = gpt_partition_type_table; e->name; e++) { + r = show_one(&table, e->name, e->uuid, e == gpt_partition_type_table); + if (r < 0) + return r; + } + } if (table) { r = table_print_with_pager(table, arg_json_format_flags, arg_pager_flags, arg_legend); diff --git a/src/systemctl/systemctl-list-dependencies.c b/src/systemctl/systemctl-list-dependencies.c index 335e32f71c..8cd9fc83a1 100644 --- a/src/systemctl/systemctl-list-dependencies.c +++ b/src/systemctl/systemctl-list-dependencies.c @@ -171,14 +171,14 @@ int verb_list_dependencies(int argc, char *argv[], void *userdata) { return r; patterns = strv_skip(argv, 1); - if (strv_isempty(patterns)) { - units = strv_new(SPECIAL_DEFAULT_TARGET); - if (!units) - return log_oom(); - } else { + if (patterns) { r = expand_unit_names(bus, patterns, NULL, &units, NULL); if (r < 0) return log_error_errno(r, "Failed to expand names: %m"); + } else { + units = strv_new(SPECIAL_DEFAULT_TARGET); + if (!units) + return log_oom(); } pager_open(arg_pager_flags); diff --git a/src/udev/udev-builtin-kmod.c b/src/udev/udev-builtin-kmod.c index e0553ce3e4..33031f7f40 100644 --- a/src/udev/udev-builtin-kmod.c +++ b/src/udev/udev-builtin-kmod.c @@ -35,7 +35,10 @@ static int builtin_kmod(UdevEvent *event, int argc, char *argv[]) { "%s: expected: load [module…]", argv[0]); char **modules = strv_skip(argv, 2); - if (strv_isempty(modules)) { + if (modules) + STRV_FOREACH(module, modules) + (void) module_load_and_warn(ctx, *module, /* verbose = */ false); + else { const char *modalias; r = sd_device_get_property_value(dev, "MODALIAS", &modalias); @@ -43,9 +46,7 @@ static int builtin_kmod(UdevEvent *event, int argc, char *argv[]) { return log_device_warning_errno(dev, r, "Failed to read property \"MODALIAS\": %m"); (void) module_load_and_warn(ctx, modalias, /* verbose = */ false); - } else - STRV_FOREACH(module, modules) - (void) module_load_and_warn(ctx, *module, /* verbose = */ false); + } return 0; } From d193cb77c76be43e3a130a19e329093168dc4a50 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 19 Nov 2024 16:51:39 +0100 Subject: [PATCH 08/10] test-execve: minor simplification --- src/test/test-execve.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/test/test-execve.c b/src/test/test-execve.c index e7a9a51377..6865ba9274 100644 --- a/src/test/test-execve.c +++ b/src/test/test-execve.c @@ -20,15 +20,13 @@ */ static int run(int argc, char **argv) { - _cleanup_close_ int fd = -EBADF; - char **args = strv_skip(argv, 1); int r; test_setup_logging(LOG_DEBUG); - args = !strv_isempty(args) ? args : STRV_MAKE("/bin/true"); + char **args = strv_skip(argv, 1) ?: STRV_MAKE("/bin/true"); - fd = open(args[0], O_RDONLY | O_CLOEXEC); + _cleanup_close_ int fd = open(args[0], O_RDONLY | O_CLOEXEC); if (fd < 0) return log_error_errno(errno, "open(%s) failed: %m", args[0]); From 6bb9caa256625572eb48a728811125ad0f5c4fcc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 19 Nov 2024 16:56:17 +0100 Subject: [PATCH 09/10] shared/exec-util: fix logging of the args of an executed program The debug logs has lots of "About to execute /some/path (null)". This occurs when the args array is empty. Instead, only print "(null)" if we failed with oom. Having strv_skip() return NULL makes this pleasant to write without repeating strv_isempty() a few times. --- src/shared/exec-util.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/shared/exec-util.c b/src/shared/exec-util.c index bc997a9383..01c6d74a00 100644 --- a/src/shared/exec-util.c +++ b/src/shared/exec-util.c @@ -146,11 +146,13 @@ static int do_execute( } if (DEBUG_LOGGING) { - _cleanup_free_ char *args = NULL; - if (argv) - args = quote_command_line(strv_skip(argv, 1), SHELL_ESCAPE_EMPTY); + _cleanup_free_ char *s = NULL; - log_debug("About to execute %s%s%s", t, argv ? " " : "", argv ? strnull(args) : ""); + char **args = strv_skip(argv, 1); + if (args) + s = quote_command_line(args, SHELL_ESCAPE_EMPTY); + + log_debug("About to execute %s%s%s", t, args ? " " : "", args ? strnull(s) : ""); } if (FLAGS_SET(flags, EXEC_DIR_WARN_WORLD_WRITABLE)) { From bcc1eebd8fbb93dca9daa44ef5da17a39fcd4235 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 19 Nov 2024 17:06:48 +0100 Subject: [PATCH 10/10] gpt-auto-generator: improve log message We said "exiting", but then the program continues to do other operations and log additional messages. --- src/gpt-auto-generator/gpt-auto-generator.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/gpt-auto-generator/gpt-auto-generator.c b/src/gpt-auto-generator/gpt-auto-generator.c index 8a49948780..0addb5a82c 100644 --- a/src/gpt-auto-generator/gpt-auto-generator.c +++ b/src/gpt-auto-generator/gpt-auto-generator.c @@ -690,8 +690,9 @@ static int add_root_mount(void) { r = efi_loader_get_device_part_uuid(/* ret_uuid= */ NULL); if (r == -ENOENT) { - log_notice("EFI loader partition unknown, exiting.\n" - "(The boot loader did not set EFI variable LoaderDevicePartUUID.)"); + log_notice("EFI loader partition unknown, not processing %s.\n" + "(The boot loader did not set EFI variable LoaderDevicePartUUID.)", + in_initrd() ? "/sysroot" : "/"); return 0; } if (r < 0)