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

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

[John Snow]

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);
+        return;
+    }
     vrng->quota_remaining = vrng->conf.max_bytes;
 
     vrng->rate_limit_timer = timer_new_ms(QEMU_CLOCK_VIRTUAL,
-- 
1.9.3

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

[Markus Armbruster]

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);
> +        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.

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

[Amit Shah]

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);
> > +        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?

Do you think the usage as in this patch is fine?

Thanks,

		Amit

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

[Markus Armbruster]

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);
>> > +        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?

> Do you think the usage as in this patch is fine?

It's not wrong, just inconsistent with existing usage.  I'd prefer
consistency.

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

[Amit Shah]

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);
> >> > +        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).

> > 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.


		Amit

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

[Markus Armbruster]

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?

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

[Amit Shah]

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.

John, can you look at Markus's comments and address them in a series?

Thanks!

		Amit

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

[Markus Armbruster]

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 :)

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

[John Snow]

[-- Attachment #1: Type: text/plain, Size: 4110 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 :)

1) /should/ the property be uint64_t? Do we really want to allow people 
to specify 16EiB? Logically the property should be unsigned of some 
sort, but the range we wish to restrict it to is probably much smaller. 
Is there an existing codepath that enforces signed/unsigned integers 
earlier, at parsing?

2) If we don't constrict the range further, then I agree: the "positive" 
error message is more user-friendly. It was a semantic choice on my 
part, but I think the less technical error is better.

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

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

[John Snow]

[-- 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 --]

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

[Eric Blake]

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

On 07/18/2014 03:14 PM, John Snow wrote:
> 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.

Yes, for that very reason, libvirt recently switched from two wrappers
per width (parse signed, parse unsigned) to three wrappers (parse
signed, parse unsigned and allow negative wraparound, parse unsigned and
reject negative wraparound).  By the way, did you realize that strtoull
is required to parse 50% more input than strtoll, all because of
negative wraparound?

> 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.

When we tightened the parsing by adding the variant that rejects
negative wraparound, we had to audit a lot of code to decide which
callers still want to allow wraparound as a convenience (for example,
it's much easier to say "copy up to -1 bytes of a file" than it is to
say "copy up to 9223372036854775807 bytes of a file", when the semantics
of copy automatically stop when EOF is hit no matter how much larger the
end bound is when treated as unsigned).

I also want to make sure we don't break QMP. The JSON parser is a bit
finicky about numbers larger than LLONG_MAX, and so there are existing
cases where for 64-bit unsigned parameters, libvirt has code to pass in
the negative 2s complement counterpart in order to not upset the parser.

> 
> 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.

Absolutely.  For 2.1, only the bare minimum for this one message is
okay; any dramatic cleanups to reject negative wraparound is 2.2 material.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> 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 :)
>
> 1) /should/ the property be uint64_t? Do we really want to allow
> people to specify 16EiB? Logically the property should be unsigned of
> some sort, but the range we wish to restrict it to is probably much
> smaller.

Is there a non-arbitrary limit?

>          Is there an existing codepath that enforces signed/unsigned
> integers earlier, at parsing?

I guess you investigated this in your other reply.  Ask again if you
want further input.

> 2) If we don't constrict the range further, then I agree: the
> "positive" error message is more user-friendly. It was a semantic
> choice on my part, but I think the less technical error is better.

Yes.

If we permit zero, we better say "non-negative".

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

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> 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.

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.

[...]

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

[John Snow]

On 07/21/2014 03:48 AM, Markus Armbruster wrote:
>
> 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.
>
> [...]

What I meant by that was to say that by the time a value was returned to 
visit_type_uint64, the value has already been possibly converted 
implicitly from a negative value, and we can't tell at this level if 
that happened without re-inspecting the string we were passed. At that 
point, why not just fix the string parsing mechanics one more layer down 
in parse_type_int() -- or by creating another routine primitive; i.e 
parse_type_uint.

As Eric Blake noted elsewhere in the thread, it would be nice to have 
the ability to have three behaviors at the lowest level -- signed, 
unsigned with wraparound, and unsigned strict. The biggest question in 
my mind is how to add the property flag to allow authors to opt-in to 
the unsigned with wraparound option, where the unsigned strict option 
makes the most sense to me as a default.

