From b0904249b945d790d077c02acf23567889db2fe1 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 20 Jun 2023 12:19:03 +0200 Subject: [PATCH 1/4] transaction: turn four bool parameters into a proper flags parameters --- src/core/manager.c | 21 +++++++++++++++----- src/core/transaction.c | 44 +++++++++++++++++++++++------------------- src/core/transaction.h | 20 ++++++++++++++----- 3 files changed, 55 insertions(+), 30 deletions(-) diff --git a/src/core/manager.c b/src/core/manager.c index 1df48d3cbc..34a6d7db2c 100644 --- a/src/core/manager.c +++ b/src/core/manager.c @@ -2052,9 +2052,15 @@ int manager_add_job( if (!tr) return -ENOMEM; - r = transaction_add_job_and_dependencies(tr, type, unit, NULL, true, false, - IN_SET(mode, JOB_IGNORE_DEPENDENCIES, JOB_IGNORE_REQUIREMENTS), - mode == JOB_IGNORE_DEPENDENCIES, error); + r = transaction_add_job_and_dependencies( + tr, + type, + unit, + /* by= */ NULL, + TRANSACTION_MATTERS | + (IN_SET(mode, JOB_IGNORE_DEPENDENCIES, JOB_IGNORE_REQUIREMENTS) ? TRANSACTION_IGNORE_REQUIREMENTS : 0) | + (mode == JOB_IGNORE_DEPENDENCIES ? TRANSACTION_IGNORE_ORDER : 0), + error); if (r < 0) return r; @@ -2132,12 +2138,17 @@ int manager_propagate_reload(Manager *m, Unit *unit, JobMode mode, sd_bus_error return -ENOMEM; /* We need an anchor job */ - r = transaction_add_job_and_dependencies(tr, JOB_NOP, unit, NULL, false, false, true, true, e); + r = transaction_add_job_and_dependencies(tr, JOB_NOP, unit, NULL, TRANSACTION_IGNORE_REQUIREMENTS|TRANSACTION_IGNORE_ORDER, e); if (r < 0) return r; /* Failure in adding individual dependencies is ignored, so this always succeeds. */ - transaction_add_propagate_reload_jobs(tr, unit, tr->anchor_job, mode == JOB_IGNORE_DEPENDENCIES, e); + transaction_add_propagate_reload_jobs( + tr, + unit, + tr->anchor_job, + mode == JOB_IGNORE_DEPENDENCIES ? TRANSACTION_IGNORE_ORDER : 0, + e); r = transaction_activate(tr, m, mode, NULL, e); if (r < 0) diff --git a/src/core/transaction.c b/src/core/transaction.c index 39f8f6c96d..bb38741ec3 100644 --- a/src/core/transaction.c +++ b/src/core/transaction.c @@ -874,7 +874,13 @@ static void transaction_unlink_job(Transaction *tr, Job *j, bool delete_dependen } } -void transaction_add_propagate_reload_jobs(Transaction *tr, Unit *unit, Job *by, bool ignore_order, sd_bus_error *e) { +void transaction_add_propagate_reload_jobs( + Transaction *tr, + Unit *unit, + Job *by, + TransactionAddFlags flags, + sd_bus_error *e) { + JobType nt; Unit *dep; int r; @@ -887,7 +893,7 @@ void transaction_add_propagate_reload_jobs(Transaction *tr, Unit *unit, Job *by, if (nt == JOB_NOP) continue; - r = transaction_add_job_and_dependencies(tr, nt, dep, by, false, false, false, ignore_order, e); + r = transaction_add_job_and_dependencies(tr, nt, dep, by, flags, e); if (r < 0) { log_unit_warning(dep, "Cannot add dependency reload job, ignoring: %s", @@ -902,10 +908,7 @@ int transaction_add_job_and_dependencies( JobType type, Unit *unit, Job *by, - bool matters, - bool conflicts, - bool ignore_requirements, - bool ignore_order, + TransactionAddFlags flags, sd_bus_error *e) { bool is_new; @@ -969,11 +972,12 @@ int transaction_add_job_and_dependencies( if (!ret) return -ENOMEM; - ret->ignore_order = ret->ignore_order || ignore_order; + if (FLAGS_SET(flags, TRANSACTION_IGNORE_ORDER)) + ret->ignore_order = true; /* Then, add a link to the job. */ if (by) { - if (!job_dependency_new(by, ret, matters, conflicts)) + if (!job_dependency_new(by, ret, FLAGS_SET(flags, TRANSACTION_MATTERS), FLAGS_SET(flags, TRANSACTION_CONFLICTS))) return -ENOMEM; } else { /* If the job has no parent job, it is the anchor job. */ @@ -981,14 +985,14 @@ int transaction_add_job_and_dependencies( tr->anchor_job = ret; } - if (is_new && !ignore_requirements && type != JOB_NOP) { + if (is_new && !FLAGS_SET(flags, TRANSACTION_IGNORE_REQUIREMENTS) && type != JOB_NOP) { _cleanup_set_free_ Set *following = NULL; /* If we are following some other unit, make sure we * add all dependencies of everybody following. */ if (unit_following_set(ret->unit, &following) > 0) SET_FOREACH(dep, following) { - r = transaction_add_job_and_dependencies(tr, type, dep, ret, false, false, false, ignore_order, e); + r = transaction_add_job_and_dependencies(tr, type, dep, ret, flags & TRANSACTION_IGNORE_ORDER, e); if (r < 0) { log_unit_full_errno(dep, r == -ERFKILL ? LOG_INFO : LOG_WARNING, r, "Cannot add dependency job, ignoring: %s", @@ -1000,7 +1004,7 @@ int transaction_add_job_and_dependencies( /* Finally, recursively add in all dependencies. */ if (IN_SET(type, JOB_START, JOB_RESTART)) { UNIT_FOREACH_DEPENDENCY(dep, ret->unit, UNIT_ATOM_PULL_IN_START) { - r = transaction_add_job_and_dependencies(tr, JOB_START, dep, ret, true, false, false, ignore_order, e); + r = transaction_add_job_and_dependencies(tr, JOB_START, dep, ret, TRANSACTION_MATTERS | (flags & TRANSACTION_IGNORE_ORDER), e); if (r < 0) { if (r != -EBADR) /* job type not applicable */ goto fail; @@ -1010,7 +1014,7 @@ int transaction_add_job_and_dependencies( } UNIT_FOREACH_DEPENDENCY(dep, ret->unit, UNIT_ATOM_PULL_IN_START_IGNORED) { - r = transaction_add_job_and_dependencies(tr, JOB_START, dep, ret, false, false, false, ignore_order, e); + r = transaction_add_job_and_dependencies(tr, JOB_START, dep, ret, flags & TRANSACTION_IGNORE_ORDER, e); if (r < 0) { /* unit masked, job type not applicable and unit not found are not considered as errors. */ log_unit_full_errno(dep, @@ -1022,7 +1026,7 @@ int transaction_add_job_and_dependencies( } UNIT_FOREACH_DEPENDENCY(dep, ret->unit, UNIT_ATOM_PULL_IN_VERIFY) { - r = transaction_add_job_and_dependencies(tr, JOB_VERIFY_ACTIVE, dep, ret, true, false, false, ignore_order, e); + r = transaction_add_job_and_dependencies(tr, JOB_VERIFY_ACTIVE, dep, ret, TRANSACTION_MATTERS | (flags & TRANSACTION_IGNORE_ORDER), e); if (r < 0) { if (r != -EBADR) /* job type not applicable */ goto fail; @@ -1032,7 +1036,7 @@ int transaction_add_job_and_dependencies( } UNIT_FOREACH_DEPENDENCY(dep, ret->unit, UNIT_ATOM_PULL_IN_STOP) { - r = transaction_add_job_and_dependencies(tr, JOB_STOP, dep, ret, true, true, false, ignore_order, e); + r = transaction_add_job_and_dependencies(tr, JOB_STOP, dep, ret, TRANSACTION_MATTERS | TRANSACTION_CONFLICTS | (flags & TRANSACTION_IGNORE_ORDER), e); if (r < 0) { if (r != -EBADR) /* job type not applicable */ goto fail; @@ -1042,7 +1046,7 @@ int transaction_add_job_and_dependencies( } UNIT_FOREACH_DEPENDENCY(dep, ret->unit, UNIT_ATOM_PULL_IN_STOP_IGNORED) { - r = transaction_add_job_and_dependencies(tr, JOB_STOP, dep, ret, false, false, false, ignore_order, e); + r = transaction_add_job_and_dependencies(tr, JOB_STOP, dep, ret, flags & TRANSACTION_IGNORE_ORDER, e); if (r < 0) { log_unit_warning(dep, "Cannot add dependency job, ignoring: %s", @@ -1069,7 +1073,7 @@ int transaction_add_job_and_dependencies( if (nt == JOB_NOP) continue; - r = transaction_add_job_and_dependencies(tr, nt, dep, ret, true, false, false, ignore_order, e); + r = transaction_add_job_and_dependencies(tr, nt, dep, ret, TRANSACTION_MATTERS | (flags & TRANSACTION_IGNORE_ORDER), e); if (r < 0) { if (r != -EBADR) /* job type not applicable */ return r; @@ -1085,7 +1089,7 @@ int transaction_add_job_and_dependencies( if (set_contains(propagated_restart, dep)) continue; - r = transaction_add_job_and_dependencies(tr, JOB_STOP, dep, ret, true, false, false, ignore_order, e); + r = transaction_add_job_and_dependencies(tr, JOB_STOP, dep, ret, TRANSACTION_MATTERS | (flags & TRANSACTION_IGNORE_ORDER), e); if (r < 0) { if (r != -EBADR) /* job type not applicable */ return r; @@ -1096,7 +1100,7 @@ int transaction_add_job_and_dependencies( } if (type == JOB_RELOAD) - transaction_add_propagate_reload_jobs(tr, ret->unit, ret, ignore_order, e); + transaction_add_propagate_reload_jobs(tr, ret->unit, ret, flags & TRANSACTION_IGNORE_ORDER, e); /* JOB_VERIFY_ACTIVE requires no dependency handling */ } @@ -1157,7 +1161,7 @@ int transaction_add_isolate_jobs(Transaction *tr, Manager *m) { if (keep) continue; - r = transaction_add_job_and_dependencies(tr, JOB_STOP, u, tr->anchor_job, true, false, false, false, NULL); + r = transaction_add_job_and_dependencies(tr, JOB_STOP, u, tr->anchor_job, TRANSACTION_MATTERS, NULL); if (r < 0) log_unit_warning_errno(u, r, "Cannot add isolate job, ignoring: %m"); } @@ -1182,7 +1186,7 @@ int transaction_add_triggering_jobs(Transaction *tr, Unit *u) { if (hashmap_contains(tr->jobs, trigger)) continue; - r = transaction_add_job_and_dependencies(tr, JOB_STOP, trigger, tr->anchor_job, true, false, false, false, NULL); + r = transaction_add_job_and_dependencies(tr, JOB_STOP, trigger, tr->anchor_job, TRANSACTION_MATTERS, NULL); if (r < 0) log_unit_warning_errno(u, r, "Cannot add triggered by job, ignoring: %m"); } diff --git a/src/core/transaction.h b/src/core/transaction.h index b42d434dd1..6ee0872c27 100644 --- a/src/core/transaction.h +++ b/src/core/transaction.h @@ -20,17 +20,27 @@ Transaction *transaction_free(Transaction *tr); Transaction *transaction_abort_and_free(Transaction *tr); DEFINE_TRIVIAL_CLEANUP_FUNC(Transaction*, transaction_abort_and_free); -void transaction_add_propagate_reload_jobs(Transaction *tr, Unit *unit, Job *by, bool ignore_order, sd_bus_error *e); +typedef enum TransactionAddFlags { + TRANSACTION_MATTERS = 1 << 0, + TRANSACTION_CONFLICTS = 1 << 1, + TRANSACTION_IGNORE_REQUIREMENTS = 1 << 2, + TRANSACTION_IGNORE_ORDER = 1 << 3, +} TransactionAddFlags; + +void transaction_add_propagate_reload_jobs( + Transaction *tr, + Unit *unit, Job *by, + TransactionAddFlags flags, + sd_bus_error *e); + int transaction_add_job_and_dependencies( Transaction *tr, JobType type, Unit *unit, Job *by, - bool matters, - bool conflicts, - bool ignore_requirements, - bool ignore_order, + TransactionAddFlags flags, sd_bus_error *e); + int transaction_activate(Transaction *tr, Manager *m, JobMode mode, Set *affected, sd_bus_error *e); int transaction_add_isolate_jobs(Transaction *tr, Manager *m); int transaction_add_triggering_jobs(Transaction *tr, Unit *u); From fab7e5e8f6bd514a9dcbbd3fb12aa73b106bf92c Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 20 Jun 2023 12:20:38 +0200 Subject: [PATCH 2/4] transaction: use more verbose dbus error data in log message where available --- src/core/transaction.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/core/transaction.c b/src/core/transaction.c index bb38741ec3..c2f3ea1062 100644 --- a/src/core/transaction.c +++ b/src/core/transaction.c @@ -1138,6 +1138,7 @@ int transaction_add_isolate_jobs(Transaction *tr, Manager *m) { assert(m); HASHMAP_FOREACH_KEY(u, k, m->units) { + _cleanup_(sd_bus_error_free) sd_bus_error e = SD_BUS_ERROR_NULL; Unit *o; /* Ignore aliases */ @@ -1161,9 +1162,9 @@ int transaction_add_isolate_jobs(Transaction *tr, Manager *m) { if (keep) continue; - r = transaction_add_job_and_dependencies(tr, JOB_STOP, u, tr->anchor_job, TRANSACTION_MATTERS, NULL); + r = transaction_add_job_and_dependencies(tr, JOB_STOP, u, tr->anchor_job, TRANSACTION_MATTERS, &e); if (r < 0) - log_unit_warning_errno(u, r, "Cannot add isolate job, ignoring: %m"); + log_unit_warning_errno(u, r, "Cannot add isolate job, ignoring: %s", bus_error_message(&e, r)); } return 0; @@ -1177,6 +1178,7 @@ int transaction_add_triggering_jobs(Transaction *tr, Unit *u) { assert(u); UNIT_FOREACH_DEPENDENCY(trigger, u, UNIT_ATOM_TRIGGERED_BY) { + _cleanup_(sd_bus_error_free) sd_bus_error e = SD_BUS_ERROR_NULL; /* No need to stop inactive jobs */ if (UNIT_IS_INACTIVE_OR_FAILED(unit_active_state(trigger)) && !trigger->job) @@ -1186,9 +1188,9 @@ int transaction_add_triggering_jobs(Transaction *tr, Unit *u) { if (hashmap_contains(tr->jobs, trigger)) continue; - r = transaction_add_job_and_dependencies(tr, JOB_STOP, trigger, tr->anchor_job, TRANSACTION_MATTERS, NULL); + r = transaction_add_job_and_dependencies(tr, JOB_STOP, trigger, tr->anchor_job, TRANSACTION_MATTERS, &e); if (r < 0) - log_unit_warning_errno(u, r, "Cannot add triggered by job, ignoring: %m"); + log_unit_warning_errno(u, r, "Cannot add triggered by job, ignoring: %s", bus_error_message(&e, r)); } return 0; From c25eeb65c0f75d5892a057e6d843d7cd9a983502 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 20 Jun 2023 12:19:29 +0200 Subject: [PATCH 3/4] transaction: rebreak comments to match current coding style --- src/core/transaction.c | 35 +++++++++++++++++------------------ 1 file changed, 17 insertions(+), 18 deletions(-) diff --git a/src/core/transaction.c b/src/core/transaction.c index c2f3ea1062..dda17c6c16 100644 --- a/src/core/transaction.c +++ b/src/core/transaction.c @@ -921,10 +921,10 @@ int transaction_add_job_and_dependencies( assert(type < _JOB_TYPE_MAX_IN_TRANSACTION); assert(unit); - /* Before adding jobs for this unit, let's ensure that its state has been loaded - * This matters when jobs are spawned as part of coldplugging itself (see e. g. path_coldplug()). - * This way, we "recursively" coldplug units, ensuring that we do not look at state of - * not-yet-coldplugged units. */ + /* Before adding jobs for this unit, let's ensure that its state has been loaded This matters when + * jobs are spawned as part of coldplugging itself (see e. g. path_coldplug()). This way, we + * "recursively" coldplug units, ensuring that we do not look at state of not-yet-coldplugged + * units. */ if (MANAGER_IS_RELOADING(unit->manager)) unit_coldplug(unit); @@ -938,15 +938,13 @@ int transaction_add_job_and_dependencies( if (type != JOB_STOP) { r = bus_unit_validate_load_state(unit, e); - /* The time-based cache allows to start new units without daemon-reload, - * but if they are already referenced (because of dependencies or ordering) - * then we have to force a load of the fragment. As an optimization, check - * first if anything in the usual paths was modified since the last time - * the cache was loaded. Also check if the last time an attempt to load the - * unit was made was before the most recent cache refresh, so that we know - * we need to try again — even if the cache is current, it might have been - * updated in a different context before we had a chance to retry loading - * this particular unit. + /* The time-based cache allows to start new units without daemon-reload, but if they are + * already referenced (because of dependencies or ordering) then we have to force a load of + * the fragment. As an optimization, check first if anything in the usual paths was modified + * since the last time the cache was loaded. Also check if the last time an attempt to load + * the unit was made was before the most recent cache refresh, so that we know we need to try + * again — even if the cache is current, it might have been updated in a different context + * before we had a chance to retry loading this particular unit. * * Given building up the transaction is a synchronous operation, attempt * to load the unit immediately. */ @@ -988,8 +986,8 @@ int transaction_add_job_and_dependencies( if (is_new && !FLAGS_SET(flags, TRANSACTION_IGNORE_REQUIREMENTS) && type != JOB_NOP) { _cleanup_set_free_ Set *following = NULL; - /* If we are following some other unit, make sure we - * add all dependencies of everybody following. */ + /* If we are following some other unit, make sure we add all dependencies of everybody + * following. */ if (unit_following_set(ret->unit, &following) > 0) SET_FOREACH(dep, following) { r = transaction_add_job_and_dependencies(tr, type, dep, ret, flags & TRANSACTION_IGNORE_ORDER, e); @@ -1016,7 +1014,8 @@ int transaction_add_job_and_dependencies( UNIT_FOREACH_DEPENDENCY(dep, ret->unit, UNIT_ATOM_PULL_IN_START_IGNORED) { r = transaction_add_job_and_dependencies(tr, JOB_START, dep, ret, flags & TRANSACTION_IGNORE_ORDER, e); if (r < 0) { - /* unit masked, job type not applicable and unit not found are not considered as errors. */ + /* unit masked, job type not applicable and unit not found are not + * considered as errors. */ log_unit_full_errno(dep, IN_SET(r, -ERFKILL, -EBADR, -ENOENT) ? LOG_DEBUG : LOG_WARNING, r, "Cannot add dependency job, ignoring: %s", @@ -1082,8 +1081,8 @@ int transaction_add_job_and_dependencies( } } - /* The 'stop' part of a restart job is also propagated to - * units with UNIT_ATOM_PROPAGATE_STOP */ + /* The 'stop' part of a restart job is also propagated to units with + * UNIT_ATOM_PROPAGATE_STOP */ UNIT_FOREACH_DEPENDENCY(dep, ret->unit, UNIT_ATOM_PROPAGATE_STOP) { /* Units experienced restart propagation are skipped */ if (set_contains(propagated_restart, dep)) From 3044510d58f6b9bcc287e701e7a3236c47e72bd5 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 20 Jun 2023 13:07:53 +0200 Subject: [PATCH 4/4] transaction: drop bus error arg from transaction_add_propagate_reload_jobs() We erase the error anyway always, hence no point in even passing anything in from callers. --- src/core/manager.c | 3 +-- src/core/transaction.c | 15 +++++++-------- src/core/transaction.h | 3 +-- 3 files changed, 9 insertions(+), 12 deletions(-) diff --git a/src/core/manager.c b/src/core/manager.c index 34a6d7db2c..23df5ce191 100644 --- a/src/core/manager.c +++ b/src/core/manager.c @@ -2147,8 +2147,7 @@ int manager_propagate_reload(Manager *m, Unit *unit, JobMode mode, sd_bus_error tr, unit, tr->anchor_job, - mode == JOB_IGNORE_DEPENDENCIES ? TRANSACTION_IGNORE_ORDER : 0, - e); + mode == JOB_IGNORE_DEPENDENCIES ? TRANSACTION_IGNORE_ORDER : 0); r = transaction_activate(tr, m, mode, NULL, e); if (r < 0) diff --git a/src/core/transaction.c b/src/core/transaction.c index dda17c6c16..9c1523c47a 100644 --- a/src/core/transaction.c +++ b/src/core/transaction.c @@ -878,8 +878,7 @@ void transaction_add_propagate_reload_jobs( Transaction *tr, Unit *unit, Job *by, - TransactionAddFlags flags, - sd_bus_error *e) { + TransactionAddFlags flags) { JobType nt; Unit *dep; @@ -889,17 +888,17 @@ void transaction_add_propagate_reload_jobs( assert(unit); UNIT_FOREACH_DEPENDENCY(dep, unit, UNIT_ATOM_PROPAGATES_RELOAD_TO) { + _cleanup_(sd_bus_error_free) sd_bus_error e = SD_BUS_ERROR_NULL; + nt = job_type_collapse(JOB_TRY_RELOAD, dep); if (nt == JOB_NOP) continue; - r = transaction_add_job_and_dependencies(tr, nt, dep, by, flags, e); - if (r < 0) { + r = transaction_add_job_and_dependencies(tr, nt, dep, by, flags, &e); + if (r < 0) log_unit_warning(dep, "Cannot add dependency reload job, ignoring: %s", - bus_error_message(e, r)); - sd_bus_error_free(e); - } + bus_error_message(&e, r)); } } @@ -1099,7 +1098,7 @@ int transaction_add_job_and_dependencies( } if (type == JOB_RELOAD) - transaction_add_propagate_reload_jobs(tr, ret->unit, ret, flags & TRANSACTION_IGNORE_ORDER, e); + transaction_add_propagate_reload_jobs(tr, ret->unit, ret, flags & TRANSACTION_IGNORE_ORDER); /* JOB_VERIFY_ACTIVE requires no dependency handling */ } diff --git a/src/core/transaction.h b/src/core/transaction.h index 6ee0872c27..5874077aef 100644 --- a/src/core/transaction.h +++ b/src/core/transaction.h @@ -30,8 +30,7 @@ typedef enum TransactionAddFlags { void transaction_add_propagate_reload_jobs( Transaction *tr, Unit *unit, Job *by, - TransactionAddFlags flags, - sd_bus_error *e); + TransactionAddFlags flags); int transaction_add_job_and_dependencies( Transaction *tr,