Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com&gt; --- src/qemu/qemu_migration.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 142faa2cf997..60ddfde65d46 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -3369,7 +3369,7 @@ qemuMigrationSrcConnect(virQEMUDriverPtr driver, { virNetSocketPtr sock; const char *host; - char *port = NULL; + g_autofree char *port = NULL; int ret = -1; host = spec->dest.host.name; @@ -3400,7 +3400,6 @@ qemuMigrationSrcConnect(virQEMUDriverPtr driver, ret = 0; cleanup: - VIR_FREE(port); if (ret < 0) VIR_FORCE_CLOSE(spec->dest.fd.qemu); return ret; -- 2.28.0

Instead of saving some data from a union up front and changing an overlayed struct before using said data, let's just set the new values after they are decided. This will increase the readability of future commit(s). Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com&gt; --- src/qemu/qemu_migration.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 60ddfde65d46..1a3cdb71f424 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -3368,24 +3368,24 @@ qemuMigrationSrcConnect(virQEMUDriverPtr driver, qemuMigrationSpecPtr spec) { virNetSocketPtr sock; - const char *host; g_autofree char *port = NULL; + int fd_qemu = -1; int ret = -1; - host = spec->dest.host.name; - port = g_strdup_printf("%d", spec->dest.host.port); - - spec->destType = MIGRATION_DEST_FD; - spec->dest.fd.qemu = -1; - if (qemuSecuritySetSocketLabel(driver->securityManager, vm->def) < 0) goto cleanup; - if (virNetSocketNewConnectTCP(host, port, + port = g_strdup_printf("%d", spec->dest.host.port); + if (virNetSocketNewConnectTCP(spec->dest.host.name, + port, AF_UNSPEC, &sock) == 0) { - spec->dest.fd.qemu = virNetSocketDupFD(sock, true); + fd_qemu = virNetSocketDupFD(sock, true); virObjectUnref(sock); } + + spec->destType = MIGRATION_DEST_FD; + spec->dest.fd.qemu = fd_qemu; + if (qemuSecurityClearSocketLabel(driver->securityManager, vm->def) < 0 || spec->dest.fd.qemu == -1) goto cleanup; -- 2.28.0

Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com&gt; --- tools/virsh-domain.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 1f3a549d9ab0..c606900a9268 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -10704,7 +10704,6 @@ doMigrate(void *opaque) virDomainPtr dom = NULL; const char *desturi = NULL; const char *opt = NULL; - int disksPort = 0; unsigned int flags = 0; virshCtrlData *data = opaque; vshControl *ctl = data->ctl; @@ -10752,11 +10751,11 @@ doMigrate(void *opaque) VIR_MIGRATE_PARAM_LISTEN_ADDRESS, opt) < 0) goto save_error; - if (vshCommandOptInt(ctl, cmd, "disks-port", &disksPort) < 0) + if (vshCommandOptInt(ctl, cmd, "disks-port", &intOpt) < 0) goto out; - if (disksPort && + if (intOpt && virTypedParamsAddInt(&params, &nparams, &maxparams, - VIR_MIGRATE_PARAM_DISKS_PORT, disksPort) < 0) + VIR_MIGRATE_PARAM_DISKS_PORT, intOpt) < 0) goto save_error; if (vshCommandOptStringReq(ctl, cmd, "dname", &opt) < 0) -- 2.28.0

