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

[Filip Navara]

On Thu, Jul 30, 2009 at 4:50 PM, Luiz Capitulino<address@hidden> wrote:
> On Wed, 29 Jul 2009 19:19:38 +0300
> Avi Kivity <address@hidden> 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.
>

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.

>  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.

It should be limited to 32-bits because of 32-bit hosts.

>  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.

...or just do (void*)(intptr_t)(int)val and watch when it breaks ;-)

>  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.

Again, intptr_t should be used here in long term. long is probably
fine for now before I post the Win64 patches.

>  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?

Agreed.

>  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.
>

Best regards,
Filip Navara