Re: [PATCH v1 1/5] tests/qtest/dbus-vmstate: Bring the test up-to-date

[Fabiano Rosas <farosas@suse.de>]

Peter Xu <peterx@redhat.com> 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 <farosas@suse.de>
>
> I don't think I'm confident reviewing the whole series, but you can take:
>
> Acked-by: Peter Xu <peterx@redhat.com>
>
> 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
>>