[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: |
Fri, 18 Jul 2014 15:16:07 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) |
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 :)
- [Qemu-devel] [PATCH v2] virtio-rng: Add human-readable error message for negative max-bytes parameter, John Snow, 2014/07/17
- Re: [Qemu-devel] [PATCH v2] virtio-rng: Add human-readable error message for negative max-bytes parameter, Markus Armbruster, 2014/07/18
- Re: [Qemu-devel] [PATCH v2] virtio-rng: Add human-readable error message for negative max-bytes parameter, Amit Shah, 2014/07/18
- Re: [Qemu-devel] [PATCH v2] virtio-rng: Add human-readable error message for negative max-bytes parameter, Markus Armbruster, 2014/07/18
- Re: [Qemu-devel] [PATCH v2] virtio-rng: Add human-readable error message for negative max-bytes parameter, Amit Shah, 2014/07/18
- Re: [Qemu-devel] [PATCH v2] virtio-rng: Add human-readable error message for negative max-bytes parameter, Markus Armbruster, 2014/07/18
- Re: [Qemu-devel] [PATCH v2] virtio-rng: Add human-readable error message for negative max-bytes parameter, Amit Shah, 2014/07/18
- Re: [Qemu-devel] [PATCH v2] virtio-rng: Add human-readable error message for negative max-bytes parameter,
Markus Armbruster <=
- Re: [Qemu-devel] [PATCH v2] virtio-rng: Add human-readable error message for negative max-bytes parameter, John Snow, 2014/07/18
- Re: [Qemu-devel] [PATCH v2] virtio-rng: Add human-readable error message for negative max-bytes parameter, Markus Armbruster, 2014/07/21
- Re: [Qemu-devel] [PATCH v2] virtio-rng: Add human-readable error message for negative max-bytes parameter, John Snow, 2014/07/18
- Re: [Qemu-devel] [PATCH v2] virtio-rng: Add human-readable error message for negative max-bytes parameter, Eric Blake, 2014/07/18
- Re: [Qemu-devel] [PATCH v2] virtio-rng: Add human-readable error message for negative max-bytes parameter, Markus Armbruster, 2014/07/21
- Re: [Qemu-devel] [PATCH v2] virtio-rng: Add human-readable error message for negative max-bytes parameter, John Snow, 2014/07/21
- Re: [Qemu-devel] [PATCH v2] virtio-rng: Add human-readable error message for negative max-bytes parameter, Markus Armbruster, 2014/07/21
- Re: [Qemu-devel] [PATCH v2] virtio-rng: Add human-readable error message for negative max-bytes parameter, John Snow, 2014/07/21
- Re: [Qemu-devel] [PATCH v2] virtio-rng: Add human-readable error message for negative max-bytes parameter, Markus Armbruster, 2014/07/21
- Re: [Qemu-devel] [PATCH v2] virtio-rng: Add human-readable error message for negative max-bytes parameter, John Snow, 2014/07/21