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

[John Snow <jsnow@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 5620 bytes --]


On 07/18/2014 09:16 AM, Markus Armbruster wrote:
> Amit Shah <amit.shah@redhat.com> writes:
>
>> On (Fri) 18 Jul 2014 [13:54:01], Markus Armbruster wrote:
>>> Amit Shah <amit.shah@redhat.com> writes:
>>>
>>>> On (Fri) 18 Jul 2014 [13:15:18], Markus Armbruster wrote:
>>>>> Amit Shah <amit.shah@redhat.com> writes:
>>>>>
>>>>>> On (Fri) 18 Jul 2014 [08:27:59], Markus Armbruster wrote:
>>>>>>> John Snow <jsnow@redhat.com> 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 <jsnow@redhat.com>
>>>>>>>> ---
>>>>>>>> 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);
>>> Missed this initially: the property name is "max-bytes", not
>>> "max_bytes".  Please fix.
>>>
>>>>>>>> +        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).
>>> Okay.
>>>
>>>>>> 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.
>>> I prefer
>>>
>>>      qemu: -device virtio-rng-pci,period=0: Parameter 'period'
>>> expects a positive number
>>>
>>> over
>>>
>>>      qemu: -device virtio-rng-pci,max-bytes=-1: Property
>>> virtio-rng.max_bytes doesn't take value -1 (minimum: 0, maximum:
>>> 9223372036854775807)
>>>
>>> because frankly, "maximum: 9223372036854775807", while precise, borders
>>> on gibberish.  Precise gibberish, I guess :)
>>>
>>> Taking a step back: the property is uint64_t.  Why isn't the upper bound
>>> simply UINT64_MAX?
>> OK - let's take this for 2.2 and get this answered.  The risk isn't
>> too great for the abort, since the user cannot innocently provide
>> negative values.
> Mekse sense.
>
>> John, can you look at Markus's comments and address them in a series?
> Yes, please.
>
> Understand that my opinion on the error messages is just an opinion :)
OK, so I took a peek at how this value comes into being and it's via 
set_uint64 --> visit_type_uint64 --> parse_type_int.

visit_type_uint32 has a boundary check where it makes sure that the 
value given to it is within its range, though it will still convert 
negatives "automatically" and depending on the negative given, it might 
pass this range check.
visit_type_uint64 by contrast cannot perform a check since everything is 
within range by definition.

Both of these calls ultimately rely on parse_type_int to do their dirty 
work, which they are configured to do so via string_input_visitor_new, 
where we create the visitor object which defines which parsing routines 
to use -- we only bother setting type_int. The visitor pattern in-use 
here does not currently allow for a "generic" unsigned version 
(type_uint), but it does offer us uint8, uint16, uint32 and uint64. It 
might be worth setting these fields to point to a new parse_type_uint 
helper that throws an error if it encounters a negative instead of 
deftly converting against our wishes. This would trickle up to /every 
property/ that uses unsigned types.

This way, there would never be any implicit conversion of negative 
integers to large, unsigned integers.

This would fix the semantic issue that Markus has pointed out wherein we 
requested an unsigned integer, but we may have already been passed a 
signed integer. Tightening the integer parsing /might/ help make parsing 
more semantically meaningful.

Another option might be to somehow leave a breadcrumb somewhere in the 
StringInputVisitor object that informs the parse_type_int/parse_str 
functions ultimately that we'd like to be strict about enforcing a "no 
sign modifiers" policy for this parsing. I am not well familiar enough 
with the properties regime as a whole to really recommend where we might 
inject such a bool.

Of course, this would have to be a 2.2+ fix. For 2.1, I might just stick 
with my original plan and make the error message nicer.

Thoughts?

--j

[-- Attachment #2: Type: text/html, Size: 7839 bytes --]