Re: [Xen-devel] [PATCH v11 03/27] tools/libxl: Add back channel to allow migration target send data back

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Fri, Mar 04, 2016 at 04:38:23PM +0000, Ian Jackson wrote:
> Changlong Xie writes ("[PATCH v11 03/27] tools/libxl: Add back channel to 
> allow migration target send data back"):
> > From: Wen Congyang <wency@xxxxxxxxxxxxxx>
> > 
> > In COLO mode, secondary needs to send the following data to primary:
> > 1. In libxl
> >    Secondary sends the following CHECKPOINT_CONTEXT to primary:
> >    CHECKPOINT_SVM_SUSPENDED, CHECKPOINT_SVM_READY and CHECKPOINT_SVM_RESUMED
> > 2. In libxc
> >    Secondary sends the dirty pfn list to primary
> 
> The overall API approach here seems good to me.
> 
> But, my reading of the code is that this new fd is currently ignored.
> This is, AFAICT, intentional in the non-colo case, and we have no colo
> case yet.
> 
> So I think that this new parameter needs to be slightly better
> documented.  I suggest:
> 
>  * In this patch, add a comment next to it saying "always pass -1".
>  * In the patch were the fd actually starts to do something, change
>     this comment to something more meaningful.
> 
> >  /*
> > + * LIBXL_HAVE_DOMAIN_CREATE_RESTORE_SEND_BACK_FD 1
> > + *
> > + * If this is defined, libxl_domain_create_restore()'s API has changed to
> > + * include a send_back_fd param which used for libxl migration back channel
> > + * during COLO.
> > + */
> > +#define LIBXL_HAVE_DOMAIN_CREATE_RESTORE_SEND_BACK_FD 1
> 
> I have a minor grammar quibble with this.  I would write:
> 
>   "If this is defined, libxl_domain_create_restore()'s API
>    includes the send_back_fd param.  This is used only with
>    COLO, for the libxl migration back channel; other callers
>    should pass -1."
> 
> And, with this definition of the API, I think that the code should
> actually check that -1 is passed.  Personally I would be happy with
> the error case either failing assert() or returning ERROR_INVAL, but
> maybe other maintainers have a specific view.
> 

I have no preference on this issue. Either calling assert or
returning ERROR_INVAL is fine by me.

Wei.

> Ian.

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