[Qemu-devel] Re: [PATCH 01/25] Introduce QEMU dictionary data type

[Luiz Capitulino <lcapitulino@redhat.com>]

On Wed, 29 Jul 2009 19:19:38 +0300
Avi Kivity <avi@redhat.com> 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.