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

[John Snow <jsnow@redhat.com>]

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.