From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46603) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aw8hc-0005z5-7i for qemu-devel@nongnu.org; Fri, 29 Apr 2016 09:43:34 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aw8hQ-0006kw-4V for qemu-devel@nongnu.org; Fri, 29 Apr 2016 09:43:22 -0400 Received: from mx1.redhat.com ([209.132.183.28]:37275) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aw8hP-0006hx-TE for qemu-devel@nongnu.org; Fri, 29 Apr 2016 09:43:16 -0400 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id C4015C049E18 for ; Fri, 29 Apr 2016 13:43:04 +0000 (UTC) References: <1461903820-3092-1-git-send-email-eblake@redhat.com> <1461903820-3092-5-git-send-email-eblake@redhat.com> <87inz0y2fg.fsf@dusky.pond.sub.org> From: Eric Blake Message-ID: <572364E7.3000002@redhat.com> Date: Fri, 29 Apr 2016 07:43:03 -0600 MIME-Version: 1.0 In-Reply-To: <87inz0y2fg.fsf@dusky.pond.sub.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="jfb1D42RxLukrinROe5KrA6cgIPHv09SP" Subject: Re: [Qemu-devel] [PATCH v3 04/18] qapi: Factor out JSON number formatting List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: qemu-devel@nongnu.org, famz@redhat.com, Luiz Capitulino This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --jfb1D42RxLukrinROe5KrA6cgIPHv09SP Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 04/29/2016 07:22 AM, Markus Armbruster wrote: > Eric Blake writes: >=20 >> 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 >> Reviewed-by: Fam Zheng >> >> /** >> + * qstring_append_json_number(): Append a JSON number to a QString. >> + * Set @errp if the number is not representable in JSON, but append t= he >> + * output anyway (callers can then choose to ignore the warning). >> + */ >> +void qstring_append_json_number(QString *qstring, double number, Erro= r **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", nu= mber); >> + } >=20 > Separate patch, please. Okay. >=20 > "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? >=20 >> + >> + /* 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. */ >=20 > 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). >=20 > I think this belongs into qobject-json.c, like the previous patch's > qstring_append_json_string(). Sounds reasonable. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --jfb1D42RxLukrinROe5KrA6cgIPHv09SP Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJXI2TnAAoJEKeha0olJ0NqaggH/ikpwJ1Vll5zu+jGRinUkdJU QjDSeTSPc581Czhj/2hgbKHsHJoWcnsWZHcyfS998/jWFHcWm8zkoNx9hEkaEnq3 qng5bCDql+4ffrGRB0gdAEVdolpW+YEAU6vrjMmHEtsnG1/3UAiIPfXuVQIww8HE BhoKMsZv6aZulDm0LMvZLTPTqQ+NNioNBAbeoc/BqOnBOCBNpiVfSlOu8Wo5j28y XbpDwkYbQCKlzqLF5FafxP0eoKRYlQJZi3S1idJ/qP+HG6/xLZg2lKpHOzmeM44r 0nVnW5/x8ivGeW0hCLQlr8clyBWd7A0uLg9NiRAVWDDvD9T1eurENuWSTq545eg= =lSSd -----END PGP SIGNATURE----- --jfb1D42RxLukrinROe5KrA6cgIPHv09SP--