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

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

[Fam Zheng]

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

[Qemu-devel] [PATCH 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, 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

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

[Fam Zheng]

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

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

[Alberto Garcia]

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

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

[Fam Zheng]

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