qemu-devel
[Top][All Lists]
Advanced

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

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


From: Markus Armbruster
Subject: Re: [Qemu-devel] [PATCH v2] virtio-rng: Add human-readable error message for negative max-bytes parameter
Date: Mon, 21 Jul 2014 09:48:29 +0200
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux)

John Snow <address@hidden> writes:

> On 07/18/2014 09:16 AM, Markus Armbruster wrote:
>> Amit Shah <address@hidden> writes:
>>
>>> On (Fri) 18 Jul 2014 [13:54:01], Markus Armbruster wrote:
>>>> Amit Shah <address@hidden> writes:
>>>>
>>>>> On (Fri) 18 Jul 2014 [13:15:18], Markus Armbruster wrote:
>>>>>> Amit Shah <address@hidden> writes:
>>>>>>
>>>>>>> On (Fri) 18 Jul 2014 [08:27:59], Markus Armbruster wrote:
>>>>>>>> John Snow <address@hidden> 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 <address@hidden>
>>>>>>>>> ---
>>>>>>>>> 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.

It certainly could check whether the value fits into uint64_t.

A quick peek at how string-input-visitor.c uses strtoll() makes me
cringe.

[...]



reply via email to

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