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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Wed, 2014-05-07 at 11:19 +0100, Wei Liu wrote:
> On Wed, May 07, 2014 at 11:05:14AM +0100, Ian Campbell wrote:
> > (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?
> > 
> 
> By the caller. How come there's unsanatized value in src? If there's,
> then the problem should be addressed by other part of the code base.
> 
> > > 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.
> > 
> 
> Even if the caller is buggy and corrupts the input value, I think the
> consumer in libxl (say, libxl_domain_create_new) should be able to
> handle this. That is, my intent is to fail in actual consumer of the
> structure but not the copy function.

I suppose that makes sense.

> > 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.
> > 
> 
> "keyed enum"? Did you mean "keyed union"?

Yes.

> 
> Then that union is not touched at all. 
> 
> > > > 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?
> > > > 
> > 
> 
> Yes I saw this, but I thought it was addressed to Ian J so I somehow
> skipped it. I think INIT_NOGC macro will do. If both of you agree I
> will write yet another patch for this. :-P
> 
> Wei.
> 



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