Re: [Qemu-devel] [PATCH 2/3] main-loop: Add qemu_idle_add()

[Marc-André Lureau]

Hi

On Thu, Feb 7, 2019 at 2:23 PM Alberto Garcia <address@hidden> wrote:
>
> This works like g_idle_add() but allows specifying a different
> GMainContext. It also returns the GSource directly instead of its ID
> number for convenience.
>
> pty_chr_state() and qio_task_thread_worker() are modified to make use
> of this new function.
>
> Signed-off-by: Alberto Garcia <address@hidden>
> ---
>  chardev/char-pty.c       | 8 ++------
>  include/qemu/main-loop.h | 8 ++++++++
>  io/task.c                | 7 ++-----
>  util/main-loop.c         | 9 +++++++++
>  4 files changed, 21 insertions(+), 11 deletions(-)
>
> diff --git a/chardev/char-pty.c b/chardev/char-pty.c
> index f16a5e8d59..6562c38f39 100644
> --- a/chardev/char-pty.c
> +++ b/chardev/char-pty.c
> @@ -209,13 +209,9 @@ static void pty_chr_state(Chardev *chr, int connected)
>          pty_chr_timer_cancel(s);
>          if (!s->connected) {
>              g_assert(s->open_source == NULL);
> -            s->open_source = g_idle_source_new();
> +            s->open_source = qemu_idle_add(qemu_chr_be_generic_open_func,
> +                                           chr, chr->gcontext);
>              s->connected = 1;
> -            g_source_set_callback(s->open_source,
> -                                  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/include/qemu/main-loop.h b/include/qemu/main-loop.h
> index e59f9ae1e9..1865afd993 100644
> --- a/include/qemu/main-loop.h
> +++ b/include/qemu/main-loop.h
> @@ -295,6 +295,14 @@ void qemu_mutex_lock_iothread_impl(const char *file, int 
> line);
>   */
>  void qemu_mutex_unlock_iothread(void);
>
> +/**
> + * qemu_idle_add: Add an idle function to a GMainContext
> + *
> + * This function is similar to GLib's g_idle_add() but it allows
> + * specifying a GMainContext instead of using the global one.
> + */
> +GSource *qemu_idle_add(GSourceFunc func, gpointer data, GMainContext *ctx);

I think you should document that returned GSource* isn't ref/owned.

> +
>  /* internal interfaces */
>
>  void qemu_fd_register(int fd);
> diff --git a/io/task.c b/io/task.c
> index c8489fb790..bdd395b0ad 100644
> --- a/io/task.c
> +++ b/io/task.c
> @@ -19,6 +19,7 @@
>   */
>
>  #include "qemu/osdep.h"
> +#include "qemu/main-loop.h"
>  #include "io/task.h"
>  #include "qapi/error.h"
>  #include "qemu/thread.h"
> @@ -105,7 +106,6 @@ static gboolean qio_task_thread_result(gpointer opaque)
>  static gpointer qio_task_thread_worker(gpointer opaque)
>  {
>      struct QIOTaskThreadData *data = opaque;
> -    GSource *idle;
>
>      trace_qio_task_thread_run(data->task);
>      data->worker(data->task, data->opaque);
> @@ -117,10 +117,7 @@ static gpointer qio_task_thread_worker(gpointer opaque)
>       */
>      trace_qio_task_thread_exit(data->task);
>
> -    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);
> +    qemu_idle_add(qio_task_thread_result, data, data->context);
>
>      return NULL;
>  }
> diff --git a/util/main-loop.c b/util/main-loop.c
> index 443cb4cfe8..1c68e3bf5f 100644
> --- a/util/main-loop.c
> +++ b/util/main-loop.c
> @@ -171,6 +171,15 @@ int qemu_init_main_loop(Error **errp)
>      return 0;
>  }
>
> +GSource *qemu_idle_add(GSourceFunc func, gpointer data, GMainContext *ctx)
> +{
> +    GSource *idle = g_idle_source_new();
> +    g_source_set_callback(idle, func, data, NULL);
> +    g_source_attach(idle, ctx);
> +    g_source_unref(idle);
> +    return idle;
> +}
> +
>  static int max_priority;
>
>  #ifndef _WIN32
> --
> 2.11.0
>

looks ok otherwise
Reviewed-by: Marc-André Lureau <address@hidden>