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

[Eric Blake]

On 06/16/2016 10:17 AM, Markus Armbruster wrote:
> Eric Blake <address@hidden> 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").

> 
> 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.

> 
>> 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 <address@hidden>
> 
> 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).

> 
> * 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.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature