load-fragment: a few modernizations

6 files changed, 52 insertions, 25 deletions

diff --git a/TODO b/TODO
index 7cc177f9f3..b42f2ff70e 100644
--- a/TODO
+++ b/TODO
@@ -22,6 +22,9 @@ Bugfixes:
 
 Features:
 
+* load-fragment: when loading a unit file via a chain of symlinks
+  verify that it isn't masked via any of the names traversed.
+
 * journald: _BOOT_ID triggers too many collisions.
 
 * journald: we currently rotate only after MaxUse+MaxFilesize has been reached.
diff --git a/src/core/load-fragment.c b/src/core/load-fragment.c
index 1ca0dece67..e0e42acbb4 100644
--- a/src/core/load-fragment.c
+++ b/src/core/load-fragment.c
@@ -2047,21 +2047,24 @@ static int open_follow(char **filename, FILE **_f, Set *names, char **_final) {
                 }
 
                 /* Try to open the file name, but don't if its a symlink */
-                if ((fd = open(*filename, O_RDONLY|O_CLOEXEC|O_NOCTTY|O_NOFOLLOW)) >= 0)
+                fd = open(*filename, O_RDONLY|O_CLOEXEC|O_NOCTTY|O_NOFOLLOW);
+                if (fd >= 0)
                         break;
 
                 if (errno != ELOOP)
                         return -errno;
 
                 /* Hmm, so this is a symlink. Let's read the name, and follow it manually */
-                if ((r = readlink_and_make_absolute(*filename, &target)) < 0)
+                r = readlink_and_make_absolute(*filename, &target);
+                if (r < 0)
                         return r;
 
                 free(*filename);
                 *filename = target;
         }
 
