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 ? > { > 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 :| -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list