From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1MWXB0-0002i7-OX for qemu-devel@nongnu.org; Thu, 30 Jul 2009 11:04:14 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1MWXAw-0002dL-Tk for qemu-devel@nongnu.org; Thu, 30 Jul 2009 11:04:14 -0400 Received: from [199.232.76.173] (port=48855 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1MWXAw-0002dB-Lv for qemu-devel@nongnu.org; Thu, 30 Jul 2009 11:04:10 -0400 Received: from mail-ew0-f210.google.com ([209.85.219.210]:63284) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1MWXAw-0002CZ-1b for qemu-devel@nongnu.org; Thu, 30 Jul 2009 11:04:10 -0400 Received: by ewy6 with SMTP id 6so805732ewy.34 for ; Thu, 30 Jul 2009 08:04:03 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <20090730115051.53495100@doriath> 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> Date: Thu, 30 Jul 2009 17:04:02 +0200 Message-ID: <5b31733c0907300804n5ccb1c7cla77e193b90718eb6@mail.gmail.com> From: Filip Navara Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable 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, Avi Kivity On Thu, Jul 30, 2009 at 4:50 PM, Luiz Capitulino wr= ote: > 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? >> > >> > =A0 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). > > =A0I 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. > > =A0The monitor accepts four argument types which have integer > representation: '/' (the fmt one), '-' (optional), 'i' (integer) > and 'l' (long). > > =A0The optional '-' is a bool, so it won't cause problems. > > =A0The 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. > > =A0The integer type 'i' has this cast (monitor.c:2786): > > args[nb_args++] =3D (void *)(long)val; > > =A0'val' is int64_t. > Even if 32-bit values are used this will cause warning on MinGW64, it is better to use intptr_t for these conversions. I have some preliminary patches to fix this for the code that is currently in GIT, but I won't mind if long is used for now and everything is fixed later at once. > =A0The 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. It should be limited to 32-bits because of 32-bit hosts. > =A0Most 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). > > =A0We could define that 'i' is 32bits and put an assertion > there, that will check if more than 32bits are being used. ...or just do (void*)(intptr_t)(int)val and watch when it breaks ;-) > =A0Now 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 > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0args[nb_args++] =3D (void *)(long)= ((val >> 32) & 0xffffffff); > #else > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0args[nb_args++] =3D (void *)0; > #endif > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0args[nb_args++] =3D (void *)(long)= (val & 0xffffffff); > > =A0'val' is the same int64_t. Again, intptr_t should be used here in long term. long is probably fine for now before I post the Win64 patches. > =A0In the dictionary patchset, this type will push the 'high' and > 'low' values. > > =A0Considering all this explanation, I think the patchset is safe, > in the worst case it's does the same thing as current code does. > > =A0Do you agree? Agreed. > =A0A last comment. My patches introduce explicit casting, as in: > > int index =3D (long) qemu_dict_get(qdict, "index"); > > =A0Although 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. > Best regards, Filip Navara