From e957b670613cdc2de5d84b806bb0432b63c990c0 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Thu, 22 Dec 2011 16:28:04 +0800 Subject: [PATCH] daemon: clean up daemonization Valgrind detected a pipe fd leak before the parent exits on success, introduced in commit 4296cea; by itself, the leak is not bad, since we immediately called _exit(), but we might as well be clean to make valgrind analysis easier. Meanwhile, if the daemon grandchild detects an error, the parent failed to flush the error message before exiting. Also, we had the possibility of both parent and child returning to the caller, such that the user could see duplicated reports of failure from the two return paths. And we might as well be robust to the (unlikely) situation of being started with stdin closed. * daemon/libvirtd.c (daemonForkIntoBackground): Use exit if an error message was generated, avoid fd leaks for valgrind's sake, avoid returning to caller in both parent and child, and don't close a just-dup'd stdin. Based on a report by Alex Jia. * How to reproduce? % service libvirtd stop % valgrind -v --track-fds=yes /usr/sbin/libvirtd --daemon * Actual valgrind result: ==16804== FILE DESCRIPTORS: 7 open at exit. ==16804== Open file descriptor 7: ==16804== at 0x321FAD8B87: pipe (in /lib64/libc-2.12.so) ==16804== by 0x41F34D: daemonForkIntoBackground (libvirtd.c:186) ==16804== by 0x4207A0: main (libvirtd.c:1420) Signed-off-by: Alex Jia Signed-off-by: Eric Blake --- daemon/libvirtd.c | 53 +++++++++++++++++++++++++++++++++-------------- 1 file changed, 37 insertions(+), 16 deletions(-) diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index d7a03d71c5..b1b542bb84 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -190,6 +190,7 @@ static int daemonForkIntoBackground(const char *argv0) switch (pid) { case 0: { + /* intermediate child */ int stdinfd = -1; int stdoutfd = -1; int nextpid; @@ -206,9 +207,9 @@ static int daemonForkIntoBackground(const char *argv0) goto cleanup; if (dup2(stdoutfd, STDERR_FILENO) != STDERR_FILENO) goto cleanup; - if (VIR_CLOSE(stdinfd) < 0) + if (stdinfd > STDERR_FILENO && VIR_CLOSE(stdinfd) < 0) goto cleanup; - if (VIR_CLOSE(stdoutfd) < 0) + if (stdoutfd > STDERR_FILENO && VIR_CLOSE(stdoutfd) < 0) goto cleanup; if (setsid() < 0) @@ -216,26 +217,28 @@ static int daemonForkIntoBackground(const char *argv0) nextpid = fork(); switch (nextpid) { - case 0: + case 0: /* grandchild */ return statuspipe[1]; - case -1: - return -1; - default: - _exit(0); + case -1: /* error */ + goto cleanup; + default: /* intermediate child succeeded */ + _exit(EXIT_SUCCESS); } cleanup: VIR_FORCE_CLOSE(stdoutfd); VIR_FORCE_CLOSE(stdinfd); - return -1; + VIR_FORCE_CLOSE(statuspipe[1]); + _exit(EXIT_FAILURE); } - case -1: - return -1; + case -1: /* error in parent */ + goto error; default: { + /* parent */ int ret; char status; @@ -243,23 +246,41 @@ static int daemonForkIntoBackground(const char *argv0) /* We wait to make sure the first child forked successfully */ if (virPidWait(pid, NULL) < 0) - return -1; + goto error; - /* Now block until the second child initializes successfully */ + /* If we get here, then the grandchild was spawned, so we + * must exit. Block until the second child initializes + * successfully */ again: ret = read(statuspipe[0], &status, 1); if (ret == -1 && errno == EINTR) goto again; - if (ret == 1 && status != 0) { + VIR_FORCE_CLOSE(statuspipe[0]); + + if (ret != 1) { + char ebuf[1024]; + + fprintf(stderr, + _("%s: error: unable to determine if daemon is " + "running: %s\n"), argv0, + virStrerror(errno, ebuf, sizeof(ebuf))); + exit(EXIT_FAILURE); + } else if (status != 0) { fprintf(stderr, - _("%s: error: %s. Check /var/log/messages or run without " - "--daemon for more info.\n"), argv0, + _("%s: error: %s. Check /var/log/messages or run " + "without --daemon for more info.\n"), argv0, virDaemonErrTypeToString(status)); + exit(EXIT_FAILURE); } - _exit(ret == 1 && status == 0 ? 0 : 1); + _exit(EXIT_SUCCESS); } } + +error: + VIR_FORCE_CLOSE(statuspipe[0]); + VIR_FORCE_CLOSE(statuspipe[1]); + return -1; } -- 2.47.2