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

qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed

* [Qemu-devel] [PATCH v5 0/2] block: Reject negative values for throttling options
@ 2016-01-20  4:21 Fam Zheng
  2016-01-20  4:21 ` [Qemu-devel] [PATCH v5 1/2] blockdev: Error out on negative throttling option values Fam Zheng
                   ` (4 more replies)
  0 siblings, 5 replies; 7+ messages in thread
From: Fam Zheng @ 2016-01-20  4:21 UTC (permalink / raw)
  To: qemu-devel; +Cc: Kevin Wolf, berto, qemu-block, Markus Armbruster, mreitz

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [Qemu-devel] [PATCH v5 1/2] blockdev: Error out on negative throttling option values
  2016-01-20  4:21 [Qemu-devel] [PATCH v5 0/2] block: Reject negative values for throttling options Fam Zheng
@ 2016-01-20  4:21 ` Fam Zheng
  2016-01-20  4:21 ` [Qemu-devel] [PATCH v5 2/2] iotests: Test that throttle values ranges Fam Zheng
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 7+ messages in thread
From: Fam Zheng @ 2016-01-20  4:21 UTC (permalink / raw)
  To: qemu-devel; +Cc: Kevin Wolf, berto, qemu-block, Markus Armbruster, mreitz

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

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [Qemu-devel] [PATCH v5 2/2] iotests: Test that throttle values ranges
  2016-01-20  4:21 [Qemu-devel] [PATCH v5 0/2] block: Reject negative values for throttling options Fam Zheng
  2016-01-20  4:21 ` [Qemu-devel] [PATCH v5 1/2] blockdev: Error out on negative throttling option values Fam Zheng
@ 2016-01-20  4:21 ` Fam Zheng
  2016-01-20  7:25 ` [Qemu-devel] [PATCH v5 0/2] block: Reject negative values for throttling options Markus Armbruster
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 7+ messages in thread
From: Fam Zheng @ 2016-01-20  4:21 UTC (permalink / raw)
  To: qemu-devel; +Cc: Kevin Wolf, berto, qemu-block, Markus Armbruster, mreitz

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

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [Qemu-devel] [PATCH v5 0/2] block: Reject negative values for throttling options
  2016-01-20  4:21 [Qemu-devel] [PATCH v5 0/2] block: Reject negative values for throttling options Fam Zheng
  2016-01-20  4:21 ` [Qemu-devel] [PATCH v5 1/2] blockdev: Error out on negative throttling option values Fam Zheng
  2016-01-20  4:21 ` [Qemu-devel] [PATCH v5 2/2] iotests: Test that throttle values ranges Fam Zheng
@ 2016-01-20  7:25 ` Markus Armbruster
  2016-01-20  7:39   ` Fam Zheng
  2016-01-20  8:07 ` Alberto Garcia
  2016-01-20 12:38 ` Kevin Wolf
  4 siblings, 1 reply; 7+ messages in thread
From: Markus Armbruster @ 2016-01-20  7:25 UTC (permalink / raw)
  To: Fam Zheng; +Cc: Kevin Wolf, berto, qemu-devel, qemu-block, mreitz

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>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Qemu-devel] [PATCH v5 0/2] block: Reject negative values for throttling options
  2016-01-20  7:25 ` [Qemu-devel] [PATCH v5 0/2] block: Reject negative values for throttling options Markus Armbruster
@ 2016-01-20  7:39   ` Fam Zheng
  0 siblings, 0 replies; 7+ messages in thread
From: Fam Zheng @ 2016-01-20  7:39 UTC (permalink / raw)
  To: Markus Armbruster; +Cc: Kevin Wolf, berto, qemu-devel, qemu-block, mreitz

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Qemu-devel] [PATCH v5 0/2] block: Reject negative values for throttling options
  2016-01-20  4:21 [Qemu-devel] [PATCH v5 0/2] block: Reject negative values for throttling options Fam Zheng
                   ` (2 preceding siblings ...)
  2016-01-20  7:25 ` [Qemu-devel] [PATCH v5 0/2] block: Reject negative values for throttling options Markus Armbruster
@ 2016-01-20  8:07 ` Alberto Garcia
  2016-01-20 12:38 ` Kevin Wolf
  4 siblings, 0 replies; 7+ messages in thread
From: Alberto Garcia @ 2016-01-20  8:07 UTC (permalink / raw)
  To: Fam Zheng, qemu-devel; +Cc: Kevin Wolf, Markus Armbruster, qemu-block, mreitz

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Qemu-devel] [PATCH v5 0/2] block: Reject negative values for throttling options
  2016-01-20  4:21 [Qemu-devel] [PATCH v5 0/2] block: Reject negative values for throttling options Fam Zheng
                   ` (3 preceding siblings ...)
  2016-01-20  8:07 ` Alberto Garcia
@ 2016-01-20 12:38 ` Kevin Wolf
  4 siblings, 0 replies; 7+ messages in thread
From: Kevin Wolf @ 2016-01-20 12:38 UTC (permalink / raw)
  To: Fam Zheng; +Cc: berto, qemu-block, Markus Armbruster, qemu-devel, mreitz

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2016-01-20 12:38 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-01-20  4:21 [Qemu-devel] [PATCH v5 0/2] block: Reject negative values for throttling options Fam Zheng
2016-01-20  4:21 ` [Qemu-devel] [PATCH v5 1/2] blockdev: Error out on negative throttling option values Fam Zheng
2016-01-20  4:21 ` [Qemu-devel] [PATCH v5 2/2] iotests: Test that throttle values ranges Fam Zheng
2016-01-20  7:25 ` [Qemu-devel] [PATCH v5 0/2] block: Reject negative values for throttling options Markus Armbruster
2016-01-20  7:39   ` Fam Zheng
2016-01-20  8:07 ` Alberto Garcia
2016-01-20 12:38 ` Kevin Wolf

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).