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@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 :|