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.

> + > +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?

> /* 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?

> + 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?

> + } 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.