On Tue, Aug 25, 2020 at 07:47:12 +0200, Martin Kletzander wrote: > Adds new typed param for migration and uses this as a UNIX socket path that > should be used for the NBD part of migration. And also adds virsh support. > > Partially resolves: https://bugzilla.redhat.com/1638889 > > Signed-off-by: Martin Kletzander <mkletzan@xxxxxxxxxx> > --- > docs/manpages/virsh.rst | 8 +++ > include/libvirt/libvirt-domain.h | 12 ++++ > src/qemu/qemu_domain.h | 1 + > src/qemu/qemu_driver.c | 33 ++++++++-- > src/qemu/qemu_migration.c | 110 ++++++++++++++++++++++--------- > src/qemu/qemu_migration.h | 3 + > src/qemu/qemu_migration_cookie.c | 22 +++++-- > src/qemu/qemu_migration_cookie.h | 1 + > tools/virsh-domain.c | 12 ++++ > 9 files changed, 160 insertions(+), 42 deletions(-) ... > diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h > index adba79aded54..71a644ca6b95 100644 > --- a/src/qemu/qemu_domain.h > +++ b/src/qemu/qemu_domain.h > @@ -166,6 +166,7 @@ struct _qemuDomainObjPrivate { > unsigned long migMaxBandwidth; > char *origname; > int nbdPort; /* Port used for migration with NBD */ > + char *nbdSocketPath; /* Port used for migration with NBD */ Copy&pasta error :-) s/Port/Socket/ > unsigned short migrationPort; > int preMigrationState; > > diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c > index b887185d012d..3f4690f8fb72 100644 > --- a/src/qemu/qemu_migration.c > +++ b/src/qemu/qemu_migration.c > @@ -379,6 +379,7 @@ qemuMigrationDstStartNBDServer(virQEMUDriverPtr driver, > size_t nmigrate_disks, > const char **migrate_disks, > int nbdPort, > + const char *nbdSocketPath, > const char *tls_alias) > { > int ret = -1; > @@ -386,7 +387,7 @@ qemuMigrationDstStartNBDServer(virQEMUDriverPtr driver, > size_t i; > virStorageNetHostDef server = { > .name = (char *)listenAddr, /* cast away const */ > - .transport = VIR_STORAGE_NET_HOST_TRANS_TCP, > + .transport = VIR_STORAGE_NET_HOST_TRANS_UNIX, I'd expect transport to remain VIR_STORAGE_NET_HOST_TRANS_TCP unless nbdSocketPath is specified. In other words, the following hunk should be sufficient. > .port = nbdPort, > }; > bool server_started = false; > @@ -397,6 +398,13 @@ qemuMigrationDstStartNBDServer(virQEMUDriverPtr driver, > return -1; > } > > + /* Prefer nbdSocketPath as there is no way to indicate we do not want to > + * listen on a port */ > + if (nbdSocketPath) { > + server.socket = (char *)nbdSocketPath; > + server.transport = VIR_STORAGE_NET_HOST_TRANS_UNIX; > + } > + > for (i = 0; i < vm->def->ndisks; i++) { > virDomainDiskDefPtr disk = vm->def->disks[i]; > g_autofree char *diskAlias = NULL; > @@ -425,7 +433,7 @@ qemuMigrationDstStartNBDServer(virQEMUDriverPtr driver, > devicename = diskAlias; > } > > - if (!server_started) { > + if (!server_started && !nbdSocketPath) { I'd probably change this to if (!server_started && server.transport == VIR_STORAGE_NET_HOST_TRANS_TCP) { to make it more obvious we only care about port allocation for TCP. > if (server.port) { > if (virPortAllocatorSetUsed(server.port) < 0) > goto cleanup; ... > @@ -885,13 +903,17 @@ qemuMigrationSrcNBDStorageCopyDriveMirror(virQEMUDriverPtr driver, > const char *diskAlias, > const char *host, > int port, > + const char *socket, > unsigned long long mirror_speed, > bool mirror_shallow) > { > g_autofree char *nbd_dest = NULL; > int mon_ret; > > - if (strchr(host, ':')) { > + if (socket) { > + nbd_dest = g_strdup_printf("nbd+unix:///%s?socket=%s", > + diskAlias, socket); Is the number of slashes correct here? The "nbd+unix://" is clear, but then we pass "/diskAlias" rather than just "diskAlias". Oh I see, since "//" is present, the path has to either be empty or start with "/" to form a valid URI. Everything seems to be correct then, although a bit confusing since diskAlias is not actually a path. But I guess that's what QEMU expects. > + } else if (strchr(host, ':')) { > nbd_dest = g_strdup_printf("nbd:[%s]:%d:exportname=%s", host, port, > diskAlias); > } else { ... > @@ -2329,6 +2357,8 @@ qemuMigrationDstPrepare(virDomainObjPtr vm, > > if (tunnel) { > migrateFrom = g_strdup("stdio"); > + } else if (g_strcmp0(protocol, "unix") == 0) { > + migrateFrom = g_strdup_printf("%s:%s", protocol, listenAddress); > } else { > bool encloseAddress = false; > bool hostIPv6Capable = false; Looks like this hunk would better fit in 8/9 (qemu: Allow migration over UNIX socket). ... With the small issues addressed and if all I said is correct Reviewed-by: Jiri Denemark <jdenemar@xxxxxxxxxx>
