Re: [Xen-devel] [PATCH RFC 2/9] libxl_json: introduce libx__object_from_json

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Mon, Apr 14, 2014 at 05:59:57PM +0100, Ian Campbell wrote:
> On Thu, 2014-04-10 at 16:40 +0100, Wei Liu wrote:
> > Given a JSON string, we need to convert it to libxl_FOO struct.
> > 
> > The approach is:
> > JSON string -> libxl__json_object -> libxl_FOO struct
> > 
> > With this approach we can make use of libxl's infrastructure to do the
> > first half (JSON string -> libxl__json_object).
> > 
> > Second half is done by auto-generated code by libxl's IDL
> > infrastructure. IDL patch(es) will come later.
> > 
> > Signed-off-by: Wei Liu <wei.liu2@xxxxxxxxxx>
> > ---
> >  tools/libxl/libxl_internal.h |    9 +++++++++
> >  tools/libxl/libxl_json.c     |   34 ++++++++++++++++++++++++++++++++++
> >  2 files changed, 43 insertions(+)
> > 
> > diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> > index b3a200d..34b6a26 100644
> > --- a/tools/libxl/libxl_internal.h
> > +++ b/tools/libxl/libxl_internal.h
> > @@ -1494,6 +1494,15 @@ typedef yajl_gen_status 
> > (*libxl__gen_json_callback)(yajl_gen hand, void *);
> >  _hidden char *libxl__object_to_json(libxl_ctx *ctx, const char *type,
> >                                      libxl__gen_json_callback gen, void *p);
> >  
> > +typedef struct libxl__json_object libxl__json_object;
> > +typedef int (*libxl__parse_json_callback)(libxl_ctx *ctx,
> > +                                          libxl__json_object *o,
> > +                                          void *p);
> 
> libxl__json_parse_callback reads slightly more naturally to me and fits
> in with the existing libxl__json_* 
> 

OK.

> > +_hidden int libxl__object_from_json(libxl_ctx *ctx, const char *type,
> > +                                    libxl__parse_json_callback parse,
> > +                                    void *p,
> > +                                    const char *s);
> > +
> >    /* holds the CPUID response for a single CPUID leaf
> >     * input contains the value of the EAX and ECX register,
> >     * and each policy string contains a filter to apply to
> > diff --git a/tools/libxl/libxl_json.c b/tools/libxl/libxl_json.c
> > index 3ea56a4..002ae2d 100644
> > --- a/tools/libxl/libxl_json.c
> > +++ b/tools/libxl/libxl_json.c
> > @@ -794,6 +794,40 @@ out:
> >      return ret;
> >  }
> >  
> > +int libxl__object_from_json(libxl_ctx *ctx, const char *type,
> 
> Internal functions should take libxl__gc *gc and avoid all the GC_INIT,
> GC_FREE stuff. You can still access ctx but as CTX (a macro which uses
> gc)
> 

Huh? But libxl__object_to_json also takes a libxl_ctx, am I missing
something?

> > +                            libxl__parse_json_callback parse,
> > +                            void *p, const char *s)
> > +{
> > +    GC_INIT(ctx);
> > +    libxl__json_object *o = NULL;
> 
> Unnecessary initialiser.
> 
> > +    int ret;
> > +
> > +    o = libxl__json_parse(gc, s);
> > +
> 
> Drop this blank line to cuddle the error check up with the function.
> 
> > +    if (!o) {
> > +        LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
> > +                   "unable to generate libxl__json_object from JSON 
> > representation for %s.",
> > +                   type);
> 
> You can use the LOG macro here.
> 

Just following the coding style in libxl__object_to_json. But I guess
LOG is the new convention to follow. I will change this.

> I think you want "representation of %s"

Yes.

> 
> > +        ret = -1;
> 
> ERROR_FAIL is more normal, but it might depend on the eventual usage.
> 

Any non-zero negative value is OK, I think.

Wei.

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