> I didn't get a chance to test this yet, but have some initial review
> comments.
>
>> Signed-off-by: Chunyan Liu
>> ---
>> src/libxl/libxl_driver.c | 617
>> ++++++++++++++++++++++++++++++++++++++++++++++
>> src/libxl/libxl_driver.h | 17 ++-
>> 2 files changed, 632 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
>> index d5fa64a..4037635 100644
>> --- a/src/libxl/libxl_driver.c
>> +++ b/src/libxl/libxl_driver.c
>> @@ -30,6 +30,12 @@
>> #include
>> #include
>> #include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>>
>> #include "internal.h"
>> #include "logging.h"
>> @@ -60,6 +66,13 @@
>> #define XEN_SCHED_CREDIT_NPARAM 2
>>
>> static libxlDriverPrivatePtr libxl_driver = NULL;
>> +static virThread migrate_receive_thread;
>>
>
> This prevents receiving concurrent migrations.
Yes. It is a problem. Defined as "static" is to be used in Begin3 function virThreadCreate and in Finish3() function virThreadJoin, but actually the thread will exit when receiving data completed or meets error, so virThreadJoin doesn't have much effect here. If a call of virThreadJoin is not needed, then can specify migrate_receive_thread as a local variable.
>
>> +
>> +typedef struct migrate_receive_args {
>> + virConnectPtr conn;
>> + virDomainObjPtr vm;
>> + int sockfd;
>> +} migrate_receive_args;
>>
>
> If there is a future need to extend this structure, will it cause
> incompatibility issues between a source with the extensions and a
> destination without? Or vise versa?
It will if send logic and receive logic doesn't match. Maybe need to add some extra check, but seems no better way to completely avoid that?
>>
>> /* Function declarations */
>> static int
>> @@ -1095,6 +1108,17 @@ libxlClose(virConnectPtr conn ATTRIBUTE_UNUSED)
>> return 0;
>> }
>>
>> +static int
>> +libxlSupportsFeature(virConnectPtr conn ATTRIBUTE_UNUSED, int feature)
>> +{
>> + switch (feature) {
>> + case VIR_DRV_FEATURE_MIGRATION_V3:
>> + return 1;
>> + default:
>> + return 0;
>> + }
>> +}
>> +
>> static const char *
>> libxlGetType(virConnectPtr conn ATTRIBUTE_UNUSED)
>> {
>> @@ -3842,12 +3866,600 @@ libxlIsAlive(virConnectPtr conn ATTRIBUTE_UNUSED)
>> return 1;
>> }
>>
>> +static int libxlReadFixedMessage(int fd, const void *msg, int msgsz)
>> +{
>> + char buf[msgsz];
>> +
>> + if (saferead(fd, buf, msgsz) != msgsz || memcmp(buf, msg, msgsz)) {
>> + return -1;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int doParseURI(const char *uri, char **p_hostname, int *p_port)
>> +{
>> + char *p, *hostname;
>> + char port[16] = "0";
>> +
>> + if (strstr (uri, "//")) { /* Full URI. */
>> + virURIPtr uriptr = virURIParse(uri);
>> + if (!uriptr) {
>> + libxlError(VIR_ERR_INVALID_ARG,
>> + _("Invalid URI"));
>> + return -1;
>> + }
>> + if (uriptr->scheme && STRCASENEQ(uriptr->scheme, "xlmigr")) {
>> + libxlError(VIR_ERR_INVALID_ARG,
>> + _("Only xlmigr://"
>> + " migrations are supported by Xen"));
>> + xmlFreeURI(uriptr);
>> + return -1;
>> + }
>>
>
> This is just tcp migration. Any reason for requiring the "xlmigr" scheme?
It's not a necessary. Just refer to xen_driver syntax, migration uri could be [xlmigr://]IP[:Port]. We can also define libxl own syntax, migration uri like IP[:Port].
>> + if (!uriptr->server) {
>> + libxlError(VIR_ERR_INVALID_ARG,
>> + _("A hostname must be"
>> + " specified in the URI"));
>> + xmlFreeURI(uriptr);
>> + return -1;
>> + }
>> + hostname = strdup(uriptr->server);
>>
>
> I think it would be better to rework this, and other uses of strdup()
> and snprintf() in this function, to use virAsprintf().
>
>> + if (!hostname) {
>> + virReportOOMError();
>> + xmlFreeURI(uriptr);
>> + return -1;
>> + }
>> + if (uriptr->port)
>> + snprintf(port, sizeof(port), "%d", uriptr->port);
>> + xmlFreeURI(uriptr);
>> + }
>> + else if ((p = strrchr(uri, ':')) != NULL) { /* "hostname:port" */
>> + int port_nr, n;
>> +
>> + if (virStrToLong_i(p+1, NULL, 10, &port_nr) < 0) {
>> + libxlError(VIR_ERR_INVALID_ARG,
>> + _("Invalid port number"));
>> + return -1;
>> + }
>> + snprintf(port, sizeof(port), "%d", port_nr);
>> +
>> + /* Get the hostname. */
>> + n = p - uri; /* n = Length of hostname in bytes. */
>> + hostname = strdup(uri);
>> + if (!hostname) {
>> + virReportOOMError();
>> + return -1;
>> + }
>> + hostname[n] = '\0';
>> + }
>> + else {/* "hostname" (or IP address) */
>> + hostname = strdup(uri);
>> + if (!hostname) {
>> + virReportOOMError();
>> + return -1;
>> + }
>> + }
>> + *p_hostname = hostname;
>> + *p_port = atoi(port);
>> + return 0;
>> +}
>> +
>> +static bool libxlMigrationFromIsAllowed(libxlDriverPrivatePtr driver,
>> virDomainObjPtr vm)
>> +{
>> + /* to be extended */
>> + VIR_DEBUG("driver=%p, vm=%p", driver, vm);
>> + return true;
>> +}
>> +
>> +static bool libxlMigrationToIsAllowed(libxlDriverPrivatePtr driver,
>> virDomainDefPtr def)
>> +{
>> + /* to be extended */
>> + VIR_DEBUG("driver=%p, def=%p", driver, def);
>> + return true;
>> +}
>>
>
> I think these functions (and their call sites) can be added in a future
> patch that provides an implementation.
>
>> +
>> +static char *
>> +libxlDomainMigrateBegin3(virDomainPtr domain,
>> + const char *xmlin,
>> + char **cookieout ATTRIBUTE_UNUSED,
>> + int *cookieoutlen ATTRIBUTE_UNUSED,
>> + unsigned long flags,
>> + const char *dname ATTRIBUTE_UNUSED,
>> + unsigned long resource ATTRIBUTE_UNUSED)
>> +{
>> + libxlDriverPrivatePtr driver = domain->conn->privateData;
>> + virDomainObjPtr vm;
>> + virDomainDefPtr def = NULL;
>> + char *xml = NULL;
>> +
>> + virCheckFlags(LIBXL_MIGRATION_FLAGS, NULL);
>> +
>> + libxlDriverLock(driver);
>> + vm = virDomainFindByUUID(&driver->domains, domain->uuid);
>> + if (!vm) {
>> + char uuidstr[VIR_UUID_STRING_BUFLEN];
>> + virUUIDFormat(domain->uuid, uuidstr);
>> + libxlError(VIR_ERR_OPERATION_INVALID,
>> + _("no domain with matching uuid '%s'"), uuidstr);
>> + goto cleanup;
>> + }
>> +
>> + if (!virDomainObjIsActive(vm)) {
>> + libxlError(VIR_ERR_OPERATION_INVALID,
>> + _("domain is not running"));
>> + goto cleanup;
>> + }
>> +
>> + if (!libxlMigrationFromIsAllowed(driver, vm)) {
>> + libxlError(VIR_ERR_OPERATION_INVALID,
>> + _("domain cannot do migration"));
>> + goto cleanup;
>> + }
>> +
>> + if (xmlin) {
>> + if (!(def = virDomainDefParseString(driver->caps, xml,
>> + 1 << VIR_DOMAIN_VIRT_XEN,
>> + VIR_DOMAIN_XML_INACTIVE)))
>> + goto cleanup;
>> +
>> + xml = virDomainDefFormat(def, VIR_DOMAIN_XML_SECURE);
>>
>
> Do we need to ensure the name provided in xmlin is the same as def->name?
Now that it supports domain name change, I didn't check it. I noticed qemu driver check that, but I couldn't think of any reason that needs to check?
>> + } else {
>> + xml = virDomainDefFormat(vm->def, VIR_DOMAIN_XML_SECURE);
>> + }
>> +
>> +cleanup:
>> + if (vm)
>> + virDomainObjUnlock(vm);
>> + libxlDriverUnlock(driver);
>> + return xml;
>> +}
>> +
>> +static void doMigrateReceive(void *opaque)
>> +{
>> + migrate_receive_args *data = opaque;
>> + virConnectPtr conn = data->conn;
>> + int sockfd = data->sockfd;
>> + virDomainObjPtr vm = data->vm;
>> + libxlDriverPrivatePtr driver = conn->privateData;
>> + int recv_fd;
>> + struct sockaddr_in new_addr;
>> + socklen_t socklen = sizeof(new_addr);
>> + int len;
>> +
>> + do {
>> + recv_fd = accept(sockfd, (struct sockaddr *)&new_addr, &socklen);
>> + } while(recv_fd < 0 && errno == EINTR);
>> +
>> + VIR_DEBUG("Accepted migration\n");
>>
>
> This should be moved after checking for a valid recv_fd.
>
>> + if (recv_fd < 0) {
>> + VIR_DEBUG("Could not accept migration connection\n");
>>
>
> libxlError?
>
>> + goto cleanup;
>> + }
>> +
>> + len = sizeof(migrate_receiver_banner);
>> + if (safewrite(recv_fd, migrate_receiver_banner, len) != len) {
>> + libxlError(VIR_ERR_OPERATION_FAILED,
>> + _("Failed to write migrate_receiver_banner"));
>> + goto cleanup;
>> + }
>> +
>> + if (libxlVmStart(driver, vm, false, recv_fd) < 0) {
>> + libxlError(VIR_ERR_INTERNAL_ERROR,
>> + _("Failed to restore domain with libxenlight"));
>> + if (!vm->persistent) {
>> + virDomainRemoveInactive(&driver->domains, vm);
>> + vm = NULL;
>> + }
>> + goto cleanup;
>> + }
>> +
>> + len = sizeof(migrate_receiver_ready);
>> + if (safewrite(recv_fd, migrate_receiver_ready, len) != len) {
>> + libxlError(VIR_ERR_OPERATION_FAILED,
>> + _("Failed to write migrate_receiver_ready"));
>> + }
>> +
>> +cleanup:
>> + if (VIR_CLOSE(recv_fd) < 0)
>> + virReportSystemError(errno, "%s", _("cannot close recv_fd"));
>> + if (VIR_CLOSE(sockfd) < 0)
>> + virReportSystemError(errno, "%s", _("cannot close sockfd"));
>> + if (vm)
>> + virDomainObjUnlock(vm);
>> + VIR_FREE(opaque);
>> + return;
>> +}
>> +
>> +static int doMigrateSend(virDomainPtr dom, unsigned long flags, int
>> sockfd)
>> +{
>> + libxlDriverPrivatePtr driver = dom->conn->privateData;
>> + virDomainObjPtr vm;
>> + libxlDomainObjPrivatePtr priv;
>> + libxl_domain_suspend_info suspinfo;
>> + virDomainEventPtr event = NULL;
>> + int live = 0;
>> + int ret = -1;
>> +
>> + if (flags & VIR_MIGRATE_LIVE)
>> + live = 1;
>> +
>> + vm = virDomainFindByUUID(&driver->domains, dom->uuid);
>> + if (!vm) {
>> + char uuidstr[VIR_UUID_STRING_BUFLEN];
>> + virUUIDFormat(dom->uuid, uuidstr);
>> + libxlError(VIR_ERR_OPERATION_INVALID,
>> + _("no domain with matching uuid '%s'"),
>> uuidstr);
>> + goto cleanup;
>> + }
>> +
>> + priv = vm->privateData;
>> +
>> + /* read fixed message from dest (ready to receive) */
>> + if (libxlReadFixedMessage(sockfd, migrate_receiver_banner,
>> + sizeof(migrate_receiver_banner))) {
>> + goto cleanup;
>> + }
>> +
>> + /* send suspend data */
>> + memset(&suspinfo, 0, sizeof(suspinfo));
>> + if (live == 1)
>> + suspinfo.flags |= XL_SUSPEND_LIVE;
>> + if (libxl_domain_suspend(&priv->ctx, &suspinfo, vm->def->id, sockfd)
>> != 0) {
>> + libxlError(VIR_ERR_INTERNAL_ERROR,
>> + _("Failed to save domain '%d' with libxenlight"),
>> + vm->def->id);
>> + goto cleanup;
>> + }
>> +
>> + /* read fixed message from dest (receive completed) */
>> + if (libxlReadFixedMessage(sockfd, migrate_receiver_ready,
>> + sizeof(migrate_receiver_ready))) {
>> + /* Src side should be resumed, but for ret < 0, virsh won't call
>> Src side
>> + * Confirm3, handle it here.
>> + */
>> + libxl_domain_resume(&priv->ctx, vm->def->id);
>> + goto cleanup;
>> + }
>> +
>> + ret = 0;
>> +
>> +cleanup:
>> + if (vm)
>> + virDomainObjUnlock(vm);
>> + if (event)
>> + libxlDomainEventQueue(driver, event);
>> + return ret;
>> +}
>> +
>> +static int
>> +libxlDomainMigratePrepare3(virConnectPtr dconn,
>> + const char *cookiein ATTRIBUTE_UNUSED,
>> + int cookieinlen ATTRIBUTE_UNUSED,
>> + char **cookieout ATTRIBUTE_UNUSED,
>> + int *cookieoutlen ATTRIBUTE_UNUSED,
>> + const char *uri_in,
>> + char **uri_out,
>> + unsigned long flags,
>> + const char *dname,
>> + unsigned long resource ATTRIBUTE_UNUSED,
>> + const char *dom_xml)
>> +{
>> + libxlDriverPrivatePtr driver = dconn->privateData;
>> + virDomainDefPtr def = NULL;
>> + virDomainObjPtr vm = NULL;
>> + int port = 0;
>> + int sockfd = -1;
>> + struct sockaddr_in addr;
>> + migrate_receive_args *args;
>> + int ret = -1;
>> +
>> + virCheckFlags(LIBXL_MIGRATION_FLAGS, -1);
>> +
>> + libxlDriverLock(driver);
>> + if (!dom_xml) {
>> + libxlError(VIR_ERR_OPERATION_INVALID,
>> + _("no domain XML passed"));
>> + goto cleanup;
>> + }
>> +
>> + def = virDomainDefParseString(driver->caps, dom_xml,
>> + 1 << VIR_DOMAIN_VIRT_XEN,
>> + VIR_DOMAIN_XML_INACTIVE);
>> +
>> + if (!libxlMigrationToIsAllowed(driver, def)) {
>> + libxlError(VIR_ERR_OPERATION_INVALID,
>> + _("cannot migrate domain to this host"));
>> + goto cleanup;
>> + }
>> +
>> + /* Target domain name, maybe renamed. */
>> + if (dname) {
>> + def->name = strdup(dname);
>>
>
> This leaks the original name.
I didn't save original name as qemu driver does. In a remote migration, seems no place needs the original name again?
>
Thanks,
Chunyan
