diff mbox

[v3,1/2] blockdev: Error out on negative throttling option values

Message ID 1452744489-3513-2-git-send-email-famz@redhat.com
State New
Headers show

Commit Message

Fam Zheng Jan. 14, 2016, 4:08 a.m. UTC 
The implicit casting from unsigned int to double changes negative values
into large positive numbers and accepts them.  We should instead print
an error.

Check the number range so this case is caught and reported.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 blockdev.c              |  3 ++-
 include/qemu/throttle.h |  2 ++
 util/throttle.c         | 16 ++++++----------
 3 files changed, 10 insertions(+), 11 deletions(-)

Comments

Max Reitz Jan. 14, 2016, 3:46 p.m. UTC | #1 
On 14.01.2016 05:08, Fam Zheng wrote:
> The implicit casting from unsigned int to double changes negative values
> into large positive numbers and accepts them.  We should instead print
> an error.
> 
> Check the number range so this case is caught and reported.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  blockdev.c              |  3 ++-
>  include/qemu/throttle.h |  2 ++
>  util/throttle.c         | 16 ++++++----------
>  3 files changed, 10 insertions(+), 11 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>

> diff --git a/blockdev.c b/blockdev.c
> index 2df0c6d..1afef87 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -348,7 +348,8 @@ static bool check_throttle_config(ThrottleConfig *cfg, Error **errp)
>      }
>  
>      if (!throttle_is_valid(cfg)) {
> -        error_setg(errp, "bps/iops/maxs values must be 0 or greater");
> +        error_setg(errp, "bps/iops/maxs values must be within [0, %" PRId64
> +                         ")", (int64_t)THROTTLE_VALUE_MAX);

I personally would have liked a simpler %lli and no cast, but I can see
why you want an explicit int64_t here.

>          return false;
>      }
>  
> diff --git a/include/qemu/throttle.h b/include/qemu/throttle.h
> index 12faaad..d0c98ed 100644
> --- a/include/qemu/throttle.h
> +++ b/include/qemu/throttle.h
> @@ -29,6 +29,8 @@
>  #include "qemu-common.h"
>  #include "qemu/timer.h"
>  
> +#define THROTTLE_VALUE_MAX 1000000000000000LL

