From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60795) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WgCJk-0006nh-Kr for qemu-devel@nongnu.org; Fri, 02 May 2014 08:11:57 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WgCJg-0006J8-7y for qemu-devel@nongnu.org; Fri, 02 May 2014 08:11:52 -0400 Received: from isrv.corpit.ru ([86.62.121.231]:40595) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WgCJf-0006Iw-Ra for qemu-devel@nongnu.org; Fri, 02 May 2014 08:11:48 -0400 Message-ID: <53638B82.4020704@msgid.tls.msk.ru> Date: Fri, 02 May 2014 16:11:46 +0400 From: Michael Tokarev MIME-Version: 1.0 References: <1398751349-20869-1-git-send-email-mjt@msgid.tls.msk.ru> <1398751349-20869-3-git-send-email-mjt@msgid.tls.msk.ru> <20140502094529.GI8005@stefanha-thinkpad.redhat.com> In-Reply-To: <20140502094529.GI8005@stefanha-thinkpad.redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 2/5] glib-compat.h: add new thread API emulation on top of pre-2.31 API List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: Benoit Canet , qemu-devel@nongnu.org, Christophe Fergeau , Alon Levy , Stefan Hajnoczi , Paolo Bonzini 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 >> --- >> 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