[libvirt] [libvirt-glib 3/3] gobject: Port to GTask API

[Zeeshan Ali (Khattak)]

Hi,

>>  /**
>> @@ -583,21 +588,22 @@ void gvir_domain_wakeup_async(GVirDomain *dom,
>>                                GAsyncReadyCallback callback,
>>                                gpointer user_data)
>>  {
>> -    GSimpleAsyncResult *res;
>> +    GTask *task;
>> +    DomainWakeupData *data;
>>
>>      g_return_if_fail(GVIR_IS_DOMAIN(dom));
>>      g_return_if_fail((cancellable == NULL) || G_IS_CANCELLABLE(cancellable));
>>
>> -    res = g_simple_async_result_new(G_OBJECT(dom),
>> -                                    callback,
>> -                                    user_data,
>> -                                    gvir_domain_wakeup_async);
>> -    g_simple_async_result_set_op_res_gssize (res, (gssize)flags);
>> -    g_simple_async_result_run_in_thread(res,
>> -                                        gvir_domain_wakeup_helper,
>> -                                        G_PRIORITY_DEFAULT,
>> -                                        cancellable);
>> -    g_object_unref(res);
>> +    data = g_slice_new0(DomainWakeupData);
>> +    data->flags = flags;
>> +
>> +    task = g_task_new(G_OBJECT(dom),
>> +                      cancellable,
>> +                      callback,
>> +                      user_data);
>> +    g_task_set_task_data(task, data, (GDestroyNotify)domain_wakeup_data_free);
>
> Maybe you can use GUINT_TO_POINTER rather than allocating memory?

I recall hearing that it's not quite that portable but now that I read
the docs, it seems its only storing of pointers in integers that is
not portable:

"... macros GPOINTER_TO_INT(), GINT_TO_POINTER(), etc. take care to do
the right thing on the every platform.

Warning: You may not store pointers in integers. This is not portable
in any way, shape or form. These macros only allow storing integers in
pointers, and only preserve 32 bits of the integer; values outside the
range of a 32-bit integer will be mangled."

So storing the flags (int) as pointer should be fine.

>>
>>      virStreamFree(handle);
>>
>> diff --git a/libvirt-gobject/libvirt-gobject-output-stream.c b/libvirt-gobject/libvirt-gobject-output-stream.c
>> index f39328b..5a84a23 100644
>> --- a/libvirt-gobject/libvirt-gobject-output-stream.c
>> +++ b/libvirt-gobject/libvirt-gobject-output-stream.c
>> @@ -45,8 +45,7 @@ struct _GVirOutputStreamPrivate
>>      GVirStream *stream;
>>
>>      /* pending operation metadata */
>> -    GSimpleAsyncResult *result;
>> -    GCancellable *cancellable;
>> +    GTask *task;
>>      const void * buffer;
>>      gsize count;
>>  };
>> @@ -103,48 +102,45 @@ gvir_output_stream_write_ready(GVirStream *stream,
>>  {
>>      GVirOutputStream *output_stream = GVIR_OUTPUT_STREAM(opaque);
>>      GVirOutputStreamPrivate *priv = output_stream->priv;
>> -    GSimpleAsyncResult *simple = priv->result;
>> +    GTask *task = priv->task;
>> +    GCancellable *cancellable = g_task_get_cancellable(task);
>>      GError *error = NULL;
>>      gssize result;
>>
>> +    priv->task = NULL;
>> +
>
> Any reason not to keep this in cleanup: ?

I actually don't recall anymore. :( I think it was cause I first tried
to get rid of cleanup tag. I'll remove this change and see if it break
anything.

>> diff --git a/libvirt-gobject/libvirt-gobject-stream.c b/libvirt-gobject/libvirt-gobject-stream.c
>> index 46dbd9a..f8a1a8c 100644
>> --- a/libvirt-gobject/libvirt-gobject-stream.c
>> +++ b/libvirt-gobject/libvirt-gobject-stream.c
>> @@ -129,14 +129,13 @@ static gboolean gvir_stream_close(GIOStream *io_stream,
>>      return (i_ret && o_ret);
>>  }
>>
>> -
>> -static void gvir_stream_close_async(GIOStream *stream,
>> -                                    int io_priority G_GNUC_UNUSED,
>> -                                    GCancellable *cancellable,
>> -                                    GAsyncReadyCallback callback,
>> -                                    gpointer user_data)
>> +static void
>> +gvir_stream_close_helper(GTask *task,
>> +                         gpointer source_object,
>> +                         gpointer task_data G_GNUC_UNUSED,
>> +                         GCancellable *cancellable)
>>  {
>> -    GSimpleAsyncResult *res;
>> +    GIOStream *stream = G_IO_STREAM(source_object);
>>      GIOStreamClass *class;
>>      GError *error;
>>
>> @@ -146,27 +145,36 @@ static void gvir_stream_close_async(GIOStream *stream,
>>      error = NULL;
>>      if (class->close_fn &&
>>          !class->close_fn(stream, cancellable, &error)) {
>> -        g_simple_async_report_take_gerror_in_idle(G_OBJECT (stream),
>> -                                                  callback, user_data,
>> -                                                  error);
>> +        g_task_return_error(task, error);
>>          return;
>>      }
>>
>> -    res = g_simple_async_result_new(G_OBJECT (stream),
>> -                                    callback,
>> -                                    user_data,
>> -                                    gvir_stream_close_async);
>> -    g_simple_async_result_complete_in_idle(res);
>> -    g_object_unref (res);
>> +    g_task_return_boolean(task, TRUE);
>> +}
>> +
>> +static void gvir_stream_close_async(GIOStream *stream,
>> +                                    int io_priority G_GNUC_UNUSED,
>> +                                    GCancellable *cancellable,
>> +                                    GAsyncReadyCallback callback,
>> +                                    gpointer user_data)
>> +{
>> +    GTask *task;
>> +
>> +    task = g_task_new(G_OBJECT(stream),
>> +                      cancellable,
>> +                      callback,
>> +                      user_data);
>> +    g_task_run_in_thread(task, gvir_stream_close_helper);
>> +    g_object_unref(task);
>>  }
>
> Was this one creating a GSimpleAsyncResult and returning immediately
> without doing anything?

I doubt so. AFAICT, it was scheduling the result to be returned in the
idle using 'g_simple_async_result_complete_in_idle'.

> The helper does not seem to be doing anything more, in which case I'd
> suggest not creating an intermediate helper thread, and to do something
> similar to what was done before. If this is a bug and more work should
> be done, I'd prefer if this was done in a separate patch.

I just failed to find a straight GTask replacement API for
'g_simple_async_result_complete_in_idle'. The docs for
'g_task_return_pointer' say:

""Completes the task" means that for an ordinary asynchronous task it
will either invoke the task's callback, or else queue that callback to
be invoked in the proper GMainContext, or in the next iteration of the
current GMainContext."

So I'm not sure if simply calling g_task_return_*() wouldn't change
the behaviour and call the callback immediately.

-- 
Regards,

Zeeshan Ali (Khattak)
________________________________________
Befriend GNOME: http://www.gnome.org/friends/