On Mon, Sep 10, 2012 at 12:30:09PM +0200, Christophe Fergeau wrote:
e5a1bee07 introduced a regression in Boxes: when Boxes is left idle
(it's still doing some libvirt calls in the background), the
libvirt connection gets closed after a few minutes. What happens is
that this code in virNetClientIOHandleOutput gets triggered:
if (!thecall)
return -1; /* Shouldn't happen, but you never know... */
and after the changes in e5a1bee07, this causes the libvirt connection
to be closed.
Upon further investigation, what happens is that
virNetClientIOHandleOutput is called from gvir_event_handle_dispatch
in libvirt-glib, which is triggered because the client fd became
writable. However, between the times gvir_event_handle_dispatch
is called, and the time the client lock is grabbed and
virNetClientIOHandleOutput is called, another thread runs and
completes the current call. 'thecall' is then NULL when the first
thread gets to run virNetClientIOHandleOutput.
After describing this situation on IRC, danpb suggested this:
11:37 < danpb> In that case I think the correct thing would be to change
'return -1' above to 'return 0' since that's not
actually an
error - its a rare, but expected event
which is what this patch is doing. I've tested it against master
libvirt, and I didn't get disconnected in ~10 minutes while this
happens in less than 5 minutes without this patch.
---
src/rpc/virnetclient.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c
index 43a9814..727ed67 100644
--- a/src/rpc/virnetclient.c
+++ b/src/rpc/virnetclient.c
@@ -1205,7 +1205,10 @@ virNetClientIOHandleOutput(virNetClientPtr client)
thecall = thecall->next;
if (!thecall)
- return -1; /* Shouldn't happen, but you never know... */
+ return 0; /* This can happen if another thread raced with us and
+ * completed the call between the time this thread woke
+ * up from poll()ing and the time we locked the client
+ */
while (thecall) {
ssize_t ret = virNetClientIOWriteMessage(client, thecall);
Considering how the code worked in the past, the comment was actually
correct originally. We never invoked virNetClientIOHandleOutput from
the event handler callback - we only used the callback for dealing
with *incoming* I/O. Only when we recently changed the keepalive
code to make use of the event handler callbacks did we now start
doing outgoiing I/O. This invalidated the comment & return code we
used here, but introducing the scenario described above.
ACK
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 :|