From d79ef4b32a7e02f6f4ed1dfe53f0ed1770dabe71 Mon Sep 17 00:00:00 2001 From: Pavel Charvat Date: Thu, 9 Oct 2014 09:20:30 +0000 Subject: [PATCH] Daemon: Fixed races in previous commit. --- ucw/daemon-ctrl.c | 38 +++++++++++++++++++++++++------------- ucw/daemon.c | 16 +++++++--------- 2 files changed, 32 insertions(+), 22 deletions(-) diff --git a/ucw/daemon-ctrl.c b/ucw/daemon-ctrl.c index c64be6c3..1e17dc6e 100644 --- a/ucw/daemon-ctrl.c +++ b/ucw/daemon-ctrl.c @@ -11,6 +11,7 @@ #include #include #include +#include #include #include @@ -40,12 +41,12 @@ daemon_read_pid(struct daemon_control_params *dc, int will_wait, int *pidp) // -- DAEMON_STATUS_ERROR, pid == 0 // -- DAEMON_STATUS_NOT_RUNNING, pid == 0 // -- DAEMON_STATUS_OK, pid > 0 - // -- DAEMON_STATUS_OK, pid == 0 (stopping) + // -- DAEMON_STATUS_OK, pid == 0 (flocked, but no pid available during start or stop) enum daemon_control_status st = DAEMON_STATUS_NOT_RUNNING; *pidp = 0; - int pid_fd = open(dc->pid_file, O_RDWR); + int pid_fd = open(dc->pid_file, O_RDONLY); if (pid_fd < 0) { if (errno == ENOENT) @@ -53,7 +54,7 @@ daemon_read_pid(struct daemon_control_params *dc, int will_wait, int *pidp) return daemon_control_err(dc, "Cannot open PID file `%s': %m", dc->pid_file); } - while (flock(pid_fd, LOCK_EX | (will_wait ? 0 : LOCK_NB)) < 0) + while (flock(pid_fd, LOCK_SH | (will_wait ? 0 : LOCK_NB)) < 0) { if (errno == EWOULDBLOCK) { @@ -67,10 +68,13 @@ daemon_read_pid(struct daemon_control_params *dc, int will_wait, int *pidp) } } - // Beware that daemon modifies PID file by write() + ftruncate() => even with - // atomic read() and write() we could get more bytes then expected. - // It should not be problem here, but it would be probably better to use - // the already existing fcntl lock or align all writes to a common size. + struct flock fl = { .l_type = F_RDLCK, .l_whence = SEEK_SET }; + while (fcntl(pid_fd, F_SETLKW, &fl) < 0) + { + if (errno != EINTR) + die("Unable to get fcntl lock on '%s': %m", dc->pid_file); + } + char buf[16]; int n = read(pid_fd, buf, sizeof(buf)); if (n < 0) @@ -85,7 +89,7 @@ daemon_read_pid(struct daemon_control_params *dc, int will_wait, int *pidp) } buf[n] = 0; - if (!strncmp(buf, "(stopped)", 9)) + if (!n) { close(pid_fd); return st; @@ -97,8 +101,9 @@ daemon_read_pid(struct daemon_control_params *dc, int will_wait, int *pidp) return DAEMON_STATUS_STALE; } - int pid = atoi(buf); - if (!pid) + int pid; + const char *next; + if (str_to_int(&pid, buf, &next, 10) || strcmp(next, "\n")) { daemon_control_err(dc, "PID file `%s' does not contain a valid PID", dc->pid_file); goto fail; @@ -167,13 +172,15 @@ daemon_control(struct daemon_control_params *dc) st = daemon_control_err(dc, "Daemon %s %s", dc->argv[0], ecmsg); goto done; } - st = DAEMON_STATUS_OK; + if (st != DAEMON_STATUS_STALE) + st = DAEMON_STATUS_OK; } break; case DAEMON_CONTROL_STOP: - if (st == DAEMON_STATUS_NOT_RUNNING) + if (st != DAEMON_STATUS_OK) { - st = DAEMON_STATUS_ALREADY_DONE; + if (st == DAEMON_STATUS_NOT_RUNNING) + st = DAEMON_STATUS_ALREADY_DONE; goto done; } if (pid) @@ -185,6 +192,11 @@ daemon_control(struct daemon_control_params *dc) goto done; } } + else + { + // With locked guard file it's sure that daemon is currently exiting + // and not starting => we can safely wait without any signal + } st = daemon_read_pid(dc, 1, &pid); if (st != DAEMON_STATUS_ERROR) st = DAEMON_STATUS_OK; diff --git a/ucw/daemon.c b/ucw/daemon.c index 5ca9543a..2f7b3c0b 100644 --- a/ucw/daemon.c +++ b/ucw/daemon.c @@ -134,14 +134,14 @@ daemon_init(struct daemon_params *dp) die("Cannot lock `%s': %m", dp->pid_file); } - // Also temporarily lock it with a fcntl lock until the master process - // finishes writing of pid -- used to avoid possible collision with - // writing of "(stopped)" + // Temporarily lock it with a fcntl lock until the master process ends + // -- used to avoid possible collision between writing of pid and + // truncating during stop daemon_fcntl_lock(dp); // Make a note that the daemon is starting - if (write(dp->pid_fd, "(starting)\n", 11) != 11 || - ftruncate(dp->pid_fd, 11) < 0) + if (ftruncate(dp->pid_fd, 0) < 0 || + write(dp->pid_fd, "(starting)\n", 11) != 11) die("Error writing `%s': %m", dp->pid_file); } } @@ -209,10 +209,8 @@ daemon_exit(struct daemon_params *dp) if (dp->pid_file) { daemon_fcntl_lock(dp); - if (lseek(dp->pid_fd, 0, SEEK_SET) < 0 || - write(dp->pid_fd, "(stopped)", 9) != 9 || - ftruncate(dp->pid_fd, 9)) - die("Error writing `%s': %m", dp->pid_file); + if (ftruncate(dp->pid_fd, 0)) + die("Error truncating `%s': %m", dp->pid_file); close(dp->pid_fd); } } -- 2.39.2