[Qemu-devel] [PATCH 0/2] migration: Fix early failure crash

[Qemu-devel] [PATCH 0/2] migration: Fix early failure crash

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

This fixes a crash for the case where a migration exits with an error
very early, this is probably due to my recent error handling change.

I also add a test to make sure this doesn't fail again, the test does
output one line of junk, suggestions for how to clean it up are welcome:

[dgilbert@dgilbert-t530 try]$ tests/migration-test 
/x86_64/migration/deprecated: OK
/x86_64/migration/bad_dest: qemu-system-x86_64: Failed to connect socket: Connection refused
OK
/x86_64/migration/postcopy/unix: OK

Dave

Dr. David Alan Gilbert (2):
  migration: Fix early failure cleanup
  tests/migration: Add test for migration to bad destination

 migration/ram.c        | 12 ++++++----
 tests/migration-test.c | 65 ++++++++++++++++++++++++++++++++++++++------------
 2 files changed, 57 insertions(+), 20 deletions(-)

-- 
2.14.3

[Qemu-devel] [PATCH 1/2] migration: Fix early failure cleanup

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Avoid crash in cleanup after a very early migration failure
(possibly due to my 688a3dcba980bf01344a  'Route errors down ...')

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 migration/ram.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index 8333d8e35e..7095c1040e 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1602,11 +1602,13 @@ static void xbzrle_load_cleanup(void)
 
 static void ram_state_cleanup(RAMState **rsp)
 {
-    migration_page_queue_free(*rsp);
-    qemu_mutex_destroy(&(*rsp)->bitmap_mutex);
-    qemu_mutex_destroy(&(*rsp)->src_page_req_mutex);
-    g_free(*rsp);
-    *rsp = NULL;
+    if (*rsp) {
+        migration_page_queue_free(*rsp);
+        qemu_mutex_destroy(&(*rsp)->bitmap_mutex);
+        qemu_mutex_destroy(&(*rsp)->src_page_req_mutex);
+        g_free(*rsp);
+        *rsp = NULL;
+    }
 }
 
 static void xbzrle_cleanup(void)
-- 
2.14.3

[Qemu-devel] [PATCH 2/2] tests/migration: Add test for migration to bad destination

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Check the source survives.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 tests/migration-test.c | 65 ++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 50 insertions(+), 15 deletions(-)

diff --git a/tests/migration-test.c b/tests/migration-test.c
index d0abad40f5..c1ebbdf08f 100644
--- a/tests/migration-test.c
+++ b/tests/migration-test.c
@@ -478,28 +478,31 @@ static void test_migrate_start(QTestState **from, QTestState **to,
     g_free(cmd_dst);
 }
 
-static void test_migrate_end(QTestState *from, QTestState *to)
+static void test_migrate_end(QTestState *from, QTestState *to, bool test_dest)
 {
     unsigned char dest_byte_a, dest_byte_b, dest_byte_c, dest_byte_d;
 
     qtest_quit(from);
 
-    qtest_memread(to, start_address, &dest_byte_a, 1);
+    if (test_dest) {
+        qtest_memread(to, start_address, &dest_byte_a, 1);
 
-    /* Destination still running, wait for a byte to change */
-    do {
-        qtest_memread(to, start_address, &dest_byte_b, 1);
-        usleep(1000 * 10);
-    } while (dest_byte_a == dest_byte_b);
+        /* Destination still running, wait for a byte to change */
+        do {
+            qtest_memread(to, start_address, &dest_byte_b, 1);
+            usleep(1000 * 10);
+        } while (dest_byte_a == dest_byte_b);
+
+        qtest_qmp_discard_response(to, "{ 'execute' : 'stop'}");
 
-    qtest_qmp_discard_response(to, "{ 'execute' : 'stop'}");
-    /* With it stopped, check nothing changes */
-    qtest_memread(to, start_address, &dest_byte_c, 1);
-    usleep(1000 * 200);
-    qtest_memread(to, start_address, &dest_byte_d, 1);
-    g_assert_cmpint(dest_byte_c, ==, dest_byte_d);
+        /* With it stopped, check nothing changes */
+        qtest_memread(to, start_address, &dest_byte_c, 1);
+        usleep(1000 * 200);
+        qtest_memread(to, start_address, &dest_byte_d, 1);
+        g_assert_cmpint(dest_byte_c, ==, dest_byte_d);
 
-    check_guests_ram(to);
+        check_guests_ram(to);
+    }
 
     qtest_quit(to);
 
@@ -591,7 +594,38 @@ static void test_migrate(void)
 
     g_free(uri);
 
-    test_migrate_end(from, to);
+    test_migrate_end(from, to, true);
+}
+
+static void test_baddest(void)
+{
+    QTestState *from, *to;
+    QDict *rsp, *rsp_return;
+    const char *status;
+    bool failed;
+
+    test_migrate_start(&from, &to, "tcp:0:0");
+    migrate(from, "tcp:0:0");
+    do {
+        rsp = wait_command(from, "{ 'execute': 'query-migrate' }");
+        rsp_return = qdict_get_qdict(rsp, "return");
+
+        status = qdict_get_str(rsp_return, "status");
+
+        g_assert(!strcmp(status, "setup") || !(strcmp(status, "failed")));
+        failed = !strcmp(status, "failed");
+        QDECREF(rsp);
+    } while (!failed);
+
+    /* Is the machine currently running? */
+    rsp = wait_command(from, "{ 'execute': 'query-status' }");
+    g_assert(qdict_haskey(rsp, "return"));
+    rsp_return = qdict_get_qdict(rsp, "return");
+    g_assert(qdict_haskey(rsp_return, "running"));
+    g_assert(qdict_get_bool(rsp_return, "running"));
+    QDECREF(rsp);
+
+    test_migrate_end(from, to, false);
 }
 
 int main(int argc, char **argv)
