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

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

[Peter Xu]

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             |  16 ++++
 dump.c                          | 204 +++++++++++++++++++++++++++++-----------
 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                 |  13 +++
 qmp-commands.hx                 |  34 ++++++-
 qmp.c                           |  14 +++
 util/error.c                    |   6 +-
 15 files changed, 352 insertions(+), 65 deletions(-)

-- 
2.4.3

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

[Peter Xu]

It might be a little bit confusing to do dump_cleanup() in these two
functions and error prone. 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>
---
 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 v4 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..7748627 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, has_detach, 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 v4 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                | 20 +++++++++++++++++---
 include/sysemu/dump.h |  2 ++
 qapi-schema.json      | 18 ++++++++++++++++++
 3 files changed, 37 insertions(+), 3 deletions(-)

diff --git a/dump.c b/dump.c
index d79e0ed..f5f3c59 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,13 +1655,14 @@ 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;
     }
 
@@ -1663,8 +1672,13 @@ void qmp_dump_guest_memory(bool paging, const char *file,
         create_vmcore(s, errp);
     }
 
+    if (*errp) {
+        s->status = DUMP_STATUS_FAILED;
+    } else {
+        s->status = DUMP_STATUS_COMPLETED;
+    }
+
     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 v4 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 f5f3c59..3cf75db 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 v4 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                | 35 ++++++++++++++++++++++-------------
 include/sysemu/dump.h |  3 +++
 2 files changed, 25 insertions(+), 13 deletions(-)

diff --git a/dump.c b/dump.c
index 3cf75db..e46749d 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,24 @@ cleanup:
     dump_cleanup(s);
 }
 
+/* this operation might be time consuming. */
+static void dump_process(DumpState *s, Error **errp)
+{
+    if (s->has_format && s->format != DUMP_GUEST_MEMORY_FORMAT_ELF) {
+        create_kdump_vmcore(s, errp);
+    } else {
+        create_vmcore(s, errp);
+    }
+
+    if (*errp) {
+        s->status = DUMP_STATUS_FAILED;
+    } else {
+        s->status = DUMP_STATUS_COMPLETED;
+    }
+
+    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,19 +1700,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, errp);
-    } else {
-        create_vmcore(s, errp);
-    }
-
-    if (*errp) {
-        s->status = DUMP_STATUS_FAILED;
-    } else {
-        s->status = DUMP_STATUS_COMPLETED;
-    }
-
-    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 v4 06/11] dump-guest-memory: disable dump when in INMIGRATE state

[Peter Xu]

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

diff --git a/dump.c b/dump.c
index e46749d..65d1f7e 100644
--- a/dump.c
+++ b/dump.c
@@ -1626,13 +1626,17 @@ 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()) {
         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.
-- 
2.4.3

[Qemu-devel] [PATCH v4 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 65d1f7e..c86bc2d 100644
--- a/dump.c
+++ b/dump.c
@@ -1615,6 +1615,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,
@@ -1625,6 +1639,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 v4 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              | 11 +++++------
 qapi-schema.json    |  3 ++-
 qapi/event.json     | 13 +++++++++++++
 qmp-commands.hx     |  5 +++--
 util/error.c        |  6 +++++-
 6 files changed, 44 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 c86bc2d..5b040b7 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
@@ -1612,6 +1613,9 @@ static void dump_process(DumpState *s, Error **errp)
         s->status = DUMP_STATUS_COMPLETED;
     }
 
+    /* send DUMP_COMPLETED message (unconditionally) */
+    qapi_event_send_dump_completed(!!(*errp), error_get_pretty(*errp),
+                                   &error_abort);
     dump_cleanup(s);
 }
 
