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@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,