Re: [Qemu-devel] [PATCH v2 4/5] w32: Replace Windows specific data types in common header files

[Stefan Hajnoczi]

On Mon, Mar 10, 2014 at 7:34 PM, Stefan Weil <address@hidden> wrote:
> Am 10.03.2014 16:17, schrieb Stefan Hajnoczi:
>> On Fri, Mar 07, 2014 at 11:17:46PM +0100, Stefan Weil wrote:
>>> diff --git a/include/qemu/thread-win32.h b/include/qemu/thread-win32.h
>>> index 7ade61a..b8b8e61 100644
>>> --- a/include/qemu/thread-win32.h
>>> +++ b/include/qemu/thread-win32.h
>>> @@ -1,24 +1,35 @@
>>>  #ifndef __QEMU_THREAD_WIN32_H
>>>  #define __QEMU_THREAD_WIN32_H 1
>>> -#include "qemu/winapi.h"
>>> +
>>> +/* WinCriticalSection is a substitute for CRITICAL_SECTION and
>>> + * introduced here to avoid dependencies on windows.h. */
>>> +
>>> +typedef struct {
>>> +    WinHandle DebugInfo;
>>> +    WinLong LockCount;
>>> +    WinLong RecursionCount;
>>> +    WinHandle OwningThread;
>>> +    WinHandle LockSemaphore;
>>> +    WinULong *SpinCount;
>>> +} WinCriticalSection;
>>
>> This is taking it a bit far.  Avoiding includes for the scalar types
>> seems okay but duplicating struct definitions makes me wonder how far
>> we'll go to reduce compile times.  (Plus wouldn't we have the same kind
>> of copyright/license issues that mingw and other projects need to be
>> very careful about when reimplementing Windows headers?)
>
>
> I don't think that we have a copyright or license issue here because we
> don't use names which were invented by MS. They have a copyright on
> win32 (that's why I typically use w32 or wxx), but not on WinXXX AFAIK.
> Our existing files with names using win32 might be a problem, although I
> doubt that anybody will complain.
>
> Instead of defining a struct, WinCriticalSection could also be an array
> which is sufficiently large to store a critical section and which uses
> the correct alignment. Do you think that would be a better solution?
>
>> I guess the problem is that qemu-thread.h is included in a lot of places
>> and you wish to avoid including <windows.h>.  Still, I would leave this
>> one out.
>>
>> Stefan
>>
>
> As you noticed, the problem is include/qemu/thread.h which includes
> include/qemu/thread-win32.h to define QemuMutex. Unfortunately,
> QemuMutex is used by value in include/block/aio.h and in
> include/exec/cpu-all.h. Most QEMU files depend on these two files.
> Breaking the dependencies of all those files on windows.h is essential
> for my patch series. I see only three solutions:
>
> * Leave the code as it is. That implies long compile time for MinGW
> builds and name space pollution because nearly every compilation needs
> windows.h. This last point is the reason for two existing hacks and one
> more hack which is still needed (both Peter and I sent patches for it),
> and we have a realistic chance to need future hacks from time to time.
>
> * Break the dependency on windows.h by using QEMU data types instead of
> Windows API data types.
>
> * Break the dependency on windows.h by avoiding the use of certain QEMU
> data types (especially QemuMutex) by value, because those QEMU data
> types use Windows data types.
>
> I must admit that I tried that third solution and gave up after a while.
>
> What do you suggest to do? For me, any of the three alternatives is
> fine. I have no personal use for QEMU on Windows, nor do I need it for
> my professional work any longer.

I don't see a hard reason against merging this series.  There are
trade-offs but you have thought through the alternatives.

Reviewed-by: Stefan Hajnoczi <address@hidden>

On Mon, Mar 10, 2014 at 7:34 PM, Stefan Weil <address@hidden> wrote:
> Am 10.03.2014 16:17, schrieb Stefan Hajnoczi:
>> On Fri, Mar 07, 2014 at 11:17:46PM +0100, Stefan Weil wrote:
>>> diff --git a/include/qemu/thread-win32.h b/include/qemu/thread-win32.h
>>> index 7ade61a..b8b8e61 100644
>>> --- a/include/qemu/thread-win32.h
>>> +++ b/include/qemu/thread-win32.h
>>> @@ -1,24 +1,35 @@
>>>  #ifndef __QEMU_THREAD_WIN32_H
>>>  #define __QEMU_THREAD_WIN32_H 1
>>> -#include "qemu/winapi.h"
>>> +
>>> +/* WinCriticalSection is a substitute for CRITICAL_SECTION and
>>> + * introduced here to avoid dependencies on windows.h. */
>>> +
>>> +typedef struct {
>>> +    WinHandle DebugInfo;
>>> +    WinLong LockCount;
>>> +    WinLong RecursionCount;
>>> +    WinHandle OwningThread;
>>> +    WinHandle LockSemaphore;
>>> +    WinULong *SpinCount;
>>> +} WinCriticalSection;
>>
>> This is taking it a bit far.  Avoiding includes for the scalar types
>> seems okay but duplicating struct definitions makes me wonder how far
>> we'll go to reduce compile times.  (Plus wouldn't we have the same kind
>> of copyright/license issues that mingw and other projects need to be
>> very careful about when reimplementing Windows headers?)
>
>
> I don't think that we have a copyright or license issue here because we
> don't use names which were invented by MS. They have a copyright on
> win32 (that's why I typically use w32 or wxx), but not on WinXXX AFAIK.
> Our existing files with names using win32 might be a problem, although I
> doubt that anybody will complain.
>
> Instead of defining a struct, WinCriticalSection could also be an array
> which is sufficiently large to store a critical section and which uses
> the correct alignment. Do you think that would be a better solution?
>
>> I guess the problem is that qemu-thread.h is included in a lot of places
>> and you wish to avoid including <windows.h>.  Still, I would leave this
>> one out.
>>
>> Stefan
>>
>
> As you noticed, the problem is include/qemu/thread.h which includes
> include/qemu/thread-win32.h to define QemuMutex. Unfortunately,
> QemuMutex is used by value in include/block/aio.h and in
> include/exec/cpu-all.h. Most QEMU files depend on these two files.
> Breaking the dependencies of all those files on windows.h is essential
> for my patch series. I see only three solutions:
>
> * Leave the code as it is. That implies long compile time for MinGW
> builds and name space pollution because nearly every compilation needs
> windows.h. This last point is the reason for two existing hacks and one
> more hack which is still needed (both Peter and I sent patches for it),
> and we have a realistic chance to need future hacks from time to time.
>
> * Break the dependency on windows.h by using QEMU data types instead of
> Windows API data types.
>
> * Break the dependency on windows.h by avoiding the use of certain QEMU
> data types (especially QemuMutex) by value, because those QEMU data
> types use Windows data types.
>
> I must admit that I tried that third solution and gave up after a while.
>
> What do you suggest to do? For me, any of the three alternatives is
> fine. I have no personal use for QEMU on Windows, nor do I need it for
> my professional work any longer.
>
> Stefan
>