@@ -1619,13 +1623,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..9b7f714 100644
--- a/qapi/event.json
+++ b/qapi/event.json
@@ -356,3 +356,16 @@
 ##
 { '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.
+#
+# 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
diff --git a/util/error.c b/util/error.c
index 80c89a2..645b9af 100644
--- a/util/error.c
+++ b/util/error.c
@@ -197,7 +197,11 @@ ErrorClass error_get_class(const Error *err)
 
 const char *error_get_pretty(Error *err)
 {
-    return err->msg;
+    if (err) {
+        return err->msg;
+    } else {
+        return NULL;
+    }
 }
 
 void error_report_err(Error *err)
-- 
2.4.3

[Qemu-devel] [PATCH v4 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 5b040b7..daa1f2c 100644
--- a/dump.c
+++ b/dump.c
@@ -333,6 +333,8 @@ static void write_data(DumpState *s, void *buf, int length, Error **errp)
     if (ret < 0) {
         error_setg(errp, "dump: failed to save memory");
     }
+
+    s->written_size += length;
 }
 
 /* write the memory to vmcore. 1 page per I/O. */
@@ -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 size_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..9c5a46b 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 */
+
+    size_t total_size;          /* total memory size (in bytes) to
+                                 * be dumped. When filter is
+                                 * enabled, this will only count
+                                 * those to be written. */
+    size_t written_size;        /* written memory size (in bytes),
+                                 * this could be used to calculate
+                                 * how many work we have
+                                 * finished. */
 } DumpState;
 
 uint16_t cpu_to_dump16(DumpState *s, uint16_t val);
-- 
2.4.3

[Qemu-devel] [PATCH v4 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 (%)

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 dump.c           | 23 +++++++++++++++++------
 qapi-schema.json | 34 ++++++++++++++++++++++++++++++++--
 qmp-commands.hx  | 31 ++++++++++++++++++++++++++++---
 3 files changed, 77 insertions(+), 11 deletions(-)

diff --git a/dump.c b/dump.c
index daa1f2c..00619c6 100644
--- a/dump.c
+++ b/dump.c
@@ -1639,11 +1639,10 @@ static void dump_process(DumpState *s, Error **errp)
         create_vmcore(s, errp);
     }
 
-    if (*errp) {
-        s->status = DUMP_STATUS_FAILED;
-    } else {
-        s->status = DUMP_STATUS_COMPLETED;
-    }
+    /* make sure status is written after written_size updates */
+    smp_wmb();
+    atomic_set(&s->status,
+               ((*errp) ? DUMP_STATUS_FAILED : DUMP_STATUS_COMPLETED));
 
     /* send DUMP_COMPLETED message (unconditionally) */
     qapi_event_send_dump_completed(!!(*errp), error_get_pretty(*errp),
@@ -1660,6 +1659,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,
@@ -1749,7 +1760,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/qmp-commands.hx b/qmp-commands.hx
index 7b6f915..5741c0c 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
@@ -882,7 +883,7 @@ EQMP
     {
         .name       = "query-dump-guest-memory-capability",
         .args_type  = "",
-    .mhandler.cmd_new = qmp_marshal_query_dump_guest_memory_capability,
+        .mhandler.cmd_new = qmp_marshal_query_dump_guest_memory_capability,
     },
 
 SQMP
@@ -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 v4 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 7748627..6190216 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 v4 01/11] dump-guest-memory: cleanup: removing dump_{error|cleanup}().

[Fam Zheng]

On Tue, 12/01 21:28, Peter Xu wrote:
> It might be a little bit confusing to do dump_cleanup() in these two
> functions and error prone. A better way is to do dump_cleanup()

I would say "It might be a little bit confusing and error prone to do
dump_cleanup() in ..."

Other than that,

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

> before dump finish, no matter whether dump has succeeded or not.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>

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

[Fam Zheng]

On Tue, 12/01 21:28, Peter Xu wrote:
> 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                | 20 +++++++++++++++++---
>  include/sysemu/dump.h |  2 ++
>  qapi-schema.json      | 18 ++++++++++++++++++
>  3 files changed, 37 insertions(+), 3 deletions(-)
> 
> diff --git a/dump.c b/dump.c
> index d79e0ed..f5f3c59 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,13 +1655,14 @@ 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;
>      }
>  
> @@ -1663,8 +1672,13 @@ void qmp_dump_guest_memory(bool paging, const char *file,
>          create_vmcore(s, errp);
>      }
>  
> +    if (*errp) {
> +        s->status = DUMP_STATUS_FAILED;
> +    } else {
> +        s->status = DUMP_STATUS_COMPLETED;
> +    }
> +

