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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

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.

> 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"?

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