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

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

[Fam Zheng]

v5: Add valid value testing in iotests. [Berto]
    Add details in commit message. [Markus]
    Use "%lld" in format string. [Markus]
    Fix ")" -> "]" in error message. [Kevin]

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 throttle values ranges

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

-- 
2.4.3

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

[Fam Zheng]

extract_common_blockdev_options() uses qemu_opt_get_number() to parse
the bps/iops numbers to uint64_t, then converts to double and stores in
ThrottleConfig.  The actual parsing is done by strtoull() in
parse_option_number().  Negative numbers are wrapped to large positive
ones, and stored.

We used to reject negative numbers since 7d81c1413c9, but this regressed
when the option parsing code was changed later. Now fix this again.

This time, define an arbitrary large upper limit (1e15),  and check the
values so both negative and impractically big numbers are 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 1392fff..07cfe25 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/max values must be within [0, %lld]",
+                   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 v5 2/2] iotests: Test that throttle values ranges

[Fam Zheng]

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

diff --git a/tests/qemu-iotests/051 b/tests/qemu-iotests/051
index d91f80b..7bfe9ff 100755
--- a/tests/qemu-iotests/051
+++ b/tests/qemu-iotests/051
@@ -263,6 +263,24 @@ 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
+# These are accepted
+run_qemu -drive file="$TEST_IMG",bps=0
+run_qemu -drive file="$TEST_IMG",bps=1
+run_qemu -drive file="$TEST_IMG",bps=1000000000000000
+# While these are not
+run_qemu -drive file="$TEST_IMG",bps=1000000000000001
+run_qemu -drive file="$TEST_IMG",bps=9999999999999999
+
+echo
 echo === Parsing protocol from file name ===
 echo
 
diff --git a/tests/qemu-iotests/051.out b/tests/qemu-iotests/051.out
index bf886ce..0f8a8d3 100644
--- a/tests/qemu-iotests/051.out
+++ b/tests/qemu-iotests/051.out
@@ -285,6 +285,45 @@ 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/max 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/max 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/max 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/max 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/max 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/max values must be within [0, 1000000000000000]
+
+Testing: -drive file=TEST_DIR/t.qcow2,bps=0
+QEMU X.Y.Z monitor - type 'help' for more information
+(qemu) q^[[K^[[Dqu^[[K^[[D^[[Dqui^[[K^[[D^[[D^[[Dquit^[[K
+
+Testing: -drive file=TEST_DIR/t.qcow2,bps=1
+QEMU X.Y.Z monitor - type 'help' for more information
+(qemu) q^[[K^[[Dqu^[[K^[[D^[[Dqui^[[K^[[D^[[D^[[Dquit^[[K
+
+Testing: -drive file=TEST_DIR/t.qcow2,bps=1000000000000000
+QEMU X.Y.Z monitor - type 'help' for more information
+(qemu) q^[[K^[[Dqu^[[K^[[D^[[Dqui^[[K^[[D^[[D^[[Dquit^[[K
+
+Testing: -drive file=TEST_DIR/t.qcow2,bps=1000000000000001
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,bps=1000000000000001: bps/iops/max values must be within [0, 1000000000000000]
+
+Testing: -drive file=TEST_DIR/t.qcow2,bps=9999999999999999
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,bps=9999999999999999: bps/iops/max 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..85fc05d 100644
--- a/tests/qemu-iotests/051.pc.out
+++ b/tests/qemu-iotests/051.pc.out
@@ -379,6 +379,45 @@ 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/max 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/max 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/max 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/max 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/max 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/max values must be within [0, 1000000000000000]
+
+Testing: -drive file=TEST_DIR/t.qcow2,bps=0
+QEMU X.Y.Z monitor - type 'help' for more information
+(qemu) q^[[K^[[Dqu^[[K^[[D^[[Dqui^[[K^[[D^[[D^[[Dquit^[[K
+
+Testing: -drive file=TEST_DIR/t.qcow2,bps=1
+QEMU X.Y.Z monitor - type 'help' for more information
+(qemu) q^[[K^[[Dqu^[[K^[[D^[[Dqui^[[K^[[D^[[D^[[Dquit^[[K
+
+Testing: -drive file=TEST_DIR/t.qcow2,bps=1000000000000000
+QEMU X.Y.Z monitor - type 'help' for more information
+(qemu) q^[[K^[[Dqu^[[K^[[D^[[Dqui^[[K^[[D^[[D^[[Dquit^[[K
+
+Testing: -drive file=TEST_DIR/t.qcow2,bps=1000000000000001
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,bps=1000000000000001: bps/iops/max values must be within [0, 1000000000000000]
+
+Testing: -drive file=TEST_DIR/t.qcow2,bps=9999999999999999
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,bps=9999999999999999: bps/iops/max values must be within [0, 1000000000000000]
+
+
 === Parsing protocol from file name ===
 
 Testing: -hda foo:bar
-- 
2.4.3

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

[Markus Armbruster]

Fam Zheng <famz@redhat.com> writes:

> v5: Add valid value testing in iotests. [Berto]
>     Add details in commit message. [Markus]
>     Use "%lld" in format string. [Markus]
>     Fix ")" -> "]" in error message. [Kevin]
>
> 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).

Series
Reviewed-by: Markus Armbruster <armbru@redhat.com>

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

[Fam Zheng]

On Wed, 01/20 08:25, Markus Armbruster wrote:
> Fam Zheng <famz@redhat.com> writes:
> 
> > v5: Add valid value testing in iotests. [Berto]
> >     Add details in commit message. [Markus]
> >     Use "%lld" in format string. [Markus]
> >     Fix ")" -> "]" in error message. [Kevin]
> >
> > 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).
> 
> Series
> Reviewed-by: Markus Armbruster <armbru@redhat.com>

Thanks!

Fam

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

[Alberto Garcia]

On Wed 20 Jan 2016 05:21:19 AM CET, Fam Zheng <famz@redhat.com> wrote:
> v5: Add valid value testing in iotests. [Berto]
>     Add details in commit message. [Markus]
>     Use "%lld" in format string. [Markus]
>     Fix ")" -> "]" in error message. [Kevin]
>
> 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).

Series
Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto

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

[Kevin Wolf]

Am 20.01.2016 um 05:21 hat Fam Zheng geschrieben:
> v5: Add valid value testing in iotests. [Berto]
>     Add details in commit message. [Markus]
>     Use "%lld" in format string. [Markus]
>     Fix ")" -> "]" in error message. [Kevin]
> 
> 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).

Thanks, applied to the block branch.

Kevin