On 09/08/2014 01:46 AM, Martin Kletzander wrote: > This way it behaves more like the daemon itself does (acquiring a > pidfile, deleting the socket before binding, etc.). > > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=927369 > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1138604 > > Signed-off-by: Martin Kletzander <mkletzan@xxxxxxxxxx> > --- > src/rpc/virnetsocket.c | 57 ++++++++++++++++++++++++++++++++++++++++++++------ > 1 file changed, 51 insertions(+), 6 deletions(-) > > diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c > index 42184bd..18a5a8f 100644 > --- a/src/rpc/virnetsocket.c > +++ b/src/rpc/virnetsocket.c > @@ -51,9 +51,11 @@ > #include "virlog.h" > #include "virfile.h" > #include "virthread.h" > +#include "virpidfile.h" > #include "virprobe.h" > #include "virprocess.h" > #include "virstring.h" > +#include "dirname.h" > #include "passfd.h" > > #if WITH_SSH2 > @@ -544,7 +546,10 @@ int virNetSocketNewConnectUNIX(const char *path, > const char *binary, > virNetSocketPtr *retsock) > { > + char *binname = NULL; > + char *pidpath = NULL; > int fd, passfd = -1; > + int pidfd = -1; > virSocketAddr localAddr; > virSocketAddr remoteAddr; > > @@ -583,16 +588,45 @@ int virNetSocketNewConnectUNIX(const char *path, > goto error; > } > > + if (!(binname = last_component(binary)) || binname[0] == '\0') { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("Cannot determine basename for binary '%s'"), > + binary); > + goto error; > + } > + > + if (virPidFileConstructPath(false, NULL, binname, &pidpath) < 0) > + goto error; > + > + if ((pidfd = virPidFileAcquirePath(pidpath, false, getpid())) < 0) { > + /* > + * This can happen in a very rare case of two clients spawning two > + * daemons at the same time, and the error in the logs that gets > + * reset here can be a clue to some future debugging. > + */ > + virResetLastError(); > + spawnDaemon = false; > + goto retry; > + } > + > if ((passfd = socket(PF_UNIX, SOCK_STREAM, 0)) < 0) { > virReportSystemError(errno, "%s", _("Failed to create socket")); > 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. > + * We already even acquired the pidfile, so noone else should be using > + * @path right now. So we're OK to unlink it and paying attention to > + * the return value makes no real sense here. Only if it's not an > + * abstract socket, of course. > + */ > + if (path[0] != '@') > + unlink(path); > + > + /* > + * 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; > @@ -612,8 +646,9 @@ int virNetSocketNewConnectUNIX(const char *path, > /* > * OK, so the subprocces failed to bind() the socket. This may mean > * that another daemon was starting at the same time and succeeded > - * with its bind(). So we'll try connecting again, but this time > - * without spawning the daemon. > + * with its bind() (even though it should not happen because we > + * using a pidfile for the race). So we'll try connecting again, > + * but this time without spawning the daemon. > */ > spawnDaemon = false; > goto retry; > @@ -632,6 +667,12 @@ int virNetSocketNewConnectUNIX(const char *path, > goto error; > } > > + /* > + * Do we need to eliminate the super-rare race here any more? It would > + * need incorporating the following VIR_FORCE_CLOSE() into a > + * virCommandHook inside a virNetSocketForkDaemon(). > + */ > + VIR_FORCE_CLOSE(pidfd); > if (virNetSocketForkDaemon(binary, passfd) < 0) > goto error; > } > @@ -648,8 +689,12 @@ int virNetSocketNewConnectUNIX(const char *path, > return 0; > > error: > + if (pidfd > 0) > + virPidFileDeletePath(pidpath); > + VIR_FREE(pidpath); > VIR_FORCE_CLOSE(fd); > VIR_FORCE_CLOSE(passfd); > + VIR_FORCE_CLOSE(pidfd); > if (spawnDaemon) > unlink(path); > return -1; > Tested this, fixes the issue for me as well. Patch looks fine AFAICT, matches similar code stanzas in daemon/libvirtd.c. So ACK from me - Cole -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list