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

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

[Peter Xu]

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 with a message. 
  - 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 (12):
  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"
  Dump: enhance the documentations.

 docs/qmp-events.txt             |  14 +++
 dump.c                          | 218 ++++++++++++++++++++++++++++++----------
 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                |  58 ++++++++++-
 qapi/event.json                 |  10 ++
 qmp-commands.hx                 |  34 ++++++-
 qmp.c                           |  14 +++
 14 files changed, 357 insertions(+), 64 deletions(-)

-- 
2.4.3

[Qemu-devel] [PATCH v3 01/12] 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 v3 02/12] dump-guest-memory: add "detach" flag for QMP/HMP interfaces.

[Peter Xu]

This patch only adds the interfaces, but not implements 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..dccb457 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_try_bool(qdict, "detach", false);
+    }
 
     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..fd81ce2 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 dump to be finished (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..bbb08e1 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 specificed, command will return immediately, without waiting
+            for dump to be finished (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 v3 03/12] 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 detach dump.

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

diff --git a/dump.c b/dump.c
index d79e0ed..c9ea878 100644
--- a/dump.c
+++ b/dump.c
@@ -1418,6 +1418,24 @@ static void get_max_mapnr(DumpState *s)
     s->max_mapnr = paddr_to_pfn(last_block->target_end);
 }
 
+/* init dump state with specific status */
+static void dump_state_prepare(DumpState *s, DumpStatus status)
+{
+    bzero(s, sizeof(*s));
+    s->status = status;
+}
+
+static DumpState *dump_state_get_global(void)
+{
+    static DumpState state;
+    static bool init = false;
+    if (!init) {
+        dump_state_prepare(&state, DUMP_STATUS_NONE);
+        init = true;
+    }
+    return &state;
+}
+
 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 +1665,14 @@ void qmp_dump_guest_memory(bool paging, const char *file,
         return;
     }
 
-    s = g_malloc0(sizeof(DumpState));
+    s = dump_state_get_global();
+    dump_state_prepare(s, DUMP_STATUS_ACTIVE);
 
     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 +1682,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 fd81ce2..3728bfc 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2139,6 +2139,24 @@
             '*format': 'DumpGuestMemoryFormat'} }
 
 ##
+# @DumpStatus
+#
+# Define the status for dump guest memory.
+#
+# @none: not started any dump-guest-memory yet.
+#
+# @active: there is one dump running in background.
+#
+# @completed: the last dump has finished sucessfully
+#
+# @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 v3 04/12] 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 c9ea878..110dbf9 100644
--- a/dump.c
+++ b/dump.c
@@ -1436,6 +1436,12 @@ static DumpState *dump_state_get_global(void)
     return &state;
 }
 
+bool dump_in_progress(void)
+{
+    DumpState *state = dump_state_get_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)
@@ -1609,6 +1615,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 v3 05/12] 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                | 19 ++++++++++++++-----
 include/sysemu/dump.h |  3 +++
 2 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/dump.c b/dump.c
index 110dbf9..c8ae5c3 100644
--- a/dump.c
+++ b/dump.c
@@ -1451,6 +1451,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);
@@ -1604,6 +1607,16 @@ 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);
+    }
+}
+
 void qmp_dump_guest_memory(bool paging, const char *file,
                            bool has_detach, bool detach,
                            bool has_begin, int64_t begin, bool has_length,
@@ -1689,11 +1702,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);
-    }
+    dump_process(s, errp);
 
     if (*errp) {
         s->status = DUMP_STATUS_FAILED;
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 v3 06/12] 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 c8ae5c3..5b9def3 100644
--- a/dump.c
+++ b/dump.c
@@ -1628,13 +1628,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 v3 07/12] 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                          | 39 ++++++++++++++++++++++++++++++++-------
 include/sysemu/dump.h           |  1 +
 include/sysemu/memory_mapping.h |  4 ++++
 memory_mapping.c                |  3 +++
 4 files changed, 40 insertions(+), 7 deletions(-)

diff --git a/dump.c b/dump.c
index 5b9def3..14fd41f 100644
--- a/dump.c
+++ b/dump.c
@@ -1615,6 +1615,28 @@ static void dump_process(DumpState *s, Error **errp)
     } else {
         create_vmcore(s, errp);
     }
+
+    if (*errp) {
+        s->status = DUMP_STATUS_FAILED;
+    } else {
+        s->status = DUMP_STATUS_COMPLETED;
+    }
+
+    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,
@@ -1627,6 +1649,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.");
@@ -1657,6 +1680,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
@@ -1706,15 +1732,14 @@ void qmp_dump_guest_memory(bool paging, const char *file,
         return;
     }
 
-    dump_process(s, errp);
-
-    if (*errp) {
-        s->status = DUMP_STATUS_FAILED;
+    if (detach_p) {
+        /* detached dump */
+        qemu_thread_create(&s->dump_thread, "dump_thread", dump_thread,
+                           s, QEMU_THREAD_DETACHED);
     } else {
-        s->status = DUMP_STATUS_COMPLETED;
+        /* sync dump */
+        dump_process(s, errp);
     }
-
-    dump_cleanup(s);
 }
 
 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 v3 08/12] dump-guest-memory: add qmp event DUMP_COMPLETED

[Peter Xu]

One new QMP event DUMP_COMPLETED is added. It is used when user
specified "detach" in dump, and triggered when the dump finishes
(either succeeded or failed). If failed, one "err" data will be
passed with specific error message.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 docs/qmp-events.txt | 14 ++++++++++++++
 dump.c              |  6 +++++-
 qapi/event.json     | 10 ++++++++++
 3 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/docs/qmp-events.txt b/docs/qmp-events.txt
