From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1MWWyi-00027C-8N for qemu-devel@nongnu.org; Thu, 30 Jul 2009 10:51:32 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1MWWyd-00023i-4T for qemu-devel@nongnu.org; Thu, 30 Jul 2009 10:51:31 -0400 Received: from [199.232.76.173] (port=56524 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1MWWyc-00023Z-Vh for qemu-devel@nongnu.org; Thu, 30 Jul 2009 10:51:27 -0400 Received: from mx2.redhat.com ([66.187.237.31]:58801) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1MWWyc-00083S-8U for qemu-devel@nongnu.org; Thu, 30 Jul 2009 10:51:26 -0400 Date: Thu, 30 Jul 2009 11:50:51 -0300 From: Luiz Capitulino Message-ID: <20090730115051.53495100@doriath> In-Reply-To: <4A70769A.7000404@redhat.com> References: <1248818713-11261-1-git-send-email-lcapitulino@redhat.com> <1248818713-11261-2-git-send-email-lcapitulino@redhat.com> <4A701AA3.4060902@redhat.com> <20090729104600.17deb355@doriath> <4A705536.3020305@redhat.com> <20090729112220.16ffe414@doriath> <4A705EAB.1030200@redhat.com> <20090729131131.2d5e4fa6@doriath> <4A70769A.7000404@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: [Qemu-devel] Re: [PATCH 01/25] Introduce QEMU dictionary data type List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Avi Kivity Cc: aliguori@us.ibm.com, jan.kiszka@siemens.com, dlaor@redhat.com, qemu-devel@nongnu.org, filip.navara@gmail.com On Wed, 29 Jul 2009 19:19:38 +0300 Avi Kivity wrote: > On 07/29/2009 07:11 PM, Luiz Capitulino wrote: > > Btw, the patches that ports command handlers have some void * to > > long type conversions (eg. patch 14) can someone please review > > them? > > > > I'm not 100% confident about them. > > > > Casting long to void * may lose precision on some hosts (Windows x64). > Much better off casting the void * to a long * and dereferencing (and > deallocating later). I was going to work on this when I remembered I have copied current's code behavior in this regard, and I _guess_ it may be correct. The monitor accepts four argument types which have integer representation: '/' (the fmt one), '-' (optional), 'i' (integer) and 'l' (long). The optional '-' is a bool, so it won't cause problems. The format one '/' actually expands for three arguments: 'count', 'format' and 'size'. 'format' and 'size' are small integers, won't cause problems. Now 'count' is used to tell 'how much to print', this could cause problems in theory, although I think it won't happen in practice. The integer type 'i' has this cast (monitor.c:2786): args[nb_args++] = (void *)(long)val; 'val' is int64_t. The bad news: if we use a > 32 integer here, there will be problems on Windows x64. The good news: as far as I could understand the handlers, this won't happen in practice. Most handlers use the 'i' type for small integers, the only exception are the handlers using it to represent addresses. This seems to be the case of do_ioport_{read,write} (which seems safe) and do_sum(), which is the only true problem (although doesn't seem serious to me). We could define that 'i' is 32bits and put an assertion there, that will check if more than 32bits are being used. Now the cool part. The 'l' (long) type is the one that should be used to store 64bits values, I think it handles it correctly (monitor.c:2790): #if TARGET_PHYS_ADDR_BITS > 32 args[nb_args++] = (void *)(long)((val >> 32) & 0xffffffff); #else args[nb_args++] = (void *)0; #endif args[nb_args++] = (void *)(long)(val & 0xffffffff); 'val' is the same int64_t. In the dictionary patchset, this type will push the 'high' and 'low' values. Considering all this explanation, I think the patchset is safe, in the worst case it's does the same thing as current code does. Do you agree? A last comment. My patches introduce explicit casting, as in: int index = (long) qemu_dict_get(qdict, "index"); Although the current code doesn't do that, it passes the 'arg[]' contents to handlers, I *guess* the compiler will do the same kind of casting will apply.