[Qemu-devel] [PATCH v5 00/11] Add basic "detach" support for dump-guest-memory

[Qemu-devel] [PATCH v5 00/11] Add basic "detach" support for dump-guest-memory

[Peter Xu]

v5 changes:
- patch 1
  - comment English fix [Fam]
- patch 2
  - pass has_detach=true always in hmp_dump_guest_memory [Paolo]
- patch 3
  - always use local_err and error_propagate() when need to check
    the result [Fam]
- patch 8
  - add "DumpQueryResult" in DUMP_COMPLETED event [Eric]
    (since DumpQueryResult is introduced in patch 10, so doing it in
    patch 10 for convenience. Please let me know if I should not do
    this, e.g., if patch re-ordering is required)

v4 changes:
- patch 2:
  - hmp: fix default value lost [Eric]
  - English errors [Eric]
- patch 3:
  - use global DumpState, leverage C99 struct init [Paolo]
  - English errors [Eric]
- patch 5:
  - more cleanup for dump_process [Paolo]
- patch 8:
  - make sure qmp-events.txt is sorted [Eric]
  - enhance error_get_pretty() [Eric]
  - emit DUMP_COMPLETED no matter detach or not
- patch 10:
  - use g_new0 to replace g_malloc0 [Eric]
  - rename "written_bytes" to "completed", "total_bytes" to "total"
    [Eric]
  - use atomic ops and [rw]mb to protect status read/write [Paolo]
- patch 12:
  - English errors [Eric]
  - merge contents into older patches [Eric]

v3 changes (patch number corresponds to v2 patch set):
- patch 1
  - fix commit message. no memory leak, only code cleanup [Fam]
- patch 2
  - better documentation for "dump-guest-memory" (new patch 9) [Fam]
- patch 3
  - remove rcu lock/unlock in dump_init() [Fam, Paolo]
  - embed mr pointer into GuestPhysBlock [Paolo]
  - remove global dump state [Paolo]
- patch 4
  - fix memory leak for error [Fam]
  - evt DUMP_COMPLETED data: change to an optional "*error" [Paolo]
- patch 5
  - fix documents [Fam]
  - change "dump-query" to "query-dump", HMP to "info dump" [Paolo]
- patch 6
  - for query-dump command: define enum for DumpStatus, use "int"
    for written/total [Paolo]
- all
  - reorder the commits as suggested, no fake values [Paolo]
  - split big commit into smaller ones [me]

v2 changes:
- fixed English errors [Drew]
- reordered the "detach" field, first make it optional, then make sure
  it's order is consistent [Drew, Fam]
- added doc for new detach flag [Eric]
- collected error msg even detached [Drew]
- added qmp event DUMP_COMPLETED to notify user [Eric, Fam]
- added "dump-query" QMP & HMP commands to query dump status [Eric]
- "stop" is not allowed when dump in background (also include
  "cont" and "dump-guest-memory") [Fam]
- added codes to calculate how many dump work finished, which could
  be queried from "dump-query" [Laszlo]
- added list to track all used MemoryRegion objects, also ref before
  use [Paolo]
- dump-guest-memory will be forbidden during incoming migrate [Paolo]
- taking rcu lock when collecting memory info [Paolo]

Test Done:
- QMP & HMP
  - test default dump (sync), work as usual
  - test detached dump, command return immediately.
  - When dump finished, will receive event DUMP_COMPLETED. 
  - test query-dump before/during/after dump
  - test kdump with zlib compression, w/ and w/o detach
- libvirt
  - test "virsh dump --memory-only" with default format and
    kdump-zlib format, work as usual

Peter Xu (11):
  dump-guest-memory: cleanup: removing dump_{error|cleanup}().
  dump-guest-memory: add "detach" flag for QMP/HMP interfaces.
  dump-guest-memory: using static DumpState, add DumpStatus
  dump-guest-memory: add dump_in_progress() helper function
  dump-guest-memory: introduce dump_process() helper function.
  dump-guest-memory: disable dump when in INMIGRATE state
  dump-guest-memory: add "detach" support
  dump-guest-memory: add qmp event DUMP_COMPLETED
  DumpState: adding total_size and written_size fields
  Dump: add qmp command "query-dump"
  Dump: add hmp command "info dump"

 docs/qmp-events.txt             |  19 ++++
 dump.c                          | 215 ++++++++++++++++++++++++++++++----------
 hmp-commands-info.hx            |  14 +++
 hmp-commands.hx                 |   5 +-
 hmp.c                           |  27 ++++-
 hmp.h                           |   1 +
 include/qemu-common.h           |   4 +
 include/sysemu/dump.h           |  15 +++
 include/sysemu/memory_mapping.h |   4 +
 memory_mapping.c                |   3 +
 qapi-schema.json                |  57 ++++++++++-
 qapi/event.json                 |  16 +++
 qmp-commands.hx                 |  32 +++++-
 qmp.c                           |  14 +++
 14 files changed, 363 insertions(+), 63 deletions(-)

-- 
2.4.3

[Qemu-devel] [PATCH v5 01/11] dump-guest-memory: cleanup: removing dump_{error|cleanup}().

[Peter Xu]

It might be a little bit confusing and error prone to do
dump_cleanup() in these two functions. A better way is to do
dump_cleanup() before dump finish, no matter whether dump has
succeeded or not.

Signed-off-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
---
 dump.c | 78 +++++++++++++++++++++++++++---------------------------------------
 1 file changed, 32 insertions(+), 46 deletions(-)

diff --git a/dump.c b/dump.c
index 78b7d84..445e739 100644
--- a/dump.c
+++ b/dump.c
@@ -82,12 +82,6 @@ static int dump_cleanup(DumpState *s)
     return 0;
 }
 
-static void dump_error(DumpState *s, const char *reason, Error **errp)
-{
-    dump_cleanup(s);
-    error_setg(errp, "%s", reason);
-}
-
 static int fd_write_vmcore(const void *buf, size_t size, void *opaque)
 {
     DumpState *s = opaque;
@@ -128,7 +122,7 @@ static void write_elf64_header(DumpState *s, Error **errp)
 
     ret = fd_write_vmcore(&elf_header, sizeof(elf_header), s);
     if (ret < 0) {
-        dump_error(s, "dump: failed to write elf header", errp);
+        error_setg(errp, "dump: failed to write elf header");
     }
 }
 
@@ -159,7 +153,7 @@ static void write_elf32_header(DumpState *s, Error **errp)
 
     ret = fd_write_vmcore(&elf_header, sizeof(elf_header), s);
     if (ret < 0) {
-        dump_error(s, "dump: failed to write elf header", errp);
+        error_setg(errp, "dump: failed to write elf header");
     }
 }
 
@@ -182,7 +176,7 @@ static void write_elf64_load(DumpState *s, MemoryMapping *memory_mapping,
 
     ret = fd_write_vmcore(&phdr, sizeof(Elf64_Phdr), s);
     if (ret < 0) {
-        dump_error(s, "dump: failed to write program header table", errp);
+        error_setg(errp, "dump: failed to write program header table");
     }
 }
 
@@ -205,7 +199,7 @@ static void write_elf32_load(DumpState *s, MemoryMapping *memory_mapping,
 
     ret = fd_write_vmcore(&phdr, sizeof(Elf32_Phdr), s);
     if (ret < 0) {
-        dump_error(s, "dump: failed to write program header table", errp);
+        error_setg(errp, "dump: failed to write program header table");
     }
 }
 
@@ -225,7 +219,7 @@ static void write_elf64_note(DumpState *s, Error **errp)
 
     ret = fd_write_vmcore(&phdr, sizeof(Elf64_Phdr), s);
     if (ret < 0) {
-        dump_error(s, "dump: failed to write program header table", errp);
+        error_setg(errp, "dump: failed to write program header table");
     }
 }
 
@@ -245,7 +239,7 @@ static void write_elf64_notes(WriteCoreDumpFunction f, DumpState *s,
         id = cpu_index(cpu);
         ret = cpu_write_elf64_note(f, cpu, id, s);
         if (ret < 0) {
-            dump_error(s, "dump: failed to write elf notes", errp);
+            error_setg(errp, "dump: failed to write elf notes");
             return;
         }
     }
@@ -253,7 +247,7 @@ static void write_elf64_notes(WriteCoreDumpFunction f, DumpState *s,
     CPU_FOREACH(cpu) {
         ret = cpu_write_elf64_qemunote(f, cpu, s);
         if (ret < 0) {
-            dump_error(s, "dump: failed to write CPU status", errp);
+            error_setg(errp, "dump: failed to write CPU status");
             return;
         }
     }
@@ -275,7 +269,7 @@ static void write_elf32_note(DumpState *s, Error **errp)
 
     ret = fd_write_vmcore(&phdr, sizeof(Elf32_Phdr), s);
     if (ret < 0) {
-        dump_error(s, "dump: failed to write program header table", errp);
+        error_setg(errp, "dump: failed to write program header table");
     }
 }
 
@@ -290,7 +284,7 @@ static void write_elf32_notes(WriteCoreDumpFunction f, DumpState *s,
         id = cpu_index(cpu);
         ret = cpu_write_elf32_note(f, cpu, id, s);
         if (ret < 0) {
-            dump_error(s, "dump: failed to write elf notes", errp);
+            error_setg(errp, "dump: failed to write elf notes");
             return;
         }
     }
@@ -298,7 +292,7 @@ static void write_elf32_notes(WriteCoreDumpFunction f, DumpState *s,
     CPU_FOREACH(cpu) {
         ret = cpu_write_elf32_qemunote(f, cpu, s);
         if (ret < 0) {
-            dump_error(s, "dump: failed to write CPU status", errp);
+            error_setg(errp, "dump: failed to write CPU status");
             return;
         }
     }
@@ -326,7 +320,7 @@ static void write_elf_section(DumpState *s, int type, Error **errp)
 
     ret = fd_write_vmcore(&shdr, shdr_size, s);
     if (ret < 0) {
-        dump_error(s, "dump: failed to write section header table", errp);
+        error_setg(errp, "dump: failed to write section header table");
     }
 }
 
