[systemd-devel] [PATCH, REVIEW, v2] find_symlinks: adds a cache of enabled unit symbolic link state

Ken Sedgwick ksedgwic at bonsai.com
Fri Oct 17 16:34:25 PDT 2014


The current find_symlinks_fd code traverses the config directories
duplicatively. This is a performance problem if 1000s of units are
being controlled. This patch adds a hashmap cache of symbolic link
state which is filled in one traversal and then consulted as needed to
prevent re-traversal.

The enabled_context cache lives in the manager struct.  Initially the
cache is empty and is filled on first use.  A pointer to the cache is
passed to all routines which can call find_symlinks_fd.  If a NULL is
passed to find_symlinks_fd a temporary cache is created and used for
the single call.

The cache has two levels, the first is keyed by config_path and the
second by the names and paths of the units.

The cache contains both forward and reverse mappings; from symbolic
link name to target and from target to symbolic link name.

The cache contains entries for both the basename of the unit and the
full path of the link name and target.

The test-enabled patch (previously submitted) checks that the results
of unit_file_get_state and unit_file_get_list do not change as a
result of this cache. This patch presumes the test-enabled patch has
already been applied.

The test-manyunits patch (previously submitted) checks that the performance
of unit_file_get_list is acceptable in the face of thousands of units.
This patch presumes the test-manyunits patch has already been applied.
---
 src/core/dbus-manager.c   |   6 +-
 src/core/manager.c        |   6 +
 src/core/manager.h        |   2 +
 src/core/unit.c           |   2 +-
 src/shared/install.c      | 273 +++++++++++++++++++++++++++++++++++++---------
 src/shared/install.h      |  18 ++-
 src/systemctl/systemctl.c |   6 +-
 src/test/test-enabled.c   |  24 ++--
 src/test/test-install.c   |  87 ++++++++-------
 src/test/test-manyunits.c |  11 +-
 src/test/test-unit-file.c |   2 +-
 11 files changed, 325 insertions(+), 112 deletions(-)

