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

Ronny Chevalier chevalier.ronny at gmail.com
Fri Oct 17 16:59:36 PDT 2014


Hi,

2014-10-18 1:34 GMT+02:00 Ken Sedgwick <ksedgwic at bonsai.com>:
> 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.
If there is dependencies between your patches, you should use
something like "git format-patch -3" to generate 3 patchs from the 3
last commits. It will add [PATCH 1/3] [PATCH 2/3]... on the subjects
of the mails. It helps knowing if a patch needs to be applied before
applying another.

> ---
>  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;
if (!ec)
        return;

> +
> +        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;
assert(h);
assert(key);
assert(val);

> +
> +        /* Make copies of the key and value */
> +        keycpy = strdup(key);
Missing OOM check

> +        valcpy = strdup(val);
Missing OOM check

> +
> +        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);
Why remove these asserts ?

> -        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
>
> _______________________________________________
> systemd-devel mailing list
> systemd-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/systemd-devel


More information about the systemd-devel mailing list