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
> ---
> 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 ?
