After g_source_attach() the GMainContext holds a reference to the
GSource, so the caller does not need to keep it.

pty_chr_state() and qio_task_thread_worker() are not doing this, so
the GSource is being leaked in both cases (pty_chr_open_src_cancel()
is the exception here because it does remove the extra reference
correctly).

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 chardev/char-pty.c | 2 +-
 io/task.c          | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/chardev/char-pty.c b/chardev/char-pty.c
index f681d637c1..f16a5e8d59 100644
--- a/chardev/char-pty.c
+++ b/chardev/char-pty.c
@@ -60,7 +60,6 @@ static void pty_chr_open_src_cancel(PtyChardev *s)
 {
     if (s->open_source) {
         g_source_destroy(s->open_source);
-        g_source_unref(s->open_source);
         s->open_source = NULL;
     }
 }
@@ -216,6 +215,7 @@ static void pty_chr_state(Chardev *chr, int connected)
                                   qemu_chr_be_generic_open_func,
                                   chr, NULL);
             g_source_attach(s->open_source, chr->gcontext);
+            g_source_unref(s->open_source);
         }
         if (!chr->gsource) {
             chr->gsource = io_add_watch_poll(chr, s->ioc,
diff --git a/io/task.c b/io/task.c
index 2886a2c1bc..c8489fb790 100644
--- a/io/task.c
+++ b/io/task.c
@@ -120,6 +120,7 @@ static gpointer qio_task_thread_worker(gpointer opaque)
     idle = g_idle_source_new();
     g_source_set_callback(idle, qio_task_thread_result, data, NULL);
     g_source_attach(idle, data->context);
+    g_source_unref(idle);
 
     return NULL;
 }
-- 
2.11.0

On Thu, Feb 07, 2019 at 03:23:21PM +0200, Alberto Garcia wrote:
> After g_source_attach() the GMainContext holds a reference to the
> GSource, so the caller does not need to keep it.
> 
> pty_chr_state() and qio_task_thread_worker() are not doing this, so
> the GSource is being leaked in both cases (pty_chr_open_src_cancel()
> is the exception here because it does remove the extra reference
> correctly).
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  chardev/char-pty.c | 2 +-
>  io/task.c          | 1 +
>  2 files changed, 2 insertions(+), 1 deletion(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

On Thu, Feb 7, 2019 at 2:23 PM Alberto Garcia <berto@igalia.com> wrote:
>
> After g_source_attach() the GMainContext holds a reference to the
> GSource, so the caller does not need to keep it.
>
> pty_chr_state() and qio_task_thread_worker() are not doing this, so
> the GSource is being leaked in both cases (pty_chr_open_src_cancel()
> is the exception here because it does remove the extra reference
> correctly).
>
> Signed-off-by: Alberto Garcia <berto@igalia.com>

You could mention this is a fix for regression from
a17536c594bfed94d05667b419f747b692f5fc7f


Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>


> ---
>  chardev/char-pty.c | 2 +-
>  io/task.c          | 1 +
>  2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/chardev/char-pty.c b/chardev/char-pty.c
> index f681d637c1..f16a5e8d59 100644
> --- a/chardev/char-pty.c
> +++ b/chardev/char-pty.c
> @@ -60,7 +60,6 @@ static void pty_chr_open_src_cancel(PtyChardev *s)
>  {
>      if (s->open_source) {
>          g_source_destroy(s->open_source);
> -        g_source_unref(s->open_source);
>          s->open_source = NULL;
>      }
>  }
> @@ -216,6 +215,7 @@ static void pty_chr_state(Chardev *chr, int connected)
>                                    qemu_chr_be_generic_open_func,
>                                    chr, NULL);
>              g_source_attach(s->open_source, chr->gcontext);
> +            g_source_unref(s->open_source);
>          }
>          if (!chr->gsource) {
>              chr->gsource = io_add_watch_poll(chr, s->ioc,
> diff --git a/io/task.c b/io/task.c
> index 2886a2c1bc..c8489fb790 100644
> --- a/io/task.c
> +++ b/io/task.c
> @@ -120,6 +120,7 @@ static gpointer qio_task_thread_worker(gpointer opaque)
>      idle = g_idle_source_new();
>      g_source_set_callback(idle, qio_task_thread_result, data, NULL);
>      g_source_attach(idle, data->context);
> +    g_source_unref(idle);
>
>      return NULL;
>  }
> --
> 2.11.0
>

On Thu, Feb 7, 2019 at 3:23 PM Marc-André Lureau
<marcandre.lureau@redhat.com> wrote:
>
> On Thu, Feb 7, 2019 at 2:23 PM Alberto Garcia <berto@igalia.com> wrote:
> >
> > After g_source_attach() the GMainContext holds a reference to the
> > GSource, so the caller does not need to keep it.
> >
> > pty_chr_state() and qio_task_thread_worker() are not doing this, so
> > the GSource is being leaked in both cases (pty_chr_open_src_cancel()
> > is the exception here because it does remove the extra reference
> > correctly).
> >
> > Signed-off-by: Alberto Garcia <berto@igalia.com>
>
> You could mention this is a fix for regression from
> a17536c594bfed94d05667b419f747b692f5fc7f
>

and 938eb9e9c83d34d90cac6ec5c5388e7998840e4e


>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>
>
> > ---
> >  chardev/char-pty.c | 2 +-
> >  io/task.c          | 1 +
> >  2 files changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/chardev/char-pty.c b/chardev/char-pty.c
> > index f681d637c1..f16a5e8d59 100644
> > --- a/chardev/char-pty.c
> > +++ b/chardev/char-pty.c
> > @@ -60,7 +60,6 @@ static void pty_chr_open_src_cancel(PtyChardev *s)
> >  {
> >      if (s->open_source) {
> >          g_source_destroy(s->open_source);
> > -        g_source_unref(s->open_source);
> >          s->open_source = NULL;
> >      }
> >  }
> > @@ -216,6 +215,7 @@ static void pty_chr_state(Chardev *chr, int connected)
> >                                    qemu_chr_be_generic_open_func,
> >                                    chr, NULL);
> >              g_source_attach(s->open_source, chr->gcontext);
> > +            g_source_unref(s->open_source);
> >          }
> >          if (!chr->gsource) {
> >              chr->gsource = io_add_watch_poll(chr, s->ioc,
> > diff --git a/io/task.c b/io/task.c
> > index 2886a2c1bc..c8489fb790 100644
> > --- a/io/task.c
> > +++ b/io/task.c
> > @@ -120,6 +120,7 @@ static gpointer qio_task_thread_worker(gpointer opaque)
> >      idle = g_idle_source_new();
> >      g_source_set_callback(idle, qio_task_thread_result, data, NULL);
> >      g_source_attach(idle, data->context);
> > +    g_source_unref(idle);
> >
> >      return NULL;
> >  }
> > --
> > 2.11.0
> >

On Thu 07 Feb 2019 03:24:40 PM CET, Marc-André Lureau wrote:
> On Thu, Feb 7, 2019 at 3:23 PM Marc-André Lureau
> <marcandre.lureau@redhat.com> wrote:
>>
>> On Thu, Feb 7, 2019 at 2:23 PM Alberto Garcia <berto@igalia.com> wrote:
>> >
>> > After g_source_attach() the GMainContext holds a reference to the
>> > GSource, so the caller does not need to keep it.
>> >
>> > pty_chr_state() and qio_task_thread_worker() are not doing this, so
>> > the GSource is being leaked in both cases (pty_chr_open_src_cancel()
>> > is the exception here because it does remove the extra reference
>> > correctly).
>> >
>> > Signed-off-by: Alberto Garcia <berto@igalia.com>
>>
>> You could mention this is a fix for regression from
>> a17536c594bfed94d05667b419f747b692f5fc7f
>
> and 938eb9e9c83d34d90cac6ec5c5388e7998840e4e

Ok, feel free to add this to the commit message if I don't need to
resend the series.

Berto

Hi

On Thu, Feb 7, 2019 at 3:37 PM Alberto Garcia <berto@igalia.com> wrote:
>
> On Thu 07 Feb 2019 03:24:40 PM CET, Marc-André Lureau wrote:
> > On Thu, Feb 7, 2019 at 3:23 PM Marc-André Lureau
> > <marcandre.lureau@redhat.com> wrote:
> >>
> >> On Thu, Feb 7, 2019 at 2:23 PM Alberto Garcia <berto@igalia.com> wrote:
> >> >
> >> > After g_source_attach() the GMainContext holds a reference to the
> >> > GSource, so the caller does not need to keep it.
> >> >
> >> > pty_chr_state() and qio_task_thread_worker() are not doing this, so
> >> > the GSource is being leaked in both cases (pty_chr_open_src_cancel()
> >> > is the exception here because it does remove the extra reference
> >> > correctly).
> >> >
> >> > Signed-off-by: Alberto Garcia <berto@igalia.com>
> >>
> >> You could mention this is a fix for regression from
> >> a17536c594bfed94d05667b419f747b692f5fc7f
> >
> > and 938eb9e9c83d34d90cac6ec5c5388e7998840e4e
>
> Ok, feel free to add this to the commit message if I don't need to
> resend the series.

Well, the series conflicts with the currently queued chardev series. I
think I will send the pullreq and let you rebase.

(I also have a related series pending review "[PATCH v2 0/6] chardev:
various cleanups and improvements")


-- 
Marc-André Lureau

On Thu 07 Feb 2019 03:43:59 PM CET, Marc-André Lureau wrote:

> Well, the series conflicts with the currently queued chardev series. I
> think I will send the pullreq and let you rebase.

I'll wait for it then

Berto

On Thu 07 Feb 2019 02:23:21 PM CET, Alberto Garcia wrote:
> After g_source_attach() the GMainContext holds a reference to the
> GSource, so the caller does not need to keep it.
>
> pty_chr_state() and qio_task_thread_worker() are not doing this, so

s/are not doing this/are not releasing their references/

I'll correct this if I need to send a new version of the series,
otherwise please fix when committing this patch.

Berto