Clean up the semantics by using one extra self-describing variable. This also fixes the port allocation when the port is specified. Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com&gt; --- src/qemu/qemu_migration.c | 37 +++++++++++++++++++++++-------------- 1 file changed, 23 insertions(+), 14 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 1a3cdb71f424..b887185d012d 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -383,12 +383,13 @@ qemuMigrationDstStartNBDServer(virQEMUDriverPtr driver, { int ret = -1; qemuDomainObjPrivatePtr priv = vm->privateData; - unsigned short port = 0; size_t i; virStorageNetHostDef server = { .name = (char *)listenAddr, /* cast away const */ .transport = VIR_STORAGE_NET_HOST_TRANS_TCP, + .port = nbdPort, }; + bool server_started = false; if (nbdPort < 0 || nbdPort > USHRT_MAX) { virReportError(VIR_ERR_INVALID_ARG, "%s", @@ -424,20 +425,27 @@ qemuMigrationDstStartNBDServer(virQEMUDriverPtr driver, devicename = diskAlias; } + if (!server_started) { + if (server.port) { + if (virPortAllocatorSetUsed(server.port) < 0) + goto cleanup; + } else { + unsigned short port = 0; + + if (virPortAllocatorAcquire(driver->migrationPorts, &port) < 0) + goto cleanup; + + server.port = port; + } + } + if (qemuDomainObjEnterMonitorAsync(driver, vm, QEMU_ASYNC_JOB_MIGRATION_IN) < 0) goto cleanup; - if (port == 0) { - if (nbdPort) - port = nbdPort; - else if (virPortAllocatorAcquire(driver->migrationPorts, &port) < 0) - goto exit_monitor; - - server.port = port; - if (qemuMonitorNBDServerStart(priv->mon, &server, tls_alias) < 0) - goto exit_monitor; - } + if (!server_started && + qemuMonitorNBDServerStart(priv->mon, &server, tls_alias) < 0) + goto exit_monitor; if (qemuMonitorNBDServerAdd(priv->mon, devicename, exportname, true, NULL) < 0) goto exit_monitor; @@ -445,12 +453,13 @@ qemuMigrationDstStartNBDServer(virQEMUDriverPtr driver, goto cleanup; } - priv->nbdPort = port; + priv->nbdPort = server.port; + ret = 0; cleanup: - if (ret < 0 && nbdPort == 0) - virPortAllocatorRelease(port); + if (ret < 0) + virPortAllocatorRelease(server.port); return ret; exit_monitor: -- 2.28.0

For this we need to make the function accessible (at least privately). The behaviour will change in following patches and the test helps explaining the change. Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com&gt; --- scripts/apibuild.py | 1 + src/libvirt-domain.c | 4 +- src/libvirt_internal.h | 2 + src/libvirt_private.syms | 1 + tests/meson.build | 1 + tests/virmigtest.c | 90 ++++++++++++++++++++++++++++++++++++++++ 6 files changed, 96 insertions(+), 3 deletions(-) create mode 100644 tests/virmigtest.c diff --git a/scripts/apibuild.py b/scripts/apibuild.py index 58ae76d29cfc..b94c0f6c09dd 100755 --- a/scripts/apibuild.py +++ b/scripts/apibuild.py @@ -81,6 +81,7 @@ ignored_words = { ignored_functions = { "virConnectSupportsFeature": "private function for remote access", + "virDomainMigrateCheckNotLocal": "private function for migration", "virDomainMigrateFinish": "private function for migration", "virDomainMigrateFinish2": "private function for migration", "virDomainMigratePerform": "private function for migration", diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index ad60a92da879..4d958ca5219d 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -3269,8 +3269,7 @@ virDomainMigrateVersion3Params(virDomainPtr domain, params, nparams, true, flags); } - -static int +int virDomainMigrateCheckNotLocal(const char *dconnuri) { g_autoptr(virURI) tempuri = NULL; @@ -3286,7 +3285,6 @@ virDomainMigrateCheckNotLocal(const char *dconnuri) return 0; } - static int virDomainMigrateUnmanagedProto2(virDomainPtr domain, const char *dconnuri, diff --git a/src/libvirt_internal.h b/src/libvirt_internal.h index 72c61274a74e..cb239b3c1c44 100644 --- a/src/libvirt_internal.h +++ b/src/libvirt_internal.h @@ -129,6 +129,8 @@ typedef enum { int virConnectSupportsFeature(virConnectPtr conn, int feature); +int virDomainMigrateCheckNotLocal(const char *dconnuri); + int virDomainMigratePrepare (virConnectPtr dconn, char **cookie, int *cookielen, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index f950a681792c..2f6d67cebe07 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1481,6 +1481,7 @@ virHostdevUpdateActiveUSBDevices; virConnectSupportsFeature; virDomainMigrateBegin3; virDomainMigrateBegin3Params; +virDomainMigrateCheckNotLocal; virDomainMigrateConfirm3; virDomainMigrateConfirm3Params; virDomainMigrateFinish; diff --git a/tests/meson.build b/tests/meson.build index b5f6e2267aaf..49bcf5eb8a8d 100644 --- a/tests/meson.build +++ b/tests/meson.build @@ -331,6 +331,7 @@ tests += [ { 'name': 'virtypedparamtest' }, { 'name': 'viruritest' }, { 'name': 'vshtabletest', 'link_with': [ libvirt_shell_lib ] }, + { 'name': 'virmigtest' }, ] if host_machine.system() == 'linux' diff --git a/tests/virmigtest.c b/tests/virmigtest.c new file mode 100644 index 000000000000..9539aadb5157 --- /dev/null +++ b/tests/virmigtest.c @@ -0,0 +1,90 @@ +/* + * Copyright (C) 2020 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>;. + */ + +#include <config.h> + +#include "libvirt_internal.h" + +#include "testutils.h" +#include "virlog.h" +#include "viruri.h" + +#define VIR_FROM_THIS VIR_FROM_RPC + +VIR_LOG_INIT("tests.migtest"); + +struct MigLocalData { + const char *uri; + bool fail; +}; + +extern int virDomainMigrateCheckNotLocal(const char *dconnuri); + +static int testMigNotLocal(const void *args) +{ + int ret = -1; + const struct MigLocalData *data = args; + + ret = virDomainMigrateCheckNotLocal(data->uri); + + if (ret == -1) { + if (data->fail) { + virResetLastError(); + return 0; + } + return -1; + } + + if (data->fail) { + VIR_TEST_DEBUG("passed instead of expected failure"); + return -1; + } + + return ret; +} + +static int +mymain(void) +{ + int ret = 0; + +#define TEST_FULL(uri, fail) \ + do { \ + const struct MigLocalData data = { \ + uri, fail \ + }; \ + if (virTestRun("Test URI " # uri, testMigNotLocal, &data) < 0) \ + ret = -1; \ + } while (0) + +#define TEST(uri) TEST_FULL(uri, false) +#define TEST_FAIL(uri) TEST_FULL(uri, true) + + TEST_FAIL("qemu:///system"); + + TEST_FAIL("//localhost"); + TEST_FAIL("tcp://localhost.localdomain/"); + + TEST("scheme://some.cryptorandom.fqdn.tld"); + + TEST_FAIL("hehe+unix:///?socket=/path/to/some-sock"); + + return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; +} + +VIR_TEST_MAIN(mymain) -- 2.28.0

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(a)redhat.com&gt; --- 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/docs/manpages/virsh.rst b/docs/manpages/virsh.rst index 9187037a5615..75f475eea6ad 100644 --- a/docs/manpages/virsh.rst +++ b/docs/manpages/virsh.rst @@ -3113,6 +3113,7 @@ migrate [--postcopy-bandwidth bandwidth] [--parallel [--parallel-connections connections]] [--bandwidth bandwidth] [--tls-destination hostname] + [--disks-socket socket-path] Migrate domain to another host. Add *--live* for live migration; <--p2p> for peer-2-peer migration; *--direct* for direct migration; or *--tunnelled* @@ -3292,6 +3293,13 @@ error if this parameter is used. Optional *disks-port* sets the port that hypervisor on destination side should bind to for incoming disks traffic. Currently it is supported only by QEMU. +Optional *disks-socket* path can also be specified (mutually exclusive with +*disks-port*) in case you need the disk migration to happen over a UNIX socket +with that specified path. In this case you need to make sure the same socket +path is accessible to both source and destination hypervisors and connecting to +the socket on the source (after hypervisor creates it on the destination) will +actually connect to the destination. + migrate-compcache ----------------- diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index 8b9d9c110c19..b6c6b402b9b6 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -981,6 +981,18 @@ typedef enum { */ # define VIR_MIGRATE_PARAM_DISKS_PORT "disks_port" +/** + * VIR_MIGRATE_PARAM_DISKS_SOCKET: + * + * virDomainMigrate* params field: path of a UNIX socket that destination server + * should use for incoming disks migration. Type is VIR_TYPED_PARAM_STRING. If + * omitted, migration will proceed over network (default). At the moment this is + * only supported by the QEMU driver. This is only usable if the management + * application makes sure that socket created with this name on the destination + * will be reachable from the source under the same exact path. + */ +# define VIR_MIGRATE_PARAM_DISKS_SOCKET "disks_socket" + /** * VIR_MIGRATE_PARAM_COMPRESSION: * 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 */ unsigned short migrationPort; int preMigrationState; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 3636716ceea1..74b0348ccc4c 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -11233,7 +11233,7 @@ qemuDomainMigratePrepare2(virConnectPtr dconn, ret = qemuMigrationDstPrepareDirect(driver, dconn, NULL, 0, NULL, NULL, /* No cookies */ uri_in, uri_out, - &def, origname, NULL, 0, NULL, 0, + &def, origname, NULL, 0, NULL, 0, NULL, migParams, flags); cleanup: @@ -11289,6 +11289,7 @@ qemuDomainMigratePerform(virDomainPtr dom, */ ret = qemuMigrationSrcPerform(driver, dom->conn, vm, NULL, NULL, dconnuri, uri, NULL, NULL, 0, NULL, 0, + NULL, migParams, cookie, cookielen, NULL, NULL, /* No output cookies in v2 */ flags, dname, resource, false); @@ -11459,7 +11460,7 @@ qemuDomainMigratePrepare3(virConnectPtr dconn, cookieout, cookieoutlen, uri_in, uri_out, &def, origname, NULL, 0, NULL, 0, - migParams, flags); + NULL, migParams, flags); } static int @@ -11485,6 +11486,7 @@ qemuDomainMigratePrepare3Params(virConnectPtr dconn, g_autofree const char **migrate_disks = NULL; g_autofree char *origname = NULL; g_autoptr(qemuMigrationParams) migParams = NULL; + const char *nbdSocketPath = NULL; virCheckFlags(QEMU_MIGRATION_FLAGS, -1); if (virTypedParamsValidate(params, nparams, QEMU_MIGRATION_PARAMETERS) < 0) @@ -11502,6 +11504,9 @@ qemuDomainMigratePrepare3Params(virConnectPtr dconn, virTypedParamsGetString(params, nparams, VIR_MIGRATE_PARAM_LISTEN_ADDRESS, &listenAddress) < 0 || + virTypedParamsGetString(params, nparams, + VIR_MIGRATE_PARAM_DISKS_SOCKET, + &nbdSocketPath) < 0 || virTypedParamsGetInt(params, nparams, VIR_MIGRATE_PARAM_DISKS_PORT, &nbdPort) < 0) @@ -11518,6 +11523,13 @@ qemuDomainMigratePrepare3Params(virConnectPtr dconn, QEMU_MIGRATION_DESTINATION))) return -1; + if (nbdSocketPath && nbdPort) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("Both port and socket requested for disk migration " + "while being mutually exclusive")); + return -1; + } + if (flags & VIR_MIGRATE_TUNNELLED) { /* this is a logical error; we never should have gotten here with * VIR_MIGRATE_TUNNELLED set @@ -11540,7 +11552,7 @@ qemuDomainMigratePrepare3Params(virConnectPtr dconn, uri_in, uri_out, &def, origname, listenAddress, nmigrate_disks, migrate_disks, nbdPort, - migParams, flags); + nbdSocketPath, migParams, flags); } @@ -11682,7 +11694,7 @@ qemuDomainMigratePerform3(virDomainPtr dom, ret = qemuMigrationSrcPerform(driver, dom->conn, vm, xmlin, NULL, dconnuri, uri, NULL, NULL, 0, NULL, 0, - migParams, + NULL, migParams, cookiein, cookieinlen, cookieout, cookieoutlen, flags, dname, resource, true); @@ -11716,6 +11728,7 @@ qemuDomainMigratePerform3Params(virDomainPtr dom, unsigned long long bandwidth = 0; int nbdPort = 0; g_autoptr(qemuMigrationParams) migParams = NULL; + const char *nbdSocketPath = NULL; int ret = -1; virCheckFlags(QEMU_MIGRATION_FLAGS, -1); @@ -11743,11 +11756,21 @@ qemuDomainMigratePerform3Params(virDomainPtr dom, virTypedParamsGetInt(params, nparams, VIR_MIGRATE_PARAM_DISKS_PORT, &nbdPort) < 0 || + virTypedParamsGetString(params, nparams, + VIR_MIGRATE_PARAM_DISKS_SOCKET, + &nbdSocketPath) < 0 || virTypedParamsGetString(params, nparams, VIR_MIGRATE_PARAM_PERSIST_XML, &persist_xml) < 0) goto cleanup; + if (nbdSocketPath && nbdPort) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("Both port and socket requested for disk migration " + "while being mutually exclusive")); + goto cleanup; + } + nmigrate_disks = virTypedParamsGetStringList(params, nparams, VIR_MIGRATE_PARAM_MIGRATE_DISKS, &migrate_disks); @@ -11768,7 +11791,7 @@ qemuDomainMigratePerform3Params(virDomainPtr dom, ret = qemuMigrationSrcPerform(driver, dom->conn, vm, dom_xml, persist_xml, dconnuri, uri, graphicsuri, listenAddress, nmigrate_disks, migrate_disks, nbdPort, - migParams, + nbdSocketPath, migParams, cookiein, cookieinlen, cookieout, cookieoutlen, flags, dname, bandwidth, true); cleanup: 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, .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) { if (server.port) { if (virPortAllocatorSetUsed(server.port) < 0) goto cleanup; @@ -453,7 +461,10 @@ qemuMigrationDstStartNBDServer(virQEMUDriverPtr driver, goto cleanup; } - priv->nbdPort = server.port; + if (nbdSocketPath) + priv->nbdSocketPath = g_strdup(server.socket); + else + priv->nbdPort = server.port; ret = 0; @@ -793,6 +804,7 @@ static virStorageSourcePtr qemuMigrationSrcNBDStorageCopyBlockdevPrepareSource(virDomainDiskDefPtr disk, const char *host, int port, + const char *socket, const char *tlsAlias) { g_autoptr(virStorageSource) copysrc = NULL; @@ -813,9 +825,14 @@ qemuMigrationSrcNBDStorageCopyBlockdevPrepareSource(virDomainDiskDefPtr disk, copysrc->hosts = g_new0(virStorageNetHostDef, 1); copysrc->nhosts = 1; - copysrc->hosts->transport = VIR_STORAGE_NET_HOST_TRANS_TCP; - copysrc->hosts->port = port; - copysrc->hosts->name = g_strdup(host); + if (socket) { + copysrc->hosts->transport = VIR_STORAGE_NET_HOST_TRANS_UNIX; + copysrc->hosts->socket = g_strdup(socket); + } else { + copysrc->hosts->transport = VIR_STORAGE_NET_HOST_TRANS_TCP; + copysrc->hosts->port = port; + copysrc->hosts->name = g_strdup(host); + } copysrc->tlsAlias = g_strdup(tlsAlias); @@ -835,6 +852,7 @@ qemuMigrationSrcNBDStorageCopyBlockdev(virQEMUDriverPtr driver, bool persistjob, const char *host, int port, + const char *socket, unsigned long long mirror_speed, unsigned int mirror_shallow, const char *tlsAlias) @@ -846,7 +864,7 @@ qemuMigrationSrcNBDStorageCopyBlockdev(virQEMUDriverPtr driver, VIR_DEBUG("starting blockdev mirror for disk=%s to host=%s", disk->dst, host); - if (!(copysrc = qemuMigrationSrcNBDStorageCopyBlockdevPrepareSource(disk, host, port, tlsAlias))) + if (!(copysrc = qemuMigrationSrcNBDStorageCopyBlockdevPrepareSource(disk, host, port, socket, tlsAlias))) return -1; /* Migration via blockdev-mirror was supported sooner than the auto-read-only @@ -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); + } else if (strchr(host, ':')) { nbd_dest = g_strdup_printf("nbd:[%s]:%d:exportname=%s", host, port, diskAlias); } else { @@ -920,6 +942,7 @@ qemuMigrationSrcNBDStorageCopyOne(virQEMUDriverPtr driver, virDomainDiskDefPtr disk, const char *host, int port, + const char *socket, unsigned long long mirror_speed, bool mirror_shallow, const char *tlsAlias, @@ -958,13 +981,13 @@ qemuMigrationSrcNBDStorageCopyOne(virQEMUDriverPtr driver, rc = qemuMigrationSrcNBDStorageCopyBlockdev(driver, vm, disk, jobname, sourcename, persistjob, - host, port, + host, port, socket, mirror_speed, mirror_shallow, tlsAlias); } else { rc = qemuMigrationSrcNBDStorageCopyDriveMirror(driver, vm, diskAlias, - host, port, + host, port, socket, mirror_speed, mirror_shallow); } @@ -1014,6 +1037,7 @@ qemuMigrationSrcNBDStorageCopy(virQEMUDriverPtr driver, const char **migrate_disks, virConnectPtr dconn, const char *tlsAlias, + const char *socket, unsigned int flags) { qemuDomainObjPrivatePtr priv = vm->privateData; @@ -1038,6 +1062,9 @@ qemuMigrationSrcNBDStorageCopy(virQEMUDriverPtr driver, port = mig->nbd->port; mig->nbd->port = 0; + if (qemuSecurityDomainSetPathLabel(driver, vm, socket, false) < 0) + return -1; + for (i = 0; i < vm->def->ndisks; i++) { virDomainDiskDefPtr disk = vm->def->disks[i]; @@ -1046,6 +1073,7 @@ qemuMigrationSrcNBDStorageCopy(virQEMUDriverPtr driver, continue; if (qemuMigrationSrcNBDStorageCopyOne(driver, vm, disk, host, port, + socket, mirror_speed, mirror_shallow, tlsAlias, flags) < 0) return -1; @@ -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; @@ -2397,6 +2427,7 @@ qemuMigrationDstPrepareAny(virQEMUDriverPtr driver, size_t nmigrate_disks, const char **migrate_disks, int nbdPort, + const char *nbdSocketPath, qemuMigrationParamsPtr migParams, unsigned long flags) { @@ -2650,7 +2681,8 @@ qemuMigrationDstPrepareAny(virQEMUDriverPtr driver, if (qemuMigrationDstStartNBDServer(driver, vm, incoming->address, nmigrate_disks, migrate_disks, - nbdPort, nbdTLSAlias) < 0) { + nbdPort, nbdSocketPath, + nbdTLSAlias) < 0) { goto stopjob; } cookieFlags |= QEMU_MIGRATION_COOKIE_NBD; @@ -2785,7 +2817,7 @@ qemuMigrationDstPrepareTunnel(virQEMUDriverPtr driver, return qemuMigrationDstPrepareAny(driver, dconn, cookiein, cookieinlen, cookieout, cookieoutlen, def, origname, st, NULL, 0, false, NULL, 0, NULL, 0, - migParams, flags); + NULL, migParams, flags); } @@ -2826,6 +2858,7 @@ qemuMigrationDstPrepareDirect(virQEMUDriverPtr driver, size_t nmigrate_disks, const char **migrate_disks, int nbdPort, + const char *nbdSocketPath, qemuMigrationParamsPtr migParams, unsigned long flags) { @@ -2840,11 +2873,13 @@ qemuMigrationDstPrepareDirect(virQEMUDriverPtr driver, VIR_DEBUG("driver=%p, dconn=%p, cookiein=%s, cookieinlen=%d, " "cookieout=%p, cookieoutlen=%p, uri_in=%s, uri_out=%p, " "def=%p, origname=%s, listenAddress=%s, " - "nmigrate_disks=%zu, migrate_disks=%p, nbdPort=%d, flags=0x%lx", + "nmigrate_disks=%zu, migrate_disks=%p, nbdPort=%d, " + "nbdSocketPath=%s, flags=0x%lx", driver, dconn, NULLSTR(cookiein), cookieinlen, cookieout, cookieoutlen, NULLSTR(uri_in), uri_out, *def, origname, NULLSTR(listenAddress), - nmigrate_disks, migrate_disks, nbdPort, flags); + nmigrate_disks, migrate_disks, nbdPort, NULLSTR(nbdSocketPath), + flags); *uri_out = NULL; @@ -2947,7 +2982,7 @@ qemuMigrationDstPrepareDirect(virQEMUDriverPtr driver, NULL, uri ? uri->scheme : "tcp", port, autoPort, listenAddress, nmigrate_disks, migrate_disks, nbdPort, - migParams, flags); + nbdSocketPath, migParams, flags); cleanup: if (ret != 0) { VIR_FREE(*uri_out); @@ -3482,7 +3517,8 @@ qemuMigrationSrcRun(virQEMUDriverPtr driver, const char *graphicsuri, size_t nmigrate_disks, const char **migrate_disks, - qemuMigrationParamsPtr migParams) + qemuMigrationParamsPtr migParams, + const char *nbdSocketPath) { int ret = -1; unsigned int migrate_flags = QEMU_MONITOR_MIGRATE_BACKGROUND; @@ -3614,7 +3650,8 @@ qemuMigrationSrcRun(virQEMUDriverPtr driver, &migrate_flags, nmigrate_disks, migrate_disks, - dconn, tlsAlias, flags) < 0) { + dconn, tlsAlias, + nbdSocketPath, flags) < 0) { goto error; } } else { @@ -3854,7 +3891,8 @@ qemuMigrationSrcPerformNative(virQEMUDriverPtr driver, const char *graphicsuri, size_t nmigrate_disks, const char **migrate_disks, - qemuMigrationParamsPtr migParams) + qemuMigrationParamsPtr migParams, + const char *nbdSocketPath) { qemuDomainObjPrivatePtr priv = vm->privateData; g_autoptr(virURI) uribits = NULL; @@ -3908,7 +3946,7 @@ qemuMigrationSrcPerformNative(virQEMUDriverPtr driver, ret = qemuMigrationSrcRun(driver, vm, persist_xml, cookiein, cookieinlen, cookieout, cookieoutlen, flags, resource, &spec, dconn, graphicsuri, nmigrate_disks, migrate_disks, - migParams); + migParams, nbdSocketPath); if (spec.destType == MIGRATION_DEST_FD) VIR_FORCE_CLOSE(spec.dest.fd.qemu); @@ -3971,7 +4009,7 @@ qemuMigrationSrcPerformTunnel(virQEMUDriverPtr driver, ret = qemuMigrationSrcRun(driver, vm, persist_xml, cookiein, cookieinlen, cookieout, cookieoutlen, flags, resource, &spec, dconn, graphicsuri, nmigrate_disks, migrate_disks, - migParams); + migParams, NULL); cleanup: VIR_FORCE_CLOSE(spec.dest.fd.qemu); @@ -4078,7 +4116,7 @@ qemuMigrationSrcPerformPeer2Peer2(virQEMUDriverPtr driver, cookie, cookielen, NULL, NULL, /* No out cookie with v2 migration */ flags, resource, dconn, NULL, 0, NULL, - migParams); + migParams, NULL); /* Perform failed. Make sure Finish doesn't overwrite the error */ if (ret < 0) @@ -4142,6 +4180,7 @@ qemuMigrationSrcPerformPeer2Peer3(virQEMUDriverPtr driver, size_t nmigrate_disks, const char **migrate_disks, int nbdPort, + const char *nbdSocketPath, qemuMigrationParamsPtr migParams, unsigned long long bandwidth, bool useParams, @@ -4226,6 +4265,11 @@ qemuMigrationSrcPerformPeer2Peer3(virQEMUDriverPtr driver, VIR_MIGRATE_PARAM_DISKS_PORT, nbdPort) < 0) goto cleanup; + if (nbdSocketPath && + virTypedParamsAddString(&params, &nparams, &maxparams, + VIR_MIGRATE_PARAM_DISKS_SOCKET, + nbdSocketPath) < 0) + goto cleanup; if (qemuMigrationParamsDump(migParams, &params, &nparams, &maxparams, &flags) < 0) @@ -4323,7 +4367,7 @@ qemuMigrationSrcPerformPeer2Peer3(virQEMUDriverPtr driver, &cookieout, &cookieoutlen, flags, bandwidth, dconn, graphicsuri, nmigrate_disks, migrate_disks, - migParams); + migParams, nbdSocketPath); } /* Perform failed. Make sure Finish doesn't overwrite the error */ @@ -4498,6 +4542,7 @@ qemuMigrationSrcPerformPeer2Peer(virQEMUDriverPtr driver, size_t nmigrate_disks, const char **migrate_disks, int nbdPort, + const char *nbdSocketPath, qemuMigrationParamsPtr migParams, unsigned long flags, const char *dname, @@ -4618,7 +4663,7 @@ qemuMigrationSrcPerformPeer2Peer(virQEMUDriverPtr driver, ret = qemuMigrationSrcPerformPeer2Peer3(driver, sconn, dconn, dconnuri, vm, xmlin, persist_xml, dname, uri, graphicsuri, listenAddress, nmigrate_disks, migrate_disks, - nbdPort, migParams, resource, + nbdPort, nbdSocketPath, migParams, resource, useParams, flags); } else { ret = qemuMigrationSrcPerformPeer2Peer2(driver, sconn, dconn, vm, @@ -4654,6 +4699,7 @@ qemuMigrationSrcPerformJob(virQEMUDriverPtr driver, size_t nmigrate_disks, const char **migrate_disks, int nbdPort, + const char *nbdSocketPath, qemuMigrationParamsPtr migParams, const char *cookiein, int cookieinlen, @@ -4692,6 +4738,7 @@ qemuMigrationSrcPerformJob(virQEMUDriverPtr driver, ret = qemuMigrationSrcPerformPeer2Peer(driver, conn, vm, xmlin, persist_xml, dconnuri, uri, graphicsuri, listenAddress, nmigrate_disks, migrate_disks, nbdPort, + nbdSocketPath, migParams, flags, dname, resource, &v3proto); } else { @@ -4699,7 +4746,7 @@ qemuMigrationSrcPerformJob(virQEMUDriverPtr driver, ret = qemuMigrationSrcPerformNative(driver, vm, persist_xml, uri, cookiein, cookieinlen, cookieout, cookieoutlen, flags, resource, NULL, NULL, 0, NULL, - migParams); + migParams, nbdSocketPath); } if (ret < 0) goto endjob; @@ -4765,7 +4812,8 @@ qemuMigrationSrcPerformPhase(virQEMUDriverPtr driver, char **cookieout, int *cookieoutlen, unsigned long flags, - unsigned long resource) + unsigned long resource, + const char *nbdSocketPath) { qemuDomainObjPrivatePtr priv = vm->privateData; qemuDomainJobPrivatePtr jobPriv = priv->job.privateData; @@ -4787,7 +4835,7 @@ qemuMigrationSrcPerformPhase(virQEMUDriverPtr driver, ret = qemuMigrationSrcPerformNative(driver, vm, persist_xml, uri, cookiein, cookieinlen, cookieout, cookieoutlen, flags, resource, NULL, graphicsuri, - nmigrate_disks, migrate_disks, migParams); + nmigrate_disks, migrate_disks, migParams, nbdSocketPath); if (ret < 0) { qemuMigrationSrcRestoreDomainState(driver, vm); @@ -4828,6 +4876,7 @@ qemuMigrationSrcPerform(virQEMUDriverPtr driver, size_t nmigrate_disks, const char **migrate_disks, int nbdPort, + const char *nbdSocketPath, qemuMigrationParamsPtr migParams, const char *cookiein, int cookieinlen, @@ -4841,11 +4890,12 @@ qemuMigrationSrcPerform(virQEMUDriverPtr driver, VIR_DEBUG("driver=%p, conn=%p, vm=%p, xmlin=%s, dconnuri=%s, " "uri=%s, graphicsuri=%s, listenAddress=%s, " "nmigrate_disks=%zu, migrate_disks=%p, nbdPort=%d, " + "nbdSocketPath=%s, " "cookiein=%s, cookieinlen=%d, cookieout=%p, cookieoutlen=%p, " "flags=0x%lx, dname=%s, resource=%lu, v3proto=%d", driver, conn, vm, NULLSTR(xmlin), NULLSTR(dconnuri), NULLSTR(uri), NULLSTR(graphicsuri), NULLSTR(listenAddress), - nmigrate_disks, migrate_disks, nbdPort, + nmigrate_disks, migrate_disks, nbdPort, NULLSTR(nbdSocketPath), NULLSTR(cookiein), cookieinlen, cookieout, cookieoutlen, flags, NULLSTR(dname), resource, v3proto); @@ -4859,7 +4909,7 @@ qemuMigrationSrcPerform(virQEMUDriverPtr driver, return qemuMigrationSrcPerformJob(driver, conn, vm, xmlin, persist_xml, dconnuri, uri, graphicsuri, listenAddress, nmigrate_disks, migrate_disks, nbdPort, - migParams, + nbdSocketPath, migParams, cookiein, cookieinlen, cookieout, cookieoutlen, flags, dname, resource, v3proto); @@ -4877,12 +4927,12 @@ qemuMigrationSrcPerform(virQEMUDriverPtr driver, migParams, cookiein, cookieinlen, cookieout, cookieoutlen, - flags, resource); + flags, resource, nbdSocketPath); } else { return qemuMigrationSrcPerformJob(driver, conn, vm, xmlin, persist_xml, NULL, uri, graphicsuri, listenAddress, nmigrate_disks, migrate_disks, nbdPort, - migParams, + nbdSocketPath, migParams, cookiein, cookieinlen, cookieout, cookieoutlen, flags, dname, resource, v3proto); diff --git a/src/qemu/qemu_migration.h b/src/qemu/qemu_migration.h index b6f88d3fd9ea..f195bda3e367 100644 --- a/src/qemu/qemu_migration.h +++ b/src/qemu/qemu_migration.h @@ -84,6 +84,7 @@ VIR_MIGRATE_PARAM_BANDWIDTH_POSTCOPY, VIR_TYPED_PARAM_ULLONG, \ VIR_MIGRATE_PARAM_PARALLEL_CONNECTIONS, VIR_TYPED_PARAM_INT, \ VIR_MIGRATE_PARAM_TLS_DESTINATION, VIR_TYPED_PARAM_STRING, \ + VIR_MIGRATE_PARAM_DISKS_SOCKET, VIR_TYPED_PARAM_STRING, \ NULL @@ -149,6 +150,7 @@ qemuMigrationDstPrepareDirect(virQEMUDriverPtr driver, size_t nmigrate_disks, const char **migrate_disks, int nbdPort, + const char *nbdSocketPath, qemuMigrationParamsPtr migParams, unsigned long flags); @@ -165,6 +167,7 @@ qemuMigrationSrcPerform(virQEMUDriverPtr driver, size_t nmigrate_disks, const char **migrate_disks, int nbdPort, + const char *nbdSocketPath, qemuMigrationParamsPtr migParams, const char *cookiein, int cookieinlen, diff --git a/src/qemu/qemu_migration_cookie.c b/src/qemu/qemu_migration_cookie.c index 81b557e0a8b4..23511bed67d5 100644 --- a/src/qemu/qemu_migration_cookie.c +++ b/src/qemu/qemu_migration_cookie.c @@ -91,6 +91,8 @@ qemuMigrationCookieNBDFree(qemuMigrationCookieNBDPtr nbd) while (nbd->ndisks) VIR_FREE(nbd->disks[--nbd->ndisks].target); VIR_FREE(nbd->disks); + + VIR_FREE(nbd->socketPath); VIR_FREE(nbd); } @@ -464,7 +466,10 @@ qemuMigrationCookieAddNBD(qemuMigrationCookiePtr mig, mig->nbd = g_new0(qemuMigrationCookieNBD, 1); - mig->nbd->port = priv->nbdPort; + if (priv->nbdSocketPath) + mig->nbd->socketPath = priv->nbdSocketPath; + else + mig->nbd->port = priv->nbdPort; mig->flags |= QEMU_MIGRATION_COOKIE_NBD; if (vm->def->ndisks == 0) @@ -988,12 +993,15 @@ qemuMigrationCookieNBDXMLParse(xmlXPathContextPtr ctxt) if (VIR_ALLOC(ret) < 0) goto error; - port = virXPathString("string(./nbd/@port)", ctxt); - if (port && virStrToLong_i(port, NULL, 10, &ret->port) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Malformed nbd port '%s'"), - port); - goto error; + ret->socketPath = virXPathString("string(./nbd/@socketPath)", ctxt); + if (!ret->socketPath) { + port = virXPathString("string(./nbd/@port)", ctxt); + if (port && virStrToLong_i(port, NULL, 10, &ret->port) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Malformed nbd port '%s'"), + port); + goto error; + } } /* Now check if source sent a list of disks to prealloc. We might be diff --git a/src/qemu/qemu_migration_cookie.h b/src/qemu/qemu_migration_cookie.h index 95e7edb89914..90a320a69c0c 100644 --- a/src/qemu/qemu_migration_cookie.h +++ b/src/qemu/qemu_migration_cookie.h @@ -93,6 +93,7 @@ typedef struct _qemuMigrationCookieNBD qemuMigrationCookieNBD; typedef qemuMigrationCookieNBD *qemuMigrationCookieNBDPtr; struct _qemuMigrationCookieNBD { int port; /* on which port does NBD server listen for incoming data */ + char *socketPath; size_t ndisks; /* Number of items in @disk array */ struct qemuMigrationCookieNBDDisk *disks; diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index c606900a9268..20b8ab9837fd 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -10639,6 +10639,10 @@ static const vshCmdOptDef opts_migrate[] = { .type = VSH_OT_INT, .help = N_("port to use by target server for incoming disks migration") }, + {.name = "disks-socket", + .type = VSH_OT_STRING, + .help = N_("UNIX socket path to use for disks migration") + }, {.name = "comp-methods", .type = VSH_OT_STRING, .help = N_("comma separated list of compression methods to be used") @@ -10758,6 +10762,14 @@ doMigrate(void *opaque) VIR_MIGRATE_PARAM_DISKS_PORT, intOpt) < 0) goto save_error; + if (vshCommandOptStringReq(ctl, cmd, "disks-socket", &opt) < 0) + goto out; + if (opt && + virTypedParamsAddString(&params, &nparams, &maxparams, + VIR_MIGRATE_PARAM_DISKS_SOCKET, + opt) < 0) + goto save_error; + if (vshCommandOptStringReq(ctl, cmd, "dname", &opt) < 0) goto out; if (opt && -- 2.28.0

On Tue, Aug 25, 2020 at 07:47:12AM +0200, Martin Kletzander wrote: For the primary migration connection we have a proper URI, so we can support new schemes without adding new parameters for each one. For the NBD connetion we have the "disk port" parameter only, not a full URI. Adding "disks socket" makes this design mistake worse. IMHO we should add a "disks uri" accepting the exact same syntax as the migration URI. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

On Tue, Aug 25, 2020 at 06:01:28PM +0200, Jiri Denemark wrote: I wanted to do it that way, but I did not want to be arguing for that. So here's what I thought others would use to argue against the URI: If we allow URI it can then be used for non-socket connections as well, but in that case we might need different IP address for source and destination, the same reason we have listenAddress for. I guess my counter-argument would be that we already have the listenAddress for that. That might have been countered by saying that someone might want to migrate the disk over different data path. At that point I might say to forward it yourself using a proxy. I have few more following arguments in my head. I'm very much fine with adding URI instead, but I am not going to be arguing for either. yeah, well, qemu should accept both nbd+unix and unix: I think. I'll see what I can do.

On Tue, Aug 25, 2020 at 07:47:13 +0200, Martin Kletzander wrote: ... I'd call this function virURICheckUnixSocket or similar as that's what it's actually doing. It doesn't really care whether the socket is connected to a proxy or not. Reviewed-by: Jiri Denemark <jdenemar(a)redhat.com&gt;

On Tue, Aug 25, 2020 at 07:47:14 +0200, Martin Kletzander wrote: This hunk looks like it should go into the previous patch 7/9 (peer2peer migration: allow connecting to local sockets). s/scenarion/scenario/ or s/a // and s/scenarion/scenarios/ ... Is there a reason for not supporting TLS and multi-fd with unix sockets? patch, though): introduce MIGRATION_DEST_SOCKET in addition to MIGRATION_DEST_CONNECT_SOCKET and use it to instruct QEMU to connect using unix: URI (unless this support was removed from QEMU). Reviewed-by: Jiri Denemark <jdenemar(a)redhat.com&gt;

On Tue, Aug 25, 2020 at 07:47:15 +0200, Martin Kletzander wrote: Reviewed-by: Jiri Denemark <jdenemar(a)redhat.com&gt; But see additional notes from Daniel (and me) which invalidate my Reviewed-by tags for patches 6 and 8.