systemctl: warn when operating on service files that changed on disk but haven't been reloaded

fixme

@@ -37,22 +37,12 @@
 
 * timeout waiting for mount devices?
 
-* default logic for serial getty, ck logging, ssh readahead
-
 * place /etc/inittab with explaining blurb.
 
-* OnFailure=foo.unit
-
-* default.target must be %ghosted...
-
 * In command lines, support both "$FOO" and $FOO
 
 * systemd-install disable should recursively kill all symlinks
 
-* in %post create all symlinks manually and use inittab data
-
-* check mtimes of dirs and unit files in systemctl
-
 * /etc must always take precedence even if we follow symlinks!
 
 * /lib/init/rw
@@ -69,6 +59,12 @@
 
 External:
 
+* default.target must be %ghosted...
+
+* in %post create all symlinks manually and use inittab data
+
+* default logic for serial getty, ck logging, ssh readahead
+
 * patch /etc/init.d/functions with:
 
   if [ $PPID -ne 1 && mountpoint /cgroup/systemd ] ; then echo "You suck!" ; fi

src/dbus-unit.c

@@ -254,6 +254,23 @@ int bus_unit_append_cgroups(Manager *m, DBusMessageIter *i, const char *property
         return 0;
 }
 
+int bus_unit_append_need_daemon_reload(Manager *m, DBusMessageIter *i, const char *property, void *data) {
+        Unit *u = data;
+        dbus_bool_t b;
+
+        assert(m);
+        assert(i);
+        assert(property);
+        assert(u);
+
+        b = unit_need_daemon_reload(u);
+
+        if (!dbus_message_iter_append_basic(i, DBUS_TYPE_BOOLEAN, &b))
+                return -ENOMEM;
+
+        return 0;
+}
+
 static DBusHandlerResult bus_unit_message_dispatch(Unit *u, DBusConnection *connection, DBusMessage *message) {
         DBusMessage *reply = NULL;
         Manager *m = u->meta.manager;

src/dbus-unit.h

@@ -88,6 +88,7 @@
         "  <property name=\"DefaultDependencies\" type=\"b\" access=\"read\"/>\n" \
         "  <property name=\"DefaultControlGroup\" type=\"s\" access=\"read\"/>\n" \
         "  <property name=\"ControlGroups\" type=\"as\" access=\"read\"/>\n" \
+        "  <property name=\"NeedDaemonReload\" type=\"b\" access=\"read\"/>\n" \
         " </interface>\n"
 
 #define BUS_UNIT_PROPERTIES \
@@ -121,7 +122,8 @@
         { "org.freedesktop.systemd1.Unit", "OnlyByDependency",     bus_property_append_bool,       "b",    &u->meta.only_by_dependency       }, \
         { "org.freedesktop.systemd1.Unit", "DefaultDependencies",  bus_property_append_bool,       "b",    &u->meta.default_dependencies     }, \
         { "org.freedesktop.systemd1.Unit", "DefaultControlGroup",  bus_unit_append_default_cgroup, "s",    u                                 }, \
-        { "org.freedesktop.systemd1.Unit", "ControlGroups",        bus_unit_append_cgroups,        "as",   u                                 }
+        { "org.freedesktop.systemd1.Unit", "ControlGroups",        bus_unit_append_cgroups,        "as",   u                                 }, \
+        { "org.freedesktop.systemd1.Unit", "NeedDaemonReload",     bus_unit_append_need_daemon_reload, "b", u                                }
 
 int bus_unit_append_names(Manager *m, DBusMessageIter *i, const char *property, void *data);
 int bus_unit_append_dependencies(Manager *m, DBusMessageIter *i, const char *property, void *data);
@@ -134,6 +136,7 @@ int bus_unit_append_can_reload(Manager *m, DBusMessageIter *i, const char *prope
 int bus_unit_append_job(Manager *m, DBusMessageIter *i, const char *property, void *data);
 int bus_unit_append_default_cgroup(Manager *m, DBusMessageIter *i, const char *property, void *data);
 int bus_unit_append_cgroups(Manager *m, DBusMessageIter *i, const char *property, void *data);
+int bus_unit_append_need_daemon_reload(Manager *m, DBusMessageIter *i, const char *property, void *data);
 
 void bus_unit_send_change_signal(Unit *u);
 void bus_unit_send_removed_signal(Unit *u);

src/load-fragment.c

@@ -29,6 +29,7 @@
 #include <sys/prctl.h>
 #include <sys/mount.h>
 #include <linux/fs.h>
+#include <sys/stat.h>
 
 #include "unit.h"
 #include "strv.h"
@@ -1558,6 +1559,7 @@ static int load_from_path(Unit *u, const char *path) {
                 { "Conflicts",              config_parse_deps,            UINT_TO_PTR(UNIT_CONFLICTS),                     "Unit"    },
                 { "Before",                 config_parse_deps,            UINT_TO_PTR(UNIT_BEFORE),                        "Unit"    },
                 { "After",                  config_parse_deps,            UINT_TO_PTR(UNIT_AFTER),                         "Unit"    },
+                { "OnFailure",              config_parse_deps,            UINT_TO_PTR(UNIT_ON_FAILURE),                    "Unit"    },
                 { "RecursiveStop",          config_parse_bool,            &u->meta.recursive_stop,                         "Unit"    },
                 { "StopWhenUnneeded",       config_parse_bool,            &u->meta.stop_when_unneeded,                     "Unit"    },
                 { "OnlyByDependency",       config_parse_bool,            &u->meta.only_by_dependency,                     "Unit"    },
@@ -1653,6 +1655,7 @@ static int load_from_path(Unit *u, const char *path) {
         FILE *f = NULL;
         char *filename = NULL, *id = NULL;
         Unit *merged;
+        struct stat st;
 
         if (!u) {
                 /* Dirty dirty hack. */
@@ -1740,6 +1743,17 @@ static int load_from_path(Unit *u, const char *path) {
                 goto finish;
         }
 
+        zero(st);
+        if (fstat(fileno(f), &st) < 0) {
+                r = -errno;
+                goto finish;
+        }
+
+        if (!S_ISREG(st.st_mode)) {
+                r = -ENOENT;
+                goto finish;
+        }
+
         /* Now, parse the file contents */
         if ((r = config_parse(filename, f, sections, items, false, u)) < 0)
                 goto finish;
@@ -1748,6 +1762,8 @@ static int load_from_path(Unit *u, const char *path) {
         u->meta.fragment_path = filename;
         filename = NULL;
 
+        u->meta.fragment_mtime = timespec_load(&st.st_mtim);
+
         u->meta.load_state = UNIT_LOADED;
         r = 0;
 

src/systemctl.c

@@ -742,6 +742,77 @@ finish:
         return r;
 }
 
+static bool unit_need_daemon_reload(DBusConnection *bus, const char *unit) {
+        DBusMessage *m, *reply;
+        dbus_bool_t b = FALSE;
+        DBusMessageIter iter, sub;
+        const char
+                *interface = "org.freedesktop.systemd1.Unit",
+                *property = "NeedDaemonReload",
+                *path;
+
+        /* We ignore all errors here, since this is used to show a warning only */
+
+        if (!(m = dbus_message_new_method_call(
+                              "org.freedesktop.systemd1",
+                              "/org/freedesktop/systemd1",
+                              "org.freedesktop.systemd1.Manager",
+                              "GetUnit")))
+                goto finish;
+
+        if (!dbus_message_append_args(m,
+                                      DBUS_TYPE_STRING, &unit,
+                                      DBUS_TYPE_INVALID))
+                goto finish;
+
+        if (!(reply = dbus_connection_send_with_reply_and_block(bus, m, -1, NULL)))
+                goto finish;
+
+        if (!dbus_message_get_args(reply, NULL,
+                                   DBUS_TYPE_OBJECT_PATH, &path,
+                                   DBUS_TYPE_INVALID))
+                goto finish;
+
+        dbus_message_unref(m);
+        if (!(m = dbus_message_new_method_call(
+                              "org.freedesktop.systemd1",
+                              path,
+                              "org.freedesktop.DBus.Properties",
+                              "Get")))
+                goto finish;
+
+        if (!dbus_message_append_args(m,
+                                      DBUS_TYPE_STRING, &interface,
+                                      DBUS_TYPE_STRING, &property,
+                                      DBUS_TYPE_INVALID)) {
+                goto finish;
+        }
+
+        dbus_message_unref(reply);
+        if (!(reply = dbus_connection_send_with_reply_and_block(bus, m, -1, NULL)))
+                goto finish;
+
+        if (!dbus_message_iter_init(reply, &iter) ||
+            dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_VARIANT)
+                goto finish;
+
+        dbus_message_iter_recurse(&iter, &sub);
+
+        if (dbus_message_iter_get_arg_type(&sub) != DBUS_TYPE_BOOLEAN)
+                goto finish;
+
+        dbus_message_iter_get_basic(&sub, &b);
+
+finish:
+        if (m)
+                dbus_message_unref(m);
+
+        if (reply)
+                dbus_message_unref(reply);
+
+        return b;
+}
+
 typedef struct WaitData {
         Set *set;
         bool failed;
@@ -860,6 +931,7 @@ static int start_unit_one(
 
         DBusMessage *m = NULL, *reply = NULL;
         DBusError error;
+        const char *path;
         int r;
 
         assert(bus);
@@ -903,17 +975,20 @@ static int start_unit_one(
                 goto finish;
         }
 
-        if (!arg_no_block) {
-                const char *path;
-                char *p;
+        if (!dbus_message_get_args(reply, &error,
+                                   DBUS_TYPE_OBJECT_PATH, &path,
+                                   DBUS_TYPE_INVALID)) {
+                log_error("Failed to parse reply: %s", error.message);
+                r = -EIO;
+                goto finish;
+        }
 
-                if (!dbus_message_get_args(reply, &error,
-                                           DBUS_TYPE_OBJECT_PATH, &path,
-                                           DBUS_TYPE_INVALID)) {
-                        log_error("Failed to parse reply: %s", error.message);
-                        r = -EIO;
-                        goto finish;
-                }
+        if (unit_need_daemon_reload(bus, name))
+                log_warning("Unit file of created job changed on disk, 'systemctl %s daemon-reload' recommended.",
+                            arg_session ? "--session" : "--system");
+
+        if (!arg_no_block) {
+                char *p;
 
                 if (!(p = strdup(path))) {
                         log_error("Failed to duplicate path.");
@@ -1287,6 +1362,8 @@ typedef struct UnitStatusInfo {
         const char *fragment_path;
         const char *default_control_group;
 
+        bool need_daemon_reload;
+
         /* Service */
         pid_t main_pid;
         pid_t control_pid;
@@ -1449,6 +1526,10 @@ static void print_status_info(UnitStatusInfo *i) {
 
                 show_cgroup_by_path(i->default_control_group, "\t\t  ", c);
         }
+
+        if (i->need_daemon_reload)
+                printf("\n" ANSI_HIGHLIGHT_ON "Warning:" ANSI_HIGHLIGHT_OFF " Unit file changed on disk, 'systemctl %s daemon-reload' recommended.\n",
+                       arg_session ? "--session" : "--system");
 }
 
 static int status_property(const char *name, DBusMessageIter *iter, UnitStatusInfo *i) {
@@ -1495,6 +1576,8 @@ static int status_property(const char *name, DBusMessageIter *iter, UnitStatusIn
 
                 if (streq(name, "Accept"))
                         i->accept = b;
+                else if (streq(name, "NeedDaemonReload"))
+                        i->need_daemon_reload = b;
 
                 break;
         }

src/unit.c

@@ -27,6 +27,7 @@
 #include <sys/poll.h>
 #include <stdlib.h>
 #include <unistd.h>
+#include <sys/stat.h>
 
 #include "set.h"
 #include "unit.h"
@@ -606,7 +607,8 @@ void unit_dump(Unit *u, FILE *f, const char *prefix) {
                 "%s\tActive Enter Timestamp: %s\n"
                 "%s\tActive Exit Timestamp: %s\n"
                 "%s\tInactive Enter Timestamp: %s\n"
-                "%s\tGC Check Good: %s\n",
+                "%s\tGC Check Good: %s\n"
+                "%s\tNeed Daemon Reload: %s\n",
                 prefix, u->meta.id,
                 prefix, unit_description(u),
                 prefix, strna(u->meta.instance),
@@ -616,7 +618,8 @@ void unit_dump(Unit *u, FILE *f, const char *prefix) {
                 prefix, strna(format_timestamp(timestamp2, sizeof(timestamp2), u->meta.active_enter_timestamp.realtime)),
                 prefix, strna(format_timestamp(timestamp3, sizeof(timestamp3), u->meta.active_exit_timestamp.realtime)),
                 prefix, strna(format_timestamp(timestamp4, sizeof(timestamp4), u->meta.inactive_enter_timestamp.realtime)),
-                prefix, yes_no(unit_check_gc(u)));
+                prefix, yes_no(unit_check_gc(u)),
+                prefix, yes_no(unit_need_daemon_reload(u)));
 
         SET_FOREACH(t, u->meta.names, i)
                 fprintf(f, "%s\tName: %s\n", prefix, t);
@@ -2029,6 +2032,24 @@ void unit_status_printf(Unit *u, const char *format, ...) {
         va_end(ap);
 }
 
+bool unit_need_daemon_reload(Unit *u) {
+        struct stat st;
+
+        assert(u);
+
+        if (!u->meta.fragment_path)
+                return false;
+
+        zero(st);
+        if (stat(u->meta.fragment_path, &st) < 0)
+                /* What, cannot access this anymore? */
+                return true;
+
+        return
+                u->meta.fragment_mtime &&
+                timespec_load(&st.st_mtim) != u->meta.fragment_mtime;
+}
+
 static const char* const unit_type_table[_UNIT_TYPE_MAX] = {
         [UNIT_SERVICE] = "service",
         [UNIT_TIMER] = "timer",

src/unit.h

@@ -141,6 +141,7 @@ struct Meta {
 
         char *description;
         char *fragment_path; /* if loaded from a config file this is the primary path to it */
+        usec_t fragment_mtime;
 
         /* If there is something to do with this unit, then this is
          * the job for it */
@@ -458,6 +459,8 @@ int unit_coldplug(Unit *u);
 
 void unit_status_printf(Unit *u, const char *format, ...);
 
+bool unit_need_daemon_reload(Unit *u);
+
 const char *unit_type_to_string(UnitType i);
 UnitType unit_type_from_string(const char *s);