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

["Andreas Färber" <afaerber@suse.de>]

Am 23.02.2014 18:02, schrieb Stefan Weil:
> 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 and LONG
> can be directly replaced by void * and long. CRITICAL_SECTION
> is replaced by a new struct of the same size.
> 
> Add an explicit dependency on sysemu/os-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: Kevin Wolf <kwolf@redhat.com>
> Signed-off-by: Stefan Weil <sw@weilnetz.de>
[...]
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index 367eda1..6ea48c4 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h

First of all, why was I not CC'ed on this change?
File is listed under "CPU" subsystem in MAINTAINERS.

Patches not getting sufficient review is one risk, causing merge
conflicts another. People should be aware of changes you make in their
files, even if "just" Windows-related.

> @@ -170,7 +170,7 @@ struct CPUState {
>  
>      struct QemuThread *thread;
>  #ifdef _WIN32
> -    HANDLE hThread;
> +    void *hThread;
>  #endif
>      int thread_id;
>      uint32_t host_tid;

I had moved the field unchanged from another header, I believe. I don't
think this is a good change (assuming the Windows API is still using
this type and not void*), especially since it's #ifdef'ed anyway.

And I don't understand why reducing the number of files dependent on
windows.h is a good thing either. Your cover letter does not explain.
We've not been trying to reduce dependencies on glib either, nor is
there precedence for replacing other pointer types elsewhere (e.g.
qemu_irq). Wherever possible, we've been using the most precise pointer
type to let the compiler or static analysis tools help us catch mismatches.

Is all this just to speed up rebuilds after MinGW updates?

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg