[PATCH 0/6] migration: Test the new "file:" migration

[PATCH 0/6] migration: Test the new "file:" migration

[Fabiano Rosas]

Based-on:
[PATCH V3 0/2] migration file URI
https://lore.kernel.org/r/1687466251-310524-1-git-send-email-steven.sistare@oracle.com

Here's the test for the file: migration.

I hit an issue with the setting of migration status. If we call
query-migrate too soon after migrate-incoming, the query returns an
empty response because we're not setting the MIGRATION_STATUS_SETUP in
the incoming path. We just send the event, but never actually change
state.

Aside from the fix, there's some tidying up to avoid duplicating too
much code in the tests.

Thanks

CI run: https://gitlab.com/farosas/qemu/-/pipelines/912226554

Fabiano Rosas (5):
  migration: Set migration status early in incoming side
  tests/qtest: migration: Expose migrate_set_capability
  tests/qtest: migration: Add migrate_incoming_qmp helper
  tests/qtest: migration: Use migrate_incoming_qmp where appropriate
  tests/qtest: migration: Add support for negative testing of
    qmp_migrate

Nikolay Borisov (1):
  tests/qtest: migration-test: Add tests for file-based migration

 migration/migration.c             |  17 ++++-
 tests/qtest/libqtest.c            |  33 ++++++++++
 tests/qtest/libqtest.h            |  28 ++++++++
 tests/qtest/meson.build           |   1 +
 tests/qtest/migration-helpers.c   |  61 +++++++++++++++++
 tests/qtest/migration-helpers.h   |  11 ++++
 tests/qtest/migration-test.c      | 106 +++++++++++++++++++++++-------
 tests/qtest/virtio-net-failover.c |  77 +++-------------------
 8 files changed, 241 insertions(+), 93 deletions(-)

-- 
2.35.3

[PATCH 1/6] migration: Set migration status early in incoming side

[Fabiano Rosas]

We are sending a migration event of MIGRATION_STATUS_SETUP at
qemu_start_incoming_migration but never actually setting the state.

This creates a window between qmp_migrate_incoming and
process_incoming_migration_co where the migration status is still
MIGRATION_STATUS_NONE. Calling query-migrate during this time will
return an empty response even though the incoming migration command
has already been issued.

Commit 7cf1fe6d68 ("migration: Add migration events on target side")
has added support to the 'events' capability to the incoming part of
migration, but chose to send the SETUP event without setting the
state. I'm assuming this was a mistake.

To avoid introducing a change in behavior, we need to keep sending the
SETUP event, even if the 'events' capability is not set.

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

