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

[Anthony PERARD <anthony.perard@citrix.com>]

On Tue, May 06, 2014 at 03:49:06PM +0100, Wei Liu wrote:
> On Tue, May 06, 2014 at 03:45:47PM +0100, Wei Liu wrote:
> > On Tue, May 06, 2014 at 03:35:27PM +0100, Anthony PERARD wrote:
> > [...]
> > > > +/* Macro to generate:
> > > > + *  libxl__uint8_parse_json
> > > > + *  libxl__uint16_parse_json
> > > > + *  libxl__uint32_parse_json
> > > > + *  libxl__uint64_parse_json
> > > > + */
> > > > +#define PARSE_UINT(width)                                               \
> > > > +    int libxl__uint ## width ## _parse_json(libxl__gc *gc,              \
> > > > +                                            const libxl__json_object *o,\
> > > > +                                            void *p)                    \
> > > > +    {                                                                   \
> > > > +        long long i;                                                    \
> > > > +                                                                        \
> > > > +        if (!libxl__json_object_is_integer(o))                          \
> > > > +            return ERROR_FAIL;                                          \
> > > > +                                                                        \
> > > > +        i = libxl__json_object_get_integer(o);                          \
> > > > +                                                                        \
> > > > +        if (i > UINT ## width ## _MAX)                                  \
> > > > +            return ERROR_FAIL;                                          \
> > > 
> > > My guest is this will always be false for uint64 and maybe for uint32.
> > > The value return by get_interger can only be <= LLONG_MAX which I
> > > suppose is still < UINT64_MAX.
> > > 
> > 
> > I was just being lazy about it.
> > 
> > > Also, 'i' might be < 0.
> > > 
> > 
> > Phew, I knew this but somehow I thought it was OK. :-(
> > 
> > So the two things combined, the check should be
> > 
> >    (i > 0 && i < UINT ## width ## _MAX)
> > 
> > What do you think?

0 is also valid ;), and I think UINTX_MAX is valid too,
so (i >= 0 && i <= UINT ## width ## _MAX) should do.
Or:
if (i < 0 || i > UINT ## width ## _MAX)
  return ERROR_FAIL;


> > > If json contain a value > LLONG_MAX, the value will be store as a string
> > > with the type number.
> > > 
> > 
> > This is also true, but I coded like this on purpose. You won't be able
> > to convert that string anyway because that's why it's stored as string
> > in the first place. We should just return ERROR_FAIL in this case, which
> > we do already with the check libxl__json_object_is_integer.
> > 
> 
> Of course with the sole exception of uint64_t parser. This is a special
> case that needs to be handled.

Right.

-- 
Anthony PERARD