From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44538) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aLivh-0006DT-BP for qemu-devel@nongnu.org; Tue, 19 Jan 2016 21:55:30 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aLivg-0000Ek-AQ for qemu-devel@nongnu.org; Tue, 19 Jan 2016 21:55:29 -0500 Date: Wed, 20 Jan 2016 10:55:18 +0800 From: Fam Zheng Message-ID: <20160120025518.GA3164@ad.usersys.redhat.com> References: <1452823763-4235-1-git-send-email-famz@redhat.com> <87a8o1hp8h.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <87a8o1hp8h.fsf@blackfin.pond.sub.org> Subject: Re: [Qemu-devel] [PATCH v4 0/2] block: Reject negative values for throttling options List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: Kevin Wolf , berto@igalia.com, qemu-devel@nongnu.org, qemu-block@nongnu.org, mreitz@redhat.com 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