Re: [Xen-devel] [PATCH V4 09/24] libxl_json: introduce parser functions for builtin types

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Tue, 2014-05-13 at 11:51 +0100, Wei Liu wrote:
> On Tue, May 13, 2014 at 11:45:04AM +0100, Ian Campbell wrote:
> > On Tue, 2014-05-13 at 11:40 +0100, Wei Liu wrote:
> > > On Tue, May 13, 2014 at 10:32:57AM +0100, Ian Campbell wrote:
> > > > On Tue, 2014-05-13 at 10:19 +0100, Wei Liu wrote:
> > > > > On Tue, May 13, 2014 at 09:26:06AM +0100, Ian Campbell wrote:
> > > > > > On Mon, 2014-05-12 at 17:43 +0100, Wei Liu wrote:
> > > > > > > On Tue, May 06, 2014 at 04:42:20PM +0100, Wei Liu wrote:
> > > > > > > [...]
> > > > > > > > +
> > > > > > > > +int libxl__uint64_parse_json(libxl__gc *gc, const 
> > > > > > > > libxl__json_object *o,
> > > > > > > > +                             void *p)
> > > > > > > > +{
> > > > > > > > +    if (!libxl__json_object_is_integer(o) &&
> > > > > > > > +        !libxl__json_object_is_number(o))
> > > > > > > > +        return ERROR_FAIL;
> > > > > > > > +
> > > > > > > > +    if (libxl__json_object_is_integer(o)) {
> > > > > > > > +        long long i = libxl__json_object_get_integer(o);
> > > > > > > > +
> > > > > > > > +        if (i < 0)
> > > > > > > > +            return ERROR_FAIL;
> > > > > > > > +
> > > > > > > 
> > > > > > > Just found out that current API will generate -1 for some fields 
> > > > > > > (say,
> > > > > > > video_memkb), so we have to loosen this check.
> > > > > > 
> > > > > > Is -1 the flag for "default" in these cases?
> > > > > > 
> > > > > 
> > > > > Yes, that's actually ~0 (LIBXL_MEMKB_DEFAULT) in libxl.
> > > > > 
> > > > > MemKB type is actually uint64 but the generator generates int64 type,
> > > > > hence the -1.
> > > > 
> > > > Is this a separate (latent) bug? What happens for uint64 values >
> > > > INT64_MAX but < UINT64_MAX?
> > > > 
> > > 
> > > It's buggy I think. After the transformation (generator -> string ->
> > > parser) it gets sign-extended when it really shouldn't.
> > > 
> > > And this bug affects all fields which have uint64 type. Sigh. This is a
> > > limitation in yajl (yajl_gen_integer) not able to generate uint64 value.
> > > Parser (json_callback_number) can cope with values that it cannot
> > > convert to long long.
> > 
> > Oh dear. Are we supposed to be using yajl_gen_number then?
> > 
> 
> I think so. Some more patches to my queue. Looks like this work is never
> coming to an end. :-)

Sorry about that. I expected there to be some yakks in this work,
interesting they haven't turned out to be the ones I was expecting. 

> And presumably if we are to make this change we also need to update
> libxl.h to have LIBXL_JSON_GENERATE_NUMBER-ish? What would you suggest
> for the name?

Does this actually change the output syntax at all? If it is just a bug
fix then we don't need to announce it I think. Any consumers of the JSON
will have to cope with the buggy version (if they want to) irrespective
of any #define.

> Now looking back to my libxl__uint64_parse_json it's actually not buggy!
> Huh.

Great!

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel