8:20 a.m.
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 version1
Patch is splitted into a set. Quite a big one as a result of the following strategy:
1. each change in behaviour even subtle one deserves a separate patch. One
patch changes one aspect in behaviour.
2. separate pure refactoring steps into patches too as rather simple refactor
steps could introduce many line changes. Mark such patches with 'refactor:'
Now every patch is easy to grasp I think.
The resulted cumulative patch is slightly different from first in behaviour but
I'm not going to describe the differece here as original patch was not reviewed
in details by anyone anyway )
src/libvirt-domain.c | 520 +++++++++++++++++++++-----------------------------
1 files changed, 216 insertions(+), 304 deletions(-)
8:20 a.m.
New subject: [libvirt] [PATCH v2 01/12] migration: refactor: get rid of use_params p2p_full
'useParams' parameter usage is an example of contol coupling. Most of the work
inside the function is done differently for different value of this flag except
for the uri check. Lets split this function into 2, one with extensible
parameters set and one with hardcoded parameter set. Common uri check we factor
out in different patch for clarity.
Aim of this patchset is to unify logic for differet parameters representation
so finally we merge this split back thru extensible parameters.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
---
src/libvirt-domain.c | 129 +++++++++++++++++++++----------------------------
1 files changed, 55 insertions(+), 74 deletions(-)
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
index 964a4d7..1a00485 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -3292,46 +3292,59 @@ virDomainMigrateVersion3Params(virDomainPtr domain,
params, nparams, true, flags);
}
+static int
+virDomainMigratePeer2PeerParams(virDomainPtr domain,
+ const char *dconnuri,
+ virTypedParameterPtr params,
+ 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);
+
+ 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);
+}
-/*
- * In normal migration, the libvirt client co-ordinates communication
- * between the 2 libvirtd instances on source & dest hosts.
- *
- * In this peer-2-peer migration alternative, the libvirt client
- * only talks to the source libvirtd instance. The source libvirtd
- * then opens its own connection to the destination and co-ordinates
- * migration itself.
- *
- * If useParams is true, params and nparams contain migration parameters and
- * we know it's safe to call the API which supports extensible parameters.
- * Otherwise, we have to use xmlin, dname, uri, and bandwidth and pass them
- * to the old-style APIs.
- */
static int
-virDomainMigratePeer2PeerFull(virDomainPtr domain,
- const char *dconnuri,
- const char *xmlin,
- const char *dname,
- const char *uri,
- unsigned long long bandwidth,
- virTypedParameterPtr params,
- int nparams,
- bool useParams,
- unsigned int flags)
+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,12 +3359,7 @@ 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,
+ 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
@@ -3375,33 +3383,6 @@ 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);
-}
-
-
/*
* In normal migration, the libvirt client co-ordinates communication
* between the 2 libvirtd instances on source & dest hosts.
@@ -3605,8 +3586,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 +3807,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 +4188,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 +4334,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 +4465,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
4:54 p.m.
New subject: [libvirt] [PATCH v2 01/12] migration: refactor: get rid of use_params p2p_full
FWIW: I figured I'd at least take a look - it's not my area of expertise
though. I also ran the changes through my Coverity checker. The first
pass found an issue in patch 10, which seems to be a result of some
changes in patch 2 and perhaps patch 3...
On 09/10/2015 09:20 AM, Nikolay Shirokovskiy wrote:
'useParams' parameter usage is an example of contol coupling.
Most of the work
s/contol/control
inside the function is done differently for different value of this
flag except
s/value of this flag//
for the uri check. Lets split this function into 2, one with
extensible
s/2/two
parameters set and one with hardcoded parameter set. Common uri check
we factor
out in different patch for clarity.
Move this last sentence to the commit message of patch 2...
Aim of this patchset is to unify logic for differet parameters representation
so finally we merge this split back thru extensible parameters.
This paragraph could state - this patch begins a series of changes to
the Peer2Peer API's to utilize a common API with extensible parameters.
Or it could be stricken or moved to the cover letter...
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
---
src/libvirt-domain.c | 129 +++++++++++++++++++++----------------------------
1 files changed, 55 insertions(+), 74 deletions(-)
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
index 964a4d7..1a00485 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -3292,46 +3292,59 @@ virDomainMigrateVersion3Params(virDomainPtr domain,
params, nparams, true, flags);
}
+static int
+virDomainMigratePeer2PeerParams(virDomainPtr domain,
+ const char *dconnuri,
+ virTypedParameterPtr params,
+ 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);
+
+ 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);
+}
If this function goes below the "Plain" function, I think the git diff
output will be a lot cleaner... Right now it's still a bit confusing.
That is Plain first then Params second.
-/*
- * In normal migration, the libvirt client co-ordinates communication
- * between the 2 libvirtd instances on source & dest hosts.
- *
- * In this peer-2-peer migration alternative, the libvirt client
- * only talks to the source libvirtd instance. The source libvirtd
- * then opens its own connection to the destination and co-ordinates
- * migration itself.
- *
- * If useParams is true, params and nparams contain migration parameters and
- * we know it's safe to call the API which supports extensible parameters.
- * Otherwise, we have to use xmlin, dname, uri, and bandwidth and pass them
- * to the old-style APIs.
- */
Perhaps a hassle, but for now the comments could have moved with their
respective function... Perhaps the first two paragraphs for the "Plain"
function with the last "Otherwise" sentence changed slightly to indicate
an "old style" static parameter listing.
Then for the "Params" function that last paragraph preceded with a
reference to the "Plain" function... Unlike the "Plain" function,
this
function uses an extensible parameter list...
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,12 +3359,7 @@ 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,
+ if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
VIR_DRV_FEATURE_MIGRATION_V3)) {
extra spacing ^^^^^^^
VIR_DEBUG("Using migration protocol 3");
return domain->conn->driver->domainMigratePerform3
@@ -3375,33 +3383,6 @@ 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);
-}
-
-
Placing "Params" here makes git diff happier or easier to read...
Nothing more beyond that. I'll let others contemplate the Plain name -
since it's going away eventually shouldn't be a problem. It could have
been "Static" too...
John
/*
* In normal migration, the libvirt client co-ordinates communication
* between the 2 libvirtd instances on source & dest hosts.
@@ -3605,8 +3586,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 +3807,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 +4188,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 +4334,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 +4465,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",
9:01 a.m.
New subject: [libvirt] [PATCH v2 01/12] migration: refactor: get rid of use_params p2p_full
On Wed, Sep 16, 2015 at 05:54:30PM -0400, John Ferlan wrote:
FWIW: I figured I'd at least take a look - it's not my area of expertise
though. I also ran the changes through my Coverity checker. The first
pass found an issue in patch 10, which seems to be a result of some
changes in patch 2 and perhaps patch 3...
On 09/10/2015 09:20 AM, Nikolay Shirokovskiy wrote:
> 'useParams' parameter usage is an example of contol coupling. Most of the
work
s/contol/control
> inside the function is done differently for different value of this flag except
s/value of this flag//
> for the uri check. Lets split this function into 2, one with extensible
s/2/two
> parameters set and one with hardcoded parameter set. Common uri check we factor
> out in different patch for clarity.
Move this last sentence to the commit message of patch 2...
>
> Aim of this patchset is to unify logic for differet parameters representation
> so finally we merge this split back thru extensible parameters.
This paragraph could state - this patch begins a series of changes to
the Peer2Peer API's to utilize a common API with extensible parameters.
Or it could be stricken or moved to the cover letter...
>
> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
> ---
> src/libvirt-domain.c | 129 +++++++++++++++++++++----------------------------
> 1 files changed, 55 insertions(+), 74 deletions(-)
>
> diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
> index 964a4d7..1a00485 100644
> --- a/src/libvirt-domain.c
> +++ b/src/libvirt-domain.c
> @@ -3292,46 +3292,59 @@ virDomainMigrateVersion3Params(virDomainPtr domain,
> params, nparams, true, flags);
> }
>
> +static int
> +virDomainMigratePeer2PeerParams(virDomainPtr domain,
> + const char *dconnuri,
> + virTypedParameterPtr params,
> + 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);
> +
> + 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);
> +}
If this function goes below the "Plain" function, I think the git diff
output will be a lot cleaner... Right now it's still a bit confusing.
That is Plain first then Params second.
I generall agree, but given this is being refactored more in later
patches, it probably isn't worth the pain of trying to rearrange it
again.
Nothing more beyond that. I'll let others contemplate the Plain
name -
since it's going away eventually shouldn't be a problem. It could have
been "Static" too...
Yeah, I think its fine given it is changing more in later patches.
Broadly speaking ACK from me, with the commit message changes John
suggests. I like this particular cleanup alot !
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
2:34 a.m.
New subject: [libvirt] [PATCH v2 01/12] migration: refactor: get rid of use_params p2p_full
On 17.09.2015 00:54, John Ferlan wrote:
FWIW: I figured I'd at least take a look - it's not my area of expertise
though. I also ran the changes through my Coverity checker. The first
pass found an issue in patch 10, which seems to be a result of some
changes in patch 2 and perhaps patch 3...
On 09/10/2015 09:20 AM, Nikolay Shirokovskiy wrote:
> 'useParams' parameter usage is an example of contol coupling. Most of the
work
s/contol/control
> inside the function is done differently for different value of this flag except
s/value of this flag//
> for the uri check. Lets split this function into 2, one with extensible
s/2/two
> parameters set and one with hardcoded parameter set. Common uri check we factor
> out in different patch for clarity.
Move this last sentence to the commit message of patch 2...
>
> Aim of this patchset is to unify logic for differet parameters representation
> so finally we merge this split back thru extensible parameters.
This paragraph could state - this patch begins a series of changes to
the Peer2Peer API's to utilize a common API with extensible parameters.
Or it could be stricken or moved to the cover letter...
>
> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
> ---
> src/libvirt-domain.c | 129 +++++++++++++++++++++----------------------------
> 1 files changed, 55 insertions(+), 74 deletions(-)
>
>
> -/*
> - * In normal migration, the libvirt client co-ordinates communication
> - * between the 2 libvirtd instances on source & dest hosts.
> - *
> - * In this peer-2-peer migration alternative, the libvirt client
> - * only talks to the source libvirtd instance. The source libvirtd
> - * then opens its own connection to the destination and co-ordinates
> - * migration itself.
> - *
> - * If useParams is true, params and nparams contain migration parameters and
> - * we know it's safe to call the API which supports extensible parameters.
> - * Otherwise, we have to use xmlin, dname, uri, and bandwidth and pass them
> - * to the old-style APIs.
> - */
Perhaps a hassle, but for now the comments could have moved with their
respective function... Perhaps the first two paragraphs for the "Plain"
function with the last "Otherwise" sentence changed slightly to indicate
an "old style" static parameter listing.
Then for the "Params" function that last paragraph preceded with a
reference to the "Plain" function... Unlike the "Plain" function,
this
function uses an extensible parameter list...
Indeed I'll better keep comments here, but as plain and params are going
to be merged back again and parameters check will be moved inside
the function i'll keep only p2p explanation part. Which will finally
merge with direct function comments too.
8:20 a.m.
New subject: [libvirt] [PATCH v2 02/12] migration: refactor: reuse p2p url check
As promised in previous patch.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
---
src/libvirt-domain.c | 43 +++++++++++++++++++++++--------------------
1 files changed, 23 insertions(+), 20 deletions(-)
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
index 1a00485..07e342f 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -3293,14 +3293,33 @@ virDomainMigrateVersion3Params(virDomainPtr domain,
}
static int
+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;
+}
+
+static int
virDomainMigratePeer2PeerParams(virDomainPtr domain,
const char *dconnuri,
virTypedParameterPtr params,
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);
@@ -3310,15 +3329,8 @@ virDomainMigratePeer2PeerParams(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);
VIR_DEBUG("Using migration protocol 3 with extensible parameters");
return domain->conn->driver->domainMigratePerform3Params
@@ -3335,8 +3347,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",
@@ -3349,15 +3359,8 @@ virDomainMigratePeer2PeerPlain(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);
if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
VIR_DRV_FEATURE_MIGRATION_V3)) {
--
1.7.1
4:58 p.m.
New subject: [libvirt] [PATCH v2 02/12] migration: refactor: reuse p2p url check
On 09/10/2015 09:20 AM, Nikolay Shirokovskiy wrote:
As promised in previous patch.
Update commit message to say what's being done from patch 1:
"Common uri check we factor out in different patch for clarity."
Or for me ;-) "Refactor dconnuri local server URI check to common API"
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
---
src/libvirt-domain.c | 43 +++++++++++++++++++++++--------------------
1 files changed, 23 insertions(+), 20 deletions(-)
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
index 1a00485..07e342f 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -3293,14 +3293,33 @@ virDomainMigrateVersion3Params(virDomainPtr domain,
}
I missed nothing this with the previous change - keep to two blank lines
between functions...
static int
+virDomainMigrateCheckNotLocal(const char *dconnuri)
+{
+ virURIPtr tempuri = NULL;
+ int ret = -1;
+
+ if (!(tempuri = virURIParse(dconnuri)))
^^ This will get us into trouble later because 'dconnuri' cannot be NULL
when being passed to virURIParse; however, by patch 10 there's a call to
virDomainMigrateUnmanaged with a NULL dconnuri "if (!(flags &
VIR_MIGRATE_PEER2PEER))"
I haven't yet followed all the future code motion, although you could
add a ATTRIBUTE_NONNULL(1) after the "static int" to be more clear even
though yes, it's perhaps not something that was in the "existing" code.
Other than spacing and missing commit message, this seems fine.
John
+ 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;
+}
+
+static int
virDomainMigratePeer2PeerParams(virDomainPtr domain,
const char *dconnuri,
virTypedParameterPtr params,
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);
@@ -3310,15 +3329,8 @@ virDomainMigratePeer2PeerParams(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);
VIR_DEBUG("Using migration protocol 3 with extensible parameters");
return domain->conn->driver->domainMigratePerform3Params
@@ -3335,8 +3347,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",
@@ -3349,15 +3359,8 @@ virDomainMigratePeer2PeerPlain(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);
if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
VIR_DRV_FEATURE_MIGRATION_V3)) {
9:03 a.m.
New subject: [libvirt] [PATCH v2 02/12] migration: refactor: reuse p2p url check
On Thu, Sep 10, 2015 at 04:20:14PM +0300, Nikolay Shirokovskiy wrote:
As promised in previous patch.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
---
src/libvirt-domain.c | 43 +++++++++++++++++++++++--------------------
1 files changed, 23 insertions(+), 20 deletions(-)
ACK with John's suggestion of ATTRIBUTE_NONNULL on the dconnuri
parameter
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
8:20 a.m.
New subject: [libvirt] [PATCH v2 03/12] migration: move implementation check to branches in p2p
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(a)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 07e342f..6f10c74 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -3353,23 +3353,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
5:24 p.m.
New subject: [libvirt] [PATCH v2 03/12] migration: move implementation check to branches in p2p
On 09/10/2015 09:20 AM, 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.
The commit message describes a bit more than what's going on... This
patch just moves the checks for driver function closer to the respective
calls...
Signed-off-by: Nikolay Shirokovskiy
<nshirokovskiy(a)virtuozzo.com>
---
src/libvirt-domain.c | 14 ++++++++------
1 files changed, 8 insertions(+), 6 deletions(-)
Seems reasonable - although it becomes interesting to note that the
following patch adds the Perform3 check to the Direct code...
Of course back to my patch 2 comment about dconnuri - here's perhaps an
assumption made at one time that the Perform & Perform3 API's will
always have a non NULL dconnuri (although I didn't see any specific
NONNULL checks).
John
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
index 07e342f..6f10c74 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -3353,23 +3353,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 "
3:03 a.m.
New subject: [libvirt] [PATCH v2 03/12] migration: move implementation check to branches in p2p
On 17.09.2015 01:24, John Ferlan wrote:
On 09/10/2015 09:20 AM, 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.
>
The commit message describes a bit more than what's going on... This
patch just moves the checks for driver function closer to the respective
calls...
Yes, I just wanted to emphasis why this patch is not marked with
pure migration subject.
9:04 a.m.
New subject: [libvirt] [PATCH v2 03/12] migration: move implementation check to branches in p2p
On Thu, Sep 10, 2015 at 04:20:15PM +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(a)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 07e342f..6f10c74 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -3353,23 +3353,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 "
ACK
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
8:20 a.m.
New subject: [libvirt] [PATCH v2 04/12] migration: remove direct migration dependency on version1 of driver
From: Michal Privoznik <mprivozn(a)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 6f10c74..15de714 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -3412,16 +3412,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 */
@@ -3437,6 +3436,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
5:40 p.m.
New subject: [libvirt] [PATCH v2 04/12] migration: remove direct migration dependency on version1 of driver
On 09/10/2015 09:20 AM, Nikolay Shirokovskiy wrote:
From: Michal Privoznik <mprivozn(a)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.
This last paragraph could go below the "---" since you're providing
"extra" information that we don't necessarily want in git history...
---
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 6f10c74..15de714 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -3412,16 +3412,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;
+ }
Ahhh... now I see where patch 10 is going... Direct has no dconnuri...
Hmmm... and I know why Coverity is complaining...
John
VIR_DEBUG("Using migration protocol 3");
/* dconn URI not relevant in direct migration, since no
* target libvirtd is involved */
@@ -3437,6 +3436,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",
9:05 a.m.
New subject: [libvirt] [PATCH v2 04/12] migration: remove direct migration dependency on version1 of driver
On Thu, Sep 10, 2015 at 04:20:16PM +0300, Nikolay Shirokovskiy wrote:
From: Michal Privoznik <mprivozn(a)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
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
8:20 a.m.
New subject: [libvirt] [PATCH v2 05/12] migration: refactor: merge direct and p2p into unmanaged
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(a)virtuozzo.com>
---
src/libvirt-domain.c | 140 ++++++++++++++------------------------------------
1 files changed, 39 insertions(+), 101 deletions(-)
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
index 15de714..1631944 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -3339,21 +3339,23 @@ virDomainMigratePeer2PeerParams(virDomainPtr domain,
}
static int
-virDomainMigratePeer2PeerPlain(virDomainPtr domain,
- const char *xmlin,
- unsigned int flags,
- const char *dname,
- const char *dconnuri,
- const char *uri,
- 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, uri=%s, bandwidth=%llu
"
+ "dconnuri=%s, xmlin=%s, dname=%s, miguri=%s, bandwidth=%llu
"
"flags=%x",
- dconnuri, NULLSTR(xmlin), NULLSTR(dname), NULLSTR(uri),
+ 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,
@@ -3365,7 +3367,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) {
@@ -3378,85 +3380,21 @@ virDomainMigratePeer2PeerPlain(virDomainPtr domain,
"migration"));
return -1;
}
- if (uri) {
- 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);
}
}
-/*
- * 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
@@ -3594,8 +3532,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;
}
@@ -3815,8 +3753,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;
}
@@ -4196,8 +4134,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 */
@@ -4208,8 +4146,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 */
@@ -4342,8 +4280,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 */
@@ -4354,8 +4292,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 */
@@ -4473,8 +4411,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",
@@ -4501,8 +4439,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
6:11 p.m.
New subject: [libvirt] [PATCH v2 05/12] migration: refactor: merge direct and p2p into unmanaged
On 09/10/2015 09:20 AM, 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(a)virtuozzo.com>
---
src/libvirt-domain.c | 140 ++++++++++++++------------------------------------
1 files changed, 39 insertions(+), 101 deletions(-)
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
index 15de714..1631944 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -3339,21 +3339,23 @@ virDomainMigratePeer2PeerParams(virDomainPtr domain,
}
static int
-virDomainMigratePeer2PeerPlain(virDomainPtr domain,
- const char *xmlin,
- unsigned int flags,
- const char *dname,
- const char *dconnuri,
- const char *uri,
- 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)
Perhaps should have just used that "Unmanaged" name all along as this
patch has two things going on - renaming a function and merging another
into it. Perhaps even with the miguri parameter change too!
I know already present but thumbs down to modules that are felt to be
self commenting - especially the *uri params ;-)
{
+ const char *uri = NULL;
+
VIR_DOMAIN_DEBUG(domain,
- "dconnuri=%s, xmlin=%s, dname=%s, uri=%s, bandwidth=%llu
"
+ "dconnuri=%s, xmlin=%s, dname=%s, miguri=%s, bandwidth=%llu
"
"flags=%x",
- dconnuri, NULLSTR(xmlin), NULLSTR(dname), NULLSTR(uri),
+ dconnuri, NULLSTR(xmlin), NULLSTR(dname), NULLSTR(miguri),
bandwidth, flags);
- if (virDomainMigrateCheckNotLocal(dconnuri) < 0)
+ if ((flags & VIR_MIGRATE_PEER2PEER) &&
virDomainMigrateCheckNotLocal(dconnuri) < 0)
Ahh... now I see... The "flags" check here is for only make this call on
VIR_MIGRATE_PEER2PEER; however, in patch 10, the dconnuri is cleared if
!(flags & VIR_MIGRATE_PEER2PEER) which I'm going to assume confuses
Coverity since flags can be "many" things. I don't have any smart ideas
how to resolve this yet, but at least it's beginning to make sense.
This API assumes only be the Peer2Peer or Direct - Peer2Peer is a
'flags' bit and Direct is a VIR_DRV_FEATURE_MIGRATION_DIRECT driver
feature. We trust (ahem) our caller to do the right thing and call us
the right way.
return -1;
if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
@@ -3365,7 +3367,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) {
@@ -3378,85 +3380,21 @@ virDomainMigratePeer2PeerPlain(virDomainPtr domain,
"migration"));
return -1;
}
- if (uri) {
- 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);
}
}
-/*
- * 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
- */
Perhaps this comment can be lifted above and your clairvoyance to know
what was supposed to come after "In this case" can shine through...
And by lifted I mean - let's use it as a basis to comment the Unmanaged
API indicating two (currently) potential uses... If 'flags' has
Peer2Peer, then dconnuri must be set; otherwise, for direct there is no
dconnuri... etc.. Take what you've learned and leave a few bread crumbs
for those that follow you down this rabbit hole! (Although there are
others that perhaps disagree with "over commenting").
In any case - beyond the couple of nits, it seems from my reading this
was a good merge of two functions.
John
FYI: I have to stop for the evening - I smell dinner cooking... I'll
pick this back up tomorrow
-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
@@ -3594,8 +3532,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;
}
@@ -3815,8 +3753,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;
}
@@ -4196,8 +4134,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 */
@@ -4208,8 +4146,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 */
@@ -4342,8 +4280,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 */
@@ -4354,8 +4292,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 */
@@ -4473,8 +4411,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",
@@ -4501,8 +4439,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;
}
3:51 a.m.
New subject: [libvirt] [PATCH v2 05/12] migration: refactor: merge direct and p2p into unmanaged
On 17.09.2015 02:11, John Ferlan wrote:
On 09/10/2015 09:20 AM, 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(a)virtuozzo.com>
> ---
> src/libvirt-domain.c | 140 ++++++++++++++------------------------------------
> 1 files changed, 39 insertions(+), 101 deletions(-)
>
> diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
> index 15de714..1631944 100644
> --- a/src/libvirt-domain.c
> +++ b/src/libvirt-domain.c
> @@ -3339,21 +3339,23 @@ virDomainMigratePeer2PeerParams(virDomainPtr domain,
> }
>
> static int
> -virDomainMigratePeer2PeerPlain(virDomainPtr domain,
> - const char *xmlin,
> - unsigned int flags,
> - const char *dname,
> - const char *dconnuri,
> - const char *uri,
> - 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)
Perhaps should have just used that "Unmanaged" name all along as this
patch has two things going on - renaming a function and merging another
into it. Perhaps even with the miguri parameter change too!
I guess you mean use unmanaged instead p2p plain, but wouldn't it be just
moving second thing from here to erlier patches staying again with two
things. I could move this rename to a different patch but it would
be too simple i guess. And what is more important, we here merge 2 functions
so the name should be "merged" too, ie should be made more abstract.
As to miguri, i'll move this into a different patch.
6:13 p.m.
New subject: [libvirt] [PATCH v2 05/12] migration: refactor: merge direct and p2p into unmanaged
Of course I sent it too quick - one extra note...
[ ... snip... ]
/**
* virDomainMigrate:
* @domain: a domain object
@@ -3594,8 +3532,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;
}
@@ -3815,8 +3753,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;
}
@@ -4196,8 +4134,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 */
@@ -4208,8 +4146,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)
Need extra spacing ^^^
Same for other Direct to Unmanaged changes...
goto error;
} else {
/* Cannot do a migration with only the perform step */
@@ -4342,8 +4280,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 */
@@ -4354,8 +4292,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)
Here
goto error;
} else {
/* Cannot do a migration with only the perform step */
@@ -4473,8 +4411,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",
@@ -4501,8 +4439,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)
Here
goto error;
}
9:13 a.m.
New subject: [libvirt] [PATCH v2 05/12] migration: refactor: merge direct and p2p into unmanaged
On Thu, Sep 10, 2015 at 04:20:17PM +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(a)virtuozzo.com>
---
src/libvirt-domain.c | 140 ++++++++++++++------------------------------------
1 files changed, 39 insertions(+), 101 deletions(-)
Took a while to understand the URI passing, but I think it looks sane, so ACK
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
8:20 a.m.
New subject: [libvirt] [PATCH v2 06/12] migration: refactor: introduce params version of unmanaged
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(a)virtuozzo.com>
---
src/libvirt-domain.c | 90 ++++++++++++++++++++++++++++++++-----------------
1 files changed, 59 insertions(+), 31 deletions(-)
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
index 1631944..79ed9f8 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -3339,21 +3339,32 @@ 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)
+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, miguri=%s, bandwidth=%llu
"
- "flags=%x",
- dconnuri, NULLSTR(xmlin), NULLSTR(dname), NULLSTR(miguri),
- bandwidth, flags);
+ 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;
+ }
+
+ VIR_DOMAIN_DEBUG(domain, "dconnuri=%s, params=%p, nparams=%d, flags=%x",
+ dconnuri, params, nparams, flags);
+ VIR_TYPED_PARAMS_DEBUG(params, nparams);
if ((flags & VIR_MIGRATE_PEER2PEER) &&
virDomainMigrateCheckNotLocal(dconnuri) < 0)
return -1;
@@ -3395,6 +3406,38 @@ virDomainMigrateUnmanaged(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
@@ -4361,10 +4404,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);
@@ -4383,17 +4422,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)) {
@@ -4411,8 +4439,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",
@@ -4439,8 +4467,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
7:42 a.m.
New subject: [libvirt] [PATCH v2 06/12] migration: refactor: introduce params version of unmanaged
On 09/10/2015 09:20 AM, 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(a)virtuozzo.com>
---
src/libvirt-domain.c | 90 ++++++++++++++++++++++++++++++++-----------------
1 files changed, 59 insertions(+), 31 deletions(-)
This patch introduces virDomainMigrateUnmanagedParams and makes
virDomainMigrateUnmanaged a shim to make the call.
Things seem reasonable here w/r/t code motion. Here Coverity is "OK"
with the NULL in dconnuri because it's directly tied to the same "if
(flags & VIR_MIGRATE_PEER2PEER)" check.
John
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
index 1631944..79ed9f8 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -3339,21 +3339,32 @@ 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)
+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, miguri=%s, bandwidth=%llu
"
- "flags=%x",
- dconnuri, NULLSTR(xmlin), NULLSTR(dname), NULLSTR(miguri),
- bandwidth, flags);
+ 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;
+ }
+
+ VIR_DOMAIN_DEBUG(domain, "dconnuri=%s, params=%p, nparams=%d, flags=%x",
+ dconnuri, params, nparams, flags);
+ VIR_TYPED_PARAMS_DEBUG(params, nparams);
if ((flags & VIR_MIGRATE_PEER2PEER) &&
virDomainMigrateCheckNotLocal(dconnuri) < 0)
return -1;
@@ -3395,6 +3406,38 @@ virDomainMigrateUnmanaged(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
@@ -4361,10 +4404,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);
@@ -4383,17 +4422,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)) {
@@ -4411,8 +4439,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",
@@ -4439,8 +4467,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;
}
9:33 a.m.
New subject: [libvirt] [PATCH v2 06/12] migration: refactor: introduce params version of unmanaged
On Thu, Sep 10, 2015 at 04:20:18PM +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(a)virtuozzo.com>
---
src/libvirt-domain.c | 90 ++++++++++++++++++++++++++++++++-----------------
1 files changed, 59 insertions(+), 31 deletions(-)
ACK
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
8:20 a.m.
New subject: [libvirt] [PATCH v2 07/12] migration: refactor: extract parameter adaption functions
Extract parametes adapdation and checking which is protocol dependent into
designated functions. Leave only branching and common checks in
virDomainMigrateUnmanagedParams.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
---
src/libvirt-domain.c | 84 +++++++++++++++++++++++++++++++++++++-------------
1 files changed, 62 insertions(+), 22 deletions(-)
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
index 79ed9f8..55efd49 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -3339,7 +3339,7 @@ virDomainMigratePeer2PeerParams(virDomainPtr domain,
}
static int
-virDomainMigrateUnmanagedParams(virDomainPtr domain,
+virDomainMigrateUnmanagedProto2(virDomainPtr domain,
const char *dconnuri,
virTypedParameterPtr params,
int nparams,
@@ -3362,6 +3362,63 @@ virDomainMigrateUnmanagedParams(virDomainPtr domain,
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);
+}
+
+static int
+virDomainMigrateUnmanagedParams(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);
@@ -3376,33 +3433,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
8:10 a.m.
New subject: [libvirt] [PATCH v2 07/12] migration: refactor: extract parameter adaption functions
On 09/10/2015 09:20 AM, Nikolay Shirokovskiy wrote:
Extract parametes adapdation and checking which is protocol dependent
into
s/parametes adapdation/parameter adaptation
designated functions. Leave only branching and common checks in
virDomainMigrateUnmanagedParams.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
---
src/libvirt-domain.c | 84 +++++++++++++++++++++++++++++++++++++-------------
1 files changed, 62 insertions(+), 22 deletions(-)
More code motion which seems OK to me, although the "Proto" could be
removed from function names for just Unmanaged3 and Unmanaged2, but I
suppose that name will be changing soon too so it's perhaps moot point.
John
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
index 79ed9f8..55efd49 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -3339,7 +3339,7 @@ virDomainMigratePeer2PeerParams(virDomainPtr domain,
}
static int
-virDomainMigrateUnmanagedParams(virDomainPtr domain,
+virDomainMigrateUnmanagedProto2(virDomainPtr domain,
const char *dconnuri,
virTypedParameterPtr params,
int nparams,
@@ -3362,6 +3362,63 @@ virDomainMigrateUnmanagedParams(virDomainPtr domain,
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);
+}
+
+static int
+virDomainMigrateUnmanagedParams(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);
@@ -3376,33 +3433,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);
}
}
9:34 a.m.
New subject: [libvirt] [PATCH v2 07/12] migration: refactor: extract parameter adaption functions
On Thu, Sep 10, 2015 at 04:20:19PM +0300, Nikolay Shirokovskiy wrote:
Extract parametes adapdation and checking which is protocol dependent
into
designated functions. Leave only branching and common checks in
virDomainMigrateUnmanagedParams.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
---
src/libvirt-domain.c | 84 +++++++++++++++++++++++++++++++++++++-------------
1 files changed, 62 insertions(+), 22 deletions(-)
ACK
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
8:20 a.m.
New subject: [libvirt] [PATCH v2 08/12] migration: refactor: gather parameters compatibility checks
Checks for migration's parameter set and support by protocol are slightly
scattered in code. Let's put it in one place, namely every protocol function
should check it's parameter set.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)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 55efd49..8b4171e 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -3345,30 +3345,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",
@@ -3391,11 +3392,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,
@@ -4439,12 +4451,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);
@@ -4459,9 +4465,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)) {
@@ -4477,17 +4480,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,
@@ -4499,13 +4496,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
9:16 a.m.
New subject: [libvirt] [PATCH v2 08/12] migration: refactor: gather parameters compatibility checks
$subj: "migration: refactor parameter compatibility checks
On 09/10/2015 09:20 AM, Nikolay Shirokovskiy wrote:
Checks for migration's parameter set and support by protocol are
slightly
s/migration's parameter set/migration parameter sets
scattered in code. Let's put it in one place, namely every
protocol function
should check it's parameter set.
Perhaps more simply said - "Use the common virTypedParamsCheck for each
virDomainMigrate* function to check its expected params list prior to
the API making the driver call. Thi"
Signed-off-by: Nikolay Shirokovskiy
<nshirokovskiy(a)virtuozzo.com>
---
src/libvirt-domain.c | 56 ++++++++++++++++++++-----------------------------
1 files changed, 23 insertions(+), 33 deletions(-)
I think the "one" (perhaps naive) mental note I make is that you add a
virTypedParamsCheck to virDomainMigrateUnmanagedProto3; however, it
seems it's unnecessary for virDomainMigratePeer2PeerParams... Yes, I see
one calls Perform3 and one calls Perform3Params
Still seems sane to me
John
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
index 55efd49..8b4171e 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -3345,30 +3345,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",
@@ -3391,11 +3392,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,
@@ -4439,12 +4451,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);
@@ -4459,9 +4465,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)) {
@@ -4477,17 +4480,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,
@@ -4499,13 +4496,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)
9:34 a.m.
New subject: [libvirt] [PATCH v2 08/12] migration: refactor: gather parameters compatibility checks
On Thu, Sep 10, 2015 at 04:20:20PM +0300, Nikolay Shirokovskiy wrote:
Checks for migration's parameter set and support by protocol are
slightly
scattered in code. Let's put it in one place, namely every protocol function
should check it's parameter set.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
---
src/libvirt-domain.c | 56 ++++++++++++++++++++-----------------------------
1 files changed, 23 insertions(+), 33 deletions(-)
ACK
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
8:20 a.m.
New subject: [libvirt] [PATCH v2 09/12] migration: merge all proto branches into single function
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(a)virtuozzo.com>
---
src/libvirt-domain.c | 53 ++++++++++++++-----------------------------------
1 files changed, 15 insertions(+), 38 deletions(-)
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
index 8b4171e..f7d0777 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -3314,31 +3314,6 @@ virDomainMigrateCheckNotLocal(const char *dconnuri)
}
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
virDomainMigrateUnmanagedProto2(virDomainPtr domain,
const char *dconnuri,
virTypedParameterPtr params,
@@ -3438,7 +3413,18 @@ virDomainMigrateUnmanagedParams(virDomainPtr domain,
if ((flags & VIR_MIGRATE_PEER2PEER) &&
virDomainMigrateCheckNotLocal(dconnuri) < 0)
return -1;
- if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
+ 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) {
@@ -4474,18 +4460,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
9:32 a.m.
New subject: [libvirt] [PATCH v2 09/12] migration: merge all proto branches into single function
On 09/10/2015 09:20 AM, 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(a)virtuozzo.com>
---
src/libvirt-domain.c | 53 ++++++++++++++-----------------------------------
1 files changed, 15 insertions(+), 38 deletions(-)
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
index 8b4171e..f7d0777 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -3314,31 +3314,6 @@ virDomainMigrateCheckNotLocal(const char *dconnuri)
}
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
virDomainMigrateUnmanagedProto2(virDomainPtr domain,
const char *dconnuri,
virTypedParameterPtr params,
@@ -3438,7 +3413,18 @@ virDomainMigrateUnmanagedParams(virDomainPtr domain,
if ((flags & VIR_MIGRATE_PEER2PEER) &&
virDomainMigrateCheckNotLocal(dconnuri) < 0)
return -1;
^^^ Note: Should have noted this in patch 5....
The && should be spread across 2 lines since the line is longer than 80
cols... It's a visual thing... There perhaps are more, this one now
stands out though.
- if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
+ 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)) {
It almost seems things could be written as:
if (flags & VIR_MIGRATE_PEER2PEER) {
if (virDomainMigrateCheckNotLocal(dconnuri) < 0)
....
if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
....)
else if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver,
...
else
something's wrong
} else {
VIR_DEBUG("Using migration protocol 2");
...
}
IOW: I'm trying to isolate the dconnuri a bit more. Up to this point it
seems the assumption calling this routine was that if we support
VIR_DRV_FEATURE_MIGRATION_V3, then this would only be called if
VIR_MIGRATE_PEER2PEER was true, correct?
Not that there's anything wrong with the method you chose, I am trying
to clarify in my mind!
It seems that the code motion went well otherwise.
John
VIR_DEBUG("Using migration protocol 3");
if (!domain->conn->driver->domainMigratePerform3) {
@@ -4474,18 +4460,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)) {
5:12 a.m.
New subject: [libvirt] [PATCH v2 09/12] migration: merge all proto branches into single function
On 17.09.2015 17:32, John Ferlan wrote:
On 09/10/2015 09:20 AM, 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(a)virtuozzo.com>
> ---
> src/libvirt-domain.c | 53 ++++++++++++++-----------------------------------
> 1 files changed, 15 insertions(+), 38 deletions(-)
>
> diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
>
> - if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
> + 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)) {
It almost seems things could be written as:
if (flags & VIR_MIGRATE_PEER2PEER) {
if (virDomainMigrateCheckNotLocal(dconnuri) < 0)
....
if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
....)
else if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver,
...
else
something's wrong
} else {
VIR_DEBUG("Using migration protocol 2");
...
}
IOW: I'm trying to isolate the dconnuri a bit more. Up to this point it
seems the assumption calling this routine was that if we support
VIR_DRV_FEATURE_MIGRATION_V3, then this would only be called if
VIR_MIGRATE_PEER2PEER was true, correct?
I can't agree. This way we get a code dup as direct and p2p switch
for protocol version are no different except for v3_params. And even
this little difference could get away in the future as the reason to
keep it is unclear.
We'd better check that dconnuri is not NULL for p2p somewhere earlier
to make things consistent. In different patch i guess.
9:35 a.m.
New subject: [libvirt] [PATCH v2 09/12] migration: merge all proto branches into single function
On Thu, Sep 10, 2015 at 04:20:21PM +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.
Yeah, that is puzzeling - when reviewing this i wondered why our
current code doesn't support it..
Signed-off-by: Nikolay Shirokovskiy
<nshirokovskiy(a)virtuozzo.com>
---
src/libvirt-domain.c | 53 ++++++++++++++-----------------------------------
1 files changed, 15 insertions(+), 38 deletions(-)
ACK
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
8:20 a.m.
New subject: [libvirt] [PATCH v2 10/12] migration: refactor: prepare to reuse flag vs feature checks
All toURI functions have same checks for flags and features compatibility for
direct and p2p case. Let's factor this checks out before we reuse common code
in toURI functions family. As a side affect we have a more clear code
representation of uri passing conventions for p2p and direct cases.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
---
src/libvirt-domain.c | 126 +++++++++++++++++++------------------------------
1 files changed, 49 insertions(+), 77 deletions(-)
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
index f7d0777..483537a 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -4187,6 +4187,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);
@@ -4211,34 +4214,25 @@ virDomainMigrateToURI(virDomainPtr domain,
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) &&
+ !VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
+ VIR_DRV_FEATURE_MIGRATION_P2P)) ||
+ (!(flags & VIR_MIGRATE_PEER2PEER) &&
+ !VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
+ VIR_DRV_FEATURE_MIGRATION_DIRECT))) {
+ virReportUnsupportedError();
+ 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;
error:
@@ -4357,34 +4351,23 @@ virDomainMigrateToURI2(virDomainPtr domain,
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)) {
- 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) &&
+ !VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
+ VIR_DRV_FEATURE_MIGRATION_P2P)) ||
+ (!(flags & VIR_MIGRATE_PEER2PEER) &&
+ !VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
+ VIR_DRV_FEATURE_MIGRATION_DIRECT))) {
+ virReportUnsupportedError();
+ goto error;
}
+ if (!(flags & VIR_MIGRATE_PEER2PEER))
+ dconnuri = NULL;
+
+ if (virDomainMigrateUnmanaged(domain, NULL, flags,
+ dname, dconnuri, miguri, bandwidth) < 0)
+ goto error;
+
return 0;
error:
@@ -4451,33 +4434,22 @@ virDomainMigrateToURI3(virDomainPtr domain,
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 (((flags & VIR_MIGRATE_PEER2PEER) &&
+ !VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
+ VIR_DRV_FEATURE_MIGRATION_P2P)) ||
+ (!(flags & VIR_MIGRATE_PEER2PEER) &&
+ !VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
+ VIR_DRV_FEATURE_MIGRATION_DIRECT))) {
+ virReportUnsupportedError();
+ 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) < 0)
+ goto error;
return 0;
--
1.7.1
9:36 a.m.
New subject: [libvirt] [PATCH v2 10/12] migration: refactor: prepare to reuse flag vs feature checks
On Thu, Sep 10, 2015 at 04:20:22PM +0300, Nikolay Shirokovskiy wrote:
All toURI functions have same checks for flags and features
compatibility for
direct and p2p case. Let's factor this checks out before we reuse common code
in toURI functions family. As a side affect we have a more clear code
representation of uri passing conventions for p2p and direct cases.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
---
src/libvirt-domain.c | 126 +++++++++++++++++++------------------------------
1 files changed, 49 insertions(+), 77 deletions(-)
ACK
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
9:56 a.m.
New subject: [libvirt] [PATCH v2 10/12] migration: refactor: prepare to reuse flag vs feature checks
On 09/10/2015 09:20 AM, Nikolay Shirokovskiy wrote:
All toURI functions have same checks for flags and features
compatibility for
direct and p2p case. Let's factor this checks out before we reuse common code
s/this/the
in toURI functions family. As a side affect we have a more clear
code
representation of uri passing conventions for p2p and direct cases.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
---
src/libvirt-domain.c | 126 +++++++++++++++++++------------------------------
1 files changed, 49 insertions(+), 77 deletions(-)
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
index f7d0777..483537a 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -4187,6 +4187,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);
@@ -4211,34 +4214,25 @@ virDomainMigrateToURI(virDomainPtr domain,
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) &&
+ !VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
+ VIR_DRV_FEATURE_MIGRATION_P2P)) ||
^^^^
Not well aligned and shall I say a very "difficult" to read construct -
multiple ! and || with a couple of &&'s for good measure!
Can there be a
bool flg_p2p = (flags & VIR_MIGRATE_PEER2PEER);
bool drv_p2p = VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver,
domain->conn,
VIR_DRV_FEATURE_MIGRATION_P2P);
bool dvr_direct = VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver,
domain->conn,
VIR_DRV_FEATURE_MIGRATION_DIRECT);
const char *dconnuri = flg_p2p ? duri : NULL;
const char *miguri = !flg_p2p ? duri : NULL;
Then:
if ((flg_p2p && !drv_p2p) || (!flg_p2p && !drv_direct)) {
virReportUnsupportedError()
+ (!(flags & VIR_MIGRATE_PEER2PEER) &&
+ !VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
+ VIR_DRV_FEATURE_MIGRATION_DIRECT))) {
+ virReportUnsupportedError();
+ goto error;
}
+ if (flags & VIR_MIGRATE_PEER2PEER)
+ dconnuri = duri;
+ else
+ miguri = duri;
+
+ if (virDomainMigrateUnmanaged(domain, NULL, flags,
+ dname, dconnuri, miguri, bandwidth) < 0)
^^
Need to move the args over...
+ goto error;
+
return 0;
error:
@@ -4357,34 +4351,23 @@ :q
(virDomainPtr domain,
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)) {
- 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) &&
+ !VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
+ VIR_DRV_FEATURE_MIGRATION_P2P)) ||
+ (!(flags & VIR_MIGRATE_PEER2PEER) &&
+ !VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
+ VIR_DRV_FEATURE_MIGRATION_DIRECT))) {
+ virReportUnsupportedError();
+ goto error;
}
Similar difficult to read construct here...
+ if (!(flags & VIR_MIGRATE_PEER2PEER))
+ dconnuri = NULL;
+
+ if (virDomainMigrateUnmanaged(domain, NULL, flags,
+ dname, dconnuri, miguri, bandwidth) < 0)
^^^
Spacing here...
This is where Coverity complains because 'dconnuri' being NULL
eventually triggers a rule violation because a call to virURIParse()
expects a NONNULL parameter. This is called via
virDomainMigrateCheckNotLocal
I'm still not sure of the best way to handle this, but may using using
local will suffice... That is after the flags, there's a
const char *duri = flg_p2p ? dconnuri : NULL;
and make the make the Unmanaged call with duri instead of dconnuri
Ugh... I just peeked ahead.... seems to mess up future changes... <sigh>
John
+ goto error;
+
return 0;
error:
@@ -4451,33 +4434,22 @@ virDomainMigrateToURI3(virDomainPtr domain,
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 (((flags & VIR_MIGRATE_PEER2PEER) &&
+ !VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
+ VIR_DRV_FEATURE_MIGRATION_P2P)) ||
+ (!(flags & VIR_MIGRATE_PEER2PEER) &&
+ !VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
+ VIR_DRV_FEATURE_MIGRATION_DIRECT))) {
+ virReportUnsupportedError();
+ 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) < 0)
+ goto error;
return 0;
8:20 a.m.
New subject: [libvirt] [PATCH v2 11/12] migration: reuse flags vs features checks in toURI family
Introduce a new function for the check. 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 consitensy of flags parameter alone. So it could be refactored further and
reused by all migrate functions bu for now let it be a matter of a different
patchset.
It is *not* a pure refactoring patch as it introduces offline check for all
versions. Looks like it must be done that way and no one will be broken too.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
---
src/libvirt-domain.c | 81 +++++++++++++++++++------------------------------
1 files changed, 32 insertions(+), 49 deletions(-)
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
index 483537a..2d98997 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -4108,6 +4108,35 @@ virDomainMigrate3(virDomainPtr domain,
return NULL;
}
+static
+int virDomainMigrateUnmanagedCheckCompat(virDomainPtr domain, unsigned int flags)
+{
+ VIR_EXCLUSIVE_FLAGS_RET(VIR_MIGRATE_NON_SHARED_DISK,
+ VIR_MIGRATE_NON_SHARED_INC,
+ -1);
+
+ if (flags & VIR_MIGRATE_OFFLINE &&
+ !VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
+ VIR_DRV_FEATURE_MIGRATION_OFFLINE)) {
+ virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
+ _("offline migration is not supported by "
+ "the source host"));
+ return -1;
+ }
+
+ if (((flags & VIR_MIGRATE_PEER2PEER) &&
+ !VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
+ VIR_DRV_FEATURE_MIGRATION_P2P)) ||
+ (!(flags & VIR_MIGRATE_PEER2PEER) &&
+ !VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
+ VIR_DRV_FEATURE_MIGRATION_DIRECT))) {
+ virReportUnsupportedError();
+ return -1;
+ }
+
+ return 0;
+}
+
/**
* virDomainMigrateToURI:
@@ -4195,34 +4224,12 @@ virDomainMigrateToURI(virDomainPtr domain,
virResetLastError();
- /* First checkout the source */
virCheckDomainReturn(domain, -1);
virCheckReadOnlyGoto(domain->conn->flags, error);
-
virCheckNonNullArgGoto(duri, error);
- VIR_EXCLUSIVE_FLAGS_GOTO(VIR_MIGRATE_NON_SHARED_DISK,
- VIR_MIGRATE_NON_SHARED_INC,
- error);
-
- if (flags & VIR_MIGRATE_OFFLINE &&
- !VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
- VIR_DRV_FEATURE_MIGRATION_OFFLINE)) {
- virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
- _("offline migration is not supported by "
- "the source host"));
- goto error;
- }
-
- if (((flags & VIR_MIGRATE_PEER2PEER) &&
- !VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
- VIR_DRV_FEATURE_MIGRATION_P2P)) ||
- (!(flags & VIR_MIGRATE_PEER2PEER) &&
- !VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
- VIR_DRV_FEATURE_MIGRATION_DIRECT))) {
- virReportUnsupportedError();
+ if (virDomainMigrateUnmanagedCheckCompat(domain, flags) < 0)
goto error;
- }
if (flags & VIR_MIGRATE_PEER2PEER)
dconnuri = duri;
@@ -4343,23 +4350,11 @@ virDomainMigrateToURI2(virDomainPtr domain,
virResetLastError();
- /* First checkout the source */
virCheckDomainReturn(domain, -1);
virCheckReadOnlyGoto(domain->conn->flags, error);
- VIR_EXCLUSIVE_FLAGS_GOTO(VIR_MIGRATE_NON_SHARED_DISK,
- VIR_MIGRATE_NON_SHARED_INC,
- error);
-
- if (((flags & VIR_MIGRATE_PEER2PEER) &&
- !VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
- VIR_DRV_FEATURE_MIGRATION_P2P)) ||
- (!(flags & VIR_MIGRATE_PEER2PEER) &&
- !VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
- VIR_DRV_FEATURE_MIGRATION_DIRECT))) {
- virReportUnsupportedError();
+ if (virDomainMigrateUnmanagedCheckCompat(domain, flags) < 0)
goto error;
- }
if (!(flags & VIR_MIGRATE_PEER2PEER))
dconnuri = NULL;
@@ -4426,23 +4421,11 @@ virDomainMigrateToURI3(virDomainPtr domain,
virResetLastError();
- /* First checkout the source */
virCheckDomainReturn(domain, -1);
virCheckReadOnlyGoto(domain->conn->flags, error);
- VIR_EXCLUSIVE_FLAGS_GOTO(VIR_MIGRATE_NON_SHARED_DISK,
- VIR_MIGRATE_NON_SHARED_INC,
- error);
-
- if (((flags & VIR_MIGRATE_PEER2PEER) &&
- !VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
- VIR_DRV_FEATURE_MIGRATION_P2P)) ||
- (!(flags & VIR_MIGRATE_PEER2PEER) &&
- !VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
- VIR_DRV_FEATURE_MIGRATION_DIRECT))) {
- virReportUnsupportedError();
+ if (virDomainMigrateUnmanagedCheckCompat(domain, flags) < 0)
goto error;
- }
if (!(flags & VIR_MIGRATE_PEER2PEER))
dconnuri = NULL;
--
1.7.1
9:37 a.m.
New subject: [libvirt] [PATCH v2 11/12] migration: reuse flags vs features checks in toURI family
On Thu, Sep 10, 2015 at 04:20:23PM +0300, Nikolay Shirokovskiy wrote:
Introduce a new function for the check.
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 consitensy of flags parameter alone. So it could be refactored further and
reused by all migrate functions bu for now let it be a matter of a different
patchset.
It is *not* a pure refactoring patch as it introduces offline check for all
versions. Looks like it must be done that way and no one will be broken too.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
---
src/libvirt-domain.c | 81 +++++++++++++++++++------------------------------
1 files changed, 32 insertions(+), 49 deletions(-)
ACK
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
10:07 a.m.
New subject: [libvirt] [PATCH v2 11/12] migration: reuse flags vs features checks in toURI family
On 09/10/2015 09:20 AM, Nikolay Shirokovskiy wrote:
Introduce a new function for the check.
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 consitensy of flags parameter alone. So it could be refactored further and
s/consitensy/consistency
reused by all migrate functions bu for now let it be a matter of a
different
patchset.
It is *not* a pure refactoring patch as it introduces offline check for all
versions. Looks like it must be done that way and no one will be broken too.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
---
src/libvirt-domain.c | 81 +++++++++++++++++++------------------------------
1 files changed, 32 insertions(+), 49 deletions(-)
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
index 483537a..2d98997 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -4108,6 +4108,35 @@ virDomainMigrate3(virDomainPtr domain,
return NULL;
}
+static
+int virDomainMigrateUnmanagedCheckCompat(virDomainPtr domain, unsigned int flags)
Should be:
static int
virDomainMigrateUnmanagedCheckCompat(virDomainPtr domain,
unsigned int flags)
The "unsigned int flags" should be a separate line
Be sure to keep 2 blank lines between functions...
Also I now see where you lifted the changes in patch 10 from. Makes me
wonder if the two should change slightly such that patch 10 becomes
create the new API with the only caller virDomainMigrateToURI. Then
patch 11 becomes, modify virDomainMigrateToURI2 && URI3 to call it
rather than compressing the checks.
BTW: I still think the local duri in current patch 10 will do enough to
make it so Coverity doesn't whine.
+{
+ 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) &&
+ !VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
+ VIR_DRV_FEATURE_MIGRATION_P2P)) ||
+ (!(flags & VIR_MIGRATE_PEER2PEER) &&
+ !VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
+ VIR_DRV_FEATURE_MIGRATION_DIRECT))) {
+ virReportUnsupportedError();
+ return -1;
+ }
+
+ return 0;
+}
+
/**
* virDomainMigrateToURI:
@@ -4195,34 +4224,12 @@ virDomainMigrateToURI(virDomainPtr domain,
virResetLastError();
- /* First checkout the source */
any particular reason this line gets delete in each of the 3 functions?
John
virCheckDomainReturn(domain, -1);
virCheckReadOnlyGoto(domain->conn->flags, error);
-
virCheckNonNullArgGoto(duri, error);
- VIR_EXCLUSIVE_FLAGS_GOTO(VIR_MIGRATE_NON_SHARED_DISK,
- VIR_MIGRATE_NON_SHARED_INC,
- error);
-
- if (flags & VIR_MIGRATE_OFFLINE &&
- !VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
- VIR_DRV_FEATURE_MIGRATION_OFFLINE)) {
- virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
- _("offline migration is not supported by "
- "the source host"));
- goto error;
- }
-
- if (((flags & VIR_MIGRATE_PEER2PEER) &&
- !VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
- VIR_DRV_FEATURE_MIGRATION_P2P)) ||
- (!(flags & VIR_MIGRATE_PEER2PEER) &&
- !VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
- VIR_DRV_FEATURE_MIGRATION_DIRECT))) {
- virReportUnsupportedError();
+ if (virDomainMigrateUnmanagedCheckCompat(domain, flags) < 0)
goto error;
- }
if (flags & VIR_MIGRATE_PEER2PEER)
dconnuri = duri;
@@ -4343,23 +4350,11 @@ virDomainMigrateToURI2(virDomainPtr domain,
virResetLastError();
- /* First checkout the source */
virCheckDomainReturn(domain, -1);
virCheckReadOnlyGoto(domain->conn->flags, error);
- VIR_EXCLUSIVE_FLAGS_GOTO(VIR_MIGRATE_NON_SHARED_DISK,
- VIR_MIGRATE_NON_SHARED_INC,
- error);
-
- if (((flags & VIR_MIGRATE_PEER2PEER) &&
- !VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
- VIR_DRV_FEATURE_MIGRATION_P2P)) ||
- (!(flags & VIR_MIGRATE_PEER2PEER) &&
- !VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
- VIR_DRV_FEATURE_MIGRATION_DIRECT))) {
- virReportUnsupportedError();
+ if (virDomainMigrateUnmanagedCheckCompat(domain, flags) < 0)
goto error;
- }
if (!(flags & VIR_MIGRATE_PEER2PEER))
dconnuri = NULL;
@@ -4426,23 +4421,11 @@ virDomainMigrateToURI3(virDomainPtr domain,
virResetLastError();
- /* First checkout the source */
virCheckDomainReturn(domain, -1);
virCheckReadOnlyGoto(domain->conn->flags, error);
- VIR_EXCLUSIVE_FLAGS_GOTO(VIR_MIGRATE_NON_SHARED_DISK,
- VIR_MIGRATE_NON_SHARED_INC,
- error);
-
- if (((flags & VIR_MIGRATE_PEER2PEER) &&
- !VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
- VIR_DRV_FEATURE_MIGRATION_P2P)) ||
- (!(flags & VIR_MIGRATE_PEER2PEER) &&
- !VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
- VIR_DRV_FEATURE_MIGRATION_DIRECT))) {
- virReportUnsupportedError();
+ if (virDomainMigrateUnmanagedCheckCompat(domain, flags) < 0)
goto error;
- }
if (!(flags & VIR_MIGRATE_PEER2PEER))
dconnuri = NULL;
8:03 a.m.
New subject: [libvirt] [PATCH v2 11/12] migration: reuse flags vs features checks in toURI family
On 17.09.2015 18:07, John Ferlan wrote:
On 09/10/2015 09:20 AM, Nikolay Shirokovskiy wrote:
> Introduce a new function for the check. 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 consitensy of flags parameter alone. So it could be refactored further and
s/consitensy/consistency
> reused by all migrate functions bu for now let it be a matter of a different
> patchset.
>
> It is *not* a pure refactoring patch as it introduces offline check for all
> versions. Looks like it must be done that way and no one will be broken too.
>
> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
> ---
> src/libvirt-domain.c | 81 +++++++++++++++++++------------------------------
> 1 files changed, 32 insertions(+), 49 deletions(-)
>
> diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
> index 483537a..2d98997 100644
> --- a/src/libvirt-domain.c
> +++ b/src/libvirt-domain.c
> @@ -4108,6 +4108,35 @@ virDomainMigrate3(virDomainPtr domain,
> return NULL;
> }
>
> +static
> +int virDomainMigrateUnmanagedCheckCompat(virDomainPtr domain, unsigned int flags)
Should be:
static int
virDomainMigrateUnmanagedCheckCompat(virDomainPtr domain,
unsigned int flags)
The "unsigned int flags" should be a separate line
Be sure to keep 2 blank lines between functions...
Also I now see where you lifted the changes in patch 10 from. Makes me
wonder if the two should change slightly such that patch 10 becomes
create the new API with the only caller virDomainMigrateToURI. Then
patch 11 becomes, modify virDomainMigrateToURI2 && URI3 to call it
rather than compressing the checks.
Good suggestion! will re-refactor)
8:20 a.m.
New subject: [libvirt] [PATCH v2 12/12] migration: refactor: one return in forURI family functions
May be a matter of a taste but this version with one return point in every
function looks simplier to understand and to exetend too. Anyway after such
a heavy refactoring a little cleanup will not hurt.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)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 2d98997..aad2849 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -4216,6 +4216,7 @@ virDomainMigrateToURI(virDomainPtr domain,
const char *dname,
unsigned long bandwidth)
{
+ int ret = -1;
const char *dconnuri = NULL;
const char *miguri = NULL;
@@ -4225,26 +4226,24 @@ virDomainMigrateToURI(virDomainPtr domain,
virResetLastError();
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;
}
@@ -4343,6 +4342,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),
@@ -4351,23 +4351,20 @@ virDomainMigrateToURI2(virDomainPtr domain,
virResetLastError();
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;
}
@@ -4415,6 +4412,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);
@@ -4422,23 +4420,20 @@ virDomainMigrateToURI3(virDomainPtr domain,
virResetLastError();
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) < 0)
- 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
9:37 a.m.
New subject: [libvirt] [PATCH v2 12/12] migration: refactor: one return in forURI family functions
On Thu, Sep 10, 2015 at 04:20:24PM +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 exetend too. Anyway after such
a heavy refactoring a little cleanup will not hurt.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
---
src/libvirt-domain.c | 61 +++++++++++++++++++++++---------------------------
1 files changed, 28 insertions(+), 33 deletions(-)
ACK
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
10:22 a.m.
New subject: [libvirt] [PATCH v2 12/12] migration: refactor: one return in forURI family functions
On 09/10/2015 09:20 AM, 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 exetend too. Anyway after such
s/exetend/extend
a heavy refactoring a little cleanup will not hurt.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
---
src/libvirt-domain.c | 61 +++++++++++++++++++++++---------------------------
1 files changed, 28 insertions(+), 33 deletions(-)
Although I know Daniel has ACK'd already - I figured I'd complete it too...
BTW: I can confirm the following will work in ToURI3:
const char *duri = (flags & VIR_MIGRATE_PEER2PEER) ? dconnuri : NULL;
...
Remove the dconnuri = NULL, replace 'dconnuri' with 'duri' in the
virDomainMigrateUnmanagedParams call and Coverity stops complaining.
Curiously, I see the *ToURI2 function has a similar construct without a
complaint, but it's calling virDomainMigrateUnmanaged and I'll assume
Coverity gets sufficiently boggled with the call path that it doesn't
matter. Your call if you want to change the *ToURI2 function to use a
local static...
Beyond that these changes seem perfectly reasonable to me as well.
John
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
index 2d98997..aad2849 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -4216,6 +4216,7 @@ virDomainMigrateToURI(virDomainPtr domain,
const char *dname,
unsigned long bandwidth)
{
+ int ret = -1;
const char *dconnuri = NULL;
const char *miguri = NULL;
@@ -4225,26 +4226,24 @@ virDomainMigrateToURI(virDomainPtr domain,
virResetLastError();
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;
}
@@ -4343,6 +4342,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),
@@ -4351,23 +4351,20 @@ virDomainMigrateToURI2(virDomainPtr domain,
virResetLastError();
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;
}
@@ -4415,6 +4412,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);
@@ -4422,23 +4420,20 @@ virDomainMigrateToURI3(virDomainPtr domain,
virResetLastError();
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) < 0)
- 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;
}
2:01 a.m.
New subject: [libvirt] [PATCH v2 12/12] migration: refactor: one return in forURI family functions
On 17.09.2015 18:22, John Ferlan wrote:
On 09/10/2015 09:20 AM, 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 exetend too. Anyway after such
s/exetend/extend
> a heavy refactoring a little cleanup will not hurt.
>
> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
> ---
> src/libvirt-domain.c | 61 +++++++++++++++++++++++---------------------------
> 1 files changed, 28 insertions(+), 33 deletions(-)
>
Although I know Daniel has ACK'd already - I figured I'd complete it too...
BTW: I can confirm the following will work in ToURI3:
const char *duri = (flags & VIR_MIGRATE_PEER2PEER) ? dconnuri : NULL;
...
Remove the dconnuri = NULL, replace 'dconnuri' with 'duri' in the
virDomainMigrateUnmanagedParams call and Coverity stops complaining.
Curiously, I see the *ToURI2 function has a similar construct without a
complaint, but it's calling virDomainMigrateUnmanaged and I'll assume
Coverity gets sufficiently boggled with the call path that it doesn't
matter. Your call if you want to change the *ToURI2 function to use a
local static...
Beyond that these changes seem perfectly reasonable to me as well.
Thanx!
Now i'm going to fix all issues you found.
John
> diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
> index 2d98997..aad2849 100644
> --- a/src/libvirt-domain.c
> +++ b/src/libvirt-domain.c
> @@ -4216,6 +4216,7 @@ virDomainMigrateToURI(virDomainPtr domain,
> const char *dname,
> unsigned long bandwidth)
> {
> + int ret = -1;
> const char *dconnuri = NULL;
> const char *miguri = NULL;
>
> @@ -4225,26 +4226,24 @@ virDomainMigrateToURI(virDomainPtr domain,
> virResetLastError();
>
> 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;
> }
>
>
> @@ -4343,6 +4342,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),
> @@ -4351,23 +4351,20 @@ virDomainMigrateToURI2(virDomainPtr domain,
> virResetLastError();
>
> 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;
> }
>
>
> @@ -4415,6 +4412,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);
> @@ -4422,23 +4420,20 @@ virDomainMigrateToURI3(virDomainPtr domain,
> virResetLastError();
>
> 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) < 0)
> - 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;
> }
>
>
>
8:12 a.m.
New subject: [libvirt] [PATCH v2 12/12] migration: refactor: one return in forURI family functions
On 17.09.2015 18:22, John Ferlan wrote:
On 09/10/2015 09:20 AM, 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 exetend too. Anyway after such
s/exetend/extend
> a heavy refactoring a little cleanup will not hurt.
>
> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
> ---
> src/libvirt-domain.c | 61 +++++++++++++++++++++++---------------------------
> 1 files changed, 28 insertions(+), 33 deletions(-)
>
Although I know Daniel has ACK'd already - I figured I'd complete it too...
BTW: I can confirm the following will work in ToURI3:
const char *duri = (flags & VIR_MIGRATE_PEER2PEER) ? dconnuri : NULL;
...
Remove the dconnuri = NULL, replace 'dconnuri' with 'duri' in the
virDomainMigrateUnmanagedParams call and Coverity stops complaining.
Curiously, I see the *ToURI2 function has a similar construct without a
complaint, but it's calling virDomainMigrateUnmanaged and I'll assume
Coverity gets sufficiently boggled with the call path that it doesn't
matter. Your call if you want to change the *ToURI2 function to use a
local static...
I think that we'd probably better just check for not null in toURI2
and toURI3 under p2p check. Not sure for Coverity but this check
is a good input parameter check. I'll propose this as a new patch.
7:11 a.m.
On Thu, Sep 10, 2015 at 04:20:12PM +0300, Nikolay Shirokovskiy wrote:
Current implementation of 'toURI' migration interfaces does
not support all
combinations of interface versions and protocol versions. For example 'toURI2'
with p2p flag will not migrate if driver supports only v3params proto.
This is not convinient as drivers that starts to support migration have to
manually support older versions of protocol. I guess this should be done in
one place, namely here.
Another issue is that there are a lot of code duplication in implementation of
toURI interfaces and it is not obvious from code how are they related.
This implementation uses extensible parameters as intermediate parameters
representation. This is possible as interfaces are done backward compatible in
terms of parameters and later versions supports all parameters of former
versions.
= Changes from version1
Patch is splitted into a set. Quite a big one as a result of the following strategy:
1. each change in behaviour even subtle one deserves a separate patch. One
patch changes one aspect in behaviour.
2. separate pure refactoring steps into patches too as rather simple refactor
steps could introduce many line changes. Mark such patches with 'refactor:'
Now every patch is easy to grasp I think.
The resulted cumulative patch is slightly different from first in behaviour but
I'm not going to describe the differece here as original patch was not reviewed
in details by anyone anyway )
src/libvirt-domain.c | 520 +++++++++++++++++++++-----------------------------
1 files changed, 216 insertions(+), 304 deletions(-)
Just a quick note to say that I haven't forgotten about this patch
series. I'm looking to review it today/tomorrow I hope.
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
9:39 a.m.
On Thu, Sep 17, 2015 at 01:11:59PM +0100, Daniel P. Berrange wrote:
On Thu, Sep 10, 2015 at 04:20:12PM +0300, Nikolay Shirokovskiy
wrote:
> Current implementation of 'toURI' migration interfaces does not support all
> combinations of interface versions and protocol versions. For example
'toURI2'
> with p2p flag will not migrate if driver supports only v3params proto.
>
> This is not convinient as drivers that starts to support migration have to
> manually support older versions of protocol. I guess this should be done in
> one place, namely here.
>
> Another issue is that there are a lot of code duplication in implementation of
> toURI interfaces and it is not obvious from code how are they related.
>
> This implementation uses extensible parameters as intermediate parameters
> representation. This is possible as interfaces are done backward compatible in
> terms of parameters and later versions supports all parameters of former
> versions.
> = Changes from version1
>
> Patch is splitted into a set. Quite a big one as a result of the following
strategy:
>
> 1. each change in behaviour even subtle one deserves a separate patch. One
> patch changes one aspect in behaviour.
>
> 2. separate pure refactoring steps into patches too as rather simple refactor
> steps could introduce many line changes. Mark such patches with
'refactor:'
>
> Now every patch is easy to grasp I think.
>
> The resulted cumulative patch is slightly different from first in behaviour but
> I'm not going to describe the differece here as original patch was not reviewed
> in details by anyone anyway )
>
> src/libvirt-domain.c | 520 +++++++++++++++++++++-----------------------------
> 1 files changed, 216 insertions(+), 304 deletions(-)
Just a quick note to say that I haven't forgotten about this patch
series. I'm looking to review it today/tomorrow I hope.
So I've finally run through this. The patches look fine, and I also compared
the final state, with the current code to understand the final logic we now
have. That all still looks good and has the nice new features you mention.
There's the few points John points out in review, but nothing too critical
I've seen so far.
Given we have a history of screwing up migration though, I'd like one of the
other migration experts, such as Jiri, to take a look too before we push it.
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
1:57 a.m.
On 17.09.2015 17:39, Daniel P. Berrange wrote:
On Thu, Sep 17, 2015 at 01:11:59PM +0100, Daniel P. Berrange wrote:
> On Thu, Sep 10, 2015 at 04:20:12PM +0300, Nikolay Shirokovskiy wrote:
>> Current implementation of 'toURI' migration interfaces does not support
all
>> combinations of interface versions and protocol versions. For example
'toURI2'
>> with p2p flag will not migrate if driver supports only v3params proto.
>>
>> This is not convinient as drivers that starts to support migration have to
>> manually support older versions of protocol. I guess this should be done in
>> one place, namely here.
>>
>> Another issue is that there are a lot of code duplication in implementation of
>> toURI interfaces and it is not obvious from code how are they related.
>>
>> This implementation uses extensible parameters as intermediate parameters
>> representation. This is possible as interfaces are done backward compatible in
>> terms of parameters and later versions supports all parameters of former
>> versions.
>> = Changes from version1
>>
>> Patch is splitted into a set. Quite a big one as a result of the following
strategy:
>>
>> 1. each change in behaviour even subtle one deserves a separate patch. One
>> patch changes one aspect in behaviour.
>>
>> 2. separate pure refactoring steps into patches too as rather simple refactor
>> steps could introduce many line changes. Mark such patches with
'refactor:'
>>
>> Now every patch is easy to grasp I think.
>>
>> The resulted cumulative patch is slightly different from first in behaviour but
>> I'm not going to describe the differece here as original patch was not
reviewed
>> in details by anyone anyway )
>>
>> src/libvirt-domain.c | 520 +++++++++++++++++++++-----------------------------
>> 1 files changed, 216 insertions(+), 304 deletions(-)
>
> Just a quick note to say that I haven't forgotten about this patch
> series. I'm looking to review it today/tomorrow I hope.
So I've finally run through this. The patches look fine, and I also compared
the final state, with the current code to understand the final logic we now
have. That all still looks good and has the nice new features you mention.
There's the few points John points out in review, but nothing too critical
I've seen so far.
Given we have a history of screwing up migration though, I'd like one of the
other migration experts, such as Jiri, to take a look too before we push it.
Thanx!
I'll incorporate yours and Jonhs suggestions in next version so Jiri will
see the "final" version.
Regards,
Daniel
3353
days inactive
3361
days old
47 comments
4 participants
participants (4)
-
Daniel P. Berrange -
John Ferlan -
Nikolay Shirokovskiy -
Nikolay Shirokovskiy