| Message ID | 1394230667-17037-5-git-send-email-sw@weilnetz.de |
|---|---|
| State | Superseded |
| Headers | show |
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 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
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
On Mon, Mar 10, 2014 at 7:34 PM, Stefan Weil <sw@weilnetz.de> 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 <stefanha@redhat.com> On Mon, Mar 10, 2014 at 7:34 PM, Stefan Weil <sw@weilnetz.de> 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 >
Il 10/03/2014 19:34, Stefan Weil ha scritto: > 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. Any solution is fine. I don't like particularly this series, but I don't dislike it enough to override your judgement as Win32 maintainer. I agree with Markus about not liking the comments on the #include lines, though. Paolo
diff --git a/cpus.c b/cpus.c index 945d85b..a96eb16 100644 --- a/cpus.c +++ b/cpus.c @@ -39,7 +39,9 @@ #include "qemu/bitmap.h" #include "qemu/seqlock.h" -#ifndef _WIN32 +#ifdef _WIN32 +#include "qemu/winapi.h" /* SuspendThread, ... */ +#else #include "qemu/compatfd.h" #endif diff --git a/hw/intc/apic.c b/hw/intc/apic.c index 361ae90..6bb2d78 100644 --- a/hw/intc/apic.c +++ b/hw/intc/apic.c @@ -16,12 +16,13 @@ * You should have received a copy of the GNU Lesser General Public * License along with this library; if not, see <http://www.gnu.org/licenses/> */ -#include "qemu/thread.h" + #include "hw/i386/apic_internal.h" #include "hw/i386/apic.h" #include "hw/i386/ioapic.h" #include "hw/pci/msi.h" #include "qemu/host-utils.h" +#include "qemu/thread.h" #include "trace.h" #include "hw/i386/pc.h" #include "hw/i386/apic-msidef.h" diff --git a/include/qemu/event_notifier.h b/include/qemu/event_notifier.h index bdca689..c5cf381 100644 --- a/include/qemu/event_notifier.h +++ b/include/qemu/event_notifier.h @@ -15,13 +15,9 @@ #include "qemu-common.h" -#ifdef _WIN32 -#include "qemu/winapi.h" -#endif - struct EventNotifier { #ifdef _WIN32 - HANDLE event; + WinHandle event; #else int rfd; int wfd; @@ -40,7 +36,7 @@ int event_notifier_set_handler(EventNotifier *, EventNotifierHandler *); void event_notifier_init_fd(EventNotifier *, int fd); int event_notifier_get_fd(EventNotifier *); #else -HANDLE event_notifier_get_handle(EventNotifier *); +WinHandle event_notifier_get_handle(EventNotifier *); #endif #endif diff --git a/include/qemu/main-loop.h b/include/qemu/main-loop.h index 6f0200a..aefdc94 100644 --- a/include/qemu/main-loop.h +++ b/include/qemu/main-loop.h @@ -152,7 +152,7 @@ typedef void WaitObjectFunc(void *opaque); * @func: A function to be called when @handle is in a signaled state. * @opaque: A pointer-size value that is passed to @func. */ -int qemu_add_wait_object(HANDLE handle, WaitObjectFunc *func, void *opaque); +int qemu_add_wait_object(WinHandle handle, WaitObjectFunc *func, void *opaque); /** * qemu_del_wait_object: Unregister a callback for a Windows handle @@ -163,7 +163,7 @@ int qemu_add_wait_object(HANDLE handle, WaitObjectFunc *func, void *opaque); * @func: The function that was passed to qemu_add_wait_object. * @opaque: A pointer-size value that was passed to qemu_add_wait_object. */ -void qemu_del_wait_object(HANDLE handle, WaitObjectFunc *func, void *opaque); +void qemu_del_wait_object(WinHandle handle, WaitObjectFunc *func, void *opaque); #endif /* async I/O support */ 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; struct QemuMutex { - CRITICAL_SECTION lock; - LONG owner; + WinCriticalSection lock; + WinLong owner; }; struct QemuCond { - LONG waiters, target; - HANDLE sema; - HANDLE continue_event; + WinLong waiters, target; + WinHandle sema; + WinHandle continue_event; }; struct QemuSemaphore { - HANDLE sema; + WinHandle sema; }; struct QemuEvent { - HANDLE event; + WinHandle event; }; typedef struct QemuThreadData QemuThreadData; @@ -28,6 +39,6 @@ struct QemuThread { }; /* Only valid for joinable threads. */ -HANDLE qemu_thread_get_handle(QemuThread *thread); +WinHandle qemu_thread_get_handle(QemuThread *thread); #endif diff --git a/include/qom/cpu.h b/include/qom/cpu.h index d734be8..6626681 100644 --- a/include/qom/cpu.h +++ b/include/qom/cpu.h @@ -171,7 +171,7 @@ struct CPUState { struct QemuThread *thread; #ifdef _WIN32 - HANDLE hThread; + WinHandle hThread; #endif int thread_id; uint32_t host_tid; diff --git a/include/sysemu/os-win32.h b/include/sysemu/os-win32.h index d625612..a23d6aa 100644 --- a/include/sysemu/os-win32.h +++ b/include/sysemu/os-win32.h @@ -54,6 +54,13 @@ # define EWOULDBLOCK WSAEWOULDBLOCK #endif +/* Define substitutes for the Win API types HANDLE, LONG, ULONG. + * These types are used to avoid dependencies on windows.h. */ + +typedef void * WinHandle; +typedef long WinLong; +typedef unsigned long WinULong; + #if defined(_WIN64) /* On w64, setjmp is implemented by _setjmp which needs a second parameter. * If this parameter is NULL, longjump does no stack unwinding. diff --git a/ui/vnc-enc-tight.c b/ui/vnc-enc-tight.c index 94bb002..2e6fb88 100644 --- a/ui/vnc-enc-tight.c +++ b/ui/vnc-enc-tight.c @@ -27,11 +27,6 @@ */ #include "config-host.h" - -/* This needs to be before jpeglib.h line because of conflict with - INT32 definitions between jmorecfg.h (included by jpeglib.h) and - Win32 basetsd.h (included by windows.h). */ -#include "qemu/winapi.h" /* TODO: workaround, remove */ #include "qemu-common.h" #ifdef CONFIG_VNC_PNG diff --git a/util/event_notifier-win32.c b/util/event_notifier-win32.c index 6dbb530..9f3f61d 100644 --- a/util/event_notifier-win32.c +++ b/util/event_notifier-win32.c @@ -13,6 +13,7 @@ #include "qemu-common.h" #include "qemu/event_notifier.h" #include "qemu/main-loop.h" +#include "qemu/winapi.h" /* CreateEvent, ... */ int event_notifier_init(EventNotifier *e, int active) { diff --git a/util/qemu-thread-win32.c b/util/qemu-thread-win32.c index 27a5217..9de0136 100644 --- a/util/qemu-thread-win32.c +++ b/util/qemu-thread-win32.c @@ -10,11 +10,10 @@ * See the COPYING file in the top-level directory. * */ + +#include "qemu/winapi.h" /* LocalFree, ... */ #include "qemu-common.h" #include "qemu/thread.h" -#include <process.h> -#include <assert.h> -#include <limits.h> static void error_exit(int err, const char *msg) { @@ -30,18 +29,20 @@ static void error_exit(int err, const char *msg) void qemu_mutex_init(QemuMutex *mutex) { mutex->owner = 0; - InitializeCriticalSection(&mutex->lock); + /* mutex->lock uses a data type which must fit CRITICAL_SECTION. */ + g_assert(sizeof(mutex->lock) == sizeof(CRITICAL_SECTION)); + InitializeCriticalSection((CRITICAL_SECTION *)&mutex->lock); } void qemu_mutex_destroy(QemuMutex *mutex) { assert(mutex->owner == 0); - DeleteCriticalSection(&mutex->lock); + DeleteCriticalSection((CRITICAL_SECTION *)&mutex->lock); } void qemu_mutex_lock(QemuMutex *mutex) { - EnterCriticalSection(&mutex->lock); + EnterCriticalSection((CRITICAL_SECTION *)&mutex->lock); /* Win32 CRITICAL_SECTIONs are recursive. Assert that we're not * using them as such. @@ -54,7 +55,7 @@ int qemu_mutex_trylock(QemuMutex *mutex) { int owned; - owned = TryEnterCriticalSection(&mutex->lock); + owned = TryEnterCriticalSection((CRITICAL_SECTION *)&mutex->lock); if (owned) { assert(mutex->owner == 0); mutex->owner = GetCurrentThreadId(); @@ -66,7 +67,7 @@ void qemu_mutex_unlock(QemuMutex *mutex) { assert(mutex->owner == GetCurrentThreadId()); mutex->owner = 0; - LeaveCriticalSection(&mutex->lock); + LeaveCriticalSection((CRITICAL_SECTION *)&mutex->lock); } void qemu_cond_init(QemuCond *cond)
These header files are used by most QEMU source files. If they depend on windows.h, all those source files do so, too. All Windows specific data types which are replaced use identical definitions for the 32 and 64 bit Windows APIs. HANDLE, LONG and CRITICAL_SECTION are replaced by the compatible types WinHandle, WinLong and WinCriticalSection. Add an explicit dependency on qemu/winapi.h for some files which need it. These sources use the Windows API (see comment after include statement) and no longer get windows.h indirectly from other header files. A workaround which was added in the previous patch is no longer needed. Now 175 *.o files remain which still depend on windows.h. Cc: Anthony Liguori <aliguori@amazon.com> Cc: Stefan Hajnoczi <stefanha@redhat.com> Cc: Andreas Färber <afaerber@suse.de> Signed-off-by: Stefan Weil <sw@weilnetz.de> --- cpus.c | 4 +++- hw/intc/apic.c | 3 ++- include/qemu/event_notifier.h | 8 ++------ include/qemu/main-loop.h | 4 ++-- include/qemu/thread-win32.h | 29 ++++++++++++++++++++--------- include/qom/cpu.h | 2 +- include/sysemu/os-win32.h | 7 +++++++ ui/vnc-enc-tight.c | 5 ----- util/event_notifier-win32.c | 1 + util/qemu-thread-win32.c | 17 +++++++++-------- 10 files changed, 47 insertions(+), 33 deletions(-)