Re: [Xen-devel] [PATCH V4 16/24] libxl: copy function for builtin types

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

(Ian J -- discussion about NOGC at the bottom may be of interest)

On Tue, 2014-05-06 at 15:36 +0100, Wei Liu wrote:
> On Tue, May 06, 2014 at 03:03:13PM +0100, Ian Campbell wrote:
> > On Thu, 2014-05-01 at 13:58 +0100, Wei Liu wrote:
> > > These functions will be used in later patch to deep-copy a structure.
> > 
> > Please can you document this in libxl.h alongside _init and _dispose
> > etc. Can you do parse_json and gen_json too (I know the second isn't
> > your mess, it's mine, sorry!).
> 
> No problem. Now this series is one patch longer (I plan to document
> gen_json in a separate patch)! Hope that won't be too inconvenient for
> you. :-)

What's one more patch :-P

> 
> > > diff --git a/tools/libxl/libxl_cpuid.c b/tools/libxl/libxl_cpuid.c
> > > index 7f27c67..aab9d0a 100644
> > > --- a/tools/libxl/libxl_cpuid.c
> > > +++ b/tools/libxl/libxl_cpuid.c
> > > @@ -462,6 +462,39 @@ int 
> > > libxl_cpuid_policy_list_length(libxl_cpuid_policy_list *l)
> > >      return i;
> > >  }
> > >  
> > > +void libxl_cpuid_policy_list_copy(libxl_ctx *ctx,
> > > +                                  libxl_cpuid_policy_list *dst,
> > > +                                  libxl_cpuid_policy_list *src)
> > 
> > Just picking on this one at random, should they return an int even if
> > all the current uses can only return success? Is there any conceivable
> > way for any of these functions to fail (other than enomem which is
> > handled already). e.g. ERROR_INVALID because the input is bogus perhaps.
> 
> Shouldn't the input already be sanatized at this point?

By what?

> Or, does it really matter if the input is bogus? Is it really copy
> function's job to sanitize input? I would expect the user of this struct
> to do the right thing when it encounters bogus input, not the copy
> function.

The caller may not know it is bogus though, it may only come to light
when the copy tries and fails.

Perhaps consider the case where to caller is buggy (so it by definition
doesn't know the input is buggy)? For example what do you do if the
discriminatator of a keyed enum is invalid? I suppose you could do
nothing with the union.

> > For consistency if any one can fail I think they should all return an
> > error code.
> > 
> 
> I can do this. But if we don't sanitize the input here, the only thing
> that can fail is memory allocation, and libxl__calloc calls _exit, we
> cannot return to caller anyway.
> 
> Wei.

Did you see this final comment?:

> > > +void libxl_uuid_copy_ctx(libxl_ctx *ctx, libxl_uuid *dst,
> > > +                         const libxl_uuid *src);
> > 
> > Hrm, this is rather unfortunate.
> > 
> > All of these only take a ctx so they can use NOGC. I wonder if a #define
> > INIT_NOGC which provides a suitable gc (with maxsize == -1) might be
> > nicer than this?
> > 
> > Ian? What do you think?
> > 



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