<pedantic>
But then you could use UINT64_C(1000000000000000) here.
</pedantic>
Max Reitz Jan. 14, 2016, 3:50 p.m. UTC | #2 
On 14.01.2016 16:46, Max Reitz wrote:
> On 14.01.2016 05:08, Fam Zheng wrote:
>> The implicit casting from unsigned int to double changes negative values
>> into large positive numbers and accepts them.  We should instead print
>> an error.
>>
>> Check the number range so this case is caught and reported.
>>
>> Signed-off-by: Fam Zheng <famz@redhat.com>
>> ---
>>  blockdev.c              |  3 ++-
>>  include/qemu/throttle.h |  2 ++
>>  util/throttle.c         | 16 ++++++----------
>>  3 files changed, 10 insertions(+), 11 deletions(-)
> 
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> 
>> diff --git a/blockdev.c b/blockdev.c
>> index 2df0c6d..1afef87 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -348,7 +348,8 @@ static bool check_throttle_config(ThrottleConfig *cfg, Error **errp)
>>      }
>>  
>>      if (!throttle_is_valid(cfg)) {
>> -        error_setg(errp, "bps/iops/maxs values must be 0 or greater");
>> +        error_setg(errp, "bps/iops/maxs values must be within [0, %" PRId64

Pre-existing, but you might want to fix this to "bps/iops/max" while
touching this line.

Max

>> +                         ")", (int64_t)THROTTLE_VALUE_MAX);
> 
> I personally would have liked a simpler %lli and no cast, but I can see
> why you want an explicit int64_t here.
> 
>>          return false;
>>      }
>>  
>> diff --git a/include/qemu/throttle.h b/include/qemu/throttle.h
>> index 12faaad..d0c98ed 100644
>> --- a/include/qemu/throttle.h
>> +++ b/include/qemu/throttle.h
>> @@ -29,6 +29,8 @@
>>  #include "qemu-common.h"
>>  #include "qemu/timer.h"
>>  
>> +#define THROTTLE_VALUE_MAX 1000000000000000LL
> 
> <pedantic>
> But then you could use UINT64_C(1000000000000000) here.
> </pedantic>
>
Fam Zheng Jan. 15, 2016, 2:06 a.m. UTC | #3 
On Thu, 01/14 16:50, Max Reitz wrote:
> On 14.01.2016 16:46, Max Reitz wrote:
> > On 14.01.2016 05:08, Fam Zheng wrote:
> >> The implicit casting from unsigned int to double changes negative values
> >> into large positive numbers and accepts them.  We should instead print
> >> an error.
> >>
> >> Check the number range so this case is caught and reported.
> >>
> >> Signed-off-by: Fam Zheng <famz@redhat.com>
> >> ---
> >>  blockdev.c              |  3 ++-
> >>  include/qemu/throttle.h |  2 ++
> >>  util/throttle.c         | 16 ++++++----------
> >>  3 files changed, 10 insertions(+), 11 deletions(-)
> > 
> > Reviewed-by: Max Reitz <mreitz@redhat.com>
> > 
> >> diff --git a/blockdev.c b/blockdev.c
> >> index 2df0c6d..1afef87 100644
> >> --- a/blockdev.c
> >> +++ b/blockdev.c
> >> @@ -348,7 +348,8 @@ static bool check_throttle_config(ThrottleConfig *cfg, Error **errp)
> >>      }
> >>  
> >>      if (!throttle_is_valid(cfg)) {
> >> -        error_setg(errp, "bps/iops/maxs values must be 0 or greater");
> >> +        error_setg(errp, "bps/iops/maxs values must be within [0, %" PRId64
> 
> Pre-existing, but you might want to fix this to "bps/iops/max" while
> touching this line.

Makes sense, I'll apply your rev-by and fix this in v4.

Fam

> 
> Max
> 
> >> +                         ")", (int64_t)THROTTLE_VALUE_MAX);
> > 
> > I personally would have liked a simpler %lli and no cast, but I can see
> > why you want an explicit int64_t here.
> > 
> >>          return false;
> >>      }
> >>  
> >> diff --git a/include/qemu/throttle.h b/include/qemu/throttle.h
> >> index 12faaad..d0c98ed 100644
> >> --- a/include/qemu/throttle.h
> >> +++ b/include/qemu/throttle.h
> >> @@ -29,6 +29,8 @@
> >>  #include "qemu-common.h"
> >>  #include "qemu/timer.h"
> >>  
> >> +#define THROTTLE_VALUE_MAX 1000000000000000LL
> > 
> > <pedantic>
> > But then you could use UINT64_C(1000000000000000) here.
> > </pedantic>
> > 
> 
>
diff mbox

Patch

diff --git a/blockdev.c b/blockdev.c
index 2df0c6d..1afef87 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -348,7 +348,8 @@  static bool check_throttle_config(ThrottleConfig *cfg, Error **errp)
     }
 
     if (!throttle_is_valid(cfg)) {
-        error_setg(errp, "bps/iops/maxs values must be 0 or greater");
+        error_setg(errp, "bps/iops/maxs values must be within [0, %" PRId64
+                         ")", (int64_t)THROTTLE_VALUE_MAX);
         return false;
     }
 
diff --git a/include/qemu/throttle.h b/include/qemu/throttle.h
index 12faaad..d0c98ed 100644
--- a/include/qemu/throttle.h
+++ b/include/qemu/throttle.h
@@ -29,6 +29,8 @@ 
 #include "qemu-common.h"
 #include "qemu/timer.h"
 
+#define THROTTLE_VALUE_MAX 1000000000000000LL
+
 typedef enum {
     THROTTLE_BPS_TOTAL,
     THROTTLE_BPS_READ,
diff --git a/util/throttle.c b/util/throttle.c
index 1113671..af4bc95 100644
--- a/util/throttle.c
+++ b/util/throttle.c
@@ -282,22 +282,18 @@  bool throttle_conflicting(ThrottleConfig *cfg)
  */
 bool throttle_is_valid(ThrottleConfig *cfg)
 {
-    bool invalid = false;
     int i;
 
     for (i = 0; i < BUCKETS_COUNT; i++) {
-        if (cfg->buckets[i].avg < 0) {
-            invalid = true;
+        if (cfg->buckets[i].avg < 0 ||
+            cfg->buckets[i].max < 0 ||
+            cfg->buckets[i].avg > THROTTLE_VALUE_MAX ||
+            cfg->buckets[i].max > THROTTLE_VALUE_MAX) {
+            return false;
         }
     }
 
-    for (i = 0; i < BUCKETS_COUNT; i++) {
-        if (cfg->buckets[i].max < 0) {
-            invalid = true;
-        }
-    }
-
-    return !invalid;
+    return true;
 }
 
 /* check if bps_max/iops_max is used without bps/iops