[Spice-devel] [PATCH v2 05/13] smartcard-manager: Use GTask instead of GSimpleAsyncResult

[Jonathon Jongsma]

A couple of comments below unrelated to your changes, but since I was looking at
this code...

On Fri, 2016-02-12 at 10:46 +0100, Fabiano Fidêncio wrote:
> Instead of using GSimpleAsyncResult, use the new GTask API, which is
> much more straightforward.
> ---
>  src/smartcard-manager.c | 34 +++++++++++++---------------------
>  1 file changed, 13 insertions(+), 21 deletions(-)
> 
> diff --git a/src/smartcard-manager.c b/src/smartcard-manager.c
> index 6578328..7cdd881 100644
> --- a/src/smartcard-manager.c
> +++ b/src/smartcard-manager.c
> @@ -476,8 +476,9 @@ end:
>      return retval;
>  }
>  
> -static void smartcard_manager_init_helper(GSimpleAsyncResult *res,
> -                                          GObject *object,
> +static void smartcard_manager_init_helper(GTask *task,
> +                                          gpointer object,
> +                                          gpointer task_data,
>                                            GCancellable *cancellable)
>  {
>      static GOnce smartcard_manager_once = G_ONCE_INIT;
> @@ -491,9 +492,9 @@ static void
> smartcard_manager_init_helper(GSimpleAsyncResult *res,
>      g_once(&smartcard_manager_once,
>             (GThreadFunc)smartcard_manager_init,
>             &args);

Do we really want to use g_once() here? It does provide thread-safety, but it
also means that if smartcard_manager_init() fails the first time we call it, it
will return the same failure on subsequent calls. It seems like it would be
smarter to try to initialize again the next time this function is called...

> +
>      if (args.err != NULL) {
> -        g_simple_async_result_set_from_error(res, args.err);
> -        g_error_free(args.err);
> +        g_task_return_error(task, args.err);

Here we return an error, but we never return a boolean success value when there
is no error. It turns out that it doesn't really matter because the only place
that calls spice_smartcard_manager_init_finish() does not actually use the
boolean return value. But probably we should return a TRUE value if there is no
error.

>      }
>  }
>  
> @@ -504,17 +505,11 @@ void spice_smartcard_manager_init_async(SpiceSession
> *session,
>                                          GAsyncReadyCallback callback,
>                                          gpointer opaque)
>  {
> -    GSimpleAsyncResult *res;
> +    GTask *task = g_task_new(session, cancellable, callback, opaque);
> +    g_task_set_source_tag(task, spice_smartcard_manager_init);

This source tag stuff doesn't seem particularly useful. I know that the old
GSimpleAsyncResult used a source tag, but I never really understood the benefit
of it. Since the earlier patches in this series didn't use it, I'd just drop it
from this one as well, unless you have a good reason to include it.

>  
> -    res = g_simple_async_result_new(G_OBJECT(session),
> -                                    callback,
> -                                    opaque,
> -                                    spice_smartcard_manager_init);
> -    g_simple_async_result_run_in_thread(res,
> -                                        smartcard_manager_init_helper,
> -                                        G_PRIORITY_DEFAULT,
> -                                        cancellable);
> -    g_object_unref(res);
> +    g_task_run_in_thread(task, smartcard_manager_init_helper);
> +    g_object_unref(task);
>  }
>  
>  G_GNUC_INTERNAL
> @@ -522,21 +517,18 @@ gboolean
> spice_smartcard_manager_init_finish(SpiceSession *session,
>                                               GAsyncResult *result,
>                                               GError **err)
>  {
> -    GSimpleAsyncResult *simple;
> +    GTask *task = G_TASK(result);
>  
>      g_return_val_if_fail(SPICE_IS_SESSION(session), FALSE);
> -    g_return_val_if_fail(G_IS_SIMPLE_ASYNC_RESULT(result), FALSE);
> +    g_return_val_if_fail(G_IS_TASK(task), FALSE);
>  
>      SPICE_DEBUG("smartcard_manager_finish");
>  
> -    simple = G_SIMPLE_ASYNC_RESULT(result);
> -    g_return_val_if_fail(g_simple_async_result_get_source_tag(simple) ==
> spice_smartcard_manager_init, FALSE);
> -    if (g_simple_async_result_propagate_error(simple, err))
> -        return FALSE;
> +    g_return_val_if_fail(g_task_get_source_tag(task) ==
> spice_smartcard_manager_init, FALSE);
>  
>      spice_smartcard_manager_update_monitor();
>  
> -    return TRUE;
> +    return g_task_propagate_boolean(task, err);
>  }
>  
>  /**


Reviewed-by: Jonathon Jongsma <jjongsma at redhat.com>