diff --git a/src/core/dbus-manager.c b/src/core/dbus-manager.c
index 57db1c9..0ce41b0 100644
--- a/src/core/dbus-manager.c
+++ b/src/core/dbus-manager.c
@@ -1403,7 +1403,7 @@ static int method_list_unit_files(sd_bus *bus, sd_bus_message *message, void *us
         if (!h)
                 return -ENOMEM;
 
-        r = unit_file_get_list(m->running_as == SYSTEMD_SYSTEM ? UNIT_FILE_SYSTEM : UNIT_FILE_USER, NULL, h);
+        r = unit_file_get_list(m->running_as == SYSTEMD_SYSTEM ? UNIT_FILE_SYSTEM : UNIT_FILE_USER, NULL, h, m->enabled);
         if (r < 0)
                 goto fail;
 
@@ -1454,7 +1454,7 @@ static int method_get_unit_file_state(sd_bus *bus, sd_bus_message *message, void
 
         scope = m->running_as == SYSTEMD_SYSTEM ? UNIT_FILE_SYSTEM : UNIT_FILE_USER;
 
-        state = unit_file_get_state(scope, NULL, name);
+        state = unit_file_get_state(scope, NULL, name, m->enabled);
         if (state < 0)
                 return state;
 
@@ -1843,7 +1843,7 @@ static int method_add_dependency_unit_files(sd_bus *bus, sd_bus_message *message
 
         scope = m->running_as == SYSTEMD_SYSTEM ? UNIT_FILE_SYSTEM : UNIT_FILE_USER;
 
-        r = unit_file_add_dependency(scope, runtime, NULL, l, target, dep, force, &changes, &n_changes);
+        r = unit_file_add_dependency(scope, runtime, NULL, l, target, dep, force, NULL, &changes, &n_changes);
         if (r < 0)
                 return r;
 
diff --git a/src/core/manager.c b/src/core/manager.c
index 726977f..f8f41fb 100644
--- a/src/core/manager.c
+++ b/src/core/manager.c
@@ -465,6 +465,10 @@ int manager_new(SystemdRunningAs running_as, bool test_run, Manager **_m) {
         if (r < 0)
                 goto fail;
 
+        m->enabled = enabled_context_new();
+        if (!m->enabled)
+                goto fail;
+
         r = set_ensure_allocated(&m->startup_units, NULL);
         if (r < 0)
                 goto fail;
@@ -822,6 +826,8 @@ void manager_free(Manager *m) {
         hashmap_free(m->watch_pids2);
         hashmap_free(m->watch_bus);
 
+        enabled_context_free(m->enabled);
+
         set_free(m->startup_units);
         set_free(m->failed_units);
 
diff --git a/src/core/manager.h b/src/core/manager.h
index 8e3c146..3f54fe0 100644
--- a/src/core/manager.h
+++ b/src/core/manager.h
@@ -72,6 +72,7 @@ typedef enum ManagerExitCode {
 #include "unit-name.h"
 #include "exit-status.h"
 #include "show-status.h"
+#include "install.h"
 #include "failure-action.h"
 
 struct Manager {
@@ -82,6 +83,7 @@ struct Manager {
         /* Active jobs and units */
         Hashmap *units;  /* name string => Unit object n:1 */
         Hashmap *jobs;   /* job id => Job object 1:1 */
+        EnabledContext *enabled; /* name string => is enabled */
 
         /* To make it easy to iterate through the units of a specific
          * type we maintain a per type linked list */
diff --git a/src/core/unit.c b/src/core/unit.c
index 0389e6e..3e29944 100644
--- a/src/core/unit.c
+++ b/src/core/unit.c
@@ -2941,7 +2941,7 @@ UnitFileState unit_get_unit_file_state(Unit *u) {
         if (u->unit_file_state < 0 && u->fragment_path)
                 u->unit_file_state = unit_file_get_state(
                                 u->manager->running_as == SYSTEMD_SYSTEM ? UNIT_FILE_SYSTEM : UNIT_FILE_USER,
-                                NULL, basename(u->fragment_path));
+                                NULL, basename(u->fragment_path), u->manager->enabled);
 
         return u->unit_file_state;
 }
diff --git a/src/shared/install.c b/src/shared/install.c
index ff5dcba..dfc1944 100644
--- a/src/shared/install.c
+++ b/src/shared/install.c
@@ -25,6 +25,7 @@
 #include <string.h>
 #include <fnmatch.h>
 
+#include "macro.h"
 #include "util.h"
 #include "mkdir.h"
 #include "hashmap.h"
@@ -382,21 +383,106 @@ static int remove_marked_symlinks(
         return r;
 }
 
-static int find_symlinks_fd(
-                const char *name,
+EnabledContext *enabled_context_new(void) {
+        EnabledContext *ec;
+        int r;
+
+        ec = new0(EnabledContext, 1);
+        if (!ec)
+                return NULL;
+
+        r = hashmap_ensure_allocated(&ec->config_paths_forward, &string_hash_ops);
+        if (r < 0) {
+                free(ec);
+                return NULL;
+        }
+
+        r = hashmap_ensure_allocated(&ec->config_paths_reverse, &string_hash_ops);
+        if (r < 0) {
+                free(ec);
+                return NULL;
+        }
+
+        return ec;
+}
+
+void enabled_context_free(EnabledContext *ec) {
+        char *key;
+        Hashmap *h;
+
+        while ((key = hashmap_first_key(ec->config_paths_forward))) {
+                h = hashmap_steal_first(ec->config_paths_forward);
+                hashmap_free_free_free(h);
+                free(key);
+        }
+        hashmap_free_free_free(ec->config_paths_forward);
+        ec->config_paths_forward = NULL;
+
+        while ((key = hashmap_first_key(ec->config_paths_reverse))) {
+                h = hashmap_steal_first(ec->config_paths_reverse);
+                hashmap_free_free_free(h);
+                free(key);
+        }
+        hashmap_free_free_free(ec->config_paths_reverse);
+        ec->config_paths_reverse = NULL;
+
+        free(ec);
+}
+
+static int insert_link_mapping(Hashmap *h, const char * key, const char *val) {
+        int r = 0;
+        char *keycpy, *valcpy;
+
+        /* Make copies of the key and value */
+        keycpy = strdup(key);
+        valcpy = strdup(val);
+
+        r = hashmap_put(h, keycpy, valcpy);
+        if (r == 1) {
+                /* Our copies were inserted, we're done */
+                return 0;
+        } else {
+                /* Either an error or the mapping already existed, either way
+                 * we don't want the key and value copies any more ...
+                 */
+                free(keycpy);
+                free(valcpy);
+                return r;
+        }
+}
+
+static int fill_enabled_context(
                 int fd,
                 const char *path,
                 const char *config_path,
-                bool *same_name_link) {
-
+                EnabledContext *ec) {
         int r = 0;
         _cleanup_closedir_ DIR *d = NULL;
-
-        assert(name);
-        assert(fd >= 0);
-        assert(path);
-        assert(config_path);
-        assert(same_name_link);
+        Hashmap *config_path_forward;
+        Hashmap *config_path_reverse;
+
+        config_path_forward = hashmap_get(ec->config_paths_forward, config_path);
+        config_path_reverse = hashmap_get(ec->config_paths_reverse, config_path);
+
+        /* If config_path_forward isn't cached, generate both directions */
+        if (config_path_forward == NULL) {
+                /* Initialize empty forward and reverse lookups for the
+                 * enabled context cache */
+                r = hashmap_ensure_allocated(&config_path_forward, &string_hash_ops);
+                if (r < 0) {
+                        return r;
+                }
+                r = hashmap_put(ec->config_paths_forward, strdup(config_path), config_path_forward);
+                if (r < 0)
+                        return r;
+                r = hashmap_ensure_allocated(&config_path_reverse, &string_hash_ops);
+                if (r < 0) {
+                        return r;
+                }
+                r = hashmap_put(ec->config_paths_reverse, strdup(config_path), config_path_reverse);
+                if (r < 0)
+                        return r;
+        }
 
         d = fdopendir(fd);
         if (!d) {
@@ -413,7 +499,7 @@ static int find_symlinks_fd(
                         return -errno;
 
                 if (!de)
-                        return r;
+                        break;
 
                 if (ignore_file(de->d_name))
                         continue;
@@ -441,15 +527,12 @@ static int find_symlinks_fd(
                         }
 
                         /* This will close nfd, regardless whether it succeeds or not */
-                        q = find_symlinks_fd(name, nfd, p, config_path, same_name_link);
-                        if (q > 0)
-                                return 1;
+                        q = fill_enabled_context(nfd, p, config_path, ec);
                         if (r == 0)
                                 r = q;
 
                 } else if (de->d_type == DT_LNK) {
                         _cleanup_free_ char *p = NULL, *dest = NULL;
-                        bool found_path, found_dest, b = false;
                         int q;
 
                         /* Acquire symlink name */
@@ -468,44 +551,121 @@ static int find_symlinks_fd(
                                 continue;
                         }
 
-                        /* Check if the symlink itself matches what we
-                         * are looking for */
-                        if (path_is_absolute(name))
-                                found_path = path_equal(p, name);
-                        else
-                                found_path = streq(de->d_name, name);
+                        /* Insert symlink's own full path and
+                         * name as keys pointing to the unit's
+                         * full path */
+                        q = insert_link_mapping(config_path_forward, p, dest);
+                        if (r == 0)
+                                r = q;
+                        q = insert_link_mapping(config_path_forward, de->d_name, dest);
+                        if (r == 0)
+                                r = q;
 
-                        /* Check if what the symlink points to
-                         * matches what we are looking for */
-                        if (path_is_absolute(name))
-                                found_dest = path_equal(dest, name);
-                        else
-                                found_dest = streq(basename(dest), name);
+                        /* Insert full path and base of the
+                         * symlink target as keys pointing to
+                         * the symlink's own full path */
+                        q = insert_link_mapping(config_path_reverse, dest, p);
+                        if (r == 0)
+                                r = q;
+                        q = insert_link_mapping(config_path_reverse, basename(dest), p);
+                        if (r == 0)
+                                r = q;
+                }
+        }
 
-                        if (found_path && found_dest) {
-                                _cleanup_free_ char *t = NULL;
+        return r;
+}
 
-                                /* Filter out same name links in the main
-                                 * config path */
-                                t = path_make_absolute(name, config_path);
-                                if (!t)
-                                        return -ENOMEM;
+static int find_symlinks_fd(
+                const char *name,
+                int fd,
+                const char *path,
+                const char *config_path,
+                bool *same_name_link,
+                EnabledContext *ec) {
 
-                                b = path_equal(t, p);
-                        }
+        int r = 0;
+        _cleanup_closedir_ DIR *d = NULL;
+        _cleanup_enabled_context_ EnabledContext *temp_ec = NULL;
+        char *symlink_path;
+        char *symlink_target_path;
+        Hashmap *config_path_forward;
+        Hashmap *config_path_reverse;
+        bool found_path, found_dest, b = false;
 
-                        if (b)
-                                *same_name_link = true;
-                        else if (found_path || found_dest)
-                                return 1;
-                }
+        assert(name);
+        assert(fd >= 0);
+        assert(path);
+        assert(config_path);
+        assert(same_name_link);
+
+        /* If no ec is passed in, use a temporary one that auto-frees */
+        if (ec == NULL) {
+                ec = enabled_context_new();
+                if (ec == NULL)
+                        return -ENOMEM;
+                temp_ec = ec;
+        }
+
+        config_path_forward = hashmap_get(ec->config_paths_forward, config_path);
+
+        /* If config_path_forward isn't cached, generate both directions */
+        if (config_path_forward == NULL) {
+                r = fill_enabled_context(fd, path, config_path, ec);
+                if (r < 0)
+                        return r;
+        } else {
+                safe_close(fd);
+        }
+
+        /* Refetch, may have been filled. */
+        config_path_forward = hashmap_get(ec->config_paths_forward, config_path);
+        config_path_reverse = hashmap_get(ec->config_paths_reverse, config_path);
+
+        /* Use the cache to perform a reverse lookup of symlink
+         * destinations to see if any one matches what we are looking
+         * for. Because both full and base names are keys, there is no
+         * need to check if "name" is a full path or base name. */
+        symlink_path = hashmap_get(config_path_reverse, name);
+        found_dest = (symlink_path != NULL);
+
+        /* Choose a strategy to determine if the unit's name matches the
+         * symlink's name. If the reverse lookup succeeded, we can skip
+         * the forward lookup. */
+        if (found_dest) {
+                if (path_is_absolute(name))
+                        found_path = path_equal(symlink_path, name);
+                else
+                        found_path = streq(basename(symlink_path), name);
+        } else {
+                symlink_target_path = hashmap_get(config_path_forward, name);
+                found_path = (symlink_target_path != NULL);
         }
+
+        if (found_path && found_dest) {
+                _cleanup_free_ char *t = NULL;
+
+                /* Filter out same name links in the main config path */
+                t = path_make_absolute(name, config_path);
+                if (!t)
+                        return -ENOMEM;
+
+                b = path_equal(t, symlink_path);
+        }
+
+        if (b)
+                *same_name_link = true;
+        else if (found_path || found_dest)
+                return 1;
+
+        return 0;
 }
 
 static int find_symlinks(
                 const char *name,
                 const char *config_path,
-                bool *same_name_link) {
+                bool *same_name_link,
+                EnabledContext *ec) {
 
         int fd;
 
@@ -521,14 +681,15 @@ static int find_symlinks(
         }
 
         /* This takes possession of fd and closes it */
-        return find_symlinks_fd(name, fd, config_path, config_path, same_name_link);
+        return find_symlinks_fd(name, fd, config_path, config_path, same_name_link, ec);
 }
 
 static int find_symlinks_in_scope(
                 UnitFileScope scope,
                 const char *root_dir,
                 const char *name,
-                UnitFileState *state) {
+                UnitFileState *state,
+                EnabledContext *ec) {
 
         int r;
         _cleanup_free_ char *path = NULL;
@@ -544,7 +705,7 @@ static int find_symlinks_in_scope(
         if (r < 0)
                 return r;
 
-        r = find_symlinks(name, path, &same_name_link_runtime);
+        r = find_symlinks(name, path, &same_name_link_runtime, ec);
         if (r < 0)
                 return r;
         else if (r > 0) {
@@ -557,7 +718,7 @@ static int find_symlinks_in_scope(
         if (r < 0)
                 return r;
 
-        r = find_symlinks(name, path, &same_name_link);
+        r = find_symlinks(name, path, &same_name_link, ec);
         if (r < 0)
                 return r;
         else if (r > 0) {
@@ -1504,6 +1665,7 @@ int unit_file_add_dependency(
                 char *target,
                 UnitDependency dep,
                 bool force,
+                EnabledContext *ec,
                 UnitFileChange **changes,
                 unsigned *n_changes) {
 
@@ -1528,7 +1690,7 @@ int unit_file_add_dependency(
         STRV_FOREACH(i, files) {
                 UnitFileState state;
 
-                state = unit_file_get_state(scope, root_dir, *i);
+                state = unit_file_get_state(scope, root_dir, *i, ec);
                 if (state < 0) {
                         log_error("Failed to get unit file state for %s: %s", *i, strerror(-state));
                         return state;
@@ -1602,7 +1764,7 @@ int unit_file_enable(
         STRV_FOREACH(i, files) {
                 UnitFileState state;
 
-                state = unit_file_get_state(scope, root_dir, *i);
+                state = unit_file_get_state(scope, root_dir, *i, NULL);
                 if (state < 0) {
                         log_error("Failed to get unit file state for %s: %s", *i, strerror(-state));
                         return state;
@@ -1784,7 +1946,8 @@ int unit_file_get_default(
 UnitFileState unit_file_get_state(
                 UnitFileScope scope,
                 const char *root_dir,
-                const char *name) {
+                const char *name,
+                EnabledContext *ec) {
 
         _cleanup_lookup_paths_free_ LookupPaths paths = {};
         UnitFileState state = _UNIT_FILE_STATE_INVALID;
@@ -1847,7 +2010,7 @@ UnitFileState unit_file_get_state(
                         }
                 }
 
-                r = find_symlinks_in_scope(scope, root_dir, name, &state);
+                r = find_symlinks_in_scope(scope, root_dir, name, &state, ec);
                 if (r < 0)
                         return r;
                 else if (r > 0)
@@ -2130,7 +2293,8 @@ DEFINE_TRIVIAL_CLEANUP_FUNC(UnitFileList*, unit_file_list_free_one);
 int unit_file_get_list(
                 UnitFileScope scope,
                 const char *root_dir,
-                Hashmap *h) {
+                Hashmap *h,
+                EnabledContext *ec) {
 
         _cleanup_lookup_paths_free_ LookupPaths paths = {};
         char **i;
@@ -2214,7 +2378,7 @@ int unit_file_get_list(
                                 goto found;
                         }
 
-                        r = find_symlinks_in_scope(scope, root_dir, de->d_name, &f->state);
+                        r = find_symlinks_in_scope(scope, root_dir, de->d_name, &f->state, ec);
                         if (r < 0)
                                 return r;
                         else if (r > 0) {
@@ -2246,7 +2410,10 @@ int unit_file_get_list(
                 }
         }
 
-        return r;
+        /* The 1 that hashmap_put in the "found" section returns on
+         * successful put can get here.  No valid error r value can
+         * get here. */
+        return 0;
 }
 
 static const char* const unit_file_state_table[_UNIT_FILE_STATE_MAX] = {
diff --git a/src/shared/install.h b/src/shared/install.h
index c0b4df6..df7b488 100644
--- a/src/shared/install.h
+++ b/src/shared/install.h
@@ -84,6 +84,11 @@ typedef struct {
         char *default_instance;
 } InstallInfo;
 
+typedef struct {
+        Hashmap* config_paths_forward; /* config_path string => symlink name/path => unit_name path */
+        Hashmap* config_paths_reverse; /* config_path string => unit_name/path string => symlink path */
+} EnabledContext;
+
 int unit_file_enable(UnitFileScope scope, bool runtime, const char *root_dir, char **files, bool force, UnitFileChange **changes, unsigned *n_changes);
 int unit_file_disable(UnitFileScope scope, bool runtime, const char *root_dir, char **files, UnitFileChange **changes, unsigned *n_changes);
 int unit_file_reenable(UnitFileScope scope, bool runtime, const char *root_dir, char **files, bool force, UnitFileChange **changes, unsigned *n_changes);
@@ -94,11 +99,11 @@ int unit_file_mask(UnitFileScope scope, bool runtime, const char *root_dir, char
 int unit_file_unmask(UnitFileScope scope, bool runtime, const char *root_dir, char **files, UnitFileChange **changes, unsigned *n_changes);
 int unit_file_set_default(UnitFileScope scope, const char *root_dir, const char *file, bool force, UnitFileChange **changes, unsigned *n_changes);
 int unit_file_get_default(UnitFileScope scope, const char *root_dir, char **name);
-int unit_file_add_dependency(UnitFileScope scope, bool runtime, const char *root_dir, char **files, char *target, UnitDependency dep, bool force, UnitFileChange **changes, unsigned *n_changes);
+int unit_file_add_dependency(UnitFileScope scope, bool runtime, const char *root_dir, char **files, char *target, UnitDependency dep, bool force, EnabledContext *ec, UnitFileChange **changes, unsigned *n_changes);
 
-UnitFileState unit_file_get_state(UnitFileScope scope, const char *root_dir, const char *filename);
+UnitFileState unit_file_get_state(UnitFileScope scope, const char *root_dir, const char *filename, EnabledContext *ec);
 
-int unit_file_get_list(UnitFileScope scope, const char *root_dir, Hashmap *h);
+int unit_file_get_list(UnitFileScope scope, const char *root_dir, Hashmap *h, EnabledContext *ec);
 
 void unit_file_list_free(Hashmap *h);
 void unit_file_changes_free(UnitFileChange *changes, unsigned n_changes);
@@ -111,5 +116,12 @@ UnitFileState unit_file_state_from_string(const char *s) _pure_;
 const char *unit_file_change_type_to_string(UnitFileChangeType s) _const_;
 UnitFileChangeType unit_file_change_type_from_string(const char *s) _pure_;
 
+EnabledContext *enabled_context_new(void);
+void enabled_context_free(EnabledContext *ec);
+
+DEFINE_TRIVIAL_CLEANUP_FUNC(EnabledContext*, enabled_context_free);
+
+#define _cleanup_enabled_context_ _cleanup_(enabled_context_freep)
+
 const char *unit_file_preset_mode_to_string(UnitFilePresetMode m) _const_;
 UnitFilePresetMode unit_file_preset_mode_from_string(const char *s) _pure_;
diff --git a/src/systemctl/systemctl.c b/src/systemctl/systemctl.c
index 28eaa6a..a42af8b 100644
--- a/src/systemctl/systemctl.c
+++ b/src/systemctl/systemctl.c
@@ -1325,7 +1325,7 @@ static int list_unit_files(sd_bus *bus, char **args) {
                 if (!h)
                         return log_oom();
 
-                r = unit_file_get_list(arg_scope, arg_root, h);
+                r = unit_file_get_list(arg_scope, arg_root, h, NULL);
                 if (r < 0) {
                         unit_file_list_free(h);
                         log_error("Failed to get unit file list: %s", strerror(-r));
@@ -5420,7 +5420,7 @@ static int add_dependency(sd_bus *bus, char **args) {
                 UnitFileChange *changes = NULL;
                 unsigned n_changes = 0;
 
-                r = unit_file_add_dependency(arg_scope, arg_runtime, arg_root, names, target, dep, arg_force, &changes, &n_changes);
+                r = unit_file_add_dependency(arg_scope, arg_runtime, arg_root, names, target, dep, arg_force, NULL, &changes, &n_changes);
 
                 if (r < 0) {
                         log_error("Can't add dependency: %s", strerror(-r));
@@ -5567,7 +5567,7 @@ static int unit_is_enabled(sd_bus *bus, char **args) {
                 STRV_FOREACH(name, names) {
                         UnitFileState state;
 
-                        state = unit_file_get_state(arg_scope, arg_root, *name);
+                        state = unit_file_get_state(arg_scope, arg_root, *name, NULL);
                         if (state < 0) {
                                 log_error("Failed to get unit file state for %s: %s", *name, strerror(-state));
                                 return state;
diff --git a/src/test/test-enabled.c b/src/test/test-enabled.c
index 104348e..fa5d596 100644
--- a/src/test/test-enabled.c
+++ b/src/test/test-enabled.c
@@ -76,9 +76,9 @@
 
 
 #define confirm_unit_state(unit, expected)                              \
-        assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, root_dir, unit) == expected)
+        assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, root_dir, unit, ec) == expected)
 
-static void test_enabled(int argc, char* argv[]) {
+static void test_enabled(int argc, char* argv[], EnabledContext *ec) {
         Hashmap *h;
         UnitFileList *p;
         Iterator i;
@@ -87,6 +87,7 @@ static void test_enabled(int argc, char* argv[]) {
 
         root_dir = strappenda(TEST_DIR, "/test-enabled-root");
 
+        /* Explicitly check each of the units. */
         confirm_unit_state("nonexistent.service",	-ENOENT);
         confirm_unit_state("invalid.service", 		-EBADMSG);
         confirm_unit_state("disabled.service", 		UNIT_FILE_DISABLED);
@@ -102,15 +103,15 @@ static void test_enabled(int argc, char* argv[]) {
         confirm_unit_state("templating at three.service",	UNIT_FILE_ENABLED);
         confirm_unit_state("unique.service", 		UNIT_FILE_ENABLED);
 
+        /* Reconcile unit_file_get_list with the return for each unit. */
         h = hashmap_new(&string_hash_ops);
-        r = unit_file_get_list(UNIT_FILE_SYSTEM, root_dir, h);
-        assert_se(r >= 0);
-
+        r = unit_file_get_list(UNIT_FILE_SYSTEM, root_dir, h, ec);
+        assert_se(r == 0);
         HASHMAP_FOREACH(p, h, i) {
                 UnitFileState s;
 
                 s = unit_file_get_state(UNIT_FILE_SYSTEM, root_dir,
-                                        basename(p->path));
+                                        basename(p->path), ec);
 
                 /* unit_file_get_list and unit_file_get_state are
                  * a little different in some cases.  Handle these
@@ -136,6 +137,15 @@ static void test_enabled(int argc, char* argv[]) {
 }
 
 int main(int argc, char* argv[]) {
-        test_enabled(argc, argv);
+        _cleanup_enabled_context_ EnabledContext *ec = NULL;
+
+        /* built-in EnabledContext */
+        test_enabled(argc, argv, NULL);
+
+        /* explicit EnabledContext */
+        ec = enabled_context_new();
+        assert(ec);
+        test_enabled(argc, argv, ec);
+
         return 0;
 }
diff --git a/src/test/test-install.c b/src/test/test-install.c
index 467970b..0c8c05b 100644
--- a/src/test/test-install.c
+++ b/src/test/test-install.c
@@ -41,24 +41,25 @@ static void dump_changes(UnitFileChange *c, unsigned n) {
         }
 }
 
-int main(int argc, char* argv[]) {
+static void test_install(int argc, char* argv[], EnabledContext *ec) {
         Hashmap *h;
         UnitFileList *p;
         Iterator i;
         int r;
-        const char *const files[] = { "avahi-daemon.service", NULL };
+        const char *root_dir = argv[1] ?: NULL;
+        const char *const files[] = { "systemd-timesyncd.service", NULL };
         const char *const files2[] = { "/home/lennart/test.service", NULL };
         UnitFileChange *changes = NULL;
         unsigned n_changes = 0;
 
         h = hashmap_new(&string_hash_ops);
-        r = unit_file_get_list(UNIT_FILE_SYSTEM, NULL, h);
+        r = unit_file_get_list(UNIT_FILE_SYSTEM, root_dir, h, ec);
         assert_se(r == 0);
 
         HASHMAP_FOREACH(p, h, i) {
                 UnitFileState s;
 
-                s = unit_file_get_state(UNIT_FILE_SYSTEM, NULL, basename(p->path));
+                s = unit_file_get_state(UNIT_FILE_SYSTEM, root_dir, basename(p->path), ec);
 
                 assert_se(p->state == s);
 
@@ -71,195 +72,205 @@ int main(int argc, char* argv[]) {
 
         log_error("enable");
 
-        r = unit_file_enable(UNIT_FILE_SYSTEM, false, NULL, (char**) files, false, &changes, &n_changes);
+        r = unit_file_enable(UNIT_FILE_SYSTEM, false, root_dir, (char**) files, false, &changes, &n_changes);
         assert_se(r >= 0);
 
         log_error("enable2");
 
-        r = unit_file_enable(UNIT_FILE_SYSTEM, false, NULL, (char**) files, false, &changes, &n_changes);
+        r = unit_file_enable(UNIT_FILE_SYSTEM, false, root_dir, (char**) files, false, &changes, &n_changes);
         assert_se(r >= 0);
 
         dump_changes(changes, n_changes);
         unit_file_changes_free(changes, n_changes);
 
-        assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, NULL, files[0]) == UNIT_FILE_ENABLED);
+        assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, root_dir, files[0], ec) == UNIT_FILE_ENABLED);
 
         log_error("disable");
 
         changes = NULL;
         n_changes = 0;
 
-        r = unit_file_disable(UNIT_FILE_SYSTEM, false, NULL, (char**) files, &changes, &n_changes);
+        r = unit_file_disable(UNIT_FILE_SYSTEM, false, root_dir, (char**) files, &changes, &n_changes);
         assert_se(r >= 0);
 
         dump_changes(changes, n_changes);
         unit_file_changes_free(changes, n_changes);
 
-        assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, NULL, files[0]) == UNIT_FILE_DISABLED);
+        assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, root_dir, files[0], ec) == UNIT_FILE_DISABLED);
 
         log_error("mask");
         changes = NULL;
         n_changes = 0;
 
-        r = unit_file_mask(UNIT_FILE_SYSTEM, false, NULL, (char**) files, false, &changes, &n_changes);
+        r = unit_file_mask(UNIT_FILE_SYSTEM, false, root_dir, (char**) files, false, &changes, &n_changes);
         assert_se(r >= 0);
         log_error("mask2");
-        r = unit_file_mask(UNIT_FILE_SYSTEM, false, NULL, (char**) files, false, &changes, &n_changes);
+        r = unit_file_mask(UNIT_FILE_SYSTEM, false, root_dir, (char**) files, false, &changes, &n_changes);
         assert_se(r >= 0);
 
         dump_changes(changes, n_changes);
         unit_file_changes_free(changes, n_changes);
 
-        assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, NULL, files[0]) == UNIT_FILE_MASKED);
+        assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, root_dir, files[0], ec) == UNIT_FILE_MASKED);
 
         log_error("unmask");
         changes = NULL;
         n_changes = 0;
 
-        r = unit_file_unmask(UNIT_FILE_SYSTEM, false, NULL, (char**) files, &changes, &n_changes);
+        r = unit_file_unmask(UNIT_FILE_SYSTEM, false, root_dir, (char**) files, &changes, &n_changes);
         assert_se(r >= 0);
         log_error("unmask2");
-        r = unit_file_unmask(UNIT_FILE_SYSTEM, false, NULL, (char**) files, &changes, &n_changes);
+        r = unit_file_unmask(UNIT_FILE_SYSTEM, false, root_dir, (char**) files, &changes, &n_changes);
         assert_se(r >= 0);
 
         dump_changes(changes, n_changes);
         unit_file_changes_free(changes, n_changes);
 
-        assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, NULL, files[0]) == UNIT_FILE_DISABLED);
+        assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, root_dir, files[0], ec) == UNIT_FILE_DISABLED);
 
         log_error("mask");
         changes = NULL;
         n_changes = 0;
 
-        r = unit_file_mask(UNIT_FILE_SYSTEM, false, NULL, (char**) files, false, &changes, &n_changes);
+        r = unit_file_mask(UNIT_FILE_SYSTEM, false, root_dir, (char**) files, false, &changes, &n_changes);
         assert_se(r >= 0);
 
         dump_changes(changes, n_changes);
         unit_file_changes_free(changes, n_changes);
 
-        assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, NULL, files[0]) == UNIT_FILE_MASKED);
+        assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, root_dir, files[0], ec) == UNIT_FILE_MASKED);
 
         log_error("disable");
         changes = NULL;
         n_changes = 0;
 
-        r = unit_file_disable(UNIT_FILE_SYSTEM, false, NULL, (char**) files, &changes, &n_changes);
+        r = unit_file_disable(UNIT_FILE_SYSTEM, false, root_dir, (char**) files, &changes, &n_changes);
         assert_se(r >= 0);
         log_error("disable2");
-        r = unit_file_disable(UNIT_FILE_SYSTEM, false, NULL, (char**) files, &changes, &n_changes);
+        r = unit_file_disable(UNIT_FILE_SYSTEM, false, root_dir, (char**) files, &changes, &n_changes);
         assert_se(r >= 0);
 
         dump_changes(changes, n_changes);
         unit_file_changes_free(changes, n_changes);
 
-        assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, NULL, files[0]) == UNIT_FILE_MASKED);
+        assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, root_dir, files[0], ec) == UNIT_FILE_MASKED);
 
         log_error("umask");
         changes = NULL;
         n_changes = 0;
 
-        r = unit_file_unmask(UNIT_FILE_SYSTEM, false, NULL, (char**) files, &changes, &n_changes);
+        r = unit_file_unmask(UNIT_FILE_SYSTEM, false, root_dir, (char**) files, &changes, &n_changes);
         assert_se(r >= 0);
 
         dump_changes(changes, n_changes);
         unit_file_changes_free(changes, n_changes);
 
-        assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, NULL, files[0]) == UNIT_FILE_DISABLED);
+        assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, root_dir, files[0], ec) == UNIT_FILE_DISABLED);
 
         log_error("enable files2");
         changes = NULL;
         n_changes = 0;
 
-        r = unit_file_enable(UNIT_FILE_SYSTEM, false, NULL, (char**) files2, false, &changes, &n_changes);
+        r = unit_file_enable(UNIT_FILE_SYSTEM, false, root_dir, (char**) files2, false, &changes, &n_changes);
         assert_se(r >= 0);
 
         dump_changes(changes, n_changes);
         unit_file_changes_free(changes, n_changes);
 
-        assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, NULL, basename(files2[0])) == UNIT_FILE_ENABLED);
+        assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, root_dir, basename(files2[0]), ec) == UNIT_FILE_ENABLED);
 
         log_error("disable files2");
         changes = NULL;
         n_changes = 0;
 
