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

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

* [Qemu-devel] [PATCH 0/2] block: Reject negative values for throttling options
@ 2016-01-11  5:42 Fam Zheng
  2016-01-11  5:42 ` [Qemu-devel] [PATCH 1/2] blockdev: Error out on negative throttling option values Fam Zheng
  2016-01-11  5:42 ` [Qemu-devel] [PATCH 2/2] iotests: Test that negative throttle values are rejected Fam Zheng
  0 siblings, 2 replies; 5+ messages in thread
From: Fam Zheng @ 2016-01-11  5:42 UTC (permalink / raw)
  To: qemu-devel; +Cc: Kevin Wolf, Markus Armbruster, qemu-block

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 throttle values are rejected

 blockdev.c                    | 26 +++++++++++++-------------
 tests/qemu-iotests/051        | 11 +++++++++++
 tests/qemu-iotests/051.out    | 21 +++++++++++++++++++++
 tests/qemu-iotests/051.pc.out | 21 +++++++++++++++++++++
 4 files changed, 66 insertions(+), 13 deletions(-)

-- 
2.4.3

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

* [Qemu-devel] [PATCH 1/2] blockdev: Error out on negative throttling option values
  2016-01-11  5:42 [Qemu-devel] [PATCH 0/2] block: Reject negative values for throttling options Fam Zheng
@ 2016-01-11  5:42 ` Fam Zheng
  2016-01-12 15:00   ` [Qemu-devel] [Qemu-block] " Alberto Garcia
  2016-01-11  5:42 ` [Qemu-devel] [PATCH 2/2] iotests: Test that negative throttle values are rejected Fam Zheng
  1 sibling, 1 reply; 5+ messages in thread
From: Fam Zheng @ 2016-01-11  5:42 UTC (permalink / raw)
  To: qemu-devel; +Cc: Kevin Wolf, Markus Armbruster, qemu-block

The implicit casting from unsigned int to double changes negative values
into large positive numbers, whereas explicitly casting to signed
integer first will let us catch the invalid value and report error
correctly:

    $ qemu-system-x86_64 -drive file=null-co://,iops=-1
    qemu-system-x86_64: -drive file=null-co://,iops=-1: bps/iops/maxs
    values must be 0 or greater

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 blockdev.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 2df0c6d..8b6b398 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -407,33 +407,33 @@ static void extract_common_blockdev_options(QemuOpts *opts, int *bdrv_flags,
     if (throttle_cfg) {
         memset(throttle_cfg, 0, sizeof(*throttle_cfg));
         throttle_cfg->buckets[THROTTLE_BPS_TOTAL].avg =
-            qemu_opt_get_number(opts, "throttling.bps-total", 0);
+            (int64_t)qemu_opt_get_number(opts, "throttling.bps-total", 0);
         throttle_cfg->buckets[THROTTLE_BPS_READ].avg  =
-            qemu_opt_get_number(opts, "throttling.bps-read", 0);
+            (int64_t)qemu_opt_get_number(opts, "throttling.bps-read", 0);
         throttle_cfg->buckets[THROTTLE_BPS_WRITE].avg =
-            qemu_opt_get_number(opts, "throttling.bps-write", 0);
+            (int64_t)qemu_opt_get_number(opts, "throttling.bps-write", 0);
         throttle_cfg->buckets[THROTTLE_OPS_TOTAL].avg =
-            qemu_opt_get_number(opts, "throttling.iops-total", 0);
+            (int64_t)qemu_opt_get_number(opts, "throttling.iops-total", 0);
         throttle_cfg->buckets[THROTTLE_OPS_READ].avg =
-            qemu_opt_get_number(opts, "throttling.iops-read", 0);
+            (int64_t)qemu_opt_get_number(opts, "throttling.iops-read", 0);
         throttle_cfg->buckets[THROTTLE_OPS_WRITE].avg =
-            qemu_opt_get_number(opts, "throttling.iops-write", 0);
+            (int64_t)qemu_opt_get_number(opts, "throttling.iops-write", 0);
 
         throttle_cfg->buckets[THROTTLE_BPS_TOTAL].max =
-            qemu_opt_get_number(opts, "throttling.bps-total-max", 0);
+            (int64_t)qemu_opt_get_number(opts, "throttling.bps-total-max", 0);
         throttle_cfg->buckets[THROTTLE_BPS_READ].max  =
-            qemu_opt_get_number(opts, "throttling.bps-read-max", 0);
+            (int64_t)qemu_opt_get_number(opts, "throttling.bps-read-max", 0);
         throttle_cfg->buckets[THROTTLE_BPS_WRITE].max =
-            qemu_opt_get_number(opts, "throttling.bps-write-max", 0);
+            (int64_t)qemu_opt_get_number(opts, "throttling.bps-write-max", 0);
         throttle_cfg->buckets[THROTTLE_OPS_TOTAL].max =
-            qemu_opt_get_number(opts, "throttling.iops-total-max", 0);
+            (int64_t)qemu_opt_get_number(opts, "throttling.iops-total-max", 0);
         throttle_cfg->buckets[THROTTLE_OPS_READ].max =
-            qemu_opt_get_number(opts, "throttling.iops-read-max", 0);
+            (int64_t)qemu_opt_get_number(opts, "throttling.iops-read-max", 0);
         throttle_cfg->buckets[THROTTLE_OPS_WRITE].max =
-            qemu_opt_get_number(opts, "throttling.iops-write-max", 0);
+            (int64_t)qemu_opt_get_number(opts, "throttling.iops-write-max", 0);
 
         throttle_cfg->op_size =
-            qemu_opt_get_number(opts, "throttling.iops-size", 0);
+            (int64_t)qemu_opt_get_number(opts, "throttling.iops-size", 0);
 
         if (!check_throttle_config(throttle_cfg, errp)) {
             return;
-- 
2.4.3

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

* Re: [Qemu-devel] [Qemu-block] [PATCH 1/2] blockdev: Error out on negative throttling option values
  2016-01-11  5:42 ` [Qemu-devel] [PATCH 1/2] blockdev: Error out on negative throttling option values Fam Zheng
