Re: [Qemu-devel] [PATCH 4/4] qobject: Output valid JSON for non-finite numbers

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@redhat.com> writes:

> On 06/16/2016 10:17 AM, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> writes:
>> 
>>> It's better to give downstream clients a valid JSON string,
>>> even if they are semantically expecting a number, than it is
>>> to give them a bare keyword extension that can cause a
>>> lexical error.
>> 
>> Incompatible change.  If all clients are choking on non-finite numbers,
>> then the incompatibility is an improvement.  If a client exists that
>> groks non-finite numbers, ...  Absence is always hard to show.
>
> The 'id' field is an outlier - there, we replay the user's input with no
> contextual interpretation (however, we DO reserve the right to reorder
> the keys in the dicts that we replay, and to canonicalize UTF-8 text or
> otherwise alter their input to something "equivalent").

Yes, the response's id must the the same JSON value, but it needn't be
the same text.

>> Moreover, it turns query-qmp-schema into a liar: the schema it returns
>> claims a certain member of the reply has "type": "number", and then we
>> go on to send a string anyway.
>
> The 'id' field is documented as sending ANY JSON value, so if we argue
> that canonicalizing their extension input of a bare inf into a proper
> JSON string on output is reasonable, then we may want this patch in
> addition to adding assertions that none of the QMP commands with
> introspectible 'number' ever output non-finite values.

I read this thrice, and I'm still not sure I got the argument :)

>>> Of course, as long as we don't recognize (certain) strings as valid
>>> numbers during a conversion to QObject,
>> 
>> That would be even crazier!
>> 
>>>                                         this means our extension
>>> of accepting bare keywords for non-finite numbers cannot undergo
>>> a round trip (once converted into a string, we never get back to
>>> a QFloat).  However, non-finite input is rare enough that it's
>>> not worth bothering with at the moment.
>>>
>>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> 
>> I'm afraid the only sane solution is to find all uses of number in QMP
>> output, audit the code producing them, then assert isfinite() in the
>> monitor.  For commands without a side effect, we could fail the command
>> instead of tripping an assertion.  We'd have to declare such commands.
>> 
>> Let's examine the occurences of "number" in output of query-qmp-schema,
>> or actually in the qmp-introspect.c that gets generated with -u:
>> 
>> * Object q_obj_migrate_set_downtime-arg member value: input
>
> Even though it's not output, it does need to be checked that it will
> behave sanely with Inf or NaN input if we extend our parser to allow
> those (behaving sanely may include a graceful error that the input was
> out of range).

Yes, *if* we extend QMP.

>> 
>> * Builtin number: d'uh!
>> 
>> * Object MigrationStats member mbps: in output of query-migrate
>> 
>> * Object XBZRLECacheStats member overflow: likewise
>> 
>> * Object KeyValue case number: not a type.
>> 
>> * Object BlockDeviceTimedStats members avg_rd_queue_depth,
>>   avg_wr_queue_depth: in output of query-blockstats
>> 
>> * Enum CommandLineParameterType member: not a type
>> 
>> * Enum JSONType member: not a type
>> 
>> * Enum KeyValueKind: not a type
>> 
>> * Object PciBusInfo member: not a type
>> 
>> So it's just query-migrate and query-blockstats.
>> 
>
> Okay, looks like I need to respin this, and the rest of my JSON output
> visitor on top of it, with this audit done first.

Audit, plus isfinite() assertions to guard the JSON output.

The (misnamed) QMP output visitor shouldn't assert, because it can
legitimately be used for purposes other than QMP.  Only the actual
conversion to JSON should assert.  Currently, to_json().  With your JSON
output visitor, it would be qstring_append_json_number(), or its caller.