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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3444 bytes --]

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 <armbru@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 <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]