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

Daniel P. Berrange berrange at redhat.com
Thu Aug 14 09:26:41 UTC 2014


On Thu, Aug 14, 2014 at 11:23:27AM +0200, Martin Kletzander wrote:
> 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 at 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 ?
> >
> 
> Which one you mean?  I'm keeping the return value as it was and the
> passfd is the fd that will be passed.

Never mind, I mis-interpreted 'passfd' as an instruction not a
value.

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

We need to retry the whole block including daemon spawn to be able to
handle it if the existing daemon is in the process of shutting down
I believe.



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 :|




More information about the libvir-list mailing list