From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59185) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X86Ko-0008Bn-Lz for qemu-devel@nongnu.org; Fri, 18 Jul 2014 07:28:26 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1X86Kh-0006Qo-76 for qemu-devel@nongnu.org; Fri, 18 Jul 2014 07:28:18 -0400 Received: from mx1.redhat.com ([209.132.183.28]:25798) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X86Kg-0006QT-W3 for qemu-devel@nongnu.org; Fri, 18 Jul 2014 07:28:11 -0400 Date: Fri, 18 Jul 2014 16:57:30 +0530 From: Amit Shah Message-ID: <20140718112730.GA26614@grmbl.mre> References: <1405630053-15052-1-git-send-email-jsnow@redhat.com> <87zjg7yyds.fsf@blackfin.pond.sub.org> <20140718074623.GC6960@grmbl.mre> <87a986vry1.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <87a986vry1.fsf@blackfin.pond.sub.org> Subject: Re: [Qemu-devel] [PATCH v2] virtio-rng: Add human-readable error message for negative max-bytes parameter List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: peter.maydell@linaro.org, John Snow , qemu-devel@nongnu.org On (Fri) 18 Jul 2014 [13:15:18], Markus Armbruster wrote: > Amit Shah writes: > > > On (Fri) 18 Jul 2014 [08:27:59], Markus Armbruster wrote: > >> John Snow writes: > >> > >> > If a negative integer is used for the max_bytes parameter, QEMU currently > >> > calls abort() and leaves behind a core dump. This patch adds a simple > >> > error message to make the reason for the termination clearer. > >> > > >> > Signed-off-by: John Snow > >> > --- > >> > v2: Changed 0L constant to (uint64_t)0 constant to match PRId64 format code > >> > on both 32bit and 64bit systems. Tested via -m32 flag. > >> > > >> > hw/virtio/virtio-rng.c | 6 +++++- > >> > 1 file changed, 5 insertions(+), 1 deletion(-) > >> > > >> > diff --git a/hw/virtio/virtio-rng.c b/hw/virtio/virtio-rng.c > >> > index 1356aca..64c7d23 100644 > >> > --- a/hw/virtio/virtio-rng.c > >> > +++ b/hw/virtio/virtio-rng.c > >> > @@ -181,7 +181,11 @@ static void virtio_rng_device_realize(DeviceState *dev, Error **errp) > >> > > >> > vrng->vq = virtio_add_queue(vdev, 8, handle_input); > >> > > >> > - assert(vrng->conf.max_bytes <= INT64_MAX); > >> > + if (vrng->conf.max_bytes > INT64_MAX) { > >> > + error_set(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE, "virtio-rng", > >> > + "max_bytes", vrng->conf.max_bytes, (uint64_t)0, INT64_MAX); > >> > + return; > >> > + } > >> > vrng->quota_remaining = vrng->conf.max_bytes; > >> > > >> > vrng->rate_limit_timer = timer_new_ms(QEMU_CLOCK_VIRTUAL, > >> > >> Elsewhere in this function, we use > >> > >> error_set(errp, QERR_INVALID_PARAMETER_VALUE, "period", > >> "a positive number"); > >> > >> Existing uses of QERR_PROPERTY_VALUE_OUT_OF_RANGE are all for intervals > >> with small bounds. > > > > That's suggestion for a 2.2 patch, right? > > This *is* a 2.2 patch, isn't it? This one I proposed for 2.1 (because a device hotplug could cause qemu to abort). > > Do you think the usage as in this patch is fine? > > It's not wrong, just inconsistent with existing usage. I'd prefer > consistency. Right. Which one do you prefer -- both using QERR_INVALID_PARAMETER_VALUE, or QERR_PROPERTY_VALUE_OUT_OF_RANGE? I prefer the latter. Amit