To detect error, it's better to use local_err plus error_propagate like a few
lines above. errp _can_ be NULL depending on callers, though in practice qmp
functions should get a non-NULL.

Fam

>      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
> 

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

[Fam Zheng]

On Tue, 12/01 21:28, Peter Xu wrote:
> No functional change. Cleanup only.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  dump.c                | 35 ++++++++++++++++++++++-------------
>  include/sysemu/dump.h |  3 +++
>  2 files changed, 25 insertions(+), 13 deletions(-)
> 
> diff --git a/dump.c b/dump.c
> index 3cf75db..e46749d 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,24 @@ cleanup:
>      dump_cleanup(s);
>  }
>  
> +/* this operation might be time consuming. */
> +static void dump_process(DumpState *s, Error **errp)
> +{
> +    if (s->has_format && s->format != DUMP_GUEST_MEMORY_FORMAT_ELF) {
> +        create_kdump_vmcore(s, errp);
> +    } else {
> +        create_vmcore(s, errp);
> +    }
> +
> +    if (*errp) {
> +        s->status = DUMP_STATUS_FAILED;
> +    } else {
> +        s->status = DUMP_STATUS_COMPLETED;
> +    }

As in patch 3, this should use local_err + error_propagate.

> +
> +    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,19 +1700,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, errp);
> -    } else {
> -        create_vmcore(s, errp);
> -    }
> -
> -    if (*errp) {
> -        s->status = DUMP_STATUS_FAILED;
> -    } else {
> -        s->status = DUMP_STATUS_COMPLETED;
> -    }
> -
> -    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
> 

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

[Fam Zheng]

On Tue, 12/01 21:28, Peter Xu wrote:
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  dump.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/dump.c b/dump.c
> index e46749d..65d1f7e 100644
> --- a/dump.c
> +++ b/dump.c
> @@ -1626,13 +1626,17 @@ 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()) {
>          error_setg(errp, "There is a dump in process, please wait.");
>          return;
>      }
> -

Blank line change, please drop.

Fam

