Re: [Qemu-devel] [PATCH] use an unsigned long for the max_sz parameter in load_image_targphys

[Alexander Graf]

On 09.03.2012, at 14:15, Mark Langsdorf wrote:

> On 03/09/2012 03:25 AM, Markus Armbruster wrote:
>> Mark Langsdorf <address@hidden> writes:
>> 
>>> Allow load_image_targphys to load files on systems with more than 2G of
>>> emulated memory by changing the max_sz parameter from an int to an
>>> unsigned long.
>>> 
>>> Signed-off-by: Mark Langsdorf <address@hidden>
>>> ---
>>> hw/loader.c |    4 ++--
>>> hw/loader.h |    3 ++-
>>> 2 files changed, 4 insertions(+), 3 deletions(-)
>>> 
>>> diff --git a/hw/loader.c b/hw/loader.c
>>> index 415cdce..a5333d0 100644
>>> --- a/hw/loader.c
>>> +++ b/hw/loader.c
>>> @@ -103,9 +103,9 @@ ssize_t read_targphys(const char *name,
>>> 
>>> /* return the size or -1 if error */
>>> int load_image_targphys(const char *filename,
>>> -                   target_phys_addr_t addr, int max_sz)
>>> +                        target_phys_addr_t addr, unsigned long max_sz)
>>> {
>>> -    int size;
>>> +    unsigned long size;
>>> 
>>>     size = get_image_size(filename);
>>>     if (size > max_sz) {
>> 
>> get_image_size() returns int.  How does widening size and max_sz here
>> improve things?
> 
> If max_sz is greater than 2GB, then:
>  int max_sz = 0xc0000000;
>  int size =   0x300;
>  if (size > max_sz)
>      return -1;
> 
> returns -1, even though size is much less than max_sz.
> 
> doing it my way:
>  unsigned long max_sz = 0xc0000000;
>  unsigned long size =   0x300;
>  if (size > max_sz)
>      return -1;
> 
> does not return -1.

So why change it to long then? Unsigned int would have the same effect.

But really what this should be changed to is target_phys_addr_t. That way it's 
aligned with the address. I guess we can leave int for return values for now 
though, since we won't get images that big.

Also, why are you hitting this in the first place? How are you calling 
read_targphys that you end up with such a big max_sz? Using INT_MAX as max_sz 
should work just as well, no? It's probably cleaner to change the size type, 
but I'm curious why nobody else hit this before :).


Alex