Re: [Qemu-devel] [PATCH v2 33/37] tests: add qtest_add_data_func_full

[Eric Blake]

On 07/28/2016 08:38 AM, address@hidden wrote:
> From: Marc-André Lureau <address@hidden>
> 
> Allows to specify a destroy function for the test data.

"Allows to" is not idiomatic English. Alternatives that sound better are
"Allows $who to specify" (most simply, "Allows one to"), or "Allows
specifying"

> 
> Signed-off-by: Marc-André Lureau <address@hidden>
> ---
>  tests/libqtest.c | 15 ++++++++++++++-
>  tests/libqtest.h |  7 ++++++-
>  2 files changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/libqtest.c b/tests/libqtest.c
> index eb00f13..6ec56a6 100644
> --- a/tests/libqtest.c
> +++ b/tests/libqtest.c
> @@ -758,8 +758,21 @@ void qtest_add_func(const char *str, void (*fn)(void))
>      g_free(path);
>  }
>  
> +void qtest_add_data_func_full(const char *str, void *data,
> +                              void (*fn)(const void *),
> +                              GDestroyNotify data_free_func)
> +{
> +#if GLIB_CHECK_VERSION(2, 34, 0)
> +    gchar *path = g_strdup_printf("/%s/%s", qtest_get_arch(), str);
> +    g_test_add_data_func_full(path, data, fn, data_free_func);
> +    g_free(path);
> +#else
> +    qtest_add_data_func(str, data, fn);
> +#endif

The commit message doesn't mention that the code is dependent on glib
versions, nor that you are still leaking the data (data_free_func
remains uncalled) on older glib.  If it is intentional (under the
argument that "anyone running on older glib can't care too much about
memory leaks encountered only by the testsuite, and the leaks don't
affect main qemu"), then stating that in the commit message would let me
feel more comfortable giving an R-b.

Is there anything we can do even in older glib to unconditionally invoke
the cleanup function in the right places?

> +
>  void qtest_add_data_func(const char *str, const void *data,
> -                         void (*fn)(const void *))
> +                              void (*fn)(const void *))

Why the spurious indentation change?


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

signature.asc
Description: OpenPGP digital signature