index d2f1ce4..7b9f835 100644
--- a/docs/qmp-events.txt
+++ b/docs/qmp-events.txt
@@ -674,3 +674,17 @@ Note: If action is "reset", "shutdown", or "pause" the WATCHDOG event is
 followed respectively by the RESET, SHUTDOWN, or STOP events.
 
 Note: this event is rate-limited.
+
+DUMP_COMPLETED
+--------------
+
+Emitted when the guest has finished one memory dump.
+
+Data:
+
+- "error": Error message when dump failed (json-string, optional)
+
+Example:
+
+{ "event": "DUMP_COMPLETED",
+  "data": {} }
diff --git a/dump.c b/dump.c
index 14fd41f..43f565d 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
@@ -1633,8 +1634,11 @@ static void *dump_thread(void *data)
     dump_process(s, &err);
 
     if (err) {
-        /* TODO: notify user the error */
+        qapi_event_send_dump_completed(true, error_get_pretty(err),
+                                       &error_abort);
         error_free(err);
+    } else {
+        qapi_event_send_dump_completed(false, NULL, &error_abort);
     }
     return NULL;
 }
diff --git a/qapi/event.json b/qapi/event.json
index f0cef01..c46214b 100644
--- a/qapi/event.json
+++ b/qapi/event.json
@@ -356,3 +356,13 @@
 ##
 { 'event': 'MEM_UNPLUG_ERROR',
   'data': { 'device': 'str', 'msg': 'str' } }
+
+##
+# @DUMP_COMPLETED
+#
+# Emitted when background dump has completed
+#
+# Since: 2.6
+##
+{ 'event': 'DUMP_COMPLETED' ,
+  'data': { '*error': 'str' } }
-- 
2.4.3

[Qemu-devel] [PATCH v3 09/12] 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 43f565d..56a2d7e 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);
@@ -1443,6 +1446,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)
@@ -1454,6 +1481,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) {
@@ -1485,6 +1513,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 v3 10/12] 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 for now contains:

- status: current dump status
- written_bytes: bytes written in latest dump
- total_bytes: bytes to write in latest dump

>From written_bytes and total_bytes, we could see how much work
finished by calculating:

  100.0 * written_bytes / total_bytes (%)

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 dump.c           | 10 ++++++++++
 qapi-schema.json | 29 +++++++++++++++++++++++++++++
 qmp-commands.hx  | 26 +++++++++++++++++++++++++-
 3 files changed, 64 insertions(+), 1 deletion(-)

diff --git a/dump.c b/dump.c
index 56a2d7e..6596bc8 100644
--- a/dump.c
+++ b/dump.c
@@ -1675,6 +1675,16 @@ static void *dump_thread(void *data)
     return NULL;
 }
 
+DumpQueryResult *qmp_query_dump(Error **errp)
+{
+    DumpQueryResult *result = g_malloc0(sizeof(*result));
+    DumpState *state = dump_state_get_global();
+    result->status = state->status;
+    result->written_bytes = state->written_size;
+    result->total_bytes = 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,
diff --git a/qapi-schema.json b/qapi-schema.json
index 3728bfc..577c381 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2157,6 +2157,35 @@
   'data': [ 'none', 'active', 'completed', 'failed' ] }
 
 ##
+# @DumpQueryResult
+#
+# The result format for 'query-dump'.
+#
+# @status: enum of @DumpStatus, which shows current dump status
+#
+# @written_bytes: bytes written in latest dump (uncompressed)
+#
+# @total_bytes: total bytes to be write in latest dump (uncompressed)
+#
+# Since 2.6
+##
+{ 'struct': 'DumpQueryResult',
+  'data': { 'status': 'DumpStatus',
+            'written_bytes': 'int',
+            'total_bytes': '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 bbb08e1..ac6d2da 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -881,7 +881,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
@@ -898,6 +898,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", "written_bytes": 1024000,
+                 "total_bytes": 2048000 } }
+
+EQMP
+
 #if defined TARGET_S390X
     {
         .name       = "dump-skeys",
-- 
2.4.3

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

[Peter Xu]

It will calculate percentage of finished work from written_bytes and
total_bytes.

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 dccb457..dee2cb5 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_bytes != 0);
+        percent = 100.0 * result->written_bytes / result->total_bytes;
+        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

[Qemu-devel] [PATCH v3 12/12] Dump: enhance the documentations.

[Peter Xu]

Add more documents to mention about "query-dump" and DUMP_COMPLETED
events.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 qapi-schema.json | 7 +++++--
 qmp-commands.hx  | 4 +++-
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/qapi-schema.json b/qapi-schema.json
index 577c381..ce4acb2 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2115,8 +2115,11 @@
 #            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 dump to be finished (since 2.6).
+# @detach: #optional if true, QMP will return immediately rather
+#          than waiting dump to be finished. If this is specified,
+#          user could later use "query-dump" to check latest dump
+#          status. When dump finishes, one event DUMP_COMPLETED will
+#          be sent to the user to notify the completion (since 2.6).
 #
 # @begin: #optional if specified, the starting physical address.
 #
diff --git a/qmp-commands.hx b/qmp-commands.hx
index ac6d2da..cd097d4 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -858,7 +858,9 @@ Arguments:
 - "protocol": destination file(started with "file:") or destination file
               descriptor (started with "fd:") (json-string)
 - "detach": if specificed, command will return immediately, without waiting
