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