[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



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

 


Rackspace

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