-            for dump to be finished (json-bool)
+            for dump to be finished. After command return, user
+            could query for latest dump status using "query-dump". When dump
+            finishes, an event DUMP_COMPLETED will be sent (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

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

[Paolo Bonzini]

On 30/11/2015 12:32, Peter Xu wrote:
>      if (*errp) {
>          s->status = DUMP_STATUS_FAILED;

Why not move this "if" to dump_process as well?

Paolo

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

[Paolo Bonzini]

On 30/11/2015 12:32, Peter Xu wrote:
> +{
> +    DumpQueryResult *result = g_malloc0(sizeof(*result));
> +    DumpState *state = dump_state_get_global();
> +    result->status = state->status;
> +    result->written_bytes = state->written_size;

You need a mutex around the reads of ->status and ->written_size.

Paolo

> +    result->total_bytes = state->total_size;
> +    return result;

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

[Paolo Bonzini]

On 30/11/2015 12:32, 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 detach dump.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  dump.c                | 30 +++++++++++++++++++++++++++---
>  include/sysemu/dump.h |  2 ++
>  qapi-schema.json      | 18 ++++++++++++++++++
>  3 files changed, 47 insertions(+), 3 deletions(-)
> 
> diff --git a/dump.c b/dump.c
> index d79e0ed..c9ea878 100644
> --- a/dump.c
> +++ b/dump.c
> @@ -1418,6 +1418,24 @@ static void get_max_mapnr(DumpState *s)
>      s->max_mapnr = paddr_to_pfn(last_block->target_end);
>  }
>  
> +/* init dump state with specific status */
> +static void dump_state_prepare(DumpState *s, DumpStatus status)
> +{
> +    bzero(s, sizeof(*s));
> +    s->status = status;

Either use memcpy, or

    s = (DumpState) { .status = status };

The latter is C99 and it's quite common in QEMU.

> +}
> +
> +static DumpState *dump_state_get_global(void)

No need for dump_state_get_global, just use a static variable.  Then you
can use &dump_state in qmp_dump_guest_memory.

> +{
> +    static DumpState state;

You can also initialize it together with the definition, using

    static DumpState state = { .status = DUMP_STATUS_NONE };

> +    static bool init = false;
> +    if (!init) {
> +        dump_state_prepare(&state, DUMP_STATUS_NONE);
> +        init = true;
> +    }
> +    return &state;
> +}
> +
>  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 +1665,14 @@ void qmp_dump_guest_memory(bool paging, const char *file,
>          return;
>      }
>  
> -    s = g_malloc0(sizeof(DumpState));
> +    s = dump_state_get_global();
> +    dump_state_prepare(s, DUMP_STATUS_ACTIVE);
>  
>      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 +1682,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 fd81ce2..3728bfc 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -2139,6 +2139,24 @@
>              '*format': 'DumpGuestMemoryFormat'} }
>  
>  ##
> +# @DumpStatus
> +#
> +# Define the status for dump guest memory.
> +#
> +# @none: not started any dump-guest-memory yet.
> +#
> +# @active: there is one dump running in background.
> +#
> +# @completed: the last dump has finished sucessfully
> +#
> +# @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
> 

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

[Eric Blake]

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

On 11/30/2015 04:32 AM, Peter Xu wrote:
> This patch only adds the interfaces, but not implements 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(-)

> +++ 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");

Here, you probe whether 'detach' is present...

>      int64_t begin = 0;
>      int64_t length = 0;
> +    bool detach = false;

...here, you default 'detach' to 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_try_bool(qdict, "detach", false);

...therefore, this line is only reachable if 'detach' is present, which
means the default will never be needed.

> +    }
>  
>      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);

There are two competing ways to simplify this; I don't care which one
you choose, although you can't do both:

1. Note that since the second assignment to detach only happens if
has_detach is true, you can use the simpler:

if (has_detach) {
    detach = qdict_get_bool(qdict, "detach");
}

If you go with this approach, you could even get away without
initializing 'detach' outside of the 'if (has_detach)' conditional
(although it's still probably wiser to avoid uninitialized memory than
to rely on qmp_dump_guest_memory() correctly ignoring 'detach' when
'has_detach' is false).

2. Note that since qdict_get_try_bool() lets you specify a default, you
can eliminate the has_detach variable and instead just do:

bool detach = qdict_get_try_bool(qdict, "detach", false);
...
qmp_dump_guest_memory(paging, prot, true, detach, ...);

> +++ 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 dump to be finished (since 2.6).

s/dump/for the dump/
s/be finished/finish/

> @@ -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 specificed, command will return immediately, without waiting

s/specificed/specified/

> +            for dump to be finished (json-bool)

s/dump/the dump/
s/be finished/finish/

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

-- 
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 v3 03/12] dump-guest-memory: using static DumpState, add DumpStatus

[Eric Blake]

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

On 11/30/2015 04:32 AM, 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 detach dump.

s/detach/detached/

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

In addition to Paolo's review,

> +++ b/qapi-schema.json
> @@ -2139,6 +2139,24 @@
>              '*format': 'DumpGuestMemoryFormat'} }
>  
>  ##
> +# @DumpStatus
> +#
> +# Define the status for dump guest memory.

Reads awkwardly.  Maybe:

Describe the status of a long-running background guest memory dump.

> +#
> +# @none: not started any dump-guest-memory yet.

@none: no dump-guest-memory has started yet

> +#
> +# @active: there is one dump running in background.
> +#
> +# @completed: the last dump has finished sucessfully

s/sucessfully/successfully/

Inconsistent on whether your lines end in '.'

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

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

[Eric Blake]

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

On 11/30/2015 04:32 AM, Peter Xu wrote:
> One new QMP event DUMP_COMPLETED is added. It is used when user
> specified "detach" in dump, and triggered when the dump finishes
> (either succeeded or failed). If failed, one "err" data will be
> passed with specific error message.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  docs/qmp-events.txt | 14 ++++++++++++++
>  dump.c              |  6 +++++-
>  qapi/event.json     | 10 ++++++++++
>  3 files changed, 29 insertions(+), 1 deletion(-)
> 
> diff --git a/docs/qmp-events.txt b/docs/qmp-events.txt
> index d2f1ce4..7b9f835 100644
> --- a/docs/qmp-events.txt
> +++ b/docs/qmp-events.txt
> @@ -674,3 +674,17 @@ Note: If action is "reset", "shutdown", or "pause" the WATCHDOG event is
>  followed respectively by the RESET, SHUTDOWN, or STOP events.
>  
>  Note: this event is rate-limited.
> +
> +DUMP_COMPLETED
> +--------------
> +
> +Emitted when the guest has finished one memory dump.
> +
> +Data:
> +
> +- "error": Error message when dump failed (json-string, optional)
> +
> +Example:
> +
> +{ "event": "DUMP_COMPLETED",
> +  "data": {} }

