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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

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.

> > However, there is other instances that some field has uint64 type and
> > actually generates -1 value.
> > 
> > The discussion below is orthogonal to the problem I found.
> > 
> > > I wonder -- should we omit fields which are set to default from the json
> > > altogether rather than encode them as some specific value? (the actual
> > > value is notionally somewhat libxl internal). This would be a case of
> > 
> > LIBXL_MEMKB_DEFAULT is in libxl.h, as with some other default
> > values, so that the value is actually visible to external users. Not
> > saying that the actual value affects that much, just to clarify.
> 
> I think this counts as being "invisible", in that it would be wrong for
> the user to ever hard code -1 or ~0 here -- they should always use
> LIBXL_MEMKB_DEFAULT and not care/know what the value happens to be. In
> principal we could change it if we wanted (ok, there probably isn't any
> other sane value in this case)
> 

Fine with me to not generate JSON for all those "default" values. The
parser should be able to cope with missing fields anyway.

Wei.

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