[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] Is it a problem that after virEventRegisterDefaultImpl we have handlers leaked




> -----Original Message-----
> From: Daniel P. Berrange [mailto:berrange redhat com]
> Sent: Tuesday, September 03, 2013 6:30 PM
> To: Wangyufei (A)
> Cc: libvir-list redhat com; Wangrui (K)
> Subject: Re: [libvirt] Is it a problem that after virEventRegisterDefaultImpl we
> have handlers leaked
> 
> On Tue, Sep 03, 2013 at 10:25:45AM +0000, Wangyufei (A) wrote:
> >
> >
> > > -----Original Message-----
> > > From: Daniel P. Berrange [mailto:berrange redhat com]
> > > Sent: Tuesday, September 03, 2013 5:14 PM
> > > To: Wangyufei (A)
> > > Cc: libvir-list redhat com; Wangrui (K)
> > > Subject: Re: [libvirt] Is it a problem that after virEventRegisterDefaultImpl
> we
> > > have handlers leaked
> > >
> > > On Mon, Sep 02, 2013 at 07:44:39AM +0000, Wangyufei (A) wrote:
> > > > Hello,
> > > > When I ran programme event-test compiled from event-test.c, I found
> a
> > > problem that, after virEventRegisterDefaultImpl I do
> virConnectOpenAuth
> > > and virConnectClose, there will be handlers of socket and pipe opened by
> > > virConnectOpenAuth leaked after virConnectClose. So I did some
> analysis,
> > > and I found the fact following:
> > > >
> > > > In the condition that we only have one connection here
> > > >
> > > > int virNetSocketAddIOCallback(virNetSocketPtr sock,
> > > >                               int events,
> > > >                               virNetSocketIOFunc func,
> > > >                               void *opaque,
> > > >                               virFreeCallback ff)
> > > > {
> > > >     int ret = -1;
> > > >
> > > >     virObjectRef(sock); //Here we add sock refers once, then we will
> get
> > > refers equal 2 after
> > > >     virObjectLock(sock);
> > > >     if (sock->watch > 0) {
> > > >         VIR_DEBUG("Watch already registered on socket %p", sock);
> > > >         goto cleanup;
> > > >     }
> > > >
> > > >     if ((sock->watch = virEventAddHandle(sock->fd, //If we have
> called
> > > virEventRegisterDefaultImpl, then here we'll get a sock watch non
> negative
> > > and will not go to cleanup.
> > > >                                          events,
> > > >
> > > virNetSocketEventHandle,
> > > >                                          sock,
> > > >
> virNetSocketEventFree))
> > > < 0) {
> > > >         VIR_DEBUG("Failed to register watch on socket %p", sock);
> > > >         goto cleanup;
> > > >     }
> > > >     sock->func = func;
> > > >     sock->opaque = opaque;
> > > >     sock->ff = ff;
> > > >
> > > >     ret = 0;
> > > >
> > > > cleanup:
> > > >     virObjectUnlock(sock);
> > > >     if (ret != 0)
> > > >         virObjectUnref(sock); //If we haven't called
> > > virEventRegisterDefaultImpl, we'll be here after virEventAddHandle, and
> > > sock refers will decrease to 1
> > > >     return ret;
> > > > }
> > > >
> > > > Condition with virEventRegisterDefaultImpl, we'll do unrefer action in
> two
> > > places:
> > > >
> > > > 1.       virEventRunDefaultImpl  ->virEventPollRunOnce
> > > ->virEventRunDefaultImpl  ->virEventPollRunOnce
> > > ->virEventPollCleanupHandles -> virNetSocketEventFree ->
> virObjectUnref
> > > >
> > > > 2.       virConnectClose ->virObjectUnref ->virConnectDispose
> > > ->remoteConnectClose  ->doRemoteClose  ->virNetClientClose
> > > ->virNetClientCloseInternal -> virNetClientIOEventLoopPassTheBuck ->
> > > virNetClientCloseLocked -> virObjectUnref
> > > >
> > > > When event dealing loop is alive, everything work fine, we'll get sock
> > > refers 2
> > > > after virConnectOpenAuth and unrefer twice in two places above after
> > > virConnectClose.
> > > > But If some accidents happened like we quit event dealing loop or
> > > virEventRunDefaultImpl
> > > > suspended, then sock refers will never be zero, and handlers will never
> be
> > > freed.
> > >
> > > Do not stop running the event loop. It is a requirement of the API that
> once
> > > you have
> > > called  virEventRegisterDefaultImpl, you *must* always execute the
> event
> > > loop forever
> > > after until your application exits.
> > >
> > >
> > > > I consider to add something like virEventDeregisterDefaultImpl to set
> > > addHandleImpl and his buddy NULL. Apparently it is far away from fixing
> it
> > > completely.
> > >
> > > No, stopping or de-registering event loop impls is a non-goal.
> > >
> > > > At last I have two questions here:
> > > >
> > > >
> > > > 1.       Is it a problem that after virEventRegisterDefaultImpl we
> have
> > > handlers leaked?
> > >
> > > There are no handlers leaked if you run the event loop, which is a
> > > requirement
> > > of the API.
> >
> > Well, Is there any tiny chance that event loop stopped?
> > If that happened now, the handlers leak will affect the whole host OS,
> maybe no handlers to use at last.
> > Is that what we expect?
> > Can we do something to reduce the impact even if something impossible
> happened?
> 
> If the event loop has bugs which cause it to stop then those
> should be fixed. This isn't something we need / want to work
> around elsewhere.

Fine, thanks a lot.
> 
> 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 :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]