[libvirt PATCH 0/3] Few fixes for migration over UNIX socket
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1638889 Martin Kletzander (3): qemu: Fix possible segfault when migrating disks docs: Slightly alter disks-uri description in virsh man qemu: Extra check for NBD URI being specified docs/manpages/virsh.rst | 22 +++++++++++----------- src/qemu/qemu_driver.c | 21 +++++++++++++++++++++ src/qemu/qemu_migration.c | 5 +++++ 3 files changed, 37 insertions(+), 11 deletions(-) -- 2.29.2
Users can provide URI without a schema. https://bugzilla.redhat.com/show_bug.cgi?id=1638889 Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_migration.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index fcb33d0364a1..90b0ec95e35a 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -408,6 +408,11 @@ qemuMigrationDstStartNBDServer(virQEMUDriverPtr driver, if (!uri) return -1; + if (!uri->scheme) { + virReportError(VIR_ERR_INVALID_ARG, _("No URI scheme specified: %s"), nbdURI); + return -1; + } + if (STREQ(uri->scheme, "tcp")) { server.transport = VIR_STORAGE_NET_HOST_TRANS_TCP; if (!uri->server || STREQ(uri->server, "")) { -- 2.29.2
It's more accurate this way. https://bugzilla.redhat.com/show_bug.cgi?id=1638889 Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- docs/manpages/virsh.rst | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst index aa3a0095fe3d..4a1500e68628 100644 --- a/docs/manpages/virsh.rst +++ b/docs/manpages/virsh.rst @@ -3346,17 +3346,17 @@ bind to for incoming disks traffic. Currently it is supported only by QEMU. Optional *disks-uri* can also be specified (mutually exclusive with *disks-port*) to specify what the remote hypervisor should bind/connect to when migrating disks. This can be *tcp://address:port* to specify a listen address -(which overrides *--listen-address* for the disk migration) and a port or -*unix:///path/to/socket* 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. If you are using SELinux -(at least on the source host) you need to make sure the socket on the source is -accessible to libvirtd/QEMU for connection. Libvirt cannot change the context -of the existing socket because it is different from the file representation of -the socket and the context is chosen by its creator (usually by using -*setsockcreatecon{,_raw}()* functions). +(which overrides *--migrate-uri* and *--listen-address* for the disk migration) +and a port or *unix:///path/to/socket* 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. If you are +using SELinux (at least on the source host) you need to make sure the socket on +the source is accessible to libvirtd/QEMU for connection. Libvirt cannot change +the context of the existing socket because it is different from the file +representation of the socket and the context is chosen by its creator (usually +by using *setsockcreatecon{,_raw}()* functions). migrate-compcache -- 2.29.2
It must be used when migration URI uses `unix:` transport because otherwise we cannot just guess where to connect for disk migration. https://bugzilla.redhat.com/show_bug.cgi?id=1638889 Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_driver.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 5f0fb0a55fee..9caaa0723720 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -11503,6 +11503,16 @@ qemuDomainMigratePrepare3Params(virConnectPtr dconn, QEMU_MIGRATION_DESTINATION))) return -1; + if (flags & (VIR_MIGRATE_NON_SHARED_DISK | VIR_MIGRATE_NON_SHARED_INC) || + nmigrate_disks > 0) { + if (uri_in && STRPREFIX(uri_in, "unix:") && !nbdURI) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("NBD URI must be supplied when " + "migration URI uses UNIX transport method")); + return -1; + } + } + if (nbdURI && nbdPort) { virReportError(VIR_ERR_INVALID_ARG, "%s", _("Both port and URI requested for disk migration " @@ -11743,6 +11753,7 @@ qemuDomainMigratePerform3Params(virDomainPtr dom, &persist_xml) < 0) goto cleanup; + if (nbdURI && nbdPort) { virReportError(VIR_ERR_INVALID_ARG, "%s", _("Both port and URI requested for disk migration " @@ -11766,6 +11777,16 @@ qemuDomainMigratePerform3Params(virDomainPtr dom, if (nmigrate_disks < 0) goto cleanup; + if (flags & (VIR_MIGRATE_NON_SHARED_DISK | VIR_MIGRATE_NON_SHARED_INC) || + nmigrate_disks > 0) { + if (uri && STRPREFIX(uri, "unix:") && !nbdURI) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("NBD URI must be supplied when " + "migration URI uses UNIX transport method")); + return -1; + } + } + if (!(migParams = qemuMigrationParamsFromFlags(params, nparams, flags, QEMU_MIGRATION_SOURCE))) goto cleanup; -- 2.29.2
On Wed, Dec 16, 2020 at 12:19:28 +0100, Martin Kletzander wrote:
It must be used when migration URI uses `unix:` transport because otherwise we cannot just guess where to connect for disk migration.
https://bugzilla.redhat.com/show_bug.cgi?id=1638889
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_driver.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 5f0fb0a55fee..9caaa0723720 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -11503,6 +11503,16 @@ qemuDomainMigratePrepare3Params(virConnectPtr dconn, QEMU_MIGRATION_DESTINATION))) return -1;
+ if (flags & (VIR_MIGRATE_NON_SHARED_DISK | VIR_MIGRATE_NON_SHARED_INC) || + nmigrate_disks > 0) {
Is this to check that NBD is used? I think that's not enough as VIR_MIGRATE_NON_SHARED_DISK/INC doesn't necessarily guarantee that NBD is used. It also depends on whether TUNELLED and other possible flags are in the mix too. We might need a helper function for that. Also 'nmigrate_disks' AFAIK requires that VIR_MIGRATE_NON_SHARED_DISK is used so it's redundant here.
+ if (uri_in && STRPREFIX(uri_in, "unix:") && !nbdURI) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("NBD URI must be supplied when " + "migration URI uses UNIX transport method"));
Please don't split error messages it's hard to grep for them if they are split.
+ return -1; + } + } + if (nbdURI && nbdPort) { virReportError(VIR_ERR_INVALID_ARG, "%s", _("Both port and URI requested for disk migration " @@ -11743,6 +11753,7 @@ qemuDomainMigratePerform3Params(virDomainPtr dom, &persist_xml) < 0) goto cleanup;
+
Spurious newline addition.
if (nbdURI && nbdPort) { virReportError(VIR_ERR_INVALID_ARG, "%s", _("Both port and URI requested for disk migration " @@ -11766,6 +11777,16 @@ qemuDomainMigratePerform3Params(virDomainPtr dom, if (nmigrate_disks < 0) goto cleanup;
+ if (flags & (VIR_MIGRATE_NON_SHARED_DISK | VIR_MIGRATE_NON_SHARED_INC) || + nmigrate_disks > 0) { + if (uri && STRPREFIX(uri, "unix:") && !nbdURI) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("NBD URI must be supplied when " + "migration URI uses UNIX transport method")); + return -1; + } + } + if (!(migParams = qemuMigrationParamsFromFlags(params, nparams, flags, QEMU_MIGRATION_SOURCE))) goto cleanup; -- 2.29.2
On Thu, Dec 17, 2020 at 11:15:11AM +0100, Peter Krempa wrote:
On Wed, Dec 16, 2020 at 12:19:28 +0100, Martin Kletzander wrote:
It must be used when migration URI uses `unix:` transport because otherwise we cannot just guess where to connect for disk migration.
https://bugzilla.redhat.com/show_bug.cgi?id=1638889
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_driver.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 5f0fb0a55fee..9caaa0723720 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -11503,6 +11503,16 @@ qemuDomainMigratePrepare3Params(virConnectPtr dconn, QEMU_MIGRATION_DESTINATION))) return -1;
+ if (flags & (VIR_MIGRATE_NON_SHARED_DISK | VIR_MIGRATE_NON_SHARED_INC) || + nmigrate_disks > 0) {
Is this to check that NBD is used? I think that's not enough as VIR_MIGRATE_NON_SHARED_DISK/INC doesn't necessarily guarantee that NBD is used. It also depends on whether TUNELLED and other possible flags are in the mix too. We might need a helper function for that.
Thanks for the review. It would be nicer to have it extracted into a separate function for that, I agree. Tunnelled migration would fortunately not work with `--migrate-uri`, so we should be safe there (although the logic lends itself for slightly more readable condition). I went through both QEMU_MIGRATION_FLAGS and QEMU_MIGRATION_PARAMETERS and I believe these are the only cases that might cause an issue. I'll move the condition to a separate function and I'll gladly accept any suggestions for improvements.
Also 'nmigrate_disks' AFAIK requires that VIR_MIGRATE_NON_SHARED_DISK is used so it's redundant here.
I only did it based on what virsh does and `virsh migrate --migrate-disks vda ...` without any `--copy-storage-*` option worked for me and it looked like virsh does not apply additional logic on top of this particular set of options.
+ if (uri_in && STRPREFIX(uri_in, "unix:") && !nbdURI) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("NBD URI must be supplied when " + "migration URI uses UNIX transport method"));
Please don't split error messages it's hard to grep for them if they are split.
Are we finally enforcing this? I'd be glad to, I guess I'm stuck in the old ways ;)
+ return -1; + } + } + if (nbdURI && nbdPort) { virReportError(VIR_ERR_INVALID_ARG, "%s", _("Both port and URI requested for disk migration " @@ -11743,6 +11753,7 @@ qemuDomainMigratePerform3Params(virDomainPtr dom, &persist_xml) < 0) goto cleanup;
+
Spurious newline addition.
if (nbdURI && nbdPort) { virReportError(VIR_ERR_INVALID_ARG, "%s", _("Both port and URI requested for disk migration " @@ -11766,6 +11777,16 @@ qemuDomainMigratePerform3Params(virDomainPtr dom, if (nmigrate_disks < 0) goto cleanup;
+ if (flags & (VIR_MIGRATE_NON_SHARED_DISK | VIR_MIGRATE_NON_SHARED_INC) || + nmigrate_disks > 0) { + if (uri && STRPREFIX(uri, "unix:") && !nbdURI) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("NBD URI must be supplied when " + "migration URI uses UNIX transport method")); + return -1; + } + } + if (!(migParams = qemuMigrationParamsFromFlags(params, nparams, flags, QEMU_MIGRATION_SOURCE))) goto cleanup; -- 2.29.2
On 12/16/20 12:19 PM, Martin Kletzander wrote:
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1638889
Martin Kletzander (3): qemu: Fix possible segfault when migrating disks docs: Slightly alter disks-uri description in virsh man qemu: Extra check for NBD URI being specified
docs/manpages/virsh.rst | 22 +++++++++++----------- src/qemu/qemu_driver.c | 21 +++++++++++++++++++++ src/qemu/qemu_migration.c | 5 +++++ 3 files changed, 37 insertions(+), 11 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
participants (3)
-
Martin Kletzander -
Michal Privoznik -
Peter Krempa