Re: [Qemu-devel] [PATCH v4 1/2] blockdev: Error out on negative throttling option values
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
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
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
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
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