>      /*
>       * kdump-compressed format need the whole memory dumped, so paging or
>       * filter is not supported here.
> -- 
> 2.4.3
> 

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

[Fam Zheng]

On Tue, 12/01 21:28, Peter Xu wrote:
> 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              | 11 +++++------
>  qapi-schema.json    |  3 ++-
>  qapi/event.json     | 13 +++++++++++++
>  qmp-commands.hx     |  5 +++--
>  util/error.c        |  6 +++++-
>  6 files changed, 44 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 c86bc2d..5b040b7 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
> @@ -1612,6 +1613,9 @@ static void dump_process(DumpState *s, Error **errp)
>          s->status = DUMP_STATUS_COMPLETED;
>      }
>  
> +    /* send DUMP_COMPLETED message (unconditionally) */
> +    qapi_event_send_dump_completed(!!(*errp), error_get_pretty(*errp),
> +                                   &error_abort);
>      dump_cleanup(s);
>  }
>  
> @@ -1619,13 +1623,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..9b7f714 100644
> --- a/qapi/event.json
> +++ b/qapi/event.json
> @@ -356,3 +356,16 @@
>  ##
>  { '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.

Please explicitly mention that successful dump emits DUMP_COMPLETED without
error, and failed dump emits DUMP_COMPLETED that has an error str.

> +#
> +# 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
> diff --git a/util/error.c b/util/error.c
> index 80c89a2..645b9af 100644
> --- a/util/error.c
> +++ b/util/error.c
> @@ -197,7 +197,11 @@ ErrorClass error_get_class(const Error *err)
>  
>  const char *error_get_pretty(Error *err)
>  {
> -    return err->msg;
> +    if (err) {
> +        return err->msg;
> +    } else {
> +        return NULL;
> +    }

This change belongs to a separate patch, if any. But personally I don't like
it, because it doesn't work very well when error_get_pretty is used in
printf-like function parameters:

    Error *err = NULL;
    error_report("error: %s", error_get_pretty(err));

will print "error: (null)" which is ugly, in which case the caller need to
check the pointer anyway. And that is the dominant use case for
error_get_pretty in the code base.

IMO the caller can always do this:

    err ? error_get_pretty(err) : NULL

in place of your proposed

    error_get_pretty(err)

So maybe leave this and change dump_process like above? Or if you insist, make
this hunk a separate patch please.

Fam

>  }
>  
>  void error_report_err(Error *err)
> -- 
> 2.4.3
> 

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

[Fam Zheng]

On Tue, 12/01 21:28, Peter Xu wrote:
> 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 5b040b7..daa1f2c 100644
> --- a/dump.c
> +++ b/dump.c
> @@ -333,6 +333,8 @@ static void write_data(DumpState *s, void *buf, int length, Error **errp)
>      if (ret < 0) {
>          error_setg(errp, "dump: failed to save memory");
>      }
> +
> +    s->written_size += length;

If (ret < 0), the incremental is inaccurate, do we want an accurate
written_size in that case? I.e. put this in "else" branch?

>  }
>  
>  /* write the memory to vmcore. 1 page per I/O. */
> @@ -1301,6 +1303,7 @@ static void write_dump_pages(DumpState *s, Error **errp)
>                  goto out;
>              }
>          }
> +        s->written_size += TARGET_PAGE_SIZE;

The same question applies here.

>      }
>  
>      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 size_t dump_calculate_size(DumpState *s)

Is size_t big enough for 64 bit guest on 32 bit host (with 4 bytes size_t)?

> +{
> +    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..9c5a46b 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 */
> +
> +    size_t total_size;          /* total memory size (in bytes) to
> +                                 * be dumped. When filter is
> +                                 * enabled, this will only count
> +                                 * those to be written. */
> +    size_t written_size;        /* written memory size (in bytes),
> +                                 * this could be used to calculate
> +                                 * how many work we have

s/many/much/

> +                                 * finished. */
>  } DumpState;
>  
>  uint16_t cpu_to_dump16(DumpState *s, uint16_t val);
> -- 
> 2.4.3
> 

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

[Peter Xu]

On Wed, Dec 02, 2015 at 08:37:36AM +0800, Fam Zheng wrote:
> On Tue, 12/01 21:28, Peter Xu wrote:
> > It might be a little bit confusing to do dump_cleanup() in these two
> > functions and error prone. A better way is to do dump_cleanup()
> 
> I would say "It might be a little bit confusing and error prone to do
> dump_cleanup() in ..."

Yes. Changing it.

Thanks.
Peter

> 
> Other than that,
> 
> Reviewed-by: Fam Zheng <famz@redhat.com>
> 
> > before dump finish, no matter whether dump has succeeded or not.
> > 
> > Signed-off-by: Peter Xu <peterx@redhat.com>

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

[Peter Xu]

On Wed, Dec 02, 2015 at 08:46:49AM +0800, Fam Zheng wrote:
> On Tue, 12/01 21:28, Peter Xu wrote:
> > +    if (*errp) {
> > +        s->status = DUMP_STATUS_FAILED;
> > +    } else {
> > +        s->status = DUMP_STATUS_COMPLETED;
> > +    }
> > +
> 
> To detect error, it's better to use local_err plus error_propagate like a few
> lines above. errp _can_ be NULL depending on callers, though in practice qmp
> functions should get a non-NULL.

Yes, you are right. I will make sure all the error handlings in the
patch set are using local error variables, and call
error_propagate() afterward.

Thanks.
Peter

> > Fam

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

[Peter Xu]

