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:57:59AM +0100, Ian Campbell wrote:
> On Wed, 2014-05-07 at 11:47 +0100, Ian Jackson wrote:
> > Ian Campbell writes ("Re: [PATCH V4 16/24] libxl: copy function for builtin 
> > types"):
> > > On Thu, 2014-05-01 at 13:58 +0100, Wei Liu wrote:
> > > > +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?
> > 
> > The problem with this is that the caller's log functions won't be
> > respected.
> 
> Ah yes, I'd forgotten this.
> 
> > I think it would be better to make all of the copy
> > functions take a ctx.
> 
> Right. I was mostly annoyed by libxl_uuid_copy vs. libxl_uuid_copy_ctx.
> 

So was I. I felt so guilty when I needed to invent a new variant with
ugly name.

> I think we can probably use LIBXL_API_VERSION to add this parameter,
> similar to how we did for libxl_domain_create_restore. That would mean
> libxl_uuid_copy would have to accept ctx==NULL, but it seems unlikely to
> need to log anyway.
> 
> Something like:
> 
>      #define LIBXL_HAVE_UUID_COPY_CTX_PARAM
> 
>         void libxl_uuid_copy(libxl_ctx *ctx, libxl_uuid *dst,
>                              const libxl_uuid *src);
>         
>         #if defined(LIBXL_API_VERSION) && LIBXL_API_VERSION < 0x040400
>         #define libxl_uuid_copy(dst, src) libxl_uuid_copy(NULL, dst, src)
>         #endif
> 

This approach looks reasonable. One question, should I bump
LIBXL_API_VERSION to 0x040500?

> ought to do it I think? Perhaps we should do all of libxl_uuid_* for
> consistency?
> 

I don't think so. Looking at the existing API not every function takes a
ctx. Other functions which don't involve memory allocation should be
fine without a ctx.

Wei.

> > Alternatively we should at least document this restriction, but also
> > then we shouldn't ever call the copy functions from within libxl which
> > (without reading the rest of the series) I think would probably defeat
> > the point.
> 
> I think they are currently all used from xl, but one of my comments
> might change that.
> 
> Ian.

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