-        r = unit_file_disable(UNIT_FILE_SYSTEM, false, NULL, (char**) files2, &changes, &n_changes);
+        r = unit_file_disable(UNIT_FILE_SYSTEM, false, root_dir, (char**) files2, &changes, &n_changes);
         assert_se(r >= 0);
 
         dump_changes(changes, n_changes);
         unit_file_changes_free(changes, n_changes);
 
-        assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, NULL, basename(files2[0])) == _UNIT_FILE_STATE_INVALID);
+        assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, root_dir, basename(files2[0]), ec) == _UNIT_FILE_STATE_INVALID);
 
         log_error("link files2");
         changes = NULL;
         n_changes = 0;
 
-        r = unit_file_link(UNIT_FILE_SYSTEM, false, NULL, (char**) files2, false, &changes, &n_changes);
+        r = unit_file_link(UNIT_FILE_SYSTEM, false, root_dir, (char**) files2, false, &changes, &n_changes);
         assert_se(r >= 0);
 
         dump_changes(changes, n_changes);
         unit_file_changes_free(changes, n_changes);
 
-        assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, NULL, basename(files2[0])) == UNIT_FILE_LINKED);
+        assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, root_dir, basename(files2[0]), ec) == UNIT_FILE_LINKED);
 
         log_error("disable files2");
         changes = NULL;
         n_changes = 0;
 
