From patchwork Mon May 5 14:49:30 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Alon Levy X-Patchwork-Id: 345736 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 7B1DB1402C9 for ; Tue, 6 May 2014 00:50:09 +1000 (EST) Received: from localhost ([::1]:57830 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WhKDX-0007lq-G5 for incoming@patchwork.ozlabs.org; Mon, 05 May 2014 10:50:07 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52996) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WhKDB-0007O8-9w for qemu-devel@nongnu.org; Mon, 05 May 2014 10:49:51 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WhKD5-0008VC-3J for qemu-devel@nongnu.org; Mon, 05 May 2014 10:49:45 -0400 Received: from mx1.redhat.com ([209.132.183.28]:40846) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WhKD4-0008V0-Rn for qemu-devel@nongnu.org; Mon, 05 May 2014 10:49:39 -0400 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s45Enbck001193 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Mon, 5 May 2014 10:49:37 -0400 Received: from blues.lan (vpn1-6-74.ams2.redhat.com [10.36.6.74]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id s45EnUZ6005329 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Mon, 5 May 2014 10:49:35 -0400 Message-ID: <5367A4FA.4040000@redhat.com> Date: Mon, 05 May 2014 17:49:30 +0300 From: Alon Levy User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.4.0 MIME-Version: 1.0 To: Michael Tokarev , qemu-devel References: <1398751349-20869-1-git-send-email-mjt@msgid.tls.msk.ru> In-Reply-To: <1398751349-20869-1-git-send-email-mjt@msgid.tls.msk.ru> X-Enigmail-Version: 1.6 X-Scanned-By: MIMEDefang 2.68 on 10.5.11.26 X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x X-Received-From: 209.132.183.28 Subject: Re: [Qemu-devel] [PATCH 0/5] glib thread interface and libcacard cleanups X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org 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 Tested-by: Alon Levy 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 Date: Mon May 5 17:41:32 2014 +0300 libcacard: remove unnecessary EOL from debug prints Signed-off-by: Alon Levy > /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(-) > 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); }