@@ -336,7 +330,7 @@ static void write_data(DumpState *s, void *buf, int length, Error **errp)
 
     ret = fd_write_vmcore(buf, length, s);
     if (ret < 0) {
-        dump_error(s, "dump: failed to save memory", errp);
+        error_setg(errp, "dump: failed to save memory");
     }
 }
 
@@ -568,11 +562,6 @@ static void dump_begin(DumpState *s, Error **errp)
     }
 }
 
-static void dump_completed(DumpState *s)
-{
-    dump_cleanup(s);
-}
-
 static int get_next_block(DumpState *s, GuestPhysBlock *block)
 {
     while (1) {
@@ -624,8 +613,6 @@ static void dump_iterate(DumpState *s, Error **errp)
         }
 
     } while (!get_next_block(s, block));
-
-    dump_completed(s);
 }
 
 static void create_vmcore(DumpState *s, Error **errp)
@@ -765,7 +752,7 @@ static void create_header32(DumpState *s, Error **errp)
     dh->status = cpu_to_dump32(s, status);
 
     if (write_buffer(s->fd, 0, dh, size) < 0) {
-        dump_error(s, "dump: failed to write disk dump header", errp);
+        error_setg(errp, "dump: failed to write disk dump header");
         goto out;
     }
 
@@ -784,7 +771,7 @@ static void create_header32(DumpState *s, Error **errp)
 
     if (write_buffer(s->fd, DISKDUMP_HEADER_BLOCKS *
                      block_size, kh, size) < 0) {
-        dump_error(s, "dump: failed to write kdump sub header", errp);
+        error_setg(errp, "dump: failed to write kdump sub header");
         goto out;
     }
 
@@ -800,7 +787,7 @@ static void create_header32(DumpState *s, Error **errp)
     }
     if (write_buffer(s->fd, offset_note, s->note_buf,
                      s->note_size) < 0) {
-        dump_error(s, "dump: failed to write notes", errp);
+        error_setg(errp, "dump: failed to write notes");
         goto out;
     }
 
@@ -865,7 +852,7 @@ static void create_header64(DumpState *s, Error **errp)
     dh->status = cpu_to_dump32(s, status);
 
     if (write_buffer(s->fd, 0, dh, size) < 0) {
-        dump_error(s, "dump: failed to write disk dump header", errp);
+        error_setg(errp, "dump: failed to write disk dump header");
         goto out;
     }
 
@@ -884,7 +871,7 @@ static void create_header64(DumpState *s, Error **errp)
 
     if (write_buffer(s->fd, DISKDUMP_HEADER_BLOCKS *
                      block_size, kh, size) < 0) {
-        dump_error(s, "dump: failed to write kdump sub header", errp);
+        error_setg(errp, "dump: failed to write kdump sub header");
         goto out;
     }
 
@@ -901,7 +888,7 @@ static void create_header64(DumpState *s, Error **errp)
 
     if (write_buffer(s->fd, offset_note, s->note_buf,
                      s->note_size) < 0) {
-        dump_error(s, "dump: failed to write notes", errp);
+        error_setg(errp, "dump: failed to write notes");
         goto out;
     }
 
@@ -1064,7 +1051,7 @@ static void write_dump_bitmap(DumpState *s, Error **errp)
     while (get_next_page(&block_iter, &pfn, NULL, s)) {
         ret = set_dump_bitmap(last_pfn, pfn, true, dump_bitmap_buf, s);
         if (ret < 0) {
-            dump_error(s, "dump: failed to set dump_bitmap", errp);
+            error_setg(errp, "dump: failed to set dump_bitmap");
             goto out;
         }
 
@@ -1081,7 +1068,7 @@ static void write_dump_bitmap(DumpState *s, Error **errp)
         ret = set_dump_bitmap(last_pfn, last_pfn + PFN_BUFBITMAP, false,
                               dump_bitmap_buf, s);
         if (ret < 0) {
-            dump_error(s, "dump: failed to sync dump_bitmap", errp);
+            error_setg(errp, "dump: failed to sync dump_bitmap");
             goto out;
         }
     }
@@ -1214,7 +1201,7 @@ static void write_dump_pages(DumpState *s, Error **errp)
     ret = write_cache(&page_data, buf, TARGET_PAGE_SIZE, false);
     g_free(buf);
     if (ret < 0) {
-        dump_error(s, "dump: failed to write page data (zero page)", errp);
+        error_setg(errp, "dump: failed to write page data (zero page)");
         goto out;
     }
 
@@ -1230,7 +1217,7 @@ static void write_dump_pages(DumpState *s, Error **errp)
             ret = write_cache(&page_desc, &pd_zero, sizeof(PageDescriptor),
                               false);
             if (ret < 0) {
-                dump_error(s, "dump: failed to write page desc", errp);
+                error_setg(errp, "dump: failed to write page desc");
                 goto out;
             }
         } else {
@@ -1255,7 +1242,7 @@ static void write_dump_pages(DumpState *s, Error **errp)
 
                 ret = write_cache(&page_data, buf_out, size_out, false);
                 if (ret < 0) {
-                    dump_error(s, "dump: failed to write page data", errp);
+                    error_setg(errp, "dump: failed to write page data");
                     goto out;
                 }
 #ifdef CONFIG_LZO
@@ -1268,7 +1255,7 @@ static void write_dump_pages(DumpState *s, Error **errp)
 
                 ret = write_cache(&page_data, buf_out, size_out, false);
                 if (ret < 0) {
-                    dump_error(s, "dump: failed to write page data", errp);
+                    error_setg(errp, "dump: failed to write page data");
                     goto out;
                 }
 #endif
@@ -1282,7 +1269,7 @@ static void write_dump_pages(DumpState *s, Error **errp)
 
                 ret = write_cache(&page_data, buf_out, size_out, false);
                 if (ret < 0) {
-                    dump_error(s, "dump: failed to write page data", errp);
+                    error_setg(errp, "dump: failed to write page data");
                     goto out;
                 }
 #endif
@@ -1297,7 +1284,7 @@ static void write_dump_pages(DumpState *s, Error **errp)
 
                 ret = write_cache(&page_data, buf, TARGET_PAGE_SIZE, false);
                 if (ret < 0) {
-                    dump_error(s, "dump: failed to write page data", errp);
+                    error_setg(errp, "dump: failed to write page data");
                     goto out;
                 }
             }
@@ -1309,7 +1296,7 @@ static void write_dump_pages(DumpState *s, Error **errp)
 
             ret = write_cache(&page_desc, &pd, sizeof(PageDescriptor), false);
             if (ret < 0) {
-                dump_error(s, "dump: failed to write page desc", errp);
+                error_setg(errp, "dump: failed to write page desc");
                 goto out;
             }
         }
@@ -1317,12 +1304,12 @@ static void write_dump_pages(DumpState *s, Error **errp)
 
     ret = write_cache(&page_desc, NULL, 0, true);
     if (ret < 0) {
-        dump_error(s, "dump: failed to sync cache for page_desc", errp);
+        error_setg(errp, "dump: failed to sync cache for page_desc");
         goto out;
     }
     ret = write_cache(&page_data, NULL, 0, true);
     if (ret < 0) {
-        dump_error(s, "dump: failed to sync cache for page_data", errp);
+        error_setg(errp, "dump: failed to sync cache for page_data");
         goto out;
     }
 
@@ -1366,7 +1353,7 @@ static void create_kdump_vmcore(DumpState *s, Error **errp)
 
     ret = write_start_flat_header(s->fd);
     if (ret < 0) {
-        dump_error(s, "dump: failed to write start flat header", errp);
+        error_setg(errp, "dump: failed to write start flat header");
         return;
     }
 
@@ -1390,11 +1377,9 @@ static void create_kdump_vmcore(DumpState *s, Error **errp)
 
     ret = write_end_flat_header(s->fd);
     if (ret < 0) {
-        dump_error(s, "dump: failed to write end flat header", errp);
+        error_setg(errp, "dump: failed to write end flat header");
         return;
     }
-
-    dump_completed(s);
 }
 
 static ram_addr_t get_start_block(DumpState *s)
@@ -1677,6 +1662,7 @@ void qmp_dump_guest_memory(bool paging, const char *file, bool has_begin,
         create_vmcore(s, errp);
     }
 
+    dump_cleanup(s);
     g_free(s);
 }
 
-- 
2.4.3

[Qemu-devel] [PATCH v5 02/11] dump-guest-memory: add "detach" flag for QMP/HMP interfaces.

[Peter Xu]

This patch only adds the interfaces, but does not implement them.
"detach" parameter is made optional, to make sure that all the old
dump-guest-memory requests will still be able to work.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 dump.c           | 5 +++--
 hmp-commands.hx  | 5 +++--
 hmp.c            | 9 +++++++--
 qapi-schema.json | 8 ++++++--
 qmp-commands.hx  | 6 ++++--
 5 files changed, 23 insertions(+), 10 deletions(-)

diff --git a/dump.c b/dump.c
index 445e739..d79e0ed 100644
--- a/dump.c
+++ b/dump.c
@@ -1580,8 +1580,9 @@ cleanup:
     dump_cleanup(s);
 }
 
