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

[Changlong Xie <xiecl.fnst@xxxxxxxxxxxxxx>]

On 03/05/2016 12:38 AM, 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.
Ok

> >   /*
> > + * 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 prefer assert(), will fix in next version

Thanks
        -Xie

> Ian.
>
>
> .


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