-        r = unit_file_disable(UNIT_FILE_SYSTEM, false, NULL, (char**) files2, &changes, &n_changes);
+        r = unit_file_disable(UNIT_FILE_SYSTEM, false, root_dir, (char**) files2, &changes, &n_changes);
         assert_se(r >= 0);
 
         dump_changes(changes, n_changes);
         unit_file_changes_free(changes, n_changes);
 
-        assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, NULL, basename(files2[0])) == _UNIT_FILE_STATE_INVALID);
+        assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, root_dir, basename(files2[0]), ec) == _UNIT_FILE_STATE_INVALID);
 
         log_error("link files2");
         changes = NULL;
         n_changes = 0;
 
-        r = unit_file_link(UNIT_FILE_SYSTEM, false, NULL, (char**) files2, false, &changes, &n_changes);
+        r = unit_file_link(UNIT_FILE_SYSTEM, false, root_dir, (char**) files2, false, &changes, &n_changes);
         assert_se(r >= 0);
 
         dump_changes(changes, n_changes);
         unit_file_changes_free(changes, n_changes);
 
-        assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, NULL, basename(files2[0])) == UNIT_FILE_LINKED);
+        assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, root_dir, basename(files2[0]), ec) == UNIT_FILE_LINKED);
 
         log_error("reenable files2");
         changes = NULL;
         n_changes = 0;
 
