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