[Qemu-devel] [PATCH] set_memory_options: remove code that make no sense

[Qemu-devel] [PATCH] set_memory_options: remove code that make no sense

[Cao jin]

Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
 vl.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/vl.c b/vl.c
index f5f7c3f..13f2c8b 100644
--- a/vl.c
+++ b/vl.c
@@ -2860,11 +2860,6 @@ static void set_memory_options(uint64_t *ram_slots, ram_addr_t *maxram_size,
     sz = 0;
     mem_str = qemu_opt_get(opts, "size");
     if (mem_str) {
-        if (!*mem_str) {
-            error_report("missing 'size' option value");
-            exit(EXIT_FAILURE);
-        }
-
         sz = qemu_opt_get_size(opts, "size", ram_size);
 
         /* Fix up legacy suffix-less format */
@@ -2886,10 +2881,6 @@ static void set_memory_options(uint64_t *ram_slots, ram_addr_t *maxram_size,
 
     sz = QEMU_ALIGN_UP(sz, 8192);
     ram_size = sz;
-    if (ram_size != sz) {
-        error_report("ram size too large");
-        exit(EXIT_FAILURE);
-    }
 
     /* store value for the future use */
     qemu_opt_set_number(opts, "size", ram_size, &error_abort);
-- 
2.1.0

Re: [Qemu-devel] [PATCH] set_memory_options: remove code that make no sense

[Michael Tokarev]

03.11.2015 15:30, Cao jin wrote:
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> ---
>  vl.c | 9 ---------
>  1 file changed, 9 deletions(-)
> 
> diff --git a/vl.c b/vl.c
> index f5f7c3f..13f2c8b 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2860,11 +2860,6 @@ static void set_memory_options(uint64_t *ram_slots, ram_addr_t *maxram_size,
>      sz = 0;
>      mem_str = qemu_opt_get(opts, "size");
>      if (mem_str) {
> -        if (!*mem_str) {
> -            error_report("missing 'size' option value");
> -            exit(EXIT_FAILURE);
> -        }

I'm not sure this one is bad or good, it is indeed possible
to specify no value for size=, but if we're to check that,
we'd have to add such checks everywhere.

But the next one...

>          sz = qemu_opt_get_size(opts, "size", ram_size);
>  
>          /* Fix up legacy suffix-less format */
> @@ -2886,10 +2881,6 @@ static void set_memory_options(uint64_t *ram_slots, ram_addr_t *maxram_size,
>  
>      sz = QEMU_ALIGN_UP(sz, 8192);
>      ram_size = sz;
> -    if (ram_size != sz) {
> -        error_report("ram size too large");
> -        exit(EXIT_FAILURE);
> -    }

is definitely wrong.

sz is uint64_t, while ram_size is ram_addr_t which is
either uint64_t or uintptr_t.  Until it is fixed to
always be 64bits, the above code makes (some) sense.

Thanks,

/mjt

Re: [Qemu-devel] [PATCH] set_memory_options: remove code that make no sense

[Cao jin]

hi Michael,
     Thanks for your explanation that make me realized I am wrong about 
my patch:-[ ...So, forget it.

On 11/03/2015 08:40 PM, Michael Tokarev wrote:
> 03.11.2015 15:30, Cao jin wrote:
>> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
>> ---
>>   vl.c | 9 ---------
>>   1 file changed, 9 deletions(-)
>>
>> diff --git a/vl.c b/vl.c
>> index f5f7c3f..13f2c8b 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -2860,11 +2860,6 @@ static void set_memory_options(uint64_t *ram_slots, ram_addr_t *maxram_size,
>>       sz = 0;
>>       mem_str = qemu_opt_get(opts, "size");
>>       if (mem_str) {
>> -        if (!*mem_str) {
>> -            error_report("missing 'size' option value");
>> -            exit(EXIT_FAILURE);
>> -        }
>
> I'm not sure this one is bad or good, it is indeed possible
> to specify no value for size=, but if we're to check that,
> we'd have to add such checks everywhere.
>

Yup..I missed to test "-m size=", just test several case with format "-m 
###", and didn`t read get_opt_value throughly, or else will find it can 
output a string like "\0" which I never encountered before;)

> But the next one...
>
>>           sz = qemu_opt_get_size(opts, "size", ram_size);
>>
>>           /* Fix up legacy suffix-less format */
>> @@ -2886,10 +2881,6 @@ static void set_memory_options(uint64_t *ram_slots, ram_addr_t *maxram_size,
>>
>>       sz = QEMU_ALIGN_UP(sz, 8192);
>>       ram_size = sz;
>> -    if (ram_size != sz) {
>> -        error_report("ram size too large");
>> -        exit(EXIT_FAILURE);
>> -    }
>
> is definitely wrong.
>
> sz is uint64_t, while ram_size is ram_addr_t which is
> either uint64_t or uintptr_t.  Until it is fixed to
> always be 64bits, the above code makes (some) sense.
>

yes,I am careless... maybe

#ifdef CONFIG_XEN_BACKEND
     if (ram_size != sz) {
         error_report("ram size too large");
         exit(EXIT_FAILURE);
     }
#endif

is better, but no big deal;)

anyway, forget this patch:P
> Thanks,
>
> /mjt
>
>

-- 
Yours Sincerely,

Cao Jin