Re: [Qemu-devel] [PATCH v4 17/28] qapi: Factor out JSON number formatting

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 06/03/2016 03:02 AM, Markus Armbruster wrote:
>
>>>> Suggest:
>>>>
>>>>  * Return 0 if the number is finite, as required by RFC 7159, else -1.
>>>>
>>>> The return value makes some sense only for symmetry with
>>>> qstring_append_json_string().  Without that, I'd ask you to keep this
>>>> function simple.  Callers could just as easily test isfinite()
>>>> themselves.
>>>
>>> I'm actually thinking of modifying this, given the recent thread
>>> pointing out that libvirt chokes hard on JSON extensions:
>>>
>>> https://lists.gnu.org/archive/html/qemu-devel/2016-05/msg04398.html
>>>
>>> That is, for symmetry with qstring_append_json_string(), I'm thinking of
>>> changing NaN to 0 and Inf to DBL_MAX, and always outputting a finite
>>> value, in addition to returning -1 to inform the caller that a
>>> substitution was made, so that the output is always strict JSON.
>> 
>> Mapping infinities to DBL_MIN and DBL_MAX is debatable, but mapping NaN
>> to zero is outright wrong.
>
> How about this alternative:
>
> Finite values remain numbers:
> "number":1
>
> But non-finite values are output as strings, so that our output is
> always valid JSON - the recipient may not be expecting a string in place
> of a number, but at least should be able to parse the output rather than
> choking hard.
> "number":"nan"
>
> The return value -1 then indicates that a stringized replacement was
> used, so that any later patch can use a strict flag on whether to allow
> the replacement output or assert.

I dislike this a lot less than mapping non-finite numbers to finite
ones.

I still dislike it, because it defeats fitting a schema to QMP: instead
of the true JSON type "number", we'd need the sum type of "number" and
"string", which this really isn't: only a few special strings are valid,
and they're not actually strings.  If the schema language can do sum
types, we'd even be stuck with their common super-type.

The most practical solution isn't always a likable one, though.

>> If we decide QMP should stick to JSON here and avoid non-finite numbers,
>> we need to treat an attempt to emit a non-finite number as a bug:
>> assert(isfinite(...)).  Making sure nothing ever attempts to emit such
>> numbers will be tedious.
>> 
>> If we decide QMP should remain as it is, we need to document non-finite
>> numbers among its JSON extensions.  We should also fix our QMP parsers
>> to accept non-finite numbers then.  Including the one in libvirt.
>> Attempts to emit non-finite numbers then *may* be bugs.  Really no
>> different than finite numbers outside their intended range, such as a
>> negative size.  Catching these bugs is of course also tedious.  The
>> difference is that they manifest in QMP as semantic instead of lexical
>> errors.  Lexical errors are the worst to handle gracefully.
>
> I may still try to tackle fixing the QMP parser to accept NaN and
> infinity on input (since it's hand-written, we at least have control
> over that)

Making json-lexer.c recognize infinities and NaNs in strtod() syntax
shouldn't be hard.  I'd omit nan(n-char-sequence-opt), because its
semantics are implementation defined.  I'd also omit all spellings other
than inf and nan.  That leaves inf, +inf, -inf, nan, +nan, -nan.

>            - it will certainly be easier than getting libvirt to parse
> non-finite numbers (libvirt uses libyajl, and my emails to the yajl
> mailing list have gone unanswered, making me think the project is not
> very vibrant and thus not very patchable).

Nobody likes to carry downstream patches, but an unresponsive upstream
may leave you no choice.

>                                             But with my proposal of
> producing a stringized non-finite value, we at least convert lexical
> into semantic errors, which I agree with your assessment is a nicer way
> of dealing with it.
>
> Of course, a policy change of outputting stringized non-finite numbers
> should be separate from refactoring patches that just move functions around.

Yes.