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

[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 redhat com>
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)
-- 
1.7.7.6


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