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

[Alon Levy <alevy@redhat.com>]

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(-)
>