From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33989) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aJtmz-0002TB-89 for qemu-devel@nongnu.org; Thu, 14 Jan 2016 21:06:58 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aJtmy-0008I4-7k for qemu-devel@nongnu.org; Thu, 14 Jan 2016 21:06:57 -0500 Date: Fri, 15 Jan 2016 10:06:46 +0800 From: Fam Zheng Message-ID: <20160115020646.GA6161@ad.usersys.redhat.com> References: <1452744489-3513-1-git-send-email-famz@redhat.com> <1452744489-3513-2-git-send-email-famz@redhat.com> <5697C2E5.2010908@redhat.com> <5697C3C8.9080706@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <5697C3C8.9080706@redhat.com> Subject: Re: [Qemu-devel] [Qemu-block] [PATCH v3 1/2] blockdev: Error out on negative throttling option values List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Reitz Cc: Kevin Wolf , qemu-devel@nongnu.org, qemu-block@nongnu.org, Markus Armbruster On Thu, 01/14 16:50, Max Reitz wrote: > On 14.01.2016 16:46, Max Reitz wrote: > > On 14.01.2016 05:08, Fam Zheng wrote: > >> 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 > >> --- > >> blockdev.c | 3 ++- > >> include/qemu/throttle.h | 2 ++ > >> util/throttle.c | 16 ++++++---------- > >> 3 files changed, 10 insertions(+), 11 deletions(-) > > > > Reviewed-by: Max Reitz > > > >> diff --git a/blockdev.c b/blockdev.c > >> index 2df0c6d..1afef87 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/maxs values must be within [0, %" PRId64 > > Pre-existing, but you might want to fix this to "bps/iops/max" while > touching this line. Makes sense, I'll apply your rev-by and fix this in v4. Fam > > Max > > >> + ")", (int64_t)THROTTLE_VALUE_MAX); > > > > I personally would have liked a simpler %lli and no cast, but I can see > > why you want an explicit int64_t here. > > > >> 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 1000000000000000LL > > > > > > But then you could use UINT64_C(1000000000000000) here. > > > > > >