From 312dad32c1f7a9ec482d438f8001423281bcc725 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 28 Oct 2024 13:36:00 +0100 Subject: [PATCH 1/5] busctl: fix timeout calculation for "busctl monitor" The --timeout= logic was implemented incorrectly, as it would not put a a limit on the runtime of the operation, but only on the IO sleep. However, spurious wakeups are possible, hence the timer would be reset too often. Fix that, by determining the absolute timestamp early, and checking against that. Follow-up for: 989e843e7543b21b91de4368da44692d674722a5 See: #34048 --- src/busctl/busctl.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/busctl/busctl.c b/src/busctl/busctl.c index 4eeb4a98e5..2abf730102 100644 --- a/src/busctl/busctl.c +++ b/src/busctl/busctl.c @@ -1268,6 +1268,9 @@ static int monitor(int argc, char **argv, int (*dump)(sd_bus_message *m, FILE *f if (r < 0) return r; + usec_t end = arg_timeout > 0 ? + usec_add(now(CLOCK_MONOTONIC), arg_timeout) : USEC_INFINITY; + /* upgrade connection; it's not used for anything else after this call */ r = sd_bus_message_new_method_call(bus, &message, @@ -1382,8 +1385,8 @@ static int monitor(int argc, char **argv, int (*dump)(sd_bus_message *m, FILE *f if (r > 0) continue; - r = sd_bus_wait(bus, arg_timeout > 0 ? arg_timeout : UINT64_MAX); - if (r == 0 && arg_timeout > 0) { + r = sd_bus_wait(bus, arg_timeout > 0 ? usec_sub_unsigned(end, now(CLOCK_MONOTONIC)) : UINT64_MAX); + if (r == 0 && arg_timeout > 0 && now(CLOCK_MONOTONIC) >= end) { if (!arg_quiet && !sd_json_format_enabled(arg_json_format_flags)) log_info("Timed out waiting for messages, exiting."); return 0; From 8187515aab0b29a3068c3acdf39bd97c24c0182a Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 28 Oct 2024 13:48:30 +0100 Subject: [PATCH 2/5] =?UTF-8?q?busctl:=20rename=20--num-matches=3D=20?= =?UTF-8?q?=E2=86=92=20--limit-messages=3D?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We should avoid unnecessary abbreviations for such messages, and this puts a maximum limit on things, hence it should indicate this in the name. Moreover, matches is a bit confusing, since most people will probably call "busctl monitor" without any match specification, i.e. zero matches, but that's not what was meant here at all. Also, add a brief switch for this (-N) since I figure in particular "-N1" might be a frequent operation people might want to use. Follow-up for: 989e843e7543b21b91de4368da44692d674722a5 See: #34048 --- man/busctl.xml | 3 ++- shell-completion/bash/busctl | 2 +- shell-completion/zsh/_busctl | 2 +- src/busctl/busctl.c | 32 +++++++++++++++----------- test/units/TEST-74-AUX-UTILS.busctl.sh | 2 +- 5 files changed, 23 insertions(+), 18 deletions(-) diff --git a/man/busctl.xml b/man/busctl.xml index 5cf6058dc3..3733d523e5 100644 --- a/man/busctl.xml +++ b/man/busctl.xml @@ -417,7 +417,8 @@ - + + When used with the monitor command, if enabled will make diff --git a/shell-completion/bash/busctl b/shell-completion/bash/busctl index 441b2c7d43..dc2714aaae 100644 --- a/shell-completion/bash/busctl +++ b/shell-completion/bash/busctl @@ -89,7 +89,7 @@ _busctl() { --allow-interactive-authorization=no --augment-creds=no --watch-bind=yes -j -l --full --xml-interface' [ARG]='--address -H --host -M --machine --match --timeout --size --json - --destination --num-matches' + --destination -N --limit-messages' ) if __contains_word "--user" ${COMP_WORDS[*]}; then diff --git a/shell-completion/zsh/_busctl b/shell-completion/zsh/_busctl index 5e16d0f1a1..296afd1e24 100644 --- a/shell-completion/zsh/_busctl +++ b/shell-completion/zsh/_busctl @@ -310,5 +310,5 @@ _arguments \ '--allow-interactive-authorization=[Allow interactive authorization for operation]:boolean:(1 0)' \ '--timeout=[Maximum time to wait for method call completion and monitoring]:timeout (seconds)' \ '--augment-creds=[Extend credential data with data read from /proc/$PID]:boolean:(1 0)' \ - '--num-matches=[Exit after receiving a number of matches while monitoring]:integer' \ + '(-M --limit-messages)'{-M,--limit-messages}'[Exit after receiving a number of messages while monitoring]:integer' \ '*::busctl command:_busctl_commands' diff --git a/src/busctl/busctl.c b/src/busctl/busctl.c index 2abf730102..1cbe07be11 100644 --- a/src/busctl/busctl.c +++ b/src/busctl/busctl.c @@ -65,7 +65,7 @@ static bool arg_augment_creds = true; static bool arg_watch_bind = false; static usec_t arg_timeout = 0; static const char *arg_destination = NULL; -static uint64_t arg_num_matches = UINT64_MAX; +static uint64_t arg_limit_messages = UINT64_MAX; STATIC_DESTRUCTOR_REGISTER(arg_matches, strv_freep); @@ -1367,10 +1367,14 @@ static int monitor(int argc, char **argv, int (*dump)(sd_bus_message *m, FILE *f dump(m, stdout); fflush(stdout); - if (arg_num_matches != UINT64_MAX && --arg_num_matches == 0) { - if (!arg_quiet && !sd_json_format_enabled(arg_json_format_flags)) - log_info("Received requested number of matching messages, exiting."); - return 0; + if (arg_limit_messages != UINT64_MAX) { + arg_limit_messages--; + + if (arg_limit_messages == 0) { + if (!arg_quiet && !sd_json_format_enabled(arg_json_format_flags)) + log_info("Received requested maximum number of messages, exiting."); + return 0; + } } if (sd_bus_message_is_signal(m, "org.freedesktop.DBus.Local", "Disconnected") > 0) { @@ -2503,7 +2507,8 @@ static int help(void) { " --watch-bind=BOOL Wait for bus AF_UNIX socket to be bound in the file\n" " system\n" " --destination=SERVICE Destination service of a signal\n" - " --num-matches=NUMBER Exit after receiving a number of matches while\n" + " -N --limit-messages=NUMBER\n" + " Exit after receiving a number of matches while\n" " monitoring\n" "\nSee the %s for details.\n", program_invocation_short_name, @@ -2544,7 +2549,6 @@ static int parse_argv(int argc, char *argv[]) { ARG_WATCH_BIND, ARG_JSON, ARG_DESTINATION, - ARG_NUM_MATCHES, }; static const struct option options[] = { @@ -2577,7 +2581,7 @@ static int parse_argv(int argc, char *argv[]) { { "watch-bind", required_argument, NULL, ARG_WATCH_BIND }, { "json", required_argument, NULL, ARG_JSON }, { "destination", required_argument, NULL, ARG_DESTINATION }, - { "num-matches", required_argument, NULL, ARG_NUM_MATCHES }, + { "limit-messages", required_argument, NULL, 'N' }, {}, }; @@ -2586,7 +2590,7 @@ static int parse_argv(int argc, char *argv[]) { assert(argc >= 0); assert(argv); - while ((c = getopt_long(argc, argv, "hH:M:C:J:qjl", options, NULL)) >= 0) + while ((c = getopt_long(argc, argv, "hH:M:C:J:qjlN:", options, NULL)) >= 0) switch (c) { @@ -2746,12 +2750,12 @@ static int parse_argv(int argc, char *argv[]) { arg_destination = optarg; break; - case ARG_NUM_MATCHES: - r = safe_atou64(optarg, &arg_num_matches); + case 'N': + r = safe_atou64(optarg, &arg_limit_messages); if (r < 0) - return log_error_errno(r, "Failed to parse --num-matches= parameter '%s': %m", optarg); - if (arg_num_matches == 0) - return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "--num-matches= parameter cannot be 0"); + return log_error_errno(r, "Failed to parse --limit-messages= parameter: %s", optarg); + if (arg_limit_messages == 0) + return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "--limit-messages= parameter cannot be 0"); break; diff --git a/test/units/TEST-74-AUX-UTILS.busctl.sh b/test/units/TEST-74-AUX-UTILS.busctl.sh index 4949f4bac7..d83f0941f5 100755 --- a/test/units/TEST-74-AUX-UTILS.busctl.sh +++ b/test/units/TEST-74-AUX-UTILS.busctl.sh @@ -117,4 +117,4 @@ busctl get-property -j \ (! busctl set-property org.freedesktop.systemd1 /org/freedesktop/systemd1 org.freedesktop.systemd1.Manager \ KExecWatchdogUSec t "foo") -busctl --quiet --timeout 1 --num-matches 1 --match "interface=org.freedesktop.systemd1.Manager" monitor >/dev/null +busctl --quiet --timeout 1 --limit-messages 1 --match "interface=org.freedesktop.systemd1.Manager" monitor >/dev/null From 0be245a637ceb53a90af85692294667f659b40a5 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 28 Oct 2024 13:52:04 +0100 Subject: [PATCH 3/5] busctl: if --timeout= or --limit-messages= are specified with no argument, reset to defaults. Follow-up for: 989e843e7543b21b91de4368da44692d674722a5 See: #34048 --- src/busctl/busctl.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/busctl/busctl.c b/src/busctl/busctl.c index 1cbe07be11..7c2a2c1b00 100644 --- a/src/busctl/busctl.c +++ b/src/busctl/busctl.c @@ -2717,6 +2717,11 @@ static int parse_argv(int argc, char *argv[]) { break; case ARG_TIMEOUT: + if (isempty(optarg)) { + arg_timeout = 0; /* Reset to default */ + break; + } + r = parse_sec(optarg, &arg_timeout); if (r < 0) return log_error_errno(r, "Failed to parse --timeout= parameter '%s': %m", optarg); @@ -2751,6 +2756,11 @@ static int parse_argv(int argc, char *argv[]) { break; case 'N': + if (isempty(optarg)) { + arg_limit_messages = UINT64_MAX; /* Reset to default */ + break; + } + r = safe_atou64(optarg, &arg_limit_messages); if (r < 0) return log_error_errno(r, "Failed to parse --limit-messages= parameter: %s", optarg); From c00c6d19594aa88d085f1b996956da094570b5ff Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 28 Oct 2024 13:59:28 +0100 Subject: [PATCH 4/5] busctl: add a testcase that definitely causes the timeout to trigger --- test/units/TEST-74-AUX-UTILS.busctl.sh | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/test/units/TEST-74-AUX-UTILS.busctl.sh b/test/units/TEST-74-AUX-UTILS.busctl.sh index d83f0941f5..0293dfd5bd 100755 --- a/test/units/TEST-74-AUX-UTILS.busctl.sh +++ b/test/units/TEST-74-AUX-UTILS.busctl.sh @@ -117,4 +117,12 @@ busctl get-property -j \ (! busctl set-property org.freedesktop.systemd1 /org/freedesktop/systemd1 org.freedesktop.systemd1.Manager \ KExecWatchdogUSec t "foo") -busctl --quiet --timeout 1 --limit-messages 1 --match "interface=org.freedesktop.systemd1.Manager" monitor >/dev/null +busctl --quiet --timeout=1 --limit-messages=1 --match "interface=org.freedesktop.systemd1.Manager" monitor + +START_USEC=$(date +%s%6N) +busctl --quiet --timeout=500ms --match "interface=io.dontexist.NeverGonnaHappen" monitor +END_USEC=$(date +%s%6N) +USEC=$((END_USEC-START_USEC)) +# Validate that the above was delayed for at least 500ms, but at most 30s (some leeway for slow CIs) +test "$USEC" -gt 500000 +test "$USEC" -lt 30000000 From 960b342dbfd5d214ebfaaefc25957a490bc44a97 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 28 Oct 2024 14:04:05 +0100 Subject: [PATCH 5/5] busctl: add the usual section highlighting to our --help texts --- src/busctl/busctl.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/src/busctl/busctl.c b/src/busctl/busctl.c index 7c2a2c1b00..047d655304 100644 --- a/src/busctl/busctl.c +++ b/src/busctl/busctl.c @@ -2455,9 +2455,9 @@ static int help(void) { pager_open(arg_pager_flags); - printf("%s [OPTIONS...] COMMAND ...\n\n" - "%sIntrospect the D-Bus IPC bus.%s\n" - "\nCommands:\n" + printf("%1$s [OPTIONS...] COMMAND ...\n\n" + "%5$sIntrospect the D-Bus IPC bus.%6$s\n" + "\n%3$sCommands%4$s:\n" " list List bus names\n" " status [SERVICE] Show bus service, process or bus owner credentials\n" " monitor [SERVICE...] Show bus traffic\n" @@ -2475,7 +2475,7 @@ static int help(void) { " set-property SERVICE OBJECT INTERFACE PROPERTY SIGNATURE ARGUMENT...\n" " Set property value\n" " help Show this help\n" - "\nOptions:\n" + "\n%3$sOptions:%4$s\n" " -h --help Show this help\n" " --version Show package version\n" " --no-pager Do not pipe output into a pager\n" @@ -2510,11 +2510,13 @@ static int help(void) { " -N --limit-messages=NUMBER\n" " Exit after receiving a number of matches while\n" " monitoring\n" - "\nSee the %s for details.\n", + "\nSee the %2$s for details.\n", program_invocation_short_name, - ansi_highlight(), + link, + ansi_underline(), ansi_normal(), - link); + ansi_highlight(), + ansi_normal()); return 0; }