[libvirt] Fix Xen virConnectPtr leak

The commit cb51aa48a777ddae6997faa9f28350cb62655ffd "Fix up connection reference counting." changed the driver closing and virConnectPtr unref-logic in virConnectClose(). Before this commit virConnectClose() closed all drivers of the given virConnectPtr and virUnrefConnect()'ed it afterwards. After this commit the driver-closing is done in virUnrefConnect() if and only if the ref-count of the virConnectPtr dropped to zero. This change in execution order leads to a virConnectPtr leak, at least for connections to Xen. The relevant call sequences: virConnectOpen() -> xenUnifiedOpen() ... ... xenInotifyOpen() -> virConnectRef(conn) ... xenStoreOpen() -> xenStoreAddWatch() -> conn->refs++ virConnectClose() -> xenUnifiedClose() ... ... xenInotifyClose() -> virUnrefConnect(conn) ... xenStoreClose() -> xenStoreRemoveWatch() -> virUnrefConnect(conn) Before the commit this additional virConnectRef/virUnrefConnect calls where no problem, because virConnectClose() closed the drivers explicitly and the additional refs added by the Xen subdrivers were removed properly. After the commit this additional refs result in a virConnectPtr leak (including a leak of the hypercall file handle; that's how I noticed this problem), because now the drivers are only close if and only if the ref-count drops to zero, but this cannot happen anymore, because the additional refs from the Xen subdrivers would only be removed if the drivers get closed, but that doesn't happen because the ref-count cannot drop to zero. The fix for this problem is simple: remove the virConnectRef/virUnrefConnect calls from the Xen subdrivers (see attached patch). Maybe someone could explain why the Xen Inotify and Xen Store driver do this extra ref-counting, but none of the other Xen subdrivers. It seems unnecessary to me and can be removed without problems. Matthias

Matthias Bolte wrote:
The fix for this problem is simple: remove the virConnectRef/virUnrefConnect calls from the Xen subdrivers (see attached patch). Maybe someone could explain why the Xen Inotify and Xen Store driver do this extra ref-counting, but none of the other Xen subdrivers. It seems unnecessary to me and can be removed without problems.
It seems like a perfectly reasonable fix to me. I would like to get a comment from Dan Berrange about it, just to be sure. -- Chris Lalancette

On Sat, Sep 19, 2009 at 11:50:21AM +0200, Matthias Bolte wrote:
The commit cb51aa48a777ddae6997faa9f28350cb62655ffd "Fix up connection reference counting." changed the driver closing and virConnectPtr unref-logic in virConnectClose().
Before this commit virConnectClose() closed all drivers of the given virConnectPtr and virUnrefConnect()'ed it afterwards. After this commit the driver-closing is done in virUnrefConnect() if and only if the ref-count of the virConnectPtr dropped to zero.
[snip]
The fix for this problem is simple: remove the virConnectRef/virUnrefConnect calls from the Xen subdrivers (see attached patch). Maybe someone could explain why the Xen Inotify and Xen Store driver do this extra ref-counting, but none of the other Xen subdrivers. It seems unnecessary to me and can be removed without problems.
Yes, this ref counting stuff in Xen internal drivers is totally bogus and can be safely removed. ACk to your patch Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

Matthias Bolte wrote:
The fix for this problem is simple: remove the virConnectRef/virUnrefConnect calls from the Xen subdrivers (see attached patch). Maybe someone could explain why the Xen Inotify and Xen Store driver do this extra ref-counting, but none of the other Xen subdrivers. It seems unnecessary to me and can be removed without problems.
I've committed this patch now (after rebasing it on the new directory layout). Thanks. -- Chris Lalancette
participants (3)
-
Chris Lalancette
-
Daniel P. Berrange
-
Matthias Bolte