On Mon, Feb 27, 2012 at 05:25:58PM +0100, Marc-André Lureau wrote:
> 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@xxxxxxxxxx>
> 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)
ACK
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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list