Re: [Qemu-devel] [PATCH RFC 2/6] qapi: use qemu_strtod() in string-input-visitor

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 11/15/18 5:09 AM, David Hildenbrand wrote:
>
>>> Three more: in qobject-input-visitor.c's
>>> qobject_input_type_number_keyval(),
>>
>> This one is interesting, as it properly bails out when parsing "inf"
>> (via isFinite()). - should we do the same for the string input visitor?
>>
>> Especially, should we forbid "inf" and "NaN" in both scenarios?
>
> JSON can't represent non-finite doubles. Internally, we might be able
> to use them, but you have a point that consistently rejecting
> non-finite in all of our QAPI parsers makes it easier to reason about
> the code base (the command line can't be used to inject a value not
> possible via QMP). So that makes sense to me.

Given the liberties we already take with JSON in QAPI, "JSON can't do X"
need not immediately end a conversation about QAPI.

That said, consistency between QObject and string visitor is desirable.

The QObject input visitor comes in two flavours: one for use with the
JSON parser, and one for use with keyval_parse().

The latter flavour's qobject_input_type_number_keyval() rejects
non-finite numbers.

The former flavour's qobject_input_type_number() leaves rejecting "bad"
numbers to the JSON parser.  The JSON parser doesn't accept special
syntax for NaN or infinity.  It maps large numbers to HUGE_VAL with the
appropriate sign.  Whether +/-HUGE_VAL are infinities depends on the
host.  If we really want to outlaw infinities, we better fix
parse_literal() to check for ERANGE.

>                                                qemu_strtod() shouldn't
> reject non-finite numbers (because it is useful for more than just
> qapi),

Yes.

>        but we could add a new qemu_strtod_finite() if that would help
> avoid duplication.

Maybe.

    qemu_strtod(str, NULL, &dbl) && isfinite(dbl)

isn't that much shorter or clearer than

    qemu_strtod_finite(str, NULL, &dbl)

>> cutil.c's do_strtosz(), and
>> json-parser.c's parse_literal().
>> 
>> The latter doesn't check for errors since the lexer ensures the input is
>> sane.  Overflow can still happen, and is silently ignored.  Feel free
>> not to convert this one.
>> 
>
> I'll do the conversion of all (allowing -ERANGE where it used to be
> allow), we can then discuss with the patches at hand if it makes sense.

Sure!