[virt-tools-list] [virt-viewer][PATCH] Revert "virt-viewer: set keepAlive on libvirt connection"

Thu Jul 16 16:07:59 UTC 2015

----- Original Message -----
> From: "Fabiano Fidencio" <ffidenci at redhat.com>
> To: "Christophe Fergeau" <cfergeau at redhat.com>
> Cc: virt-tools-list at redhat.com
> Sent: Thursday, July 16, 2015 6:01:14 PM
> Subject: Re: [virt-tools-list] [virt-viewer][PATCH] Revert "virt-viewer: set keepAlive on libvirt connection"
> 
> 
> 
> ----- Original Message -----
> > From: "Christophe Fergeau" <cfergeau at redhat.com>
> > To: "Fabiano Fidencio" <ffidenci at redhat.com>
> > Cc: virt-tools-list at redhat.com
> > Sent: Thursday, July 16, 2015 5:19:32 PM
> > Subject: Re: [virt-tools-list] [virt-viewer][PATCH] Revert "virt-viewer:
> > set keepAlive on libvirt connection"
> > 
> > On Thu, Jul 16, 2015 at 11:01:06AM -0400, Fabiano Fidencio wrote:
> > > 
> > > 
> > > ----- Original Message -----
> > > > From: "Fabiano Fidencio" <ffidenci at redhat.com>
> > > > To: virt-tools-list at redhat.com
> > > > Sent: Thursday, July 16, 2015 4:45:42 PM
> > > > Subject: Re: [virt-tools-list] [virt-viewer][PATCH] Revert
> > > > "virt-viewer:
> > > > set keepAlive on libvirt connection"
> > > > 
> > > > Howdy!
> > > > 
> > > > ----- Original Message -----
> > > > > From: "Fabiano Fidêncio" <fidencio at redhat.com>
> > > > > To: virt-tools-list at redhat.com
> > > > > Cc: "Fabiano Fidêncio" <fidencio at redhat.com>
> > > > > Sent: Thursday, July 16, 2015 2:04:33 PM
> > > > > Subject: [virt-viewer][PATCH] Revert "virt-viewer: set keepAlive on
> > > > > libvirt
> > > > > connection"
> > > > > 
> > > > > This reverts commit 08378ec4dc3623792c64a3bae6279eac1c3c153e.
> > > > > 
> > > > > The commit in question was done in order to fix rhbz#1164052, but has
> > > > > been proven that it's not necessary depending on the libvirt version
> > > > > on
> > > > > the remote host.
> > > > > Considering we don't want to keep a workaround that can, actually,
> > > > > hide
> > > > > a proper bug that must be fixed (or in virt-viewer, or elsewhere),
> > > > > reverting this patch seems the safest option for now.
> > > > 
> > > > Jonathon asked me on IRC:
> > > > 11:08 <~ jjongsma> fidencio, any additional context about that patch
> > > > reverting the keepalive?
> > > > 11:08 <~ jjongsma> which versions is it unnecessary for? How did you
> > > > find
> > > > that? etc.
> > > > 11:13 <@ fidencio> jjongsma: hmmm. I will add more info there.
> > > > 
> > > > So, to be added to the commit message:
> > > > 
> > > > The patch was not necessary when connecting to a rhel6 host
> > > > (libvirt-0.10.2-54) neither connecting to a fedora22 host
> > > > (libvirt-1.2.13.1-2). The problem could be reproduce on rhel7 host
> > > > (libvirt-1.2.17-1). And these are the systems that I have installed on
> > > > my
> > > > remote host machine.
> > > 
> > > Actually, I still can reproduce the issue on a fedora22 machine.
> > > Hmm. Maybe would be better adding a big comment there than reverting the
> > > patch.
> > 
> > Maybe it's a change of behaviour in newer libvirt releases? older ones
> > did not need keep alive, newer ones do?
> 
> Hmm. Maybe.
> https://bugzilla.redhat.com/show_bug.cgi?id=832081#c21
> 
> The patch seems to be hiding a crash on libvirt ... see these 2 backtraces
> (related to rhbz#1243228):
> 
> With the patch applied:
> (gdb) where
> #0  0x00007efcae715095 in g_io_create_watch () from /lib64/libglib-2.0.so.0
> #1  0x00007efcae7150ef in g_io_add_watch_full () from /lib64/libglib-2.0.so.0
> #2  0x00000000004275ba in virt_viewer_events_update_handle (watch=<optimized
> out>, events=1) at
> virt-viewer-events.c:158
> #3  0x00007efcb1a62dce in virNetSocketUpdateIOCallback (sock=0x1e75c00,
> events=1) at rpc/virnetsocket.c:1981
> #4  0x00007efcb1a50113 in virNetClientIOUpdateCallback (client=<optimized
> out>, enableCallback=<optimized out>) at
> rpc/virnetclient.c:1639
> #5  0x00007efcb1a50f82 in virNetClientIO (thiscall=0x20e0170,
> client=0x1f2e060) at rpc/virnetclient.c:1793
> #6  virNetClientSendInternal (client=client at entry=0x1f2e060,
> msg=msg at entry=0x20e0100,
> expectReply=expectReply at entry=false, nonBlock=nonBlock at entry=true) at
> rpc/virnetclient.c:1962
> #7  0x00007efcb1a52413 in virNetClientSendNonBlock (client=0x1f2e060,
> msg=msg at entry=0x20e0100) at
> rpc/virnetclient.c:2036
> #8  0x00007efcb1a5243d in virNetClientKeepAliveSendCB (opaque=<optimized
> out>, msg=0x20e0100) at
> rpc/virnetclient.c:293
> #9  0x00007efcb1a5ba02 in virKeepAliveTimer (timer=<optimized out>,
> opaque=0x20d3d00) at rpc/virkeepalive.c:176
> #10 0x00000000004272e9 in virt_viewer_events_dispatch_timeout
> (opaque=0x1e6cd30) at virt-viewer-events.c:233
> #11 0x00007efcae7231b3 in g_timeout_dispatch () from /lib64/libglib-2.0.so.0
> #12 0x00007efcae72279a in g_main_context_dispatch () from
> /lib64/libglib-2.0.so.0
> #13 0x00007efcae722ae8 in g_main_context_iterate.isra.24 () from
> /lib64/libglib-2.0.so.0
> #14 0x00007efcae722dba in g_main_loop_run () from /lib64/libglib-2.0.so.0
> #15 0x00007efcb054a045 in gtk_main () from /lib64/libgtk-3.so.0
> #16 0x0000000000410a9c in main (argc=1, argv=0x7ffde58a7978) at
> virt-viewer-main.c:124
> 
> Without:
> (gdb) where
> #0  0x00007f2dba9bf2f0 in virClassIsDerivedFrom (klass=0x48,
> parent=0x2bd45c0) at util/virobject.c:169
> #1  0x00007f2dba9bf62e in virObjectIsClass (anyobj=anyobj at entry=0x2bd3860,
> klass=<optimized out>) at
> util/virobject.c:365
> #2  0x00007f2dba9bf654 in virObjectLock (anyobj=0x2bd3860) at
> util/virobject.c:317
> #3  0x00007f2dba994da5 in virDispatchError (conn=conn at entry=0x2bd3860) at
> util/virerror.c:620
> #4  0x00007f2dbaa80958 in virConnectDomainEventDeregisterAny (conn=0x2bd3860,
> callbackID=0) at libvirt-domain.c:9282
> #5  0x00000000004294d2 in virt_viewer_dispose (object=0x2c2b180) at
> virt-viewer.c:644
> #6  0x00007f2db7aaaa82 in g_object_unref () from /lib64/libgobject-2.0.so.0
> #7  0x0000000000410a22 in main (argc=1, argv=0x7ffc6a948c38) at
> virt-viewer-main.c:130
> 
> An important not is that I was not able to reproduce none of the crashes by
> myself. :-(

Anyway, regardless of the crashes, that happen with or without the patch according to the reporter, I guess we can keep the patch in the end and see if it will cause any kind of problem in the future.

Best Regards,
--
Fabiano Fidêncio