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