dbus: pass along information why a job failed when it failed (dbus api change!)

9 files changed, 115 insertions, 46 deletions

diff --git a/src/dbus-job.c b/src/dbus-job.c
index 18da72d67a..16aa8d071e 100644
--- a/src/dbus-job.c
+++ b/src/dbus-job.c
@@ -295,10 +295,10 @@ oom:
         log_error("Failed to allocate job change signal.");
 }
 
-void bus_job_send_removed_signal(Job *j, bool success) {
+void bus_job_send_removed_signal(Job *j) {
         char *p = NULL;
         DBusMessage *m = NULL;
-        dbus_bool_t b = success;
+        const char *r;
 
         assert(j);
 
@@ -314,10 +314,12 @@ void bus_job_send_removed_signal(Job *j, bool success) {
         if (!(m = dbus_message_new_signal("/org/freedesktop/systemd1", "org.freedesktop.systemd1.Manager", "JobRemoved")))
                 goto oom;
 
+        r = job_result_to_string(j->result);
+
         if (!dbus_message_append_args(m,
                                       DBUS_TYPE_UINT32, &j->id,
                                       DBUS_TYPE_OBJECT_PATH, &p,
-                                      DBUS_TYPE_BOOLEAN, &b,
+                                      DBUS_TYPE_STRING, &r,
                                       DBUS_TYPE_INVALID))
                 goto oom;
 
diff --git a/src/dbus-job.h b/src/dbus-job.h
index 801e3e6349..103c2ff3ec 100644
--- a/src/dbus-job.h
+++ b/src/dbus-job.h
@@ -24,8 +24,10 @@
 
 #include <dbus/dbus.h>
 
+#include "job.h"
+
 void bus_job_send_change_signal(Job *j);
-void bus_job_send_removed_signal(Job *j, bool success);
+void bus_job_send_removed_signal(Job *j);
 
 extern const DBusObjectPathVTable bus_job_vtable;
 
diff --git a/src/dbus-manager.c b/src/dbus-manager.c
index 6f98aa7308..1b3bddc348 100644
--- a/src/dbus-manager.c
+++ b/src/dbus-manager.c
@@ -143,7 +143,7 @@
         "  <signal name=\"JobRemoved\">\n"                              \
         "   <arg name=\"id\" type=\"u\"/>\n"                            \
         "   <arg name=\"job\" type=\"o\"/>\n"                           \
-        "   <arg name=\"success\" type=\"b\"/>\n"                       \
+        "   <arg name=\"result\" type=\"s\"/>\n"                        \
         "  </signal>"
 
 
diff --git a/src/job.c b/src/job.c
index ec57144cb7..53c47d4a72 100644
--- a/src/job.c
+++ b/src/job.c
@@ -60,7 +60,7 @@ void job_free(Job *j) {
 
         /* Detach from next 'bigger' objects */
         if (j->installed) {
-                bus_job_send_removed_signal(j, !j->failed);
+                bus_job_send_removed_signal(j);
 
                 if (j->unit->meta.job == j) {
                         j->unit->meta.job = NULL;
@@ -376,7 +376,6 @@ int job_run_and_invalidate(Job *j) {
 
         j->state = JOB_RUNNING;
         job_add_to_dbus_queue(j);
-        job_start_timer(j);
 
         /* While we execute this operation the job might go away (for
          * example: because it is replaced by a new, conflicting
@@ -457,17 +456,19 @@ int job_run_and_invalidate(Job *j) {
 
         if ((j = manager_get_job(m, id))) {
                 if (r == -EALREADY)
-                        r = job_finish_and_invalidate(j, true);
+                        r = job_finish_and_invalidate(j, JOB_DONE);
                 else if (r == -EAGAIN)
                         j->state = JOB_WAITING;
                 else if (r < 0)
-                        r = job_finish_and_invalidate(j, false);
+                        r = job_finish_and_invalidate(j, JOB_FAILED);
+                else
+                        job_start_timer(j);
         }
 
         return r;
 }
 
-int job_finish_and_invalidate(Job *j, bool success) {
+int job_finish_and_invalidate(Job *j, JobResult result) {
         Unit *u;
         Unit *other;
         JobType t;
@@ -479,7 +480,7 @@ int job_finish_and_invalidate(Job *j, bool success) {
         job_add_to_dbus_queue(j);
 
         /* Patch restart jobs so that they become normal start jobs */
-        if (success && (j->type == JOB_RESTART || j->type == JOB_TRY_RESTART)) {
+        if (result == JOB_DONE && (j->type == JOB_RESTART || j->type == JOB_TRY_RESTART)) {
 
                 log_debug("Converting job %s/%s -> %s/%s",
                           j->unit->meta.id, job_type_to_string(j->type),
@@ -492,22 +493,26 @@ int job_finish_and_invalidate(Job *j, bool success) {
                 return 0;
         }
 
-        j->failed = !success;
+        j->result = result;
 
-        log_debug("Job %s/%s finished, success=%s", j->unit->meta.id, job_type_to_string(j->type), yes_no(success));
+        log_debug("Job %s/%s finished, result=%s", j->unit->meta.id, job_type_to_string(j->type), job_result_to_string(result));
 
-        if (j->failed)
+        if (result == JOB_FAILED)
                 j->manager->n_failed_jobs ++;
 
         u = j->unit;
         t = j->type;
         job_free(j);
 
-        if (!success && j->type == JOB_START)
+        if (result == JOB_FAILED && j->type == JOB_START)
                 unit_status_printf(u, "Starting %s " ANSI_HIGHLIGHT_ON "failed" ANSI_HIGHLIGHT_OFF ", see 'systemctl status %s' for details.\n", unit_description(u), u->meta.id);
+        else if (result == JOB_TIMEOUT && j->type == JOB_START)
+                unit_status_printf(u, "Starting %s " ANSI_HIGHLIGHT_ON "timed out" ANSI_HIGHLIGHT_OFF ".\n", unit_description(u), u->meta.id);
+        else if (result == JOB_TIMEOUT && j->type == JOB_STOP)
+                unit_status_printf(u, "Stopping %s " ANSI_HIGHLIGHT_ON "timed out" ANSI_HIGHLIGHT_OFF ".\n", unit_description(u), u->meta.id);
 
         /* Fail depending jobs on failure */
-        if (!success) {
+        if (result != JOB_DONE) {
 
                 if (t == JOB_START ||
                     t == JOB_VERIFY_ACTIVE ||
@@ -518,14 +523,14 @@ int job_finish_and_invalidate(Job *j, bool success) {
                                     (other->meta.job->type == JOB_START ||
                                      other->meta.job->type == JOB_VERIFY_ACTIVE ||
                                      other->meta.job->type == JOB_RELOAD_OR_START))
-                                        job_finish_and_invalidate(other->meta.job, false);
+                                        job_finish_and_invalidate(other->meta.job, JOB_DEPENDENCY);
 
                         SET_FOREACH(other, u->meta.dependencies[UNIT_BOUND_BY], i)
                                 if (other->meta.job &&
                                     (other->meta.job->type == JOB_START ||
                                      other->meta.job->type == JOB_VERIFY_ACTIVE ||
                                      other->meta.job->type == JOB_RELOAD_OR_START))
-                                        job_finish_and_invalidate(other->meta.job, false);
+                                        job_finish_and_invalidate(other->meta.job, JOB_DEPENDENCY);
 
                         SET_FOREACH(other, u->meta.dependencies[UNIT_REQUIRED_BY_OVERRIDABLE], i)
                                 if (other->meta.job &&
@@ -533,7 +538,7 @@ int job_finish_and_invalidate(Job *j, bool success) {
                                     (other->meta.job->type == JOB_START ||
                                      other->meta.job->type == JOB_VERIFY_ACTIVE ||
                                      other->meta.job->type == JOB_RELOAD_OR_START))
-                                        job_finish_and_invalidate(other->meta.job, false);
+                                        job_finish_and_invalidate(other->meta.job, JOB_DEPENDENCY);
 
                 } else if (t == JOB_STOP) {
 
@@ -542,7 +547,7 @@ int job_finish_and_invalidate(Job *j, bool success) {
                                     (other->meta.job->type == JOB_START ||
                                      other->meta.job->type == JOB_VERIFY_ACTIVE ||
                                      other->meta.job->type == JOB_RELOAD_OR_START))
-                                        job_finish_and_invalidate(other->meta.job, false);
+                                        job_finish_and_invalidate(other->meta.job, JOB_DEPENDENCY);
                 }
         }
 
@@ -648,7 +653,7 @@ void job_timer_event(Job *j, uint64_t n_elapsed, Watch *w) {
         assert(w == &j->timer_watch);
 
         log_warning("Job %s/%s timed out.", j->unit->meta.id, job_type_to_string(j->type));
-        job_finish_and_invalidate(j, false);
+        job_finish_and_invalidate(j, JOB_TIMEOUT);
 }
 
 static const char* const job_state_table[_JOB_STATE_MAX] = {
@@ -678,3 +683,13 @@ static const char* const job_mode_table[_JOB_MODE_MAX] = {
 };
 
 DEFINE_STRING_TABLE_LOOKUP(job_mode, JobMode);
+
+static const char* const job_result_table[_JOB_RESULT_MAX] = {
+        [JOB_DONE] = "done",
+        [JOB_CANCELED] = "canceled",
+        [JOB_TIMEOUT] = "timeout",
+        [JOB_FAILED] = "failed",
+        [JOB_DEPENDENCY] = "dependency"
+};
+
+DEFINE_STRING_TABLE_LOOKUP(job_result, JobResult);
diff --git a/src/job.h b/src/job.h
index 2499e6492a..de0c1d2317 100644
--- a/src/job.h
+++ b/src/job.h
@@ -30,6 +30,7 @@ typedef struct JobDependency JobDependency;
 typedef enum JobType JobType;
 typedef enum JobState JobState;
 typedef enum JobMode JobMode;
+typedef enum JobResult JobResult;
 
 #include "manager.h"
 #include "unit.h"
@@ -71,6 +72,16 @@ enum JobMode {
         _JOB_MODE_INVALID = -1
 };
 
+enum JobResult {
+        JOB_DONE,
+        JOB_CANCELED,
+        JOB_TIMEOUT,
+        JOB_FAILED,
+        JOB_DEPENDENCY,
+        _JOB_RESULT_MAX,
+        _JOB_RESULT_INVALID = -1
+};
+
 struct JobDependency {
         /* Encodes that the 'subject' job needs the 'object' job in
          * some way. This structure is used only while building a transaction. */
@@ -110,13 +121,14 @@ struct Job {
         DBusConnection *bus;
         char *bus_client;
 
+        JobResult result;
+
         bool installed:1;
         bool in_run_queue:1;
         bool matters_to_anchor:1;
         bool override:1;
         bool in_dbus_queue:1;
         bool sent_dbus_new_signal:1;
-        bool failed:1;
         bool ignore_deps:1;
 };
 
@@ -146,7 +158,7 @@ int job_start_timer(Job *j);
 void job_timer_event(Job *j, uint64_t n_elapsed, Watch *w);
 
 int job_run_and_invalidate(Job *j);
-int job_finish_and_invalidate(Job *j, bool success);
+int job_finish_and_invalidate(Job *j, JobResult result);
 
 char *job_dbus_path(Job *j);
 
@@ -159,4 +171,7 @@ JobState job_state_from_string(const char *s);
 const char* job_mode_to_string(JobMode t);
 JobMode job_mode_from_string(const char *s);
 
+const char* job_result_to_string(JobResult t);
+JobResult job_result_from_string(const char *s);
+
 #endif
diff --git a/src/systemadm.vala b/src/systemadm.vala
index 33777b6b8b..c262794cb7 100644
--- a/src/systemadm.vala
+++ b/src/systemadm.vala
@@ -762,7 +762,7 @@ public class MainWindow : Window {
                 } while (unit_model.iter_next(ref iter));
         }
 
-        public void on_job_removed(uint32 id, ObjectPath path, bool success) {
+        public void on_job_removed(uint32 id, ObjectPath path, string res) {
                 TreeIter iter;
                 if (!(job_model.get_iter_first(out iter)))
                         return;
diff --git a/src/systemctl.c b/src/systemctl.c
index eacc63e94d..82741bc6e7 100644
--- a/src/systemctl.c
+++ b/src/systemctl.c
@@ -1120,7 +1120,7 @@ finish:
 
 typedef struct WaitData {
         Set *set;
-        bool failed;
+        char *result;
 } WaitData;
 
 static DBusHandlerResult wait_filter(DBusConnection *connection, DBusMessage *message, void *data) {
@@ -1144,26 +1144,52 @@ static DBusHandlerResult wait_filter(DBusConnection *connection, DBusMessage *me
 
         } else if (dbus_message_is_signal(message, "org.freedesktop.systemd1.Manager", "JobRemoved")) {
                 uint32_t id;
-                const char *path;
+                const char *path, *result;
                 dbus_bool_t success = true;
 
-                if (!dbus_message_get_args(message, &error,
-                                           DBUS_TYPE_UINT32, &id,
-                                           DBUS_TYPE_OBJECT_PATH, &path,
-                                           DBUS_TYPE_BOOLEAN, &success,
-                                           DBUS_TYPE_INVALID))
-                        log_error("Failed to parse message: %s", bus_error_message(&error));
-                else {
+                if (dbus_message_get_args(message, &error,
+                                          DBUS_TYPE_UINT32, &id,
+                                          DBUS_TYPE_OBJECT_PATH, &path,
+                                          DBUS_TYPE_STRING, &result,
+                                          DBUS_TYPE_INVALID)) {
+                        char *p;
+
+                        if ((p = set_remove(d->set, (char*) path)))
+                                free(p);
+
+                        if (*result)
+                                d->result = strdup(result);
+
+                        goto finish;
+                }
+#ifndef LEGACY
+                dbus_error_free(&error);
+
+                if (dbus_message_get_args(message, &error,
+                                          DBUS_TYPE_UINT32, &id,
+                                          DBUS_TYPE_OBJECT_PATH, &path,
+                                          DBUS_TYPE_BOOLEAN, &success,
+                                          DBUS_TYPE_INVALID)) {
                         char *p;
 
+                        /* Compatibility with older systemd versions <
+                         * 19 during upgrades. This should be dropped
+                         * one day */
+
                         if ((p = set_remove(d->set, (char*) path)))
                                 free(p);
 
                         if (!success)
-                                d->failed = true;
+                                d->result = strdup("failed");
+
+                        goto finish;
                 }
+#endif
+
+                log_error("Failed to parse message: %s", bus_error_message(&error));
         }
 
+finish:
         dbus_error_free(&error);
         return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
 }
@@ -1204,7 +1230,6 @@ static int wait_for_jobs(DBusConnection *bus, Set *s) {
 
         zero(d);
         d.set = s;
-        d.failed = false;
 
         if (!dbus_connection_add_filter(bus, wait_filter, &d, NULL)) {
                 log_error("Failed to add filter.");
@@ -1216,10 +1241,19 @@ static int wait_for_jobs(DBusConnection *bus, Set *s) {
                dbus_connection_read_write_dispatch(bus, -1))
                 ;
 
-        if (!arg_quiet && d.failed)
-                log_error("Job failed. See system logs and 'systemctl status' for details.");
+        if (!arg_quiet && d.result) {
+                if (streq(d.result, "timeout"))
+                        log_error("Job timed out.");
+                else if (streq(d.result, "canceled"))
+                        log_error("Job canceled.");
+                else if (streq(d.result, "dependency"))
+                        log_error("A dependency job failed. See system logs for details.");
+                else
+                        log_error("Job failed. See system logs and 'systemctl status' for details.");
+        }
 
-        r = d.failed ? -EIO : 0;
+        r = d.result ? -EIO : 0;
+        free(d.result);
 
 finish:
         /* This is slightly dirty, since we don't undo the filter registration. */
@@ -2750,11 +2784,12 @@ static DBusHandlerResult monitor_filter(DBusConnection *connection, DBusMessage
         } else if (dbus_message_is_signal(message, "org.freedesktop.systemd1.Manager", "JobNew") ||
                    dbus_message_is_signal(message, "org.freedesktop.systemd1.Manager", "JobRemoved")) {
                 uint32_t id;
-                const char *path;
+                const char *path, *result;
 
                 if (!dbus_message_get_args(message, &error,
                                            DBUS_TYPE_UINT32, &id,
                                            DBUS_TYPE_OBJECT_PATH, &path,
+                                           DBUS_TYPE_STRING, &result,
                                            DBUS_TYPE_INVALID))
                         log_error("Failed to parse message: %s", bus_error_message(&error));
                 else if (streq(dbus_message_get_member(message), "JobNew"))
diff --git a/src/systemd-interfaces.vala b/src/systemd-interfaces.vala
index e3b7941288..a380f7970b 100644
--- a/src/systemd-interfaces.vala
+++ b/src/systemd-interfaces.vala
@@ -85,7 +85,7 @@ public interface Manager : DBusProxy {
         public abstract signal void unit_new(string id, ObjectPath path);
         public abstract signal void unit_removed(string id, ObjectPath path);
         public abstract signal void job_new(uint32 id, ObjectPath path);
-        public abstract signal void job_removed(uint32 id, ObjectPath path, bool success);
+        public abstract signal void job_removed(uint32 id, ObjectPath path, string res);
 }
 
 [DBus (name = "org.freedesktop.systemd1.Unit")]
diff --git a/src/unit.c b/src/unit.c
index 0d5312376c..1ca8e82af7 100644
--- a/src/unit.c
+++ b/src/unit.c
@@ -1123,12 +1123,12 @@ void unit_notify(Unit *u, UnitActiveState os, UnitActiveState ns, bool reload_su
                 case JOB_VERIFY_ACTIVE:
 
                         if (UNIT_IS_ACTIVE_OR_RELOADING(ns))
-                                job_finish_and_invalidate(u->meta.job, true);
+                                job_finish_and_invalidate(u->meta.job, JOB_DONE);
                         else if (u->meta.job->state == JOB_RUNNING && ns != UNIT_ACTIVATING) {
                                 unexpected = true;
 
                                 if (UNIT_IS_INACTIVE_OR_FAILED(ns))
-                                        job_finish_and_invalidate(u->meta.job, ns != UNIT_FAILED);
+                                        job_finish_and_invalidate(u->meta.job, ns == UNIT_FAILED ? JOB_FAILED : JOB_DONE);
                         }
 
                         break;
@@ -1138,12 +1138,12 @@ void unit_notify(Unit *u, UnitActiveState os, UnitActiveState ns, bool reload_su
 
                         if (u->meta.job->state == JOB_RUNNING) {
                                 if (ns == UNIT_ACTIVE)
-                                        job_finish_and_invalidate(u->meta.job, reload_success);
+                                        job_finish_and_invalidate(u->meta.job, reload_success ? JOB_DONE : JOB_FAILED);
                                 else if (ns != UNIT_ACTIVATING && ns != UNIT_RELOADING) {
                                         unexpected = true;
 
                                         if (UNIT_IS_INACTIVE_OR_FAILED(ns))
-                                                job_finish_and_invalidate(u->meta.job, ns != UNIT_FAILED);
+                                                job_finish_and_invalidate(u->meta.job, ns == UNIT_FAILED ? JOB_FAILED : JOB_DONE);
                                 }
                         }
 
@@ -1154,10 +1154,10 @@ void unit_notify(Unit *u, UnitActiveState os, UnitActiveState ns, bool reload_su
                 case JOB_TRY_RESTART:
 
                         if (UNIT_IS_INACTIVE_OR_FAILED(ns))
-                                job_finish_and_invalidate(u->meta.job, true);
+                                job_finish_and_invalidate(u->meta.job, JOB_DONE);
                         else if (u->meta.job->state == JOB_RUNNING && ns != UNIT_DEACTIVATING) {
                                 unexpected = true;
-                                job_finish_and_invalidate(u->meta.job, false);
+                                job_finish_and_invalidate(u->meta.job, JOB_FAILED);
                         }
 
                         break;