Please keep this file sorted.  The insertion should be between
DEVICE_TRAY_MOVED and GUEST_PANICKED.

> diff --git a/dump.c b/dump.c
> index 14fd41f..43f565d 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
> @@ -1633,8 +1634,11 @@ static void *dump_thread(void *data)
>      dump_process(s, &err);
>  
>      if (err) {
> -        /* TODO: notify user the error */
> +        qapi_event_send_dump_completed(true, error_get_pretty(err),
> +                                       &error_abort);
>          error_free(err);
> +    } else {
> +        qapi_event_send_dump_completed(false, NULL, &error_abort);
>      }

Hmmm. I wonder if error_get_pretty() should be improved to return NULL
when there is no error.  Then we could write:

qapi_event_send_dump_completed(!!err, error_get_pretty(err),
                               &error_abort);
error_free(err);

But that doesn't affect the correctness of your patch.

>      return NULL;
>  }
> diff --git a/qapi/event.json b/qapi/event.json
> index f0cef01..c46214b 100644
> --- a/qapi/event.json
> +++ b/qapi/event.json
> @@ -356,3 +356,13 @@
>  ##
>  { 'event': 'MEM_UNPLUG_ERROR',
>    'data': { 'device': 'str', 'msg': 'str' } }
> +
> +##
> +# @DUMP_COMPLETED
> +#
> +# Emitted when background dump has completed
> +#
> +# Since: 2.6

Missing documentation of 'error'.

> +##
> +{ 'event': 'DUMP_COMPLETED' ,
> +  'data': { '*error': 'str' } }
> 

-- 
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 v3 10/12] Dump: add qmp command "query-dump"

[Eric Blake]

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

On 11/30/2015 04:32 AM, 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 for now contains:
> 
> - status: current dump status
> - written_bytes: bytes written in latest dump
> - total_bytes: bytes to write in latest dump
> 
>>From written_bytes and total_bytes, we could see how much work
> finished by calculating:
> 
>   100.0 * written_bytes / total_bytes (%)
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  dump.c           | 10 ++++++++++
>  qapi-schema.json | 29 +++++++++++++++++++++++++++++
>  qmp-commands.hx  | 26 +++++++++++++++++++++++++-
>  3 files changed, 64 insertions(+), 1 deletion(-)
> 
> diff --git a/dump.c b/dump.c
> index 56a2d7e..6596bc8 100644
> --- a/dump.c
> +++ b/dump.c
> @@ -1675,6 +1675,16 @@ static void *dump_thread(void *data)
>      return NULL;
>  }
>  
> +DumpQueryResult *qmp_query_dump(Error **errp)
> +{
> +    DumpQueryResult *result = g_malloc0(sizeof(*result));

Might be nicer as g_new0(DumpQueryResult, 1). Markus has been switching
to g_new0 where a type name was already mentioned, although here you
used *result rather than a typename.


> +++ b/qapi-schema.json
> @@ -2157,6 +2157,35 @@
>    'data': [ 'none', 'active', 'completed', 'failed' ] }
>  
>  ##
> +# @DumpQueryResult
> +#
> +# The result format for 'query-dump'.
> +#
> +# @status: enum of @DumpStatus, which shows current dump status
> +#
> +# @written_bytes: bytes written in latest dump (uncompressed)
> +#
> +# @total_bytes: total bytes to be write in latest dump (uncompressed)

s/be write/written/

> +#
> +# Since 2.6
> +##
> +{ 'struct': 'DumpQueryResult',
> +  'data': { 'status': 'DumpStatus',
> +            'written_bytes': 'int',
> +            'total_bytes': 'int' } }

Prefer '-' over '_' in new QMP (as in 'total-bytes' rather than
'total_bytes').  Furthermore, QMP already defaults to bytes, so it would
be sufficient to name these merely 'written'/'total' or even
'complete'/'total'.

> +++ b/qmp-commands.hx
> @@ -881,7 +881,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,
>      },

Unrelated hunk.

-- 
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 v3 12/12] Dump: enhance the documentations.

[Eric Blake]

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

On 11/30/2015 04:32 AM, Peter Xu wrote:
> Add more documents to mention about "query-dump" and DUMP_COMPLETED
> events.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  qapi-schema.json | 7 +++++--
>  qmp-commands.hx  | 4 +++-
>  2 files changed, 8 insertions(+), 3 deletions(-)

Why not squash this in to the patch that introduces the options?  No
need to have churn within the series by tweaking the documentation more
than once.

> 
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 577c381..ce4acb2 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -2115,8 +2115,11 @@
>  #            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 dump to be finished (since 2.6).
> +# @detach: #optional if true, QMP will return immediately rather
> +#          than waiting dump to be finished. If this is specified,
> +#          user could later use "query-dump" to check latest dump
> +#          status. When dump finishes, one event DUMP_COMPLETED will

s/one event DUMP_COMPLETED/a DUMP_COMPLETED event/

> +#          be sent to the user to notify the completion (since 2.6).

s/to the user to notify the completion//

