[systemd-commits] 4 commits - src/core
Michal Schmidt
michich at kemper.freedesktop.org
Mon Apr 23 16:56:29 PDT 2012
src/core/dbus-job.c | 6 -
src/core/job.c | 162 ++++++++++++++++++++++++++++++++++++++++++++++---
src/core/job.h | 6 +
src/core/transaction.c | 162 +++++++++++++++++++++----------------------------
src/core/unit.c | 47 +++++++++++---
src/core/unit.h | 4 -
6 files changed, 275 insertions(+), 112 deletions(-)
New commits:
commit 39a18c60d07319ebfcfd476556729c2cadd616d6
Author: Michal Schmidt <mschmidt at redhat.com>
Date: Mon Apr 23 01:24:04 2012 +0200
job: serialize jobs properly
Jobs were not preserved correctly over a daemon-reload operation.
A systemctl process waiting for a job completion received a job removal
signal. The job itself changed its id. The job timeout started ticking all
over again.
This fixes the deficiencies.
diff --git a/src/core/dbus-job.c b/src/core/dbus-job.c
index ef839ff..4b20d49 100644
--- a/src/core/dbus-job.c
+++ b/src/core/dbus-job.c
@@ -232,7 +232,7 @@ static int job_send_message(Job *j, DBusMessage* (*new_message)(Job *j)) {
assert(j);
assert(new_message);
- if (bus_has_subscriber(j->manager)) {
+ if (bus_has_subscriber(j->manager) || j->forgot_bus_clients) {
m = new_message(j);
if (!m)
goto oom;
@@ -347,7 +347,7 @@ void bus_job_send_change_signal(Job *j) {
j->in_dbus_queue = false;
}
- if (!bus_has_subscriber(j->manager) && !j->bus_client_list) {
+ if (!bus_has_subscriber(j->manager) && !j->bus_client_list && !j->forgot_bus_clients) {
j->sent_dbus_new_signal = true;
return;
}
@@ -366,7 +366,7 @@ oom:
void bus_job_send_removed_signal(Job *j) {
assert(j);
- if (!bus_has_subscriber(j->manager) && !j->bus_client_list)
+ if (!bus_has_subscriber(j->manager) && !j->bus_client_list && !j->forgot_bus_clients)
return;
if (!j->sent_dbus_new_signal)
diff --git a/src/core/job.c b/src/core/job.c
index 07d4fc3..326460d 100644
--- a/src/core/job.c
+++ b/src/core/job.c
@@ -47,22 +47,36 @@ JobBusClient* job_bus_client_new(DBusConnection *connection, const char *name) {
return cl;
}
-Job* job_new(Unit *unit, JobType type) {
+Job* job_new_raw(Unit *unit) {
Job *j;
- assert(type < _JOB_TYPE_MAX);
+ /* used for deserialization */
+
assert(unit);
- if (!(j = new0(Job, 1)))
+ j = new0(Job, 1);
+ if (!j)
return NULL;
j->manager = unit->manager;
- j->id = j->manager->current_job_id++;
- j->type = type;
j->unit = unit;
-
j->timer_watch.type = WATCH_INVALID;
+ return j;
+}
+
+Job* job_new(Unit *unit, JobType type) {
+ Job *j;
+
+ assert(type < _JOB_TYPE_MAX);
+
+ j = job_new_raw(unit);
+ if (!j)
+ return NULL;
+
+ j->id = j->manager->current_job_id++;
+ j->type = type;
+
/* We don't link it here, that's what job_dependency() is for */
return j;
@@ -105,7 +119,9 @@ void job_uninstall(Job *j) {
assert(j->unit->job == j);
/* Detach from next 'bigger' objects */
- bus_job_send_removed_signal(j);
+ /* daemon-reload should be transparent to job observers */
+ if (j->manager->n_reloading <= 0)
+ bus_job_send_removed_signal(j);
j->unit->job = NULL;
unit_add_to_gc_queue(j->unit);
@@ -180,6 +196,18 @@ Job* job_install(Job *j) {
return j;
}
+void job_install_deserialized(Job *j) {
+ assert(!j->installed);
+
+ if (j->unit->job) {
+ log_debug("Unit %s already has a job installed. Not installing deserialized job.", j->unit->id);
+ return;
+ }
+ j->unit->job = j;
+ j->installed = true;
+ log_debug("Reinstalled deserialized job %s/%s as %u", j->unit->id, job_type_to_string(j->type), (unsigned) j->id);
+}
+
JobDependency* job_dependency_new(Job *subject, Job *object, bool matters, bool conflicts) {
JobDependency *l;
@@ -733,6 +761,126 @@ void job_timer_event(Job *j, uint64_t n_elapsed, Watch *w) {
job_finish_and_invalidate(j, JOB_TIMEOUT);
}
+int job_serialize(Job *j, FILE *f, FDSet *fds) {
+ fprintf(f, "job-id=%u\n", j->id);
+ fprintf(f, "job-type=%s\n", job_type_to_string(j->type));
+ fprintf(f, "job-state=%s\n", job_state_to_string(j->state));
+ fprintf(f, "job-override=%s\n", yes_no(j->override));
+ fprintf(f, "job-sent-dbus-new-signal=%s\n", yes_no(j->sent_dbus_new_signal));
+ fprintf(f, "job-ignore-order=%s\n", yes_no(j->ignore_order));
+ /* Cannot save bus clients. Just note the fact that we're losing
+ * them. job_send_message() will fallback to broadcasting. */
+ fprintf(f, "job-forgot-bus-clients=%s\n",
+ yes_no(j->forgot_bus_clients || j->bus_client_list));
+ if (j->timer_watch.type == WATCH_JOB_TIMER) {
+ int copy = fdset_put_dup(fds, j->timer_watch.fd);
+ if (copy < 0)
+ return copy;
+ fprintf(f, "job-timer-watch-fd=%d\n", copy);
+ }
+
+ /* End marker */
+ fputc('\n', f);
+ return 0;
+}
+
+int job_deserialize(Job *j, FILE *f, FDSet *fds) {
+ for (;;) {
+ char line[LINE_MAX], *l, *v;
+ size_t k;
+
+ if (!fgets(line, sizeof(line), f)) {
+ if (feof(f))
+ return 0;
+ return -errno;
+ }
+
+ char_array_0(line);
+ l = strstrip(line);
+
+ /* End marker */
+ if (l[0] == 0)
+ return 0;
+
+ k = strcspn(l, "=");
+
+ if (l[k] == '=') {
+ l[k] = 0;
+ v = l+k+1;
+ } else
+ v = l+k;
+
+ if (streq(l, "job-id")) {
+ if (safe_atou32(v, &j->id) < 0)
+ log_debug("Failed to parse job id value %s", v);
+ } else if (streq(l, "job-type")) {
+ JobType t = job_type_from_string(v);
+ if (t < 0)
+ log_debug("Failed to parse job type %s", v);
+ else
+ j->type = t;
+ } else if (streq(l, "job-state")) {
+ JobState s = job_state_from_string(v);
+ if (s < 0)
+ log_debug("Failed to parse job state %s", v);
+ else
+ j->state = s;
+ } else if (streq(l, "job-override")) {
+ int b = parse_boolean(v);
+ if (b < 0)
+ log_debug("Failed to parse job override flag %s", v);
+ else
+ j->override = j->override || b;
+ } else if (streq(l, "job-sent-dbus-new-signal")) {
+ int b = parse_boolean(v);
+ if (b < 0)
+ log_debug("Failed to parse job sent_dbus_new_signal flag %s", v);
+ else
+ j->sent_dbus_new_signal = j->sent_dbus_new_signal || b;
+ } else if (streq(l, "job-ignore-order")) {
+ int b = parse_boolean(v);
+ if (b < 0)
+ log_debug("Failed to parse job ignore_order flag %s", v);
+ else
+ j->ignore_order = j->ignore_order || b;
+ } else if (streq(l, "job-forgot-bus-clients")) {
+ int b = parse_boolean(v);
+ if (b < 0)
+ log_debug("Failed to parse job forgot_bus_clients flag %s", v);
+ else
+ j->forgot_bus_clients = j->forgot_bus_clients || b;
+ } else if (streq(l, "job-timer-watch-fd")) {
+ int fd;
+ if (safe_atoi(v, &fd) < 0 || fd < 0 || !fdset_contains(fds, fd))
+ log_debug("Failed to parse job-timer-watch-fd value %s", v);
+ else {
+ if (j->timer_watch.type == WATCH_JOB_TIMER)
+ close_nointr_nofail(j->timer_watch.fd);
+
+ j->timer_watch.type = WATCH_JOB_TIMER;
+ j->timer_watch.fd = fdset_remove(fds, fd);
+ j->timer_watch.data.job = j;
+ }
+ }
+ }
+}
+
+int job_coldplug(Job *j) {
+ struct epoll_event ev;
+
+ if (j->timer_watch.type != WATCH_JOB_TIMER)
+ return 0;
+
+ zero(ev);
+ ev.data.ptr = &j->timer_watch;
+ ev.events = EPOLLIN;
+
+ if (epoll_ctl(j->manager->epoll_fd, EPOLL_CTL_ADD, j->timer_watch.fd, &ev) < 0)
+ return -errno;
+
+ return 0;
+}
+
static const char* const job_state_table[_JOB_STATE_MAX] = {
[JOB_WAITING] = "waiting",
[JOB_RUNNING] = "running"
diff --git a/src/core/job.h b/src/core/job.h
index e869856..9da7e84 100644
--- a/src/core/job.h
+++ b/src/core/job.h
@@ -141,15 +141,21 @@ struct Job {
bool in_dbus_queue:1;
bool sent_dbus_new_signal:1;
bool ignore_order:1;
+ bool forgot_bus_clients:1;
};
JobBusClient* job_bus_client_new(DBusConnection *connection, const char *name);
Job* job_new(Unit *unit, JobType type);
+Job* job_new_raw(Unit *unit);
void job_free(Job *job);
Job* job_install(Job *j);
+void job_install_deserialized(Job *j);
void job_uninstall(Job *j);
void job_dump(Job *j, FILE*f, const char *prefix);
+int job_serialize(Job *j, FILE *f, FDSet *fds);
+int job_deserialize(Job *j, FILE *f, FDSet *fds);
+int job_coldplug(Job *j);
JobDependency* job_dependency_new(Job *subject, Job *object, bool matters, bool conflicts);
void job_dependency_free(JobDependency *l);
diff --git a/src/core/unit.c b/src/core/unit.c
index ed3f176..1f8d52b 100644
--- a/src/core/unit.c
+++ b/src/core/unit.c
@@ -2288,8 +2288,10 @@ int unit_serialize(Unit *u, FILE *f, FDSet *fds) {
if ((r = UNIT_VTABLE(u)->serialize(u, f, fds)) < 0)
return r;
- if (u->job)
- unit_serialize_item(u, f, "job", job_type_to_string(u->job->type));
+ if (u->job) {
+ fprintf(f, "job\n");
+ job_serialize(u->job, f, fds);
+ }
dual_timestamp_serialize(f, "inactive-exit-timestamp", &u->inactive_exit_timestamp);
dual_timestamp_serialize(f, "active-enter-timestamp", &u->active_enter_timestamp);
@@ -2368,13 +2370,32 @@ int unit_deserialize(Unit *u, FILE *f, FDSet *fds) {
v = l+k;
if (streq(l, "job")) {
- JobType type;
-
- if ((type = job_type_from_string(v)) < 0)
- log_debug("Failed to parse job type value %s", v);
- else
- u->deserialized_job = type;
+ if (v[0] == '\0') {
+ /* new-style serialized job */
+ Job *j = job_new_raw(u);
+ if (!j)
+ return -ENOMEM;
+
+ r = job_deserialize(j, f, fds);
+ if (r < 0) {
+ job_free(j);
+ return r;
+ }
+ job_install_deserialized(j);
+ r = hashmap_put(u->manager->jobs, UINT32_TO_PTR(j->id), j);
+ if (r < 0) {
+ job_free(j);
+ return r;
+ }
+ } else {
+ /* legacy */
+ JobType type = job_type_from_string(v);
+ if (type < 0)
+ log_debug("Failed to parse job type value %s", v);
+ else
+ u->deserialized_job = type;
+ }
continue;
} else if (streq(l, "inactive-exit-timestamp")) {
dual_timestamp_deserialize(v, &u->inactive_exit_timestamp);
@@ -2450,8 +2471,14 @@ int unit_coldplug(Unit *u) {
if ((r = UNIT_VTABLE(u)->coldplug(u)) < 0)
return r;
- if (u->deserialized_job >= 0) {
- if ((r = manager_add_job(u->manager, u->deserialized_job, u, JOB_IGNORE_REQUIREMENTS, false, NULL, NULL)) < 0)
+ if (u->job) {
+ r = job_coldplug(u->job);
+ if (r < 0)
+ return r;
+ } else if (u->deserialized_job >= 0) {
+ /* legacy */
+ r = manager_add_job(u->manager, u->deserialized_job, u, JOB_IGNORE_REQUIREMENTS, false, NULL, NULL);
+ if (r < 0)
return r;
u->deserialized_job = _JOB_TYPE_INVALID;
diff --git a/src/core/unit.h b/src/core/unit.h
index d8f4644..1b777bf 100644
--- a/src/core/unit.h
+++ b/src/core/unit.h
@@ -200,7 +200,9 @@ struct Unit {
unsigned gc_marker;
/* When deserializing, temporarily store the job type for this
- * unit here, if there was a job scheduled */
+ * unit here, if there was a job scheduled.
+ * Only for deserializing from a legacy version. New style uses full
+ * serialized jobs. */
int deserialized_job; /* This is actually of type JobType */
/* Error code when we didn't manage to load the unit (negative) */
commit 1b9cea0caa85dce6d9f117638a296b141c49a8fd
Author: Michal Schmidt <mschmidt at redhat.com>
Date: Sun Apr 22 10:54:58 2012 +0200
transaction: abort does not need to use recursive deletion
Recursion is unnecessary, because we're deleting all transaction jobs
anyway. And the recursive deletion produces debug messages that are
pointless in transaction abort.
diff --git a/src/core/transaction.c b/src/core/transaction.c
index 394c181..8b41168 100644
--- a/src/core/transaction.c
+++ b/src/core/transaction.c
@@ -51,7 +51,7 @@ void transaction_abort(Transaction *tr) {
assert(tr);
while ((j = hashmap_first(tr->jobs)))
- transaction_delete_job(tr, j, true);
+ transaction_delete_job(tr, j, false);
assert(hashmap_isempty(tr->jobs));
}
commit 4e7bd268ae1f39675988b9ac61b9378a67e3ae3e
Author: Michal Schmidt <mschmidt at redhat.com>
Date: Sun Apr 22 02:09:04 2012 +0200
transaction: fix detection of cycles involving installed jobs
A transaction can be acyclic, but when it's added to installed jobs,
a cycle may result.
transaction_verify_order_one() attempts to detect these cases, but it
fails because the installed jobs often have the exact generation number
that makes them look as if they were walked already.
Fix it by resetting the generation numbers of all installed jobs before
detecting cycles.
An alternative fix could be to add the generation counter to the
Manager and use it instead of starting always from 1 in
transaction_activate(). But I prefer not having to worry about it
wrapping around.
diff --git a/src/core/transaction.c b/src/core/transaction.c
index 3984947..394c181 100644
--- a/src/core/transaction.c
+++ b/src/core/transaction.c
@@ -603,6 +603,8 @@ rollback:
}
int transaction_activate(Transaction *tr, Manager *m, JobMode mode, DBusError *e) {
+ Iterator i;
+ Job *j;
int r;
unsigned generation = 1;
@@ -611,6 +613,12 @@ int transaction_activate(Transaction *tr, Manager *m, JobMode mode, DBusError *e
/* This applies the changes recorded in tr->jobs to
* the actual list of jobs, if possible. */
+ /* Reset the generation counter of all installed jobs. The detection of cycles
+ * looks at installed jobs. If they had a non-zero generation from some previous
+ * walk of the graph, the algorithm would break. */
+ HASHMAP_FOREACH(j, m->jobs, i)
+ j->generation = 0;
+
/* First step: figure out which jobs matter */
transaction_find_jobs_that_matter_to_anchor(tr->anchor_job, generation++);
commit 055163ad15a5ca1eb5626c63fa7163e152698e2b
Author: Michal Schmidt <mschmidt at redhat.com>
Date: Sat Apr 21 21:40:40 2012 +0200
transaction: improve readability
The functions looked complicated with the nested loops with breaks,
continues, and "while (again)".
Here using goto actually makes them easier to understand.
Also correcting the comment about redundant jobs.
diff --git a/src/core/transaction.c b/src/core/transaction.c
index a8b7e4c..3984947 100644
--- a/src/core/transaction.c
+++ b/src/core/transaction.c
@@ -279,44 +279,32 @@ static int transaction_merge_jobs(Transaction *tr, DBusError *e) {
}
static void transaction_drop_redundant(Transaction *tr) {
- bool again;
-
- assert(tr);
-
- /* Goes through the transaction and removes all jobs that are
- * a noop */
-
- do {
- Job *j;
- Iterator i;
-
- again = false;
-
- HASHMAP_FOREACH(j, tr->jobs, i) {
- bool changes_something = false;
- Job *k;
+ Job *j;
+ Iterator i;
- LIST_FOREACH(transaction, k, j) {
+ /* Goes through the transaction and removes all jobs of the units
+ * whose jobs are all noops. If not all of a unit's jobs are
+ * redundant, they are kept. */
- if (tr->anchor_job != k &&
- job_type_is_redundant(k->type, unit_active_state(k->unit)) &&
- (!k->unit->job || !job_type_is_conflicting(k->type, k->unit->job->type)))
- continue;
+ assert(tr);
- changes_something = true;
- break;
- }
+rescan:
+ HASHMAP_FOREACH(j, tr->jobs, i) {
+ Job *k;
- if (changes_something)
- continue;
+ LIST_FOREACH(transaction, k, j) {
- /* log_debug("Found redundant job %s/%s, dropping.", j->unit->id, job_type_to_string(j->type)); */
- transaction_delete_job(tr, j, false);
- again = true;
- break;
+ if (tr->anchor_job == k ||
+ !job_type_is_redundant(k->type, unit_active_state(k->unit)) ||
+ (k->unit->job && job_type_is_conflicting(k->type, k->unit->job->type)))
+ goto next_unit;
}
- } while (again);
+ /* log_debug("Found redundant job %s/%s, dropping.", j->unit->id, job_type_to_string(j->type)); */
+ transaction_delete_job(tr, j, false);
+ goto rescan;
+ next_unit:;
+ }
}
static bool unit_matters_to_anchor(Unit *u, Job *j) {
@@ -451,34 +439,27 @@ static int transaction_verify_order(Transaction *tr, unsigned *generation, DBusE
}
static void transaction_collect_garbage(Transaction *tr) {
- bool again;
+ Iterator i;
+ Job *j;
assert(tr);
/* Drop jobs that are not required by any other job */
- do {
- Iterator i;
- Job *j;
-
- again = false;
-
- HASHMAP_FOREACH(j, tr->jobs, i) {
- if (tr->anchor_job == j || j->object_list) {
- /* log_debug("Keeping job %s/%s because of %s/%s", */
- /* j->unit->id, job_type_to_string(j->type), */
- /* j->object_list->subject ? j->object_list->subject->unit->id : "root", */
- /* j->object_list->subject ? job_type_to_string(j->object_list->subject->type) : "root"); */
- continue;
- }
-
- /* log_debug("Garbage collecting job %s/%s", j->unit->id, job_type_to_string(j->type)); */
- transaction_delete_job(tr, j, true);
- again = true;
- break;
+rescan:
+ HASHMAP_FOREACH(j, tr->jobs, i) {
+ if (tr->anchor_job == j || j->object_list) {
+ /* log_debug("Keeping job %s/%s because of %s/%s", */
+ /* j->unit->id, job_type_to_string(j->type), */
+ /* j->object_list->subject ? j->object_list->subject->unit->id : "root", */
+ /* j->object_list->subject ? job_type_to_string(j->object_list->subject->type) : "root"); */
+ continue;
}
- } while (again);
+ /* log_debug("Garbage collecting job %s/%s", j->unit->id, job_type_to_string(j->type)); */
+ transaction_delete_job(tr, j, true);
+ goto rescan;
+ }
}
static int transaction_is_destructive(Transaction *tr, DBusError *e) {
@@ -508,59 +489,50 @@ static int transaction_is_destructive(Transaction *tr, DBusError *e) {
}
static void transaction_minimize_impact(Transaction *tr) {
- bool again;
+ Job *j;
+ Iterator i;
+
assert(tr);
/* Drops all unnecessary jobs that reverse already active jobs
* or that stop a running service. */
- do {
- Job *j;
- Iterator i;
-
- again = false;
-
- HASHMAP_FOREACH(j, tr->jobs, i) {
- LIST_FOREACH(transaction, j, j) {
- bool stops_running_service, changes_existing_job;
+rescan:
+ HASHMAP_FOREACH(j, tr->jobs, i) {
+ LIST_FOREACH(transaction, j, j) {
+ bool stops_running_service, changes_existing_job;
- /* If it matters, we shouldn't drop it */
- if (j->matters_to_anchor)
- continue;
+ /* If it matters, we shouldn't drop it */
+ if (j->matters_to_anchor)
+ continue;
- /* Would this stop a running service?
- * Would this change an existing job?
- * If so, let's drop this entry */
+ /* Would this stop a running service?
+ * Would this change an existing job?
+ * If so, let's drop this entry */
- stops_running_service =
- j->type == JOB_STOP && UNIT_IS_ACTIVE_OR_ACTIVATING(unit_active_state(j->unit));
+ stops_running_service =
+ j->type == JOB_STOP && UNIT_IS_ACTIVE_OR_ACTIVATING(unit_active_state(j->unit));
- changes_existing_job =
- j->unit->job &&
- job_type_is_conflicting(j->type, j->unit->job->type);
+ changes_existing_job =
+ j->unit->job &&
+ job_type_is_conflicting(j->type, j->unit->job->type);
- if (!stops_running_service && !changes_existing_job)
- continue;
+ if (!stops_running_service && !changes_existing_job)
+ continue;
- if (stops_running_service)
- log_debug("%s/%s would stop a running service.", j->unit->id, job_type_to_string(j->type));
+ if (stops_running_service)
+ log_debug("%s/%s would stop a running service.", j->unit->id, job_type_to_string(j->type));
- if (changes_existing_job)
- log_debug("%s/%s would change existing job.", j->unit->id, job_type_to_string(j->type));
+ if (changes_existing_job)
+ log_debug("%s/%s would change existing job.", j->unit->id, job_type_to_string(j->type));
- /* Ok, let's get rid of this */
- log_debug("Deleting %s/%s to minimize impact.", j->unit->id, job_type_to_string(j->type));
+ /* Ok, let's get rid of this */
+ log_debug("Deleting %s/%s to minimize impact.", j->unit->id, job_type_to_string(j->type));
- transaction_delete_job(tr, j, true);
- again = true;
- break;
- }
-
- if (again)
- break;
+ transaction_delete_job(tr, j, true);
+ goto rescan;
}
-
- } while (again);
+ }
}
static int transaction_apply(Transaction *tr, Manager *m, JobMode mode) {
More information about the systemd-commits
mailing list