[libvirt] [PATCH 0/9] Many migration enhancements & bug fixes

This is a series containing all my pending migration related enhancements and bugfixes. It includes these previous patches to change the migration v3 protocol API + ABI http://www.redhat.com/archives/libvir-list/2011-May/msg01243.html Patch 9 also changes the v3 protocol API/ABI to deal with an error reporting problem I discovered in testing.

This extends the v3 migration protocol such that the virDomainMigrateBegin3 and virDomainMigratePerform3 methods accept an application supplied XML config for the target VM. If the 'xmlin' parameter is NULL, then Begin3 uses the current guest XML as normal. A driver implementing the Begin3 method should either reject all non-NULL 'xmlin' parameters, or strictly validate that the app supplied XML does not change guest ABI. The Perform3 method also needed the xmlin parameter to cope with the Peer2Peer migration sequence. NB it is not yet possible to use this capability since neither of the public virDomainMigrate/virDomainMigrateToURI methods have a way to pass in XML. * daemon/remote.c, src/remote/remote_driver.c, src/remote/remote_protocol.x, src/remote_protocol-structs: Add 'remote_string xmlin' parameter to begin3/perform3 RPC messages * src/libvirt.c, src/driver.h, src/libvirt_internal.h: Add 'const char *xmlin' parameter to Begin3/Perform3 methods * src/qemu/qemu_driver.c, src/qemu/qemu_migration.c, src/qemu/qemu_migration.h: Pass xmlin parameter around migration methods --- daemon/remote.c | 8 +++++- src/driver.h | 2 + src/libvirt.c | 49 ++++++++++++++++++++++++++++++----------- src/libvirt_internal.h | 2 + src/qemu/qemu_driver.c | 8 ++++-- src/qemu/qemu_migration.c | 17 ++++++++++++-- src/qemu/qemu_migration.h | 2 + src/remote/remote_driver.c | 4 +++ src/remote/remote_protocol.x | 2 + src/remote_protocol-structs | 2 + 10 files changed, 75 insertions(+), 21 deletions(-) diff --git a/daemon/remote.c b/daemon/remote.c index 80783b3..60f01b4 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -3128,6 +3128,7 @@ remoteDispatchDomainMigrateBegin3(struct qemud_server *server ATTRIBUTE_UNUSED, char *xml = NULL; virDomainPtr dom = NULL; char *dname; + char *xmlin; char *cookieout = NULL; int cookieoutlen = 0; int rv = -1; @@ -3140,9 +3141,10 @@ remoteDispatchDomainMigrateBegin3(struct qemud_server *server ATTRIBUTE_UNUSED, if (!(dom = get_nonnull_domain(conn, args->dom))) goto cleanup; + xmlin = args->xmlin == NULL ? NULL : *args->xmlin; dname = args->dname == NULL ? NULL : *args->dname; - if (!(xml = virDomainMigrateBegin3(dom, + if (!(xml = virDomainMigrateBegin3(dom, xmlin, &cookieout, &cookieoutlen, args->flags, dname, args->resource))) goto cleanup; @@ -3288,6 +3290,7 @@ remoteDispatchDomainMigratePerform3(struct qemud_server *server ATTRIBUTE_UNUSED remote_domain_migrate_perform3_ret *ret) { virDomainPtr dom = NULL; + char *xmlin; char *dname; char *cookieout = NULL; int cookieoutlen = 0; @@ -3301,9 +3304,10 @@ remoteDispatchDomainMigratePerform3(struct qemud_server *server ATTRIBUTE_UNUSED if (!(dom = get_nonnull_domain(conn, args->dom))) goto cleanup; + xmlin = args->xmlin == NULL ? NULL : *args->xmlin; dname = args->dname == NULL ? NULL : *args->dname; - if (virDomainMigratePerform3(dom, + if (virDomainMigratePerform3(dom, xmlin, args->cookie_in.cookie_in_val, args->cookie_in.cookie_in_len, &cookieout, &cookieoutlen, diff --git a/src/driver.h b/src/driver.h index 450dd53..a1468e8 100644 --- a/src/driver.h +++ b/src/driver.h @@ -538,6 +538,7 @@ typedef int typedef char * (*virDrvDomainMigrateBegin3) (virDomainPtr domain, + const char *xmlin, char **cookieout, int *cookieoutlen, unsigned long flags, @@ -575,6 +576,7 @@ typedef int typedef int (*virDrvDomainMigratePerform3) (virDomainPtr dom, + const char *xmlin, const char *cookiein, int cookieinlen, char **cookieout, diff --git a/src/libvirt.c b/src/libvirt.c index ff16c48..7b7323e 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -3719,6 +3719,7 @@ finish: static virDomainPtr virDomainMigrateVersion3(virDomainPtr domain, virConnectPtr dconn, + const char *xmlin, unsigned long flags, const char *dname, const char *uri, @@ -3748,8 +3749,8 @@ virDomainMigrateVersion3(virDomainPtr domain, VIR_DEBUG("Begin3 %p", domain->conn); dom_xml = domain->conn->driver->domainMigrateBegin3 - (domain, &cookieout, &cookieoutlen, flags, dname, - bandwidth); + (domain, xmlin, &cookieout, &cookieoutlen, + flags, dname, bandwidth); if (!dom_xml) goto done; @@ -3792,7 +3793,8 @@ virDomainMigrateVersion3(virDomainPtr domain, cookieout = NULL; cookieoutlen = 0; ret = domain->conn->driver->domainMigratePerform3 - (domain, cookiein, cookieinlen, &cookieout, &cookieoutlen, + (domain, NULL, cookiein, cookieinlen, + &cookieout, &cookieoutlen, uri, flags, dname, bandwidth); /* Perform failed. Make sure Finish doesn't overwrite the error */ @@ -3872,6 +3874,7 @@ finish: */ static int virDomainMigratePeer2Peer (virDomainPtr domain, + const char *xmlin, unsigned long flags, const char *dname, const char *uri, @@ -3907,6 +3910,7 @@ virDomainMigratePeer2Peer (virDomainPtr domain, VIR_DRV_FEATURE_MIGRATION_V3)) { VIR_DEBUG("Using migration protocol 3"); return domain->conn->driver->domainMigratePerform3(domain, + xmlin, NULL, /* cookiein */ 0, /* cookieinlen */ NULL, /* cookieoutlen */ @@ -3917,6 +3921,11 @@ virDomainMigratePeer2Peer (virDomainPtr domain, bandwidth); } else { VIR_DEBUG("Using migration protocol 2"); + if (xmlin) { + virLibConnError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to change target guest XML during migration")); + return -1; + } return domain->conn->driver->domainMigratePerform(domain, NULL, /* cookie */ 0, /* cookielen */ @@ -3941,6 +3950,7 @@ virDomainMigratePeer2Peer (virDomainPtr domain, */ static int virDomainMigrateDirect (virDomainPtr domain, + const char *xmlin, unsigned long flags, const char *dname, const char *uri, @@ -3959,6 +3969,7 @@ virDomainMigrateDirect (virDomainPtr domain, VIR_DRV_FEATURE_MIGRATION_V3)) { VIR_DEBUG("Using migration protocol 3"); return domain->conn->driver->domainMigratePerform3(domain, + xmlin, NULL, /* cookiein */ 0, /* cookieinlen */ NULL, /* cookieoutlen */ @@ -3969,6 +3980,11 @@ virDomainMigrateDirect (virDomainPtr domain, bandwidth); } else { VIR_DEBUG("Using migration protocol 2"); + if (xmlin) { + virLibConnError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to change target guest XML during migration")); + return -1; + } return domain->conn->driver->domainMigratePerform(domain, NULL, /* cookie */ 0, /* cookielen */ @@ -4093,7 +4109,8 @@ virDomainMigrate (virDomainPtr domain, } VIR_DEBUG("Using peer2peer migration"); - if (virDomainMigratePeer2Peer(domain, flags, dname, uri ? uri : dstURI, bandwidth) < 0) { + if (virDomainMigratePeer2Peer(domain, NULL, flags, dname, + uri ? uri : dstURI, bandwidth) < 0) { VIR_FREE(dstURI); goto error; } @@ -4118,7 +4135,7 @@ virDomainMigrate (virDomainPtr domain, VIR_DRV_SUPPORTS_FEATURE(dconn->driver, dconn, VIR_DRV_FEATURE_MIGRATION_V3)) { VIR_DEBUG("Using migration protocol 3"); - ddomain = virDomainMigrateVersion3(domain, dconn, flags, dname, uri, bandwidth); + ddomain = virDomainMigrateVersion3(domain, dconn, NULL, flags, dname, uri, bandwidth); } else if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, VIR_DRV_FEATURE_MIGRATION_V2) && VIR_DRV_SUPPORTS_FEATURE(dconn->driver, dconn, @@ -4238,7 +4255,8 @@ virDomainMigrateToURI (virDomainPtr domain, if (VIR_DRV_SUPPORTS_FEATURE (domain->conn->driver, domain->conn, VIR_DRV_FEATURE_MIGRATION_P2P)) { VIR_DEBUG("Using peer2peer migration"); - if (virDomainMigratePeer2Peer (domain, flags, dname, duri, bandwidth) < 0) + if (virDomainMigratePeer2Peer(domain, NULL, flags, + dname, duri, bandwidth) < 0) goto error; } else { /* No peer to peer migration supported */ @@ -4249,7 +4267,8 @@ virDomainMigrateToURI (virDomainPtr domain, if (VIR_DRV_SUPPORTS_FEATURE (domain->conn->driver, domain->conn, VIR_DRV_FEATURE_MIGRATION_DIRECT)) { VIR_DEBUG("Using direct migration"); - if (virDomainMigrateDirect (domain, flags, dname, duri, bandwidth) < 0) + if (virDomainMigrateDirect(domain, NULL, flags, + dname, duri, bandwidth) < 0) goto error; } else { /* Cannot do a migration with only the perform step */ @@ -4567,6 +4586,7 @@ error: */ char * virDomainMigrateBegin3(virDomainPtr domain, + const char *xmlin, char **cookieout, int *cookieoutlen, unsigned long flags, @@ -4575,9 +4595,9 @@ virDomainMigrateBegin3(virDomainPtr domain, { virConnectPtr conn; - VIR_DOMAIN_DEBUG(domain, "cookieout=%p, cookieoutlen=%p, " + VIR_DOMAIN_DEBUG(domain, "xmlin=%s cookieout=%p, cookieoutlen=%p, " "flags=%lu, dname=%s, bandwidth=%lu", - cookieout, cookieoutlen, flags, + NULLSTR(xmlin), cookieout, cookieoutlen, flags, NULLSTR(dname), bandwidth); virResetLastError(); @@ -4596,7 +4616,7 @@ virDomainMigrateBegin3(virDomainPtr domain, if (conn->driver->domainMigrateBegin3) { char *xml; - xml = conn->driver->domainMigrateBegin3(domain, + xml = conn->driver->domainMigrateBegin3(domain, xmlin, cookieout, cookieoutlen, flags, dname, bandwidth); VIR_DEBUG("xml %s", NULLSTR(xml)); @@ -4733,6 +4753,7 @@ error: */ int virDomainMigratePerform3(virDomainPtr domain, + const char *xmlin, const char *cookiein, int cookieinlen, char **cookieout, @@ -4744,9 +4765,11 @@ virDomainMigratePerform3(virDomainPtr domain, { virConnectPtr conn; - VIR_DOMAIN_DEBUG(domain, "cookiein=%p, cookieinlen=%d, cookieout=%p, cookieoutlen=%p," + VIR_DOMAIN_DEBUG(domain, "xmlin=%s cookiein=%p, cookieinlen=%d, " + "cookieout=%p, cookieoutlen=%p, " "uri=%s, flags=%lu, dname=%s, bandwidth=%lu", - cookiein, cookieinlen, cookieout, cookieoutlen, + NULLSTR(xmlin), cookiein, cookieinlen, + cookieout, cookieoutlen, uri, flags, NULLSTR(dname), bandwidth); virResetLastError(); @@ -4765,7 +4788,7 @@ virDomainMigratePerform3(virDomainPtr domain, if (conn->driver->domainMigratePerform3) { int ret; - ret = conn->driver->domainMigratePerform3(domain, + ret = conn->driver->domainMigratePerform3(domain, xmlin, cookiein, cookieinlen, cookieout, cookieoutlen, uri, diff --git a/src/libvirt_internal.h b/src/libvirt_internal.h index 81d0c56..c7c1932 100644 --- a/src/libvirt_internal.h +++ b/src/libvirt_internal.h @@ -124,6 +124,7 @@ int virDomainMigratePrepareTunnel(virConnectPtr dconn, char *virDomainMigrateBegin3(virDomainPtr domain, + const char *xmlin, char **cookieout, int *cookieoutlen, unsigned long flags, @@ -155,6 +156,7 @@ int virDomainMigratePrepareTunnel3(virConnectPtr dconn, int virDomainMigratePerform3(virDomainPtr dom, + const char *xmlin, const char *cookiein, int cookieinlen, char **cookieout, diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index db299a2..e7b0fa6 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5963,7 +5963,7 @@ qemudDomainMigratePerform (virDomainPtr dom, * Consume any cookie we were able to decode though */ ret = qemuMigrationPerform(driver, dom->conn, vm, - uri, cookie, cookielen, + NULL, uri, cookie, cookielen, NULL, NULL, /* No output cookies in v2 */ flags, dname, resource, true); @@ -6032,6 +6032,7 @@ cleanup: static char * qemuDomainMigrateBegin3(virDomainPtr domain, + const char *xmlin, char **cookieout, int *cookieoutlen, unsigned long flags, @@ -6061,7 +6062,7 @@ qemuDomainMigrateBegin3(virDomainPtr domain, goto cleanup; } - xml = qemuMigrationBegin(driver, vm, + xml = qemuMigrationBegin(driver, vm, xmlin, cookieout, cookieoutlen); cleanup: @@ -6178,6 +6179,7 @@ cleanup: static int qemuDomainMigratePerform3(virDomainPtr dom, + const char *xmlin, const char *cookiein, int cookieinlen, char **cookieout, @@ -6210,7 +6212,7 @@ qemuDomainMigratePerform3(virDomainPtr dom, goto cleanup; } - ret = qemuMigrationPerform(driver, dom->conn, vm, + ret = qemuMigrationPerform(driver, dom->conn, vm, xmlin, uri, cookiein, cookieinlen, cookieout, cookieoutlen, flags, dname, resource, false); diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 97138b5..6044b87 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -863,12 +863,19 @@ qemuDomainMigrateGraphicsRelocate(struct qemud_driver *driver, char *qemuMigrationBegin(struct qemud_driver *driver, virDomainObjPtr vm, + const char *xmlin, char **cookieout, int *cookieoutlen) { char *rv = NULL; qemuMigrationCookiePtr mig = NULL; + if (xmlin) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("Passing XML for the target VM is not yet supported")); + goto cleanup; + } + if (!virDomainObjIsActive(vm)) { qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", _("domain is not running")); @@ -1779,6 +1786,7 @@ static int doPeer2PeerMigrate3(struct qemud_driver *driver, virConnectPtr sconn, virConnectPtr dconn, virDomainObjPtr vm, + const char *xmlin, const char *uri, unsigned long flags, const char *dname, @@ -1797,7 +1805,7 @@ static int doPeer2PeerMigrate3(struct qemud_driver *driver, virStreamPtr st = NULL; VIR_DEBUG("Begin3 %p", sconn); - dom_xml = qemuMigrationBegin(driver, vm, + dom_xml = qemuMigrationBegin(driver, vm, xmlin, &cookieout, &cookieoutlen); if (!dom_xml) goto cleanup; @@ -1943,6 +1951,7 @@ finish: static int doPeer2PeerMigrate(struct qemud_driver *driver, virConnectPtr sconn, virDomainObjPtr vm, + const char *xmlin, const char *uri, unsigned long flags, const char *dname, @@ -1987,7 +1996,7 @@ static int doPeer2PeerMigrate(struct qemud_driver *driver, } if (v3) - ret = doPeer2PeerMigrate3(driver, sconn, dconn, vm, + ret = doPeer2PeerMigrate3(driver, sconn, dconn, vm, xmlin, uri, flags, dname, resource); else ret = doPeer2PeerMigrate2(driver, sconn, dconn, vm, @@ -2006,6 +2015,7 @@ cleanup: int qemuMigrationPerform(struct qemud_driver *driver, virConnectPtr conn, virDomainObjPtr vm, + const char *xmlin, const char *uri, const char *cookiein, int cookieinlen, @@ -2048,7 +2058,8 @@ int qemuMigrationPerform(struct qemud_driver *driver, goto endjob; } - if (doPeer2PeerMigrate(driver, conn, vm, uri, flags, dname, resource) < 0) + if (doPeer2PeerMigrate(driver, conn, vm, xmlin, + uri, flags, dname, resource) < 0) /* doPeer2PeerMigrate already set the error, so just get out */ goto endjob; } else { diff --git a/src/qemu/qemu_migration.h b/src/qemu/qemu_migration.h index 781e58c..ece1350 100644 --- a/src/qemu/qemu_migration.h +++ b/src/qemu/qemu_migration.h @@ -34,6 +34,7 @@ int qemuMigrationWaitForCompletion(struct qemud_driver *driver, virDomainObjPtr char *qemuMigrationBegin(struct qemud_driver *driver, virDomainObjPtr vm, + const char *xmlin, char **cookieout, int *cookieoutlen); @@ -61,6 +62,7 @@ int qemuMigrationPrepareDirect(struct qemud_driver *driver, int qemuMigrationPerform(struct qemud_driver *driver, virConnectPtr conn, virDomainObjPtr vm, + const char *xmlin, const char *uri, const char *cookiein, int cookieinlen, diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 1691dab..66393fb 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -4982,6 +4982,7 @@ done: static char * remoteDomainMigrateBegin3(virDomainPtr domain, + const char *xmlin, char **cookieout, int *cookieoutlen, unsigned long flags, @@ -4999,6 +5000,7 @@ remoteDomainMigrateBegin3(virDomainPtr domain, memset(&ret, 0, sizeof(ret)); make_nonnull_domain (&args.dom, domain); + args.xmlin = xmlin == NULL ? NULL : (char **) &xmlin; args.flags = flags; args.dname = dname == NULL ? NULL : (char **) &dname; args.resource = resource; @@ -5167,6 +5169,7 @@ error: static int remoteDomainMigratePerform3(virDomainPtr dom, + const char *xmlin, const char *cookiein, int cookieinlen, char **cookieout, @@ -5188,6 +5191,7 @@ remoteDomainMigratePerform3(virDomainPtr dom, make_nonnull_domain(&args.dom, dom); + args.xmlin = xmlin == NULL ? NULL : (char **) &xmlin; args.cookie_in.cookie_in_val = (char *)cookiein; args.cookie_in.cookie_in_len = cookieinlen; args.uri = (char *) uri; diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index f0da95d..842fd0b 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -1973,6 +1973,7 @@ struct remote_domain_get_state_ret { struct remote_domain_migrate_begin3_args { remote_nonnull_domain dom; + remote_string xmlin; unsigned hyper flags; remote_string dname; unsigned hyper resource; @@ -2011,6 +2012,7 @@ struct remote_domain_migrate_prepare_tunnel3_ret { struct remote_domain_migrate_perform3_args { remote_nonnull_domain dom; + remote_string xmlin; opaque cookie_in<REMOTE_MIGRATE_COOKIE_MAX>; remote_nonnull_string uri; unsigned hyper flags; diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs index 414b4d5..971f53a 100644 --- a/src/remote_protocol-structs +++ b/src/remote_protocol-structs @@ -1457,6 +1457,7 @@ struct remote_domain_get_state_ret { }; struct remote_domain_migrate_begin3_args { remote_nonnull_domain dom; + remote_string xmlin; uint64_t flags; remote_string dname; uint64_t resource; @@ -1504,6 +1505,7 @@ struct remote_domain_migrate_prepare_tunnel3_ret { }; struct remote_domain_migrate_perform3_args { remote_nonnull_domain dom; + remote_string xmlin; struct { u_int cookie_in_len; char * cookie_in_val; -- 1.7.4.4

2011/5/24 Daniel P. Berrange <berrange@redhat.com>:
This extends the v3 migration protocol such that the virDomainMigrateBegin3 and virDomainMigratePerform3 methods accept an application supplied XML config for the target VM.
If the 'xmlin' parameter is NULL, then Begin3 uses the current guest XML as normal. A driver implementing the Begin3 method should either reject all non-NULL 'xmlin' parameters, or strictly validate that the app supplied XML does not change guest ABI.
The Perform3 method also needed the xmlin parameter to cope with the Peer2Peer migration sequence.
NB it is not yet possible to use this capability since neither of the public virDomainMigrate/virDomainMigrateToURI methods have a way to pass in XML.
* daemon/remote.c, src/remote/remote_driver.c, src/remote/remote_protocol.x, src/remote_protocol-structs: Add 'remote_string xmlin' parameter to begin3/perform3 RPC messages * src/libvirt.c, src/driver.h, src/libvirt_internal.h: Add 'const char *xmlin' parameter to Begin3/Perform3 methods * src/qemu/qemu_driver.c, src/qemu/qemu_migration.c, src/qemu/qemu_migration.h: Pass xmlin parameter around migration methods --- daemon/remote.c | 8 +++++- src/driver.h | 2 + src/libvirt.c | 49 ++++++++++++++++++++++++++++++----------- src/libvirt_internal.h | 2 + src/qemu/qemu_driver.c | 8 ++++-- src/qemu/qemu_migration.c | 17 ++++++++++++-- src/qemu/qemu_migration.h | 2 + src/remote/remote_driver.c | 4 +++ src/remote/remote_protocol.x | 2 + src/remote_protocol-structs | 2 + 10 files changed, 75 insertions(+), 21 deletions(-)
ACK. Matthias

The virDomainMigratePerform3 currently has a single URI parameter whose meaning varies. It is either - A QEMU migration URI (normal migration) - A libvirtd connection URI (peer2peer migration) Unfortunately when using peer2peer migration, without also using tunnelled migration, it is possible that both URIs are required. This adds a second URI parameter to the virDomainMigratePerform3 method, to cope with this scenario. Each parameter how has a fixed meaning. NB, there is no way to actually take advantage of this yet, since virDomainMigrate/virDomainMigrateToURI do not have any way to provide the 2 separate URIs * daemon/remote.c, src/remote/remote_driver.c, src/remote/remote_protocol.x, src/remote_protocol-structs: Add the second URI parameter to perform3 message * src/driver.h, src/libvirt.c, src/libvirt_internal.h: Add the second URI parameter to Perform3 method * src/libvirt_internal.h, src/qemu/qemu_migration.c, src/qemu/qemu_migration.h: Update to handle URIs correctly --- daemon/remote.c | 13 +++++++++++-- src/driver.h | 2 ++ src/libvirt.c | 37 +++++++++++++++++++++++++------------ src/libvirt_internal.h | 6 ++++-- src/qemu/qemu_driver.c | 12 ++++++++++-- src/qemu/qemu_migration.c | 26 +++++++++++++++++--------- src/qemu/qemu_migration.h | 1 + src/remote/remote_driver.c | 8 ++++++-- src/remote/remote_protocol.x | 6 ++++-- src/remote_protocol-structs | 1 + 10 files changed, 81 insertions(+), 31 deletions(-) diff --git a/daemon/remote.c b/daemon/remote.c index 60f01b4..f85d760 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -3292,6 +3292,8 @@ remoteDispatchDomainMigratePerform3(struct qemud_server *server ATTRIBUTE_UNUSED virDomainPtr dom = NULL; char *xmlin; char *dname; + char *uri; + char *dconnuri; char *cookieout = NULL; int cookieoutlen = 0; int rv = -1; @@ -3306,12 +3308,14 @@ remoteDispatchDomainMigratePerform3(struct qemud_server *server ATTRIBUTE_UNUSED xmlin = args->xmlin == NULL ? NULL : *args->xmlin; dname = args->dname == NULL ? NULL : *args->dname; + uri = args->uri == NULL ? NULL : *args->uri; + dconnuri = args->dconnuri == NULL ? NULL : *args->dconnuri; if (virDomainMigratePerform3(dom, xmlin, args->cookie_in.cookie_in_val, args->cookie_in.cookie_in_len, &cookieout, &cookieoutlen, - args->uri, + dconnuri, uri, args->flags, dname, args->resource) < 0) goto cleanup; @@ -3343,6 +3347,8 @@ remoteDispatchDomainMigrateFinish3(struct qemud_server *server ATTRIBUTE_UNUSED, virDomainPtr dom = NULL; char *cookieout = NULL; int cookieoutlen = 0; + char *uri; + char *dconnuri; int rv = -1; if (!conn) { @@ -3350,11 +3356,14 @@ remoteDispatchDomainMigrateFinish3(struct qemud_server *server ATTRIBUTE_UNUSED, goto cleanup; } + uri = args->uri == NULL ? NULL : *args->uri; + dconnuri = args->dconnuri == NULL ? NULL : *args->dconnuri; + if (virDomainMigrateFinish3(conn, args->dname, args->cookie_in.cookie_in_val, args->cookie_in.cookie_in_len, &cookieout, &cookieoutlen, - args->uri, + dconnuri, uri, args->flags, args->cancelled, &dom) < 0) diff --git a/src/driver.h b/src/driver.h index a1468e8..a5d8fe5 100644 --- a/src/driver.h +++ b/src/driver.h @@ -581,6 +581,7 @@ typedef int int cookieinlen, char **cookieout, int *cookieoutlen, + const char *dconnuri, const char *uri, unsigned long flags, const char *dname, @@ -594,6 +595,7 @@ typedef int int cookieinlen, char **cookieout, int *cookieoutlen, + const char *dconnuri, const char *uri, unsigned long flags, int cancelled, diff --git a/src/libvirt.c b/src/libvirt.c index 7b7323e..2b2c1fd 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -3792,9 +3792,10 @@ virDomainMigrateVersion3(virDomainPtr domain, cookieinlen = cookieoutlen; cookieout = NULL; cookieoutlen = 0; + /* dconnuri not relevant in non-P2P modes, so left NULL here */ ret = domain->conn->driver->domainMigratePerform3 (domain, NULL, cookiein, cookieinlen, - &cookieout, &cookieoutlen, + &cookieout, &cookieoutlen, NULL, uri, flags, dname, bandwidth); /* Perform failed. Make sure Finish doesn't overwrite the error */ @@ -3822,7 +3823,7 @@ finish: dname = dname ? dname : domain->name; ret = dconn->driver->domainMigrateFinish3 (dconn, dname, cookiein, cookieinlen, &cookieout, &cookieoutlen, - uri, flags, cancelled, &ddomain); + NULL, uri, flags, cancelled, &ddomain); /* If ret is 0 then 'ddomain' indicates whether the VM is * running on the dest. If not running, we can restart @@ -3877,6 +3878,7 @@ virDomainMigratePeer2Peer (virDomainPtr domain, const char *xmlin, unsigned long flags, const char *dname, + const char *dconnuri, const char *uri, unsigned long bandwidth) { @@ -3888,7 +3890,7 @@ virDomainMigratePeer2Peer (virDomainPtr domain, return -1; } - tempuri = xmlParseURI(uri); + tempuri = xmlParseURI(dconnuri); if (!tempuri) { virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); virDispatchError(domain->conn); @@ -3915,6 +3917,7 @@ virDomainMigratePeer2Peer (virDomainPtr domain, 0, /* cookieinlen */ NULL, /* cookieoutlen */ NULL, /* cookieoutlen */ + dconnuri, uri, flags, dname, @@ -3926,10 +3929,15 @@ virDomainMigratePeer2Peer (virDomainPtr domain, _("Unable to change target guest XML during migration")); return -1; } + if (uri) { + virLibConnError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to override peer2peer migration URI")); + return -1; + } return domain->conn->driver->domainMigratePerform(domain, NULL, /* cookie */ 0, /* cookielen */ - uri, + dconnuri, flags, dname, bandwidth); @@ -3968,12 +3976,15 @@ virDomainMigrateDirect (virDomainPtr domain, if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, VIR_DRV_FEATURE_MIGRATION_V3)) { VIR_DEBUG("Using migration protocol 3"); + /* dconn URI not relevant in direct migration, since no + * target libvirtd is involved */ return domain->conn->driver->domainMigratePerform3(domain, xmlin, NULL, /* cookiein */ 0, /* cookieinlen */ NULL, /* cookieoutlen */ NULL, /* cookieoutlen */ + NULL, /* dconnuri */ uri, flags, dname, @@ -4110,7 +4121,7 @@ virDomainMigrate (virDomainPtr domain, VIR_DEBUG("Using peer2peer migration"); if (virDomainMigratePeer2Peer(domain, NULL, flags, dname, - uri ? uri : dstURI, bandwidth) < 0) { + uri ? uri : dstURI, NULL, bandwidth) < 0) { VIR_FREE(dstURI); goto error; } @@ -4256,7 +4267,7 @@ virDomainMigrateToURI (virDomainPtr domain, VIR_DRV_FEATURE_MIGRATION_P2P)) { VIR_DEBUG("Using peer2peer migration"); if (virDomainMigratePeer2Peer(domain, NULL, flags, - dname, duri, bandwidth) < 0) + dname, duri, NULL, bandwidth) < 0) goto error; } else { /* No peer to peer migration supported */ @@ -4758,6 +4769,7 @@ virDomainMigratePerform3(virDomainPtr domain, int cookieinlen, char **cookieout, int *cookieoutlen, + const char *dconnuri, const char *uri, unsigned long flags, const char *dname, @@ -4766,10 +4778,10 @@ virDomainMigratePerform3(virDomainPtr domain, virConnectPtr conn; VIR_DOMAIN_DEBUG(domain, "xmlin=%s cookiein=%p, cookieinlen=%d, " - "cookieout=%p, cookieoutlen=%p, " + "cookieout=%p, cookieoutlen=%p, dconnuri=%s, " "uri=%s, flags=%lu, dname=%s, bandwidth=%lu", NULLSTR(xmlin), cookiein, cookieinlen, - cookieout, cookieoutlen, + cookieout, cookieoutlen, NULLSTR(dconnuri), uri, flags, NULLSTR(dname), bandwidth); virResetLastError(); @@ -4791,7 +4803,7 @@ virDomainMigratePerform3(virDomainPtr domain, ret = conn->driver->domainMigratePerform3(domain, xmlin, cookiein, cookieinlen, cookieout, cookieoutlen, - uri, + dconnuri, uri, flags, dname, bandwidth); if (ret < 0) goto error; @@ -4817,15 +4829,16 @@ virDomainMigrateFinish3(virConnectPtr dconn, int cookieinlen, char **cookieout, int *cookieoutlen, + const char *dconnuri, const char *uri, unsigned long flags, int cancelled, virDomainPtr *newdom) { VIR_DEBUG("dconn=%p, dname=%s, cookiein=%p, cookieinlen=%d, cookieout=%p," - "cookieoutlen=%p, uri=%s, flags=%lu, retcode=%d newdom=%p", + "cookieoutlen=%p, dconnuri=%s, uri=%s, flags=%lu, retcode=%d newdom=%p", dconn, NULLSTR(dname), cookiein, cookieinlen, cookieout, - cookieoutlen, uri, flags, cancelled, newdom); + cookieoutlen, NULLSTR(dconnuri), NULLSTR(uri), flags, cancelled, newdom); virResetLastError(); @@ -4845,7 +4858,7 @@ virDomainMigrateFinish3(virConnectPtr dconn, ret = dconn->driver->domainMigrateFinish3(dconn, dname, cookiein, cookieinlen, cookieout, cookieoutlen, - uri, flags, + dconnuri, uri, flags, cancelled, newdom); if (ret < 0) diff --git a/src/libvirt_internal.h b/src/libvirt_internal.h index c7c1932..3144271 100644 --- a/src/libvirt_internal.h +++ b/src/libvirt_internal.h @@ -161,7 +161,8 @@ int virDomainMigratePerform3(virDomainPtr dom, int cookieinlen, char **cookieout, int *cookieoutlen, - const char *uri, + const char *dconnuri, /* libvirtd URI if Peer2Peer, NULL otherwise */ + const char *uri, /* VM Migration URI */ unsigned long flags, const char *dname, unsigned long resource); @@ -172,7 +173,8 @@ int virDomainMigrateFinish3(virConnectPtr dconn, int cookieinlen, char **cookieout, int *cookieoutlen, - const char *uri, + const char *dconnuri, /* libvirtd URI if Peer2Peer, NULL otherwise */ + const char *uri, /* VM Migration URI */ unsigned long flags, int cancelled, /* Kill the dst VM */ virDomainPtr *newdom); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index e7b0fa6..978a63a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5936,6 +5936,7 @@ qemudDomainMigratePerform (virDomainPtr dom, struct qemud_driver *driver = dom->conn->privateData; virDomainObjPtr vm; int ret = -1; + const char *dconnuri = NULL; virCheckFlags(VIR_MIGRATE_LIVE | VIR_MIGRATE_PEER2PEER | @@ -5956,6 +5957,11 @@ qemudDomainMigratePerform (virDomainPtr dom, goto cleanup; } + if (flags & VIR_MIGRATE_PEER2PEER) { + dconnuri = uri; + uri = NULL; + } + /* Do not output cookies in v2 protocol, since the cookie * length was not sufficiently large, causing failures * migrating between old & new libvirtd. @@ -5963,7 +5969,7 @@ qemudDomainMigratePerform (virDomainPtr dom, * Consume any cookie we were able to decode though */ ret = qemuMigrationPerform(driver, dom->conn, vm, - NULL, uri, cookie, cookielen, + NULL, dconnuri, uri, cookie, cookielen, NULL, NULL, /* No output cookies in v2 */ flags, dname, resource, true); @@ -6184,6 +6190,7 @@ qemuDomainMigratePerform3(virDomainPtr dom, int cookieinlen, char **cookieout, int *cookieoutlen, + const char *dconnuri, const char *uri, unsigned long flags, const char *dname, @@ -6213,7 +6220,7 @@ qemuDomainMigratePerform3(virDomainPtr dom, } ret = qemuMigrationPerform(driver, dom->conn, vm, xmlin, - uri, cookiein, cookieinlen, + dconnuri, uri, cookiein, cookieinlen, cookieout, cookieoutlen, flags, dname, resource, false); @@ -6230,6 +6237,7 @@ qemuDomainMigrateFinish3(virConnectPtr dconn, int cookieinlen, char **cookieout, int *cookieoutlen, + const char *dconnuri ATTRIBUTE_UNUSED, const char *uri ATTRIBUTE_UNUSED, unsigned long flags, int cancelled, diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 6044b87..2e2ccd5 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1652,7 +1652,7 @@ static int doPeer2PeerMigrate2(struct qemud_driver *driver, virConnectPtr sconn, virConnectPtr dconn, virDomainObjPtr vm, - const char *uri, + const char *dconnuri, unsigned long flags, const char *dname, unsigned long resource) @@ -1753,7 +1753,7 @@ finish: qemuDomainObjEnterRemoteWithDriver(driver, vm); ddomain = dconn->driver->domainMigrateFinish2 (dconn, dname, cookie, cookielen, - uri_out ? uri_out : uri, flags, cancelled); + uri_out ? uri_out : dconnuri, flags, cancelled); qemuDomainObjExitRemoteWithDriver(driver, vm); cleanup: @@ -1787,6 +1787,7 @@ static int doPeer2PeerMigrate3(struct qemud_driver *driver, virConnectPtr dconn, virDomainObjPtr vm, const char *xmlin, + const char *dconnuri, const char *uri, unsigned long flags, const char *dname, @@ -1832,7 +1833,7 @@ static int doPeer2PeerMigrate3(struct qemud_driver *driver, qemuDomainObjEnterRemoteWithDriver(driver, vm); ret = dconn->driver->domainMigratePrepare3 (dconn, cookiein, cookieinlen, &cookieout, &cookieoutlen, - NULL, &uri_out, flags, dname, resource, dom_xml); + uri, &uri_out, flags, dname, resource, dom_xml); qemuDomainObjExitRemoteWithDriver(driver, vm); } VIR_FREE(dom_xml); @@ -1852,7 +1853,7 @@ static int doPeer2PeerMigrate3(struct qemud_driver *driver, * running, but in paused state until the destination can * confirm migration completion. */ - VIR_DEBUG("Perform3 %p uri=%s", sconn, uri_out); + VIR_DEBUG("Perform3 %p uri=%s uri_out=%s", sconn, uri, uri_out); VIR_FREE(cookiein); cookiein = cookieout; cookieinlen = cookieoutlen; @@ -1895,7 +1896,7 @@ finish: qemuDomainObjEnterRemoteWithDriver(driver, vm); ret = dconn->driver->domainMigrateFinish3 (dconn, dname, cookiein, cookieinlen, &cookieout, &cookieoutlen, - uri_out ? uri_out : uri, flags, cancelled, &ddomain); + dconnuri, uri_out ? uri_out : uri, flags, cancelled, &ddomain); qemuDomainObjExitRemoteWithDriver(driver, vm); /* If ret is 0 then 'ddomain' indicates whether the VM is @@ -1952,6 +1953,7 @@ static int doPeer2PeerMigrate(struct qemud_driver *driver, virConnectPtr sconn, virDomainObjPtr vm, const char *xmlin, + const char *dconnuri, const char *uri, unsigned long flags, const char *dname, @@ -1967,7 +1969,7 @@ static int doPeer2PeerMigrate(struct qemud_driver *driver, */ qemuDomainObjEnterRemoteWithDriver(driver, vm); - dconn = virConnectOpen(uri); + dconn = virConnectOpen(dconnuri); qemuDomainObjExitRemoteWithDriver(driver, vm); if (dconn == NULL) { qemuReportError(VIR_ERR_OPERATION_FAILED, @@ -1997,10 +1999,10 @@ static int doPeer2PeerMigrate(struct qemud_driver *driver, if (v3) ret = doPeer2PeerMigrate3(driver, sconn, dconn, vm, xmlin, - uri, flags, dname, resource); + dconnuri, uri, flags, dname, resource); else ret = doPeer2PeerMigrate2(driver, sconn, dconn, vm, - uri, flags, dname, resource); + dconnuri, flags, dname, resource); cleanup: /* don't call virConnectClose(), because that resets any pending errors */ @@ -2016,6 +2018,7 @@ int qemuMigrationPerform(struct qemud_driver *driver, virConnectPtr conn, virDomainObjPtr vm, const char *xmlin, + const char *dconnuri, const char *uri, const char *cookiein, int cookieinlen, @@ -2059,10 +2062,15 @@ int qemuMigrationPerform(struct qemud_driver *driver, } if (doPeer2PeerMigrate(driver, conn, vm, xmlin, - uri, flags, dname, resource) < 0) + dconnuri, uri, flags, dname, resource) < 0) /* doPeer2PeerMigrate already set the error, so just get out */ goto endjob; } else { + if (dconnuri) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("Unexpected dconnuri parameter with non-peer2peer migration")); + goto endjob; + } if (doNativeMigrate(driver, vm, uri, cookiein, cookieinlen, cookieout, cookieoutlen, flags, dname, resource) < 0) diff --git a/src/qemu/qemu_migration.h b/src/qemu/qemu_migration.h index ece1350..08e3acc 100644 --- a/src/qemu/qemu_migration.h +++ b/src/qemu/qemu_migration.h @@ -63,6 +63,7 @@ int qemuMigrationPerform(struct qemud_driver *driver, virConnectPtr conn, virDomainObjPtr vm, const char *xmlin, + const char *dconnuri, const char *uri, const char *cookiein, int cookieinlen, diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 66393fb..64f5620 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -5174,6 +5174,7 @@ remoteDomainMigratePerform3(virDomainPtr dom, int cookieinlen, char **cookieout, int *cookieoutlen, + const char *dconnuri, const char *uri, unsigned long flags, const char *dname, @@ -5194,9 +5195,10 @@ remoteDomainMigratePerform3(virDomainPtr dom, args.xmlin = xmlin == NULL ? NULL : (char **) &xmlin; args.cookie_in.cookie_in_val = (char *)cookiein; args.cookie_in.cookie_in_len = cookieinlen; - args.uri = (char *) uri; args.flags = flags; args.dname = dname == NULL ? NULL : (char **) &dname; + args.uri = uri == NULL ? NULL : (char **) &uri; + args.dconnuri = dconnuri == NULL ? NULL : (char **) &dconnuri; args.resource = resource; if (call (dom->conn, priv, 0, REMOTE_PROC_DOMAIN_MIGRATE_PERFORM3, @@ -5233,6 +5235,7 @@ remoteDomainMigrateFinish3(virConnectPtr dconn, int cookieinlen, char **cookieout, int *cookieoutlen, + const char *dconnuri, const char *uri, unsigned long flags, int cancelled, @@ -5252,7 +5255,8 @@ remoteDomainMigrateFinish3(virConnectPtr dconn, args.cookie_in.cookie_in_val = (char *)cookiein; args.cookie_in.cookie_in_len = cookieinlen; args.dname = (char *) dname; - args.uri = (char *) uri; + args.uri = uri == NULL ? NULL : (char **) &uri; + args.dconnuri = dconnuri == NULL ? NULL : (char **) &dconnuri; args.flags = flags; args.cancelled = cancelled; diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index 842fd0b..58afee0 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -2014,7 +2014,8 @@ struct remote_domain_migrate_perform3_args { remote_nonnull_domain dom; remote_string xmlin; opaque cookie_in<REMOTE_MIGRATE_COOKIE_MAX>; - remote_nonnull_string uri; + remote_string dconnuri; + remote_string uri; unsigned hyper flags; remote_string dname; unsigned hyper resource; @@ -2027,7 +2028,8 @@ struct remote_domain_migrate_perform3_ret { struct remote_domain_migrate_finish3_args { remote_nonnull_string dname; opaque cookie_in<REMOTE_MIGRATE_COOKIE_MAX>; - remote_nonnull_string uri; + remote_string dconnuri; + remote_string uri; unsigned hyper flags; int cancelled; }; diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs index 971f53a..014e5ac 100644 --- a/src/remote_protocol-structs +++ b/src/remote_protocol-structs @@ -1510,6 +1510,7 @@ struct remote_domain_migrate_perform3_args { u_int cookie_in_len; char * cookie_in_val; } cookie_in; + remote_string dconnuri; remote_nonnull_string uri; uint64_t flags; remote_string dname; -- 1.7.4.4

2011/5/24 Daniel P. Berrange <berrange@redhat.com>:
The virDomainMigratePerform3 currently has a single URI parameter whose meaning varies. It is either
- A QEMU migration URI (normal migration) - A libvirtd connection URI (peer2peer migration)
Unfortunately when using peer2peer migration, without also using tunnelled migration, it is possible that both URIs are required.
This adds a second URI parameter to the virDomainMigratePerform3 method, to cope with this scenario. Each parameter how has a fixed meaning.
NB, there is no way to actually take advantage of this yet, since virDomainMigrate/virDomainMigrateToURI do not have any way to provide the 2 separate URIs
* daemon/remote.c, src/remote/remote_driver.c, src/remote/remote_protocol.x, src/remote_protocol-structs: Add the second URI parameter to perform3 message * src/driver.h, src/libvirt.c, src/libvirt_internal.h: Add the second URI parameter to Perform3 method * src/libvirt_internal.h, src/qemu/qemu_migration.c, src/qemu/qemu_migration.h: Update to handle URIs correctly --- daemon/remote.c | 13 +++++++++++-- src/driver.h | 2 ++ src/libvirt.c | 37 +++++++++++++++++++++++++------------ src/libvirt_internal.h | 6 ++++-- src/qemu/qemu_driver.c | 12 ++++++++++-- src/qemu/qemu_migration.c | 26 +++++++++++++++++--------- src/qemu/qemu_migration.h | 1 + src/remote/remote_driver.c | 8 ++++++-- src/remote/remote_protocol.x | 6 ++++-- src/remote_protocol-structs | 1 + 10 files changed, 81 insertions(+), 31 deletions(-)
diff --git a/src/libvirt.c b/src/libvirt.c index 7b7323e..2b2c1fd 100644 --- a/src/libvirt.c +++ b/src/libvirt.c
@@ -4766,10 +4778,10 @@ virDomainMigratePerform3(virDomainPtr domain, virConnectPtr conn;
VIR_DOMAIN_DEBUG(domain, "xmlin=%s cookiein=%p, cookieinlen=%d, " - "cookieout=%p, cookieoutlen=%p, " + "cookieout=%p, cookieoutlen=%p, dconnuri=%s, " "uri=%s, flags=%lu, dname=%s, bandwidth=%lu", NULLSTR(xmlin), cookiein, cookieinlen, - cookieout, cookieoutlen, + cookieout, cookieoutlen, NULLSTR(dconnuri), uri, flags, NULLSTR(dname), bandwidth);
uri can be NULL here, can't it? So this should use NULLSTR(uri) instead of plain uri.
@@ -4817,15 +4829,16 @@ virDomainMigrateFinish3(virConnectPtr dconn, int cookieinlen, char **cookieout, int *cookieoutlen, + const char *dconnuri, const char *uri, unsigned long flags, int cancelled, virDomainPtr *newdom) { VIR_DEBUG("dconn=%p, dname=%s, cookiein=%p, cookieinlen=%d, cookieout=%p," - "cookieoutlen=%p, uri=%s, flags=%lu, retcode=%d newdom=%p", + "cookieoutlen=%p, dconnuri=%s, uri=%s, flags=%lu, retcode=%d newdom=%p", dconn, NULLSTR(dname), cookiein, cookieinlen, cookieout, - cookieoutlen, uri, flags, cancelled, newdom); + cookieoutlen, NULLSTR(dconnuri), NULLSTR(uri), flags, cancelled, newdom);
virResetLastError();
Here it's correct.
diff --git a/src/libvirt_internal.h b/src/libvirt_internal.h index c7c1932..3144271 100644 --- a/src/libvirt_internal.h +++ b/src/libvirt_internal.h @@ -161,7 +161,8 @@ int virDomainMigratePerform3(virDomainPtr dom, int cookieinlen, char **cookieout, int *cookieoutlen, - const char *uri, + const char *dconnuri, /* libvirtd URI if Peer2Peer, NULL otherwise */ + const char *uri, /* VM Migration URI */ unsigned long flags, const char *dname, unsigned long resource); @@ -172,7 +173,8 @@ int virDomainMigrateFinish3(virConnectPtr dconn, int cookieinlen, char **cookieout, int *cookieoutlen, - const char *uri, + const char *dconnuri, /* libvirtd URI if Peer2Peer, NULL otherwise */ + const char *uri, /* VM Migration URI */ unsigned long flags, int cancelled, /* Kill the dst VM */ virDomainPtr *newdom);
Maybe state in this comments that uri is NULL in the Peer2Peer case, just for clarity.
diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs index 971f53a..014e5ac 100644 --- a/src/remote_protocol-structs +++ b/src/remote_protocol-structs @@ -1510,6 +1510,7 @@ struct remote_domain_migrate_perform3_args { u_int cookie_in_len; char * cookie_in_val; } cookie_in; + remote_string dconnuri; remote_nonnull_string uri; uint64_t flags; remote_string dname;
As uri is now optional you need to change its type from remote_nonnull_string to remote_string here and in remote_domain_migrate_finish3_args. And you need to add remote_string dconnuri to remote_domain_migrate_finish3_args to satisfy make syntax-check. ACK, with this nits fixed. Matthias

There are two pieces of information which are desirable for migration, which cannot be supplied by applications - The explicit QEMU migration URI, while using Peer2Peer migration - An override for the target VM XML This introduces two new public APIs to support these extra parameters. There is no need for extra wire protocool changes, since this is supported by the v3 migration enhancements * include/libvirt/libvirt.h.in, src/libvirt.c, src/libvirt_public.syms: Add virDomainMigrate2 and virDomainMigrateToURI2 --- include/libvirt/libvirt.h.in | 12 ++ src/libvirt.c | 321 +++++++++++++++++++++++++++++++++++++++++- src/libvirt_public.syms | 2 + 3 files changed, 332 insertions(+), 3 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 7cd6e13..a3c771a 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -532,11 +532,23 @@ typedef enum { virDomainPtr virDomainMigrate (virDomainPtr domain, virConnectPtr dconn, unsigned long flags, const char *dname, const char *uri, unsigned long bandwidth); +virDomainPtr virDomainMigrate2(virDomainPtr domain, virConnectPtr dconn, + const char *dxml, + unsigned long flags, const char *dname, + const char *uri, unsigned long bandwidth); int virDomainMigrateToURI (virDomainPtr domain, const char *duri, unsigned long flags, const char *dname, unsigned long bandwidth); +int virDomainMigrateToURI2(virDomainPtr domain, + const char *dconnuri, + const char *miguri, + const char *dxml, + unsigned long flags, + const char *dname, + unsigned long bandwidth); + int virDomainMigrateSetMaxDowntime (virDomainPtr domain, unsigned long long downtime, unsigned int flags); diff --git a/src/libvirt.c b/src/libvirt.c index 2b2c1fd..4bdcd43 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -4146,19 +4146,204 @@ virDomainMigrate (virDomainPtr domain, VIR_DRV_SUPPORTS_FEATURE(dconn->driver, dconn, VIR_DRV_FEATURE_MIGRATION_V3)) { VIR_DEBUG("Using migration protocol 3"); - ddomain = virDomainMigrateVersion3(domain, dconn, NULL, flags, dname, uri, bandwidth); + ddomain = virDomainMigrateVersion3(domain, dconn, NULL, + flags, dname, uri, bandwidth); } else if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, VIR_DRV_FEATURE_MIGRATION_V2) && VIR_DRV_SUPPORTS_FEATURE(dconn->driver, dconn, VIR_DRV_FEATURE_MIGRATION_V2)) { VIR_DEBUG("Using migration protocol 2"); - ddomain = virDomainMigrateVersion2(domain, dconn, flags, dname, uri, bandwidth); + ddomain = virDomainMigrateVersion2(domain, dconn, flags, + dname, uri, bandwidth); } else if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, VIR_DRV_FEATURE_MIGRATION_V1) && VIR_DRV_SUPPORTS_FEATURE(dconn->driver, dconn, VIR_DRV_FEATURE_MIGRATION_V1)) { VIR_DEBUG("Using migration protocol 1"); - ddomain = virDomainMigrateVersion1(domain, dconn, flags, dname, uri, bandwidth); + ddomain = virDomainMigrateVersion1(domain, dconn, flags, + dname, uri, bandwidth); + } else { + /* This driver does not support any migration method */ + virLibConnError(VIR_ERR_NO_SUPPORT, __FUNCTION__); + goto error; + } + } + + if (ddomain == NULL) + goto error; + + return ddomain; + +error: + virDispatchError(domain->conn); + return NULL; +} + + +/** + * virDomainMigrate2: + * @domain: a domain object + * @dconn: destination host (a connection object) + * @flags: flags + * @dxml: optional XML config for launching guest on target + * @dname: (optional) rename domain to this at destination + * @uri: (optional) dest hostname/URI as seen from the source host + * @bandwidth: (optional) specify migration bandwidth limit in Mbps + * + * Migrate the domain object from its current host to the destination + * host given by dconn (a connection to the destination host). + * + * Flags may be one of more of the following: + * VIR_MIGRATE_LIVE Do not pause the VM during migration + * VIR_MIGRATE_PEER2PEER Direct connection between source & destination hosts + * VIR_MIGRATE_TUNNELLED Tunnel migration data over the libvirt RPC channel + * VIR_MIGRATE_PERSIST_DEST If the migration is successful, persist the domain + * on the destination host. + * VIR_MIGRATE_UNDEFINE_SOURCE If the migration is successful, undefine the + * domain on the source host. + * VIR_MIGRATE_PAUSED Leave the domain suspended on the remote side. + * + * VIR_MIGRATE_TUNNELLED requires that VIR_MIGRATE_PEER2PEER be set. + * Applications using the VIR_MIGRATE_PEER2PEER flag will probably + * prefer to invoke virDomainMigrateToURI, avoiding the need to + * open connection to the destination host themselves. + * + * If a hypervisor supports renaming domains during migration, + * then you may set the dname parameter to the new name (otherwise + * it keeps the same name). If this is not supported by the + * hypervisor, dname must be NULL or else you will get an error. + * + * If the VIR_MIGRATE_PEER2PEER flag is set, the uri parameter + * must be a valid libvirt connection URI, by which the source + * libvirt driver can connect to the destination libvirt. If + * omitted, the dconn connection object will be queried for its + * current URI. + * + * If the VIR_MIGRATE_PEER2PEER flag is NOT set, the URI parameter + * takes a hypervisor specific format. The hypervisor capabilities + * XML includes details of the support URI schemes. If omitted + * the dconn will be asked for a default URI. + * + * In either case it is typically only necessary to specify a + * URI if the destination host has multiple interfaces and a + * specific interface is required to transmit migration data. + * + * The maximum bandwidth (in Mbps) that will be used to do migration + * can be specified with the bandwidth parameter. If set to 0, + * libvirt will choose a suitable default. Some hypervisors do + * not support this feature and will return an error if bandwidth + * is not 0. + * + * To see which features are supported by the current hypervisor, + * see virConnectGetCapabilities, /capabilities/host/migration_features. + * + * There are many limitations on migration imposed by the underlying + * technology - for example it may not be possible to migrate between + * different processors even with the same architecture, or between + * different types of hypervisor. + * + * Returns the new domain object if the migration was successful, + * or NULL in case of error. Note that the new domain object + * exists in the scope of the destination connection (dconn). + */ +virDomainPtr +virDomainMigrate2(virDomainPtr domain, + virConnectPtr dconn, + const char *dxml, + unsigned long flags, + const char *dname, + const char *uri, + unsigned long bandwidth) +{ + virDomainPtr ddomain = NULL; + + VIR_DOMAIN_DEBUG(domain, "dconn=%p, flags=%lu, dname=%s, uri=%s, bandwidth=%lu", + dconn, flags, NULLSTR(dname), NULLSTR(uri), bandwidth); + + virResetLastError(); + + /* First checkout the source */ + if (!VIR_IS_CONNECTED_DOMAIN (domain)) { + virLibDomainError(VIR_ERR_INVALID_DOMAIN, __FUNCTION__); + virDispatchError(NULL); + return NULL; + } + if (domain->conn->flags & VIR_CONNECT_RO) { + virLibDomainError(VIR_ERR_OPERATION_DENIED, __FUNCTION__); + goto error; + } + + /* Now checkout the destination */ + if (!VIR_IS_CONNECT(dconn)) { + virLibConnError(VIR_ERR_INVALID_CONN, __FUNCTION__); + goto error; + } + if (dconn->flags & VIR_CONNECT_RO) { + /* NB, deliberately report error against source object, not dest */ + virLibDomainError(VIR_ERR_OPERATION_DENIED, __FUNCTION__); + goto error; + } + + if (flags & VIR_MIGRATE_PEER2PEER) { + if (VIR_DRV_SUPPORTS_FEATURE (domain->conn->driver, domain->conn, + VIR_DRV_FEATURE_MIGRATION_P2P)) { + char *dstURI = virConnectGetURI(dconn); + if (!dstURI) + return NULL; + + VIR_DEBUG("Using peer2peer migration"); + if (virDomainMigratePeer2Peer(domain, dxml, flags, dname, + dstURI, uri, bandwidth) < 0) { + VIR_FREE(dstURI); + goto error; + } + VIR_FREE(dstURI); + + ddomain = virDomainLookupByName (dconn, dname ? dname : domain->name); + } else { + /* This driver does not support peer to peer migration */ + virLibConnError(VIR_ERR_NO_SUPPORT, __FUNCTION__); + goto error; + } + } else { + if (flags & VIR_MIGRATE_TUNNELLED) { + virLibConnError(VIR_ERR_OPERATION_INVALID, + _("cannot perform tunnelled migration without using peer2peer flag")); + goto error; + } + + /* Check that migration is supported by both drivers. */ + if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, + VIR_DRV_FEATURE_MIGRATION_V3) && + VIR_DRV_SUPPORTS_FEATURE(dconn->driver, dconn, + VIR_DRV_FEATURE_MIGRATION_V3)) { + VIR_DEBUG("Using migration protocol 3"); + ddomain = virDomainMigrateVersion3(domain, dconn, dxml, + flags, dname, uri, bandwidth); + } else if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, + VIR_DRV_FEATURE_MIGRATION_V2) && + VIR_DRV_SUPPORTS_FEATURE(dconn->driver, dconn, + VIR_DRV_FEATURE_MIGRATION_V2)) { + VIR_DEBUG("Using migration protocol 2"); + if (dxml) { + virLibConnError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to change target guest XML during migration")); + goto error; + } + ddomain = virDomainMigrateVersion2(domain, dconn, flags, + dname, uri, bandwidth); + } else if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, + VIR_DRV_FEATURE_MIGRATION_V1) && + VIR_DRV_SUPPORTS_FEATURE(dconn->driver, dconn, + VIR_DRV_FEATURE_MIGRATION_V1)) { + VIR_DEBUG("Using migration protocol 1"); + if (dxml) { + virLibConnError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to change target guest XML during migration")); + goto error; + } + ddomain = virDomainMigrateVersion1(domain, dconn, flags, + dname, uri, bandwidth); } else { /* This driver does not support any migration method */ virLibConnError(VIR_ERR_NO_SUPPORT, __FUNCTION__); @@ -4296,6 +4481,136 @@ error: } +/** + * virDomainMigrateToURI2: + * @domain: a domain object + * @dconnuri: optional URI for target libvirtd if @flags includes VIR_MIGRATE_PEER2PEER + * @miguri: optional URI for invoking the migration, not if @flags includs VIR_MIGRATE_TUNNELLED + * @dxml: optional XML config for launching guest on target + * @flags: flags + * @dname: (optional) rename domain to this at destination + * @bandwidth: (optional) specify migration bandwidth limit in Mbps + * + * Migrate the domain object from its current host to the destination + * host given by duri. + * + * Flags may be one of more of the following: + * VIR_MIGRATE_LIVE Do not pause the VM during migration + * VIR_MIGRATE_PEER2PEER Direct connection between source & destination hosts + * VIR_MIGRATE_TUNNELLED Tunnel migration data over the libvirt RPC channel + * VIR_MIGRATE_PERSIST_DEST If the migration is successful, persist the domain + * on the destination host. + * VIR_MIGRATE_UNDEFINE_SOURCE If the migration is successful, undefine the + * domain on the source host. + * + * The operation of this API hinges on the VIR_MIGRATE_PEER2PEER flag. + * + * If the VIR_MIGRATE_PEER2PEER flag is set, the @dconnuri parameter + * must be a valid libvirt connection URI, by which the source + * libvirt driver can connect to the destination libvirt. If the + * VIR_MIGRATE_PEER2PEER flag is NOT set, then @dconnuri must be + * NULL. + * + * If the VIR_MIGRATE_TUNNELLED flag is NOT set, then the @miguri + * parameter allows specification of a URI to use to initiate the + * VM migration. It takes a hypervisor specific format. The uri_transports + * element of the hypervisor capabilities XML includes details of the + * supported URI schemes. + * + * VIR_MIGRATE_TUNNELLED requires that VIR_MIGRATE_PEER2PEER be set. + * + * If a hypervisor supports changing the configuration of the guest + * during migration, the @dxml parameter specifies the new config + * for the guest. The configuration must include an identical set + * of virtual devices, to ensure a stable guest ABI across migration. + * Only parameters related to host side configuration can be + * changed in the XML. Hypervisors will validate this and refuse to + * allow migration if the provided XML would cause a change in the + * guest ABI, + * + * If a hypervisor supports renaming domains during migration, + * the dname parameter specifies the new name for the domain. + * Setting dname to NULL keeps the domain name the same. If domain + * renaming is not supported by the hypervisor, dname must be NULL or + * else an error will be returned. + * + * The maximum bandwidth (in Mbps) that will be used to do migration + * can be specified with the bandwidth parameter. If set to 0, + * libvirt will choose a suitable default. Some hypervisors do + * not support this feature and will return an error if bandwidth + * is not 0. + * + * To see which features are supported by the current hypervisor, + * see virConnectGetCapabilities, /capabilities/host/migration_features. + * + * There are many limitations on migration imposed by the underlying + * technology - for example it may not be possible to migrate between + * different processors even with the same architecture, or between + * different types of hypervisor. + * + * Returns 0 if the migration succeeded, -1 upon error. + */ +int +virDomainMigrateToURI2(virDomainPtr domain, + const char *dconnuri, + const char *miguri, + const char *dxml, + unsigned long flags, + const char *dname, + unsigned long bandwidth) +{ + VIR_DOMAIN_DEBUG(domain, "dconnuri=%s, miguri=%s, dxml=%s, " + "flags=%lu, dname=%s, bandwidth=%lu", + NULLSTR(dconnuri), NULLSTR(miguri), NULLSTR(dxml), + flags, NULLSTR(dname), bandwidth); + + virResetLastError(); + + /* First checkout the source */ + if (!VIR_IS_CONNECTED_DOMAIN (domain)) { + virLibDomainError(VIR_ERR_INVALID_DOMAIN, __FUNCTION__); + virDispatchError(NULL); + return -1; + } + if (domain->conn->flags & VIR_CONNECT_RO) { + virLibDomainError(VIR_ERR_OPERATION_DENIED, __FUNCTION__); + goto error; + } + + if (flags & VIR_MIGRATE_PEER2PEER) { + if (VIR_DRV_SUPPORTS_FEATURE (domain->conn->driver, domain->conn, + VIR_DRV_FEATURE_MIGRATION_P2P)) { + VIR_DEBUG("Using peer2peer migration"); + if (virDomainMigratePeer2Peer(domain, dxml, flags, + dname, dconnuri, miguri, bandwidth) < 0) + goto error; + } else { + /* No peer to peer migration supported */ + virLibConnError(VIR_ERR_NO_SUPPORT, __FUNCTION__); + goto error; + } + } else { + if (VIR_DRV_SUPPORTS_FEATURE (domain->conn->driver, domain->conn, + VIR_DRV_FEATURE_MIGRATION_DIRECT)) { + VIR_DEBUG("Using direct migration"); + if (virDomainMigrateDirect(domain, dxml, flags, + dname, miguri, bandwidth) < 0) + goto error; + } else { + /* Cannot do a migration with only the perform step */ + virLibConnError(VIR_ERR_NO_SUPPORT, __FUNCTION__); + goto error; + } + } + + return 0; + +error: + virDispatchError(domain->conn); + return -1; +} + + /* * Not for public use. This function is part of the internal * implementation of migration in the remote case. diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index 0590535..cd23f69 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -442,6 +442,8 @@ LIBVIRT_0.9.2 { virDomainInjectNMI; virDomainScreenshot; virDomainSetSchedulerParametersFlags; + virDomainMigrate2; + virDomainMigrateToURI2; } LIBVIRT_0.9.0; # .... define new API here using predicted next version number .... -- 1.7.4.4

2011/5/24 Daniel P. Berrange <berrange@redhat.com>:
There are two pieces of information which are desirable for migration, which cannot be supplied by applications
- The explicit QEMU migration URI, while using Peer2Peer migration - An override for the target VM XML
This introduces two new public APIs to support these extra parameters. There is no need for extra wire protocool changes, since this is supported by the v3 migration enhancements
* include/libvirt/libvirt.h.in, src/libvirt.c, src/libvirt_public.syms: Add virDomainMigrate2 and virDomainMigrateToURI2 --- include/libvirt/libvirt.h.in | 12 ++ src/libvirt.c | 321 +++++++++++++++++++++++++++++++++++++++++- src/libvirt_public.syms | 2 + 3 files changed, 332 insertions(+), 3 deletions(-)
diff --git a/src/libvirt.c b/src/libvirt.c index 2b2c1fd..4bdcd43 100644 --- a/src/libvirt.c +++ b/src/libvirt.c
+ +/** + * virDomainMigrate2: + * @domain: a domain object + * @dconn: destination host (a connection object) + * @flags: flags + * @dxml: optional XML config for launching guest on target
The other optional parameters have their optional in (). So this optional should be in () too, or the others should have their () removed.
+ * @dname: (optional) rename domain to this at destination + * @uri: (optional) dest hostname/URI as seen from the source host + * @bandwidth: (optional) specify migration bandwidth limit in Mbps + * + * Migrate the domain object from its current host to the destination + * host given by dconn (a connection to the destination host). + *
+/** + * virDomainMigrateToURI2: + * @domain: a domain object + * @dconnuri: optional URI for target libvirtd if @flags includes VIR_MIGRATE_PEER2PEER + * @miguri: optional URI for invoking the migration, not if @flags includs VIR_MIGRATE_TUNNELLED + * @dxml: optional XML config for launching guest on target
The same comment from above about the word optional applies here too.
+ * @flags: flags + * @dname: (optional) rename domain to this at destination + * @bandwidth: (optional) specify migration bandwidth limit in Mbps
ACK to be patch as it, but do we really want to add a new public API to 0.9.2 for a feature that no driver implements yet and might not implement it before 0.9.2? As I understand it the URI separation already works but the XML override isn't implemented yet. Or do you already have the XML override implemented but did post the patch yet? Matthias

On Wed, May 25, 2011 at 09:45:50AM +0200, Matthias Bolte wrote:
2011/5/24 Daniel P. Berrange <berrange@redhat.com>:
There are two pieces of information which are desirable for migration, which cannot be supplied by applications
- The explicit QEMU migration URI, while using Peer2Peer migration - An override for the target VM XML
This introduces two new public APIs to support these extra parameters. There is no need for extra wire protocool changes, since this is supported by the v3 migration enhancements
* include/libvirt/libvirt.h.in, src/libvirt.c, src/libvirt_public.syms: Add virDomainMigrate2 and virDomainMigrateToURI2 --- include/libvirt/libvirt.h.in | 12 ++ src/libvirt.c | 321 +++++++++++++++++++++++++++++++++++++++++- src/libvirt_public.syms | 2 + 3 files changed, 332 insertions(+), 3 deletions(-)
diff --git a/src/libvirt.c b/src/libvirt.c index 2b2c1fd..4bdcd43 100644 --- a/src/libvirt.c +++ b/src/libvirt.c
+ +/** + * virDomainMigrate2: + * @domain: a domain object + * @dconn: destination host (a connection object) + * @flags: flags + * @dxml: optional XML config for launching guest on target
The other optional parameters have their optional in (). So this optional should be in () too, or the others should have their () removed.
+ * @dname: (optional) rename domain to this at destination + * @uri: (optional) dest hostname/URI as seen from the source host + * @bandwidth: (optional) specify migration bandwidth limit in Mbps + * + * Migrate the domain object from its current host to the destination + * host given by dconn (a connection to the destination host). + *
+/** + * virDomainMigrateToURI2: + * @domain: a domain object + * @dconnuri: optional URI for target libvirtd if @flags includes VIR_MIGRATE_PEER2PEER + * @miguri: optional URI for invoking the migration, not if @flags includs VIR_MIGRATE_TUNNELLED + * @dxml: optional XML config for launching guest on target
The same comment from above about the word optional applies here too.
+ * @flags: flags + * @dname: (optional) rename domain to this at destination + * @bandwidth: (optional) specify migration bandwidth limit in Mbps
ACK to be patch as it, but do we really want to add a new public API to 0.9.2 for a feature that no driver implements yet and might not implement it before 0.9.2? As I understand it the URI separation already works but the XML override isn't implemented yet. Or do you already have the XML override implemented but did post the patch yet?
I don't have XML override implemented yet. QEMU returns an error if you attempt to use it. I figured that since I had made the migration wire protocol support it, we might as well have the public API too. I can work up some QEMU code without much trouble too. The only missing bit is the ability to compare two virDomainDefPtr instances and validate that they have the same guest ABI (ie same list of device models + device addresses). Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

2011/5/25 Daniel P. Berrange <berrange@redhat.com>:
On Wed, May 25, 2011 at 09:45:50AM +0200, Matthias Bolte wrote:
2011/5/24 Daniel P. Berrange <berrange@redhat.com>:
There are two pieces of information which are desirable for migration, which cannot be supplied by applications
- The explicit QEMU migration URI, while using Peer2Peer migration - An override for the target VM XML
This introduces two new public APIs to support these extra parameters. There is no need for extra wire protocool changes, since this is supported by the v3 migration enhancements
* include/libvirt/libvirt.h.in, src/libvirt.c, src/libvirt_public.syms: Add virDomainMigrate2 and virDomainMigrateToURI2 --- include/libvirt/libvirt.h.in | 12 ++ src/libvirt.c | 321 +++++++++++++++++++++++++++++++++++++++++- src/libvirt_public.syms | 2 + 3 files changed, 332 insertions(+), 3 deletions(-)
diff --git a/src/libvirt.c b/src/libvirt.c index 2b2c1fd..4bdcd43 100644 --- a/src/libvirt.c +++ b/src/libvirt.c
+ +/** + * virDomainMigrate2: + * @domain: a domain object + * @dconn: destination host (a connection object) + * @flags: flags + * @dxml: optional XML config for launching guest on target
The other optional parameters have their optional in (). So this optional should be in () too, or the others should have their () removed.
+ * @dname: (optional) rename domain to this at destination + * @uri: (optional) dest hostname/URI as seen from the source host + * @bandwidth: (optional) specify migration bandwidth limit in Mbps + * + * Migrate the domain object from its current host to the destination + * host given by dconn (a connection to the destination host). + *
+/** + * virDomainMigrateToURI2: + * @domain: a domain object + * @dconnuri: optional URI for target libvirtd if @flags includes VIR_MIGRATE_PEER2PEER + * @miguri: optional URI for invoking the migration, not if @flags includs VIR_MIGRATE_TUNNELLED + * @dxml: optional XML config for launching guest on target
The same comment from above about the word optional applies here too.
+ * @flags: flags + * @dname: (optional) rename domain to this at destination + * @bandwidth: (optional) specify migration bandwidth limit in Mbps
ACK to be patch as it, but do we really want to add a new public API to 0.9.2 for a feature that no driver implements yet and might not implement it before 0.9.2? As I understand it the URI separation already works but the XML override isn't implemented yet. Or do you already have the XML override implemented but did post the patch yet?
I don't have XML override implemented yet. QEMU returns an error if you attempt to use it. I figured that since I had made the migration wire protocol support it, we might as well have the public API too.
I can work up some QEMU code without much trouble too. The only missing bit is the ability to compare two virDomainDefPtr instances and validate that they have the same guest ABI (ie same list of device models + device addresses).
Daniel
My concern here is that we add a public API and don't implemented it in a driver for 0.9.2 and later on when we implement it we might figure out that we should have made the public API for it different, but we already released it and can't change it anymore. To be fair this concern is quite weak and I don't see how this could actually happen in this particular case, but I'd like to be on the safe side with something that we can't change easily later on anymore. So, I would appreciate it if we get this actually implemented for 0.9.2, but if not I don't consider this as a showstopper for this patch. Matthias

On Wed, May 25, 2011 at 03:09:53PM +0200, Matthias Bolte wrote:
2011/5/25 Daniel P. Berrange <berrange@redhat.com>:
On Wed, May 25, 2011 at 09:45:50AM +0200, Matthias Bolte wrote:
2011/5/24 Daniel P. Berrange <berrange@redhat.com>:
There are two pieces of information which are desirable for migration, which cannot be supplied by applications
- The explicit QEMU migration URI, while using Peer2Peer migration - An override for the target VM XML
This introduces two new public APIs to support these extra parameters. There is no need for extra wire protocool changes, since this is supported by the v3 migration enhancements
* include/libvirt/libvirt.h.in, src/libvirt.c, src/libvirt_public.syms: Add virDomainMigrate2 and virDomainMigrateToURI2 --- include/libvirt/libvirt.h.in | 12 ++ src/libvirt.c | 321 +++++++++++++++++++++++++++++++++++++++++- src/libvirt_public.syms | 2 + 3 files changed, 332 insertions(+), 3 deletions(-)
diff --git a/src/libvirt.c b/src/libvirt.c index 2b2c1fd..4bdcd43 100644 --- a/src/libvirt.c +++ b/src/libvirt.c
+ +/** + * virDomainMigrate2: + * @domain: a domain object + * @dconn: destination host (a connection object) + * @flags: flags + * @dxml: optional XML config for launching guest on target
The other optional parameters have their optional in (). So this optional should be in () too, or the others should have their () removed.
+ * @dname: (optional) rename domain to this at destination + * @uri: (optional) dest hostname/URI as seen from the source host + * @bandwidth: (optional) specify migration bandwidth limit in Mbps + * + * Migrate the domain object from its current host to the destination + * host given by dconn (a connection to the destination host). + *
+/** + * virDomainMigrateToURI2: + * @domain: a domain object + * @dconnuri: optional URI for target libvirtd if @flags includes VIR_MIGRATE_PEER2PEER + * @miguri: optional URI for invoking the migration, not if @flags includs VIR_MIGRATE_TUNNELLED + * @dxml: optional XML config for launching guest on target
The same comment from above about the word optional applies here too.
+ * @flags: flags + * @dname: (optional) rename domain to this at destination + * @bandwidth: (optional) specify migration bandwidth limit in Mbps
ACK to be patch as it, but do we really want to add a new public API to 0.9.2 for a feature that no driver implements yet and might not implement it before 0.9.2? As I understand it the URI separation already works but the XML override isn't implemented yet. Or do you already have the XML override implemented but did post the patch yet?
I don't have XML override implemented yet. QEMU returns an error if you attempt to use it. I figured that since I had made the migration wire protocol support it, we might as well have the public API too.
I can work up some QEMU code without much trouble too. The only missing bit is the ability to compare two virDomainDefPtr instances and validate that they have the same guest ABI (ie same list of device models + device addresses).
Daniel
My concern here is that we add a public API and don't implemented it in a driver for 0.9.2 and later on when we implement it we might figure out that we should have made the public API for it different, but we already released it and can't change it anymore. To be fair this concern is quite weak and I don't see how this could actually happen in this particular case, but I'd like to be on the safe side with something that we can't change easily later on anymore.
Even without the public API addition, the previous patches add the functionality at the wire protocol, so we're tied down in that respect already.
So, I would appreciate it if we get this actually implemented for 0.9.2, but if not I don't consider this as a showstopper for this patch.
I'll work on a QEMU impl for it asap. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

Before running perform in peer-2-peer migration, the current guest state must be recorded, so that non-live migration can currently unpause a running guest on completion. * src/qemu/qemu_migration.c: Move check for offline guest to fix non-live migration --- src/qemu/qemu_migration.c | 19 ++++++++++++++----- 1 files changed, 14 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 2e2ccd5..d0e8e14 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1313,6 +1313,13 @@ static int doNativeMigrate(struct qemud_driver *driver, goto cleanup; } + /* Before EnterMonitor, since qemuProcessStopCPUs already does that */ + if (!(flags & VIR_MIGRATE_LIVE) && + virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING) { + if (qemuMigrationSetOffline(driver, vm) < 0) + goto cleanup; + } + qemuDomainObjEnterMonitorWithDriver(driver, vm); if (resource > 0 && qemuMonitorSetMigrationSpeed(priv->mon, resource) < 0) { @@ -1540,6 +1547,13 @@ static int doTunnelMigrate(struct qemud_driver *driver, VIR_WARN("unable to provide data for graphics client relocation"); /* 3. start migration on source */ + /* Before EnterMonitor, since qemuProcessStopCPUs already does that */ + if (!(flags & VIR_MIGRATE_LIVE) && + virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING) { + if (qemuMigrationSetOffline(driver, vm) < 0) + goto cleanup; + } + qemuDomainObjEnterMonitorWithDriver(driver, vm); if (resource > 0 && qemuMonitorSetMigrationSpeed(priv->mon, resource) < 0) { @@ -2048,11 +2062,6 @@ int qemuMigrationPerform(struct qemud_driver *driver, priv->jobInfo.type = VIR_DOMAIN_JOB_UNBOUNDED; resume = virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING; - if (!(flags & VIR_MIGRATE_LIVE) && - virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING) { - if (qemuMigrationSetOffline(driver, vm) < 0) - goto endjob; - } if ((flags & (VIR_MIGRATE_TUNNELLED | VIR_MIGRATE_PEER2PEER))) { if (cookieinlen) { -- 1.7.4.4

2011/5/24 Daniel P. Berrange <berrange@redhat.com>:
Before running perform in peer-2-peer migration, the current guest state must be recorded, so that non-live migration can currently unpause a running guest on completion.
* src/qemu/qemu_migration.c: Move check for offline guest to fix non-live migration --- src/qemu/qemu_migration.c | 19 ++++++++++++++----- 1 files changed, 14 insertions(+), 5 deletions(-)
ACK. Matthias

--- src/qemu/qemu_migration.c | 34 ++++++++++++++++++++-------------- 1 files changed, 20 insertions(+), 14 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index d0e8e14..fa1a75b 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -79,8 +79,10 @@ struct _qemuMigrationCookie { int flagsMandatory; /* Host properties */ - unsigned char hostuuid[VIR_UUID_BUFLEN]; - char *hostname; + unsigned char localHostuuid[VIR_UUID_BUFLEN]; + unsigned char remoteHostuuid[VIR_UUID_BUFLEN]; + char *localHostname; + char *remoteHostname; /* Guest properties */ unsigned char uuid[VIR_UUID_BUFLEN]; @@ -108,7 +110,8 @@ static void qemuMigrationCookieFree(qemuMigrationCookiePtr mig) if (mig->flags & QEMU_MIGRATION_COOKIE_GRAPHICS) qemuMigrationCookieGraphicsFree(mig->graphics); - VIR_FREE(mig->hostname); + VIR_FREE(mig->localHostname); + VIR_FREE(mig->remoteHostname); VIR_FREE(mig->name); VIR_FREE(mig); } @@ -233,9 +236,9 @@ qemuMigrationCookieNew(virDomainObjPtr dom) goto no_memory; memcpy(mig->uuid, dom->def->uuid, VIR_UUID_BUFLEN); - if (!(mig->hostname = virGetHostname(NULL))) + if (!(mig->localHostname = virGetHostname(NULL))) goto no_memory; - if (virGetHostUUID(mig->hostuuid) < 0) { + if (virGetHostUUID(mig->localHostuuid) < 0) { qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Unable to obtain host UUID")); goto error; @@ -301,12 +304,12 @@ static void qemuMigrationCookieXMLFormat(virBufferPtr buf, int i; virUUIDFormat(mig->uuid, uuidstr); - virUUIDFormat(mig->hostuuid, hostuuidstr); + virUUIDFormat(mig->localHostuuid, hostuuidstr); virBufferAsprintf(buf, "<qemu-migration>\n"); virBufferEscapeString(buf, " <name>%s</name>\n", mig->name); virBufferAsprintf(buf, " <uuid>%s</uuid>\n", uuidstr); - virBufferEscapeString(buf, " <hostname>%s</hostname>\n", mig->hostname); + virBufferEscapeString(buf, " <hostname>%s</hostname>\n", mig->localHostname); virBufferAsprintf(buf, " <hostuuid>%s</hostuuid>\n", hostuuidstr); for (i = 0 ; i < QEMU_MIGRATION_COOKIE_FLAG_LAST ; i++) { @@ -434,26 +437,29 @@ qemuMigrationCookieXMLParse(qemuMigrationCookiePtr mig, VIR_FREE(tmp); /* Check & forbid "localhost" migration */ - if (!(tmp = virXPathString("string(./hostname[1])", ctxt))) { + if (!(mig->remoteHostname = virXPathString("string(./hostname[1])", ctxt))) { qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("missing hostname element in migration data")); goto error; } - if (STREQ(tmp, mig->hostname)) { + if (STREQ(mig->remoteHostname, mig->localHostname)) { qemuReportError(VIR_ERR_INTERNAL_ERROR, _("Attempt to migrate guest to the same host %s"), - tmp); + mig->remoteHostname); goto error; } - VIR_FREE(tmp); if (!(tmp = virXPathString("string(./hostuuid[1])", ctxt))) { qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("missing hostuuid element in migration data")); goto error; } - virUUIDFormat(mig->hostuuid, uuidstr); - if (STREQ(tmp, uuidstr)) { + if (virUUIDParse(tmp, mig->remoteHostuuid) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("malformed hostuuid element in migration data")); + goto error; + } + if (memcmp(mig->remoteHostuuid, mig->localHostuuid, VIR_UUID_BUFLEN) == 0) { qemuReportError(VIR_ERR_INTERNAL_ERROR, _("Attempt to migrate guest to the same host %s"), tmp); @@ -851,7 +857,7 @@ qemuDomainMigrateGraphicsRelocate(struct qemud_driver *driver, qemuDomainObjEnterMonitorWithDriver(driver, vm); ret = qemuMonitorGraphicsRelocate(priv->mon, cookie->graphics->type, - cookie->hostname, + cookie->remoteHostname, cookie->graphics->port, cookie->graphics->tlsPort, cookie->graphics->tlsSubject); -- 1.7.4.4

2011/5/24 Daniel P. Berrange <berrange@redhat.com>:
--- src/qemu/qemu_migration.c | 34 ++++++++++++++++++++-------------- 1 files changed, 20 insertions(+), 14 deletions(-)
@@ -434,26 +437,29 @@ qemuMigrationCookieXMLParse(qemuMigrationCookiePtr mig, VIR_FREE(tmp);
/* Check & forbid "localhost" migration */ - if (!(tmp = virXPathString("string(./hostname[1])", ctxt))) { + if (!(mig->remoteHostname = virXPathString("string(./hostname[1])", ctxt))) { qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("missing hostname element in migration data")); goto error; } - if (STREQ(tmp, mig->hostname)) { + if (STREQ(mig->remoteHostname, mig->localHostname)) { qemuReportError(VIR_ERR_INTERNAL_ERROR, _("Attempt to migrate guest to the same host %s"), - tmp); + mig->remoteHostname); goto error; } - VIR_FREE(tmp);
if (!(tmp = virXPathString("string(./hostuuid[1])", ctxt))) { qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("missing hostuuid element in migration data")); goto error; } - virUUIDFormat(mig->hostuuid, uuidstr); - if (STREQ(tmp, uuidstr)) { + if (virUUIDParse(tmp, mig->remoteHostuuid) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("malformed hostuuid element in migration data")); + goto error; + } + if (memcmp(mig->remoteHostuuid, mig->localHostuuid, VIR_UUID_BUFLEN) == 0) { qemuReportError(VIR_ERR_INTERNAL_ERROR, _("Attempt to migrate guest to the same host %s"), tmp);
It took me a moment to figure out what the problem here is and how this patch fixes it. So I suggest you add some explanation to the commit message. The problem is that qemuMigrationCookieXMLParse doesn't really parse the XML formatted cookie into the qemuMigrationCookiePtr pointer but just compares and validates the XML against the local cookie. But in qemuDomainMigrateGraphicsRelocate this local cookie is expected to contain the remote hostname that was never inserted in this cookie.
@@ -851,7 +857,7 @@ qemuDomainMigrateGraphicsRelocate(struct qemud_driver *driver, qemuDomainObjEnterMonitorWithDriver(driver, vm); ret = qemuMonitorGraphicsRelocate(priv->mon, cookie->graphics->type, - cookie->hostname, + cookie->remoteHostname, cookie->graphics->port, cookie->graphics->tlsPort, cookie->graphics->tlsSubject);
This patch fixes that problem by making qemuMigrationCookieXMLParse storing the remote hostname in the local cookie. ACK, with an extended commit message. Matthias

On 05/25/2011 02:37 AM, Matthias Bolte wrote:
It took me a moment to figure out what the problem here is and how this patch fixes it. So I suggest you add some explanation to the commit message.
Also, while touching the commit message, s/seemless/seamless/
ACK, with an extended commit message.
-- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

Even when failing to start CPUs, the finish method was returning a success result. Fix this so that the QEMU process is killed off when finish fails under v3 protocol. Also rename the killOnFinish boolean to 'v3proto' to make it clearer that this is a tunable based on the migration protocol version * src/qemu/qemu_driver.c: Update for API change * src/qemu/qemu_migration.c, src/qemu/qemu_migration.h: Kill VM in qemuMigrationFinish if failing to start CPUs --- src/qemu/qemu_driver.c | 8 ++++---- src/qemu/qemu_migration.c | 37 ++++++++++++++++++++++++++++++++----- src/qemu/qemu_migration.h | 5 +++-- 3 files changed, 39 insertions(+), 11 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 978a63a..43425e6 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5971,7 +5971,7 @@ qemudDomainMigratePerform (virDomainPtr dom, ret = qemuMigrationPerform(driver, dom->conn, vm, NULL, dconnuri, uri, cookie, cookielen, NULL, NULL, /* No output cookies in v2 */ - flags, dname, resource, true); + flags, dname, resource, false); cleanup: qemuDriverUnlock(driver); @@ -6020,7 +6020,7 @@ qemudDomainMigrateFinish2 (virConnectPtr dconn, */ dom = qemuMigrationFinish(driver, dconn, vm, NULL, 0, NULL, NULL, /* No cookies */ - flags, retcode); + flags, retcode, false); cleanup: if (orig_err) { @@ -6222,7 +6222,7 @@ qemuDomainMigratePerform3(virDomainPtr dom, ret = qemuMigrationPerform(driver, dom->conn, vm, xmlin, dconnuri, uri, cookiein, cookieinlen, cookieout, cookieoutlen, - flags, dname, resource, false); + flags, dname, resource, true); cleanup: qemuDriverUnlock(driver); @@ -6271,7 +6271,7 @@ qemuDomainMigrateFinish3(virConnectPtr dconn, *newdom = qemuMigrationFinish(driver, dconn, vm, cookiein, cookieinlen, cookieout, cookieoutlen, - flags, cancelled); + flags, cancelled, true); ret = 0; diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index fa1a75b..4388214 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -2047,7 +2047,7 @@ int qemuMigrationPerform(struct qemud_driver *driver, unsigned long flags, const char *dname, unsigned long resource, - bool killOnFinish) + bool v3proto) { virDomainEventPtr event = NULL; int ret = -1; @@ -2092,8 +2092,13 @@ int qemuMigrationPerform(struct qemud_driver *driver, goto endjob; } - /* Clean up the source domain. */ - if (killOnFinish) { + /* + * In v3 protocol, the source VM is not killed off until the + * confirm step. + */ + if (v3proto) { + resume = 0; + } else { qemuProcessStop(driver, vm, 1, VIR_DOMAIN_SHUTOFF_MIGRATED); qemuAuditDomainStop(vm, "migrated"); resume = 0; @@ -2193,7 +2198,8 @@ qemuMigrationFinish(struct qemud_driver *driver, char **cookieout, int *cookieoutlen, unsigned long flags, - int retcode) + int retcode, + bool v3proto) { virDomainPtr dom = NULL; virDomainEventPtr event = NULL; @@ -2257,7 +2263,6 @@ qemuMigrationFinish(struct qemud_driver *driver, event = NULL; } - dom = virGetDomain (dconn, vm->def->name, vm->def->uuid); if (!(flags & VIR_MIGRATE_PAUSED)) { /* run 'cont' on the destination, which allows migration on qemu @@ -2266,6 +2271,26 @@ qemuMigrationFinish(struct qemud_driver *driver, */ if (qemuProcessStartCPUs(driver, vm, dconn, VIR_DOMAIN_RUNNING_MIGRATED) < 0) { + /* + * In v3 protocol, the source VM is still available to + * restart during confirm() step, so we kill it off + * now. + * In v2 protocol, the source is dead, so we leave + * target in paused state, in case admin can fix + * things up + */ + if (v3proto) { + qemuProcessStop(driver, vm, 1, VIR_DOMAIN_SHUTOFF_FAILED); + qemuAuditDomainStop(vm, "failed"); + event = virDomainEventNewFromObj(vm, + VIR_DOMAIN_EVENT_STOPPED, + VIR_DOMAIN_EVENT_STOPPED_FAILED); + if (!vm->persistent) { + if (qemuDomainObjEndJob(vm) > 0) + virDomainRemoveInactive(&driver->domains, vm); + vm = NULL; + } + } if (virGetLastError() == NULL) qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("resume operation failed")); @@ -2273,6 +2298,8 @@ qemuMigrationFinish(struct qemud_driver *driver, } } + dom = virGetDomain (dconn, vm->def->name, vm->def->uuid); + event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_RESUMED, VIR_DOMAIN_EVENT_RESUMED_MIGRATED); diff --git a/src/qemu/qemu_migration.h b/src/qemu/qemu_migration.h index 08e3acc..d3a3743 100644 --- a/src/qemu/qemu_migration.h +++ b/src/qemu/qemu_migration.h @@ -72,7 +72,7 @@ int qemuMigrationPerform(struct qemud_driver *driver, unsigned long flags, const char *dname, unsigned long resource, - bool killOnFinish); + bool v3proto); virDomainPtr qemuMigrationFinish(struct qemud_driver *driver, virConnectPtr dconn, @@ -82,7 +82,8 @@ virDomainPtr qemuMigrationFinish(struct qemud_driver *driver, char **cookieout, int *cookieoutlen, unsigned long flags, - int retcode); + int retcode, + bool v3proto); int qemuMigrationConfirm(struct qemud_driver *driver, virConnectPtr conn, -- 1.7.4.4

2011/5/24 Daniel P. Berrange <berrange@redhat.com>:
Even when failing to start CPUs, the finish method was returning a success result. Fix this so that the QEMU process is killed off when finish fails under v3 protocol. Also rename the killOnFinish boolean to 'v3proto' to make it clearer that this is a tunable based on the migration protocol version
* src/qemu/qemu_driver.c: Update for API change * src/qemu/qemu_migration.c, src/qemu/qemu_migration.h: Kill VM in qemuMigrationFinish if failing to start CPUs --- src/qemu/qemu_driver.c | 8 ++++---- src/qemu/qemu_migration.c | 37 ++++++++++++++++++++++++++++++++----- src/qemu/qemu_migration.h | 5 +++-- 3 files changed, 39 insertions(+), 11 deletions(-)
ACK. Matthias

* src/libvirt.c: Add further debug lines in helper APIs for migration * src/qemu/qemu_migration.c: Add debug lines for all internal migration API parameters --- src/libvirt.c | 19 ++++++++++++++++- src/qemu/qemu_migration.c | 47 ++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 63 insertions(+), 3 deletions(-) diff --git a/src/libvirt.c b/src/libvirt.c index 4bdcd43..baff80b 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -3514,6 +3514,8 @@ virDomainMigrateVersion1 (virDomainPtr domain, char *cookie = NULL; int cookielen = 0, ret; virDomainInfo info; + VIR_DOMAIN_DEBUG(domain, "dconn=%p flags=%lu, dname=%s, uri=%s, bandwidth=%lu", + dconn, flags, NULLSTR(dname), NULLSTR(uri), bandwidth); ret = virDomainGetInfo (domain, &info); if (ret == 0 && info.state == VIR_DOMAIN_PAUSED) { @@ -3604,6 +3606,8 @@ virDomainMigrateVersion2 (virDomainPtr domain, virDomainInfo info; virErrorPtr orig_err = NULL; int cancelled; + VIR_DOMAIN_DEBUG(domain, "dconn=%p flags=%lu, dname=%s, uri=%s, bandwidth=%lu", + dconn, flags, NULLSTR(dname), NULLSTR(uri), bandwidth); /* Prepare the migration. * @@ -3637,7 +3641,7 @@ virDomainMigrateVersion2 (virDomainPtr domain, flags |= VIR_MIGRATE_PAUSED; } - VIR_DEBUG("Prepare2 %p", dconn); + VIR_DEBUG("Prepare2 %p flags=%lu", dconn, flags); ret = dconn->driver->domainMigratePrepare2 (dconn, &cookie, &cookielen, uri, &uri_out, flags, dname, bandwidth, dom_xml); @@ -3736,6 +3740,10 @@ virDomainMigrateVersion3(virDomainPtr domain, virDomainInfo info; virErrorPtr orig_err = NULL; int cancelled; + VIR_DOMAIN_DEBUG(domain, "dconn=%p xmlin=%s, flags=%lu, " + "dname=%s, uri=%s, bandwidth=%lu", + dconn, NULLSTR(xmlin), flags, + NULLSTR(dname), NULLSTR(uri), bandwidth); if (!domain->conn->driver->domainMigrateBegin3 || !domain->conn->driver->domainMigratePerform3 || @@ -3759,7 +3767,7 @@ virDomainMigrateVersion3(virDomainPtr domain, flags |= VIR_MIGRATE_PAUSED; } - VIR_DEBUG("Prepare3 %p", dconn); + VIR_DEBUG("Prepare3 %p flags=%lu", dconn, flags); cookiein = cookieout; cookieinlen = cookieoutlen; cookieout = NULL; @@ -3883,6 +3891,10 @@ virDomainMigratePeer2Peer (virDomainPtr domain, unsigned long bandwidth) { xmlURIPtr tempuri = NULL; + VIR_DOMAIN_DEBUG(domain, "xmlin=%s, flags=%lu, dname=%s, " + "dconnuri=%s, uri=%s, bandwidth=%lu", + NULLSTR(xmlin), flags, NULLSTR(dname), + NULLSTR(dconnuri), NULLSTR(uri), bandwidth); if (!domain->conn->driver->domainMigratePerform) { virLibConnError(VIR_ERR_NO_SUPPORT, __FUNCTION__); @@ -3964,6 +3976,9 @@ virDomainMigrateDirect (virDomainPtr domain, const char *uri, unsigned long bandwidth) { + VIR_DOMAIN_DEBUG(domain, "xmlin=%s, flags=%lu, dname=%s, uri=%s, bandwidth=%lu", + NULLSTR(xmlin), flags, NULLSTR(dname), NULLSTR(uri), bandwidth); + if (!domain->conn->driver->domainMigratePerform) { virLibConnError(VIR_ERR_NO_SUPPORT, __FUNCTION__); virDispatchError(domain->conn); diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 4388214..280b8cb 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -875,6 +875,8 @@ char *qemuMigrationBegin(struct qemud_driver *driver, { char *rv = NULL; qemuMigrationCookiePtr mig = NULL; + VIR_DEBUG("driver=%p, vm=%p, xmlin=%s, cookieout=%p, cookieoutlen=%p", + driver, vm, NULLSTR(xmlin), cookieout, cookieoutlen); if (xmlin) { qemuReportError(VIR_ERR_INTERNAL_ERROR, @@ -935,6 +937,10 @@ qemuMigrationPrepareTunnel(struct qemud_driver *driver, qemuDomainObjPrivatePtr priv = NULL; struct timeval now; qemuMigrationCookiePtr mig = NULL; + VIR_DEBUG("driver=%p, dconn=%p, cookiein=%s, cookieinlen=%d, " + "cookieout=%p, cookieoutlen=%p, st=%p, dname=%s, dom_xml=%s", + driver, dconn, NULLSTR(cookiein), cookieinlen, + cookieout, cookieoutlen, st, NULLSTR(dname), dom_xml); if (gettimeofday(&now, NULL) < 0) { virReportSystemError(errno, "%s", @@ -1090,6 +1096,12 @@ qemuMigrationPrepareDirect(struct qemud_driver *driver, qemuDomainObjPrivatePtr priv = NULL; struct timeval now; qemuMigrationCookiePtr mig = NULL; + VIR_DEBUG("driver=%p, dconn=%p, cookiein=%s, cookieinlen=%d, " + "cookieout=%p, cookieoutlen=%p, uri_in=%s, uri_out=%p, " + "dname=%s, dom_xml=%s", + driver, dconn, NULLSTR(cookiein), cookieinlen, + cookieout, cookieoutlen, NULLSTR(uri_in), uri_out, + NULLSTR(dname), dom_xml); if (gettimeofday(&now, NULL) < 0) { virReportSystemError(errno, "%s", @@ -1292,6 +1304,10 @@ static int doNativeMigrate(struct qemud_driver *driver, qemuDomainObjPrivatePtr priv = vm->privateData; unsigned int background_flags = QEMU_MONITOR_MIGRATE_BACKGROUND; qemuMigrationCookiePtr mig = NULL; + VIR_DEBUG("driver=%p, vm=%p, uri=%s, cookiein=%s, cookieinlen=%d, " + "cookieout=%p, cookieoutlen=%p, flags=%u, dname=%s, resource=%lu", + driver, vm, uri, NULLSTR(cookiein), cookieinlen, + cookieout, cookieoutlen, flags, dname, resource); if (!(mig = qemuMigrationEatCookie(vm, cookiein, cookieinlen, QEMU_MIGRATION_COOKIE_GRAPHICS))) @@ -1485,6 +1501,10 @@ static int doTunnelMigrate(struct qemud_driver *driver, int ret = -1; qemuMigrationCookiePtr mig = NULL; qemuMigrationIOThreadPtr iothread = NULL; + VIR_DEBUG("driver=%p, vm=%p, st=%p, cookiein=%s, cookieinlen=%d, " + "cookieout=%p, cookieoutlen=%p, flags=%lu, resource=%lu", + driver, vm, st, NULLSTR(cookiein), cookieinlen, + cookieout, cookieoutlen, flags, resource); if (!qemuCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATE_QEMU_UNIX) && !qemuCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATE_QEMU_EXEC)) { @@ -1685,6 +1705,10 @@ static int doPeer2PeerMigrate2(struct qemud_driver *driver, virErrorPtr orig_err = NULL; int cancelled; virStreamPtr st = NULL; + VIR_DEBUG("driver=%p, sconn=%p, dconn=%p, vm=%p, dconnuri=%s, " + "flags=%lu, dname=%s, resource=%lu", + driver, sconn, dconn, vm, NULLSTR(dconnuri), + flags, dname, resource); /* In version 2 of the protocol, the prepare step is slightly * different. We fetch the domain XML of the source domain @@ -1824,8 +1848,11 @@ static int doPeer2PeerMigrate3(struct qemud_driver *driver, virErrorPtr orig_err = NULL; int cancelled; virStreamPtr st = NULL; + VIR_DEBUG("driver=%p, sconn=%p, dconn=%p, vm=%p, xmlin=%s, " + "dconnuri=%s, uri=%s, flags=%lu, dname=%s, resource=%lu", + driver, sconn, dconn, vm, NULLSTR(xmlin), + NULLSTR(dconnuri), NULLSTR(uri), flags, dname, resource); - VIR_DEBUG("Begin3 %p", sconn); dom_xml = qemuMigrationBegin(driver, vm, xmlin, &cookieout, &cookieoutlen); if (!dom_xml) @@ -1983,6 +2010,10 @@ static int doPeer2PeerMigrate(struct qemud_driver *driver, virConnectPtr dconn = NULL; bool p2p; bool v3; + VIR_DEBUG("driver=%p, sconn=%p, vm=%p, xmlin=%s, dconnuri=%s, " + "uri=%s, flags=%lu, dname=%s, resource=%lu", + driver, sconn, vm, NULLSTR(xmlin), NULLSTR(dconnuri), + NULLSTR(uri), flags, dname, resource); /* the order of operations is important here; we make sure the * destination side is completely setup before we touch the source @@ -2053,6 +2084,12 @@ int qemuMigrationPerform(struct qemud_driver *driver, int ret = -1; int resume = 0; qemuDomainObjPrivatePtr priv = vm->privateData; + VIR_DEBUG("driver=%p, conn=%p, vm=%p, xmlin=%s, dconnuri=%s, " + "uri=%s, cookiein=%s, cookieinlen=%d, cookieout=%p, " + "cookieoutlen=%p, flags=%lu, dname=%s, resource=%lu, v3proto=%d", + driver, conn, vm, NULLSTR(xmlin), NULLSTR(dconnuri), + NULLSTR(uri), NULLSTR(cookiein), cookieinlen, + cookieout, cookieoutlen, flags, dname, resource, v3proto); if (qemuDomainObjBeginJobWithDriver(driver, vm) < 0) goto cleanup; @@ -2206,6 +2243,10 @@ qemuMigrationFinish(struct qemud_driver *driver, int newVM = 1; qemuDomainObjPrivatePtr priv = NULL; qemuMigrationCookiePtr mig = NULL; + VIR_DEBUG("driver=%p, dconn=%p, vm=%p, cookiein=%s, cookieinlen=%d, " + "cookieout=%p, cookieoutlen=%p, flags=%lu, retcode=%d", + driver, dconn, vm, NULLSTR(cookiein), cookieinlen, + cookieout, cookieoutlen, flags, retcode); priv = vm->privateData; if (priv->jobActive != QEMU_JOB_MIGRATION_IN) { @@ -2356,6 +2397,10 @@ int qemuMigrationConfirm(struct qemud_driver *driver, qemuMigrationCookiePtr mig; virDomainEventPtr event = NULL; int rv = -1; + VIR_DEBUG("driver=%p, conn=%p, vm=%p, cookiein=%s, cookieinlen=%d, " + "flags=%u, retcode=%d", + driver, conn, vm, NULLSTR(cookiein), cookieinlen, + flags, retcode); if (!(mig = qemuMigrationEatCookie(vm, cookiein, cookieinlen, 0))) return -1; -- 1.7.4.4

2011/5/24 Daniel P. Berrange <berrange@redhat.com>:
* src/libvirt.c: Add further debug lines in helper APIs for migration * src/qemu/qemu_migration.c: Add debug lines for all internal migration API parameters --- src/libvirt.c | 19 ++++++++++++++++- src/qemu/qemu_migration.c | 47 ++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 63 insertions(+), 3 deletions(-)
@@ -1292,6 +1304,10 @@ static int doNativeMigrate(struct qemud_driver *driver, qemuDomainObjPrivatePtr priv = vm->privateData; unsigned int background_flags = QEMU_MONITOR_MIGRATE_BACKGROUND; qemuMigrationCookiePtr mig = NULL; + VIR_DEBUG("driver=%p, vm=%p, uri=%s, cookiein=%s, cookieinlen=%d, " + "cookieout=%p, cookieoutlen=%p, flags=%u, dname=%s, resource=%lu", + driver, vm, uri, NULLSTR(cookiein), cookieinlen, + cookieout, cookieoutlen, flags, dname, resource);
dname can be NULL, needs to be NULLSTR(dname).
@@ -1685,6 +1705,10 @@ static int doPeer2PeerMigrate2(struct qemud_driver *driver, virErrorPtr orig_err = NULL; int cancelled; virStreamPtr st = NULL; + VIR_DEBUG("driver=%p, sconn=%p, dconn=%p, vm=%p, dconnuri=%s, " + "flags=%lu, dname=%s, resource=%lu", + driver, sconn, dconn, vm, NULLSTR(dconnuri), + flags, dname, resource);
dname can be NULL, needs to be NULLSTR(dname).
@@ -1824,8 +1848,11 @@ static int doPeer2PeerMigrate3(struct qemud_driver *driver, virErrorPtr orig_err = NULL; int cancelled; virStreamPtr st = NULL; + VIR_DEBUG("driver=%p, sconn=%p, dconn=%p, vm=%p, xmlin=%s, " + "dconnuri=%s, uri=%s, flags=%lu, dname=%s, resource=%lu", + driver, sconn, dconn, vm, NULLSTR(xmlin), + NULLSTR(dconnuri), NULLSTR(uri), flags, dname, resource);
dname can be NULL, needs to be NULLSTR(dname).
@@ -1983,6 +2010,10 @@ static int doPeer2PeerMigrate(struct qemud_driver *driver, virConnectPtr dconn = NULL; bool p2p; bool v3; + VIR_DEBUG("driver=%p, sconn=%p, vm=%p, xmlin=%s, dconnuri=%s, " + "uri=%s, flags=%lu, dname=%s, resource=%lu", + driver, sconn, vm, NULLSTR(xmlin), NULLSTR(dconnuri), + NULLSTR(uri), flags, dname, resource);
And again dname.
@@ -2053,6 +2084,12 @@ int qemuMigrationPerform(struct qemud_driver *driver, int ret = -1; int resume = 0; qemuDomainObjPrivatePtr priv = vm->privateData; + VIR_DEBUG("driver=%p, conn=%p, vm=%p, xmlin=%s, dconnuri=%s, " + "uri=%s, cookiein=%s, cookieinlen=%d, cookieout=%p, " + "cookieoutlen=%p, flags=%lu, dname=%s, resource=%lu, v3proto=%d", + driver, conn, vm, NULLSTR(xmlin), NULLSTR(dconnuri), + NULLSTR(uri), NULLSTR(cookiein), cookieinlen, + cookieout, cookieoutlen, flags, dname, resource, v3proto);
And another dname. ACK, with this dnames wrapped in NULLSTR. Matthias

When doing migration, if an error occurs in Perform, it must not be overwritten during Finish/Confirm steps. If an error occurs in Finish, it must not be overwritten in Confirm. Previous commit a9d12c2444e43a0d3e5135eb15b4b62a7c011427 added code to qemudDomainMigrateFinish2 to preserve the error. This is not the right place, because it is not applicable in non-p2p migration. The src/libvirt.c virDomainMigrateV2/3 methods need code to preserve errors for non-p2p migration, while the doPeer2PeerMigrate2 and doPeer2PeerMigrate3 methods contain code to preverse errors for p2p migration. Remove the bogus error preservation from qemudDomainMigrateFinish2 and qemudDomainMigrateFinish3. Fix virDomainMigrateV3 and doPeer2PeerMigrate3 so that they preserve any error hit during the Finish3 step, before invoking Confirm3. Finally if qemuMigrationFinish fails to resume the CPUs, it must preserve the error before tearing down the VM, so that VM cleanup doesn't overwrite it. * src/libvirt.c: Preserve error before invoking Confirm3 * src/qemu/qemu_driver.c: Remove bogus error preservation code in qemudDomainMigrateFinish2/qemudDomainMigrateFinish3 * src/qemu/qemu_migration.c: Preserve error before invoking Confirm3 and after resume fails in qemuMigrationFinish. --- src/libvirt.c | 6 ++++++ src/qemu/qemu_driver.c | 24 ------------------------ src/qemu/qemu_migration.c | 23 ++++++++++++++++++++--- 3 files changed, 26 insertions(+), 27 deletions(-) diff --git a/src/libvirt.c b/src/libvirt.c index baff80b..e714468 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -3842,6 +3842,12 @@ finish: */ cancelled = ret == 0 && ddomain == NULL ? 1 : 0; + /* If finish3 set an error, and we don't have an earlier + * one we need to preserve it in case confirm3 overwrites + */ + if (!orig_err) + orig_err = virSaveLastError(); + /* * If cancelled, then src VM will be restarted, else * it will be killed diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 43425e6..877f86f 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5992,7 +5992,6 @@ qemudDomainMigrateFinish2 (virConnectPtr dconn, struct qemud_driver *driver = dconn->privateData; virDomainObjPtr vm; virDomainPtr dom = NULL; - virErrorPtr orig_err; virCheckFlags(VIR_MIGRATE_LIVE | VIR_MIGRATE_PEER2PEER | @@ -6003,9 +6002,6 @@ qemudDomainMigrateFinish2 (virConnectPtr dconn, VIR_MIGRATE_NON_SHARED_DISK | VIR_MIGRATE_NON_SHARED_INC, NULL); - /* Migration failed. Save the current error so nothing squashes it */ - orig_err = virSaveLastError(); - qemuDriverLock(driver); vm = virDomainFindByName(&driver->domains, dname); if (!vm) { @@ -6023,10 +6019,6 @@ qemudDomainMigrateFinish2 (virConnectPtr dconn, flags, retcode, false); cleanup: - if (orig_err) { - virSetError(orig_err); - virFreeError(orig_err); - } qemuDriverUnlock(driver); return dom; } @@ -6245,7 +6237,6 @@ qemuDomainMigrateFinish3(virConnectPtr dconn, { struct qemud_driver *driver = dconn->privateData; virDomainObjPtr vm; - virErrorPtr orig_err; int ret = -1; virCheckFlags(VIR_MIGRATE_LIVE | @@ -6257,9 +6248,6 @@ qemuDomainMigrateFinish3(virConnectPtr dconn, VIR_MIGRATE_NON_SHARED_DISK | VIR_MIGRATE_NON_SHARED_INC, -1); - /* Migration failed. Save the current error so nothing squashes it */ - orig_err = virSaveLastError(); - qemuDriverLock(driver); vm = virDomainFindByName(&driver->domains, dname); if (!vm) { @@ -6276,10 +6264,6 @@ qemuDomainMigrateFinish3(virConnectPtr dconn, ret = 0; cleanup: - if (orig_err) { - virSetError(orig_err); - virFreeError(orig_err); - } qemuDriverUnlock(driver); return ret; } @@ -6294,7 +6278,6 @@ qemuDomainMigrateConfirm3(virDomainPtr domain, struct qemud_driver *driver = domain->conn->privateData; virDomainObjPtr vm; int ret = -1; - virErrorPtr orig_err; virCheckFlags(VIR_MIGRATE_LIVE | VIR_MIGRATE_PEER2PEER | @@ -6322,8 +6305,6 @@ qemuDomainMigrateConfirm3(virDomainPtr domain, cookiein, cookieinlen, flags, cancelled); - orig_err = virSaveLastError(); - if (qemuDomainObjEndJob(vm) == 0) { vm = NULL; } else if (!virDomainObjIsActive(vm) && @@ -6334,11 +6315,6 @@ qemuDomainMigrateConfirm3(virDomainPtr domain, vm = NULL; } - if (orig_err) { - virSetError(orig_err); - virFreeError(orig_err); - } - cleanup: if (vm) virDomainObjUnlock(vm); diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 280b8cb..a0365ac 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1955,6 +1955,12 @@ finish: */ cancelled = ret == 0 && ddomain == NULL ? 1 : 0; + /* If finish3 set an error, and we don't have an earlier + * one we need to preserve it in case confirm3 overwrites + */ + if (!orig_err) + orig_err = virSaveLastError(); + /* * If cancelled, then src VM will be restarted, else * it will be killed @@ -2247,6 +2253,7 @@ qemuMigrationFinish(struct qemud_driver *driver, "cookieout=%p, cookieoutlen=%p, flags=%lu, retcode=%d", driver, dconn, vm, NULLSTR(cookiein), cookieinlen, cookieout, cookieoutlen, flags, retcode); + virErrorPtr orig_err = NULL; priv = vm->privateData; if (priv->jobActive != QEMU_JOB_MIGRATION_IN) { @@ -2312,6 +2319,14 @@ qemuMigrationFinish(struct qemud_driver *driver, */ if (qemuProcessStartCPUs(driver, vm, dconn, VIR_DOMAIN_RUNNING_MIGRATED) < 0) { + if (virGetLastError() == NULL) + qemuReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("resume operation failed")); + /* Need to save the current error, in case shutting + * down the process overwrites it + */ + orig_err = virSaveLastError(); + /* * In v3 protocol, the source VM is still available to * restart during confirm() step, so we kill it off @@ -2332,9 +2347,6 @@ qemuMigrationFinish(struct qemud_driver *driver, vm = NULL; } } - if (virGetLastError() == NULL) - qemuReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("resume operation failed")); goto endjob; } } @@ -2382,6 +2394,11 @@ cleanup: if (event) qemuDomainEventQueue(driver, event); qemuMigrationCookieFree(mig); + if (orig_err) { + virSetError(orig_err); + virFreeError(orig_err); + orig_err = virGetLastError(); + } return dom; } -- 1.7.4.4

2011/5/24 Daniel P. Berrange <berrange@redhat.com>:
When doing migration, if an error occurs in Perform, it must not be overwritten during Finish/Confirm steps. If an error occurs in Finish, it must not be overwritten in Confirm.
Previous commit a9d12c2444e43a0d3e5135eb15b4b62a7c011427 added code to qemudDomainMigrateFinish2 to preserve the error. This is not the right place, because it is not applicable in non-p2p migration. The src/libvirt.c virDomainMigrateV2/3 methods need code to preserve errors for non-p2p migration, while the doPeer2PeerMigrate2 and doPeer2PeerMigrate3 methods contain code to preverse errors for p2p migration.
Remove the bogus error preservation from qemudDomainMigrateFinish2 and qemudDomainMigrateFinish3.
Fix virDomainMigrateV3 and doPeer2PeerMigrate3 so that they preserve any error hit during the Finish3 step, before invoking Confirm3.
Finally if qemuMigrationFinish fails to resume the CPUs, it must preserve the error before tearing down the VM, so that VM cleanup doesn't overwrite it.
* src/libvirt.c: Preserve error before invoking Confirm3 * src/qemu/qemu_driver.c: Remove bogus error preservation code in qemudDomainMigrateFinish2/qemudDomainMigrateFinish3 * src/qemu/qemu_migration.c: Preserve error before invoking Confirm3 and after resume fails in qemuMigrationFinish. --- src/libvirt.c | 6 ++++++ src/qemu/qemu_driver.c | 24 ------------------------ src/qemu/qemu_migration.c | 23 ++++++++++++++++++++--- 3 files changed, 26 insertions(+), 27 deletions(-)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 280b8cb..a0365ac 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c
@@ -2382,6 +2394,11 @@ cleanup: if (event) qemuDomainEventQueue(driver, event); qemuMigrationCookieFree(mig); + if (orig_err) { + virSetError(orig_err); + virFreeError(orig_err); + orig_err = virGetLastError(); + } return dom; }
The last virGetLastError isn't necessary and can be removed, can't it? ACK. Matthias

The current virDomainMigrateFinish3 method signature attempts to distinguish two types of errors, by allowing return with ret== 0, but ddomain == NULL, to indicate a failure to start the guest. This is flawed, because when ret == 0, there is no way for the virErrorPtr details to be sent back to the client. Change the signature of virDomainMigrateFinish3 so it simply returns a virDomainPtr, in the same way as virDomainMigrateFinish2 The disk locking code will protect against the only possible failure mode this doesn't account for (loosing conenctivity to libvirtd after Finish3 starts the CPUs, but before the client sees the reply for Finish3). * src/driver.h, src/libvirt.c, src/libvirt_internal.h: Change virDomainMigrateFinish3 to return a virDomainPtr instead of int * src/remote/remote_driver.c, src/remote/remote_protocol.x, daemon/remote.c, src/qemu/qemu_driver.c, src/qemu/qemu_migration.c: Update for API change --- daemon/remote.c | 35 ++++++++--------------------------- src/driver.h | 5 ++--- src/libvirt.c | 41 ++++++++++++++++++++--------------------- src/libvirt_internal.h | 21 ++++++++++----------- src/qemu/qemu_driver.c | 21 +++++++++------------ src/qemu/qemu_migration.c | 19 ++++++++++--------- src/remote/remote_driver.c | 29 ++++------------------------- src/remote/remote_protocol.x | 2 +- 8 files changed, 64 insertions(+), 109 deletions(-) diff --git a/daemon/remote.c b/daemon/remote.c index f85d760..35129aa 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -76,7 +76,6 @@ static virStorageVolPtr get_nonnull_storage_vol(virConnectPtr conn, remote_nonnu static virSecretPtr get_nonnull_secret(virConnectPtr conn, remote_nonnull_secret secret); static virNWFilterPtr get_nonnull_nwfilter(virConnectPtr conn, remote_nonnull_nwfilter nwfilter); static virDomainSnapshotPtr get_nonnull_domain_snapshot(virDomainPtr dom, remote_nonnull_domain_snapshot snapshot); -static int make_domain(remote_domain *dom_dst, virDomainPtr dom_src); static void make_nonnull_domain(remote_nonnull_domain *dom_dst, virDomainPtr dom_src); static void make_nonnull_network(remote_nonnull_network *net_dst, virNetworkPtr net_src); static void make_nonnull_interface(remote_nonnull_interface *interface_dst, virInterfacePtr interface_src); @@ -3359,19 +3358,16 @@ remoteDispatchDomainMigrateFinish3(struct qemud_server *server ATTRIBUTE_UNUSED, uri = args->uri == NULL ? NULL : *args->uri; dconnuri = args->dconnuri == NULL ? NULL : *args->dconnuri; - if (virDomainMigrateFinish3(conn, args->dname, - args->cookie_in.cookie_in_val, - args->cookie_in.cookie_in_len, - &cookieout, &cookieoutlen, - dconnuri, uri, - args->flags, - args->cancelled, - &dom) < 0) + if (!(dom = virDomainMigrateFinish3(conn, args->dname, + args->cookie_in.cookie_in_val, + args->cookie_in.cookie_in_len, + &cookieout, &cookieoutlen, + dconnuri, uri, + args->flags, + args->cancelled))) goto cleanup; - if (dom && - make_domain(&ret->ddom, dom) < 0) - goto cleanup; + make_nonnull_domain(&ret->dom, dom); /* remoteDispatchClientRequest will free cookie */ @@ -3493,21 +3489,6 @@ get_nonnull_domain_snapshot(virDomainPtr dom, remote_nonnull_domain_snapshot sna } /* Make remote_nonnull_domain and remote_nonnull_network. */ -static int -make_domain(remote_domain *dom_dst, virDomainPtr dom_src) -{ - remote_domain rdom; - if (VIR_ALLOC(rdom) < 0) - return -1; - - rdom->id = dom_src->id; - rdom->name = strdup(dom_src->name); - memcpy(rdom->uuid, dom_src->uuid, VIR_UUID_BUFLEN); - - *dom_dst = rdom; - return 0; -} - static void make_nonnull_domain(remote_nonnull_domain *dom_dst, virDomainPtr dom_src) { diff --git a/src/driver.h b/src/driver.h index a5d8fe5..875ffcb 100644 --- a/src/driver.h +++ b/src/driver.h @@ -587,7 +587,7 @@ typedef int const char *dname, unsigned long resource); -typedef int +typedef virDomainPtr (*virDrvDomainMigrateFinish3) (virConnectPtr dconn, const char *dname, @@ -598,8 +598,7 @@ typedef int const char *dconnuri, const char *uri, unsigned long flags, - int cancelled, - virDomainPtr *newdom); + int cancelled); typedef int (*virDrvDomainMigrateConfirm3) diff --git a/src/libvirt.c b/src/libvirt.c index e714468..419ca94 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -3829,18 +3829,19 @@ finish: cookieout = NULL; cookieoutlen = 0; dname = dname ? dname : domain->name; - ret = dconn->driver->domainMigrateFinish3 + ddomain = dconn->driver->domainMigrateFinish3 (dconn, dname, cookiein, cookieinlen, &cookieout, &cookieoutlen, - NULL, uri, flags, cancelled, &ddomain); - - /* If ret is 0 then 'ddomain' indicates whether the VM is - * running on the dest. If not running, we can restart - * the source. If ret is -1, we can't be sure what happened - * to the VM on the dest, thus the only safe option is to - * kill the VM on the source, even though that may leave - * no VM at all on either host. + NULL, uri, flags, cancelled); + + /* If ddomain is NULL, then we were unable to start + * the guest on the target, and must restart on the + * source. There is a small chance that the ddomain + * is NULL due to an RPC failure, in which case + * ddomain could in fact be running on the dest. + * The lock manager plugins should take care of + * safety in this scenario. */ - cancelled = ret == 0 && ddomain == NULL ? 1 : 0; + cancelled = ddomain == NULL ? 1 : 0; /* If finish3 set an error, and we don't have an earlier * one we need to preserve it in case confirm3 overwrites @@ -5158,7 +5159,7 @@ error: * Not for public use. This function is part of the internal * implementation of migration in the remote case. */ -int +virDomainPtr virDomainMigrateFinish3(virConnectPtr dconn, const char *dname, const char *cookiein, @@ -5168,20 +5169,19 @@ virDomainMigrateFinish3(virConnectPtr dconn, const char *dconnuri, const char *uri, unsigned long flags, - int cancelled, - virDomainPtr *newdom) + int cancelled) { VIR_DEBUG("dconn=%p, dname=%s, cookiein=%p, cookieinlen=%d, cookieout=%p," - "cookieoutlen=%p, dconnuri=%s, uri=%s, flags=%lu, retcode=%d newdom=%p", + "cookieoutlen=%p, dconnuri=%s, uri=%s, flags=%lu, retcode=%d", dconn, NULLSTR(dname), cookiein, cookieinlen, cookieout, - cookieoutlen, NULLSTR(dconnuri), NULLSTR(uri), flags, cancelled, newdom); + cookieoutlen, NULLSTR(dconnuri), NULLSTR(uri), flags, cancelled); virResetLastError(); if (!VIR_IS_CONNECT (dconn)) { virLibConnError(VIR_ERR_INVALID_CONN, __FUNCTION__); virDispatchError(NULL); - return -1; + return NULL; } if (dconn->flags & VIR_CONNECT_RO) { @@ -5190,14 +5190,13 @@ virDomainMigrateFinish3(virConnectPtr dconn, } if (dconn->driver->domainMigrateFinish3) { - int ret; + virDomainPtr ret; ret = dconn->driver->domainMigrateFinish3(dconn, dname, cookiein, cookieinlen, cookieout, cookieoutlen, dconnuri, uri, flags, - cancelled, - newdom); - if (ret < 0) + cancelled); + if (!ret) goto error; return ret; } @@ -5206,7 +5205,7 @@ virDomainMigrateFinish3(virConnectPtr dconn, error: virDispatchError(dconn); - return -1; + return NULL; } diff --git a/src/libvirt_internal.h b/src/libvirt_internal.h index 3144271..39ef822 100644 --- a/src/libvirt_internal.h +++ b/src/libvirt_internal.h @@ -167,17 +167,16 @@ int virDomainMigratePerform3(virDomainPtr dom, const char *dname, unsigned long resource); -int virDomainMigrateFinish3(virConnectPtr dconn, - const char *dname, - const char *cookiein, - int cookieinlen, - char **cookieout, - int *cookieoutlen, - const char *dconnuri, /* libvirtd URI if Peer2Peer, NULL otherwise */ - const char *uri, /* VM Migration URI */ - unsigned long flags, - int cancelled, /* Kill the dst VM */ - virDomainPtr *newdom); +virDomainPtr virDomainMigrateFinish3(virConnectPtr dconn, + const char *dname, + const char *cookiein, + int cookieinlen, + char **cookieout, + int *cookieoutlen, + const char *dconnuri, /* libvirtd URI if Peer2Peer, NULL otherwise */ + const char *uri, /* VM Migration URI */ + unsigned long flags, + int cancelled); /* Kill the dst VM */ int virDomainMigrateConfirm3(virDomainPtr domain, const char *cookiein, diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 877f86f..4e09c61 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6222,7 +6222,7 @@ cleanup: } -static int +static virDomainPtr qemuDomainMigrateFinish3(virConnectPtr dconn, const char *dname, const char *cookiein, @@ -6232,12 +6232,11 @@ qemuDomainMigrateFinish3(virConnectPtr dconn, const char *dconnuri ATTRIBUTE_UNUSED, const char *uri ATTRIBUTE_UNUSED, unsigned long flags, - int cancelled, - virDomainPtr *newdom) + int cancelled) { struct qemud_driver *driver = dconn->privateData; virDomainObjPtr vm; - int ret = -1; + virDomainPtr dom = NULL; virCheckFlags(VIR_MIGRATE_LIVE | VIR_MIGRATE_PEER2PEER | @@ -6246,7 +6245,7 @@ qemuDomainMigrateFinish3(virConnectPtr dconn, VIR_MIGRATE_UNDEFINE_SOURCE | VIR_MIGRATE_PAUSED | VIR_MIGRATE_NON_SHARED_DISK | - VIR_MIGRATE_NON_SHARED_INC, -1); + VIR_MIGRATE_NON_SHARED_INC, NULL); qemuDriverLock(driver); vm = virDomainFindByName(&driver->domains, dname); @@ -6256,16 +6255,14 @@ qemuDomainMigrateFinish3(virConnectPtr dconn, goto cleanup; } - *newdom = qemuMigrationFinish(driver, dconn, vm, - cookiein, cookieinlen, - cookieout, cookieoutlen, - flags, cancelled, true); - - ret = 0; + dom = qemuMigrationFinish(driver, dconn, vm, + cookiein, cookieinlen, + cookieout, cookieoutlen, + flags, cancelled, true); cleanup: qemuDriverUnlock(driver); - return ret; + return dom; } static int diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index a0365ac..8c447b4 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1941,19 +1941,20 @@ finish: cookieoutlen = 0; dname = dname ? dname : vm->def->name; qemuDomainObjEnterRemoteWithDriver(driver, vm); - ret = dconn->driver->domainMigrateFinish3 + ddomain = dconn->driver->domainMigrateFinish3 (dconn, dname, cookiein, cookieinlen, &cookieout, &cookieoutlen, - dconnuri, uri_out ? uri_out : uri, flags, cancelled, &ddomain); + dconnuri, uri_out ? uri_out : uri, flags, cancelled); qemuDomainObjExitRemoteWithDriver(driver, vm); - /* If ret is 0 then 'ddomain' indicates whether the VM is - * running on the dest. If not running, we can restart - * the source. If ret is -1, we can't be sure what happened - * to the VM on the dest, thus the only safe option is to - * kill the VM on the source, even though that may leave - * no VM at all on either host. + /* If ddomain is NULL, then we were unable to start + * the guest on the target, and must restart on the + * source. There is a small chance that the ddomain + * is NULL due to an RPC failure, in which case + * ddomain could in fact be running on the dest. + * The lock manager plugins should take care of + * safety in this scenario. */ - cancelled = ret == 0 && ddomain == NULL ? 1 : 0; + cancelled = ddomain == NULL ? 1 : 0; /* If finish3 set an error, and we don't have an earlier * one we need to preserve it in case confirm3 overwrites diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 64f5620..14f9908 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -238,7 +238,6 @@ static int remoteAuthPolkit (virConnectPtr conn, struct private_data *priv, int virReportErrorHelper(VIR_FROM_REMOTE, code, __FILE__, \ __FUNCTION__, __LINE__, __VA_ARGS__) -static virDomainPtr get_domain (virConnectPtr conn, remote_domain domain); static virDomainPtr get_nonnull_domain (virConnectPtr conn, remote_nonnull_domain domain); static virNetworkPtr get_nonnull_network (virConnectPtr conn, remote_nonnull_network network); static virNWFilterPtr get_nonnull_nwfilter (virConnectPtr conn, remote_nonnull_nwfilter nwfilter); @@ -5228,7 +5227,7 @@ error: } -static int +static virDomainPtr remoteDomainMigrateFinish3(virConnectPtr dconn, const char *dname, const char *cookiein, @@ -5238,17 +5237,15 @@ remoteDomainMigrateFinish3(virConnectPtr dconn, const char *dconnuri, const char *uri, unsigned long flags, - int cancelled, - virDomainPtr *ddom) + int cancelled) { remote_domain_migrate_finish3_args args; remote_domain_migrate_finish3_ret ret; struct private_data *priv = dconn->privateData; - int rv = -1; + virDomainPtr rv = NULL; remoteDriverLock(priv); - *ddom = NULL; memset(&args, 0, sizeof(args)); memset(&ret, 0, sizeof(ret)); @@ -5265,7 +5262,7 @@ remoteDomainMigrateFinish3(virConnectPtr dconn, (xdrproc_t) xdr_remote_domain_migrate_finish3_ret, (char *) &ret) == -1) goto done; - *ddom = get_domain(dconn, ret.ddom); + rv = get_nonnull_domain(dconn, ret.dom); if (ret.cookie_out.cookie_out_len > 0) { if (!cookieout || !cookieoutlen) { @@ -5281,8 +5278,6 @@ remoteDomainMigrateFinish3(virConnectPtr dconn, xdr_free ((xdrproc_t) &xdr_remote_domain_migrate_finish3_ret, (char *) &ret); - rv = 0; - done: remoteDriverUnlock(priv); return rv; @@ -6614,22 +6609,6 @@ remoteDomainEventQueue(struct private_data *priv, virDomainEventPtr event) * but if they do then virterror_internal.has been set. */ static virDomainPtr -get_domain (virConnectPtr conn, remote_domain domain) -{ - virDomainPtr dom = NULL; - if (domain) { - dom = virGetDomain (conn, domain->name, BAD_CAST domain->uuid); - if (dom) dom->id = domain->id; - } - return dom; -} - -/* get_nonnull_domain and get_nonnull_network turn an on-wire - * (name, uuid) pair into virDomainPtr or virNetworkPtr object. - * These can return NULL if underlying memory allocations fail, - * but if they do then virterror_internal.has been set. - */ -static virDomainPtr get_nonnull_domain (virConnectPtr conn, remote_nonnull_domain domain) { virDomainPtr dom; diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index 58afee0..d8c0e53 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -2035,7 +2035,7 @@ struct remote_domain_migrate_finish3_args { }; struct remote_domain_migrate_finish3_ret { - remote_domain ddom; + remote_nonnull_domain dom; opaque cookie_out<REMOTE_MIGRATE_COOKIE_MAX>; }; -- 1.7.4.4

2011/5/24 Daniel P. Berrange <berrange@redhat.com>:
The current virDomainMigrateFinish3 method signature attempts to distinguish two types of errors, by allowing return with ret== 0, but ddomain == NULL, to indicate a failure to start the guest. This is flawed, because when ret == 0, there is no way for the virErrorPtr details to be sent back to the client.
Change the signature of virDomainMigrateFinish3 so it simply returns a virDomainPtr, in the same way as virDomainMigrateFinish2 The disk locking code will protect against the only possible failure mode this doesn't account for (loosing conenctivity to libvirtd after Finish3 starts the CPUs, but before the client sees the reply for Finish3).
* src/driver.h, src/libvirt.c, src/libvirt_internal.h: Change virDomainMigrateFinish3 to return a virDomainPtr instead of int * src/remote/remote_driver.c, src/remote/remote_protocol.x, daemon/remote.c, src/qemu/qemu_driver.c, src/qemu/qemu_migration.c: Update for API change --- daemon/remote.c | 35 ++++++++--------------------------- src/driver.h | 5 ++--- src/libvirt.c | 41 ++++++++++++++++++++--------------------- src/libvirt_internal.h | 21 ++++++++++----------- src/qemu/qemu_driver.c | 21 +++++++++------------ src/qemu/qemu_migration.c | 19 ++++++++++--------- src/remote/remote_driver.c | 29 ++++------------------------- src/remote/remote_protocol.x | 2 +- 8 files changed, 64 insertions(+), 109 deletions(-)
ACK. Matthias

On Tue, May 24, 2011 at 10:47:40AM -0400, Daniel P. Berrange wrote:
This is a series containing all my pending migration related enhancements and bugfixes.
It includes these previous patches to change the migration v3 protocol API + ABI
http://www.redhat.com/archives/libvir-list/2011-May/msg01243.html
Patch 9 also changes the v3 protocol API/ABI to deal with an error reporting problem I discovered in testing.
I've pushed this series, with all the review comments addressed Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
participants (3)
-
Daniel P. Berrange
-
Eric Blake
-
Matthias Bolte