-        if (!(f = fdopen(fd, "re"))) {
+        f = fdopen(fd, "re");
+        if (!f) {
                 r = -errno;
                 close_nointr_nofail(fd);
                 return r;
@@ -2085,7 +2088,8 @@ static int merge_by_names(Unit **u, Set *names, const char *id) {
 
                 /* First try to merge in the other name into our
                  * unit */
-                if ((r = unit_merge_by_name(*u, k)) < 0) {
+                r = unit_merge_by_name(*u, k);
+                if (r < 0) {
                         Unit *other;
 
                         /* Hmm, we couldn't merge the other unit into
@@ -2095,11 +2099,13 @@ static int merge_by_names(Unit **u, Set *names, const char *id) {
                         other = manager_get_unit((*u)->manager, k);
                         free(k);
 
-                        if (other)
-                                if ((r = unit_merge(other, *u)) >= 0) {
+                        if (other) {
+                                r = unit_merge(other, *u);
+                                if (r >= 0) {
                                         *u = other;
                                         return merge_by_names(u, names, NULL);
                                 }
+                        }
 
                         return r;
                 }
@@ -2130,12 +2136,14 @@ static int load_from_path(Unit *u, const char *path) {
 
         if (path_is_absolute(path)) {
 
-                if (!(filename = strdup(path))) {
+                filename = strdup(path);
+                if (!filename) {
                         r = -ENOMEM;
                         goto finish;
                 }
 
-                if ((r = open_follow(&filename, &f, symlink_names, &id)) < 0) {
+                r = open_follow(&filename, &f, symlink_names, &id);
+                if (r < 0) {
                         free(filename);
                         filename = NULL;
 
@@ -2151,7 +2159,8 @@ static int load_from_path(Unit *u, const char *path) {
                         /* Instead of opening the path right away, we manually
                          * follow all symlinks and add their name to our unit
                          * name set while doing so */
-                        if (!(filename = path_make_absolute(path, *p))) {
+                        filename = path_make_absolute(path, *p);
+                        if (!filename) {
                                 r = -ENOMEM;
                                 goto finish;
                         }
@@ -2163,8 +2172,6 @@ static int load_from_path(Unit *u, const char *path) {
                                 r = open_follow(&filename, &f, symlink_names, &id);
 
                         if (r < 0) {
-                                char *sn;
-
                                 free(filename);
                                 filename = NULL;
 
@@ -2172,9 +2179,7 @@ static int load_from_path(Unit *u, const char *path) {
                                         goto finish;
 
                                 /* Empty the symlink names for the next run */
-                                while ((sn = set_steal_first(symlink_names)))
-                                        free(sn);
-
+                                set_clear_free(symlink_names);
                                 continue;
                         }
 
@@ -2189,7 +2194,8 @@ static int load_from_path(Unit *u, const char *path) {
         }
 
         merged = u;
-        if ((r = merge_by_names(&merged, symlink_names, id)) < 0)
+        r = merge_by_names(&merged, symlink_names, id);
+        if (r < 0)
                 goto finish;
 
         if (merged != u) {
@@ -2252,7 +2258,8 @@ int unit_load_fragment(Unit *u) {
         /* First, try to find the unit under its id. We always look
          * for unit files in the default directories, to make it easy
          * to override things by placing things in /etc/systemd/system */
-        if ((r = load_from_path(u, u->id)) < 0)
+        r = load_from_path(u, u->id);
+        if (r < 0)
                 return r;
 
         /* Try to find an alias we can load this with */
@@ -2262,7 +2269,8 @@ int unit_load_fragment(Unit *u) {
                         if (t == u->id)
                                 continue;
 
-                        if ((r = load_from_path(u, t)) < 0)
+                        r = load_from_path(u, t);
+                        if (r < 0)
                                 return r;
 
                         if (u->load_state != UNIT_STUB)
@@ -2272,7 +2280,8 @@ int unit_load_fragment(Unit *u) {
         /* And now, try looking for it under the suggested (originally linked) path */
         if (u->load_state == UNIT_STUB && u->fragment_path) {
 
-                if ((r = load_from_path(u, u->fragment_path)) < 0)
+                r = load_from_path(u, u->fragment_path);
+                if (r < 0)
                         return r;
 
                 if (u->load_state == UNIT_STUB) {
@@ -2288,7 +2297,8 @@ int unit_load_fragment(Unit *u) {
         if (u->load_state == UNIT_STUB && u->instance) {
                 char *k;
 
-                if (!(k = unit_name_template(u->id)))
+                k = unit_name_template(u->id);
+                if (!k)
                         return -ENOMEM;
 
                 r = load_from_path(u, k);
@@ -2303,7 +2313,8 @@ int unit_load_fragment(Unit *u) {
                                 if (t == u->id)
                                         continue;
 
-                                if (!(k = unit_name_template(t)))
+                                k = unit_name_template(t);
+                                if (!k)
                                         return -ENOMEM;
 
                                 r = load_from_path(u, k);
diff --git a/src/shared/hashmap.c b/src/shared/hashmap.c
index 5d1e63208a..ab00957306 100644
--- a/src/shared/hashmap.c
+++ b/src/shared/hashmap.c
@@ -277,11 +277,7 @@ void hashmap_free(Hashmap*h) {
 }
 
 void hashmap_free_free(Hashmap *h) {
-        void *p;
-
-        while ((p = hashmap_steal_first(h)))
-                free(p);
-
+        hashmap_clear_free(h);
         hashmap_free(h);
 }
 
@@ -293,6 +289,15 @@ void hashmap_clear(Hashmap *h) {
                 remove_entry(h, h->iterate_list_head);
 }
 
+void hashmap_clear_free(Hashmap *h) {
+        void *p;
+
+        assert(h);
+
+        while ((p = hashmap_steal_first(h)))
+                free(p);
+}
+
 static struct hashmap_entry *hash_scan(Hashmap *h, unsigned hash, const void *key) {
         struct hashmap_entry *e;
         assert(h);
diff --git a/src/shared/hashmap.h b/src/shared/hashmap.h
index fcf2cb1c81..deefd9a804 100644
--- a/src/shared/hashmap.h
+++ b/src/shared/hashmap.h
@@ -71,6 +71,8 @@ void *hashmap_iterate_backwards(Hashmap *h, Iterator *i, const void **key);
 void *hashmap_iterate_skip(Hashmap *h, const void *key, Iterator *i);
 
 void hashmap_clear(Hashmap *h);
+void hashmap_clear_free(Hashmap *h);
+
 void *hashmap_steal_first(Hashmap *h);
 void *hashmap_steal_first_key(Hashmap *h);
 void* hashmap_first(Hashmap *h);
diff --git a/src/shared/set.c b/src/shared/set.c
index 20e457eda4..f5c7c37cab 100644
--- a/src/shared/set.c
+++ b/src/shared/set.c
@@ -116,3 +116,7 @@ Set* set_copy(Set *s) {
 void set_clear(Set *s) {
         hashmap_clear(MAKE_HASHMAP(s));
 }
+
+void set_clear_free(Set *s) {
+        hashmap_clear_free(MAKE_HASHMAP(s));
+}
diff --git a/src/shared/set.h b/src/shared/set.h
index a4653b3a45..18921df427 100644
--- a/src/shared/set.h
+++ b/src/shared/set.h
@@ -56,6 +56,8 @@ void *set_iterate_backwards(Set *s, Iterator *i);
 void *set_iterate_skip(Set *s, void *value, Iterator *i);
 
 void set_clear(Set *s);
+void set_clear_free(Set *s);
+
 void *set_steal_first(Set *s);
 void* set_first(Set *s);
 void* set_last(Set *s);