diff --git a/migration/migration.c b/migration/migration.c
index 7c8292d4d4..562b78261d 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -424,13 +424,26 @@ void migrate_add_address(SocketAddress *address)
 static void qemu_start_incoming_migration(const char *uri, Error **errp)
 {
     const char *p = NULL;
+    MigrationIncomingState *mis = migration_incoming_get_current();
 
     /* URI is not suitable for migration? */
     if (!migration_channels_and_uri_compatible(uri, errp)) {
         return;
     }
 
-    qapi_event_send_migration(MIGRATION_STATUS_SETUP);
+    migrate_set_state(&mis->state, MIGRATION_STATUS_NONE,
+                      MIGRATION_STATUS_SETUP);
+    /*
+     * QMP clients should have set the 'events' migration capability
+     * if they want to receive this event, in which case the
+     * migrate_set_state() call above will have already sent the
+     * event. We still need to send the event for compatibility even
+     * if migration events are disabled.
+     */
+    if (!migrate_events()) {
+        qapi_event_send_migration(MIGRATION_STATUS_SETUP);
+    }
+
     if (strstart(uri, "tcp:", &p) ||
         strstart(uri, "unix:", NULL) ||
         strstart(uri, "vsock:", NULL)) {
@@ -524,7 +537,7 @@ process_incoming_migration_co(void *opaque)
 
     mis->largest_page_size = qemu_ram_pagesize_largest();
     postcopy_state_set(POSTCOPY_INCOMING_NONE);
-    migrate_set_state(&mis->state, MIGRATION_STATUS_NONE,
+    migrate_set_state(&mis->state, MIGRATION_STATUS_SETUP,
                       MIGRATION_STATUS_ACTIVE);
 
     mis->loadvm_co = qemu_coroutine_self();
-- 
2.35.3

[PATCH 2/6] tests/qtest: migration: Expose migrate_set_capability

[Fabiano Rosas]

The following patch will make use of this function from within
migrate-helpers.c, so move it there.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 tests/qtest/migration-helpers.c | 11 +++++++++++
 tests/qtest/migration-helpers.h |  3 +++
 tests/qtest/migration-test.c    | 11 -----------
 3 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
index be00c52d00..2df198c99e 100644
--- a/tests/qtest/migration-helpers.c
+++ b/tests/qtest/migration-helpers.c
@@ -70,6 +70,17 @@ void migrate_qmp(QTestState *who, const char *uri, const char *fmt, ...)
                              "{ 'execute': 'migrate', 'arguments': %p}", args);
 }
 
+void migrate_set_capability(QTestState *who, const char *capability,
+                            bool value)
+{
+    qtest_qmp_assert_success(who,
+                             "{ 'execute': 'migrate-set-capabilities',"
+                             "'arguments': { "
+                             "'capabilities': [ { "
+                             "'capability': %s, 'state': %i } ] } }",
+                             capability, value);
+}
+
 /*
  * Note: caller is responsible to free the returned object via
  * qobject_unref() after use
diff --git a/tests/qtest/migration-helpers.h b/tests/qtest/migration-helpers.h
index 009e250e90..484d7c960f 100644
--- a/tests/qtest/migration-helpers.h
+++ b/tests/qtest/migration-helpers.h
@@ -23,6 +23,9 @@ bool migrate_watch_for_resume(QTestState *who, const char *name,
 G_GNUC_PRINTF(3, 4)
 void migrate_qmp(QTestState *who, const char *uri, const char *fmt, ...);
 
+void migrate_set_capability(QTestState *who, const char *capability,
+                            bool value);
+
 QDict *migrate_query(QTestState *who);
 QDict *migrate_query_not_failed(QTestState *who);
 
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 800ad23b75..310627be39 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -472,17 +472,6 @@ static void migrate_cancel(QTestState *who)
     qtest_qmp_assert_success(who, "{ 'execute': 'migrate_cancel' }");
 }
 
-static void migrate_set_capability(QTestState *who, const char *capability,
-                                   bool value)
-{
-    qtest_qmp_assert_success(who,
-                             "{ 'execute': 'migrate-set-capabilities',"
-                             "'arguments': { "
-                             "'capabilities': [ { "
-                             "'capability': %s, 'state': %i } ] } }",
-                             capability, value);
-}
-
 static void migrate_postcopy_start(QTestState *from, QTestState *to)
 {
     qtest_qmp_assert_success(from, "{ 'execute': 'migrate-start-postcopy' }");
-- 
2.35.3

[PATCH 3/6] tests/qtest: migration: Add migrate_incoming_qmp helper

[Fabiano Rosas]

file-based migration requires the target to initiate its migration after
the source has finished writing out the data in the file. Currently
there's no easy way to initiate 'migrate-incoming', allow this by
introducing migrate_incoming_qmp helper, similarly to migrate_qmp.

Also make sure migration events are enabled and wait for the incoming
migration to start before returning. This avoid a race when querying
the migration status too soon after issuing the command.

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

diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
index 2df198c99e..bc54b29184 100644
--- a/tests/qtest/migration-helpers.c
+++ b/tests/qtest/migration-helpers.c
@@ -81,6 +81,34 @@ void migrate_set_capability(QTestState *who, const char *capability,
                              capability, value);
 }
 
+void migrate_incoming_qmp(QTestState *to, const char *uri, const char *fmt, ...)
+{
+    va_list ap;
+    QDict *args, *rsp, *data;
+
+    va_start(ap, fmt);
+    args = qdict_from_vjsonf_nofail(fmt, ap);
+    va_end(ap);
+
+    g_assert(!qdict_haskey(args, "uri"));
+    qdict_put_str(args, "uri", uri);
+
+    migrate_set_capability(to, "events", true);
+
+    rsp = qtest_qmp(to, "{ 'execute': 'migrate-incoming', 'arguments': %p}",
+                    args);
+    g_assert(qdict_haskey(rsp, "return"));
+
+    rsp = qtest_qmp_eventwait_ref(to, "MIGRATION");
+    g_assert(qdict_haskey(rsp, "data"));
+
+    data = qdict_get_qdict(rsp, "data");
+    g_assert(qdict_haskey(data, "status"));
+    g_assert_cmpstr(qdict_get_str(data, "status"), ==, "setup");
+
+    qobject_unref(rsp);
+}
+
 /*
  * Note: caller is responsible to free the returned object via
  * qobject_unref() after use
diff --git a/tests/qtest/migration-helpers.h b/tests/qtest/migration-helpers.h
index 484d7c960f..57d295a4fe 100644
--- a/tests/qtest/migration-helpers.h
+++ b/tests/qtest/migration-helpers.h
@@ -23,6 +23,10 @@ bool migrate_watch_for_resume(QTestState *who, const char *name,
 G_GNUC_PRINTF(3, 4)
 void migrate_qmp(QTestState *who, const char *uri, const char *fmt, ...);
 
+G_GNUC_PRINTF(3, 4)
+void migrate_incoming_qmp(QTestState *who, const char *uri,
+                          const char *fmt, ...);
+
 void migrate_set_capability(QTestState *who, const char *capability,
                             bool value);
 
-- 
2.35.3

[PATCH 4/6] tests/qtest: migration: Use migrate_incoming_qmp where appropriate

[Fabiano Rosas]

Use the new migrate_incoming_qmp helper in the places that currently
open-code calling migrate-incoming.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 tests/qtest/meson.build           |  1 +
 tests/qtest/migration-test.c      | 12 ++---
 tests/qtest/virtio-net-failover.c | 77 ++++---------------------------
 3 files changed, 14 insertions(+), 76 deletions(-)

diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
index 5fa6833ad7..c14bfeccd5 100644
--- a/tests/qtest/meson.build
+++ b/tests/qtest/meson.build
@@ -313,6 +313,7 @@ qtests = {
   'tpm-tis-i2c-test': [io, tpmemu_files, 'qtest_aspeed.c'],
   'tpm-tis-device-swtpm-test': [io, tpmemu_files, 'tpm-tis-util.c'],
   'tpm-tis-device-test': [io, tpmemu_files, 'tpm-tis-util.c'],
+  'virtio-net-failover': files('migration-helpers.c'),
   'vmgenid-test': files('boot-sector.c', 'acpi-utils.c'),
   'netdev-socket': files('netdev-socket.c', '../unit/socket-helpers.c'),
 }
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 310627be39..aafecb4f02 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -1819,8 +1819,7 @@ static void *test_migrate_fd_start_hook(QTestState *from,
     close(pair[0]);
 
     /* Start incoming migration from the 1st socket */
-    qtest_qmp_assert_success(to, "{ 'execute': 'migrate-incoming',"
-                             "  'arguments': { 'uri': 'fd:fd-mig' }}");
+    migrate_incoming_qmp(to, "fd:fd-mig", "{}");
 
     /* Send the 2nd socket to the target */
     qtest_qmp_fds_assert_success(from, &pair[1], 1,
@@ -2042,8 +2041,7 @@ test_migrate_precopy_tcp_multifd_start_common(QTestState *from,
     migrate_set_capability(to, "multifd", true);
 
     /* Start incoming migration from the 1st socket */
-    qtest_qmp_assert_success(to, "{ 'execute': 'migrate-incoming',"
-                             "  'arguments': { 'uri': 'tcp:127.0.0.1:0' }}");
+    migrate_incoming_qmp(to, "tcp:127.0.0.1:0", "{}");
 
     return NULL;
 }
@@ -2295,8 +2293,7 @@ static void test_multifd_tcp_cancel(void)
     migrate_set_capability(to, "multifd", true);
 
     /* Start incoming migration from the 1st socket */
-    qtest_qmp_assert_success(to, "{ 'execute': 'migrate-incoming',"
-                             "  'arguments': { 'uri': 'tcp:127.0.0.1:0' }}");
+    migrate_incoming_qmp(to, "tcp:127.0.0.1:0", "{}");
 
     /* Wait for the first serial output from the source */
     wait_for_serial("src_serial");
@@ -2326,8 +2323,7 @@ static void test_multifd_tcp_cancel(void)
     migrate_set_capability(to2, "multifd", true);
 
     /* Start incoming migration from the 1st socket */
-    qtest_qmp_assert_success(to2, "{ 'execute': 'migrate-incoming',"
-                             "  'arguments': { 'uri': 'tcp:127.0.0.1:0' }}");
+    migrate_incoming_qmp(to2, "tcp:127.0.0.1:0", "{}");
 
     g_free(uri);
     uri = migrate_get_socket_address(to2, "socket-address");
diff --git a/tests/qtest/virtio-net-failover.c b/tests/qtest/virtio-net-failover.c
index 4a809590bf..0d40bc1f2d 100644
--- a/tests/qtest/virtio-net-failover.c
+++ b/tests/qtest/virtio-net-failover.c
@@ -11,6 +11,7 @@
 #include "libqtest.h"
 #include "libqos/pci.h"
 #include "libqos/pci-pc.h"
+#include "migration-helpers.h"
 #include "qapi/qmp/qdict.h"
 #include "qapi/qmp/qlist.h"
 #include "qapi/qmp/qjson.h"
@@ -736,26 +737,10 @@ static void test_migrate_out(gconstpointer opaque)
     machine_stop(qts);
 }
 
-static QDict *get_migration_event(QTestState *qts)
-{
-    QDict *resp;
-    QDict *data;
-
-    resp = qtest_qmp_eventwait_ref(qts, "MIGRATION");
-    g_assert(qdict_haskey(resp, "data"));
-
-    data = qdict_get_qdict(resp, "data");
-    g_assert(qdict_haskey(data, "status"));
-    qobject_ref(data);
-    qobject_unref(resp);
-
-    return data;
-}
-
 static void test_migrate_in(gconstpointer opaque)
 {
     QTestState *qts;
-    QDict *resp, *args, *ret;
+    QDict *resp, *ret;
     g_autofree gchar *uri = g_strdup_printf("exec: cat %s", (gchar *)opaque);
 
     qts = machine_start(BASE_MACHINE
@@ -787,18 +772,7 @@ static void test_migrate_in(gconstpointer opaque)
     check_one_card(qts, true, "standby0", MAC_STANDBY0);
     check_one_card(qts, false, "primary0", MAC_PRIMARY0);
 
-    args = qdict_from_jsonf_nofail("{}");
-    g_assert_nonnull(args);
-    qdict_put_str(args, "uri", uri);
-
-    resp = qtest_qmp(qts, "{ 'execute': 'migrate-incoming', 'arguments': %p}",
-                     args);
-    g_assert(qdict_haskey(resp, "return"));
-    qobject_unref(resp);
-
-    resp = get_migration_event(qts);
-    g_assert_cmpstr(qdict_get_str(resp, "status"), ==, "setup");
-    qobject_unref(resp);
+    migrate_incoming_qmp(qts, uri, "{}");
 
     resp = get_failover_negociated_event(qts);
     g_assert_cmpstr(qdict_get_str(resp, "device-id"), ==, "standby0");
@@ -888,7 +862,7 @@ static void test_off_migrate_out(gconstpointer opaque)
 static void test_off_migrate_in(gconstpointer opaque)
 {
     QTestState *qts;
-    QDict *resp, *args, *ret;
+    QDict *ret;
     g_autofree gchar *uri = g_strdup_printf("exec: cat %s", (gchar *)opaque);
 
     qts = machine_start(BASE_MACHINE
@@ -920,18 +894,7 @@ static void test_off_migrate_in(gconstpointer opaque)
     check_one_card(qts, true, "standby0", MAC_STANDBY0);
     check_one_card(qts, true, "primary0", MAC_PRIMARY0);
 
-    args = qdict_from_jsonf_nofail("{}");
-    g_assert_nonnull(args);
-    qdict_put_str(args, "uri", uri);
-
-    resp = qtest_qmp(qts, "{ 'execute': 'migrate-incoming', 'arguments': %p}",
-                     args);
-    g_assert(qdict_haskey(resp, "return"));
-    qobject_unref(resp);
-
-    resp = get_migration_event(qts);
-    g_assert_cmpstr(qdict_get_str(resp, "status"), ==, "setup");
-    qobject_unref(resp);
+    migrate_incoming_qmp(qts, uri, "{}");
 
     check_one_card(qts, true, "standby0", MAC_STANDBY0);
     check_one_card(qts, true, "primary0", MAC_PRIMARY0);
@@ -1026,7 +989,7 @@ static void test_guest_off_migrate_out(gconstpointer opaque)
 static void test_guest_off_migrate_in(gconstpointer opaque)
 {
     QTestState *qts;
-    QDict *resp, *args, *ret;
+    QDict *ret;
     g_autofree gchar *uri = g_strdup_printf("exec: cat %s", (gchar *)opaque);
 
     qts = machine_start(BASE_MACHINE
@@ -1058,18 +1021,7 @@ static void test_guest_off_migrate_in(gconstpointer opaque)
     check_one_card(qts, true, "standby0", MAC_STANDBY0);
     check_one_card(qts, false, "primary0", MAC_PRIMARY0);
 
-    args = qdict_from_jsonf_nofail("{}");
-    g_assert_nonnull(args);
-    qdict_put_str(args, "uri", uri);
-
-    resp = qtest_qmp(qts, "{ 'execute': 'migrate-incoming', 'arguments': %p}",
-                     args);
-    g_assert(qdict_haskey(resp, "return"));
-    qobject_unref(resp);
-
-    resp = get_migration_event(qts);
-    g_assert_cmpstr(qdict_get_str(resp, "status"), ==, "setup");
-    qobject_unref(resp);
+    migrate_incoming_qmp(qts, uri, "{}");
 
     check_one_card(qts, true, "standby0", MAC_STANDBY0);
     check_one_card(qts, false, "primary0", MAC_PRIMARY0);
@@ -1728,7 +1680,7 @@ static void test_multi_out(gconstpointer opaque)
 static void test_multi_in(gconstpointer opaque)
 {
     QTestState *qts;
-    QDict *resp, *args, *ret;
+    QDict *resp, *ret;
     g_autofree gchar *uri = g_strdup_printf("exec: cat %s", (gchar *)opaque);
 
     qts = machine_start(BASE_MACHINE
@@ -1794,18 +1746,7 @@ static void test_multi_in(gconstpointer opaque)
     check_one_card(qts, true, "standby1", MAC_STANDBY1);
     check_one_card(qts, false, "primary1", MAC_PRIMARY1);
 
-    args = qdict_from_jsonf_nofail("{}");
-    g_assert_nonnull(args);
-    qdict_put_str(args, "uri", uri);
-
-    resp = qtest_qmp(qts, "{ 'execute': 'migrate-incoming', 'arguments': %p}",
-                     args);
-    g_assert(qdict_haskey(resp, "return"));
-    qobject_unref(resp);
-
-    resp = get_migration_event(qts);
-    g_assert_cmpstr(qdict_get_str(resp, "status"), ==, "setup");
-    qobject_unref(resp);
+    migrate_incoming_qmp(qts, uri, "{}");
 
     resp = get_failover_negociated_event(qts);
     g_assert_cmpstr(qdict_get_str(resp, "device-id"), ==, "standby0");
-- 
2.35.3

[PATCH 5/6] tests/qtest: migration: Add support for negative testing of qmp_migrate

[Fabiano Rosas]

There is currently no way to write a test for errors that happened in
qmp_migrate before the migration has started.

Add a version of qmp_migrate that ensures an error happens and tests
the error message. To make use of it a test needs to declare:

    MigrateCommon args = {
        .result = MIG_TEST_QMP_ERROR,
        .error_str = "error message"
    }

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 tests/qtest/libqtest.c          | 33 +++++++++++++++++++++++++++++++++
 tests/qtest/libqtest.h          | 28 ++++++++++++++++++++++++++++
 tests/qtest/migration-helpers.c | 22 ++++++++++++++++++++++
 tests/qtest/migration-helpers.h |  4 ++++
 tests/qtest/migration-test.c    | 17 +++++++++++++----
 5 files changed, 100 insertions(+), 4 deletions(-)

diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
index de03ef5f60..14ed7d6d83 100644
--- a/tests/qtest/libqtest.c
+++ b/tests/qtest/libqtest.c
@@ -1243,6 +1243,28 @@ void qtest_memset(QTestState *s, uint64_t addr, uint8_t pattern, size_t size)
     qtest_rsp(s);
 }
 
+QDict *qtest_vqmp_assert_failure_ref(QTestState *qts,
+                                     const char *fmt, va_list args)
+{
+    QDict *response;
+    QDict *ret;
+
+    response = qtest_vqmp(qts, fmt, args);
+
+    g_assert(response);
+    if (!qdict_haskey(response, "error")) {
+        g_autoptr(GString) s = qobject_to_json_pretty(QOBJECT(response), true);
+        g_test_message("%s", s->str);
+    }
+    g_assert(qdict_haskey(response, "error"));
+    g_assert(!qdict_haskey(response, "return"));
+    ret = qdict_get_qdict(response, "error");
+    qobject_ref(ret);
+    qobject_unref(response);
+
+    return ret;
+}
+
 QDict *qtest_vqmp_assert_success_ref(QTestState *qts,
                                      const char *fmt, va_list args)
 {
@@ -1305,6 +1327,17 @@ void qtest_vqmp_fds_assert_success(QTestState *qts, int *fds, size_t nfds,
 }
 #endif /* !_WIN32 */
 
+QDict *qtest_qmp_assert_failure_ref(QTestState *qts, const char *fmt, ...)
+{
+    QDict *response;
+    va_list ap;
+
+    va_start(ap, fmt);
+    response = qtest_vqmp_assert_failure_ref(qts, fmt, ap);
+    va_end(ap);
+    return response;
+}
+
 QDict *qtest_qmp_assert_success_ref(QTestState *qts, const char *fmt, ...)
 {
     QDict *response;
diff --git a/tests/qtest/libqtest.h b/tests/qtest/libqtest.h
index a12acf7fa9..1fed533d7d 100644
--- a/tests/qtest/libqtest.h
+++ b/tests/qtest/libqtest.h
@@ -799,6 +799,34 @@ void qtest_vqmp_fds_assert_success(QTestState *qts, int *fds, size_t nfds,
     G_GNUC_PRINTF(4, 0);
 #endif /* !_WIN32 */
 
+/**
+ * qtest_qmp_assert_failure_ref:
+ * @qts: QTestState instance to operate on
+ * @fmt: QMP message to send to qemu, formatted like
+ * qobject_from_jsonf_nofail().  See parse_interpolation() for what's
+ * supported after '%'.
+ *
+ * Sends a QMP message to QEMU, asserts that an 'error' key is present in
+ * the response, and returns the response.
+ */
+QDict *qtest_qmp_assert_failure_ref(QTestState *qts, const char *fmt, ...)
+    G_GNUC_PRINTF(2, 3);
+
+/**
+ * qtest_vqmp_assert_failure_ref:
+ * @qts: QTestState instance to operate on
+ * @fmt: QMP message to send to qemu, formatted like
+ * qobject_from_jsonf_nofail().  See parse_interpolation() for what's
+ * supported after '%'.
+ * @args: variable arguments for @fmt
+ *
+ * Sends a QMP message to QEMU, asserts that an 'error' key is present in
+ * the response, and returns the response.
+ */
+QDict *qtest_vqmp_assert_failure_ref(QTestState *qts,
+                                     const char *fmt, va_list args)
+    G_GNUC_PRINTF(2, 0);
+
 /**
  * qtest_qmp_assert_success_ref:
  * @qts: QTestState instance to operate on
diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
index bc54b29184..5e021ba078 100644
--- a/tests/qtest/migration-helpers.c
+++ b/tests/qtest/migration-helpers.c
@@ -49,6 +49,28 @@ bool migrate_watch_for_resume(QTestState *who, const char *name,
     return false;
 }
 
+void migrate_qmp_with_error(QTestState *who, const char *uri,
+                            const char *error_str, const char *fmt, ...)
+{
+    va_list ap;
+    QDict *args, *err;
+
+    va_start(ap, fmt);
+    args = qdict_from_vjsonf_nofail(fmt, ap);
+    va_end(ap);
+
+    g_assert(!qdict_haskey(args, "uri"));
+    qdict_put_str(args, "uri", uri);
+
+    err = qtest_qmp_assert_failure_ref(
+        who, "{ 'execute': 'migrate', 'arguments': %p}", args);
+
+    g_assert(qdict_haskey(err, "desc"));
+    g_assert_cmpstr(qdict_get_str(err, "desc"), ==, error_str);
+
+    qobject_unref(err);
+}
+
 /*
  * Send QMP command "migrate".
  * Arguments are built from @fmt... (formatted like
diff --git a/tests/qtest/migration-helpers.h b/tests/qtest/migration-helpers.h
index 57d295a4fe..991efd57a6 100644
--- a/tests/qtest/migration-helpers.h
+++ b/tests/qtest/migration-helpers.h
@@ -27,6 +27,10 @@ G_GNUC_PRINTF(3, 4)
 void migrate_incoming_qmp(QTestState *who, const char *uri,
                           const char *fmt, ...);
 
+G_GNUC_PRINTF(4, 5)
+void migrate_qmp_with_error(QTestState *who, const char *uri,
+                            const char *error_str, const char *fmt, ...);
+
 void migrate_set_capability(QTestState *who, const char *capability,
                             bool value);
 
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index aafecb4f02..acb778a8cd 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -564,6 +564,8 @@ typedef struct {
         MIG_TEST_FAIL,
         /* This test should fail, dest qemu should fail with abnormal status */
         MIG_TEST_FAIL_DEST_QUIT_ERR,
+        /* The QMP command for this migration should fail with an error */
+        MIG_TEST_QMP_ERROR,
     } result;
 
     /* Optional: set number of migration passes to wait for, if live==true */
@@ -582,6 +584,7 @@ typedef struct {
     /* Postcopy specific fields */
     void *postcopy_data;
     bool postcopy_preempt;
+    gchar *error_str;
 } MigrateCommon;
 
 static int test_migrate_start(QTestState **from, QTestState **to,
@@ -1379,6 +1382,7 @@ static void test_precopy_common(MigrateCommon *args)
 {
     QTestState *from, *to;
     void *data_hook = NULL;
+    g_autofree char *connect_uri = NULL;
 
     if (test_migrate_start(&from, &to, args->listen_uri, &args->start)) {
         return;
@@ -1419,13 +1423,17 @@ static void test_precopy_common(MigrateCommon *args)
     }
 
     if (!args->connect_uri) {
-        g_autofree char *local_connect_uri =
-            migrate_get_socket_address(to, "socket-address");
-        migrate_qmp(from, local_connect_uri, "{}");
+        connect_uri = migrate_get_socket_address(to, "socket-address");
     } else {
-        migrate_qmp(from, args->connect_uri, "{}");
+        connect_uri = g_strdup(args->connect_uri);
     }
 
+    if (args->result == MIG_TEST_QMP_ERROR) {
+        migrate_qmp_with_error(from, connect_uri, args->error_str, "{}");
+        goto finish;
+    }
+
+    migrate_qmp(from, connect_uri, "{}");
 
     if (args->result != MIG_TEST_SUCCEED) {
         bool allow_active = args->result == MIG_TEST_FAIL;
@@ -1474,6 +1482,7 @@ static void test_precopy_common(MigrateCommon *args)
         wait_for_serial("dest_serial");
     }
 
+finish:
     if (args->finish_hook) {
         args->finish_hook(from, to, data_hook);
     }
-- 
2.35.3

[PATCH 6/6] tests/qtest: migration-test: Add tests for file-based migration

[Fabiano Rosas]

From: Nikolay Borisov <nborisov@suse.com>

Add basic tests for file-based migration.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 tests/qtest/migration-test.c | 66 ++++++++++++++++++++++++++++++++++++
 1 file changed, 66 insertions(+)

diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index acb778a8cd..5a77257a53 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -763,6 +763,7 @@ static void test_migrate_end(QTestState *from, QTestState *to, bool test_dest)
     cleanup("migsocket");
     cleanup("src_serial");
     cleanup("dest_serial");
+    cleanup("migfile");
 }
 
 #ifdef CONFIG_GNUTLS
@@ -1460,11 +1461,28 @@ static void test_precopy_common(MigrateCommon *args)
              */
             wait_for_migration_complete(from);
 
+            /*
+             * For file based migration the target must begin its
+             * migration after the source has finished.
+             */
+            if (strstr(connect_uri, "file:")) {
+                migrate_incoming_qmp(to, connect_uri, "{}");
+            }
+
             if (!got_src_stop) {
                 qtest_qmp_eventwait(from, "STOP");
             }
         } else {
             wait_for_migration_complete(from);
+
+            /*
+             * For file based migration the target must begin its
+             * migration after the source has finished.
+             */
+            if (strstr(connect_uri, "file:")) {
+                migrate_incoming_qmp(to, connect_uri, "{}");
+            }
+
             /*
              * Must wait for dst to finish reading all incoming
              * data on the socket before issuing 'cont' otherwise
@@ -1682,6 +1700,46 @@ static void test_precopy_unix_compress_nowait(void)
     test_precopy_common(&args);
 }
 
+static void test_precopy_file(void)
+{
+    g_autofree char *uri = g_strdup_printf("file:%s/migfile", tmpfs);
+    MigrateCommon args = {
+        .connect_uri = uri,
+        .listen_uri = "defer",
+    };
+
+    test_precopy_common(&args);
+}
+
+static void test_precopy_file_offset(void)
+{
+    g_autofree char *uri = g_strdup_printf("file:%s/migfile,offset=0x1000",
+                                           tmpfs);
+    MigrateCommon args = {
+        .connect_uri = uri,
+        .listen_uri = "defer",
+    };
+
+    test_precopy_common(&args);
+}
+
+static void test_precopy_file_offset_bad(void)
+{
+    /* using a value not supported by qemu_strtosz() */
+    g_autofree char *uri = g_strdup_printf("file:%s/migfile,offset=0x20M",
+                                           tmpfs);
+    MigrateCommon args = {
+        .connect_uri = uri,
+        .listen_uri = "defer",
+        .error_str = g_strdup(
+            "file URI has bad offset 0x20M: Unknown error -22"),
+        .result = MIG_TEST_QMP_ERROR,
+    };
+
+    test_precopy_common(&args);
+    g_free(args.error_str);
+}
+
 static void test_precopy_tcp_plain(void)
 {
     MigrateCommon args = {
@@ -2704,6 +2762,14 @@ int main(int argc, char **argv)
         qtest_add_func("/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);
+
 #ifdef CONFIG_GNUTLS
     qtest_add_func("/migration/precopy/unix/tls/psk",
                    test_precopy_unix_tls_psk);
-- 
2.35.3

Re: [PATCH 6/6] tests/qtest: migration-test: Add tests for file-based migration

[Daniel P. Berrangé]

On Mon, Jun 26, 2023 at 03:22:10PM -0300, Fabiano Rosas wrote:
> From: Nikolay Borisov <nborisov@suse.com>
> 
> Add basic tests for file-based migration.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>  tests/qtest/migration-test.c | 66 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 66 insertions(+)
> 
> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
> index acb778a8cd..5a77257a53 100644
> --- a/tests/qtest/migration-test.c
> +++ b/tests/qtest/migration-test.c
> @@ -763,6 +763,7 @@ static void test_migrate_end(QTestState *from, QTestState *to, bool test_dest)
>      cleanup("migsocket");
>      cleanup("src_serial");
>      cleanup("dest_serial");
> +    cleanup("migfile");
>  }
>  
>  #ifdef CONFIG_GNUTLS
> @@ -1460,11 +1461,28 @@ static void test_precopy_common(MigrateCommon *args)
>               */
>              wait_for_migration_complete(from);
>  
> +            /*
> +             * For file based migration the target must begin its
> +             * migration after the source has finished.
> +             */
> +            if (strstr(connect_uri, "file:")) {
> +                migrate_incoming_qmp(to, connect_uri, "{}");
> +            }
> +
>              if (!got_src_stop) {
>                  qtest_qmp_eventwait(from, "STOP");
>              }
>          } else {
>              wait_for_migration_complete(from);
> +
> +            /*
> +             * For file based migration the target must begin its
> +             * migration after the source has finished.
> +             */
> +            if (strstr(connect_uri, "file:")) {
> +                migrate_incoming_qmp(to, connect_uri, "{}");
> +            }
> +
>              /*
>               * Must wait for dst to finish reading all incoming
>               * data on the socket before issuing 'cont' otherwise
> @@ -1682,6 +1700,46 @@ static void test_precopy_unix_compress_nowait(void)
>      test_precopy_common(&args);
>  }
>  
> +static void test_precopy_file(void)
> +{
> +    g_autofree char *uri = g_strdup_printf("file:%s/migfile", tmpfs);

There's no unlink(), so I presume you're relying on the entire 'tmpfs'
being recursively deleted ?

> +    MigrateCommon args = {
> +        .connect_uri = uri,
> +        .listen_uri = "defer",
> +    };
> +
> +    test_precopy_common(&args);
> +}
> +
> +static void test_precopy_file_offset(void)
> +{
> +    g_autofree char *uri = g_strdup_printf("file:%s/migfile,offset=0x1000",
> +                                           tmpfs);
> +    MigrateCommon args = {
> +        .connect_uri = uri,
> +        .listen_uri = "defer",
> +    };
> +
> +    test_precopy_common(&args);

There ought to be something here that values 0->0x1000 bytes are
all zeroes in the file.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 1/6] migration: Set migration status early in incoming side

[Juan Quintela]

Fabiano Rosas <farosas@suse.de> wrote:
> We are sending a migration event of MIGRATION_STATUS_SETUP at
> qemu_start_incoming_migration but never actually setting the state.
>
> This creates a window between qmp_migrate_incoming and
> process_incoming_migration_co where the migration status is still
> MIGRATION_STATUS_NONE. Calling query-migrate during this time will
> return an empty response even though the incoming migration command
> has already been issued.
>
> Commit 7cf1fe6d68 ("migration: Add migration events on target side")
> has added support to the 'events' capability to the incoming part of
> migration, but chose to send the SETUP event without setting the
> state. I'm assuming this was a mistake.
>
> To avoid introducing a change in behavior, we need to keep sending the
> SETUP event, even if the 'events' capability is not set.
>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>  migration/migration.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index 7c8292d4d4..562b78261d 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -424,13 +424,26 @@ void migrate_add_address(SocketAddress *address)
>  static void qemu_start_incoming_migration(const char *uri, Error **errp)
>  {
>      const char *p = NULL;
> +    MigrationIncomingState *mis = migration_incoming_get_current();
>  
>      /* URI is not suitable for migration? */
>      if (!migration_channels_and_uri_compatible(uri, errp)) {
>          return;
>      }
>  
> -    qapi_event_send_migration(MIGRATION_STATUS_SETUP);
> +    migrate_set_state(&mis->state, MIGRATION_STATUS_NONE,
> +                      MIGRATION_STATUS_SETUP);
> +    /*
> +     * QMP clients should have set the 'events' migration capability
> +     * if they want to receive this event, in which case the
> +     * migrate_set_state() call above will have already sent the
> +     * event. We still need to send the event for compatibility even
> +     * if migration events are disabled.
> +     */
> +    if (!migrate_events()) {
> +        qapi_event_send_migration(MIGRATION_STATUS_SETUP);
> +    }

Can we add and test for a property here, so we can drop this at some
point in the future?

> +
>      if (strstart(uri, "tcp:", &p) ||
>          strstart(uri, "unix:", NULL) ||
>          strstart(uri, "vsock:", NULL)) {
> @@ -524,7 +537,7 @@ process_incoming_migration_co(void *opaque)
>  
>      mis->largest_page_size = qemu_ram_pagesize_largest();
>      postcopy_state_set(POSTCOPY_INCOMING_NONE);
> -    migrate_set_state(&mis->state, MIGRATION_STATUS_NONE,
> +    migrate_set_state(&mis->state, MIGRATION_STATUS_SETUP,
>                        MIGRATION_STATUS_ACTIVE);
>  
>      mis->loadvm_co = qemu_coroutine_self();

Re: [PATCH 2/6] tests/qtest: migration: Expose migrate_set_capability

[Juan Quintela]

Fabiano Rosas <farosas@suse.de> wrote:
> The following patch will make use of this function from within
> migrate-helpers.c, so move it there.
>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>

Reviewed-by: Juan Quintela <quintela@redhat.com>

Re: [PATCH 4/6] tests/qtest: migration: Use migrate_incoming_qmp where appropriate

[Juan Quintela]

Fabiano Rosas <farosas@suse.de> wrote:
> Use the new migrate_incoming_qmp helper in the places that currently
> open-code calling migrate-incoming.
>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>

Reviewed-by: Juan Quintela <quintela@redhat.com>

Re: [PATCH 1/6] migration: Set migration status early in incoming side

[Fabiano Rosas]

Juan Quintela <quintela@redhat.com> writes:

> Fabiano Rosas <farosas@suse.de> wrote:
>> We are sending a migration event of MIGRATION_STATUS_SETUP at
>> qemu_start_incoming_migration but never actually setting the state.
>>
>> This creates a window between qmp_migrate_incoming and
>> process_incoming_migration_co where the migration status is still
>> MIGRATION_STATUS_NONE. Calling query-migrate during this time will
>> return an empty response even though the incoming migration command
>> has already been issued.
>>
>> Commit 7cf1fe6d68 ("migration: Add migration events on target side")
>> has added support to the 'events' capability to the incoming part of
>> migration, but chose to send the SETUP event without setting the
>> state. I'm assuming this was a mistake.
>>
>> To avoid introducing a change in behavior, we need to keep sending the
>> SETUP event, even if the 'events' capability is not set.
>>
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>>  migration/migration.c | 17 +++++++++++++++--
>>  1 file changed, 15 insertions(+), 2 deletions(-)
>>
>> diff --git a/migration/migration.c b/migration/migration.c
>> index 7c8292d4d4..562b78261d 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -424,13 +424,26 @@ void migrate_add_address(SocketAddress *address)
>>  static void qemu_start_incoming_migration(const char *uri, Error **errp)
>>  {
>>      const char *p = NULL;
>> +    MigrationIncomingState *mis = migration_incoming_get_current();
>>  
>>      /* URI is not suitable for migration? */
>>      if (!migration_channels_and_uri_compatible(uri, errp)) {
>>          return;
>>      }
>>  
>> -    qapi_event_send_migration(MIGRATION_STATUS_SETUP);
>> +    migrate_set_state(&mis->state, MIGRATION_STATUS_NONE,
>> +                      MIGRATION_STATUS_SETUP);
>> +    /*
>> +     * QMP clients should have set the 'events' migration capability
>> +     * if they want to receive this event, in which case the
>> +     * migrate_set_state() call above will have already sent the
>> +     * event. We still need to send the event for compatibility even
>> +     * if migration events are disabled.
>> +     */
>> +    if (!migrate_events()) {
>> +        qapi_event_send_migration(MIGRATION_STATUS_SETUP);
>> +    }
>
> Can we add and test for a property here, so we can drop this at some
> point in the future?
>

Yes, I'll add for v2.

Thanks

Re: [PATCH 6/6] tests/qtest: migration-test: Add tests for file-based migration

[Fabiano Rosas]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Mon, Jun 26, 2023 at 03:22:10PM -0300, Fabiano Rosas wrote:
>> From: Nikolay Borisov <nborisov@suse.com>
>> 
>> Add basic tests for file-based migration.
>> 
>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>>  tests/qtest/migration-test.c | 66 ++++++++++++++++++++++++++++++++++++
>>  1 file changed, 66 insertions(+)
>> 
>> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
>> index acb778a8cd..5a77257a53 100644
>> --- a/tests/qtest/migration-test.c
>> +++ b/tests/qtest/migration-test.c
>> @@ -763,6 +763,7 @@ static void test_migrate_end(QTestState *from, QTestState *to, bool test_dest)
>>      cleanup("migsocket");
>>      cleanup("src_serial");
>>      cleanup("dest_serial");
>> +    cleanup("migfile");
>>  }
>>  
>>  #ifdef CONFIG_GNUTLS
>> @@ -1460,11 +1461,28 @@ static void test_precopy_common(MigrateCommon *args)
>>               */
>>              wait_for_migration_complete(from);
>>  
>> +            /*
>> +             * For file based migration the target must begin its
>> +             * migration after the source has finished.
>> +             */
>> +            if (strstr(connect_uri, "file:")) {
>> +                migrate_incoming_qmp(to, connect_uri, "{}");
>> +            }
>> +
>>              if (!got_src_stop) {
>>                  qtest_qmp_eventwait(from, "STOP");
>>              }
>>          } else {
>>              wait_for_migration_complete(from);
>> +
>> +            /*
>> +             * For file based migration the target must begin its
>> +             * migration after the source has finished.
>> +             */
>> +            if (strstr(connect_uri, "file:")) {
>> +                migrate_incoming_qmp(to, connect_uri, "{}");
>> +            }
>> +
>>              /*
>>               * Must wait for dst to finish reading all incoming
>>               * data on the socket before issuing 'cont' otherwise
>> @@ -1682,6 +1700,46 @@ static void test_precopy_unix_compress_nowait(void)
>>      test_precopy_common(&args);
>>  }
>>  
>> +static void test_precopy_file(void)
>> +{
>> +    g_autofree char *uri = g_strdup_printf("file:%s/migfile", tmpfs);
>
> There's no unlink(), so I presume you're relying on the entire 'tmpfs'
> being recursively deleted ?
>

The first hunk adds a call to cleanup(), which has the unlink().

>> +    MigrateCommon args = {
>> +        .connect_uri = uri,
>> +        .listen_uri = "defer",
>> +    };
>> +
>> +    test_precopy_common(&args);
>> +}
>> +
>> +static void test_precopy_file_offset(void)
>> +{
>> +    g_autofree char *uri = g_strdup_printf("file:%s/migfile,offset=0x1000",
>> +                                           tmpfs);
>> +    MigrateCommon args = {
>> +        .connect_uri = uri,
>> +        .listen_uri = "defer",
>> +    };
>> +
>> +    test_precopy_common(&args);
>
> There ought to be something here that values 0->0x1000 bytes are
> all zeroes in the file.
>

Good idea, thanks.

[PATCH 6/6] tests/qtest: migration-test: Add tests for file-based migration

[Fabiano Rosas]

Add basic tests for file-based migration.

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

diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index acb778a8cd..b3019f54de 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -52,6 +52,10 @@ static bool got_dst_resume;
  */
 #define DIRTYLIMIT_TOLERANCE_RANGE  25  /* MB/s */
 
+#define QEMU_VM_FILE_MAGIC 0x5145564d
+#define FILE_TEST_FILENAME "migfile"
+#define FILE_TEST_OFFSET 0x1000
+
 #if defined(__linux__)
 #include <sys/syscall.h>
 #include <sys/vfs.h>
@@ -763,6 +767,7 @@ static void test_migrate_end(QTestState *from, QTestState *to, bool test_dest)
     cleanup("migsocket");
     cleanup("src_serial");
     cleanup("dest_serial");
+    cleanup(FILE_TEST_FILENAME);
 }
 
 #ifdef CONFIG_GNUTLS
@@ -1460,11 +1465,28 @@ static void test_precopy_common(MigrateCommon *args)
              */
             wait_for_migration_complete(from);
 
+            /*
+             * For file based migration the target must begin its
+             * migration after the source has finished.
+             */
+            if (strstr(connect_uri, "file:")) {
+                migrate_incoming_qmp(to, connect_uri, "{}");
+            }
+
             if (!got_src_stop) {
                 qtest_qmp_eventwait(from, "STOP");
             }
         } else {
             wait_for_migration_complete(from);
+
+            /*
+             * For file based migration the target must begin its
+             * migration after the source has finished.
+             */
+            if (strstr(connect_uri, "file:")) {
+                migrate_incoming_qmp(to, connect_uri, "{}");
+            }
+
             /*
              * Must wait for dst to finish reading all incoming
              * data on the socket before issuing 'cont' otherwise
@@ -1682,6 +1704,78 @@ static void test_precopy_unix_compress_nowait(void)
     test_precopy_common(&args);
 }
 
+static void test_precopy_file(void)
+{
+    g_autofree char *uri = g_strdup_printf("file:%s/%s", tmpfs,
+                                           FILE_TEST_FILENAME);
+    MigrateCommon args = {
+        .connect_uri = uri,
+        .listen_uri = "defer",
+    };
+
+    test_precopy_common(&args);
+}
+
+#if defined(__linux__)
+static void file_offset_finish_hook(QTestState *from, QTestState *to, void *opaque)
+{
+    g_autofree char *path = g_strdup_printf("%s/%s", tmpfs, FILE_TEST_FILENAME);
+    size_t size = FILE_TEST_OFFSET + sizeof(QEMU_VM_FILE_MAGIC);
+    uintptr_t *addr, *p;
+    int fd;
+
+    fd = open(path, O_RDONLY);
+    g_assert(fd != -1);
+    addr = mmap(NULL, size, PROT_READ, MAP_PRIVATE, fd, 0);
+    g_assert(addr != MAP_FAILED);
+
+    /*
+     * Ensure the skipped offset contains zeros and the migration
+     * stream starts at the right place.
+     */
+    p = addr;
+    while (p < addr + FILE_TEST_OFFSET / sizeof(uintptr_t)) {
+        g_assert(*p == 0);
+        p++;
+    }
+    g_assert_cmpint(cpu_to_be32(*p), ==, QEMU_VM_FILE_MAGIC);
+
+    munmap(addr, size);
+    close(fd);
+}
+
+static void test_precopy_file_offset(void)
+{
+    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",
+        .finish_hook = file_offset_finish_hook,
+    };
+
+    test_precopy_common(&args);
+}
+#endif
+
+static void test_precopy_file_offset_bad(void)
+{
+    /* using a value not supported by qemu_strtosz() */
+    g_autofree char *uri = g_strdup_printf("file:%s/migfile,offset=0x20M",
+                                           tmpfs);
+    MigrateCommon args = {
+        .connect_uri = uri,
+        .listen_uri = "defer",
+        .error_str = g_strdup(
+            "file URI has bad offset 0x20M: Unknown error -22"),
+        .result = MIG_TEST_QMP_ERROR,
+    };
+
+    test_precopy_common(&args);
+    g_free(args.error_str);
+}
+
 static void test_precopy_tcp_plain(void)
 {
     MigrateCommon args = {
@@ -2704,6 +2798,16 @@ int main(int argc, char **argv)
         qtest_add_func("/migration/precopy/unix/compress/nowait",
                        test_precopy_unix_compress_nowait);
     }
+
+    qtest_add_func("/migration/precopy/file",
+                   test_precopy_file);
+#if defined(__linux__)
+    qtest_add_func("/migration/precopy/file/offset",
+                   test_precopy_file_offset);
+#endif
+    qtest_add_func("/migration/precopy/file/offset/bad",
+                   test_precopy_file_offset_bad);
+
 #ifdef CONFIG_GNUTLS
     qtest_add_func("/migration/precopy/unix/tls/psk",
                    test_precopy_unix_tls_psk);
-- 
2.35.3

Re: [PATCH 6/6] tests/qtest: migration-test: Add tests for file-based migration

[Peter Xu]

On Wed, Jun 28, 2023 at 01:55:42PM -0300, Fabiano Rosas wrote:
> Add basic tests for file-based migration.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>  tests/qtest/migration-test.c | 104 +++++++++++++++++++++++++++++++++++
>  1 file changed, 104 insertions(+)
> 
> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
> index acb778a8cd..b3019f54de 100644
> --- a/tests/qtest/migration-test.c
> +++ b/tests/qtest/migration-test.c
> @@ -52,6 +52,10 @@ static bool got_dst_resume;
>   */
>  #define DIRTYLIMIT_TOLERANCE_RANGE  25  /* MB/s */
>  
> +#define QEMU_VM_FILE_MAGIC 0x5145564d
> +#define FILE_TEST_FILENAME "migfile"
> +#define FILE_TEST_OFFSET 0x1000
> +
>  #if defined(__linux__)
>  #include <sys/syscall.h>
>  #include <sys/vfs.h>
> @@ -763,6 +767,7 @@ static void test_migrate_end(QTestState *from, QTestState *to, bool test_dest)
>      cleanup("migsocket");
>      cleanup("src_serial");
>      cleanup("dest_serial");
> +    cleanup(FILE_TEST_FILENAME);
>  }
>  
>  #ifdef CONFIG_GNUTLS
> @@ -1460,11 +1465,28 @@ static void test_precopy_common(MigrateCommon *args)
>               */
>              wait_for_migration_complete(from);
>  
> +            /*
> +             * For file based migration the target must begin its
> +             * migration after the source has finished.
> +             */
> +            if (strstr(connect_uri, "file:")) {
> +                migrate_incoming_qmp(to, connect_uri, "{}");
> +            }
> +
>              if (!got_src_stop) {
>                  qtest_qmp_eventwait(from, "STOP");
>              }
>          } else {
>              wait_for_migration_complete(from);
> +
> +            /*
> +             * For file based migration the target must begin its
> +             * migration after the source has finished.
> +             */
> +            if (strstr(connect_uri, "file:")) {
> +                migrate_incoming_qmp(to, connect_uri, "{}");
> +            }
> +
>              /*
>               * Must wait for dst to finish reading all incoming
>               * data on the socket before issuing 'cont' otherwise
> @@ -1682,6 +1704,78 @@ static void test_precopy_unix_compress_nowait(void)
>      test_precopy_common(&args);
>  }
>  
> +static void test_precopy_file(void)
> +{
> +    g_autofree char *uri = g_strdup_printf("file:%s/%s", tmpfs,
> +                                           FILE_TEST_FILENAME);
> +    MigrateCommon args = {
> +        .connect_uri = uri,
> +        .listen_uri = "defer",
> +    };
> +
> +    test_precopy_common(&args);
> +}
> +
> +#if defined(__linux__)
> +static void file_offset_finish_hook(QTestState *from, QTestState *to, void *opaque)
> +{
> +    g_autofree char *path = g_strdup_printf("%s/%s", tmpfs, FILE_TEST_FILENAME);
> +    size_t size = FILE_TEST_OFFSET + sizeof(QEMU_VM_FILE_MAGIC);
> +    uintptr_t *addr, *p;
> +    int fd;
> +
> +    fd = open(path, O_RDONLY);
> +    g_assert(fd != -1);
> +    addr = mmap(NULL, size, PROT_READ, MAP_PRIVATE, fd, 0);

Not something that matters a lot, but RO mapping a file with private is a
bit weird.  Maybe just use MAP_SHARED?

> +    g_assert(addr != MAP_FAILED);
> +
> +    /*
> +     * Ensure the skipped offset contains zeros and the migration
> +     * stream starts at the right place.
> +     */
> +    p = addr;
> +    while (p < addr + FILE_TEST_OFFSET / sizeof(uintptr_t)) {
> +        g_assert(*p == 0);
> +        p++;
> +    }
> +    g_assert_cmpint(cpu_to_be32(*p), ==, QEMU_VM_FILE_MAGIC);
> +
> +    munmap(addr, size);
> +    close(fd);
> +}
> +
> +static void test_precopy_file_offset(void)
> +{
> +    g_autofree char *uri = g_strdup_printf("file:%s/%s,offset=%d", tmpfs,
> +                                           FILE_TEST_FILENAME,
> +                                           FILE_TEST_OFFSET);

Is it intended to also only run this test with linux?  IIUC it should also
work for others. Maybe only file_offset_finish_hook() is optional?  Or am i
wrong?

> +    MigrateCommon args = {
> +        .connect_uri = uri,
> +        .listen_uri = "defer",
> +        .finish_hook = file_offset_finish_hook,
> +    };
> +
> +    test_precopy_common(&args);
> +}
> +#endif
> +
> +static void test_precopy_file_offset_bad(void)
> +{
> +    /* using a value not supported by qemu_strtosz() */
> +    g_autofree char *uri = g_strdup_printf("file:%s/migfile,offset=0x20M",
> +                                           tmpfs);
> +    MigrateCommon args = {
> +        .connect_uri = uri,
> +        .listen_uri = "defer",
> +        .error_str = g_strdup(
> +            "file URI has bad offset 0x20M: Unknown error -22"),

"Unknown error" may imply that in Steve's patch the errno is inverted..

Shall we not rely on the string in the test?  It might be too strict, I
worry, because error strings should be defined for human readers, and we
may not want some e.g. grammar / trivial change to break a test.

> +        .result = MIG_TEST_QMP_ERROR,
> +    };
> +
> +    test_precopy_common(&args);
> +    g_free(args.error_str);
> +}
> +
>  static void test_precopy_tcp_plain(void)
>  {
>      MigrateCommon args = {
> @@ -2704,6 +2798,16 @@ int main(int argc, char **argv)
>          qtest_add_func("/migration/precopy/unix/compress/nowait",
>                         test_precopy_unix_compress_nowait);
>      }
> +
> +    qtest_add_func("/migration/precopy/file",
> +                   test_precopy_file);
> +#if defined(__linux__)
> +    qtest_add_func("/migration/precopy/file/offset",
> +                   test_precopy_file_offset);
> +#endif
> +    qtest_add_func("/migration/precopy/file/offset/bad",
> +                   test_precopy_file_offset_bad);
> +
>  #ifdef CONFIG_GNUTLS
>      qtest_add_func("/migration/precopy/unix/tls/psk",
>                     test_precopy_unix_tls_psk);
> -- 
> 2.35.3
> 

-- 
Peter Xu

Re: [PATCH 6/6] tests/qtest: migration-test: Add tests for file-based migration

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> On Wed, Jun 28, 2023 at 01:55:42PM -0300, Fabiano Rosas wrote:
>> Add basic tests for file-based migration.
>> 
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>>  tests/qtest/migration-test.c | 104 +++++++++++++++++++++++++++++++++++
>>  1 file changed, 104 insertions(+)
>> 
>> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
>> index acb778a8cd..b3019f54de 100644
>> --- a/tests/qtest/migration-test.c
>> +++ b/tests/qtest/migration-test.c
>> @@ -52,6 +52,10 @@ static bool got_dst_resume;
>>   */
>>  #define DIRTYLIMIT_TOLERANCE_RANGE  25  /* MB/s */
>>  
>> +#define QEMU_VM_FILE_MAGIC 0x5145564d
>> +#define FILE_TEST_FILENAME "migfile"
>> +#define FILE_TEST_OFFSET 0x1000
>> +
>>  #if defined(__linux__)
>>  #include <sys/syscall.h>
>>  #include <sys/vfs.h>
>> @@ -763,6 +767,7 @@ static void test_migrate_end(QTestState *from, QTestState *to, bool test_dest)
>>      cleanup("migsocket");
>>      cleanup("src_serial");
>>      cleanup("dest_serial");
>> +    cleanup(FILE_TEST_FILENAME);
>>  }
>>  
>>  #ifdef CONFIG_GNUTLS
>> @@ -1460,11 +1465,28 @@ static void test_precopy_common(MigrateCommon *args)
>>               */
>>              wait_for_migration_complete(from);
>>  
>> +            /*
>> +             * For file based migration the target must begin its
>> +             * migration after the source has finished.
>> +             */
>> +            if (strstr(connect_uri, "file:")) {
>> +                migrate_incoming_qmp(to, connect_uri, "{}");
>> +            }
>> +
>>              if (!got_src_stop) {
>>                  qtest_qmp_eventwait(from, "STOP");
>>              }
>>          } else {
>>              wait_for_migration_complete(from);
>> +
>> +            /*
>> +             * For file based migration the target must begin its
>> +             * migration after the source has finished.
>> +             */
>> +            if (strstr(connect_uri, "file:")) {
>> +                migrate_incoming_qmp(to, connect_uri, "{}");
>> +            }
>> +
>>              /*
>>               * Must wait for dst to finish reading all incoming
>>               * data on the socket before issuing 'cont' otherwise
>> @@ -1682,6 +1704,78 @@ static void test_precopy_unix_compress_nowait(void)
>>      test_precopy_common(&args);
>>  }
>>  
>> +static void test_precopy_file(void)
>> +{
>> +    g_autofree char *uri = g_strdup_printf("file:%s/%s", tmpfs,
>> +                                           FILE_TEST_FILENAME);
>> +    MigrateCommon args = {
>> +        .connect_uri = uri,
>> +        .listen_uri = "defer",
>> +    };
>> +
>> +    test_precopy_common(&args);
>> +}
>> +
>> +#if defined(__linux__)
>> +static void file_offset_finish_hook(QTestState *from, QTestState *to, void *opaque)
>> +{
>> +    g_autofree char *path = g_strdup_printf("%s/%s", tmpfs, FILE_TEST_FILENAME);
>> +    size_t size = FILE_TEST_OFFSET + sizeof(QEMU_VM_FILE_MAGIC);
>> +    uintptr_t *addr, *p;
>> +    int fd;
>> +
>> +    fd = open(path, O_RDONLY);
>> +    g_assert(fd != -1);
>> +    addr = mmap(NULL, size, PROT_READ, MAP_PRIVATE, fd, 0);
>
> Not something that matters a lot, but RO mapping a file with private is a
> bit weird.  Maybe just use MAP_SHARED?
>

Yep

>> +    g_assert(addr != MAP_FAILED);
>> +
>> +    /*
>> +     * Ensure the skipped offset contains zeros and the migration
>> +     * stream starts at the right place.
>> +     */
>> +    p = addr;
>> +    while (p < addr + FILE_TEST_OFFSET / sizeof(uintptr_t)) {
>> +        g_assert(*p == 0);
>> +        p++;
>> +    }
>> +    g_assert_cmpint(cpu_to_be32(*p), ==, QEMU_VM_FILE_MAGIC);
>> +
>> +    munmap(addr, size);
>> +    close(fd);
>> +}
>> +
>> +static void test_precopy_file_offset(void)
>> +{
>> +    g_autofree char *uri = g_strdup_printf("file:%s/%s,offset=%d", tmpfs,
>> +                                           FILE_TEST_FILENAME,
>> +                                           FILE_TEST_OFFSET);
>
> Is it intended to also only run this test with linux?  IIUC it should also
> work for others. Maybe only file_offset_finish_hook() is optional?  Or am i
> wrong?
>

Yes, only the hook is linux-specific. I'll change it.

>> +    MigrateCommon args = {
>> +        .connect_uri = uri,
>> +        .listen_uri = "defer",
>> +        .finish_hook = file_offset_finish_hook,
>> +    };
>> +
>> +    test_precopy_common(&args);
>> +}
>> +#endif
>> +
>> +static void test_precopy_file_offset_bad(void)
>> +{
>> +    /* using a value not supported by qemu_strtosz() */
>> +    g_autofree char *uri = g_strdup_printf("file:%s/migfile,offset=0x20M",
>> +                                           tmpfs);
>> +    MigrateCommon args = {
>> +        .connect_uri = uri,
>> +        .listen_uri = "defer",
>> +        .error_str = g_strdup(
>> +            "file URI has bad offset 0x20M: Unknown error -22"),
>
> "Unknown error" may imply that in Steve's patch the errno is inverted..
>
> Shall we not rely on the string in the test?  It might be too strict, I
> worry, because error strings should be defined for human readers, and we
> may not want some e.g. grammar / trivial change to break a test.
>

Well, you just caught an issue with the errno by looking at the string,
so maybe testing it is a good thing?

I'd expect anyone changing the string to run the test and catch the
mismatch before sending a patch anyway.

I don't have a strong opinion about it, though. I can remove the
error_str.

Re: [PATCH 6/6] tests/qtest: migration-test: Add tests for file-based migration

[Steven Sistare]

On 6/30/2023 11:05 AM, Fabiano Rosas wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
>> On Wed, Jun 28, 2023 at 01:55:42PM -0300, Fabiano Rosas wrote:
>>> Add basic tests for file-based migration.
>>>
>>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>>> ---
>>>  tests/qtest/migration-test.c | 104 +++++++++++++++++++++++++++++++++++
>>>  1 file changed, 104 insertions(+)
>>>
>>> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
>>> index acb778a8cd..b3019f54de 100644
>>> --- a/tests/qtest/migration-test.c
>>> +++ b/tests/qtest/migration-test.c
>>> @@ -52,6 +52,10 @@ static bool got_dst_resume;
>>>   */
>>>  #define DIRTYLIMIT_TOLERANCE_RANGE  25  /* MB/s */
>>>  
>>> +#define QEMU_VM_FILE_MAGIC 0x5145564d
>>> +#define FILE_TEST_FILENAME "migfile"
>>> +#define FILE_TEST_OFFSET 0x1000
>>> +
>>>  #if defined(__linux__)
>>>  #include <sys/syscall.h>
>>>  #include <sys/vfs.h>
>>> @@ -763,6 +767,7 @@ static void test_migrate_end(QTestState *from, QTestState *to, bool test_dest)
>>>      cleanup("migsocket");
>>>      cleanup("src_serial");
>>>      cleanup("dest_serial");
>>> +    cleanup(FILE_TEST_FILENAME);
>>>  }
>>>  
>>>  #ifdef CONFIG_GNUTLS
>>> @@ -1460,11 +1465,28 @@ static void test_precopy_common(MigrateCommon *args)
>>>               */
>>>              wait_for_migration_complete(from);
>>>  
>>> +            /*
>>> +             * For file based migration the target must begin its
>>> +             * migration after the source has finished.
>>> +             */
>>> +            if (strstr(connect_uri, "file:")) {
>>> +                migrate_incoming_qmp(to, connect_uri, "{}");
>>> +            }
>>> +
>>>              if (!got_src_stop) {
>>>                  qtest_qmp_eventwait(from, "STOP");
>>>              }
>>>          } else {
>>>              wait_for_migration_complete(from);
>>> +
>>> +            /*
>>> +             * For file based migration the target must begin its
>>> +             * migration after the source has finished.
>>> +             */
>>> +            if (strstr(connect_uri, "file:")) {
>>> +                migrate_incoming_qmp(to, connect_uri, "{}");
>>> +            }
>>> +
>>>              /*
>>>               * Must wait for dst to finish reading all incoming
>>>               * data on the socket before issuing 'cont' otherwise
>>> @@ -1682,6 +1704,78 @@ static void test_precopy_unix_compress_nowait(void)
>>>      test_precopy_common(&args);
>>>  }
>>>  
>>> +static void test_precopy_file(void)
>>> +{
>>> +    g_autofree char *uri = g_strdup_printf("file:%s/%s", tmpfs,
>>> +                                           FILE_TEST_FILENAME);
>>> +    MigrateCommon args = {
>>> +        .connect_uri = uri,
>>> +        .listen_uri = "defer",
>>> +    };
>>> +
>>> +    test_precopy_common(&args);
>>> +}
>>> +
>>> +#if defined(__linux__)
>>> +static void file_offset_finish_hook(QTestState *from, QTestState *to, void *opaque)
>>> +{
>>> +    g_autofree char *path = g_strdup_printf("%s/%s", tmpfs, FILE_TEST_FILENAME);
>>> +    size_t size = FILE_TEST_OFFSET + sizeof(QEMU_VM_FILE_MAGIC);
>>> +    uintptr_t *addr, *p;
>>> +    int fd;
>>> +
>>> +    fd = open(path, O_RDONLY);
>>> +    g_assert(fd != -1);
>>> +    addr = mmap(NULL, size, PROT_READ, MAP_PRIVATE, fd, 0);
>>
>> Not something that matters a lot, but RO mapping a file with private is a
>> bit weird.  Maybe just use MAP_SHARED?
>>
> 
> Yep
> 
>>> +    g_assert(addr != MAP_FAILED);
>>> +
>>> +    /*
>>> +     * Ensure the skipped offset contains zeros and the migration
>>> +     * stream starts at the right place.
>>> +     */
>>> +    p = addr;
>>> +    while (p < addr + FILE_TEST_OFFSET / sizeof(uintptr_t)) {
>>> +        g_assert(*p == 0);
>>> +        p++;
>>> +    }
>>> +    g_assert_cmpint(cpu_to_be32(*p), ==, QEMU_VM_FILE_MAGIC);
>>> +
>>> +    munmap(addr, size);
>>> +    close(fd);
>>> +}
>>> +
>>> +static void test_precopy_file_offset(void)
>>> +{
>>> +    g_autofree char *uri = g_strdup_printf("file:%s/%s,offset=%d", tmpfs,
>>> +                                           FILE_TEST_FILENAME,
>>> +                                           FILE_TEST_OFFSET);
>>
>> Is it intended to also only run this test with linux?  IIUC it should also
>> work for others. Maybe only file_offset_finish_hook() is optional?  Or am i
>> wrong?
>>
> 
> Yes, only the hook is linux-specific. I'll change it.
> 
>>> +    MigrateCommon args = {
>>> +        .connect_uri = uri,
>>> +        .listen_uri = "defer",
>>> +        .finish_hook = file_offset_finish_hook,
>>> +    };
>>> +
>>> +    test_precopy_common(&args);
>>> +}
>>> +#endif
>>> +
>>> +static void test_precopy_file_offset_bad(void)
>>> +{
>>> +    /* using a value not supported by qemu_strtosz() */
>>> +    g_autofree char *uri = g_strdup_printf("file:%s/migfile,offset=0x20M",
>>> +                                           tmpfs);
>>> +    MigrateCommon args = {
>>> +        .connect_uri = uri,
>>> +        .listen_uri = "defer",
>>> +        .error_str = g_strdup(
>>> +            "file URI has bad offset 0x20M: Unknown error -22"),
>>
>> "Unknown error" may imply that in Steve's patch the errno is inverted..
>>
>> Shall we not rely on the string in the test?  It might be too strict, I
>> worry, because error strings should be defined for human readers, and we
>> may not want some e.g. grammar / trivial change to break a test.
>>
> 
> Well, you just caught an issue with the errno by looking at the string,
> so maybe testing it is a good thing?
> 
> I'd expect anyone changing the string to run the test and catch the
> mismatch before sending a patch anyway.
> 
> I don't have a strong opinion about it, though. I can remove the
> error_str.

FWIW, I fixed the errno, so it should say
  file URI has bad offset 0x20M: EINVAL

- Steve

Re: [PATCH 6/6] tests/qtest: migration-test: Add tests for file-based migration

[Peter Xu]

On Fri, Jun 30, 2023 at 12:05:23PM -0300, Fabiano Rosas wrote:
> >> +static void test_precopy_file_offset_bad(void)
> >> +{
> >> +    /* using a value not supported by qemu_strtosz() */
> >> +    g_autofree char *uri = g_strdup_printf("file:%s/migfile,offset=0x20M",
> >> +                                           tmpfs);
> >> +    MigrateCommon args = {
> >> +        .connect_uri = uri,
> >> +        .listen_uri = "defer",
> >> +        .error_str = g_strdup(
> >> +            "file URI has bad offset 0x20M: Unknown error -22"),
> >
> > "Unknown error" may imply that in Steve's patch the errno is inverted..
> >
> > Shall we not rely on the string in the test?  It might be too strict, I
> > worry, because error strings should be defined for human readers, and we
> > may not want some e.g. grammar / trivial change to break a test.
> >
> 
> Well, you just caught an issue with the errno by looking at the string,
> so maybe testing it is a good thing?
> 
> I'd expect anyone changing the string to run the test and catch the
> mismatch before sending a patch anyway.
> 
> I don't have a strong opinion about it, though. I can remove the
> error_str.

I can give a few other examples outside "grammar error" (which in this case
we can guarantee there's no grammar issue..): we can always try to append
something to an error, when error_setg_errno() got refactored, or even
someone just thinks better to make the 1st letter capitalized ("f" -> "F").

It's just too fragile to me..

Thanks,

-- 
Peter Xu