From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34071) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X9YZl-00014y-3E for qemu-devel@nongnu.org; Tue, 22 Jul 2014 07:49:52 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1X9YZd-0000Fi-Jf for qemu-devel@nongnu.org; Tue, 22 Jul 2014 07:49:45 -0400 Received: from mx1.redhat.com ([209.132.183.28]:21022) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X9YZd-0000FZ-Ad for qemu-devel@nongnu.org; Tue, 22 Jul 2014 07:49:37 -0400 Date: Tue, 22 Jul 2014 17:18:50 +0530 From: Amit Shah Message-ID: <20140722114850.GE18209@grmbl.mre> References: <1405979077-18163-1-git-send-email-jsnow@redhat.com> <20140722111617.GD18209@grmbl.mre> <877g35vcw8.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <877g35vcw8.fsf@blackfin.pond.sub.org> Subject: Re: [Qemu-devel] [PATCH v3] 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 (Tue) 22 Jul 2014 [13:41:43], Markus Armbruster wrote: > Amit Shah writes: > > > On (Mon) 21 Jul 2014 [17:44:37], John Snow wrote: > >> 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. > >> > >> There is an underlying insufficiency in the parameter parsing code of QEMU > >> that renders it unable to reject negative values for unsigned properties, > >> thus the error message "a non-negative integer below 2^63" is the most > >> user-friendly and correct message we can give until the underlying > >> insufficiency is corrected. > >> > >> Signed-off-by: John Snow > >> --- > >> v3: Adjusted the error message to be more semantically meaningful, but > >> while acknowledging the limitations of the current unsigned integer > >> parsing routines. > >> > >> hw/virtio/virtio-rng.c | 8 +++++++- > >> 1 file changed, 7 insertions(+), 1 deletion(-) > >> > >> diff --git a/hw/virtio/virtio-rng.c b/hw/virtio/virtio-rng.c > >> index 1356aca..7c5a675 100644 > >> --- a/hw/virtio/virtio-rng.c > >> +++ b/hw/virtio/virtio-rng.c > >> @@ -181,7 +181,13 @@ 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); > >> + /* Workaround: Property parsing does not enforce unsigned integers, > >> + * So this is a hack to reject such numbers. */ > >> + if (vrng->conf.max_bytes > INT64_MAX) { > >> + error_set(errp, QERR_INVALID_PARAMETER_VALUE, "max-bytes", > >> + "a non-negative integer below 2^63"); > > > > Huh, why do we allow 0? There's no reason to have 0 as a max-bytes > > value as well... > > Could be treated as separate problem. Yep, don't mean to hold this up for that one. Thanks for the reviewed-by. Amit