[libvirt] [PATCH] migration: support all toURI and proto combos

Current implementation of 'toURI' migration interfaces does not support all combinations of interface versions and protocol versions. For example 'toURI2' with p2p flag will not migrate if driver supports only v3params proto. This is not convinient as drivers that starts to support migration have to manually support older versions of protocol. I guess this should be done in one place, namely here. Another issue is that there are a lot of code duplication in implementation of toURI interfaces and it is not obvious from code how are they related. This implementation uses extensible parameters as intermediate parameters representation. This is possible as interfaces are done backward compatible in terms of parameters and later versions supports all parameters of former versions. Another change that is convinient to add here is reusing the code for p2p and direct migration. The only major difference between them is how URIs are passed in places that supports both connection and migration uri. Now this difference is plain in code. See comments in code for details. Minor p2p vs direct difference is that direct not support proto with extensible parameters. This behaviour is saved. Unobvious check of migration dconnuri in case of p2p migration is actually protection from migration to localhost (from cover letter to patch 59d13aae that introduced it). So reformat this check to make it goal obvious. ==Behaviour changes Previous implementation of direct migration demands that proto v1 is implemented even if proto v3 is used. As this is somewhat against the purpuse of this patch this behaviour is dropped. Another behavior change that introduced is that now offline flag examined against driver migration capabilities for all api version not only for toURI1. It was odd taking into account that all toURI uses common peer2peer and direct implementation. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/libvirt-domain.c | 564 +++++++++++++++++++++----------------------------- 1 files changed, 232 insertions(+), 332 deletions(-) diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index cbf08fc..721d820 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -3291,181 +3291,209 @@ virDomainMigrateVersion3Params(virDomainPtr domain, params, nparams, true, flags); } - -/* - * In normal migration, the libvirt client co-ordinates communication - * between the 2 libvirtd instances on source & dest hosts. - * - * In this peer-2-peer migration alternative, the libvirt client - * only talks to the source libvirtd instance. The source libvirtd - * then opens its own connection to the destination and co-ordinates - * migration itself. - * - * If useParams is true, params and nparams contain migration parameters and - * we know it's safe to call the API which supports extensible parameters. - * Otherwise, we have to use xmlin, dname, uri, and bandwidth and pass them - * to the old-style APIs. - */ static int -virDomainMigratePeer2PeerFull(virDomainPtr domain, - const char *dconnuri, - const char *xmlin, - const char *dname, - const char *uri, - unsigned long long bandwidth, - virTypedParameterPtr params, - int nparams, - bool useParams, - unsigned int flags) +virDomainMigrateURIproto3(virDomainPtr domain, + const char *dconnuri, + virTypedParameterPtr params, + int nparams, + unsigned int flags) { - virURIPtr tempuri = NULL; + const char *compatParams[] = { VIR_MIGRATE_PARAM_URI, + VIR_MIGRATE_PARAM_DEST_NAME, + VIR_MIGRATE_PARAM_DEST_XML, + VIR_MIGRATE_PARAM_BANDWIDTH }; + const char *uri = NULL; + const char *dname = NULL; + const char *dxml = NULL; + unsigned long long bandwidth = 0; - VIR_DOMAIN_DEBUG(domain, - "dconnuri=%s, xmlin=%s, dname=%s, uri=%s, bandwidth=%llu " - "params=%p, nparams=%d, useParams=%d, flags=%x", - dconnuri, NULLSTR(xmlin), NULLSTR(dname), NULLSTR(uri), - bandwidth, params, nparams, useParams, flags); - VIR_TYPED_PARAMS_DEBUG(params, nparams); + if (!virTypedParamsCheck(params, nparams, compatParams, + ARRAY_CARDINALITY(compatParams))) { + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", + _("Migration does not support some of parameters.")); + return -1; + } - if ((useParams && !domain->conn->driver->domainMigratePerform3Params) || - (!useParams && - !domain->conn->driver->domainMigratePerform && - !domain->conn->driver->domainMigratePerform3)) { - virReportUnsupportedError(); + if (virTypedParamsGetString(params, nparams, + VIR_MIGRATE_PARAM_URI, &uri) < 0 || + virTypedParamsGetString(params, nparams, + VIR_MIGRATE_PARAM_DEST_NAME, &dname) < 0 || + virTypedParamsGetString(params, nparams, + VIR_MIGRATE_PARAM_DEST_XML, &dxml) < 0 || + virTypedParamsGetULLong(params, nparams, + VIR_MIGRATE_PARAM_BANDWIDTH, &bandwidth) < 0) { return -1; } - if (!(tempuri = virURIParse(dconnuri))) + VIR_DEBUG("Using migration protocol 3"); + return domain->conn->driver->domainMigratePerform3 + (domain, dxml, + NULL, /* cookiein */ + 0, /* cookieinlen */ + NULL, /* cookieoutlen */ + NULL, /* cookieoutlen */ + dconnuri, uri, flags, dname, bandwidth); +} + +static int +virDomainMigrateURIproto1(virDomainPtr domain, + const char *dconnuri, + virTypedParameterPtr params, + int nparams, + unsigned int flags) +{ + /* uri parameter is added for direct case */ + const char *compatParams[] = { VIR_MIGRATE_PARAM_DEST_NAME, + VIR_MIGRATE_PARAM_BANDWIDTH, + VIR_MIGRATE_PARAM_URI }; + const char *dname = NULL; + const char *miguri = NULL; + unsigned long long bandwidth = 0; + const char *duri = NULL; + + if (!virTypedParamsCheck(params, nparams, compatParams, + ARRAY_CARDINALITY(compatParams))) { + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", + _("Migration does not support some of parameters.")); return -1; - if (!tempuri->server || STRPREFIX(tempuri->server, "localhost")) { - virReportInvalidArg(dconnuri, "%s", - _("unable to parse server from dconnuri")); - virURIFree(tempuri); + } + + if (virTypedParamsGetString(params, nparams, + VIR_MIGRATE_PARAM_DEST_NAME, &dname) < 0 || + virTypedParamsGetULLong(params, nparams, + VIR_MIGRATE_PARAM_BANDWIDTH, &bandwidth) < 0 || + virTypedParamsGetString(params, nparams, + VIR_MIGRATE_PARAM_URI, &miguri) < 0) { return -1; } - virURIFree(tempuri); - if (useParams) { - VIR_DEBUG("Using migration protocol 3 with extensible parameters"); - return domain->conn->driver->domainMigratePerform3Params - (domain, dconnuri, params, nparams, - NULL, 0, NULL, NULL, flags); - } else if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, - VIR_DRV_FEATURE_MIGRATION_V3)) { - VIR_DEBUG("Using migration protocol 3"); - return domain->conn->driver->domainMigratePerform3 - (domain, xmlin, NULL, 0, NULL, NULL, dconnuri, - uri, flags, dname, bandwidth); - } else { - VIR_DEBUG("Using migration protocol 2"); - if (xmlin) { + /* see explanation in correspondent part of virDomainMigrateToURI */ + if ((flags & VIR_MIGRATE_PEER2PEER)) { + if (miguri) { virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", - _("Unable to change target guest XML during " - "migration")); + _("Migration does not support some of parameters.")); return -1; } - if (uri) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Unable to override peer2peer migration URI")); - return -1; - } - return domain->conn->driver->domainMigratePerform - (domain, NULL, 0, dconnuri, flags, dname, bandwidth); + duri = dconnuri; + } else { + duri = miguri; } -} + VIR_DEBUG("Using migration protocol 2"); + return domain->conn->driver->domainMigratePerform + (domain, + NULL, /* cookie */ + 0, /* cookielen */ + duri, flags, dname, bandwidth); +} static int -virDomainMigratePeer2Peer(virDomainPtr domain, - const char *xmlin, - unsigned long flags, - const char *dname, - const char *dconnuri, - const char *uri, - unsigned long bandwidth) +virDomainMigrateCheckNotLocal(const char *dconnuri) { - return virDomainMigratePeer2PeerFull(domain, dconnuri, xmlin, dname, uri, - bandwidth, NULL, 0, false, flags); -} + virURIPtr tempuri = NULL; + int ret = -1; + + if (!(tempuri = virURIParse(dconnuri))) + goto cleanup; + if (!tempuri->server || STRPREFIX(tempuri->server, "localhost")) { + virReportInvalidArg(dconnuri, "%s", + _("Attempt to migrate guest to the same host %s")); + goto cleanup; + } + ret = 0; + cleanup: -static int -virDomainMigratePeer2PeerParams(virDomainPtr domain, - const char *dconnuri, - virTypedParameterPtr params, - int nparams, - unsigned int flags) -{ - return virDomainMigratePeer2PeerFull(domain, dconnuri, NULL, NULL, NULL, 0, - params, nparams, true, flags); + virURIFree(tempuri); + return ret; } - /* * In normal migration, the libvirt client co-ordinates communication * between the 2 libvirtd instances on source & dest hosts. * - * Some hypervisors support an alternative, direct migration where - * there is no requirement for a libvirtd instance on the dest host. - * In this case - * - * eg, XenD can talk direct to XenD, so libvirtd on dest does not - * need to be involved at all, or even running + * In this peer-2-peer migration alternative, the libvirt client + * only talks to the source libvirtd instance. The source libvirtd + * then opens its own connection to the destination and co-ordinates + * migration itself. */ static int -virDomainMigrateDirect(virDomainPtr domain, - const char *xmlin, - unsigned long flags, - const char *dname, - const char *uri, - unsigned long bandwidth) +virDomainMigrateToURI_(virDomainPtr domain, + const char *dconnuri, + virTypedParameterPtr params, + int nparams, + unsigned int flags) { - VIR_DOMAIN_DEBUG(domain, - "xmlin=%s, flags=%lx, dname=%s, uri=%s, bandwidth=%lu", - NULLSTR(xmlin), flags, NULLSTR(dname), NULLSTR(uri), - bandwidth); + int ret = -1; - if (!domain->conn->driver->domainMigratePerform) { - virReportUnsupportedError(); - return -1; + /* First checkout the source */ + virCheckDomainReturn(domain, -1); + virCheckReadOnlyGoto(domain->conn->flags, cleanup); + + VIR_EXCLUSIVE_FLAGS_GOTO(VIR_MIGRATE_NON_SHARED_DISK, + VIR_MIGRATE_NON_SHARED_INC, + cleanup); + + if (flags & VIR_MIGRATE_OFFLINE && + !VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, + VIR_DRV_FEATURE_MIGRATION_OFFLINE)) { + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", + _("offline migration is not supported by " + "the source host")); + goto cleanup; } - /* Perform the migration. The driver isn't supposed to return - * until the migration is complete. - */ - 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, - bandwidth); + if (flags & VIR_MIGRATE_PEER2PEER) { + if (!VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, + VIR_DRV_FEATURE_MIGRATION_P2P)) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("Peer-to-peer migration is not supported by " + "the connection driver")); + goto cleanup; + } + if (virDomainMigrateCheckNotLocal(dconnuri)) + goto cleanup; + } else { - VIR_DEBUG("Using migration protocol 2"); - if (xmlin) { - virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", - _("Unable to change target guest XML during migration")); - return -1; + if (!VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, + VIR_DRV_FEATURE_MIGRATION_DIRECT)) { + /* Cannot do a migration with only the perform step */ + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("Direct migration is not supported by the" + " connection driver")); + goto cleanup; } - return domain->conn->driver->domainMigratePerform(domain, - NULL, /* cookie */ - 0, /* cookielen */ - uri, - flags, - dname, - bandwidth); + + VIR_DEBUG("Using direct migration"); } -} + /* for some reason direct migration is not supported thru migration + with extensible parameters */ + if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, + VIR_DRV_FEATURE_MIGRATION_PARAMS) && + domain->conn->driver->domainMigratePerform3Params && + (flags & VIR_MIGRATE_PEER2PEER)) { + VIR_DEBUG("Using migration protocol 3 with extensible parameters"); + ret = domain->conn->driver->domainMigratePerform3Params + (domain, dconnuri, params, nparams, + NULL, /* cookiein */ + 0, /* cookieinlen */ + NULL, /* cookieoutlen */ + NULL, /* cookieoutlen */ + flags); + } else if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, + VIR_DRV_FEATURE_MIGRATION_V3) && + domain->conn->driver->domainMigratePerform3) { + ret = virDomainMigrateURIproto3(domain, dconnuri, params, nparams, flags); + } else if (domain->conn->driver->domainMigratePerform) { + ret = virDomainMigrateURIproto1(domain, dconnuri, params, nparams, flags); + } else { + virReportUnsupportedError(); + } + + cleanup: + return ret; +} /** * virDomainMigrate: @@ -3594,29 +3622,22 @@ virDomainMigrate(virDomainPtr domain, } if (flags & VIR_MIGRATE_PEER2PEER) { - if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, - VIR_DRV_FEATURE_MIGRATION_P2P)) { - char *dstURI = NULL; - if (uri == NULL) { - dstURI = virConnectGetURI(dconn); - if (!dstURI) - return NULL; - } + char *dstURI = NULL; + if (uri == NULL) { + dstURI = virConnectGetURI(dconn); + if (!dstURI) + return NULL; + } - VIR_DEBUG("Using peer2peer migration"); - if (virDomainMigratePeer2Peer(domain, NULL, flags, dname, - uri ? uri : dstURI, NULL, bandwidth) < 0) { - VIR_FREE(dstURI); - goto error; - } + VIR_DEBUG("Using peer2peer migration"); + if (virDomainMigrateToURI(domain, uri ? uri : dstURI, flags, + dname, bandwidth) < 0) { VIR_FREE(dstURI); - - ddomain = virDomainLookupByName(dconn, dname ? dname : domain->name); - } else { - /* This driver does not support peer to peer migration */ - virReportUnsupportedError(); goto error; } + VIR_FREE(dstURI); + + ddomain = virDomainLookupByName(dconn, dname ? dname : domain->name); } else { /* Change protection requires support only on source side, and * is only needed in v3 migration, which automatically re-adds @@ -3825,8 +3846,8 @@ virDomainMigrate2(virDomainPtr domain, return NULL; VIR_DEBUG("Using peer2peer migration"); - if (virDomainMigratePeer2Peer(domain, dxml, flags, dname, - dstURI, uri, bandwidth) < 0) { + if (virDomainMigrateToURI2(domain, dstURI, uri, dxml, flags, dname, + bandwidth) < 0) { VIR_FREE(dstURI); goto error; } @@ -4099,7 +4120,6 @@ virDomainMigrate3(virDomainPtr domain, return NULL; } - /** * virDomainMigrateToURI: * @domain: a domain object @@ -4178,65 +4198,45 @@ virDomainMigrateToURI(virDomainPtr domain, const char *dname, unsigned long bandwidth) { + virTypedParameterPtr params = NULL; + int nparams = 0; + int maxparams = 0; + int ret = -1; + const char *dconnuri = NULL; + VIR_DOMAIN_DEBUG(domain, "duri=%p, flags=%lx, dname=%s, bandwidth=%lu", NULLSTR(duri), flags, NULLSTR(dname), bandwidth); virResetLastError(); - /* First checkout the source */ - virCheckDomainReturn(domain, -1); - virCheckReadOnlyGoto(domain->conn->flags, error); - - virCheckNonNullArgGoto(duri, error); - - VIR_EXCLUSIVE_FLAGS_GOTO(VIR_MIGRATE_NON_SHARED_DISK, - VIR_MIGRATE_NON_SHARED_INC, - error); - - if (flags & VIR_MIGRATE_OFFLINE && - !VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, - VIR_DRV_FEATURE_MIGRATION_OFFLINE)) { - virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", - _("offline migration is not supported by " - "the source host")); - goto error; - } + if (virTypedParamsAddString(¶ms, &nparams, &maxparams, + VIR_MIGRATE_PARAM_DEST_NAME, dname) < 0 || + virTypedParamsAddULLong(¶ms, &nparams, &maxparams, + VIR_MIGRATE_PARAM_BANDWIDTH, bandwidth) < 0) + goto cleanup; + /* Later versions of migration to uri interface use 2 uris, dconnuri and miguri. + And they have different meaning in p2p and direct case. In p2p case + dconnuri has meaining of duri and must be set while miguri is optional. + In direct case miguri has meaning of duri and dconnuri is not used. We follow + this conventions here. */ 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, NULL, flags, - dname, duri, NULL, bandwidth) < 0) - goto error; - } else { - /* No peer to peer migration supported */ - virReportUnsupportedError(); - goto error; - } + dconnuri = duri; } else { - if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, - VIR_DRV_FEATURE_MIGRATION_DIRECT)) { - VIR_DEBUG("Using direct migration"); - if (virDomainMigrateDirect(domain, NULL, flags, - dname, duri, bandwidth) < 0) - goto error; - } else { - /* Cannot do a migration with only the perform step */ - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("direct migration is not supported by the" - " connection driver")); - goto error; - } + if (virTypedParamsAddString(¶ms, &nparams, &maxparams, + VIR_MIGRATE_PARAM_URI, duri) < 0) + goto cleanup; } - return 0; + ret = virDomainMigrateToURI_(domain, dconnuri, params, nparams, flags); - error: - virDispatchError(domain->conn); - return -1; -} + cleanup: + virTypedParamsFree(params, nparams); + if (ret) + virDispatchError(domain->conn); + return ret; +} /** * virDomainMigrateToURI2: @@ -4333,6 +4333,11 @@ virDomainMigrateToURI2(virDomainPtr domain, const char *dname, unsigned long bandwidth) { + int ret = -1; + virTypedParameterPtr params = NULL; + int nparams = 0; + int maxparams = 0; + VIR_DOMAIN_DEBUG(domain, "dconnuri=%s, miguri=%s, dxml=%s, " "flags=%lx, dname=%s, bandwidth=%lu", NULLSTR(dconnuri), NULLSTR(miguri), NULLSTR(dxml), @@ -4340,47 +4345,25 @@ virDomainMigrateToURI2(virDomainPtr domain, virResetLastError(); - /* First checkout the source */ - virCheckDomainReturn(domain, -1); - virCheckReadOnlyGoto(domain->conn->flags, error); + if (virTypedParamsAddString(¶ms, &nparams, &maxparams, + VIR_MIGRATE_PARAM_URI, miguri) < 0 || + virTypedParamsAddString(¶ms, &nparams, &maxparams, + VIR_MIGRATE_PARAM_DEST_NAME, dname) < 0 || + virTypedParamsAddString(¶ms, &nparams, &maxparams, + VIR_MIGRATE_PARAM_DEST_XML, dxml) < 0 || + virTypedParamsAddULLong(¶ms, &nparams, &maxparams, + VIR_MIGRATE_PARAM_BANDWIDTH, bandwidth) < 0) + goto cleanup; - VIR_EXCLUSIVE_FLAGS_GOTO(VIR_MIGRATE_NON_SHARED_DISK, - VIR_MIGRATE_NON_SHARED_INC, - error); + ret = virDomainMigrateToURI_(domain, dconnuri, params, nparams, flags); - 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 */ - virReportUnsupportedError(); - 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 */ - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("direct migration is not supported by the" - " connection driver")); - goto error; - } - } + cleanup: + virTypedParamsFree(params, nparams); - return 0; + if (ret) + virDispatchError(domain->conn); - error: - virDispatchError(domain->conn); - return -1; + return ret; } @@ -4428,99 +4411,16 @@ virDomainMigrateToURI3(virDomainPtr domain, unsigned int nparams, unsigned int flags) { - bool compat; - const char *compatParams[] = { VIR_MIGRATE_PARAM_URI, - VIR_MIGRATE_PARAM_DEST_NAME, - VIR_MIGRATE_PARAM_DEST_XML, - VIR_MIGRATE_PARAM_BANDWIDTH }; - const char *uri = NULL; - const char *dname = NULL; - const char *dxml = NULL; - unsigned long long bandwidth = 0; - + int ret = -1; VIR_DOMAIN_DEBUG(domain, "dconnuri=%s, params=%p, nparms=%u flags=%x", NULLSTR(dconnuri), params, nparams, flags); VIR_TYPED_PARAMS_DEBUG(params, nparams); virResetLastError(); + if ((ret = virDomainMigrateToURI_(domain, dconnuri, params, nparams, flags)) < 0) + virDispatchError(domain->conn); - /* First checkout the source */ - virCheckDomainReturn(domain, -1); - virCheckReadOnlyGoto(domain->conn->flags, error); - - VIR_EXCLUSIVE_FLAGS_GOTO(VIR_MIGRATE_NON_SHARED_DISK, - VIR_MIGRATE_NON_SHARED_INC, - error); - - compat = virTypedParamsCheck(params, nparams, compatParams, - ARRAY_CARDINALITY(compatParams)); - - if (virTypedParamsGetString(params, nparams, - VIR_MIGRATE_PARAM_URI, &uri) < 0 || - virTypedParamsGetString(params, nparams, - VIR_MIGRATE_PARAM_DEST_NAME, &dname) < 0 || - virTypedParamsGetString(params, nparams, - VIR_MIGRATE_PARAM_DEST_XML, &dxml) < 0 || - virTypedParamsGetULLong(params, nparams, - VIR_MIGRATE_PARAM_BANDWIDTH, &bandwidth) < 0) { - goto error; - } - - if (flags & VIR_MIGRATE_PEER2PEER) { - if (!VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, - VIR_DRV_FEATURE_MIGRATION_P2P)) { - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", - _("Peer-to-peer migration is not supported by " - "the connection driver")); - goto error; - } - - if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, - VIR_DRV_FEATURE_MIGRATION_PARAMS)) { - VIR_DEBUG("Using peer2peer migration with extensible parameters"); - if (virDomainMigratePeer2PeerParams(domain, dconnuri, params, - nparams, flags) < 0) - goto error; - } else if (compat) { - VIR_DEBUG("Using peer2peer migration"); - if (virDomainMigratePeer2Peer(domain, dxml, flags, dname, - dconnuri, uri, bandwidth) < 0) - goto error; - } else { - virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", - _("Peer-to-peer migration with extensible " - "parameters is not supported but extended " - "parameters were passed")); - goto error; - } - } else { - if (!VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, - VIR_DRV_FEATURE_MIGRATION_DIRECT)) { - /* Cannot do a migration with only the perform step */ - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", - _("Direct migration is not supported by the" - " connection driver")); - goto error; - } - - if (!compat) { - virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", - _("Direct migration does not support extensible " - "parameters")); - goto error; - } - - VIR_DEBUG("Using direct migration"); - if (virDomainMigrateDirect(domain, dxml, flags, - dname, uri, bandwidth) < 0) - goto error; - } - - return 0; - - error: - virDispatchError(domain->conn); - return -1; + return ret; } -- 1.7.1

On Tue, Sep 08, 2015 at 13:43:09 +0300, Nikolay Shirokovskiy wrote:
Current implementation of 'toURI' migration interfaces does not support all combinations of interface versions and protocol versions. For example 'toURI2' with p2p flag will not migrate if driver supports only v3params proto.
In general, this patch is pretty large and hard to review, I suggest splitting it into a series of several shorter patches. They all need to compile and work, but it shouldn't be too hard in this case.
This is not convinient as drivers that starts to support migration have to manually support older versions of protocol. I guess this should be done in one place, namely here.
Well, the thing is all the code you're changing runs on a *client* and then the appropriate API calls are sent as RPC to a daemon. So just checking what APIs are implemented by the *client's* remote driver to choose what API will be called on the daemon is wrong. The client can be new enough to support all migration APIs while the daemon may be pretty old supporting only a few of them. Thus, for backward compatibility, the client has to either check what API is supported by the daemon (via VIR_DRV_SUPPORTS_FEATURE(..., VIR_DRV_FEATURE_MIGRATION_...)) or it has to be conservative and call the oldest API which supports the parameters passed by the application/user. Moreover, various versions of virDomainMigrateToURI are not really about the migration protocol to be used. They differ only in the set of parameters, the actual migration protocol will be decided by the daemon itself after making a connection to the destination daemon. That said, it should be fairly easy to implement all the entry points in a driver while still allowing only protocol version 3.
Another issue is that there are a lot of code duplication in implementation of toURI interfaces and it is not obvious from code how are they related.
This implementation uses extensible parameters as intermediate parameters representation. This is possible as interfaces are done backward compatible in terms of parameters and later versions supports all parameters of former versions.
Another change that is convinient to add here is reusing the code for p2p and direct migration. The only major difference between them is how URIs are passed in places that supports both connection and migration uri. Now this difference is plain in code. See comments in code for details.
Sounds like this could be separated.
Minor p2p vs direct difference is that direct not support proto with extensible parameters. This behaviour is saved.
Unobvious check of migration dconnuri in case of p2p migration is actually protection from migration to localhost (from cover letter to patch 59d13aae that introduced it). So reformat this check to make it goal obvious.
Another very good candidate for a separate patch.
==Behaviour changes
Previous implementation of direct migration demands that proto v1 is implemented even if proto v3 is used. As this is somewhat against the purpuse of this patch this behaviour is dropped.
Overall, if you see a way to refactor and improve the code so that it's more readable and maintainable, just go ahead, but be careful about this kind of changes in behavior. Jirka

On Tue, Sep 08, 2015 at 13:43:09 +0300, Nikolay Shirokovskiy wrote:
Current implementation of 'toURI' migration interfaces does not support all combinations of interface versions and protocol versions. For example 'toURI2' with p2p flag will not migrate if driver supports only v3params proto.
In general, this patch is pretty large and hard to review, I suggest splitting it into a series of several shorter patches. They all need to compile and work, but it shouldn't be too hard in this case.
This is not convinient as drivers that starts to support migration have to manually support older versions of protocol. I guess this should be done in one place, namely here.
Well, the thing is all the code you're changing runs on a *client* and then the appropriate API calls are sent as RPC to a daemon. So just checking what APIs are implemented by the *client's* remote driver to choose what API will be called on the daemon is wrong. The client can be new enough to support all migration APIs while the daemon may be pretty old supporting only a few of them. Thus, for backward compatibility, the client has to either check what API is supported by the daemon (via VIR_DRV_SUPPORTS_FEATURE(..., VIR_DRV_FEATURE_MIGRATION_...)) or it has to be conservative and call the oldest API which supports the parameters passed by the application/user. This approach(namely checking driver capabilities beforehand) is already
On 08.09.2015 15:14, Jiri Denemark wrote: present in code and i dont change it.
Overall, if you see a way to refactor and improve the code so that it's more readable and maintainable, just go ahead, but be careful about this kind of changes in behavior.
Ok. I'll split into a more managable patches.
Jirka

On Tue, Sep 08, 2015 at 02:14:32PM +0200, Jiri Denemark wrote:
On Tue, Sep 08, 2015 at 13:43:09 +0300, Nikolay Shirokovskiy wrote:
Current implementation of 'toURI' migration interfaces does not support all combinations of interface versions and protocol versions. For example 'toURI2' with p2p flag will not migrate if driver supports only v3params proto.
In general, this patch is pretty large and hard to review, I suggest splitting it into a series of several shorter patches. They all need to compile and work, but it shouldn't be too hard in this case.
This is not convinient as drivers that starts to support migration have to manually support older versions of protocol. I guess this should be done in one place, namely here.
Well, the thing is all the code you're changing runs on a *client* and then the appropriate API calls are sent as RPC to a daemon. So just checking what APIs are implemented by the *client's* remote driver to choose what API will be called on the daemon is wrong. The client can be new enough to support all migration APIs while the daemon may be pretty old supporting only a few of them. Thus, for backward compatibility, the client has to either check what API is supported by the daemon (via VIR_DRV_SUPPORTS_FEATURE(..., VIR_DRV_FEATURE_MIGRATION_...)) or it has to be conservative and call the oldest API which supports the parameters passed by the application/user.
Moreover, various versions of virDomainMigrateToURI are not really about the migration protocol to be used. They differ only in the set of parameters, the actual migration protocol will be decided by the daemon itself after making a connection to the destination daemon. That said, it should be fairly easy to implement all the entry points in a driver while still allowing only protocol version 3.
It seems to be that the only real critical thing is that all drivers support both V3 and V3Params driver callbacks. We could easily achieve that by creating a src/migration-helpers.c that provides the stub impls of V3 callbacks which simply pack args into a virTypedParams array and then invoke the corresponding V3Params callbacks. Those helpers can be just referenced from the virHypervisorDriver struct. So I don't think we need to change src/libvirt-host.c for this particular problem See the same recommendation I just made here wrt libxl driver which has the same need as vz https://www.redhat.com/archives/libvir-list/2015-September/msg00372.html Regards, 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 :|

On 09.09.2015 17:13, Daniel P. Berrange wrote:
On Tue, Sep 08, 2015 at 02:14:32PM +0200, Jiri Denemark wrote:
On Tue, Sep 08, 2015 at 13:43:09 +0300, Nikolay Shirokovskiy wrote:
Current implementation of 'toURI' migration interfaces does not support all combinations of interface versions and protocol versions. For example 'toURI2' with p2p flag will not migrate if driver supports only v3params proto.
In general, this patch is pretty large and hard to review, I suggest splitting it into a series of several shorter patches. They all need to compile and work, but it shouldn't be too hard in this case.
This is not convinient as drivers that starts to support migration have to manually support older versions of protocol. I guess this should be done in one place, namely here.
Well, the thing is all the code you're changing runs on a *client* and then the appropriate API calls are sent as RPC to a daemon. So just checking what APIs are implemented by the *client's* remote driver to choose what API will be called on the daemon is wrong. The client can be new enough to support all migration APIs while the daemon may be pretty old supporting only a few of them. Thus, for backward compatibility, the client has to either check what API is supported by the daemon (via VIR_DRV_SUPPORTS_FEATURE(..., VIR_DRV_FEATURE_MIGRATION_...)) or it has to be conservative and call the oldest API which supports the parameters passed by the application/user.
Moreover, various versions of virDomainMigrateToURI are not really about the migration protocol to be used. They differ only in the set of parameters, the actual migration protocol will be decided by the daemon itself after making a connection to the destination daemon. That said, it should be fairly easy to implement all the entry points in a driver while still allowing only protocol version 3.
It seems to be that the only real critical thing is that all drivers support both V3 and V3Params driver callbacks. We could easily achieve that by creating a src/migration-helpers.c that provides the stub impls of V3 callbacks which simply pack args into a virTypedParams array and then invoke the corresponding V3Params callbacks. Those helpers can be just referenced from the virHypervisorDriver struct.
So I don't think we need to change src/libvirt-host.c for this particular problem
See the same recommendation I just made here wrt libxl driver which has the same need as vz
https://www.redhat.com/archives/libvir-list/2015-September/msg00372.html
Regards, Daniel
Well I'm just about to finish the splitting of combo patch into more understanable set. Although helpers could be a solution this patchset has another value that it reduces a tangle in this aspect and code dups in libvirt-domain.c. Hope you'll check it.
participants (4)
-
Daniel P. Berrange
-
Jiri Denemark
-
Nikolay Shirokovskiy
-
Nikolay Shirokovskiy