>  #
>  # @begin: #optional if specified, the starting physical address.
>  #
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index ac6d2da..cd097d4 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -858,7 +858,9 @@ Arguments:
>  - "protocol": destination file(started with "file:") or destination file
>                descriptor (started with "fd:") (json-string)
>  - "detach": if specificed, command will return immediately, without waiting
> -            for dump to be finished (json-bool)
> +            for dump to be finished. After command return, user
> +            could query for latest dump status using "query-dump". When dump
> +            finishes, an event DUMP_COMPLETED will be sent (json-bool)

Awkward.  How about:

- "detach": if specified, the command will return immediately rather
than waiting for the dump completion. The user can track progress with
"query-dump", and a DUMP_COMPLETED event will occur at the end. (json-bool)

-- 
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 v3 02/12] dump-guest-memory: add "detach" flag for QMP/HMP interfaces.

[Peter Xu]

On Mon, Nov 30, 2015 at 03:05:10PM -0700, Eric Blake wrote:
> On 11/30/2015 04:32 AM, Peter Xu wrote:
> > This patch only adds the interfaces, but not implements 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(-)
> 
> > +++ 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");
> 
> Here, you probe whether 'detach' is present...
> 
> >      int64_t begin = 0;
> >      int64_t length = 0;
> > +    bool detach = false;
> 
> ...here, you default 'detach' to 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_try_bool(qdict, "detach", false);
> 
> ...therefore, this line is only reachable if 'detach' is present, which
> means the default will never be needed.
> 
> > +    }
> >  
> >      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);
> 
> There are two competing ways to simplify this; I don't care which one
> you choose, although you can't do both:
> 
> 1. Note that since the second assignment to detach only happens if
> has_detach is true, you can use the simpler:
> 
> if (has_detach) {
>     detach = qdict_get_bool(qdict, "detach");
> }
> 
> If you go with this approach, you could even get away without
> initializing 'detach' outside of the 'if (has_detach)' conditional
> (although it's still probably wiser to avoid uninitialized memory than
> to rely on qmp_dump_guest_memory() correctly ignoring 'detach' when
> 'has_detach' is false).
> 
> 2. Note that since qdict_get_try_bool() lets you specify a default, you
> can eliminate the has_detach variable and instead just do:
> 
> bool detach = qdict_get_try_bool(qdict, "detach", false);
> ...
> qmp_dump_guest_memory(paging, prot, true, detach, ...);

Yes, the default is lost. Thanks.

I think (2) is better in term of lines of codes (and also
clear). However I may need to keep the QMP interface (to keep the
has_detach parameter in qmp_dump_guest_memory), so I'd like to
choose (1).

> 
> > +++ 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 dump to be finished (since 2.6).
> 
> s/dump/for the dump/
> s/be finished/finish/
> 
> > @@ -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 specificed, command will return immediately, without waiting
> 
> s/specificed/specified/
> 
> > +            for dump to be finished (json-bool)
> 
> s/dump/the dump/
> s/be finished/finish/

Thanks for review again on the English!

Peter

> 
> >  - "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
> > 
> 
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 

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

[Peter Xu]

On Mon, Nov 30, 2015 at 02:00:28PM +0100, Paolo Bonzini wrote:
> > +/* init dump state with specific status */
> > +static void dump_state_prepare(DumpState *s, DumpStatus status)
> > +{
> > +    bzero(s, sizeof(*s));
> > +    s->status = status;
> 
> Either use memcpy, or
> 
>     s = (DumpState) { .status = status };
> 
> The latter is C99 and it's quite common in QEMU.

Thanks to let me know this. :) Will use it in v4.

> 
> > +}
> > +
> > +static DumpState *dump_state_get_global(void)
> 
> No need for dump_state_get_global, just use a static variable.  Then you
> can use &dump_state in qmp_dump_guest_memory.

Ok.

> 
> > +{
> > +    static DumpState state;
> 
> You can also initialize it together with the definition, using
> 
>     static DumpState state = { .status = DUMP_STATUS_NONE };
> 

Yes. Thanks.

Peter

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

[Peter Xu]

On Mon, Nov 30, 2015 at 03:08:24PM -0700, Eric Blake wrote:
> On 11/30/2015 04:32 AM, 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 detach dump.
> 
> s/detach/detached/
> 
> > 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  dump.c                | 30 +++++++++++++++++++++++++++---
> >  include/sysemu/dump.h |  2 ++
> >  qapi-schema.json      | 18 ++++++++++++++++++
> >  3 files changed, 47 insertions(+), 3 deletions(-)
> > 
> 
> In addition to Paolo's review,
> 
> > +++ b/qapi-schema.json
> > @@ -2139,6 +2139,24 @@
> >              '*format': 'DumpGuestMemoryFormat'} }
> >  
> >  ##
> > +# @DumpStatus
> > +#
> > +# Define the status for dump guest memory.
> 
> Reads awkwardly.  Maybe:
> 
> Describe the status of a long-running background guest memory dump.
> 
> > +#
> > +# @none: not started any dump-guest-memory yet.
> 
> @none: no dump-guest-memory has started yet
> 
> > +#
> > +# @active: there is one dump running in background.
> > +#
> > +# @completed: the last dump has finished sucessfully
> 
> s/sucessfully/successfully/
> 
> Inconsistent on whether your lines end in '.'

Will fix above all in v4.

Thanks!
Peter

> 
> > +#
> > +# @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
> > 
> 
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 

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

[Peter Xu]

