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

[Markus Armbruster]

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

[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