Re: [Xen-devel] [PATCH 12/13] libxl: New API for providing OS events to libxl

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Ian Campbell writes ("Re: [Xen-devel] [PATCH 12/13] libxl: New API for 
providing OS events to libxl"):
> On Mon, 2011-11-28 at 16:59 +0000, Ian Jackson wrote:
> > @@ -89,10 +99,25 @@ int libxl_ctx_alloc(libxl_ctx **pctx, int version,
> > 
> >  int libxl_ctx_free(libxl_ctx *ctx)
> >  {
> > +    int i;
> > +    GC_INIT(ctx);
> 
> You never call GC_FREE in this function. It's a bit of an odd one to
> need in the free function I guess but I think you still need it after
> the last call to a function which takes a gc argument?

Yes, you are right.

Also this function failed to free(ctx), so I have added (in my tree)
another patch to this series to do that.

> > +    for (i = 0; i < ctx->watch_nslots; i++)
> > +        assert(!libxl__watch_slot_contents(gc, i));
> > +    libxl__ev_fd_deregister(gc, &ctx->watch_efd);
> > +
> > +    assert(LIBXL_LIST_EMPTY(&ctx->efds));
> > +    assert(LIBXL_TAILQ_EMPTY(&ctx->etimes));
> 
> Am I right that libxl__ev_fd_deregister is what ensures this is the
> case? If not then who is responsible for unregistering akk events before
> freeing a context?

libxl is supposed to do this.  In this patch, the only consumer of
events is the watch machinery.  But in the next patch, consumers
inside libxl of events are supposed to deregister them in
libxl_ctx_free.

I have added some comments to this effect in this and the subsequent
patch.

Also, while looking at this area it turns out that I wasn't doing this
deregistration properly for disk events, and that libxl_ctx_free
leaked events which had occurred but not been reported.  I have fixed
these too.

Ian.

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