Re: [Qemu-devel] [PATCH v3] virtio-rng: Add human-readable error message for negative max-bytes parameter

[John Snow <jsnow@redhat.com>]

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