Re: [Qemu-devel] [PATCH 1/5] do not call g_thread_init() for glib >= 2.31

Re: [Qemu-devel] [PATCH 1/5] do not call g_thread_init() for glib >= 2.31

[Stefan Hajnoczi]

On Tue, Apr 29, 2014 at 10:02:24AM +0400, Michael Tokarev wrote:
> glib >= 2.31 always enables thread support and g_thread_supported()
> is #defined to 1, there's no need to call g_thread_init() anymore,
> and it definitely does not need to report error which never happens.
> Keep code for old < 2.31 glibc anyway for now, just #ifdef it
> differently.
> 
> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
> ---
>  coroutine-gthread.c |    7 ++-----
>  util/osdep.c        |   21 +++++++++------------
>  2 files changed, 11 insertions(+), 17 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

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

[Stefan Hajnoczi]

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 <mjt@tls.msk.ru>
> ---
>  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.

I have another approach here:
https://lists.gnu.org/archive/html/qemu-devel/2014-02/msg00236.html

What do you think?

Stefan

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 <mjt@tls.msk.ru>
>> ---
>>  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

Re: [Qemu-devel] [PATCH 0/5] glib thread interface and libcacard cleanups

[Alon Levy]

On 04/29/2014 09:02 AM, Michael Tokarev wrote:
> Basically libgthread has been rewritten in glib version 2.31, and old ways
> to use thread primitives stopped working while new ways appeared.  The two
> interfaces were sufficiently different to warrant large ifdeffery across all
> code using it.
> 
> Here's a patchset which tries to clean usage of glib thread interface across
> this major rewrite.
> 
> The main change was that certain primitives - conditionals and mutexes -
> were dynamic-only before 2.31 (ie, should be allocated using foo_new() and
> freed using foo_free()), while in 2.31 and up, _new()/_free() has been
> deprecated, and new primitives, _init()/_clear(), were added.  So before
> 2.31, we had to declare a pointer call foo_new() to allocate actual object,
> and use this pointer when calling all functions which use this object,
> while in 2.31+, we have to declare actual object and use its address when
> calling functions.
> 
> The trick to make this stuff happy for old glib which I used is to re-define
> actual type to be a pointer to that type, using #define, like this:
> 
>   #define GMutex GMutex*
> 
> so every time the code refers to GMutex it actually refers to a pointer to
> that object.  Plus wrapper #define and inline functioins which accept such
> a pointer and call actual glib function after dereferencing it, like this:
> 
>   static inline g_forward_compat_mutex_lock(GMutex **mutex)
>   {
>     g_mutex_lock(*mutex);
>   }
>   #undef g_mutex_lock
>   #define g_mutex_lock(mutex) g_forward_compat_mutex_lock(mutex)
> 
> This way, we use new, 2.31+, glib interface everywhere, but for pre-2.31
> glib, this interface is wrapped using old API and by converting the
> actual object to a pointer to actual object behind the scenes.
> 
> It is hackish, but this allows to write very very clean, new-style, code,
> and compile it with old glib.
> 
> The only difference with actual new interface is that in new, 2.31+, glib,
> those objects, when declared statically, don't need to be initialized and
> will just work when passed to their functions.  While for old interface,
> actual objects needs to be allocated using g_foo_new().  So I added a
> set of functions, g_foo_init_static(), which should be called in the same
> place where old code expected to have g_foo_new().  For new interface
> those functions evaluates to nothing, but for old interface they call
> the allocation routine.
> 
> It is not the same as g_foo_init(), -- I wanted to distinguish this
> _static() method from regular init() (tho any of those can be used),
> because it is easy this way to find places in the code which can
> benefit from cleanups later when we'll drop support for glib < 2.31.
> 
> The series consists of 5 patches:
> 
> - do not call g_thread_init() for glib >= 2.31
> 
>  This is a cleanup patch, cleaning g_thread_init() call.  In 2.31+,
>  threads are always enabled and initialized (and glib can't be built
>  without threads), and g_thread_supported() is #defined to be 1.
>  So the #ifdeffery didn't make any sense to start with, especially
>  printing error message and aborting if !g_thread_supported().
> 
> - glib-compat.h: add new thread API emulation on top of pre-2.31 API
> 
>  This is the main and largest part.  Introducing described above
>  compatibility layer into include/glib-compat.h.
> 
>  This patch also cleans up direct usage of GCond and GMutex in the code
>  in 2 fles: coroutine-gthread.c and trace/simple.c.  In the latter,
>  whole ifdeffery is eliminated this way completely, while in the
>  latter, there's one more primitive which received rewrite in the
>  same version of glib, -- thread private data, GPrivate and GStaticPrivate,
>  which still uses #ifdefs.
> 
>  I had to introduce the compat layer together with the changes in usage,
>  because else the ifdeffery around usage conflicts with the compat layer.
> 
>  In coroutine-gthread.c, I also replaced GStaticMutex (from old glib)
>  with regular GMutex.  The thing is that actually, GStaticMutex was
>  very similar to what I've done with the actual object vs a pointer to
>  object - it works in term of GMutex, but stores just a pointer of it,
>  and allocates it on demand dynamically.  Using GMutex directly makes
>  it a bit faster.
> 
> - vscclient: use glib thread primitives not qemu
> - libcacard: replace qemu thread primitives with glib ones
> 
>  convert libcacard from qemu thread primitives to glib thread primitives
>  using the new compatibility layer.  This way, libcacard becomes stand-alone
>  library and does not depend on qemu anymore, so programs using it are
>  not required to mess with qemu objects.
> 
>  an unrelated-to-glib change which I had to apply to libcacard here was
>  to replace pstrcpy() back to strncpy().  The reverse conversion has been
>  done in the past, this patch reverts it back to usage of strncpy().
> 
>  and we've some tiny OS-compat code added to vscclient.c here.
> 
> - libcacard: actually use symbols file
> 
>  this is the change which started whole series.  This patch makes export
>  list for libcacard.so to be strict, exporting only really necessary
>  symbols, omitting internal symbols.  Previously, libcacard used and
>  exported many of qemu internals, including thread functions.  Now
>  it not only stopped exporting them, but also stopped using them.
> 
> The whole thing has been compile-tested with both new and old glib
> versions on linux and FreeBSD, and runtime-tested on linux (again,
> both old and new versions) with --with-coroutine=gthread.  I didn't
> test libcacard much, because I found no testcases for it, but at
> least it _appears_ to work.
> 
> The diffstat below does not look like a diffstat of a cleanup, because
> the patchset adds about 2 times more lines than it removes.  This is
> because of large addition to glib-compat.h, plus addition of compat
> code to vscclient, to make it independent of qemu.
> 
> and a few others.
> 
> Thanks,
> 

