Re: [Xen-devel] [PATCH RFC V2 05/10] libxl_json: introduce parser functions for builtin types

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Wed, 2014-04-23 at 11:01 +0100, Wei Liu wrote:

> The aformentioned "calloc" falls into #1 but not this one.
> 
> My comment is a bit confusing. The comment here actually refers to all
> the malloc'ed memory in the loop, which will be freed by
> libxl_cpuid_dispose.

You mean when the caller eventually gets rid of the result, or the error
path here does it?

I don't think that needs an explicit comment, it's just the expected
behaviour of #1 type allocations.

> > I wonder if it is too late to change the json representation of a
> > bitmask that we are using. Thoughts?
> > 
> 
> Probably yes. It's a public function in libxl API so I wouldn't be
> surprised if some high level tool depends on it.

True, oh well.
> > 
> > I think we may need to be mindful of potentially malicious json code
> > which a toolstack is feeding to these parser functions, don't we?
> > 
> > IOW asserting and crashing the entire daemon would be unfortunate.
> > 
> 
> I think "return -1" will be more appropriate in this case.

return ERROR_* please.

> > > +
> > > +    return 0;
> > > +}
> > > +
> > >  yajl_gen_status libxl_string_list_gen_json(yajl_gen hand, 
> > > libxl_string_list *pl)
> > >  {
> > >      libxl_string_list l = *pl;
> > > @@ -201,6 +343,27 @@ out:
> > >      return s;
> > >  }
> > >  
> > > +int libxl_hwcap_parse_json(libxl_ctx *ctx, const libxl__json_object *o,
> > > +                           libxl_hwcap *p)
> > > +{
> > > +    int i;
> > > +
> > > +    if (!libxl__json_object_is_array(o))
> > > +        return -1;
> > 
> > I jut noticed the error handling here, but I'm sure it was the same for
> > all the preceding. libxl functions should return an ERROR_* code and not
> > -1 unless you have a good reason not to.
> > 
> 
> OK. Will change to that.

Thanks.



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