| Message ID | c741408825d01f5a186901c5a8248126ffebe9fd.1549545591.git.berto@igalia.com |
|---|---|
| State | New |
| Headers | show |
| Series | char-socket: Fix race condition | expand |
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
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
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")
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
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; }
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(-)