Attachment 704566 Details for Bug 917075 – patch to fix race issues with pid files

[patch] patch to fix race issues with pid files

0001-core-path-install-inotify-watches-top-down-instead-o.patch (text/plain), 6.57 KB, created by Zbigniew Jędrzejewski-Szmek on 2013-03-03 14:23:04 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Zbigniew Jędrzejewski-Szmek

Created: 2013-03-03 14:23:04 UTC

Size: 6.57 KB

patch

obsolete

>From 0ba819649bbe587557abb4e11f0bacc8d80d66be Mon Sep 17 00:00:00 2001
>From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= <zbyszek@in.waw.pl>
>Date: Sun, 3 Mar 2013 01:32:34 -0500
>Subject: [PATCH] core/path: install inotify watches top-down instead of
> bottom-up
>
>When watches are installed from the bottom, it is always possible
>to race, and miss a file creation event. The race can be avoided
>if a watch is first established for a parent directory, and then for
>the file in the directory. If the file is created in the time between,
>the watch on the parent directory will fire.
>
>Some messages (mostly at debug level) are added to help diagnose
>pidfile issues.
>
>Should fix https://bugzilla.redhat.com/show_bug.cgi?id=917075.
>---
> src/core/path.c    | 78 +++++++++++++++++++++++++++++++++---------------------
> src/core/service.c | 16 +++++++++--
> 2 files changed, 62 insertions(+), 32 deletions(-)
>
>diff --git a/src/core/path.c b/src/core/path.c
>index fc10128..01a2b08 100644
>--- a/src/core/path.c
>+++ b/src/core/path.c
>@@ -53,8 +53,8 @@ int path_spec_watch(PathSpec *s, Unit *u) {
>         };
> 
>         bool exists = false;
>-        char _cleanup_free_ *k = NULL;
>-        char *slash;
>+        char _cleanup_free_ *path = NULL;
>+        char *slash, *oldslash = NULL;
>         int r;
> 
>         assert(u);
>@@ -62,8 +62,8 @@ int path_spec_watch(PathSpec *s, Unit *u) {
> 
>         path_spec_unwatch(s, u);
> 
>-        k = strdup(s->path);
>-        if (!k)
>+        path = strdup(s->path);
>+        if (!path)
>                 return -ENOMEM;
> 
>         s->inotify_fd = inotify_init1(IN_NONBLOCK|IN_CLOEXEC);
>@@ -76,43 +76,61 @@ int path_spec_watch(PathSpec *s, Unit *u) {
>         if (r < 0)
>                 goto fail;
> 
>-        s->primary_wd = inotify_add_watch(s->inotify_fd, k, flags_table[s->type]);
>-        if (s->primary_wd >= 0)
>-                exists = true;
>-        else if (errno != EACCES && errno != ENOENT) {
>-                log_error("Failed to add watch on %s: %m", k);
>-                r = -errno;
>-                goto fail;
>-        }
>+        /* This assumes the path was passed through path_kill_slashes()! */
> 
>-        do {
>+        for(slash = strchr(path, '/'); ; slash = strchr(slash+1, '/')) {
>                 int flags;
>+                char tmp;
>+
>+                if (slash) {
>+                        tmp = slash[slash == path];
>+                        slash[slash == path] = '\0';
>+                        flags = IN_MOVE_SELF | IN_DELETE_SELF | IN_ATTRIB | IN_CREATE | IN_MOVED_TO;
>+                } else {
>+                        flags = flags_table[s->type];
>+                }
> 
>-                /* This assumes the path was passed through path_kill_slashes()! */
>-                slash = strrchr(k, '/');
>-                if (!slash)
>-                        break;
>-
>-                /* Trim the path at the last slash. Keep the slash if it's the root dir. */
>-                slash[slash == k] = 0;
>-
>-                flags = IN_MOVE_SELF;
>-                if (!exists)
>-                        flags |= IN_DELETE_SELF | IN_ATTRIB | IN_CREATE | IN_MOVED_TO;
>+                r = inotify_add_watch(s->inotify_fd, path, flags);
>+                if (r < 0) {
>+                        if (errno == EACCES || errno == ENOENT)
>+                                break;
> 
>-                if (inotify_add_watch(s->inotify_fd, k, flags) >= 0)
>-                        exists = true;
>-                else if (errno != EACCES && errno != ENOENT) {
>-                        log_error("Failed to add watch on %s: %m", k);
>+                        log_warning("Failed to add watch on %s: %m", path);
>                         r = -errno;
>                         goto fail;
>+                } else {
>+                        exists = true;
>+
>+                        /* Path exists, we don't need to watch parent
>+                           too closely. */
>+                        if (oldslash) {
>+                                char tmp2 = oldslash[oldslash == path];
>+                                oldslash[oldslash == path] = '\0';
>+
>+                                inotify_add_watch(s->inotify_fd, path, IN_MOVE_SELF);
>+                                /* Error is ignored, the worst can happen is
>+                                   we get spurious events. */
>+
>+                                oldslash[oldslash == path] = tmp2;
>+                        }
>                 }
>-        } while (slash != k);
>+
>+                if (slash) {
>+                        slash[slash == path] = tmp;
>+                        oldslash = slash;
>+                } else {
>+                        /* whole path has been iterated over */
>+                        s->primary_wd = r;
>+                        break;
>+                }
>+        }
>+
>+        assert(errno == EACCES || errno == ENOENT || streq(path, s->path));
> 
>         if (!exists) {
>                 log_error("Failed to add watch on any of the components of %s: %m",
>                           s->path);
>-                r = -errno;
>+                r = -errno; /* either EACCESS or ENOENT */
>                 goto fail;
>         }
> 
>diff --git a/src/core/service.c b/src/core/service.c
>index 3f8aabc..61b150c 100644
>--- a/src/core/service.c
>+++ b/src/core/service.c
>@@ -1400,8 +1400,13 @@ static int service_load_pid_file(Service *s, bool may_warn) {
>         }
> 
>         r = parse_pid(k, &pid);
>-        if (r < 0)
>+        if (r < 0) {
>+                if (may_warn)
>+                        log_info_unit(UNIT(s)->id,
>+                                      "Failed to read PID from file %s: %s",
>+                                      s->pid_file, strerror(-r));
>                 return r;
>+        }
> 
>         if (kill(pid, 0) < 0 && errno != EPERM) {
>                 if (may_warn)
>@@ -1429,9 +1434,13 @@ static int service_load_pid_file(Service *s, bool may_warn) {
>                 return r;
> 
>         r = unit_watch_pid(UNIT(s), pid);
>-        if (r < 0)
>+        if (r < 0) {
>                 /* FIXME: we need to do something here */
>+                log_warning_unit(UNIT(s)->id,
>+                                 "Failed to watch PID %lu from service %s",
>+                                 (unsigned long) pid, UNIT(s)->id);
>                 return r;
>+        }
> 
>         return 0;
> }
>@@ -2824,6 +2833,9 @@ static int service_watch_pid_file(Service *s) {
>                 goto fail;
> 
>         /* the pidfile might have appeared just before we set the watch */
>+        log_debug_unit(UNIT(s)->id,
>+                       "Trying to read %s's PID file %s in case it changed",
>+                       UNIT(s)->id, s->pid_file_pathspec->path);
>         service_retry_pid_file(s);
> 
>         return 0;
>-- 
>1.8.1.2
>

Actions: View | Diff

Attachments on bug 917075: 704566