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 :|