For now I will just fix the error message. We can draft ideas for how to 
fix the semantic issues in parsing later.

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

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On 07/21/2014 03:48 AM, Markus Armbruster wrote:
>>
>> 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.
>>
>> [...]
>
> What I meant by that was to say that by the time a value was returned
> to visit_type_uint64, the value has already been possibly converted
> implicitly from a negative value, and we can't tell at this level if
> that happened without re-inspecting the string we were passed. At that
> point, why not just fix the string parsing mechanics one more layer
> down in parse_type_int() -- or by creating another routine primitive;
> i.e parse_type_uint.
>
> As Eric Blake noted elsewhere in the thread, it would be nice to have
> the ability to have three behaviors at the lowest level -- signed,
> unsigned with wraparound, and unsigned strict. The biggest question in
> my mind is how to add the property flag to allow authors to opt-in to
> the unsigned with wraparound option, where the unsigned strict option
> makes the most sense to me as a default.

Do we have a use case for silently mapping negative numbers to positive
ones?

> For now I will just fix the error message. We can draft ideas for how
> to fix the semantic issues in parsing later.

Yes, that makes sense.

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

[John Snow]

On 07/21/2014 01:33 PM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> On 07/21/2014 03:48 AM, Markus Armbruster wrote:
>>> 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.
>>>
>>> [...]
>> What I meant by that was to say that by the time a value was returned
>> to visit_type_uint64, the value has already been possibly converted
>> implicitly from a negative value, and we can't tell at this level if
>> that happened without re-inspecting the string we were passed. At that
>> point, why not just fix the string parsing mechanics one more layer
>> down in parse_type_int() -- or by creating another routine primitive;
>> i.e parse_type_uint.
>>
>> As Eric Blake noted elsewhere in the thread, it would be nice to have
>> the ability to have three behaviors at the lowest level -- signed,
>> unsigned with wraparound, and unsigned strict. The biggest question in
>> my mind is how to add the property flag to allow authors to opt-in to
>> the unsigned with wraparound option, where the unsigned strict option
>> makes the most sense to me as a default.
> Do we have a use case for silently mapping negative numbers to positive
> ones?

Via Eric Blake, for cases where "-1" is a convenient shorthand for "MAX" 
in lieu of writing out gibberish values like 4 billion or 18 
quintillion. I don't know if anyone actually relies on this behavior, 
but I don't know that they're not. I can easily imagine something like 
--max-log-messages=-1, for instance.

eblake had said that libvirt recently made a similar parsing change and 
they wound up with those three backing cases.

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

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On 07/21/2014 01:33 PM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>>
>>> On 07/21/2014 03:48 AM, Markus Armbruster wrote:
>>>> 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.
>>>>
>>>> [...]
>>> What I meant by that was to say that by the time a value was returned
>>> to visit_type_uint64, the value has already been possibly converted
>>> implicitly from a negative value, and we can't tell at this level if
>>> that happened without re-inspecting the string we were passed. At that
>>> point, why not just fix the string parsing mechanics one more layer
>>> down in parse_type_int() -- or by creating another routine primitive;
>>> i.e parse_type_uint.
>>>
>>> As Eric Blake noted elsewhere in the thread, it would be nice to have
>>> the ability to have three behaviors at the lowest level -- signed,
>>> unsigned with wraparound, and unsigned strict. The biggest question in
>>> my mind is how to add the property flag to allow authors to opt-in to
>>> the unsigned with wraparound option, where the unsigned strict option
>>> makes the most sense to me as a default.
>> Do we have a use case for silently mapping negative numbers to positive
>> ones?
>
> Via Eric Blake, for cases where "-1" is a convenient shorthand for
> "MAX" in lieu of writing out gibberish values like 4 billion or 18
> quintillion. I don't know if anyone actually relies on this behavior,
> but I don't know that they're not. I can easily imagine something like
> --max-log-messages=-1, for instance.

That's a pretty horrid way to let people say "maximum, please".

But I accept a backward compatibility argument.

> eblake had said that libvirt recently made a similar parsing change
> and they wound up with those three backing cases.

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

[John Snow]

