From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58645) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X9c1Z-0004KY-DG for qemu-devel@nongnu.org; Tue, 22 Jul 2014 11:30:48 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1X9c1R-0005IC-I9 for qemu-devel@nongnu.org; Tue, 22 Jul 2014 11:30:41 -0400 Received: from mx1.redhat.com ([209.132.183.28]:1394) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X9c1R-0005I2-43 for qemu-devel@nongnu.org; Tue, 22 Jul 2014 11:30:33 -0400 Message-ID: <53CE8394.6070101@redhat.com> Date: Tue, 22 Jul 2014 11:30:28 -0400 From: John Snow MIME-Version: 1.0 References: <1405979077-18163-1-git-send-email-jsnow@redhat.com> <20140722111617.GD18209@grmbl.mre> <877g35vcw8.fsf@blackfin.pond.sub.org> <20140722114850.GE18209@grmbl.mre> In-Reply-To: <20140722114850.GE18209@grmbl.mre> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit 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: Amit Shah , Markus Armbruster Cc: peter.maydell@linaro.org, qemu-devel@nongnu.org On 07/22/2014 07:48 AM, Amit Shah wrote: > 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 Yes, 0 makes no sense, but there are a lot of extremely low values that cause problems. The current release allows you to input 0 so I left it as-is. The decision for what a reasonable minimum might be is perhaps up to the user, unless a better technical limit is found (like 1K? 2K? 4K?) We could also change parsing for this property to use the "size" attribute (instead of unsigned integers) to allow users to specify e.g, 4K/ms or 16K/ms and so on. It changes the nature of the sign problem for this property, though that problem for parsing in general still needs to be addressed in a future release. Thanks :)