Re: [Qemu-devel] [PATCH 2/5] glib-compat.h: add new thread API emulation on top of pre-2.31 API

[Michael Tokarev]

02.05.2014 13:45, Stefan Hajnoczi wrote:
> On Tue, Apr 29, 2014 at 10:02:25AM +0400, Michael Tokarev wrote:
>> Thread API changed in glib-2.31 significantly.  Before that version,
>> conditionals and mutexes were only allocated dynamically, using
>> _new()/_free() interface.  in 2.31 and up, they're allocated statically
>> as regular variables, and old interface is deprecated.
>>
>> (Note: glib docs says the new interface is available since version
>> 2.32, but it was actually introduced in version 2.31).
>>
>> Create the new interface using old primitives, re-defining the base
>> types (GCond and GMutex) to be pointers.
>>
>> Replace #ifdeffery around GCond and GMutex in trace/simple.c and
>> coroutine-gthread.c too because it does not work anymore with the new
>> glib-compat.h.
>>
>> Signed-off-by: Michael Tokarev <address@hidden>
>> ---
>>  coroutine-gthread.c   |   30 +++++---------
>>  include/glib-compat.h |  103 
>> +++++++++++++++++++++++++++++++++++++++++++++++++
>>  trace/simple.c        |   50 +++++++-----------------
>>  3 files changed, 126 insertions(+), 57 deletions(-)
> 
> The approach in this patch is more invasive due to the #ifdef of the
> pointer types.

This statement is two-fold.

My approach is more invasive as it redefines glib internals (namely 2 types
used by glib, GMutex and GCond).  So it is invasive because it modifies glib
interface behind the scenes without no one noticing, so to say.

This might be problematic only when we #include some 3rd party header which
uses those types, especially as arguments to its functions or part of its
structures (and this will not be catched by the compiler after qemu redefines
these types).  And hell will freeze when someone will try to run such a
program, because that 3rd party library was compiled and expects one type,
but actually is given another (a pointer).

The thing is, it is not that often when a library expects GMutexes and GConds.
It can expect GThread or some more high-level primitives, and use GMutexes
internally.

The possible only problem with this approach is when we will use such a 3rd
party library which declares structures which _embeds_ GMutex or GCond in
a public structure which is allocated by the caller.  By changing GMutex
to be a pointer to GMutex, we change the size of that strucure, and so
we'll allocate less than needed, which leads to memory corruption.  But
even there, a 3rd party lib can't use GMutex type with old glib, because
old glib did not declare GMutex itself, it only declared it as some struct.
So a 3rd party lib can use GMutex type directly only with _new_ glib, for
which we don't re-define this type.  If, however, it uses pointer to
GMutex in this structure, it will be substituted with a pointer to a
pointer, so the structure itself wont change, and everything will work
just fine.

This is confusing, that's for sure.  But so far, I don't see any really
problematic usage, at least not any which can not be detected at compile
time.


But, returning to the original statement about invasiveness.  My approach
is significantly LESS invasive to the resulting code.  Thre resulting code
uses stright, documented, new glib API everywhere.  Except of one thing -
the primitive initialization, which I already mentioned in the introduction
message.

Yes, it is easy to overlook and omit/forget about this initializers, but
it will be immediately catched by first actual usage of these primitives
at runtime, with a nice SIGSEGV.  And it is not that often when we're
adding usage of new glib-specific primitives into qemu, because it has
its own, based on posix (or win32).  (Which, in turn, can just be converted
to glib now, once it has the nice compat layer.. but not for GPrivate, so
it is a bit premature.  Again, using the nice unobfuscated new glib API).

> I have another approach here:
> https://lists.gnu.org/archive/html/qemu-devel/2014-02/msg00236.html
> 
> What do you think?

This works too.  And should be fine too.  Except that it adds the compat
layer to users of the interface, forcing users to use compat API not
the main glib API.  So this way, it is more invasive than mine. ;)

Thanks,

/mjt