From ccb5a185e4bc449f6f53ebe55fa37d85669c5cf0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 10 Apr 2026 18:07:29 +0200 Subject: [PATCH 1/3] executor: convert to the new option parser, define OPTION_COMMON_LOG_* Co-developed-by: Claude Opus 4.6 --- src/core/executor.c | 124 ++++++++++++++----------------------------- src/shared/options.h | 12 +++++ 2 files changed, 53 insertions(+), 83 deletions(-) diff --git a/src/core/executor.c b/src/core/executor.c index 9ca15cf351..80d4d8c71a 100644 --- a/src/core/executor.c +++ b/src/core/executor.c @@ -1,6 +1,5 @@ /* SPDX-License-Identifier: LGPL-2.1-or-later */ -#include #include #include "sd-messages.h" @@ -18,9 +17,10 @@ #include "fd-util.h" #include "fdset.h" #include "fileio.h" -#include "getopt-defs.h" +#include "format-table.h" #include "label-util.h" #include "log.h" +#include "options.h" #include "parse-util.h" #include "pretty-print.h" #include "selinux-util.h" @@ -32,125 +32,90 @@ STATIC_DESTRUCTOR_REGISTER(arg_serialization, fclosep); static int help(void) { _cleanup_free_ char *link = NULL; + _cleanup_(table_unrefp) Table *options = NULL; int r; r = terminal_urlify_man("systemd", "1", &link); if (r < 0) return log_oom(); + r = option_parser_get_help_table(&options); + if (r < 0) + return r; + printf("%s [OPTIONS...]\n\n" - "%sSandbox and execute processes.%s\n\n" - " -h --help Show this help and exit\n" - " --version Print version string and exit\n" - " --log-target=TARGET Set log target (console, journal,\n" - " journal-or-kmsg,\n" - " kmsg, null)\n" - " --log-level=LEVEL Set log level (debug, info, notice,\n" - " warning, err, crit,\n" - " alert, emerg)\n" - " --log-color=BOOL Highlight important messages\n" - " --log-location=BOOL Include code location in messages\n" - " --log-time=BOOL Prefix messages with current time\n" - " --deserialize=FD Deserialize process config from FD\n" - "\nSee the %s for details.\n", + "%sSandbox and execute processes.%s\n\n", program_invocation_short_name, ansi_highlight(), - ansi_normal(), - link); + ansi_normal()); + r = table_print_or_warn(options); + if (r < 0) + return r; + + printf("\nSee the %s for details.\n", link); return 0; } static int parse_argv(int argc, char *argv[]) { - enum { - COMMON_GETOPT_ARGS, - ARG_VERSION, - ARG_DESERIALIZE, - }; - - static const struct option options[] = { - { "log-level", required_argument, NULL, ARG_LOG_LEVEL }, - { "log-target", required_argument, NULL, ARG_LOG_TARGET }, - { "log-color", required_argument, NULL, ARG_LOG_COLOR }, - { "log-location", required_argument, NULL, ARG_LOG_LOCATION }, - { "log-time", required_argument, NULL, ARG_LOG_TIME }, - { "help", no_argument, NULL, 'h' }, - { "version", no_argument, NULL, ARG_VERSION }, - { "deserialize", required_argument, NULL, ARG_DESERIALIZE }, - {} - }; - - int c, r; + int r; assert(argc >= 0); assert(argv); - while ((c = getopt_long(argc, argv, "h", options, NULL)) >= 0) + OptionParser state = { argc, argv }; + const char *arg; + + FOREACH_OPTION(&state, c, &arg, /* on_error= */ return c) switch (c) { - case 'h': + + OPTION_COMMON_HELP: return help(); - case ARG_VERSION: + OPTION_COMMON_VERSION: return version(); - case ARG_LOG_LEVEL: - r = log_set_max_level_from_string(optarg); + OPTION_COMMON_LOG_LEVEL: + r = log_set_max_level_from_string(arg); if (r < 0) - return log_error_errno(r, "Failed to parse log level \"%s\": %m", optarg); - + return log_error_errno(r, "Failed to parse log level \"%s\": %m", arg); break; - case ARG_LOG_TARGET: - r = log_set_target_from_string(optarg); + OPTION_COMMON_LOG_TARGET: + r = log_set_target_from_string(arg); if (r < 0) - return log_error_errno(r, "Failed to parse log target \"%s\": %m", optarg); - + return log_error_errno(r, "Failed to parse log target \"%s\": %m", arg); break; - case ARG_LOG_COLOR: - r = log_show_color_from_string(optarg); + OPTION_COMMON_LOG_COLOR: + r = log_show_color_from_string(arg); if (r < 0) - return log_error_errno( - r, - "Failed to parse log color setting \"%s\": %m", - optarg); - + return log_error_errno(r, "Failed to parse log color setting \"%s\": %m", arg); break; - case ARG_LOG_LOCATION: - r = log_show_location_from_string(optarg); + OPTION_COMMON_LOG_LOCATION: + r = log_show_location_from_string(arg); if (r < 0) - return log_error_errno( - r, - "Failed to parse log location setting \"%s\": %m", - optarg); - + return log_error_errno(r, "Failed to parse log location setting \"%s\": %m", arg); break; - case ARG_LOG_TIME: - r = log_show_time_from_string(optarg); + OPTION_COMMON_LOG_TIME: + r = log_show_time_from_string(arg); if (r < 0) - return log_error_errno( - r, - "Failed to parse log time setting \"%s\": %m", - optarg); - + return log_error_errno(r, "Failed to parse log time setting \"%s\": %m", arg); break; - case ARG_DESERIALIZE: { + OPTION_LONG("deserialize", "FD", "Deserialize process config from FD"): { _cleanup_close_ int fd = -EBADF; FILE *f; - fd = parse_fd(optarg); + fd = parse_fd(arg); if (fd < 0) - return log_error_errno(fd, - "Failed to parse serialization fd \"%s\": %m", - optarg); + return log_error_errno(fd, "Failed to parse serialization fd \"%s\": %m", arg); r = fd_cloexec(fd, /* cloexec= */ true); if (r < 0) - return log_error_errno(r, - "Failed to set serialization fd %d to close-on-exec: %m", + return log_error_errno(r, "Failed to set serialization fd %d to close-on-exec: %m", fd); f = take_fdopen(&fd, "r"); @@ -159,15 +124,8 @@ static int parse_argv(int argc, char *argv[]) { safe_fclose(arg_serialization); arg_serialization = f; - break; } - - case '?': - return -EINVAL; - - default: - assert_not_reached(); } if (!arg_serialization) diff --git a/src/shared/options.h b/src/shared/options.h index e8256059d1..262196c8d9 100644 --- a/src/shared/options.h +++ b/src/shared/options.h @@ -60,6 +60,18 @@ typedef struct Option { OPTION_LONG("no-pager", NULL, "Do not start a pager") #define OPTION_COMMON_NO_LEGEND \ OPTION_LONG("no-legend", NULL, "Do not show headers and footers") +#define OPTION_COMMON_LOG_LEVEL \ + OPTION_LONG("log-level", "LEVEL", \ + "Set log level (debug, info, notice, warning, err, crit, alert, emerg)") +#define OPTION_COMMON_LOG_TARGET \ + OPTION_LONG("log-target", "TARGET", \ + "Set log target (console, journal, journal-or-kmsg, kmsg, null)") +#define OPTION_COMMON_LOG_COLOR \ + OPTION_LONG("log-color", "BOOL", "Highlight important messages") +#define OPTION_COMMON_LOG_LOCATION \ + OPTION_LONG("log-location", "BOOL", "Include code location in messages") +#define OPTION_COMMON_LOG_TIME \ + OPTION_LONG("log-time", "BOOL", "Prefix messages with current time") #define OPTION_COMMON_CAT_CONFIG \ OPTION_LONG("cat-config", NULL, "Show configuration files") #define OPTION_COMMON_TLDR \ From 56836138b0752c1704585a573e4b12dd28f70b9d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 10 Apr 2026 23:18:10 +0200 Subject: [PATCH 2/3] executor: do not abort on invalid serialization fd E.g. --deserialize=15 would cause the program to abrt in safe_close. But in fact, we shouldn't try to do the close in any case: if the fd is not valid, we should return an error without modifying state. And if it _is_ valid, we set O_CLOEXEC on it, so it'll be closed automatically later. --- src/core/executor.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/core/executor.c b/src/core/executor.c index 80d4d8c71a..8089d376fe 100644 --- a/src/core/executor.c +++ b/src/core/executor.c @@ -16,7 +16,6 @@ #include "exit-status.h" #include "fd-util.h" #include "fdset.h" -#include "fileio.h" #include "format-table.h" #include "label-util.h" #include "log.h" @@ -106,19 +105,19 @@ static int parse_argv(int argc, char *argv[]) { break; OPTION_LONG("deserialize", "FD", "Deserialize process config from FD"): { - _cleanup_close_ int fd = -EBADF; - FILE *f; - - fd = parse_fd(arg); + int fd = parse_fd(arg); if (fd < 0) return log_error_errno(fd, "Failed to parse serialization fd \"%s\": %m", arg); + /* Set O_CLOEXEC and as a side effect, verify that the fd is valid. */ r = fd_cloexec(fd, /* cloexec= */ true); + if (r == -EBADF) + return log_error_errno(r, "Serialization fd %d is not valid.", fd); if (r < 0) return log_error_errno(r, "Failed to set serialization fd %d to close-on-exec: %m", fd); - f = take_fdopen(&fd, "r"); + FILE *f = fdopen(fd, "r"); if (!f) return log_error_errno(errno, "Failed to open serialization fd %d: %m", fd); From 961e92854aac36444c91fab3957b3ed14d91e7f3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 13 Apr 2026 14:20:43 +0200 Subject: [PATCH 3/3] executor: move reopening of the console after option parsing It seems to be interfering with systemd:check-help-systemd-executor test in CI. In practice, any messages from parse_argv() are going to be from manual invocations, since if called from PID1 the option syntax is going to be correct. So I hope this fixes the redirection of --help but otherwise is of little consequence. --- src/core/executor.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/executor.c b/src/core/executor.c index 8089d376fe..94bb481db4 100644 --- a/src/core/executor.c +++ b/src/core/executor.c @@ -156,7 +156,6 @@ static int run(int argc, char *argv[]) { cgroup_context_init(&cgroup_context); /* We might be starting the journal itself, we'll be told by the caller what to do */ - log_set_always_reopen_console(true); log_set_prohibit_ipc(true); log_setup(); @@ -165,6 +164,7 @@ static int run(int argc, char *argv[]) { return r; /* Now that we know the intended log target, allow IPC and open the final log target. */ + log_set_always_reopen_console(true); log_set_prohibit_ipc(false); log_open();