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

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Tue, May 06, 2014 at 04:42:20PM +0100, Wei Liu wrote:
> On Tue, May 06, 2014 at 04:14:53PM +0100, Anthony PERARD wrote:
> > 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.
> >
> 
> For your convenient I paste the relevant code snippet here:
> 
> +/* Macro to generate:
> + *  libxl__uint8_parse_json
> + *  libxl__uint16_parse_json
> + *  libxl__uint32_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 < 0 || i > UINT ## width ## _MAX)                         \
> +            return ERROR_FAIL;                                          \
> +                                                                        \
> +        *((uint ## width ## _t *)p) = i;                                \
> +                                                                        \
> +        return 0;                                                       \
> +    }
> +
> +PARSE_UINT(8);
> +PARSE_UINT(16);
> +PARSE_UINT(32);
> +
> +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;
> +
> +        *((uint64_t *)p) = i;
> +    } else {
> +        const char *s;
> +        unsigned long long i;
> +        int saved_errno = errno;
> +
> +        s = libxl__json_object_get_number(o);
> +
> +        errno = 0;
> +        i = strtoull(s, NULL, 10);
> +
> +        if (i == ULLONG_MAX && errno == ERANGE)
> +            return ERROR_FAIL;
> +
> +        errno = saved_errno;
> +        *((uint64_t *)p) = i;
> +    }
> +
> +    return 0;
> +}

That look good now.
Ack on the all patch.

-- 
Anthony PERARD

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