[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v3 8/9] rpc: pass listen FD to the daemon being started



On Wed, Jul 23, 2014 at 04:27:12PM +0200, Martin Kletzander wrote:
> This eliminates the need for active waiting.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=927369
> 
> Signed-off-by: Martin Kletzander <mkletzan redhat com>
> ---
>  src/rpc/virnetsocket.c | 102 ++++++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 83 insertions(+), 19 deletions(-)
> 
> diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c
> index a94b2bc..46be541 100644
> --- a/src/rpc/virnetsocket.c
> +++ b/src/rpc/virnetsocket.c
> @@ -1,7 +1,7 @@
>  /*
>   * virnetsocket.c: generic network socket handling
>   *
> - * Copyright (C) 2006-2013 Red Hat, Inc.
> + * Copyright (C) 2006-2014 Red Hat, Inc.
>   * Copyright (C) 2006 Daniel P. Berrange
>   *
>   * This library is free software; you can redistribute it and/or
> @@ -121,7 +121,7 @@ VIR_ONCE_GLOBAL_INIT(virNetSocket)
> 
> 
>  #ifndef WIN32
> -static int virNetSocketForkDaemon(const char *binary)
> +static int virNetSocketForkDaemon(const char *binary, int passfd)

s/int/bool/ perhaps ?

>  {
>      int ret;
>      virCommandPtr cmd = virCommandNewArgList(binary,
> @@ -134,6 +134,10 @@ static int virNetSocketForkDaemon(const char *binary)
>      virCommandAddEnvPassBlockSUID(cmd, "XDG_RUNTIME_DIR", NULL);
>      virCommandClearCaps(cmd);
>      virCommandDaemonize(cmd);
> +    if (passfd) {
> +        virCommandPassFD(cmd, passfd, VIR_COMMAND_PASS_FD_CLOSE_PARENT);
> +        virCommandPassListenFDs(cmd);
> +    }
>      ret = virCommandRun(cmd, NULL);
>      virCommandFree(cmd);
>      return ret;
> @@ -540,10 +544,11 @@ int virNetSocketNewConnectUNIX(const char *path,
>                                 const char *binary,
>                                 virNetSocketPtr *retsock)
>  {
> +    char *buf = NULL;
> +    int errfd[2] = { -1, -1 };
> +    int fd, passfd;
>      virSocketAddr localAddr;
>      virSocketAddr remoteAddr;
> -    int fd;
> -    int retries = 0;
> 
>      memset(&localAddr, 0, sizeof(localAddr));
>      memset(&remoteAddr, 0, sizeof(remoteAddr));
> @@ -569,28 +574,82 @@ int virNetSocketNewConnectUNIX(const char *path,
>      if (remoteAddr.data.un.sun_path[0] == '@')
>          remoteAddr.data.un.sun_path[0] = '\0';
> 
> - retry:
> -    if (connect(fd, &remoteAddr.data.sa, remoteAddr.len) < 0) {
> -        if ((errno == ECONNREFUSED ||
> -             errno == ENOENT) &&
> -            spawnDaemon && retries < 20) {
> -            VIR_DEBUG("Connection refused for %s, trying to spawn %s",
> -                      path, binary);
> -            if (retries == 0 &&
> -                virNetSocketForkDaemon(binary) < 0)
> -                goto error;
> +    if (spawnDaemon) {
> +        int err = 0;
> +        int rv = -1;
> +        int status = 0;
> +        pid_t pid = 0;
> 
> -            retries++;
> -            usleep(1000 * 100 * retries);
> -            goto retry;
> +        if ((passfd = socket(PF_UNIX, SOCK_STREAM, 0)) < 0) {
> +            virReportSystemError(errno, "%s", _("Failed to create socket"));
> +            goto error;
>          }
> 
> -        virReportSystemError(errno,
> -                             _("Failed to connect socket to '%s'"),
> +        if (pipe2(errfd, O_CLOEXEC) < 0) {
> +            virReportSystemError(errno, "%s",
> +                                 _("Cannot create pipe for child"));
> +            goto error;
> +        }
> +
> +        /*
> +         * We have to fork() here, because umask() is set
> +         * per-process, chmod() is racy and fchmod() has undefined
> +         * behaviour on sockets according to POSIX, so it doesn't
> +         * work outside Linux.
> +         */
> +
> +        if ((pid = virFork()) < 0)
> +            goto error;
> +
> +        if (pid == 0) {
> +            VIR_FORCE_CLOSE(errfd[0]);
> +
> +            umask(0077);
> +            rv = bind(passfd, &remoteAddr.data.sa, remoteAddr.len);
> +            if (rv < 0) {
> +                ignore_value(safewrite(errfd[1], &errno, sizeof(int)));
> +            }
> +            VIR_FORCE_CLOSE(errfd[1]);
> +            _exit(rv < 0 ? EXIT_FAILURE : EXIT_SUCCESS);
> +        }
> +
> +        VIR_FORCE_CLOSE(errfd[1]);
> +        rv = virProcessWait(pid, &status, false);
> +        ignore_value(saferead(errfd[0], &err, sizeof(int)));
> +        VIR_FORCE_CLOSE(errfd[0]);
> +
> +        if (rv < 0 || status != EXIT_SUCCESS) {
> +            if (err) {
> +                virReportSystemError(err,
> +                                     _("Failed to bind socket to %s "
> +                                       "in child process"),
> +                                     path);
> +            } else {
> +                virReportError(VIR_ERR_INTERNAL_ERROR,
> +                               _("Failed to bind socket to %s "
> +                                 "in child process"),
> +                               path);
> +            }
> +            goto error;
> +        }
> +
> +        if (listen(passfd, 0) < 0) {
> +            virReportSystemError(errno, "%s",
> +                                 _("Failed to listen on socket that's about "
> +                                   "to be passed to the daemon"));
> +            goto error;
> +        }

Perhaps I'm miss-reading the code, but isn't this block gonig to
result in failure if libvirtd is already running (& listening on
the socket we try to bind to) and we requested auto-spawn ?

> +    }
> +
> +    if (connect(fd, &remoteAddr.data.sa, remoteAddr.len) < 0) {
> +        virReportSystemError(errno, _("Failed to connect socket to '%s'"),
>                               path);
>          goto error;
>      }

Also I think there's still a race here because the already running
libvirtd daemon could be unfortunately shutting down right at the
same time we try to connect. So I think we still need to have a
re-try loop around the connect attempt to address that race. Obviously
it should be pretty damn rare now so won't have the problems that the
current loop has.


> 
> +    if (spawnDaemon && virNetSocketForkDaemon(binary, passfd) < 0)
> +        goto error;
> +

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]