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.

-/* - * 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",

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@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)) {

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@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; }

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; }

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@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); } }

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@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)) {

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

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@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; }