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@xxxxxxxxxxxx>
> ---
> 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,
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list