On 07/21/2014 03:15 PM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> On 07/21/2014 01:33 PM, Markus Armbruster wrote:
>>> John Snow <jsnow@redhat.com> writes:
>>>
>>>> On 07/21/2014 03:48 AM, Markus Armbruster wrote:
>>>>> 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.
>>>>>
>>>>> [...]
>>>> What I meant by that was to say that by the time a value was returned
>>>> to visit_type_uint64, the value has already been possibly converted
>>>> implicitly from a negative value, and we can't tell at this level if
>>>> that happened without re-inspecting the string we were passed. At that
>>>> point, why not just fix the string parsing mechanics one more layer
>>>> down in parse_type_int() -- or by creating another routine primitive;
>>>> i.e parse_type_uint.
>>>>
>>>> As Eric Blake noted elsewhere in the thread, it would be nice to have
>>>> the ability to have three behaviors at the lowest level -- signed,
>>>> unsigned with wraparound, and unsigned strict. The biggest question in
>>>> my mind is how to add the property flag to allow authors to opt-in to
>>>> the unsigned with wraparound option, where the unsigned strict option
>>>> makes the most sense to me as a default.
>>> Do we have a use case for silently mapping negative numbers to positive
>>> ones?
>> Via Eric Blake, for cases where "-1" is a convenient shorthand for
>> "MAX" in lieu of writing out gibberish values like 4 billion or 18
>> quintillion. I don't know if anyone actually relies on this behavior,
>> but I don't know that they're not. I can easily imagine something like
>> --max-log-messages=-1, for instance.
> That's a pretty horrid way to let people say "maximum, please".
>
> But I accept a backward compatibility argument.
>

Semantically and from an end-user usability standpoint, I certainly 
don't disagree. In almost all cases, --no-limit or --use-maximum or 
similar explicit commands are more meaningful, but knowing whether or 
not we need to support parsing negative integers for unsigned properties 
will come later. Perhaps in our case we will be able to avoid supporting 
such a case and force people to use semantically meaningful properties.

I can certainly grep through the code to find out who is using unsigned 
properties. In the case of uint32, -1 I believe will already wrap around 
but then overflow (because we parse as uint64_t) and throw an error, so 
I don't expect we will see anyone using -1 to signify "MAX" for less 
than 64bit properties. In the case of uint64, it may be more difficult 
to see who, if anyone, is abusing such behavior.

However, from a quick look-see it looks like DEFINE_PROP_UINT64 is used 
in 26 places. The fourth argument is "default value" and you can see 
many authors using -1 here, so either these authors expect wraparound or 
are trying to set the default value to something invalid that they will 
try to catch later on somehow.

CC'ing Eric Blake again for input, since he went through a similar 
ordeal recently and might have some input.

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

[Eric Blake]

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

On 07/21/2014 02:13 PM, John Snow wrote:

> I can certainly grep through the code to find out who is using unsigned
> properties. In the case of uint32, -1 I believe will already wrap around
> but then overflow (because we parse as uint64_t) and throw an error, so
> I don't expect we will see anyone using -1 to signify "MAX" for less
> than 64bit properties. In the case of uint64, it may be more difficult
> to see who, if anyone, is abusing such behavior.

Actually, you may find that behavior on uint32 is MORE likely to be
confused, rather than less.  The _reason_ libvirt started tightening up
is because we hit a case where we were parsing an unsigned 32-bit
integer, but had different behavior on 32-bit hosts than on 64-bit
hosts, and it all boiled down to type promotion rules (basically,
strtoul("-1") on 32-bit platforms wrapped around to a 32-bit value,
which was still in range for uint32, while strtoul("-1") on 64-bit
platforms wrapped around to a 64-bit value which then appeared different
when truncated to uint32).  At least strtoull("-1") behaves the same on
both 32-bit and 64-bit hosts.

> 
> However, from a quick look-see it looks like DEFINE_PROP_UINT64 is used
> in 26 places. The fourth argument is "default value" and you can see
> many authors using -1 here, so either these authors expect wraparound or
> are trying to set the default value to something invalid that they will
> try to catch later on somehow.
> 
> CC'ing Eric Blake again for input, since he went through a similar
> ordeal recently and might have some input.

Tightening semantics is always a pain - in libvirt, we had to audit all
callers and make a case-by-case judgment call on whether the tighter
semantics of rejecting negatives made sense.  We ended up with very few
callers that still allowed wraparound, but there's no magic fix short of
just auditing the callers.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]