Merge pull request #15954 from keszybz/unit-file-leak
Fix leak in unit path cache and another small optimization
This commit is contained in:
Lennart Poettering
GitHub
commit
5fc20ede0f
|
|
@ -55,6 +55,8 @@ void path_hash_func(const char *q, struct siphash *state) {
|
|||
}
|
||||
|
||||
DEFINE_HASH_OPS(path_hash_ops, char, path_hash_func, path_compare);
|
||||
DEFINE_HASH_OPS_WITH_KEY_DESTRUCTOR(path_hash_ops_free,
|
||||
char, path_hash_func, path_compare, free);
|
||||
|
||||
void trivial_hash_func(const void *p, struct siphash *state) {
|
||||
siphash24_compress(&p, sizeof(p), state);
|
||||
|
|
|
|||
|
|
@ -81,6 +81,7 @@ extern const struct hash_ops string_hash_ops_free_free;
|
|||
|
||||
void path_hash_func(const char *p, struct siphash *state);
|
||||
extern const struct hash_ops path_hash_ops;
|
||||
extern const struct hash_ops path_hash_ops_free;
|
||||
|
||||
/* This will compare the passed pointers directly, and will not dereference them. This is hence not useful for strings
|
||||
* or suchlike. */
|
||||
|
|
|
|||
|
|
@ -89,6 +89,14 @@ OrderedHashmap *internal_ordered_hashmap_new(const struct hash_ops *hash_ops HA
|
|||
#define hashmap_new(ops) internal_hashmap_new(ops HASHMAP_DEBUG_SRC_ARGS)
|
||||
#define ordered_hashmap_new(ops) internal_ordered_hashmap_new(ops HASHMAP_DEBUG_SRC_ARGS)
|
||||
|
||||
#define hashmap_free_and_replace(a, b) \
|
||||
({ \
|
||||
hashmap_free(a); \
|
||||
(a) = (b); \
|
||||
(b) = NULL; \
|
||||
0; \
|
||||
})
|
||||
|
||||
HashmapBase *internal_hashmap_free(HashmapBase *h, free_func_t default_free_key, free_func_t default_free_value);
|
||||
static inline Hashmap *hashmap_free(Hashmap *h) {
|
||||
return (void*) internal_hashmap_free(HASHMAP_BASE(h), NULL, NULL);
|
||||
|
|
|
|||
|
|
@ -5,6 +5,14 @@
|
|||
#include "hashmap.h"
|
||||
#include "macro.h"
|
||||
|
||||
#define set_free_and_replace(a, b) \
|
||||
({ \
|
||||
set_free(a); \
|
||||
(a) = (b); \
|
||||
(b) = NULL; \
|
||||
0; \
|
||||
})
|
||||
|
||||
Set *internal_set_new(const struct hash_ops *hash_ops HASHMAP_DEBUG_PARAMS);
|
||||
#define set_new(ops) internal_set_new(ops HASHMAP_DEBUG_SRC_ARGS)
|
||||
|
||||
|
|
|
|||
|
|
@ -263,6 +263,10 @@ int job_install_deserialized(Job *j) {
|
|||
return log_unit_debug_errno(j->unit, SYNTHETIC_ERRNO(EEXIST),
|
||||
"Unit already has a job installed. Not installing deserialized job.");
|
||||
|
||||
r = hashmap_ensure_allocated(&j->manager->jobs, NULL);
|
||||
if (r < 0)
|
||||
return r;
|
||||
|
||||
r = hashmap_put(j->manager->jobs, UINT32_TO_PTR(j->id), j);
|
||||
if (r == -EEXIST)
|
||||
return log_unit_debug_errno(j->unit, r, "Job ID %" PRIu32 " already used, cannot deserialize job.", j->id);
|
||||
|
|
|
|||
|
|
@ -696,7 +696,7 @@ static int manager_setup_prefix(Manager *m) {
|
|||
static void manager_free_unit_name_maps(Manager *m) {
|
||||
m->unit_id_map = hashmap_free(m->unit_id_map);
|
||||
m->unit_name_map = hashmap_free(m->unit_name_map);
|
||||
m->unit_path_cache = set_free_free(m->unit_path_cache);
|
||||
m->unit_path_cache = set_free(m->unit_path_cache);
|
||||
m->unit_cache_mtime = 0;
|
||||
}
|
||||
|
||||
|
|
@ -833,10 +833,6 @@ int manager_new(UnitFileScope scope, ManagerTestRunFlags test_run_flags, Manager
|
|||
if (r < 0)
|
||||
return r;
|
||||
|
||||
r = hashmap_ensure_allocated(&m->jobs, NULL);
|
||||
if (r < 0)
|
||||
return r;
|
||||
|
||||
r = hashmap_ensure_allocated(&m->cgroup_unit, &path_hash_ops);
|
||||
if (r < 0)
|
||||
return r;
|
||||
|
|
@ -3963,6 +3959,11 @@ void manager_check_finished(Manager *m) {
|
|||
return;
|
||||
}
|
||||
|
||||
/* The jobs hashmap tends to grow a lot during boot, and then it's not reused until shutdown. Let's
|
||||
kill the hashmap if it is relatively large. */
|
||||
if (hashmap_buckets(m->jobs) > hashmap_size(m->units) / 10)
|
||||
m->jobs = hashmap_free(m->jobs);
|
||||
|
||||
manager_flip_auto_status(m, false, "boot finished");
|
||||
|
||||
/* Notify Type=idle units that we are done now */
|
||||
|
|
|
|||
|
|
@ -656,6 +656,10 @@ static int transaction_apply(
|
|||
assert(!j->transaction_prev);
|
||||
assert(!j->transaction_next);
|
||||
|
||||
r = hashmap_ensure_allocated(&m->jobs, NULL);
|
||||
if (r < 0)
|
||||
return r;
|
||||
|
||||
r = hashmap_put(m->jobs, UINT32_TO_PTR(j->id), j);
|
||||
if (r < 0)
|
||||
goto rollback;
|
||||
|
|
|
|||
117
src/core/unit.c
117
src/core/unit.c
|
|
@ -501,9 +501,7 @@ static void bidi_set_free(Unit *u, Hashmap *h) {
|
|||
/* Frees the hashmap and makes sure we are dropped from the inverse pointers */
|
||||
|
||||
HASHMAP_FOREACH_KEY(v, other, h, i) {
|
||||
UnitDependency d;
|
||||
|
||||
for (d = 0; d < _UNIT_DEPENDENCY_MAX; d++)
|
||||
for (UnitDependency d = 0; d < _UNIT_DEPENDENCY_MAX; d++)
|
||||
hashmap_remove(other->dependencies[d], u);
|
||||
|
||||
unit_add_to_gc_queue(other);
|
||||
|
|
@ -599,7 +597,6 @@ static void unit_done(Unit *u) {
|
|||
}
|
||||
|
||||
void unit_free(Unit *u) {
|
||||
UnitDependency d;
|
||||
Iterator i;
|
||||
char *t;
|
||||
|
||||
|
|
@ -650,7 +647,7 @@ void unit_free(Unit *u) {
|
|||
job_free(j);
|
||||
}
|
||||
|
||||
for (d = 0; d < _UNIT_DEPENDENCY_MAX; d++)
|
||||
for (UnitDependency d = 0; d < _UNIT_DEPENDENCY_MAX; d++)
|
||||
bidi_set_free(u, u->dependencies[d]);
|
||||
|
||||
if (u->on_console)
|
||||
|
|
@ -875,19 +872,17 @@ static void merge_dependencies(Unit *u, Unit *other, const char *other_id, UnitD
|
|||
assert(d < _UNIT_DEPENDENCY_MAX);
|
||||
|
||||
/* Fix backwards pointers. Let's iterate through all dependent units of the other unit. */
|
||||
HASHMAP_FOREACH_KEY(v, back, other->dependencies[d], i) {
|
||||
UnitDependency k;
|
||||
HASHMAP_FOREACH_KEY(v, back, other->dependencies[d], i)
|
||||
|
||||
/* Let's now iterate through the dependencies of that dependencies of the other units, looking for
|
||||
* pointers back, and let's fix them up, to instead point to 'u'. */
|
||||
|
||||
for (k = 0; k < _UNIT_DEPENDENCY_MAX; k++) {
|
||||
/* Let's now iterate through the dependencies of that dependencies of the other units,
|
||||
* looking for pointers back, and let's fix them up, to instead point to 'u'. */
|
||||
for (UnitDependency k = 0; k < _UNIT_DEPENDENCY_MAX; k++)
|
||||
if (back == u) {
|
||||
/* Do not add dependencies between u and itself. */
|
||||
if (hashmap_remove(back->dependencies[k], other))
|
||||
maybe_warn_about_dependency(u, other_id, k);
|
||||
} else {
|
||||
UnitDependencyInfo di_u, di_other, di_merged;
|
||||
UnitDependencyInfo di_u, di_other;
|
||||
|
||||
/* Let's drop this dependency between "back" and "other", and let's create it between
|
||||
* "back" and "u" instead. Let's merge the bit masks of the dependency we are moving,
|
||||
|
|
@ -899,7 +894,7 @@ static void merge_dependencies(Unit *u, Unit *other, const char *other_id, UnitD
|
|||
|
||||
di_u.data = hashmap_get(back->dependencies[k], u);
|
||||
|
||||
di_merged = (UnitDependencyInfo) {
|
||||
UnitDependencyInfo di_merged = {
|
||||
.origin_mask = di_u.origin_mask | di_other.origin_mask,
|
||||
.destination_mask = di_u.destination_mask | di_other.destination_mask,
|
||||
};
|
||||
|
|
@ -911,9 +906,6 @@ static void merge_dependencies(Unit *u, Unit *other, const char *other_id, UnitD
|
|||
|
||||
/* assert_se(hashmap_remove_and_replace(back->dependencies[k], other, u, di_merged.data) >= 0); */
|
||||
}
|
||||
}
|
||||
|
||||
}
|
||||
|
||||
/* Also do not move dependencies on u to itself */
|
||||
back = hashmap_remove(other->dependencies[d], u);
|
||||
|
|
@ -927,7 +919,6 @@ static void merge_dependencies(Unit *u, Unit *other, const char *other_id, UnitD
|
|||
}
|
||||
|
||||
int unit_merge(Unit *u, Unit *other) {
|
||||
UnitDependency d;
|
||||
const char *other_id = NULL;
|
||||
int r;
|
||||
|
||||
|
|
@ -966,7 +957,7 @@ int unit_merge(Unit *u, Unit *other) {
|
|||
other_id = strdupa(other->id);
|
||||
|
||||
/* Make reservations to ensure merge_dependencies() won't fail */
|
||||
for (d = 0; d < _UNIT_DEPENDENCY_MAX; d++) {
|
||||
for (UnitDependency d = 0; d < _UNIT_DEPENDENCY_MAX; d++) {
|
||||
r = reserve_dependencies(u, other, d);
|
||||
/*
|
||||
* We don't rollback reservations if we fail. We don't have
|
||||
|
|
@ -986,7 +977,7 @@ int unit_merge(Unit *u, Unit *other) {
|
|||
unit_ref_set(other->refs_by_target, other->refs_by_target->source, u);
|
||||
|
||||
/* Merge dependencies */
|
||||
for (d = 0; d < _UNIT_DEPENDENCY_MAX; d++)
|
||||
for (UnitDependency d = 0; d < _UNIT_DEPENDENCY_MAX; d++)
|
||||
merge_dependencies(u, other, other_id, d);
|
||||
|
||||
other->load_state = UNIT_MERGED;
|
||||
|
|
@ -1221,7 +1212,6 @@ static void print_unit_dependency_mask(FILE *f, const char *kind, UnitDependency
|
|||
|
||||
void unit_dump(Unit *u, FILE *f, const char *prefix) {
|
||||
char *t, **j;
|
||||
UnitDependency d;
|
||||
Iterator i;
|
||||
const char *prefix2;
|
||||
char timestamp[5][FORMAT_TIMESTAMP_MAX], timespan[FORMAT_TIMESPAN_MAX];
|
||||
|
|
@ -1377,7 +1367,7 @@ void unit_dump(Unit *u, FILE *f, const char *prefix) {
|
|||
prefix, strna(format_timestamp(timestamp[0], sizeof(timestamp[0]), u->assert_timestamp.realtime)),
|
||||
prefix, yes_no(u->assert_result));
|
||||
|
||||
for (d = 0; d < _UNIT_DEPENDENCY_MAX; d++) {
|
||||
for (UnitDependency d = 0; d < _UNIT_DEPENDENCY_MAX; d++) {
|
||||
UnitDependencyInfo di;
|
||||
Unit *other;
|
||||
|
||||
|
|
@ -1509,7 +1499,6 @@ int unit_add_default_target_dependency(Unit *u, Unit *target) {
|
|||
}
|
||||
|
||||
static int unit_add_slice_dependencies(Unit *u) {
|
||||
UnitDependencyMask mask;
|
||||
assert(u);
|
||||
|
||||
if (!UNIT_HAS_CGROUP_CONTEXT(u))
|
||||
|
|
@ -1518,7 +1507,7 @@ static int unit_add_slice_dependencies(Unit *u) {
|
|||
/* Slice units are implicitly ordered against their parent slices (as this relationship is encoded in the
|
||||
name), while all other units are ordered based on configuration (as in their case Slice= configures the
|
||||
relationship). */
|
||||
mask = u->type == UNIT_SLICE ? UNIT_DEPENDENCY_IMPLICIT : UNIT_DEPENDENCY_FILE;
|
||||
UnitDependencyMask mask = u->type == UNIT_SLICE ? UNIT_DEPENDENCY_IMPLICIT : UNIT_DEPENDENCY_FILE;
|
||||
|
||||
if (UNIT_ISSET(u->slice))
|
||||
return unit_add_two_dependencies(u, UNIT_AFTER, UNIT_REQUIRES, UNIT_DEREF(u->slice), true, mask);
|
||||
|
|
@ -3205,6 +3194,43 @@ char *unit_dbus_path_invocation_id(Unit *u) {
|
|||
return unit_dbus_path_from_name(u->invocation_id_string);
|
||||
}
|
||||
|
||||
static int unit_set_invocation_id(Unit *u, sd_id128_t id) {
|
||||
int r;
|
||||
|
||||
assert(u);
|
||||
|
||||
/* Set the invocation ID for this unit. If we cannot, this will not roll back, but reset the whole thing. */
|
||||
|
||||
if (sd_id128_equal(u->invocation_id, id))
|
||||
return 0;
|
||||
|
||||
if (!sd_id128_is_null(u->invocation_id))
|
||||
(void) hashmap_remove_value(u->manager->units_by_invocation_id, &u->invocation_id, u);
|
||||
|
||||
if (sd_id128_is_null(id)) {
|
||||
r = 0;
|
||||
goto reset;
|
||||
}
|
||||
|
||||
r = hashmap_ensure_allocated(&u->manager->units_by_invocation_id, &id128_hash_ops);
|
||||
if (r < 0)
|
||||
goto reset;
|
||||
|
||||
u->invocation_id = id;
|
||||
sd_id128_to_string(id, u->invocation_id_string);
|
||||
|
||||
r = hashmap_put(u->manager->units_by_invocation_id, &u->invocation_id, u);
|
||||
if (r < 0)
|
||||
goto reset;
|
||||
|
||||
return 0;
|
||||
|
||||
reset:
|
||||
u->invocation_id = SD_ID128_NULL;
|
||||
u->invocation_id_string[0] = 0;
|
||||
return r;
|
||||
}
|
||||
|
||||
int unit_set_slice(Unit *u, Unit *slice) {
|
||||
assert(u);
|
||||
assert(slice);
|
||||
|
|
@ -5344,43 +5370,6 @@ void unit_notify_user_lookup(Unit *u, uid_t uid, gid_t gid) {
|
|||
unit_add_to_dbus_queue(u);
|
||||
}
|
||||
|
||||
int unit_set_invocation_id(Unit *u, sd_id128_t id) {
|
||||
int r;
|
||||
|
||||
assert(u);
|
||||
|
||||
/* Set the invocation ID for this unit. If we cannot, this will not roll back, but reset the whole thing. */
|
||||
|
||||
if (sd_id128_equal(u->invocation_id, id))
|
||||
return 0;
|
||||
|
||||
if (!sd_id128_is_null(u->invocation_id))
|
||||
(void) hashmap_remove_value(u->manager->units_by_invocation_id, &u->invocation_id, u);
|
||||
|
||||
if (sd_id128_is_null(id)) {
|
||||
r = 0;
|
||||
goto reset;
|
||||
}
|
||||
|
||||
r = hashmap_ensure_allocated(&u->manager->units_by_invocation_id, &id128_hash_ops);
|
||||
if (r < 0)
|
||||
goto reset;
|
||||
|
||||
u->invocation_id = id;
|
||||
sd_id128_to_string(id, u->invocation_id_string);
|
||||
|
||||
r = hashmap_put(u->manager->units_by_invocation_id, &u->invocation_id, u);
|
||||
if (r < 0)
|
||||
goto reset;
|
||||
|
||||
return 0;
|
||||
|
||||
reset:
|
||||
u->invocation_id = SD_ID128_NULL;
|
||||
u->invocation_id_string[0] = 0;
|
||||
return r;
|
||||
}
|
||||
|
||||
int unit_acquire_invocation_id(Unit *u) {
|
||||
sd_id128_t id;
|
||||
int r;
|
||||
|
|
@ -5502,8 +5491,6 @@ static void unit_update_dependency_mask(Unit *u, UnitDependency d, Unit *other,
|
|||
}
|
||||
|
||||
void unit_remove_dependencies(Unit *u, UnitDependencyMask mask) {
|
||||
UnitDependency d;
|
||||
|
||||
assert(u);
|
||||
|
||||
/* Removes all dependencies u has on other units marked for ownership by 'mask'. */
|
||||
|
|
@ -5511,7 +5498,7 @@ void unit_remove_dependencies(Unit *u, UnitDependencyMask mask) {
|
|||
if (mask == 0)
|
||||
return;
|
||||
|
||||
for (d = 0; d < _UNIT_DEPENDENCY_MAX; d++) {
|
||||
for (UnitDependency d = 0; d < _UNIT_DEPENDENCY_MAX; d++) {
|
||||
bool done;
|
||||
|
||||
do {
|
||||
|
|
@ -5522,8 +5509,6 @@ void unit_remove_dependencies(Unit *u, UnitDependencyMask mask) {
|
|||
done = true;
|
||||
|
||||
HASHMAP_FOREACH_KEY(di.data, other, u->dependencies[d], i) {
|
||||
UnitDependency q;
|
||||
|
||||
if ((di.origin_mask & ~mask) == di.origin_mask)
|
||||
continue;
|
||||
di.origin_mask &= ~mask;
|
||||
|
|
@ -5534,7 +5519,7 @@ void unit_remove_dependencies(Unit *u, UnitDependencyMask mask) {
|
|||
* all dependency types on the other unit and delete all those which point to us and
|
||||
* have the right mask set. */
|
||||
|
||||
for (q = 0; q < _UNIT_DEPENDENCY_MAX; q++) {
|
||||
for (UnitDependency q = 0; q < _UNIT_DEPENDENCY_MAX; q++) {
|
||||
UnitDependencyInfo dj;
|
||||
|
||||
dj.data = hashmap_get(other->dependencies[q], u);
|
||||
|
|
|
|||
|
|
@ -829,7 +829,6 @@ void unit_unref_uid_gid(Unit *u, bool destroy_now);
|
|||
|
||||
void unit_notify_user_lookup(Unit *u, uid_t uid, gid_t gid);
|
||||
|
||||
int unit_set_invocation_id(Unit *u, sd_id128_t id);
|
||||
int unit_acquire_invocation_id(Unit *u);
|
||||
|
||||
bool unit_shall_confirm_spawn(Unit *u);
|
||||
|
|
|
|||
|
|
@ -229,9 +229,9 @@ static bool lookup_paths_mtime_good(const LookupPaths *lp, usec_t mtime) {
|
|||
int unit_file_build_name_map(
|
||||
const LookupPaths *lp,
|
||||
usec_t *cache_mtime,
|
||||
Hashmap **ret_unit_ids_map,
|
||||
Hashmap **ret_unit_names_map,
|
||||
Set **ret_path_cache) {
|
||||
Hashmap **unit_ids_map,
|
||||
Hashmap **unit_names_map,
|
||||
Set **path_cache) {
|
||||
|
||||
/* Build two mappings: any name → main unit (i.e. the end result of symlink resolution), unit name →
|
||||
* all aliases (i.e. the entry for a given key is a a list of all names which point to this key). The
|
||||
|
|
@ -239,7 +239,8 @@ int unit_file_build_name_map(
|
|||
* have a key, but it is not present in the value for itself, there was an alias pointing to it, but
|
||||
* the unit itself is not loadable.
|
||||
*
|
||||
* At the same, build a cache of paths where to find units.
|
||||
* At the same, build a cache of paths where to find units. The non-const parameters are for input
|
||||
* and output. Existing contents will be freed before the new contents are stored.
|
||||
*/
|
||||
|
||||
_cleanup_hashmap_free_ Hashmap *ids = NULL, *names = NULL;
|
||||
|
|
@ -253,8 +254,8 @@ int unit_file_build_name_map(
|
|||
if (cache_mtime && *cache_mtime > 0 && lookup_paths_mtime_good(lp, *cache_mtime))
|
||||
return 0;
|
||||
|
||||
if (ret_path_cache) {
|
||||
paths = set_new(&path_hash_ops);
|
||||
if (path_cache) {
|
||||
paths = set_new(&path_hash_ops_free);
|
||||
if (!paths)
|
||||
return log_oom();
|
||||
}
|
||||
|
|
@ -296,7 +297,7 @@ int unit_file_build_name_map(
|
|||
if (!filename)
|
||||
return log_oom();
|
||||
|
||||
if (ret_path_cache) {
|
||||
if (paths) {
|
||||
r = set_consume(paths, filename);
|
||||
if (r < 0)
|
||||
return log_oom();
|
||||
|
|
@ -418,10 +419,11 @@ int unit_file_build_name_map(
|
|||
|
||||
if (cache_mtime)
|
||||
*cache_mtime = mtime;
|
||||
*ret_unit_ids_map = TAKE_PTR(ids);
|
||||
*ret_unit_names_map = TAKE_PTR(names);
|
||||
if (ret_path_cache)
|
||||
*ret_path_cache = TAKE_PTR(paths);
|
||||
|
||||
hashmap_free_and_replace(*unit_ids_map, ids);
|
||||
hashmap_free_and_replace(*unit_names_map, names);
|
||||
if (path_cache)
|
||||
set_free_and_replace(*path_cache, paths);
|
||||
|
||||
return 1;
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in New Issue