Re: [Xen-devel] [PATCH 1/2] libxc: introduce XC_SAVE_ID_TOOLSTACK

[Tim Deegan <tim@xxxxxxx>]

At 12:32 +0000 on 20 Jan (1327062730), Ian Campbell wrote:
> On Fri, 2012-01-20 at 12:24 +0000, Stefano Stabellini wrote:
> > On Fri, 20 Jan 2012, Ian Campbell wrote:
> > > On Fri, 2012-01-20 at 11:18 +0000, Stefano Stabellini wrote:
> > > > diff --git a/tools/libxc/xc_domain_save.c b/tools/libxc/xc_domain_save.c
> > > > index a6bb894..3d11c39 100644
> > > > --- a/tools/libxc/xc_domain_save.c
> > > > +++ b/tools/libxc/xc_domain_save.c
> > > > @@ -1676,6 +1676,22 @@ int xc_domain_save(xc_interface *xch, int io_fd, 
> > > > uint32_t dom, uint32_t max_iter
> > > >          }
> > > >      }
> > > > 
> > > > +    if ( callbacks != NULL && callbacks->toolstack_save != NULL )
> > > > +    {
> > > > +        int id = XC_SAVE_ID_TOOLSTACK;
> > > > +        uint8_t *buf;
> > > > +        uint32_t len;
> > > > +
> > > > +        if ( callbacks->toolstack_save(dom, &buf, &len, 
> > > > callbacks->data) < 0 )
> > > > +        {
> > > > +            PERROR("Error calling toolstack_save");
> > > > +            goto out;
> > > > +        }
> > > > +        wrexact(io_fd, &id, sizeof(id));
> > > > +        wrexact(io_fd, &len, sizeof(len));
> > > > +        wrexact(io_fd, buf, len);
> > > 
> > > Where is buf free'd? You say below "callee allocates and frees the
> > > buffer" but there is no way that can be the case since the caller uses
> > > the buffer after the callback.
> > 
> > The callee can free the buffer after the completion of xc_domain_save.
> > So libxl allocates the buffer in toolstack_save and frees it after
> > xc_domain_save returns.
> > 
> > 
> > > I think it has to be callee-alloc, caller-free (perhaps callee-free on
> > > error).
> > 
> > I though that it is more straightforward if the callee does both, but if
> > you think that is actually more confusing, I'll change it.
> 
> Well, it enforces that the caller has some sort of infrastructure (like
> gc) which they may not have. I suppose they could have a little struct
> passed via the closure where they stash the allocation to free after
> xc_domain_save but I'd contend that this is even weirder/more confusing
> than doing caller-free -- you just don't see this because libxl's gc
> hides it from you.

Other idioms for this sort of thing include having two callbacks, the
first of which returns the size (erk), or two callbacks, one just for
freeing the pointer (meh).  On *NIx, requring that the callback return a
pointer suitable for free()ing is OK, I think.  On Windows that kind of
thing can be a real PITA, trying to be sure that both sides have the
same heap.   But maybe that's a bridge better crossed when we come to
it. 

Tim.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel