In libxlDomainMigrationPrepare(), the cleanup label handles error conditions and should be renamed as such for clarity. Signed-off-by: Jim Fehlig <jfehlig(a)suse.com&gt; --- src/libxl/libxl_migration.c | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c index 53d961c..cfbbcd0 100644 --- a/src/libxl/libxl_migration.c +++ b/src/libxl/libxl_migration.c @@ -288,31 +288,31 @@ libxlDomainMigrationPrepare(virConnectPtr dconn, VIR_DOMAIN_OBJ_LIST_ADD_LIVE | VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE, NULL))) - goto cleanup; + goto error; /* Create socket connection to receive migration data */ if (!uri_in) { if ((hostname = virGetHostname()) == NULL) - goto cleanup; + goto error; if (STRPREFIX(hostname, "localhost")) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("hostname on destination resolved to localhost," " but migration requires an FQDN")); - goto cleanup; + goto error; } if (virPortAllocatorAcquire(driver->migrationPorts, &port) < 0) - goto cleanup; + goto error; if (virAsprintf(uri_out, "tcp://%s:%d", hostname, port) < 0) - goto cleanup; + goto error; } else { if (!(STRPREFIX(uri_in, "tcp://"))) { /* not full URI, add prefix tcp:// */ char *tmp; if (virAsprintf(&tmp, "tcp://%s", uri_in) < 0) - goto cleanup; + goto error; uri = virURIParse(tmp); VIR_FREE(tmp); } else { @@ -323,28 +323,28 @@ libxlDomainMigrationPrepare(virConnectPtr dconn, virReportError(VIR_ERR_INVALID_ARG, _("unable to parse URI: %s"), uri_in); - goto cleanup; + goto error; } if (uri->server == NULL) { virReportError(VIR_ERR_INVALID_ARG, _("missing host in migration URI: %s"), uri_in); - goto cleanup; + goto error; } else { hostname = uri->server; } if (uri->port == 0) { if (virPortAllocatorAcquire(driver->migrationPorts, &port) < 0) - goto cleanup; + goto error; } else { port = uri->port; } if (virAsprintf(uri_out, "tcp://%s:%d", hostname, port) < 0) - goto cleanup; + goto error; } snprintf(portstr, sizeof(portstr), "%d", port); @@ -352,14 +352,14 @@ libxlDomainMigrationPrepare(virConnectPtr dconn, if (virNetSocketNewListenTCP(hostname, portstr, &socks, &nsocks) < 0) { virReportError(VIR_ERR_OPERATION_FAILED, "%s", _("Fail to create socket for incoming migration")); - goto cleanup; + goto error; } if (libxlMigrationDstArgsInitialize() < 0) - goto cleanup; + goto error; if (!(args = virObjectNew(libxlMigrationDstArgsClass))) - goto cleanup; + goto error; args->conn = dconn; args->vm = vm; @@ -395,12 +395,12 @@ libxlDomainMigrationPrepare(virConnectPtr dconn, virObjectUnref(args); if (!nsocks_listen) - goto cleanup; + goto error; ret = 0; goto done; - cleanup: + error: for (i = 0; i < nsocks; i++) { virNetSocketClose(socks[i]); virObjectUnref(socks[i]); -- 1.8.4.5

In libxlDomainMigrationPrepare(), a new virDomainObj is created from the incoming domain def and added to the driver's domain list, but never removed if there are subsequent failures during the prepare phase. targethost# virsh list --all sourcehost# virsh migrate --live dom xen+ssh://targethost/system error: operation failed: Fail to create socket for incoming migration. targethost# virsh list --all error: Failed to list domains error: name in virGetDomain must not be NULL After adding code to remove the domain on prepare failure, noticed that libvirtd crashed due to double free of the virDomainDef. Similar to the qemu driver, pass a pointer to virDomainDefPtr so it can be set to NULL once a virDomainObj is created from it. Signed-off-by: Jim Fehlig <jfehlig(a)suse.com&gt; --- src/libxl/libxl_driver.c | 2 +- src/libxl/libxl_migration.c | 10 ++++++++-- src/libxl/libxl_migration.h | 2 +- 3 files changed, 10 insertions(+), 4 deletions(-) diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 9aec78d..b5f8703 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -4435,7 +4435,7 @@ libxlDomainMigratePrepare3Params(virConnectPtr dconn, if (virDomainMigratePrepare3ParamsEnsureACL(dconn, def) < 0) goto error; - if (libxlDomainMigrationPrepare(dconn, def, uri_in, uri_out, flags) < 0) + if (libxlDomainMigrationPrepare(dconn, &def, uri_in, uri_out, flags) < 0) goto error; return 0; diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c index cfbbcd0..334aa85 100644 --- a/src/libxl/libxl_migration.c +++ b/src/libxl/libxl_migration.c @@ -265,7 +265,7 @@ libxlDomainMigrationPrepareDef(libxlDriverPrivatePtr driver, int libxlDomainMigrationPrepare(virConnectPtr dconn, - virDomainDefPtr def, + virDomainDefPtr *def, const char *uri_in, char **uri_out, unsigned int flags) @@ -283,12 +283,13 @@ libxlDomainMigrationPrepare(virConnectPtr dconn, size_t i; int ret = -1; - if (!(vm = virDomainObjListAdd(driver->domains, def, + if (!(vm = virDomainObjListAdd(driver->domains, *def, driver->xmlopt, VIR_DOMAIN_OBJ_LIST_ADD_LIVE | VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE, NULL))) goto error; + *def = NULL; /* Create socket connection to receive migration data */ if (!uri_in) { @@ -405,6 +406,11 @@ libxlDomainMigrationPrepare(virConnectPtr dconn, virNetSocketClose(socks[i]); virObjectUnref(socks[i]); } + /* Remove virDomainObj from domain list */ + if (vm) { + virDomainObjListRemove(driver->domains, vm); + vm = NULL; + } done: virURIFree(uri); diff --git a/src/libxl/libxl_migration.h b/src/libxl/libxl_migration.h index aab96f5..20b45d8 100644 --- a/src/libxl/libxl_migration.h +++ b/src/libxl/libxl_migration.h @@ -50,7 +50,7 @@ libxlDomainMigrationPrepareDef(libxlDriverPrivatePtr driver, int libxlDomainMigrationPrepare(virConnectPtr dconn, - virDomainDefPtr def, + virDomainDefPtr *def, const char *uri_in, char **uri_out, unsigned int flags); -- 1.8.4.5

During migration, the libxl driver starts a modify job in the begin phase, ending the job in the confirm phase. This is essentially VIR_MIGRATE_CHANGE_PROTECTION semantics, but the driver does not support that flag. Without CHANGE_PROTECTION support, the job would never be terminated in error conditions where migrate confirm phase is not executed. Further attempts to modify the domain would result in failure to acquire a job after LIBXL_JOB_WAIT_TIME. Similar to the qemu driver, end the job in the begin phase. Protecting the domain object across all phases of migration can be done in a future patch adding CHANGE_PROTECTION support. Signed-off-by: Jim Fehlig <jfehlig(a)suse.com&gt; --- src/libxl/libxl_driver.c | 5 +---- src/libxl/libxl_migration.c | 16 ++++------------ 2 files changed, 5 insertions(+), 16 deletions(-) diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index b5f8703..b27581e 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -4491,11 +4491,8 @@ libxlDomainMigratePerform3Params(virDomainPtr dom, goto cleanup; if (libxlDomainMigrationPerform(driver, vm, dom_xml, dconnuri, - uri, dname, flags) < 0) { - /* Job terminated and vm unlocked if MigrationPerform failed */ - vm = NULL; + uri, dname, flags) < 0) goto cleanup; - } ret = 0; diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c index 334aa85..ce3f9d5 100644 --- a/src/libxl/libxl_migration.c +++ b/src/libxl/libxl_migration.c @@ -215,6 +215,10 @@ libxlDomainMigrationBegin(virConnectPtr conn, xml = virDomainDefFormat(def, VIR_DOMAIN_XML_SECURE); + endjob: + if (!libxlDomainObjEndJob(driver, vm)) + vm = NULL; + cleanup: if (vm) virObjectUnlock(vm); @@ -222,11 +226,6 @@ libxlDomainMigrationBegin(virConnectPtr conn, virDomainDefFree(tmpdef); virObjectUnref(cfg); return xml; - - endjob: - if (!libxlDomainObjEndJob(driver, vm)) - vm = NULL; - goto cleanup; } virDomainDefPtr @@ -468,11 +467,6 @@ libxlDomainMigrationPerform(libxlDriverPrivatePtr driver, virObjectLock(vm); cleanup: - /* If failure, terminate the job started in MigrationBegin */ - if (ret == -1) { - if (libxlDomainObjEndJob(driver, vm)) - virObjectUnlock(vm); - } VIR_FORCE_CLOSE(sockfd); virURIFree(uri); return ret; @@ -578,8 +572,6 @@ libxlDomainMigrationConfirm(libxlDriverPrivatePtr driver, ret = 0; cleanup: - if (!libxlDomainObjEndJob(driver, vm)) - vm = NULL; if (event) libxlDomainEventQueue(driver, event); if (vm) -- 1.8.4.5

In libxlDomainMigrationConfirm(), a transient domain is removed from the domain list after successful migration. Later in cleanup, the domain object is unlocked, resulting in a crash Program received signal SIGSEGV, Segmentation fault. [Switching to Thread 0x7fb4208ed700 (LWP 12044)] 0x00007fb4267251e6 in virClassIsDerivedFrom (klass=0xdeadbeef, parent=0x7fb42830d0c0) at util/virobject.c:169 169 if (klass->magic == parent->magic) (gdb) bt 0 0x00007fb4267251e6 in virClassIsDerivedFrom (klass=0xdeadbeef, parent=0x7fb42830d0c0) at util/virobject.c:169 1 0x00007fb42672591b in virObjectIsClass (anyobj=0x7fb4100082b0, klass=0x7fb42830d0c0) at util/virobject.c:365 2 0x00007fb42672583c in virObjectUnlock (anyobj=0x7fb4100082b0) at util/virobject.c:338 3 0x00007fb41a8c7d7a in libxlDomainMigrationConfirm (driver=0x7fb4100404c0, vm=0x7fb4100082b0, flags=1, cancelled=0) at libxl/libxl_migration.c:583 Fix by setting the virDomainObjPtr to NULL after removing it from the domain list. Signed-off-by: Jim Fehlig <jfehlig(a)suse.com&gt; --- src/libxl/libxl_migration.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c index ce3f9d5..dbb5a8f 100644 --- a/src/libxl/libxl_migration.c +++ b/src/libxl/libxl_migration.c @@ -566,8 +566,10 @@ libxlDomainMigrationConfirm(libxlDriverPrivatePtr driver, if (flags & VIR_MIGRATE_UNDEFINE_SOURCE) virDomainDeleteConfig(cfg->configDir, cfg->autostartDir, vm); - if (!vm->persistent || (flags & VIR_MIGRATE_UNDEFINE_SOURCE)) + if (!vm->persistent || (flags & VIR_MIGRATE_UNDEFINE_SOURCE)) { virDomainObjListRemove(driver->domains, vm); + vm = NULL; + } ret = 0; -- 1.8.4.5