Merge pull request #4481 from poettering/perpetual

Add "perpetual" unit concept, sysctl fixes, networkd fixes, systemctl color fixes, nspawn discard.

15 files changed, 213 insertions, 110 deletions

diff --git a/src/basic/special.h b/src/basic/special.h
index 084d3dfa23..5276bcf598 100644
--- a/src/basic/special.h
+++ b/src/basic/special.h
@@ -117,3 +117,6 @@
 
 /* The scope unit systemd itself lives in. */
 #define SPECIAL_INIT_SCOPE "init.scope"
+
+/* The root directory. */
+#define SPECIAL_ROOT_MOUNT "-.mount"
diff --git a/src/core/dbus-unit.c b/src/core/dbus-unit.c
index 8f34fa1a52..69e249c844 100644
--- a/src/core/dbus-unit.c
+++ b/src/core/dbus-unit.c
@@ -263,10 +263,7 @@ static int property_get_can_stop(
         assert(reply);
         assert(u);
 
-        /* On the lower levels we assume that every unit we can start
-         * we can also stop */
-
-        return sd_bus_message_append(reply, "b", unit_can_start(u) && !u->refuse_manual_stop);
+        return sd_bus_message_append(reply, "b", unit_can_stop(u) && !u->refuse_manual_stop);
 }
 
 static int property_get_can_reload(
@@ -760,6 +757,7 @@ const sd_bus_vtable bus_unit_vtable[] = {
         SD_BUS_PROPERTY("Asserts", "a(sbbsi)", property_get_conditions, offsetof(Unit, asserts), 0),
         SD_BUS_PROPERTY("LoadError", "(ss)", property_get_load_error, 0, SD_BUS_VTABLE_PROPERTY_CONST),
         SD_BUS_PROPERTY("Transient", "b", bus_property_get_bool, offsetof(Unit, transient), SD_BUS_VTABLE_PROPERTY_CONST),
+        SD_BUS_PROPERTY("Perpetual", "b", bus_property_get_bool, offsetof(Unit, perpetual), SD_BUS_VTABLE_PROPERTY_CONST),
         SD_BUS_PROPERTY("StartLimitIntervalSec", "t", bus_property_get_usec, offsetof(Unit, start_limit.interval), SD_BUS_VTABLE_PROPERTY_CONST),
         SD_BUS_PROPERTY("StartLimitBurst", "u", bus_property_get_unsigned, offsetof(Unit, start_limit.burst), SD_BUS_VTABLE_PROPERTY_CONST),
         SD_BUS_PROPERTY("StartLimitAction", "s", property_get_emergency_action, offsetof(Unit, start_limit_action), SD_BUS_VTABLE_PROPERTY_CONST),
diff --git a/src/core/device.c b/src/core/device.c
index 8a3e888e5e..bd87a447cd 100644
--- a/src/core/device.c
+++ b/src/core/device.c
@@ -331,11 +331,7 @@ static int device_setup_unit(Manager *m, struct udev_device *dev, const char *pa
         if (!u) {
                 delete = true;
 
-                u = unit_new(m, sizeof(Device));
-                if (!u)
-                        return log_oom();
-
-                r = unit_add_name(u, e);
+                r = unit_new_for_name(m, sizeof(Device), e, &u);
                 if (r < 0)
                         goto fail;
 
diff --git a/src/core/mount.c b/src/core/mount.c
index da480001e1..d749e49df5 100644
--- a/src/core/mount.c
+++ b/src/core/mount.c
@@ -159,17 +159,6 @@ static void mount_init(Unit *u) {
         m->timeout_usec = u->manager->default_timeout_start_usec;
         m->directory_mode = 0755;
 
-        if (unit_has_name(u, "-.mount")) {
-                /* Don't allow start/stop for root directory */
-                u->refuse_manual_start = true;
-                u->refuse_manual_stop = true;
-        } else {
-                /* The stdio/kmsg bridge socket is on /, in order to avoid a
-                 * dep loop, don't use kmsg logging for -.mount */
-                m->exec_context.std_output = u->manager->default_std_output;
-                m->exec_context.std_error = u->manager->default_std_error;
-        }
-
         /* We need to make sure that /usr/bin/mount is always called
          * in the same process group as us, so that the autofs kernel
          * side doesn't send us another mount request while we are
@@ -577,6 +566,25 @@ static int mount_add_extras(Mount *m) {
         return 0;
 }
 
+static int mount_load_root_mount(Unit *u) {
+        assert(u);
+
+        if (!unit_has_name(u, SPECIAL_ROOT_MOUNT))
+                return 0;
+
+        u->perpetual = true;
+        u->default_dependencies = false;
+
+        /* The stdio/kmsg bridge socket is on /, in order to avoid a dep loop, don't use kmsg logging for -.mount */
+        MOUNT(u)->exec_context.std_output = EXEC_OUTPUT_NULL;
+        MOUNT(u)->exec_context.std_input = EXEC_INPUT_NULL;
+
+        if (!u->description)
+                u->description = strdup("Root Mount");
+
+        return 1;
+}
+
 static int mount_load(Unit *u) {
         Mount *m = MOUNT(u);
         int r;
@@ -584,11 +592,14 @@ static int mount_load(Unit *u) {
         assert(u);
         assert(u->load_state == UNIT_STUB);
 
-        if (m->from_proc_self_mountinfo)
+        r = mount_load_root_mount(u);
+        if (r < 0)
+                return r;
+
+        if (m->from_proc_self_mountinfo || u->perpetual)
                 r = unit_load_fragment_and_dropin_optional(u);
         else
                 r = unit_load_fragment_and_dropin(u);
-
         if (r < 0)
                 return r;
 
@@ -1393,11 +1404,7 @@ static int mount_setup_unit(
         if (!u) {
                 delete = true;
 
-                u = unit_new(m, sizeof(Mount));
-                if (!u)
-                        return log_oom();
-
-                r = unit_add_name(u, e);
+                r = unit_new_for_name(m, sizeof(Mount), e, &u);
                 if (r < 0)
                         goto fail;
 
@@ -1592,11 +1599,46 @@ static int mount_get_timeout(Unit *u, usec_t *timeout) {
         return 1;
 }
 
+static int synthesize_root_mount(Manager *m) {
+        Unit *u;
+        int r;
+
+        assert(m);
+
+        /* Whatever happens, we know for sure that the root directory is around, and cannot go away. Let's
+         * unconditionally synthesize it here and mark it as perpetual. */
+
+        u = manager_get_unit(m, SPECIAL_ROOT_MOUNT);
+        if (!u) {
+                r = unit_new_for_name(m, sizeof(Mount), SPECIAL_ROOT_MOUNT, &u);
+                if (r < 0)
+                        return log_error_errno(r, "Failed to allocate the special " SPECIAL_ROOT_MOUNT " unit: %m");
+        }
+
+        u->perpetual = true;
+        MOUNT(u)->deserialized_state = MOUNT_MOUNTED;
+
+        unit_add_to_load_queue(u);
+        unit_add_to_dbus_queue(u);
+
+        return 0;
+}
+
+static bool mount_is_mounted(Mount *m) {
+        assert(m);
+
+        return UNIT(m)->perpetual || m->is_mounted;
+}
+
 static void mount_enumerate(Manager *m) {
         int r;
 
         assert(m);
 
+        r = synthesize_root_mount(m);
+        if (r < 0)
+                goto fail;
+
         mnt_init_debug(0);
 
         if (!m->mount_monitor) {
@@ -1703,7 +1745,7 @@ static int mount_dispatch_io(sd_event_source *source, int fd, uint32_t revents,
         LIST_FOREACH(units_by_type, u, m->units_by_type[UNIT_MOUNT]) {
                 Mount *mount = MOUNT(u);
 
-                if (!mount->is_mounted) {
+                if (!mount_is_mounted(mount)) {
 
                         /* A mount point is not around right now. It
                          * might be gone, or might never have
@@ -1764,7 +1806,7 @@ static int mount_dispatch_io(sd_event_source *source, int fd, uint32_t revents,
                         }
                 }
 
-                if (mount->is_mounted &&
+                if (mount_is_mounted(mount) &&
                     mount->from_proc_self_mountinfo &&
                     mount->parameters_proc_self_mountinfo.what) {
 
diff --git a/src/core/scope.c b/src/core/scope.c
index af0c43c7da..d6e1f8e392 100644
--- a/src/core/scope.c
+++ b/src/core/scope.c
@@ -154,15 +154,13 @@ static int scope_load_init_scope(Unit *u) {
                 return 0;
 
         u->transient = true;
-        u->no_gc = true;
+        u->perpetual = true;
 
         /* init.scope is a bit special, as it has to stick around forever. Because of its special semantics we
          * synthesize it here, instead of relying on the unit file on disk. */
 
         u->default_dependencies = false;
         u->ignore_on_isolate = true;
-        u->refuse_manual_start = true;
-        u->refuse_manual_stop = true;
 
         SCOPE(u)->kill_context.kill_signal = SIGRTMIN+14;
 
@@ -565,22 +563,15 @@ static void scope_enumerate(Manager *m) {
 
         u = manager_get_unit(m, SPECIAL_INIT_SCOPE);
         if (!u) {
-                u = unit_new(m, sizeof(Scope));
-                if (!u) {
-                        log_oom();
-                        return;
-                }
-
-                r = unit_add_name(u, SPECIAL_INIT_SCOPE);
+                r = unit_new_for_name(m, sizeof(Scope), SPECIAL_INIT_SCOPE, &u);
                 if (r < 0)  {
-                        unit_free(u);
-                        log_error_errno(r, "Failed to add the " SPECIAL_INIT_SCOPE " name: %m");
+                        log_error_errno(r, "Failed to allocate the special " SPECIAL_INIT_SCOPE " unit: %m");
                         return;
                 }
         }
 
         u->transient = true;
-        u->no_gc = true;
+        u->perpetual = true;
         SCOPE(u)->deserialized_state = SCOPE_RUNNING;
 
         unit_add_to_load_queue(u);
diff --git a/src/core/slice.c b/src/core/slice.c
index 0fef29661f..ed5d3fd701 100644
--- a/src/core/slice.c
+++ b/src/core/slice.c
@@ -136,15 +136,13 @@ static int slice_load_root_slice(Unit *u) {
         if (!unit_has_name(u, SPECIAL_ROOT_SLICE))
                 return 0;
 
-        u->no_gc = true;
+        u->perpetual = true;
 
         /* The root slice is a bit special. For example it is always running and cannot be terminated. Because of its
          * special semantics we synthesize it here, instead of relying on the unit file on disk. */
 
         u->default_dependencies = false;
         u->ignore_on_isolate = true;
-        u->refuse_manual_start = true;
-        u->refuse_manual_stop = true;
 
         if (!u->description)
                 u->description = strdup("Root Slice");
@@ -301,21 +299,14 @@ static void slice_enumerate(Manager *m) {
 
         u = manager_get_unit(m, SPECIAL_ROOT_SLICE);
         if (!u) {
-                u = unit_new(m, sizeof(Slice));
-                if (!u)  {
-                        log_oom();
-                        return;
-                }
-
-                r = unit_add_name(u, SPECIAL_ROOT_SLICE);
+                r = unit_new_for_name(m, sizeof(Slice), SPECIAL_ROOT_SLICE, &u);
                 if (r < 0) {
-                        unit_free(u);
-                        log_error_errno(r, "Failed to add the "SPECIAL_ROOT_SLICE " name: %m");
+                        log_error_errno(r, "Failed to allocate the special " SPECIAL_ROOT_SLICE " unit: %m");
                         return;
                 }
         }
 
-        u->no_gc = true;
+        u->perpetual = true;
         SLICE(u)->deserialized_state = SLICE_ACTIVE;
 
         unit_add_to_load_queue(u);
diff --git a/src/core/swap.c b/src/core/swap.c
index b592abb9fb..2228a254bb 100644
--- a/src/core/swap.c
+++ b/src/core/swap.c
@@ -381,11 +381,7 @@ static int swap_setup_unit(
         if (!u) {
                 delete = true;
 
-                u = unit_new(m, sizeof(Swap));
-                if (!u)
-                        return log_oom();
-
-                r = unit_add_name(u, e);
+                r = unit_new_for_name(m, sizeof(Swap), e, &u);
                 if (r < 0)
                         goto fail;
 
diff --git a/src/core/unit.c b/src/core/unit.c
index 8e6cef103b..463e6d6a62 100644
--- a/src/core/unit.c
+++ b/src/core/unit.c
@@ -109,6 +109,24 @@ Unit *unit_new(Manager *m, size_t size) {
         return u;
 }
 
+int unit_new_for_name(Manager *m, size_t size, const char *name, Unit **ret) {
+        Unit *u;
+        int r;
+
+        u = unit_new(m, size);
+        if (!u)
+                return -ENOMEM;
+
+        r = unit_add_name(u, name);
+        if (r < 0) {
+                unit_free(u);
+                return r;
+        }
+
+        *ret = u;
+        return r;
+}
+
 bool unit_has_name(Unit *u, const char *name) {
         assert(u);
         assert(name);
@@ -325,7 +343,7 @@ bool unit_check_gc(Unit *u) {
         if (!inactive)
                 return true;
 
-        if (u->no_gc)
+        if (u->perpetual)
                 return true;
 
         if (u->refs)
@@ -926,6 +944,7 @@ void unit_dump(Unit *u, FILE *f, const char *prefix) {
                 "%s\tGC Check Good: %s\n"
                 "%s\tNeed Daemon Reload: %s\n"
                 "%s\tTransient: %s\n"
+                "%s\tPerpetual: %s\n"
                 "%s\tSlice: %s\n"
                 "%s\tCGroup: %s\n"
                 "%s\tCGroup realized: %s\n"
@@ -944,6 +963,7 @@ void unit_dump(Unit *u, FILE *f, const char *prefix) {
                 prefix, yes_no(unit_check_gc(u)),
                 prefix, yes_no(unit_need_daemon_reload(u)),
                 prefix, yes_no(u->transient),
+                prefix, yes_no(u->perpetual),
                 prefix, strna(unit_slice_name(u)),
                 prefix, strna(u->cgroup_path),
                 prefix, yes_no(u->cgroup_realized),
@@ -1618,6 +1638,18 @@ int unit_stop(Unit *u) {
         return UNIT_VTABLE(u)->stop(u);
 }
 
+bool unit_can_stop(Unit *u) {
+        assert(u);
+
+        if (!unit_supported(u))
+                return false;
+
+        if (u->perpetual)
+                return false;
+
+        return !!UNIT_VTABLE(u)->stop;
+}
+
 /* Errors:
  *         -EBADR:    This unit type does not support reloading.
  *         -ENOEXEC:  Unit is not started.
@@ -2152,13 +2184,20 @@ bool unit_job_is_applicable(Unit *u, JobType j) {
 
         case JOB_VERIFY_ACTIVE:
         case JOB_START:
-        case JOB_STOP:
         case JOB_NOP:
+                /* Note that we don't check unit_can_start() here. That's because .device units and suchlike are not
+                 * startable by us but may appear due to external events, and it thus makes sense to permit enqueing
+                 * jobs for it. */
                 return true;
 
+        case JOB_STOP:
+                /* Similar as above. However, perpetual units can never be stopped (neither explicitly nor due to
+                 * external events), hence it makes no sense to permit enqueing such a request either. */
+                return !u->perpetual;
+
         case JOB_RESTART:
         case JOB_TRY_RESTART:
-                return unit_can_start(u);
+                return unit_can_stop(u) && unit_can_start(u);
 
         case JOB_RELOAD:
         case JOB_TRY_RELOAD:
diff --git a/src/core/unit.h b/src/core/unit.h
index e01ec7a775..991543664b 100644
--- a/src/core/unit.h
+++ b/src/core/unit.h
@@ -236,6 +236,9 @@ struct Unit {
         /* Is this a transient unit? */
         bool transient;
 
+        /* Is this a unit that is always running and cannot be stopped? */
+        bool perpetual;
+
         bool in_load_queue:1;
         bool in_dbus_queue:1;
         bool in_cleanup_queue:1;
@@ -244,8 +247,6 @@ struct Unit {
 
         bool sent_dbus_new_signal:1;
 
-        bool no_gc:1;
-
         bool in_audit:1;
 
         bool cgroup_realized:1;
@@ -480,6 +481,7 @@ DEFINE_CAST(SCOPE, Scope);
 Unit *unit_new(Manager *m, size_t size);
 void unit_free(Unit *u);
 
+int unit_new_for_name(Manager *m, size_t size, const char *name, Unit **ret);
 int unit_add_name(Unit *u, const char *name);
 
 int unit_add_dependency(Unit *u, UnitDependency d, Unit *other, bool add_reference);
@@ -524,6 +526,7 @@ void unit_dump(Unit *u, FILE *f, const char *prefix);
 
 bool unit_can_reload(Unit *u) _pure_;
 bool unit_can_start(Unit *u) _pure_;
+bool unit_can_stop(Unit *u) _pure_;
 bool unit_can_isolate(Unit *u) _pure_;
 
 int unit_start(Unit *u);
diff --git a/src/network/networkd-link.c b/src/network/networkd-link.c
index d9e060b6cf..aefe7335b9 100644
--- a/src/network/networkd-link.c
+++ b/src/network/networkd-link.c
@@ -514,13 +514,12 @@ static void link_free(Link *link) {
         sd_lldp_unref(link->lldp);
         free(link->lldp_file);
 
+        ndisc_flush(link);
+
         sd_ipv4ll_unref(link->ipv4ll);
         sd_dhcp6_client_unref(link->dhcp6_client);
         sd_ndisc_unref(link->ndisc);
 
-        set_free_free(link->ndisc_rdnss);
-        set_free_free(link->ndisc_dnssl);
-
         if (link->manager)
                 hashmap_remove(link->manager->links, INT_TO_PTR(link->ifindex));
 
@@ -2427,6 +2426,8 @@ static int link_drop_config(Link *link) {
                         return r;
         }
 
+        ndisc_flush(link);
+
         return 0;
 }
 
diff --git a/src/network/networkd-ndisc.c b/src/network/networkd-ndisc.c
index b282634e4b..4853791aa5 100644
--- a/src/network/networkd-ndisc.c
+++ b/src/network/networkd-ndisc.c
@@ -680,13 +680,22 @@ void ndisc_vacuum(Link *link) {
 
         SET_FOREACH(r, link->ndisc_rdnss, i)
                 if (r->valid_until < time_now) {
-                        (void) set_remove(link->ndisc_rdnss, r);
+                        free(set_remove(link->ndisc_rdnss, r));
                         link_dirty(link);
                 }
 
         SET_FOREACH(d, link->ndisc_dnssl, i)
                 if (d->valid_until < time_now) {
-                        (void) set_remove(link->ndisc_dnssl, d);
+                        free(set_remove(link->ndisc_dnssl, d));
                         link_dirty(link);
                 }
 }
+
+void ndisc_flush(Link *link) {
+        assert(link);
+
+        /* Removes all RDNSS and DNSSL entries, without exception */
+
+        link->ndisc_rdnss = set_free_free(link->ndisc_rdnss);
+        link->ndisc_dnssl = set_free_free(link->ndisc_dnssl);
+}
diff --git a/src/network/networkd-ndisc.h b/src/network/networkd-ndisc.h
index 2002f55107..127126190e 100644
--- a/src/network/networkd-ndisc.h
+++ b/src/network/networkd-ndisc.h
@@ -37,3 +37,4 @@ static inline char* NDISC_DNSSL_DOMAIN(const NDiscDNSSL *n) {
 
 int ndisc_configure(Link *link);
 void ndisc_vacuum(Link *link);
+void ndisc_flush(Link *link);
diff --git a/src/nspawn/nspawn.c b/src/nspawn/nspawn.c
index 295293858e..c56af6e6f4 100644
--- a/src/nspawn/nspawn.c
+++ b/src/nspawn/nspawn.c
@@ -2260,7 +2260,7 @@ static int dissect_image(
 static int mount_device(const char *what, const char *where, const char *directory, bool rw) {
 #ifdef HAVE_BLKID
         _cleanup_blkid_free_probe_ blkid_probe b = NULL;
-        const char *fstype, *p;
+        const char *fstype, *p, *options;
         int r;
 
         assert(what);
@@ -2309,7 +2309,17 @@ static int mount_device(const char *what, const char *where, const char *directo
                 return -EOPNOTSUPP;
         }
 
-        return mount_verbose(LOG_ERR, what, p, fstype, MS_NODEV|(rw ? 0 : MS_RDONLY), NULL);
+        /* If this is a loopback device then let's mount the image with discard, so that the underlying file remains
+         * sparse when possible. */
+        if (STR_IN_SET(fstype, "btrfs", "ext4", "vfat", "xfs")) {
+                const char *l;
+
+                l = path_startswith(what, "/dev");
+                if (l && startswith(l, "loop"))
+                        options = "discard";
+        }
+
+        return mount_verbose(LOG_ERR, what, p, fstype, MS_NODEV|(rw ? 0 : MS_RDONLY), options);
 #else
         log_error("--image= is not supported, compiled without blkid support.");
         return -EOPNOTSUPP;
diff --git a/src/sysctl/sysctl.c b/src/sysctl/sysctl.c
index fbc1e0eb1a..b3587e249d 100644
--- a/src/sysctl/sysctl.c
+++ b/src/sysctl/sysctl.c
@@ -51,19 +51,46 @@ static int apply_all(OrderedHashmap *sysctl_options) {
 
                 k = sysctl_write(property, value);
                 if (k < 0) {
-                        log_full_errno(k == -ENOENT ? LOG_INFO : LOG_WARNING, k,
-                                       "Couldn't write '%s' to '%s', ignoring: %m", value, property);
-
-                        if (r == 0 && k != -ENOENT)
-                                r = k;
+                        /* If the sysctl is not available in the kernel or we are running with reduced privileges and
+                         * cannot write it, then log about the issue at LOG_NOTICE level, and proceed without
+                         * failing. (EROFS is treated as a permission problem here, since that's how container managers
+                         * usually protected their sysctls.) In all other cases log an error and make the tool fail. */
+
+                        if (IN_SET(k, -EPERM, -EACCES, -EROFS, -ENOENT))
+                                log_notice_errno(k, "Couldn't write '%s' to '%s', ignoring: %m", value, property);
+                        else {
+                                log_error_errno(k, "Couldn't write '%s' to '%s': %m", value, property);
+                                if (r == 0)
+                                        r = k;
+                        }
                 }
         }
 
         return r;
 }
 
+static bool test_prefix(const char *p) {
+        char **i;
+
+        if (strv_isempty(arg_prefixes))
+                return true;
+
+        STRV_FOREACH(i, arg_prefixes) {
+                const char *t;
+
+                t = path_startswith(*i, "/proc/sys/");
+                if (!t)
+                        t = *i;
+                if (path_startswith(p, t))
+                        return true;
+        }
+
+        return false;
+}
+
 static int parse_file(OrderedHashmap *sysctl_options, const char *path, bool ignore_enoent) {
         _cleanup_fclose_ FILE *f = NULL;
+        unsigned c = 0;
         int r;
 
         assert(path);
@@ -77,7 +104,7 @@ static int parse_file(OrderedHashmap *sysctl_options, const char *path, bool ign
         }
 
         log_debug("Parsing %s", path);
-        while (!feof(f)) {
+        for (;;) {
                 char l[LINE_MAX], *p, *value, *new_value, *property, *existing;
                 void *v;
                 int k;
@@ -89,6 +116,8 @@ static int parse_file(OrderedHashmap *sysctl_options, const char *path, bool ign
                         return log_error_errno(errno, "Failed to read file '%s', ignoring: %m", path);
                 }
 
+                c++;
+
                 p = strstrip(l);
                 if (!*p)
                         continue;
@@ -98,7 +127,7 @@ static int parse_file(OrderedHashmap *sysctl_options, const char *path, bool ign
 
                 value = strchr(p, '=');
                 if (!value) {
-                        log_error("Line is not an assignment in file '%s': %s", path, value);
+                        log_error("Line is not an assignment at '%s:%u': %s", path, c, value);
 
                         if (r == 0)
                                 r = -EINVAL;
@@ -111,26 +140,15 @@ static int parse_file(OrderedHashmap *sysctl_options, const char *path, bool ign
                 p = sysctl_normalize(strstrip(p));
                 value = strstrip(value);
 
-                if (!strv_isempty(arg_prefixes)) {
-                        char **i, *t;
-                        STRV_FOREACH(i, arg_prefixes) {
-                                t = path_startswith(*i, "/proc/sys/");
-                                if (t == NULL)
-                                        t = *i;
-                                if (path_startswith(p, t))
-                                        goto found;
-                        }
-                        /* not found */
+                if (!test_prefix(p))
                         continue;
-                }
 
-found:
                 existing = ordered_hashmap_get2(sysctl_options, p, &v);
                 if (existing) {
                         if (streq(value, existing))
                                 continue;
 
-                        log_debug("Overwriting earlier assignment of %s in file '%s'.", p, path);
+                        log_debug("Overwriting earlier assignment of %s at '%s:%u'.", p, path, c);
                         free(ordered_hashmap_remove(sysctl_options, p));
                         free(v);
                 }
@@ -229,12 +247,12 @@ static int parse_argv(int argc, char *argv[]) {
 }
 
 int main(int argc, char *argv[]) {
+        OrderedHashmap *sysctl_options = NULL;
         int r = 0, k;
-        OrderedHashmap *sysctl_options;
 
         r = parse_argv(argc, argv);
         if (r <= 0)
-                return r < 0 ? EXIT_FAILURE : EXIT_SUCCESS;
+                goto finish;
 
         log_set_target(LOG_TARGET_AUTO);
         log_parse_environment();
diff --git a/src/systemctl/systemctl.c b/src/systemctl/systemctl.c
index b6d66aa363..dd3b931cd6 100644
--- a/src/systemctl/systemctl.c
+++ b/src/systemctl/systemctl.c
@@ -410,23 +410,24 @@ static bool output_show_unit(const UnitInfo *u, char **patterns) {
 }
 
 static int output_units_list(const UnitInfo *unit_infos, unsigned c) {
-        unsigned circle_len = 0, id_len, max_id_len, load_len, active_len, sub_len, job_len;
+        unsigned circle_len = 0, id_len, max_id_len, load_len, active_len, sub_len, job_len, desc_len, max_desc_len;
         const UnitInfo *u;
         unsigned n_shown = 0;
-        int job_count = 0, desc_len;
+        int job_count = 0;
 
         max_id_len = strlen("UNIT");
         load_len = strlen("LOAD");
         active_len = strlen("ACTIVE");
         sub_len = strlen("SUB");
         job_len = strlen("JOB");
-        desc_len = 0;
+        max_desc_len = strlen("DESCRIPTION");
 
         for (u = unit_infos; u < unit_infos + c; u++) {
                 max_id_len = MAX(max_id_len, strlen(u->id) + (u->machine ? strlen(u->machine)+1 : 0));
                 load_len = MAX(load_len, strlen(u->load_state));
                 active_len = MAX(active_len, strlen(u->active_state));
                 sub_len = MAX(sub_len, strlen(u->sub_state));
+                max_desc_len = MAX(max_desc_len, strlen(u->description));
 
                 if (u->job_id != 0) {
                         job_len = MAX(job_len, strlen(u->job_type));
@@ -442,7 +443,7 @@ static int output_units_list(const UnitInfo *unit_infos, unsigned c) {
         if (!arg_full && original_stdout_is_tty) {
                 unsigned basic_len;
 
-                id_len = MIN(max_id_len, 25u);
+                id_len = MIN(max_id_len, 25u); /* as much as it needs, but at most 25 for now */
                 basic_len = circle_len + 5 + id_len + 5 + active_len + sub_len;
 
                 if (job_count)
@@ -455,19 +456,21 @@ static int output_units_list(const UnitInfo *unit_infos, unsigned c) {
                         /* Either UNIT already got 25, or is fully satisfied.
                          * Grant up to 25 to DESC now. */
                         incr = MIN(extra_len, 25u);
-                        desc_len += incr;
+                        desc_len = incr;
                         extra_len -= incr;
 
-                        /* split the remaining space between UNIT and DESC,
-                         * but do not give UNIT more than it needs. */
+                        /* Of the remainder give as much as the ID needs to the ID, and give the rest to the
+                         * description but not more than it needs. */
                         if (extra_len > 0) {
-                                incr = MIN(extra_len / 2, max_id_len - id_len);
+                                incr = MIN(max_id_len - id_len, extra_len);
                                 id_len += incr;
-                                desc_len += extra_len - incr;
+                                desc_len += MIN(extra_len - incr, max_desc_len - desc_len);
                         }
                 }
-        } else
+        } else {
                 id_len = max_id_len;
+                desc_len = max_desc_len;
+        }
 
         for (u = unit_infos; u < unit_infos + c; u++) {
                 _cleanup_free_ char *e = NULL, *j = NULL;
@@ -493,8 +496,9 @@ static int output_units_list(const UnitInfo *unit_infos, unsigned c) {
                         if (job_count)
                                 printf("%-*s ", job_len, "JOB");
 
-                        printf("%.*s%s\n",
-                               !arg_full && arg_no_pager ? desc_len : -1,
+                        printf("%-*.*s%s\n",
+                               desc_len,
+                               !arg_full && arg_no_pager ? (int) desc_len : -1,
                                "DESCRIPTION",
                                ansi_normal());
                 }
@@ -513,13 +517,13 @@ static int output_units_list(const UnitInfo *unit_infos, unsigned c) {
                         off_circle = ansi_normal();
                         circle = true;
                         on_loaded = underline ? ansi_highlight_red_underline() : ansi_highlight_red();
-                        off_loaded = on_underline;
+                        off_loaded = underline ? on_underline : ansi_normal();
                 } else if (streq(u->active_state, "failed") && !arg_plain) {
                         on_circle = ansi_highlight_red();
                         off_circle = ansi_normal();
                         circle = true;
                         on_active = underline ? ansi_highlight_red_underline() : ansi_highlight_red();
-                        off_active = on_underline;
+                        off_active = underline ? on_underline : ansi_normal();
                 }
 
                 if (u->machine) {
@@ -550,8 +554,9 @@ static int output_units_list(const UnitInfo *unit_infos, unsigned c) {
                        sub_len, u->sub_state, off_active,
                        job_count ? job_len + 1 : 0, u->job_id ? u->job_type : "");
 
-                printf("%.*s%s\n",
-                       desc_len > 0 ? desc_len : -1,
+                printf("%-*.*s%s\n",
+                       desc_len,
+                       !arg_full && arg_no_pager ? (int) desc_len : -1,
                        u->description,
                        off_underline);
         }