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

[Fabiano Rosas <farosas@suse.de>]

Marc-André Lureau <marcandre.lureau@redhat.com> writes:

> Hi Fabiano
>
> On Mon, May 4, 2026 at 6:32 PM Fabiano Rosas <farosas@suse.de> wrote:
>>
>> 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.
>
> Should we keep skipping this test then? should I wait for an updated series?
>

No, it's all good. If I decide to change this, I'll send a patch on top.