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@xxxxxxxxxxxxx>
> ---
> 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",
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list