On Wed, Dec 02, 2015 at 08:50:48AM +0800, Fam Zheng wrote:
> On Tue, 12/01 21:28, Peter Xu wrote:
> >      /* 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;
> >      }
> > -
> 
> Blank line change, please drop.

Ok.

Thanks.
Peter

> 
> Fam

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

[Peter Xu]

On Wed, Dec 02, 2015 at 09:11:31AM +0800, Fam Zheng wrote:
> On Tue, 12/01 21:28, Peter Xu wrote:
> > +
> > +##
> > +# @DUMP_COMPLETED
> > +#
> > +# Emitted when background dump has completed
> > +#
> > +# @error: #optional human-readable error string that provides
> > +#         hint on why dump failed.
> 
> Please explicitly mention that successful dump emits DUMP_COMPLETED without
> error, and failed dump emits DUMP_COMPLETED that has an error str.

Ok. I can add more words to describe it. Maybe something like:

# @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.

How do you think about this one?

IMHO, the "#optional" and the name "error" itself is clear enough
though, to show that it will be there only if error happens.

> 
> >  const char *error_get_pretty(Error *err)
> >  {
> > -    return err->msg;
> > +    if (err) {
> > +        return err->msg;
> > +    } else {
> > +        return NULL;
> > +    }
> 
> This change belongs to a separate patch, if any. But personally I don't like
> it, because it doesn't work very well when error_get_pretty is used in
> printf-like function parameters:
> 
>     Error *err = NULL;
>     error_report("error: %s", error_get_pretty(err));
> 
> will print "error: (null)" which is ugly, in which case the caller need to
> check the pointer anyway. And that is the dominant use case for
> error_get_pretty in the code base.
> 
> IMO the caller can always do this:
> 
>     err ? error_get_pretty(err) : NULL
> 
> in place of your proposed
> 
>     error_get_pretty(err)
> 
> So maybe leave this and change dump_process like above? Or if you insist, make
> this hunk a separate patch please.

I think both should work as long as the modification is backward
compatible and without performance drop (at least, it is
safer). Whatever, since this is the first patch from me, I'd like to
take your advice to avoid modifying shared codes. :)

Thanks.
Peter

> 
> Fam
> 
> >  }
> >  
> >  void error_report_err(Error *err)
> > -- 
> > 2.4.3
> > 

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

[Peter Xu]

On Wed, Dec 02, 2015 at 09:32:57AM +0800, Fam Zheng wrote:
> On Tue, 12/01 21:28, Peter Xu wrote:
> > @@ -333,6 +333,8 @@ static void write_data(DumpState *s, void *buf, int length, Error **errp)
> >      if (ret < 0) {
> >          error_setg(errp, "dump: failed to save memory");
> >      }
> > +
> > +    s->written_size += length;
> 
> If (ret < 0), the incremental is inaccurate, do we want an accurate
> written_size in that case? I.e. put this in "else" branch?

Yes, I think I'd better put it into an else block... Thanks.

Actually even that could not be accurate, since the write() could be
partly done and failed. But I hope that's good enough for dump
status queries. :)

> 
> >  }
> >  
> >  /* write the memory to vmcore. 1 page per I/O. */
> > @@ -1301,6 +1303,7 @@ static void write_dump_pages(DumpState *s, Error **errp)
> >                  goto out;
> >              }
> >          }
> > +        s->written_size += TARGET_PAGE_SIZE;
> 
> The same question applies here.

For kdump case, it is using "goto out" when error happens. So it
seems to have no problem here?

> 
> >      }
> >  
> >      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 size_t dump_calculate_size(DumpState *s)
> 
> Is size_t big enough for 64 bit guest on 32 bit host (with 4 bytes size_t)?

Thanks to point out. Will use int64_t here (and also written_size
and total_size).

