Re: [Qemu-devel] [Qemu-trivial] [PATCH] fix MinGW compilation when --enable-vnc-jpeg is specified

[Stefan Weil <weil@mail.berlios.de>]

Am 23.06.2011 15:52, schrieb Stefan Hajnoczi:
> On Sat, Jun 18, 2011 at 10:35:57AM +0200, Stefan Weil wrote:
>> Am 18.06.2011 07:13, schrieb Roy Tam:
>>> This patch fix conflicting types for 'INT32' in basetsd.h in including
>>> qemu-common.h first.
>>>
>>>
>>> Sign-off-by: Roy Tam<roytam@gmail.com>
>>> --
>>> diff --git a/ui/vnc-enc-tight.c b/ui/vnc-enc-tight.c
>>> index 87fdf35..1591df0 100644
>>> --- a/ui/vnc-enc-tight.c
>>> +++ b/ui/vnc-enc-tight.c
>>> @@ -28,6 +28,8 @@
>>>
>>> #include "config-host.h"
>>>
>>> +#include "qemu-common.h"
>>> +
>>> #ifdef CONFIG_VNC_PNG
>>> #include<png.h>
>>> #endif
>>> @@ -36,8 +38,6 @@
>>> #include<jpeglib.h>
>>> #endif
>>>
>>> -#include "qemu-common.h"
>>> -
>>> #include "bswap.h"
>>> #include "qint.h"
>>> #include "vnc.h"
>>
>> Acked-by: Stefan Weil <weil@mail.berlios.de>
>>
>> The conflicting declaration is in jmorecfg.h which is included from
>> jpeglib.h.
>
> Is the problem that the Windows headers included from qemu-common.h try
> to #define INT32?
> http://msdn.microsoft.com/en-us/library/aa383751(v=vs.85).aspx
>
> In that case I think an explicit fix is better:
>
> #ifdef _WIN32
> /* Include this before jpeglib.h for the INT32 definition */
> #include <basetsd.h>
> #endif
>
> ...followed by png/jpeg includes...
>
> Simply moving qemu-common.h provides no hints and is rather indirect.
> Someone may move it back in the future.
>
> Stefan

INT32 is declared in basetsd.h which is included from windows.h
(with some indirections) which is included from qemu-os-win32.h
which is included from qemu-common.h.

INT32 is not a #define, but a data type (typedef) and very common
for w32 compilations. Windows programmers don't include basetsd.h
directly, but usually use windows.h.

Including qemu-common.h right at the beginning (after config.h where
needed) should be good practice for QEMU source code - like this:

#include "config.h"        /* optional */
#include "qemu-common.h"
#include <system includes> /* without those that are included from 
qemu-common.h */
...
#include "other local includes"
...

As long as the maintainers don't accept patches which simply move
qemu-common.h, there is no danger. :-)

Of course a comment might be added. In most cases, I'm a great friend
of good comments, but in this special case, I don't think it is necessary.
It's a very special case of a typedef conflict caused by the mingw32
version of jpeglib (which is obviously rarely used), and it might be fixed
in a newer version of jpeglib (so the comment would be no longer
valid, and nobody would notice that).

Stefan