Re: [libvirt] virNetClientPtr leak in remote driver
2011/8/2 Daniel P. Berrange <berrange(a)redhat.com>:
On Mon, Aug 01, 2011 at 06:35:21PM +0200, Matthias Bolte wrote:
> 2011/8/1 Eric Blake <eblake(a)redhat.com>:
> > On 07/28/2011 12:07 PM, Matthias Bolte wrote:
> >> 2011/7/27 Matthias Bolte <matthias.bolte(a)googlemail.com>:
> >>> doRemoteClose doesn't free the virNetClientPtr and this creates a
> >>> 260kb leak per remote connection. This happens because
> >>> virNetClientFree doesn't remove the last ref, because
virNetClientNew
> >>> creates the virNetClientPtr with a refcount of 2.
> >>>
> >
> >> The memory leak I saw was due to virsh calling
> >> virEventRegisterDefaultImpl, but not calling virEventRunDefaultImpl.
> >> Because the event loop is initialized, the call to
> >> virNetSocketAddIOCallback in virNetClientNew succeeds. But virsh not
> >> driving the event loop results in not removing callbacks that were
> >> marked for deletion. Finally this leaks the virNetClientPtr using in
> >> the remote driver. I used a virsh -c qemu:///system to test.
> >>
> >> I was able fix this by calling virEventRunDefaultImpl after
> >> virConnectClose in virsh. But I don't think that this is the correct
> >> general solution.
> >
> > Where do we stand with 0.9.4? Is this something where we need the
> > general fix before the release, or is just the virsh hack to call
> > virEventRunDefaultImpl good enough for the release, or is this problem
> > not severe enough and we can release as-is?
>
> virsh is a bit special here as it registers/initializes the default
> even loop but doesn't drive it properly. It only drives it for the
> console command. That's the problem in virsh. This can be avoided by
> calling virEventRunDefaultImpl after virConnectClose iff it's a remote
> connection, in other cases virEventRunDefaultImpl might block.
> Therefore, we shouldn't be calling it in general after a
> virConnectClose in virsh.
>
> If we assume that an application that registers an event loop will
> drive it properly then this problem is not critical, as it doesn't
> exist. Just virsh is a special case that leaks 260kb per closed remote
> connection. When we assume that a typical virsh user uses a single
> connection per virsh invocation then we can view this as a static
> leak.
Yeah, this is a case I never thought of. The "easy" fix is to just
make virsh spawn a new thread to run the event loop in the background.
The problem here will probably be the console command with this lines
while (!con->quit) {
if (virEventRunDefaultImpl() < 0)
break;
}
in console.c. Having two threads calling virEventRunDefaultImpl
probably isn't a good idea.
The "hard" fix is to make virsh I/O entirely event driven,
so that
we don't just sit in a blocking read of stdin waiting for input,
but instead use the event loop to process stdin.
Daniel
--
Matthias Bolte
http://photron.blogspot.com