[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH RFC V2 05/10] libxl_json: introduce parser functions for builtin types
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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |