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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

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. :-)

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

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.

> 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.

> > diff --git a/tools/libxl/libxl_uuid.h b/tools/libxl/libxl_uuid.h
> > index 93c65a7..541f0f8 100644
> > --- a/tools/libxl/libxl_uuid.h
> > +++ b/tools/libxl/libxl_uuid.h
> > @@ -53,10 +53,14 @@ typedef struct {
> >  
> >  #endif
> >  
> > +typedef struct libxl__ctx libxl_ctx;
> > +
> >  int libxl_uuid_is_nil(libxl_uuid *uuid);
> >  void libxl_uuid_generate(libxl_uuid *uuid);
> >  int libxl_uuid_from_string(libxl_uuid *uuid, const char *in);
> >  void libxl_uuid_copy(libxl_uuid *dst, const libxl_uuid *src);
> > +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