[libvirt] [PATCH v2 09/12] migration: merge all proto branches into single function

[John Ferlan]

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