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 <we...@cn.fujitsu.com>
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@lists.xen.org
http://lists.xen.org/xen-devel
