gtask-helper provide methods that can easily be used for returning in idle, as a few issues have been found in the GTask code used in spice-gtk due to a immediately return when a return in idle was expected. As examples of these issues, you can take a look on commits 7774b8c and e81d97c. Not all the functions introduced in gtask-helper.h are being used, but I still decided to add them for completeness reasons. Also, all the functions called in idle are the same that were being called in idle when using GSimpleAsyncResult. So, no issues should be found after this change and no behavior change should noticed. Signed-off-by: Fabiano Fidêncio <fidencio@xxxxxxxxxx> --- src/Makefile.am | 2 + src/channel-base.c | 4 +- src/channel-main.c | 12 ++-- src/channel-usbredir.c | 12 ++-- src/gtask-helper.c | 153 +++++++++++++++++++++++++++++++++++++++++++ src/gtask-helper.h | 34 ++++++++++ src/spice-channel.c | 4 +- src/spice-gstaudio.c | 6 +- src/usb-acl-helper.c | 12 ++-- src/usb-device-manager.c | 18 ++--- src/vmcstream.c | 35 ++-------- src/win-usb-driver-install.c | 21 +++--- 12 files changed, 246 insertions(+), 67 deletions(-) create mode 100644 src/gtask-helper.c create mode 100644 src/gtask-helper.h diff --git a/src/Makefile.am b/src/Makefile.am index 66ba58b..783db37 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -235,6 +235,8 @@ libspice_client_glib_2_0_la_SOURCES = \ coroutine.h \ gio-coroutine.c \ gio-coroutine.h \ + gtask-helper.c \ + gtask-helper.h \ \ channel-base.c \ channel-webdav.c \ diff --git a/src/channel-base.c b/src/channel-base.c index de04b89..67352e5 100644 --- a/src/channel-base.c +++ b/src/channel-base.c @@ -17,6 +17,8 @@ */ #include "config.h" +#include "gtask-helper.h" + #include "spice-client.h" #include "spice-common.h" @@ -243,7 +245,7 @@ vmc_write_free_cb(uint8_t *data, void *user_data) GTask *task = user_data; gsize count = GPOINTER_TO_SIZE(g_task_get_task_data(task)); - g_task_return_int(task, count); + g_task_helper_return_int_in_idle(task, count); g_object_unref(task); } diff --git a/src/channel-main.c b/src/channel-main.c index 1c19de1..3873c45 100644 --- a/src/channel-main.c +++ b/src/channel-main.c @@ -21,6 +21,8 @@ #include <spice/vd_agent.h> #include <glib/gstdio.h> +#include "gtask-helper.h" + #include "spice-client.h" #include "spice-common.h" #include "spice-marshal.h" @@ -923,7 +925,7 @@ static gboolean flush_foreach_remove(gpointer key G_GNUC_UNUSED, { gboolean success = GPOINTER_TO_UINT(user_data); GTask *result = value; - g_task_return_boolean(result, success); + g_task_helper_return_boolean_in_idle(result, success); return TRUE; } @@ -946,7 +948,7 @@ static void file_xfer_flush_async(SpiceMainChannel *channel, GCancellable *cance was_empty = g_queue_is_empty(c->agent_msg_queue); if (was_empty) { - g_task_return_boolean(task, TRUE); + g_task_helper_return_boolean_in_idle(task, TRUE); g_object_unref(task); return; } @@ -981,7 +983,7 @@ static void agent_send_msg_queue(SpiceMainChannel *channel) task = g_hash_table_lookup(c->flushing, out); if (task) { /* if there's a flush task waiting for this message, finish it */ - g_task_return_boolean(task, TRUE); + g_task_helper_return_boolean_in_idle(task, TRUE); g_hash_table_remove(c->flushing, out); } } @@ -1790,9 +1792,9 @@ static void file_xfer_close_cb(GObject *object, self->priv->user_data); if (self->priv->error) { - g_task_return_error(task, self->priv->error); + g_task_helper_return_error_in_idle(task, self->priv->error); } else { - g_task_return_boolean(task, TRUE); + g_task_helper_return_boolean_in_idle(task, TRUE); if (spice_util_get_debug()) { gint64 now = g_get_monotonic_time(); gchar *basename = g_file_get_basename(self->priv->file); diff --git a/src/channel-usbredir.c b/src/channel-usbredir.c index ab90800..9549763 100644 --- a/src/channel-usbredir.c +++ b/src/channel-usbredir.c @@ -32,6 +32,8 @@ #include "usbutil.h" #endif +#include "gtask-helper.h" + #include "spice-client.h" #include "spice-common.h" @@ -301,9 +303,9 @@ static void spice_usbredir_channel_open_acl_cb( g_boxed_free(spice_usb_device_get_type(), priv->spice_device); priv->spice_device = NULL; priv->state = STATE_DISCONNECTED; - g_task_return_error(priv->task, err); + g_task_helper_return_error_in_idle(priv->task, err); } else { - g_task_return_boolean(priv->task, TRUE); + g_task_helper_return_boolean_in_idle(priv->task, TRUE); } g_clear_object(&priv->acl_helper); @@ -340,14 +342,14 @@ void spice_usbredir_channel_connect_device_async( task = g_task_new(channel, cancellable, callback, user_data); if (!priv->host) { - g_task_return_new_error(task, + g_task_helper_return_new_error_in_idle(task, SPICE_CLIENT_ERROR, SPICE_CLIENT_ERROR_FAILED, "Error libusb context not set"); goto done; } if (priv->state != STATE_DISCONNECTED) { - g_task_return_new_error(task, + g_task_helper_return_new_error_in_idle(task, SPICE_CLIENT_ERROR, SPICE_CLIENT_ERROR_FAILED, "Error channel is busy"); goto done; @@ -371,7 +373,7 @@ void spice_usbredir_channel_connect_device_async( return; #else if (!spice_usbredir_channel_open_device(channel, &err)) { - g_task_return_error(task, err); + g_task_helper_return_error_in_idle(task, err); libusb_unref_device(priv->device); priv->device = NULL; g_boxed_free(spice_usb_device_get_type(), priv->spice_device); diff --git a/src/gtask-helper.c b/src/gtask-helper.c new file mode 100644 index 0000000..3c72396 --- /dev/null +++ b/src/gtask-helper.c @@ -0,0 +1,153 @@ +/* -*- Mode: C; c-basic-offset: 4; indent-tabs-mode: nil -*- */ +/* + Copyright (C) 2016 Red Hat, Inc. + + This library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.0 of the License, or (at your option) any later version. + + This library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with this library; if not, write to the Free Software + Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA +*/ +#include "config.h" + +#include "gtask-helper.h" + +typedef struct _GTaskHelperData +{ + GTask *task; + GError *error; + gint integer; + gboolean boolean; + gpointer pointer; + GDestroyNotify destroy_notify_cb; +} GTaskHelperData; + +static GTaskHelperData * +g_task_helper_new(void) +{ + return g_new0(GTaskHelperData, 1); +} + +static void +g_task_helper_data_free(GTaskHelperData *data) +{ + g_clear_object(&data->task); + g_free(data); +} + +static gboolean +complete_in_idle_boolean_cb(gpointer user_data) +{ + GTaskHelperData *data = user_data; + + g_task_return_boolean(data->task, data->boolean); + g_task_helper_data_free(data); + + return FALSE; +} + +static gboolean +complete_in_idle_int_cb(gpointer user_data) +{ + GTaskHelperData *data = user_data; + + g_task_return_int(data->task, data->integer); + g_task_helper_data_free(data); + + return FALSE; +} + +static gboolean +complete_in_idle_error_cb(gpointer user_data) +{ + GTaskHelperData *data = user_data; + + g_task_return_error(data->task, data->error); + g_task_helper_data_free(data); + + return FALSE; +} + +static gboolean +complete_in_idle_pointer_cb(gpointer user_data) +{ + GTaskHelperData *data = user_data; + + g_task_return_pointer(data->task, data->pointer, data->destroy_notify_cb); + g_task_helper_data_free(data); + + return FALSE; +} + +void +g_task_helper_return_boolean_in_idle(GTask *task, + gboolean result) +{ + GTaskHelperData *data = g_task_helper_new(); + data->task = g_object_ref(task); + data->boolean = result; + + g_idle_add(complete_in_idle_boolean_cb, data); +} + +void +g_task_helper_return_int_in_idle(GTask *task, + gssize result) +{ + GTaskHelperData *data = g_task_helper_new(); + data->task = g_object_ref(task); + data->integer = result; + + g_idle_add(complete_in_idle_int_cb, data); +} + + +void +g_task_helper_return_error_in_idle(GTask *task, + GError *error) +{ + GTaskHelperData *data = g_task_helper_new(); + data->task = g_object_ref(task); + g_propagate_error(&data->error, error); + + g_idle_add(complete_in_idle_error_cb, data); +} + + +void __attribute__((format(gnu_printf, 4, 5))) +g_task_helper_return_new_error_in_idle(GTask *task, + GQuark domain, + gint code, + const char *format, + ...) +{ + GTaskHelperData *data = g_task_helper_new(); + va_list args; + + data->task = g_object_ref(task); + va_start(args, format); + data->error = g_error_new_valist(domain, code, format, args); + va_end(args); + + g_idle_add(complete_in_idle_error_cb, data); +} + +void g_task_helper_return_pointer_in_idle(GTask *task, + gpointer result, + GDestroyNotify result_destroy) +{ + GTaskHelperData *data = g_task_helper_new(); + data->task = g_object_ref(task); + data->pointer = result; + data->destroy_notify_cb = result_destroy; + + g_idle_add(complete_in_idle_pointer_cb, data); +} diff --git a/src/gtask-helper.h b/src/gtask-helper.h new file mode 100644 index 0000000..81c041f --- /dev/null +++ b/src/gtask-helper.h @@ -0,0 +1,34 @@ +/* -*- Mode: C; c-basic-offset: 4; indent-tabs-mode: nil -*- */ +/* + Copyright (C) 2016 Red Hat, Inc. + + This library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.0 of the License, or (at your option) any later version. + + This library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with this library; if not, write to the Free Software + Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA +*/ +#ifndef __G_TASK_HELPER_H__ +#define __G_TASK_HELPER_H__ + +#include <gio/gio.h> + +G_BEGIN_DECLS + +void g_task_helper_return_boolean_in_idle(GTask *task, gboolean result); +void g_task_helper_return_int_in_idle(GTask *task, gssize result); +void g_task_helper_return_error_in_idle(GTask *task, GError *error); +void g_task_helper_return_new_error_in_idle(GTask *task, GQuark domain, gint code, const char *format, ...); +void g_task_helper_return_pointer_in_idle(GTask *task, gpointer result, GDestroyNotify result_destroy); + +G_END_DECLS + +#endif /* __G_TASK_HELPER_H__ */ diff --git a/src/spice-channel.c b/src/spice-channel.c index 8ae0e4d..fdbc45b 100644 --- a/src/spice-channel.c +++ b/src/spice-channel.c @@ -17,6 +17,8 @@ */ #include "config.h" +#include "gtask-helper.h" + #include "spice-client.h" #include "spice-common.h" @@ -2191,7 +2193,7 @@ static void spice_channel_flushed(SpiceChannel *channel, gboolean success) GSList *l; for (l = c->flushing; l != NULL; l = l->next) { - g_task_return_boolean(G_TASK(l->data), success); + g_task_helper_return_boolean_in_idle(G_TASK(l->data), success); } g_slist_free_full(c->flushing, g_object_unref); diff --git a/src/spice-gstaudio.c b/src/spice-gstaudio.c index a7c3c24..d810911 100644 --- a/src/spice-gstaudio.c +++ b/src/spice-gstaudio.c @@ -22,6 +22,8 @@ #include <gst/app/gstappsink.h> #include <gst/audio/streamvolume.h> +#include "gtask-helper.h" + #include "spice-gstaudio.h" #include "spice-common.h" #include "spice-session.h" @@ -572,7 +574,7 @@ static void spice_gstaudio_get_playback_volume_info_async(SpiceAudio *audio, { GTask *task = g_task_new(audio, cancellable, callback, user_data); - g_task_return_boolean(task, TRUE); + g_task_helper_return_boolean_in_idle(task, TRUE); } static gboolean spice_gstaudio_get_playback_volume_info_finish(SpiceAudio *audio, @@ -654,7 +656,7 @@ static void spice_gstaudio_get_record_volume_info_async(SpiceAudio *audio, { GTask *task = g_task_new(audio, cancellable, callback, user_data); - g_task_return_boolean(task, TRUE); + g_task_helper_return_boolean_in_idle(task, TRUE); } static gboolean spice_gstaudio_get_record_volume_info_finish(SpiceAudio *audio, diff --git a/src/usb-acl-helper.c b/src/usb-acl-helper.c index 487e1ee..5d03879 100644 --- a/src/usb-acl-helper.c +++ b/src/usb-acl-helper.c @@ -25,6 +25,8 @@ #include <stdio.h> #include <string.h> +#include "gtask-helper.h" + #include "usb-acl-helper.h" /* ------------------------------------------------------------------ */ @@ -91,7 +93,7 @@ static void spice_usb_acl_helper_class_init(SpiceUsbAclHelperClass *klass) static void async_result_set_cancelled(GTask *task) { - g_task_return_new_error(task, + g_task_helper_return_new_error_in_idle(task, G_IO_ERROR, G_IO_ERROR_CANCELLED, "Setting USB device node ACL cancelled"); } @@ -120,11 +122,11 @@ static gboolean cb_out_watch(GIOChannel *channel, string[strlen(string) - 1] = 0; if (!strcmp(string, "SUCCESS")) { success = TRUE; - g_task_return_boolean(priv->task, TRUE); + g_task_helper_return_boolean_in_idle(priv->task, TRUE); } else if (!strcmp(string, "CANCELED")) { async_result_set_cancelled(priv->task); } else { - g_task_return_new_error(priv->task, + g_task_helper_return_new_error_in_idle(priv->task, SPICE_CLIENT_ERROR, SPICE_CLIENT_ERROR_FAILED, "Error setting USB device node ACL: '%s'", string); @@ -132,10 +134,10 @@ static gboolean cb_out_watch(GIOChannel *channel, g_free(string); break; case G_IO_STATUS_ERROR: - g_task_return_error(priv->task, err); + g_task_helper_return_error_in_idle(priv->task, err); break; case G_IO_STATUS_EOF: - g_task_return_new_error(priv->task, + g_task_helper_return_new_error_in_idle(priv->task, SPICE_CLIENT_ERROR, SPICE_CLIENT_ERROR_FAILED, "Unexpected EOF reading from acl helper stdout"); break; diff --git a/src/usb-device-manager.c b/src/usb-device-manager.c index 85231a1..eaaf6a4 100644 --- a/src/usb-device-manager.c +++ b/src/usb-device-manager.c @@ -42,6 +42,8 @@ #include "usbutil.h" #endif +#include "gtask-helper.h" + #include "spice-session-priv.h" #include "spice-client.h" #include "spice-marshal.h" @@ -1442,7 +1444,7 @@ _spice_usb_device_manager_connect_device_async(SpiceUsbDeviceManager *self, guint i; if (spice_usb_device_manager_is_device_connected(self, device)) { - g_task_return_new_error(task, + g_task_helper_return_new_error_in_idle(task, SPICE_CLIENT_ERROR, SPICE_CLIENT_ERROR_FAILED, "Cannot connect an already connected usb device"); goto done; @@ -1466,10 +1468,10 @@ _spice_usb_device_manager_connect_device_async(SpiceUsbDeviceManager *self, g_ptr_array_remove(priv->devices, device); g_signal_emit(self, signals[DEVICE_REMOVED], 0, device); spice_usb_device_unref(device); - g_task_return_new_error(task, - SPICE_CLIENT_ERROR, - SPICE_CLIENT_ERROR_FAILED, - _("Device was not found")); + g_task_helper_return_new_error_in_idle(task, + SPICE_CLIENT_ERROR, + SPICE_CLIENT_ERROR_FAILED, + _("Device was not found")); goto done; } #endif @@ -1484,9 +1486,9 @@ _spice_usb_device_manager_connect_device_async(SpiceUsbDeviceManager *self, } #endif - g_task_return_new_error(task, - SPICE_CLIENT_ERROR, SPICE_CLIENT_ERROR_FAILED, - _("No free USB channel")); + g_task_helper_return_new_error_in_idle(task, + SPICE_CLIENT_ERROR, SPICE_CLIENT_ERROR_FAILED, + _("No free USB channel")); #ifdef USE_USBREDIR done: #endif diff --git a/src/vmcstream.c b/src/vmcstream.c index 5dd2799..2df31e5 100644 --- a/src/vmcstream.c +++ b/src/vmcstream.c @@ -19,6 +19,7 @@ #include <string.h> +#include "gtask-helper.h" #include "vmcstream.h" #include "spice-channel-priv.h" #include "gio-coroutine.h" @@ -97,24 +98,6 @@ spice_vmc_input_stream_new(void) return self; } -typedef struct _complete_in_idle_cb_data { - GTask *task; - gssize pos; -} complete_in_idle_cb_data; - -static gboolean -complete_in_idle_cb(gpointer user_data) -{ - complete_in_idle_cb_data *data = user_data; - - g_task_return_int(data->task, data->pos); - - g_object_unref (data->task); - g_free (data); - - return FALSE; -} - /* coroutine */ /** * Feed a SpiceVmc stream with new data from a coroutine @@ -134,8 +117,6 @@ spice_vmc_input_stream_co_data(SpiceVmcInputStream *self, self->coroutine = coroutine_self(); while (size > 0) { - complete_in_idle_cb_data *cb_data; - SPICE_DEBUG("spicevmc co_data %p", self->task); if (!self->task) coroutine_yield(NULL); @@ -157,13 +138,7 @@ spice_vmc_input_stream_co_data(SpiceVmcInputStream *self, if (self->all && min > 0 && self->pos != self->count) continue; - /* Let's deal with the task complete in idle by ourselves, as GTask - * heuristic only makes sense in a non-coroutine case. - */ - cb_data = g_new(complete_in_idle_cb_data , 1); - cb_data->task = g_object_ref(self->task); - cb_data->pos = self->pos; - g_idle_add(complete_in_idle_cb, cb_data); + g_task_helper_return_int_in_idle(self->task, self->pos); g_cancellable_disconnect(g_task_get_cancellable(self->task), self->cancel_id); g_clear_object(&self->task); @@ -179,9 +154,9 @@ read_cancelled(GCancellable *cancellable, SpiceVmcInputStream *self = SPICE_VMC_INPUT_STREAM(user_data); SPICE_DEBUG("read cancelled, %p", self->task); - g_task_return_new_error(self->task, - G_IO_ERROR, G_IO_ERROR_CANCELLED, - "read cancelled"); + g_task_helper_return_new_error_in_idle(self->task, + G_IO_ERROR, G_IO_ERROR_CANCELLED, + "read cancelled"); g_clear_object(&self->task); diff --git a/src/win-usb-driver-install.c b/src/win-usb-driver-install.c index 0d4627a..14de369 100644 --- a/src/win-usb-driver-install.c +++ b/src/win-usb-driver-install.c @@ -31,6 +31,7 @@ #include <gio/gio.h> #include <gio/gwin32inputstream.h> #include <gio/gwin32outputstream.h> +#include "gtask-helper.h" #include "spice-util.h" #include "win-usb-clerk.h" #include "win-usb-driver-install.h" @@ -143,13 +144,13 @@ void win_usb_driver_handle_reply_cb(GObject *gobject, if (err) { g_warning("failed to read reply from usbclerk (%s)", err->message); - g_task_return_error(priv->task, err); + g_task_helper_return_error_in_idle(priv->task, err); goto failed_reply; } if (bytes == 0) { g_warning("unexpected EOF from usbclerk"); - g_task_return_new_error(priv->task, + g_task_helper_return_new_error_in_idle(priv->task, SPICE_WIN_USB_DRIVER_ERROR, SPICE_WIN_USB_DRIVER_ERROR_FAILED, "unexpected EOF from usbclerk"); @@ -166,7 +167,7 @@ void win_usb_driver_handle_reply_cb(GObject *gobject, if (priv->reply.hdr.magic != USB_CLERK_MAGIC) { g_warning("usbclerk magic mismatch: mine=0x%04x server=0x%04x", USB_CLERK_MAGIC, priv->reply.hdr.magic); - g_task_return_new_error(priv->task, + g_task_helper_return_new_error_in_idle(priv->task, SPICE_WIN_USB_DRIVER_ERROR, SPICE_WIN_USB_DRIVER_ERROR_MESSAGE, "usbclerk magic mismatch"); @@ -176,7 +177,7 @@ void win_usb_driver_handle_reply_cb(GObject *gobject, if (priv->reply.hdr.version != USB_CLERK_VERSION) { g_warning("usbclerk version mismatch: mine=0x%04x server=0x%04x", USB_CLERK_VERSION, priv->reply.hdr.version); - g_task_return_new_error(priv->task, + g_task_helper_return_new_error_in_idle(priv->task, SPICE_WIN_USB_DRIVER_ERROR, SPICE_WIN_USB_DRIVER_ERROR_MESSAGE, "usbclerk version mismatch"); @@ -185,7 +186,7 @@ void win_usb_driver_handle_reply_cb(GObject *gobject, if (priv->reply.hdr.type != USB_CLERK_REPLY) { g_warning("usbclerk message with unexpected type %d", priv->reply.hdr.type); - g_task_return_new_error(priv->task, + g_task_helper_return_new_error_in_idle(priv->task, SPICE_WIN_USB_DRIVER_ERROR, SPICE_WIN_USB_DRIVER_ERROR_MESSAGE, "usbclerk message with unexpected type"); @@ -195,7 +196,7 @@ void win_usb_driver_handle_reply_cb(GObject *gobject, if (priv->reply.hdr.size != bytes) { g_warning("usbclerk message size mismatch: read %"G_GSSIZE_FORMAT" bytes hdr.size=%d", bytes, priv->reply.hdr.size); - g_task_return_new_error(priv->task, + g_task_helper_return_new_error_in_idle(priv->task, SPICE_WIN_USB_DRIVER_ERROR, SPICE_WIN_USB_DRIVER_ERROR_MESSAGE, "usbclerk message with unexpected size"); @@ -203,14 +204,14 @@ void win_usb_driver_handle_reply_cb(GObject *gobject, } if (priv->reply.status == 0) { - g_task_return_new_error(priv->task, + g_task_helper_return_new_error_in_idle(priv->task, SPICE_WIN_USB_DRIVER_ERROR, SPICE_WIN_USB_DRIVER_ERROR_MESSAGE, "usbclerk error reply"); goto failed_reply; } - g_task_return_boolean (priv->task, TRUE); + g_task_helper_return_boolean_in_idle(priv->task, TRUE); failed_reply: g_clear_object(&priv->task); @@ -312,7 +313,7 @@ void spice_win_usb_driver_op(SpiceWinUsbDriver *self, if (priv->task) { /* allow one install/uninstall request at a time */ g_warning("Another request exists -- try later"); - g_task_return_new_error(task, + g_task_helper_return_new_error_in_idle(task, SPICE_WIN_USB_DRIVER_ERROR, SPICE_WIN_USB_DRIVER_ERROR_FAILED, "Another request exists -- try later"); goto failed_request; @@ -325,7 +326,7 @@ void spice_win_usb_driver_op(SpiceWinUsbDriver *self, if (!spice_win_usb_driver_send_request(self, op_type, vid, pid, &err)) { g_warning("failed to send a request to usbclerk %s", err->message); - g_task_return_error(task, err); + g_task_helper_return_error_in_idle(task, err); goto failed_request; } -- 2.5.5 _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel