Re: [Qemu-devel] [PATCH v3 04/18] qapi: Factor out JSON number formatting

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2836 bytes --]

On 04/29/2016 07:22 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> Pull out a new qstring_append_json_number() helper, so that all
>> JSON output producers can use a consistent style for printing
>> floating point without duplicating code (since we are doing more
>> data massaging than a simple printf format can handle).
>>
>> Address one FIXME by adding an Error parameter and warning the
>> caller if the requested number cannot be represented in JSON;
>> but add another FIXME in its place because we have no way to
>> report the problem higher up the stack.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> Reviewed-by: Fam Zheng <famz@redhat.com>
>>

>>  /**
>> + * qstring_append_json_number(): Append a JSON number to a QString.
>> + * Set @errp if the number is not representable in JSON, but append the
>> + * output anyway (callers can then choose to ignore the warning).
>> + */
>> +void qstring_append_json_number(QString *qstring, double number, Error **errp)
>> +{
>> +    char buffer[1024];
>> +    int len;
>> +
>> +    /* JSON does not allow Inf or NaN; append it but set errp */
>> +    if (!isfinite(number)) {
>> +        error_setg(errp, "Non-finite number %f is not valid JSON", number);
>> +    }
> 
> Separate patch, please.

Okay.

> 
> "Append it but set errp" feels odd.  Normally, returning with an error
> set means the function failed to do its job.

This one's weird because by the end of the series, it will be used by
the new JSON visitor (which wants the error message because that is not
valid JSON, and doesn't care if the QString is slightly longer); as well
as the existing QMP output visitor (where existing behavior ignores that
it is not valid JSON, and we don't really have a convenient way to pass
errors back up the stack).  Is it worth trying to plumb in better error
reporting to the QMP output visitor, and/or add assertions that values
are finite, and/or document that QMP has an extension beyond JSON in
that it accepts and also might produce Inf/NaN?

> 
>> +
>> +    /* FIXME: snprintf() is locale dependent; but JSON requires
>> +     * numbers to be formatted as if in the C locale. Dependence
>> +     * on C locale is a pervasive issue in QEMU. */
>> +    /* FIXME: This risks printing Inf or NaN, which are not valid
>> +     * JSON values. */
> 
> Your !isfinite() conditional addresses this, doesn't it?

Yep. Looks like I messed up the rebase (I realized I had to re-move
updated code, but didn't scrub the comments after the move).


> 
> I think this belongs into qobject-json.c, like the previous patch's
> qstring_append_json_string().

Sounds reasonable.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]