From 9ff7c5b031fb95783884f1e755b5875712f46ea5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 27 May 2020 14:17:43 +0200 Subject: [PATCH 1/6] basic/hashmap: make _ensure_allocated return 1 on actual allocations Also, make test_hashmap_ensure_allocated() actually test hashmap_ensure_allocated(). --- src/basic/hashmap.c | 2 +- src/test/test-hashmap-plain.c | 25 +++++++++++++------------ 2 files changed, 14 insertions(+), 13 deletions(-) diff --git a/src/basic/hashmap.c b/src/basic/hashmap.c index efbe95bb9e3..b45938cd3bc 100644 --- a/src/basic/hashmap.c +++ b/src/basic/hashmap.c @@ -835,7 +835,7 @@ static int hashmap_base_ensure_allocated(HashmapBase **h, const struct hash_ops return -ENOMEM; *h = q; - return 0; + return 1; } int internal_hashmap_ensure_allocated(Hashmap **h, const struct hash_ops *hash_ops HASHMAP_DEBUG_PARAMS) { diff --git a/src/test/test-hashmap-plain.c b/src/test/test-hashmap-plain.c index 57cf89ff534..09fe71f2055 100644 --- a/src/test/test-hashmap-plain.c +++ b/src/test/test-hashmap-plain.c @@ -251,7 +251,7 @@ static void test_hashmap_put(void) { log_info("/* %s */", __func__); - assert_se(hashmap_ensure_allocated(&m, &string_hash_ops) >= 0); + assert_se(hashmap_ensure_allocated(&m, &string_hash_ops) == 1); assert_se(m); valid_hashmap_put = hashmap_put(m, "key 1", val1); @@ -451,18 +451,20 @@ static void test_hashmap_remove_and_replace(void) { } static void test_hashmap_ensure_allocated(void) { - Hashmap *m; - int valid_hashmap; + _cleanup_hashmap_free_ Hashmap *m = NULL; + int r; log_info("/* %s */", __func__); - m = hashmap_new(&string_hash_ops); + r = hashmap_ensure_allocated(&m, &string_hash_ops); + assert_se(r == 1); - valid_hashmap = hashmap_ensure_allocated(&m, &string_hash_ops); - assert_se(valid_hashmap == 0); + r = hashmap_ensure_allocated(&m, &string_hash_ops); + assert_se(r == 0); - assert_se(m); - hashmap_free(m); + /* different hash ops shouldn't matter at this point */ + r = hashmap_ensure_allocated(&m, &trivial_hash_ops); + assert_se(r == 0); } static void test_hashmap_foreach_key(void) { @@ -557,8 +559,7 @@ static void test_hashmap_foreach(void) { } static void test_hashmap_merge(void) { - Hashmap *m; - Hashmap *n; + Hashmap *m, *n; char *val1, *val2, *val3, *val4, *r; log_info("/* %s */", __func__); @@ -572,8 +573,8 @@ static void test_hashmap_merge(void) { val4 = strdup("my val4"); assert_se(val4); - n = hashmap_new(&string_hash_ops); m = hashmap_new(&string_hash_ops); + n = hashmap_new(&string_hash_ops); hashmap_put(m, "Key 1", val1); hashmap_put(m, "Key 2", val2); @@ -586,8 +587,8 @@ static void test_hashmap_merge(void) { r = hashmap_get(m, "Key 4"); assert_se(r && streq(r, "my val4")); - assert_se(n); assert_se(m); + assert_se(n); hashmap_free(n); hashmap_free_free(m); } From 4562c35527c199e1ed9b41d78c0dd81c88c655c9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 27 May 2020 15:49:17 +0200 Subject: [PATCH 2/6] core: store unit aliases in a separate set We allocated the names set for each unit, but in the majority of cases, we'd put only one name in the set: $ systemctl show --value -p Names '*'|grep .|grep -v ' '|wc -l 564 $ systemctl show --value -p Names '*'|grep .|grep ' '|wc -l 16 So let's add a separate .id field, and only store aliases in the set, and only create the set if there's at least one alias. This requires a bit of gymnastics in the code, but I think this optimization is worth the trouble, because we save one object for many loaded units. In particular set_complete_move() wasn't very useful because the target unit would always have at least one name defined, i.e. the optimization to move the whole set over would never fire. --- src/core/dbus-unit.c | 12 ++- src/core/load-dropin.c | 5 +- src/core/load-dropin.h | 2 +- src/core/unit.c | 169 +++++++++++++++++++++----------------- src/core/unit.h | 4 +- src/shared/dropin.c | 21 +++-- src/shared/dropin.h | 3 +- src/systemctl/systemctl.c | 2 +- 8 files changed, 122 insertions(+), 96 deletions(-) diff --git a/src/core/dbus-unit.c b/src/core/dbus-unit.c index dedc3956664..320e830728e 100644 --- a/src/core/dbus-unit.c +++ b/src/core/dbus-unit.c @@ -102,20 +102,24 @@ static int property_get_names( void *userdata, sd_bus_error *error) { - Set **s = userdata; + Unit *u = userdata; Iterator i; const char *t; int r; assert(bus); assert(reply); - assert(s); + assert(u); r = sd_bus_message_open_container(reply, 'a', "s"); if (r < 0) return r; - SET_FOREACH(t, *s, i) { + r = sd_bus_message_append(reply, "s", u->id); + if (r < 0) + return r; + + SET_FOREACH(t, u->aliases, i) { r = sd_bus_message_append(reply, "s", t); if (r < 0) return r; @@ -841,7 +845,7 @@ const sd_bus_vtable bus_unit_vtable[] = { SD_BUS_VTABLE_START(0), SD_BUS_PROPERTY("Id", "s", NULL, offsetof(Unit, id), SD_BUS_VTABLE_PROPERTY_CONST), - SD_BUS_PROPERTY("Names", "as", property_get_names, offsetof(Unit, names), SD_BUS_VTABLE_PROPERTY_CONST), + SD_BUS_PROPERTY("Names", "as", property_get_names, 0, SD_BUS_VTABLE_PROPERTY_CONST), SD_BUS_PROPERTY("Following", "s", property_get_following, 0, 0), SD_BUS_PROPERTY("Requires", "as", property_get_dependencies, offsetof(Unit, dependencies[UNIT_REQUIRES]), SD_BUS_VTABLE_PROPERTY_CONST), SD_BUS_PROPERTY("Requisite", "as", property_get_dependencies, offsetof(Unit, dependencies[UNIT_REQUISITE]), SD_BUS_VTABLE_PROPERTY_CONST), diff --git a/src/core/load-dropin.c b/src/core/load-dropin.c index f61e9da6f28..9bc6857cdf9 100644 --- a/src/core/load-dropin.c +++ b/src/core/load-dropin.c @@ -19,9 +19,8 @@ static int process_deps(Unit *u, UnitDependency dependency, const char *dir_suff r = unit_file_find_dropin_paths(NULL, u->manager->lookup_paths.search_path, u->manager->unit_path_cache, - dir_suffix, - NULL, - u->names, + dir_suffix, NULL, + u->id, u->aliases, &paths); if (r < 0) return r; diff --git a/src/core/load-dropin.h b/src/core/load-dropin.h index ea15554d88b..5e2ec0d80ab 100644 --- a/src/core/load-dropin.h +++ b/src/core/load-dropin.h @@ -13,7 +13,7 @@ static inline int unit_find_dropin_paths(Unit *u, char ***paths) { u->manager->lookup_paths.search_path, u->manager->unit_path_cache, ".d", ".conf", - u->names, + u->id, u->aliases, paths); } diff --git a/src/core/unit.c b/src/core/unit.c index b287c77117c..5c31559cfb3 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -93,10 +93,6 @@ Unit *unit_new(Manager *m, size_t size) { if (!u) return NULL; - u->names = set_new(&string_hash_ops); - if (!u->names) - return mfree(u); - u->manager = m; u->type = _UNIT_TYPE_INVALID; u->default_dependencies = true; @@ -152,7 +148,8 @@ bool unit_has_name(const Unit *u, const char *name) { assert(u); assert(name); - return set_contains(u->names, (char*) name); + return streq_ptr(name, u->id) || + set_contains(u->aliases, name); } static void unit_init(Unit *u) { @@ -207,8 +204,25 @@ static void unit_init(Unit *u) { UNIT_VTABLE(u)->init(u); } +static int unit_add_alias(Unit *u, char *donated_name) { + int r; + + /* Make sure that u->names is allocated. We may leave u->names + * empty if we fail later, but this is not a problem. */ + r = set_ensure_allocated(&u->aliases, &string_hash_ops); + if (r < 0) + return r; + + r = set_put(u->aliases, donated_name); + if (r < 0) + return r; + assert(r > 0); + + return 0; +} + int unit_add_name(Unit *u, const char *text) { - _cleanup_free_ char *s = NULL, *i = NULL; + _cleanup_free_ char *name = NULL, *instance = NULL; UnitType t; int r; @@ -216,99 +230,101 @@ int unit_add_name(Unit *u, const char *text) { assert(text); if (unit_name_is_valid(text, UNIT_NAME_TEMPLATE)) { - if (!u->instance) return log_unit_debug_errno(u, SYNTHETIC_ERRNO(EINVAL), "instance is not set when adding name '%s': %m", text); - r = unit_name_replace_instance(text, u->instance, &s); + r = unit_name_replace_instance(text, u->instance, &name); if (r < 0) return log_unit_debug_errno(u, r, "failed to build instance name from '%s': %m", text); } else { - s = strdup(text); - if (!s) + name = strdup(text); + if (!name) return -ENOMEM; } - if (set_contains(u->names, s)) + if (unit_has_name(u, name)) return 0; - if (hashmap_contains(u->manager->units, s)) + + if (hashmap_contains(u->manager->units, name)) return log_unit_debug_errno(u, SYNTHETIC_ERRNO(EEXIST), - "unit already exist when adding name '%s': %m", text); + "unit already exist when adding name '%s': %m", name); - if (!unit_name_is_valid(s, UNIT_NAME_PLAIN|UNIT_NAME_INSTANCE)) + if (!unit_name_is_valid(name, UNIT_NAME_PLAIN|UNIT_NAME_INSTANCE)) return log_unit_debug_errno(u, SYNTHETIC_ERRNO(EINVAL), - "name '%s' is invalid: %m", text); + "name '%s' is invalid: %m", name); - t = unit_name_to_type(s); + t = unit_name_to_type(name); if (t < 0) return log_unit_debug_errno(u, SYNTHETIC_ERRNO(EINVAL), - "failed to to derive unit type from name '%s': %m", text); + "failed to to derive unit type from name '%s': %m", name); if (u->type != _UNIT_TYPE_INVALID && t != u->type) return log_unit_debug_errno(u, SYNTHETIC_ERRNO(EINVAL), "unit type is illegal: u->type(%d) and t(%d) for name '%s': %m", - u->type, t, text); + u->type, t, name); - r = unit_name_to_instance(s, &i); + r = unit_name_to_instance(name, &instance); if (r < 0) - return log_unit_debug_errno(u, r, "failed to extract instance from name '%s': %m", text); + return log_unit_debug_errno(u, r, "failed to extract instance from name '%s': %m", name); - if (i && !unit_type_may_template(t)) - return log_unit_debug_errno(u, SYNTHETIC_ERRNO(EINVAL), "templates are not allowed for name '%s': %m", text); + if (instance && !unit_type_may_template(t)) + return log_unit_debug_errno(u, SYNTHETIC_ERRNO(EINVAL), "templates are not allowed for name '%s': %m", name); /* Ensure that this unit is either instanced or not instanced, * but not both. Note that we do allow names with different * instance names however! */ - if (u->type != _UNIT_TYPE_INVALID && !u->instance != !i) + if (u->type != _UNIT_TYPE_INVALID && !!u->instance != !!instance) return log_unit_debug_errno(u, SYNTHETIC_ERRNO(EINVAL), "instance is illegal: u->type(%d), u->instance(%s) and i(%s) for name '%s': %m", - u->type, u->instance, i, text); + u->type, u->instance, instance, name); - if (!unit_type_may_alias(t) && !set_isempty(u->names)) - return log_unit_debug_errno(u, SYNTHETIC_ERRNO(EEXIST), "symlinks are not allowed for name '%s': %m", text); + if (u->id && !unit_type_may_alias(t)) + return log_unit_debug_errno(u, SYNTHETIC_ERRNO(EEXIST), "aliases are not allowed for name '%s': %m", name); if (hashmap_size(u->manager->units) >= MANAGER_MAX_NAMES) return log_unit_debug_errno(u, SYNTHETIC_ERRNO(E2BIG), "too many units: %m"); - r = set_put(u->names, s); + /* Add name to the global hashmap first, because that's easier to undo */ + r = hashmap_put(u->manager->units, name, u); if (r < 0) - return r; - assert(r > 0); - - r = hashmap_put(u->manager->units, s, u); - if (r < 0) { - (void) set_remove(u->names, s); return log_unit_debug_errno(u, r, "add unit to hashmap failed for name '%s': %m", text); - } - if (u->type == _UNIT_TYPE_INVALID) { + if (u->id) { + r = unit_add_alias(u, name); /* unit_add_alias() takes ownership of the name on success */ + if (r < 0) { + hashmap_remove(u->manager->units, name); + return r; + } + TAKE_PTR(name); + + } else { + /* A new name, we don't need the set yet. */ + assert(u->type == _UNIT_TYPE_INVALID); + assert(!u->instance); + u->type = t; - u->id = s; - u->instance = TAKE_PTR(i); + u->id = TAKE_PTR(name); + u->instance = TAKE_PTR(instance); LIST_PREPEND(units_by_type, u->manager->units_by_type[t], u); - unit_init(u); } - s = NULL; - unit_add_to_dbus_queue(u); return 0; } int unit_choose_id(Unit *u, const char *name) { - _cleanup_free_ char *t = NULL; - char *s, *i; + _cleanup_free_ char *t = NULL, *i = NULL; + char *s; int r; assert(u); assert(name); if (unit_name_is_valid(name, UNIT_NAME_TEMPLATE)) { - if (!u->instance) return -EINVAL; @@ -319,17 +335,27 @@ int unit_choose_id(Unit *u, const char *name) { name = t; } - /* Selects one of the names of this unit as the id */ - s = set_get(u->names, (char*) name); + if (streq_ptr(u->id, name)) + return 0; /* Nothing to do. */ + + /* Selects one of the aliases of this unit as the id */ + s = set_get(u->aliases, (char*) name); if (!s) return -ENOENT; /* Determine the new instance from the new id */ - r = unit_name_to_instance(s, &i); + r = unit_name_to_instance(name, &i); if (r < 0) return r; - u->id = s; + if (u->id) { + r = set_remove_and_put(u->aliases, name, u->id); + if (r < 0) + return r; + } else + assert_se(set_remove(u->aliases, name)); /* see set_get() above… */ + + u->id = s; /* Old u->id is now stored in the set, and s is not stored anywhere */ free(u->instance); u->instance = i; @@ -632,8 +658,10 @@ void unit_free(Unit *u) { unit_free_requires_mounts_for(u); - SET_FOREACH(t, u->names, i) + SET_FOREACH(t, u->aliases, i) hashmap_remove_value(u->manager->units, t, u); + if (u->id) + hashmap_remove_value(u->manager->units, u->id, u); if (!sd_id128_is_null(u->invocation_id)) hashmap_remove_value(u->manager->units_by_invocation_id, &u->invocation_id, u); @@ -730,11 +758,11 @@ void unit_free(Unit *u) { free(u->instance); free(u->job_timeout_reboot_arg); - - set_free_free(u->names); - free(u->reboot_arg); + set_free_free(u->aliases); + free(u->id); + free(u); } @@ -789,21 +817,6 @@ const char* unit_sub_state_to_string(Unit *u) { return UNIT_VTABLE(u)->sub_state_to_string(u); } -static int set_complete_move(Set **s, Set **other) { - assert(s); - assert(other); - - if (!other) - return 0; - - if (*s) - return set_move(*s, *other); - else - *s = TAKE_PTR(*other); - - return 0; -} - static int hashmap_complete_move(Hashmap **s, Hashmap **other) { assert(s); assert(other); @@ -820,23 +833,28 @@ static int hashmap_complete_move(Hashmap **s, Hashmap **other) { } static int merge_names(Unit *u, Unit *other) { - char *t; + char *name; Iterator i; int r; assert(u); assert(other); - r = set_complete_move(&u->names, &other->names); + r = unit_add_alias(u, other->id); if (r < 0) return r; - set_free_free(other->names); - other->names = NULL; - other->id = NULL; + r = set_move(u->aliases, other->aliases); + if (r < 0) { + set_remove(u->aliases, other->id); + return r; + } - SET_FOREACH(t, u->names, i) - assert_se(hashmap_replace(u->manager->units, t, u) == 0); + TAKE_PTR(other->id); + other->aliases = set_free_free(other->aliases); + + SET_FOREACH(name, u->aliases, i) + assert_se(hashmap_replace(u->manager->units, name, u) == 0); return 0; } @@ -1241,9 +1259,8 @@ void unit_dump(Unit *u, FILE *f, const char *prefix) { "%s-> Unit %s:\n", prefix, u->id); - SET_FOREACH(t, u->names, i) - if (!streq(t, u->id)) - fprintf(f, "%s\tAlias: %s\n", prefix, t); + SET_FOREACH(t, u->aliases, i) + fprintf(f, "%s\tAlias: %s\n", prefix, t); fprintf(f, "%s\tDescription: %s\n" diff --git a/src/core/unit.h b/src/core/unit.h index 6659406c2d0..ffa003da6b9 100644 --- a/src/core/unit.h +++ b/src/core/unit.h @@ -117,10 +117,10 @@ typedef struct Unit { FreezerState freezer_state; sd_bus_message *pending_freezer_message; - char *id; /* One name is special because we use it for identification. Points to an entry in the names set */ + char *id; /* The one special name that we use for identification */ char *instance; - Set *names; + Set *aliases; /* All the other names. */ /* For each dependency type we maintain a Hashmap whose key is the Unit* object, and the value encodes why the * dependency exists, using the UnitDependencyInfo type */ diff --git a/src/shared/dropin.c b/src/shared/dropin.c index 6844c2b647b..2693b63233e 100644 --- a/src/shared/dropin.c +++ b/src/shared/dropin.c @@ -226,30 +226,35 @@ int unit_file_find_dropin_paths( Set *unit_path_cache, const char *dir_suffix, const char *file_suffix, - const Set *names, + const char *name, + const Set *aliases, char ***ret) { _cleanup_strv_free_ char **dirs = NULL; - char *name, **p; + const char *n; + char **p; Iterator i; int r; assert(ret); - SET_FOREACH(name, names, i) + if (name) STRV_FOREACH(p, lookup_path) (void) unit_file_find_dirs(original_root, unit_path_cache, *p, name, dir_suffix, &dirs); + SET_FOREACH(n, aliases, i) + STRV_FOREACH(p, lookup_path) + (void) unit_file_find_dirs(original_root, unit_path_cache, *p, n, dir_suffix, &dirs); + /* All the names in the unit are of the same type so just grab one. */ - name = (char*) set_first(names); - if (name) { + n = name ?: (const char*) set_first(aliases); + if (n) { UnitType type = _UNIT_TYPE_INVALID; - type = unit_name_to_type(name); + type = unit_name_to_type(n); if (type < 0) return log_error_errno(SYNTHETIC_ERRNO(EINVAL), - "Failed to to derive unit type from unit name: %s", - name); + "Failed to to derive unit type from unit name: %s", n); /* Special top level drop in for ".". Add this last as it's the most generic * and should be able to be overridden by more specific drop-ins. */ diff --git a/src/shared/dropin.h b/src/shared/dropin.h index 89a2ab1098b..addf1dab140 100644 --- a/src/shared/dropin.h +++ b/src/shared/dropin.h @@ -21,5 +21,6 @@ int unit_file_find_dropin_paths( Set *unit_path_cache, const char *dir_suffix, const char *file_suffix, - const Set *names, + const char *name, + const Set *aliases, char ***paths); diff --git a/src/systemctl/systemctl.c b/src/systemctl/systemctl.c index 460e7f69b9a..c2ea0b3206a 100644 --- a/src/systemctl/systemctl.c +++ b/src/systemctl/systemctl.c @@ -2673,7 +2673,7 @@ static int unit_find_paths( if (ret_dropin_paths) { r = unit_file_find_dropin_paths(arg_root, lp->search_path, NULL, ".d", ".conf", - names, &dropins); + NULL, names, &dropins); if (r < 0) return r; } From d383acad25f7f57692f88326e5505e9194ffe6dc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 27 May 2020 16:39:35 +0200 Subject: [PATCH 3/6] core: when adding names to unit, require matching instance strings We would check that the instance is present in both units (or missing in both). But when it is defined, it should be the same in both. The comment in the code was explicitly saying that differing instance strings are allowed, but this mostly seems to be a left-over from old times. The man page is pretty clear: > the instance (if any) is always uniquely defined for a given unit and all its > aliases. --- src/core/unit.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/src/core/unit.c b/src/core/unit.c index 5c31559cfb3..4412f6b7b77 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -272,10 +272,8 @@ int unit_add_name(Unit *u, const char *text) { if (instance && !unit_type_may_template(t)) return log_unit_debug_errno(u, SYNTHETIC_ERRNO(EINVAL), "templates are not allowed for name '%s': %m", name); - /* Ensure that this unit is either instanced or not instanced, - * but not both. Note that we do allow names with different - * instance names however! */ - if (u->type != _UNIT_TYPE_INVALID && !!u->instance != !!instance) + /* Ensure that this unit either has no instance, or that the instance matches. */ + if (u->type != _UNIT_TYPE_INVALID && !streq_ptr(u->instance, instance)) return log_unit_debug_errno(u, SYNTHETIC_ERRNO(EINVAL), "instance is illegal: u->type(%d), u->instance(%s) and i(%s) for name '%s': %m", u->type, u->instance, instance, name); @@ -962,15 +960,15 @@ int unit_merge(Unit *u, Unit *other) { if (u->type != other->type) return -EINVAL; - if (!u->instance != !other->instance) - return -EINVAL; - if (!unit_type_may_alias(u->type)) /* Merging only applies to unit names that support aliases */ return -EEXIST; if (!IN_SET(other->load_state, UNIT_STUB, UNIT_NOT_FOUND)) return -EEXIST; + if (!streq_ptr(u->instance, other->instance)) + return -EINVAL; + if (other->job) return -EEXIST; From ada4b34ec7159e01af11068f99b28cce8a891ad1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 29 May 2020 11:22:14 +0200 Subject: [PATCH 4/6] core: rework error messages in unit_add_name() They were added recently in acd1987a1867fc3390ef2516962b2313cbeb366e. We can make them more informative by using unit_type_to_string() and not repeating unit names as much. Also, %m should not be used together with SYNTHETIC_ERRNO(). --- src/core/unit.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/core/unit.c b/src/core/unit.c index 4412f6b7b77..091372b9e75 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -275,14 +275,16 @@ int unit_add_name(Unit *u, const char *text) { /* Ensure that this unit either has no instance, or that the instance matches. */ if (u->type != _UNIT_TYPE_INVALID && !streq_ptr(u->instance, instance)) return log_unit_debug_errno(u, SYNTHETIC_ERRNO(EINVAL), - "instance is illegal: u->type(%d), u->instance(%s) and i(%s) for name '%s': %m", - u->type, u->instance, instance, name); + "cannot add name %s, the instances don't match (\"%s\" != \"%s\").", + name, instance, u->instance); if (u->id && !unit_type_may_alias(t)) - return log_unit_debug_errno(u, SYNTHETIC_ERRNO(EEXIST), "aliases are not allowed for name '%s': %m", name); + return log_unit_debug_errno(u, SYNTHETIC_ERRNO(EEXIST), + "cannot add name %s, aliases are not allowed for %s units.", + name, unit_type_to_string(t)); if (hashmap_size(u->manager->units) >= MANAGER_MAX_NAMES) - return log_unit_debug_errno(u, SYNTHETIC_ERRNO(E2BIG), "too many units: %m"); + return log_unit_warning_errno(u, SYNTHETIC_ERRNO(E2BIG), "cannot add name, manager has too many units: %m"); /* Add name to the global hashmap first, because that's easier to undo */ r = hashmap_put(u->manager->units, name, u); From 934ef6a5220a0486ef4c212c01a91064024cb998 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 29 May 2020 14:24:12 +0200 Subject: [PATCH 5/6] core: create socket service instances with the correct name from the start Upon an incoming connection for an accepting socket, we'd create a unit like foo@0.service, then figure out that the instance name should be e.g. "0-41-0", and then add the name foo@0-41-0.service to the unit. This obviously violates the rule that any service needs to have a constance instance part. So let's reverse the order: we first determine the instance name and then create the unit with the correct name from the start. There are two cases where we don't know the instance name: - analyze-verify: we just do a quick check that the instance unit can be created. So let's use a bogus instance string. - selinux: the code wants to load the service unit to extract the ExecStart path and query it for the selinux label. Do the same as above. Note that in both cases it is possible that the real unit that is loaded could be different than the one with the bogus instance value, for example if there is a dropin for a specific instance name. We can't do much about this, since we can't figure out the instance name in advance. The old code had the same shortcoming. --- src/analyze/analyze-verify.c | 24 ++---- src/core/socket.c | 142 ++++++++++++++++++----------------- src/core/socket.h | 2 +- 3 files changed, 82 insertions(+), 86 deletions(-) diff --git a/src/analyze/analyze-verify.c b/src/analyze/analyze-verify.c index 8275360adc3..30cb79d5096 100644 --- a/src/analyze/analyze-verify.c +++ b/src/analyze/analyze-verify.c @@ -94,6 +94,7 @@ static int generate_path(char **var, char **filenames) { } static int verify_socket(Unit *u) { + Unit *service; int r; assert(u); @@ -101,26 +102,15 @@ static int verify_socket(Unit *u) { if (u->type != UNIT_SOCKET) return 0; - /* Cannot run this without the service being around */ - - /* This makes sure instance is created if necessary. */ - r = socket_instantiate_service(SOCKET(u)); + r = socket_load_service_unit(SOCKET(u), -1, &service); if (r < 0) - return log_unit_error_errno(u, r, "Socket cannot be started, failed to create instance: %m"); + return log_unit_error_errno(u, r, "service unit for the socket cannot be loaded: %m"); - /* This checks both type of sockets */ - if (UNIT_ISSET(SOCKET(u)->service)) { - Service *service; - - service = SERVICE(UNIT_DEREF(SOCKET(u)->service)); - log_unit_debug(u, "Using %s", UNIT(service)->id); - - if (UNIT(service)->load_state != UNIT_LOADED) { - log_unit_error(u, "Service %s not loaded, %s cannot be started.", UNIT(service)->id, u->id); - return -ENOENT; - } - } + if (service->load_state != UNIT_LOADED) + return log_unit_error_errno(u, SYNTHETIC_ERRNO(ENOENT), + "service %s not loaded, socket cannot be started.", service->id); + log_unit_debug(u, "using service unit %s.", service->id); return 0; } diff --git a/src/core/socket.c b/src/core/socket.c index 218b4b245df..b9ccde89708 100644 --- a/src/core/socket.c +++ b/src/core/socket.c @@ -205,38 +205,25 @@ static int socket_arm_timer(Socket *s, usec_t usec) { return 0; } -int socket_instantiate_service(Socket *s) { - _cleanup_free_ char *prefix = NULL, *name = NULL; +static int socket_instantiate_service(Socket *s, int cfd) { + Unit *service; int r; - Unit *u; assert(s); + assert(cfd >= 0); - /* This fills in s->service if it isn't filled in yet. For - * Accept=yes sockets we create the next connection service - * here. For Accept=no this is mostly a NOP since the service - * is figured out at load time anyway. */ + /* This fills in s->service if it isn't filled in yet. For Accept=yes sockets we create the next + * connection service here. For Accept=no this is mostly a NOP since the service is figured out at + * load time anyway. */ - if (UNIT_DEREF(s->service)) - return 0; - - if (!s->accept) - return 0; - - r = unit_name_to_prefix(UNIT(s)->id, &prefix); + r = socket_load_service_unit(s, cfd, &service); if (r < 0) return r; - if (asprintf(&name, "%s@%u.service", prefix, s->n_accepted) < 0) - return -ENOMEM; + unit_ref_set(&s->service, UNIT(s), service); - r = manager_load_unit(UNIT(s)->manager, name, NULL, NULL, &u); - if (r < 0) - return r; - - unit_ref_set(&s->service, UNIT(s), u); - - return unit_add_two_dependencies(UNIT(s), UNIT_BEFORE, UNIT_TRIGGERS, u, false, UNIT_DEPENDENCY_IMPLICIT); + return unit_add_two_dependencies(UNIT(s), UNIT_BEFORE, UNIT_TRIGGERS, service, + false, UNIT_DEPENDENCY_IMPLICIT); } static bool have_non_accept_socket(Socket *s) { @@ -1398,37 +1385,81 @@ clear: return r; } +int socket_load_service_unit(Socket *s, int cfd, Unit **ret) { + /* Figure out what the unit that will be used to handle the connections on the socket looks like. + * + * If cfd < 0, then we don't have a connection yet. In case of Accept=yes sockets, use a fake + * instance name. + */ + + if (UNIT_ISSET(s->service)) { + *ret = UNIT_DEREF(s->service); + return 0; + } + + if (!s->accept) + return -ENODATA; + + /* Build the instance name and load the unit */ + _cleanup_free_ char *prefix = NULL, *instance = NULL, *name = NULL; + int r; + + r = unit_name_to_prefix(UNIT(s)->id, &prefix); + if (r < 0) + return r; + + if (cfd >= 0) { + r = instance_from_socket(cfd, s->n_accepted, &instance); + if (r == -ENOTCONN) + /* ENOTCONN is legitimate if TCP RST was received. + * This connection is over, but the socket unit lives on. */ + return log_unit_debug_errno(UNIT(s), r, + "Got ENOTCONN on incoming socket, assuming aborted connection attempt, ignoring."); + if (r < 0) + return r; + } + + /* For accepting sockets, we don't know how the instance will be called until we get a connection and + * can figure out what the peer name is. So let's use "internal" as the instance to make it clear + * that this is not an actual peer name. We use "unknown" when we cannot figure out the peer. */ + r = unit_name_build(prefix, instance ?: "internal", ".service", &name); + if (r < 0) + return r; + + return manager_load_unit(UNIT(s)->manager, name, NULL, NULL, ret); +} + static int socket_determine_selinux_label(Socket *s, char **ret) { - Service *service; - ExecCommand *c; - _cleanup_free_ char *path = NULL; int r; assert(s); assert(ret); if (s->selinux_context_from_net) { - /* If this is requested, get label from the network label */ + /* If this is requested, get the label from the network label */ r = mac_selinux_get_our_label(ret); if (r == -EOPNOTSUPP) goto no_label; } else { - /* Otherwise, get it from the executable we are about to start */ - r = socket_instantiate_service(s); + /* Otherwise, get it from the executable we are about to start. */ + + Unit *service; + ExecCommand *c; + _cleanup_free_ char *path = NULL; + + r = socket_load_service_unit(s, -1, &service); + if (r == -ENODATA) + goto no_label; if (r < 0) return r; - if (!UNIT_ISSET(s->service)) - goto no_label; - - service = SERVICE(UNIT_DEREF(s->service)); - c = service->exec_command[SERVICE_EXEC_START]; + c = SERVICE(service)->exec_command[SERVICE_EXEC_START]; if (!c) goto no_label; - r = chase_symlinks(c->path, service->exec_context.root_directory, CHASE_PREFIX_ROOT, &path, NULL); + r = chase_symlinks(c->path, SERVICE(service)->exec_context.root_directory, CHASE_PREFIX_ROOT, &path, NULL); if (r < 0) goto no_label; @@ -1614,8 +1645,8 @@ static int socket_open_fds(Socket *_s) { case SOCKET_SOCKET: if (!know_label) { - /* Figure out label, if we don't it know yet. We do it once, for the first socket where - * we need this and remember it for the rest. */ + /* Figure out the label, if we don't it know yet. We do it once for the first + * socket where we need this and remember it for the rest. */ r = socket_determine_selinux_label(s, &label); if (r < 0) @@ -2332,7 +2363,6 @@ static void socket_enter_running(Socket *s, int cfd) { socket_set_state(s, SOCKET_RUNNING); } else { - _cleanup_free_ char *prefix = NULL, *instance = NULL, *name = NULL; _cleanup_(socket_peer_unrefp) SocketPeer *p = NULL; Service *service; @@ -2344,9 +2374,9 @@ static void socket_enter_running(Socket *s, int cfd) { if (s->max_connections_per_source > 0) { r = socket_acquire_peer(s, cfd, &p); - if (r < 0) { + if (r < 0) goto refuse; - } else if (r > 0 && p->n_ref > s->max_connections_per_source) { + if (r > 0 && p->n_ref > s->max_connections_per_source) { _cleanup_free_ char *t = NULL; (void) sockaddr_pretty(&p->peer.sa, p->peer_salen, true, false, &t); @@ -2358,30 +2388,7 @@ static void socket_enter_running(Socket *s, int cfd) { } } - r = socket_instantiate_service(s); - if (r < 0) - goto fail; - - r = instance_from_socket(cfd, s->n_accepted, &instance); - if (r < 0) { - if (r != -ENOTCONN) - goto fail; - - /* ENOTCONN is legitimate if TCP RST was received. - * This connection is over, but the socket unit lives on. */ - log_unit_debug(UNIT(s), "Got ENOTCONN on incoming socket, assuming aborted connection attempt, ignoring."); - goto refuse; - } - - r = unit_name_to_prefix(UNIT(s)->id, &prefix); - if (r < 0) - goto fail; - - r = unit_name_build(prefix, instance, ".service", &name); - if (r < 0) - goto fail; - - r = unit_add_name(UNIT_DEREF(s->service), name); + r = socket_instantiate_service(s, cfd); if (r < 0) goto fail; @@ -2389,21 +2396,20 @@ static void socket_enter_running(Socket *s, int cfd) { unit_ref_unset(&s->service); s->n_accepted++; - unit_choose_id(UNIT(service), name); r = service_set_socket_fd(service, cfd, s, s->selinux_context_from_net); if (r < 0) goto fail; - cfd = -1; /* We passed ownership of the fd to the service now. Forget it here. */ + TAKE_FD(cfd); /* We passed ownership of the fd to the service now. Forget it here. */ s->n_connections++; service->peer = TAKE_PTR(p); /* Pass ownership of the peer reference */ r = manager_add_job(UNIT(s)->manager, JOB_START, UNIT(service), JOB_REPLACE, NULL, &error, NULL); if (r < 0) { - /* We failed to activate the new service, but it still exists. Let's make sure the service - * closes and forgets the connection fd again, immediately. */ + /* We failed to activate the new service, but it still exists. Let's make sure the + * service closes and forgets the connection fd again, immediately. */ service_close_socket_fd(service); goto fail; } diff --git a/src/core/socket.h b/src/core/socket.h index 9e0be15ba8d..848eeb9c67c 100644 --- a/src/core/socket.h +++ b/src/core/socket.h @@ -165,7 +165,7 @@ void socket_connection_unref(Socket *s); void socket_free_ports(Socket *s); -int socket_instantiate_service(Socket *s); +int socket_load_service_unit(Socket *s, int cfd, Unit **ret); char *socket_fdname(Socket *s); From e2ea0056818bc46d355d751321731a0d8d6d3a88 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 27 May 2020 16:36:38 +0200 Subject: [PATCH 6/6] core: do not touch instance from unit_choose_id() unit_choose_id() is about marking one of the aliases of the unit as the main name. With the preparatory work in previous patches, all aliases of the unit must have the same instance, so the operation to update the instance is a noop. --- src/core/unit.c | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/src/core/unit.c b/src/core/unit.c index 091372b9e75..fdeed7e7c54 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -317,7 +317,7 @@ int unit_add_name(Unit *u, const char *text) { } int unit_choose_id(Unit *u, const char *name) { - _cleanup_free_ char *t = NULL, *i = NULL; + _cleanup_free_ char *t = NULL; char *s; int r; @@ -343,11 +343,6 @@ int unit_choose_id(Unit *u, const char *name) { if (!s) return -ENOENT; - /* Determine the new instance from the new id */ - r = unit_name_to_instance(name, &i); - if (r < 0) - return r; - if (u->id) { r = set_remove_and_put(u->aliases, name, u->id); if (r < 0) @@ -356,10 +351,6 @@ int unit_choose_id(Unit *u, const char *name) { assert_se(set_remove(u->aliases, name)); /* see set_get() above… */ u->id = s; /* Old u->id is now stored in the set, and s is not stored anywhere */ - - free(u->instance); - u->instance = i; - unit_add_to_dbus_queue(u); return 0;