[libvirt] [PATCHv2] daemon: clean up daemonization

[Daniel Veillard]

On Fri, Dec 23, 2011 at 02:13:53PM -0700, Eric Blake wrote:
> 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 <ajia at redhat.com>
> Signed-off-by: Eric Blake <eblake at redhat.com>
> ---
>  daemon/libvirtd.c |   50 ++++++++++++++++++++++++++++++++++----------------
>  1 files changed, 34 insertions(+), 16 deletions(-)
> 
> diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
> index d7a03d7..f52e73f 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,38 @@ 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) {
> +                fprintf(stderr,
> +                        _("%s: error: unable to determine if daemon is "
> +                          "running: %s\n"), argv0, strerror(errno));
> +                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;
>  }
> 

  ACK to the Chrismas patch :-)

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel at veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/