9:41 a.m.
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(a)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,