@@ -615,6 +649,7 @@ int main(int argc, char **argv)
 
     qtest_add_func("/migration/postcopy/unix", test_migrate);
     qtest_add_func("/migration/deprecated", test_deprecated);
+    qtest_add_func("/migration/bad_dest", test_baddest);
 
     ret = g_test_run();
 
-- 
2.14.3

Re: [Qemu-devel] [PATCH 0/2] migration: Fix early failure crash

[Peter Xu]

On Mon, Feb 12, 2018 at 04:03:38PM +0000, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> This fixes a crash for the case where a migration exits with an error
> very early, this is probably due to my recent error handling change.
> 
> I also add a test to make sure this doesn't fail again, the test does
> output one line of junk, suggestions for how to clean it up are welcome:
> 
> [dgilbert@dgilbert-t530 try]$ tests/migration-test 
> /x86_64/migration/deprecated: OK
> /x86_64/migration/bad_dest: qemu-system-x86_64: Failed to connect socket: Connection refused
> OK
> /x86_64/migration/postcopy/unix: OK

So we have more than one way to log things (error_report routes things
directly to stderr while we also have the qemu log stuff).

A stupid but fast way I can think of is just don't dump this in
migrate_fd_cleanup, since after all it's only for HMP and people
should also see that when query migration status.  But it'll be a bit
inconvenient for HMP users encountering failures.

Or maybe we can hack around fd 2 specifically in that test?  It's at
least ugly though...

Anyway, the patches look good to me.

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

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 2/2] tests/migration: Add test for migration to bad destination

[Thomas Huth]

On 12.02.2018 17:03, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> Check the source survives.
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>  tests/migration-test.c | 65 ++++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 50 insertions(+), 15 deletions(-)
[...]
> @@ -615,6 +649,7 @@ int main(int argc, char **argv)
>  
>      qtest_add_func("/migration/postcopy/unix", test_migrate);
>      qtest_add_func("/migration/deprecated", test_deprecated);
> +    qtest_add_func("/migration/bad_dest", test_baddest);

While running "make check", I now see some "Failed to connect socket:
Connection refused" messages popping up, which is a little bit
confusing. Would it be possible to silence these messages somehow?

 Thomas

Re: [Qemu-devel] [PATCH 2/2] tests/migration: Add test for migration to bad destination

[Juan Quintela]

Thomas Huth <thuth@redhat.com> wrote:
> On 12.02.2018 17:03, Dr. David Alan Gilbert (git) wrote:
>> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>> 
>> Check the source survives.
>> 
>> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>> ---
>>  tests/migration-test.c | 65 ++++++++++++++++++++++++++++++++++++++------------
>>  1 file changed, 50 insertions(+), 15 deletions(-)
> [...]
>> @@ -615,6 +649,7 @@ int main(int argc, char **argv)
>>  
>>      qtest_add_func("/migration/postcopy/unix", test_migrate);
>>      qtest_add_func("/migration/deprecated", test_deprecated);
>> +    qtest_add_func("/migration/bad_dest", test_baddest);
>
> While running "make check", I now see some "Failed to connect socket:
> Connection refused" messages popping up, which is a little bit
> confusing. Would it be possible to silence these messages somehow?


I will take a look at that.  I haven't looked where they came from.
Thanks.

Later, Juan.