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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Tue, 2014-04-15 at 10:19 +0100, Wei Liu wrote:
> On Mon, Apr 14, 2014 at 05:59:57PM +0100, Ian Campbell wrote:
> > > 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?

I guess there are exceptions to every rule ;-)

I guess this is because libxl__object_to_json is a helper and the public
functions are very thin autogenerated wrappers, so it makes sense to
bend the rules a bit.

So I suspect the same applies to your new function, sorry for the noise.

> > 
> > > +        ret = -1;
> > 
> > ERROR_FAIL is more normal, but it might depend on the eventual usage.
> > 
> 
> Any non-zero negative value is OK, I think.

Public libxl functions normally return ERROR_* so please do for
consistency. Even for internal functions it is usually best to use
ERROR_* since the error may well be propagated to the user eventually.

Also we conventionally use "rc" for variables which are going to contain
ERROR_*.

Ian.


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