On Mon, Nov 30, 2015 at 01:55:01PM +0100, Paolo Bonzini wrote:
> 
> 
> On 30/11/2015 12:32, Peter Xu wrote:
> >      if (*errp) {
> >          s->status = DUMP_STATUS_FAILED;
> 
> Why not move this "if" to dump_process as well?

Yes. I did that in another patch. I will move it to this patch,
which should be better.

Thanks.
Peter

> 
> Paolo

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

[Peter Xu]

On Mon, Nov 30, 2015 at 03:12:31PM -0700, Eric Blake wrote:
> On 11/30/2015 04:32 AM, Peter Xu wrote:
> > +Example:
> > +
> > +{ "event": "DUMP_COMPLETED",
> > +  "data": {} }
> 
> Please keep this file sorted.  The insertion should be between
> DEVICE_TRAY_MOVED and GUEST_PANICKED.

Sorry for the incaution to miss that. Will fix in v4.

> 
> > diff --git a/dump.c b/dump.c
> > index 14fd41f..43f565d 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
> > @@ -1633,8 +1634,11 @@ static void *dump_thread(void *data)
> >      dump_process(s, &err);
> >  
> >      if (err) {
> > -        /* TODO: notify user the error */
> > +        qapi_event_send_dump_completed(true, error_get_pretty(err),
> > +                                       &error_abort);
> >          error_free(err);
> > +    } else {
> > +        qapi_event_send_dump_completed(false, NULL, &error_abort);
> >      }
> 
> Hmmm. I wonder if error_get_pretty() should be improved to return NULL
> when there is no error.  Then we could write:
> 
> qapi_event_send_dump_completed(!!err, error_get_pretty(err),
>                                &error_abort);
> error_free(err);
> 
> But that doesn't affect the correctness of your patch.

After seeing that improving error_get_pretty() is simple (and also
will not break the other callers), I'd like to take your advice,
which is clean enough. Thanks.

> 
> >      return NULL;
> >  }
> > diff --git a/qapi/event.json b/qapi/event.json
> > index f0cef01..c46214b 100644
> > --- a/qapi/event.json
> > +++ b/qapi/event.json
> > @@ -356,3 +356,13 @@
> >  ##
> >  { 'event': 'MEM_UNPLUG_ERROR',
> >    'data': { 'device': 'str', 'msg': 'str' } }
> > +
> > +##
> > +# @DUMP_COMPLETED
> > +#
> > +# Emitted when background dump has completed
> > +#
> > +# Since: 2.6
> 
> Missing documentation of 'error'.

Added.

Thanks!
Peter

> 
> > +##
> > +{ 'event': 'DUMP_COMPLETED' ,
> > +  'data': { '*error': 'str' } }
> > 
> 
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 

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

[Peter Xu]

On Mon, Nov 30, 2015 at 01:56:42PM +0100, Paolo Bonzini wrote:
> 
> 
> On 30/11/2015 12:32, Peter Xu wrote:
> > +{
> > +    DumpQueryResult *result = g_malloc0(sizeof(*result));
> > +    DumpState *state = dump_state_get_global();
> > +    result->status = state->status;
> > +    result->written_bytes = state->written_size;
> 
> You need a mutex around the reads of ->status and ->written_size.

Could I avoid using mutex here? Let me try to explain what I
thought.

The concurrency of this should only happen when:

- detached dump thread is working (dump thread)
- user queries dump status (main thread)

What the dump thread is doing should be something like:

- [start dumping]
- inc written_size
- inc written_size
- ...
- inc written_size
- set ->status to COMPLETED|FAILED
- [end dumping]

Now if the main thread tries to fetch dump status during it's
working, the worst thing is that, the ->written_size fetched by main
thread is not exactly the one when it was fetching ->status. Or say,
we might get some kind of inaccuracy (which should be really small)
without the lock. Meanwhile, we could avoid a lock if we could allow
the very small difference in written_size.

Another thing could happen is when user queries duing it's finishing
(or say, user query between dump thread modify written_size and
status), we might got this:

{ "status": "active", "written": "100", "total": "100" }

Rather than:

{ "status": "completed", "written": "100", "total": "100" }

As long as we make sure we fetch "status" first rather than
"written_size" (that's what I did in current codes). It should still
be acceptable?

Here, the reason I would like to avoid using lock is that: if I use
lock here, I need to use it whenever dump thread increases the
->written_size. That's a operation very frequently happens in dump
thread. I could enhance it though by updating ->written_bytes in
periods, but it might be awkward comparing to directly drop the lock
(if possible) by losing some kind of accuracy.

Not sure whether I missed anything. Also, please let me know if you
still suggest using lock here.

(btw, if using lock, would spinlock be better?)

Thanks!
Peter

> 
> Paolo
> 
> > +    result->total_bytes = state->total_size;
> > +    return result;

Re: [Qemu-devel] [PATCH v3 12/12] Dump: enhance the documentations.

[Peter Xu]

On Mon, Nov 30, 2015 at 03:22:37PM -0700, Eric Blake wrote:
> On 11/30/2015 04:32 AM, Peter Xu wrote:
> > Add more documents to mention about "query-dump" and DUMP_COMPLETED
> > events.
> > 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  qapi-schema.json | 7 +++++--
> >  qmp-commands.hx  | 4 +++-
> >  2 files changed, 8 insertions(+), 3 deletions(-)
> 
> Why not squash this in to the patch that introduces the options?  No
> need to have churn within the series by tweaking the documentation more
> than once.

Ok. Will do that. 

> 
> > 
> > diff --git a/qapi-schema.json b/qapi-schema.json
> > index 577c381..ce4acb2 100644
> > --- a/qapi-schema.json
> > +++ b/qapi-schema.json
> > @@ -2115,8 +2115,11 @@
> >  #            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 dump to be finished (since 2.6).
> > +# @detach: #optional if true, QMP will return immediately rather
> > +#          than waiting dump to be finished. If this is specified,
> > +#          user could later use "query-dump" to check latest dump
> > +#          status. When dump finishes, one event DUMP_COMPLETED will
> 
> s/one event DUMP_COMPLETED/a DUMP_COMPLETED event/
> 
> > +#          be sent to the user to notify the completion (since 2.6).
> 
> s/to the user to notify the completion//
> 
> >  #
> >  # @begin: #optional if specified, the starting physical address.
> >  #
> > diff --git a/qmp-commands.hx b/qmp-commands.hx
> > index ac6d2da..cd097d4 100644
> > --- a/qmp-commands.hx
> > +++ b/qmp-commands.hx
> > @@ -858,7 +858,9 @@ Arguments:
> >  - "protocol": destination file(started with "file:") or destination file
> >                descriptor (started with "fd:") (json-string)
> >  - "detach": if specificed, command will return immediately, without waiting
> > -            for dump to be finished (json-bool)
> > +            for dump to be finished. After command return, user
> > +            could query for latest dump status using "query-dump". When dump
> > +            finishes, an event DUMP_COMPLETED will be sent (json-bool)
> 
> Awkward.  How about:
> 
> - "detach": if specified, the command will return immediately rather
> than waiting for the dump completion. The user can track progress with
> "query-dump", and a DUMP_COMPLETED event will occur at the end. (json-bool)

Sorry for the awkward English...

Will take your version. Thanks. :)

Peter

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

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

[Peter Xu]

On Mon, Nov 30, 2015 at 03:17:01PM -0700, Eric Blake wrote:
> On 11/30/2015 04:32 AM, Peter Xu wrote:
> > +DumpQueryResult *qmp_query_dump(Error **errp)
> > +{
> > +    DumpQueryResult *result = g_malloc0(sizeof(*result));
> 
> Might be nicer as g_new0(DumpQueryResult, 1). Markus has been switching
> to g_new0 where a type name was already mentioned, although here you
> used *result rather than a typename.

Sure. Thanks.

> 
> 
> > +++ b/qapi-schema.json
> > @@ -2157,6 +2157,35 @@
> >    'data': [ 'none', 'active', 'completed', 'failed' ] }
> >  
> >  ##
> > +# @DumpQueryResult
> > +#
> > +# The result format for 'query-dump'.
> > +#
> > +# @status: enum of @DumpStatus, which shows current dump status
> > +#
> > +# @written_bytes: bytes written in latest dump (uncompressed)
> > +#
> > +# @total_bytes: total bytes to be write in latest dump (uncompressed)
> 
> s/be write/written/
> 
> > +#
> > +# Since 2.6
> > +##
> > +{ 'struct': 'DumpQueryResult',
> > +  'data': { 'status': 'DumpStatus',
> > +            'written_bytes': 'int',
> > +            'total_bytes': 'int' } }
> 
> Prefer '-' over '_' in new QMP (as in 'total-bytes' rather than
> 'total_bytes').  Furthermore, QMP already defaults to bytes, so it would
> be sufficient to name these merely 'written'/'total' or even
> 'complete'/'total'.

I will directly take "completed" and "total" then.

> 
> > +++ b/qmp-commands.hx
> > @@ -881,7 +881,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,
> >      },
> 
> Unrelated hunk.

