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

["Marc-André Lureau" <marcandre.lureau@gmail.com>]

Hi

On Fri, Jul 29, 2016 at 2:16 AM, Eric Blake <eblake@redhat.com> wrote:
> On 07/28/2016 08:38 AM, marcandre.lureau@redhat.com wrote:
>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>
>> 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 <marcandre.lureau@redhat.com>
>> ---
>>  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.

ok

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

Yes, calling the undocumented g_test_add_vtable(), with some casts. Is
that acceptable?

-- 
Marc-André Lureau