From 6906ac9a6920b1849db51450df7739100c5100ed Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 14 Nov 2019 15:23:05 +0100 Subject: [PATCH 1/3] Remove path_compare_func() alias for path_compare() Non sunt multiplicanda entia sine necessitate. --- src/basic/hash-funcs.c | 6 +----- src/basic/hash-funcs.h | 1 - src/cgtop/cgtop.c | 2 +- 3 files changed, 2 insertions(+), 7 deletions(-) diff --git a/src/basic/hash-funcs.c b/src/basic/hash-funcs.c index 11cd371fad..fce339512c 100644 --- a/src/basic/hash-funcs.c +++ b/src/basic/hash-funcs.c @@ -52,11 +52,7 @@ void path_hash_func(const char *q, struct siphash *state) { } } -int path_compare_func(const char *a, const char *b) { - return path_compare(a, b); -} - -DEFINE_HASH_OPS(path_hash_ops, char, path_hash_func, path_compare_func); +DEFINE_HASH_OPS(path_hash_ops, char, path_hash_func, path_compare); void trivial_hash_func(const void *p, struct siphash *state) { siphash24_compress(&p, sizeof(p), state); diff --git a/src/basic/hash-funcs.h b/src/basic/hash-funcs.h index 0d2d428389..7bb5d1cd02 100644 --- a/src/basic/hash-funcs.h +++ b/src/basic/hash-funcs.h @@ -79,7 +79,6 @@ extern const struct hash_ops string_hash_ops; extern const struct hash_ops string_hash_ops_free_free; void path_hash_func(const char *p, struct siphash *state); -int path_compare_func(const char *a, const char *b) _pure_; extern const struct hash_ops path_hash_ops; /* This will compare the passed pointers directly, and will not dereference them. This is hence not useful for strings diff --git a/src/cgtop/cgtop.c b/src/cgtop/cgtop.c index 2494c70238..de25aaae5d 100644 --- a/src/cgtop/cgtop.c +++ b/src/cgtop/cgtop.c @@ -897,7 +897,7 @@ static const char* counting_what(void) { return "userspace processes (excl. kernel)"; } -DEFINE_PRIVATE_HASH_OPS_WITH_VALUE_DESTRUCTOR(group_hash_ops, char, path_hash_func, path_compare_func, Group, group_free); +DEFINE_PRIVATE_HASH_OPS_WITH_VALUE_DESTRUCTOR(group_hash_ops, char, path_hash_func, path_compare, Group, group_free); static int run(int argc, char *argv[]) { _cleanup_hashmap_free_ Hashmap *a = NULL, *b = NULL; From 7a16cd4b05cc3e975d90aee72ed9d77ada22a093 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 18 Nov 2019 14:13:05 +0100 Subject: [PATCH 2/3] core/path: serialize the previous_exists state Without that we are prone to generate spurious events after daemon reload/restart. --- src/core/path.c | 55 +++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 51 insertions(+), 4 deletions(-) diff --git a/src/core/path.c b/src/core/path.c index dff551f377..ed3a0132c2 100644 --- a/src/core/path.c +++ b/src/core/path.c @@ -9,12 +9,14 @@ #include "bus-util.h" #include "dbus-path.h" #include "dbus-unit.h" +#include "escape.h" #include "fd-util.h" #include "fs-util.h" #include "glob-util.h" #include "macro.h" #include "mkdir.h" #include "path.h" +#include "path-util.h" #include "serialize.h" #include "special.h" #include "stat-util.h" @@ -27,19 +29,18 @@ static const UnitActiveState state_translation_table[_PATH_STATE_MAX] = { [PATH_DEAD] = UNIT_INACTIVE, [PATH_WAITING] = UNIT_ACTIVE, [PATH_RUNNING] = UNIT_ACTIVE, - [PATH_FAILED] = UNIT_FAILED + [PATH_FAILED] = UNIT_FAILED, }; static int path_dispatch_io(sd_event_source *source, int fd, uint32_t revents, void *userdata); int path_spec_watch(PathSpec *s, sd_event_io_handler_t handler) { - static const int flags_table[_PATH_TYPE_MAX] = { [PATH_EXISTS] = IN_DELETE_SELF|IN_MOVE_SELF|IN_ATTRIB, [PATH_EXISTS_GLOB] = IN_DELETE_SELF|IN_MOVE_SELF|IN_ATTRIB, [PATH_CHANGED] = IN_DELETE_SELF|IN_MOVE_SELF|IN_ATTRIB|IN_CLOSE_WRITE|IN_CREATE|IN_DELETE|IN_MOVED_FROM|IN_MOVED_TO, [PATH_MODIFIED] = IN_DELETE_SELF|IN_MOVE_SELF|IN_ATTRIB|IN_CLOSE_WRITE|IN_CREATE|IN_DELETE|IN_MOVED_FROM|IN_MOVED_TO|IN_MODIFY, - [PATH_DIRECTORY_NOT_EMPTY] = IN_DELETE_SELF|IN_MOVE_SELF|IN_ATTRIB|IN_CREATE|IN_MOVED_TO + [PATH_DIRECTORY_NOT_EMPTY] = IN_DELETE_SELF|IN_MOVE_SELF|IN_ATTRIB|IN_CREATE|IN_MOVED_TO, }; bool exists = false; @@ -601,6 +602,7 @@ static int path_stop(Unit *u) { static int path_serialize(Unit *u, FILE *f, FDSet *fds) { Path *p = PATH(u); + PathSpec *s; assert(u); assert(f); @@ -609,6 +611,19 @@ static int path_serialize(Unit *u, FILE *f, FDSet *fds) { (void) serialize_item(f, "state", path_state_to_string(p->state)); (void) serialize_item(f, "result", path_result_to_string(p->result)); + LIST_FOREACH(spec, s, p->specs) { + _cleanup_free_ char *escaped = NULL; + + escaped = cescape(s->path); + if (!escaped) + return log_oom(); + + (void) serialize_item_format(f, "path-spec", "%s %i %s", + path_type_to_string(s->type), + s->previous_exists, + s->path); + } + return 0; } @@ -638,6 +653,38 @@ static int path_deserialize_item(Unit *u, const char *key, const char *value, FD else if (f != PATH_SUCCESS) p->result = f; + } else if (streq(key, "path-spec")) { + int previous_exists, skip = 0, r; + _cleanup_free_ char *type_str = NULL; + + if (sscanf(value, "%ms %i %n", &type_str, &previous_exists, &skip) < 2) + log_unit_debug(u, "Failed to parse path-spec value: %s", value); + else { + _cleanup_free_ char *unescaped = NULL; + PathType type; + PathSpec *s; + + type = path_type_from_string(type_str); + if (type < 0) { + log_unit_warning(u, "Unknown path type \"%s\", ignoring.", type_str); + return 0; + } + + r = cunescape(value+skip, 0, &unescaped); + if (r < 0) { + log_unit_warning_errno(u, r, "Failed to unescape serialize path: %m"); + return 0; + } + + LIST_FOREACH(spec, s, p->specs) + if (s->type == type && + path_equal(s->path, unescaped)) { + + s->previous_exists = previous_exists; + break; + } + } + } else log_unit_debug(u, "Unknown serialization key: %s", key); @@ -670,7 +717,7 @@ static int path_dispatch_io(sd_event_source *source, int fd, uint32_t revents, v if (!IN_SET(p->state, PATH_WAITING, PATH_RUNNING)) return 0; - /* log_debug("inotify wakeup on %s.", u->id); */ + /* log_debug("inotify wakeup on %s.", UNIT(p)->id); */ LIST_FOREACH(spec, s, p->specs) if (path_spec_owns_inotify_fd(s, fd)) From d7cf8c24d4ef6ed4c9d711ee82ba57a529baad34 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 18 Nov 2019 14:20:05 +0100 Subject: [PATCH 3/3] core/path: fix spurious triggering of PathExists= on restart/reload Our handling of the condition was inconsistent. Normally, we'd only fire when the file was created (or removed and subsequently created again). But on restarts, we'd do a "recheck" from path_coldplug(), and if the file existed, we'd always trigger. Daemon restarts and reloads should not be observeable, in the sense that they should not trigger units which were already triggered and would not be started again under normal circumstances. Note that the mechanism for checks is racy: we get a notification from inotify, and by the time we check, the file could have been created and removed again, or removed and created again. It would be better if we inotify would give as an unambiguous signal that the file was created, but it doesn't: IN_DELETE_SELF triggers on inode removal, not directory entry, so we need to include IN_ATTRIB, which obviously triggers on other conditions. Fixes #12801. --- src/core/path.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/core/path.c b/src/core/path.c index ed3a0132c2..1491a07fa5 100644 --- a/src/core/path.c +++ b/src/core/path.c @@ -175,12 +175,14 @@ int path_spec_fd_event(PathSpec *s, uint32_t revents) { } static bool path_spec_check_good(PathSpec *s, bool initial) { - bool good = false; + bool b, good = false; switch (s->type) { case PATH_EXISTS: - good = access(s->path, F_OK) >= 0; + b = access(s->path, F_OK) >= 0; + good = b && !s->previous_exists; + s->previous_exists = b; break; case PATH_EXISTS_GLOB: @@ -196,14 +198,11 @@ static bool path_spec_check_good(PathSpec *s, bool initial) { } case PATH_CHANGED: - case PATH_MODIFIED: { - bool b; - + case PATH_MODIFIED: b = access(s->path, F_OK) >= 0; good = !initial && b != s->previous_exists; s->previous_exists = b; break; - } default: ;