Re: [Xen-devel] [PATCH 9/9] libxl: Convert to asynchronous: device removal

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Ian Campbell writes ("Re: [Xen-devel] [PATCH 9/9] libxl: Convert to 
asynchronous: device removal"):
> On Fri, 2012-01-13 at 19:25 +0000, Ian Jackson wrote:
> > Convert libxl_FOO_device_remove, and the function which does the bulk
> > of the work, libxl__device_remove, to the new async ops scheme.
...
> 
> After all the internal complexity the actual usage is refreshingly
> simple. Phew!

Yes, that was my main aim ...

> >  int libxl_device_disk_destroy(libxl_ctx *ctx, uint32_t domid,
> > @@ -1536,11 +1540,11 @@ int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t 
> > domid, libxl_device_disk *disk)
> >  out:
> 
> The async capability here ought to be propagated to the
> libxl_cdrom_insert interface too. I guess that would mean handling
> compound asynchronous operations.

Yes.  Compound asynchronous operations are already supported by the
current scheme: the code which implements them just sets up an
appropriate series of callbacks.

> > +int libxl__initiate_device_remove(libxl__ao *ao, libxl__device *dev)
> >  {
> > +    /* Arranges that dev will be removed from its guest.  When
> > +     * this is done, the ao will be completed.  An error
> > +     * return from libxl__device_remove means that the ao
> > +     * will _not_ be completed and the caller must do so.
> 
> Do you mean aborted or cancelled rather than completed here?

No.  An ao cannot be aborted or cancelled, only completed (perhaps
with an error code).

I see that the comment is missing "initiate_" from the function name,
which I will fix.

> > diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> > index b7f0f54..9920fb9 100644
> > --- a/tools/libxl/libxl_internal.h
> > +++ b/tools/libxl/libxl_internal.h
> > @@ -653,35 +653,15 @@ _hidden char *libxl__device_backend_path(libxl__gc 
> > *gc, libxl__device *device);
> [...]
> > +/* Arranges that dev will be removed from its guest.  When
> > + * this is done, the ao will be completed.  An error
> > + * return from libxl__device_remove means that the ao
> > + * will _not_ be completed and the caller must do so. */
> 
> This is the same comment as at the head of the implementation so the
> same comment re aborting applies. Do we need both comments?

No.  The comment should be in the header file only.  I'll fix this.

> > -    if (libxl_device_nic_remove(ctx, domid, &nic)) {
> > +    if (libxl_device_nic_remove(ctx, domid, &nic, 0)) {
...
> 
> There aren't actually any examples of a caller using the ao-ness in xl
> are there?

No.

> I know that sync is for the most part ao+wait but I'm a bit concerned
> that e.g. several of the paths in libxl__ao_complete probably haven't
> been run and one of the flow-charts added to tools/libxl/libxl_event.c
> in patch 6/8 has probably never happened either.

Yes.

> IMHO this isn't a blocker for this patch but since xl is, in addition to
> being a toolstack, a testbed for libxl perhaps one or more "gratuitously
> asynchronous" calls could be made? Perhaps the libxl_cdrom_insert case
> would be an interesting one? In particular the case in the
> create_domain() event loop (e.g. so that the response to a cdrom eject
> does not block shutdown processing).

That would be definitely worthwhile.  I'll put it (mentally) on my
todo list.

Ian.

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