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

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

[Fam Zheng]

v4: Add Max's rev-by in both patches, while fixing the "maxs" typo.

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 v4 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>
Reviewed-by: Max Reitz <mreitz@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..b925e5d 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, %" 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

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

[Markus Armbruster]

Fam Zheng <famz@redhat.com> writes:

> The implicit casting from unsigned int to double changes negative values
> into large positive numbers and accepts them.  We should instead print
> an error.

--verbose:

* extract_common_blockdev_options() uses qemu_opt_get_number() to parse
  the number 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.  Numbers out of range get clipped to ULLONG_MAX.

* qmp_block_set_io_throttle() uses QMP core to parse the JSON number to
  int64_t.  The actual parsing is done by stroll() in parse_literal().
  Numbers out of range get parsed as double instead.  Since the QAPI
  schema asks for 'int', this is a type error.

Correct?

Since the actual configuration value is a double, I wonder why we don't
just parse a double and be done with it.

> Check the number range so this case is caught and reported.

I think you should mention the patch restricts the valid range to
0..1e15.  Without that, the commit message kind of suggests it's
0..INT64_MAX.

> Signed-off-by: Fam Zheng <famz@redhat.com>
> Reviewed-by: Max Reitz <mreitz@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..b925e5d 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, %" PRId64
> +                         ")", (int64_t)THROTTLE_VALUE_MAX);

What's wrong with %lld and no cast?  T

>          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

The range gets checked after conversion to double, which is fine since
1e15 is exactly representable in double.

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

[Kevin Wolf]

Am 15.01.2016 um 03:09 hat Fam Zheng geschrieben:
> 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>
> Reviewed-by: Max Reitz <mreitz@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..b925e5d 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, %" PRId64
> +                         ")", (int64_t)THROTTLE_VALUE_MAX);

I think that should be "]". If you agree, I'll fix it up while applying.

>          return false;
>      }

Kevin

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

[Fam Zheng]

On Fri, 01/15 15:28, Kevin Wolf wrote:
> Am 15.01.2016 um 03:09 hat Fam Zheng geschrieben:
> > 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>
> > Reviewed-by: Max Reitz <mreitz@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..b925e5d 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, %" PRId64
> > +                         ")", (int64_t)THROTTLE_VALUE_MAX);
> 
> I think that should be "]". If you agree, I'll fix it up while applying.

Yes, that's right. Thanks.

Fam

> 
> >          return false;
> >      }
> 
> Kevin
> 

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

[Alberto Garcia]

On Mon 18 Jan 2016 02:09:22 AM CET, Fam Zheng <famz@redhat.com> wrote:
>> > -        error_setg(errp, "bps/iops/maxs values must be 0 or greater");
>> > +        error_setg(errp, "bps/iops/max values must be within [0, %" PRId64
>> > +                         ")", (int64_t)THROTTLE_VALUE_MAX);
>> 
>> I think that should be "]". If you agree, I'll fix it up while
>> applying.
>
> Yes, that's right. Thanks.

That should also be fixed in the iotest.

Berto

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

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Max Reitz <mreitz@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] [PATCH v4 2/2] iotests: Test that negative and large throttle values are rejected

[Alberto Garcia]

On Fri 15 Jan 2016 03:09:23 AM CET, Fam Zheng wrote:
>  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

Now that you're at it, you could also check that 0 and 1000000000000000
_do_ work.

Either way,

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

Berto

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

[Markus Armbruster]

Fam Zheng <famz@redhat.com> writes:

> v4: Add Max's rev-by in both patches, while fixing the "maxs" typo.
>
> 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).

I think PATCH 1's commit message could explain the problem in a bit more
detail, and it should mention the changed valid range.

Other than that, I had two questions: why cast THROTTLE_VALUE_MAX for
printing (in scope for the series), and why parse the settings as
integers even though they're really floating-point (probably not in
scope).

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

[Fam Zheng]

On Tue, 01/19 10:50, Markus Armbruster wrote:
> Fam Zheng <famz@redhat.com> writes:
> 
> > v4: Add Max's rev-by in both patches, while fixing the "maxs" typo.
> >
> > 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).
> 
> I think PATCH 1's commit message could explain the problem in a bit more
> detail, and it should mention the changed valid range.

OK, I'll update the commit message.

> 
> Other than that, I had two questions: why cast THROTTLE_VALUE_MAX for
> printing (in scope for the series),

Not quite intentionally. I started with "L" suffix and thought definitely 64
bit is safer than "%ld" for 32 bit machines, without realizing "L" suffix is
not safe for old compilers. Then it became "LL" and int64_t casting.

I can use "%lld" in v5 while fixing the commit message and covering the valid
range in iotests.

> and why parse the settings as
> integers even though they're really floating-point (probably not in
> scope).

I don't know if it's worth to extend the option interface with floating-point.
If it's for this case I'd say no, because using floating-point in the code is
more for the computation, rather than the precision we support on the
parameters (I might be wrong).

Fam