[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