-void qmp_dump_guest_memory(bool paging, const char *file, bool has_begin,
-                           int64_t begin, bool has_length,
+void qmp_dump_guest_memory(bool paging, const char *file,
+                           bool has_detach, bool detach,
+                           bool has_begin, int64_t begin, bool has_length,
                            int64_t length, bool has_format,
                            DumpGuestMemoryFormat format, Error **errp)
 {
diff --git a/hmp-commands.hx b/hmp-commands.hx
index bb52e4d..664d794 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1056,10 +1056,11 @@ ETEXI
 
     {
         .name       = "dump-guest-memory",
-        .args_type  = "paging:-p,zlib:-z,lzo:-l,snappy:-s,filename:F,begin:i?,length:i?",
-        .params     = "[-p] [-z|-l|-s] filename [begin length]",
+        .args_type  = "paging:-p,detach:-d,zlib:-z,lzo:-l,snappy:-s,filename:F,begin:i?,length:i?",
+        .params     = "[-p] [-d] [-z|-l|-s] filename [begin length]",
         .help       = "dump guest memory into file 'filename'.\n\t\t\t"
                       "-p: do paging to get guest's memory mapping.\n\t\t\t"
+                      "-d: return immediately (do not wait for completion).\n\t\t\t"
                       "-z: dump in kdump-compressed format, with zlib compression.\n\t\t\t"
                       "-l: dump in kdump-compressed format, with lzo compression.\n\t\t\t"
                       "-s: dump in kdump-compressed format, with snappy compression.\n\t\t\t"
diff --git a/hmp.c b/hmp.c
index 2140605..1f4d0b6 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1586,8 +1586,10 @@ void hmp_dump_guest_memory(Monitor *mon, const QDict *qdict)
     const char *file = qdict_get_str(qdict, "filename");
     bool has_begin = qdict_haskey(qdict, "begin");
     bool has_length = qdict_haskey(qdict, "length");
+    bool has_detach = qdict_haskey(qdict, "detach");
     int64_t begin = 0;
     int64_t length = 0;
+    bool detach = false;
     enum DumpGuestMemoryFormat dump_format = DUMP_GUEST_MEMORY_FORMAT_ELF;
     char *prot;
 
@@ -1615,11 +1617,14 @@ void hmp_dump_guest_memory(Monitor *mon, const QDict *qdict)
     if (has_length) {
         length = qdict_get_int(qdict, "length");
     }
+    if (has_detach) {
+        detach = qdict_get_bool(qdict, "detach");
+    }
 
     prot = g_strconcat("file:", file, NULL);
 
-    qmp_dump_guest_memory(paging, prot, has_begin, begin, has_length, length,
-                          true, dump_format, &err);
+    qmp_dump_guest_memory(paging, prot, true, detach, has_begin, begin,
+                          has_length, length, true, dump_format, &err);
     hmp_handle_error(mon, &err);
     g_free(prot);
 }
diff --git a/qapi-schema.json b/qapi-schema.json
index 8b1a423..97c3ac4 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2115,6 +2115,9 @@
 #            2. fd: the protocol starts with "fd:", and the following string
 #               is the fd's name.
 #
+# @detach: #optional if true, QMP will return immediately rather than
+#          waiting for the dump to finish. (since 2.6).
+#
 # @begin: #optional if specified, the starting physical address.
 #
 # @length: #optional if specified, the memory size, in bytes. If you don't
@@ -2131,8 +2134,9 @@
 # Since: 1.2
 ##
 { 'command': 'dump-guest-memory',
-  'data': { 'paging': 'bool', 'protocol': 'str', '*begin': 'int',
-            '*length': 'int', '*format': 'DumpGuestMemoryFormat' } }
+  'data': { 'paging': 'bool', 'protocol': 'str', '*detach': 'bool',
+            '*begin': 'int', '*length': 'int',
+            '*format': 'DumpGuestMemoryFormat'} }
 
 ##
 # @DumpGuestMemoryCapability:
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 9d8b42f..6b51585 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -840,8 +840,8 @@ EQMP
 
     {
         .name       = "dump-guest-memory",
-        .args_type  = "paging:b,protocol:s,begin:i?,end:i?,format:s?",
-        .params     = "-p protocol [begin] [length] [format]",
+        .args_type  = "paging:b,protocol:s,detach:b?,begin:i?,end:i?,format:s?",
+        .params     = "-p protocol [-d] [begin] [length] [format]",
         .help       = "dump guest memory to file",
         .mhandler.cmd_new = qmp_marshal_dump_guest_memory,
     },
@@ -857,6 +857,8 @@ Arguments:
 - "paging": do paging to get guest's memory mapping (json-bool)
 - "protocol": destination file(started with "file:") or destination file
               descriptor (started with "fd:") (json-string)
+- "detach": if specified, command will return immediately, without waiting
+            for the dump to finish (json-bool)
 - "begin": the starting physical address. It's optional, and should be specified
            with length together (json-int)
 - "length": the memory size, in bytes. It's optional, and should be specified
-- 
2.4.3

[Qemu-devel] [PATCH v5 03/11] dump-guest-memory: using static DumpState, add DumpStatus

[Peter Xu]

Instead of malloc/free each time for DumpState, make it
static. Added DumpStatus to show status for dump.

This is to be used for detached dump.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 dump.c                | 21 ++++++++++++++++-----
 include/sysemu/dump.h |  2 ++
 qapi-schema.json      | 18 ++++++++++++++++++
 3 files changed, 36 insertions(+), 5 deletions(-)

diff --git a/dump.c b/dump.c
index d79e0ed..b48cec2 100644
--- a/dump.c
+++ b/dump.c
@@ -1418,6 +1418,14 @@ static void get_max_mapnr(DumpState *s)
     s->max_mapnr = paddr_to_pfn(last_block->target_end);
 }
 
+static DumpState dump_state_global = { .status = DUMP_STATUS_NONE };
+
+static void dump_state_prepare(DumpState *s)
+{
+    /* zero the struct, setting status to active */
+    *s = (DumpState) { .status = DUMP_STATUS_ACTIVE };
+}
+
 static void dump_init(DumpState *s, int fd, bool has_format,
                       DumpGuestMemoryFormat format, bool paging, bool has_filter,
                       int64_t begin, int64_t length, Error **errp)
@@ -1647,24 +1655,27 @@ void qmp_dump_guest_memory(bool paging, const char *file,
         return;
     }
 
-    s = g_malloc0(sizeof(DumpState));
+    s = &dump_state_global;
+    dump_state_prepare(s);
 
     dump_init(s, fd, has_format, format, paging, has_begin,
               begin, length, &local_err);
     if (local_err) {
-        g_free(s);
         error_propagate(errp, local_err);
+        s->status = DUMP_STATUS_FAILED;
         return;
     }
 
     if (has_format && format != DUMP_GUEST_MEMORY_FORMAT_ELF) {
-        create_kdump_vmcore(s, errp);
+        create_kdump_vmcore(s, &local_err);
     } else {
-        create_vmcore(s, errp);
+        create_vmcore(s, &local_err);
     }
 
+    s->status = (local_err ? DUMP_STATUS_FAILED : DUMP_STATUS_COMPLETED);
+    error_propagate(errp, local_err);
+
     dump_cleanup(s);
-    g_free(s);
 }
 
 DumpGuestMemoryCapability *qmp_query_dump_guest_memory_capability(Error **errp)
diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
index 7e4ec5c..affef38 100644
--- a/include/sysemu/dump.h
+++ b/include/sysemu/dump.h
@@ -45,6 +45,7 @@
 
 #include "sysemu/dump-arch.h"
 #include "sysemu/memory_mapping.h"
+#include "qapi-types.h"
 
 typedef struct QEMU_PACKED MakedumpfileHeader {
     char signature[16];     /* = "makedumpfile" */
@@ -183,6 +184,7 @@ typedef struct DumpState {
     off_t offset_page;          /* offset of page part in vmcore */
     size_t num_dumpable;        /* number of page that can be dumped */
     uint32_t flag_compress;     /* indicate the compression format */
+    DumpStatus status;          /* current dump status */
 } DumpState;
 
 uint16_t cpu_to_dump16(DumpState *s, uint16_t val);
diff --git a/qapi-schema.json b/qapi-schema.json
index 97c3ac4..691a130 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2139,6 +2139,24 @@
             '*format': 'DumpGuestMemoryFormat'} }
 
 ##
+# @DumpStatus
+#
+# Describe the status of a long-running background guest memory dump.
+#
+# @none: no dump-guest-memory has started yet.
+#
+# @active: there is one dump running in background.
+#
+# @completed: the last dump has finished successfully.
+#
+# @failed: the last dump has failed.
+#
+# Since 2.6
+##
+{ 'enum': 'DumpStatus',
+  'data': [ 'none', 'active', 'completed', 'failed' ] }
+
+##
 # @DumpGuestMemoryCapability:
 #
 # A list of the available formats for dump-guest-memory
-- 
2.4.3

[Qemu-devel] [PATCH v5 04/11] dump-guest-memory: add dump_in_progress() helper function

[Peter Xu]

For now, it has no effect. It will be used in dump detach support.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 dump.c                | 13 +++++++++++++
 include/qemu-common.h |  4 ++++
 qmp.c                 | 14 ++++++++++++++
 3 files changed, 31 insertions(+)

diff --git a/dump.c b/dump.c
index b48cec2..ccd56c8 100644
--- a/dump.c
+++ b/dump.c
@@ -1426,6 +1426,12 @@ static void dump_state_prepare(DumpState *s)
     *s = (DumpState) { .status = DUMP_STATUS_ACTIVE };
 }
 
+bool dump_in_progress(void)
+{
+    DumpState *state = &dump_state_global;
+    return (state->status == DUMP_STATUS_ACTIVE);
+}
+
 static void dump_init(DumpState *s, int fd, bool has_format,
                       DumpGuestMemoryFormat format, bool paging, bool has_filter,
                       int64_t begin, int64_t length, Error **errp)
