[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

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

>  /*
> + *
> + * 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.
> + */

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.


Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.