Re: [Qemu-devel] [PULL 00/26] Leak patches

[Eric Blake]

On 09/07/2016 01:57 PM, Marc-André Lureau wrote:

>>> I'm afraid this doesn't build with our minimum glib version:
>>>
>>> /Users/pm215/src/qemu-for-merges/tests/libqtest.c:771:42: error: expected
>>> ')'
>>>                       (GTestFixtureFunc) fn, (GTestFixtureFunc)
>>>                       data_free_func);

>>> The GTestFixtureFunc typedef was only introduced in glib 2.28, and our
>>> minimum is 2.22.
>>
>> Argh,..
>>
>>>
>>> Also, g_test_add_vtable() in glib 2.22 has this prototype:
>>>

>>>                                          void          (*data_test)
>>>                                          (void),
>>>                                          void          (*data_teardown)
>>>                                          (void));
>>>
>>> but GTestFixtureFunc is typedefed in newer glib as
>>> void (*GTestFixtureFunc) (gpointer fixture, gconstpointer user_data);

So when glib prototyped it, they changed from (void) to (gpointer,
gconstpointer) as the signature.  Which means there's no real portable
way to cast, other than to create a conditional #define typedef to the
appropriate variant for the glib being targeted.

>>>
>>> so it looks like this function has changed signature somewhere
>>> between glib versions, which makes me a bit nervous about using it.

Do you use either 'fixture' or 'user_data' in the updated signature?  If
not, and the older signature of (void) matches how we are using things,
then the casting just exists to shut up the compiler (and we are relying
on the ABI, rather than C, to happen to do the right thing).  If we ARE
using either argument, then you'd have to refactor things to pass the
arguments through a global rather than as a function parameter.

>>
>> Perhaps we should get back to the simpler version, only using
>> g_test_add_data_func_full() with 2.34:
>> https://patchwork.kernel.org/patch/9251373/
>>
>> I can update the patch that way with a comment about expected leaks < 2.34.

Living with leaks on older glib is starting to sound the most palatable;
but it needs more comments than what was listed in that patchwork entry.

>>
> 
> Eric, since you suggested some compat code for older versions, would you be 
> fine with the above plan? I can send a new patch for review.

I'm fine with the #if GLIB_CHECK_VERSION(2, 34, 0), but only if there is
corresponding comments in both the code and the commit message that
documents that we have exhausted all other reasonable possibilities of
catering to older glib, and that it doesn't matter if the test leaks.

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

signature.asc
Description: OpenPGP digital signature