[Spice-devel] [spice-gtk 2/2] Introduce gtask-helper.[ch]

[Fabiano Fidêncio]

On Wed, Mar 23, 2016 at 11:34 AM, Christophe Fergeau
<cfergeau at redhat.com> 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 at redhat.com>
>> ---
>>  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