Is it ok to have this line here? I just saw it and fixed it. Please
let me know if I should move it out of this patch.

Thanks.
Peter

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

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

[Paolo Bonzini]

On 01/12/2015 04:57, Peter Xu wrote:
>> > You need a mutex around the reads of ->status and ->written_size.
> Could I avoid using mutex here? Let me try to explain what I
> thought.
> 
> The concurrency of this should only happen when:
> 
> - detached dump thread is working (dump thread)
> - user queries dump status (main thread)
> 
> What the dump thread is doing should be something like:
> 
> - [start dumping]
> - inc written_size
> - inc written_size
> - ...
> - inc written_size
> - set ->status to COMPLETED|FAILED
> - [end dumping]

Yes, it's possible but you need to use atomic_mb_read/atomic_mb_set to
write ->status.  Otherwise a CPU can see the write to ->status before
some of the final writes to ->written_size.

Paolo

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

[Peter Xu]

On Tue, Dec 01, 2015 at 10:54:48AM +0100, Paolo Bonzini wrote:
> 
> 
> On 01/12/2015 04:57, Peter Xu wrote:
> >> > You need a mutex around the reads of ->status and ->written_size.
> > Could I avoid using mutex here? Let me try to explain what I
> > thought.
> > 
> > The concurrency of this should only happen when:
> > 
> > - detached dump thread is working (dump thread)
> > - user queries dump status (main thread)
> > 
> > What the dump thread is doing should be something like:
> > 
> > - [start dumping]
> > - inc written_size
> > - inc written_size
> > - ...
> > - inc written_size
> > - set ->status to COMPLETED|FAILED
> > - [end dumping]
> 
> Yes, it's possible but you need to use atomic_mb_read/atomic_mb_set to
> write ->status.  Otherwise a CPU can see the write to ->status before
> some of the final writes to ->written_size.

Hi, Paolo,

Thanks to point out. However, would it be confusing to use
atomic_mb_{read|set} rather than directly use smp_rmb() and
smp_wmb()? Like:

In dump thread:

- inc written_size
- inc written_size
- ...
- inc written_size
- smp_wmb()
- atomic_set(status, COMPLETED|FAILED)

In main thread:

- atomic_read(status)
- smp_rmb()
- read written_size

What I understand from the doc (seems written by you, thanks :) ) is
that: atomic_mb_{read|set} is the pair of helper functions for _one_
specific variable, to make sure its operations are always in order
as long as we are using atomic_mb_* functions to access it all the
time. However, in the dump thread case, it's related to read/write
order of two variables (status and written_size).

Thanks!
Peter

> 
> Paolo

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

[Paolo Bonzini]

