On Tue, 01/19 10:50, Markus Armbruster wrote: > Fam Zheng <f...@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
