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

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(a)redhat.com&gt; >>--- >> 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?