On Wed, Mar 23, 2016 at 11:34 AM, Christophe Fergeau
<cfergeau@xxxxxxxxxx> wrote:
> On Wed, Mar 23, 2016 at 10:48:31AM +0100, Fabiano Fidêncio wrote:
>> As noticed, GTask's heurestic for return a task in idle or immediately
>> doesn't work when using Coroutine
>
> s/heurestic/heuristic/
>
> Imo we need a detailed description of what is not working well with
> GTask + coroutine, and one concrete example when the right thing is not
> done (either in this log, or as a comment in gtask-helper.[ch]). This
> will be really helpful if we ever need to revisit this.
Will be added in the v2
>
>
>> and that's okay, we just need to do
>> the idle ourself. And in order to avoid code duplication, let's
>> introduce and make usage of the new g_task_helper_return_* functions.
>>
>> These functions match exactly with the existing g_task_return_*
>> functions and the only difference is that they return in idle instead of
>> returning immediately.
>>
>> Signed-off-by: Fabiano Fidêncio <fidencio@xxxxxxxxxx>
>> ---
>> m4/manywarnings.m4 | 1 -
>> src/Makefile.am | 2 +
>> src/channel-base.c | 4 +-
>> src/channel-main.c | 12 ++--
>> src/channel-usbredir.c | 12 ++--
>> src/gtask-helper.c | 152 +++++++++++++++++++++++++++++++++++++++++++
>> src/gtask-helper.h | 43 ++++++++++++
>> 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 +++---
>> 13 files changed, 254 insertions(+), 68 deletions(-)
>> create mode 100644 src/gtask-helper.c
>> create mode 100644 src/gtask-helper.h
>
> Do we need to use the helper everywhere, or only in specific cases?
Well, I've decided to use it in all places where a return in idle was
being used before.
We have seem some issues with coroutine context, some issues without
coroutine context ...
>
>>
>> diff --git a/m4/manywarnings.m4 b/m4/manywarnings.m4
>> index 3e6dd21..ead38a2 100644
>> --- a/m4/manywarnings.m4
>> +++ b/m4/manywarnings.m4
>> @@ -182,7 +182,6 @@ AC_DEFUN([gl_MANYWARN_ALL_GCC],
>> -Wstrict-overflow \
>> -Wstrict-prototypes \
>> -Wsuggest-attribute=const \
>> - -Wsuggest-attribute=format \
>
> Unrelated ?
Actually, related, but shouldn't be here. I removed the warning
because I was getting and ended up not fixing it:
gtask-helper.c: In function ‘g_task_helper_return_new_error’:
gtask-helper.c:136:5: error: function might be possible candidate for
‘gnu_printf’ format attribute [-Werror=suggest-attribute=format]
data->error = g_error_new_valist(domain, code, format, args);
^
>
>> -Wsuggest-attribute=noreturn \
>> -Wsuggest-attribute=pure \
>> -Wswitch \
>
>
>> diff --git a/src/gtask-helper.h b/src/gtask-helper.h
>> new file mode 100644
>> index 0000000..9020506
>> --- /dev/null
>> +++ b/src/gtask-helper.h
>> @@ -0,0 +1,43 @@
>> +/* -*- 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(GTask *task,
>> + gboolean boolean);
>
> In my opinion, there should be a 'idle' somewhere in the name of all
> these helpers. Someone reading unrelated code would have to check what
> these helpers are about with the current naming, with an 'idle' in the
> name, it's much easier to guess what they are about and decide not to
> care about the details.
Sure, I can change it.
>
>
>> +void g_task_helper_return_int(GTask *task,
>> + gint integer);
>
> g_task_return_int takes a gssize, not a gint.
Right.
>
> Christophe
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel