qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v3] virtio-rng: Add human-readable error message


From: John Snow
Subject: Re: [Qemu-devel] [PATCH v3] virtio-rng: Add human-readable error message for negative max-bytes parameter
Date: Tue, 22 Jul 2014 11:30:28 -0400
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.6.0


On 07/22/2014 07:48 AM, Amit Shah wrote:
On (Tue) 22 Jul 2014 [13:41:43], Markus Armbruster wrote:
Amit Shah <address@hidden> 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 <address@hidden>
---
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 :)



reply via email to

[Prev in Thread] Current Thread [Next in Thread]