[Qemu-devel] [PATCH v3 0/2] block: Reject negative values for throttling options

[Qemu-devel] [PATCH v3 0/2] block: Reject negative values for throttling options

[Fam Zheng]

v3: Address comments:
    - Add test for large value; [Berto]
    - Fix typos "negative" & "caught"; [Eric, Berto]
    - Use "LL" suffix to the upper limit constant. [Berto]

v2: Check the value range and report an appropriate error. [Berto]

Now the negative values are silently converted to a huge positive number
because we are doing implicit casting from uint64_t to double. Fix it and add a
test case (this was once fixed in 7d81c1413c9 but regressed when the block
device option parsing code was changed).


Fam Zheng (2):
  blockdev: Error out on negative throttling option values
  iotests: Test that negative and large throttle values are rejected

 blockdev.c                    |  3 ++-
 include/qemu/throttle.h       |  2 ++
 tests/qemu-iotests/051        | 12 ++++++++++++
 tests/qemu-iotests/051.out    | 24 ++++++++++++++++++++++++
 tests/qemu-iotests/051.pc.out | 24 ++++++++++++++++++++++++
 util/throttle.c               | 16 ++++++----------
 6 files changed, 70 insertions(+), 11 deletions(-)

-- 
2.4.3

[Qemu-devel] [PATCH v3 1/2] blockdev: Error out on negative throttling option values

[Fam Zheng]

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(-)

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

[Qemu-devel] [PATCH v3 2/2] iotests: Test that negative and large throttle values are rejected

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 tests/qemu-iotests/051        | 12 ++++++++++++
 tests/qemu-iotests/051.out    | 24 ++++++++++++++++++++++++
 tests/qemu-iotests/051.pc.out | 24 ++++++++++++++++++++++++
 3 files changed, 60 insertions(+)

diff --git a/tests/qemu-iotests/051 b/tests/qemu-iotests/051
index d91f80b..cdf72d4 100755
--- a/tests/qemu-iotests/051
+++ b/tests/qemu-iotests/051
@@ -263,6 +263,18 @@ run_qemu -drive file="$TEST_IMG",iops_size=1234,throttling.iops-size=5678
 run_qemu -drive file="$TEST_IMG",readonly=on,read-only=off
 
 echo
+echo === Catching negative/large throttling values ===
+echo
+
+run_qemu -drive file="$TEST_IMG",iops=-1
+run_qemu -drive file="$TEST_IMG",bps=-2
+run_qemu -drive file="$TEST_IMG",bps_rd=-3
+run_qemu -drive file="$TEST_IMG",bps_rd_max=-3
+run_qemu -drive file="$TEST_IMG",throttling.iops-total=-4
+run_qemu -drive file="$TEST_IMG",throttling.bps-total=-5
+run_qemu -drive file="$TEST_IMG",bps=1000000000000001
+
+echo
 echo === Parsing protocol from file name ===
 echo
 
diff --git a/tests/qemu-iotests/051.out b/tests/qemu-iotests/051.out
index bf886ce..efbb39c 100644
--- a/tests/qemu-iotests/051.out
+++ b/tests/qemu-iotests/051.out
@@ -285,6 +285,30 @@ Testing: -drive file=TEST_DIR/t.qcow2,readonly=on,read-only=off
 QEMU_PROG: -drive file=TEST_DIR/t.qcow2,readonly=on,read-only=off: 'read-only' and its alias 'readonly' can't be used at the same time
 
 
+=== Catching negative/large throttling values ===
+
+Testing: -drive file=TEST_DIR/t.qcow2,iops=-1
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,iops=-1: bps/iops/maxs values must be within [0, 1000000000000000)
+
+Testing: -drive file=TEST_DIR/t.qcow2,bps=-2
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,bps=-2: bps/iops/maxs values must be within [0, 1000000000000000)
+
+Testing: -drive file=TEST_DIR/t.qcow2,bps_rd=-3
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,bps_rd=-3: bps/iops/maxs values must be within [0, 1000000000000000)
+
+Testing: -drive file=TEST_DIR/t.qcow2,bps_rd_max=-3
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,bps_rd_max=-3: bps/iops/maxs values must be within [0, 1000000000000000)
+
+Testing: -drive file=TEST_DIR/t.qcow2,throttling.iops-total=-4
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,throttling.iops-total=-4: bps/iops/maxs values must be within [0, 1000000000000000)
+
+Testing: -drive file=TEST_DIR/t.qcow2,throttling.bps-total=-5
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,throttling.bps-total=-5: bps/iops/maxs values must be within [0, 1000000000000000)
+
+Testing: -drive file=TEST_DIR/t.qcow2,bps=1000000000000001
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,bps=1000000000000001: bps/iops/maxs values must be within [0, 1000000000000000)
+
+
 === Parsing protocol from file name ===
 
 Testing: -hda foo:bar
diff --git a/tests/qemu-iotests/051.pc.out b/tests/qemu-iotests/051.pc.out
index a5dfc33..0cb1506 100644
--- a/tests/qemu-iotests/051.pc.out
+++ b/tests/qemu-iotests/051.pc.out
@@ -379,6 +379,30 @@ Testing: -drive file=TEST_DIR/t.qcow2,readonly=on,read-only=off
 QEMU_PROG: -drive file=TEST_DIR/t.qcow2,readonly=on,read-only=off: 'read-only' and its alias 'readonly' can't be used at the same time
 
 
+=== Catching negative/large throttling values ===
+
+Testing: -drive file=TEST_DIR/t.qcow2,iops=-1
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,iops=-1: bps/iops/maxs values must be within [0, 1000000000000000)
+
+Testing: -drive file=TEST_DIR/t.qcow2,bps=-2
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,bps=-2: bps/iops/maxs values must be within [0, 1000000000000000)
+
+Testing: -drive file=TEST_DIR/t.qcow2,bps_rd=-3
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,bps_rd=-3: bps/iops/maxs values must be within [0, 1000000000000000)
+
+Testing: -drive file=TEST_DIR/t.qcow2,bps_rd_max=-3
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,bps_rd_max=-3: bps/iops/maxs values must be within [0, 1000000000000000)
+
+Testing: -drive file=TEST_DIR/t.qcow2,throttling.iops-total=-4
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,throttling.iops-total=-4: bps/iops/maxs values must be within [0, 1000000000000000)
+
+Testing: -drive file=TEST_DIR/t.qcow2,throttling.bps-total=-5
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,throttling.bps-total=-5: bps/iops/maxs values must be within [0, 1000000000000000)
+
+Testing: -drive file=TEST_DIR/t.qcow2,bps=1000000000000001
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,bps=1000000000000001: bps/iops/maxs values must be within [0, 1000000000000000)
+
+
 === Parsing protocol from file name ===
 
 Testing: -hda foo:bar
-- 
2.4.3

Re: [Qemu-devel] [Qemu-block] [PATCH v3 1/2] blockdev: Error out on negative throttling option values

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1572 bytes --]

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>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH v3 1/2] blockdev: Error out on negative throttling option values

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1776 bytes --]

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



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH v3 2/2] iotests: Test that negative and large throttle values are rejected

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 358 bytes --]

On 14.01.2016 05:08, Fam Zheng wrote:
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  tests/qemu-iotests/051        | 12 ++++++++++++
>  tests/qemu-iotests/051.out    | 24 ++++++++++++++++++++++++
>  tests/qemu-iotests/051.pc.out | 24 ++++++++++++++++++++++++
>  3 files changed, 60 insertions(+)

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH v3 1/2] blockdev: Error out on negative throttling option values

[Fam Zheng]

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