From 1c8d653d2b8a82ed3944f03c2dea25ad1e2cc967 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Mon, 30 Jun 2025 17:57:08 +0200 Subject: [PATCH 1/4] core/transaction: do not attempt to log "n/a" as a journal field Follow-up for 3cf6a3a3d4acf8347ccd0250274f517e6b2e9fe6 --- src/core/transaction.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/core/transaction.c b/src/core/transaction.c index 1fe3c950e2..0fc82f6716 100644 --- a/src/core/transaction.c +++ b/src/core/transaction.c @@ -414,7 +414,7 @@ static int transaction_verify_order_one(Transaction *tr, Job *j, Job *from, unsi log_struct(LOG_ERR, LOG_UNIT_MESSAGE(j->unit, "%s", cycle_path_text), LOG_MESSAGE_ID(SD_MESSAGE_UNIT_ORDERING_CYCLE_STR), - LOG_ITEM("%s", strna(unit_ids))); + LOG_ITEM("%s", strempty(unit_ids))); if (delete) { const char *status; @@ -427,7 +427,7 @@ static int transaction_verify_order_one(Transaction *tr, Job *j, Job *from, unsi LOG_MESSAGE_ID(SD_MESSAGE_DELETING_JOB_BECAUSE_ORDERING_CYCLE_STR), LOG_ITEM("DELETED_UNIT=%s", delete->unit->id), LOG_ITEM("DELETED_TYPE=%s", job_type_to_string(delete->type)), - LOG_ITEM("%s", strna(unit_ids))); + LOG_ITEM("%s", strempty(unit_ids))); if (log_get_show_color()) status = ANSI_HIGHLIGHT_RED " SKIP " ANSI_NORMAL; @@ -447,7 +447,7 @@ static int transaction_verify_order_one(Transaction *tr, Job *j, Job *from, unsi LOG_UNIT_MESSAGE(j->unit, "Unable to break cycle starting with %s/%s", j->unit->id, job_type_to_string(j->type)), LOG_MESSAGE_ID(SD_MESSAGE_CANT_BREAK_ORDERING_CYCLE_STR), - LOG_ITEM("%s", strna(unit_ids))); + LOG_ITEM("%s", strempty(unit_ids))); return sd_bus_error_setf(e, BUS_ERROR_TRANSACTION_ORDER_IS_CYCLIC, "Transaction order is cyclic. See system logs for details."); From 1cce8cbf63c82e4eeaa564da31cf9af7bafafd27 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Mon, 30 Jun 2025 17:14:05 +0200 Subject: [PATCH 2/4] core/transaction: inline one iterator var --- src/core/transaction.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/core/transaction.c b/src/core/transaction.c index 0fc82f6716..01ca67553f 100644 --- a/src/core/transaction.c +++ b/src/core/transaction.c @@ -369,8 +369,8 @@ static int transaction_verify_order_one(Transaction *tr, Job *j, Job *from, unsi /* Have we seen this before? */ if (j->generation == generation) { - Job *k, *delete = NULL; _cleanup_free_ char **array = NULL, *unit_ids = NULL; + Job *delete = NULL; /* If the marker is NULL we have been here already and decided the job was loop-free from * here. Hence shortcut things and return right-away. */ @@ -380,7 +380,7 @@ static int transaction_verify_order_one(Transaction *tr, Job *j, Job *from, unsi /* So, the marker is not NULL and we already have been here. We have a cycle. Let's try to * break it. We go backwards in our path and try to find a suitable job to remove. We use the * marker to find our way back, since smart how we are we stored our way back in there. */ - for (k = from; k; k = ((k->generation == generation && k->marker != k) ? k->marker : NULL)) { + for (Job *k = from; k; k = (k->generation == generation && k->marker != k) ? k->marker : NULL) { /* For logging below */ if (strv_push_pair(&array, k->unit->id, (char*) job_type_to_string(k->type)) < 0) From 3aa5e91b1eef8e132cf957caccfe25984c67aecd Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Mon, 30 Jun 2025 17:29:38 +0200 Subject: [PATCH 3/4] core/transaction: use strextendf_with_separator() where appropriate --- src/core/transaction.c | 32 ++++---------------------------- 1 file changed, 4 insertions(+), 28 deletions(-) diff --git a/src/core/transaction.c b/src/core/transaction.c index 01ca67553f..23933936a5 100644 --- a/src/core/transaction.c +++ b/src/core/transaction.c @@ -325,32 +325,6 @@ static bool job_matters_to_anchor(Job *job) { return false; } -static char* merge_unit_ids(const char* unit_log_field, char * const* pairs) { - _cleanup_free_ char *ans = NULL; - size_t size = 0; - - assert(unit_log_field); - - STRV_FOREACH_PAIR(unit_id, job_type, pairs) { - size_t next; - - if (size > 0) - ans[size - 1] = '\n'; - - next = strlen(unit_log_field) + strlen(*unit_id); - if (!GREEDY_REALLOC(ans, size + next + 1)) - return NULL; - - sprintf(ans + size, "%s%s", unit_log_field, *unit_id); - size += next + 1; - } - - if (!ans) - return strdup(""); - - return TAKE_PTR(ans); -} - static int transaction_verify_order_one(Transaction *tr, Job *j, Job *from, unsigned generation, sd_bus_error *e) { static const UnitDependencyAtom directions[] = { @@ -369,7 +343,7 @@ static int transaction_verify_order_one(Transaction *tr, Job *j, Job *from, unsi /* Have we seen this before? */ if (j->generation == generation) { - _cleanup_free_ char **array = NULL, *unit_ids = NULL; + _cleanup_free_ char **array = NULL; Job *delete = NULL; /* If the marker is NULL we have been here already and decided the job was loop-free from @@ -395,7 +369,9 @@ static int transaction_verify_order_one(Transaction *tr, Job *j, Job *from, unsi break; } - unit_ids = merge_unit_ids(unit_log_field(j->unit), array); /* ignore error */ + _cleanup_free_ char *unit_ids = NULL; + STRV_FOREACH_PAIR(unit_id, job_type, array) + (void) strextendf_with_separator(&unit_ids, "\n", "%s%s", unit_log_field(j->unit), *unit_id); size_t m = strv_length(array); From 6912eb315fabe0bbf25593ab897265fa79a7e24b Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Mon, 30 Jun 2025 17:37:58 +0200 Subject: [PATCH 4/4] core/transaction: be explicit about cyclic ordering relationship Inspired by https://github.com/systemd/systemd/pull/35417#discussion_r1867868221 --- src/core/transaction.c | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/src/core/transaction.c b/src/core/transaction.c index 23933936a5..e57907766a 100644 --- a/src/core/transaction.c +++ b/src/core/transaction.c @@ -373,17 +373,15 @@ static int transaction_verify_order_one(Transaction *tr, Job *j, Job *from, unsi STRV_FOREACH_PAIR(unit_id, job_type, array) (void) strextendf_with_separator(&unit_ids, "\n", "%s%s", unit_log_field(j->unit), *unit_id); - size_t m = strv_length(array); - _cleanup_free_ char *cycle_path_text = strdup("Found ordering cycle"); - if (m > 0) { - (void) strextendf(&cycle_path_text, " on %s/%s", array[0], array[1]); - if (m > 2) - (void) strextendf(&cycle_path_text, "; has dependency on %s/%s", array[2], array[3]); - } + if (!strv_isempty(array)) { + (void) strextendf(&cycle_path_text, ": %s/%s", array[0], array[1]); - STRV_FOREACH_PAIR(unit_id, job_type, strv_skip(array, 4)) - (void) strextendf(&cycle_path_text, ", %s/%s", *unit_id, *job_type); + STRV_FOREACH_PAIR(unit_id, job_type, strv_skip(array, 2)) + (void) strextendf(&cycle_path_text, " after %s/%s", *unit_id, *job_type); + + (void) strextendf(&cycle_path_text, " - after %s", array[0]); + } /* logging for j not k here to provide a consistent narrative */ if (cycle_path_text)