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

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

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.

Ian.

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