On Wed, Mar 23, 2016 at 2:45 PM, Victor Toso <lists@xxxxxxxxxxxxxx> wrote:
> 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.
Well, it was not the only one as 7774b8c shows ...
> The other on
> spice_usbredir_channel_open_acl_cb() was quite simple (finalize before
> calling the callback).
... a crash for the same root cause ...
>
> Not having any issue to address and including this g-task-helper does
> not make much sense for me.
>
I really would prefer to play safe than sorry, but that's just my opinion.
> cheers,
> toso
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
I'm not going to waste my time on this anymore ...
Best Regards,
--
Fabiano Fidêncio
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel