[libvirt] [PATCH v3 0/14] 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 version2 1. fix misc typos and overall english in commit messages 2. fix misc style issues 3. add 2 new patches 1. migration: refactor: rename uri parameter to miguri 2. migration: check dconnuri in p2p mode 4. rework succession of 2 patches 1. migration: refactor: prepare to reuse flag vs feature checks 2. migration: reuse flags vs features checks in toURI family into 1. migration: refactor: introduce parameter checking function 2. migration: reuse parameters check in toURI2 and toURI3 5. rearrange functions as suggested by reviwer to make better diffs The overall diff as compared to the previous patchset is really small. src/libvirt-domain.c | 531 ++++++++++++++++++++++--------------------------- 1 files changed, 238 insertions(+), 293 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
On Fri, Sep 18, 2015 at 18:05:39 +0300, Nikolay Shirokovskiy wrote:
'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(-)
ACK Jirka
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, Sep 18, 2015 at 18:05:40 +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"));
Hmm, I don't think this new error message is better than the old one. But anyway, we shouldn't need this code at all, checking dconnuri on a client and then passing it to libvirtd which will have to check it again does not make a lot of sense. In other words, I think we should just completely remove the dconuri checks from virDomainMigratePeer2Peer*. Jirka
On 25.09.2015 17:12, Jiri Denemark wrote:
On Fri, Sep 18, 2015 at 18:05:40 +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"));
Hmm, I don't think this new error message is better than the old one. But anyway, we shouldn't need this code at all, checking dconnuri on a client and then passing it to libvirtd which will have to check it again does not make a lot of sense.
In other words, I think we should just completely remove the dconuri checks from virDomainMigratePeer2Peer*.
Jirka
Sorry, for delay. I hoped you'll finish series review. The meaning of this check is unclear, so i found it origins, see https://www.redhat.com/archives/libvir-list/2011-January/msg00437.html. This message states that without this check we can get a deadlock, thus I leave it. Also I investigate if this check had been moved to qemu code as reviewer suggested. Looks likes it is not.
On Wed, Sep 30, 2015 at 10:20:23 +0300, Nikolay Shirokovskiy wrote:
On 25.09.2015 17:12, Jiri Denemark wrote:
On Fri, Sep 18, 2015 at 18:05:40 +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"));
Hmm, I don't think this new error message is better than the old one. But anyway, we shouldn't need this code at all, checking dconnuri on a client and then passing it to libvirtd which will have to check it again does not make a lot of sense.
In other words, I think we should just completely remove the dconuri checks from virDomainMigratePeer2Peer*.
Jirka
Sorry, for delay. I hoped you'll finish series review.
Yeah, sorry about that. A long weekend came into my review session :-)
The meaning of this check is unclear, so i found it origins, see https://www.redhat.com/archives/libvir-list/2011-January/msg00437.html. This message states that without this check we can get a deadlock, thus I leave it. Also I investigate if this check had been moved to qemu code as reviewer suggested. Looks likes it is not.
I see, unless we have the check in the qemu driver since a long time ago, I'm afraid we need to keep it in libvirt-domain.c. 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
On Fri, Sep 18, 2015 at 18:05:41 +0300, Nikolay Shirokovskiy wrote:
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(-)
ACK Jirka
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
On Fri, Sep 18, 2015 at 18:05:42 +0300, Nikolay Shirokovskiy wrote:
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(-)
ACK Jirka
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
On Fri, Sep 18, 2015 at 18:05:43 +0300, Nikolay Shirokovskiy wrote:
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(-)
ACK Jirka
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..f52c3bf 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 incapsulates 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
On Fri, Sep 18, 2015 at 18:05:44 +0300, Nikolay Shirokovskiy wrote:
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..f52c3bf 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 incapsulates 2 alternatives to the above case.
s/incapsulates/encapsulates/ ACK Jirka
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 | 92 +++++++++++++++++++++++++++++++++----------------- 1 files changed, 61 insertions(+), 31 deletions(-) diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index f52c3bf..330ccab 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,40 @@ 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 (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, xmlin) < 0 || + 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 +4425,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 +4443,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 +4460,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 +4488,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
On Fri, Sep 18, 2015 at 18:05:45 +0300, Nikolay Shirokovskiy wrote:
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 | 92 +++++++++++++++++++++++++++++++++----------------- 1 files changed, 61 insertions(+), 31 deletions(-)
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index f52c3bf..330ccab 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c ... +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 (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, xmlin) < 0 || + virTypedParamsAddULLong(¶ms, &nparams, &maxparams, + VIR_MIGRATE_PARAM_BANDWIDTH, bandwidth) < 0) + goto cleanup;
All the *AddString calls should only be done when the appropriate value is not NULL, otherwise *AddString will actually add "" into params and thus you will get "" rather than NULL in virDomainMigrateUnmanagedParams. In other words, something like if (miguri && virTypedParamsAddString(¶ms, &nparams, &maxparams, VIR_MIGRATE_PARAM_URI, miguri) < 0) goto cleanup; ... Jirka
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 330ccab..f87a22d 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
On Fri, Sep 18, 2015 at 18:05:46 +0300, Nikolay Shirokovskiy wrote:
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 330ccab..f87a22d 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",
While at it, you could also change VIR_ERR_INTERNAL_ERROR to a more appropriate VIR_ERR_ARGUMENT_UNSUPPORTED. But it can be done in a separate patch. ... At first sight I was thinking this patch makes the code even worse since a new parameter would need to be added in more places, but that's addressed in the next patch. ACK Jirka
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 | 56 ++++++++++++++++++++----------------------------- 1 files changed, 23 insertions(+), 33 deletions(-) diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index f87a22d..abed9d6 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -3322,30 +3322,31 @@ 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", + _("Migration does not support some of parameters.")); + 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 +3370,22 @@ 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", + _("Migration does not support some of parameters.")); + return -1; + } + if (virTypedParamsGetString(params, nparams, VIR_MIGRATE_PARAM_URI, &miguri) < 0 || virTypedParamsGetString(params, nparams, @@ -4462,12 +4474,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); @@ -4482,9 +4488,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)) { @@ -4500,17 +4503,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, @@ -4522,13 +4519,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, Sep 18, 2015 at 18:05:47 +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 | 56 ++++++++++++++++++++----------------------------- 1 files changed, 23 insertions(+), 33 deletions(-)
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index f87a22d..abed9d6 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -3322,30 +3322,31 @@ 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", + _("Migration does not support some of parameters."));
How about "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", _("Unable to override peer2peer migration URI")); return -1; }
Can we merge this check with with the others by adding VIR_MIGRATE_PARAM_URI to compatParams only if VIR_MIGRATE_PEER2PEER was passed? I think it would be a bit cleaner, although it depends on how dirty way of doing that you choose :-)
@@ -3369,11 +3370,22 @@ 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", + _("Migration does not support some of parameters."));
How about "Some parameters are not supported by migration protocol 3"?
+ return -1; + } + if (virTypedParamsGetString(params, nparams, VIR_MIGRATE_PARAM_URI, &miguri) < 0 || virTypedParamsGetString(params, nparams,
Jirka
On 30.09.2015 16:38, Jiri Denemark wrote:
On Fri, Sep 18, 2015 at 18:05:47 +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 | 56 ++++++++++++++++++++----------------------------- 1 files changed, 23 insertions(+), 33 deletions(-)
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index f87a22d..abed9d6 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -3322,30 +3322,31 @@ 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", + _("Migration does not support some of parameters."));
How about "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", _("Unable to override peer2peer migration URI")); return -1; }
Can we merge this check with with the others by adding VIR_MIGRATE_PARAM_URI to compatParams only if VIR_MIGRATE_PEER2PEER was passed? I think it would be a bit cleaner, although it depends on how dirty way of doing that you choose :-) I'm not sure is this considered dirty?
1. char** cast (could be fixed by changing function signature to const char * const * 2. unchecked array accessing - safe until somebody will edit this code const char *compatParams[3] = { VIR_MIGRATE_PARAM_DEST_NAME, VIR_MIGRATE_PARAM_BANDWIDTH, NULL}; size_t ncompatParams = 0; ncompatParams = virStringListLength((char**)compatParams); if (!(flags & VIR_MIGRATE_PEER2PEER)) compatParams[ncompatParams++] = VIR_MIGRATE_PARAM_URI;
@@ -3369,11 +3370,22 @@ 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", + _("Migration does not support some of parameters."));
How about "Some parameters are not supported by migration protocol 3"?
+ return -1; + } + if (virTypedParamsGetString(params, nparams, VIR_MIGRATE_PARAM_URI, &miguri) < 0 || virTypedParamsGetString(params, nparams,
Jirka
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, Oct 01, 2015 at 10:34:49 +0300, Nikolay Shirokovskiy wrote:
On 30.09.2015 16:38, Jiri Denemark wrote:
On Fri, Sep 18, 2015 at 18:05:47 +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 | 56 ++++++++++++++++++++----------------------------- 1 files changed, 23 insertions(+), 33 deletions(-)
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index f87a22d..abed9d6 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -3322,30 +3322,31 @@ 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", + _("Migration does not support some of parameters."));
How about "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", _("Unable to override peer2peer migration URI")); return -1; }
Can we merge this check with with the others by adding VIR_MIGRATE_PARAM_URI to compatParams only if VIR_MIGRATE_PEER2PEER was passed? I think it would be a bit cleaner, although it depends on how dirty way of doing that you choose :-) I'm not sure is this considered dirty?
1. char** cast (could be fixed by changing function signature to const char * const * 2. unchecked array accessing - safe until somebody will edit this code
const char *compatParams[3] = { VIR_MIGRATE_PARAM_DEST_NAME, VIR_MIGRATE_PARAM_BANDWIDTH, NULL}; size_t ncompatParams = 0;
ncompatParams = virStringListLength((char**)compatParams); if (!(flags & VIR_MIGRATE_PEER2PEER)) compatParams[ncompatParams++] = VIR_MIGRATE_PARAM_URI;
I don't know, I was thinking about something similar: const char *compatParams[] = {VIR_MIGRATE_PARAM_DEST_NAME, VIR_MIGRATE_PARAM_BANDWIDTH, VIR_MIGRATE_PARAM_URI}; size_t ncompatParams = ARRAY_CARDINALITY(compatParams); if (flags & VIR_MIGRATE_PEER2PEER) ncompatParams--; if (!virTypedParamsCheck(params, nparams, compatParams, ncompatParams) ... but it looked similarly fragile. Maybe defining two arrays, one for p2p and one for direct migration would be the right clean way of doing this... Jirka
On 01.10.2015 12:30, Jiri Denemark wrote:
On Thu, Oct 01, 2015 at 10:34:49 +0300, Nikolay Shirokovskiy wrote:
On 30.09.2015 16:38, Jiri Denemark wrote:
On Fri, Sep 18, 2015 at 18:05:47 +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 | 56 ++++++++++++++++++++----------------------------- 1 files changed, 23 insertions(+), 33 deletions(-)
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index f87a22d..abed9d6 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -3322,30 +3322,31 @@ 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", + _("Migration does not support some of parameters."));
How about "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", _("Unable to override peer2peer migration URI")); return -1; }
Can we merge this check with with the others by adding VIR_MIGRATE_PARAM_URI to compatParams only if VIR_MIGRATE_PEER2PEER was passed? I think it would be a bit cleaner, although it depends on how dirty way of doing that you choose :-) I'm not sure is this considered dirty?
1. char** cast (could be fixed by changing function signature to const char * const * 2. unchecked array accessing - safe until somebody will edit this code
const char *compatParams[3] = { VIR_MIGRATE_PARAM_DEST_NAME, VIR_MIGRATE_PARAM_BANDWIDTH, NULL}; size_t ncompatParams = 0;
ncompatParams = virStringListLength((char**)compatParams); if (!(flags & VIR_MIGRATE_PEER2PEER)) compatParams[ncompatParams++] = VIR_MIGRATE_PARAM_URI;
I don't know, I was thinking about something similar:
const char *compatParams[] = {VIR_MIGRATE_PARAM_DEST_NAME, VIR_MIGRATE_PARAM_BANDWIDTH, VIR_MIGRATE_PARAM_URI}; size_t ncompatParams = ARRAY_CARDINALITY(compatParams);
if (flags & VIR_MIGRATE_PEER2PEER) ncompatParams--;
if (!virTypedParamsCheck(params, nparams, compatParams, ncompatParams) ...
but it looked similarly fragile. Maybe defining two arrays, one for p2p and one for direct migration would be the right clean way of doing this...
then it became bulky, like const char *compatParamsP2P[] = { VIR_MIGRATE_PARAM_DEST_NAME, VIR_MIGRATE_PARAM_BANDWIDTH }; const char *compatParamsDirect[] = { 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; unsigned long long bandwidth = 0; if (flags & VIR_MIGRATE_PEER2PEER) { if (!virTypedParamsCheck(params, nparams, compatParamsP2P, ARRAY_CARDINALITY(compatParamsP2P))) { virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", _("Some parameters are not supported by migration " "protocol 2.")); return -1; } } else { if (!virTypedParamsCheck(params, nparams, compatParamsDirect, ARRAY_CARDINALITY(compatParamsDirect))) { virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", _("Some parameters are not supported by migration " "protocol 2.")); return -1; } } May be return to the first variant. Anyway uri parameter is special case.
Jirka
On Thu, Oct 01, 2015 at 16:48:44 +0300, Nikolay Shirokovskiy wrote:
On 01.10.2015 12:30, Jiri Denemark wrote:
On Thu, Oct 01, 2015 at 10:34:49 +0300, Nikolay Shirokovskiy wrote:
On 30.09.2015 16:38, Jiri Denemark wrote:
On Fri, Sep 18, 2015 at 18:05:47 +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 | 56 ++++++++++++++++++++----------------------------- 1 files changed, 23 insertions(+), 33 deletions(-)
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index f87a22d..abed9d6 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -3322,30 +3322,31 @@ 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", + _("Migration does not support some of parameters."));
How about "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", _("Unable to override peer2peer migration URI")); return -1; }
Can we merge this check with with the others by adding VIR_MIGRATE_PARAM_URI to compatParams only if VIR_MIGRATE_PEER2PEER was passed? I think it would be a bit cleaner, although it depends on how dirty way of doing that you choose :-) I'm not sure is this considered dirty?
1. char** cast (could be fixed by changing function signature to const char * const * 2. unchecked array accessing - safe until somebody will edit this code
const char *compatParams[3] = { VIR_MIGRATE_PARAM_DEST_NAME, VIR_MIGRATE_PARAM_BANDWIDTH, NULL}; size_t ncompatParams = 0;
ncompatParams = virStringListLength((char**)compatParams); if (!(flags & VIR_MIGRATE_PEER2PEER)) compatParams[ncompatParams++] = VIR_MIGRATE_PARAM_URI;
I don't know, I was thinking about something similar:
const char *compatParams[] = {VIR_MIGRATE_PARAM_DEST_NAME, VIR_MIGRATE_PARAM_BANDWIDTH, VIR_MIGRATE_PARAM_URI}; size_t ncompatParams = ARRAY_CARDINALITY(compatParams);
if (flags & VIR_MIGRATE_PEER2PEER) ncompatParams--;
if (!virTypedParamsCheck(params, nparams, compatParams, ncompatParams) ...
but it looked similarly fragile. Maybe defining two arrays, one for p2p and one for direct migration would be the right clean way of doing this...
then it became bulky, like
const char *compatParamsP2P[] = { VIR_MIGRATE_PARAM_DEST_NAME, VIR_MIGRATE_PARAM_BANDWIDTH }; const char *compatParamsDirect[] = { 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; unsigned long long bandwidth = 0;
if (flags & VIR_MIGRATE_PEER2PEER) { if (!virTypedParamsCheck(params, nparams, compatParamsP2P, ARRAY_CARDINALITY(compatParamsP2P))) { virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", _("Some parameters are not supported by migration " "protocol 2.")); return -1; } } else { if (!virTypedParamsCheck(params, nparams, compatParamsDirect, ARRAY_CARDINALITY(compatParamsDirect))) { virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", _("Some parameters are not supported by migration " "protocol 2.")); return -1; } }
Well, you could make it a bit shorter: const char **compat; size_t ncompat; if (flags & VIR_MIGRATE_PEER2PEER) { compat = compatParamsP2P; ncompat = ARRAY_CARDINALITY(compatParamsP2P); } else { compat = compatParamsDirect; ncompat = ARRAY_CARDINALITY(compatParamsDirect); } if (!virTypedParamsCheck(params, nparams, compat, ncompat) ...
May be return to the first variant. Anyway uri parameter is special case.
Yeah, looking at all other options I think your original code was the best :-) So just keep it unchanged. 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 abed9d6..5c22460 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -3432,8 +3432,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(); @@ -3454,32 +3465,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, @@ -4497,18 +4482,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
On Fri, Sep 18, 2015 at 18:05:48 +0300, Nikolay Shirokovskiy wrote:
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 abed9d6..5c22460 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -3432,8 +3432,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) &&
I though one of the reason you're doing all this is to remove the need to implement older API for direct migration so I was expecting this part of the condition to go away...
+ 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();
Jirka
On 30.09.2015 16:38, Jiri Denemark wrote:
On Fri, Sep 18, 2015 at 18:05:48 +0300, Nikolay Shirokovskiy wrote:
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 abed9d6..5c22460 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -3432,8 +3432,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) &&
I though one of the reason you're doing all this is to remove the need to implement older API for direct migration so I was expecting this part of the condition to go away...
This patchset is large enough) I'd better leave the answer to the question is this check could be safely deleted to different patch series.
+ 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();
Jirka
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/libvirt-domain.c | 83 +++++++++++++++++++++++++++----------------------- 1 files changed, 45 insertions(+), 38 deletions(-) diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 5c22460..eec45bd 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -4131,6 +4131,39 @@ virDomainMigrate3(virDomainPtr domain, } +static +int virDomainMigrateUnmanagedCheckCompat(virDomainPtr domain, + unsigned int flags) +{ + int feat; + + 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) + feat = VIR_DRV_FEATURE_MIGRATION_P2P; + else + feat = VIR_DRV_FEATURE_MIGRATION_DIRECT; + + if (!VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, feat)) { + virReportUnsupportedError(); + return -1; + } + + return 0; +} + + /** * virDomainMigrateToURI: * @domain: a domain object @@ -4209,6 +4242,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); @@ -4220,46 +4256,17 @@ virDomainMigrateToURI(virDomainPtr domain, 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; -- 1.7.1
On Fri, Sep 18, 2015 at 18:05:49 +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.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/libvirt-domain.c | 83 +++++++++++++++++++++++++++----------------------- 1 files changed, 45 insertions(+), 38 deletions(-)
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 5c22460..eec45bd 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -4131,6 +4131,39 @@ virDomainMigrate3(virDomainPtr domain, }
+static +int virDomainMigrateUnmanagedCheckCompat(virDomainPtr domain, + unsigned int flags) +{ + int feat; + + 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) + feat = VIR_DRV_FEATURE_MIGRATION_P2P; + else + feat = VIR_DRV_FEATURE_MIGRATION_DIRECT; + + if (!VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, feat)) { + virReportUnsupportedError();
Use virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, ...), virReportUnsupportedError is for unsupported APIs while VIR_ERR_ARGUMENT_UNSUPPORTED says the API is supported but some of its arguments are not. While at it, I think it's better to keep the code a bit verbose and separate checks for p2p and direct migration to be able to provide a usable error message. Jirka
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 | 75 ++++++++++---------------------------------------- 1 files changed, 15 insertions(+), 60 deletions(-) diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index eec45bd..fc61830 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -4253,7 +4253,6 @@ virDomainMigrateToURI(virDomainPtr domain, /* First checkout the source */ virCheckDomainReturn(domain, -1); virCheckReadOnlyGoto(domain->conn->flags, error); - virCheckNonNullArgGoto(duri, error); if (virDomainMigrateUnmanagedCheckCompat(domain, flags) < 0) @@ -4382,37 +4381,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; @@ -4476,37 +4453,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, Sep 18, 2015 at 18:05:50 +0300, Nikolay Shirokovskiy wrote:
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 | 75 ++++++++++---------------------------------------- 1 files changed, 15 insertions(+), 60 deletions(-)
I think you could squash this patch into the previous one which does similar thing in *ToURI.
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index eec45bd..fc61830 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -4253,7 +4253,6 @@ virDomainMigrateToURI(virDomainPtr domain, /* First checkout the source */ virCheckDomainReturn(domain, -1); virCheckReadOnlyGoto(domain->conn->flags, error); - virCheckNonNullArgGoto(duri, error);
if (virDomainMigrateUnmanagedCheckCompat(domain, flags) < 0)
And this hunk definitely doesn't belong to this patch... one more reason to squash patch 12 and 11 :-) Jirka
May be a matter of a taste but this version with one return point in every function looks simplier to understand and to extend too. Anyway after such a heavy refactoring a little cleanup will not hurt. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/libvirt-domain.c | 61 +++++++++++++++++++++++--------------------------- 1 files changed, 28 insertions(+), 33 deletions(-) diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index fc61830..f012f4b 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -4242,6 +4242,7 @@ virDomainMigrateToURI(virDomainPtr domain, const char *dname, unsigned long bandwidth) { + int ret = -1; const char *dconnuri = NULL; const char *miguri = NULL; @@ -4252,26 +4253,24 @@ virDomainMigrateToURI(virDomainPtr domain, /* First checkout the source */ virCheckDomainReturn(domain, -1); - virCheckReadOnlyGoto(domain->conn->flags, error); - virCheckNonNullArgGoto(duri, error); + virCheckReadOnlyGoto(domain->conn->flags, cleanup); + virCheckNonNullArgGoto(duri, cleanup); if (virDomainMigrateUnmanagedCheckCompat(domain, flags) < 0) - goto error; + goto cleanup; if (flags & VIR_MIGRATE_PEER2PEER) dconnuri = duri; else miguri = duri; - if (virDomainMigrateUnmanaged(domain, NULL, flags, - dname, dconnuri, miguri, bandwidth) < 0) - goto error; - - return 0; + ret = virDomainMigrateUnmanaged(domain, NULL, flags, + dname, dconnuri, miguri, bandwidth); + cleanup: + if (ret < 0) + virDispatchError(domain->conn); - error: - virDispatchError(domain->conn); - return -1; + return ret; } @@ -4370,6 +4369,7 @@ virDomainMigrateToURI2(virDomainPtr domain, const char *dname, unsigned long bandwidth) { + int ret = -1; VIR_DOMAIN_DEBUG(domain, "dconnuri=%s, miguri=%s, dxml=%s, " "flags=%lx, dname=%s, bandwidth=%lu", NULLSTR(dconnuri), NULLSTR(miguri), NULLSTR(dxml), @@ -4379,23 +4379,20 @@ virDomainMigrateToURI2(virDomainPtr domain, /* First checkout the source */ virCheckDomainReturn(domain, -1); - virCheckReadOnlyGoto(domain->conn->flags, error); + virCheckReadOnlyGoto(domain->conn->flags, cleanup); if (virDomainMigrateUnmanagedCheckCompat(domain, flags) < 0) - goto error; + goto cleanup; if (!(flags & VIR_MIGRATE_PEER2PEER)) dconnuri = NULL; - if (virDomainMigrateUnmanaged(domain, NULL, flags, - dname, dconnuri, miguri, bandwidth) < 0) - goto error; - - return 0; - - error: - virDispatchError(domain->conn); - return -1; + ret = virDomainMigrateUnmanaged(domain, NULL, flags, + dname, dconnuri, miguri, bandwidth); + cleanup: + if (ret < 0) + virDispatchError(domain->conn); + return ret; } @@ -4443,6 +4440,7 @@ virDomainMigrateToURI3(virDomainPtr domain, unsigned int nparams, unsigned int flags) { + 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); @@ -4451,23 +4449,20 @@ virDomainMigrateToURI3(virDomainPtr domain, /* First checkout the source */ virCheckDomainReturn(domain, -1); - virCheckReadOnlyGoto(domain->conn->flags, error); + virCheckReadOnlyGoto(domain->conn->flags, cleanup); if (virDomainMigrateUnmanagedCheckCompat(domain, flags) < 0) - goto error; + goto cleanup; if (!(flags & VIR_MIGRATE_PEER2PEER)) dconnuri = NULL; - if (virDomainMigrateUnmanagedParams(domain, dconnuri, - params, nparams, flags) < -1) - goto error; - - return 0; - - error: - virDispatchError(domain->conn); - return -1; + ret = virDomainMigrateUnmanagedParams(domain, dconnuri, + params, nparams, flags); + cleanup: + if (ret < 0) + virDispatchError(domain->conn); + return ret; } -- 1.7.1
On Fri, Sep 18, 2015 at 18:05:51 +0300, Nikolay Shirokovskiy wrote:
May be a matter of a taste but this version with one return point in every function looks simplier to understand and to extend too. Anyway after such a heavy refactoring a little cleanup will not hurt.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/libvirt-domain.c | 61 +++++++++++++++++++++++--------------------------- 1 files changed, 28 insertions(+), 33 deletions(-)
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index fc61830..f012f4b 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -4242,6 +4242,7 @@ virDomainMigrateToURI(virDomainPtr domain, const char *dname, unsigned long bandwidth) { + int ret = -1; const char *dconnuri = NULL; const char *miguri = NULL;
@@ -4252,26 +4253,24 @@ virDomainMigrateToURI(virDomainPtr domain,
/* First checkout the source */ virCheckDomainReturn(domain, -1); - virCheckReadOnlyGoto(domain->conn->flags, error); - virCheckNonNullArgGoto(duri, error); + virCheckReadOnlyGoto(domain->conn->flags, cleanup); + virCheckNonNullArgGoto(duri, cleanup);
if (virDomainMigrateUnmanagedCheckCompat(domain, flags) < 0) - goto error; + goto cleanup;
if (flags & VIR_MIGRATE_PEER2PEER) dconnuri = duri; else miguri = duri;
- if (virDomainMigrateUnmanaged(domain, NULL, flags, - dname, dconnuri, miguri, bandwidth) < 0) - goto error; - - return 0; + ret = virDomainMigrateUnmanaged(domain, NULL, flags, + dname, dconnuri, miguri, bandwidth); + cleanup: + if (ret < 0) + virDispatchError(domain->conn);
- error: - virDispatchError(domain->conn); - return -1; + return ret; }
Well, we tend to use cleanup labels when there's some common cleanup to be done for both successful and error case, which is not the case here. And by applying this patch, all *ToURI* functions will be quite different from any other function in this file. That said, I think you could just drop this patch :-) 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 f012f4b..380f374 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -4384,7 +4384,9 @@ virDomainMigrateToURI2(virDomainPtr domain, if (virDomainMigrateUnmanagedCheckCompat(domain, flags) < 0) goto cleanup; - if (!(flags & VIR_MIGRATE_PEER2PEER)) + if (flags & VIR_MIGRATE_PEER2PEER) + virCheckNonNullArgGoto(dconnuri, cleanup); + else dconnuri = NULL; ret = virDomainMigrateUnmanaged(domain, NULL, flags, @@ -4454,7 +4456,9 @@ virDomainMigrateToURI3(virDomainPtr domain, if (virDomainMigrateUnmanagedCheckCompat(domain, flags) < 0) goto cleanup; - if (!(flags & VIR_MIGRATE_PEER2PEER)) + if (flags & VIR_MIGRATE_PEER2PEER) + virCheckNonNullArgGoto(dconnuri, cleanup); + else dconnuri = NULL; ret = virDomainMigrateUnmanagedParams(domain, dconnuri, -- 1.7.1
On Fri, Sep 18, 2015 at 18:05:52 +0300, Nikolay Shirokovskiy wrote:
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 f012f4b..380f374 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -4384,7 +4384,9 @@ virDomainMigrateToURI2(virDomainPtr domain, if (virDomainMigrateUnmanagedCheckCompat(domain, flags) < 0) goto cleanup;
- if (!(flags & VIR_MIGRATE_PEER2PEER)) + if (flags & VIR_MIGRATE_PEER2PEER) + virCheckNonNullArgGoto(dconnuri, cleanup); + else dconnuri = NULL;
ret = virDomainMigrateUnmanaged(domain, NULL, flags, @@ -4454,7 +4456,9 @@ virDomainMigrateToURI3(virDomainPtr domain, if (virDomainMigrateUnmanagedCheckCompat(domain, flags) < 0) goto cleanup;
- if (!(flags & VIR_MIGRATE_PEER2PEER)) + if (flags & VIR_MIGRATE_PEER2PEER) + virCheckNonNullArgGoto(dconnuri, cleanup); + else dconnuri = NULL;
ret = virDomainMigrateUnmanagedParams(domain, dconnuri,
ACK, although you may need to update the labels after dropping the previous patch. Jirka
participants (3)
-
Jiri Denemark -
Nikolay Shirokovskiy -
Nikolay Shirokovskiy