Re: [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&gt; > --- > 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.

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...