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

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 
> > Reviewed-by: Max Reitz 
> > ---
> >  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

[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 
> Reviewed-by: Max Reitz 
> ---
>  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

[Markus Armbruster]

Fam Zheng  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 
> Reviewed-by: Max Reitz 
> ---
>  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 1000LL
> +
>  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.

[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 
Reviewed-by: Max Reitz 
---
 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 1000LL
+
 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