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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Thu, 2012-06-14 at 16:26 +0100, Ian Jackson wrote:
> 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" ?

Sounds fine to me, unless you want to work the word "failover" into it?

> > >  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.

Yes, sorry.

>   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.

Thanks.

Ian.



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