Eric Blake <ebl...@redhat.com> 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 <arm...@redhat.com>
>> ---
>> 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 <ebl...@redhat.com>
Thanks!