@@ -1599,6 +1605,13 @@ void qmp_dump_guest_memory(bool paging, const char *file,
     DumpState *s;
     Error *local_err = NULL;
 
+    /* if there is a dump in background, we should wait until the dump
+     * finished */
+    if (dump_in_progress()) {
+        error_setg(errp, "There is a dump in process, please wait.");
+        return;
+    }
+
     /*
      * kdump-compressed format need the whole memory dumped, so paging or
      * filter is not supported here.
diff --git a/include/qemu-common.h b/include/qemu-common.h
index 405364f..7b74961 100644
--- a/include/qemu-common.h
+++ b/include/qemu-common.h
@@ -501,4 +501,8 @@ int parse_debug_env(const char *name, int max, int initial);
 const char *qemu_ether_ntoa(const MACAddr *mac);
 void page_size_init(void);
 
+/* returns non-zero if dump is in progress, otherwise zero is
+ * returned. */
+bool dump_in_progress(void);
+
 #endif
diff --git a/qmp.c b/qmp.c
index 0a1fa19..055586d 100644
--- a/qmp.c
+++ b/qmp.c
@@ -102,6 +102,13 @@ void qmp_quit(Error **errp)
 
 void qmp_stop(Error **errp)
 {
+    /* if there is a dump in background, we should wait until the dump
+     * finished */
+    if (dump_in_progress()) {
+        error_setg(errp, "There is a dump in process, please wait.");
+        return;
+    }
+
     if (runstate_check(RUN_STATE_INMIGRATE)) {
         autostart = 0;
     } else {
@@ -174,6 +181,13 @@ void qmp_cont(Error **errp)
     BlockBackend *blk;
     BlockDriverState *bs;
 
+    /* if there is a dump in background, we should wait until the dump
+     * finished */
+    if (dump_in_progress()) {
+        error_setg(errp, "There is a dump in process, please wait.");
+        return;
+    }
+
     if (runstate_needs_reset()) {
         error_setg(errp, "Resetting the Virtual Machine is required");
         return;
-- 
2.4.3

[Qemu-devel] [PATCH v5 05/11] dump-guest-memory: introduce dump_process() helper function.

[Peter Xu]

No functional change. Cleanup only.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 dump.c                | 31 +++++++++++++++++++++----------
 include/sysemu/dump.h |  3 +++
 2 files changed, 24 insertions(+), 10 deletions(-)

diff --git a/dump.c b/dump.c
index ccd56c8..f0ee9a8 100644
--- a/dump.c
+++ b/dump.c
@@ -1441,6 +1441,9 @@ static void dump_init(DumpState *s, int fd, bool has_format,
     Error *err = NULL;
     int ret;
 
+    s->has_format = has_format;
+    s->format = format;
+
     /* kdump-compressed is conflict with paging and filter */
     if (has_format && format != DUMP_GUEST_MEMORY_FORMAT_ELF) {
         assert(!paging && !has_filter);
@@ -1594,6 +1597,23 @@ cleanup:
     dump_cleanup(s);
 }
 
+/* this operation might be time consuming. */
+static void dump_process(DumpState *s, Error **errp)
+{
+    Error *local_err = NULL;
+
+    if (s->has_format && s->format != DUMP_GUEST_MEMORY_FORMAT_ELF) {
+        create_kdump_vmcore(s, &local_err);
+    } else {
+        create_vmcore(s, &local_err);
+    }
+
+    s->status = (local_err ? DUMP_STATUS_FAILED : DUMP_STATUS_COMPLETED);
+    error_propagate(errp, local_err);
+
+    dump_cleanup(s);
+}
+
 void qmp_dump_guest_memory(bool paging, const char *file,
                            bool has_detach, bool detach,
                            bool has_begin, int64_t begin, bool has_length,
@@ -1679,16 +1699,7 @@ void qmp_dump_guest_memory(bool paging, const char *file,
         return;
     }
 
-    if (has_format && format != DUMP_GUEST_MEMORY_FORMAT_ELF) {
-        create_kdump_vmcore(s, &local_err);
-    } else {
-        create_vmcore(s, &local_err);
-    }
-
-    s->status = (local_err ? DUMP_STATUS_FAILED : DUMP_STATUS_COMPLETED);
-    error_propagate(errp, local_err);
-
-    dump_cleanup(s);
+    dump_process(s, errp);
 }
 
 DumpGuestMemoryCapability *qmp_query_dump_guest_memory_capability(Error **errp)
diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
index affef38..d6f4a9c 100644
--- a/include/sysemu/dump.h
+++ b/include/sysemu/dump.h
@@ -185,6 +185,9 @@ typedef struct DumpState {
     size_t num_dumpable;        /* number of page that can be dumped */
     uint32_t flag_compress;     /* indicate the compression format */
     DumpStatus status;          /* current dump status */
+
+    bool has_format;              /* whether format is provided */
+    DumpGuestMemoryFormat format; /* valid only if has_format == true */
 } DumpState;
 
 uint16_t cpu_to_dump16(DumpState *s, uint16_t val);
-- 
2.4.3

[Qemu-devel] [PATCH v5 06/11] dump-guest-memory: disable dump when in INMIGRATE state

[Peter Xu]

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 dump.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/dump.c b/dump.c
index f0ee9a8..aa9d1f8 100644
--- a/dump.c
+++ b/dump.c
@@ -1625,6 +1625,11 @@ void qmp_dump_guest_memory(bool paging, const char *file,
     DumpState *s;
     Error *local_err = NULL;
 
+    if (runstate_check(RUN_STATE_INMIGRATE)) {
+        error_setg(errp, "Dump not allowed during incoming migration.");
+        return;
+    }
+
     /* if there is a dump in background, we should wait until the dump
      * finished */
     if (dump_in_progress()) {
-- 
2.4.3

[Qemu-devel] [PATCH v5 07/11] dump-guest-memory: add "detach" support

[Peter Xu]

If "detach" is provided, one thread is created to do the dump work,
while main thread will return immediately. For each GuestPhysBlock,
adding one more field "mr" to points to MemoryRegion that it
belongs, also ref the mr before use.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 dump.c                          | 27 ++++++++++++++++++++++++++-
 include/sysemu/dump.h           |  1 +
 include/sysemu/memory_mapping.h |  4 ++++
 memory_mapping.c                |  3 +++
 4 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/dump.c b/dump.c
index aa9d1f8..630c67c 100644
--- a/dump.c
+++ b/dump.c
@@ -1614,6 +1614,20 @@ static void dump_process(DumpState *s, Error **errp)
     dump_cleanup(s);
 }
 
+static void *dump_thread(void *data)
+{
+    Error *err = NULL;
+    DumpState *s = (DumpState *)data;
+
+    dump_process(s, &err);
+
+    if (err) {
+        /* TODO: notify user the error */
+        error_free(err);
+    }
+    return NULL;
+}
+
 void qmp_dump_guest_memory(bool paging, const char *file,
                            bool has_detach, bool detach,
                            bool has_begin, int64_t begin, bool has_length,
@@ -1624,6 +1638,7 @@ void qmp_dump_guest_memory(bool paging, const char *file,
     int fd = -1;
     DumpState *s;
     Error *local_err = NULL;
+    bool detach_p = false;
 
     if (runstate_check(RUN_STATE_INMIGRATE)) {
         error_setg(errp, "Dump not allowed during incoming migration.");
@@ -1655,6 +1670,9 @@ void qmp_dump_guest_memory(bool paging, const char *file,
         error_setg(errp, QERR_MISSING_PARAMETER, "begin");
         return;
     }
+    if (has_detach) {
+        detach_p = detach;
+    }
 
     /* check whether lzo/snappy is supported */
 #ifndef CONFIG_LZO
@@ -1704,7 +1722,14 @@ void qmp_dump_guest_memory(bool paging, const char *file,
         return;
     }
 
-    dump_process(s, errp);
+    if (detach_p) {
+        /* detached dump */
+        qemu_thread_create(&s->dump_thread, "dump_thread", dump_thread,
+                           s, QEMU_THREAD_DETACHED);
+    } else {
+        /* sync dump */
+        dump_process(s, errp);
+    }
 }
 
 DumpGuestMemoryCapability *qmp_query_dump_guest_memory_capability(Error **errp)
diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
index d6f4a9c..31930c6 100644
--- a/include/sysemu/dump.h
+++ b/include/sysemu/dump.h
@@ -188,6 +188,7 @@ typedef struct DumpState {
 
     bool has_format;              /* whether format is provided */
     DumpGuestMemoryFormat format; /* valid only if has_format == true */
+    QemuThread dump_thread;       /* thread for detached dump */
 } DumpState;
 
 uint16_t cpu_to_dump16(DumpState *s, uint16_t val);
diff --git a/include/sysemu/memory_mapping.h b/include/sysemu/memory_mapping.h
index a75d59a..d46d879 100644
--- a/include/sysemu/memory_mapping.h
+++ b/include/sysemu/memory_mapping.h
@@ -16,6 +16,7 @@
 
 #include "qemu/queue.h"
 #include "qemu/typedefs.h"
+#include "exec/memory.h"
 
 typedef struct GuestPhysBlock {
     /* visible to guest, reflects PCI hole, etc */
@@ -27,6 +28,9 @@ typedef struct GuestPhysBlock {
     /* points into host memory */
     uint8_t *host_addr;
 
+    /* points to the MemoryRegion that this block belongs to */
+    MemoryRegion *mr;
+
     QTAILQ_ENTRY(GuestPhysBlock) next;
 } GuestPhysBlock;
 
diff --git a/memory_mapping.c b/memory_mapping.c
index 36d6b26..f4f0622 100644
--- a/memory_mapping.c
+++ b/memory_mapping.c
@@ -177,6 +177,7 @@ void guest_phys_blocks_free(GuestPhysBlockList *list)
 
     QTAILQ_FOREACH_SAFE(p, &list->head, next, q) {
         QTAILQ_REMOVE(&list->head, p, next);
+        memory_region_unref(p->mr);
         g_free(p);
     }
     list->num = 0;
@@ -240,6 +241,8 @@ static void guest_phys_blocks_region_add(MemoryListener *listener,
         block->target_start = target_start;
         block->target_end   = target_end;
         block->host_addr    = host_addr;
+        block->mr           = section->mr;
+        memory_region_ref(section->mr);
 
         QTAILQ_INSERT_TAIL(&g->list->head, block, next);
         ++g->list->num;
-- 
2.4.3

[Qemu-devel] [PATCH v5 08/11] dump-guest-memory: add qmp event DUMP_COMPLETED

[Peter Xu]

One new QMP event DUMP_COMPLETED is added. When a dump finishes, one
DUMP_COMPLETED event will occur to notify the user.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 docs/qmp-events.txt | 16 ++++++++++++++++
 dump.c              | 15 ++++++++-------
 qapi-schema.json    |  3 ++-
 qapi/event.json     | 14 ++++++++++++++
 qmp-commands.hx     |  5 +++--
 5 files changed, 43 insertions(+), 10 deletions(-)

diff --git a/docs/qmp-events.txt b/docs/qmp-events.txt
index d2f1ce4..1f79588 100644
--- a/docs/qmp-events.txt
+++ b/docs/qmp-events.txt
@@ -220,6 +220,22 @@ Data:
   },
   "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
 
+DUMP_COMPLETED
+--------------
+
+Emitted when the guest has finished one memory dump.
+
+Data:
+
+- "error": Error message when dump failed. This is only a
+  human-readable string provided when dump failed. It should not be
+  parsed in any way (json-string, optional)
+
+Example:
+
+{ "event": "DUMP_COMPLETED",
+  "data": {} }
+
 GUEST_PANICKED
 --------------
 
diff --git a/dump.c b/dump.c
index 630c67c..1a65fcf 100644
--- a/dump.c
+++ b/dump.c
@@ -25,6 +25,7 @@
 #include "qapi/error.h"
 #include "qapi/qmp/qerror.h"
 #include "qmp-commands.h"
+#include "qapi-event.h"
 
 #include <zlib.h>
 #ifdef CONFIG_LZO
@@ -1609,8 +1610,13 @@ static void dump_process(DumpState *s, Error **errp)
     }
 
     s->status = (local_err ? DUMP_STATUS_FAILED : DUMP_STATUS_COMPLETED);
-    error_propagate(errp, local_err);
 
+    /* send DUMP_COMPLETED message (unconditionally) */
+    qapi_event_send_dump_completed(!!local_err, (local_err ? \
+                                   error_get_pretty(local_err) : NULL),
+                                   &error_abort);
+
+    error_propagate(errp, local_err);
     dump_cleanup(s);
 }
 
@@ -1618,13 +1624,8 @@ static void *dump_thread(void *data)
 {
     Error *err = NULL;
     DumpState *s = (DumpState *)data;
-
     dump_process(s, &err);
-
-    if (err) {
-        /* TODO: notify user the error */
-        error_free(err);
-    }
+    error_free(err);
     return NULL;
 }
 
diff --git a/qapi-schema.json b/qapi-schema.json
index 691a130..f0d3c4a 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2116,7 +2116,8 @@
 #               is the fd's name.
 #
 # @detach: #optional if true, QMP will return immediately rather than
-#          waiting for the dump to finish. (since 2.6).
+#          waiting for the dump to finish. A DUMP_COMPLETED event will
+#          occur at the end. (since 2.6).
 #
 # @begin: #optional if specified, the starting physical address.
 #
diff --git a/qapi/event.json b/qapi/event.json
index f0cef01..b18ed66 100644
--- a/qapi/event.json
+++ b/qapi/event.json
@@ -356,3 +356,17 @@
 ##
 { 'event': 'MEM_UNPLUG_ERROR',
   'data': { 'device': 'str', 'msg': 'str' } }
+
+##
+# @DUMP_COMPLETED
+#
+# Emitted when background dump has completed
+#
+# @error: #optional human-readable error string that provides
+#         hint on why dump failed. Only presents on failure. The
+#         user should not try to interpret the error string.
+#
+# Since: 2.6
+##
+{ 'event': 'DUMP_COMPLETED' ,
+  'data': { '*error': 'str' } }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 6b51585..7b6f915 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -857,8 +857,9 @@ Arguments:
 - "paging": do paging to get guest's memory mapping (json-bool)
 - "protocol": destination file(started with "file:") or destination file
               descriptor (started with "fd:") (json-string)
-- "detach": if specified, command will return immediately, without waiting
-            for the dump to finish (json-bool)
+- "detach": if specified, command will return immediately rather than waiting
+            for the dump completion. A DUMP_COMPLETED event will occur at
+            the end. (json-bool)
 - "begin": the starting physical address. It's optional, and should be specified
            with length together (json-int)
 - "length": the memory size, in bytes. It's optional, and should be specified
-- 
2.4.3

[Qemu-devel] [PATCH v5 09/11] DumpState: adding total_size and written_size fields

[Peter Xu]

Here, total_size is the size in bytes to be dumped (raw data, which
means before compression), while written_size are bytes handled (raw
size too).

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 dump.c                | 32 ++++++++++++++++++++++++++++++++
 include/sysemu/dump.h |  9 +++++++++
 2 files changed, 41 insertions(+)

diff --git a/dump.c b/dump.c
index 1a65fcf..ff543ae 100644
--- a/dump.c
+++ b/dump.c
@@ -332,6 +332,8 @@ static void write_data(DumpState *s, void *buf, int length, Error **errp)
     ret = fd_write_vmcore(buf, length, s);
     if (ret < 0) {
         error_setg(errp, "dump: failed to save memory");
+    } else {
+        s->written_size += length;
     }
 }
 
@@ -1301,6 +1303,7 @@ static void write_dump_pages(DumpState *s, Error **errp)
                 goto out;
             }
         }
+        s->written_size += TARGET_PAGE_SIZE;
     }
 
     ret = write_cache(&page_desc, NULL, 0, true);
@@ -1433,6 +1436,30 @@ bool dump_in_progress(void)
     return (state->status == DUMP_STATUS_ACTIVE);
 }
 
+/* calculate total size of memory to be dumped (taking filter into
+ * acoount.) */
+static int64_t dump_calculate_size(DumpState *s)
+{
+    GuestPhysBlock *block;
+    int64_t size = 0, total = 0, left = 0, right = 0;
+
+    QTAILQ_FOREACH(block, &s->guest_phys_blocks.head, next) {
+        if (s->has_filter) {
+            /* calculate the overlapped region. */
+            left = MAX(s->begin, block->target_start);
+            right = MIN(s->begin + s->length, block->target_end);
+            size = right - left;
+            size = size > 0 ? size : 0;
+        } else {
+            /* count the whole region in */
+            size = (block->target_end - block->target_start);
+        }
+        total += size;
+    }
+
+    return total;
+}
+
 static void dump_init(DumpState *s, int fd, bool has_format,
                       DumpGuestMemoryFormat format, bool paging, bool has_filter,
                       int64_t begin, int64_t length, Error **errp)
@@ -1444,6 +1471,7 @@ static void dump_init(DumpState *s, int fd, bool has_format,
 
     s->has_format = has_format;
     s->format = format;
+    s->written_size = 0;
 
     /* kdump-compressed is conflict with paging and filter */
     if (has_format && format != DUMP_GUEST_MEMORY_FORMAT_ELF) {
@@ -1475,6 +1503,10 @@ static void dump_init(DumpState *s, int fd, bool has_format,
 
     guest_phys_blocks_init(&s->guest_phys_blocks);
     guest_phys_blocks_append(&s->guest_phys_blocks);
+    s->total_size = dump_calculate_size(s);
+#ifdef DEBUG_DUMP_GUEST_MEMORY
+    fprintf(stderr, "DUMP: total memory to dump: %lu\n", s->total_size);
+#endif
 
     s->start = get_start_block(s);
     if (s->start == -1) {
diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
index 31930c6..ce262fa 100644
--- a/include/sysemu/dump.h
+++ b/include/sysemu/dump.h
@@ -189,6 +189,15 @@ typedef struct DumpState {
     bool has_format;              /* whether format is provided */
     DumpGuestMemoryFormat format; /* valid only if has_format == true */
     QemuThread dump_thread;       /* thread for detached dump */
+
+    int64_t total_size;          /* total memory size (in bytes) to
+                                  * be dumped. When filter is
+                                  * enabled, this will only count
+                                  * those to be written. */
+    int64_t written_size;        /* written memory size (in bytes),
+                                  * this could be used to calculate
+                                  * how much work we have
+                                  * finished. */
 } DumpState;
 
 uint16_t cpu_to_dump16(DumpState *s, uint16_t val);
-- 
2.4.3

[Qemu-devel] [PATCH v5 10/11] Dump: add qmp command "query-dump"

[Peter Xu]

When dump-guest-memory is requested with detach flag, after its
return, user could query its status using "query-dump" command (with
no argument). The result contains:

- status: current dump status
- completed: bytes written in the latest dump
- total: bytes to write in the latest dump

>From completed and total, we could know how much work
finished by calculating:

  100.0 * completed / total (%)

Also, enrich DUMP_COMPLETED event to contain dump results when
finished.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 docs/qmp-events.txt |  5 ++++-
 dump.c              | 28 ++++++++++++++++++++++++----
 qapi-schema.json    | 34 ++++++++++++++++++++++++++++++++--
 qapi/event.json     |  4 +++-
 qmp-commands.hx     | 29 +++++++++++++++++++++++++++--
 5 files changed, 90 insertions(+), 10 deletions(-)

diff --git a/docs/qmp-events.txt b/docs/qmp-events.txt
index 1f79588..60af2ab 100644
--- a/docs/qmp-events.txt
+++ b/docs/qmp-events.txt
@@ -227,6 +227,8 @@ Emitted when the guest has finished one memory dump.
 
 Data:
 
+- "result": DumpQueryResult type as described in qapi-schema.json
+
 - "error": Error message when dump failed. This is only a
   human-readable string provided when dump failed. It should not be
   parsed in any way (json-string, optional)
@@ -234,7 +236,8 @@ Data:
 Example:
 
 { "event": "DUMP_COMPLETED",
-  "data": {} }
+  "data": {"result": {"total": 1090650112, "status": "completed",
+                      "completed": 1090650112} } }
 
 GUEST_PANICKED
 --------------
diff --git a/dump.c b/dump.c
index ff543ae..c566a32 100644
--- a/dump.c
+++ b/dump.c
@@ -1433,7 +1433,7 @@ static void dump_state_prepare(DumpState *s)
 bool dump_in_progress(void)
 {
     DumpState *state = &dump_state_global;
-    return (state->status == DUMP_STATUS_ACTIVE);
+    return (atomic_read(&state->status) == DUMP_STATUS_ACTIVE);
 }
 
 /* calculate total size of memory to be dumped (taking filter into
@@ -1634,6 +1634,7 @@ cleanup:
 static void dump_process(DumpState *s, Error **errp)
 {
     Error *local_err = NULL;
+    DumpQueryResult *result = NULL;
 
     if (s->has_format && s->format != DUMP_GUEST_MEMORY_FORMAT_ELF) {
         create_kdump_vmcore(s, &local_err);
@@ -1641,12 +1642,19 @@ static void dump_process(DumpState *s, Error **errp)
         create_vmcore(s, &local_err);
     }
 
-    s->status = (local_err ? DUMP_STATUS_FAILED : DUMP_STATUS_COMPLETED);
+    /* make sure status is written after written_size updates */
+    smp_wmb();
+    atomic_set(&s->status,
+               (local_err ? DUMP_STATUS_FAILED : DUMP_STATUS_COMPLETED));
 
     /* send DUMP_COMPLETED message (unconditionally) */
-    qapi_event_send_dump_completed(!!local_err, (local_err ? \
+    result = qmp_query_dump(NULL);
+    /* should never fails */
+    assert(result);
+    qapi_event_send_dump_completed(result, !!local_err, (local_err ? \
                                    error_get_pretty(local_err) : NULL),
                                    &error_abort);
+    qapi_free_DumpQueryResult(result);
 
     error_propagate(errp, local_err);
     dump_cleanup(s);
@@ -1661,6 +1669,18 @@ static void *dump_thread(void *data)
     return NULL;
 }
 
+DumpQueryResult *qmp_query_dump(Error **errp)
+{
+    DumpQueryResult *result = g_new(DumpQueryResult, 1);
+    DumpState *state = &dump_state_global;
+    result->status = atomic_read(&state->status);
+    /* make sure we are reading status and written_size in order */
+    smp_rmb();
+    result->completed = state->written_size;
+    result->total = state->total_size;
+    return result;
+}
+
 void qmp_dump_guest_memory(bool paging, const char *file,
                            bool has_detach, bool detach,
                            bool has_begin, int64_t begin, bool has_length,
@@ -1751,7 +1771,7 @@ void qmp_dump_guest_memory(bool paging, const char *file,
               begin, length, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
-        s->status = DUMP_STATUS_FAILED;
+        atomic_set(&s->status, DUMP_STATUS_FAILED);
         return;
     }
 
diff --git a/qapi-schema.json b/qapi-schema.json
index f0d3c4a..71f6523 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2116,8 +2116,9 @@
 #               is the fd's name.
 #
 # @detach: #optional if true, QMP will return immediately rather than
-#          waiting for the dump to finish. A DUMP_COMPLETED event will
-#          occur at the end. (since 2.6).
+#          waiting for the dump to finish. The user can track progress
+#          using "query-dump". A DUMP_COMPLETED event will occur
+#          at the end. (since 2.6).
 #
 # @begin: #optional if specified, the starting physical address.
 #
@@ -2158,6 +2159,35 @@
   'data': [ 'none', 'active', 'completed', 'failed' ] }
 
 ##
+# @DumpQueryResult
+#
+# The result format for 'query-dump'.
+#
+# @status: enum of @DumpStatus, which shows current dump status
+#
+# @completed: bytes written in latest dump (uncompressed)
+#
+# @total: total bytes to be written in latest dump (uncompressed)
+#
+# Since 2.6
+##
+{ 'struct': 'DumpQueryResult',
+  'data': { 'status': 'DumpStatus',
+            'completed': 'int',
+            'total': 'int' } }
+
+##
+# @query-dump
+#
+# Query latest dump status.
+#
+# Returns: A @DumpStatus object showing the dump status.
+#
+# Since: 2.6
+##
+{ 'command': 'query-dump', 'returns': 'DumpQueryResult' }
+
+##
 # @DumpGuestMemoryCapability:
 #
 # A list of the available formats for dump-guest-memory
diff --git a/qapi/event.json b/qapi/event.json
index b18ed66..bf7dd61 100644
--- a/qapi/event.json
+++ b/qapi/event.json
@@ -362,6 +362,8 @@
 #
 # Emitted when background dump has completed
 #
+# @result: DumpQueryResult type described in qapi-schema.json.
+#
 # @error: #optional human-readable error string that provides
 #         hint on why dump failed. Only presents on failure. The
 #         user should not try to interpret the error string.
@@ -369,4 +371,4 @@
 # Since: 2.6
 ##
 { 'event': 'DUMP_COMPLETED' ,
-  'data': { '*error': 'str' } }
+  'data': { 'result': 'DumpQueryResult', '*error': 'str' } }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 7b6f915..eb5bfe2 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -858,8 +858,9 @@ Arguments:
 - "protocol": destination file(started with "file:") or destination file
               descriptor (started with "fd:") (json-string)
 - "detach": if specified, command will return immediately rather than waiting
-            for the dump completion. A DUMP_COMPLETED event will occur at
-            the end. (json-bool)
+            for the dump completion. The user can track progress using
+            "query-dump" A DUMP_COMPLETED event will occur at the
+            end. (json-bool)
 - "begin": the starting physical address. It's optional, and should be specified
            with length together (json-int)
 - "length": the memory size, in bytes. It's optional, and should be specified
@@ -899,6 +900,30 @@ Example:
 
 EQMP
 
+    {
+        .name       = "query-dump",
+        .args_type  = "",
+        .params     = "",
+        .help       = "query background dump status",
+        .mhandler.cmd_new = qmp_marshal_query_dump,
+    },
+
+SQMP
+query-dump
+----------
+
+Query background dump status.
+
+Arguments: None.
+
+Example:
+
+-> { "execute": "query-dump" }
+<- { "return": { "status": "active", "completed": 1024000,
+                 "total": 2048000 } }
+
+EQMP
+
 #if defined TARGET_S390X
     {
         .name       = "dump-skeys",
-- 
2.4.3

[Qemu-devel] [PATCH v5 11/11] Dump: add hmp command "info dump"

[Peter Xu]

It will calculate percentage of finished work from completed and
total.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hmp-commands-info.hx | 14 ++++++++++++++
 hmp.c                | 18 ++++++++++++++++++
 hmp.h                |  1 +
 3 files changed, 33 insertions(+)

diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
index 9b71351..52539c3 100644
--- a/hmp-commands-info.hx
+++ b/hmp-commands-info.hx
@@ -786,6 +786,20 @@ STEXI
 Display the value of a storage key (s390 only)
 ETEXI
 
+    {
+        .name       = "dump",
+        .args_type  = "",
+        .params     = "",
+        .help       = "Display the latest dump status",
+        .mhandler.cmd = hmp_info_dump,
+    },
+
+STEXI
+@item info dump
+@findex dump
+Display the latest dump status.
+ETEXI
+
 STEXI
 @end table
 ETEXI
diff --git a/hmp.c b/hmp.c
index 1f4d0b6..b001283 100644
--- a/hmp.c
+++ b/hmp.c
@@ -2383,3 +2383,21 @@ void hmp_rocker_of_dpa_groups(Monitor *mon, const QDict *qdict)
 
     qapi_free_RockerOfDpaGroupList(list);
 }
+
+void hmp_info_dump(Monitor *mon, const QDict *qdict)
+{
+    Error *not_used = NULL;
+    DumpQueryResult *result = qmp_query_dump(&not_used);
+
+    assert(result->status < DUMP_STATUS_MAX);
+    monitor_printf(mon, "Status: %s\n", DumpStatus_lookup[result->status]);
+
+    if (result->status == DUMP_STATUS_ACTIVE) {
+        float percent = 0;
+        assert(result->total != 0);
+        percent = 100.0 * result->completed / result->total;
+        monitor_printf(mon, "Finished: %.2f %%\n", percent);
+    }
+
+    qapi_free_DumpQueryResult(result);
+}
diff --git a/hmp.h b/hmp.h
index a8c5b5a..093d65f 100644
--- a/hmp.h
+++ b/hmp.h
@@ -131,5 +131,6 @@ void hmp_rocker(Monitor *mon, const QDict *qdict);
 void hmp_rocker_ports(Monitor *mon, const QDict *qdict);
 void hmp_rocker_of_dpa_flows(Monitor *mon, const QDict *qdict);
 void hmp_rocker_of_dpa_groups(Monitor *mon, const QDict *qdict);
+void hmp_info_dump(Monitor *mon, const QDict *qdict);
 
 #endif
-- 
2.4.3

Re: [Qemu-devel] [PATCH v5 06/11] dump-guest-memory: disable dump when in INMIGRATE state

[Fam Zheng]

On Mon, 12/07 13:56, Peter Xu wrote:
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  dump.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/dump.c b/dump.c
> index f0ee9a8..aa9d1f8 100644
> --- a/dump.c
> +++ b/dump.c
> @@ -1625,6 +1625,11 @@ void qmp_dump_guest_memory(bool paging, const char *file,
>      DumpState *s;
>      Error *local_err = NULL;
>  
> +    if (runstate_check(RUN_STATE_INMIGRATE)) {
> +        error_setg(errp, "Dump not allowed during incoming migration.");
> +        return;
> +    }
> +

Detached dump when "inmigrate" is disabled, that's OK.  But what about sync
dump? It used to be possible, but now is disabled. Just asking to make sure
this is the intention rather than oversight.

Fam

>      /* if there is a dump in background, we should wait until the dump
>       * finished */
>      if (dump_in_progress()) {
> -- 
> 2.4.3
> 

Re: [Qemu-devel] [PATCH v5 10/11] Dump: add qmp command "query-dump"

[Fam Zheng]

On Mon, 12/07 13:56, Peter Xu wrote:
> When dump-guest-memory is requested with detach flag, after its
> return, user could query its status using "query-dump" command (with
> no argument). The result contains:
> 
> - status: current dump status
> - completed: bytes written in the latest dump
> - total: bytes to write in the latest dump
> 
> From completed and total, we could know how much work
> finished by calculating:
> 
>   100.0 * completed / total (%)
> 
> Also, enrich DUMP_COMPLETED event to contain dump results when
> finished.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  docs/qmp-events.txt |  5 ++++-
>  dump.c              | 28 ++++++++++++++++++++++++----
>  qapi-schema.json    | 34 ++++++++++++++++++++++++++++++++--
>  qapi/event.json     |  4 +++-
>  qmp-commands.hx     | 29 +++++++++++++++++++++++++++--
>  5 files changed, 90 insertions(+), 10 deletions(-)
> 
> diff --git a/docs/qmp-events.txt b/docs/qmp-events.txt
> index 1f79588..60af2ab 100644
> --- a/docs/qmp-events.txt
> +++ b/docs/qmp-events.txt
> @@ -227,6 +227,8 @@ Emitted when the guest has finished one memory dump.
>  
>  Data:
>  
> +- "result": DumpQueryResult type as described in qapi-schema.json
> +
>  - "error": Error message when dump failed. This is only a
>    human-readable string provided when dump failed. It should not be
>    parsed in any way (json-string, optional)
> @@ -234,7 +236,8 @@ Data:
>  Example:
>  
>  { "event": "DUMP_COMPLETED",
> -  "data": {} }
> +  "data": {"result": {"total": 1090650112, "status": "completed",
> +                      "completed": 1090650112} } }
>  
>  GUEST_PANICKED
>  --------------
> diff --git a/dump.c b/dump.c
> index ff543ae..c566a32 100644
> --- a/dump.c
> +++ b/dump.c
> @@ -1433,7 +1433,7 @@ static void dump_state_prepare(DumpState *s)
>  bool dump_in_progress(void)
>  {
>      DumpState *state = &dump_state_global;
> -    return (state->status == DUMP_STATUS_ACTIVE);
> +    return (atomic_read(&state->status) == DUMP_STATUS_ACTIVE);
>  }
>  
>  /* calculate total size of memory to be dumped (taking filter into
> @@ -1634,6 +1634,7 @@ cleanup:
>  static void dump_process(DumpState *s, Error **errp)
>  {
>      Error *local_err = NULL;
> +    DumpQueryResult *result = NULL;
>  
>      if (s->has_format && s->format != DUMP_GUEST_MEMORY_FORMAT_ELF) {
>          create_kdump_vmcore(s, &local_err);
> @@ -1641,12 +1642,19 @@ static void dump_process(DumpState *s, Error **errp)
>          create_vmcore(s, &local_err);
>      }
>  
> -    s->status = (local_err ? DUMP_STATUS_FAILED : DUMP_STATUS_COMPLETED);
> +    /* make sure status is written after written_size updates */
> +    smp_wmb();
> +    atomic_set(&s->status,
> +               (local_err ? DUMP_STATUS_FAILED : DUMP_STATUS_COMPLETED));
>  
>      /* send DUMP_COMPLETED message (unconditionally) */
> -    qapi_event_send_dump_completed(!!local_err, (local_err ? \
> +    result = qmp_query_dump(NULL);
> +    /* should never fails */

s/fails/fail/

> +    assert(result);
> +    qapi_event_send_dump_completed(result, !!local_err, (local_err ? \
>                                     error_get_pretty(local_err) : NULL),
>                                     &error_abort);
> +    qapi_free_DumpQueryResult(result);
>  
>      error_propagate(errp, local_err);
>      dump_cleanup(s);
> @@ -1661,6 +1669,18 @@ static void *dump_thread(void *data)
>      return NULL;
>  }
>  
> +DumpQueryResult *qmp_query_dump(Error **errp)
> +{
> +    DumpQueryResult *result = g_new(DumpQueryResult, 1);
> +    DumpState *state = &dump_state_global;
> +    result->status = atomic_read(&state->status);
> +    /* make sure we are reading status and written_size in order */
> +    smp_rmb();
> +    result->completed = state->written_size;
> +    result->total = state->total_size;
> +    return result;
> +}
> +
>  void qmp_dump_guest_memory(bool paging, const char *file,
>                             bool has_detach, bool detach,
>                             bool has_begin, int64_t begin, bool has_length,
> @@ -1751,7 +1771,7 @@ void qmp_dump_guest_memory(bool paging, const char *file,
>                begin, length, &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
> -        s->status = DUMP_STATUS_FAILED;
> +        atomic_set(&s->status, DUMP_STATUS_FAILED);
>          return;
>      }
>  
> diff --git a/qapi-schema.json b/qapi-schema.json
> index f0d3c4a..71f6523 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -2116,8 +2116,9 @@
>  #               is the fd's name.
>  #
>  # @detach: #optional if true, QMP will return immediately rather than
> -#          waiting for the dump to finish. A DUMP_COMPLETED event will
> -#          occur at the end. (since 2.6).
> +#          waiting for the dump to finish. The user can track progress
> +#          using "query-dump". A DUMP_COMPLETED event will occur
> +#          at the end. (since 2.6).
>  #
>  # @begin: #optional if specified, the starting physical address.
>  #
> @@ -2158,6 +2159,35 @@
>    'data': [ 'none', 'active', 'completed', 'failed' ] }
>  
>  ##
> +# @DumpQueryResult
> +#
> +# The result format for 'query-dump'.
> +#
> +# @status: enum of @DumpStatus, which shows current dump status
> +#
> +# @completed: bytes written in latest dump (uncompressed)
> +#
> +# @total: total bytes to be written in latest dump (uncompressed)
> +#
> +# Since 2.6
> +##
> +{ 'struct': 'DumpQueryResult',
> +  'data': { 'status': 'DumpStatus',
> +            'completed': 'int',
> +            'total': 'int' } }
> +
> +##
> +# @query-dump
> +#
> +# Query latest dump status.
> +#
> +# Returns: A @DumpStatus object showing the dump status.
> +#
> +# Since: 2.6
> +##
> +{ 'command': 'query-dump', 'returns': 'DumpQueryResult' }
> +
> +##
>  # @DumpGuestMemoryCapability:
>  #
>  # A list of the available formats for dump-guest-memory
> diff --git a/qapi/event.json b/qapi/event.json
> index b18ed66..bf7dd61 100644
> --- a/qapi/event.json
> +++ b/qapi/event.json
> @@ -362,6 +362,8 @@
>  #
>  # Emitted when background dump has completed
>  #
> +# @result: DumpQueryResult type described in qapi-schema.json.
> +#
>  # @error: #optional human-readable error string that provides
>  #         hint on why dump failed. Only presents on failure. The
>  #         user should not try to interpret the error string.
> @@ -369,4 +371,4 @@
>  # Since: 2.6
>  ##
>  { 'event': 'DUMP_COMPLETED' ,
> -  'data': { '*error': 'str' } }
> +  'data': { 'result': 'DumpQueryResult', '*error': 'str' } }
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 7b6f915..eb5bfe2 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -858,8 +858,9 @@ Arguments:
>  - "protocol": destination file(started with "file:") or destination file
>                descriptor (started with "fd:") (json-string)
>  - "detach": if specified, command will return immediately rather than waiting
> -            for the dump completion. A DUMP_COMPLETED event will occur at
> -            the end. (json-bool)
> +            for the dump completion. The user can track progress using
> +            "query-dump" A DUMP_COMPLETED event will occur at the

Missing "." before "A DUMP_COMPLETED"?

> +            end. (json-bool)

Re: [Qemu-devel] [PATCH v5 00/11] Add basic "detach" support for dump-guest-memory

[Fam Zheng]

On Mon, 12/07 13:56, Peter Xu wrote:
> v5 changes:
> - patch 1
>   - comment English fix [Fam]
> - patch 2
>   - pass has_detach=true always in hmp_dump_guest_memory [Paolo]
> - patch 3
>   - always use local_err and error_propagate() when need to check
>     the result [Fam]
> - patch 8
>   - add "DumpQueryResult" in DUMP_COMPLETED event [Eric]
>     (since DumpQueryResult is introduced in patch 10, so doing it in
>     patch 10 for convenience. Please let me know if I should not do
>     this, e.g., if patch re-ordering is required)

Apart from the questions that I am not sure,

Reviewed-by: Fam Zheng <famz@redhat.com>

> 
> v4 changes:
> - patch 2:
>   - hmp: fix default value lost [Eric]
>   - English errors [Eric]
> - patch 3:
>   - use global DumpState, leverage C99 struct init [Paolo]
>   - English errors [Eric]
> - patch 5:
>   - more cleanup for dump_process [Paolo]
> - patch 8:
>   - make sure qmp-events.txt is sorted [Eric]
>   - enhance error_get_pretty() [Eric]
>   - emit DUMP_COMPLETED no matter detach or not
> - patch 10:
>   - use g_new0 to replace g_malloc0 [Eric]
>   - rename "written_bytes" to "completed", "total_bytes" to "total"
>     [Eric]
>   - use atomic ops and [rw]mb to protect status read/write [Paolo]
> - patch 12:
>   - English errors [Eric]
>   - merge contents into older patches [Eric]
> 
> v3 changes (patch number corresponds to v2 patch set):
> - patch 1
>   - fix commit message. no memory leak, only code cleanup [Fam]
> - patch 2
>   - better documentation for "dump-guest-memory" (new patch 9) [Fam]
> - patch 3
>   - remove rcu lock/unlock in dump_init() [Fam, Paolo]
>   - embed mr pointer into GuestPhysBlock [Paolo]
>   - remove global dump state [Paolo]
> - patch 4
>   - fix memory leak for error [Fam]
>   - evt DUMP_COMPLETED data: change to an optional "*error" [Paolo]
> - patch 5
>   - fix documents [Fam]
>   - change "dump-query" to "query-dump", HMP to "info dump" [Paolo]
> - patch 6
>   - for query-dump command: define enum for DumpStatus, use "int"
>     for written/total [Paolo]
> - all
>   - reorder the commits as suggested, no fake values [Paolo]
>   - split big commit into smaller ones [me]
> 
> v2 changes:
> - fixed English errors [Drew]
> - reordered the "detach" field, first make it optional, then make sure
>   it's order is consistent [Drew, Fam]
> - added doc for new detach flag [Eric]
> - collected error msg even detached [Drew]
> - added qmp event DUMP_COMPLETED to notify user [Eric, Fam]
> - added "dump-query" QMP & HMP commands to query dump status [Eric]
> - "stop" is not allowed when dump in background (also include
>   "cont" and "dump-guest-memory") [Fam]
> - added codes to calculate how many dump work finished, which could
>   be queried from "dump-query" [Laszlo]
> - added list to track all used MemoryRegion objects, also ref before
>   use [Paolo]
> - dump-guest-memory will be forbidden during incoming migrate [Paolo]
> - taking rcu lock when collecting memory info [Paolo]
> 
> Test Done:
> - QMP & HMP
>   - test default dump (sync), work as usual
>   - test detached dump, command return immediately.
>   - When dump finished, will receive event DUMP_COMPLETED. 
>   - test query-dump before/during/after dump
>   - test kdump with zlib compression, w/ and w/o detach
> - libvirt
>   - test "virsh dump --memory-only" with default format and
>     kdump-zlib format, work as usual
> 
> Peter Xu (11):
>   dump-guest-memory: cleanup: removing dump_{error|cleanup}().
>   dump-guest-memory: add "detach" flag for QMP/HMP interfaces.
>   dump-guest-memory: using static DumpState, add DumpStatus
>   dump-guest-memory: add dump_in_progress() helper function
>   dump-guest-memory: introduce dump_process() helper function.
>   dump-guest-memory: disable dump when in INMIGRATE state
>   dump-guest-memory: add "detach" support
>   dump-guest-memory: add qmp event DUMP_COMPLETED
>   DumpState: adding total_size and written_size fields
>   Dump: add qmp command "query-dump"
>   Dump: add hmp command "info dump"
> 
>  docs/qmp-events.txt             |  19 ++++
>  dump.c                          | 215 ++++++++++++++++++++++++++++++----------
>  hmp-commands-info.hx            |  14 +++
>  hmp-commands.hx                 |   5 +-
>  hmp.c                           |  27 ++++-
>  hmp.h                           |   1 +
>  include/qemu-common.h           |   4 +
>  include/sysemu/dump.h           |  15 +++
>  include/sysemu/memory_mapping.h |   4 +
>  memory_mapping.c                |   3 +
>  qapi-schema.json                |  57 ++++++++++-
>  qapi/event.json                 |  16 +++
>  qmp-commands.hx                 |  32 +++++-
>  qmp.c                           |  14 +++
>  14 files changed, 363 insertions(+), 63 deletions(-)
> 
> -- 
> 2.4.3
> 

Re: [Qemu-devel] [PATCH v5 06/11] dump-guest-memory: disable dump when in INMIGRATE state

[Peter Xu]

On Mon, Dec 07, 2015 at 02:14:11PM +0800, Fam Zheng wrote:
> On Mon, 12/07 13:56, Peter Xu wrote:
> > +    if (runstate_check(RUN_STATE_INMIGRATE)) {
> > +        error_setg(errp, "Dump not allowed during incoming migration.");
> > +        return;
> > +    }
> > +
> 
> Detached dump when "inmigrate" is disabled, that's OK.  But what about sync
> dump? It used to be possible, but now is disabled. Just asking to make sure
> this is the intention rather than oversight.

That's my intention here. This should be an idea from one of Paolo's
review comment (forgot which one) and I hope I did not miss
anything. I took this since I failed to find a use case that user
might dump guest memory during incoming migration.

Thanks.
Peter

> 
> Fam

Re: [Qemu-devel] [PATCH v5 10/11] Dump: add qmp command "query-dump"

[Peter Xu]

On Mon, Dec 07, 2015 at 02:21:44PM +0800, Fam Zheng wrote:
> On Mon, 12/07 13:56, Peter Xu wrote:
> >      /* send DUMP_COMPLETED message (unconditionally) */
> > -    qapi_event_send_dump_completed(!!local_err, (local_err ? \
> > +    result = qmp_query_dump(NULL);
> > +    /* should never fails */
> 
> s/fails/fail/

Got.

> 
> > diff --git a/qmp-commands.hx b/qmp-commands.hx
> > index 7b6f915..eb5bfe2 100644
> > --- a/qmp-commands.hx
> > +++ b/qmp-commands.hx
> > @@ -858,8 +858,9 @@ Arguments:
> >  - "protocol": destination file(started with "file:") or destination file
> >                descriptor (started with "fd:") (json-string)
> >  - "detach": if specified, command will return immediately rather than waiting
> > -            for the dump completion. A DUMP_COMPLETED event will occur at
> > -            the end. (json-bool)
> > +            for the dump completion. The user can track progress using
> > +            "query-dump" A DUMP_COMPLETED event will occur at the
> 
> Missing "." before "A DUMP_COMPLETED"?

Possibly... Changing both. Thanks!

Peter

> 
> > +            end. (json-bool)

Re: [Qemu-devel] [PATCH v5 00/11] Add basic "detach" support for dump-guest-memory

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1481 bytes --]

On 12/06/2015 10:56 PM, Peter Xu wrote:
> v5 changes:
> - patch 1
>   - comment English fix [Fam]
> - patch 2
>   - pass has_detach=true always in hmp_dump_guest_memory [Paolo]
> - patch 3
>   - always use local_err and error_propagate() when need to check
>     the result [Fam]
> - patch 8
>   - add "DumpQueryResult" in DUMP_COMPLETED event [Eric]
>     (since DumpQueryResult is introduced in patch 10, so doing it in
>     patch 10 for convenience. Please let me know if I should not do
>     this, e.g., if patch re-ordering is required)

All patches should build in isolation.  It looks like you met that goal
(you introduce 'DUMP_COMPLETED' event in 8 without a 'result' member,
then modify it in 10), so that it at least builds.  But it results in
churn, in that you have multiple different definitions of
'DUMP_COMPLETED' over the life of the series.

It's not a requirement to rework things since each step builds, but if I
were writing the series, I do find it conceptually easier to supply
patches in an order that minimizes churn (the first patch that
introduces a type uses its final form, rather than going through several
iterations of that type).  So on that grounds, introducing
DumpQueryResult as a separate patch, before either DUMP_COMPLETED or
query-dump, might be easier to review, if there is a reason for a v6 spin.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v5 00/11] Add basic "detach" support for dump-guest-memory

[Peter Xu]

On Mon, Dec 07, 2015 at 03:13:02PM -0700, Eric Blake wrote:
> On 12/06/2015 10:56 PM, Peter Xu wrote:
> > - patch 8
> >   - add "DumpQueryResult" in DUMP_COMPLETED event [Eric]
> >     (since DumpQueryResult is introduced in patch 10, so doing it in
> >     patch 10 for convenience. Please let me know if I should not do
> >     this, e.g., if patch re-ordering is required)
> 
> All patches should build in isolation.  It looks like you met that goal
> (you introduce 'DUMP_COMPLETED' event in 8 without a 'result' member,
> then modify it in 10), so that it at least builds.  But it results in
> churn, in that you have multiple different definitions of
> 'DUMP_COMPLETED' over the life of the series.
> 
> It's not a requirement to rework things since each step builds, but if I
> were writing the series, I do find it conceptually easier to supply
> patches in an order that minimizes churn (the first patch that
> introduces a type uses its final form, rather than going through several
> iterations of that type).  So on that grounds, introducing
> DumpQueryResult as a separate patch, before either DUMP_COMPLETED or
> query-dump, might be easier to review, if there is a reason for a v6 spin.

Yes, it's harder for review. Sorry for that.

I think there should have a v6 spin, I will put DUMP_COMPLETE patch
to the end of the patch set.

Thanks.
Peter