From 728ba51e98959dfd9ad9f70cf6526b0a84b529a0 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 10 Dec 2018 18:52:11 +0100 Subject: [PATCH 1/7] job: update job_free() to follow our usual return-NULL style --- src/core/job.c | 4 ++-- src/core/job.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/core/job.c b/src/core/job.c index af5070b8cf..0f224f2691 100644 --- a/src/core/job.c +++ b/src/core/job.c @@ -89,7 +89,7 @@ void job_unlink(Job *j) { j->timer_event_source = sd_event_source_unref(j->timer_event_source); } -void job_free(Job *j) { +Job* job_free(Job *j) { assert(j); assert(!j->installed); assert(!j->transaction_prev); @@ -102,7 +102,7 @@ void job_free(Job *j) { sd_bus_track_unref(j->bus_track); strv_free(j->deserialized_clients); - free(j); + return mfree(j); } static void job_set_state(Job *j, JobState state) { diff --git a/src/core/job.h b/src/core/job.h index 5f9c14012f..1f5bce85a4 100644 --- a/src/core/job.h +++ b/src/core/job.h @@ -162,7 +162,7 @@ struct Job { Job* job_new(Unit *unit, JobType type); Job* job_new_raw(Unit *unit); void job_unlink(Job *job); -void job_free(Job *job); +Job* job_free(Job *job); Job* job_install(Job *j); int job_install_deserialized(Job *j); void job_uninstall(Job *j); From 4a53080be637f6dad91515fc896d9f77fbc17f0c Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 10 Dec 2018 18:52:28 +0100 Subject: [PATCH 2/7] core: don't track jobs-finishing-during-reload explicitly Memory management is borked for this, and moreover this is unnecessary since f0831ed2a03, i.e. since coldplug() and catchup() are two different concepts: the former restoring the state from before a reload, the latter than adjusting it again to the actual status in effect after the reload. Fixes: #10716 Mostly reverts: #8803 --- src/core/job.c | 20 +------------------- src/core/job.h | 1 - src/core/manager.c | 14 -------------- src/core/manager.h | 3 --- 4 files changed, 1 insertion(+), 37 deletions(-) diff --git a/src/core/job.c b/src/core/job.c index 0f224f2691..7fa3673474 100644 --- a/src/core/job.c +++ b/src/core/job.c @@ -260,7 +260,6 @@ int job_install_deserialized(Job *j) { *pj = j; j->installed = true; - j->reloaded = true; if (j->state == JOB_RUNNING) j->unit->manager->n_running_jobs++; @@ -968,19 +967,6 @@ static void job_fail_dependencies(Unit *u, UnitDependency d) { } } -static int job_save_pending_finished_job(Job *j) { - int r; - - assert(j); - - r = set_ensure_allocated(&j->manager->pending_finished_jobs, NULL); - if (r < 0) - return r; - - job_unlink(j); - return set_put(j->manager->pending_finished_jobs, j); -} - int job_finish_and_invalidate(Job *j, JobResult result, bool recursive, bool already) { Unit *u; Unit *other; @@ -1020,11 +1006,7 @@ int job_finish_and_invalidate(Job *j, JobResult result, bool recursive, bool alr j->manager->n_failed_jobs++; job_uninstall(j); - /* Keep jobs started before the reload to send singal later, free all others */ - if (!MANAGER_IS_RELOADING(j->manager) || - !j->reloaded || - job_save_pending_finished_job(j) < 0) - job_free(j); + job_free(j); /* Fail depending jobs on failure */ if (result != JOB_DONE && recursive) { diff --git a/src/core/job.h b/src/core/job.h index 1f5bce85a4..649f812a57 100644 --- a/src/core/job.h +++ b/src/core/job.h @@ -156,7 +156,6 @@ struct Job { bool irreversible:1; bool in_gc_queue:1; bool ref_by_private_bus:1; - bool reloaded:1; }; Job* job_new(Unit *unit, JobType type); diff --git a/src/core/manager.c b/src/core/manager.c index 2398dcf4ea..35d9753b12 100644 --- a/src/core/manager.c +++ b/src/core/manager.c @@ -3464,17 +3464,6 @@ int manager_deserialize(Manager *m, FILE *f, FDSet *fds) { return manager_deserialize_units(m, f, fds); } -static void manager_flush_finished_jobs(Manager *m) { - Job *j; - - while ((j = set_steal_first(m->pending_finished_jobs))) { - bus_job_send_removed_signal(j); - job_free(j); - } - - m->pending_finished_jobs = set_free(m->pending_finished_jobs); -} - int manager_reload(Manager *m) { _cleanup_(manager_reloading_stopp) Manager *reloading = NULL; _cleanup_fdset_free_ FDSet *fds = NULL; @@ -3560,9 +3549,6 @@ int manager_reload(Manager *m) { manager_ready(m); - if (!MANAGER_IS_RELOADING(m)) - manager_flush_finished_jobs(m); - m->send_reloading_done = true; return 0; } diff --git a/src/core/manager.h b/src/core/manager.h index f3ad44d26e..bce8020cfd 100644 --- a/src/core/manager.h +++ b/src/core/manager.h @@ -337,9 +337,6 @@ struct Manager { /* non-zero if we are reloading or reexecuting, */ int n_reloading; - /* A set which contains all jobs that started before reload and finished - * during it */ - Set *pending_finished_jobs; unsigned n_installed_jobs; unsigned n_failed_jobs; From 48235ad6b7ef168e029d48701a08b62b616bcdd2 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 10 Dec 2018 19:38:38 +0100 Subject: [PATCH 3/7] job: be more careful when removing job object from jobs hash table Let's validate that the ID is actually allocated to us before remove a job. This is relevant as various bits of code will call job_free() on partially set up Job objects, and we really shouldn't remove another job object accidentally from the hash table, when the set up didn't complete. --- src/core/job.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/job.c b/src/core/job.c index 7fa3673474..8106a78e66 100644 --- a/src/core/job.c +++ b/src/core/job.c @@ -151,7 +151,7 @@ void job_uninstall(Job *j) { unit_add_to_gc_queue(j->unit); - hashmap_remove(j->manager->jobs, UINT32_TO_PTR(j->id)); + hashmap_remove_value(j->manager->jobs, UINT32_TO_PTR(j->id), j); j->installed = false; } From b17c9620c87a1d8117da72b30a2e096df02a2bb5 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 10 Dec 2018 19:40:37 +0100 Subject: [PATCH 4/7] core: rework how we deserialize jobs Let's add a helper call unit_deserialize_job() for this purpose, and let's move registration in the global jobs hash table into job_install_deserialized() so that it it is done after all superficial checks are done, and before transitioning into installed states, so that rollback code is not necessary anymore. --- src/core/job.c | 16 ++++++++++----- src/core/job.h | 2 ++ src/core/unit.c | 52 +++++++++++++++++++++++++------------------------ 3 files changed, 40 insertions(+), 30 deletions(-) diff --git a/src/core/job.c b/src/core/job.c index 8106a78e66..f635b7e933 100644 --- a/src/core/job.c +++ b/src/core/job.c @@ -244,19 +244,25 @@ Job* job_install(Job *j) { int job_install_deserialized(Job *j) { Job **pj; + int r; assert(!j->installed); if (j->type < 0 || j->type >= _JOB_TYPE_MAX_IN_TRANSACTION) - return log_debug_errno(SYNTHETIC_ERRNO(EINVAL), + return log_unit_debug_errno(j->unit, SYNTHETIC_ERRNO(EINVAL), "Invalid job type %s in deserialization.", strna(job_type_to_string(j->type))); pj = (j->type == JOB_NOP) ? &j->unit->nop_job : &j->unit->job; - if (*pj) { - log_unit_debug(j->unit, "Unit already has a job installed. Not installing deserialized job."); - return -EEXIST; - } + if (*pj) + return log_unit_debug_errno(j->unit, SYNTHETIC_ERRNO(EEXIST), + "Unit already has a job installed. Not installing deserialized job."); + + r = hashmap_put(j->manager->jobs, UINT32_TO_PTR(j->id), j); + if (r == -EEXIST) + return log_unit_debug_errno(j->unit, r, "Job ID %" PRIu32 " already used, cannot deserialize job.", j->id); + if (r < 0) + return log_unit_debug_errno(j->unit, r, "Failed to insert job into jobs hash table: %m"); *pj = j; j->installed = true; diff --git a/src/core/job.h b/src/core/job.h index 649f812a57..1b9bcdd895 100644 --- a/src/core/job.h +++ b/src/core/job.h @@ -222,6 +222,8 @@ void job_add_to_gc_queue(Job *j); int job_get_before(Job *j, Job*** ret); int job_get_after(Job *j, Job*** ret); +DEFINE_TRIVIAL_CLEANUP_FUNC(Job*, job_free); + const char* job_type_to_string(JobType t) _const_; JobType job_type_from_string(const char *s) _pure_; diff --git a/src/core/unit.c b/src/core/unit.c index a9303a0f3f..ac8f403e0a 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -3288,6 +3288,29 @@ int unit_serialize(Unit *u, FILE *f, FDSet *fds, bool serialize_jobs) { return 0; } +static int unit_deserialize_job(Unit *u, FILE *f) { + _cleanup_(job_freep) Job *j = NULL; + int r; + + assert(u); + assert(f); + + j = job_new_raw(u); + if (!j) + return log_oom(); + + r = job_deserialize(j, f); + if (r < 0) + return r; + + r = job_install_deserialized(j); + if (r < 0) + return r; + + TAKE_PTR(j); + return 0; +} + int unit_deserialize(Unit *u, FILE *f, FDSet *fds) { int r; @@ -3321,32 +3344,11 @@ int unit_deserialize(Unit *u, FILE *f, FDSet *fds) { if (streq(l, "job")) { if (v[0] == '\0') { - /* new-style serialized job */ - Job *j; - - j = job_new_raw(u); - if (!j) - return log_oom(); - - r = job_deserialize(j, f); - if (r < 0) { - job_free(j); + /* New-style serialized job */ + r = unit_deserialize_job(u, f); + if (r < 0) return r; - } - - r = hashmap_put(u->manager->jobs, UINT32_TO_PTR(j->id), j); - if (r < 0) { - job_free(j); - return r; - } - - r = job_install_deserialized(j); - if (r < 0) { - hashmap_remove(u->manager->jobs, UINT32_TO_PTR(j->id)); - job_free(j); - return r; - } - } else /* legacy for pre-44 */ + } else /* Legacy for pre-44 */ log_unit_warning(u, "Update from too old systemd versions are unsupported, cannot deserialize job: %s", v); continue; } else if (streq(l, "state-change-timestamp")) { From 16c74914d233ec93012d77e5f93cf90e42939669 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 10 Dec 2018 20:56:57 +0100 Subject: [PATCH 5/7] core: split out all logic that updates a Job on a unit's unit_notify() invocation Just some refactoring, no change in behaviour. --- src/core/unit.c | 136 +++++++++++++++++++++++++----------------------- 1 file changed, 71 insertions(+), 65 deletions(-) diff --git a/src/core/unit.c b/src/core/unit.c index ac8f403e0a..5568ccd7b2 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -2328,6 +2328,73 @@ static void unit_emit_audit_stop(Unit *u, UnitActiveState state) { } } +static bool unit_process_job(Job *j, UnitActiveState ns, UnitNotifyFlags flags) { + bool unexpected = false; + + assert(j); + + if (j->state == JOB_WAITING) + + /* So we reached a different state for this job. Let's see if we can run it now if it failed previously + * due to EAGAIN. */ + job_add_to_run_queue(j); + + /* Let's check whether the unit's new state constitutes a finished job, or maybe contradicts a running job and + * hence needs to invalidate jobs. */ + + switch (j->type) { + + case JOB_START: + case JOB_VERIFY_ACTIVE: + + if (UNIT_IS_ACTIVE_OR_RELOADING(ns)) + job_finish_and_invalidate(j, JOB_DONE, true, false); + else if (j->state == JOB_RUNNING && ns != UNIT_ACTIVATING) { + unexpected = true; + + if (UNIT_IS_INACTIVE_OR_FAILED(ns)) + job_finish_and_invalidate(j, ns == UNIT_FAILED ? JOB_FAILED : JOB_DONE, true, false); + } + + break; + + case JOB_RELOAD: + case JOB_RELOAD_OR_START: + case JOB_TRY_RELOAD: + + if (j->state == JOB_RUNNING) { + if (ns == UNIT_ACTIVE) + job_finish_and_invalidate(j, (flags & UNIT_NOTIFY_RELOAD_FAILURE) ? JOB_FAILED : JOB_DONE, true, false); + else if (!IN_SET(ns, UNIT_ACTIVATING, UNIT_RELOADING)) { + unexpected = true; + + if (UNIT_IS_INACTIVE_OR_FAILED(ns)) + job_finish_and_invalidate(j, ns == UNIT_FAILED ? JOB_FAILED : JOB_DONE, true, false); + } + } + + break; + + case JOB_STOP: + case JOB_RESTART: + case JOB_TRY_RESTART: + + if (UNIT_IS_INACTIVE_OR_FAILED(ns)) + job_finish_and_invalidate(j, JOB_DONE, true, false); + else if (j->state == JOB_RUNNING && ns != UNIT_DEACTIVATING) { + unexpected = true; + job_finish_and_invalidate(j, JOB_FAILED, true, false); + } + + break; + + default: + assert_not_reached("Job type unknown"); + } + + return unexpected; +} + void unit_notify(Unit *u, UnitActiveState os, UnitActiveState ns, UnitNotifyFlags flags) { bool unexpected; const char *reason; @@ -2373,71 +2440,10 @@ void unit_notify(Unit *u, UnitActiveState os, UnitActiveState ns, UnitNotifyFlag unit_update_on_console(u); - if (u->job) { - unexpected = false; - - if (u->job->state == JOB_WAITING) - - /* So we reached a different state for this - * job. Let's see if we can run it now if it - * failed previously due to EAGAIN. */ - job_add_to_run_queue(u->job); - - /* Let's check whether this state change constitutes a - * finished job, or maybe contradicts a running job and - * hence needs to invalidate jobs. */ - - switch (u->job->type) { - - case JOB_START: - case JOB_VERIFY_ACTIVE: - - if (UNIT_IS_ACTIVE_OR_RELOADING(ns)) - job_finish_and_invalidate(u->job, JOB_DONE, true, false); - else if (u->job->state == JOB_RUNNING && ns != UNIT_ACTIVATING) { - unexpected = true; - - if (UNIT_IS_INACTIVE_OR_FAILED(ns)) - job_finish_and_invalidate(u->job, ns == UNIT_FAILED ? JOB_FAILED : JOB_DONE, true, false); - } - - break; - - case JOB_RELOAD: - case JOB_RELOAD_OR_START: - case JOB_TRY_RELOAD: - - if (u->job->state == JOB_RUNNING) { - if (ns == UNIT_ACTIVE) - job_finish_and_invalidate(u->job, (flags & UNIT_NOTIFY_RELOAD_FAILURE) ? JOB_FAILED : JOB_DONE, true, false); - else if (!IN_SET(ns, UNIT_ACTIVATING, UNIT_RELOADING)) { - unexpected = true; - - if (UNIT_IS_INACTIVE_OR_FAILED(ns)) - job_finish_and_invalidate(u->job, ns == UNIT_FAILED ? JOB_FAILED : JOB_DONE, true, false); - } - } - - break; - - case JOB_STOP: - case JOB_RESTART: - case JOB_TRY_RESTART: - - if (UNIT_IS_INACTIVE_OR_FAILED(ns)) - job_finish_and_invalidate(u->job, JOB_DONE, true, false); - else if (u->job->state == JOB_RUNNING && ns != UNIT_DEACTIVATING) { - unexpected = true; - job_finish_and_invalidate(u->job, JOB_FAILED, true, false); - } - - break; - - default: - assert_not_reached("Job type unknown"); - } - - } else + /* Let's propagate state changes to the job */ + if (u->job) + unexpected = unit_process_job(u->job, ns, flags); + else unexpected = true; if (!MANAGER_IS_RELOADING(m)) { From a1c7334b619de28ec20392628db3a0f46b5f0bdc Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 11 Dec 2018 11:59:39 +0100 Subject: [PATCH 6/7] core: when a unit state changes only propagate to jobs after reloading is complete Previously, we'd immediately propagate unit state changes into any jobs pending for them, always. With this we only do this if the manager is out of the "reload" state. This fixes the problem #8803 tried to address, by simply not completing jobs until after the reload (and thus reestablishment of the dbus connection) is complete. Note that there's no need to later on explicitly catch up with the missed job state changes (i.e. there's no need to call unit_process_job() later one explicitly). That's because for jobs in JOB_WAITING state on deserialization all jobs are requeued into the run queue anyway, and thus checked again if they can complete now. And for JOB_RUNNING jobs unit_catchup() phase is going to trigger missed out state changes *after* the reload complete anyway (after all that's what distinguishes from unit_coldplug()). Replaces: #8803 --- src/core/unit.c | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/src/core/unit.c b/src/core/unit.c index 5568ccd7b2..171567022f 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -2396,7 +2396,6 @@ static bool unit_process_job(Job *j, UnitActiveState ns, UnitNotifyFlags flags) } void unit_notify(Unit *u, UnitActiveState os, UnitActiveState ns, UnitNotifyFlags flags) { - bool unexpected; const char *reason; Manager *m; @@ -2440,20 +2439,18 @@ void unit_notify(Unit *u, UnitActiveState os, UnitActiveState ns, UnitNotifyFlag unit_update_on_console(u); - /* Let's propagate state changes to the job */ - if (u->job) - unexpected = unit_process_job(u->job, ns, flags); - else - unexpected = true; - if (!MANAGER_IS_RELOADING(m)) { + bool unexpected; - /* If this state change happened without being - * requested by a job, then let's retroactively start - * or stop dependencies. We skip that step when - * deserializing, since we don't want to create any - * additional jobs just because something is already - * activated. */ + /* Let's propagate state changes to the job */ + if (u->job) + unexpected = unit_process_job(u->job, ns, flags); + else + unexpected = true; + + /* If this state change happened without being requested by a job, then let's retroactively start or + * stop dependencies. We skip that step when deserializing, since we don't want to create any + * additional jobs just because something is already activated. */ if (unexpected) { if (UNIT_IS_INACTIVE_OR_FAILED(os) && UNIT_IS_ACTIVE_OR_ACTIVATING(ns)) From a95c0505ad89b77878ed05192fe91175cf42e8b5 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 11 Dec 2018 15:22:10 +0100 Subject: [PATCH 7/7] core: extend comments regarding coldplug() vs. catchup() --- src/core/unit.h | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/core/unit.h b/src/core/unit.h index e300cc4f2f..312742f11b 100644 --- a/src/core/unit.h +++ b/src/core/unit.h @@ -433,11 +433,16 @@ typedef struct UnitVTable { int (*load)(Unit *u); /* During deserialization we only record the intended state to return to. With coldplug() we actually put the - * deserialized state in effect. This is where unit_notify() should be called to start things up. */ + * deserialized state in effect. This is where unit_notify() should be called to start things up. Note that + * this callback is invoked *before* we leave the reloading state of the manager, i.e. *before* we consider the + * reloading to be complete. Thus, this callback should just restore the exact same state for any unit that was + * in effect before the reload, i.e. units should not catch up with changes happened during the reload. That's + * what catchup() below is for. */ int (*coldplug)(Unit *u); - /* This is called shortly after all units' coldplug() call was invoked. It's supposed to catch up state changes - * we missed so far (for example because they took place while we were reloading/reexecing) */ + /* This is called shortly after all units' coldplug() call was invoked, and *after* the manager left the + * reloading state. It's supposed to catch up with state changes due to external events we missed so far (for + * example because they took place while we were reloading/reexecing) */ void (*catchup)(Unit *u); void (*dump)(Unit *u, FILE *f, const char *prefix);