diff mbox

[1/2] blockdev: Error out on negative throttling option values

Message ID 1452490959-10387-2-git-send-email-famz@redhat.com
State New
Headers show

Commit Message

Fam Zheng Jan. 11, 2016, 5:42 a.m. UTC 
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(-)

Comments

Alberto Garcia Jan. 12, 2016, 3 p.m. UTC | #1 
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
Fam Zheng Jan. 13, 2016, 12:29 a.m. UTC | #2 
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
diff mbox

Patch

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;