[libvirt] [PATCH] Fix connection to already running session libvirtd

Since 1b807f92, connecting with virsh to an already running session libvirtd fails with: $ virsh list --all error: failed to connect to the hypervisor error: no valid connection error: Failed to connect socket to '/run/user/1000/libvirt/libvirt-sock': Transport endpoint is already connected This is caused by a logic error in virNetSocketNewConnectUnix: even if the connection to the daemon socket succeeded, we still try to spawn the daemon and then connect to it. This commit changes the logic to not try to spawn libvirtd if we successfully connected to its socket. With whitespace changes removed, this patch becomes just this: diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c index f913365..79540b3 100644 --- a/src/rpc/virnetsocket.c +++ b/src/rpc/virnetsocket.c @@ -574,7 +574,8 @@ int virNetSocketNewConnectUNIX(const char *path, remoteAddr.data.un.sun_path[0] = '\0'; retry: - if (connect(fd, &remoteAddr.data.sa, remoteAddr.len) < 0 && !spawnDaemon) { + if (connect(fd, &remoteAddr.data.sa, remoteAddr.len) < 0) { + if (!spawnDaemon) { virReportSystemError(errno, _("Failed to connect socket to '%s'"), path); goto error; @@ -634,6 +635,7 @@ int virNetSocketNewConnectUNIX(const char *path, if (virNetSocketForkDaemon(binary, passfd) < 0) goto error; } + } localAddr.len = sizeof(localAddr.data); if (getsockname(fd, &localAddr.data.sa, &localAddr.len) < 0) { --- src/rpc/virnetsocket.c | 102 +++++++++++++++++++++++++------------------------ 1 file changed, 52 insertions(+), 50 deletions(-) diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c index 79258ef..8fc5d80 100644 --- a/src/rpc/virnetsocket.c +++ b/src/rpc/virnetsocket.c @@ -573,65 +573,67 @@ int virNetSocketNewConnectUNIX(const char *path, remoteAddr.data.un.sun_path[0] = '\0'; retry: - if (connect(fd, &remoteAddr.data.sa, remoteAddr.len) < 0 && !spawnDaemon) { - virReportSystemError(errno, _("Failed to connect socket to '%s'"), - path); - goto error; - } else if (spawnDaemon) { - int status = 0; - pid_t pid = 0; - - if ((passfd = socket(PF_UNIX, SOCK_STREAM, 0)) < 0) { - virReportSystemError(errno, "%s", _("Failed to create socket")); + if (connect(fd, &remoteAddr.data.sa, remoteAddr.len) < 0) { + if (!spawnDaemon) { + virReportSystemError(errno, _("Failed to connect socket to '%s'"), + path); goto error; - } + } else if (spawnDaemon) { + int status = 0; + pid_t pid = 0; - /* - * 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 ((passfd = socket(PF_UNIX, SOCK_STREAM, 0)) < 0) { + virReportSystemError(errno, "%s", _("Failed to create socket")); + goto error; + } - if (pid == 0) { - umask(0077); - if (bind(passfd, &remoteAddr.data.sa, remoteAddr.len) < 0) - _exit(EXIT_FAILURE); + /* + * 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; - _exit(EXIT_SUCCESS); - } + if (pid == 0) { + umask(0077); + if (bind(passfd, &remoteAddr.data.sa, remoteAddr.len) < 0) + _exit(EXIT_FAILURE); - if (virProcessWait(pid, &status, false) < 0) - goto error; + _exit(EXIT_SUCCESS); + } - if (status != EXIT_SUCCESS) { - /* - * 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. - */ - spawnDaemon = false; - goto retry; - } + if (virProcessWait(pid, &status, false) < 0) + 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; - } + if (status != EXIT_SUCCESS) { + /* + * 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. + */ + spawnDaemon = false; + goto retry; + } - if (connect(fd, &remoteAddr.data.sa, remoteAddr.len) < 0) { - virReportSystemError(errno, _("Failed to connect socket to '%s'"), - 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; + } - if (virNetSocketForkDaemon(binary, passfd) < 0) - goto error; + if (connect(fd, &remoteAddr.data.sa, remoteAddr.len) < 0) { + virReportSystemError(errno, _("Failed to connect socket to '%s'"), + path); + goto error; + } + + if (virNetSocketForkDaemon(binary, passfd) < 0) + goto error; + } } localAddr.len = sizeof(localAddr.data); -- 1.9.3

On Fri, Aug 29, 2014 at 10:37:21AM +0200, Christophe Fergeau wrote:
Since 1b807f92, connecting with virsh to an already running session libvirtd fails with: $ virsh list --all error: failed to connect to the hypervisor error: no valid connection error: Failed to connect socket to '/run/user/1000/libvirt/libvirt-sock': Transport endpoint is already connected
This is caused by a logic error in virNetSocketNewConnectUnix: even if the connection to the daemon socket succeeded, we still try to spawn the daemon and then connect to it. This commit changes the logic to not try to spawn libvirtd if we successfully connected to its socket.
Thanks for trying that, that was a flaw in my condition-optimization mechanism, I guess. Although my git was a bit confused by the diff included in the commit message. I'd suggest just saying that most of the commit is a whitespace change; people can see that using '-w' themselves. That toggle should even work with format-patch, but I'm not sure that applies cleanly all the time. ACK with the commit cleaned up and safe for 1.2.8. Thank you, Martin

Hey, On Fri, Aug 29, 2014 at 11:08:53AM +0200, Martin Kletzander wrote:
Although my git was a bit confused by the diff included in the commit message. I'd suggest just saying that most of the commit is a whitespace change; people can see that using '-w' themselves. That toggle should even work with format-patch, but I'm not sure that applies cleanly all the time.
Ah I did not know git format-patch has such as switch. This produces a readable patch, but it's indeed not possible to apply it cleanly. I could have sent the readable version in reply to the full version though. I've removed the diff from the commit log, but on second thought I forgot to remove one 'always true' test so I'll send a v2. Christophe

On 08/29/2014 06:17 AM, Christophe Fergeau wrote:
Hey,
On Fri, Aug 29, 2014 at 11:08:53AM +0200, Martin Kletzander wrote:
Although my git was a bit confused by the diff included in the commit message. I'd suggest just saying that most of the commit is a whitespace change; people can see that using '-w' themselves. That toggle should even work with format-patch, but I'm not sure that applies cleanly all the time.
Ah I did not know git format-patch has such as switch. This produces a readable patch, but it's indeed not possible to apply it cleanly. I could have sent the readable version in reply to the full version though.
I've removed the diff from the commit log, but on second thought I forgot to remove one 'always true' test so I'll send a v2.
Sometimes, when a patch is that invasive, I'll do it in two parts - the change with wrong indentation, followed by another patch that is indentation-only. Much easier to review. Also, are you using the patience algorithm with git? (git config diff.algorithm patience) It makes indentation patches easier to review, by not trying to interleave lone } and blank lines that correspond to different code from pre- and post-patch. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

Hi, On Fri, Aug 29, 2014 at 10:22:33AM -0600, Eric Blake wrote:
Sometimes, when a patch is that invasive, I'll do it in two parts - the change with wrong indentation, followed by another patch that is indentation-only. Much easier to review.
Ah right, I remember seeing that in the past, I didn't think at all about doing that for this patch :( With 1.2.8 being so close, I'll push v2 though instead of doing this in a v3 as I'd rather not miss the release because of the additional round of review.
Also, are you using the patience algorithm with git? (git config diff.algorithm patience) It makes indentation patches easier to review, by not trying to interleave lone } and blank lines that correspond to different code from pre- and post-patch.
I've changed that now, and the diff is indeed much better, thanks for the tip! Christophe
participants (3)
-
Christophe Fergeau
-
Eric Blake
-
Martin Kletzander