[libvirt] [PATCH v4 0/12] 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. = Changes from version 3 * fix spellings * make error messages more specific as suggested * in 'migration: refactor: introduce params version of unmanaged': fix conversion for null values of explicit string params into virTypedParameterPtr. * squash 'migration: reuse parameters check in toURI2 and toURI3' into 'migration: introduce parameter checking function' * 'migration: refactor: one return in forURI family functions' is dropped as suggested src/libvirt-domain.c | 509 +++++++++++++++++++++++--------------------------- 1 files changed, 237 insertions(+), 272 deletions(-)
'useParams' parameter usage is an example of control coupling. Most of the work inside the function is done differently except for the uri check. Lets split this function into two, one with extensible parameters set and one with hardcoded parameter set. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/libvirt-domain.c | 103 +++++++++++++++++++++++-------------------------- 1 files changed, 48 insertions(+), 55 deletions(-) diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 1edd73e..697d58d 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -3301,37 +3301,26 @@ virDomainMigrateVersion3Params(virDomainPtr domain, * 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) +virDomainMigratePeer2PeerPlain(virDomainPtr domain, + const char *xmlin, + unsigned int flags, + const char *dname, + const char *dconnuri, + const char *uri, + unsigned long long bandwidth) { virURIPtr tempuri = NULL; VIR_DOMAIN_DEBUG(domain, "dconnuri=%s, xmlin=%s, dname=%s, uri=%s, bandwidth=%llu " - "params=%p, nparams=%d, useParams=%d, flags=%x", + "flags=%x", dconnuri, NULLSTR(xmlin), NULLSTR(dname), NULLSTR(uri), - bandwidth, params, nparams, useParams, flags); - VIR_TYPED_PARAMS_DEBUG(params, nparams); + bandwidth, flags); - if ((useParams && !domain->conn->driver->domainMigratePerform3Params) || - (!useParams && - !domain->conn->driver->domainMigratePerform && - !domain->conn->driver->domainMigratePerform3)) { + if (!domain->conn->driver->domainMigratePerform && + !domain->conn->driver->domainMigratePerform3) { virReportUnsupportedError(); return -1; } @@ -3346,13 +3335,8 @@ virDomainMigratePeer2PeerFull(virDomainPtr domain, } 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)) { + 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, @@ -3377,28 +3361,37 @@ virDomainMigratePeer2PeerFull(virDomainPtr domain, static int -virDomainMigratePeer2Peer(virDomainPtr domain, - const char *xmlin, - unsigned long flags, - const char *dname, - const char *dconnuri, - const char *uri, - unsigned long bandwidth) -{ - return virDomainMigratePeer2PeerFull(domain, dconnuri, xmlin, dname, uri, - bandwidth, NULL, 0, false, flags); -} - - -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); + virURIPtr tempuri = NULL; + + VIR_DOMAIN_DEBUG(domain, "dconnuri=%s, params=%p, nparams=%d, flags=%x", + dconnuri, params, nparams, flags); + VIR_TYPED_PARAMS_DEBUG(params, nparams); + + if (!domain->conn->driver->domainMigratePerform3Params) { + virReportUnsupportedError(); + return -1; + } + + if (!(tempuri = virURIParse(dconnuri))) + return -1; + if (!tempuri->server || STRPREFIX(tempuri->server, "localhost")) { + virReportInvalidArg(dconnuri, "%s", + _("unable to parse server from dconnuri")); + virURIFree(tempuri); + return -1; + } + virURIFree(tempuri); + + VIR_DEBUG("Using migration protocol 3 with extensible parameters"); + return domain->conn->driver->domainMigratePerform3Params + (domain, dconnuri, params, nparams, + NULL, 0, NULL, NULL, flags); } @@ -3605,8 +3598,8 @@ virDomainMigrate(virDomainPtr domain, } VIR_DEBUG("Using peer2peer migration"); - if (virDomainMigratePeer2Peer(domain, NULL, flags, dname, - uri ? uri : dstURI, NULL, bandwidth) < 0) { + if (virDomainMigratePeer2PeerPlain(domain, NULL, flags, dname, + uri ? uri : dstURI, NULL, bandwidth) < 0) { VIR_FREE(dstURI); goto error; } @@ -3826,8 +3819,8 @@ virDomainMigrate2(virDomainPtr domain, return NULL; VIR_DEBUG("Using peer2peer migration"); - if (virDomainMigratePeer2Peer(domain, dxml, flags, dname, - dstURI, uri, bandwidth) < 0) { + if (virDomainMigratePeer2PeerPlain(domain, dxml, flags, dname, + dstURI, uri, bandwidth) < 0) { VIR_FREE(dstURI); goto error; } @@ -4207,8 +4200,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, NULL, flags, - dname, duri, NULL, bandwidth) < 0) + if (virDomainMigratePeer2PeerPlain(domain, NULL, flags, + dname, duri, NULL, bandwidth) < 0) goto error; } else { /* No peer to peer migration supported */ @@ -4353,8 +4346,8 @@ virDomainMigrateToURI2(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, dxml, flags, - dname, dconnuri, miguri, bandwidth) < 0) + if (virDomainMigratePeer2PeerPlain(domain, dxml, flags, + dname, dconnuri, miguri, bandwidth) < 0) goto error; } else { /* No peer to peer migration supported */ @@ -4484,8 +4477,8 @@ virDomainMigrateToURI3(virDomainPtr domain, goto error; } else if (compat) { VIR_DEBUG("Using peer2peer migration"); - if (virDomainMigratePeer2Peer(domain, dxml, flags, dname, - dconnuri, uri, bandwidth) < 0) + if (virDomainMigratePeer2PeerPlain(domain, dxml, flags, dname, + dconnuri, uri, bandwidth) < 0) goto error; } else { virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", -- 1.7.1
Refactor dconnuri local server URI check to common API. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/libvirt-domain.c | 44 ++++++++++++++++++++++++-------------------- 1 files changed, 24 insertions(+), 20 deletions(-) diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 697d58d..2e43062 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -3293,6 +3293,28 @@ virDomainMigrateVersion3Params(virDomainPtr domain, } +static int ATTRIBUTE_NONNULL(1) +virDomainMigrateCheckNotLocal(const char *dconnuri) +{ + 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: + + virURIFree(tempuri); + return ret; +} + + /* * In normal migration, the libvirt client co-ordinates communication * between the 2 libvirtd instances on source & dest hosts. @@ -3311,8 +3333,6 @@ virDomainMigratePeer2PeerPlain(virDomainPtr domain, const char *uri, unsigned long long bandwidth) { - virURIPtr tempuri = NULL; - VIR_DOMAIN_DEBUG(domain, "dconnuri=%s, xmlin=%s, dname=%s, uri=%s, bandwidth=%llu " "flags=%x", @@ -3325,15 +3345,8 @@ virDomainMigratePeer2PeerPlain(virDomainPtr domain, return -1; } - if (!(tempuri = virURIParse(dconnuri))) + if (virDomainMigrateCheckNotLocal(dconnuri) < 0) return -1; - if (!tempuri->server || STRPREFIX(tempuri->server, "localhost")) { - virReportInvalidArg(dconnuri, "%s", - _("unable to parse server from dconnuri")); - virURIFree(tempuri); - return -1; - } - virURIFree(tempuri); if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, VIR_DRV_FEATURE_MIGRATION_V3)) { @@ -3367,8 +3380,6 @@ virDomainMigratePeer2PeerParams(virDomainPtr domain, int nparams, unsigned int flags) { - virURIPtr tempuri = NULL; - VIR_DOMAIN_DEBUG(domain, "dconnuri=%s, params=%p, nparams=%d, flags=%x", dconnuri, params, nparams, flags); VIR_TYPED_PARAMS_DEBUG(params, nparams); @@ -3378,15 +3389,8 @@ virDomainMigratePeer2PeerParams(virDomainPtr domain, return -1; } - if (!(tempuri = virURIParse(dconnuri))) - return -1; - if (!tempuri->server || STRPREFIX(tempuri->server, "localhost")) { - virReportInvalidArg(dconnuri, "%s", - _("unable to parse server from dconnuri")); - virURIFree(tempuri); + if (virDomainMigrateCheckNotLocal(dconnuri) < 0) return -1; - } - virURIFree(tempuri); VIR_DEBUG("Using migration protocol 3 with extensible parameters"); return domain->conn->driver->domainMigratePerform3Params -- 1.7.1
On Fri, Oct 02, 2015 at 10:52:42 +0300, Nikolay Shirokovskiy wrote:
Refactor dconnuri local server URI check to common API.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/libvirt-domain.c | 44 ++++++++++++++++++++++++-------------------- 1 files changed, 24 insertions(+), 20 deletions(-)
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 697d58d..2e43062 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -3293,6 +3293,28 @@ virDomainMigrateVersion3Params(virDomainPtr domain, }
+static int ATTRIBUTE_NONNULL(1) +virDomainMigrateCheckNotLocal(const char *dconnuri) +{ + 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: +
We usually put an empty line before the label, not after it.
+ virURIFree(tempuri); + return ret; +}
Jirka
This is more structured code so it will be easier to add branch for _PARAMS protocol here. It is not a pure refactoring strictly speaking as we remove scenarios for broken cases when driver defines V3 feature and implements perform function. So it is additionally a more solid code. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/libvirt-domain.c | 14 ++++++++------ 1 files changed, 8 insertions(+), 6 deletions(-) diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 2e43062..a7d3fbd 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -3339,23 +3339,25 @@ virDomainMigratePeer2PeerPlain(virDomainPtr domain, dconnuri, NULLSTR(xmlin), NULLSTR(dname), NULLSTR(uri), bandwidth, flags); - if (!domain->conn->driver->domainMigratePerform && - !domain->conn->driver->domainMigratePerform3) { - virReportUnsupportedError(); - return -1; - } - if (virDomainMigrateCheckNotLocal(dconnuri) < 0) return -1; if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, VIR_DRV_FEATURE_MIGRATION_V3)) { VIR_DEBUG("Using migration protocol 3"); + if (!domain->conn->driver->domainMigratePerform3) { + virReportUnsupportedError(); + return -1; + } return domain->conn->driver->domainMigratePerform3 (domain, xmlin, NULL, 0, NULL, NULL, dconnuri, uri, flags, dname, bandwidth); } else { VIR_DEBUG("Using migration protocol 2"); + if (!domain->conn->driver->domainMigratePerform) { + virReportUnsupportedError(); + return -1; + } if (xmlin) { virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", _("Unable to change target guest XML during " -- 1.7.1
From: Michal Privoznik <mprivozn@redhat.com> Direct migration should work if *perform3 is present but *perform is not. This is situation when driver migration is implemented after new version of driver function is introduced. We should not be forced to support old version too as its parameter space is subspace of newer one. --- This patch has been already sent to mailist and has form that finally suggested by Michal Privoznik. I just include it in the set as it is not merged yet. --- src/libvirt-domain.c | 13 ++++++++----- 1 files changed, 8 insertions(+), 5 deletions(-) diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index a7d3fbd..1c6e27b 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -3425,16 +3425,15 @@ virDomainMigrateDirect(virDomainPtr domain, NULLSTR(xmlin), flags, NULLSTR(dname), NULLSTR(uri), bandwidth); - if (!domain->conn->driver->domainMigratePerform) { - virReportUnsupportedError(); - return -1; - } - /* 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)) { + if (!domain->conn->driver->domainMigratePerform3) { + virReportUnsupportedError(); + return -1; + } VIR_DEBUG("Using migration protocol 3"); /* dconn URI not relevant in direct migration, since no * target libvirtd is involved */ @@ -3450,6 +3449,10 @@ virDomainMigrateDirect(virDomainPtr domain, dname, bandwidth); } else { + if (!domain->conn->driver->domainMigratePerform) { + virReportUnsupportedError(); + return -1; + } VIR_DEBUG("Using migration protocol 2"); if (xmlin) { virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", -- 1.7.1
We use miguri name for this parameter in other places. So make naming more consitent. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/libvirt-domain.c | 12 ++++++------ 1 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 1c6e27b..d164782 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -3330,13 +3330,13 @@ virDomainMigratePeer2PeerPlain(virDomainPtr domain, unsigned int flags, const char *dname, const char *dconnuri, - const char *uri, + const char *miguri, unsigned long long bandwidth) { VIR_DOMAIN_DEBUG(domain, - "dconnuri=%s, xmlin=%s, dname=%s, uri=%s, bandwidth=%llu " - "flags=%x", - dconnuri, NULLSTR(xmlin), NULLSTR(dname), NULLSTR(uri), + "dconnuri=%s, xmlin=%s, dname=%s, migrui=%s, " + "bandwidth=%llu, flags=%x", + dconnuri, NULLSTR(xmlin), NULLSTR(dname), NULLSTR(miguri), bandwidth, flags); if (virDomainMigrateCheckNotLocal(dconnuri) < 0) @@ -3351,7 +3351,7 @@ virDomainMigratePeer2PeerPlain(virDomainPtr domain, } return domain->conn->driver->domainMigratePerform3 (domain, xmlin, NULL, 0, NULL, NULL, dconnuri, - uri, flags, dname, bandwidth); + miguri, flags, dname, bandwidth); } else { VIR_DEBUG("Using migration protocol 2"); if (!domain->conn->driver->domainMigratePerform) { @@ -3364,7 +3364,7 @@ virDomainMigratePeer2PeerPlain(virDomainPtr domain, "migration")); return -1; } - if (uri) { + if (miguri) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Unable to override peer2peer migration URI")); return -1; -- 1.7.1
p2p plain and direct function are good candidates for code reuse. Their main function is same - to branch among different versions of migration protocol and implementation of this function is also same. Also they have other common functionality in lesser aspects. So let's merge them. But as they have different signatures we have to get to convention on how to pass direct migration 'uri' in 'dconnuri' and 'miguri'. Fortunately we alreay have such convention in parameters passed to toURI2 function, just let's follow it. 'uri' is passed in miguri and dconnuri is ignored. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/libvirt-domain.c | 148 ++++++++++++++++---------------------------------- 1 files changed, 46 insertions(+), 102 deletions(-) diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index d164782..36db29c 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -3319,27 +3319,35 @@ virDomainMigrateCheckNotLocal(const char *dconnuri) * 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. + * This function encapsulates 2 alternatives to the above case. + * + * 1. peer-2-peer migration, 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. + * + * 2. direct migration, where there is no requirement for a libvirtd instance + * on the dest host. Eg, XenD can talk direct to XenD, so libvirtd on dest + * does not need to be involved at all, or even running. */ static int -virDomainMigratePeer2PeerPlain(virDomainPtr domain, - const char *xmlin, - unsigned int flags, - const char *dname, - const char *dconnuri, - const char *miguri, - unsigned long long bandwidth) +virDomainMigrateUnmanaged(virDomainPtr domain, + const char *xmlin, + unsigned int flags, + const char *dname, + const char *dconnuri, + const char *miguri, + unsigned long long bandwidth) { + const char *uri = NULL; + VIR_DOMAIN_DEBUG(domain, "dconnuri=%s, xmlin=%s, dname=%s, migrui=%s, " "bandwidth=%llu, flags=%x", dconnuri, NULLSTR(xmlin), NULLSTR(dname), NULLSTR(miguri), bandwidth, flags); - if (virDomainMigrateCheckNotLocal(dconnuri) < 0) + if ((flags & VIR_MIGRATE_PEER2PEER) && + virDomainMigrateCheckNotLocal(dconnuri) < 0) return -1; if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, @@ -3364,13 +3372,18 @@ virDomainMigratePeer2PeerPlain(virDomainPtr domain, "migration")); return -1; } - if (miguri) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Unable to override peer2peer migration URI")); - return -1; + if (flags & VIR_MIGRATE_PEER2PEER) { + if (miguri) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to override peer2peer migration URI")); + return -1; + } + uri = dconnuri; + } else { + uri = miguri; } return domain->conn->driver->domainMigratePerform - (domain, NULL, 0, dconnuri, flags, dname, bandwidth); + (domain, NULL, 0, uri, flags, dname, bandwidth); } } @@ -3401,75 +3414,6 @@ virDomainMigratePeer2PeerParams(virDomainPtr domain, } -/* - * 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 - */ -static int -virDomainMigrateDirect(virDomainPtr domain, - const char *xmlin, - unsigned long flags, - const char *dname, - const char *uri, - unsigned long bandwidth) -{ - VIR_DOMAIN_DEBUG(domain, - "xmlin=%s, flags=%lx, dname=%s, uri=%s, bandwidth=%lu", - NULLSTR(xmlin), flags, NULLSTR(dname), NULLSTR(uri), - bandwidth); - - /* 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)) { - if (!domain->conn->driver->domainMigratePerform3) { - virReportUnsupportedError(); - return -1; - } - 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); - } else { - if (!domain->conn->driver->domainMigratePerform) { - virReportUnsupportedError(); - return -1; - } - VIR_DEBUG("Using migration protocol 2"); - if (xmlin) { - virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", - _("Unable to change target guest XML during migration")); - return -1; - } - return domain->conn->driver->domainMigratePerform(domain, - NULL, /* cookie */ - 0, /* cookielen */ - uri, - flags, - dname, - bandwidth); - } -} - - /** * virDomainMigrate: * @domain: a domain object @@ -3607,8 +3551,8 @@ virDomainMigrate(virDomainPtr domain, } VIR_DEBUG("Using peer2peer migration"); - if (virDomainMigratePeer2PeerPlain(domain, NULL, flags, dname, - uri ? uri : dstURI, NULL, bandwidth) < 0) { + if (virDomainMigrateUnmanaged(domain, NULL, flags, dname, + uri ? uri : dstURI, NULL, bandwidth) < 0) { VIR_FREE(dstURI); goto error; } @@ -3828,8 +3772,8 @@ virDomainMigrate2(virDomainPtr domain, return NULL; VIR_DEBUG("Using peer2peer migration"); - if (virDomainMigratePeer2PeerPlain(domain, dxml, flags, dname, - dstURI, uri, bandwidth) < 0) { + if (virDomainMigrateUnmanaged(domain, dxml, flags, dname, + dstURI, uri, bandwidth) < 0) { VIR_FREE(dstURI); goto error; } @@ -4209,8 +4153,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 (virDomainMigratePeer2PeerPlain(domain, NULL, flags, - dname, duri, NULL, bandwidth) < 0) + if (virDomainMigrateUnmanaged(domain, NULL, flags, + dname, duri, NULL, bandwidth) < 0) goto error; } else { /* No peer to peer migration supported */ @@ -4221,8 +4165,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, NULL, flags, - dname, duri, bandwidth) < 0) + if (virDomainMigrateUnmanaged(domain, NULL, flags, + dname, NULL, duri, bandwidth) < 0) goto error; } else { /* Cannot do a migration with only the perform step */ @@ -4355,8 +4299,8 @@ virDomainMigrateToURI2(virDomainPtr domain, if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, VIR_DRV_FEATURE_MIGRATION_P2P)) { VIR_DEBUG("Using peer2peer migration"); - if (virDomainMigratePeer2PeerPlain(domain, dxml, flags, - dname, dconnuri, miguri, bandwidth) < 0) + if (virDomainMigrateUnmanaged(domain, dxml, flags, + dname, dconnuri, miguri, bandwidth) < 0) goto error; } else { /* No peer to peer migration supported */ @@ -4367,8 +4311,8 @@ virDomainMigrateToURI2(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, dxml, flags, - dname, miguri, bandwidth) < 0) + if (virDomainMigrateUnmanaged(domain, dxml, flags, + dname, NULL, miguri, bandwidth) < 0) goto error; } else { /* Cannot do a migration with only the perform step */ @@ -4486,8 +4430,8 @@ virDomainMigrateToURI3(virDomainPtr domain, goto error; } else if (compat) { VIR_DEBUG("Using peer2peer migration"); - if (virDomainMigratePeer2PeerPlain(domain, dxml, flags, dname, - dconnuri, uri, bandwidth) < 0) + if (virDomainMigrateUnmanaged(domain, dxml, flags, dname, + dconnuri, uri, bandwidth) < 0) goto error; } else { virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", @@ -4514,8 +4458,8 @@ virDomainMigrateToURI3(virDomainPtr domain, } VIR_DEBUG("Using direct migration"); - if (virDomainMigrateDirect(domain, dxml, flags, - dname, uri, bandwidth) < 0) + if (virDomainMigrateUnmanaged(domain, dxml, flags, + dname, NULL, uri, bandwidth) < 0) goto error; } -- 1.7.1
Let's put main functionality into params version of virDomainMigrateUnmanaged as a preparation step for merging it with virDomainMigratePeer2PeerParams. virDomainMigrateUnmanaged then does nothing more then just adapting arguments. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/libvirt-domain.c | 98 ++++++++++++++++++++++++++++++++++---------------- 1 files changed, 67 insertions(+), 31 deletions(-) diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 36db29c..3f2bb4a 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -3330,21 +3330,32 @@ virDomainMigrateCheckNotLocal(const char *dconnuri) * does not need to be involved at all, or even running. */ static int -virDomainMigrateUnmanaged(virDomainPtr domain, - const char *xmlin, - unsigned int flags, - const char *dname, - const char *dconnuri, - const char *miguri, - unsigned long long bandwidth) +virDomainMigrateUnmanagedParams(virDomainPtr domain, + const char *dconnuri, + virTypedParameterPtr params, + int nparams, + unsigned int flags) { const char *uri = NULL; + const char *miguri = NULL; + const char *dname = NULL; + const char *xmlin = NULL; + unsigned long long bandwidth = 0; - VIR_DOMAIN_DEBUG(domain, - "dconnuri=%s, xmlin=%s, dname=%s, migrui=%s, " - "bandwidth=%llu, flags=%x", - dconnuri, NULLSTR(xmlin), NULLSTR(dname), NULLSTR(miguri), - bandwidth, flags); + VIR_DOMAIN_DEBUG(domain, "dconnuri=%s, params=%p, nparams=%d, flags=%x", + dconnuri, params, nparams, flags); + VIR_TYPED_PARAMS_DEBUG(params, nparams); + + if (virTypedParamsGetString(params, nparams, + VIR_MIGRATE_PARAM_URI, &miguri) < 0 || + virTypedParamsGetString(params, nparams, + VIR_MIGRATE_PARAM_DEST_NAME, &dname) < 0 || + virTypedParamsGetString(params, nparams, + VIR_MIGRATE_PARAM_DEST_XML, &xmlin) < 0 || + virTypedParamsGetULLong(params, nparams, + VIR_MIGRATE_PARAM_BANDWIDTH, &bandwidth) < 0) { + return -1; + } if ((flags & VIR_MIGRATE_PEER2PEER) && virDomainMigrateCheckNotLocal(dconnuri) < 0) @@ -3414,6 +3425,46 @@ virDomainMigratePeer2PeerParams(virDomainPtr domain, } +static int +virDomainMigrateUnmanaged(virDomainPtr domain, + const char *xmlin, + unsigned int flags, + const char *dname, + const char *dconnuri, + const char *miguri, + unsigned long long bandwidth) +{ + int ret = -1; + virTypedParameterPtr params = NULL; + int nparams = 0; + int maxparams = 0; + + if (miguri && + virTypedParamsAddString(¶ms, &nparams, &maxparams, + VIR_MIGRATE_PARAM_URI, miguri) < 0) + goto cleanup; + if (dname && + virTypedParamsAddString(¶ms, &nparams, &maxparams, + VIR_MIGRATE_PARAM_DEST_NAME, dname) < 0) + goto cleanup; + if (xmlin && + virTypedParamsAddString(¶ms, &nparams, &maxparams, + VIR_MIGRATE_PARAM_DEST_XML, xmlin) < 0) + goto cleanup; + if (virTypedParamsAddULLong(¶ms, &nparams, &maxparams, + VIR_MIGRATE_PARAM_BANDWIDTH, bandwidth) < 0) + goto cleanup; + + ret = virDomainMigrateUnmanagedParams(domain, dconnuri, params, + nparams, flags); + + cleanup: + virTypedParamsFree(params, nparams); + + return ret; +} + + /** * virDomainMigrate: * @domain: a domain object @@ -4380,10 +4431,6 @@ virDomainMigrateToURI3(virDomainPtr domain, 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, params=%p, nparms=%u flags=%x", NULLSTR(dconnuri), params, nparams, flags); @@ -4402,17 +4449,6 @@ virDomainMigrateToURI3(virDomainPtr domain, 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)) { @@ -4430,8 +4466,8 @@ virDomainMigrateToURI3(virDomainPtr domain, goto error; } else if (compat) { VIR_DEBUG("Using peer2peer migration"); - if (virDomainMigrateUnmanaged(domain, dxml, flags, dname, - dconnuri, uri, bandwidth) < 0) + if (virDomainMigrateUnmanagedParams(domain, dconnuri, params, + nparams, flags) < 0) goto error; } else { virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", @@ -4458,8 +4494,8 @@ virDomainMigrateToURI3(virDomainPtr domain, } VIR_DEBUG("Using direct migration"); - if (virDomainMigrateUnmanaged(domain, dxml, flags, - dname, NULL, uri, bandwidth) < 0) + if (virDomainMigrateUnmanagedParams(domain, NULL, params, + nparams, flags) < 0) goto error; } -- 1.7.1
Extract parameter adaptation and checking which is protocol dependent into designated functions. Leave only branching and common checks in virDomainMigrateUnmanagedParams. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/libvirt-domain.c | 118 ++++++++++++++++++++++++++++++++++---------------- 1 files changed, 80 insertions(+), 38 deletions(-) diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 3f2bb4a..fab9564 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -3315,6 +3315,82 @@ virDomainMigrateCheckNotLocal(const char *dconnuri) } +static int +virDomainMigrateUnmanagedProto2(virDomainPtr domain, + const char *dconnuri, + virTypedParameterPtr params, + int nparams, + unsigned int flags) +{ + const char *uri = NULL; + const char *miguri = NULL; + const char *dname = NULL; + const char *xmlin = NULL; + unsigned long long bandwidth = 0; + + if (virTypedParamsGetString(params, nparams, + VIR_MIGRATE_PARAM_URI, &miguri) < 0 || + virTypedParamsGetString(params, nparams, + VIR_MIGRATE_PARAM_DEST_NAME, &dname) < 0 || + virTypedParamsGetString(params, nparams, + VIR_MIGRATE_PARAM_DEST_XML, &xmlin) < 0 || + virTypedParamsGetULLong(params, nparams, + VIR_MIGRATE_PARAM_BANDWIDTH, &bandwidth) < 0) { + return -1; + } + + if (xmlin) { + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", + _("Unable to change target guest XML during " + "migration")); + return -1; + } + + if (flags & VIR_MIGRATE_PEER2PEER) { + if (miguri) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to override peer2peer migration URI")); + return -1; + } + uri = dconnuri; + } else { + uri = miguri; + } + + return domain->conn->driver->domainMigratePerform + (domain, NULL, 0, uri, flags, dname, bandwidth); +} + + +static int +virDomainMigrateUnmanagedProto3(virDomainPtr domain, + const char *dconnuri, + virTypedParameterPtr params, + int nparams, + unsigned int flags) +{ + const char *miguri = NULL; + const char *dname = NULL; + const char *xmlin = NULL; + unsigned long long bandwidth = 0; + + if (virTypedParamsGetString(params, nparams, + VIR_MIGRATE_PARAM_URI, &miguri) < 0 || + virTypedParamsGetString(params, nparams, + VIR_MIGRATE_PARAM_DEST_NAME, &dname) < 0 || + virTypedParamsGetString(params, nparams, + VIR_MIGRATE_PARAM_DEST_XML, &xmlin) < 0 || + virTypedParamsGetULLong(params, nparams, + VIR_MIGRATE_PARAM_BANDWIDTH, &bandwidth) < 0) { + return -1; + } + + return domain->conn->driver->domainMigratePerform3 + (domain, xmlin, NULL, 0, NULL, NULL, dconnuri, + miguri, flags, dname, bandwidth); +} + + /* * In normal migration, the libvirt client co-ordinates communication * between the 2 libvirtd instances on source & dest hosts. @@ -3336,27 +3412,10 @@ virDomainMigrateUnmanagedParams(virDomainPtr domain, int nparams, unsigned int flags) { - const char *uri = NULL; - const char *miguri = NULL; - const char *dname = NULL; - const char *xmlin = NULL; - unsigned long long bandwidth = 0; - VIR_DOMAIN_DEBUG(domain, "dconnuri=%s, params=%p, nparams=%d, flags=%x", dconnuri, params, nparams, flags); VIR_TYPED_PARAMS_DEBUG(params, nparams); - if (virTypedParamsGetString(params, nparams, - VIR_MIGRATE_PARAM_URI, &miguri) < 0 || - virTypedParamsGetString(params, nparams, - VIR_MIGRATE_PARAM_DEST_NAME, &dname) < 0 || - virTypedParamsGetString(params, nparams, - VIR_MIGRATE_PARAM_DEST_XML, &xmlin) < 0 || - virTypedParamsGetULLong(params, nparams, - VIR_MIGRATE_PARAM_BANDWIDTH, &bandwidth) < 0) { - return -1; - } - if ((flags & VIR_MIGRATE_PEER2PEER) && virDomainMigrateCheckNotLocal(dconnuri) < 0) return -1; @@ -3368,33 +3427,16 @@ virDomainMigrateUnmanagedParams(virDomainPtr domain, virReportUnsupportedError(); return -1; } - return domain->conn->driver->domainMigratePerform3 - (domain, xmlin, NULL, 0, NULL, NULL, dconnuri, - miguri, flags, dname, bandwidth); + return virDomainMigrateUnmanagedProto3(domain, dconnuri, + params, nparams, flags); } else { VIR_DEBUG("Using migration protocol 2"); if (!domain->conn->driver->domainMigratePerform) { virReportUnsupportedError(); return -1; } - if (xmlin) { - virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", - _("Unable to change target guest XML during " - "migration")); - return -1; - } - if (flags & VIR_MIGRATE_PEER2PEER) { - if (miguri) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Unable to override peer2peer migration URI")); - return -1; - } - uri = dconnuri; - } else { - uri = miguri; - } - return domain->conn->driver->domainMigratePerform - (domain, NULL, 0, uri, flags, dname, bandwidth); + return virDomainMigrateUnmanagedProto2(domain, dconnuri, + params, nparams, flags); } } -- 1.7.1
Move virDomainMigrateUnmanagedProto* expected params list check into function itself and use common virTypedParamsCheck for this purpose. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/libvirt-domain.c | 58 +++++++++++++++++++++---------------------------- 1 files changed, 25 insertions(+), 33 deletions(-) diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index fab9564..6261f6b 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -3322,30 +3322,32 @@ virDomainMigrateUnmanagedProto2(virDomainPtr domain, 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 *uri = NULL; const char *miguri = NULL; const char *dname = NULL; - const char *xmlin = NULL; unsigned long long bandwidth = 0; + if (!virTypedParamsCheck(params, nparams, compatParams, + ARRAY_CARDINALITY(compatParams))) { + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", + _("Some parameters are not supported by migration " + "protocol 2.")); + return -1; + } + if (virTypedParamsGetString(params, nparams, VIR_MIGRATE_PARAM_URI, &miguri) < 0 || virTypedParamsGetString(params, nparams, VIR_MIGRATE_PARAM_DEST_NAME, &dname) < 0 || - virTypedParamsGetString(params, nparams, - VIR_MIGRATE_PARAM_DEST_XML, &xmlin) < 0 || virTypedParamsGetULLong(params, nparams, VIR_MIGRATE_PARAM_BANDWIDTH, &bandwidth) < 0) { return -1; } - if (xmlin) { - virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", - _("Unable to change target guest XML during " - "migration")); - return -1; - } - if (flags & VIR_MIGRATE_PEER2PEER) { if (miguri) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -3369,11 +3371,23 @@ virDomainMigrateUnmanagedProto3(virDomainPtr domain, int nparams, unsigned int flags) { + const char *compatParams[] = { VIR_MIGRATE_PARAM_URI, + VIR_MIGRATE_PARAM_DEST_NAME, + VIR_MIGRATE_PARAM_DEST_XML, + VIR_MIGRATE_PARAM_BANDWIDTH }; const char *miguri = NULL; const char *dname = NULL; const char *xmlin = NULL; unsigned long long bandwidth = 0; + if (!virTypedParamsCheck(params, nparams, compatParams, + ARRAY_CARDINALITY(compatParams))) { + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", + _("Some parameters are not supported by migration " + "protocol 3")); + return -1; + } + if (virTypedParamsGetString(params, nparams, VIR_MIGRATE_PARAM_URI, &miguri) < 0 || virTypedParamsGetString(params, nparams, @@ -4468,12 +4482,6 @@ 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 }; - VIR_DOMAIN_DEBUG(domain, "dconnuri=%s, params=%p, nparms=%u flags=%x", NULLSTR(dconnuri), params, nparams, flags); VIR_TYPED_PARAMS_DEBUG(params, nparams); @@ -4488,9 +4496,6 @@ virDomainMigrateToURI3(virDomainPtr domain, VIR_MIGRATE_NON_SHARED_INC, error); - compat = virTypedParamsCheck(params, nparams, compatParams, - ARRAY_CARDINALITY(compatParams)); - if (flags & VIR_MIGRATE_PEER2PEER) { if (!VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, VIR_DRV_FEATURE_MIGRATION_P2P)) { @@ -4506,17 +4511,11 @@ virDomainMigrateToURI3(virDomainPtr domain, if (virDomainMigratePeer2PeerParams(domain, dconnuri, params, nparams, flags) < 0) goto error; - } else if (compat) { + } else { VIR_DEBUG("Using peer2peer migration"); if (virDomainMigrateUnmanagedParams(domain, dconnuri, params, nparams, flags) < 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, @@ -4528,13 +4527,6 @@ virDomainMigrateToURI3(virDomainPtr domain, 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 (virDomainMigrateUnmanagedParams(domain, NULL, params, nparams, flags) < 0) -- 1.7.1
On Fri, Oct 02, 2015 at 10:52:49 +0300, Nikolay Shirokovskiy wrote:
Move virDomainMigrateUnmanagedProto* expected params list check into function itself and use common virTypedParamsCheck for this purpose.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/libvirt-domain.c | 58 +++++++++++++++++++++---------------------------- 1 files changed, 25 insertions(+), 33 deletions(-)
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index fab9564..6261f6b 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -3322,30 +3322,32 @@ virDomainMigrateUnmanagedProto2(virDomainPtr domain, 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 *uri = NULL; const char *miguri = NULL; const char *dname = NULL; - const char *xmlin = NULL; unsigned long long bandwidth = 0;
+ if (!virTypedParamsCheck(params, nparams, compatParams, + ARRAY_CARDINALITY(compatParams))) { + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", + _("Some parameters are not supported by migration " + "protocol 2."));
Drop '.' at the end of the message. Jirka
Finally on this step we get what we were aimed for - toURI{1, 2} (and migration{*} APIs too) now can work thru V3_PARAMS protocol. Execution path goes thru unchanged virDomainMigrateUnmanaged adapter function which is called by all target places. Note that we keep the fact that direct migration never works thru V3_PARAMS proto. We can't change this aspect without further investigation. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/libvirt-domain.c | 56 ++++++++++++++----------------------------------- 1 files changed, 16 insertions(+), 40 deletions(-) diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 6261f6b..2778a15 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -3434,8 +3434,19 @@ virDomainMigrateUnmanagedParams(virDomainPtr domain, virDomainMigrateCheckNotLocal(dconnuri) < 0) return -1; - if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, - VIR_DRV_FEATURE_MIGRATION_V3)) { + if ((flags & VIR_MIGRATE_PEER2PEER) && + VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, + VIR_DRV_FEATURE_MIGRATION_PARAMS)) { + VIR_DEBUG("Using migration protocol 3 with extensible parameters"); + if (!domain->conn->driver->domainMigratePerform3Params) { + virReportUnsupportedError(); + return -1; + } + 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"); if (!domain->conn->driver->domainMigratePerform3) { virReportUnsupportedError(); @@ -3456,32 +3467,6 @@ virDomainMigrateUnmanagedParams(virDomainPtr domain, static int -virDomainMigratePeer2PeerParams(virDomainPtr domain, - const char *dconnuri, - virTypedParameterPtr params, - int nparams, - unsigned int flags) -{ - VIR_DOMAIN_DEBUG(domain, "dconnuri=%s, params=%p, nparams=%d, flags=%x", - dconnuri, params, nparams, flags); - VIR_TYPED_PARAMS_DEBUG(params, nparams); - - if (!domain->conn->driver->domainMigratePerform3Params) { - virReportUnsupportedError(); - return -1; - } - - if (virDomainMigrateCheckNotLocal(dconnuri) < 0) - return -1; - - VIR_DEBUG("Using migration protocol 3 with extensible parameters"); - return domain->conn->driver->domainMigratePerform3Params - (domain, dconnuri, params, nparams, - NULL, 0, NULL, NULL, flags); -} - - -static int virDomainMigrateUnmanaged(virDomainPtr domain, const char *xmlin, unsigned int flags, @@ -4505,18 +4490,9 @@ virDomainMigrateToURI3(virDomainPtr domain, 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 { - VIR_DEBUG("Using peer2peer migration"); - if (virDomainMigrateUnmanagedParams(domain, dconnuri, params, - nparams, flags) < 0) - goto error; - } + VIR_DEBUG("Using peer2peer migration"); + if (virDomainMigrateUnmanagedParams(domain, dconnuri, params, nparams, flags) < 0) + goto error; } else { if (!VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, VIR_DRV_FEATURE_MIGRATION_DIRECT)) { -- 1.7.1
virDomainMigrateUnmanagedParams is not a good candidate for this functionality as it is used by migrate family functions too and its have its own checks that are superset of extracted and we don't need to check twice. Actually name of the function is slightly misleading as there is also a check for consistensy of flags parameter alone. So it could be refactored further and reused by all migrate functions but for now let it be a matter of a different patchset. It is *not* a pure refactoring patch as it introduces offline check for older versions. Looks like it must be done that way and no one will be broken too. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/libvirt-domain.c | 165 ++++++++++++++++++++----------------------------- 1 files changed, 67 insertions(+), 98 deletions(-) diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 2778a15..35dfa3f 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -4139,6 +4139,46 @@ virDomainMigrate3(virDomainPtr domain, } +static +int virDomainMigrateUnmanagedCheckCompat(virDomainPtr domain, + unsigned int flags) +{ + VIR_EXCLUSIVE_FLAGS_RET(VIR_MIGRATE_NON_SHARED_DISK, + VIR_MIGRATE_NON_SHARED_INC, + -1); + + 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")); + return -1; + } + + if (flags & VIR_MIGRATE_PEER2PEER) { + if (!VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, + VIR_DRV_FEATURE_MIGRATION_P2P)) { + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", + _("p2p migration is not supported by " + "the source host")); + return -1; + } + } else { + if (!VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, + VIR_DRV_FEATURE_MIGRATION_DIRECT)) { + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", + _("direct migration is not supported by " + "the source host")); + return -1; + } + } + + + return 0; +} + + /** * virDomainMigrateToURI: * @domain: a domain object @@ -4217,6 +4257,9 @@ virDomainMigrateToURI(virDomainPtr domain, const char *dname, unsigned long bandwidth) { + const char *dconnuri = NULL; + const char *miguri = NULL; + VIR_DOMAIN_DEBUG(domain, "duri=%p, flags=%lx, dname=%s, bandwidth=%lu", NULLSTR(duri), flags, NULLSTR(dname), bandwidth); @@ -4225,49 +4268,19 @@ virDomainMigrateToURI(virDomainPtr domain, /* 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")); + if (virDomainMigrateUnmanagedCheckCompat(domain, flags) < 0) 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 (virDomainMigrateUnmanaged(domain, NULL, flags, - dname, duri, NULL, 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 (virDomainMigrateUnmanaged(domain, NULL, flags, - dname, NULL, 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 (flags & VIR_MIGRATE_PEER2PEER) + dconnuri = duri; + else + miguri = duri; + + if (virDomainMigrateUnmanaged(domain, NULL, flags, + dname, dconnuri, miguri, bandwidth) < 0) + goto error; return 0; @@ -4383,37 +4396,15 @@ virDomainMigrateToURI2(virDomainPtr domain, virCheckDomainReturn(domain, -1); virCheckReadOnlyGoto(domain->conn->flags, error); - VIR_EXCLUSIVE_FLAGS_GOTO(VIR_MIGRATE_NON_SHARED_DISK, - VIR_MIGRATE_NON_SHARED_INC, - error); + if (virDomainMigrateUnmanagedCheckCompat(domain, flags) < 0) + 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 (virDomainMigrateUnmanaged(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 (virDomainMigrateUnmanaged(domain, dxml, flags, - dname, NULL, 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; - } - } + if (!(flags & VIR_MIGRATE_PEER2PEER)) + dconnuri = NULL; + + if (virDomainMigrateUnmanaged(domain, NULL, flags, + dname, dconnuri, miguri, bandwidth) < 0) + goto error; return 0; @@ -4477,37 +4468,15 @@ virDomainMigrateToURI3(virDomainPtr domain, virCheckDomainReturn(domain, -1); virCheckReadOnlyGoto(domain->conn->flags, error); - VIR_EXCLUSIVE_FLAGS_GOTO(VIR_MIGRATE_NON_SHARED_DISK, - VIR_MIGRATE_NON_SHARED_INC, - 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 (virDomainMigrateUnmanagedCheckCompat(domain, flags) < 0) + goto error; - VIR_DEBUG("Using peer2peer migration"); - if (virDomainMigrateUnmanagedParams(domain, dconnuri, params, nparams, flags) < 0) - 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 (!(flags & VIR_MIGRATE_PEER2PEER)) + dconnuri = NULL; - VIR_DEBUG("Using direct migration"); - if (virDomainMigrateUnmanagedParams(domain, NULL, params, - nparams, flags) < 0) - goto error; - } + if (virDomainMigrateUnmanagedParams(domain, dconnuri, + params, nparams, flags) < -1) + goto error; return 0; -- 1.7.1
On Fri, Oct 02, 2015 at 10:52:51 +0300, Nikolay Shirokovskiy wrote:
virDomainMigrateUnmanagedParams is not a good candidate for this functionality as it is used by migrate family functions too and its have its own checks that are superset of extracted and we don't need to check twice.
Actually name of the function is slightly misleading as there is also a check for consistensy of flags parameter alone. So it could be refactored further and reused by all migrate functions but for now let it be a matter of a different patchset.
It is *not* a pure refactoring patch as it introduces offline check for older versions. Looks like it must be done that way and no one will be broken too.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/libvirt-domain.c | 165 ++++++++++++++++++++----------------------------- 1 files changed, 67 insertions(+), 98 deletions(-)
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 2778a15..35dfa3f 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -4139,6 +4139,46 @@ virDomainMigrate3(virDomainPtr domain, }
+static +int virDomainMigrateUnmanagedCheckCompat(virDomainPtr domain, + unsigned int flags) +{ + VIR_EXCLUSIVE_FLAGS_RET(VIR_MIGRATE_NON_SHARED_DISK, + VIR_MIGRATE_NON_SHARED_INC, + -1); + + 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")); + return -1; + } + + if (flags & VIR_MIGRATE_PEER2PEER) { + if (!VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, + VIR_DRV_FEATURE_MIGRATION_P2P)) { + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", + _("p2p migration is not supported by " + "the source host")); + return -1; + } + } else { + if (!VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, + VIR_DRV_FEATURE_MIGRATION_DIRECT)) { + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", + _("direct migration is not supported by " + "the source host")); + return -1; + } + } + +
One empty line would be enough :-)
+ return 0; +}
Jirka
Check dconnuri is not null or we will catch nullpointer later. I hope this makes Coverity happy. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/libvirt-domain.c | 8 ++++++-- 1 files changed, 6 insertions(+), 2 deletions(-) diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 35dfa3f..b9e6ac9 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -4399,7 +4399,9 @@ virDomainMigrateToURI2(virDomainPtr domain, if (virDomainMigrateUnmanagedCheckCompat(domain, flags) < 0) goto error; - if (!(flags & VIR_MIGRATE_PEER2PEER)) + if (flags & VIR_MIGRATE_PEER2PEER) + virCheckNonNullArgGoto(dconnuri, error); + else dconnuri = NULL; if (virDomainMigrateUnmanaged(domain, NULL, flags, @@ -4471,7 +4473,9 @@ virDomainMigrateToURI3(virDomainPtr domain, if (virDomainMigrateUnmanagedCheckCompat(domain, flags) < 0) goto error; - if (!(flags & VIR_MIGRATE_PEER2PEER)) + if (flags & VIR_MIGRATE_PEER2PEER) + virCheckNonNullArgGoto(dconnuri, error); + else dconnuri = NULL; if (virDomainMigrateUnmanagedParams(domain, dconnuri, -- 1.7.1
On Fri, Oct 02, 2015 at 10:52:40 +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.
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.
= Changes from version 3
* fix spellings * make error messages more specific as suggested * in 'migration: refactor: introduce params version of unmanaged': fix conversion for null values of explicit string params into virTypedParameterPtr. * squash 'migration: reuse parameters check in toURI2 and toURI3' into 'migration: introduce parameter checking function' * 'migration: refactor: one return in forURI family functions' is dropped as suggested
src/libvirt-domain.c | 509 +++++++++++++++++++++++--------------------------- 1 files changed, 237 insertions(+), 272 deletions(-)
ACK series and pushed with the three cosmetics issues fixed. Thanks for your work. Jirka
participants (2)
-
Jiri Denemark -
Nikolay Shirokovskiy