Hi,
On Wed, Mar 23, 2016 at 02:32:06PM +0100, Christophe Fergeau wrote:
> On Wed, Mar 23, 2016 at 01:04:03PM +0100, Fabiano Fidêncio wrote:
> > 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.
>
> This is indeed the safe way to go given the few crashes we had. However,
> it would be nice to try to get rid of as much of these idle calls as
> possible (when it's safe to do so) in the future so that the places
> which need to be special are obvious.
>
> >
>
>
> > 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 @@
> > +
> > +
> > +void __attribute__((format(gnu_printf, 4, 5)))
>
> glib provides G_GNUC_PRINTF()
> Doesn't this belong in the header file though rather than in the .c
> file?
>
> > +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");
>
> Indentation is broken here
>
> > }
> > @@ -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);
>
> and there.
>
> > @@ -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");
>
> Indentation
>
> >
> > 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");
>
> Indentation (there are more below, and maybe before too).
>
>
> Acked-by: Christophe Fergeau <cfergeau@xxxxxxxxxx>
>
> Christophe
I would rather that we wait to see if we find more issues. At the
moment, we don't have any. The coroutine context is the only one so far
that it was necessary to complete-in-idle. The other on
spice_usbredir_channel_open_acl_cb() was quite simple (finalize before
calling the callback).
Not having any issue to address and including this g-task-helper does
not make much sense for me.
cheers,
toso
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel