[Qemu-devel] [RFC 07/29] migration: better error handling with QEMUFile

[Peter Xu]

If the postcopy down due to some reason, we can always see this on dst:

  qemu-system-x86_64: RP: Received invalid message 0x0000 length 0x0000

However in most cases that's not the real issue. The problem is that
qemu_get_be16() has no way to show whether the returned data is valid or
not, and we are _always_ assuming it is valid. That's possibly not wise.

The best approach to solve this would be: refactoring QEMUFile interface
to allow the APIs to return error if there is. However it needs quite a
bit of work and testing. For now, let's explicitly check the validity
first before using the data in all places for qemu_get_*().

This patch tries to fix most of the cases I can see. Only if we are with
this, can we make sure we are processing the valid data, and also can we
make sure we can capture the channel down events correctly.

Signed-off-by: Peter Xu <address@hidden>
---
 migration/migration.c |  5 +++++
 migration/ram.c       | 22 ++++++++++++++++++----
 migration/savevm.c    | 29 +++++++++++++++++++++++++++--
 3 files changed, 50 insertions(+), 6 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index bdc4445..5b2602e 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1543,6 +1543,11 @@ static void *source_return_path_thread(void *opaque)
         header_type = qemu_get_be16(rp);
         header_len = qemu_get_be16(rp);
 
+        if (qemu_file_get_error(rp)) {
+            mark_source_rp_bad(ms);
+            goto out;
+        }
+
         if (header_type >= MIG_RP_MSG_MAX ||
             header_type == MIG_RP_MSG_INVALID) {
             error_report("RP: Received invalid message 0x%04x length 0x%04x",
diff --git a/migration/ram.c b/migration/ram.c
index c12358d..7f4cb0f 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -2416,7 +2416,7 @@ static int ram_load_postcopy(QEMUFile *f)
     void *last_host = NULL;
     bool all_zero = false;
 
-    while (!ret && !(flags & RAM_SAVE_FLAG_EOS)) {
+    while (!(flags & RAM_SAVE_FLAG_EOS)) {
         ram_addr_t addr;
         void *host = NULL;
         void *page_buffer = NULL;
@@ -2425,6 +2425,16 @@ static int ram_load_postcopy(QEMUFile *f)
         uint8_t ch;
 
         addr = qemu_get_be64(f);
+
+        /*
+         * If qemu file error, we should stop here, and then "addr"
+         * may be invalid
+         */
+        if (qemu_file_get_error(f)) {
+            ret = qemu_file_get_error(f);
+            break;
+        }
+
         flags = addr & ~TARGET_PAGE_MASK;
         addr &= TARGET_PAGE_MASK;
 
@@ -2505,6 +2515,13 @@ static int ram_load_postcopy(QEMUFile *f)
             error_report("Unknown combination of migration flags: %#x"
                          " (postcopy mode)", flags);
             ret = -EINVAL;
+            break;
+        }
+
+        /* Detect for any possible file errors */
+        if (qemu_file_get_error(f)) {
+            ret = qemu_file_get_error(f);
+            break;
         }
 
         if (place_needed) {
@@ -2519,9 +2536,6 @@ static int ram_load_postcopy(QEMUFile *f)
                                           place_source, block);
             }
         }
-        if (!ret) {
-            ret = qemu_file_get_error(f);
-        }
     }
 
     return ret;
diff --git a/migration/savevm.c b/migration/savevm.c
index fdd15fa..13ae9d6 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1720,6 +1720,11 @@ static int loadvm_process_command(QEMUFile *f)
     cmd = qemu_get_be16(f);
     len = qemu_get_be16(f);
 
+    /* Check validity before continue processing of cmds */
+    if (qemu_file_get_error(f)) {
+        return qemu_file_get_error(f);
+    }
+
     trace_loadvm_process_command(cmd, len);
     if (cmd >= MIG_CMD_MAX || cmd == MIG_CMD_INVALID) {
         error_report("MIG_CMD 0x%x unknown (len 0x%x)", cmd, len);
@@ -1855,6 +1860,11 @@ qemu_loadvm_section_start_full(QEMUFile *f, 
MigrationIncomingState *mis)
         return -EINVAL;
     }
 
+    /* Check validity before load the vmstate */
+    if (qemu_file_get_error(f)) {
+        return qemu_file_get_error(f);
+    }
+
     ret = vmstate_load(f, se);
     if (ret < 0) {
         error_report("error while loading state for instance 0x%x of"
@@ -1888,6 +1898,11 @@ qemu_loadvm_section_part_end(QEMUFile *f, 
MigrationIncomingState *mis)
         return -EINVAL;
     }
 
+    /* Check validity before load the vmstate */
+    if (qemu_file_get_error(f)) {
+        return qemu_file_get_error(f);
+    }
+
     ret = vmstate_load(f, se);
     if (ret < 0) {
         error_report("error while loading state section id %d(%s)",
@@ -1944,8 +1959,14 @@ static int qemu_loadvm_state_main(QEMUFile *f, 
MigrationIncomingState *mis)
     uint8_t section_type;
     int ret = 0;
 
-    while ((section_type = qemu_get_byte(f)) != QEMU_VM_EOF) {
-        ret = 0;
+    while (true) {
+        section_type = qemu_get_byte(f);
+
+        if (qemu_file_get_error(f)) {
+            ret = qemu_file_get_error(f);
+            break;
+        }
+
         trace_qemu_loadvm_state_section(section_type);
         switch (section_type) {
         case QEMU_VM_SECTION_START:
@@ -1969,6 +1990,10 @@ static int qemu_loadvm_state_main(QEMUFile *f, 
MigrationIncomingState *mis)
                 goto out;
             }
             break;
+        case QEMU_VM_EOF:
+            /* This is the end of migration */
+            goto out;
+            break;
         default:
             error_report("Unknown savevm section type %d", section_type);
             ret = -EINVAL;
-- 
2.7.4