Re: [Qemu-devel] [PATCH] qga/commands-posix.c: Use correct types with g_base64_decode()

[Paolo Bonzini <pbonzini@redhat.com>]

On 14/04/2015 15:45, Peter Maydell wrote:
> On 14 April 2015 at 14:42, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>>
>> On 08/04/2015 22:02, Peter Maydell wrote:
>>> The second argument of g_base64_decode() is a 'gsize *', not a
>>> 'size_t *'. Some compilation environments (like building 32-bit PPC
>>> binaries on a PPC64 system) will complain about the mismatch:
>>>
>>>   CC    qga/commands-posix.o
>>> qga/commands-posix.c: In function 'qmp_guest_set_user_password':
>>> qga/commands-posix.c:1908:5: error: passing argument 2 of 'g_base64_decode' from incompatible pointer type [-Werror]
>>> In file included from /usr/include/glib-2.0/glib.h:37:0,
>>>                  from qga/commands-posix.c:14:
>>> /usr/include/glib-2.0/glib/gbase64.h:49:9: note: expected ‘gsize *’ but argument is of type ‘size_t *’
>>>
>>> (We previously fixed errors of this type in commit 3d1bba20.)
>>>
>>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>>
>> I think this patch is wrong.  Considering what Thomas was doing
>> ("playing around with --extra-cflags=-m32" on x86) this looks like
>> you're using the 64-bit glib headers while doing a 32-bit compilation.
> 
> I sort of agree, but I don't think the patch is wrong as such.
> The API of the function we're calling demands a pointer to a
> 'gsize', so we should do that, not rely implicitly on 'gsize'
> and 'size_t' being interchangeable.

But you can: gsize is defined to be "An unsigned integer type of the
result of the sizeof operator, corresponding to the size_t type defined
in C99. This type is wide enough to hold the numeric value of a pointer,
so it is usually 32bit wide on a 32bit platform and 64bit wide on a
64bit platform".

So it's impossible to disagree---the patch is correct in the sense that
it fixes a warning.  But it is not correct as far as its original
rationale (compiling with -m32) goes.  What you'll get is a library
compiled for 32-bit gsize, receving arguments from a program that passes
64-bit gsize.  That's an ABI mismatch, and one which is unlikely to work
on most 32-bit architectures; in this sense the patch is wrong.

If anything, I would add a QEMU_BUILD_BUG_ON(sizeof(gsize) !=
sizeof(size_t)) to catch the problem, since we've had many experienced
developers caught unprepared.  At which point we've added another safety
net and the patch becomes 101% correct---but you get a compilation error
anyway, so the original purpose of the patch is not fulfilled.

All in all, I don't think this patch is 2.3 material.

Paolo

> (I have no idea why glib sees fit to define its own versions
> of the standard types, but given that it does we should get
> the interfacing to them right...)

That's 1995-vintage portability for you. :(

Paolo