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

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

> There're some defaults that are not visible to external world though.
> 
> > comparing them against their IDL init_val in the gen_json function I
> > think.
> > 
> 
> Or the other way around, export all default values to libxl.h so that
> libxl users can interpret them as well.
> 
> Not sure what's the best approach.

Ian.


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