On 01/12/2015 13:32, Peter Xu wrote:
> On Tue, Dec 01, 2015 at 10:54:48AM +0100, Paolo Bonzini wrote:
>>
>>
>> On 01/12/2015 04:57, Peter Xu wrote:
>>>>> You need a mutex around the reads of ->status and ->written_size.
>>> Could I avoid using mutex here? Let me try to explain what I
>>> thought.
>>>
>>> The concurrency of this should only happen when:
>>>
>>> - detached dump thread is working (dump thread)
>>> - user queries dump status (main thread)
>>>
>>> What the dump thread is doing should be something like:
>>>
>>> - [start dumping]
>>> - inc written_size
>>> - inc written_size
>>> - ...
>>> - inc written_size
>>> - set ->status to COMPLETED|FAILED
>>> - [end dumping]
>>
>> Yes, it's possible but you need to use atomic_mb_read/atomic_mb_set to
>> write ->status.  Otherwise a CPU can see the write to ->status before
>> some of the final writes to ->written_size.
> 
> Hi, Paolo,
> 
> Thanks to point out. However, would it be confusing to use
> atomic_mb_{read|set} rather than directly use smp_rmb() and
> smp_wmb()? Like:
> 
> In dump thread:
> 
> - inc written_size
> - inc written_size
> - ...
> - inc written_size
> - smp_wmb()
> - atomic_set(status, COMPLETED|FAILED)
> 
> In main thread:
> 
> - atomic_read(status)
> - smp_rmb()
> - read written_size
> 
> What I understand from the doc (seems written by you, thanks :) ) is
> that: atomic_mb_{read|set} is the pair of helper functions for _one_
> specific variable, to make sure its operations are always in order
> as long as we are using atomic_mb_* functions to access it all the
> time. However, in the dump thread case, it's related to read/write
> order of two variables (status and written_size).

atomic_mb_{read,set} does order accesses to the variable against 
all other accesses.  In this case I'd prefer it to smp_wmb/rmb, because 
the writes to written_size are far from the writes to status.

Compare with thread-pool.c:

        req->ret = ret;
        /* Write ret before state.  */
        smp_wmb();
        req->state = THREAD_DONE;

        /* Read state before ret.  */
        smp_rmb();

        /* Schedule ourselves in case elem->common.cb() calls aio_poll() to
         * wait for another request that completed at the same time.
         */
        qemu_bh_schedule(pool->completion_bh);

        elem->common.cb(elem->common.opaque, elem->ret);

It's a matter of taste though.  What you wrote above is certainly okay
as well.

Paolo

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

[Peter Xu]

On Tue, Dec 01, 2015 at 01:37:37PM +0100, Paolo Bonzini wrote:
> atomic_mb_{read,set} does order accesses to the variable against 
> all other accesses.  In this case I'd prefer it to smp_wmb/rmb, because 
> the writes to written_size are far from the writes to status.

Yes... It should be again all the other variables.

> 
> Compare with thread-pool.c:
> 
>         req->ret = ret;
>         /* Write ret before state.  */
>         smp_wmb();
>         req->state = THREAD_DONE;
> 
>         /* Read state before ret.  */
>         smp_rmb();
> 
>         /* Schedule ourselves in case elem->common.cb() calls aio_poll() to
>          * wait for another request that completed at the same time.
>          */
>         qemu_bh_schedule(pool->completion_bh);
> 
>         elem->common.cb(elem->common.opaque, elem->ret);
> 
> It's a matter of taste though.  What you wrote above is certainly okay
> as well.

Thanks for the explanation. Then I will choose smp_*() this time and
post v4 later.

Peter

> 
> Paolo

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

[Paolo Bonzini]

On 01/12/2015 13:45, Peter Xu wrote:
>> > It's a matter of taste though.  What you wrote above is certainly okay
>> > as well.
> Thanks for the explanation. Then I will choose smp_*() this time and
> post v4 later.

In that case, remember to add a comment before each smp_*(), similar to
the one in thread-pool.c.

Paolo

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

[Peter Xu]

On Tue, Dec 01, 2015 at 01:47:08PM +0100, Paolo Bonzini wrote:
> 
> 
> On 01/12/2015 13:45, Peter Xu wrote:
> >> > It's a matter of taste though.  What you wrote above is certainly okay
> >> > as well.
> > Thanks for the explanation. Then I will choose smp_*() this time and
> > post v4 later.
> 
> In that case, remember to add a comment before each smp_*(), similar to
> the one in thread-pool.c.

Ok.

Thanks.
Peter

> 
> Paolo

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

[Eric Blake]

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

On 11/30/2015 09:40 PM, Peter Xu wrote:

>>> +++ b/qmp-commands.hx
>>> @@ -881,7 +881,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,
>>>      },
>>
>> Unrelated hunk.
> 
> Is it ok to have this line here? I just saw it and fixed it. Please
> let me know if I should move it out of this patch.

It's small enough that you could separate that hunk into its own patch
and cc qemu-trivial.

As it is, Marc-Andre has queued patches that get rid of qmp-commands.hx;
so if Markus and I can get through the backlog of qapi patches, this
problem may just disappear on its own.

-- 
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 v3 02/12] dump-guest-memory: add "detach" flag for QMP/HMP interfaces.

[Paolo Bonzini]

On 01/12/2015 03:18, Peter Xu wrote:
> I think (2) is better in term of lines of codes (and also
> clear). However I may need to keep the QMP interface (to keep the
> has_detach parameter in qmp_dump_guest_memory), so I'd like to
> choose (1).

It's also okay to also pass has_detach = true to qmp_dump_guest_memory.

Paolo

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

[Peter Xu]

On Tue, Dec 01, 2015 at 04:09:48PM +0100, Paolo Bonzini wrote:
> 
> 
> On 01/12/2015 03:18, Peter Xu wrote:
> > I think (2) is better in term of lines of codes (and also
> > clear). However I may need to keep the QMP interface (to keep the
> > has_detach parameter in qmp_dump_guest_memory), so I'd like to
> > choose (1).
> 
> It's also okay to also pass has_detach = true to qmp_dump_guest_memory.

Yes. That's more clear. Will put it into v5.

Thanks.
Peter

> 
> Paolo