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

[Markus Armbruster]

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?