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

[Martin Kletzander <mkletzan@xxxxxxxxxx>]

On Wed, Aug 13, 2014 at 04:09:09PM +0100, Daniel P. Berrange wrote:
> 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@xxxxxxxxxx>
> > ---
> >  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 ?

Which one you mean?  I'm keeping the return value as it was and the
passfd is the fd that will be passed.

[...]
> > @@ -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.

Wouldn't connect(), bind(), connect() be enough (for both issues)?  Or
do we need to try the dance few times again?

Martin

Attachment: signature.asc
Description: Digital signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list