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.
> @@ -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 *)
> @@ -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>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
