From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1MWX6I-0006da-Dl for qemu-devel@nongnu.org; Thu, 30 Jul 2009 10:59:22 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1MWX6D-0006Xm-Dd for qemu-devel@nongnu.org; Thu, 30 Jul 2009 10:59:21 -0400 Received: from [199.232.76.173] (port=32956 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1MWX6C-0006XY-R1 for qemu-devel@nongnu.org; Thu, 30 Jul 2009 10:59:16 -0400 Received: from mx2.redhat.com ([66.187.237.31]:40082) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1MWX6C-0001Ix-48 for qemu-devel@nongnu.org; Thu, 30 Jul 2009 10:59:16 -0400 Message-ID: <4A71B65B.1010700@redhat.com> Date: Thu, 30 Jul 2009 18:03:55 +0300 From: Avi Kivity MIME-Version: 1.0 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> <20090730115051.53495100@doriath> In-Reply-To: <20090730115051.53495100@doriath> Content-Type: text/plain; charset=ISO-8859-1; format=flowed 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: Luiz Capitulino Cc: aliguori@us.ibm.com, jan.kiszka@siemens.com, dlaor@redhat.com, qemu-devel@nongnu.org, filip.navara@gmail.com On 07/30/2009 05:50 PM, Luiz Capitulino wrote: > 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? > I think so. > 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. > So the existing code splits 64-bit types into two 32-bit values. As long as you do the same (for now), we should be safe. Later of course we can replace it with a QNumber or QInteger. -- error compiling committee.c: too many arguments to function