[libvirt PATCH 6/9] qemu: Allow NBD migration over UNIX socket

[Jiri Denemark]

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 at redhat.com>
> ---
>  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 at redhat.com>