@ 2016-01-12 15:00   ` Alberto Garcia
  2016-01-13  0:29     ` Fam Zheng
  0 siblings, 1 reply; 5+ messages in thread
From: Alberto Garcia @ 2016-01-12 15:00 UTC (permalink / raw)
  To: Fam Zheng, qemu-devel; +Cc: Kevin Wolf, Markus Armbruster, qemu-block

On Mon 11 Jan 2016 06:42:38 AM CET, Fam Zheng <famz@redhat.com> wrote:

> The implicit casting from unsigned int to double changes negative
> values into large positive numbers, whereas explicitly casting to
> signed integer first will let us catch the invalid value and report
> error correctly:
>
>     $ qemu-system-x86_64 -drive file=null-co://,iops=-1
>     qemu-system-x86_64: -drive file=null-co://,iops=-1: bps/iops/maxs
>     values must be 0 or greater
>

>          throttle_cfg->buckets[THROTTLE_BPS_TOTAL].avg =
> -            qemu_opt_get_number(opts, "throttling.bps-total", 0);
> +            (int64_t)qemu_opt_get_number(opts, "throttling.bps-total", 0);

It seems to me that the problem is that qemu_opt_get_number() returns a
value different from the one specified in the comand-line.

How do we even tell the difference between a negative number and its
bit-to-bit positive equivalent?

If we are going to reject very large numbers I would rather check that
the throtting values are within a sane range and throw an error
otherwise.

Berto

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

* Re: [Qemu-devel] [Qemu-block] [PATCH 1/2] blockdev: Error out on negative throttling option values
  2016-01-12 15:00   ` [Qemu-devel] [Qemu-block] " Alberto Garcia
@ 2016-01-13  0:29     ` Fam Zheng
  0 siblings, 0 replies; 5+ messages in thread
