[PATCH 0/4] migration: Sanitize handling of 'migrate_disks' parameter
Improve error reporting when users select disk's to migrate but don't enable non-shared storage migration. Peter Krempa (4): qemuMigrationSrcBeginPhase: Properly report error when non-shared storage migration is requested over tunnel qemuMigrationSrcBeginPhase: Require storage migration when 'migrate_disks' parameter is specified virsh: doMigrate: Rework virsh option to migration flag conversion virsh: doMigrate: Require --copy-storage-(all|inc) with --migrate-disks src/qemu/qemu_migration.c | 7 +++ tools/virsh-domain.c | 113 ++++++++++++++++---------------------- 2 files changed, 54 insertions(+), 66 deletions(-) -- 2.41.0
When VIR_MIGRATE_TUNNELLED is used without VIR_MIGRATE_NON_SHARED_DISK/VIR_MIGRATE_NON_SHARED_INC an error was reported without actually returning failure. This was caused by a refactor which dropped many error paths. Fixes: 6111b235224 Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_migration.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index ed41a03851..a3fe6be4e9 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -2617,6 +2617,7 @@ qemuMigrationSrcBeginPhase(virQEMUDriver *driver, if (flags & VIR_MIGRATE_TUNNELLED) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", _("migration of non-shared storage is not supported with tunnelled migration and this QEMU")); + return NULL; } if (nmigrate_disks) { -- 2.41.0
On a Wednesday in 2023, Peter Krempa wrote:
When VIR_MIGRATE_TUNNELLED is used without VIR_MIGRATE_NON_SHARED_DISK/VIR_MIGRATE_NON_SHARED_INC an error was reported without actually returning failure.
This was caused by a refactor which dropped many error paths.
Fixes: 6111b235224 Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_migration.c | 1 + 1 file changed, 1 insertion(+)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
If a user passes a list of disks to migrate but don't actually use 'VIR_MIGRATE_NON_SHARED_DISK' or 'VIR_MIGRATE_NON_SHARED_INC' flags the parameter would be simply ignored without informing the user of the error. Add a proper error in such case. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_migration.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index a3fe6be4e9..9057590c94 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -2644,6 +2644,12 @@ qemuMigrationSrcBeginPhase(virQEMUDriver *driver, migrate_disks, nmigrate_disks)) cookieFlags |= QEMU_MIGRATION_COOKIE_NBD; + } else { + if (nmigrate_disks > 0) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("use of the 'VIR_MIGRATE_PARAM_MIGRATE_DISKS' requires use of 'VIR_MIGRATE_NON_SHARED_DISK' or 'VIR_MIGRATE_NON_SHARED_INC' flag")); + return NULL; + } } if (virDomainDefHasMemoryHotplug(vm->def) || -- 2.41.0
On a Wednesday in 2023, Peter Krempa wrote:
If a user passes a list of disks to migrate but don't actually use 'VIR_MIGRATE_NON_SHARED_DISK' or 'VIR_MIGRATE_NON_SHARED_INC' flags the parameter would be simply ignored without informing the user of the error.
Add a proper error in such case.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_migration.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index a3fe6be4e9..9057590c94 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -2644,6 +2644,12 @@ qemuMigrationSrcBeginPhase(virQEMUDriver *driver, migrate_disks, nmigrate_disks)) cookieFlags |= QEMU_MIGRATION_COOKIE_NBD; + } else { + if (nmigrate_disks > 0) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("use of the 'VIR_MIGRATE_PARAM_MIGRATE_DISKS' requires use of 'VIR_MIGRATE_NON_SHARED_DISK' or 'VIR_MIGRATE_NON_SHARED_INC' flag"));
d/the/ Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Convert the flags declaratively as in the vast majority of cases it's a simple binary addition if the flag exists. In one instance there was also an additional check, which was moved up after the new code, and the error message was fixed. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/virsh-domain.c | 108 +++++++++++++++++-------------------------- 1 file changed, 42 insertions(+), 66 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index f8758f18a3..84bf62057b 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -11082,6 +11082,11 @@ static const vshCmdOptDef opts_migrate[] = { {.name = NULL} }; +struct doMigrateFlagMapping { + const char *optionname; + unsigned int migflag; +}; + static void doMigrate(void *opaque) { @@ -11099,6 +11104,32 @@ doMigrate(void *opaque) unsigned long long ullOpt = 0; int rv; virConnectPtr dconn = data->dconn; + size_t i; + + static const struct doMigrateFlagMapping flagmap[] = { + { "live", VIR_MIGRATE_LIVE }, + { "p2p", VIR_MIGRATE_PEER2PEER }, + { "tunnelled", VIR_MIGRATE_TUNNELLED }, + { "persistent", VIR_MIGRATE_PERSIST_DEST }, + { "undefinesource", VIR_MIGRATE_UNDEFINE_SOURCE }, + { "copy-storage-all", VIR_MIGRATE_NON_SHARED_DISK }, + { "copy-storage-inc", VIR_MIGRATE_NON_SHARED_INC }, + { "copy-storage-synchronous-writes", VIR_MIGRATE_NON_SHARED_SYNCHRONOUS_WRITES }, + { "change-protection", VIR_MIGRATE_CHANGE_PROTECTION }, + { "unsafe", VIR_MIGRATE_UNSAFE }, + { "compressed", VIR_MIGRATE_COMPRESSED }, + { "auto-converge", VIR_MIGRATE_AUTO_CONVERGE }, + { "rdma-pin-all", VIR_MIGRATE_RDMA_PIN_ALL }, + { "offline", VIR_MIGRATE_OFFLINE }, + { "abort-on-error", VIR_MIGRATE_ABORT_ON_ERROR }, + { "postcopy", VIR_MIGRATE_POSTCOPY }, + { "postcopy-resume", VIR_MIGRATE_POSTCOPY_RESUME }, + { "zerocopy", VIR_MIGRATE_ZEROCOPY }, + { "tls", VIR_MIGRATE_TLS }, + { "parallel", VIR_MIGRATE_PARALLEL }, + { "suspend", VIR_MIGRATE_PAUSED }, + }; + #ifndef WIN32 sigset_t sigmask, oldsigmask; @@ -11108,6 +11139,17 @@ doMigrate(void *opaque) goto out_sig; #endif /* !WIN32 */ + for (i = 0; i < G_N_ELEMENTS(flagmap); i++) { + if (vshCommandOptBool(cmd, flagmap[i].optionname)) + flags |= flagmap[i].migflag; + } + + if (flags & VIR_MIGRATE_NON_SHARED_SYNCHRONOUS_WRITES && + !(flags & (VIR_MIGRATE_NON_SHARED_DISK | VIR_MIGRATE_NON_SHARED_DISK))) { + vshError(ctl, "'--copy-storage-synchronous-writes' requires one of '--copy-storage-all', '--copy-storage-inc'"); + goto out; + } + if (!(dom = virshCommandOptDomain(ctl, cmd, NULL))) goto out; @@ -11325,72 +11367,6 @@ doMigrate(void *opaque) VIR_MIGRATE_PARAM_TLS_DESTINATION, opt) < 0) goto save_error; - if (vshCommandOptBool(cmd, "live")) - flags |= VIR_MIGRATE_LIVE; - if (vshCommandOptBool(cmd, "p2p")) - flags |= VIR_MIGRATE_PEER2PEER; - if (vshCommandOptBool(cmd, "tunnelled")) - flags |= VIR_MIGRATE_TUNNELLED; - - if (vshCommandOptBool(cmd, "persistent")) - flags |= VIR_MIGRATE_PERSIST_DEST; - if (vshCommandOptBool(cmd, "undefinesource")) - flags |= VIR_MIGRATE_UNDEFINE_SOURCE; - - if (vshCommandOptBool(cmd, "suspend")) - flags |= VIR_MIGRATE_PAUSED; - - if (vshCommandOptBool(cmd, "copy-storage-all")) - flags |= VIR_MIGRATE_NON_SHARED_DISK; - - if (vshCommandOptBool(cmd, "copy-storage-inc")) - flags |= VIR_MIGRATE_NON_SHARED_INC; - - if (vshCommandOptBool(cmd, "copy-storage-synchronous-writes")) { - if (!(flags & VIR_MIGRATE_NON_SHARED_DISK) && - !(flags & VIR_MIGRATE_NON_SHARED_INC)) { - vshError(ctl, "'--copy-storage-synchronous-writes' requires one of '--copy-storage-all', 'copy-storage-inc'"); - goto out; - } - flags |= VIR_MIGRATE_NON_SHARED_SYNCHRONOUS_WRITES; - } - - if (vshCommandOptBool(cmd, "change-protection")) - flags |= VIR_MIGRATE_CHANGE_PROTECTION; - - if (vshCommandOptBool(cmd, "unsafe")) - flags |= VIR_MIGRATE_UNSAFE; - - if (vshCommandOptBool(cmd, "compressed")) - flags |= VIR_MIGRATE_COMPRESSED; - - if (vshCommandOptBool(cmd, "auto-converge")) - flags |= VIR_MIGRATE_AUTO_CONVERGE; - - if (vshCommandOptBool(cmd, "rdma-pin-all")) - flags |= VIR_MIGRATE_RDMA_PIN_ALL; - - if (vshCommandOptBool(cmd, "offline")) - flags |= VIR_MIGRATE_OFFLINE; - - if (vshCommandOptBool(cmd, "abort-on-error")) - flags |= VIR_MIGRATE_ABORT_ON_ERROR; - - if (vshCommandOptBool(cmd, "postcopy")) - flags |= VIR_MIGRATE_POSTCOPY; - - if (vshCommandOptBool(cmd, "postcopy-resume")) - flags |= VIR_MIGRATE_POSTCOPY_RESUME; - - if (vshCommandOptBool(cmd, "zerocopy")) - flags |= VIR_MIGRATE_ZEROCOPY; - - if (vshCommandOptBool(cmd, "tls")) - flags |= VIR_MIGRATE_TLS; - - if (vshCommandOptBool(cmd, "parallel")) - flags |= VIR_MIGRATE_PARALLEL; - if (flags & VIR_MIGRATE_PEER2PEER || vshCommandOptBool(cmd, "direct")) { if (virDomainMigrateToURI3(dom, desturi, params, nparams, flags) == 0) data->ret = 0; -- 2.41.0
On a Wednesday in 2023, Peter Krempa wrote:
Convert the flags declaratively as in the vast majority of cases it's a simple binary addition if the flag exists.
In one instance there was also an additional check, which was moved up after the new code, and the error message was fixed.
I don't see the difference between the old and new messages.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/virsh-domain.c | 108 +++++++++++++++++-------------------------- 1 file changed, 42 insertions(+), 66 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Users need to enable non-shared-storage migration, otherwise the disks specified via '--migrate-disks' will be ignored. Add an error message to inform the users of their wrong config. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/virsh-domain.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 84bf62057b..e0776c991f 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -11204,6 +11204,11 @@ doMigrate(void *opaque) if (opt) { g_autofree char **val = NULL; + if (!(flags & (VIR_MIGRATE_NON_SHARED_DISK | VIR_MIGRATE_NON_SHARED_INC))) { + vshError(ctl, "'--migrate-disks' requires one of '--copy-storage-all', '--copy-storage-inc'"); + goto out; + } + val = g_strsplit(opt, ",", 0); if (virTypedParamsAddStringList(¶ms, -- 2.41.0
On a Wednesday in 2023, Peter Krempa wrote:
Users need to enable non-shared-storage migration, otherwise the disks specified via '--migrate-disks' will be ignored.
Add an error message to inform the users of their wrong config.
Wouldn't it be friendlier to imply, for example, --copy-storage-all?
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/virsh-domain.c | 5 +++++ 1 file changed, 5 insertions(+)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (2)
-
Ján Tomko -
Peter Krempa