Eric Blake <ebl...@redhat.com> 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!