From: Fam Zheng @ 2016-01-13  0:29 UTC (permalink / raw)
  To: Alberto Garcia; +Cc: Kevin Wolf, qemu-devel, qemu-block, Markus Armbruster

On Tue, 01/12 16:00, Alberto Garcia wrote:
> On Mon 11 Jan 2016 06:42:38 AM CET, Fam Zheng <famz@redhat.com> wrote:
> 
> > The implicit casting from unsigned int to double changes negative
> > values into large positive numbers, whereas explicitly casting to
> > signed integer first will let us catch the invalid value and report
> > error correctly:
> >
> >     $ qemu-system-x86_64 -drive file=null-co://,iops=-1
> >     qemu-system-x86_64: -drive file=null-co://,iops=-1: bps/iops/maxs
> >     values must be 0 or greater
> >
> 
> >          throttle_cfg->buckets[THROTTLE_BPS_TOTAL].avg =
> > -            qemu_opt_get_number(opts, "throttling.bps-total", 0);
> > +            (int64_t)qemu_opt_get_number(opts, "throttling.bps-total", 0);
> 
> It seems to me that the problem is that qemu_opt_get_number() returns a
> value different from the one specified in the comand-line.
> 
> How do we even tell the difference between a negative number and its
> bit-to-bit positive equivalent?

We can't. :(

> 
> If we are going to reject very large numbers I would rather check that
> the throtting values are within a sane range and throw an error
> otherwise.

Yes, that is probably more accurate to the user.

Fam

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

* [Qemu-devel] [PATCH 2/2] iotests: Test that negative throttle values are rejected
  2016-01-11  5:42 [Qemu-devel] [PATCH 0/2] block: Reject negative values for throttling options Fam Zheng
  2016-01-11  5:42 ` [Qemu-devel] [PATCH 1/2] blockdev: Error out on negative throttling option values Fam Zheng
@ 2016-01-11  5:42 ` Fam Zheng
  1 sibling, 0 replies; 5+ messages in thread
From: Fam Zheng @ 2016-01-11  5:42 UTC (permalink / raw)
  To: qemu-devel; +Cc: Kevin Wolf, Markus Armbruster, qemu-block

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

diff --git a/tests/qemu-iotests/051 b/tests/qemu-iotests/051
index d91f80b..2c9b07e 100755
--- a/tests/qemu-iotests/051
+++ b/tests/qemu-iotests/051
@@ -263,6 +263,17 @@ 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 nagative 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
+
+echo
 echo === Parsing protocol from file name ===
 echo
 
diff --git a/tests/qemu-iotests/051.out b/tests/qemu-iotests/051.out
index bf886ce..a8c0679 100644
--- a/tests/qemu-iotests/051.out
+++ b/tests/qemu-iotests/051.out
@@ -285,6 +285,27 @@ 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 nagative 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 0 or greater
+
+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 0 or greater
+
+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 0 or greater
+
+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 0 or greater
+
+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 0 or greater
+
+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 0 or greater
+
+
 === 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..b455411 100644
--- a/tests/qemu-iotests/051.pc.out
+++ b/tests/qemu-iotests/051.pc.out
@@ -379,6 +379,27 @@ 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 nagative 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 0 or greater
+
+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 0 or greater
+
+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 0 or greater
+
+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 0 or greater
+
+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 0 or greater
+
+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 0 or greater
+
+
 === Parsing protocol from file name ===
 
 Testing: -hda foo:bar
-- 
2.4.3

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

end of thread, other threads:[~2016-01-13  0:29 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-01-11  5:42 [Qemu-devel] [PATCH 0/2] block: Reject negative values for throttling options Fam Zheng
2016-01-11  5:42 ` [Qemu-devel] [PATCH 1/2] blockdev: Error out on negative throttling option values Fam Zheng
2016-01-12 15:00   ` [Qemu-devel] [Qemu-block] " Alberto Garcia
2016-01-13  0:29     ` Fam Zheng
2016-01-11  5:42 ` [Qemu-devel] [PATCH 2/2] iotests: Test that negative throttle values are rejected Fam Zheng

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