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

[Eric Blake <eblake@redhat.com>]

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