Libguestfs - Aug 2018 - Re: [PATCH v3 1/4] mltools: Rename Yajl module as JSON_parser and move to common/mltools.

On 08/17/2018 10:16 AM, Richard W.M. Jones wrote:
> Commit bd1c5c9f4dcf38458099db8a0bf4659a07ef055d changed all the code
> to use Jansson instead of yajl.  However it didn't change the OCaml
> API name (which was still Yajl).
> 
Are you aware that Jansson can't parse all JSON generated by qemu, and 
that the developers of Jansson did not seem sympathetic to patches that 
would make it possible?  Libvirt recently reverted their use of Jansson 
because of its inability to deal with unsigned 64-bit numbers (and 
sadly, RFC7159 does not define bounds for what forms valid JSON numbers, 
but merely leaves it up to implementations to decide for themselves).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

On Friday, 17 August 2018 17:30:35 CEST Eric Blake
wrote:
> On 08/17/2018 10:16 AM, Richard W.M. Jones wrote:
> > Commit bd1c5c9f4dcf38458099db8a0bf4659a07ef055d changed all the code
> > to use Jansson instead of yajl.  However it didn't change the
OCaml
> > API name (which was still Yajl).
> > 
> 
> Are you aware that Jansson can't parse all JSON generated by qemu, and 
> that the developers of Jansson did not seem sympathetic to patches that 
> would make it possible?  Libvirt recently reverted their use of Jansson 
> because of its inability to deal with unsigned 64-bit numbers (and 
> sadly, RFC7159 does not define bounds for what forms valid JSON numbers, 
> but merely leaves it up to implementations to decide for themselves).
The problematic values that qemu outputs are, at least to my knowledge,
only in the some of the messages in the QMP communication, which we do
not do.  We parse JSON from:
- output of ldmtool
- output of qemu-img
- QMP schema
- SimpleStream files

Furthermore, why not just fix qemu, instead of letting its
"JSON implementation" rule out perfectly compliant JSON parsers?

-- 
Pino Toscano

On Fri, Aug 17, 2018 at 10:30:35AM -0500, Eric Blake
wrote:
> On 08/17/2018 10:16 AM, Richard W.M. Jones wrote:
> >Commit bd1c5c9f4dcf38458099db8a0bf4659a07ef055d changed all the code
> >to use Jansson instead of yajl.  However it didn't change the OCaml
> >API name (which was still Yajl).
> >
> 
> Are you aware that Jansson can't parse all JSON generated by qemu,
> and that the developers of Jansson did not seem sympathetic to
> patches that would make it possible?  Libvirt recently reverted
> their use of Jansson because of its inability to deal with unsigned
> 64-bit numbers (and sadly, RFC7159 does not define bounds for what
> forms valid JSON numbers, but merely leaves it up to implementations
> to decide for themselves).
Yes, painfully aware.  Not sure what to do about it however, since the
alternative (ie. switching back to yajl) as libvirt did is not going
to be pleasant.

Currently we're OK as long as disk sizes don't exceed 8 petabytes, if
my quick calculation is correct.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/

On Fri, Aug 17, 2018 at 05:44:58PM +0200, Pino Toscano
wrote:
> On Friday, 17 August 2018 17:30:35 CEST Eric Blake wrote:
> > On 08/17/2018 10:16 AM, Richard W.M. Jones wrote:
> > > Commit bd1c5c9f4dcf38458099db8a0bf4659a07ef055d changed all the
code
> > > to use Jansson instead of yajl.  However it didn't change the
OCaml
> > > API name (which was still Yajl).
> > > 
> > 
> > Are you aware that Jansson can't parse all JSON generated by qemu,
and
> > that the developers of Jansson did not seem sympathetic to patches
that
> > would make it possible?  Libvirt recently reverted their use of
Jansson
> > because of its inability to deal with unsigned 64-bit numbers (and 
> > sadly, RFC7159 does not define bounds for what forms valid JSON
numbers,
> > but merely leaves it up to implementations to decide for themselves).
> 
> The problematic values that qemu outputs are, at least to my knowledge,
> only in the some of the messages in the QMP communication, which we do
> not do.  We parse JSON from:
> - output of ldmtool
> - output of qemu-img
> - QMP schema
> - SimpleStream files
> 
> Furthermore, why not just fix qemu, instead of letting its
> "JSON implementation" rule out perfectly compliant JSON parsers?
I'm going to guess because this would change the qemu API
(if you mean printing out a string instead of an int).

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/

On 08/17/2018 10:48 AM, Richard W.M. Jones wrote:
> On Fri, Aug 17, 2018 at 10:30:35AM -0500, Eric Blake wrote:
>> On 08/17/2018 10:16 AM, Richard W.M. Jones wrote:
>>> Commit bd1c5c9f4dcf38458099db8a0bf4659a07ef055d changed all the
code
>>> to use Jansson instead of yajl.  However it didn't change the
OCaml
>>> API name (which was still Yajl).
>>>
>>
>> Are you aware that Jansson can't parse all JSON generated by qemu,
>> and that the developers of Jansson did not seem sympathetic to
>> patches that would make it possible?  Libvirt recently reverted
>> their use of Jansson because of its inability to deal with unsigned
>> 64-bit numbers (and sadly, RFC7159 does not define bounds for what
>> forms valid JSON numbers, but merely leaves it up to implementations
>> to decide for themselves).
> 
> Yes, painfully aware.  Not sure what to do about it however, since the
> alternative (ie. switching back to yajl) as libvirt did is not going
> to be pleasant.
> 
> Currently we're OK as long as disk sizes don't exceed 8 petabytes,
if
> my quick calculation is correct.
The problem comes anywhere that qemu outputs an unsigned 64-bit number 
as unsigned AND where that value is larger than INT64_MAX (jansson uses 
strtoll, rather than strtoull).  But since disk sizes cannot exceed 
off_t, which is a signed 64-bit number, it does not matter whether qemu 
outputs those as signed or unsigned - they will still be parseable as a 
signed number.  Thus, you are correct that disk sizes in qemu output 
won't trigger the Jansson limitation.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org