[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 04/18] qapi: Factor out JSON number formattin
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH v3 04/18] qapi: Factor out JSON number formatting |
Date: |
Fri, 29 Apr 2016 07:43:03 -0600 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 |
On 04/29/2016 07:22 AM, Markus Armbruster wrote:
> Eric Blake <address@hidden> 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 <address@hidden>
>> Reviewed-by: Fam Zheng <address@hidden>
>>
>> /**
>> + * 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
signature.asc
Description: OpenPGP digital signature
- [Qemu-devel] [PATCH v3 02/18] qapi: Improve use of qmp/types.h, (continued)
- [Qemu-devel] [PATCH v3 02/18] qapi: Improve use of qmp/types.h, Eric Blake, 2016/04/29
- [Qemu-devel] [PATCH v3 06/18] qapi: Add qstring_append_format(), Eric Blake, 2016/04/29
- [Qemu-devel] [PATCH v3 01/18] qapi: Rename (one) qjson.h to qobject-json.h, Eric Blake, 2016/04/29
- [Qemu-devel] [PATCH v3 11/18] qjson: Remove unused file, Eric Blake, 2016/04/29
- [Qemu-devel] [PATCH v3 16/18] sockets: Use new QAPI cloning, Eric Blake, 2016/04/29
- [Qemu-devel] [PATCH v3 04/18] qapi: Factor out JSON number formatting, Eric Blake, 2016/04/29
- [Qemu-devel] [PATCH v3 07/18] qapi: Add json output visitor, Eric Blake, 2016/04/29
- [Qemu-devel] [PATCH v3 10/18] vmstate: Use new JSON output visitor, Eric Blake, 2016/04/29
- [Qemu-devel] [PATCH v3 15/18] qapi: Add new clone visitor, Eric Blake, 2016/04/29
- [Qemu-devel] [PATCH v3 13/18] qapi: Support pretty printing in JSON output visitor, Eric Blake, 2016/04/29
- [Qemu-devel] [PATCH v3 14/18] qemu-img: Use new JSON output formatter, Eric Blake, 2016/04/29
- [Qemu-devel] [PATCH v3 12/18] qapi: Add qobject_to_json_pretty_prefix(), Eric Blake, 2016/04/29
- [Qemu-devel] [PATCH v3 17/18] replay: Use new QAPI cloning, Eric Blake, 2016/04/29
- [Qemu-devel] [PATCH v3 18/18] qapi: Add parameter to visit_end_*, Eric Blake, 2016/04/29