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

[Markus Armbruster]

Eric Blake <address@hidden> writes:

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

QMP is what it is.  We can try to get it back closer to pure JSON, and
declaring attempts to emit non-finite numbers bugs would be a step in
that direction.

What kind of bugs would that be?

If it's a programming bug, then assert.  How would a user of the QMP
output visitor avoid this programming bug?

If it's not a programming bug, then fail with error set.  How would a
user of the QMP output visitor handle this error?

That said, I'm reluctant to mess with QMP now.  We really, really need
to control the scope of your ongoing QAPI work, or we'll never get to
Marc-André's.

Instead, I'd prefer to document what we have.  If we plan to change it,
say so, and add a suitable TODO or FIXME comment.

With things kept as they are, I can see two ways to avoid
qstring_append_json_number()'s unusual behavior:

1. Add a @must_be_finite parameter.  Set the error only when it's true,
and don't do anything else then.

2. Lift the decision whether non-finite is okay into the callers.
Instead of

    // non-finite is okay
    qstring_append_json_number(qs, num, NULL);

    // non-finite is not okay
    qstring_append_json_number(qs, num, &err);
    if (err) {
        // handle error...
        // return, goto out, or similar
    }

do

    // non-finite is okay
    qstring_append_json_number(qs, num);

    // non-finite is not okay
    if (!isfinite(num) {
        error_setg(&err, "Non-finite number %f is not valid JSON", num);
        // handle error...
        // return, goto out, or similar
    }
    qstring_append_json_number(qs, num);

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