Re: [PATCH v2 05/12] migration: refactor: merge direct and p2p into unmanaged

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]