[PATCH v1 0/7] migration cleanups and testing improvements

[PATCH v1 0/7] migration cleanups and testing improvements

[Fabiano Rosas]

Hi,

These are some general cleanups and improvements to testing and
debugging that I collected over the past month.

Fabiano Rosas (7):
  migration/multifd: Remove MultiFDPages_t::packet_num
  migration/multifd: Remove QEMUFile from where it is not needed
  migration/multifd: Change multifd_pages_init argument
  migration: Report error in incoming migration
  tests/qtest/migration: Print migration incoming errors
  tests/qtest/migration: Add a wrapper to print test names
  tests/qtest/migration: Use the new migration_test_add

 migration/migration.c           |   7 ++
 migration/multifd.c             |  19 ++-
 migration/multifd.h             |   6 +-
 migration/ram.c                 |  15 ++-
 tests/qtest/migration-helpers.c |  38 ++++++
 tests/qtest/migration-helpers.h |   1 +
 tests/qtest/migration-test.c    | 201 +++++++++++++++++---------------
 7 files changed, 168 insertions(+), 119 deletions(-)

-- 
2.35.3

[PATCH v1 1/7] migration/multifd: Remove MultiFDPages_t::packet_num

[Fabiano Rosas]

This was introduced by commit 34c55a94b1 ("migration: Create multipage
support") and never used.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/multifd.c | 1 -
 migration/multifd.h | 2 --
 2 files changed, 3 deletions(-)

diff --git a/migration/multifd.c b/migration/multifd.c
index ec58c58082..e7dd9c6317 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -250,7 +250,6 @@ static void multifd_pages_clear(MultiFDPages_t *pages)
 {
     pages->num = 0;
     pages->allocated = 0;
-    pages->packet_num = 0;
     pages->block = NULL;
     g_free(pages->offset);
     pages->offset = NULL;
diff --git a/migration/multifd.h b/migration/multifd.h
index a835643b48..b0ff610c37 100644
--- a/migration/multifd.h
+++ b/migration/multifd.h
@@ -58,8 +58,6 @@ typedef struct {
     uint32_t num;
     /* number of allocated pages */
     uint32_t allocated;
-    /* global number of generated multifd packets */
-    uint64_t packet_num;
     /* offset of each page */
     ram_addr_t *offset;
     RAMBlock *block;
-- 
2.35.3

Re: [PATCH v1 1/7] migration/multifd: Remove MultiFDPages_t::packet_num

[Peter Xu]

On Fri, Nov 24, 2023 at 01:14:26PM -0300, Fabiano Rosas wrote:
> This was introduced by commit 34c55a94b1 ("migration: Create multipage
> support") and never used.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

[PATCH v1 2/7] migration/multifd: Remove QEMUFile from where it is not needed

[Fabiano Rosas]

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/multifd.c | 12 ++++++------
 migration/multifd.h |  4 ++--
 migration/ram.c     | 15 +++++++--------
 3 files changed, 15 insertions(+), 16 deletions(-)

diff --git a/migration/multifd.c b/migration/multifd.c
index e7dd9c6317..7e7fe59fcb 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -390,7 +390,7 @@ struct {
  * false.
  */
 
-static int multifd_send_pages(QEMUFile *f)
+static int multifd_send_pages(void)
 {
     int i;
     static int next_channel;
@@ -436,7 +436,7 @@ static int multifd_send_pages(QEMUFile *f)
     return 1;
 }
 
-int multifd_queue_page(QEMUFile *f, RAMBlock *block, ram_addr_t offset)
+int multifd_queue_page(RAMBlock *block, ram_addr_t offset)
 {
     MultiFDPages_t *pages = multifd_send_state->pages;
     bool changed = false;
@@ -456,12 +456,12 @@ int multifd_queue_page(QEMUFile *f, RAMBlock *block, ram_addr_t offset)
         changed = true;
     }
 
-    if (multifd_send_pages(f) < 0) {
+    if (multifd_send_pages() < 0) {
         return -1;
     }
 
     if (changed) {
-        return multifd_queue_page(f, block, offset);
+        return multifd_queue_page(block, offset);
     }
 
     return 1;
@@ -583,7 +583,7 @@ static int multifd_zero_copy_flush(QIOChannel *c)
     return ret;
 }
 
-int multifd_send_sync_main(QEMUFile *f)
+int multifd_send_sync_main(void)
 {
     int i;
     bool flush_zero_copy;
@@ -592,7 +592,7 @@ int multifd_send_sync_main(QEMUFile *f)
         return 0;
     }
     if (multifd_send_state->pages->num) {
-        if (multifd_send_pages(f) < 0) {
+        if (multifd_send_pages() < 0) {
             error_report("%s: multifd_send_pages fail", __func__);
             return -1;
         }
diff --git a/migration/multifd.h b/migration/multifd.h
index b0ff610c37..35d11f103c 100644
--- a/migration/multifd.h
+++ b/migration/multifd.h
@@ -21,8 +21,8 @@ void multifd_load_shutdown(void);
 bool multifd_recv_all_channels_created(void);
 void multifd_recv_new_channel(QIOChannel *ioc, Error **errp);
 void multifd_recv_sync_main(void);
-int multifd_send_sync_main(QEMUFile *f);
-int multifd_queue_page(QEMUFile *f, RAMBlock *block, ram_addr_t offset);
+int multifd_send_sync_main(void);
+int multifd_queue_page(RAMBlock *block, ram_addr_t offset);
 
 /* Multifd Compression flags */
 #define MULTIFD_FLAG_SYNC (1 << 0)
diff --git a/migration/ram.c b/migration/ram.c
index 8c7886ab79..b4f2f5108f 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1250,10 +1250,9 @@ static int ram_save_page(RAMState *rs, PageSearchStatus *pss)
     return pages;
 }
 
-static int ram_save_multifd_page(QEMUFile *file, RAMBlock *block,
-                                 ram_addr_t offset)
+static int ram_save_multifd_page(RAMBlock *block, ram_addr_t offset)
 {
-    if (multifd_queue_page(file, block, offset) < 0) {
+    if (multifd_queue_page(block, offset) < 0) {
         return -1;
     }
     stat64_add(&mig_stats.normal_pages, 1);
@@ -1336,7 +1335,7 @@ static int find_dirty_block(RAMState *rs, PageSearchStatus *pss)
             if (migrate_multifd() &&
                 !migrate_multifd_flush_after_each_section()) {
                 QEMUFile *f = rs->pss[RAM_CHANNEL_PRECOPY].pss_channel;
-                int ret = multifd_send_sync_main(f);
+                int ret = multifd_send_sync_main();
                 if (ret < 0) {
                     return ret;
                 }
@@ -2067,7 +2066,7 @@ static int ram_save_target_page_legacy(RAMState *rs, PageSearchStatus *pss)
      * still see partially copied pages which is data corruption.
      */
     if (migrate_multifd() && !migration_in_postcopy()) {
-        return ram_save_multifd_page(pss->pss_channel, block, offset);
+        return ram_save_multifd_page(block, offset);
     }
 
     return ram_save_page(rs, pss);
@@ -2985,7 +2984,7 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
     migration_ops->ram_save_target_page = ram_save_target_page_legacy;
 
     qemu_mutex_unlock_iothread();
-    ret = multifd_send_sync_main(f);
+    ret = multifd_send_sync_main();
     qemu_mutex_lock_iothread();
     if (ret < 0) {
         return ret;
@@ -3109,7 +3108,7 @@ out:
     if (ret >= 0
         && migration_is_setup_or_active(migrate_get_current()->state)) {
         if (migrate_multifd() && migrate_multifd_flush_after_each_section()) {
-            ret = multifd_send_sync_main(rs->pss[RAM_CHANNEL_PRECOPY].pss_channel);
+            ret = multifd_send_sync_main();
             if (ret < 0) {
                 return ret;
             }
@@ -3183,7 +3182,7 @@ static int ram_save_complete(QEMUFile *f, void *opaque)
         }
     }
 
-    ret = multifd_send_sync_main(rs->pss[RAM_CHANNEL_PRECOPY].pss_channel);
+    ret = multifd_send_sync_main();
     if (ret < 0) {
         return ret;
     }
-- 
2.35.3

Re: [PATCH v1 2/7] migration/multifd: Remove QEMUFile from where it is not needed

[Peter Xu]

On Fri, Nov 24, 2023 at 01:14:27PM -0300, Fabiano Rosas wrote:
> Signed-off-by: Fabiano Rosas <farosas@suse.de>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

[PATCH v1 3/7] migration/multifd: Change multifd_pages_init argument

[Fabiano Rosas]

The 'size' argument is actually the number of pages that fit in a
multifd packet. Change it to uint32_t and rename.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/multifd.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/migration/multifd.c b/migration/multifd.c
index 7e7fe59fcb..a9ae8fbb12 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -236,12 +236,12 @@ static int multifd_recv_initial_packet(QIOChannel *c, Error **errp)
     return msg.id;
 }
 
-static MultiFDPages_t *multifd_pages_init(size_t size)
+static MultiFDPages_t *multifd_pages_init(uint32_t n)
 {
     MultiFDPages_t *pages = g_new0(MultiFDPages_t, 1);
 
-    pages->allocated = size;
-    pages->offset = g_new0(ram_addr_t, size);
+    pages->allocated = n;
+    pages->offset = g_new0(ram_addr_t, n);
 
     return pages;
 }
-- 
2.35.3

Re: [PATCH v1 3/7] migration/multifd: Change multifd_pages_init argument

[Peter Xu]

On Fri, Nov 24, 2023 at 01:14:28PM -0300, Fabiano Rosas wrote:
> The 'size' argument is actually the number of pages that fit in a
> multifd packet. Change it to uint32_t and rename.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

[PATCH v1 4/7] migration: Report error in incoming migration

[Fabiano Rosas]

We're not currently reporting the errors set with migrate_set_error()
when incoming migration fails.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/migration.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/migration/migration.c b/migration/migration.c
index 28a34c9068..cca32c553c 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -698,6 +698,13 @@ process_incoming_migration_co(void *opaque)
     }
 
     if (ret < 0) {
+        MigrationState *s = migrate_get_current();
+
+        if (migrate_has_error(s)) {
+            WITH_QEMU_LOCK_GUARD(&s->error_mutex) {
+                error_report_err(s->error);
+            }
+        }
         error_report("load of migration failed: %s", strerror(-ret));
         goto fail;
     }
-- 
2.35.3

Re: [PATCH v1 4/7] migration: Report error in incoming migration

[Peter Xu]

On Fri, Nov 24, 2023 at 01:14:29PM -0300, Fabiano Rosas wrote:
> We're not currently reporting the errors set with migrate_set_error()
> when incoming migration fails.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

[PATCH v1 5/7] tests/qtest/migration: Print migration incoming errors

[Fabiano Rosas]

We're currently just asserting when incoming migration fails. Let's
print the error message from QMP as well.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 tests/qtest/migration-helpers.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
index 24fb7b3525..f1106128a9 100644
--- a/tests/qtest/migration-helpers.c
+++ b/tests/qtest/migration-helpers.c
@@ -118,6 +118,12 @@ void migrate_incoming_qmp(QTestState *to, const char *uri, const char *fmt, ...)
 
     rsp = qtest_qmp(to, "{ 'execute': 'migrate-incoming', 'arguments': %p}",
                     args);
+
+    if (!qdict_haskey(rsp, "return")) {
+        g_autoptr(GString) s = qobject_to_json_pretty(QOBJECT(rsp), true);
+        g_test_message("%s", s->str);
+    }
+
     g_assert(qdict_haskey(rsp, "return"));
     qobject_unref(rsp);
 
-- 
2.35.3

Re: [PATCH v1 5/7] tests/qtest/migration: Print migration incoming errors

[Peter Xu]

On Fri, Nov 24, 2023 at 01:14:30PM -0300, Fabiano Rosas wrote:
> We're currently just asserting when incoming migration fails. Let's
> print the error message from QMP as well.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>  tests/qtest/migration-helpers.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
> index 24fb7b3525..f1106128a9 100644
> --- a/tests/qtest/migration-helpers.c
> +++ b/tests/qtest/migration-helpers.c
> @@ -118,6 +118,12 @@ void migrate_incoming_qmp(QTestState *to, const char *uri, const char *fmt, ...)
>  
>      rsp = qtest_qmp(to, "{ 'execute': 'migrate-incoming', 'arguments': %p}",
>                      args);
> +
> +    if (!qdict_haskey(rsp, "return")) {
> +        g_autoptr(GString) s = qobject_to_json_pretty(QOBJECT(rsp), true);
> +        g_test_message("%s", s->str);
> +    }

This traps the "migrate-incoming" command only (which, afaiu, only setup
the listening), would this capture the incoming error?

> +
>      g_assert(qdict_haskey(rsp, "return"));
>      qobject_unref(rsp);
>  
> -- 
> 2.35.3
> 

-- 
Peter Xu

Re: [PATCH v1 5/7] tests/qtest/migration: Print migration incoming errors

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> On Fri, Nov 24, 2023 at 01:14:30PM -0300, Fabiano Rosas wrote:
>> We're currently just asserting when incoming migration fails. Let's
>> print the error message from QMP as well.
>> 
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>>  tests/qtest/migration-helpers.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>> 
>> diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
>> index 24fb7b3525..f1106128a9 100644
>> --- a/tests/qtest/migration-helpers.c
>> +++ b/tests/qtest/migration-helpers.c
>> @@ -118,6 +118,12 @@ void migrate_incoming_qmp(QTestState *to, const char *uri, const char *fmt, ...)
>>  
>>      rsp = qtest_qmp(to, "{ 'execute': 'migrate-incoming', 'arguments': %p}",
>>                      args);
>> +
>> +    if (!qdict_haskey(rsp, "return")) {
>> +        g_autoptr(GString) s = qobject_to_json_pretty(QOBJECT(rsp), true);
>> +        g_test_message("%s", s->str);
>> +    }
>
> This traps the "migrate-incoming" command only (which, afaiu, only setup
> the listening), would this capture the incoming error?

This is about the migrate-incoming only. We could replace "incoming
migration" with "qmp_migrate_incoming" in the commit message to clarify.

Re: [PATCH v1 5/7] tests/qtest/migration: Print migration incoming errors

[Peter Xu]

On Mon, Nov 27, 2023 at 12:52:38PM -0300, Fabiano Rosas wrote:
> >> @@ -118,6 +118,12 @@ void migrate_incoming_qmp(QTestState *to, const char *uri, const char *fmt, ...)
> >>  
> >>      rsp = qtest_qmp(to, "{ 'execute': 'migrate-incoming', 'arguments': %p}",
> >>                      args);
> >> +
> >> +    if (!qdict_haskey(rsp, "return")) {
> >> +        g_autoptr(GString) s = qobject_to_json_pretty(QOBJECT(rsp), true);
> >> +        g_test_message("%s", s->str);
> >> +    }
> >
> > This traps the "migrate-incoming" command only (which, afaiu, only setup
> > the listening), would this capture the incoming error?
> 
> This is about the migrate-incoming only. We could replace "incoming
> migration" with "qmp_migrate_incoming" in the commit message to clarify.

Ah.. Did you ever see this failure in any of your runs in these tests?  I
think it means you hit the assertion right below this part, but I'm just
curious how, as the URIs in the test cases are pretty constant.

-- 
Peter Xu

Re: [PATCH v1 5/7] tests/qtest/migration: Print migration incoming errors

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> On Mon, Nov 27, 2023 at 12:52:38PM -0300, Fabiano Rosas wrote:
>> >> @@ -118,6 +118,12 @@ void migrate_incoming_qmp(QTestState *to, const char *uri, const char *fmt, ...)
>> >>  
>> >>      rsp = qtest_qmp(to, "{ 'execute': 'migrate-incoming', 'arguments': %p}",
>> >>                      args);
>> >> +
>> >> +    if (!qdict_haskey(rsp, "return")) {
>> >> +        g_autoptr(GString) s = qobject_to_json_pretty(QOBJECT(rsp), true);
>> >> +        g_test_message("%s", s->str);
>> >> +    }
>> >
>> > This traps the "migrate-incoming" command only (which, afaiu, only setup
>> > the listening), would this capture the incoming error?
>> 
>> This is about the migrate-incoming only. We could replace "incoming
>> migration" with "qmp_migrate_incoming" in the commit message to clarify.
>
> Ah.. Did you ever see this failure in any of your runs in these tests?  I
> think it means you hit the assertion right below this part, but I'm just
> curious how, as the URIs in the test cases are pretty constant.

Yes, I don't remember what exactly, but we changed the code that parses
the URIs in this release and I'm also working on
file_start_incoming_migration.

Re: [PATCH v1 5/7] tests/qtest/migration: Print migration incoming errors

[Peter Xu]

On Mon, Nov 27, 2023 at 05:32:45PM -0300, Fabiano Rosas wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > On Mon, Nov 27, 2023 at 12:52:38PM -0300, Fabiano Rosas wrote:
> >> >> @@ -118,6 +118,12 @@ void migrate_incoming_qmp(QTestState *to, const char *uri, const char *fmt, ...)
> >> >>  
> >> >>      rsp = qtest_qmp(to, "{ 'execute': 'migrate-incoming', 'arguments': %p}",
> >> >>                      args);
> >> >> +
> >> >> +    if (!qdict_haskey(rsp, "return")) {
> >> >> +        g_autoptr(GString) s = qobject_to_json_pretty(QOBJECT(rsp), true);
> >> >> +        g_test_message("%s", s->str);
> >> >> +    }
> >> >
> >> > This traps the "migrate-incoming" command only (which, afaiu, only setup
> >> > the listening), would this capture the incoming error?
> >> 
> >> This is about the migrate-incoming only. We could replace "incoming
> >> migration" with "qmp_migrate_incoming" in the commit message to clarify.
> >
> > Ah.. Did you ever see this failure in any of your runs in these tests?  I
> > think it means you hit the assertion right below this part, but I'm just
> > curious how, as the URIs in the test cases are pretty constant.
> 
> Yes, I don't remember what exactly, but we changed the code that parses
> the URIs in this release and I'm also working on
> file_start_incoming_migration.

OK then.

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

[PATCH v1 6/7] tests/qtest/migration: Add a wrapper to print test names

[Fabiano Rosas]

Our usage of gtest results in us losing the very basic functionality
of "knowing which test failed". The issue is that gtest only prints
test names ("paths" in gtest parlance) once the test has finished, but
we use asserts in the tests and crash gtest itself before it can print
anything. We also use a final abort when the result of g_test_run is
not 0.

Depending on how the test failed/broke we can see the function that
trigged the abort, which may be representative of the test, but it
could also just be some generic function.

We have been relying on the primitive method of looking at the name of
the previous successful test and then looking at the code to figure
out which test should have come next.

Add a wrapper to the test registration that does the job of printing
the test name before running.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 tests/qtest/migration-helpers.c | 32 ++++++++++++++++++++++++++++++++
 tests/qtest/migration-helpers.h |  1 +
 2 files changed, 33 insertions(+)

diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
index f1106128a9..164e09c299 100644
--- a/tests/qtest/migration-helpers.c
+++ b/tests/qtest/migration-helpers.c
@@ -298,3 +298,35 @@ char *resolve_machine_version(const char *alias, const char *var1,
 
     return find_common_machine_version(machine_name, var1, var2);
 }
+
+typedef struct {
+    char *name;
+    void (*func)(void);
+} MigrationTest;
+
+static void migration_test_destroy(gpointer data)
+{
+    MigrationTest *test = (MigrationTest *)data;
+
+    g_free(test->name);
+    g_free(test);
+}
+
+static void migration_test_wrapper(const void *data)
+{
+    MigrationTest *test = (MigrationTest *)data;
+
+    g_test_message("Running /%s%s", qtest_get_arch(), test->name);
+    test->func();
+}
+
+void migration_test_add(const char *path, void (*fn)(void))
+{
+    MigrationTest *test = g_new0(MigrationTest, 1);
+
+    test->func = fn;
+    test->name = g_strdup(path);
+
+    qtest_add_data_func_full(path, test, migration_test_wrapper,
+                             migration_test_destroy);
+}
diff --git a/tests/qtest/migration-helpers.h b/tests/qtest/migration-helpers.h
index e31dc85cc7..0d9a02edc7 100644
--- a/tests/qtest/migration-helpers.h
+++ b/tests/qtest/migration-helpers.h
@@ -47,4 +47,5 @@ char *find_common_machine_version(const char *mtype, const char *var1,
                                   const char *var2);
 char *resolve_machine_version(const char *alias, const char *var1,
                               const char *var2);
+void migration_test_add(const char *path, void (*fn)(void));
 #endif /* MIGRATION_HELPERS_H */
-- 
2.35.3

Re: [PATCH v1 6/7] tests/qtest/migration: Add a wrapper to print test names

[Peter Xu]

On Fri, Nov 24, 2023 at 01:14:31PM -0300, Fabiano Rosas wrote:
> Our usage of gtest results in us losing the very basic functionality
> of "knowing which test failed". The issue is that gtest only prints
> test names ("paths" in gtest parlance) once the test has finished, but
> we use asserts in the tests and crash gtest itself before it can print
> anything. We also use a final abort when the result of g_test_run is
> not 0.
> 
> Depending on how the test failed/broke we can see the function that
> trigged the abort, which may be representative of the test, but it
> could also just be some generic function.
> 
> We have been relying on the primitive method of looking at the name of
> the previous successful test and then looking at the code to figure
> out which test should have come next.
> 
> Add a wrapper to the test registration that does the job of printing
> the test name before running.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>  tests/qtest/migration-helpers.c | 32 ++++++++++++++++++++++++++++++++
>  tests/qtest/migration-helpers.h |  1 +
>  2 files changed, 33 insertions(+)
> 
> diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
> index f1106128a9..164e09c299 100644
> --- a/tests/qtest/migration-helpers.c
> +++ b/tests/qtest/migration-helpers.c
> @@ -298,3 +298,35 @@ char *resolve_machine_version(const char *alias, const char *var1,
>  
>      return find_common_machine_version(machine_name, var1, var2);
>  }
> +
> +typedef struct {
> +    char *name;
> +    void (*func)(void);
> +} MigrationTest;
> +
> +static void migration_test_destroy(gpointer data)
> +{
> +    MigrationTest *test = (MigrationTest *)data;
> +
> +    g_free(test->name);
> +    g_free(test);
> +}
> +
> +static void migration_test_wrapper(const void *data)
> +{
> +    MigrationTest *test = (MigrationTest *)data;
> +
> +    g_test_message("Running /%s%s", qtest_get_arch(), test->name);

/%s/%s?

> +    test->func();
> +}

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

Re: [PATCH v1 6/7] tests/qtest/migration: Add a wrapper to print test names

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> On Fri, Nov 24, 2023 at 01:14:31PM -0300, Fabiano Rosas wrote:
>> Our usage of gtest results in us losing the very basic functionality
>> of "knowing which test failed". The issue is that gtest only prints
>> test names ("paths" in gtest parlance) once the test has finished, but
>> we use asserts in the tests and crash gtest itself before it can print
>> anything. We also use a final abort when the result of g_test_run is
>> not 0.
>> 
>> Depending on how the test failed/broke we can see the function that
>> trigged the abort, which may be representative of the test, but it
>> could also just be some generic function.
>> 
>> We have been relying on the primitive method of looking at the name of
>> the previous successful test and then looking at the code to figure
>> out which test should have come next.
>> 
>> Add a wrapper to the test registration that does the job of printing
>> the test name before running.
>> 
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>>  tests/qtest/migration-helpers.c | 32 ++++++++++++++++++++++++++++++++
>>  tests/qtest/migration-helpers.h |  1 +
>>  2 files changed, 33 insertions(+)
>> 
>> diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
>> index f1106128a9..164e09c299 100644
>> --- a/tests/qtest/migration-helpers.c
>> +++ b/tests/qtest/migration-helpers.c
>> @@ -298,3 +298,35 @@ char *resolve_machine_version(const char *alias, const char *var1,
>>  
>>      return find_common_machine_version(machine_name, var1, var2);
>>  }
>> +
>> +typedef struct {
>> +    char *name;
>> +    void (*func)(void);
>> +} MigrationTest;
>> +
>> +static void migration_test_destroy(gpointer data)
>> +{
>> +    MigrationTest *test = (MigrationTest *)data;
>> +
>> +    g_free(test->name);
>> +    g_free(test);
>> +}
>> +
>> +static void migration_test_wrapper(const void *data)
>> +{
>> +    MigrationTest *test = (MigrationTest *)data;
>> +
>> +    g_test_message("Running /%s%s", qtest_get_arch(), test->name);
>
> /%s/%s?

The test name contains a leading slash.

Re: [PATCH v1 6/7] tests/qtest/migration: Add a wrapper to print test names

[Peter Xu]

On Mon, Nov 27, 2023 at 12:44:53PM -0300, Fabiano Rosas wrote:
> >> +static void migration_test_wrapper(const void *data)
> >> +{
> >> +    MigrationTest *test = (MigrationTest *)data;
> >> +
> >> +    g_test_message("Running /%s%s", qtest_get_arch(), test->name);
> >
> > /%s/%s?
> 
> The test name contains a leading slash.

Then I suppose qtest_add_func() just tried to omit the duplicated '/'?

Never mind then.  My R-b holds.  Thanks,

-- 
Peter Xu

[PATCH v1 7/7] tests/qtest/migration: Use the new migration_test_add

[Fabiano Rosas]

Replace the tests registration with the new function that prints tests
names.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 tests/qtest/migration-test.c | 201 ++++++++++++++++++-----------------
 1 file changed, 104 insertions(+), 97 deletions(-)

diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 0fbaa6a90f..4884a40be2 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -3339,62 +3339,65 @@ int main(int argc, char **argv)
     module_call_init(MODULE_INIT_QOM);
 
     if (has_uffd) {
-        qtest_add_func("/migration/postcopy/plain", test_postcopy);
-        qtest_add_func("/migration/postcopy/recovery/plain",
-                       test_postcopy_recovery);
-        qtest_add_func("/migration/postcopy/preempt/plain", test_postcopy_preempt);
-        qtest_add_func("/migration/postcopy/preempt/recovery/plain",
-                       test_postcopy_preempt_recovery);
+        migration_test_add("/migration/postcopy/plain", test_postcopy);
+        migration_test_add("/migration/postcopy/recovery/plain",
+                           test_postcopy_recovery);
+        migration_test_add("/migration/postcopy/preempt/plain",
+                           test_postcopy_preempt);
+        migration_test_add("/migration/postcopy/preempt/recovery/plain",
+                           test_postcopy_preempt_recovery);
         if (getenv("QEMU_TEST_FLAKY_TESTS")) {
-            qtest_add_func("/migration/postcopy/compress/plain",
-                           test_postcopy_compress);
-            qtest_add_func("/migration/postcopy/recovery/compress/plain",
-                           test_postcopy_recovery_compress);
+            migration_test_add("/migration/postcopy/compress/plain",
+                               test_postcopy_compress);
+            migration_test_add("/migration/postcopy/recovery/compress/plain",
+                               test_postcopy_recovery_compress);
         }
 #ifndef _WIN32
-        qtest_add_func("/migration/postcopy/recovery/double-failures",
-                       test_postcopy_recovery_double_fail);
+        migration_test_add("/migration/postcopy/recovery/double-failures",
+                           test_postcopy_recovery_double_fail);
 #endif /* _WIN32 */
 
     }
 
-    qtest_add_func("/migration/bad_dest", test_baddest);
+    migration_test_add("/migration/bad_dest", test_baddest);
 #ifndef _WIN32
     if (!g_str_equal(arch, "s390x")) {
-        qtest_add_func("/migration/analyze-script", test_analyze_script);
+        migration_test_add("/migration/analyze-script", test_analyze_script);
     }
 #endif
-    qtest_add_func("/migration/precopy/unix/plain", test_precopy_unix_plain);
-    qtest_add_func("/migration/precopy/unix/xbzrle", test_precopy_unix_xbzrle);
+    migration_test_add("/migration/precopy/unix/plain",
+                       test_precopy_unix_plain);
+    migration_test_add("/migration/precopy/unix/xbzrle",
+                       test_precopy_unix_xbzrle);
     /*
      * Compression fails from time to time.
      * Put test here but don't enable it until everything is fixed.
      */
     if (getenv("QEMU_TEST_FLAKY_TESTS")) {
-        qtest_add_func("/migration/precopy/unix/compress/wait",
-                       test_precopy_unix_compress);
-        qtest_add_func("/migration/precopy/unix/compress/nowait",
-                       test_precopy_unix_compress_nowait);
+        migration_test_add("/migration/precopy/unix/compress/wait",
+                           test_precopy_unix_compress);
+        migration_test_add("/migration/precopy/unix/compress/nowait",
+                           test_precopy_unix_compress_nowait);
     }
 
-    qtest_add_func("/migration/precopy/file",
-                   test_precopy_file);
-    qtest_add_func("/migration/precopy/file/offset",
-                   test_precopy_file_offset);
-    qtest_add_func("/migration/precopy/file/offset/bad",
-                   test_precopy_file_offset_bad);
+    migration_test_add("/migration/precopy/file",
+                       test_precopy_file);
+    migration_test_add("/migration/precopy/file/offset",
+                       test_precopy_file_offset);
+    migration_test_add("/migration/precopy/file/offset/bad",
+                       test_precopy_file_offset_bad);
 
     /*
      * Our CI system has problems with shared memory.
      * Don't run this test until we find a workaround.
      */
     if (getenv("QEMU_TEST_FLAKY_TESTS")) {
-        qtest_add_func("/migration/mode/reboot", test_mode_reboot);
+        migration_test_add("/migration/mode/reboot", test_mode_reboot);
     }
 
 #ifdef CONFIG_GNUTLS
-    qtest_add_func("/migration/precopy/unix/tls/psk",
-                   test_precopy_unix_tls_psk);
+    migration_test_add("/migration/precopy/unix/tls/psk",
+                       test_precopy_unix_tls_psk);
 
     if (has_uffd) {
         /*
@@ -3402,110 +3405,114 @@ int main(int argc, char **argv)
          * channels are tested under precopy.  Here what we want to test is the
          * general postcopy path that has TLS channel enabled.
          */
-        qtest_add_func("/migration/postcopy/tls/psk", test_postcopy_tls_psk);
-        qtest_add_func("/migration/postcopy/recovery/tls/psk",
-                       test_postcopy_recovery_tls_psk);
-        qtest_add_func("/migration/postcopy/preempt/tls/psk",
-                       test_postcopy_preempt_tls_psk);
-        qtest_add_func("/migration/postcopy/preempt/recovery/tls/psk",
-                       test_postcopy_preempt_all);
+        migration_test_add("/migration/postcopy/tls/psk",
+                           test_postcopy_tls_psk);
+        migration_test_add("/migration/postcopy/recovery/tls/psk",
+                           test_postcopy_recovery_tls_psk);
+        migration_test_add("/migration/postcopy/preempt/tls/psk",
+                           test_postcopy_preempt_tls_psk);
+        migration_test_add("/migration/postcopy/preempt/recovery/tls/psk",
+                           test_postcopy_preempt_all);
     }
 #ifdef CONFIG_TASN1
-    qtest_add_func("/migration/precopy/unix/tls/x509/default-host",
-                   test_precopy_unix_tls_x509_default_host);
-    qtest_add_func("/migration/precopy/unix/tls/x509/override-host",
-                   test_precopy_unix_tls_x509_override_host);
+    migration_test_add("/migration/precopy/unix/tls/x509/default-host",
+                       test_precopy_unix_tls_x509_default_host);
+    migration_test_add("/migration/precopy/unix/tls/x509/override-host",
+                       test_precopy_unix_tls_x509_override_host);
 #endif /* CONFIG_TASN1 */
 #endif /* CONFIG_GNUTLS */
 
-    qtest_add_func("/migration/precopy/tcp/plain", test_precopy_tcp_plain);
+    migration_test_add("/migration/precopy/tcp/plain", test_precopy_tcp_plain);
 
-    qtest_add_func("/migration/precopy/tcp/plain/switchover-ack",
-                   test_precopy_tcp_switchover_ack);
+    migration_test_add("/migration/precopy/tcp/plain/switchover-ack",
+                       test_precopy_tcp_switchover_ack);
 
 #ifdef CONFIG_GNUTLS
-    qtest_add_func("/migration/precopy/tcp/tls/psk/match",
-                   test_precopy_tcp_tls_psk_match);
-    qtest_add_func("/migration/precopy/tcp/tls/psk/mismatch",
-                   test_precopy_tcp_tls_psk_mismatch);
+    migration_test_add("/migration/precopy/tcp/tls/psk/match",
+                       test_precopy_tcp_tls_psk_match);
+    migration_test_add("/migration/precopy/tcp/tls/psk/mismatch",
+                       test_precopy_tcp_tls_psk_mismatch);
 #ifdef CONFIG_TASN1
-    qtest_add_func("/migration/precopy/tcp/tls/x509/default-host",
-                   test_precopy_tcp_tls_x509_default_host);
-    qtest_add_func("/migration/precopy/tcp/tls/x509/override-host",
-                   test_precopy_tcp_tls_x509_override_host);
-    qtest_add_func("/migration/precopy/tcp/tls/x509/mismatch-host",
-                   test_precopy_tcp_tls_x509_mismatch_host);
-    qtest_add_func("/migration/precopy/tcp/tls/x509/friendly-client",
-                   test_precopy_tcp_tls_x509_friendly_client);
-    qtest_add_func("/migration/precopy/tcp/tls/x509/hostile-client",
-                   test_precopy_tcp_tls_x509_hostile_client);
-    qtest_add_func("/migration/precopy/tcp/tls/x509/allow-anon-client",
-                   test_precopy_tcp_tls_x509_allow_anon_client);
-    qtest_add_func("/migration/precopy/tcp/tls/x509/reject-anon-client",
-                   test_precopy_tcp_tls_x509_reject_anon_client);
+    migration_test_add("/migration/precopy/tcp/tls/x509/default-host",
+                       test_precopy_tcp_tls_x509_default_host);
+    migration_test_add("/migration/precopy/tcp/tls/x509/override-host",
+                       test_precopy_tcp_tls_x509_override_host);
+    migration_test_add("/migration/precopy/tcp/tls/x509/mismatch-host",
+                       test_precopy_tcp_tls_x509_mismatch_host);
+    migration_test_add("/migration/precopy/tcp/tls/x509/friendly-client",
+                       test_precopy_tcp_tls_x509_friendly_client);
+    migration_test_add("/migration/precopy/tcp/tls/x509/hostile-client",
+                       test_precopy_tcp_tls_x509_hostile_client);
+    migration_test_add("/migration/precopy/tcp/tls/x509/allow-anon-client",
+                       test_precopy_tcp_tls_x509_allow_anon_client);
+    migration_test_add("/migration/precopy/tcp/tls/x509/reject-anon-client",
+                       test_precopy_tcp_tls_x509_reject_anon_client);
 #endif /* CONFIG_TASN1 */
 #endif /* CONFIG_GNUTLS */
 
-    /* qtest_add_func("/migration/ignore_shared", test_ignore_shared); */
+    /* migration_test_add("/migration/ignore_shared", test_ignore_shared); */
 #ifndef _WIN32
-    qtest_add_func("/migration/fd_proto", test_migrate_fd_proto);
+    migration_test_add("/migration/fd_proto", test_migrate_fd_proto);
 #endif
-    qtest_add_func("/migration/validate_uuid", test_validate_uuid);
-    qtest_add_func("/migration/validate_uuid_error", test_validate_uuid_error);
-    qtest_add_func("/migration/validate_uuid_src_not_set",
-                   test_validate_uuid_src_not_set);
-    qtest_add_func("/migration/validate_uuid_dst_not_set",
-                   test_validate_uuid_dst_not_set);
+    migration_test_add("/migration/validate_uuid", test_validate_uuid);
+    migration_test_add("/migration/validate_uuid_error",
+                       test_validate_uuid_error);
+    migration_test_add("/migration/validate_uuid_src_not_set",
+                       test_validate_uuid_src_not_set);
+    migration_test_add("/migration/validate_uuid_dst_not_set",
+                       test_validate_uuid_dst_not_set);
     /*
      * See explanation why this test is slow on function definition
      */
     if (g_test_slow()) {
-        qtest_add_func("/migration/auto_converge", test_migrate_auto_converge);
+        migration_test_add("/migration/auto_converge",
+                           test_migrate_auto_converge);
         if (g_str_equal(arch, "x86_64") &&
             has_kvm && kvm_dirty_ring_supported()) {
-            qtest_add_func("/migration/dirty_limit", test_migrate_dirty_limit);
+            migration_test_add("/migration/dirty_limit",
+                               test_migrate_dirty_limit);
         }
     }
-    qtest_add_func("/migration/multifd/tcp/plain/none",
-                   test_multifd_tcp_none);
+    migration_test_add("/migration/multifd/tcp/plain/none",
+                       test_multifd_tcp_none);
     /*
      * This test is flaky and sometimes fails in CI and otherwise:
      * don't run unless user opts in via environment variable.
      */
     if (getenv("QEMU_TEST_FLAKY_TESTS")) {
-        qtest_add_func("/migration/multifd/tcp/plain/cancel",
-                       test_multifd_tcp_cancel);
+        migration_test_add("/migration/multifd/tcp/plain/cancel",
+                           test_multifd_tcp_cancel);
     }
-    qtest_add_func("/migration/multifd/tcp/plain/zlib",
-                   test_multifd_tcp_zlib);
+    migration_test_add("/migration/multifd/tcp/plain/zlib",
+                       test_multifd_tcp_zlib);
 #ifdef CONFIG_ZSTD
-    qtest_add_func("/migration/multifd/tcp/plain/zstd",
-                   test_multifd_tcp_zstd);
+    migration_test_add("/migration/multifd/tcp/plain/zstd",
+                       test_multifd_tcp_zstd);
 #endif
 #ifdef CONFIG_GNUTLS
-    qtest_add_func("/migration/multifd/tcp/tls/psk/match",
-                   test_multifd_tcp_tls_psk_match);
-    qtest_add_func("/migration/multifd/tcp/tls/psk/mismatch",
-                   test_multifd_tcp_tls_psk_mismatch);
+    migration_test_add("/migration/multifd/tcp/tls/psk/match",
+                       test_multifd_tcp_tls_psk_match);
+    migration_test_add("/migration/multifd/tcp/tls/psk/mismatch",
+                       test_multifd_tcp_tls_psk_mismatch);
 #ifdef CONFIG_TASN1
-    qtest_add_func("/migration/multifd/tcp/tls/x509/default-host",
-                   test_multifd_tcp_tls_x509_default_host);
-    qtest_add_func("/migration/multifd/tcp/tls/x509/override-host",
-                   test_multifd_tcp_tls_x509_override_host);
-    qtest_add_func("/migration/multifd/tcp/tls/x509/mismatch-host",
-                   test_multifd_tcp_tls_x509_mismatch_host);
-    qtest_add_func("/migration/multifd/tcp/tls/x509/allow-anon-client",
-                   test_multifd_tcp_tls_x509_allow_anon_client);
-    qtest_add_func("/migration/multifd/tcp/tls/x509/reject-anon-client",
-                   test_multifd_tcp_tls_x509_reject_anon_client);
+    migration_test_add("/migration/multifd/tcp/tls/x509/default-host",
+                       test_multifd_tcp_tls_x509_default_host);
+    migration_test_add("/migration/multifd/tcp/tls/x509/override-host",
+                       test_multifd_tcp_tls_x509_override_host);
+    migration_test_add("/migration/multifd/tcp/tls/x509/mismatch-host",
+                       test_multifd_tcp_tls_x509_mismatch_host);
+    migration_test_add("/migration/multifd/tcp/tls/x509/allow-anon-client",
+                       test_multifd_tcp_tls_x509_allow_anon_client);
+    migration_test_add("/migration/multifd/tcp/tls/x509/reject-anon-client",
+                       test_multifd_tcp_tls_x509_reject_anon_client);
 #endif /* CONFIG_TASN1 */
 #endif /* CONFIG_GNUTLS */
 
     if (g_str_equal(arch, "x86_64") && has_kvm && kvm_dirty_ring_supported()) {
-        qtest_add_func("/migration/dirty_ring",
-                       test_precopy_unix_dirty_ring);
-        qtest_add_func("/migration/vcpu_dirty_limit",
-                       test_vcpu_dirty_limit);
+        migration_test_add("/migration/dirty_ring",
+                           test_precopy_unix_dirty_ring);
+        migration_test_add("/migration/vcpu_dirty_limit",
+                           test_vcpu_dirty_limit);
     }
 
     ret = g_test_run();
-- 
2.35.3

Re: [PATCH v1 7/7] tests/qtest/migration: Use the new migration_test_add

[Peter Xu]

On Fri, Nov 24, 2023 at 01:14:32PM -0300, Fabiano Rosas wrote:
> Replace the tests registration with the new function that prints tests
> names.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

Re: [PATCH v1 0/7] migration cleanups and testing improvements

[Peter Xu]

On Fri, Nov 24, 2023 at 01:14:25PM -0300, Fabiano Rosas wrote:
> Hi,
> 
> These are some general cleanups and improvements to testing and
> debugging that I collected over the past month.
> 
> Fabiano Rosas (7):
>   migration/multifd: Remove MultiFDPages_t::packet_num
>   migration/multifd: Remove QEMUFile from where it is not needed
>   migration/multifd: Change multifd_pages_init argument
>   migration: Report error in incoming migration
>   tests/qtest/migration: Print migration incoming errors
>   tests/qtest/migration: Add a wrapper to print test names
>   tests/qtest/migration: Use the new migration_test_add

Fabiano,

It seems this doesn't apply anymore, since I queued and tested the other
ones first I left this one for next, my apologies.

Please repost when you have time.

-- 
Peter Xu