On 05/10/13 14:22, Eric Blake wrote: > On 05/09/2013 08:20 PM, Michael Roth wrote: >> Currently our JSON parser assumes that numbers lacking a mantissa are >> integers and attempts to store them as QInt/int64 values. This breaks in >> the case where the number overflows/underflows int64 values (which is >> still valid JSON) >> >> Fix this by detecting such cases and using a QFloat to store the value >> instead. >> >> Signed-off-by: Michael Roth <mdr...@linux.vnet.ibm.com> >> --- >> qobject/json-parser.c | 26 +++++++++++++++++++++++--- >> 1 file changed, 23 insertions(+), 3 deletions(-) > > This changes the error message handed back to QMP clients, and possibly > exposes problems in other qemu code that receives the result of json > parses. Previously, for an 'int' argument, if you passed in a too-large > number, you got an error that the argument was too large for int. Now, > the number is accepted as a double; are we guaranteed that in a context > that expects a qint, when that code is now handed a qfloat (a case which > was previously impossible because qint_from_int protected it), that the > code will still behave correctly?
I tried to consider this while reviewing... Maybe I was wrong. The pre-patch code for JSON_INTEGER: obj = QOBJECT(qint_from_int(strtoll(token_get_value(token), NULL, 10))); doesn't check for errors at all. (I assume that JSON_INTEGER is selected by the parser, token_get_type(), based on syntax purely.) I thought when the pre-patch version encounters an int-looking decimal string that's actually too big in magnitude for an int, you'd simply end up with LLONG_MIN or LLONG_MAX, but no error. strtoll() clamps the value, errno is lost, and qint_from_int() sees nothing wrong. With the patch, you end up with a float instead of an int-typed LLONG_MIN/LLONG_MAX, and also no error. > At any rate, libvirt already checks that all numbers that fall outside > the range of int64_t are never passed over qmp when passing an int > argument (and yes, this is annoying, in that large 64-bit unsigned > numbers have to be passed as negative numbers, rather than exceeding > INT64_MAX), so libvirt should not be triggering this newly exposed code > path. But even if libvirt doesn't plan on triggering it, I'd still feel > better if your commit message documented evidence of testing what > happens in this case. For example, compare what > {"execute":"add-fd","arguments":{"fdset-id":"99999999999999999999"}} > does before and after this patch. That would be likely interesting to test, yes. Laszlo