Re: [Qemu-devel] [PATCH for-2.5 1/2] tests: Use proper functions types instead of void (*fn)

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 12/02/2015 01:20 PM, Markus Armbruster wrote:
>> We have several function parameters declared as void (*fn).  This is
>> just a stupid way to write void *, and the only purpose writing it
>> like that could serve is obscuring the sin of bypassing the type
>> system without need.
>
> Presumably, someone meant to write 'void (*fn)()' or some other argument
> list to designate fn as a function pointer; but I agree with the
> approach you took of using typedefs rather than spelling it out raw.
>
>> 
>> The original sin is commit 49ee359: its qtest_add_func() is a wrapper
>> for g_test_add_func().  Fix the parameter type to match
>> g_test_add_func()'s.  This uncovers type errors in ide-test.c; fix
>> them.
>> 
>> Commit 7949c0e faithfully repeated the sin for qtest_add_data_func().
>> Fix it the same way, along with a harmless type error uncovered in
>> vhost-user-test.c.
>> 
>> Commit 063c23d repeated it for qtest_add_abrt_handler().  The screwy
>> parameter gets assigned to GHook member func, so change its type to
>> match.  Requires wrapping kill_qemu() to keep the type checker happy.
>> 
>> Signed-off-by: Markus Armbruster <address@hidden>
>> ---
>>  tests/ide-test.c        |  4 ++--
>>  tests/libqtest.c        | 13 +++++++++----
>>  tests/libqtest.h        |  6 +++---
>>  tests/vhost-user-test.c |  3 ++-
>>  4 files changed, 16 insertions(+), 10 deletions(-)
>> 
>
>> +++ b/tests/libqtest.c
>> @@ -110,6 +110,11 @@ static void kill_qemu(QTestState *s)
>>      }
>>  }
>>  
>> +static void kill_qemu_hook_func(void *s)
>> +{
>> +    kill_qemu(s);
>
> Getting the implicit conversion from void * to QTestState *, without
> having to cast the actual function signature.  Makes sense.

I'm not fond of wrappers, but I really dislike function pointer casts;
I've seen too many of them with undefined behavior.

>> @@ -133,7 +138,7 @@ static void cleanup_sigabrt_handler(void)
>>      sigaction(SIGABRT, &sigact_old, NULL);
>>  }
>>  
>> -void qtest_add_abrt_handler(void (*fn), const void *data)
>> +void qtest_add_abrt_handler(GHookFunc fn, const void *data)
>
> I had to look it up:
>
> /usr/include/glib-2.0/glib/ghook.h:typedef void               (*GHookFunc)    
> (gpointer      data);
>
> which is the same as
>
> void (*fn)(void *)

Speaking of function pointer cases...  note struct GHook's member func's
documentation:

    gpointer func; the function to call when this hook is invoked. The
                   possible signatures for this function are GHookFunc
                   and GHookCheckFunc

GHookFunc is void (*)(gpointer data), and to be used when the hook is
passed to g_hook_list_invoke().

GHookCheckFunc is gboolean (*)(gpointer data), and to be used when the
hook is passed to g_hook_list_invoke_check().

Undefined behavior when you get it wrong.

This interface displays no evidence of adult supervision.  Oh well,
"strong typing is for people with weak memories."

>> @@ -755,14 +760,14 @@ void qtest_memread(QTestState *s, uint64_t addr, void 
>> *data, size_t size)
>>      g_strfreev(args);
>>  }
>>  
>> -void qtest_add_func(const char *str, void (*fn))
>> +void qtest_add_func(const char *str, GTestFunc fn)
>>  {
>>      gchar *path = g_strdup_printf("/%s/%s", qtest_get_arch(), str);
>>      g_test_add_func(path, fn);
>
> /usr/include/glib-2.0/glib/gtestutils.h:typedef void (*GTestFunc)
>  (void);
>
>>      g_free(path);
>>  }
>>  
>> -void qtest_add_data_func(const char *str, const void *data, void (*fn))
>> +void qtest_add_data_func(const char *str, const void *data, GTestDataFunc 
>> fn)
>
> /usr/include/glib-2.0/glib/gtestutils.h:typedef void (*GTestDataFunc)
>  (gconstpointer user_data);
>
>> +++ b/tests/vhost-user-test.c
>> @@ -172,8 +172,9 @@ static void wait_for_fds(TestServer *s)
>>      g_mutex_unlock(&s->data_mutex);
>>  }
>>  
>> -static void read_guest_mem(TestServer *s)
>> +static void read_guest_mem(const void *data)
>>  {
>> +    TestServer *s = (void *)data;
>
> And here you have to cast away const.
>
> At any rate, all the fixes look sane, and the fact that it compiles with
> now-tighter types is in its favor.
>
> Reviewed-by: Eric Blake <address@hidden>

Thanks!