Re: [libvirt] [libvirt-glib] a leak in libvirt-glib

Hi, In libvirt-glib, we call virStreamEventAddCallback() and give it a ref, that is supposed to be unref when the stream event is removed. But this doesn't happen! I tracked it down to: static void remoteStreamCallbackFree(void *opaque) {     struct remoteStreamCallbackData *cbdata = opaque;     if (!cbdata->cb && cbdata->ff)         (cbdata->ff)(cbdata->opaque); Why are we checking for cbdata->cb here? That gives us a leak when taking screenshots. So far I solved it with the attached patch for libvirt-glib. I noticed it because that of a resulting process & fd leakage in the libvirtd side. It might be that the fix should be in libvirt, but anyway, the proposed patch doesn't need a libvirt depedency update and also keeps the object reference manangement at the libvirt-glib level, which is nice. -- Marc-André Lureau

From e3ff31c92edd2390def3226647681cc7252c1a1a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Lureau?= <marcandre.lureau(a)redhat.com&gt; Date: Thu, 2 Feb 2012 21:22:42 +0100 Subject: [PATCH libvirt-glib] Don't leak GVirStream Do not give away a reference to virStreamEventAddCallback, but manage stream life-time and event instead. It seems to be a bug in current implementation of libvirt remoteStreamCallbackFree() that doesn't call the free/notify cb for some reason. (even if it did, I am not sure it would work correctly, so I prefer that patch) --- libvirt-gobject/libvirt-gobject-stream.c | 29 ++++++++++++++++++----------- 1 files changed, 18 insertions(+), 11 deletions(-) diff --git a/libvirt-gobject/libvirt-gobject-stream.c b/libvirt-gobject/libvirt-gobject-stream.c index a1039b1..5486b95 100644 --- a/libvirt-gobject/libvirt-gobject-stream.c +++ b/libvirt-gobject/libvirt-gobject-stream.c @@ -67,6 +67,7 @@ enum { #define GVIR_STREAM_ERROR gvir_stream_error_quark() +static void gvir_stream_update_events(GVirStream *stream); static GQuark gvir_stream_error_quark(void) @@ -204,13 +205,6 @@ static void gvir_stream_finalize(GObject *object) if (self->priv->input_stream) g_object_unref(self->priv->input_stream); - if (priv->handle) { - if (virStreamFinish(priv->handle) < 0) - g_critical("cannot finish stream"); - - virStreamFree(priv->handle); - } - tmp = priv->sources; while (tmp) { GVirStreamSource *source = tmp->data; @@ -218,6 +212,16 @@ static void gvir_stream_finalize(GObject *object) tmp = tmp->next; } g_list_free(priv->sources); + priv->sources = NULL; + + if (priv->handle) { + gvir_stream_update_events(self); + + if (virStreamFinish(priv->handle) < 0) + g_critical("cannot finish stream"); + + virStreamFree(priv->handle); + } G_OBJECT_CLASS(gvir_stream_parent_class)->finalize(object); } @@ -550,8 +554,8 @@ static void gvir_stream_update_events(GVirStream *stream) } else { virStreamEventAddCallback(priv->handle, mask, gvir_stream_handle_events, - g_object_ref(stream), - g_object_unref); + stream, + NULL); priv->eventRegistered = TRUE; } } else { @@ -600,8 +604,9 @@ static void gvir_stream_source_finalize(GSource *source) GVirStreamPrivate *priv = gsource->stream->priv; priv->sources = g_list_remove(priv->sources, source); - gvir_stream_update_events(gsource->stream); + + g_clear_object(&gsource->stream); } GSourceFuncs gvir_stream_source_funcs = { @@ -657,12 +662,14 @@ guint gvir_stream_add_watch_full(GVirStream *stream, gpointer opaque, GDestroyNotify notify) { + g_return_val_if_fail(GVIR_IS_STREAM(stream), 0); + GVirStreamPrivate *priv = stream->priv; GVirStreamSource *source = (GVirStreamSource*)g_source_new(&gvir_stream_source_funcs, sizeof(GVirStreamSource)); guint ret; - source->stream = stream; + source->stream = g_object_ref(stream); source->cond = cond; if (priority != G_PRIORITY_DEFAULT)