-        r = unit_file_reenable(UNIT_FILE_SYSTEM, false, NULL, (char**) files2, false, &changes, &n_changes);
+        r = unit_file_reenable(UNIT_FILE_SYSTEM, false, root_dir, (char**) files2, false, &changes, &n_changes);
         assert_se(r >= 0);
 
         dump_changes(changes, n_changes);
         unit_file_changes_free(changes, n_changes);
 
-        assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, NULL, basename(files2[0])) == UNIT_FILE_ENABLED);
+        assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, root_dir, basename(files2[0]), ec) == UNIT_FILE_ENABLED);
 
         log_error("disable files2");
         changes = NULL;
         n_changes = 0;
 
-        r = unit_file_disable(UNIT_FILE_SYSTEM, false, NULL, (char**) files2, &changes, &n_changes);
+        r = unit_file_disable(UNIT_FILE_SYSTEM, false, root_dir, (char**) files2, &changes, &n_changes);
         assert_se(r >= 0);
 
         dump_changes(changes, n_changes);
         unit_file_changes_free(changes, n_changes);
 
-        assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, NULL, basename(files2[0])) == _UNIT_FILE_STATE_INVALID);
+        assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, root_dir, basename(files2[0]), ec) == _UNIT_FILE_STATE_INVALID);
         log_error("preset files");
         changes = NULL;
         n_changes = 0;
 
