Following commit f46bfdbfc8f95cf65d7818ef68a801e063c40332 (util/cutils: Change
qemu_strtosz*() from int64_t to uint64_t) which added a similar check to
cvtnum. As a result there is no need to check it separately outside of cvtnum.

Acked-by: Mark Kanda <mark.kanda@oracle.com>
Signed-off-by: Eyal Moscovici <eyal.moscovici@oracle.com>
---
 qemu-img.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 6a4327aaba..116a9c6349 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -4307,7 +4307,7 @@ static int img_bench(int argc, char **argv)
             int64_t sval;
 
             sval = cvtnum(optarg);
-            if (sval < 0 || sval > INT_MAX) {
+            if (sval < 0) {
                 error_report("Invalid buffer size specified");
                 return 1;
             }
@@ -4320,7 +4320,7 @@ static int img_bench(int argc, char **argv)
             int64_t sval;
 
             sval = cvtnum(optarg);
-            if (sval < 0 || sval > INT_MAX) {
+            if (sval < 0) {
                 error_report("Invalid step size specified");
                 return 1;
             }
@@ -4493,7 +4493,7 @@ static int img_dd_bs(const char *arg,
 
     res = cvtnum(arg);
 
-    if (res <= 0 || res > INT_MAX) {
+    if (res <= 0) {
         error_report("invalid number: '%s'", arg);
         return 1;
     }
-- 
2.17.2 (Apple Git-113)

On 5/6/20 4:34 PM, Eyal Moscovici wrote:
> Following commit f46bfdbfc8f95cf65d7818ef68a801e063c40332 (util/cutils: Change
> qemu_strtosz*() from int64_t to uint64_t) which added a similar check to
> cvtnum. As a result there is no need to check it separately outside of cvtnum.
> 
> Acked-by: Mark Kanda <mark.kanda@oracle.com>
> Signed-off-by: Eyal Moscovici <eyal.moscovici@oracle.com>
> ---
>   qemu-img.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index 6a4327aaba..116a9c6349 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -4307,7 +4307,7 @@ static int img_bench(int argc, char **argv)
>               int64_t sval;
>   
>               sval = cvtnum(optarg);
> -            if (sval < 0 || sval > INT_MAX) {
> +            if (sval < 0) {
>                   error_report("Invalid buffer size specified");

INT_MAX is smaller than cvtnum's check for INT64_MAX.  This code change 
allows larger buffer sizes, which is probably not a good idea.

>                   return 1;
>               }
> @@ -4320,7 +4320,7 @@ static int img_bench(int argc, char **argv)
>               int64_t sval;
>   
>               sval = cvtnum(optarg);
> -            if (sval < 0 || sval > INT_MAX) {
> +            if (sval < 0) {
>                   error_report("Invalid step size specified");
>                   return 1;
>               }
> @@ -4493,7 +4493,7 @@ static int img_dd_bs(const char *arg,
>   
>       res = cvtnum(arg);
>   
> -    if (res <= 0 || res > INT_MAX) {
> +    if (res <= 0) {
>           error_report("invalid number: '%s'", arg);
>           return 1;
>       }
> 

NACK.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On 07/05/2020 0:49, Eric Blake wrote:
> On 5/6/20 4:34 PM, Eyal Moscovici wrote:
>> Following commit f46bfdbfc8f95cf65d7818ef68a801e063c40332 
>> (util/cutils: Change
>> qemu_strtosz*() from int64_t to uint64_t) which added a similar check to
>> cvtnum. As a result there is no need to check it separately outside 
>> of cvtnum.
>>
>> Acked-by: Mark Kanda <mark.kanda@oracle.com>
>> Signed-off-by: Eyal Moscovici <eyal.moscovici@oracle.com>
>> ---
>>   qemu-img.c | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/qemu-img.c b/qemu-img.c
>> index 6a4327aaba..116a9c6349 100644
>> --- a/qemu-img.c
>> +++ b/qemu-img.c
>> @@ -4307,7 +4307,7 @@ static int img_bench(int argc, char **argv)
>>               int64_t sval;
>>                 sval = cvtnum(optarg);
>> -            if (sval < 0 || sval > INT_MAX) {
>> +            if (sval < 0) {
>>                   error_report("Invalid buffer size specified");
>
> INT_MAX is smaller than cvtnum's check for INT64_MAX.  This code 
> change allows larger buffer sizes, which is probably not a good idea.
I was the most hesitant about this patch because of the size difference. 
I decided to submit it because the type is int64 which pairs better with 
the MAX_INT64 check and I couldn't find a concrete reason to cap the 
variable at MAX_INT. Do you a concrete reason? Because the max size 
should rerally come into effect on very fringe cases and if you are 
asking for a really big buffer you should know the risks.
>
>>                   return 1;
>>               }
>> @@ -4320,7 +4320,7 @@ static int img_bench(int argc, char **argv)
>>               int64_t sval;
>>                 sval = cvtnum(optarg);
>> -            if (sval < 0 || sval > INT_MAX) {
>> +            if (sval < 0) {
>>                   error_report("Invalid step size specified");
>>                   return 1;
>>               }
>> @@ -4493,7 +4493,7 @@ static int img_dd_bs(const char *arg,
>>         res = cvtnum(arg);
>>   -    if (res <= 0 || res > INT_MAX) {
>> +    if (res <= 0) {
>>           error_report("invalid number: '%s'", arg);
>>           return 1;
>>       }
>>
>
> NACK.
>

On 5/12/20 4:39 AM, Eyal Moscovici wrote:

>>> +++ b/qemu-img.c
>>> @@ -4307,7 +4307,7 @@ static int img_bench(int argc, char **argv)
>>>               int64_t sval;
>>>                 sval = cvtnum(optarg);
>>> -            if (sval < 0 || sval > INT_MAX) {
>>> +            if (sval < 0) {
>>>                   error_report("Invalid buffer size specified");
>>
>> INT_MAX is smaller than cvtnum's check for INT64_MAX.  This code 
>> change allows larger buffer sizes, which is probably not a good idea.
> I was the most hesitant about this patch because of the size difference. 
> I decided to submit it because the type is int64 which pairs better with 
> the MAX_INT64 check and I couldn't find a concrete reason to cap the 
> variable at MAX_INT. Do you a concrete reason? Because the max size 
> should rerally come into effect on very fringe cases and if you are 
> asking for a really big buffer you should know the risks.

The commit message does not call out a change in behavior.  If you are 
sure that you want a change in behavior, then justify that: mention that 
your patch specifically changes the behavior to allow larger buffers, 
and why that is okay.  Otherwise, it looks like your patch is accidental 
in its behavior change, which costs reviewer time.

In this particular case, I see no reason to allow larger buffers.  That 
is, the existing limits made sense (benchmarking anything larger than 
the maximum qcow2 cluster size of 2M is unlikely to produce useful 
results, let alone something as large as 2G, so allowing > 2G is not 
helping the user).  So even if your commit message did a good job of 
explaining that the change was intentional, it is a tough sell why we 
need it.  If all your commit is intended to do is refactoring, then it 
should not be changing behavior.


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org