:p
atchew
Login
The following changes since commit 8dd5bceb2f9cc58481e9d22355a8d998220896de: Open 11.0 development tree (2025-12-23 14:45:38 +1100) are available in the Git repository at: https://gitlab.com/peterx/qemu.git tags/next-pull-request for you to fetch changes up to bcb411a005fdf39b76e99c14f3618c7b70f7774d: MAINTAINERS: remove David from "Memory API" section (2025-12-23 09:27:02 -0500) ---------------------------------------------------------------- memory + migration pull - Pawel's misc fixes to mapped-ram when x-ignore-share is enabled - Peter's series to cleanup migration error reporting - Peter's added debug property for x-ignore-shared - Part of Fabiano's series on unify capabilities and parameters - Chuang's log_clear optimization on unaligned ramblocks - Maintainer file update from Ben (CPR++) and David (MemoryAPI-) ---------------------------------------------------------------- Ben Chaney (1): MAINTAINERS: Update reviewers for CPR Chuang Xu (1): migration: merge fragmented clear_dirty ioctls David Hildenbrand (Red Hat) (1): MAINTAINERS: remove David from "Memory API" section Fabiano Rosas (15): migration: Fix leak of block_bitmap_mapping migration: Fix leak of cpr_exec_command migration: Add a qdev property for StrOrNull tests/qtest/migration: Add a NULL parameters test for TLS migration: Normalize tls arguments migration: Remove MigrateSetParameters qapi/migration: Don't document MigrationParameter migration: Run a post update routine after setting parameters migration: Add a flag to track block-bitmap-mapping input migration: Remove checks for s->parameters has_* fields migration: Do away with usage of QERR_INVALID_PARAMETER_VALUE migration: Extract code to mark all parameters as present migration: Use QAPI_CLONE_MEMBERS in query_migrate_parameters tests/qtest/migration: Pass MigrateCommon into test functions tests/qtest/migration: Pass MigrateStart into cancel tests Markus Armbruster (1): error: Poison g_autoptr(Error) to prevent its use Pawel Zmarzly (4): migration: fix parsing snapshots with x-ignore-shared flag migration: Fix writing mapped_ram + ignore_shared snapshots scripts/analyze-migration: Rename RAM_SAVE_FLAG_COMPRESS to RAM_SAVE_FLAG_ZERO scripts/analyze-migration: Support mapped-ram snapshot format Peter Xu (8): migration: Use explicit error_free() instead of g_autoptr Revert "error: define g_autoptr() cleanup function for the Error type" migration: Make migration_connect_set_error() own the error migration: Make multifd_send_set_error() own the error migration: Make multifd_recv_terminate_threads() own the error migration: Replace migrate_set_error() with migrate_error_propagate() migration: Use error_propagate() in migrate_error_propagate() migration/options: Add x-ignore-shared MAINTAINERS | 3 +- qapi/migration.json | 395 +--------------- qapi/pragma.json | 1 + include/qapi/error.h | 20 +- include/system/physmem.h | 7 +- migration/migration.h | 9 +- migration/options.h | 1 + tests/qtest/migration/migration-qmp.h | 1 + tests/qtest/migration/migration-util.h | 8 +- accel/tcg/cputlb.c | 5 +- migration/channel.c | 1 - migration/cpr-exec.c | 5 +- migration/migration-hmp-cmds.c | 13 +- migration/migration.c | 59 ++- migration/multifd-device-state.c | 6 +- migration/multifd.c | 30 +- migration/options.c | 525 ++++++++++++---------- migration/page_cache.c | 6 +- migration/postcopy-ram.c | 5 +- migration/ram.c | 102 +++-- migration/savevm.c | 18 +- migration/tls.c | 2 +- system/memory.c | 2 +- system/physmem.c | 48 +- tests/qtest/migration/compression-tests.c | 131 +++--- tests/qtest/migration/cpr-tests.c | 75 ++-- tests/qtest/migration/file-tests.c | 202 ++++----- tests/qtest/migration/migration-qmp.c | 9 + tests/qtest/migration/migration-util.c | 26 +- tests/qtest/migration/misc-tests.c | 112 ++--- tests/qtest/migration/postcopy-tests.c | 82 ++-- tests/qtest/migration/precopy-tests.c | 373 +++++++-------- tests/qtest/migration/tls-tests.c | 499 ++++++++++---------- scripts/analyze-migration.py | 72 ++- 34 files changed, 1282 insertions(+), 1571 deletions(-) -- 2.50.1
From: Pawel Zmarzly <pzmarzly0@gmail.com> Snapshots made with mapped-ram and x-ignore-shared flags are not parsed properly. The ignore-shared feature adds and extra field in the stream, which needs to be consumed on the destination side. Even though mapped-ram has a fixed header format, the ignore-shared is part of the "generic" stream infomation so the mapped-ram code is currently skipping that be64 read which incorrectly offsets every subsequent read from the stream. The current ignore-shared handling can simply be moved earlier in the code to encompass mapped-ram as well since the ignore-shared doubleword is the first one read when parsing the ramblock section of the stream. Co-authored-by: Peter Xu <peterx@redhat.com> Signed-off-by: Pawel Zmarzly <pzmarzly0@gmail.com> Reviewed-by: Fabiano Rosas <farosas@suse.de> Link: https://lore.kernel.org/r/20251126121233.542473-1-pzmarzly0@gmail.com [peterx: enhance commit log per fabiano] Signed-off-by: Peter Xu <peterx@redhat.com> --- migration/ram.c | 21 +++++++++++---------- tests/qtest/migration/file-tests.c | 18 ++++++++++++++++++ 2 files changed, 29 insertions(+), 10 deletions(-) diff --git a/migration/ram.c b/migration/ram.c index XXXXXXX..XXXXXXX 100644 --- a/migration/ram.c +++ b/migration/ram.c @@ -XXX,XX +XXX,XX @@ static int parse_ramblock(QEMUFile *f, RAMBlock *block, ram_addr_t length) assert(block); + if (migrate_ignore_shared()) { + hwaddr addr = qemu_get_be64(f); + if (migrate_ram_is_ignored(block) && + block->mr->addr != addr) { + error_report("Mismatched GPAs for block %s " + "%" PRId64 "!= %" PRId64, block->idstr, + (uint64_t)addr, (uint64_t)block->mr->addr); + return -EINVAL; + } + } + if (migrate_mapped_ram()) { parse_ramblock_mapped_ram(f, block, length, &local_err); if (local_err) { @@ -XXX,XX +XXX,XX @@ static int parse_ramblock(QEMUFile *f, RAMBlock *block, ram_addr_t length) return -EINVAL; } } - if (migrate_ignore_shared()) { - hwaddr addr = qemu_get_be64(f); - if (migrate_ram_is_ignored(block) && - block->mr->addr != addr) { - error_report("Mismatched GPAs for block %s " - "%" PRId64 "!= %" PRId64, block->idstr, - (uint64_t)addr, (uint64_t)block->mr->addr); - return -EINVAL; - } - } ret = rdma_block_notification_handle(f, block->idstr); if (ret < 0) { qemu_file_set_error(f, ret); diff --git a/tests/qtest/migration/file-tests.c b/tests/qtest/migration/file-tests.c index XXXXXXX..XXXXXXX 100644 --- a/tests/qtest/migration/file-tests.c +++ b/tests/qtest/migration/file-tests.c @@ -XXX,XX +XXX,XX @@ static void migration_test_add_file_smoke(MigrationTestEnv *env) test_multifd_file_mapped_ram_dio); } +static void test_precopy_file_mapped_ram_ignore_shared(void) +{ + g_autofree char *uri = g_strdup_printf("file:%s/%s", tmpfs, + FILE_TEST_FILENAME); + MigrateCommon args = { + .connect_uri = uri, + .listen_uri = "defer", + .start = { + .caps[MIGRATION_CAPABILITY_MAPPED_RAM] = true, + .caps[MIGRATION_CAPABILITY_X_IGNORE_SHARED] = true, + }, + }; + + test_file_common(&args, true); +} + void migration_test_add_file(MigrationTestEnv *env) { tmpfs = env->tmpfs; @@ -XXX,XX +XXX,XX @@ void migration_test_add_file(MigrationTestEnv *env) migration_test_add("/migration/multifd/file/mapped-ram", test_multifd_file_mapped_ram); + migration_test_add("/migration/multifd/file/mapped-ram/ignore-shared", + test_precopy_file_mapped_ram_ignore_shared); migration_test_add("/migration/multifd/file/mapped-ram/live", test_multifd_file_mapped_ram_live); -- 2.50.1
From: Pawel Zmarzly <pzmarzly0@gmail.com> Currently if you set these flags and have any shared memory object, saving a snapshot will fail with: Failed to write bitmap to file: Unable to write to file: Bad address We need to skip writing RAMBlocks that are backed by shared objects. Also, we should mark these RAMBlocks as skipped, so the snapshot format stays readable to tools that later don't know QEMU's command line (for example scripts/analyze-migration.py). I used bitmap_offset=0 pages_offset=0 for this. This minor change to snapshot format should be safe, as offset=0 should not have ever been possible. Signed-off-by: Pawel Zmarzly <pzmarzly0@gmail.com> Link: https://lore.kernel.org/r/20251126154734.940066-1-pzmarzly0@gmail.com Signed-off-by: Peter Xu <peterx@redhat.com> --- migration/ram.c | 53 ++++++++++++++++++++++++++++++++----------------- 1 file changed, 35 insertions(+), 18 deletions(-) diff --git a/migration/ram.c b/migration/ram.c index XXXXXXX..XXXXXXX 100644 --- a/migration/ram.c +++ b/migration/ram.c @@ -XXX,XX +XXX,XX @@ static void mapped_ram_setup_ramblock(QEMUFile *file, RAMBlock *block) header = g_new0(MappedRamHeader, 1); header_size = sizeof(MappedRamHeader); - num_pages = block->used_length >> TARGET_PAGE_BITS; - bitmap_size = BITS_TO_LONGS(num_pages) * sizeof(unsigned long); - - /* - * Save the file offsets of where the bitmap and the pages should - * go as they are written at the end of migration and during the - * iterative phase, respectively. - */ - block->bitmap_offset = qemu_get_offset(file) + header_size; - block->pages_offset = ROUND_UP(block->bitmap_offset + - bitmap_size, - MAPPED_RAM_FILE_OFFSET_ALIGNMENT); - header->version = cpu_to_be32(MAPPED_RAM_HDR_VERSION); header->page_size = cpu_to_be64(TARGET_PAGE_SIZE); - header->bitmap_offset = cpu_to_be64(block->bitmap_offset); - header->pages_offset = cpu_to_be64(block->pages_offset); + + if (migrate_ram_is_ignored(block)) { + header->bitmap_offset = 0; + header->pages_offset = 0; + } else { + num_pages = block->used_length >> TARGET_PAGE_BITS; + bitmap_size = BITS_TO_LONGS(num_pages) * sizeof(unsigned long); + + /* + * Save the file offsets of where the bitmap and the pages should + * go as they are written at the end of migration and during the + * iterative phase, respectively. + */ + block->bitmap_offset = qemu_get_offset(file) + header_size; + block->pages_offset = ROUND_UP(block->bitmap_offset + + bitmap_size, + MAPPED_RAM_FILE_OFFSET_ALIGNMENT); + + header->bitmap_offset = cpu_to_be64(block->bitmap_offset); + header->pages_offset = cpu_to_be64(block->pages_offset); + } qemu_put_buffer(file, (uint8_t *) header, header_size); - /* prepare offset for next ramblock */ - qemu_set_offset(file, block->pages_offset + block->used_length, SEEK_SET); + if (!migrate_ram_is_ignored(block)) { + /* leave space for block data */ + qemu_set_offset(file, block->pages_offset + block->used_length, + SEEK_SET); + } } static bool mapped_ram_read_header(QEMUFile *file, MappedRamHeader *header, @@ -XXX,XX +XXX,XX @@ static int ram_save_setup(QEMUFile *f, void *opaque, Error **errp) if (migrate_ignore_shared()) { qemu_put_be64(f, block->mr->addr); } - if (migrate_mapped_ram()) { mapped_ram_setup_ramblock(f, block); } @@ -XXX,XX +XXX,XX @@ static void ram_save_file_bmap(QEMUFile *f) RAMBlock *block; RAMBLOCK_FOREACH_MIGRATABLE(block) { + if (migrate_ram_is_ignored(block)) { + continue; + } + long num_pages = block->used_length >> TARGET_PAGE_BITS; long bitmap_size = BITS_TO_LONGS(num_pages) * sizeof(unsigned long); @@ -XXX,XX +XXX,XX @@ static void parse_ramblock_mapped_ram(QEMUFile *f, RAMBlock *block, return; } + if (migrate_ignore_shared() && + header.bitmap_offset == 0 && header.pages_offset == 0) { + return; + } + block->pages_offset = header.pages_offset; /* -- 2.50.1
From: Pawel Zmarzly <pzmarzly0@gmail.com> It has been renamed on the C side a few years ago. In modern QEMU versions, fill_byte must be zero. Updating the Python script to make grepping and understanding the code easier. Signed-off-by: Pawel Zmarzly <pzmarzly0@gmail.com> Link: https://lore.kernel.org/r/20251125173007.245607-1-pzmarzly0@gmail.com [peterx: fix over-long line] Signed-off-by: Peter Xu <peterx@redhat.com> --- scripts/analyze-migration.py | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/scripts/analyze-migration.py b/scripts/analyze-migration.py index XXXXXXX..XXXXXXX 100755 --- a/scripts/analyze-migration.py +++ b/scripts/analyze-migration.py @@ -XXX,XX +XXX,XX @@ def close(self): self.file.close() class RamSection(object): - RAM_SAVE_FLAG_COMPRESS = 0x02 + RAM_SAVE_FLAG_ZERO = 0x02 RAM_SAVE_FLAG_MEM_SIZE = 0x04 RAM_SAVE_FLAG_PAGE = 0x08 RAM_SAVE_FLAG_EOS = 0x10 @@ -XXX,XX +XXX,XX @@ def read(self): mr_addr = self.file.read64() flags &= ~self.RAM_SAVE_FLAG_MEM_SIZE - if flags & self.RAM_SAVE_FLAG_COMPRESS: + if flags & self.RAM_SAVE_FLAG_ZERO: if flags & self.RAM_SAVE_FLAG_CONTINUE: flags &= ~self.RAM_SAVE_FLAG_CONTINUE else: self.name = self.file.readstr() - fill_char = self.file.read8() - # The page in question is filled with fill_char now - if self.write_memory and fill_char != 0: - self.files[self.name].seek(addr, os.SEEK_SET) - self.files[self.name].write(chr(fill_char) * self.TARGET_PAGE_SIZE) + _fill_char = self.file.read8() if self.dump_memory: - self.memory['%s (0x%016x)' % (self.name, addr)] = 'Filled with 0x%02x' % fill_char - flags &= ~self.RAM_SAVE_FLAG_COMPRESS + self.memory['%s (0x%016x)' % + (self.name, addr)] = 'Filled with 0x00' + flags &= ~self.RAM_SAVE_FLAG_ZERO elif flags & self.RAM_SAVE_FLAG_PAGE: if flags & self.RAM_SAVE_FLAG_CONTINUE: flags &= ~self.RAM_SAVE_FLAG_CONTINUE -- 2.50.1
From: Pawel Zmarzly <pzmarzly0@gmail.com> The script has not been updated to read mapped-ram snapshots and is currently crashing when trying to read such a file. With this commit, it can now read a snapshot created with: (qemu) migrate_set_capability x-ignore-shared on (qemu) migrate_set_capability mapped-ram on (qemu) migrate -d file:vm.state Signed-off-by: Pawel Zmarzly <pzmarzly0@gmail.com> Link: https://lore.kernel.org/r/20251126155015.941129-1-pzmarzly0@gmail.com [peterx: space fixes, introduce parseMappedRamBlob(), add comments, etc.] Signed-off-by: Peter Xu <peterx@redhat.com> --- scripts/analyze-migration.py | 57 ++++++++++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/scripts/analyze-migration.py b/scripts/analyze-migration.py index XXXXXXX..XXXXXXX 100755 --- a/scripts/analyze-migration.py +++ b/scripts/analyze-migration.py @@ -XXX,XX +XXX,XX @@ import json import os +import math import argparse import collections import struct @@ -XXX,XX +XXX,XX @@ def __init__(self, file, version_id, ramargs, section_key): self.dump_memory = ramargs['dump_memory'] self.write_memory = ramargs['write_memory'] self.ignore_shared = ramargs['ignore_shared'] + self.mapped_ram = ramargs['mapped_ram'] self.sizeinfo = collections.OrderedDict() self.data = collections.OrderedDict() self.data['section sizes'] = self.sizeinfo @@ -XXX,XX +XXX,XX @@ def __str__(self): def getDict(self): return self.data + def parseMappedRamBlob(self, len): + version = self.file.read32() + if version != 1: + raise Exception("Unsupported MappedRamHeader version %s" % version) + + page_size = self.file.read64() + if page_size != self.TARGET_PAGE_SIZE: + raise Exception("Page size mismatch in MappedRamHeader") + + bitmap_offset = self.file.read64() + pages_offset = self.file.read64() + + if self.ignore_shared and bitmap_offset == 0 and pages_offset == 0: + # This is a shared ramblock, x-ignore-share must have been + # enabled, and mapped-ram didn't allocate bitmap or page blob + # for it. + return + + if self.dump_memory or self.write_memory: + num_pages = len // page_size + + self.file.seek(bitmap_offset, os.SEEK_SET) + bitmap_len = int(math.ceil(num_pages / 8)) + bitmap = self.file.readvar(size=bitmap_len) + + self.file.seek(pages_offset, os.SEEK_SET) + for page_num in range(num_pages): + page_addr = page_num * page_size + + is_filled = (bitmap[page_num // 8] >> page_num % 8) & 1 + if is_filled: + data = self.file.readvar(size=self.TARGET_PAGE_SIZE) + if self.write_memory: + self.files[self.name].seek(page_addr, os.SEEK_SET) + self.files[self.name].write(data) + if self.dump_memory: + hexdata = " ".join("{0:02x}".format(c) for c in data) + self.memory['%s (0x%016x)' % + (self.name, page_addr)] = hexdata + else: + self.file.seek(self.TARGET_PAGE_SIZE, os.SEEK_CUR) + if self.write_memory: + self.files[self.name].seek(page_addr, os.SEEK_SET) + self.files[self.name].write( + b'\x00' * self.TARGET_PAGE_SIZE) + if self.dump_memory: + self.memory['%s (0x%016x)' % + (self.name, page_addr)] = 'Filled with 0x00' + + self.file.seek(pages_offset + len, os.SEEK_SET) + def read(self): # Read all RAM sections while True: @@ -XXX,XX +XXX,XX @@ def read(self): self.files[self.name] = f if self.ignore_shared: mr_addr = self.file.read64() + if self.mapped_ram: + self.parseMappedRamBlob(len) flags &= ~self.RAM_SAVE_FLAG_MEM_SIZE if flags & self.RAM_SAVE_FLAG_ZERO: @@ -XXX,XX +XXX,XX @@ def read(self, desc_only = False, dump_memory = False, ramargs['dump_memory'] = dump_memory ramargs['write_memory'] = write_memory ramargs['ignore_shared'] = False + ramargs['mapped_ram'] = False self.section_classes[('ram',0)][1] = ramargs while True: @@ -XXX,XX +XXX,XX @@ def read(self, desc_only = False, dump_memory = False, section = ConfigurationSection(file, config_desc) section.read() ramargs['ignore_shared'] = section.has_capability('x-ignore-shared') + ramargs['mapped_ram'] = section.has_capability('mapped-ram') elif section_type == self.QEMU_VM_SECTION_START or section_type == self.QEMU_VM_SECTION_FULL: section_id = file.read32() name = file.readstr() -- 2.50.1
There're only two use cases of g_autoptr to free Error objects in migration code paths. Due to the nature of how Error should be used (normally ownership will be passed over to Error APIs, like error_report_err), auto-free functions may be error prone on its own. The auto cleanup function was merged without proper review, as pointed out by Dan and Markus: https://lore.kernel.org/r/aSWSLMi6ZhTCS_p2@redhat.com Remove the two use cases so that we can remove the auto cleanup function, hence suggest to not use auto frees for Errors. Suggested-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Markus Armbruster <armbru@redhat.com> Link: https://lore.kernel.org/r/20251201194510.1121221-2-peterx@redhat.com Signed-off-by: Peter Xu <peterx@redhat.com> --- migration/multifd-device-state.c | 3 ++- migration/savevm.c | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/migration/multifd-device-state.c b/migration/multifd-device-state.c index XXXXXXX..XXXXXXX 100644 --- a/migration/multifd-device-state.c +++ b/migration/multifd-device-state.c @@ -XXX,XX +XXX,XX @@ static void multifd_device_state_save_thread_data_free(void *opaque) static int multifd_device_state_save_thread(void *opaque) { SaveCompletePrecopyThreadData *data = opaque; - g_autoptr(Error) local_err = NULL; + Error *local_err = NULL; if (!data->hdlr(data, &local_err)) { MigrationState *s = migrate_get_current(); @@ -XXX,XX +XXX,XX @@ static int multifd_device_state_save_thread(void *opaque) * return we end setting is purely arbitrary. */ migrate_set_error(s, local_err); + error_free(local_err); } return 0; diff --git a/migration/savevm.c b/migration/savevm.c index XXXXXXX..XXXXXXX 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -XXX,XX +XXX,XX @@ static int qemu_loadvm_load_thread(void *thread_opaque) { struct LoadThreadData *data = thread_opaque; MigrationIncomingState *mis = migration_incoming_get_current(); - g_autoptr(Error) local_err = NULL; + Error *local_err = NULL; if (!data->function(data->opaque, &mis->load_threads_abort, &local_err)) { MigrationState *s = migrate_get_current(); @@ -XXX,XX +XXX,XX @@ static int qemu_loadvm_load_thread(void *thread_opaque) * return we end setting is purely arbitrary. */ migrate_set_error(s, local_err); + error_free(local_err); } return 0; -- 2.50.1
This reverts commit 18eb55546a54e443d94a4c49286348176ad4b00a. Due to the nature of how Error should be used (normally ownership will be passed over to Error APIs, like error_report_err), auto-free functions may be error prone on its own. The auto cleanup function was merged without proper review as pointed out by Dan and Markus: https://lore.kernel.org/r/aSWSLMi6ZhTCS_p2@redhat.com Cc: Cédric Le Goater <clg@redhat.com> Acked-by: Maciej S. Szmigiero <mail@maciej.szmigiero.name> Reviewed-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Cédric Le Goater <clg@redhat.com> Acked-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com> Link: https://lore.kernel.org/r/20251201194510.1121221-3-peterx@redhat.com Signed-off-by: Peter Xu <peterx@redhat.com> --- include/qapi/error.h | 2 -- 1 file changed, 2 deletions(-) diff --git a/include/qapi/error.h b/include/qapi/error.h index XXXXXXX..XXXXXXX 100644 --- a/include/qapi/error.h +++ b/include/qapi/error.h @@ -XXX,XX +XXX,XX @@ Error *error_copy(const Error *err); */ void error_free(Error *err); -G_DEFINE_AUTOPTR_CLEANUP_FUNC(Error, error_free) - /* * Convenience function to assert that *@errp is set, then silently free it. */ -- 2.50.1
From: Markus Armbruster <armbru@redhat.com> The previous commit reverted support for g_autoptr(Error). This one should stop it from coming back. Suggested-by: Peter Maydell <peter.maydell@linaro.org> Signed-off-by: Markus Armbruster <armbru@redhat.com> Tested-by: Cédric Le Goater <clg@redhat.com> Reviewed-by: Cédric Le Goater <clg@redhat.com> Link: https://lore.kernel.org/r/20251201194510.1121221-4-peterx@redhat.com Signed-off-by: Peter Xu <peterx@redhat.com> --- include/qapi/error.h | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/include/qapi/error.h b/include/qapi/error.h index XXXXXXX..XXXXXXX 100644 --- a/include/qapi/error.h +++ b/include/qapi/error.h @@ -XXX,XX +XXX,XX @@ Error *error_copy(const Error *err); */ void error_free(Error *err); +/* + * Poison g_autoptr(Error) to prevent its use. + * + * Functions that report or propagate an error take ownership of the + * Error object. Explicit error_free() is needed when you handle an + * error in some other way. This is rare. + * + * g_autoptr(Error) would call error_free() automatically on return. + * To avoid a double-free, we'd have to manually clear the pointer + * every time we propagate or report. + * + * Thus, g_autoptr(Error) would make the rare case easier to get right + * (less prone to leaks), and the common case easier to get wrong + * (more prone to double-free). + */ +extern void +__attribute__((error("Do not use g_autoptr() to declare Error * variables"))) +error_free_poisoned(Error *err); +G_DEFINE_AUTOPTR_CLEANUP_FUNC(Error, error_free_poisoned) + /* * Convenience function to assert that *@errp is set, then silently free it. */ -- 2.50.1
Make migration_connect_set_error() take ownership of the error always. Paving way for making migrate_set_error() to take ownership. When at it, renaming it to migration_connect_error_propagate(), following Error API, to imply the Error object ownership transition. NOTE: this patch also makes migration_connect() to take ownership of the Error passed in. Reviewed-by: Markus Armbruster <armbru@redhat.com> Link: https://lore.kernel.org/r/20251201194510.1121221-5-peterx@redhat.com Signed-off-by: Peter Xu <peterx@redhat.com> --- migration/channel.c | 1 - migration/migration.c | 9 +++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/migration/channel.c b/migration/channel.c index XXXXXXX..XXXXXXX 100644 --- a/migration/channel.c +++ b/migration/channel.c @@ -XXX,XX +XXX,XX @@ void migration_channel_connect(MigrationState *s, } } migration_connect(s, error); - error_free(error); } diff --git a/migration/migration.c b/migration/migration.c index XXXXXXX..XXXXXXX 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -XXX,XX +XXX,XX @@ static void migrate_error_free(MigrationState *s) } } -static void migration_connect_set_error(MigrationState *s, const Error *error) +static void migration_connect_error_propagate(MigrationState *s, Error *error) { MigrationStatus current = s->state; MigrationStatus next; @@ -XXX,XX +XXX,XX @@ static void migration_connect_set_error(MigrationState *s, const Error *error) migrate_set_state(&s->state, current, next); migrate_set_error(s, error); + error_free(error); } void migration_cancel(void) @@ -XXX,XX +XXX,XX @@ void qmp_migrate(const char *uri, bool has_channels, out: if (local_err) { - migration_connect_set_error(s, local_err); + migration_connect_error_propagate(s, error_copy(local_err)); error_propagate(errp, local_err); } } @@ -XXX,XX +XXX,XX @@ static void qmp_migrate_finish(MigrationAddress *addr, bool resume_requested, if (!resume_requested) { yank_unregister_instance(MIGRATION_YANK_INSTANCE); } - migration_connect_set_error(s, local_err); + migration_connect_error_propagate(s, error_copy(local_err)); error_propagate(errp, local_err); return; } @@ -XXX,XX +XXX,XX @@ void migration_connect(MigrationState *s, Error *error_in) s->expected_downtime = migrate_downtime_limit(); if (error_in) { - migration_connect_set_error(s, error_in); + migration_connect_error_propagate(s, error_in); if (resume) { /* * Don't do cleanup for resume if channel is invalid, but only dump -- 2.50.1
Make multifd_send_set_error() take ownership of the error always. Paving way for making migrate_set_error() to take ownership. When at it, rename it to multifd_send_error_propagate() to imply the ownership transition following Error API's naming style. Reviewed-by: Markus Armbruster <armbru@redhat.com> Link: https://lore.kernel.org/r/20251201194510.1121221-6-peterx@redhat.com Signed-off-by: Peter Xu <peterx@redhat.com> --- migration/multifd.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/migration/multifd.c b/migration/multifd.c index XXXXXXX..XXXXXXX 100644 --- a/migration/multifd.c +++ b/migration/multifd.c @@ -XXX,XX +XXX,XX @@ bool multifd_send(MultiFDSendData **send_data) } /* Multifd send side hit an error; remember it and prepare to quit */ -static void multifd_send_set_error(Error *err) +static void multifd_send_error_propagate(Error *err) { /* * We don't want to exit each threads twice. Depending on where @@ -XXX,XX +XXX,XX @@ static void multifd_send_set_error(Error *err) if (err) { MigrationState *s = migrate_get_current(); migrate_set_error(s, err); + error_free(err); if (s->state == MIGRATION_STATUS_SETUP || s->state == MIGRATION_STATUS_PRE_SWITCHOVER || s->state == MIGRATION_STATUS_DEVICE || @@ -XXX,XX +XXX,XX @@ out: if (ret) { assert(local_err); trace_multifd_send_error(p->id); - multifd_send_set_error(local_err); + multifd_send_error_propagate(local_err); multifd_send_kick_main(p); - error_free(local_err); } rcu_unregister_thread(); @@ -XXX,XX +XXX,XX @@ out: } trace_multifd_new_send_channel_async_error(p->id, local_err); - multifd_send_set_error(local_err); + multifd_send_error_propagate(local_err); /* * For error cases (TLS or non-TLS), IO channel is always freed here * rather than when cleanup multifd: since p->c is not set, multifd * cleanup code doesn't even know its existence. */ object_unref(OBJECT(ioc)); - error_free(local_err); } static bool multifd_new_send_channel_create(gpointer opaque, Error **errp) -- 2.50.1
Make multifd_recv_terminate_threads() take ownership of the error always. Paving way for making migrate_set_error() to take ownership. Reviewed-by: Markus Armbruster <armbru@redhat.com> Link: https://lore.kernel.org/r/20251201194510.1121221-7-peterx@redhat.com Signed-off-by: Peter Xu <peterx@redhat.com> --- migration/multifd.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/migration/multifd.c b/migration/multifd.c index XXXXXXX..XXXXXXX 100644 --- a/migration/multifd.c +++ b/migration/multifd.c @@ -XXX,XX +XXX,XX @@ static void multifd_recv_terminate_threads(Error *err) if (err) { MigrationState *s = migrate_get_current(); migrate_set_error(s, err); + error_free(err); if (s->state == MIGRATION_STATUS_SETUP || s->state == MIGRATION_STATUS_ACTIVE) { migrate_set_state(&s->state, s->state, @@ -XXX,XX +XXX,XX @@ static void *multifd_recv_thread(void *opaque) if (local_err) { multifd_recv_terminate_threads(local_err); - error_free(local_err); } rcu_unregister_thread(); @@ -XXX,XX +XXX,XX @@ void multifd_recv_new_channel(QIOChannel *ioc, Error **errp) if (use_packets) { id = multifd_recv_initial_packet(ioc, &local_err); if (id < 0) { - multifd_recv_terminate_threads(local_err); + multifd_recv_terminate_threads(error_copy(local_err)); error_propagate_prepend(errp, local_err, "failed to receive packet" " via multifd channel %d: ", @@ -XXX,XX +XXX,XX @@ void multifd_recv_new_channel(QIOChannel *ioc, Error **errp) if (p->c != NULL) { error_setg(&local_err, "multifd: received id '%d' already setup'", id); - multifd_recv_terminate_threads(local_err); + multifd_recv_terminate_threads(error_copy(local_err)); error_propagate(errp, local_err); return; } -- 2.50.1
migrate_set_error() currently doesn't take ownership of the error being passed in. It's not aligned with the error API and meanwhile it also makes most of the caller free the error explicitly. Change the API to take the ownership of the Error object instead. This should save a lot of error_copy() invocations. Reviewed-by: Markus Armbruster <armbru@redhat.com> Link: https://lore.kernel.org/r/20251201194510.1121221-8-peterx@redhat.com [peterx: break line for qemu_savevm_send_packaged, per markus] Signed-off-by: Peter Xu <peterx@redhat.com> --- migration/migration.h | 2 +- migration/cpr-exec.c | 5 ++-- migration/migration.c | 44 +++++++++++++++----------------- migration/multifd-device-state.c | 5 +--- migration/multifd.c | 19 +++++++------- migration/postcopy-ram.c | 5 ++-- migration/ram.c | 4 +-- migration/savevm.c | 17 +++++------- 8 files changed, 44 insertions(+), 57 deletions(-) diff --git a/migration/migration.h b/migration/migration.h index XXXXXXX..XXXXXXX 100644 --- a/migration/migration.h +++ b/migration/migration.h @@ -XXX,XX +XXX,XX @@ void migration_incoming_process(void); bool migration_has_all_channels(void); -void migrate_set_error(MigrationState *s, const Error *error); +void migrate_error_propagate(MigrationState *s, Error *error); bool migrate_has_error(MigrationState *s); void migration_connect(MigrationState *s, Error *error_in); diff --git a/migration/cpr-exec.c b/migration/cpr-exec.c index XXXXXXX..XXXXXXX 100644 --- a/migration/cpr-exec.c +++ b/migration/cpr-exec.c @@ -XXX,XX +XXX,XX @@ static void cpr_exec_cb(void *opaque) error_report_err(error_copy(err)); migrate_set_state(&s->state, s->state, MIGRATION_STATUS_FAILED); - migrate_set_error(s, err); - error_free(err); + + migrate_error_propagate(s, err); + /* We must reset the error because it'll be reused later */ err = NULL; /* Note, we can go from state COMPLETED to FAILED */ diff --git a/migration/migration.c b/migration/migration.c index XXXXXXX..XXXXXXX 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -XXX,XX +XXX,XX @@ process_incoming_migration_co(void *opaque) fail: migrate_set_state(&mis->state, MIGRATION_STATUS_ACTIVE, MIGRATION_STATUS_FAILED); - migrate_set_error(s, local_err); - error_free(local_err); - + migrate_error_propagate(s, local_err); migration_incoming_state_destroy(); if (mis->exit_on_error) { @@ -XXX,XX +XXX,XX @@ static void migration_cleanup_bh(void *opaque) migration_cleanup(opaque); } -void migrate_set_error(MigrationState *s, const Error *error) +/* + * Propagate the Error* object to migration core. The caller mustn't + * reference the error pointer after the function returned, because the + * Error* object might be freed. + */ +void migrate_error_propagate(MigrationState *s, Error *error) { QEMU_LOCK_GUARD(&s->error_mutex); - trace_migrate_error(error_get_pretty(error)); if (!s->error) { - s->error = error_copy(error); + s->error = error; + } else { + error_free(error); } } @@ -XXX,XX +XXX,XX @@ static void migration_connect_error_propagate(MigrationState *s, Error *error) } migrate_set_state(&s->state, current, next); - migrate_set_error(s, error); - error_free(error); + migrate_error_propagate(s, error); } void migration_cancel(void) @@ -XXX,XX +XXX,XX @@ void qmp_migrate_pause(Error **errp) /* Tell the core migration that we're pausing */ error_setg(&error, "Postcopy migration is paused by the user"); - migrate_set_error(ms, error); - error_free(error); + migrate_error_propagate(ms, error); qemu_mutex_lock(&ms->qemu_file_lock); if (ms->to_dst_file) { @@ -XXX,XX +XXX,XX @@ static void *source_return_path_thread(void *opaque) out: if (err) { - migrate_set_error(ms, err); - error_free(err); + migrate_error_propagate(ms, err); trace_source_return_path_thread_bad_end(); } @@ -XXX,XX +XXX,XX @@ static void migration_completion(MigrationState *s) fail: if (qemu_file_get_error_obj(s->to_dst_file, &local_err)) { - migrate_set_error(s, local_err); - error_free(local_err); + migrate_error_propagate(s, local_err); } else if (ret) { error_setg_errno(&local_err, -ret, "Error in migration completion"); - migrate_set_error(s, local_err); - error_free(local_err); + migrate_error_propagate(s, local_err); } if (s->state != MIGRATION_STATUS_CANCELLING) { @@ -XXX,XX +XXX,XX @@ static MigThrError migration_detect_error(MigrationState *s) } if (local_error) { - migrate_set_error(s, local_error); - error_free(local_error); + migrate_error_propagate(s, local_error); } if (state == MIGRATION_STATUS_POSTCOPY_ACTIVE && ret) { @@ -XXX,XX +XXX,XX @@ static MigIterateState migration_iteration_run(MigrationState *s) if (must_precopy <= s->threshold_size && can_switchover && qatomic_read(&s->start_postcopy)) { if (postcopy_start(s, &local_err)) { - migrate_set_error(s, local_err); + migrate_error_propagate(s, error_copy(local_err)); error_report_err(local_err); } return MIG_ITERATE_SKIP; @@ -XXX,XX +XXX,XX @@ static void *migration_thread(void *opaque) * devices to unplug. This to preserve migration state transitions. */ if (ret) { - migrate_set_error(s, local_err); - error_free(local_err); + migrate_error_propagate(s, local_err); migrate_set_state(&s->state, MIGRATION_STATUS_ACTIVE, MIGRATION_STATUS_FAILED); goto out; @@ -XXX,XX +XXX,XX @@ static void *bg_migration_thread(void *opaque) * devices to unplug. This to preserve migration state transitions. */ if (ret) { - migrate_set_error(s, local_err); - error_free(local_err); + migrate_error_propagate(s, local_err); migrate_set_state(&s->state, MIGRATION_STATUS_ACTIVE, MIGRATION_STATUS_FAILED); goto fail_setup; @@ -XXX,XX +XXX,XX @@ void migration_connect(MigrationState *s, Error *error_in) return; fail: - migrate_set_error(s, local_err); + migrate_error_propagate(s, error_copy(local_err)); if (s->state != MIGRATION_STATUS_CANCELLING) { migrate_set_state(&s->state, s->state, MIGRATION_STATUS_FAILED); } diff --git a/migration/multifd-device-state.c b/migration/multifd-device-state.c index XXXXXXX..XXXXXXX 100644 --- a/migration/multifd-device-state.c +++ b/migration/multifd-device-state.c @@ -XXX,XX +XXX,XX @@ static int multifd_device_state_save_thread(void *opaque) Error *local_err = NULL; if (!data->hdlr(data, &local_err)) { - MigrationState *s = migrate_get_current(); - /* * Can't call abort_device_state_save_threads() here since new * save threads could still be in process of being launched @@ -XXX,XX +XXX,XX @@ static int multifd_device_state_save_thread(void *opaque) * In case of multiple save threads failing which thread error * return we end setting is purely arbitrary. */ - migrate_set_error(s, local_err); - error_free(local_err); + migrate_error_propagate(migrate_get_current(), local_err); } return 0; diff --git a/migration/multifd.c b/migration/multifd.c index XXXXXXX..XXXXXXX 100644 --- a/migration/multifd.c +++ b/migration/multifd.c @@ -XXX,XX +XXX,XX @@ static void multifd_send_error_propagate(Error *err) if (err) { MigrationState *s = migrate_get_current(); - migrate_set_error(s, err); - error_free(err); + + migrate_error_propagate(s, err); + if (s->state == MIGRATION_STATUS_SETUP || s->state == MIGRATION_STATUS_PRE_SWITCHOVER || s->state == MIGRATION_STATUS_DEVICE || @@ -XXX,XX +XXX,XX @@ void multifd_send_shutdown(void) Error *local_err = NULL; if (!multifd_send_cleanup_channel(p, &local_err)) { - migrate_set_error(migrate_get_current(), local_err); - error_free(local_err); + migrate_error_propagate(migrate_get_current(), local_err); } } @@ -XXX,XX +XXX,XX @@ bool multifd_send_setup(void) p->write_flags = 0; if (!multifd_new_send_channel_create(p, &local_err)) { - migrate_set_error(s, local_err); - error_free(local_err); + migrate_error_propagate(s, local_err); ret = -1; } } @@ -XXX,XX +XXX,XX @@ bool multifd_send_setup(void) ret = multifd_send_state->ops->send_setup(p, &local_err); if (ret) { - migrate_set_error(s, local_err); - error_free(local_err); + migrate_error_propagate(s, local_err); goto err; } assert(p->iov); @@ -XXX,XX +XXX,XX @@ static void multifd_recv_terminate_threads(Error *err) if (err) { MigrationState *s = migrate_get_current(); - migrate_set_error(s, err); - error_free(err); + + migrate_error_propagate(s, err); + if (s->state == MIGRATION_STATUS_SETUP || s->state == MIGRATION_STATUS_ACTIVE) { migrate_set_state(&s->state, s->state, diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c index XXXXXXX..XXXXXXX 100644 --- a/migration/postcopy-ram.c +++ b/migration/postcopy-ram.c @@ -XXX,XX +XXX,XX @@ postcopy_preempt_send_channel_done(MigrationState *s, QIOChannel *ioc, Error *local_err) { if (local_err) { - migrate_set_error(s, local_err); - error_free(local_err); + migrate_error_propagate(s, local_err); } else { migration_ioc_register_yank(ioc); s->postcopy_qemufile_src = qemu_file_new_output(ioc); @@ -XXX,XX +XXX,XX @@ static void *postcopy_listen_thread(void *opaque) * exit depending on if postcopy-exit-on-error is true, but the * migration cannot be recovered. */ - migrate_set_error(migr, local_err); + migrate_error_propagate(migr, error_copy(local_err)); error_report_err(local_err); migrate_set_state(&mis->state, mis->state, MIGRATION_STATUS_FAILED); goto out; diff --git a/migration/ram.c b/migration/ram.c index XXXXXXX..XXXXXXX 100644 --- a/migration/ram.c +++ b/migration/ram.c @@ -XXX,XX +XXX,XX @@ static void ram_mig_ram_block_resized(RAMBlockNotifier *n, void *host, * Abort and indicate a proper reason. */ error_setg(&err, "RAM block '%s' resized during precopy.", rb->idstr); - migrate_set_error(migrate_get_current(), err); - error_free(err); - + migrate_error_propagate(migrate_get_current(), err); migration_cancel(); } diff --git a/migration/savevm.c b/migration/savevm.c index XXXXXXX..XXXXXXX 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -XXX,XX +XXX,XX @@ void qemu_savevm_send_open_return_path(QEMUFile *f) int qemu_savevm_send_packaged(QEMUFile *f, const uint8_t *buf, size_t len) { uint32_t tmp; - MigrationState *ms = migrate_get_current(); Error *local_err = NULL; if (len > MAX_VM_CMD_PACKAGED_SIZE) { error_setg(&local_err, "%s: Unreasonably large packaged state: %zu", __func__, len); - migrate_set_error(ms, local_err); + migrate_error_propagate(migrate_get_current(), + error_copy(local_err)); error_report_err(local_err); return -1; } @@ -XXX,XX +XXX,XX @@ int qemu_savevm_state_setup(QEMUFile *f, Error **errp) if (se->vmsd && se->vmsd->early_setup) { ret = vmstate_save(f, se, vmdesc, errp); if (ret) { - migrate_set_error(ms, *errp); + migrate_error_propagate(ms, error_copy(*errp)); qemu_file_set_error(f, ret); break; } @@ -XXX,XX +XXX,XX @@ int qemu_savevm_state_complete_precopy_non_iterable(QEMUFile *f, ret = vmstate_save(f, se, vmdesc, &local_err); if (ret) { - migrate_set_error(ms, local_err); + migrate_error_propagate(ms, error_copy(local_err)); error_report_err(local_err); qemu_file_set_error(f, ret); return ret; @@ -XXX,XX +XXX,XX @@ void qemu_savevm_live_state(QEMUFile *f) int qemu_save_device_state(QEMUFile *f) { - MigrationState *ms = migrate_get_current(); Error *local_err = NULL; SaveStateEntry *se; @@ -XXX,XX +XXX,XX @@ int qemu_save_device_state(QEMUFile *f) } ret = vmstate_save(f, se, NULL, &local_err); if (ret) { - migrate_set_error(ms, local_err); + migrate_error_propagate(migrate_get_current(), + error_copy(local_err)); error_report_err(local_err); return ret; } @@ -XXX,XX +XXX,XX @@ static int qemu_loadvm_load_thread(void *thread_opaque) Error *local_err = NULL; if (!data->function(data->opaque, &mis->load_threads_abort, &local_err)) { - MigrationState *s = migrate_get_current(); - /* * Can't set load_threads_abort here since processing of main migration * channel data could still be happening, resulting in launching of new @@ -XXX,XX +XXX,XX @@ static int qemu_loadvm_load_thread(void *thread_opaque) * In case of multiple load threads failing which thread error * return we end setting is purely arbitrary. */ - migrate_set_error(s, local_err); - error_free(local_err); + migrate_error_propagate(migrate_get_current(), local_err); } return 0; -- 2.50.1
It improves readability, as suggested by Markus. Suggested-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Markus Armbruster <armbru@redhat.com> Link: https://lore.kernel.org/r/20251202175317.1186544-1-peterx@redhat.com Signed-off-by: Peter Xu <peterx@redhat.com> --- migration/migration.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/migration/migration.c b/migration/migration.c index XXXXXXX..XXXXXXX 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -XXX,XX +XXX,XX @@ void migrate_error_propagate(MigrationState *s, Error *error) { QEMU_LOCK_GUARD(&s->error_mutex); trace_migrate_error(error_get_pretty(error)); - - if (!s->error) { - s->error = error; - } else { - error_free(error); - } + error_propagate(&s->error, error); } bool migrate_has_error(MigrationState *s) -- 2.50.1
This aids scriptings only. Reviewed-by: Fabiano Rosas <farosas@suse.de> Link: https://lore.kernel.org/r/20251205172054.288909-1-peterx@redhat.com [peterx: make the property x-ignore-shared to match, per cedric] [peterx: fix over-80 line] Signed-off-by: Peter Xu <peterx@redhat.com> --- migration/options.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/migration/options.c b/migration/options.c index XXXXXXX..XXXXXXX 100644 --- a/migration/options.c +++ b/migration/options.c @@ -XXX,XX +XXX,XX @@ const Property migration_properties[] = { MIGRATION_CAPABILITY_SWITCHOVER_ACK), DEFINE_PROP_MIG_CAP("x-dirty-limit", MIGRATION_CAPABILITY_DIRTY_LIMIT), DEFINE_PROP_MIG_CAP("mapped-ram", MIGRATION_CAPABILITY_MAPPED_RAM), + DEFINE_PROP_MIG_CAP("x-ignore-shared", + MIGRATION_CAPABILITY_X_IGNORE_SHARED), }; const size_t migration_properties_count = ARRAY_SIZE(migration_properties); -- 2.50.1
From: Ben Chaney <bchaney@akamai.com> Signed-off-by: Ben Chaney <bchaney@akamai.com> Reviewed-by: Fabiano Rosas <farosas@suse.de> Link: https://lore.kernel.org/r/20251210143624.416697-1-bchaney@akamai.com Signed-off-by: Peter Xu <peterx@redhat.com> --- MAINTAINERS | 2 ++ 1 file changed, 2 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index XXXXXXX..XXXXXXX 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -XXX,XX +XXX,XX @@ T: git https://gitlab.com/vsementsov/qemu.git block CheckPoint and Restart (CPR) R: Peter Xu <peterx@redhat.com> R: Fabiano Rosas <farosas@suse.de> +R: Mark Kanda <mark.kanda@oracle.com> +R: Ben Chaney <bchaney@akamai.com> S: Supported F: hw/vfio/cpr* F: include/hw/vfio/vfio-cpr.h -- 2.50.1
From: Fabiano Rosas <farosas@suse.de> Caught by inspection, but ASAN also reports: Direct leak of 16 byte(s) in 1 object(s) allocated from: #0 in malloc #1 in g_malloc #2 in g_memdup #3 in qapi_clone_start_struct ../qapi/qapi-clone-visitor.c:40:12 #4 in qapi_clone_start_list ../qapi/qapi-clone-visitor.c:59:12 #5 in visit_start_list ../qapi/qapi-visit-core.c:80:10 #6 in visit_type_BitmapMigrationNodeAliasList qapi/qapi-visit-migration.c:639:10 #7 in migrate_params_apply ../migration/options.c:1407:13 #8 in qmp_migrate_set_parameters ../migration/options.c:1463:5 #9 in qmp_marshal_migrate_set_parameters qapi/qapi-commands-migration.c:214:5 #10 in do_qmp_dispatch_bh ../qapi/qmp-dispatch.c:128:5 Note that this is entirely harmless because the migration object which contains the MigrationParameters structure is kept until the QEMU process exits. Reviewed-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Peter Xu <peterx@redhat.com> Signed-off-by: Fabiano Rosas <farosas@suse.de> Link: https://lore.kernel.org/r/20251215220041.12657-2-farosas@suse.de Signed-off-by: Peter Xu <peterx@redhat.com> --- migration/migration.c | 1 + 1 file changed, 1 insertion(+) diff --git a/migration/migration.c b/migration/migration.c index XXXXXXX..XXXXXXX 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -XXX,XX +XXX,XX @@ static void migration_instance_finalize(Object *obj) { MigrationState *ms = MIGRATION_OBJ(obj); + qapi_free_BitmapMigrationNodeAliasList(ms->parameters.block_bitmap_mapping); qemu_mutex_destroy(&ms->error_mutex); qemu_mutex_destroy(&ms->qemu_file_lock); qemu_sem_destroy(&ms->wait_unplug_sem); -- 2.50.1
From: Fabiano Rosas <farosas@suse.de> Signed-off-by: Fabiano Rosas <farosas@suse.de> Reviewed-by: Peter Xu <peterx@redhat.com> Link: https://lore.kernel.org/r/20251215220041.12657-3-farosas@suse.de Signed-off-by: Peter Xu <peterx@redhat.com> --- migration/migration.c | 1 + 1 file changed, 1 insertion(+) diff --git a/migration/migration.c b/migration/migration.c index XXXXXXX..XXXXXXX 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -XXX,XX +XXX,XX @@ static void migration_instance_finalize(Object *obj) MigrationState *ms = MIGRATION_OBJ(obj); qapi_free_BitmapMigrationNodeAliasList(ms->parameters.block_bitmap_mapping); + qapi_free_strList(ms->parameters.cpr_exec_command); qemu_mutex_destroy(&ms->error_mutex); qemu_mutex_destroy(&ms->qemu_file_lock); qemu_sem_destroy(&ms->wait_unplug_sem); -- 2.50.1
From: Fabiano Rosas <farosas@suse.de> The MigrationState is a QOM object with TYPE_DEVICE as a parent. This was done about eight years ago so the migration code could make use of qdev properties to define the defaults for the migration parameters and to be able to expose migration knobs for debugging via the '-global migration' command line option. Due to unrelated historical reasons, three of the migration parameters (TLS options) received different types when used via the query-migrate-parameters QMP command than with the migrate-set-parameters command. This has created a lot of duplication in the migration code and in the QAPI documentation because the whole of MigrationParameters had to be duplicated as well. The migration code is now being fixed to remove the duplication and for that to happen the offending fields need to be reconciled into a single type. The StrOrNull type is going to be used. To keep the command line compatibility, the parameters need to continue being exposed via qdev properties accessible from the command line. Introduce a qdev property StrOrNull just for that. Note that this code is being kept in migration/options.c as this version of StrOrNull doesn't need to handle QNULL because it was never a valid option in the previous command line, which took a string. Signed-off-by: Fabiano Rosas <farosas@suse.de> Acked-by: Peter Xu <peterx@redhat.com> Link: https://lore.kernel.org/r/20251215220041.12657-4-farosas@suse.de Signed-off-by: Peter Xu <peterx@redhat.com> --- migration/options.c | 67 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 67 insertions(+) diff --git a/migration/options.c b/migration/options.c index XXXXXXX..XXXXXXX 100644 --- a/migration/options.c +++ b/migration/options.c @@ -XXX,XX +XXX,XX @@ #define DEFINE_PROP_MIG_CAP(name, x) \ DEFINE_PROP_BOOL(name, MigrationState, capabilities[x], false) +const PropertyInfo qdev_prop_StrOrNull; +#define DEFINE_PROP_STR_OR_NULL(_name, _state, _field) \ + DEFINE_PROP(_name, _state, _field, qdev_prop_StrOrNull, StrOrNull *, \ + .set_default = true) + #define DEFAULT_MIGRATE_VCPU_DIRTY_LIMIT_PERIOD 1000 /* milliseconds */ #define DEFAULT_MIGRATE_VCPU_DIRTY_LIMIT 1 /* MB/s */ @@ -XXX,XX +XXX,XX @@ const Property migration_properties[] = { }; const size_t migration_properties_count = ARRAY_SIZE(migration_properties); +static void get_StrOrNull(Object *obj, Visitor *v, const char *name, + void *opaque, Error **errp) +{ + const Property *prop = opaque; + StrOrNull **ptr = object_field_prop_ptr(obj, prop); + StrOrNull *str_or_null = *ptr; + + if (!str_or_null) { + str_or_null = g_new0(StrOrNull, 1); + str_or_null->type = QTYPE_QSTRING; + str_or_null->u.s = g_strdup(""); + } else { + /* the setter doesn't allow QNULL */ + assert(str_or_null->type != QTYPE_QNULL); + } + visit_type_str(v, name, &str_or_null->u.s, errp); +} + +static void set_StrOrNull(Object *obj, Visitor *v, const char *name, + void *opaque, Error **errp) +{ + const Property *prop = opaque; + StrOrNull **ptr = object_field_prop_ptr(obj, prop); + StrOrNull *str_or_null = g_new0(StrOrNull, 1); + + /* + * Only str to keep compatibility, QNULL was never used via + * command line. + */ + str_or_null->type = QTYPE_QSTRING; + if (!visit_type_str(v, name, &str_or_null->u.s, errp)) { + return; + } + + qapi_free_StrOrNull(*ptr); + *ptr = str_or_null; +} + +static void release_StrOrNull(Object *obj, const char *name, void *opaque) +{ + const Property *prop = opaque; + qapi_free_StrOrNull(*(StrOrNull **)object_field_prop_ptr(obj, prop)); +} + +static void set_default_value_tls_opt(ObjectProperty *op, const Property *prop) +{ + object_property_set_default_str(op, ""); +} + +/* + * String property like qdev_prop_string, except it's backed by a + * StrOrNull instead of a char *. This is intended for + * TYPE_MIGRATION's TLS options. + */ +const PropertyInfo qdev_prop_StrOrNull = { + .type = "StrOrNull", + .get = get_StrOrNull, + .set = set_StrOrNull, + .release = release_StrOrNull, + .set_default_value = set_default_value_tls_opt, +}; + bool migrate_auto_converge(void) { MigrationState *s = migrate_get_current(); -- 2.50.1
From: Fabiano Rosas <farosas@suse.de> Make sure the TLS options handling is working correctly with a NULL parameter. This is relevant due to the usage of StrOrNull for the tls-creds, tls-authz and tls-hostname options. With this, all manners of passing TLS options are somehow covered by the tests, we should not need to do manual testing when touching TLS options code. Signed-off-by: Fabiano Rosas <farosas@suse.de> Reviewed-by: Peter Xu <peterx@redhat.com> Link: https://lore.kernel.org/r/20251215220041.12657-5-farosas@suse.de Signed-off-by: Peter Xu <peterx@redhat.com> --- tests/qtest/migration/migration-qmp.h | 1 + tests/qtest/migration/migration-qmp.c | 9 +++++ tests/qtest/migration/tls-tests.c | 56 +++++++++++++++++++++++++++ 3 files changed, 66 insertions(+) diff --git a/tests/qtest/migration/migration-qmp.h b/tests/qtest/migration/migration-qmp.h index XXXXXXX..XXXXXXX 100644 --- a/tests/qtest/migration/migration-qmp.h +++ b/tests/qtest/migration/migration-qmp.h @@ -XXX,XX +XXX,XX @@ void migrate_set_parameter_str(QTestState *who, const char *parameter, const char *value); void migrate_set_parameter_strv(QTestState *who, const char *parameter, char **strv); +void migrate_set_parameter_null(QTestState *who, const char *parameter); void migrate_set_parameter_bool(QTestState *who, const char *parameter, int value); void migrate_ensure_non_converge(QTestState *who); diff --git a/tests/qtest/migration/migration-qmp.c b/tests/qtest/migration/migration-qmp.c index XXXXXXX..XXXXXXX 100644 --- a/tests/qtest/migration/migration-qmp.c +++ b/tests/qtest/migration/migration-qmp.c @@ -XXX,XX +XXX,XX @@ void migrate_set_parameter_strv(QTestState *who, const char *parameter, qtest_qmp_assert_success(who, command, parameter); } +void migrate_set_parameter_null(QTestState *who, const char *parameter) +{ + qtest_qmp_assert_success(who, + "{ 'execute': 'migrate-set-parameters'," + "'arguments': { %s: null } }", + parameter); + migrate_check_parameter_str(who, parameter, ""); +} + static long long migrate_get_parameter_bool(QTestState *who, const char *parameter) { diff --git a/tests/qtest/migration/tls-tests.c b/tests/qtest/migration/tls-tests.c index XXXXXXX..XXXXXXX 100644 --- a/tests/qtest/migration/tls-tests.c +++ b/tests/qtest/migration/tls-tests.c @@ -XXX,XX +XXX,XX @@ static void test_precopy_tcp_tls_psk_mismatch(void) test_precopy_common(&args); } +static void *migrate_hook_start_no_tls(QTestState *from, QTestState *to) +{ + struct TestMigrateTLSPSKData *data = + g_new0(struct TestMigrateTLSPSKData, 1); + + migrate_set_parameter_null(from, "tls-creds"); + migrate_set_parameter_null(to, "tls-creds"); + + return data; +} + +static void test_precopy_tcp_no_tls(void) +{ + MigrateCommon args = { + .listen_uri = "tcp:127.0.0.1:0", + .start_hook = migrate_hook_start_no_tls, + .end_hook = migrate_hook_end_tls_psk, + }; + + test_precopy_common(&args); +} + +static void * +migrate_hook_start_tls_x509_no_host(QTestState *from, QTestState *to) +{ + TestMigrateTLSX509 args = { + .verifyclient = true, + .clientcert = true, + .authzclient = true, + }; + TestMigrateTLSX509Data *data = migrate_hook_start_tls_x509_common(from, to, + &args); + migrate_set_parameter_null(from, "tls-hostname"); + migrate_set_parameter_null(to, "tls-hostname"); + + return data; +} + +static void test_precopy_tcp_tls_no_hostname(void) +{ + MigrateCommon args = { + .listen_uri = "tcp:127.0.0.1:0", + .start_hook = migrate_hook_start_tls_x509_no_host, + .end_hook = migrate_hook_end_tls_x509, + .result = MIG_TEST_FAIL_DEST_QUIT_ERR, + .start.hide_stderr = true, + }; + + test_precopy_common(&args); +} + #ifdef CONFIG_TASN1 static void test_precopy_tcp_tls_x509_default_host(void) { @@ -XXX,XX +XXX,XX @@ void migration_test_add_tls(MigrationTestEnv *env) return; } + migration_test_add("/migration/precopy/tcp/no-tls", + test_precopy_tcp_no_tls); + migration_test_add("/migration/precopy/tcp/tls/no-hostname", + test_precopy_tcp_tls_no_hostname); + migration_test_add("/migration/precopy/unix/tls/psk", test_precopy_unix_tls_psk); -- 2.50.1
From: Fabiano Rosas <farosas@suse.de> The migration parameters tls_creds, tls_authz and tls_hostname currently have a non-uniform handling. When used as arguments to migrate-set-parameters, their type is StrOrNull and when used as return value from query-migrate-parameters their type is a plain string. Not only having to convert between the types is cumbersome, but it also creates the issue of requiring two different QAPI types to be used, one for each command. MigrateSetParameters is used for migrate-set-parameters with the TLS arguments as StrOrNull while MigrationParameters is used for query-migrate-parameters with the TLS arguments as str. Since StrOrNull could be considered a superset of str, change the type of the TLS arguments in MigrationParameters to StrOrNull. Also ensure that QTYPE_QNULL is never used. 1) migrate-set-parameters will always write QTYPE_QSTRING to s->parameters, either an empty or non-empty string. 2) query-migrate-parameters will always return a QTYPE_QSTRING, either empty or non-empty. 3) the migrate_tls_* helpers will always return a non-empty string or NULL, for the internal migration code's consumption. Points (1) and (2) above help simplify the parameters validation and the query command handling because s->parameters is already kept in the format that query-migrate-parameters (and info migrate_paramters) expect. Point (3) is so people don't need to care about StrOrNull in migration code. This will allow the type duplication to be removed in the next patches. Note that the type of @tls_creds, @tls-hostname, @tls-authz changes from str to StrOrNull in introspection of the query-migrate-parameters command. We accept this imprecision to enable de-duplication. There's no need to free the TLS options in migration_instance_finalize() because they're freed by the qdev properties .release method. Temporary in this patch: migrate_params_test_apply() copies s->parameters into a temporary structure, so it's necessary to drop the references to the TLS options if they were not set by the user to avoid double-free. This is fixed in the next patches. Acked-by: Markus Armbruster <armbru@redhat.com> Signed-off-by: Fabiano Rosas <farosas@suse.de> Link: https://lore.kernel.org/r/20251215220041.12657-6-farosas@suse.de [peterx: in hmp_info_migrate_parameters(), remove an extra dump of max_postcopy_bandwidth, introduced likely by accident] Signed-off-by: Peter Xu <peterx@redhat.com> --- qapi/migration.json | 6 +- migration/options.h | 1 + migration/migration-hmp-cmds.c | 6 +- migration/options.c | 144 +++++++++++++++++++-------------- migration/tls.c | 2 +- 5 files changed, 93 insertions(+), 66 deletions(-) diff --git a/qapi/migration.json b/qapi/migration.json index XXXXXXX..XXXXXXX 100644 --- a/qapi/migration.json +++ b/qapi/migration.json @@ -XXX,XX +XXX,XX @@ '*cpu-throttle-initial': 'uint8', '*cpu-throttle-increment': 'uint8', '*cpu-throttle-tailslow': 'bool', - '*tls-creds': 'str', - '*tls-hostname': 'str', - '*tls-authz': 'str', + '*tls-creds': 'StrOrNull', + '*tls-hostname': 'StrOrNull', + '*tls-authz': 'StrOrNull', '*max-bandwidth': 'size', '*avail-switchover-bandwidth': 'size', '*downtime-limit': 'uint64', diff --git a/migration/options.h b/migration/options.h index XXXXXXX..XXXXXXX 100644 --- a/migration/options.h +++ b/migration/options.h @@ -XXX,XX +XXX,XX @@ ZeroPageDetection migrate_zero_page_detection(void); bool migrate_params_check(MigrationParameters *params, Error **errp); void migrate_params_init(MigrationParameters *params); +void migrate_tls_opts_free(MigrationParameters *params); #endif diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c index XXXXXXX..XXXXXXX 100644 --- a/migration/migration-hmp-cmds.c +++ b/migration/migration-hmp-cmds.c @@ -XXX,XX +XXX,XX @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict) assert(params->tls_creds); monitor_printf(mon, "%s: '%s'\n", MigrationParameter_str(MIGRATION_PARAMETER_TLS_CREDS), - params->tls_creds); + params->tls_creds->u.s); assert(params->tls_hostname); monitor_printf(mon, "%s: '%s'\n", MigrationParameter_str(MIGRATION_PARAMETER_TLS_HOSTNAME), - params->tls_hostname); + params->tls_hostname->u.s); assert(params->tls_authz); monitor_printf(mon, "%s: '%s'\n", MigrationParameter_str(MIGRATION_PARAMETER_TLS_AUTHZ), - params->tls_authz); + params->tls_authz->u.s); assert(params->has_max_bandwidth); monitor_printf(mon, "%s: %" PRIu64 " bytes/second\n", MigrationParameter_str(MIGRATION_PARAMETER_MAX_BANDWIDTH), diff --git a/migration/options.c b/migration/options.c index XXXXXXX..XXXXXXX 100644 --- a/migration/options.c +++ b/migration/options.c @@ -XXX,XX +XXX,XX @@ const Property migration_properties[] = { DEFINE_PROP_SIZE("announce-step", MigrationState, parameters.announce_step, DEFAULT_MIGRATE_ANNOUNCE_STEP), - DEFINE_PROP_STRING("tls-creds", MigrationState, parameters.tls_creds), - DEFINE_PROP_STRING("tls-hostname", MigrationState, parameters.tls_hostname), - DEFINE_PROP_STRING("tls-authz", MigrationState, parameters.tls_authz), + DEFINE_PROP_STR_OR_NULL("tls-creds", MigrationState, parameters.tls_creds), + DEFINE_PROP_STR_OR_NULL("tls-hostname", MigrationState, + parameters.tls_hostname), + DEFINE_PROP_STR_OR_NULL("tls-authz", MigrationState, parameters.tls_authz), DEFINE_PROP_UINT64("x-vcpu-dirty-limit-period", MigrationState, parameters.x_vcpu_dirty_limit_period, DEFAULT_MIGRATE_VCPU_DIRTY_LIMIT_PERIOD), @@ -XXX,XX +XXX,XX @@ static void release_StrOrNull(Object *obj, const char *name, void *opaque) static void set_default_value_tls_opt(ObjectProperty *op, const Property *prop) { + /* + * Initialization to the empty string here is important so + * query-migrate-parameters doesn't need to deal with a NULL value + * when it's called before any TLS option has been set. + */ object_property_set_default_str(op, ""); } @@ -XXX,XX +XXX,XX @@ bool migrate_rdma(void) return s->rdma_migration; } -bool migrate_tls(void) -{ - MigrationState *s = migrate_get_current(); - - return s->parameters.tls_creds && *s->parameters.tls_creds; -} - typedef enum WriteTrackingSupport { WT_SUPPORT_UNKNOWN = 0, WT_SUPPORT_ABSENT, @@ -XXX,XX +XXX,XX @@ const char *migrate_tls_authz(void) { MigrationState *s = migrate_get_current(); - return s->parameters.tls_authz; + if (*s->parameters.tls_authz->u.s) { + return s->parameters.tls_authz->u.s; + } + + return NULL; } const char *migrate_tls_creds(void) { MigrationState *s = migrate_get_current(); - return s->parameters.tls_creds; + if (*s->parameters.tls_creds->u.s) { + return s->parameters.tls_creds->u.s; + } + + return NULL; } const char *migrate_tls_hostname(void) { MigrationState *s = migrate_get_current(); - return s->parameters.tls_hostname; + if (*s->parameters.tls_hostname->u.s) { + return s->parameters.tls_hostname->u.s; + } + + return NULL; +} + +bool migrate_tls(void) +{ + return !!migrate_tls_creds(); } uint64_t migrate_vcpu_dirty_limit_period(void) @@ -XXX,XX +XXX,XX @@ AnnounceParameters *migrate_announce_params(void) return ≈ } +void migrate_tls_opts_free(MigrationParameters *params) +{ + qapi_free_StrOrNull(params->tls_creds); + qapi_free_StrOrNull(params->tls_hostname); + qapi_free_StrOrNull(params->tls_authz); +} + +/* normalize QTYPE_QNULL to QTYPE_QSTRING "" */ +static void tls_opt_to_str(StrOrNull *opt) +{ + if (!opt || opt->type == QTYPE_QSTRING) { + return; + } + + qobject_unref(opt->u.n); + opt->type = QTYPE_QSTRING; + opt->u.s = g_strdup(""); +} + MigrationParameters *qmp_query_migrate_parameters(Error **errp) { MigrationParameters *params; @@ -XXX,XX +XXX,XX @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp) params->cpu_throttle_increment = s->parameters.cpu_throttle_increment; params->has_cpu_throttle_tailslow = true; params->cpu_throttle_tailslow = s->parameters.cpu_throttle_tailslow; - params->tls_creds = g_strdup(s->parameters.tls_creds); - params->tls_hostname = g_strdup(s->parameters.tls_hostname); - params->tls_authz = g_strdup(s->parameters.tls_authz ? - s->parameters.tls_authz : ""); + params->tls_creds = QAPI_CLONE(StrOrNull, s->parameters.tls_creds); + params->tls_hostname = QAPI_CLONE(StrOrNull, s->parameters.tls_hostname); + params->tls_authz = QAPI_CLONE(StrOrNull, s->parameters.tls_authz); params->has_max_bandwidth = true; params->max_bandwidth = s->parameters.max_bandwidth; params->has_avail_switchover_bandwidth = true; @@ -XXX,XX +XXX,XX @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp) void migrate_params_init(MigrationParameters *params) { - params->tls_hostname = g_strdup(""); - params->tls_creds = g_strdup(""); - /* Set has_* up only for parameter checks */ params->has_throttle_trigger_threshold = true; params->has_cpu_throttle_initial = true; @@ -XXX,XX +XXX,XX @@ bool migrate_params_check(MigrationParameters *params, Error **errp) #ifdef CONFIG_LINUX if (migrate_zero_copy_send() && ((params->has_multifd_compression && params->multifd_compression) || - (params->tls_creds && *params->tls_creds))) { + *params->tls_creds->u.s)) { error_setg(errp, "Zero copy only available for non-compressed non-TLS multifd migration"); return false; @@ -XXX,XX +XXX,XX @@ static void migrate_params_test_apply(MigrateSetParameters *params, } if (params->tls_creds) { - assert(params->tls_creds->type == QTYPE_QSTRING); - dest->tls_creds = params->tls_creds->u.s; + dest->tls_creds = QAPI_CLONE(StrOrNull, params->tls_creds); + } else { + /* clear the reference, it's owned by s->parameters */ + dest->tls_creds = NULL; } if (params->tls_hostname) { - assert(params->tls_hostname->type == QTYPE_QSTRING); - dest->tls_hostname = params->tls_hostname->u.s; + dest->tls_hostname = QAPI_CLONE(StrOrNull, params->tls_hostname); + } else { + /* clear the reference, it's owned by s->parameters */ + dest->tls_hostname = NULL; } if (params->tls_authz) { - assert(params->tls_authz->type == QTYPE_QSTRING); - dest->tls_authz = params->tls_authz->u.s; + dest->tls_authz = QAPI_CLONE(StrOrNull, params->tls_authz); + } else { + /* clear the reference, it's owned by s->parameters */ + dest->tls_authz = NULL; } if (params->has_max_bandwidth) { @@ -XXX,XX +XXX,XX @@ static void migrate_params_apply(MigrateSetParameters *params, Error **errp) } if (params->tls_creds) { - g_free(s->parameters.tls_creds); - assert(params->tls_creds->type == QTYPE_QSTRING); - s->parameters.tls_creds = g_strdup(params->tls_creds->u.s); + qapi_free_StrOrNull(s->parameters.tls_creds); + s->parameters.tls_creds = QAPI_CLONE(StrOrNull, params->tls_creds); } if (params->tls_hostname) { - g_free(s->parameters.tls_hostname); - assert(params->tls_hostname->type == QTYPE_QSTRING); - s->parameters.tls_hostname = g_strdup(params->tls_hostname->u.s); + qapi_free_StrOrNull(s->parameters.tls_hostname); + s->parameters.tls_hostname = QAPI_CLONE(StrOrNull, + params->tls_hostname); } if (params->tls_authz) { - g_free(s->parameters.tls_authz); - assert(params->tls_authz->type == QTYPE_QSTRING); - s->parameters.tls_authz = g_strdup(params->tls_authz->u.s); + qapi_free_StrOrNull(s->parameters.tls_authz); + s->parameters.tls_authz = QAPI_CLONE(StrOrNull, params->tls_authz); } if (params->has_max_bandwidth) { @@ -XXX,XX +XXX,XX @@ void qmp_migrate_set_parameters(MigrateSetParameters *params, Error **errp) { MigrationParameters tmp; - /* TODO Rewrite "" to null instead for all three tls_* parameters */ - if (params->tls_creds - && params->tls_creds->type == QTYPE_QNULL) { - qobject_unref(params->tls_creds->u.n); - params->tls_creds->type = QTYPE_QSTRING; - params->tls_creds->u.s = strdup(""); - } - if (params->tls_hostname - && params->tls_hostname->type == QTYPE_QNULL) { - qobject_unref(params->tls_hostname->u.n); - params->tls_hostname->type = QTYPE_QSTRING; - params->tls_hostname->u.s = strdup(""); - } - if (params->tls_authz - && params->tls_authz->type == QTYPE_QNULL) { - qobject_unref(params->tls_authz->u.n); - params->tls_authz->type = QTYPE_QSTRING; - params->tls_authz->u.s = strdup(""); - } + /* + * Convert QTYPE_QNULL and NULL to the empty string (""). Even + * though NULL is cleaner to deal with in C code, that would force + * query-migrate-parameters to convert it once more to the empty + * string, so avoid that. The migrate_tls_*() helpers that expose + * the options to the rest of the migration code already use + * return NULL when the empty string is found. + */ + tls_opt_to_str(params->tls_creds); + tls_opt_to_str(params->tls_hostname); + tls_opt_to_str(params->tls_authz); migrate_params_test_apply(params, &tmp); - if (!migrate_params_check(&tmp, errp)) { - /* Invalid parameter */ - return; + if (migrate_params_check(&tmp, errp)) { + migrate_params_apply(params, errp); } - migrate_params_apply(params, errp); + migrate_tls_opts_free(&tmp); } diff --git a/migration/tls.c b/migration/tls.c index XXXXXXX..XXXXXXX 100644 --- a/migration/tls.c +++ b/migration/tls.c @@ -XXX,XX +XXX,XX @@ QIOChannelTLS *migration_tls_client_create(QIOChannel *ioc, } const char *tls_hostname = migrate_tls_hostname(); - if (tls_hostname && *tls_hostname) { + if (tls_hostname) { hostname = tls_hostname; } -- 2.50.1
From: Fabiano Rosas <farosas@suse.de> Now that the TLS options have been made the same between migrate-set-parameters and query-migrate-parameters, a single type can be used. Remove MigrateSetParameters. The TLS options documentation from MigrationParameters were replaced with the ones from MigrateSetParameters which was more complete. Acked-by: Markus Armbruster <armbru@redhat.com> Acked-by: Peter Xu <peterx@redhat.com> Signed-off-by: Fabiano Rosas <farosas@suse.de> Link: https://lore.kernel.org/r/20251215220041.12657-7-farosas@suse.de Signed-off-by: Peter Xu <peterx@redhat.com> --- qapi/migration.json | 239 +++------------------------------ migration/migration-hmp-cmds.c | 4 +- migration/options.c | 6 +- 3 files changed, 26 insertions(+), 223 deletions(-) diff --git a/qapi/migration.json b/qapi/migration.json index XXXXXXX..XXXXXXX 100644 --- a/qapi/migration.json +++ b/qapi/migration.json @@ -XXX,XX +XXX,XX @@ 'cpr-exec-command'] } ## -# @MigrateSetParameters: +# @migrate-set-parameters: +# +# Set migration parameters. All arguments are optional. +# +# Since: 2.4 +# +# .. qmp-example:: +# +# -> { "execute": "migrate-set-parameters" , +# "arguments": { "multifd-channels": 5 } } +# <- { "return": {} } +## +{ 'command': 'migrate-set-parameters', 'boxed': true, + 'data': 'MigrationParameters' } + +## +# @MigrationParameters: # # @announce-initial: Initial delay (in milliseconds) before sending # the first announce (Since 4.0) @@ -XXX,XX +XXX,XX @@ # @unstable: Members @x-checkpoint-delay and # @x-vcpu-dirty-limit-period are experimental. # -# TODO: either fuse back into `MigrationParameters`, or make -# `MigrationParameters` members mandatory -# -# Since: 2.4 -## -{ 'struct': 'MigrateSetParameters', - 'data': { '*announce-initial': 'size', - '*announce-max': 'size', - '*announce-rounds': 'size', - '*announce-step': 'size', - '*throttle-trigger-threshold': 'uint8', - '*cpu-throttle-initial': 'uint8', - '*cpu-throttle-increment': 'uint8', - '*cpu-throttle-tailslow': 'bool', - '*tls-creds': 'StrOrNull', - '*tls-hostname': 'StrOrNull', - '*tls-authz': 'StrOrNull', - '*max-bandwidth': 'size', - '*avail-switchover-bandwidth': 'size', - '*downtime-limit': 'uint64', - '*x-checkpoint-delay': { 'type': 'uint32', - 'features': [ 'unstable' ] }, - '*multifd-channels': 'uint8', - '*xbzrle-cache-size': 'size', - '*max-postcopy-bandwidth': 'size', - '*max-cpu-throttle': 'uint8', - '*multifd-compression': 'MultiFDCompression', - '*multifd-zlib-level': 'uint8', - '*multifd-qatzip-level': 'uint8', - '*multifd-zstd-level': 'uint8', - '*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ], - '*x-vcpu-dirty-limit-period': { 'type': 'uint64', - 'features': [ 'unstable' ] }, - '*vcpu-dirty-limit': 'uint64', - '*mode': 'MigMode', - '*zero-page-detection': 'ZeroPageDetection', - '*direct-io': 'bool', - '*cpr-exec-command': [ 'str' ]} } - -## -# @migrate-set-parameters: -# -# Set various migration parameters. -# -# Since: 2.4 -# -# .. qmp-example:: -# -# -> { "execute": "migrate-set-parameters" , -# "arguments": { "multifd-channels": 5 } } -# <- { "return": {} } -## -{ 'command': 'migrate-set-parameters', 'boxed': true, - 'data': 'MigrateSetParameters' } - -## -# @MigrationParameters: -# -# The optional members aren't actually optional. -# -# @announce-initial: Initial delay (in milliseconds) before sending -# the first announce (Since 4.0) -# -# @announce-max: Maximum delay (in milliseconds) between packets in -# the announcement (Since 4.0) -# -# @announce-rounds: Number of self-announce packets sent after -# migration (Since 4.0) -# -# @announce-step: Increase in delay (in milliseconds) between -# subsequent packets in the announcement (Since 4.0) -# -# @throttle-trigger-threshold: The ratio of bytes_dirty_period and -# bytes_xfer_period to trigger throttling. It is expressed as -# percentage. The default value is 50. (Since 5.0) -# -# @cpu-throttle-initial: Initial percentage of time guest cpus are -# throttled when migration auto-converge is activated. -# (Since 2.7) -# -# @cpu-throttle-increment: throttle percentage increase each time -# auto-converge detects that migration is not making progress. -# (Since 2.7) -# -# @cpu-throttle-tailslow: Make CPU throttling slower at tail stage. -# At the tail stage of throttling, the Guest is very sensitive to -# CPU percentage while the @cpu-throttle -increment is excessive -# usually at tail stage. If this parameter is true, we will -# compute the ideal CPU percentage used by the Guest, which may -# exactly make the dirty rate match the dirty rate threshold. -# Then we will choose a smaller throttle increment between the one -# specified by @cpu-throttle-increment and the one generated by -# ideal CPU percentage. Therefore, it is compatible to -# traditional throttling, meanwhile the throttle increment won't -# be excessive at tail stage. The default value is false. -# (Since 5.1) -# -# @tls-creds: ID of the 'tls-creds' object that provides credentials -# for establishing a TLS connection over the migration data -# channel. On the outgoing side of the migration, the credentials -# must be for a 'client' endpoint, while for the incoming side the -# credentials must be for a 'server' endpoint. An empty string -# means that QEMU will use plain text mode for migration, rather -# than TLS. (Since 2.7) -# -# Note: 2.8 omits empty @tls-creds instead. -# -# @tls-hostname: migration target's hostname for validating the -# server's x509 certificate identity. If empty, QEMU will use the -# hostname from the migration URI, if any. (Since 2.7) -# -# Note: 2.8 omits empty @tls-hostname instead. -# -# @tls-authz: ID of the 'authz' object subclass that provides access -# control checking of the TLS x509 certificate distinguished name. -# (Since 4.0) -# -# @max-bandwidth: maximum speed for migration, in bytes per second. -# (Since 2.8) -# -# @avail-switchover-bandwidth: to set the available bandwidth that -# migration can use during switchover phase. **Note:** this does -# not limit the bandwidth during switchover, but only for -# calculations when making decisions to switchover. By default, -# this value is zero, which means QEMU will estimate the bandwidth -# automatically. This can be set when the estimated value is not -# accurate, while the user is able to guarantee such bandwidth is -# available when switching over. When specified correctly, this -# can make the switchover decision much more accurate. -# (Since 8.2) -# -# @downtime-limit: set maximum tolerated downtime for migration. -# maximum downtime in milliseconds (Since 2.8) -# -# @x-checkpoint-delay: the delay time between two COLO checkpoints. -# (Since 2.8) -# -# @multifd-channels: Number of channels used to migrate data in -# parallel. This is the same number that the number of sockets -# used for migration. The default value is 2 (since 4.0) -# -# @xbzrle-cache-size: cache size to be used by XBZRLE migration. It -# needs to be a multiple of the target page size and a power of 2 -# (Since 2.11) -# -# @max-postcopy-bandwidth: Background transfer bandwidth during -# postcopy. Defaults to 0 (unlimited). In bytes per second. -# (Since 3.0) -# -# @max-cpu-throttle: maximum cpu throttle percentage. Defaults to 99. -# (Since 3.1) -# -# @multifd-compression: Which compression method to use. Defaults to -# none. (Since 5.0) -# -# @multifd-zlib-level: Set the compression level to be used in live -# migration, the compression level is an integer between 0 and 9, -# where 0 means no compression, 1 means the best compression -# speed, and 9 means best compression ratio which will consume -# more CPU. Defaults to 1. (Since 5.0) -# -# @multifd-qatzip-level: Set the compression level to be used in live -# migration. The level is an integer between 1 and 9, where 1 -# means the best compression speed, and 9 means the best -# compression ratio which will consume more CPU. Defaults to 1. -# (Since 9.2) -# -# @multifd-zstd-level: Set the compression level to be used in live -# migration, the compression level is an integer between 0 and 20, -# where 0 means no compression, 1 means the best compression -# speed, and 20 means best compression ratio which will consume -# more CPU. Defaults to 1. (Since 5.0) -# -# @block-bitmap-mapping: Maps block nodes and bitmaps on them to -# aliases for the purpose of dirty bitmap migration. Such aliases -# may for example be the corresponding names on the opposite site. -# The mapping must be one-to-one, but not necessarily complete: On -# the source, unmapped bitmaps and all bitmaps on unmapped nodes -# will be ignored. On the destination, encountering an unmapped -# alias in the incoming migration stream will result in a report, -# and all further bitmap migration data will then be discarded. -# Note that the destination does not know about bitmaps it does -# not receive, so there is no limitation or requirement regarding -# the number of bitmaps received, or how they are named, or on -# which nodes they are placed. By default (when this parameter -# has never been set), bitmap names are mapped to themselves. -# Nodes are mapped to their block device name if there is one, and -# to their node name otherwise. (Since 5.2) -# -# @x-vcpu-dirty-limit-period: Periodic time (in milliseconds) of dirty -# limit during live migration. Should be in the range 1 to -# 1000ms. Defaults to 1000ms. (Since 8.1) -# -# @vcpu-dirty-limit: Dirtyrate limit (MB/s) during live migration. -# Defaults to 1. (Since 8.1) -# -# @mode: Migration mode. See description in `MigMode`. Default is -# 'normal'. (Since 8.2) -# -# @zero-page-detection: Whether and how to detect zero pages. See -# description in `ZeroPageDetection`. Default is 'multifd'. -# (since 9.0) -# -# @direct-io: Open migration files with O_DIRECT when possible. This -# only has effect if the @mapped-ram capability is enabled. -# (Since 9.1) -# -# @cpr-exec-command: Command to start the new QEMU process when @mode -# is @cpr-exec. The first list element is the program's filename, -# the remainder its arguments. (Since 10.2) -# -# Features: -# -# @unstable: Members @x-checkpoint-delay and -# @x-vcpu-dirty-limit-period are experimental. -# # Since: 2.4 ## { 'struct': 'MigrationParameters', @@ -XXX,XX +XXX,XX @@ ## # @query-migrate-parameters: # -# Return information about the current migration parameters +# Return information about the current migration parameters. Optional +# members of the return value are always present, except +# @block-bitmap-mapping, which is only present if it has been +# previously set. # # Since: 2.4 # diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c index XXXXXXX..XXXXXXX 100644 --- a/migration/migration-hmp-cmds.c +++ b/migration/migration-hmp-cmds.c @@ -XXX,XX +XXX,XX @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict) const char *param = qdict_get_str(qdict, "parameter"); const char *valuestr = qdict_get_str(qdict, "value"); Visitor *v = string_input_visitor_new(valuestr); - MigrateSetParameters *p = g_new0(MigrateSetParameters, 1); + MigrationParameters *p = g_new0(MigrationParameters, 1); uint64_t valuebw = 0; uint64_t cache_size; Error *err = NULL; @@ -XXX,XX +XXX,XX @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict) qmp_migrate_set_parameters(p, &err); cleanup: - qapi_free_MigrateSetParameters(p); + qapi_free_MigrationParameters(p); visit_free(v); hmp_handle_error(mon, err); } diff --git a/migration/options.c b/migration/options.c index XXXXXXX..XXXXXXX 100644 --- a/migration/options.c +++ b/migration/options.c @@ -XXX,XX +XXX,XX @@ bool migrate_params_check(MigrationParameters *params, Error **errp) return true; } -static void migrate_params_test_apply(MigrateSetParameters *params, +static void migrate_params_test_apply(MigrationParameters *params, MigrationParameters *dest) { *dest = migrate_get_current()->parameters; @@ -XXX,XX +XXX,XX @@ static void migrate_params_test_apply(MigrateSetParameters *params, } } -static void migrate_params_apply(MigrateSetParameters *params, Error **errp) +static void migrate_params_apply(MigrationParameters *params, Error **errp) { MigrationState *s = migrate_get_current(); @@ -XXX,XX +XXX,XX @@ static void migrate_params_apply(MigrateSetParameters *params, Error **errp) } } -void qmp_migrate_set_parameters(MigrateSetParameters *params, Error **errp) +void qmp_migrate_set_parameters(MigrationParameters *params, Error **errp) { MigrationParameters tmp; -- 2.50.1
From: Fabiano Rosas <farosas@suse.de> The MigrationParameter (singular) enumeration is not part of the migration QMP API, it's only used for nicely converting HMP strings into MigrationParameters (plural) members and for providing readline completion. Documenting this enum only serves to duplicate documentation between MigrationParameter and MigrationParameters. Add an exception to QAPIs pragma.json and stop documenting it. The generated "QEMU QMP Reference Manual" now lists the enum members as "Not documented." Tolerable. Acked-by: Markus Armbruster <armbru@redhat.com> Acked-by: Peter Xu <peterx@redhat.com> Signed-off-by: Fabiano Rosas <farosas@suse.de> Link: https://lore.kernel.org/r/20251215220041.12657-8-farosas@suse.de Signed-off-by: Peter Xu <peterx@redhat.com> --- qapi/migration.json | 154 +------------------------------------------- qapi/pragma.json | 1 + 2 files changed, 3 insertions(+), 152 deletions(-) diff --git a/qapi/migration.json b/qapi/migration.json index XXXXXXX..XXXXXXX 100644 --- a/qapi/migration.json +++ b/qapi/migration.json @@ -XXX,XX +XXX,XX @@ ## # @MigrationParameter: # -# Migration parameters enumeration -# -# @announce-initial: Initial delay (in milliseconds) before sending -# the first announce (Since 4.0) -# -# @announce-max: Maximum delay (in milliseconds) between packets in -# the announcement (Since 4.0) -# -# @announce-rounds: Number of self-announce packets sent after -# migration (Since 4.0) -# -# @announce-step: Increase in delay (in milliseconds) between -# subsequent packets in the announcement (Since 4.0) -# -# @throttle-trigger-threshold: The ratio of bytes_dirty_period and -# bytes_xfer_period to trigger throttling. It is expressed as -# percentage. The default value is 50. (Since 5.0) -# -# @cpu-throttle-initial: Initial percentage of time guest cpus are -# throttled when migration auto-converge is activated. The -# default value is 20. (Since 2.7) -# -# @cpu-throttle-increment: throttle percentage increase each time -# auto-converge detects that migration is not making progress. -# The default value is 10. (Since 2.7) -# -# @cpu-throttle-tailslow: Make CPU throttling slower at tail stage. -# At the tail stage of throttling, the Guest is very sensitive to -# CPU percentage while the @cpu-throttle -increment is excessive -# usually at tail stage. If this parameter is true, we will -# compute the ideal CPU percentage used by the Guest, which may -# exactly make the dirty rate match the dirty rate threshold. -# Then we will choose a smaller throttle increment between the one -# specified by @cpu-throttle-increment and the one generated by -# ideal CPU percentage. Therefore, it is compatible to -# traditional throttling, meanwhile the throttle increment won't -# be excessive at tail stage. The default value is false. -# (Since 5.1) -# -# @tls-creds: ID of the 'tls-creds' object that provides credentials -# for establishing a TLS connection over the migration data -# channel. On the outgoing side of the migration, the credentials -# must be for a 'client' endpoint, while for the incoming side the -# credentials must be for a 'server' endpoint. Setting this to a -# non-empty string enables TLS for all migrations. An empty -# string means that QEMU will use plain text mode for migration, -# rather than TLS. (Since 2.7) -# -# @tls-hostname: migration target's hostname for validating the -# server's x509 certificate identity. If empty, QEMU will use the -# hostname from the migration URI, if any. A non-empty value is -# required when using x509 based TLS credentials and the migration -# URI does not include a hostname, such as fd: or exec: based -# migration. (Since 2.7) -# -# Note: empty value works only since 2.9. -# -# @tls-authz: ID of the 'authz' object subclass that provides access -# control checking of the TLS x509 certificate distinguished name. -# This object is only resolved at time of use, so can be deleted -# and recreated on the fly while the migration server is active. -# If missing, it will default to denying access (Since 4.0) -# -# @max-bandwidth: maximum speed for migration, in bytes per second. -# (Since 2.8) -# -# @avail-switchover-bandwidth: to set the available bandwidth that -# migration can use during switchover phase. **Note:** this does -# not limit the bandwidth during switchover, but only for -# calculations when making decisions to switchover. By default, -# this value is zero, which means QEMU will estimate the bandwidth -# automatically. This can be set when the estimated value is not -# accurate, while the user is able to guarantee such bandwidth is -# available when switching over. When specified correctly, this -# can make the switchover decision much more accurate. -# (Since 8.2) -# -# @downtime-limit: set maximum tolerated downtime for migration. -# maximum downtime in milliseconds (Since 2.8) -# -# @x-checkpoint-delay: The delay time (in ms) between two COLO -# checkpoints in periodic mode. (Since 2.8) -# -# @multifd-channels: Number of channels used to migrate data in -# parallel. This is the same number that the number of sockets -# used for migration. The default value is 2 (since 4.0) -# -# @xbzrle-cache-size: cache size to be used by XBZRLE migration. It -# needs to be a multiple of the target page size and a power of 2 -# (Since 2.11) -# -# @max-postcopy-bandwidth: Background transfer bandwidth during -# postcopy. Defaults to 0 (unlimited). In bytes per second. -# (Since 3.0) -# -# @max-cpu-throttle: maximum cpu throttle percentage. Defaults to 99. -# (Since 3.1) -# -# @multifd-compression: Which compression method to use. Defaults to -# none. (Since 5.0) -# -# @multifd-zlib-level: Set the compression level to be used in live -# migration, the compression level is an integer between 0 and 9, -# where 0 means no compression, 1 means the best compression -# speed, and 9 means best compression ratio which will consume -# more CPU. Defaults to 1. (Since 5.0) -# -# @multifd-qatzip-level: Set the compression level to be used in live -# migration. The level is an integer between 1 and 9, where 1 -# means the best compression speed, and 9 means the best -# compression ratio which will consume more CPU. Defaults to 1. -# (Since 9.2) -# -# @multifd-zstd-level: Set the compression level to be used in live -# migration, the compression level is an integer between 0 and 20, -# where 0 means no compression, 1 means the best compression -# speed, and 20 means best compression ratio which will consume -# more CPU. Defaults to 1. (Since 5.0) -# -# @block-bitmap-mapping: Maps block nodes and bitmaps on them to -# aliases for the purpose of dirty bitmap migration. Such aliases -# may for example be the corresponding names on the opposite site. -# The mapping must be one-to-one, but not necessarily complete: On -# the source, unmapped bitmaps and all bitmaps on unmapped nodes -# will be ignored. On the destination, encountering an unmapped -# alias in the incoming migration stream will result in a report, -# and all further bitmap migration data will then be discarded. -# Note that the destination does not know about bitmaps it does -# not receive, so there is no limitation or requirement regarding -# the number of bitmaps received, or how they are named, or on -# which nodes they are placed. By default (when this parameter -# has never been set), bitmap names are mapped to themselves. -# Nodes are mapped to their block device name if there is one, and -# to their node name otherwise. (Since 5.2) -# -# @x-vcpu-dirty-limit-period: Periodic time (in milliseconds) of dirty -# limit during live migration. Should be in the range 1 to -# 1000ms. Defaults to 1000ms. (Since 8.1) -# -# @vcpu-dirty-limit: Dirtyrate limit (MB/s) during live migration. -# Defaults to 1. (Since 8.1) -# -# @mode: Migration mode. See description in `MigMode`. Default is -# 'normal'. (Since 8.2) -# -# @zero-page-detection: Whether and how to detect zero pages. See -# description in `ZeroPageDetection`. Default is 'multifd'. -# (since 9.0) -# -# @direct-io: Open migration files with O_DIRECT when possible. This -# only has effect if the @mapped-ram capability is enabled. -# (Since 9.1) +# Migration parameters enumeration. The enumeration values mirror the +# members of @MigrationParameters. # # @cpr-exec-command: Command to start the new QEMU process when @mode # is @cpr-exec. The first list element is the program's filename, diff --git a/qapi/pragma.json b/qapi/pragma.json index XXXXXXX..XXXXXXX 100644 --- a/qapi/pragma.json +++ b/qapi/pragma.json @@ -XXX,XX +XXX,XX @@ 'IscsiTransport', 'KeyValueKind', 'MemoryDeviceInfoKind', + 'MigrationParameter', 'NetClientDriver', 'ObjectType', 'QKeyCode', -- 2.50.1
From: Fabiano Rosas <farosas@suse.de> Some migration parameters are updated immediately once they are set via migrate-set-parameters. Move that work outside of migrate_params_apply() and leave that function with the single responsibility of setting s->parameters and not doing any side-effects. Reviewed-by: Peter Xu <peterx@redhat.com> Signed-off-by: Fabiano Rosas <farosas@suse.de> Link: https://lore.kernel.org/r/20251215220041.12657-9-farosas@suse.de Signed-off-by: Peter Xu <peterx@redhat.com> --- migration/options.c | 38 ++++++++++++++++++++++++++++---------- migration/ram.c | 2 +- 2 files changed, 29 insertions(+), 11 deletions(-) diff --git a/migration/options.c b/migration/options.c index XXXXXXX..XXXXXXX 100644 --- a/migration/options.c +++ b/migration/options.c @@ -XXX,XX +XXX,XX @@ void migrate_params_init(MigrationParameters *params) params->has_cpr_exec_command = true; } +static void migrate_post_update_params(MigrationParameters *new, Error **errp) +{ + MigrationState *s = migrate_get_current(); + + if (new->has_max_bandwidth) { + if (s->to_dst_file && !migration_in_postcopy()) { + migration_rate_set(new->max_bandwidth); + } + } + + if (new->has_x_checkpoint_delay) { + colo_checkpoint_delay_set(); + } + + if (new->has_xbzrle_cache_size) { + xbzrle_cache_resize(new->xbzrle_cache_size, errp); + } + + if (new->has_max_postcopy_bandwidth) { + if (s->to_dst_file && migration_in_postcopy()) { + migration_rate_set(new->max_postcopy_bandwidth); + } + } +} + /* * Check whether the parameters are valid. Error will be put into errp * (if provided). Return true if valid, otherwise false. @@ -XXX,XX +XXX,XX @@ static void migrate_params_test_apply(MigrationParameters *params, } } -static void migrate_params_apply(MigrationParameters *params, Error **errp) +static void migrate_params_apply(MigrationParameters *params) { MigrationState *s = migrate_get_current(); @@ -XXX,XX +XXX,XX @@ static void migrate_params_apply(MigrationParameters *params, Error **errp) if (params->has_max_bandwidth) { s->parameters.max_bandwidth = params->max_bandwidth; - if (s->to_dst_file && !migration_in_postcopy()) { - migration_rate_set(s->parameters.max_bandwidth); - } } if (params->has_avail_switchover_bandwidth) { @@ -XXX,XX +XXX,XX @@ static void migrate_params_apply(MigrationParameters *params, Error **errp) if (params->has_x_checkpoint_delay) { s->parameters.x_checkpoint_delay = params->x_checkpoint_delay; - colo_checkpoint_delay_set(); } if (params->has_multifd_channels) { @@ -XXX,XX +XXX,XX @@ static void migrate_params_apply(MigrationParameters *params, Error **errp) } if (params->has_xbzrle_cache_size) { s->parameters.xbzrle_cache_size = params->xbzrle_cache_size; - xbzrle_cache_resize(params->xbzrle_cache_size, errp); } if (params->has_max_postcopy_bandwidth) { s->parameters.max_postcopy_bandwidth = params->max_postcopy_bandwidth; - if (s->to_dst_file && migration_in_postcopy()) { - migration_rate_set(s->parameters.max_postcopy_bandwidth); - } } if (params->has_max_cpu_throttle) { s->parameters.max_cpu_throttle = params->max_cpu_throttle; @@ -XXX,XX +XXX,XX @@ void qmp_migrate_set_parameters(MigrationParameters *params, Error **errp) migrate_params_test_apply(params, &tmp); if (migrate_params_check(&tmp, errp)) { - migrate_params_apply(params, errp); + migrate_params_apply(params); + migrate_post_update_params(params, errp); } migrate_tls_opts_free(&tmp); diff --git a/migration/ram.c b/migration/ram.c index XXXXXXX..XXXXXXX 100644 --- a/migration/ram.c +++ b/migration/ram.c @@ -XXX,XX +XXX,XX @@ static void XBZRLE_cache_unlock(void) /** * xbzrle_cache_resize: resize the xbzrle cache * - * This function is called from migrate_params_apply in main + * This function is called from migrate_post_update_params in main * thread, possibly while a migration is in progress. A running * migration may be using the cache and might finish during this call, * hence changes to the cache are protected by XBZRLE.lock(). -- 2.50.1
From: Fabiano Rosas <farosas@suse.de> The QAPI converts an empty list on the block-bitmap-mapping input into a NULL BitmapMigrationNodeAliasList. The empty list is a valid input for the block-bitmap-mapping option, so commit 3cba22c9ad ("migration: Fix block_bitmap_mapping migration") started using the s->parameters.has_block_bitmap_mapping field to tell when the user has passed in an empty list vs. when no list has been passed at all. Using s->parameters.has_block_bitmap_mapping field is only possible because MigrationParameters has had its members made optional due to historical reasons. In order to make improvements to the way configuration options are set for a migration, we'd like to reduce the open-coded usage of the has_* fields of the global configuration object (s->parameters). Add a separate boolean to track the status of the block_bitmap_mapping option. No functional change intended. (this was verified to not regress iotest 300, which is the test that 3cba22c9ad refers to) CC: Kevin Wolf <kwolf@redhat.com> Acked-by: Peter Xu <peterx@redhat.com> Signed-off-by: Fabiano Rosas <farosas@suse.de> Link: https://lore.kernel.org/r/20251215220041.12657-10-farosas@suse.de Signed-off-by: Peter Xu <peterx@redhat.com> --- migration/migration.h | 7 +++++++ migration/migration-hmp-cmds.c | 3 ++- migration/options.c | 6 +++--- 3 files changed, 12 insertions(+), 4 deletions(-) diff --git a/migration/migration.h b/migration/migration.h index XXXXXXX..XXXXXXX 100644 --- a/migration/migration.h +++ b/migration/migration.h @@ -XXX,XX +XXX,XX @@ struct MigrationState { QemuEvent postcopy_package_loaded_event; GSource *hup_source; + + /* + * The block-bitmap-mapping option is allowed to be an empty list, + * therefore we need a way to know whether the user has given + * anything as input. + */ + bool has_block_bitmap_mapping; }; void migrate_set_state(MigrationStatus *state, MigrationStatus old_state, diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c index XXXXXXX..XXXXXXX 100644 --- a/migration/migration-hmp-cmds.c +++ b/migration/migration-hmp-cmds.c @@ -XXX,XX +XXX,XX @@ static void monitor_print_cpr_exec_command(Monitor *mon, strList *args) void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict) { MigrationParameters *params; + MigrationState *s = migrate_get_current(); params = qmp_query_migrate_parameters(NULL); @@ -XXX,XX +XXX,XX @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict) MigrationParameter_str(MIGRATION_PARAMETER_XBZRLE_CACHE_SIZE), params->xbzrle_cache_size); - if (params->has_block_bitmap_mapping) { + if (s->has_block_bitmap_mapping) { const BitmapMigrationNodeAliasList *bmnal; monitor_printf(mon, "%s:\n", diff --git a/migration/options.c b/migration/options.c index XXXXXXX..XXXXXXX 100644 --- a/migration/options.c +++ b/migration/options.c @@ -XXX,XX +XXX,XX @@ bool migrate_has_block_bitmap_mapping(void) { MigrationState *s = migrate_get_current(); - return s->parameters.has_block_bitmap_mapping; + return s->has_block_bitmap_mapping; } uint32_t migrate_checkpoint_delay(void) @@ -XXX,XX +XXX,XX @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp) params->has_announce_step = true; params->announce_step = s->parameters.announce_step; - if (s->parameters.has_block_bitmap_mapping) { + if (s->has_block_bitmap_mapping) { params->has_block_bitmap_mapping = true; params->block_bitmap_mapping = QAPI_CLONE(BitmapMigrationNodeAliasList, @@ -XXX,XX +XXX,XX @@ static void migrate_params_apply(MigrationParameters *params) qapi_free_BitmapMigrationNodeAliasList( s->parameters.block_bitmap_mapping); - s->parameters.has_block_bitmap_mapping = true; + s->has_block_bitmap_mapping = true; s->parameters.block_bitmap_mapping = QAPI_CLONE(BitmapMigrationNodeAliasList, params->block_bitmap_mapping); -- 2.50.1
From: Fabiano Rosas <farosas@suse.de> The migration parameters validation produces a temporary structure which is the merge of the current parameter values (s->parameters, MigrationParameters) with the new parameters set by the user (former MigrateSetParameters). When copying the values from s->parameters into the temporary structure, the has_* fields are copied along, but when merging the user-input values they are not. During migrate_params_check(), only the parameters that have the corresponding has_* field will be checked, so only the parameters that were initialized in migrate_params_init() will be validated. This causes (almost) all of the migration parameters to be validated every time a parameter is set, regardless of which fields the user touched, but it also skips validation of any values that are not set in migrate_params_init(). It's not clear what was the intention of the original code, whether to validate all fields always, or only validate what the user input changed. Since the current situation is closer to the former option, make the choice of validating all parameters by removing the checks for the has_* fields when validating. Note that bringing the user input into the temporary structure for validation still needs to look at the has_* fields, otherwise any parameters not set by the user (i.e. 0) would override the corresponding value in s->parameters. The empty migrate_params_init() will be kept because subsequent patches will add code to it. Signed-off-by: Fabiano Rosas <farosas@suse.de> Link: https://lore.kernel.org/r/20251215220041.12657-11-farosas@suse.de Signed-off-by: Peter Xu <peterx@redhat.com> --- migration/migration.c | 2 +- migration/options.c | 102 ++++++++++++------------------------------ 2 files changed, 30 insertions(+), 74 deletions(-) diff --git a/migration/migration.c b/migration/migration.c index XXXXXXX..XXXXXXX 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -XXX,XX +XXX,XX @@ static bool migrate_prepare(MigrationState *s, bool resume, Error **errp) } if (migrate_mode() == MIG_MODE_CPR_EXEC && - !s->parameters.has_cpr_exec_command) { + !s->parameters.cpr_exec_command) { error_setg(errp, "cpr-exec mode requires setting cpr-exec-command"); return false; } diff --git a/migration/options.c b/migration/options.c index XXXXXXX..XXXXXXX 100644 --- a/migration/options.c +++ b/migration/options.c @@ -XXX,XX +XXX,XX @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp) void migrate_params_init(MigrationParameters *params) { - /* Set has_* up only for parameter checks */ - params->has_throttle_trigger_threshold = true; - params->has_cpu_throttle_initial = true; - params->has_cpu_throttle_increment = true; - params->has_cpu_throttle_tailslow = true; - params->has_max_bandwidth = true; - params->has_downtime_limit = true; - params->has_x_checkpoint_delay = true; - params->has_multifd_channels = true; - params->has_multifd_compression = true; - params->has_multifd_zlib_level = true; - params->has_multifd_qatzip_level = true; - params->has_multifd_zstd_level = true; - params->has_xbzrle_cache_size = true; - params->has_max_postcopy_bandwidth = true; - params->has_max_cpu_throttle = true; - params->has_announce_initial = true; - params->has_announce_max = true; - params->has_announce_rounds = true; - params->has_announce_step = true; - params->has_x_vcpu_dirty_limit_period = true; - params->has_vcpu_dirty_limit = true; - params->has_mode = true; - params->has_zero_page_detection = true; - params->has_direct_io = true; - params->has_cpr_exec_command = true; } static void migrate_post_update_params(MigrationParameters *new, Error **errp) @@ -XXX,XX +XXX,XX @@ bool migrate_params_check(MigrationParameters *params, Error **errp) { ERRP_GUARD(); - if (params->has_throttle_trigger_threshold && - (params->throttle_trigger_threshold < 1 || - params->throttle_trigger_threshold > 100)) { + if (params->throttle_trigger_threshold < 1 || + params->throttle_trigger_threshold > 100) { error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "throttle_trigger_threshold", "an integer in the range of 1 to 100"); return false; } - if (params->has_cpu_throttle_initial && - (params->cpu_throttle_initial < 1 || - params->cpu_throttle_initial > 99)) { + if (params->cpu_throttle_initial < 1 || + params->cpu_throttle_initial > 99) { error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cpu_throttle_initial", "an integer in the range of 1 to 99"); return false; } - if (params->has_cpu_throttle_increment && - (params->cpu_throttle_increment < 1 || - params->cpu_throttle_increment > 99)) { + if (params->cpu_throttle_increment < 1 || + params->cpu_throttle_increment > 99) { error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cpu_throttle_increment", "an integer in the range of 1 to 99"); return false; } - if (params->has_max_bandwidth && (params->max_bandwidth > SIZE_MAX)) { + if (params->max_bandwidth > SIZE_MAX) { error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "max_bandwidth", "an integer in the range of 0 to "stringify(SIZE_MAX) @@ -XXX,XX +XXX,XX @@ bool migrate_params_check(MigrationParameters *params, Error **errp) return false; } - if (params->has_avail_switchover_bandwidth && - (params->avail_switchover_bandwidth > SIZE_MAX)) { + if (params->avail_switchover_bandwidth > SIZE_MAX) { error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "avail_switchover_bandwidth", "an integer in the range of 0 to "stringify(SIZE_MAX) @@ -XXX,XX +XXX,XX @@ bool migrate_params_check(MigrationParameters *params, Error **errp) return false; } - if (params->has_downtime_limit && - (params->downtime_limit > MAX_MIGRATE_DOWNTIME)) { + if (params->downtime_limit > MAX_MIGRATE_DOWNTIME) { error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "downtime_limit", "an integer in the range of 0 to " @@ -XXX,XX +XXX,XX @@ bool migrate_params_check(MigrationParameters *params, Error **errp) /* x_checkpoint_delay is now always positive */ - if (params->has_multifd_channels && (params->multifd_channels < 1)) { + if (params->multifd_channels < 1) { error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "multifd_channels", "a value between 1 and 255"); return false; } - if (params->has_multifd_zlib_level && - (params->multifd_zlib_level > 9)) { + if (params->multifd_zlib_level > 9) { error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "multifd_zlib_level", "a value between 0 and 9"); return false; } - if (params->has_multifd_qatzip_level && - ((params->multifd_qatzip_level > 9) || - (params->multifd_qatzip_level < 1))) { + if (params->multifd_qatzip_level > 9 || + params->multifd_qatzip_level < 1) { error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "multifd_qatzip_level", "a value between 1 and 9"); return false; } - if (params->has_multifd_zstd_level && - (params->multifd_zstd_level > 20)) { + if (params->multifd_zstd_level > 20) { error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "multifd_zstd_level", "a value between 0 and 20"); return false; } - if (params->has_xbzrle_cache_size && - (params->xbzrle_cache_size < qemu_target_page_size() || - !is_power_of_2(params->xbzrle_cache_size))) { + if (params->xbzrle_cache_size < qemu_target_page_size() || + !is_power_of_2(params->xbzrle_cache_size)) { error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "xbzrle_cache_size", "a power of two no less than the target page size"); return false; } - if (params->has_max_cpu_throttle && - (params->max_cpu_throttle < params->cpu_throttle_initial || - params->max_cpu_throttle > 99)) { + if (params->max_cpu_throttle < params->cpu_throttle_initial || + params->max_cpu_throttle > 99) { error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "max_cpu_throttle", "an integer in the range of cpu_throttle_initial to 99"); return false; } - if (params->has_announce_initial && - params->announce_initial > 100000) { + if (params->announce_initial > 100000) { error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "announce_initial", "a value between 0 and 100000"); return false; } - if (params->has_announce_max && - params->announce_max > 100000) { + if (params->announce_max > 100000) { error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "announce_max", "a value between 0 and 100000"); return false; } - if (params->has_announce_rounds && - params->announce_rounds > 1000) { + if (params->announce_rounds > 1000) { error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "announce_rounds", "a value between 0 and 1000"); return false; } - if (params->has_announce_step && - (params->announce_step < 1 || - params->announce_step > 10000)) { + if (params->announce_step < 1 || + params->announce_step > 10000) { error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "announce_step", "a value between 0 and 10000"); return false; } - if (params->has_block_bitmap_mapping && - !check_dirty_bitmap_mig_alias_map(params->block_bitmap_mapping, errp)) { + if (!check_dirty_bitmap_mig_alias_map(params->block_bitmap_mapping, errp)) { error_prepend(errp, "Invalid mapping given for block-bitmap-mapping: "); return false; } #ifdef CONFIG_LINUX if (migrate_zero_copy_send() && - ((params->has_multifd_compression && params->multifd_compression) || - *params->tls_creds->u.s)) { + (params->multifd_compression || *params->tls_creds->u.s)) { error_setg(errp, "Zero copy only available for non-compressed non-TLS multifd migration"); return false; @@ -XXX,XX +XXX,XX @@ bool migrate_params_check(MigrationParameters *params, Error **errp) return false; } - if (params->has_x_vcpu_dirty_limit_period && - (params->x_vcpu_dirty_limit_period < 1 || - params->x_vcpu_dirty_limit_period > 1000)) { + if (params->x_vcpu_dirty_limit_period < 1 || + params->x_vcpu_dirty_limit_period > 1000) { error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "x-vcpu-dirty-limit-period", "a value between 1 and 1000"); return false; } - if (params->has_vcpu_dirty_limit && - (params->vcpu_dirty_limit < 1)) { + if (params->vcpu_dirty_limit < 1) { error_setg(errp, "Parameter 'vcpu_dirty_limit' must be greater than 1 MB/s"); return false; } - if (params->has_direct_io && params->direct_io && !qemu_has_direct_io()) { + if (params->direct_io && !qemu_has_direct_io()) { error_setg(errp, "No build-time support for direct-io"); return false; } -- 2.50.1
From: Fabiano Rosas <farosas@suse.de> The QERR_INVALID_PARAMETER_VALUE macro is documented as not to be used in new code. Remove the usage from migration/options.c. Reviewed-by: Peter Xu <peterx@redhat.com> Signed-off-by: Fabiano Rosas <farosas@suse.de> Link: https://lore.kernel.org/r/20251215220041.12657-12-farosas@suse.de Signed-off-by: Peter Xu <peterx@redhat.com> --- migration/migration.c | 3 +-- migration/options.c | 56 +++++++++++++++--------------------------- migration/page_cache.c | 6 ++--- migration/ram.c | 3 +-- 4 files changed, 24 insertions(+), 44 deletions(-) diff --git a/migration/migration.c b/migration/migration.c index XXXXXXX..XXXXXXX 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -XXX,XX +XXX,XX @@ static void qmp_migrate_finish(MigrationAddress *addr, bool resume_requested, } else if (addr->transport == MIGRATION_ADDRESS_TYPE_FILE) { file_start_outgoing_migration(s, &addr->u.file, &local_err); } else { - error_setg(&local_err, QERR_INVALID_PARAMETER_VALUE, "uri", - "a valid migration protocol"); + error_setg(&local_err, "uri is not a valid migration protocol"); migrate_set_state(&s->state, MIGRATION_STATUS_SETUP, MIGRATION_STATUS_FAILED); } diff --git a/migration/options.c b/migration/options.c index XXXXXXX..XXXXXXX 100644 --- a/migration/options.c +++ b/migration/options.c @@ -XXX,XX +XXX,XX @@ bool migrate_params_check(MigrationParameters *params, Error **errp) if (params->throttle_trigger_threshold < 1 || params->throttle_trigger_threshold > 100) { - error_setg(errp, QERR_INVALID_PARAMETER_VALUE, - "throttle_trigger_threshold", + error_setg(errp, "Option throttle_trigger_threshold expects " "an integer in the range of 1 to 100"); return false; } if (params->cpu_throttle_initial < 1 || params->cpu_throttle_initial > 99) { - error_setg(errp, QERR_INVALID_PARAMETER_VALUE, - "cpu_throttle_initial", + error_setg(errp, "Option cpu_throttle_initial expects " "an integer in the range of 1 to 99"); return false; } if (params->cpu_throttle_increment < 1 || params->cpu_throttle_increment > 99) { - error_setg(errp, QERR_INVALID_PARAMETER_VALUE, - "cpu_throttle_increment", + error_setg(errp, "Option cpu_throttle_increment expects " "an integer in the range of 1 to 99"); return false; } if (params->max_bandwidth > SIZE_MAX) { - error_setg(errp, QERR_INVALID_PARAMETER_VALUE, - "max_bandwidth", + error_setg(errp, "Option max_bandwidth expects " "an integer in the range of 0 to "stringify(SIZE_MAX) " bytes/second"); return false; } if (params->avail_switchover_bandwidth > SIZE_MAX) { - error_setg(errp, QERR_INVALID_PARAMETER_VALUE, - "avail_switchover_bandwidth", + error_setg(errp, "Option avail_switchover_bandwidth expects " "an integer in the range of 0 to "stringify(SIZE_MAX) " bytes/second"); return false; } if (params->downtime_limit > MAX_MIGRATE_DOWNTIME) { - error_setg(errp, QERR_INVALID_PARAMETER_VALUE, - "downtime_limit", + error_setg(errp, "Option downtime_limit expects " "an integer in the range of 0 to " stringify(MAX_MIGRATE_DOWNTIME)" ms"); return false; } - /* x_checkpoint_delay is now always positive */ - if (params->multifd_channels < 1) { - error_setg(errp, QERR_INVALID_PARAMETER_VALUE, - "multifd_channels", + error_setg(errp, "Option multifd_channels expects " "a value between 1 and 255"); return false; } if (params->multifd_zlib_level > 9) { - error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "multifd_zlib_level", + error_setg(errp, "Option multifd_zlib_level expects " "a value between 0 and 9"); return false; } if (params->multifd_qatzip_level > 9 || params->multifd_qatzip_level < 1) { - error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "multifd_qatzip_level", + error_setg(errp, "Option multifd_qatzip_level expects " "a value between 1 and 9"); return false; } if (params->multifd_zstd_level > 20) { - error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "multifd_zstd_level", + error_setg(errp, "Option multifd_zstd_level expects " "a value between 0 and 20"); return false; } if (params->xbzrle_cache_size < qemu_target_page_size() || !is_power_of_2(params->xbzrle_cache_size)) { - error_setg(errp, QERR_INVALID_PARAMETER_VALUE, - "xbzrle_cache_size", + error_setg(errp, "Option xbzrle_cache_size expects " "a power of two no less than the target page size"); return false; } if (params->max_cpu_throttle < params->cpu_throttle_initial || params->max_cpu_throttle > 99) { - error_setg(errp, QERR_INVALID_PARAMETER_VALUE, - "max_cpu_throttle", + error_setg(errp, "max_Option cpu_throttle expects " "an integer in the range of cpu_throttle_initial to 99"); return false; } if (params->announce_initial > 100000) { - error_setg(errp, QERR_INVALID_PARAMETER_VALUE, - "announce_initial", + error_setg(errp, "Option announce_initial expects " "a value between 0 and 100000"); return false; } if (params->announce_max > 100000) { - error_setg(errp, QERR_INVALID_PARAMETER_VALUE, - "announce_max", + error_setg(errp, "Option announce_max expects " "a value between 0 and 100000"); - return false; + return false; } if (params->announce_rounds > 1000) { - error_setg(errp, QERR_INVALID_PARAMETER_VALUE, - "announce_rounds", + error_setg(errp, "Option announce_rounds expects " "a value between 0 and 1000"); - return false; + return false; } if (params->announce_step < 1 || params->announce_step > 10000) { - error_setg(errp, QERR_INVALID_PARAMETER_VALUE, - "announce_step", + error_setg(errp, "Option announce_step expects " "a value between 0 and 10000"); - return false; + return false; } if (!check_dirty_bitmap_mig_alias_map(params->block_bitmap_mapping, errp)) { @@ -XXX,XX +XXX,XX @@ bool migrate_params_check(MigrationParameters *params, Error **errp) if (params->x_vcpu_dirty_limit_period < 1 || params->x_vcpu_dirty_limit_period > 1000) { - error_setg(errp, QERR_INVALID_PARAMETER_VALUE, - "x-vcpu-dirty-limit-period", + error_setg(errp, "Option x-vcpu-dirty-limit-period expects " "a value between 1 and 1000"); return false; } diff --git a/migration/page_cache.c b/migration/page_cache.c index XXXXXXX..XXXXXXX 100644 --- a/migration/page_cache.c +++ b/migration/page_cache.c @@ -XXX,XX +XXX,XX @@ PageCache *cache_init(uint64_t new_size, size_t page_size, Error **errp) PageCache *cache; if (new_size < page_size) { - error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cache size", - "is smaller than one target page size"); + error_setg(errp, "cache size is smaller than target page size"); return NULL; } /* round down to the nearest power of 2 */ if (!is_power_of_2(num_pages)) { - error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cache size", - "is not a power of two number of pages"); + error_setg(errp, "number of pages is not a power of two"); return NULL; } diff --git a/migration/ram.c b/migration/ram.c index XXXXXXX..XXXXXXX 100644 --- a/migration/ram.c +++ b/migration/ram.c @@ -XXX,XX +XXX,XX @@ int xbzrle_cache_resize(uint64_t new_size, Error **errp) /* Check for truncation */ if (new_size != (size_t)new_size) { - error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cache size", - "exceeding address space"); + error_setg(errp, "xbzrle cache size integer overflow"); return -1; } -- 2.50.1
From: Fabiano Rosas <farosas@suse.de> MigrationParameters needs to have all of its has_* fields marked as true when used as the return of query_migrate_parameters because the corresponding QMP command has all of its members non-optional by design, despite them being marked as optional in migration.json. Extract this code into a function and make it assert if any field is missing. With this we ensure future changes will not inadvertently leave any parameters missing. Note that the block-bitmap-mapping is a special case because the empty list is considered a valid value, so it has historically not been present in the command's output if it has never been set. CC: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Fabiano Rosas <farosas@suse.de> Link: https://lore.kernel.org/r/20251215220041.12657-13-farosas@suse.de Signed-off-by: Peter Xu <peterx@redhat.com> --- migration/options.c | 89 ++++++++++++++++++++++++++++----------------- 1 file changed, 55 insertions(+), 34 deletions(-) diff --git a/migration/options.c b/migration/options.c index XXXXXXX..XXXXXXX 100644 --- a/migration/options.c +++ b/migration/options.c @@ -XXX,XX +XXX,XX @@ static void tls_opt_to_str(StrOrNull *opt) opt->u.s = g_strdup(""); } +/* + * query-migrate-parameters expects all members of MigrationParameters + * to be present, but we cannot mark them non-optional in QAPI because + * the structure is also used for migrate-set-parameters, which needs + * the optionality. Force all parameters to be seen as present + * now. Note that this depends on some form of default being set for + * every member of MigrationParameters, currently done during qdev + * init using migration_properties defined in this file. The TLS + * options are a special case because they don't have a default and + * need to be normalized before use. + */ +static void migrate_mark_all_params_present(MigrationParameters *p) +{ + int len, n_str_args = 3; /* tls-creds, tls-hostname, tls-authz */ + bool *has_fields[] = { + &p->has_throttle_trigger_threshold, &p->has_cpu_throttle_initial, + &p->has_cpu_throttle_increment, &p->has_cpu_throttle_tailslow, + &p->has_max_bandwidth, &p->has_avail_switchover_bandwidth, + &p->has_downtime_limit, &p->has_x_checkpoint_delay, + &p->has_multifd_channels, &p->has_multifd_compression, + &p->has_multifd_zlib_level, &p->has_multifd_qatzip_level, + &p->has_multifd_zstd_level, &p->has_xbzrle_cache_size, + &p->has_max_postcopy_bandwidth, &p->has_max_cpu_throttle, + &p->has_announce_initial, &p->has_announce_max, &p->has_announce_rounds, + &p->has_announce_step, &p->has_block_bitmap_mapping, + &p->has_x_vcpu_dirty_limit_period, &p->has_vcpu_dirty_limit, + &p->has_mode, &p->has_zero_page_detection, &p->has_direct_io, + &p->has_cpr_exec_command, + }; + + len = ARRAY_SIZE(has_fields); + assert(len + n_str_args == MIGRATION_PARAMETER__MAX); + + for (int i = 0; i < len; i++) { + *has_fields[i] = true; + } +} + MigrationParameters *qmp_query_migrate_parameters(Error **errp) { MigrationParameters *params; @@ -XXX,XX +XXX,XX @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp) /* TODO use QAPI_CLONE() instead of duplicating it inline */ params = g_malloc0(sizeof(*params)); - params->has_throttle_trigger_threshold = true; + params->throttle_trigger_threshold = s->parameters.throttle_trigger_threshold; - params->has_cpu_throttle_initial = true; params->cpu_throttle_initial = s->parameters.cpu_throttle_initial; - params->has_cpu_throttle_increment = true; params->cpu_throttle_increment = s->parameters.cpu_throttle_increment; - params->has_cpu_throttle_tailslow = true; params->cpu_throttle_tailslow = s->parameters.cpu_throttle_tailslow; params->tls_creds = QAPI_CLONE(StrOrNull, s->parameters.tls_creds); params->tls_hostname = QAPI_CLONE(StrOrNull, s->parameters.tls_hostname); params->tls_authz = QAPI_CLONE(StrOrNull, s->parameters.tls_authz); - params->has_max_bandwidth = true; params->max_bandwidth = s->parameters.max_bandwidth; - params->has_avail_switchover_bandwidth = true; params->avail_switchover_bandwidth = s->parameters.avail_switchover_bandwidth; - params->has_downtime_limit = true; params->downtime_limit = s->parameters.downtime_limit; - params->has_x_checkpoint_delay = true; params->x_checkpoint_delay = s->parameters.x_checkpoint_delay; - params->has_multifd_channels = true; params->multifd_channels = s->parameters.multifd_channels; - params->has_multifd_compression = true; params->multifd_compression = s->parameters.multifd_compression; - params->has_multifd_zlib_level = true; params->multifd_zlib_level = s->parameters.multifd_zlib_level; - params->has_multifd_qatzip_level = true; params->multifd_qatzip_level = s->parameters.multifd_qatzip_level; - params->has_multifd_zstd_level = true; params->multifd_zstd_level = s->parameters.multifd_zstd_level; - params->has_xbzrle_cache_size = true; params->xbzrle_cache_size = s->parameters.xbzrle_cache_size; - params->has_max_postcopy_bandwidth = true; params->max_postcopy_bandwidth = s->parameters.max_postcopy_bandwidth; - params->has_max_cpu_throttle = true; params->max_cpu_throttle = s->parameters.max_cpu_throttle; - params->has_announce_initial = true; params->announce_initial = s->parameters.announce_initial; - params->has_announce_max = true; params->announce_max = s->parameters.announce_max; - params->has_announce_rounds = true; params->announce_rounds = s->parameters.announce_rounds; - params->has_announce_step = true; params->announce_step = s->parameters.announce_step; - - if (s->has_block_bitmap_mapping) { - params->has_block_bitmap_mapping = true; - params->block_bitmap_mapping = - QAPI_CLONE(BitmapMigrationNodeAliasList, - s->parameters.block_bitmap_mapping); - } - - params->has_x_vcpu_dirty_limit_period = true; params->x_vcpu_dirty_limit_period = s->parameters.x_vcpu_dirty_limit_period; - params->has_vcpu_dirty_limit = true; params->vcpu_dirty_limit = s->parameters.vcpu_dirty_limit; - params->has_mode = true; params->mode = s->parameters.mode; - params->has_zero_page_detection = true; params->zero_page_detection = s->parameters.zero_page_detection; - params->has_direct_io = true; params->direct_io = s->parameters.direct_io; - params->has_cpr_exec_command = true; params->cpr_exec_command = QAPI_CLONE(strList, s->parameters.cpr_exec_command); + params->block_bitmap_mapping = + QAPI_CLONE(BitmapMigrationNodeAliasList, + s->parameters.block_bitmap_mapping); + + migrate_mark_all_params_present(params); + + /* + * The block-bitmap-mapping breaks the expected API of + * query-migrate-parameters of having all members present. To keep + * compatibility, only emit this field if it's actually been + * set. The empty list is a valid value. + */ + if (!s->has_block_bitmap_mapping) { + params->has_block_bitmap_mapping = false; + qapi_free_BitmapMigrationNodeAliasList(params->block_bitmap_mapping); + } return params; } -- 2.50.1
From: Fabiano Rosas <farosas@suse.de> QAPI_CLONE_MEMBERS is a better option than copying parameters one by one because it operates on the entire struct and follows pointers. It also avoids the need to alter this function every time a new parameter is added. For this to work, the has_* fields of s->parameters need to be already set beforehand, so move migrate_mark_all_params_present() to the init routine. Reviewed-by: Peter Xu <peterx@redhat.com> Signed-off-by: Fabiano Rosas <farosas@suse.de> Link: https://lore.kernel.org/r/20251215220041.12657-14-farosas@suse.de Signed-off-by: Peter Xu <peterx@redhat.com> --- migration/options.c | 43 +++---------------------------------------- 1 file changed, 3 insertions(+), 40 deletions(-) diff --git a/migration/options.c b/migration/options.c index XXXXXXX..XXXXXXX 100644 --- a/migration/options.c +++ b/migration/options.c @@ -XXX,XX +XXX,XX @@ static void migrate_mark_all_params_present(MigrationParameters *p) MigrationParameters *qmp_query_migrate_parameters(Error **errp) { - MigrationParameters *params; MigrationState *s = migrate_get_current(); - - /* TODO use QAPI_CLONE() instead of duplicating it inline */ - params = g_malloc0(sizeof(*params)); - - params->throttle_trigger_threshold = s->parameters.throttle_trigger_threshold; - params->cpu_throttle_initial = s->parameters.cpu_throttle_initial; - params->cpu_throttle_increment = s->parameters.cpu_throttle_increment; - params->cpu_throttle_tailslow = s->parameters.cpu_throttle_tailslow; - params->tls_creds = QAPI_CLONE(StrOrNull, s->parameters.tls_creds); - params->tls_hostname = QAPI_CLONE(StrOrNull, s->parameters.tls_hostname); - params->tls_authz = QAPI_CLONE(StrOrNull, s->parameters.tls_authz); - params->max_bandwidth = s->parameters.max_bandwidth; - params->avail_switchover_bandwidth = s->parameters.avail_switchover_bandwidth; - params->downtime_limit = s->parameters.downtime_limit; - params->x_checkpoint_delay = s->parameters.x_checkpoint_delay; - params->multifd_channels = s->parameters.multifd_channels; - params->multifd_compression = s->parameters.multifd_compression; - params->multifd_zlib_level = s->parameters.multifd_zlib_level; - params->multifd_qatzip_level = s->parameters.multifd_qatzip_level; - params->multifd_zstd_level = s->parameters.multifd_zstd_level; - params->xbzrle_cache_size = s->parameters.xbzrle_cache_size; - params->max_postcopy_bandwidth = s->parameters.max_postcopy_bandwidth; - params->max_cpu_throttle = s->parameters.max_cpu_throttle; - params->announce_initial = s->parameters.announce_initial; - params->announce_max = s->parameters.announce_max; - params->announce_rounds = s->parameters.announce_rounds; - params->announce_step = s->parameters.announce_step; - params->x_vcpu_dirty_limit_period = s->parameters.x_vcpu_dirty_limit_period; - params->vcpu_dirty_limit = s->parameters.vcpu_dirty_limit; - params->mode = s->parameters.mode; - params->zero_page_detection = s->parameters.zero_page_detection; - params->direct_io = s->parameters.direct_io; - params->cpr_exec_command = QAPI_CLONE(strList, - s->parameters.cpr_exec_command); - params->block_bitmap_mapping = - QAPI_CLONE(BitmapMigrationNodeAliasList, - s->parameters.block_bitmap_mapping); - - migrate_mark_all_params_present(params); + MigrationParameters *params = QAPI_CLONE(MigrationParameters, + &s->parameters); /* * The block-bitmap-mapping breaks the expected API of @@ -XXX,XX +XXX,XX @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp) void migrate_params_init(MigrationParameters *params) { + migrate_mark_all_params_present(params); } static void migrate_post_update_params(MigrationParameters *new, Error **errp) -- 2.50.1
From: Fabiano Rosas <farosas@suse.de> With the upcoming addition of the config QDict, the tests will need a better way of managing the memory of the test data than putting the test arguments on the stack of the test functions. The config QDict will need to be merged into the arguments of migrate_qmp* functions, which causes a refcount increment, so the test functions would need to allocate and deref the config QDict themselves. A better approach is to already pass the arguments into the test functions and do the memory management in the existing wrapper. There is already migration_test_destroy(), which is called for every test. Do the following: - merge the two existing wrappers, migration_test_wrapper() and migration_test_wrapper_full(). The latter was pioneer in passing data into the tests, but now all tests will receive data, so we don't need it anymore. The usage of migration_test_wrapper_full() was in passing a slightly different test name string into the cancel tests, so still keep the migration_test_add_suffix() function. - add (char *name, MigrateCommon *args) to the signature of all test functions. - alter any code to stop allocating args on the stack and instead use the object that came as parameter. - pass args around as needed. - while here, order args (MigrateCommon) before args->start (MigrateStart) and put a blank like in between. No functional change. Signed-off-by: Fabiano Rosas <farosas@suse.de> Link: https://lore.kernel.org/r/20251215220041.12657-26-farosas@suse.de [peterx: fix a conflict with newly added mapped-ram+ignore-share test] Signed-off-by: Peter Xu <peterx@redhat.com> --- tests/qtest/migration/migration-util.h | 8 +- tests/qtest/migration/compression-tests.c | 131 +++--- tests/qtest/migration/cpr-tests.c | 75 ++-- tests/qtest/migration/file-tests.c | 206 +++++----- tests/qtest/migration/migration-util.c | 26 +- tests/qtest/migration/misc-tests.c | 112 +++-- tests/qtest/migration/postcopy-tests.c | 82 ++-- tests/qtest/migration/precopy-tests.c | 348 +++++++--------- tests/qtest/migration/tls-tests.c | 477 ++++++++++------------ 9 files changed, 652 insertions(+), 813 deletions(-) diff --git a/tests/qtest/migration/migration-util.h b/tests/qtest/migration/migration-util.h index XXXXXXX..XXXXXXX 100644 --- a/tests/qtest/migration/migration-util.h +++ b/tests/qtest/migration/migration-util.h @@ -XXX,XX +XXX,XX @@ #include "libqtest.h" +#include "migration/framework.h" + typedef struct QTestMigrationState { bool stop_seen; bool resume_seen; @@ -XXX,XX +XXX,XX @@ static inline bool probe_o_direct_support(const char *tmpfs) bool ufd_version_check(bool *uffd_feature_thread_id); bool kvm_dirty_ring_supported(void); -void migration_test_add(const char *path, void (*fn)(void)); + +void migration_test_add(const char *path, + void (*fn)(char *name, MigrateCommon *args)); void migration_test_add_suffix(const char *path, const char *suffix, - void (*fn)(void *)); + void (*fn)(char *name, MigrateCommon *args)); char *migrate_get_connect_uri(QTestState *who); void migrate_set_ports(QTestState *to, QList *channel_list); diff --git a/tests/qtest/migration/compression-tests.c b/tests/qtest/migration/compression-tests.c index XXXXXXX..XXXXXXX 100644 --- a/tests/qtest/migration/compression-tests.c +++ b/tests/qtest/migration/compression-tests.c @@ -XXX,XX +XXX,XX @@ migrate_hook_start_precopy_tcp_multifd_zstd(QTestState *from, return migrate_hook_start_precopy_tcp_multifd_common(from, to, "zstd"); } -static void test_multifd_tcp_zstd(void) +static void test_multifd_tcp_zstd(char *name, MigrateCommon *args) { - MigrateCommon args = { - .listen_uri = "defer", - .start = { - .caps[MIGRATION_CAPABILITY_MULTIFD] = true, - }, - .start_hook = migrate_hook_start_precopy_tcp_multifd_zstd, - }; - test_precopy_common(&args); + args->listen_uri = "defer"; + args->start_hook = migrate_hook_start_precopy_tcp_multifd_zstd; + + args->start.caps[MIGRATION_CAPABILITY_MULTIFD] = true; + + test_precopy_common(args); } -static void test_multifd_postcopy_tcp_zstd(void) +static void test_multifd_postcopy_tcp_zstd(char *name, MigrateCommon *args) { - MigrateCommon args = { - .listen_uri = "defer", - .start = { - .caps[MIGRATION_CAPABILITY_MULTIFD] = true, - .caps[MIGRATION_CAPABILITY_POSTCOPY_RAM] = true, - }, - .start_hook = migrate_hook_start_precopy_tcp_multifd_zstd, - }; - - test_precopy_common(&args); + args->listen_uri = "defer"; + args->start_hook = migrate_hook_start_precopy_tcp_multifd_zstd, + + args->start.caps[MIGRATION_CAPABILITY_MULTIFD] = true; + args->start.caps[MIGRATION_CAPABILITY_POSTCOPY_RAM] = true; + + test_precopy_common(args); } #endif /* CONFIG_ZSTD */ @@ -XXX,XX +XXX,XX @@ migrate_hook_start_precopy_tcp_multifd_qatzip(QTestState *from, return migrate_hook_start_precopy_tcp_multifd_common(from, to, "qatzip"); } -static void test_multifd_tcp_qatzip(void) +static void test_multifd_tcp_qatzip(char *name, MigrateCommon *args) { - MigrateCommon args = { - .listen_uri = "defer", - .start = { - .caps[MIGRATION_CAPABILITY_MULTIFD] = true, - }, - .start_hook = migrate_hook_start_precopy_tcp_multifd_qatzip, - }; - test_precopy_common(&args); + args->listen_uri = "defer"; + args->start_hook = migrate_hook_start_precopy_tcp_multifd_qatzip; + + args->start.caps[MIGRATION_CAPABILITY_MULTIFD] = true; + + test_precopy_common(args); } #endif @@ -XXX,XX +XXX,XX @@ migrate_hook_start_precopy_tcp_multifd_qpl(QTestState *from, return migrate_hook_start_precopy_tcp_multifd_common(from, to, "qpl"); } -static void test_multifd_tcp_qpl(void) +static void test_multifd_tcp_qpl(char *name, MigrateCommon *args) { - MigrateCommon args = { - .listen_uri = "defer", - .start = { - .caps[MIGRATION_CAPABILITY_MULTIFD] = true, - }, - .start_hook = migrate_hook_start_precopy_tcp_multifd_qpl, - }; - test_precopy_common(&args); + args->listen_uri = "defer"; + args->start_hook = migrate_hook_start_precopy_tcp_multifd_qpl; + + args->start.caps[MIGRATION_CAPABILITY_MULTIFD] = true; + + test_precopy_common(args); } #endif /* CONFIG_QPL */ @@ -XXX,XX +XXX,XX @@ migrate_hook_start_precopy_tcp_multifd_uadk(QTestState *from, return migrate_hook_start_precopy_tcp_multifd_common(from, to, "uadk"); } -static void test_multifd_tcp_uadk(void) +static void test_multifd_tcp_uadk(char *name, MigrateCommon *args) { - MigrateCommon args = { - .listen_uri = "defer", - .start = { - .caps[MIGRATION_CAPABILITY_MULTIFD] = true, - }, - .start_hook = migrate_hook_start_precopy_tcp_multifd_uadk, - }; - test_precopy_common(&args); + args->listen_uri = "defer"; + args->start_hook = migrate_hook_start_precopy_tcp_multifd_uadk; + + args->start.caps[MIGRATION_CAPABILITY_MULTIFD] = true; + + test_precopy_common(args); } #endif /* CONFIG_UADK */ @@ -XXX,XX +XXX,XX @@ migrate_hook_start_xbzrle(QTestState *from, return NULL; } -static void test_precopy_unix_xbzrle(void) +static void test_precopy_unix_xbzrle(char *name, MigrateCommon *args) { g_autofree char *uri = g_strdup_printf("unix:%s/migsocket", tmpfs); - MigrateCommon args = { - .connect_uri = uri, - .listen_uri = uri, - .start_hook = migrate_hook_start_xbzrle, - .iterations = 2, - .start = { - .caps[MIGRATION_CAPABILITY_XBZRLE] = true, - }, - /* - * XBZRLE needs pages to be modified when doing the 2nd+ round - * iteration to have real data pushed to the stream. - */ - .live = true, - }; - - test_precopy_common(&args); + + args->connect_uri = uri; + args->listen_uri = uri; + args->start_hook = migrate_hook_start_xbzrle; + args->iterations = 2; + /* + * XBZRLE needs pages to be modified when doing the 2nd+ round + * iteration to have real data pushed to the stream. + */ + args->live = true; + + args->start.caps[MIGRATION_CAPABILITY_XBZRLE] = true; + + test_precopy_common(args); } static void * @@ -XXX,XX +XXX,XX @@ migrate_hook_start_precopy_tcp_multifd_zlib(QTestState *from, return migrate_hook_start_precopy_tcp_multifd_common(from, to, "zlib"); } -static void test_multifd_tcp_zlib(void) +static void test_multifd_tcp_zlib(char *name, MigrateCommon *args) { - MigrateCommon args = { - .listen_uri = "defer", - .start = { - .caps[MIGRATION_CAPABILITY_MULTIFD] = true, - }, - .start_hook = migrate_hook_start_precopy_tcp_multifd_zlib, - }; - test_precopy_common(&args); + args->listen_uri = "defer"; + args->start_hook = migrate_hook_start_precopy_tcp_multifd_zlib; + + args->start.caps[MIGRATION_CAPABILITY_MULTIFD] = true; + + test_precopy_common(args); } static void migration_test_add_compression_smoke(MigrationTestEnv *env) diff --git a/tests/qtest/migration/cpr-tests.c b/tests/qtest/migration/cpr-tests.c index XXXXXXX..XXXXXXX 100644 --- a/tests/qtest/migration/cpr-tests.c +++ b/tests/qtest/migration/cpr-tests.c @@ -XXX,XX +XXX,XX @@ static void *migrate_hook_start_mode_reboot(QTestState *from, QTestState *to) return NULL; } -static void test_mode_reboot(void) +static void test_mode_reboot(char *name, MigrateCommon *args) { g_autofree char *uri = g_strdup_printf("file:%s/%s", tmpfs, FILE_TEST_FILENAME); - MigrateCommon args = { - .start.mem_type = MEM_TYPE_SHMEM, - .connect_uri = uri, - .listen_uri = "defer", - .start_hook = migrate_hook_start_mode_reboot, - .start = { - .caps[MIGRATION_CAPABILITY_X_IGNORE_SHARED] = true, - }, - }; - - test_file_common(&args, true); + + args->connect_uri = uri; + args->listen_uri = "defer"; + args->start_hook = migrate_hook_start_mode_reboot; + + args->start.mem_type = MEM_TYPE_SHMEM; + args->start.caps[MIGRATION_CAPABILITY_X_IGNORE_SHARED] = true; + + test_file_common(args, true); } static void *test_mode_transfer_start(QTestState *from, QTestState *to) @@ -XXX,XX +XXX,XX @@ static void *test_mode_transfer_start(QTestState *from, QTestState *to) * migration, and cannot connect synchronously to the monitor, so defer * the target connection. */ -static void test_mode_transfer_common(bool incoming_defer) +static void test_mode_transfer_common(MigrateCommon *args, bool incoming_defer) { g_autofree char *cpr_path = g_strdup_printf("%s/cpr.sock", tmpfs); g_autofree char *mig_path = g_strdup_printf("%s/migsocket", tmpfs); @@ -XXX,XX +XXX,XX @@ static void test_mode_transfer_common(bool incoming_defer) opts_target = g_strdup_printf("-incoming cpr,addr.transport=socket," "addr.type=fd,addr.str=%d %s", cpr_sockfd, opts); - MigrateCommon args = { - .start.opts_source = opts, - .start.opts_target = opts_target, - .start.defer_target_connect = true, - .start.mem_type = MEM_TYPE_MEMFD, - .listen_uri = incoming_defer ? "defer" : uri, - .connect_channels = connect_channels, - .cpr_channel = cpr_channel, - .start_hook = test_mode_transfer_start, - }; - - if (test_precopy_common(&args) < 0) { + + args->listen_uri = incoming_defer ? "defer" : uri; + args->connect_channels = connect_channels; + args->cpr_channel = cpr_channel; + args->start_hook = test_mode_transfer_start; + + args->start.opts_source = opts; + args->start.opts_target = opts_target; + args->start.defer_target_connect = true; + args->start.mem_type = MEM_TYPE_MEMFD; + + if (test_precopy_common(args) < 0) { close(cpr_sockfd); unlink(cpr_path); } } -static void test_mode_transfer(void) +static void test_mode_transfer(char *name, MigrateCommon *args) { - test_mode_transfer_common(NULL); + test_mode_transfer_common(args, false); } -static void test_mode_transfer_defer(void) +static void test_mode_transfer_defer(char *name, MigrateCommon *args) { - test_mode_transfer_common(true); + test_mode_transfer_common(args, true); } static void set_cpr_exec_args(QTestState *who, MigrateCommon *args) @@ -XXX,XX +XXX,XX @@ static void *test_mode_exec_start(QTestState *from, QTestState *to) return NULL; } -static void test_mode_exec(void) +static void test_mode_exec(char *name, MigrateCommon *args) { g_autofree char *uri = g_strdup_printf("file:%s/%s", tmpfs, FILE_TEST_FILENAME); g_autofree char *listen_uri = g_strdup_printf("defer"); - MigrateCommon args = { - .start.only_source = true, - .start.opts_source = "-machine aux-ram-share=on -nodefaults", - .start.mem_type = MEM_TYPE_MEMFD, - .connect_uri = uri, - .listen_uri = listen_uri, - .start_hook = test_mode_exec_start, - }; + args->connect_uri = uri; + args->listen_uri = listen_uri; + args->start_hook = test_mode_exec_start; + + args->start.only_source = true; + args->start.opts_source = "-machine aux-ram-share=on -nodefaults"; + args->start.mem_type = MEM_TYPE_MEMFD; - test_cpr_exec(&args); + test_cpr_exec(args); } void migration_test_add_cpr(MigrationTestEnv *env) diff --git a/tests/qtest/migration/file-tests.c b/tests/qtest/migration/file-tests.c index XXXXXXX..XXXXXXX 100644 --- a/tests/qtest/migration/file-tests.c +++ b/tests/qtest/migration/file-tests.c @@ -XXX,XX +XXX,XX @@ static char *tmpfs; -static void test_precopy_file(void) +static void test_precopy_file(char *name, MigrateCommon *args) { g_autofree char *uri = g_strdup_printf("file:%s/%s", tmpfs, FILE_TEST_FILENAME); - MigrateCommon args = { - .connect_uri = uri, - .listen_uri = "defer", - }; + args->connect_uri = uri; + args->listen_uri = "defer"; - test_file_common(&args, true); + test_file_common(args, true); } #ifndef _WIN32 @@ -XXX,XX +XXX,XX @@ static void *migrate_hook_start_file_offset_fdset(QTestState *from, return NULL; } -static void test_precopy_file_offset_fdset(void) +static void test_precopy_file_offset_fdset(char *name, MigrateCommon *args) { g_autofree char *uri = g_strdup_printf("file:/dev/fdset/1,offset=%d", FILE_TEST_OFFSET); - MigrateCommon args = { - .connect_uri = uri, - .listen_uri = "defer", - .start_hook = migrate_hook_start_file_offset_fdset, - }; + args->connect_uri = uri; + args->listen_uri = "defer"; + args->start_hook = migrate_hook_start_file_offset_fdset; - test_file_common(&args, false); + test_file_common(args, false); } #endif -static void test_precopy_file_offset(void) +static void test_precopy_file_offset(char *name, MigrateCommon *args) { g_autofree char *uri = g_strdup_printf("file:%s/%s,offset=%d", tmpfs, FILE_TEST_FILENAME, FILE_TEST_OFFSET); - MigrateCommon args = { - .connect_uri = uri, - .listen_uri = "defer", - }; - test_file_common(&args, false); + args->connect_uri = uri; + args->listen_uri = "defer"; + + test_file_common(args, false); } -static void test_precopy_file_offset_bad(void) +static void test_precopy_file_offset_bad(char *name, MigrateCommon *args) { /* using a value not supported by qemu_strtosz() */ g_autofree char *uri = g_strdup_printf("file:%s/%s,offset=0x20M", tmpfs, FILE_TEST_FILENAME); - MigrateCommon args = { - .connect_uri = uri, - .listen_uri = "defer", - .result = MIG_TEST_QMP_ERROR, - }; - test_file_common(&args, false); + args->connect_uri = uri; + args->listen_uri = "defer"; + args->result = MIG_TEST_QMP_ERROR; + + test_file_common(args, false); } -static void test_precopy_file_mapped_ram_live(void) +static void test_precopy_file_mapped_ram_live(char *name, MigrateCommon *args) { g_autofree char *uri = g_strdup_printf("file:%s/%s", tmpfs, FILE_TEST_FILENAME); - MigrateCommon args = { - .connect_uri = uri, - .listen_uri = "defer", - .start = { - .caps[MIGRATION_CAPABILITY_MAPPED_RAM] = true, - }, - }; - - test_file_common(&args, false); + + args->connect_uri = uri; + args->listen_uri = "defer"; + + args->start.caps[MIGRATION_CAPABILITY_MAPPED_RAM] = true; + + test_file_common(args, false); } -static void test_precopy_file_mapped_ram(void) +static void test_precopy_file_mapped_ram(char *name, MigrateCommon *args) { g_autofree char *uri = g_strdup_printf("file:%s/%s", tmpfs, FILE_TEST_FILENAME); - MigrateCommon args = { - .connect_uri = uri, - .listen_uri = "defer", - .start = { - .caps[MIGRATION_CAPABILITY_MAPPED_RAM] = true, - }, - }; - - test_file_common(&args, true); + + args->connect_uri = uri; + args->listen_uri = "defer"; + + args->start.caps[MIGRATION_CAPABILITY_MAPPED_RAM] = true; + + test_file_common(args, true); } -static void test_multifd_file_mapped_ram_live(void) +static void test_multifd_file_mapped_ram_live(char *name, MigrateCommon *args) { g_autofree char *uri = g_strdup_printf("file:%s/%s", tmpfs, FILE_TEST_FILENAME); - MigrateCommon args = { - .connect_uri = uri, - .listen_uri = "defer", - .start = { - .caps[MIGRATION_CAPABILITY_MULTIFD] = true, - .caps[MIGRATION_CAPABILITY_MAPPED_RAM] = true, - }, - }; - - test_file_common(&args, false); + args->connect_uri = uri; + args->listen_uri = "defer"; + + args->start.caps[MIGRATION_CAPABILITY_MULTIFD] = true; + args->start.caps[MIGRATION_CAPABILITY_MAPPED_RAM] = true; + + test_file_common(args, false); } -static void test_multifd_file_mapped_ram(void) +static void test_multifd_file_mapped_ram(char *name, MigrateCommon *args) { g_autofree char *uri = g_strdup_printf("file:%s/%s", tmpfs, FILE_TEST_FILENAME); - MigrateCommon args = { - .connect_uri = uri, - .listen_uri = "defer", - .start = { - .caps[MIGRATION_CAPABILITY_MULTIFD] = true, - .caps[MIGRATION_CAPABILITY_MAPPED_RAM] = true, - }, - }; - - test_file_common(&args, true); + + args->connect_uri = uri; + args->listen_uri = "defer"; + + args->start.caps[MIGRATION_CAPABILITY_MULTIFD] = true; + args->start.caps[MIGRATION_CAPABILITY_MAPPED_RAM] = true; + + test_file_common(args, true); } static void *migrate_hook_start_multifd_mapped_ram_dio(QTestState *from, @@ -XXX,XX +XXX,XX @@ static void *migrate_hook_start_multifd_mapped_ram_dio(QTestState *from, return NULL; } -static void test_multifd_file_mapped_ram_dio(void) +static void test_multifd_file_mapped_ram_dio(char *name, MigrateCommon *args) { g_autofree char *uri = g_strdup_printf("file:%s/%s", tmpfs, FILE_TEST_FILENAME); - MigrateCommon args = { - .connect_uri = uri, - .listen_uri = "defer", - .start_hook = migrate_hook_start_multifd_mapped_ram_dio, - .start = { - .caps[MIGRATION_CAPABILITY_MAPPED_RAM] = true, - .caps[MIGRATION_CAPABILITY_MULTIFD] = true, - }, - }; + args->connect_uri = uri; + args->listen_uri = "defer"; + args->start_hook = migrate_hook_start_multifd_mapped_ram_dio; + + args->start.caps[MIGRATION_CAPABILITY_MAPPED_RAM] = true; + args->start.caps[MIGRATION_CAPABILITY_MULTIFD] = true; if (!probe_o_direct_support(tmpfs)) { g_test_skip("Filesystem does not support O_DIRECT"); return; } - test_file_common(&args, true); + test_file_common(args, true); } #ifndef _WIN32 @@ -XXX,XX +XXX,XX @@ static void *migrate_hook_start_multifd_mapped_ram_fdset(QTestState *from, return NULL; } -static void test_multifd_file_mapped_ram_fdset(void) +static void test_multifd_file_mapped_ram_fdset(char *name, MigrateCommon *args) { g_autofree char *uri = g_strdup_printf("file:/dev/fdset/1,offset=%d", FILE_TEST_OFFSET); - MigrateCommon args = { - .connect_uri = uri, - .listen_uri = "defer", - .start_hook = migrate_hook_start_multifd_mapped_ram_fdset, - .end_hook = migrate_hook_end_multifd_mapped_ram_fdset, - .start = { - .caps[MIGRATION_CAPABILITY_MAPPED_RAM] = true, - .caps[MIGRATION_CAPABILITY_MULTIFD] = true, - }, - }; - - test_file_common(&args, true); + + args->connect_uri = uri; + args->listen_uri = "defer"; + args->start_hook = migrate_hook_start_multifd_mapped_ram_fdset; + args->end_hook = migrate_hook_end_multifd_mapped_ram_fdset; + + args->start.caps[MIGRATION_CAPABILITY_MAPPED_RAM] = true; + args->start.caps[MIGRATION_CAPABILITY_MULTIFD] = true; + + test_file_common(args, true); } -static void test_multifd_file_mapped_ram_fdset_dio(void) +static void test_multifd_file_mapped_ram_fdset_dio(char *name, + MigrateCommon *args) { g_autofree char *uri = g_strdup_printf("file:/dev/fdset/1,offset=%d", FILE_TEST_OFFSET); - MigrateCommon args = { - .connect_uri = uri, - .listen_uri = "defer", - .start_hook = migrate_hook_start_multifd_mapped_ram_fdset_dio, - .end_hook = migrate_hook_end_multifd_mapped_ram_fdset, - .start = { - .caps[MIGRATION_CAPABILITY_MAPPED_RAM] = true, - .caps[MIGRATION_CAPABILITY_MULTIFD] = true, - }, - }; + args->connect_uri = uri; + args->listen_uri = "defer"; + args->start_hook = migrate_hook_start_multifd_mapped_ram_fdset_dio; + args->end_hook = migrate_hook_end_multifd_mapped_ram_fdset; + + args->start.caps[MIGRATION_CAPABILITY_MAPPED_RAM] = true; + args->start.caps[MIGRATION_CAPABILITY_MULTIFD] = true; if (!probe_o_direct_support(tmpfs)) { g_test_skip("Filesystem does not support O_DIRECT"); return; } - test_file_common(&args, true); + test_file_common(args, true); } #endif /* !_WIN32 */ @@ -XXX,XX +XXX,XX @@ static void migration_test_add_file_smoke(MigrationTestEnv *env) test_multifd_file_mapped_ram_dio); } -static void test_precopy_file_mapped_ram_ignore_shared(void) +static void +test_precopy_file_mapped_ram_ignore_shared(char *name, MigrateCommon *args) { g_autofree char *uri = g_strdup_printf("file:%s/%s", tmpfs, FILE_TEST_FILENAME); - MigrateCommon args = { - .connect_uri = uri, - .listen_uri = "defer", - .start = { - .caps[MIGRATION_CAPABILITY_MAPPED_RAM] = true, - .caps[MIGRATION_CAPABILITY_X_IGNORE_SHARED] = true, - }, - }; - - test_file_common(&args, true); + args->connect_uri = uri; + args->listen_uri = "defer"; + + args->start.caps[MIGRATION_CAPABILITY_MAPPED_RAM] = true; + args->start.caps[MIGRATION_CAPABILITY_X_IGNORE_SHARED] = true; + + test_file_common(args, true); } void migration_test_add_file(MigrationTestEnv *env) diff --git a/tests/qtest/migration/migration-util.c b/tests/qtest/migration/migration-util.c index XXXXXXX..XXXXXXX 100644 --- a/tests/qtest/migration/migration-util.c +++ b/tests/qtest/migration/migration-util.c @@ -XXX,XX +XXX,XX @@ char *resolve_machine_version(const char *alias, const char *var1, typedef struct { char *name; - void (*func)(void); - void (*func_full)(void *); + MigrateCommon *data; + void (*func)(char *name, MigrateCommon *args); } MigrationTest; static void migration_test_destroy(gpointer data) { MigrationTest *test = (MigrationTest *)data; + g_free(test->data); g_free(test->name); g_free(test); } @@ -XXX,XX +XXX,XX @@ static void migration_test_wrapper(const void *data) { MigrationTest *test = (MigrationTest *)data; + test->data = g_new0(MigrateCommon, 1); + g_test_message("Running /%s%s", qtest_get_arch(), test->name); - test->func(); + test->func(test->name, test->data); } -void migration_test_add(const char *path, void (*fn)(void)) +void migration_test_add(const char *path, + void (*fn)(char *name, MigrateCommon *args)) { MigrationTest *test = g_new0(MigrationTest, 1); @@ -XXX,XX +XXX,XX @@ void migration_test_add(const char *path, void (*fn)(void)) migration_test_destroy); } -static void migration_test_wrapper_full(const void *data) -{ - MigrationTest *test = (MigrationTest *)data; - - g_test_message("Running /%s%s", qtest_get_arch(), test->name); - test->func_full(test->name); -} - void migration_test_add_suffix(const char *path, const char *suffix, - void (*fn)(void *)) + void (*fn)(char *name, MigrateCommon *args)) { MigrationTest *test = g_new0(MigrationTest, 1); g_assert(g_str_has_suffix(path, "/")); g_assert(!g_str_has_prefix(suffix, "/")); - test->func_full = fn; + test->func = fn; test->name = g_strconcat(path, suffix, NULL); - qtest_add_data_func_full(test->name, test, migration_test_wrapper_full, + qtest_add_data_func_full(test->name, test, migration_test_wrapper, migration_test_destroy); } diff --git a/tests/qtest/migration/misc-tests.c b/tests/qtest/migration/misc-tests.c index XXXXXXX..XXXXXXX 100644 --- a/tests/qtest/migration/misc-tests.c +++ b/tests/qtest/migration/misc-tests.c @@ -XXX,XX +XXX,XX @@ static char *tmpfs; -static void test_baddest(void) +static void test_baddest(char *name, MigrateCommon *args) { - MigrateStart args = { - .hide_stderr = true - }; QTestState *from, *to; - if (migrate_start(&from, &to, "tcp:127.0.0.1:0", &args)) { + args->start.hide_stderr = true; + + if (migrate_start(&from, &to, "tcp:127.0.0.1:0", &args->start)) { return; } migrate_qmp(from, to, "tcp:127.0.0.1:0", NULL, "{}"); @@ -XXX,XX +XXX,XX @@ static void test_baddest(void) } #ifndef _WIN32 -static void test_analyze_script(void) +static void test_analyze_script(char *name, MigrateCommon *args) { - MigrateStart args = { - .opts_source = "-uuid 11111111-1111-1111-1111-111111111111", - }; QTestState *from, *to; g_autofree char *uri = NULL; g_autofree char *file = NULL; int pid, wstatus; const char *python = g_getenv("PYTHON"); + args->start.opts_source = "-uuid 11111111-1111-1111-1111-111111111111"; + if (!python) { g_test_skip("PYTHON variable not set"); return; } /* dummy url */ - if (migrate_start(&from, &to, "tcp:127.0.0.1:0", &args)) { + if (migrate_start(&from, &to, "tcp:127.0.0.1:0", &args->start)) { return; } @@ -XXX,XX +XXX,XX @@ static void test_analyze_script(void) } #endif -static void test_ignore_shared(void) +static void test_ignore_shared(char *name, MigrateCommon *args) { g_autofree char *uri = g_strdup_printf("unix:%s/migsocket", tmpfs); QTestState *from, *to; - MigrateStart args = { - .mem_type = MEM_TYPE_SHMEM, - .caps[MIGRATION_CAPABILITY_X_IGNORE_SHARED] = true, - }; - if (migrate_start(&from, &to, uri, &args)) { + args->start.mem_type = MEM_TYPE_SHMEM; + args->start.caps[MIGRATION_CAPABILITY_X_IGNORE_SHARED] = true; + + if (migrate_start(&from, &to, uri, &args->start)) { return; } @@ -XXX,XX +XXX,XX @@ static void do_test_validate_uuid(MigrateStart *args, bool should_fail) migrate_end(from, to, false); } -static void test_validate_uuid(void) +static void test_validate_uuid(char *name, MigrateCommon *args) { - MigrateStart args = { - .opts_source = "-uuid 11111111-1111-1111-1111-111111111111", - .opts_target = "-uuid 11111111-1111-1111-1111-111111111111", - }; + args->start.opts_source = "-uuid 11111111-1111-1111-1111-111111111111"; + args->start.opts_target = "-uuid 11111111-1111-1111-1111-111111111111"; - do_test_validate_uuid(&args, false); + do_test_validate_uuid(&args->start, false); } -static void test_validate_uuid_error(void) +static void test_validate_uuid_error(char *name, MigrateCommon *args) { - MigrateStart args = { - .opts_source = "-uuid 11111111-1111-1111-1111-111111111111", - .opts_target = "-uuid 22222222-2222-2222-2222-222222222222", - .hide_stderr = true, - }; + args->start.opts_source = "-uuid 11111111-1111-1111-1111-111111111111"; + args->start.opts_target = "-uuid 22222222-2222-2222-2222-222222222222"; + args->start.hide_stderr = true; - do_test_validate_uuid(&args, true); + do_test_validate_uuid(&args->start, true); } -static void test_validate_uuid_src_not_set(void) +static void test_validate_uuid_src_not_set(char *name, MigrateCommon *args) { - MigrateStart args = { - .opts_target = "-uuid 22222222-2222-2222-2222-222222222222", - .hide_stderr = true, - }; + args->start.opts_target = "-uuid 22222222-2222-2222-2222-222222222222"; + args->start.hide_stderr = true; - do_test_validate_uuid(&args, false); + do_test_validate_uuid(&args->start, false); } -static void test_validate_uuid_dst_not_set(void) +static void test_validate_uuid_dst_not_set(char *name, MigrateCommon *args) { - MigrateStart args = { - .opts_source = "-uuid 11111111-1111-1111-1111-111111111111", - .hide_stderr = true, - }; + args->start.opts_source = "-uuid 11111111-1111-1111-1111-111111111111"; + args->start.hide_stderr = true; - do_test_validate_uuid(&args, false); + do_test_validate_uuid(&args->start, false); } static void do_test_validate_uri_channel(MigrateCommon *args) @@ -XXX,XX +XXX,XX @@ static void do_test_validate_uri_channel(MigrateCommon *args) migrate_end(from, to, false); } -static void test_validate_uri_channels_both_set(void) +static void test_validate_uri_channels_both_set(char *name, MigrateCommon *args) { - MigrateCommon args = { - .start = { - .hide_stderr = true, - }, - .listen_uri = "defer", - .connect_uri = "tcp:127.0.0.1:0", - .connect_channels = ("[ { ""'channel-type': 'main'," - " 'addr': { 'transport': 'socket'," - " 'type': 'inet'," - " 'host': '127.0.0.1'," - " 'port': '0' } } ]"), - }; - - do_test_validate_uri_channel(&args); + args->listen_uri = "defer", + args->connect_uri = "tcp:127.0.0.1:0", + args->connect_channels = ("[ { ""'channel-type': 'main'," + " 'addr': { 'transport': 'socket'," + " 'type': 'inet'," + " 'host': '127.0.0.1'," + " 'port': '0' } } ]"), + + args->start.hide_stderr = true; + + do_test_validate_uri_channel(args); } -static void test_validate_uri_channels_none_set(void) +static void test_validate_uri_channels_none_set(char *name, MigrateCommon *args) { - MigrateCommon args = { - .start = { - .hide_stderr = true, - }, - .listen_uri = "defer", - }; - - do_test_validate_uri_channel(&args); + args->listen_uri = "defer"; + args->start.hide_stderr = true; + + do_test_validate_uri_channel(args); } static void migration_test_add_misc_smoke(MigrationTestEnv *env) diff --git a/tests/qtest/migration/postcopy-tests.c b/tests/qtest/migration/postcopy-tests.c index XXXXXXX..XXXXXXX 100644 --- a/tests/qtest/migration/postcopy-tests.c +++ b/tests/qtest/migration/postcopy-tests.c @@ -XXX,XX +XXX,XX @@ #include "qemu/range.h" #include "qemu/sockets.h" -static void test_postcopy(void) +static void test_postcopy(char *name, MigrateCommon *args) { - MigrateCommon args = { }; - - test_postcopy_common(&args); + test_postcopy_common(args); } -static void test_postcopy_suspend(void) +static void test_postcopy_suspend(char *name, MigrateCommon *args) { - MigrateCommon args = { - .start.suspend_me = true, - }; + args->start.suspend_me = true; - test_postcopy_common(&args); + test_postcopy_common(args); } -static void test_postcopy_preempt(void) +static void test_postcopy_preempt(char *name, MigrateCommon *args) { - MigrateCommon args = { - .start = { - .caps[MIGRATION_CAPABILITY_POSTCOPY_PREEMPT] = true, - }, - }; + args->start.caps[MIGRATION_CAPABILITY_POSTCOPY_PREEMPT] = true; - test_postcopy_common(&args); + test_postcopy_common(args); } -static void test_postcopy_recovery(void) +static void test_postcopy_recovery(char *name, MigrateCommon *args) { - MigrateCommon args = { }; - - test_postcopy_recovery_common(&args); + test_postcopy_recovery_common(args); } -static void test_postcopy_recovery_fail_handshake(void) +static void test_postcopy_recovery_fail_handshake(char *name, + MigrateCommon *args) { - MigrateCommon args = { - .postcopy_recovery_fail_stage = POSTCOPY_FAIL_RECOVERY, - }; + args->postcopy_recovery_fail_stage = POSTCOPY_FAIL_RECOVERY; - test_postcopy_recovery_common(&args); + test_postcopy_recovery_common(args); } -static void test_postcopy_recovery_fail_reconnect(void) +static void test_postcopy_recovery_fail_reconnect(char *name, + MigrateCommon *args) { - MigrateCommon args = { - .postcopy_recovery_fail_stage = POSTCOPY_FAIL_CHANNEL_ESTABLISH, - }; + args->postcopy_recovery_fail_stage = POSTCOPY_FAIL_CHANNEL_ESTABLISH; - test_postcopy_recovery_common(&args); + test_postcopy_recovery_common(args); } -static void test_postcopy_preempt_recovery(void) +static void test_postcopy_preempt_recovery(char *name, MigrateCommon *args) { - MigrateCommon args = { - .start = { - .caps[MIGRATION_CAPABILITY_POSTCOPY_PREEMPT] = true, - }, - }; + args->start.caps[MIGRATION_CAPABILITY_POSTCOPY_PREEMPT] = true; - test_postcopy_recovery_common(&args); + test_postcopy_recovery_common(args); } static void migration_test_add_postcopy_smoke(MigrationTestEnv *env) @@ -XXX,XX +XXX,XX @@ static void migration_test_add_postcopy_smoke(MigrationTestEnv *env) } } -static void test_multifd_postcopy(void) +static void test_multifd_postcopy(char *name, MigrateCommon *args) { - MigrateCommon args = { - .start = { - .caps[MIGRATION_CAPABILITY_MULTIFD] = true, - }, - }; + args->start.caps[MIGRATION_CAPABILITY_MULTIFD] = true; - test_postcopy_common(&args); + test_postcopy_common(args); } -static void test_multifd_postcopy_preempt(void) +static void test_multifd_postcopy_preempt(char *name, MigrateCommon *args) { - MigrateCommon args = { - .start = { - .caps[MIGRATION_CAPABILITY_MULTIFD] = true, - .caps[MIGRATION_CAPABILITY_POSTCOPY_PREEMPT] = true, - }, - }; - - test_postcopy_common(&args); + args->start.caps[MIGRATION_CAPABILITY_MULTIFD] = true; + args->start.caps[MIGRATION_CAPABILITY_POSTCOPY_PREEMPT] = true; + + test_postcopy_common(args); } void migration_test_add_postcopy(MigrationTestEnv *env) diff --git a/tests/qtest/migration/precopy-tests.c b/tests/qtest/migration/precopy-tests.c index XXXXXXX..XXXXXXX 100644 --- a/tests/qtest/migration/precopy-tests.c +++ b/tests/qtest/migration/precopy-tests.c @@ -XXX,XX +XXX,XX @@ static char *tmpfs; -static void test_precopy_unix_plain(void) +static void test_precopy_unix_plain(char *name, MigrateCommon *args) { g_autofree char *uri = g_strdup_printf("unix:%s/migsocket", tmpfs); - MigrateCommon args = { - .listen_uri = uri, - .connect_uri = uri, - /* - * The simplest use case of precopy, covering smoke tests of - * get-dirty-log dirty tracking. - */ - .live = true, - }; - test_precopy_common(&args); + args->listen_uri = uri; + args->connect_uri = uri; + /* + * The simplest use case of precopy, covering smoke tests of + * get-dirty-log dirty tracking. + */ + args->live = true; + + test_precopy_common(args); } -static void test_precopy_unix_suspend_live(void) +static void test_precopy_unix_suspend_live(char *name, MigrateCommon *args) { g_autofree char *uri = g_strdup_printf("unix:%s/migsocket", tmpfs); - MigrateCommon args = { - .listen_uri = uri, - .connect_uri = uri, - /* - * despite being live, the test is fast because the src - * suspends immediately. - */ - .live = true, - .start.suspend_me = true, - }; - test_precopy_common(&args); + args->listen_uri = uri; + args->connect_uri = uri; + /* + * despite being live, the test is fast because the src + * suspends immediately. + */ + args->live = true; + + args->start.suspend_me = true; + + test_precopy_common(args); } -static void test_precopy_unix_suspend_notlive(void) +static void test_precopy_unix_suspend_notlive(char *name, MigrateCommon *args) { g_autofree char *uri = g_strdup_printf("unix:%s/migsocket", tmpfs); - MigrateCommon args = { - .listen_uri = uri, - .connect_uri = uri, - .start.suspend_me = true, - }; - test_precopy_common(&args); + args->listen_uri = uri; + args->connect_uri = uri; + args->start.suspend_me = true; + + test_precopy_common(args); } -static void test_precopy_unix_dirty_ring(void) +static void test_precopy_unix_dirty_ring(char *name, MigrateCommon *args) { g_autofree char *uri = g_strdup_printf("unix:%s/migsocket", tmpfs); - MigrateCommon args = { - .start = { - .use_dirty_ring = true, - }, - .listen_uri = uri, - .connect_uri = uri, - /* - * Besides the precopy/unix basic test, cover dirty ring interface - * rather than get-dirty-log. - */ - .live = true, - }; - test_precopy_common(&args); + args->listen_uri = uri; + args->connect_uri = uri; + /* + * Besides the precopy/unix basic test, cover dirty ring interface + * rather than get-dirty-log. + */ + args->live = true; + + args->start.use_dirty_ring = true; + + test_precopy_common(args); } #ifdef CONFIG_RDMA @@ -XXX,XX +XXX,XX @@ static int new_rdma_link(char *buffer, bool ipv6) return -1; } -static void __test_precopy_rdma_plain(bool ipv6) +static void __test_precopy_rdma_plain(MigrateCommon *args, bool ipv6) { char buffer[128] = {}; @@ -XXX,XX +XXX,XX @@ static void __test_precopy_rdma_plain(bool ipv6) **/ g_autofree char *uri = g_strdup_printf("rdma:%s:29200", buffer); - MigrateCommon args = { - .listen_uri = uri, - .connect_uri = uri, - }; + args->listen_uri = uri; + args->connect_uri = uri; - test_precopy_common(&args); + test_precopy_common(args); } -static void test_precopy_rdma_plain(void) +static void test_precopy_rdma_plain(char *name, MigrateCommon *args) { - __test_precopy_rdma_plain(false); + __test_precopy_rdma_plain(args, false); } -static void test_precopy_rdma_plain_ipv6(void) +static void test_precopy_rdma_plain_ipv6(char *name, MigrateCommon *args) { - __test_precopy_rdma_plain(true); + __test_precopy_rdma_plain(args, true); } #endif -static void test_precopy_tcp_plain(void) +static void test_precopy_tcp_plain(char *name, MigrateCommon *args) { - MigrateCommon args = { - .listen_uri = "tcp:127.0.0.1:0", - }; + args->listen_uri = "tcp:127.0.0.1:0"; - test_precopy_common(&args); + test_precopy_common(args); } -static void test_precopy_tcp_switchover_ack(void) +static void test_precopy_tcp_switchover_ack(char *name, MigrateCommon *args) { - MigrateCommon args = { - .listen_uri = "tcp:127.0.0.1:0", - .start = { - .caps[MIGRATION_CAPABILITY_RETURN_PATH] = true, - .caps[MIGRATION_CAPABILITY_SWITCHOVER_ACK] = true, - }, - /* - * Source VM must be running in order to consider the switchover ACK - * when deciding to do switchover or not. - */ - .live = true, - }; + args->listen_uri = "tcp:127.0.0.1:0"; + /* + * Source VM must be running in order to consider the switchover ACK + * when deciding to do switchover or not. + */ + args->live = true; - test_precopy_common(&args); + args->start.caps[MIGRATION_CAPABILITY_RETURN_PATH] = true; + args->start.caps[MIGRATION_CAPABILITY_SWITCHOVER_ACK] = true; + + test_precopy_common(args); } #ifndef _WIN32 @@ -XXX,XX +XXX,XX @@ static void migrate_hook_end_fd(QTestState *from, qobject_unref(rsp); } -static void test_precopy_fd_socket(void) +static void test_precopy_fd_socket(char *name, MigrateCommon *args) { - MigrateCommon args = { - .listen_uri = "defer", - .connect_uri = "fd:fd-mig", - .start_hook = migrate_hook_start_fd, - .end_hook = migrate_hook_end_fd, - }; - test_precopy_common(&args); + args->listen_uri = "defer"; + args->connect_uri = "fd:fd-mig"; + args->start_hook = migrate_hook_start_fd; + args->end_hook = migrate_hook_end_fd; + + test_precopy_common(args); } static void *migrate_hook_start_precopy_fd_file(QTestState *from, @@ -XXX,XX +XXX,XX @@ static void *migrate_hook_start_precopy_fd_file(QTestState *from, return NULL; } -static void test_precopy_fd_file(void) +static void test_precopy_fd_file(char *name, MigrateCommon *args) { - MigrateCommon args = { - .listen_uri = "defer", - .connect_uri = "fd:fd-mig", - .start_hook = migrate_hook_start_precopy_fd_file, - .end_hook = migrate_hook_end_fd, - }; - test_file_common(&args, true); + args->listen_uri = "defer"; + args->connect_uri = "fd:fd-mig"; + args->start_hook = migrate_hook_start_precopy_fd_file; + args->end_hook = migrate_hook_end_fd; + + test_file_common(args, true); } #endif /* _WIN32 */ @@ -XXX,XX +XXX,XX @@ static void test_precopy_fd_file(void) * To make things even worse, we need to run the initial stage at * 3MB/s so we enter autoconverge even when host is (over)loaded. */ -static void test_auto_converge(void) +static void test_auto_converge(char *name, MigrateCommon *args) { g_autofree char *uri = g_strdup_printf("unix:%s/migsocket", tmpfs); - MigrateStart args = {}; QTestState *from, *to; int64_t percentage; @@ -XXX,XX +XXX,XX @@ static void test_auto_converge(void) uint64_t prev_dirty_sync_cnt, dirty_sync_cnt; int max_try_count, hit = 0; - if (migrate_start(&from, &to, uri, &args)) { + if (migrate_start(&from, &to, uri, &args->start)) { return; } @@ -XXX,XX +XXX,XX @@ migrate_hook_start_precopy_tcp_multifd_no_zero_page(QTestState *from, return NULL; } -static void test_multifd_tcp_uri_none(void) +static void test_multifd_tcp_uri_none(char *name, MigrateCommon *args) { - MigrateCommon args = { - .listen_uri = "defer", - .start_hook = migrate_hook_start_precopy_tcp_multifd, - .start = { - .caps[MIGRATION_CAPABILITY_MULTIFD] = true, - }, - /* - * Multifd is more complicated than most of the features, it - * directly takes guest page buffers when sending, make sure - * everything will work alright even if guest page is changing. - */ - .live = true, - }; - test_precopy_common(&args); + args->listen_uri = "defer"; + args->start_hook = migrate_hook_start_precopy_tcp_multifd; + /* + * Multifd is more complicated than most of the features, it + * directly takes guest page buffers when sending, make sure + * everything will work alright even if guest page is changing. + */ + args->live = true; + + args->start.caps[MIGRATION_CAPABILITY_MULTIFD] = true; + + test_precopy_common(args); } -static void test_multifd_tcp_zero_page_legacy(void) +static void test_multifd_tcp_zero_page_legacy(char *name, MigrateCommon *args) { - MigrateCommon args = { - .listen_uri = "defer", - .start_hook = migrate_hook_start_precopy_tcp_multifd_zero_page_legacy, - .start = { - .caps[MIGRATION_CAPABILITY_MULTIFD] = true, - }, - /* - * Multifd is more complicated than most of the features, it - * directly takes guest page buffers when sending, make sure - * everything will work alright even if guest page is changing. - */ - .live = true, - }; - test_precopy_common(&args); + args->listen_uri = "defer"; + args->start_hook = migrate_hook_start_precopy_tcp_multifd_zero_page_legacy; + /* + * Multifd is more complicated than most of the features, it + * directly takes guest page buffers when sending, make sure + * everything will work alright even if guest page is changing. + */ + args->live = true; + + args->start.caps[MIGRATION_CAPABILITY_MULTIFD] = true; + + test_precopy_common(args); } -static void test_multifd_tcp_no_zero_page(void) +static void test_multifd_tcp_no_zero_page(char *name, MigrateCommon *args) { - MigrateCommon args = { - .listen_uri = "defer", - .start_hook = migrate_hook_start_precopy_tcp_multifd_no_zero_page, - .start = { - .caps[MIGRATION_CAPABILITY_MULTIFD] = true, - }, - /* - * Multifd is more complicated than most of the features, it - * directly takes guest page buffers when sending, make sure - * everything will work alright even if guest page is changing. - */ - .live = true, - }; - test_precopy_common(&args); + args->listen_uri = "defer"; + args->start_hook = migrate_hook_start_precopy_tcp_multifd_no_zero_page; + /* + * Multifd is more complicated than most of the features, it + * directly takes guest page buffers when sending, make sure + * everything will work alright even if guest page is changing. + */ + args->live = true; + + args->start.caps[MIGRATION_CAPABILITY_MULTIFD] = true; + + test_precopy_common(args); } -static void test_multifd_tcp_channels_none(void) +static void test_multifd_tcp_channels_none(char *name, MigrateCommon *args) { - MigrateCommon args = { - .listen_uri = "defer", - .start_hook = migrate_hook_start_precopy_tcp_multifd, - .live = true, - .start = { - .caps[MIGRATION_CAPABILITY_MULTIFD] = true, - }, - .connect_channels = ("[ { 'channel-type': 'main'," + args->listen_uri = "defer"; + args->start_hook = migrate_hook_start_precopy_tcp_multifd; + args->live = true; + args->connect_channels = ("[ { 'channel-type': 'main'," " 'addr': { 'transport': 'socket'," " 'type': 'inet'," " 'host': '127.0.0.1'," - " 'port': '0' } } ]"), - }; - test_precopy_common(&args); + " 'port': '0' } } ]"); + + args->start.caps[MIGRATION_CAPABILITY_MULTIFD] = true; + + test_precopy_common(args); } /* @@ -XXX,XX +XXX,XX @@ static void test_multifd_tcp_channels_none(void) * * And see that it works */ -static void test_multifd_tcp_cancel(bool postcopy_ram) +static void test_multifd_tcp_cancel(MigrateCommon *args, bool postcopy_ram) { - MigrateStart args = { - .hide_stderr = true, - }; QTestState *from, *to, *to2; - if (migrate_start(&from, &to, "defer", &args)) { + args->start.hide_stderr = true; + + if (migrate_start(&from, &to, "defer", &args->start)) { return; } @@ -XXX,XX +XXX,XX @@ static void test_multifd_tcp_cancel(bool postcopy_ram) */ wait_for_migration_status(from, "cancelled", NULL); - args = (MigrateStart){ - .only_target = true, - }; + args->start.only_target = true; - if (migrate_start(&from, &to2, "defer", &args)) { + if (migrate_start(&from, &to2, "defer", &args->start)) { return; } @@ -XXX,XX +XXX,XX @@ static void test_multifd_tcp_cancel(bool postcopy_ram) migrate_end(from, to2, true); } -static void test_multifd_precopy_tcp_cancel(void) +static void test_multifd_precopy_tcp_cancel(char *name, MigrateCommon *args) { - test_multifd_tcp_cancel(false); + test_multifd_tcp_cancel(args, false); } -static void test_multifd_postcopy_tcp_cancel(void) +static void test_multifd_postcopy_tcp_cancel(char *name, MigrateCommon *args) { - test_multifd_tcp_cancel(true); + test_multifd_tcp_cancel(args, true); } static void test_cancel_src_after_failed(QTestState *from, QTestState *to, @@ -XXX,XX +XXX,XX @@ static void test_cancel_src_pre_switchover(QTestState *from, QTestState *to, (const char * []) { "completed", NULL }); } -static void test_cancel_src_after_status(void *opaque) +static void test_cancel_src_after_status(char *test_path, MigrateCommon *args) { - const char *test_path = opaque; g_autofree char *phase = g_path_get_basename(test_path); g_autofree char *uri = g_strdup_printf("unix:%s/migsocket", tmpfs); QTestState *from, *to; - MigrateStart args = { - .hide_stderr = true, - }; - if (migrate_start(&from, &to, "defer", &args)) { + args->start.hide_stderr = true; + + if (migrate_start(&from, &to, "defer", &args->start)) { return; } @@ -XXX,XX +XXX,XX @@ static void dirtylimit_stop_vm(QTestState *vm) unlink(path); } -static void test_vcpu_dirty_limit(void) +static void test_vcpu_dirty_limit(char *name, MigrateCommon *args) { QTestState *vm; int64_t origin_rate; @@ -XXX,XX +XXX,XX @@ static void migrate_dirty_limit_wait_showup(QTestState *from, * And see if dirty limit migration works correctly. * This test case involves many passes, so it runs in slow mode only. */ -static void test_dirty_limit(void) +static void test_dirty_limit(char *name, MigrateCommon *args) { g_autofree char *uri = g_strdup_printf("unix:%s/migsocket", tmpfs); QTestState *from, *to; @@ -XXX,XX +XXX,XX @@ static void test_dirty_limit(void) */ const int64_t expected_threshold = max_bandwidth * downtime_limit / 1000; int max_try_count = 10; - MigrateCommon args = { - .start = { - .hide_stderr = true, - .use_dirty_ring = true, - }, - .listen_uri = uri, - .connect_uri = uri, - }; + + args->start.hide_stderr = true; + args->start.use_dirty_ring = true; + + args->listen_uri = uri; + args->connect_uri = uri; /* Start src, dst vm */ - if (migrate_start(&from, &to, args.listen_uri, &args.start)) { + if (migrate_start(&from, &to, args->listen_uri, &args->start)) { return; } @@ -XXX,XX +XXX,XX @@ static void test_dirty_limit(void) migrate_dirty_limit_wait_showup(from, dirtylimit_period, dirtylimit_value); /* Start migrate */ - migrate_qmp(from, to, args.connect_uri, NULL, "{}"); + migrate_qmp(from, to, args->connect_uri, NULL, "{}"); /* Wait for dirty limit throttle begin */ throttle_us_per_full = 0; @@ -XXX,XX +XXX,XX @@ static void test_dirty_limit(void) /* Assert dirty limit is not in service */ g_assert_cmpint(throttle_us_per_full, ==, 0); - args = (MigrateCommon) { - .start = { - .only_target = true, - .use_dirty_ring = true, - }, - .listen_uri = uri, - .connect_uri = uri, - }; + args->listen_uri = uri; + args->connect_uri = uri; + + args->start.only_target = true; + args->start.use_dirty_ring = true; /* Restart dst vm, src vm already show up so we needn't wait anymore */ - if (migrate_start(&from, &to, args.listen_uri, &args.start)) { + if (migrate_start(&from, &to, args->listen_uri, &args->start)) { return; } /* Start migrate */ - migrate_qmp(from, to, args.connect_uri, NULL, "{}"); + migrate_qmp(from, to, args->connect_uri, NULL, "{}"); /* Wait for dirty limit throttle begin */ throttle_us_per_full = 0; diff --git a/tests/qtest/migration/tls-tests.c b/tests/qtest/migration/tls-tests.c index XXXXXXX..XXXXXXX 100644 --- a/tests/qtest/migration/tls-tests.c +++ b/tests/qtest/migration/tls-tests.c @@ -XXX,XX +XXX,XX @@ migrate_hook_end_tls_x509(QTestState *from, } #endif /* CONFIG_TASN1 */ -static void test_postcopy_tls_psk(void) +static void test_postcopy_tls_psk(char *name, MigrateCommon *args) { - MigrateCommon args = { - .start_hook = migrate_hook_start_tls_psk_match, - .end_hook = migrate_hook_end_tls_psk, - }; + args->start_hook = migrate_hook_start_tls_psk_match; + args->end_hook = migrate_hook_end_tls_psk; - test_postcopy_common(&args); + test_postcopy_common(args); } -static void test_postcopy_preempt_tls_psk(void) +static void test_postcopy_preempt_tls_psk(char *name, MigrateCommon *args) { - MigrateCommon args = { - .start_hook = migrate_hook_start_tls_psk_match, - .end_hook = migrate_hook_end_tls_psk, - .start = { - .caps[MIGRATION_CAPABILITY_POSTCOPY_PREEMPT] = true, - }, - }; + args->start_hook = migrate_hook_start_tls_psk_match; + args->end_hook = migrate_hook_end_tls_psk; - test_postcopy_common(&args); + args->start.caps[MIGRATION_CAPABILITY_POSTCOPY_PREEMPT] = true; + + test_postcopy_common(args); } -static void test_postcopy_recovery_tls_psk(void) +static void test_postcopy_recovery_tls_psk(char *name, MigrateCommon *args) { - MigrateCommon args = { - .start_hook = migrate_hook_start_tls_psk_match, - .end_hook = migrate_hook_end_tls_psk, - }; + args->start_hook = migrate_hook_start_tls_psk_match; + args->end_hook = migrate_hook_end_tls_psk; - test_postcopy_recovery_common(&args); + test_postcopy_recovery_common(args); } -static void test_multifd_postcopy_recovery_tls_psk(void) +static void test_multifd_postcopy_recovery_tls_psk(char *name, + MigrateCommon *args) { - MigrateCommon args = { - .start_hook = migrate_hook_start_tls_psk_match, - .end_hook = migrate_hook_end_tls_psk, - .start = { - .caps[MIGRATION_CAPABILITY_MULTIFD] = true, - }, - }; + args->start_hook = migrate_hook_start_tls_psk_match; + args->end_hook = migrate_hook_end_tls_psk; + + args->start.caps[MIGRATION_CAPABILITY_MULTIFD] = true; - test_postcopy_recovery_common(&args); + test_postcopy_recovery_common(args); } /* This contains preempt+recovery+tls test altogether */ -static void test_postcopy_preempt_all(void) -{ - MigrateCommon args = { - .start_hook = migrate_hook_start_tls_psk_match, - .end_hook = migrate_hook_end_tls_psk, - .start = { - .caps[MIGRATION_CAPABILITY_POSTCOPY_PREEMPT] = true, - }, - }; +static void test_postcopy_preempt_all(char *name, MigrateCommon *args) +{ + args->start_hook = migrate_hook_start_tls_psk_match; + args->end_hook = migrate_hook_end_tls_psk; - test_postcopy_recovery_common(&args); + args->start.caps[MIGRATION_CAPABILITY_POSTCOPY_PREEMPT] = true; + + test_postcopy_recovery_common(args); } -static void test_multifd_postcopy_preempt_recovery_tls_psk(void) +static void test_multifd_postcopy_preempt_recovery_tls_psk(char *name, + MigrateCommon *args) { - MigrateCommon args = { - .start_hook = migrate_hook_start_tls_psk_match, - .end_hook = migrate_hook_end_tls_psk, - .start = { - .caps[MIGRATION_CAPABILITY_MULTIFD] = true, - .caps[MIGRATION_CAPABILITY_POSTCOPY_PREEMPT] = true, - }, - }; + args->start_hook = migrate_hook_start_tls_psk_match; + args->end_hook = migrate_hook_end_tls_psk; - test_postcopy_recovery_common(&args); + args->start.caps[MIGRATION_CAPABILITY_MULTIFD] = true; + args->start.caps[MIGRATION_CAPABILITY_POSTCOPY_PREEMPT] = true; + + test_postcopy_recovery_common(args); } -static void test_precopy_unix_tls_psk(void) +static void test_precopy_unix_tls_psk(char *name, MigrateCommon *args) { g_autofree char *uri = g_strdup_printf("unix:%s/migsocket", tmpfs); - MigrateCommon args = { - .connect_uri = uri, - .listen_uri = uri, - .start_hook = migrate_hook_start_tls_psk_match, - .end_hook = migrate_hook_end_tls_psk, - }; - test_precopy_common(&args); + args->connect_uri = uri; + args->listen_uri = uri; + args->start_hook = migrate_hook_start_tls_psk_match; + args->end_hook = migrate_hook_end_tls_psk; + + test_precopy_common(args); } #ifdef CONFIG_TASN1 -static void test_precopy_unix_tls_x509_default_host(void) +static void test_precopy_unix_tls_x509_default_host(char *name, + MigrateCommon *args) { g_autofree char *uri = g_strdup_printf("unix:%s/migsocket", tmpfs); - MigrateCommon args = { - .start = { - .hide_stderr = true, - }, - .connect_uri = uri, - .listen_uri = uri, - .start_hook = migrate_hook_start_tls_x509_default_host, - .end_hook = migrate_hook_end_tls_x509, - .result = MIG_TEST_FAIL_DEST_QUIT_ERR, - }; - test_precopy_common(&args); + args->connect_uri = uri; + args->listen_uri = uri; + args->start_hook = migrate_hook_start_tls_x509_default_host; + args->end_hook = migrate_hook_end_tls_x509; + args->result = MIG_TEST_FAIL_DEST_QUIT_ERR; + + args->start.hide_stderr = true; + + test_precopy_common(args); } -static void test_precopy_unix_tls_x509_override_host(void) +static void test_precopy_unix_tls_x509_override_host(char *name, + MigrateCommon *args) { g_autofree char *uri = g_strdup_printf("unix:%s/migsocket", tmpfs); - MigrateCommon args = { - .connect_uri = uri, - .listen_uri = uri, - .start_hook = migrate_hook_start_tls_x509_override_host, - .end_hook = migrate_hook_end_tls_x509, - }; - test_precopy_common(&args); + args->connect_uri = uri; + args->listen_uri = uri; + args->start_hook = migrate_hook_start_tls_x509_override_host; + args->end_hook = migrate_hook_end_tls_x509; + + test_precopy_common(args); } #endif /* CONFIG_TASN1 */ -static void test_precopy_tcp_tls_psk_match(void) +static void test_precopy_tcp_tls_psk_match(char *name, MigrateCommon *args) { - MigrateCommon args = { - .listen_uri = "tcp:127.0.0.1:0", - .start_hook = migrate_hook_start_tls_psk_match, - .end_hook = migrate_hook_end_tls_psk, - }; + args->listen_uri = "tcp:127.0.0.1:0"; + args->start_hook = migrate_hook_start_tls_psk_match; + args->end_hook = migrate_hook_end_tls_psk; - test_precopy_common(&args); + test_precopy_common(args); } -static void test_precopy_tcp_tls_psk_mismatch(void) +static void test_precopy_tcp_tls_psk_mismatch(char *name, MigrateCommon *args) { - MigrateCommon args = { - .start = { - .hide_stderr = true, - }, - .listen_uri = "tcp:127.0.0.1:0", - .start_hook = migrate_hook_start_tls_psk_mismatch, - .end_hook = migrate_hook_end_tls_psk, - .result = MIG_TEST_FAIL, - }; + args->listen_uri = "tcp:127.0.0.1:0"; + args->start_hook = migrate_hook_start_tls_psk_mismatch; + args->end_hook = migrate_hook_end_tls_psk; + args->result = MIG_TEST_FAIL; + + args->start.hide_stderr = true; - test_precopy_common(&args); + test_precopy_common(args); } static void *migrate_hook_start_no_tls(QTestState *from, QTestState *to) @@ -XXX,XX +XXX,XX @@ static void *migrate_hook_start_no_tls(QTestState *from, QTestState *to) return data; } -static void test_precopy_tcp_no_tls(void) +static void test_precopy_tcp_no_tls(char *name, MigrateCommon *args) { - MigrateCommon args = { - .listen_uri = "tcp:127.0.0.1:0", - .start_hook = migrate_hook_start_no_tls, - .end_hook = migrate_hook_end_tls_psk, - }; + args->listen_uri = "tcp:127.0.0.1:0"; + args->start_hook = migrate_hook_start_no_tls; + args->end_hook = migrate_hook_end_tls_psk; - test_precopy_common(&args); + test_precopy_common(args); } static void * @@ -XXX,XX +XXX,XX @@ migrate_hook_start_tls_x509_no_host(QTestState *from, QTestState *to) return data; } -static void test_precopy_tcp_tls_no_hostname(void) +static void test_precopy_tcp_tls_no_hostname(char *name, MigrateCommon *args) { - MigrateCommon args = { - .listen_uri = "tcp:127.0.0.1:0", - .start_hook = migrate_hook_start_tls_x509_no_host, - .end_hook = migrate_hook_end_tls_x509, - .result = MIG_TEST_FAIL_DEST_QUIT_ERR, - .start.hide_stderr = true, - }; + args->listen_uri = "tcp:127.0.0.1:0"; + args->start_hook = migrate_hook_start_tls_x509_no_host; + args->end_hook = migrate_hook_end_tls_x509; + args->result = MIG_TEST_FAIL_DEST_QUIT_ERR; + + args->start.hide_stderr = true; - test_precopy_common(&args); + test_precopy_common(args); } #ifdef CONFIG_TASN1 -static void test_precopy_tcp_tls_x509_default_host(void) +static void test_precopy_tcp_tls_x509_default_host(char *name, + MigrateCommon *args) { - MigrateCommon args = { - .listen_uri = "tcp:127.0.0.1:0", - .start_hook = migrate_hook_start_tls_x509_default_host, - .end_hook = migrate_hook_end_tls_x509, - }; + args->listen_uri = "tcp:127.0.0.1:0"; + args->start_hook = migrate_hook_start_tls_x509_default_host; + args->end_hook = migrate_hook_end_tls_x509; - test_precopy_common(&args); + test_precopy_common(args); } -static void test_precopy_tcp_tls_x509_override_host(void) +static void test_precopy_tcp_tls_x509_override_host(char *name, + MigrateCommon *args) { - MigrateCommon args = { - .listen_uri = "tcp:127.0.0.1:0", - .start_hook = migrate_hook_start_tls_x509_override_host, - .end_hook = migrate_hook_end_tls_x509, - }; + args->listen_uri = "tcp:127.0.0.1:0"; + args->start_hook = migrate_hook_start_tls_x509_override_host; + args->end_hook = migrate_hook_end_tls_x509; - test_precopy_common(&args); + test_precopy_common(args); } -static void test_precopy_tcp_tls_x509_mismatch_host(void) +static void test_precopy_tcp_tls_x509_mismatch_host(char *name, + MigrateCommon *args) { - MigrateCommon args = { - .start = { - .hide_stderr = true, - }, - .listen_uri = "tcp:127.0.0.1:0", - .start_hook = migrate_hook_start_tls_x509_mismatch_host, - .end_hook = migrate_hook_end_tls_x509, - .result = MIG_TEST_FAIL_DEST_QUIT_ERR, - }; + args->listen_uri = "tcp:127.0.0.1:0"; + args->start_hook = migrate_hook_start_tls_x509_mismatch_host; + args->end_hook = migrate_hook_end_tls_x509; + args->result = MIG_TEST_FAIL_DEST_QUIT_ERR; + + args->start.hide_stderr = true; - test_precopy_common(&args); + test_precopy_common(args); } -static void test_precopy_tcp_tls_x509_friendly_client(void) +static void test_precopy_tcp_tls_x509_friendly_client(char *name, + MigrateCommon *args) { - MigrateCommon args = { - .listen_uri = "tcp:127.0.0.1:0", - .start_hook = migrate_hook_start_tls_x509_friendly_client, - .end_hook = migrate_hook_end_tls_x509, - }; + args->listen_uri = "tcp:127.0.0.1:0"; + args->start_hook = migrate_hook_start_tls_x509_friendly_client; + args->end_hook = migrate_hook_end_tls_x509; - test_precopy_common(&args); + test_precopy_common(args); } -static void test_precopy_tcp_tls_x509_hostile_client(void) +static void test_precopy_tcp_tls_x509_hostile_client(char *name, + MigrateCommon *args) { - MigrateCommon args = { - .start = { - .hide_stderr = true, - }, - .listen_uri = "tcp:127.0.0.1:0", - .start_hook = migrate_hook_start_tls_x509_hostile_client, - .end_hook = migrate_hook_end_tls_x509, - .result = MIG_TEST_FAIL, - }; + args->listen_uri = "tcp:127.0.0.1:0"; + args->start_hook = migrate_hook_start_tls_x509_hostile_client; + args->end_hook = migrate_hook_end_tls_x509; + args->result = MIG_TEST_FAIL; + + args->start.hide_stderr = true; - test_precopy_common(&args); + test_precopy_common(args); } -static void test_precopy_tcp_tls_x509_allow_anon_client(void) +static void test_precopy_tcp_tls_x509_allow_anon_client(char *name, + MigrateCommon *args) { - MigrateCommon args = { - .listen_uri = "tcp:127.0.0.1:0", - .start_hook = migrate_hook_start_tls_x509_allow_anon_client, - .end_hook = migrate_hook_end_tls_x509, - }; + args->listen_uri = "tcp:127.0.0.1:0"; + args->start_hook = migrate_hook_start_tls_x509_allow_anon_client; + args->end_hook = migrate_hook_end_tls_x509; - test_precopy_common(&args); + test_precopy_common(args); } -static void test_precopy_tcp_tls_x509_reject_anon_client(void) +static void test_precopy_tcp_tls_x509_reject_anon_client(char *name, + MigrateCommon *args) { - MigrateCommon args = { - .start = { - .hide_stderr = true, - }, - .listen_uri = "tcp:127.0.0.1:0", - .start_hook = migrate_hook_start_tls_x509_reject_anon_client, - .end_hook = migrate_hook_end_tls_x509, - .result = MIG_TEST_FAIL, - }; + args->listen_uri = "tcp:127.0.0.1:0"; + args->start_hook = migrate_hook_start_tls_x509_reject_anon_client; + args->end_hook = migrate_hook_end_tls_x509; + args->result = MIG_TEST_FAIL; - test_precopy_common(&args); + args->start.hide_stderr = true; + + test_precopy_common(args); } #endif /* CONFIG_TASN1 */ @@ -XXX,XX +XXX,XX @@ migrate_hook_start_multifd_tls_x509_reject_anon_client(QTestState *from, } #endif /* CONFIG_TASN1 */ -static void test_multifd_tcp_tls_psk_match(void) +static void test_multifd_tcp_tls_psk_match(char *name, MigrateCommon *args) { - MigrateCommon args = { - .listen_uri = "defer", - .start_hook = migrate_hook_start_multifd_tcp_tls_psk_match, - .end_hook = migrate_hook_end_tls_psk, - .start = { - .caps[MIGRATION_CAPABILITY_MULTIFD] = true, - }, - }; - test_precopy_common(&args); + args->listen_uri = "defer"; + args->start_hook = migrate_hook_start_multifd_tcp_tls_psk_match; + args->end_hook = migrate_hook_end_tls_psk; + + args->start.caps[MIGRATION_CAPABILITY_MULTIFD] = true; + + test_precopy_common(args); } -static void test_multifd_tcp_tls_psk_mismatch(void) +static void test_multifd_tcp_tls_psk_mismatch(char *name, MigrateCommon *args) { - MigrateCommon args = { - .start = { - .hide_stderr = true, - .caps[MIGRATION_CAPABILITY_MULTIFD] = true, - }, - .listen_uri = "defer", - .start_hook = migrate_hook_start_multifd_tcp_tls_psk_mismatch, - .end_hook = migrate_hook_end_tls_psk, - .result = MIG_TEST_FAIL, - }; - test_precopy_common(&args); + args->listen_uri = "defer"; + args->start_hook = migrate_hook_start_multifd_tcp_tls_psk_mismatch; + args->end_hook = migrate_hook_end_tls_psk; + args->result = MIG_TEST_FAIL; + + args->start.hide_stderr = true; + args->start.caps[MIGRATION_CAPABILITY_MULTIFD] = true; + + test_precopy_common(args); } -static void test_multifd_postcopy_tcp_tls_psk_match(void) +static void test_multifd_postcopy_tcp_tls_psk_match(char *name, + MigrateCommon *args) { - MigrateCommon args = { - .start = { - .caps[MIGRATION_CAPABILITY_MULTIFD] = true, - .caps[MIGRATION_CAPABILITY_POSTCOPY_RAM] = true, - }, - .listen_uri = "defer", - .start_hook = migrate_hook_start_multifd_tcp_tls_psk_match, - .end_hook = migrate_hook_end_tls_psk, - }; + args->listen_uri = "defer"; + args->start_hook = migrate_hook_start_multifd_tcp_tls_psk_match; + args->end_hook = migrate_hook_end_tls_psk; - test_precopy_common(&args); + args->start.caps[MIGRATION_CAPABILITY_MULTIFD] = true; + args->start.caps[MIGRATION_CAPABILITY_POSTCOPY_RAM] = true; + + test_precopy_common(args); } #ifdef CONFIG_TASN1 -static void test_multifd_tcp_tls_x509_default_host(void) -{ - MigrateCommon args = { - .listen_uri = "defer", - .start_hook = migrate_hook_start_multifd_tls_x509_default_host, - .end_hook = migrate_hook_end_tls_x509, - .start = { - .caps[MIGRATION_CAPABILITY_MULTIFD] = true, - }, - }; - test_precopy_common(&args); +static void test_multifd_tcp_tls_x509_default_host(char *name, + MigrateCommon *args) +{ + args->listen_uri = "defer"; + args->start_hook = migrate_hook_start_multifd_tls_x509_default_host; + args->end_hook = migrate_hook_end_tls_x509; + + args->start.caps[MIGRATION_CAPABILITY_MULTIFD] = true; + + test_precopy_common(args); } -static void test_multifd_tcp_tls_x509_override_host(void) +static void test_multifd_tcp_tls_x509_override_host(char *name, + MigrateCommon *args) { - MigrateCommon args = { - .listen_uri = "defer", - .start_hook = migrate_hook_start_multifd_tls_x509_override_host, - .end_hook = migrate_hook_end_tls_x509, - .start = { - .caps[MIGRATION_CAPABILITY_MULTIFD] = true, - }, - }; - test_precopy_common(&args); + args->listen_uri = "defer"; + args->start_hook = migrate_hook_start_multifd_tls_x509_override_host; + args->end_hook = migrate_hook_end_tls_x509; + + args->start.caps[MIGRATION_CAPABILITY_MULTIFD] = true; + + test_precopy_common(args); } -static void test_multifd_tcp_tls_x509_mismatch_host(void) +static void test_multifd_tcp_tls_x509_mismatch_host(char *name, + MigrateCommon *args) { /* * This has different behaviour to the non-multifd case. @@ -XXX,XX +XXX,XX @@ static void test_multifd_tcp_tls_x509_mismatch_host(void) * to load migration state, and thus just aborts the migration * without exiting. */ - MigrateCommon args = { - .start = { - .hide_stderr = true, - .caps[MIGRATION_CAPABILITY_MULTIFD] = true, - }, - .listen_uri = "defer", - .start_hook = migrate_hook_start_multifd_tls_x509_mismatch_host, - .end_hook = migrate_hook_end_tls_x509, - .result = MIG_TEST_FAIL, - }; - test_precopy_common(&args); + args->listen_uri = "defer"; + args->start_hook = migrate_hook_start_multifd_tls_x509_mismatch_host; + args->end_hook = migrate_hook_end_tls_x509; + args->result = MIG_TEST_FAIL; + + args->start.hide_stderr = true; + args->start.caps[MIGRATION_CAPABILITY_MULTIFD] = true; + + test_precopy_common(args); } -static void test_multifd_tcp_tls_x509_allow_anon_client(void) +static void test_multifd_tcp_tls_x509_allow_anon_client(char *name, + MigrateCommon *args) { - MigrateCommon args = { - .listen_uri = "defer", - .start_hook = migrate_hook_start_multifd_tls_x509_allow_anon_client, - .end_hook = migrate_hook_end_tls_x509, - .start = { - .caps[MIGRATION_CAPABILITY_MULTIFD] = true, - }, - }; - test_precopy_common(&args); + args->listen_uri = "defer"; + args->start_hook = migrate_hook_start_multifd_tls_x509_allow_anon_client; + args->end_hook = migrate_hook_end_tls_x509; + + args->start.caps[MIGRATION_CAPABILITY_MULTIFD] = true; + + test_precopy_common(args); } -static void test_multifd_tcp_tls_x509_reject_anon_client(void) +static void test_multifd_tcp_tls_x509_reject_anon_client(char *name, + MigrateCommon *args) { - MigrateCommon args = { - .start = { - .hide_stderr = true, - .caps[MIGRATION_CAPABILITY_MULTIFD] = true, - }, - .listen_uri = "defer", - .start_hook = migrate_hook_start_multifd_tls_x509_reject_anon_client, - .end_hook = migrate_hook_end_tls_x509, - .result = MIG_TEST_FAIL, - }; - test_precopy_common(&args); + args->listen_uri = "defer"; + args->start_hook = migrate_hook_start_multifd_tls_x509_reject_anon_client; + args->end_hook = migrate_hook_end_tls_x509; + args->result = MIG_TEST_FAIL; + + args->start.hide_stderr = true; + args->start.caps[MIGRATION_CAPABILITY_MULTIFD] = true; + + test_precopy_common(args); } #endif /* CONFIG_TASN1 */ -- 2.50.1
From: Fabiano Rosas <farosas@suse.de> Pass the "args" parameter to the cancel tests so they can access the config object which will be part of this struct. Signed-off-by: Fabiano Rosas <farosas@suse.de> Link: https://lore.kernel.org/r/20251215220041.12657-27-farosas@suse.de Signed-off-by: Peter Xu <peterx@redhat.com> --- tests/qtest/migration/precopy-tests.c | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/tests/qtest/migration/precopy-tests.c b/tests/qtest/migration/precopy-tests.c index XXXXXXX..XXXXXXX 100644 --- a/tests/qtest/migration/precopy-tests.c +++ b/tests/qtest/migration/precopy-tests.c @@ -XXX,XX +XXX,XX @@ static void test_multifd_postcopy_tcp_cancel(char *name, MigrateCommon *args) } static void test_cancel_src_after_failed(QTestState *from, QTestState *to, - const char *uri, const char *phase) + const char *uri, const char *phase, + MigrateStart *args) { /* * No migrate_incoming_qmp() at the start to force source into @@ -XXX,XX +XXX,XX @@ static void test_cancel_src_after_failed(QTestState *from, QTestState *to, } static void test_cancel_src_after_cancelled(QTestState *from, QTestState *to, - const char *uri, const char *phase) + const char *uri, const char *phase, + MigrateStart *args) { migrate_incoming_qmp(to, uri, NULL, "{ 'exit-on-error': false }"); @@ -XXX,XX +XXX,XX @@ static void test_cancel_src_after_cancelled(QTestState *from, QTestState *to, } static void test_cancel_src_after_complete(QTestState *from, QTestState *to, - const char *uri, const char *phase) + const char *uri, const char *phase, + MigrateStart *args) { migrate_incoming_qmp(to, uri, NULL, "{ 'exit-on-error': false }"); @@ -XXX,XX +XXX,XX @@ static void test_cancel_src_after_complete(QTestState *from, QTestState *to, } static void test_cancel_src_after_none(QTestState *from, QTestState *to, - const char *uri, const char *phase) + const char *uri, const char *phase, + MigrateStart *args) { /* * Test that cancelling without a migration happening does not @@ -XXX,XX +XXX,XX @@ static void test_cancel_src_after_none(QTestState *from, QTestState *to, } static void test_cancel_src_pre_switchover(QTestState *from, QTestState *to, - const char *uri, const char *phase) + const char *uri, const char *phase, + MigrateStart *args) { migrate_set_capability(from, "pause-before-switchover", true); migrate_set_capability(to, "pause-before-switchover", true); @@ -XXX,XX +XXX,XX @@ static void test_cancel_src_after_status(char *test_path, MigrateCommon *args) if (g_str_equal(phase, "cancelling") || g_str_equal(phase, "cancelled")) { - test_cancel_src_after_cancelled(from, to, uri, phase); + test_cancel_src_after_cancelled(from, to, uri, phase, &args->start); } else if (g_str_equal(phase, "completed")) { - test_cancel_src_after_complete(from, to, uri, phase); + test_cancel_src_after_complete(from, to, uri, phase, &args->start); } else if (g_str_equal(phase, "failed")) { - test_cancel_src_after_failed(from, to, uri, phase); + test_cancel_src_after_failed(from, to, uri, phase, &args->start); } else if (g_str_equal(phase, "none")) { - test_cancel_src_after_none(from, to, uri, phase); + test_cancel_src_after_none(from, to, uri, phase, &args->start); } else { /* any state that comes before pre-switchover */ - test_cancel_src_pre_switchover(from, to, uri, phase); + test_cancel_src_pre_switchover(from, to, uri, phase, &args->start); } migrate_end(from, to, false); -- 2.50.1
From: Chuang Xu <xuchuangxclwt@bytedance.com> In our long-term experience in Bytedance, we've found that under the same load, live migration of larger VMs with more devices is often more difficult to converge (requiring a larger downtime limit). Through some testing and calculations, we conclude that bitmap sync time affects the calculation of live migration bandwidth. When the addresses processed are not aligned, a large number of clear_dirty ioctl occur (e.g. a 4MB misaligned memory can generate 2048 clear_dirty ioctls from two different memory_listener), which increases the time required for bitmap_sync and makes it more difficult for dirty pages to converge. For a 64C256G vm with 8 vhost-user-net(32 queue per nic) and 16 vhost-user-blk(4 queue per blk), the sync time is as high as *73ms* (tested with 10GBps dirty rate, the sync time increases as the dirty page rate increases), Here are each part of the sync time: - sync from kvm to ram_list: 2.5ms - vhost_log_sync:3ms - sync aligned memory from ram_list to RAMBlock: 5ms - sync misaligned memory from ram_list to RAMBlock: 61ms Attempt to merge those fragmented clear_dirty ioctls, then syncing misaligned memory from ram_list to RAMBlock takes only about 1ms, and the total sync time is only *12ms*. Signed-off-by: Chuang Xu <xuchuangxclwt@bytedance.com> Reviewed-by: Fabiano Rosas <farosas@suse.de> Link: https://lore.kernel.org/r/20251218114220.83354-1-xuchuangxclwt@bytedance.com [peterx: drop var "offset" in physical_memory_sync_dirty_bitmap] Signed-off-by: Peter Xu <peterx@redhat.com> --- include/system/physmem.h | 7 +++--- accel/tcg/cputlb.c | 5 +++-- migration/ram.c | 19 +++++----------- system/memory.c | 2 +- system/physmem.c | 48 +++++++++++++++++++++++++++------------- 5 files changed, 46 insertions(+), 35 deletions(-) diff --git a/include/system/physmem.h b/include/system/physmem.h index XXXXXXX..XXXXXXX 100644 --- a/include/system/physmem.h +++ b/include/system/physmem.h @@ -XXX,XX +XXX,XX @@ uint64_t physical_memory_set_dirty_lebitmap(unsigned long *bitmap, void physical_memory_dirty_bits_cleared(ram_addr_t start, ram_addr_t length); -bool physical_memory_test_and_clear_dirty(ram_addr_t start, - ram_addr_t length, - unsigned client); +uint64_t physical_memory_test_and_clear_dirty(ram_addr_t start, + ram_addr_t length, + unsigned client, + unsigned long *bmap); DirtyBitmapSnapshot * physical_memory_snapshot_and_clear_dirty(MemoryRegion *mr, hwaddr offset, diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c index XXXXXXX..XXXXXXX 100644 --- a/accel/tcg/cputlb.c +++ b/accel/tcg/cputlb.c @@ -XXX,XX +XXX,XX @@ void tlb_flush_page_bits_by_mmuidx_all_cpus_synced(CPUState *src_cpu, void tlb_protect_code(ram_addr_t ram_addr) { physical_memory_test_and_clear_dirty(ram_addr & TARGET_PAGE_MASK, - TARGET_PAGE_SIZE, - DIRTY_MEMORY_CODE); + TARGET_PAGE_SIZE, + DIRTY_MEMORY_CODE, + NULL); } /* update the TLB so that writes in physical page 'phys_addr' are no longer diff --git a/migration/ram.c b/migration/ram.c index XXXXXXX..XXXXXXX 100644 --- a/migration/ram.c +++ b/migration/ram.c @@ -XXX,XX +XXX,XX @@ static uint64_t physical_memory_sync_dirty_bitmap(RAMBlock *rb, ram_addr_t start, ram_addr_t length) { - ram_addr_t addr; unsigned long word = BIT_WORD((start + rb->offset) >> TARGET_PAGE_BITS); uint64_t num_dirty = 0; unsigned long *dest = rb->bmap; @@ -XXX,XX +XXX,XX @@ static uint64_t physical_memory_sync_dirty_bitmap(RAMBlock *rb, memory_region_clear_dirty_bitmap(rb->mr, start, length); } } else { - ram_addr_t offset = rb->offset; - - for (addr = 0; addr < length; addr += TARGET_PAGE_SIZE) { - if (physical_memory_test_and_clear_dirty( - start + addr + offset, - TARGET_PAGE_SIZE, - DIRTY_MEMORY_MIGRATION)) { - long k = (start + addr) >> TARGET_PAGE_BITS; - if (!test_and_set_bit(k, dest)) { - num_dirty++; - } - } - } + num_dirty = physical_memory_test_and_clear_dirty( + start + rb->offset, + length, + DIRTY_MEMORY_MIGRATION, + dest); } return num_dirty; diff --git a/system/memory.c b/system/memory.c index XXXXXXX..XXXXXXX 100644 --- a/system/memory.c +++ b/system/memory.c @@ -XXX,XX +XXX,XX @@ void memory_region_reset_dirty(MemoryRegion *mr, hwaddr addr, { assert(mr->ram_block); physical_memory_test_and_clear_dirty( - memory_region_get_ram_addr(mr) + addr, size, client); + memory_region_get_ram_addr(mr) + addr, size, client, NULL); } int memory_region_get_fd(MemoryRegion *mr) diff --git a/system/physmem.c b/system/physmem.c index XXXXXXX..XXXXXXX 100644 --- a/system/physmem.c +++ b/system/physmem.c @@ -XXX,XX +XXX,XX @@ void physical_memory_set_dirty_range(ram_addr_t start, ram_addr_t length, } } -/* Note: start and end must be within the same ram block. */ -bool physical_memory_test_and_clear_dirty(ram_addr_t start, +/* + * Note: start and end must be within the same ram block. + * + * @bmap usage: + * - When @bmap is provided, set bits for dirty pages, but + * only count those pages if the bit wasn't already set in @bmap. + * - When @bmap is NULL, count all dirty pages in the range. + * + * @return: + * - Number of dirty guest pages found within [start, start + length). + */ +uint64_t physical_memory_test_and_clear_dirty(ram_addr_t start, ram_addr_t length, - unsigned client) + unsigned client, + unsigned long *bmap) { DirtyMemoryBlocks *blocks; unsigned long end, page, start_page; - bool dirty = false; + uint64_t num_dirty = 0; RAMBlock *ramblock; uint64_t mr_offset, mr_size; if (length == 0) { - return false; + return 0; } end = TARGET_PAGE_ALIGN(start + length) >> TARGET_PAGE_BITS; @@ -XXX,XX +XXX,XX @@ bool physical_memory_test_and_clear_dirty(ram_addr_t start, while (page < end) { unsigned long idx = page / DIRTY_MEMORY_BLOCK_SIZE; unsigned long offset = page % DIRTY_MEMORY_BLOCK_SIZE; - unsigned long num = MIN(end - page, - DIRTY_MEMORY_BLOCK_SIZE - offset); - dirty |= bitmap_test_and_clear_atomic(blocks->blocks[idx], - offset, num); - page += num; + if (bitmap_test_and_clear_atomic(blocks->blocks[idx], offset, 1)) { + if (bmap) { + unsigned long k = page - (ramblock->offset >> TARGET_PAGE_BITS); + if (!test_and_set_bit(k, bmap)) { + num_dirty++; + } + } else { + num_dirty++; + } + } + + page++; } mr_offset = (ram_addr_t)(start_page << TARGET_PAGE_BITS) - ramblock->offset; @@ -XXX,XX +XXX,XX @@ bool physical_memory_test_and_clear_dirty(ram_addr_t start, memory_region_clear_dirty_bitmap(ramblock->mr, mr_offset, mr_size); } - if (dirty) { + if (num_dirty) { physical_memory_dirty_bits_cleared(start, length); } - return dirty; + return num_dirty; } static void physical_memory_clear_dirty_range(ram_addr_t addr, ram_addr_t length) { - physical_memory_test_and_clear_dirty(addr, length, DIRTY_MEMORY_MIGRATION); - physical_memory_test_and_clear_dirty(addr, length, DIRTY_MEMORY_VGA); - physical_memory_test_and_clear_dirty(addr, length, DIRTY_MEMORY_CODE); + physical_memory_test_and_clear_dirty(addr, length, DIRTY_MEMORY_MIGRATION, NULL); + physical_memory_test_and_clear_dirty(addr, length, DIRTY_MEMORY_VGA, NULL); + physical_memory_test_and_clear_dirty(addr, length, DIRTY_MEMORY_CODE, NULL); } DirtyBitmapSnapshot *physical_memory_snapshot_and_clear_dirty -- 2.50.1
From: "David Hildenbrand (Red Hat)" <david@kernel.org> I don't have a lot of capacity to do any maintanance (or even review) of "Memory API" lately, so remove myself. Fortunately we still do have two other maintainers and one reviewer :) Cc: Paolo Bonzini <pbonzini@redhat.com> Cc: Peter Xu <peterx@redhat.com> Cc: Philippe Mathieu-Daudé <philmd@linaro.org> Signed-off-by: David Hildenbrand (Red Hat) <david@kernel.org> Link: https://lore.kernel.org/r/20251222141438.409218-1-david@kernel.org Signed-off-by: Peter Xu <peterx@redhat.com> --- MAINTAINERS | 1 - 1 file changed, 1 deletion(-) diff --git a/MAINTAINERS b/MAINTAINERS index XXXXXXX..XXXXXXX 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -XXX,XX +XXX,XX @@ T: git https://gitlab.com/stsquad/qemu gdbstub/next Memory API M: Paolo Bonzini <pbonzini@redhat.com> M: Peter Xu <peterx@redhat.com> -M: David Hildenbrand <david@kernel.org> R: Philippe Mathieu-Daudé <philmd@linaro.org> S: Supported F: include/system/ioport.h -- 2.50.1
The following changes since commit 30e8a06b64aa58a3990ba39cb5d09531e7d265e0: Merge tag 'net-pull-request' of https://github.com/jasowang/qemu into staging (2026-06-29 17:41:42 +0200) are available in the Git repository at: https://gitlab.com/peterx/qemu.git tags/next-pull-request for you to fetch changes up to 60f2281450c4b4e9a16922fd69c66486a8207abb: migration: clean up postcopy blocktime presentation (2026-07-17 12:33:42 -0400) ---------------------------------------------------------------- Migration pull for rc1 - Bin's cleanup on migration blocktime (partly) - Yujun's fix on print format ---------------------------------------------------------------- Bin Guo (2): migration: fix ineffective overflow assert in postcopy blocktime migration: clean up postcopy blocktime presentation yujun (1): migration: Fix invalid %ud format and trace arg typo migration/migration-hmp-cmds.c | 2 +- migration/postcopy-ram.c | 14 ++++++-------- migration/trace-events | 6 +++--- 3 files changed, 10 insertions(+), 12 deletions(-) -- 2.54.0
From: yujun <yujun@kylinos.cn> Standard printf has no %ud conversion; glibc treats it as %u followed by a literal 'd', so postcopy userfaultfd error messages printed event values like "5d" instead of "5". The same typo existed in two postcopy discard trace format strings. Also rename the misspelled ram_save_iterate_big_wait() trace argument milliconds to milliseconds. Fixes: 00fa4fc85b ("postcopy: Allow registering of fd handler") Signed-off-by: yujun <yujun@kylinos.cn> Reviewed-by: Fabiano Rosas <farosas@suse.de> Link: https://lore.kernel.org/r/20260630031324.43453-1-yujun@kylinos.cn Signed-off-by: Peter Xu <peterx@redhat.com> --- migration/postcopy-ram.c | 4 ++-- migration/trace-events | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c index XXXXXXX..XXXXXXX 100644 --- a/migration/postcopy-ram.c +++ b/migration/postcopy-ram.c @@ -XXX,XX +XXX,XX @@ static void *postcopy_ram_fault_thread(void *opaque) } } if (msg.event != UFFD_EVENT_PAGEFAULT) { - error_report("%s: Read unexpected event %ud from userfaultfd", + error_report("%s: Read unexpected event %u from userfaultfd", __func__, msg.event); continue; /* It's not a page fault, shouldn't happen */ } @@ -XXX,XX +XXX,XX @@ retry: } } if (msg.event != UFFD_EVENT_PAGEFAULT) { - error_report("%s: Read unexpected event %ud " + error_report("%s: Read unexpected event %u " "from userfaultfd (shared)", __func__, msg.event); continue; /* It's not a page fault, shouldn't happen */ diff --git a/migration/trace-events b/migration/trace-events index XXXXXXX..XXXXXXX 100644 --- a/migration/trace-events +++ b/migration/trace-events @@ -XXX,XX +XXX,XX @@ loadvm_postcopy_handle_run(void) "" loadvm_postcopy_handle_resume(void) "" loadvm_postcopy_ram_handle_discard(void) "" loadvm_postcopy_ram_handle_discard_end(void) "" -loadvm_postcopy_ram_handle_discard_header(const char *ramid, uint16_t len) "%s: %ud" +loadvm_postcopy_ram_handle_discard_header(const char *ramid, uint16_t len) "%s: %u" loadvm_process_command(const char *s, uint16_t len) "com=%s len=%d" loadvm_process_command_ping(uint32_t val) "0x%x" loadvm_approve_switchover(unsigned int switchover_ack_pending_num) "Switchover ack pending num=%u" postcopy_ram_listen_thread_exit(void) "" postcopy_ram_listen_thread_start(void) "" qemu_savevm_send_postcopy_advise(void) "" -qemu_savevm_send_postcopy_ram_discard(const char *id, uint16_t len) "%s: %ud" +qemu_savevm_send_postcopy_ram_discard(const char *id, uint16_t len) "%s: %u" savevm_command_send(uint16_t command, uint16_t len) "com=0x%x len=%d" savevm_section_start(const char *id, unsigned int section_id) "%s, section_id %u" savevm_section_end(const char *id, unsigned int section_id, int ret) "%s, section_id %u -> %d" @@ -XXX,XX +XXX,XX @@ colo_flush_ram_cache_begin(uint64_t dirty_pages) "dirty_pages %" PRIu64 colo_flush_ram_cache_end(void) "" save_xbzrle_page_skipping(void) "" save_xbzrle_page_overflow(void) "" -ram_save_iterate_big_wait(uint64_t milliconds, int iterations) "big wait: %" PRIu64 " milliseconds, %d iterations" +ram_save_iterate_big_wait(uint64_t milliseconds, int iterations) "big wait: %" PRIu64 " milliseconds, %d iterations" ram_load_start(void) "" ram_load_complete(int ret, uint64_t seq_iter) "exit_code %d seq iteration %" PRIu64 ram_write_tracking_ramblock_start(const char *block_id, size_t page_size, void *addr, size_t length) "%s: page_size: %zu addr: %p length: %zu" -- 2.54.0
From: Bin Guo <guobin@linux.alibaba.com> vcpu_faults_current[] is uint8_t. The overflow assert was checked after the post-increment, so 255 would wrap to 0 and the assert would pass silently. Move the check before the increment and use < 255. Signed-off-by: Bin Guo <guobin@linux.alibaba.com> Link: https://lore.kernel.org/r/20260716101952.65329-2-guobin@linux.alibaba.com Signed-off-by: Peter Xu <peterx@redhat.com> --- migration/postcopy-ram.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c index XXXXXXX..XXXXXXX 100644 --- a/migration/postcopy-ram.c +++ b/migration/postcopy-ram.c @@ -XXX,XX +XXX,XX @@ void mark_postcopy_blocktime_begin(uintptr_t addr, uint32_t ptid, /* * Account how many concurrent faults on this vCPU we trapped. See * comments above vcpu_faults_current[] on why it can be more than one. + * + * vcpu_faults_current[] is uint8_t, so assert before incrementing to + * catch overflow before it wraps. */ + assert(dc->vcpu_faults_current[cpu] < 255); if (dc->vcpu_faults_current[cpu]++ == 0) { dc->smp_cpus_down++; /* @@ -XXX,XX +XXX,XX @@ void mark_postcopy_blocktime_begin(uintptr_t addr, uint32_t ptid, */ dc->last_begin = current; } - - /* Making sure it won't overflow - it really should never! */ - assert(dc->vcpu_faults_current[cpu] <= 255); } else { /* * For non-vCPU thread faults, we don't care about tid or cpu index -- 2.54.0
From: Bin Guo <guobin@linux.alibaba.com> Small cleanups for the postcopy blocktime fine-grained tracking feature: * Remove a redundant memset() on latency_buckets after g_new0(). * Use singular "Latency" in the HMP label for postcopy non-vCPU latency, consistent with other single-value labels. Signed-off-by: Bin Guo <guobin@linux.alibaba.com> Link: https://lore.kernel.org/r/20260716101952.65329-4-guobin@linux.alibaba.com Signed-off-by: Peter Xu <peterx@redhat.com> --- migration/migration-hmp-cmds.c | 2 +- migration/postcopy-ram.c | 3 --- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c index XXXXXXX..XXXXXXX 100644 --- a/migration/migration-hmp-cmds.c +++ b/migration/migration-hmp-cmds.c @@ -XXX,XX +XXX,XX @@ static void migration_dump_blocktime(Monitor *mon, MigrationInfo *info) } if (info->has_postcopy_non_vcpu_latency) { - monitor_printf(mon, "Postcopy non-vCPU Latencies (ns): %" PRIu64 "\n", + monitor_printf(mon, "Postcopy non-vCPU Latency (ns): %" PRIu64 "\n", info->postcopy_non_vcpu_latency); } diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c index XXXXXXX..XXXXXXX 100644 --- a/migration/postcopy-ram.c +++ b/migration/postcopy-ram.c @@ -XXX,XX +XXX,XX @@ static struct PostcopyBlocktimeContext *blocktime_context_new(void) unsigned int smp_cpus = ms->smp.cpus; PostcopyBlocktimeContext *ctx = g_new0(PostcopyBlocktimeContext, 1); - /* Initialize all counters to be zeros */ - memset(ctx->latency_buckets, 0, sizeof(ctx->latency_buckets)); - ctx->vcpu_blocktime_total = g_new0(uint64_t, smp_cpus); ctx->vcpu_faults_count = g_new0(uint64_t, smp_cpus); ctx->vcpu_faults_current = g_new0(uint8_t, smp_cpus); -- 2.54.0