On 24.04.2015 12:03, Jiri Denemark wrote: > ==26726== by 0x673CD67: __vasprintf_chk (vasprintf_chk.c:80) > ==26726== by 0x5673605: UnknownInlinedFun (stdio2.h:210) > ==26726== by 0x5673605: virVasprintfInternal (virstring.c:476) > ==26726== by 0x56736EE: virAsprintfInternal (virstring.c:497) > ==26726== by 0x5680C37: virGetUserRuntimeDirectory (virutil.c:866) > ==26726== by 0x5783A89: virNetSocketNewConnectUNIX (virnetsocket.c:572) > ==26726== by 0x57751AF: virNetClientNewUNIX (virnetclient.c:344) > ==26726== by 0x57689B3: doRemoteOpen (remote_driver.c:895) > ==26726== by 0x5769F8E: remoteConnectOpen (remote_driver.c:1195) > ==26726== by 0x57092DF: do_open (libvirt.c:1189) > ==26726== by 0x570A7BF: virConnectOpenAuth (libvirt.c:1341) > > https://bugzilla.redhat.com/show_bug.cgi?id=1215042 > Signed-off-by: Jiri Denemark <jdenemar@xxxxxxxxxx> > --- > src/rpc/virnetsocket.c | 44 +++++++++++++++++++++----------------------- > 1 file changed, 21 insertions(+), 23 deletions(-) > > diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c > index a59e3e1..7ae2796 100644 > --- a/src/rpc/virnetsocket.c > +++ b/src/rpc/virnetsocket.c > @@ -546,6 +546,7 @@ int virNetSocketNewConnectUNIX(const char *path, > virSocketAddr localAddr; > virSocketAddr remoteAddr; > char *rundir = NULL; > + int ret = -1; > > memset(&localAddr, 0, sizeof(localAddr)); > memset(&remoteAddr, 0, sizeof(remoteAddr)); > @@ -559,50 +560,50 @@ int virNetSocketNewConnectUNIX(const char *path, > virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > _("Auto-spawn of daemon requested, " > "but no binary specified")); > - goto error; > + goto cleanup; > } > > if (!(binname = last_component(binary)) || binname[0] == '\0') { > virReportError(VIR_ERR_INTERNAL_ERROR, > _("Cannot determine basename for binary '%s'"), > binary); > - goto error; > + goto cleanup; > } > > if (!(rundir = virGetUserRuntimeDirectory())) > - goto error; > + goto cleanup; > > if (virFileMakePathWithMode(rundir, 0700) < 0) { > virReportSystemError(errno, > _("Cannot create user runtime directory '%s'"), > rundir); > - goto error; > + goto cleanup; > } > > if (virAsprintf(&lockpath, "%s/%s.lock", rundir, binname) < 0) > - goto error; > + goto cleanup; > > if ((lockfd = open(lockpath, O_RDWR | O_CREAT, 0600)) < 0 || > virSetCloseExec(lockfd) < 0) { > virReportSystemError(errno, _("Unable to create lock '%s'"), lockpath); > - goto error; > + goto cleanup; > } > > if (virFileLock(lockfd, false, 0, 1, true) < 0) { > virReportSystemError(errno, _("Unable to lock '%s'"), lockpath); > - goto error; > + goto cleanup; > } > } > > if ((fd = socket(PF_UNIX, SOCK_STREAM, 0)) < 0) { > virReportSystemError(errno, "%s", _("Failed to create socket")); > - goto error; > + goto cleanup; > } > > remoteAddr.data.un.sun_family = AF_UNIX; > if (virStrcpyStatic(remoteAddr.data.un.sun_path, path) == NULL) { > virReportSystemError(ENOMEM, _("Path %s too long for unix socket"), path); > - goto error; > + goto cleanup; > } > if (remoteAddr.data.un.sun_path[0] == '@') > remoteAddr.data.un.sun_path[0] = '\0'; > @@ -612,42 +613,39 @@ int virNetSocketNewConnectUNIX(const char *path, > if (!(spawnDaemon && errno == ENOENT)) { > virReportSystemError(errno, _("Failed to connect socket to '%s'"), > path); > - goto error; > + goto cleanup; > } > > if (virNetSocketForkDaemon(binary) < 0) > - goto error; > + goto cleanup; > > retries--; > usleep(5000); > } > > - if (lockfd != -1) { > - unlink(lockpath); > - VIR_FORCE_CLOSE(lockfd); > - VIR_FREE(lockpath); > - } > - This extends the critical section, making others to wait a bit longer. But I can live with that. > localAddr.len = sizeof(localAddr.data); > if (getsockname(fd, &localAddr.data.sa, &localAddr.len) < 0) { > virReportSystemError(errno, "%s", _("Unable to get local socket name")); > - goto error; > + goto cleanup; > } > > if (!(*retsock = virNetSocketNew(&localAddr, &remoteAddr, true, fd, -1, 0))) > - goto error; > + goto cleanup; > > - return 0; > + ret = 0; > > - error: > + cleanup: > if (lockfd != -1) { > unlink(lockpath); > VIR_FORCE_CLOSE(lockfd); > } > VIR_FREE(lockpath); > VIR_FREE(rundir); > - VIR_FORCE_CLOSE(fd); > - return -1; > + > + if (ret == -1) s/== -1/< 0/ please. It is more future proof IMO. For instance, if we invent new return value to describe more specifically what went wrong, and we still want the socket to close. > + VIR_FORCE_CLOSE(fd); > + > + return ret; > } > #else > int virNetSocketNewConnectUNIX(const char *path ATTRIBUTE_UNUSED, > ACK Michal -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list