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: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?

> > > 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.

Indeed, in fact now I think of it if we want people to be able to use
JSON as a domain config format it is required to do so.

Ian.


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