Re: [PATCH 1/6] libxl: libxl_device_disk_local_attach return a new libxl_device_disk

[Stefano Stabellini <stefano.stabellini@eu.citrix.com>]

On Tue, 27 Mar 2012, Ian Campbell wrote:
> On Tue, 2012-03-27 at 14:59 +0100, Stefano Stabellini wrote:
> > The caller passes an additional pre-allocated libxl_device_disk struct
> > to libxl_device_disk_local_attach, that it is going to fill it with
> > informations about the new locally attached disk.
> > The new libxl_device_disk should be passed to
> > libxl_device_disk_local_detach afterwards.
> > 
> > This patch is just about adding the new parameter and updating the
> > caller.
> > 
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > ---
> >  tools/libxl/libxl.c            |   46 ++++++++++++++++++++++++++--------------
> >  tools/libxl/libxl.h            |    3 +-
> >  tools/libxl/libxl_bootloader.c |    9 +++++--
> >  3 files changed, 38 insertions(+), 20 deletions(-)
> > 
> > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> > index 5344366..d33fbdf 100644
> > --- a/tools/libxl/libxl.c
> > +++ b/tools/libxl/libxl.c
> > @@ -1644,63 +1644,77 @@ out:
> >      return ret;
> >  }
> >  
> > -char * libxl_device_disk_local_attach(libxl_ctx *ctx, libxl_device_disk *disk)
> > +char * libxl_device_disk_local_attach(libxl_ctx *ctx, const libxl_device_disk *disk,
> > +        libxl_device_disk **new_disk)
> 
> I think this would be better as a caller allocated libxl_device_disk *.
> That's much simpler since the caller can just put it on the stack or in
> an existing datastructure (I expect a suitable one comes into being with
> IanJ's async patch series).
>
> Do we need this as a public facing API?
> Does anything use it? I think it
> should be an implementation detail of libxl_device_disk_add where domid
> == SELF? That would also allow you to use the gc here.

Yes, you are right, it is better as an internal function. In that
case we can just as easily allocate the new disk on the gc because the
caller doesn't need to care about deallocating it.

I am not sure about making local_attach an implementation detail of
libxl_device_disk, especially considering that it is not yet clear how
to get "SELF" right now so I would avoid doing it in this patch.