> > +    size_t written_size;        /* written memory size (in bytes),
> > +                                 * this could be used to calculate
> > +                                 * how many work we have
> 
> s/many/much/

Will fix it. Thanks!

Peter

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

[Fam Zheng]

On Wed, 12/02 16:49, Peter Xu wrote:
> On Wed, Dec 02, 2015 at 09:32:57AM +0800, Fam Zheng wrote:
> > > @@ -1301,6 +1303,7 @@ static void write_dump_pages(DumpState *s, Error **errp)
> > >                  goto out;
> > >              }
> > >          }
> > > +        s->written_size += TARGET_PAGE_SIZE;
> > 
> > The same question applies here.
> 
> For kdump case, it is using "goto out" when error happens. So it
> seems to have no problem here?

write_cache can fail after you increment it here, no?

Fam

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

[Fam Zheng]

On Wed, 12/02 16:20, Peter Xu wrote:
> On Wed, Dec 02, 2015 at 09:11:31AM +0800, Fam Zheng wrote:
> > On Tue, 12/01 21:28, Peter Xu wrote:
> > > +
> > > +##
> > > +# @DUMP_COMPLETED
> > > +#
> > > +# Emitted when background dump has completed
> > > +#
> > > +# @error: #optional human-readable error string that provides
> > > +#         hint on why dump failed.
> > 
> > Please explicitly mention that successful dump emits DUMP_COMPLETED without
> > error, and failed dump emits DUMP_COMPLETED that has an error str.
> 
> Ok. I can add more words to describe it. Maybe something like:
> 
> # @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.
> 
> How do you think about this one?

That looks ok.

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

[Peter Xu]

On Wed, Dec 02, 2015 at 05:49:18PM +0800, Fam Zheng wrote:
> On Wed, 12/02 16:49, Peter Xu wrote:
> > On Wed, Dec 02, 2015 at 09:32:57AM +0800, Fam Zheng wrote:
> > > > @@ -1301,6 +1303,7 @@ static void write_dump_pages(DumpState *s, Error **errp)
> > > >                  goto out;
> > > >              }
> > > >          }
> > > > +        s->written_size += TARGET_PAGE_SIZE;
> > > 
> > > The same question applies here.
> > 
> > For kdump case, it is using "goto out" when error happens. So it
> > seems to have no problem here?
> 
> write_cache can fail after you increment it here, no?

I am adding it at the end of loop. It looks like:

    while (get_next_page(&block_iter, &pfn_iter, &buf, s)) {
        if (is_zero_page(buf, TARGET_PAGE_SIZE)) {
            ret = write_cache(&page_desc, &pd_zero, sizeof(PageDescriptor),
                              false);
            if (ret < 0) {
                error_setg(errp, "XXXXXXXX");
                goto out;
            }
        } else {
		    ...
            ret = write_cache(&page_desc, &pd, sizeof(PageDescriptor), false);
            if (ret < 0) {
                error_setg(errp, "XXXXXXXX");
                goto out;
            }
			...
        }
        s->written_size += TARGET_PAGE_SIZE;
    }

Label "out" is out of the loop. So, when error happens, it sets the
errp and directly jump out of the loop. Did I miss anything?

Thanks!
Peter

> 
> Fam

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

[Fam Zheng]

On Wed, 12/02 18:41, Peter Xu wrote:
> On Wed, Dec 02, 2015 at 05:49:18PM +0800, Fam Zheng wrote:
> > On Wed, 12/02 16:49, Peter Xu wrote:
> > > On Wed, Dec 02, 2015 at 09:32:57AM +0800, Fam Zheng wrote:
> > > > > @@ -1301,6 +1303,7 @@ static void write_dump_pages(DumpState *s, Error **errp)
> > > > >                  goto out;
> > > > >              }
> > > > >          }
> > > > > +        s->written_size += TARGET_PAGE_SIZE;
> > > > 
> > > > The same question applies here.
> > > 
> > > For kdump case, it is using "goto out" when error happens. So it
> > > seems to have no problem here?
> > 
> > write_cache can fail after you increment it here, no?
> 
> I am adding it at the end of loop. It looks like:
> 
>     while (get_next_page(&block_iter, &pfn_iter, &buf, s)) {
>         if (is_zero_page(buf, TARGET_PAGE_SIZE)) {
>             ret = write_cache(&page_desc, &pd_zero, sizeof(PageDescriptor),
>                               false);
>             if (ret < 0) {
>                 error_setg(errp, "XXXXXXXX");
>                 goto out;
>             }
>         } else {
> 		    ...
>             ret = write_cache(&page_desc, &pd, sizeof(PageDescriptor), false);
>             if (ret < 0) {
>                 error_setg(errp, "XXXXXXXX");
>                 goto out;
>             }
> 			...
>         }
>         s->written_size += TARGET_PAGE_SIZE;
>     }
> 
> Label "out" is out of the loop. So, when error happens, it sets the
> errp and directly jump out of the loop. Did I miss anything?
> 

You are right, I misread the context of this incremental. Thanks.

Fam

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

[Peter Xu]

On Wed, Dec 02, 2015 at 08:51:48PM +0800, Fam Zheng wrote:
> On Wed, 12/02 18:41, Peter Xu wrote:
> > On Wed, Dec 02, 2015 at 05:49:18PM +0800, Fam Zheng wrote:
> > Label "out" is out of the loop. So, when error happens, it sets the
> > errp and directly jump out of the loop. Did I miss anything?
> > 
> 
> You are right, I misread the context of this incremental. Thanks.

Thanks for review my first patch (even until v4 and more). :)

Peter

> 
> Fam

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

[Eric Blake]

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

On 12/01/2015 06:11 PM, Fam Zheng wrote:
> On Tue, 12/01 21:28, Peter Xu wrote:
>> 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>
>> ---

>> +++ b/qapi/event.json
>> @@ -356,3 +356,16 @@
>>  ##
>>  { '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.
> 
> Please explicitly mention that successful dump emits DUMP_COMPLETED without
> error, and failed dump emits DUMP_COMPLETED that has an error str.

In fact, I wonder if it would also be worth having a
'status':'DumpStatus' field, which records the final status of the dump
(either 'completed' or 'failed'), and which is always present.


>> +++ b/util/error.c
>> @@ -197,7 +197,11 @@ ErrorClass error_get_class(const Error *err)
>>  
>>  const char *error_get_pretty(Error *err)
>>  {
>> -    return err->msg;
>> +    if (err) {
>> +        return err->msg;
>> +    } else {
>> +        return NULL;
>> +    }
> 
> This change belongs to a separate patch, if any.

Indeed.  When I was musing about the idea, I was not expecting you to
actually implement it, so much as questioning whether it is a worthwhile
idea.  But as it impacts more than just your series, it definitely needs
to be a separate patch, if at all.

> But personally I don't like
> it, because it doesn't work very well when error_get_pretty is used in
> printf-like function parameters:
> 
>     Error *err = NULL;
>     error_report("error: %s", error_get_pretty(err));
> 
> will print "error: (null)" which is ugly,

Or even segfault.  glibc is nice for printing "(null)", but the behavior
is undefined by POSIX and other libc aren't as nice as glibc.  And that
was not a consequence I thought about when first raising the question of
whether it was even worth changing the contract of error_get_pretty().

-- 
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 v4 08/11] dump-guest-memory: add qmp event DUMP_COMPLETED

[Peter Xu]

On Wed, Dec 02, 2015 at 07:45:52AM -0700, Eric Blake wrote:
> On 12/01/2015 06:11 PM, Fam Zheng wrote:
> > Please explicitly mention that successful dump emits DUMP_COMPLETED without
> > error, and failed dump emits DUMP_COMPLETED that has an error str.
> 
> In fact, I wonder if it would also be worth having a
> 'status':'DumpStatus' field, which records the final status of the dump
> (either 'completed' or 'failed'), and which is always present.

Will the raw memory total size useful in any way? I am totally ok to
add this, just failed to find a way for user to use it besides
calculating finished work during dump... :(

> 
> 
> >> +++ b/util/error.c
> >> @@ -197,7 +197,11 @@ ErrorClass error_get_class(const Error *err)
> >>  
> >>  const char *error_get_pretty(Error *err)
> >>  {
> >> -    return err->msg;
> >> +    if (err) {
> >> +        return err->msg;
> >> +    } else {
> >> +        return NULL;
> >> +    }
> > 
> > This change belongs to a separate patch, if any.
> 
> Indeed.  When I was musing about the idea, I was not expecting you to
> actually implement it, so much as questioning whether it is a worthwhile
> idea.  But as it impacts more than just your series, it definitely needs
> to be a separate patch, if at all.

Sorry for the misunderstanding. I will make sure to use a new patch
when needed next time. For this change, I will revert it and take
Fam's latter suggestion to avoid modifying error.c.

Thanks.
Peter

> 
> > But personally I don't like
> > it, because it doesn't work very well when error_get_pretty is used in
> > printf-like function parameters:
> > 
> >     Error *err = NULL;
> >     error_report("error: %s", error_get_pretty(err));
> > 
> > will print "error: (null)" which is ugly,
> 
> Or even segfault.  glibc is nice for printing "(null)", but the behavior
> is undefined by POSIX and other libc aren't as nice as glibc.  And that
> was not a consequence I thought about when first raising the question of
> whether it was even worth changing the contract of error_get_pretty().
> 
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 

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

[Eric Blake]

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

On 12/02/2015 08:21 AM, Peter Xu wrote:
> On Wed, Dec 02, 2015 at 07:45:52AM -0700, Eric Blake wrote:
>> On 12/01/2015 06:11 PM, Fam Zheng wrote:
>>> Please explicitly mention that successful dump emits DUMP_COMPLETED without
>>> error, and failed dump emits DUMP_COMPLETED that has an error str.
>>
>> In fact, I wonder if it would also be worth having a
>> 'status':'DumpStatus' field, which records the final status of the dump
>> (either 'completed' or 'failed'), and which is always present.
> 
> Will the raw memory total size useful in any way? I am totally ok to
> add this, just failed to find a way for user to use it besides
> calculating finished work during dump... :(

Good idea.  You never know if it will be helpful, but the information is
basically free to provide and doesn't seem like too much of a
maintenance burden to promise to always include the total.  And in the
case of an error, knowing the final values of complete/total might also
be useful to see how far things got before failure (for example, if it
failed because of ENOSPACE, knowing how much was complete may give an
idea of how much additional space should be added before retrying).

-- 
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 v4 08/11] dump-guest-memory: add qmp event DUMP_COMPLETED

[Peter Xu]

On Wed, Dec 02, 2015 at 09:01:16AM -0700, Eric Blake wrote:
> On 12/02/2015 08:21 AM, Peter Xu wrote:
> > Will the raw memory total size useful in any way? I am totally ok to
> > add this, just failed to find a way for user to use it besides
> > calculating finished work during dump... :(
> 
> Good idea.  You never know if it will be helpful, but the information is
> basically free to provide and doesn't seem like too much of a
> maintenance burden to promise to always include the total.  And in the
> case of an error, knowing the final values of complete/total might also
> be useful to see how far things got before failure (for example, if it
> failed because of ENOSPACE, knowing how much was complete may give an
> idea of how much additional space should be added before retrying).

Yes, it's more meaningful when it fails. And maybe you are right,
it's free to provide it. :)

I can add it in v5.

One thing to mention is that, since the written_byte field is only
for raw memory size, which means (e.g., for kdump-zlib), the number
could first goes to 70% of total very quickly in less than a second
(possibly due to zero pages, so actually very little data is written
to disk), then it will use another ten seconds to finish the rest
30% (which contains most of the data of the final dump file). So the
number would still help little even with ENOSPACE. When user sees a
70% of "written" when failed, it will not mean "we need extra of 30%
more spaces", it actually means "we need 100% more", but the user
would never figure out the real situation from the number only. :(

Thanks!
Peter

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