[libvirt] [PATCH 10/21] qemu: migration: src: qemu `drive-mirror` to UNIX

[John Ferlan]

On 11/18/2015 01:13 PM, Pavel Boldin wrote:
> Make qemuMigrationDriveMirror able to instruct QEMU to connect to
> a local UNIX socket used for tunnelling.
> 
> Signed-off-by: Pavel Boldin <pboldin at mirantis.com>
> ---
>  src/qemu/qemu_migration.c | 45 ++++++++++++++++++++++++++++++---------------
>  1 file changed, 30 insertions(+), 15 deletions(-)
> 
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index d587c56..d95cd66 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -2048,7 +2048,8 @@ static int
>  qemuMigrationDriveMirror(virQEMUDriverPtr driver,
>                           virDomainObjPtr vm,
>                           qemuMigrationCookiePtr mig,
> -                         const char *host,
> +                         bool dest_host,
> +                         const char *dest,

You'll need to update comments for function including the parameters

In particular if 'dest_host' is not set, then the expectation is that
the incoming 'dest' contains the unix socket file/tunnel.

FWIW: When I first read it, I thought the change would be to have the
new flag be the new option, not the old way.

Perhaps it'd be clearer to have the calling code check for
MIGRATION_DEST_FD in order to determine that we "could" have this ndb
socket.

However, something that's not quite clear (yet) - as I read it,
pec.nbd_tunnel_unix_socket.file is only generated when doTunnelMigrate
determines 'nmigrate_disks' is true. So if it's not true, but yet some
existing 'destType == MIGRATION_DEST_FD', then because this code has a
check for a NULL 'dest' value, it would seem a failure would occur when
perhaps it didn't previously or wasn't expected if there were no disks
to migrate.

Again, I have limited exposure/knowledge of the overall environment/
setup for migration, but something just doesn't seem right.

It would seem that the code now assumes that nmigrate_disks would be
true when MIGRATION_DEST_FD is set.

>                           unsigned long speed,
>                           unsigned int *migrate_flags,
>                           size_t nmigrate_disks,
> @@ -2057,7 +2058,7 @@ qemuMigrationDriveMirror(virQEMUDriverPtr driver,
>  {
>      qemuDomainObjPrivatePtr priv = vm->privateData;
>      int ret = -1;
> -    int port;
> +    int port = -1;
>      size_t i;
>      char *diskAlias = NULL;
>      char *nbd_dest = NULL;
> @@ -2068,16 +2069,20 @@ qemuMigrationDriveMirror(virQEMUDriverPtr driver,
>  
>      VIR_DEBUG("Starting drive mirrors for domain %s", vm->def->name);
>  
> -    /* steal NBD port and thus prevent its propagation back to destination */
> -    port = mig->nbd->port;
> -    mig->nbd->port = 0;
> +    virCheckNonNullArgGoto(dest, cleanup);

Hmm.. wouldn't ever expect this to trigger!  If 'host' had been NULL,
the strchr() below would have failed miserably.

> +
> +    if (dest_host) {
> +        /* steal NBD port and thus prevent its propagation back to destination */
> +        port = mig->nbd->port;
> +        mig->nbd->port = 0;
>  
> -    /* escape literal IPv6 address */
> -    if (strchr(host, ':')) {
> -        if (virAsprintf(&hoststr, "[%s]", host) < 0)
> +        /* escape literal IPv6 address */
> +        if (strchr(dest, ':')) {
> +            if (virAsprintf(&hoststr, "[%s]", dest) < 0)
> +                goto cleanup;
> +        } else if (VIR_STRDUP(hoststr, dest) < 0) {
>              goto cleanup;
> -    } else if (VIR_STRDUP(hoststr, host) < 0) {
> -        goto cleanup;
> +        }
>      }
>  
>      if (*migrate_flags & QEMU_MONITOR_MIGRATE_NON_SHARED_INC)
> @@ -2092,11 +2097,18 @@ qemuMigrationDriveMirror(virQEMUDriverPtr driver,
>          if (!qemuMigrateDisk(disk, nmigrate_disks, migrate_disks))
>              continue;
>  
> -        if ((virAsprintf(&diskAlias, "%s%s",
> -                         QEMU_DRIVE_HOST_PREFIX, disk->info.alias) < 0) ||
> -            (virAsprintf(&nbd_dest, "nbd:%s:%d:exportname=%s",
> -                         hoststr, port, diskAlias) < 0))
> +        if (virAsprintf(&diskAlias, "%s%s",
> +                        QEMU_DRIVE_HOST_PREFIX, disk->info.alias) < 0)
>              goto cleanup;
> +        if (dest_host) {
> +            if (virAsprintf(&nbd_dest, "nbd:%s:%d:exportname=%s",
> +                            hoststr, port, diskAlias) < 0)
> +                goto cleanup;
> +        } else {
> +            if (virAsprintf(&nbd_dest, "nbd:unix:%s:exportname=%s",
> +                            dest, diskAlias) < 0)
> +                goto cleanup;
> +        }
>  
>          if (qemuDomainObjEnterMonitorAsync(driver, vm,
>                                             QEMU_ASYNC_JOB_MIGRATION_OUT) < 0)
> @@ -4281,10 +4293,13 @@ qemuMigrationRun(virQEMUDriverPtr driver,
>  
>      if (migrate_flags & (QEMU_MONITOR_MIGRATE_NON_SHARED_DISK |
>                           QEMU_MONITOR_MIGRATE_NON_SHARED_INC)) {
> +        bool dest_host = spec->destType == MIGRATION_DEST_HOST;
> +        const char *dest = dest_host ? spec->dest.host.name :
> +                                       spec->nbd_tunnel_unix_socket.file;

Seems to me that perhaps the bool should be "dest_fd_migrate_disks" and
would be set if (spec->nbd_tunnel_unix_socket.file). It would only be
checked/set if "spec->destType == MIGRATION_DEST_FD"; otherwise, we use
the existing code and no boolean.

IOW: The assumption I see here is that "any" destType !=
MIGRATION_DEST_HOST means it's MIGRATION_DEST_FD, which perhaps isn't
true since I also see a MIGRATION_DEST_CONNECT_HOST and it doesn't
preclude something in the future being added.


John
>          if (mig->nbd) {
>              /* This will update migrate_flags on success */
>              if (qemuMigrationDriveMirror(driver, vm, mig,
> -                                         spec->dest.host.name,
> +                                         dest_host, dest,
>                                           migrate_speed,
>                                           &migrate_flags,
>                                           nmigrate_disks,
>