Re: [Xen-devel] [PATCH 04/19] libxl: domain save: API changes for asynchrony

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Ian Campbell writes ("Re: [Xen-devel] [PATCH 04/19] libxl: domain save: API 
changes for asynchrony"):
> On Fri, 2012-06-08 at 18:34 +0100, Ian Jackson wrote:
> > Change the internal and external APIs for domain save (suspend) to be
> > capable of asynchronous operation.  The implementation remains
> > synchronous.  The interfaces surrounding device model saving are still
> > synchronous.
...
> >  * The `suspend_callback' function passed to libxl_domain_save is
> >    never called by the existing implementation in libxl.  Abolish it.
> 
> Furthermore xl never passes one in either.

Right.  I will update the commit message to note this too.

> > Signed-off-by: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> 
> A few minor comments below, but otherwise looks good to me.

Thanks...

> > +static void remus_crashed_cb(libxl__egc *egc,
> > +                             libxl__domain_suspend_state *dss, int rc)
> 
> I'm not sure "crashed" is quite right here, it's finished for whatever
> reason which may not necessarily be a crash (going forward it should
> rarely be a crash, I think). It's "stopped" or "done" or something.

How about "remus_target_gone_cb" ?

> > +int libxl_domain_suspend(libxl_ctx *ctx, uint32_t domid, int fd, int flags,
> > +                         const libxl_asyncop_how *ao_how)

> > +    if (type < 0) {
> > +        LOG(ERROR,"domain %"PRIu32": unable to determine domain type", 
> > domid);
> 
> libxl__domain_type now logs for you.

I will bring forward the relevant hunk from my domain type final
fixups patch.

> >  int libxl_domain_pause(libxl_ctx *ctx, uint32_t domid)
> [...]
> > @@ -1903,10 +1915,27 @@ struct libxl__domain_create_state {
> > 
> >  /*----- Domain suspend (save) functions -----*/
> > 
> > -_hidden int libxl__domain_suspend_common(libxl__gc *gc, uint32_t domid, 
> > int fd,
> > -                                         libxl_domain_type type,
> > -                                         int live, int debug,
> > -                                         const libxl_domain_remus_info 
> > *r_info);
> > +/* calls callback when done */
> 
> Which callback? dss->callback I guess.

What a confusing way to quote this.  You're referring to this function
of course:

> > +_hidden void libxl__domain_suspend(libxl__egc *egc,
> > +                                   libxl__domain_suspend_state *dss);

Gripe gripe these comments before functions are inducing you to
top-post !

Anyway, yes.  I will clarify this comment.

Ian.

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