Reviewed-by: Alon Levy <alevy@redhat.com>
Tested-by: Alon Levy <alevy@redhat.com>

This would be nice to have too (it has nothing to do with your patchset,
but it would save me a pull request):

commit 2fc95f8bc1912e2de243389d9d102a5a28267f31
Author: Alon Levy <alevy@redhat.com>
Date:   Mon May 5 17:41:32 2014 +0300

    libcacard: remove unnecessary EOL from debug prints

    Signed-off-by: Alon Levy <alevy@redhat.com>

diff --git a/libcacard/vreader.c b/libcacard/vreader.c
index 91799b4..d1e05af 100644
--- a/libcacard/vreader.c
+++ b/libcacard/vreader.c
@@ -272,12 +272,12 @@ vreader_xfr_bytes(VReader *reader,
         response = vcard_make_response(status);
         card_status = VCARD_DONE;
     } else {
-        g_debug("%s: CLS=0x%x,INS=0x%x,P1=0x%x,P2=0x%x,Lc=%d,Le=%d %s\n",
+        g_debug("%s: CLS=0x%x,INS=0x%x,P1=0x%x,P2=0x%x,Lc=%d,Le=%d %s",
               __func__, apdu->a_cla, apdu->a_ins, apdu->a_p1, apdu->a_p2,
               apdu->a_Lc, apdu->a_Le, apdu_ins_to_string(apdu->a_ins));
         card_status = vcard_process_apdu(card, apdu, &response);
         if (response) {
-            g_debug("%s: status=%d sw1=0x%x sw2=0x%x len=%d (total=%d)\n",
+            g_debug("%s: status=%d sw1=0x%x sw2=0x%x len=%d (total=%d)",
                   __func__, response->b_status, response->b_sw1,
                   response->b_sw2, response->b_len, response->b_total_len);
         }


> /mjt
> 
> Michael Tokarev (5):
>   do not call g_thread_init() for glib >= 2.31
>   glib-compat.h: add new thread API emulation on top of pre-2.31 API
>   vscclient: use glib thread primitives not qemu
>   libcacard: replace qemu thread primitives with glib ones
>   libcacard: actually use symbols file
> 
>  coroutine-gthread.c        |   37 ++++++----------
>  include/glib-compat.h      |  103 ++++++++++++++++++++++++++++++++++++++++++++
>  libcacard/Makefile         |   10 +----
>  libcacard/event.c          |   25 ++++++-----
>  libcacard/vcard_emul_nss.c |    3 +-
>  libcacard/vreader.c        |   19 ++++----
>  libcacard/vscclient.c      |   75 +++++++++++++++++++-------------
>  trace/simple.c             |   50 ++++++---------------
>  util/osdep.c               |   21 ++++-----
>  9 files changed, 206 insertions(+), 137 deletions(-)
> 

Re: [Qemu-devel] [PATCH 0/5] glib thread interface and libcacard cleanups

[Michael Tokarev]

05.05.2014 18:49, Alon Levy wrote:
> On 04/29/2014 09:02 AM, Michael Tokarev wrote:
>> Basically libgthread has been rewritten in glib version 2.31, and old ways
>> to use thread primitives stopped working while new ways appeared.  The two
>> interfaces were sufficiently different to warrant large ifdeffery across all
>> code using it.
[...]
[]
> Reviewed-by: Alon Levy <alevy@redhat.com>
> Tested-by: Alon Levy <alevy@redhat.com>

Hmm.  Now I'm a bit confused.  Which version did you test? :)

I posted a v2 patch which splits one of the changes into two
(pstrcpy to memcpy conversion in libcacard), added some more
(minor) changes (which should not affect libcacard code in
any way), and adjusted commit messages.

The main code is not affected (or should not be), so Tested-by
probably may stay, except of the pstrcpy to memcpy patch
(http://patchwork.ozlabs.org/patch/345002/) which may affect
libcacard.

Here's the v2: http://lists.gnu.org/archive/html/qemu-devel/2014-05/msg00286.html

If you tested the git branch which I referred to, that's the
v2, not original submission which you're replying to.

> This would be nice to have too (it has nothing to do with your patchset,
> but it would save me a pull request):
> 
> commit 2fc95f8bc1912e2de243389d9d102a5a28267f31
> Author: Alon Levy <alevy@redhat.com>
> Date:   Mon May 5 17:41:32 2014 +0300
> 
>     libcacard: remove unnecessary EOL from debug prints

Well, this can easily go to -trivial, as I'm planning to send a pull
request for it soon anyway.

Thank you!

/mjt

Re: [Qemu-devel] [PATCH 0/5] glib thread interface and libcacard cleanups

[Alon Levy]

On 05/05/2014 07:36 PM, Michael Tokarev wrote:
> 05.05.2014 18:49, Alon Levy wrote:
>> On 04/29/2014 09:02 AM, Michael Tokarev wrote:
>>> Basically libgthread has been rewritten in glib version 2.31, and old ways
>>> to use thread primitives stopped working while new ways appeared.  The two
>>> interfaces were sufficiently different to warrant large ifdeffery across all
>>> code using it.
> [...]
> []
>> Reviewed-by: Alon Levy <alevy@redhat.com>
>> Tested-by: Alon Levy <alevy@redhat.com>
> 
> Hmm.  Now I'm a bit confused.  Which version did you test? :)
> 
> I posted a v2 patch which splits one of the changes into two
> (pstrcpy to memcpy conversion in libcacard), added some more
> (minor) changes (which should not affect libcacard code in
> any way), and adjusted commit messages.
> 
> The main code is not affected (or should not be), so Tested-by
> probably may stay, except of the pstrcpy to memcpy patch
> (http://patchwork.ozlabs.org/patch/345002/) which may affect
> libcacard.
> 
> Here's the v2: http://lists.gnu.org/archive/html/qemu-devel/2014-05/msg00286.html
> 
> If you tested the git branch which I referred to, that's the
> v2, not original submission which you're replying to.

I've tested and reviewed 7191b2c43eecc52994924245720c534ea1a0dc84 so v2,
my bad.

> 
>> This would be nice to have too (it has nothing to do with your patchset,
>> but it would save me a pull request):
>>
>> commit 2fc95f8bc1912e2de243389d9d102a5a28267f31
>> Author: Alon Levy <alevy@redhat.com>
>> Date:   Mon May 5 17:41:32 2014 +0300
>>
>>     libcacard: remove unnecessary EOL from debug prints
> 
> Well, this can easily go to -trivial, as I'm planning to send a pull
> request for it soon anyway.
> 
> Thank you!
> 
> /mjt
>