-        r = unit_file_preset(UNIT_FILE_SYSTEM, false, NULL, (char**) files, UNIT_FILE_PRESET_FULL, false, &changes, &n_changes);
+        r = unit_file_preset(UNIT_FILE_SYSTEM, false, root_dir, (char**) files, UNIT_FILE_PRESET_FULL, false, &changes, &n_changes);
         assert_se(r >= 0);
 
         dump_changes(changes, n_changes);
         unit_file_changes_free(changes, n_changes);
 
-        assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, NULL, basename(files[0])) == UNIT_FILE_ENABLED);
+        assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, root_dir, basename(files[0]), ec) == UNIT_FILE_ENABLED);
+}
+
+int main(int argc, char* argv[]) {
+        EnabledContext *ec;
+
+        test_install(argc, argv, NULL);
+
+        ec = enabled_context_new();
+        assert(ec);
+        test_install(argc, argv, ec);
 
         return 0;
 }
diff --git a/src/test/test-manyunits.c b/src/test/test-manyunits.c
index e76f04c..f43fb78 100644
--- a/src/test/test-manyunits.c
+++ b/src/test/test-manyunits.c
@@ -93,7 +93,7 @@ static void setup_manyunits(void) {
         }
 }
 
-static void test_manyunits(void) {
+static void test_manyunits(EnabledContext *ec) {
         time_t t0, t1;
         int r = 0;
         int count = 0;
@@ -105,7 +105,7 @@ static void test_manyunits(void) {
 
         t0 = time(NULL);
         h = hashmap_new(&string_hash_ops);
-        r = unit_file_get_list(UNIT_FILE_SYSTEM, root_dir, h);
+        r = unit_file_get_list(UNIT_FILE_SYSTEM, root_dir, h, ec);
         assert_se_cleanup(r >= 0);
         HASHMAP_FOREACH(p, h, i) {
                 ++count;
@@ -121,11 +121,16 @@ static void test_manyunits(void) {
 }
 
 int main(int argc, char* argv[]) {
+        _cleanup_enabled_context_ EnabledContext *ec = NULL;
+
         root_dir = strappenda(TEST_DIR, "/test-enabled-root");
 
         setup_manyunits();
 
-        test_manyunits();
+        ec = enabled_context_new();
+        assert(ec);
+
+        test_manyunits(ec);
 
         cleanup_manyunits();
 
diff --git a/src/test/test-unit-file.c b/src/test/test-unit-file.c
index 03b3e25..227ec10 100644
--- a/src/test/test-unit-file.c
+++ b/src/test/test-unit-file.c
@@ -47,7 +47,7 @@ static int test_unit_file_get_set(void) {
         h = hashmap_new(&string_hash_ops);
         assert_se(h);
 
-        r = unit_file_get_list(UNIT_FILE_SYSTEM, NULL, h);
+        r = unit_file_get_list(UNIT_FILE_SYSTEM, NULL, h, NULL);
 
         if (r == -EPERM || r == -EACCES) {
                 printf("Skipping test: unit_file_get_list: %s", strerror(-r));
-- 
1.9.3



More information about the systemd-devel mailing list