From: Rami Rosen <ramir...@gmail.com> This patch fixes Bug 72611 (systemctl is-enabled scales poorly with many units) by using a hashmap for enabled/disabled services instead. The hashmap is a member of the manager and is initialized by a new method, build_enabled_cache(), which is called from the manager_startup() method. --- Makefile.am | 3 +- src/core/dbus-manager.c | 54 ++++++++++------ src/core/manager.c | 9 ++- src/core/manager.h | 1 + src/core/unit.c | 2 +- src/shared/install.c | 154 +++++++++++++++++++++++++++++++++++++++++++++- src/shared/install.h | 6 +- src/systemctl/systemctl.c | 2 +- src/test/test-install.c | 51 +++++++++------ 9 files changed, 235 insertions(+), 47 deletions(-)
diff --git a/Makefile.am b/Makefile.am index 5d84605..fa294f1 100644 --- a/Makefile.am +++ b/Makefile.am @@ -1520,7 +1520,8 @@ test_install_LDADD = \ libsystemd-units.la \ libsystemd-label.la \ libsystemd-shared.la \ - libsystemd-internal.la + libsystemd-internal.la \ + libsystemd-core.la test_watchdog_SOURCES = \ src/test/test-watchdog.c diff --git a/src/core/dbus-manager.c b/src/core/dbus-manager.c index 135d314..7f196dd 100644 --- a/src/core/dbus-manager.c +++ b/src/core/dbus-manager.c @@ -1298,7 +1298,6 @@ static int method_list_unit_files(sd_bus *bus, sd_bus_message *message, void *us _cleanup_bus_message_unref_ sd_bus_message *reply = NULL; Manager *m = userdata; UnitFileList *item; - Hashmap *h; Iterator i; int r; @@ -1314,27 +1313,17 @@ static int method_list_unit_files(sd_bus *bus, sd_bus_message *message, void *us if (r < 0) return r; - h = hashmap_new(string_hash_func, string_compare_func); - if (!h) - return -ENOMEM; - - r = unit_file_get_list(m->running_as == SYSTEMD_SYSTEM ? UNIT_FILE_SYSTEM : UNIT_FILE_USER, NULL, h); - if (r < 0) - goto fail; - r = sd_bus_message_open_container(reply, 'a', "(ss)"); if (r < 0) goto fail; - HASHMAP_FOREACH(item, h, i) { + HASHMAP_FOREACH(item, m->enabled, i) { r = sd_bus_message_append(reply, "(ss)", item->path, unit_file_state_to_string(item->state)); if (r < 0) goto fail; } - unit_file_list_free(h); - r = sd_bus_message_close_container(reply); if (r < 0) return r; @@ -1342,7 +1331,6 @@ static int method_list_unit_files(sd_bus *bus, sd_bus_message *message, void *us return sd_bus_send(bus, reply, NULL); fail: - unit_file_list_free(h); return r; } @@ -1350,7 +1338,7 @@ static int method_get_unit_file_state(sd_bus *bus, sd_bus_message *message, void Manager *m = userdata; const char *name; UnitFileState state; - UnitFileScope scope; + UnitFileList *f; int r; assert(bus); @@ -1365,12 +1353,11 @@ static int method_get_unit_file_state(sd_bus *bus, sd_bus_message *message, void if (r < 0) return r; - scope = m->running_as == SYSTEMD_SYSTEM ? UNIT_FILE_SYSTEM : UNIT_FILE_USER; - - state = unit_file_get_state(scope, NULL, name); - if (state < 0) - return state; - + f = hashmap_get(m->enabled, name); + if (f) + state = f->state; + else + state = UNIT_FILE_INVALID; return sd_bus_reply_method_return(message, "s", unit_file_state_to_string(state)); } @@ -1480,6 +1467,7 @@ static int method_enable_unit_files_generic( unsigned n_changes = 0; UnitFileScope scope; int runtime, force, r; + char **j; assert(bus); assert(message); @@ -1501,6 +1489,22 @@ static int method_enable_unit_files_generic( } } #endif + STRV_FOREACH(j, l) { + UnitFileList *f; + char *val = strdup(*j); + + f = hashmap_get(m->enabled, val); + if (!f) { + f = new0(UnitFileList, 1); + f->path = val; + } + f->state = UNIT_FILE_ENABLED; + r = hashmap_put(m->enabled, val, f); + if (r < 0) { + free(val); + return r; + } + } r = sd_bus_message_read(message, "bb", &runtime, &force); if (r < 0) @@ -1548,6 +1552,7 @@ static int method_disable_unit_files_generic( unsigned n_changes = 0; UnitFileScope scope; int r, runtime; + char **j; assert(bus); assert(message); @@ -1561,6 +1566,15 @@ static int method_disable_unit_files_generic( if (r < 0) return r; + STRV_FOREACH(j, l) { + char *val = strdup(*j); + UnitFileList *f; + + f = hashmap_get(m->enabled, val); + if (f) + f->state = UNIT_FILE_DISABLED; + } + r = sd_bus_message_read(message, "b", &runtime); if (r < 0) return r; diff --git a/src/core/manager.c b/src/core/manager.c index ce8759e..2d1deac 100644 --- a/src/core/manager.c +++ b/src/core/manager.c @@ -459,6 +459,10 @@ int manager_new(SystemdRunningAs running_as, Manager **_m) { if (r < 0) goto fail; + r = hashmap_ensure_allocated(&m->enabled, string_hash_func, string_compare_func); + if (r < 0) + goto fail; + r = sd_event_default(&m->event); if (r < 0) goto fail; @@ -792,6 +796,7 @@ void manager_free(Manager *m) { hashmap_free(m->watch_pids1); hashmap_free(m->watch_pids2); hashmap_free(m->watch_bus); + hashmap_free(m->enabled); set_free(m->failed_units); @@ -1027,7 +1032,9 @@ int manager_startup(Manager *m, FILE *serialization, FDSet *fds) { * finished */ m->send_reloading_done = true; } - + r = build_enabled_cache(m, NULL); + if (r < 0) + log_error("Failed to build the enabled cache"); return r; } diff --git a/src/core/manager.h b/src/core/manager.h index 14cdf81..1884cee 100644 --- a/src/core/manager.h +++ b/src/core/manager.h @@ -78,6 +78,7 @@ struct Manager { /* Active jobs and units */ Hashmap *units; /* name string => Unit object n:1 */ Hashmap *jobs; /* job id => Job object 1:1 */ + Hashmap *enabled; /* a hashmap for enabled Units */ /* 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 153b79b..5ac1062 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -2800,7 +2800,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); return u->unit_file_state; } diff --git a/src/shared/install.c b/src/shared/install.c index 7409046..2b2bc7d 100644 --- a/src/shared/install.c +++ b/src/shared/install.c @@ -1654,17 +1654,18 @@ int unit_file_get_default( return -ENOENT; } - UnitFileState unit_file_get_state( UnitFileScope scope, const char *root_dir, - const char *name) { + const char *name, + Manager *m) { _cleanup_lookup_paths_free_ LookupPaths paths = {}; UnitFileState state = _UNIT_FILE_STATE_INVALID; char **i; _cleanup_free_ char *path = NULL; int r; + UnitFileList *f = NULL; assert(scope >= 0); assert(scope < _UNIT_FILE_SCOPE_MAX); @@ -1718,6 +1719,10 @@ UnitFileState unit_file_get_state( return state; } } + if (m && (m->enabled != NULL)) + f = hashmap_get(m->enabled, name); + if (f) + return f->state; r = find_symlinks_in_scope(scope, root_dir, name, &state); if (r < 0) @@ -1983,6 +1988,7 @@ int unit_file_get_list( } r = find_symlinks_in_scope(scope, root_dir, de->d_name, &f->state); + if (r < 0) return r; else if (r > 0) { @@ -1991,6 +1997,7 @@ int unit_file_get_list( } r = unit_file_can_install(&paths, root_dir, f->path, true); + if (r == -EINVAL || /* Invalid setting? */ r == -EBADMSG || /* Invalid format? */ r == -ENOENT /* Included file not found? */) @@ -2013,6 +2020,149 @@ int unit_file_get_list( return r; } +int build_enabled_cache(Manager *m, const char *root_dir) +{ + char **i; + _cleanup_free_ char *buf = NULL; + _cleanup_closedir_ DIR *d = NULL; + int r; + _cleanup_lookup_paths_free_ LookupPaths paths = {}; + + r = lookup_paths_init_from_scope(&paths, m->running_as == SYSTEMD_SYSTEM ? UNIT_FILE_SYSTEM : UNIT_FILE_USER); + if (r < 0) + return r; + + STRV_FOREACH(i, paths.unit_path) { + const char *units_dir = *i; + if (d) + closedir(d); + + d = opendir(units_dir); + if (!d) { + if (errno == ENOENT) + continue; + + return -errno; + } + for (;;) { + struct dirent *de; + errno = 0; + de = readdir(d); + if (!de && errno != 0) + return -errno; + + if (!de) + break; + + if (ignore_file(de->d_name)) + continue; + + if ( (de->d_type != DT_DIR) && (!unit_name_is_valid(de->d_name, TEMPLATE_VALID))) + continue; + + r = dirent_ensure_type(d, de); + + if (r < 0) { + if (r == -ENOENT) + continue; + return r; + } + + if (de->d_type != DT_LNK && de->d_type != DT_REG && de->d_type != DT_DIR) + continue; + + if (de->d_type == DT_DIR) { + char inner_dir[PATH_MAX]; + DIR *d1 = NULL; + if (d1) + closedir(d1); + snprintf(inner_dir, sizeof(units_dir) + sizeof(de->d_name), + "%s/%s",units_dir, de->d_name); + d1 = opendir(inner_dir); + if (!d1) { + if (errno == ENOENT) + continue; + + return -errno; + } + for (;;) { + struct dirent *de1; + errno = 0; + + de1 = readdir(d1); + if (!de1 && errno != 0) + return -errno; + + if (!de1) + break; + + if (ignore_file(de1->d_name)) + continue; + + if (!unit_name_is_valid(de1->d_name, TEMPLATE_VALID)) + continue; + + r = dirent_ensure_type(d, de1); + + if (r < 0) { + if (r == -ENOENT) + continue; + + return r; + } + + if (de1->d_type != DT_LNK && de1->d_type != DT_REG) + continue; + if (de1->d_type == DT_LNK) { + UnitFileList *f; + char *val1 = strdup(de1->d_name); + f = new0(UnitFileList, 1); + if (!f) + return -ENOMEM; + f->path = path_make_absolute(de1->d_name, inner_dir); + + if (!f->path) + return -ENOMEM; + + r = unit_file_can_install(&m->lookup_paths, root_dir, de1->d_name, true); + + if (r < 0 && errno != ENOENT) + return r; + else + if (r == 0) + f->state = UNIT_FILE_STATIC; + else + f->state = UNIT_FILE_ENABLED; + hashmap_put(m->enabled, val1, f); + } + } + } + else if (de->d_type == DT_LNK) { + UnitFileList *f; + char *val = strdup(de->d_name); + f = new0(UnitFileList, 1); + if (!f) + return -ENOMEM; + f->path = path_make_absolute(de->d_name, units_dir); + if (!f->path) + return -ENOMEM; + + r = unit_file_can_install(&m->lookup_paths, root_dir, de->d_name, true); + + if (r < 0 && errno != ENOENT) + return r; + else + if (r == 0) + f->state = UNIT_FILE_STATIC; + else + f->state = UNIT_FILE_ENABLED; + hashmap_put(m->enabled, val, f); + } + } + } + return 0; +} + static const char* const unit_file_state_table[_UNIT_FILE_STATE_MAX] = { [UNIT_FILE_ENABLED] = "enabled", [UNIT_FILE_ENABLED_RUNTIME] = "enabled-runtime", diff --git a/src/shared/install.h b/src/shared/install.h index 5d57b1b..6ba3319 100644 --- a/src/shared/install.h +++ b/src/shared/install.h @@ -52,6 +52,8 @@ typedef enum UnitFileChangeType { _UNIT_FILE_CHANGE_TYPE_INVALID = -1 } UnitFileChangeType; +#include "manager.h" + typedef struct UnitFileChange { UnitFileChangeType type; char *path; @@ -83,7 +85,7 @@ int unit_file_unmask(UnitFileScope scope, bool runtime, const char *root_dir, ch 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); -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, Manager *m); int unit_file_get_list(UnitFileScope scope, const char *root_dir, Hashmap *h); @@ -97,3 +99,5 @@ 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_; + +int build_enabled_cache(Manager *m, const char *root_dir); diff --git a/src/systemctl/systemctl.c b/src/systemctl/systemctl.c index 653a324..8675567 100644 --- a/src/systemctl/systemctl.c +++ b/src/systemctl/systemctl.c @@ -5289,7 +5289,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-install.c b/src/test/test-install.c index 2087d52..e6bc0da 100644 --- a/src/test/test-install.c +++ b/src/test/test-install.c @@ -46,19 +46,32 @@ int main(int argc, char* argv[]) { UnitFileList *p; Iterator i; int r; + Manager *m = NULL; + FILE *serial = NULL; + FDSet *fdset = NULL; const char *const files[] = { "avahi-daemon.service", NULL }; const char *const files2[] = { "/home/lennart/test.service", NULL }; UnitFileChange *changes = NULL; unsigned n_changes = 0; - h = hashmap_new(string_hash_func, string_compare_func); - r = unit_file_get_list(UNIT_FILE_SYSTEM, NULL, h); + r = manager_new(SYSTEMD_SYSTEM, &m); + if (r == -EPERM || r == -EACCES) { + puts("manager_new: Permission denied. Skipping test."); + return EXIT_TEST_SKIP; + } + assert(r >= 0); + assert_se(manager_startup(m, serial, fdset) >= 0); + + r = build_enabled_cache(m, NULL); assert_se(r == 0); + h = m->enabled; + + 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, NULL, basename(p->path), m); assert_se(p->state == s); @@ -67,8 +80,6 @@ int main(int argc, char* argv[]) { unit_file_state_to_string(p->state)); } - unit_file_list_free(h); - log_error("enable"); r = unit_file_enable(UNIT_FILE_SYSTEM, false, NULL, (char**) files, false, &changes, &n_changes); @@ -82,7 +93,7 @@ int main(int argc, char* argv[]) { 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, NULL, files[0], NULL) == UNIT_FILE_ENABLED); log_error("disable"); @@ -95,7 +106,7 @@ int main(int argc, char* argv[]) { 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, NULL, files[0], NULL) == UNIT_FILE_DISABLED); log_error("mask"); changes = NULL; @@ -110,7 +121,7 @@ int main(int argc, char* argv[]) { 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, NULL, files[0], m) == UNIT_FILE_MASKED); log_error("unmask"); changes = NULL; @@ -125,7 +136,7 @@ int main(int argc, char* argv[]) { 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, NULL, files[0], NULL) == UNIT_FILE_DISABLED); log_error("mask"); changes = NULL; @@ -137,7 +148,7 @@ int main(int argc, char* argv[]) { 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, NULL, files[0], NULL) == UNIT_FILE_MASKED); log_error("disable"); changes = NULL; @@ -152,7 +163,7 @@ int main(int argc, char* argv[]) { 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, NULL, files[0], NULL) == UNIT_FILE_MASKED); log_error("umask"); changes = NULL; @@ -164,7 +175,7 @@ int main(int argc, char* argv[]) { 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, NULL, files[0], NULL) == UNIT_FILE_DISABLED); log_error("enable files2"); changes = NULL; @@ -176,7 +187,7 @@ int main(int argc, char* argv[]) { 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, NULL, basename(files2[0]), NULL) == UNIT_FILE_ENABLED); log_error("disable files2"); changes = NULL; @@ -188,7 +199,7 @@ int main(int argc, char* argv[]) { 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, NULL, basename(files2[0]), NULL) == _UNIT_FILE_STATE_INVALID); log_error("link files2"); changes = NULL; @@ -200,7 +211,7 @@ int main(int argc, char* argv[]) { 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, NULL, basename(files2[0]), NULL) == UNIT_FILE_LINKED); log_error("disable files2"); changes = NULL; @@ -212,7 +223,7 @@ int main(int argc, char* argv[]) { 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, NULL, basename(files2[0]), NULL) == _UNIT_FILE_STATE_INVALID); log_error("link files2"); changes = NULL; @@ -224,7 +235,7 @@ int main(int argc, char* argv[]) { 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, NULL, basename(files2[0]), NULL) == UNIT_FILE_LINKED); log_error("reenable files2"); changes = NULL; @@ -236,7 +247,7 @@ int main(int argc, char* argv[]) { 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, NULL, basename(files2[0]), NULL) == UNIT_FILE_ENABLED); log_error("disable files2"); changes = NULL; @@ -248,7 +259,7 @@ int main(int argc, char* argv[]) { 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, NULL, basename(files2[0]), NULL) == _UNIT_FILE_STATE_INVALID); log_error("preset files"); changes = NULL; n_changes = 0; @@ -259,7 +270,7 @@ int main(int argc, char* argv[]) { 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, NULL, basename(files[0]), NULL) == UNIT_FILE_ENABLED); return 0; } -- 1.8.5.3 _______________________________________________ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel