Peter Xu <[email protected]> writes:
> On Wed, Apr 29, 2026 at 04:05:46PM -0300, Fabiano Rosas wrote:
>> The dbus-vmstate-test has been disabled for years. Here's the things
>> that have changed in the meantime and how to update the test:
>>
>> - Migration tests got new headers.
>> Update the includes.
>>
>> - migrate_qmp got new parameters.
>> Update the caller.
>>
>> - migrate_incoming_qmp is now used instead of -incoming URL.
>> Use -incoming defer.
>>
>> - Tests expecting failure should not check non-zero return code.
>> Check for failed migration state instead.
>>
>> - The test result enum was introduced.
>> Replace the migration_fail flag with the enum.
>>
>> - The DEVICE state was added.
>> Replace wait_for_migration_complete with migration_event_wait, which
>> won't trip on intermediary states.
>>
>> - Migration completion was reworked.
>> Explicitly wait for the RESUME event before asserting runstate is
>> RUNNING to avoid checking too quickly and seeing FINISH_MIGRATE
>> instead.
>>
>> - The FAILING state was added.
>> Wait for it before waiting for the RESUME event.
>>
>> - Sanity checks were added to migration_get_env().
>> Start calling that function in main.
>>
>> - qtest_add_func now has a wrapper.
>> Replace qtest_add_func with migration_test_add. Update tests'
>> signatures to take MigrationCommon, although it's unused.
>>
>> - meson now sets up G_TEST_DBUS_DAEMON.
>> Remove the logic around it.
>>
>> Signed-off-by: Fabiano Rosas <[email protected]>
>
> I don't think I'm confident reviewing the whole series, but you can take:
>
> Acked-by: Peter Xu <[email protected]>
>
> One trivial question below,
>
>> ---
>> tests/qtest/dbus-vmstate-test.c | 71 +++++++++++++++++++--------------
>> tests/qtest/meson.build | 7 +++-
>> 2 files changed, 46 insertions(+), 32 deletions(-)
>>
>> diff --git a/tests/qtest/dbus-vmstate-test.c
>> b/tests/qtest/dbus-vmstate-test.c
>> index 0a82cc9f93..90c050b448 100644
>> --- a/tests/qtest/dbus-vmstate-test.c
>> +++ b/tests/qtest/dbus-vmstate-test.c
>> @@ -2,8 +2,8 @@
>> #include <glib/gstdio.h>
>> #include <gio/gio.h>
>> #include "libqtest.h"
>> +#include "migration/migration-qmp.h"
>> #include "dbus-vmstate1.h"
>> -#include "migration-helpers.h"
>>
>> static char *workdir;
>>
>> @@ -29,7 +29,7 @@ typedef struct TestServer {
>>
>> typedef struct Test {
>> const char *id_list;
>> - bool migrate_fail;
>> + int result;
>> bool without_dst_b;
>> TestServer srcA;
>> TestServer dstA;
>> @@ -190,6 +190,7 @@ test_dbus_vmstate(Test *test)
>> g_autofree char *uri = NULL;
>> QTestState *src_qemu = NULL, *dst_qemu = NULL;
>> guint ownsrcA, ownsrcB, owndstA, owndstB;
>> + QTestMigrationState src_state = { };
>>
>> uri = g_strdup_printf("unix:%s/migsocket", workdir);
>>
>> @@ -224,17 +225,33 @@ test_dbus_vmstate(Test *test)
>>
>> src_qemu = qtest_init(src_qemu_args);
>> dst_qemu = qtest_init(dst_qemu_args);
>> +
>> + migrate_set_capability(src_qemu, "events", true);
>> + qtest_qmp_set_event_callback(src_qemu, migrate_watch_for_events,
>> + &src_state);
>> +
>> set_id_list(test, src_qemu);
>> set_id_list(test, dst_qemu);
>>
>> thread = g_thread_new("dbus-vmstate-thread", dbus_vmstate_thread, loop);
>>
>> migrate_incoming_qmp(dst_qemu, uri, NULL, "{}");
>> - migrate_qmp(src_qemu, uri, "{}");
>> + migrate_ensure_converge(src_qemu);
>> + migrate_qmp(src_qemu, NULL, uri, NULL, "{}");
>> test->src_qemu = src_qemu;
>> - if (test->migrate_fail) {
>> - wait_for_migration_fail(src_qemu, true);
>> - qtest_set_expected_status(dst_qemu, EXIT_FAILURE);
>> +
>> + if (test->result != MIG_TEST_SUCCEED) {
>> + QDict *rsp;
>> +
>> + migration_event_wait(src_qemu, "failing");
>> + wait_for_resume(src_qemu, &src_state);
>> + migration_event_wait(src_qemu, "failed");
>
> Not sure if we need such detailed checks over failing, resume, failed
> events on this one, but it looks ok.
>
I actually had a hang when not checking for failing. I didn't give it
much thought because I know we added a new state.
> Do you plan to remove wait_for_migration_fail() finally? We still have a
> few other users. IIUC, we could also switch to using events for
> wait_for_migration_fail().
>
Seems like a good idea to not need so many query-migrate
invocations. Except that we could hide some potential races with QMP
commands. And we lose the timeout as well. I'll take a look at what can
be done.
>> +
>> + rsp = qtest_qmp_assert_success_ref(src_qemu,
>> + "{ 'execute': 'query-status' }");
>> + g_assert(qdict_haskey(rsp, "running"));
>> + g_assert(qdict_get_bool(rsp, "running"));
>> + qobject_unref(rsp);
>> } else {
>> wait_for_migration_complete(src_qemu);
>> }
>> @@ -270,7 +287,7 @@ check_migrated(TestServer *s, TestServer *d)
>> }
>>
>> static void
>> -test_dbus_vmstate_without_list(void)
>> +test_dbus_vmstate_without_list(char *name, MigrateCommon *args)
>> {
>> Test test = { 0, };
>>
>> @@ -281,7 +298,7 @@ test_dbus_vmstate_without_list(void)
>> }
>>
>> static void
>> -test_dbus_vmstate_with_list(void)
>> +test_dbus_vmstate_with_list(char *name, MigrateCommon *args)
>> {
>> Test test = { .id_list = "idA,idB" };
>>
>> @@ -292,7 +309,7 @@ test_dbus_vmstate_with_list(void)
>> }
>>
>> static void
>> -test_dbus_vmstate_only_a(void)
>> +test_dbus_vmstate_only_a(char *name, MigrateCommon *args)
>> {
>> Test test = { .id_list = "idA" };
>>
>> @@ -303,9 +320,10 @@ test_dbus_vmstate_only_a(void)
>> }
>>
>> static void
>> -test_dbus_vmstate_missing_src(void)
>> +test_dbus_vmstate_missing_src(char *name, MigrateCommon *args)
>> {
>> - Test test = { .id_list = "idA,idC", .migrate_fail = true };
>> + Test test = { .id_list = "idA,idC",
>> + .result = MIG_TEST_FAIL };
>>
>> /* run in subprocess to silence QEMU error reporting */
>> if (g_test_subprocess()) {
>> @@ -320,11 +338,11 @@ test_dbus_vmstate_missing_src(void)
>> }
>>
>> static void
>> -test_dbus_vmstate_missing_dst(void)
>> +test_dbus_vmstate_missing_dst(char *name, MigrateCommon *args)
>> {
>> Test test = { .id_list = "idA,idB",
>> .without_dst_b = true,
>> - .migrate_fail = true };
>> + .result = MIG_TEST_FAIL };
>>
>> /* run in subprocess to silence QEMU error reporting */
>> if (g_test_subprocess()) {
>> @@ -343,15 +361,8 @@ int
>> main(int argc, char **argv)
>> {
>> GError *err = NULL;
>> - g_autofree char *dbus_daemon = NULL;
>> int ret;
>>
>> - dbus_daemon = g_build_filename(G_STRINGIFY(SRCDIR),
>> - "tests",
>> - "dbus-vmstate-daemon.sh",
>> - NULL);
>> - g_setenv("G_TEST_DBUS_DAEMON", dbus_daemon, true);
>> -
>> g_test_init(&argc, &argv, NULL);
>>
>> workdir = g_dir_make_tmp("dbus-vmstate-test-XXXXXX", &err);
>> @@ -362,16 +373,16 @@ main(int argc, char **argv)
>>
>> g_setenv("DBUS_VMSTATE_TEST_TMPDIR", workdir, true);
>>
>> - qtest_add_func("/dbus-vmstate/without-list",
>> - test_dbus_vmstate_without_list);
>> - qtest_add_func("/dbus-vmstate/with-list",
>> - test_dbus_vmstate_with_list);
>> - qtest_add_func("/dbus-vmstate/only-a",
>> - test_dbus_vmstate_only_a);
>> - qtest_add_func("/dbus-vmstate/missing-src",
>> - test_dbus_vmstate_missing_src);
>> - qtest_add_func("/dbus-vmstate/missing-dst",
>> - test_dbus_vmstate_missing_dst);
>> + migration_test_add("/dbus-vmstate/without-list",
>> + test_dbus_vmstate_without_list);
>> + migration_test_add("/dbus-vmstate/with-list",
>> + test_dbus_vmstate_with_list);
>> + migration_test_add("/dbus-vmstate/only-a",
>> + test_dbus_vmstate_only_a);
>> + migration_test_add("/dbus-vmstate/missing-src",
>> + test_dbus_vmstate_missing_src);
>> + migration_test_add("/dbus-vmstate/missing-dst",
>> + test_dbus_vmstate_missing_dst);
>>
>> ret = g_test_run();
>>
>> diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
>> index b735f55fc4..0d04e2cbaa 100644
>> --- a/tests/qtest/meson.build
>> +++ b/tests/qtest/meson.build
>> @@ -126,10 +126,12 @@ if dbus_daemon.found() and gdbus_codegen.found()
>> # Temporarily disabled due to Patchew failures:
>> #qtests_i386 += ['dbus-vmstate-test']
>> dbus_vmstate1 = custom_target('dbus-vmstate description',
>> - output: ['dbus-vmstate1.h',
>> 'dbus-vmstate1.c'],
>> + build_by_default: true,
>> + output: [ 'dbus-vmstate1.h',
>> 'dbus-vmstate1.c'],
>> input: meson.project_source_root() /
>> 'backends/dbus-vmstate1.xml',
>> command: [gdbus_codegen, '@INPUT@',
>> '--interface-prefix', 'org.qemu',
>> + '--output-directory',
>> meson.current_build_dir(),
>> '--generate-c-code',
>> '@BASENAME@']).to_list()
>> else
>> dbus_vmstate1 = []
>> @@ -385,7 +387,8 @@ qtests = {
>> 'bios-tables-test': [io, 'boot-sector.c', 'acpi-utils.c', 'tpm-emu.c'],
>> 'cdrom-test': files('boot-sector.c'),
>> 'dbus-vmstate-test': files('migration/migration-qmp.c',
>> - 'migration/migration-util.c') + dbus_vmstate1,
>> + 'migration/migration-util.c') + dbus_vmstate1 +
>> + [gio],
>> 'erst-test': files('erst-test.c'),
>> 'ivshmem-test': [rt, '../../contrib/ivshmem-server/ivshmem-server.c'],
>> 'migration-test': test_migration_files + migration_tls_files +
>> migration_colo_files,
>> --
>> 2.51.0
>>
