From mboxrd@z Thu Jan 1 00:00:00 1970 From: Wei Liu Subject: Re: [PATCH RFC 2/9] libxl_json: introduce libx__object_from_json Date: Tue, 15 Apr 2014 10:19:50 +0100 Message-ID: <20140415091950.GG3635@zion.uk.xensource.com> References: <1397144422-18906-1-git-send-email-wei.liu2@citrix.com> <1397144422-18906-4-git-send-email-wei.liu2@citrix.com> <1397494797.7802.25.camel@dagon.hellion.org.uk> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <1397494797.7802.25.camel@dagon.hellion.org.uk> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell Cc: ian.jackson@eu.citrix.com, Wei Liu , xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org 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 > > --- > > 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.