From b8f4a205f77f68055103d295074403b8ac636de9 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Mon, 1 Jan 2024 15:45:05 +0800 Subject: [PATCH 1/9] bus-wait-for-jobs: sort includes --- src/shared/bus-wait-for-jobs.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/shared/bus-wait-for-jobs.c b/src/shared/bus-wait-for-jobs.c index b3280c15df..b4bea9c926 100644 --- a/src/shared/bus-wait-for-jobs.c +++ b/src/shared/bus-wait-for-jobs.c @@ -1,13 +1,13 @@ /* SPDX-License-Identifier: LGPL-2.1-or-later */ #include "alloc-util.h" -#include "bus-wait-for-jobs.h" -#include "set.h" -#include "bus-util.h" #include "bus-internal.h" -#include "unit-def.h" +#include "bus-util.h" +#include "bus-wait-for-jobs.h" #include "escape.h" +#include "set.h" #include "strv.h" +#include "unit-def.h" typedef struct BusWaitForJobs { sd_bus *bus; From a8af9745e5c011a79687cabee459704d5e77fda9 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Mon, 1 Jan 2024 15:57:55 +0800 Subject: [PATCH 2/9] bus-wait-for-jobs: reorganize functions --- src/shared/bus-wait-for-jobs.c | 34 +++++++++++++++++----------------- src/shared/bus-wait-for-jobs.h | 8 ++++---- 2 files changed, 21 insertions(+), 21 deletions(-) diff --git a/src/shared/bus-wait-for-jobs.c b/src/shared/bus-wait-for-jobs.c index b4bea9c926..db8ed71274 100644 --- a/src/shared/bus-wait-for-jobs.c +++ b/src/shared/bus-wait-for-jobs.c @@ -23,6 +23,23 @@ typedef struct BusWaitForJobs { sd_bus_slot *slot_disconnected; } BusWaitForJobs; +BusWaitForJobs* bus_wait_for_jobs_free(BusWaitForJobs *d) { + if (!d) + return NULL; + + set_free(d->jobs); + + sd_bus_slot_unref(d->slot_disconnected); + sd_bus_slot_unref(d->slot_job_removed); + + sd_bus_unref(d->bus); + + free(d->name); + free(d->result); + + return mfree(d); +} + static int match_disconnected(sd_bus_message *m, void *userdata, sd_bus_error *error) { assert(m); @@ -60,23 +77,6 @@ static int match_job_removed(sd_bus_message *m, void *userdata, sd_bus_error *er return 0; } -BusWaitForJobs* bus_wait_for_jobs_free(BusWaitForJobs *d) { - if (!d) - return NULL; - - set_free(d->jobs); - - sd_bus_slot_unref(d->slot_disconnected); - sd_bus_slot_unref(d->slot_job_removed); - - sd_bus_unref(d->bus); - - free(d->name); - free(d->result); - - return mfree(d); -} - int bus_wait_for_jobs_new(sd_bus *bus, BusWaitForJobs **ret) { _cleanup_(bus_wait_for_jobs_freep) BusWaitForJobs *d = NULL; int r; diff --git a/src/shared/bus-wait-for-jobs.h b/src/shared/bus-wait-for-jobs.h index 4d56c079ab..2336b1332a 100644 --- a/src/shared/bus-wait-for-jobs.h +++ b/src/shared/bus-wait-for-jobs.h @@ -5,17 +5,17 @@ #include "macro.h" +typedef struct BusWaitForJobs BusWaitForJobs; + typedef enum WaitJobsFlags { BUS_WAIT_JOBS_LOG_ERROR = 1 << 0, BUS_WAIT_JOBS_LOG_SUCCESS = 1 << 1, } WaitJobsFlags; -typedef struct BusWaitForJobs BusWaitForJobs; +BusWaitForJobs* bus_wait_for_jobs_free(BusWaitForJobs *d); +DEFINE_TRIVIAL_CLEANUP_FUNC(BusWaitForJobs*, bus_wait_for_jobs_free); int bus_wait_for_jobs_new(sd_bus *bus, BusWaitForJobs **ret); -BusWaitForJobs* bus_wait_for_jobs_free(BusWaitForJobs *d); int bus_wait_for_jobs_add(BusWaitForJobs *d, const char *path); int bus_wait_for_jobs(BusWaitForJobs *d, WaitJobsFlags flags, const char* const* extra_args); int bus_wait_for_jobs_one(BusWaitForJobs *d, const char *path, WaitJobsFlags flags, const char* const* extra_args); - -DEFINE_TRIVIAL_CLEANUP_FUNC(BusWaitForJobs*, bus_wait_for_jobs_free); From e945d395e25dc3e4fd3a26758c4620e878af668f Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Mon, 1 Jan 2024 15:58:52 +0800 Subject: [PATCH 3/9] bus-wait-for-jobs: rephrase error message --- src/shared/bus-wait-for-jobs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/shared/bus-wait-for-jobs.c b/src/shared/bus-wait-for-jobs.c index db8ed71274..00f5322a6e 100644 --- a/src/shared/bus-wait-for-jobs.c +++ b/src/shared/bus-wait-for-jobs.c @@ -43,7 +43,7 @@ BusWaitForJobs* bus_wait_for_jobs_free(BusWaitForJobs *d) { static int match_disconnected(sd_bus_message *m, void *userdata, sd_bus_error *error) { assert(m); - log_error("Warning! D-Bus connection terminated."); + log_warning("D-Bus connection terminated while waiting for jobs."); sd_bus_close(sd_bus_message_get_bus(m)); return 0; From 6aac0ff01cc922bfcddaaa2291c52a6bd9e2070f Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Mon, 1 Jan 2024 16:07:46 +0800 Subject: [PATCH 4/9] bus-wait-for-jobs: a few modernizations --- src/shared/bus-wait-for-jobs.c | 85 ++++++++++++++-------------------- 1 file changed, 35 insertions(+), 50 deletions(-) diff --git a/src/shared/bus-wait-for-jobs.c b/src/shared/bus-wait-for-jobs.c index 00f5322a6e..cadb323046 100644 --- a/src/shared/bus-wait-for-jobs.c +++ b/src/shared/bus-wait-for-jobs.c @@ -50,29 +50,25 @@ static int match_disconnected(sd_bus_message *m, void *userdata, sd_bus_error *e } static int match_job_removed(sd_bus_message *m, void *userdata, sd_bus_error *error) { - const char *path, *unit, *result; BusWaitForJobs *d = ASSERT_PTR(userdata); - uint32_t id; - char *found; + _cleanup_free_ char *job_found = NULL; + const char *path, *unit, *result; int r; assert(m); - r = sd_bus_message_read(m, "uoss", &id, &path, &unit, &result); + r = sd_bus_message_read(m, "uoss", /* id = */ NULL, &path, &unit, &result); if (r < 0) { bus_log_parse_error(r); return 0; } - found = set_remove(d->jobs, (char*) path); - if (!found) + job_found = set_remove(d->jobs, (char*) path); + if (!job_found) return 0; - free(found); - - (void) free_and_strdup(&d->result, empty_to_null(result)); - (void) free_and_strdup(&d->name, empty_to_null(unit)); + (void) free_and_strdup(&d->result, empty_to_null(result)); return 0; } @@ -92,9 +88,8 @@ int bus_wait_for_jobs_new(sd_bus *bus, BusWaitForJobs **ret) { .bus = sd_bus_ref(bus), }; - /* When we are a bus client we match by sender. Direct - * connections OTOH have no initialized sender field, and - * hence we ignore the sender then */ + /* When we are a bus client we match by sender. Direct connections OTOH have no initialized sender + * field, and hence we ignore the sender then */ r = sd_bus_match_signal_async( bus, &d->slot_job_removed, @@ -138,12 +133,12 @@ static int bus_process_wait(sd_bus *bus) { } } -static int bus_job_get_service_result(BusWaitForJobs *d, char **result) { +static int bus_job_get_service_result(BusWaitForJobs *d, char **ret) { _cleanup_free_ char *dbus_path = NULL; assert(d); assert(d->name); - assert(result); + assert(ret); if (!endswith(d->name, ".service")) return -EINVAL; @@ -158,67 +153,57 @@ static int bus_job_get_service_result(BusWaitForJobs *d, char **result) { "org.freedesktop.systemd1.Service", "Result", NULL, - result); + ret); } static void log_job_error_with_service_result(const char* service, const char *result, const char* const* extra_args) { - _cleanup_free_ char *service_shell_quoted = NULL; - const char *systemctl = "systemctl", *journalctl = "journalctl"; static const struct { const char *result, *explanation; } explanations[] = { - { "resources", "of unavailable resources or another system error" }, + { "resources", "of unavailable resources or another system error" }, { "protocol", "the service did not take the steps required by its unit configuration" }, - { "timeout", "a timeout was exceeded" }, - { "exit-code", "the control process exited with error code" }, - { "signal", "a fatal signal was delivered to the control process" }, + { "timeout", "a timeout was exceeded" }, + { "exit-code", "the control process exited with error code" }, + { "signal", "a fatal signal was delivered to the control process" }, { "core-dump", "a fatal signal was delivered causing the control process to dump core" }, - { "watchdog", "the service failed to send watchdog ping" }, - { "start-limit", "start of the service was attempted too often" } + { "watchdog", "the service failed to send watchdog ping" }, + { "start-limit", "start of the service was attempted too often" }, }; + _cleanup_free_ char *service_shell_quoted = NULL; + const char *systemctl = "systemctl", *journalctl = "journalctl"; + assert(service); service_shell_quoted = shell_maybe_quote(service, 0); - if (!strv_isempty((char**) extra_args)) { + if (!strv_isempty((char* const*) extra_args)) { _cleanup_free_ char *t = NULL; - t = strv_join((char**) extra_args, " "); + t = strv_join((char* const*) extra_args, " "); systemctl = strjoina("systemctl ", t ?: ""); journalctl = strjoina("journalctl ", t ?: ""); } - if (!isempty(result)) { - size_t i; - - for (i = 0; i < ELEMENTSOF(explanations); ++i) - if (streq(result, explanations[i].result)) - break; - - if (i < ELEMENTSOF(explanations)) { - log_error("Job for %s failed because %s.\n" - "See \"%s status %s\" and \"%s -xeu %s\" for details.\n", - service, - explanations[i].explanation, - systemctl, - service_shell_quoted ?: "", - journalctl, - service_shell_quoted ?: ""); - goto finish; - } - } + if (!isempty(result)) + FOREACH_ARRAY(i, explanations, ELEMENTSOF(explanations)) + if (streq(result, i->result)) { + log_error("Job for %s failed because %s.\n" + "See \"%s status %s\" and \"%s -xeu %s\" for details.\n", + service, i->explanation, + systemctl, service_shell_quoted ?: "", + journalctl, service_shell_quoted ?: ""); + goto extra; + } log_error("Job for %s failed.\n" "See \"%s status %s\" and \"%s -xeu %s\" for details.\n", service, - systemctl, - service_shell_quoted ?: "", - journalctl, - service_shell_quoted ?: ""); + systemctl, service_shell_quoted ?: "", + journalctl, service_shell_quoted ?: ""); -finish: +extra: /* For some results maybe additional explanation is required */ if (streq_ptr(result, "start-limit")) log_info("To force a start use \"%1$s reset-failed %2$s\"\n" From edba11cbd1ce3e875f6e00443f76920ed7dcd2ae Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Mon, 1 Jan 2024 18:32:01 +0800 Subject: [PATCH 5/9] bus-wait-for-jobs: remove unneeded strna and refactor code a bit --- src/shared/bus-wait-for-jobs.c | 74 +++++++++++++++++----------------- 1 file changed, 38 insertions(+), 36 deletions(-) diff --git a/src/shared/bus-wait-for-jobs.c b/src/shared/bus-wait-for-jobs.c index cadb323046..70f2043347 100644 --- a/src/shared/bus-wait-for-jobs.c +++ b/src/shared/bus-wait-for-jobs.c @@ -213,41 +213,53 @@ extra: } static int check_wait_response(BusWaitForJobs *d, WaitJobsFlags flags, const char* const* extra_args) { + int r; + assert(d); assert(d->name); assert(d->result); + if (streq(d->result, "done")) { + if (FLAGS_SET(flags, BUS_WAIT_JOBS_LOG_SUCCESS)) + log_info("Job for %s finished.", d->name); + + return 0; + } else if (streq(d->result, "skipped")) { + if (FLAGS_SET(flags, BUS_WAIT_JOBS_LOG_SUCCESS)) + log_info("Job for %s was skipped.", d->name); + + return 0; + } + if (FLAGS_SET(flags, BUS_WAIT_JOBS_LOG_ERROR)) { if (streq(d->result, "canceled")) - log_error("Job for %s canceled.", strna(d->name)); + log_error("Job for %s canceled.", d->name); else if (streq(d->result, "timeout")) - log_error("Job for %s timed out.", strna(d->name)); + log_error("Job for %s timed out.", d->name); else if (streq(d->result, "dependency")) - log_error("A dependency job for %s failed. See 'journalctl -xe' for details.", strna(d->name)); + log_error("A dependency job for %s failed. See 'journalctl -xe' for details.", d->name); else if (streq(d->result, "invalid")) - log_error("%s is not active, cannot reload.", strna(d->name)); + log_error("%s is not active, cannot reload.", d->name); else if (streq(d->result, "assert")) - log_error("Assertion failed on job for %s.", strna(d->name)); + log_error("Assertion failed on job for %s.", d->name); else if (streq(d->result, "unsupported")) - log_error("Operation on or unit type of %s not supported on this system.", strna(d->name)); + log_error("Operation on or unit type of %s not supported on this system.", d->name); else if (streq(d->result, "collected")) - log_error("Queued job for %s was garbage collected.", strna(d->name)); + log_error("Queued job for %s was garbage collected.", d->name); else if (streq(d->result, "once")) - log_error("Unit %s was started already once and can't be started again.", strna(d->name)); - else if (!STR_IN_SET(d->result, "done", "skipped")) { + log_error("Unit %s was started already once and can't be started again.", d->name); + else if (endswith(d->name, ".service")) { + /* Job result is unknown. For services, let's also try Result property. */ + _cleanup_free_ char *result = NULL; - if (d->name && endswith(d->name, ".service")) { - _cleanup_free_ char *result = NULL; - int q; + r = bus_job_get_service_result(d, &result); + if (r < 0) + log_debug_errno(r, "Failed to get Result property of unit %s, ignoring: %m", + d->name); - q = bus_job_get_service_result(d, &result); - if (q < 0) - log_debug_errno(q, "Failed to get Result property of unit %s: %m", d->name); - - log_job_error_with_service_result(d->name, result, extra_args); - } else - log_error("Job failed. See \"journalctl -xe\" for details."); - } + log_job_error_with_service_result(d->name, result, extra_args); + } else /* Otherwise we just show a generic message. */ + log_error("Job failed. See \"journalctl -xe\" for details."); } if (STR_IN_SET(d->result, "canceled", "collected")) @@ -264,18 +276,10 @@ static int check_wait_response(BusWaitForJobs *d, WaitJobsFlags flags, const cha return -EOPNOTSUPP; else if (streq(d->result, "once")) return -ESTALE; - else if (streq(d->result, "done")) { - if (FLAGS_SET(flags, BUS_WAIT_JOBS_LOG_SUCCESS)) - log_info("Job for %s finished.", strna(d->name)); - return 0; - } else if (streq(d->result, "skipped")) { - if (FLAGS_SET(flags, BUS_WAIT_JOBS_LOG_SUCCESS)) - log_info("Job for %s was skipped.", strna(d->name)); - return 0; - } - return log_debug_errno(SYNTHETIC_ERRNO(EIO), - "Unexpected job result, assuming server side newer than us: %s", d->result); + return log_debug_errno(SYNTHETIC_ERRNO(ENOMEDIUM), + "Unexpected job result '%s' for unit '%s', assuming server side newer than us.", + d->result, d->name); } int bus_wait_for_jobs(BusWaitForJobs *d, WaitJobsFlags flags, const char* const* extra_args) { @@ -292,13 +296,11 @@ int bus_wait_for_jobs(BusWaitForJobs *d, WaitJobsFlags flags, const char* const* if (d->name && d->result) { q = check_wait_response(d, flags, extra_args); - /* Return the first error as it is most likely to be - * meaningful. */ - if (q < 0 && r == 0) - r = q; + /* Return the first error as it is most likely to be meaningful. */ + RET_GATHER(r, q); log_full_errno_zerook(LOG_DEBUG, q, - "Got result %s/%m for job %s", d->result, d->name); + "Got result %s/%m for job %s.", d->result, d->name); } d->name = mfree(d->name); From a490aa5a2da20610c180415c99d2d8103da49dd0 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Mon, 1 Jan 2024 17:15:23 +0800 Subject: [PATCH 6/9] bus-wait-for-units: use hashmap_remove_value where appropriate --- src/shared/bus-wait-for-units.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/shared/bus-wait-for-units.c b/src/shared/bus-wait-for-units.c index 0dd2a295dd..c32b0a1e14 100644 --- a/src/shared/bus-wait-for-units.c +++ b/src/shared/bus-wait-for-units.c @@ -63,7 +63,7 @@ static WaitForItem *wait_for_item_free(WaitForItem *item) { log_debug_errno(r, "Failed to drop reference to unit %s, ignoring: %m", item->bus_path); } - assert_se(hashmap_remove(item->parent->items, item->bus_path) == item); + assert_se(hashmap_remove_value(item->parent->items, item->bus_path, item)); if (item->parent->current == item) item->parent->current = NULL; From 0ffcc7539bb9b5dfffcf43108bcb6f2ecc0a6a7c Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Mon, 1 Jan 2024 17:16:11 +0800 Subject: [PATCH 7/9] bus-wait-for-units: rephrase error message --- src/shared/bus-wait-for-units.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/shared/bus-wait-for-units.c b/src/shared/bus-wait-for-units.c index c32b0a1e14..d73fff3dc1 100644 --- a/src/shared/bus-wait-for-units.c +++ b/src/shared/bus-wait-for-units.c @@ -109,7 +109,7 @@ static int match_disconnected(sd_bus_message *m, void *userdata, sd_bus_error *e assert(m); - log_error("Warning! D-Bus connection terminated."); + log_warning("D-Bus connection terminated while waiting for unit."); bus_wait_for_units_clear(d); From 86dea48ddf4bd85c5e6bca3339bb35349acd6211 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Mon, 1 Jan 2024 17:19:35 +0800 Subject: [PATCH 8/9] bus-wait-for-units: correctly report state if disconnected from bus --- src/shared/bus-wait-for-units.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/shared/bus-wait-for-units.c b/src/shared/bus-wait-for-units.c index d73fff3dc1..49963ffe34 100644 --- a/src/shared/bus-wait-for-units.c +++ b/src/shared/bus-wait-for-units.c @@ -114,7 +114,7 @@ static int match_disconnected(sd_bus_message *m, void *userdata, sd_bus_error *e bus_wait_for_units_clear(d); if (d->ready_callback) - d->ready_callback(d, false, d->userdata); + d->ready_callback(d, BUS_WAIT_FAILURE, d->userdata); else /* If no ready callback is specified close the connection so that the event loop exits */ sd_bus_close(sd_bus_message_get_bus(m)); From cbdb8732dd769c765c47d95aeee29a7a1036b100 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Mon, 1 Jan 2024 17:52:35 +0800 Subject: [PATCH 9/9] bus-wait-for-units: rearrange function --- src/shared/bus-wait-for-units.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/shared/bus-wait-for-units.h b/src/shared/bus-wait-for-units.h index 2623e72ced..c87eabcbbc 100644 --- a/src/shared/bus-wait-for-units.h +++ b/src/shared/bus-wait-for-units.h @@ -25,11 +25,11 @@ typedef void (*bus_wait_for_units_ready_callback)(BusWaitForUnits *d, BusWaitFor typedef void (*bus_wait_for_units_unit_callback)(BusWaitForUnits *d, const char *unit_path, bool good, void *userdata); int bus_wait_for_units_new(sd_bus *bus, BusWaitForUnits **ret); + BusWaitForUnits* bus_wait_for_units_free(BusWaitForUnits *d); +DEFINE_TRIVIAL_CLEANUP_FUNC(BusWaitForUnits*, bus_wait_for_units_free); BusWaitForUnitsState bus_wait_for_units_state(BusWaitForUnits *d); void bus_wait_for_units_set_ready_callback(BusWaitForUnits *d, bus_wait_for_units_ready_callback callback, void *userdata); int bus_wait_for_units_add_unit(BusWaitForUnits *d, const char *unit, BusWaitForUnitsFlags flags, bus_wait_for_units_unit_callback callback, void *userdata); int bus_wait_for_units_run(BusWaitForUnits *d); - -DEFINE_TRIVIAL_CLEANUP_FUNC(BusWaitForUnits*, bus_wait_for_units_free);