[PULL 0/8] Dump patches

[PULL 0/8] Dump patches

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

The following changes since commit 6c9ae1ce82b65faa3f266fd103729878cf11e07e:

  Merge tag 'for-upstream' of https://repo.or.cz/qemu/kevin into staging (2023-11-01 06:58:11 +0900)

are available in the Git repository at:

  https://gitlab.com/marcandre.lureau/qemu.git tags/dump-pull-request

for you to fetch changes up to 4023839757b2c01e0350ce92902c6f74dce7d011:

  dump: Drop redundant check for empty dump (2023-11-02 18:40:50 +0400)

----------------------------------------------------------------
dump queue

Hi

The "dump" queue, with:
- [PATCH v3 qemu 0/3] Allow dump-guest-memory to output standard kdump format
- [PATCH v2 0/5] dump: Minor fixes & improvements

----------------------------------------------------------------

Markus Armbruster (5):
  dump: Rename qmp_dump_guest_memory() parameter to match QAPI schema
  dump: Fix g_array_unref(NULL) in dump-guest-memory
  dump: Recognize "fd:" protocols on Windows hosts
  dump: Improve some dump-guest-memory error messages
  dump: Drop redundant check for empty dump

Stephen Brennan (3):
  dump: Pass DumpState to write_ functions
  dump: Allow directly outputting raw kdump format
  dump: Add command interface for kdump-raw formats

 qapi/dump.json        |  24 +++++--
 include/sysemu/dump.h |   3 +-
 dump/dump-hmp-cmds.c  |  21 ++++--
 dump/dump.c           | 158 +++++++++++++++++++++++-------------------
 hmp-commands.hx       |   9 ++-
 5 files changed, 134 insertions(+), 81 deletions(-)

-- 
2.41.0

[PULL 1/8] dump: Pass DumpState to write_ functions

[marcandre.lureau]

From: Stephen Brennan <stephen.s.brennan@oracle.com>

For the next patch, we need a reference to DumpState when writing data.

Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Message-Id: <20230918233233.1431858-2-stephen.s.brennan@oracle.com>
---
 include/sysemu/dump.h |  2 +-
 dump/dump.c           | 40 ++++++++++++++++++++--------------------
 2 files changed, 21 insertions(+), 21 deletions(-)

diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
index 7008d43d04..e27af8fb34 100644
--- a/include/sysemu/dump.h
+++ b/include/sysemu/dump.h
@@ -137,7 +137,7 @@ typedef struct QEMU_PACKED KdumpSubHeader64 {
 } KdumpSubHeader64;
 
 typedef struct DataCache {
-    int fd;             /* fd of the file where to write the cached data */
+    DumpState *state;   /* dump state related to this data */
     uint8_t *buf;       /* buffer for cached data */
     size_t buf_size;    /* size of the buf */
     size_t data_size;   /* size of cached data in buf */
diff --git a/dump/dump.c b/dump/dump.c
index d355ada62e..eec34c4738 100644
--- a/dump/dump.c
+++ b/dump/dump.c
@@ -809,7 +809,7 @@ static void create_vmcore(DumpState *s, Error **errp)
     dump_end(s, errp);
 }
 
-static int write_start_flat_header(int fd)
+static int write_start_flat_header(DumpState *s)
 {
     MakedumpfileHeader *mh;
     int ret = 0;
@@ -824,7 +824,7 @@ static int write_start_flat_header(int fd)
     mh->version = cpu_to_be64(VERSION_FLAT_HEADER);
 
     size_t written_size;
-    written_size = qemu_write_full(fd, mh, MAX_SIZE_MDF_HEADER);
+    written_size = qemu_write_full(s->fd, mh, MAX_SIZE_MDF_HEADER);
     if (written_size != MAX_SIZE_MDF_HEADER) {
         ret = -1;
     }
@@ -833,7 +833,7 @@ static int write_start_flat_header(int fd)
     return ret;
 }
 
-static int write_end_flat_header(int fd)
+static int write_end_flat_header(DumpState *s)
 {
     MakedumpfileDataHeader mdh;
 
@@ -841,7 +841,7 @@ static int write_end_flat_header(int fd)
     mdh.buf_size = END_FLAG_FLAT_HEADER;
 
     size_t written_size;
-    written_size = qemu_write_full(fd, &mdh, sizeof(mdh));
+    written_size = qemu_write_full(s->fd, &mdh, sizeof(mdh));
     if (written_size != sizeof(mdh)) {
         return -1;
     }
@@ -849,7 +849,7 @@ static int write_end_flat_header(int fd)
     return 0;
 }
 
-static int write_buffer(int fd, off_t offset, const void *buf, size_t size)
+static int write_buffer(DumpState *s, off_t offset, const void *buf, size_t size)
 {
     size_t written_size;
     MakedumpfileDataHeader mdh;
@@ -857,12 +857,12 @@ static int write_buffer(int fd, off_t offset, const void *buf, size_t size)
     mdh.offset = cpu_to_be64(offset);
     mdh.buf_size = cpu_to_be64(size);
 
-    written_size = qemu_write_full(fd, &mdh, sizeof(mdh));
+    written_size = qemu_write_full(s->fd, &mdh, sizeof(mdh));
     if (written_size != sizeof(mdh)) {
         return -1;
     }
 
-    written_size = qemu_write_full(fd, buf, size);
+    written_size = qemu_write_full(s->fd, buf, size);
     if (written_size != size) {
         return -1;
     }
@@ -982,7 +982,7 @@ static void create_header32(DumpState *s, Error **errp)
 #endif
     dh->status = cpu_to_dump32(s, status);
 
-    if (write_buffer(s->fd, 0, dh, size) < 0) {
+    if (write_buffer(s, 0, dh, size) < 0) {
         error_setg(errp, "dump: failed to write disk dump header");
         goto out;
     }
@@ -1012,7 +1012,7 @@ static void create_header32(DumpState *s, Error **errp)
     kh->offset_note = cpu_to_dump64(s, offset_note);
     kh->note_size = cpu_to_dump32(s, s->note_size);
 
-    if (write_buffer(s->fd, DISKDUMP_HEADER_BLOCKS *
+    if (write_buffer(s, DISKDUMP_HEADER_BLOCKS *
                      block_size, kh, size) < 0) {
         error_setg(errp, "dump: failed to write kdump sub header");
         goto out;
@@ -1027,7 +1027,7 @@ static void create_header32(DumpState *s, Error **errp)
     if (*errp) {
         goto out;
     }
-    if (write_buffer(s->fd, offset_note, s->note_buf,
+    if (write_buffer(s, offset_note, s->note_buf,
                      s->note_size) < 0) {
         error_setg(errp, "dump: failed to write notes");
         goto out;
@@ -1093,7 +1093,7 @@ static void create_header64(DumpState *s, Error **errp)
 #endif
     dh->status = cpu_to_dump32(s, status);
 
-    if (write_buffer(s->fd, 0, dh, size) < 0) {
+    if (write_buffer(s, 0, dh, size) < 0) {
         error_setg(errp, "dump: failed to write disk dump header");
         goto out;
     }
@@ -1123,7 +1123,7 @@ static void create_header64(DumpState *s, Error **errp)
     kh->offset_note = cpu_to_dump64(s, offset_note);
     kh->note_size = cpu_to_dump64(s, s->note_size);
 
-    if (write_buffer(s->fd, DISKDUMP_HEADER_BLOCKS *
+    if (write_buffer(s, DISKDUMP_HEADER_BLOCKS *
                      block_size, kh, size) < 0) {
         error_setg(errp, "dump: failed to write kdump sub header");
         goto out;
@@ -1139,7 +1139,7 @@ static void create_header64(DumpState *s, Error **errp)
         goto out;
     }
 
-    if (write_buffer(s->fd, offset_note, s->note_buf,
+    if (write_buffer(s, offset_note, s->note_buf,
                      s->note_size) < 0) {
         error_setg(errp, "dump: failed to write notes");
         goto out;
@@ -1204,7 +1204,7 @@ static int set_dump_bitmap(uint64_t last_pfn, uint64_t pfn, bool value,
     while (old_offset < new_offset) {
         /* calculate the offset and write dump_bitmap */
         offset_bitmap1 = s->offset_dump_bitmap + old_offset;
-        if (write_buffer(s->fd, offset_bitmap1, buf,
+        if (write_buffer(s, offset_bitmap1, buf,
                          bitmap_bufsize) < 0) {
             return -1;
         }
@@ -1212,7 +1212,7 @@ static int set_dump_bitmap(uint64_t last_pfn, uint64_t pfn, bool value,
         /* dump level 1 is chosen, so 1st and 2nd bitmap are same */
         offset_bitmap2 = s->offset_dump_bitmap + s->len_dump_bitmap +
                          old_offset;
-        if (write_buffer(s->fd, offset_bitmap2, buf,
+        if (write_buffer(s, offset_bitmap2, buf,
                          bitmap_bufsize) < 0) {
             return -1;
         }
@@ -1380,7 +1380,7 @@ out:
 static void prepare_data_cache(DataCache *data_cache, DumpState *s,
                                off_t offset)
 {
-    data_cache->fd = s->fd;
+    data_cache->state = s;
     data_cache->data_size = 0;
     data_cache->buf_size = 4 * dump_bitmap_get_bufsize(s);
     data_cache->buf = g_malloc0(data_cache->buf_size);
@@ -1399,11 +1399,11 @@ static int write_cache(DataCache *dc, const void *buf, size_t size,
     /*
      * if flag_sync is set, synchronize data in dc->buf into vmcore.
      * otherwise check if the space is enough for caching data in buf, if not,
-     * write the data in dc->buf to dc->fd and reset dc->buf
+     * write the data in dc->buf to dc->state->fd and reset dc->buf
      */
     if ((!flag_sync && dc->data_size + size > dc->buf_size) ||
         (flag_sync && dc->data_size > 0)) {
-        if (write_buffer(dc->fd, dc->offset, dc->buf, dc->data_size) < 0) {
+        if (write_buffer(dc->state, dc->offset, dc->buf, dc->data_size) < 0) {
             return -1;
         }
 
@@ -1644,7 +1644,7 @@ static void create_kdump_vmcore(DumpState *s, Error **errp)
      *  +------------------------------------------+
      */
 
-    ret = write_start_flat_header(s->fd);
+    ret = write_start_flat_header(s);
     if (ret < 0) {
         error_setg(errp, "dump: failed to write start flat header");
         return;
@@ -1665,7 +1665,7 @@ static void create_kdump_vmcore(DumpState *s, Error **errp)
         return;
     }
 
-    ret = write_end_flat_header(s->fd);
+    ret = write_end_flat_header(s);
     if (ret < 0) {
         error_setg(errp, "dump: failed to write end flat header");
         return;
-- 
2.41.0

[PULL 2/8] dump: Allow directly outputting raw kdump format

[marcandre.lureau]

From: Stephen Brennan <stephen.s.brennan@oracle.com>

The flattened format (currently output by QEMU) is used by makedumpfile
only when it is outputting a vmcore to a file which is not seekable. The
flattened format functions essentially as a set of instructions of the
form "seek to the given offset, then write the given bytes out".

The flattened format can be reconstructed using makedumpfile -R, or
makedumpfile-R.pl, but it is a slow process because it requires copying
the entire vmcore. The flattened format can also be directly read by
crash, but still, it requires a lengthy reassembly phase.

To sum up, the flattened format is not an ideal one: it should only be
used on files which are actually not seekable. This is the exact
strategy which makedumpfile uses, as seen in the implementation of
"write_buffer()" in makedumpfile [1]. However, QEMU has always used the
flattened format. For compatibility it is best not to change the default
output format without warning. So, add a flag to DumpState which changes
the output to use the normal (i.e. raw) format. This flag will be added
to the QMP and HMP commands in the next change.

[1]: https://github.com/makedumpfile/makedumpfile/blob/f23bb943568188a2746dbf9b6692668f5a2ac3b6/makedumpfile.c#L5008-L5040

Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
[ Marc-André: replace loff_t with off_t ]
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Message-Id: <20230918233233.1431858-3-stephen.s.brennan@oracle.com>
---
 include/sysemu/dump.h |  1 +
 dump/dump.c           | 32 +++++++++++++++++++++++++-------
 2 files changed, 26 insertions(+), 7 deletions(-)

diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
index e27af8fb34..d702854853 100644
--- a/include/sysemu/dump.h
+++ b/include/sysemu/dump.h
@@ -157,6 +157,7 @@ typedef struct DumpState {
     MemoryMappingList list;
     bool resume;
     bool detached;
+    bool kdump_raw;
     hwaddr memory_offset;
     int fd;
 
diff --git a/dump/dump.c b/dump/dump.c
index eec34c4738..0f913e1f5c 100644
--- a/dump/dump.c
+++ b/dump/dump.c
@@ -814,6 +814,10 @@ static int write_start_flat_header(DumpState *s)
     MakedumpfileHeader *mh;
     int ret = 0;
 
+    if (s->kdump_raw) {
+        return 0;
+    }
+
     QEMU_BUILD_BUG_ON(sizeof *mh > MAX_SIZE_MDF_HEADER);
     mh = g_malloc0(MAX_SIZE_MDF_HEADER);
 
@@ -837,6 +841,10 @@ static int write_end_flat_header(DumpState *s)
 {
     MakedumpfileDataHeader mdh;
 
+    if (s->kdump_raw) {
+        return 0;
+    }
+
     mdh.offset = END_FLAG_FLAT_HEADER;
     mdh.buf_size = END_FLAG_FLAT_HEADER;
 
@@ -853,13 +861,21 @@ static int write_buffer(DumpState *s, off_t offset, const void *buf, size_t size
 {
     size_t written_size;
     MakedumpfileDataHeader mdh;
+    off_t seek_loc;
 
-    mdh.offset = cpu_to_be64(offset);
-    mdh.buf_size = cpu_to_be64(size);
+    if (s->kdump_raw) {
+        seek_loc = lseek(s->fd, offset, SEEK_SET);
+        if (seek_loc == (off_t) -1) {
+            return -1;
+        }
+    } else {
+        mdh.offset = cpu_to_be64(offset);
+        mdh.buf_size = cpu_to_be64(size);
 
-    written_size = qemu_write_full(s->fd, &mdh, sizeof(mdh));
-    if (written_size != sizeof(mdh)) {
-        return -1;
+        written_size = qemu_write_full(s->fd, &mdh, sizeof(mdh));
+        if (written_size != sizeof(mdh)) {
+            return -1;
+        }
     }
 
     written_size = qemu_write_full(s->fd, buf, size);
@@ -1775,7 +1791,8 @@ static void vmcoreinfo_update_phys_base(DumpState *s)
 
 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)
+                      int64_t begin, int64_t length, bool kdump_raw,
+                      Error **errp)
 {
     ERRP_GUARD();
     VMCoreInfoState *vmci = vmcoreinfo_find();
@@ -1786,6 +1803,7 @@ static void dump_init(DumpState *s, int fd, bool has_format,
     s->has_format = has_format;
     s->format = format;
     s->written_size = 0;
+    s->kdump_raw = kdump_raw;
 
     /* kdump-compressed is conflict with paging and filter */
     if (has_format && format != DUMP_GUEST_MEMORY_FORMAT_ELF) {
@@ -2168,7 +2186,7 @@ void qmp_dump_guest_memory(bool paging, const char *file,
     dump_state_prepare(s);
 
     dump_init(s, fd, has_format, format, paging, has_begin,
-              begin, length, errp);
+              begin, length, false, errp);
     if (*errp) {
         qatomic_set(&s->status, DUMP_STATUS_FAILED);
         return;
-- 
2.41.0

[PULL 3/8] dump: Add command interface for kdump-raw formats

[marcandre.lureau]

From: Stephen Brennan <stephen.s.brennan@oracle.com>

The QMP dump API represents the dump format as an enumeration. Add three
new enumerators, one for each supported kdump compression, each named
"kdump-raw-*".

For the HMP command line, rather than adding a new flag corresponding to
each format, it seems more human-friendly to add a single flag "-R" to
switch the kdump formats to "raw" mode. The choice of "-R" also
correlates nicely to the "makedumpfile -R" option, which would serve to
reassemble a flattened vmcore.

Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
[ Marc-André: replace loff_t with off_t, indent fixes ]
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Message-Id: <20230918233233.1431858-4-stephen.s.brennan@oracle.com>
---
 qapi/dump.json       | 24 ++++++++++++++++++++----
 dump/dump-hmp-cmds.c | 21 +++++++++++++++++----
 dump/dump.c          | 33 ++++++++++++++++++++++++++++++++-
 hmp-commands.hx      |  9 +++++++--
 4 files changed, 76 insertions(+), 11 deletions(-)

diff --git a/qapi/dump.json b/qapi/dump.json
index 4ae1f722a9..5cbc237ad9 100644
--- a/qapi/dump.json
+++ b/qapi/dump.json
@@ -15,11 +15,23 @@
 #
 # @elf: elf format
 #
-# @kdump-zlib: kdump-compressed format with zlib-compressed
+# @kdump-zlib: makedumpfile flattened, kdump-compressed format with zlib
+#     compression
 #
-# @kdump-lzo: kdump-compressed format with lzo-compressed
+# @kdump-lzo: makedumpfile flattened, kdump-compressed format with lzo
+#     compression
 #
-# @kdump-snappy: kdump-compressed format with snappy-compressed
+# @kdump-snappy: makedumpfile flattened, kdump-compressed format with snappy
+#     compression
+#
+# @kdump-raw-zlib: raw assembled kdump-compressed format with zlib compression
+#     (since 8.2)
+#
+# @kdump-raw-lzo: raw assembled kdump-compressed format with lzo compression
+#     (since 8.2)
+#
+# @kdump-raw-snappy: raw assembled kdump-compressed format with snappy
+#     compression (since 8.2)
 #
 # @win-dmp: Windows full crashdump format, can be used instead of ELF
 #     converting (since 2.13)
@@ -27,7 +39,11 @@
 # Since: 2.0
 ##
 { 'enum': 'DumpGuestMemoryFormat',
-  'data': [ 'elf', 'kdump-zlib', 'kdump-lzo', 'kdump-snappy', 'win-dmp' ] }
+  'data': [
+      'elf',
+      'kdump-zlib', 'kdump-lzo', 'kdump-snappy',
+      'kdump-raw-zlib', 'kdump-raw-lzo', 'kdump-raw-snappy',
+      'win-dmp' ] }
 
 ##
 # @dump-guest-memory:
diff --git a/dump/dump-hmp-cmds.c b/dump/dump-hmp-cmds.c
index b038785fee..b428ec33df 100644
--- a/dump/dump-hmp-cmds.c
+++ b/dump/dump-hmp-cmds.c
@@ -19,6 +19,7 @@ void hmp_dump_guest_memory(Monitor *mon, const QDict *qdict)
     bool paging = qdict_get_try_bool(qdict, "paging", false);
     bool zlib = qdict_get_try_bool(qdict, "zlib", false);
     bool lzo = qdict_get_try_bool(qdict, "lzo", false);
+    bool raw = qdict_get_try_bool(qdict, "raw", false);
     bool snappy = qdict_get_try_bool(qdict, "snappy", false);
     const char *file = qdict_get_str(qdict, "filename");
     bool has_begin = qdict_haskey(qdict, "begin");
@@ -40,16 +41,28 @@ void hmp_dump_guest_memory(Monitor *mon, const QDict *qdict)
         dump_format = DUMP_GUEST_MEMORY_FORMAT_WIN_DMP;
     }
 
-    if (zlib) {
-        dump_format = DUMP_GUEST_MEMORY_FORMAT_KDUMP_ZLIB;
+    if (zlib && raw) {
+        if (raw) {
+            dump_format = DUMP_GUEST_MEMORY_FORMAT_KDUMP_RAW_ZLIB;
+        } else {
+            dump_format = DUMP_GUEST_MEMORY_FORMAT_KDUMP_ZLIB;
+        }
     }
 
     if (lzo) {
-        dump_format = DUMP_GUEST_MEMORY_FORMAT_KDUMP_LZO;
+        if (raw) {
+            dump_format = DUMP_GUEST_MEMORY_FORMAT_KDUMP_RAW_LZO;
+        } else {
+            dump_format = DUMP_GUEST_MEMORY_FORMAT_KDUMP_LZO;
+        }
     }
 
     if (snappy) {
-        dump_format = DUMP_GUEST_MEMORY_FORMAT_KDUMP_SNAPPY;
+        if (raw) {
+            dump_format = DUMP_GUEST_MEMORY_FORMAT_KDUMP_RAW_SNAPPY;
+        } else {
+            dump_format = DUMP_GUEST_MEMORY_FORMAT_KDUMP_SNAPPY;
+        }
     }
 
     if (has_begin) {
diff --git a/dump/dump.c b/dump/dump.c
index 0f913e1f5c..44b27fef45 100644
--- a/dump/dump.c
+++ b/dump/dump.c
@@ -2090,6 +2090,7 @@ void qmp_dump_guest_memory(bool paging, const char *file,
     int fd = -1;
     DumpState *s;
     bool detach_p = false;
+    bool kdump_raw = false;
 
     if (runstate_check(RUN_STATE_INMIGRATE)) {
         error_setg(errp, "Dump not allowed during incoming migration.");
@@ -2103,6 +2104,29 @@ void qmp_dump_guest_memory(bool paging, const char *file,
         return;
     }
 
+    /*
+     * externally, we represent kdump-raw-* as separate formats, but internally
+     * they are handled the same, except for the "raw" flag
+     */
+    if (has_format) {
+        switch (format) {
+        case DUMP_GUEST_MEMORY_FORMAT_KDUMP_RAW_ZLIB:
+            format = DUMP_GUEST_MEMORY_FORMAT_KDUMP_ZLIB;
+            kdump_raw = true;
+            break;
+        case DUMP_GUEST_MEMORY_FORMAT_KDUMP_RAW_LZO:
+            format = DUMP_GUEST_MEMORY_FORMAT_KDUMP_LZO;
+            kdump_raw = true;
+            break;
+        case DUMP_GUEST_MEMORY_FORMAT_KDUMP_RAW_SNAPPY:
+            format = DUMP_GUEST_MEMORY_FORMAT_KDUMP_SNAPPY;
+            kdump_raw = true;
+            break;
+        default:
+            break;
+        }
+    }
+
     /*
      * kdump-compressed format need the whole memory dumped, so paging or
      * filter is not supported here.
@@ -2166,6 +2190,10 @@ void qmp_dump_guest_memory(bool paging, const char *file,
         error_setg(errp, QERR_INVALID_PARAMETER, "protocol");
         return;
     }
+    if (kdump_raw && lseek(fd, 0, SEEK_CUR) == (off_t) -1) {
+        error_setg(errp, "kdump-raw formats require a seekable file");
+        return;
+    }
 
     if (!dump_migration_blocker) {
         error_setg(&dump_migration_blocker,
@@ -2186,7 +2214,7 @@ void qmp_dump_guest_memory(bool paging, const char *file,
     dump_state_prepare(s);
 
     dump_init(s, fd, has_format, format, paging, has_begin,
-              begin, length, false, errp);
+              begin, length, kdump_raw, errp);
     if (*errp) {
         qatomic_set(&s->status, DUMP_STATUS_FAILED);
         return;
@@ -2214,15 +2242,18 @@ DumpGuestMemoryCapability *qmp_query_dump_guest_memory_capability(Error **errp)
 
     /* kdump-zlib is always available */
     QAPI_LIST_APPEND(tail, DUMP_GUEST_MEMORY_FORMAT_KDUMP_ZLIB);
+    QAPI_LIST_APPEND(tail, DUMP_GUEST_MEMORY_FORMAT_KDUMP_RAW_ZLIB);
 
     /* add new item if kdump-lzo is available */
 #ifdef CONFIG_LZO
     QAPI_LIST_APPEND(tail, DUMP_GUEST_MEMORY_FORMAT_KDUMP_LZO);
+    QAPI_LIST_APPEND(tail, DUMP_GUEST_MEMORY_FORMAT_KDUMP_RAW_LZO);
 #endif
 
     /* add new item if kdump-snappy is available */
 #ifdef CONFIG_SNAPPY
     QAPI_LIST_APPEND(tail, DUMP_GUEST_MEMORY_FORMAT_KDUMP_SNAPPY);
+    QAPI_LIST_APPEND(tail, DUMP_GUEST_MEMORY_FORMAT_KDUMP_RAW_SNAPPY);
 #endif
 
     if (win_dump_available(NULL)) {
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 63eac22734..c0a27688b6 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1085,14 +1085,16 @@ ERST
 
     {
         .name       = "dump-guest-memory",
-        .args_type  = "paging:-p,detach:-d,windmp:-w,zlib:-z,lzo:-l,snappy:-s,filename:F,begin:l?,length:l?",
-        .params     = "[-p] [-d] [-z|-l|-s|-w] filename [begin length]",
+        .args_type  = "paging:-p,detach:-d,windmp:-w,zlib:-z,lzo:-l,snappy:-s,raw:-R,filename:F,begin:l?,length:l?",
+        .params     = "[-p] [-d] [-z|-l|-s|-w] [-R] 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"
+                      "-R: when using kdump (-z, -l, -s), use raw rather than makedumpfile-flattened\n\t\t\t"
+                      "    format\n\t\t\t"
                       "-w: dump in Windows crashdump format (can be used instead of ELF-dump converting),\n\t\t\t"
                       "    for Windows x86 and x64 guests with vmcoreinfo driver only.\n\t\t\t"
                       "begin: the starting physical address.\n\t\t\t"
@@ -1115,6 +1117,9 @@ SRST
     dump in kdump-compressed format, with lzo compression.
   ``-s``
     dump in kdump-compressed format, with snappy compression.
+  ``-R``
+    when using kdump (-z, -l, -s), use raw rather than makedumpfile-flattened
+    format
   ``-w``
     dump in Windows crashdump format (can be used instead of ELF-dump converting),
     for Windows x64 guests with vmcoreinfo driver only
-- 
2.41.0

Re: [PULL 3/8] dump: Add command interface for kdump-raw formats

[Peter Maydell]

On Fri, 3 Nov 2023 at 07:02, <marcandre.lureau@redhat.com> wrote:
>
> From: Stephen Brennan <stephen.s.brennan@oracle.com>
>
> The QMP dump API represents the dump format as an enumeration. Add three
> new enumerators, one for each supported kdump compression, each named
> "kdump-raw-*".
>
> For the HMP command line, rather than adding a new flag corresponding to
> each format, it seems more human-friendly to add a single flag "-R" to
> switch the kdump formats to "raw" mode. The choice of "-R" also
> correlates nicely to the "makedumpfile -R" option, which would serve to
> reassemble a flattened vmcore.
>
> Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com>
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> [ Marc-André: replace loff_t with off_t, indent fixes ]
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Message-Id: <20230918233233.1431858-4-stephen.s.brennan@oracle.com>

Hi; Coverity points out some issues in this commit:

> diff --git a/dump/dump-hmp-cmds.c b/dump/dump-hmp-cmds.c
> index b038785fee..b428ec33df 100644
> --- a/dump/dump-hmp-cmds.c
> +++ b/dump/dump-hmp-cmds.c
> @@ -19,6 +19,7 @@ void hmp_dump_guest_memory(Monitor *mon, const QDict *qdict)
>      bool paging = qdict_get_try_bool(qdict, "paging", false);
>      bool zlib = qdict_get_try_bool(qdict, "zlib", false);
>      bool lzo = qdict_get_try_bool(qdict, "lzo", false);
> +    bool raw = qdict_get_try_bool(qdict, "raw", false);
>      bool snappy = qdict_get_try_bool(qdict, "snappy", false);
>      const char *file = qdict_get_str(qdict, "filename");
>      bool has_begin = qdict_haskey(qdict, "begin");
> @@ -40,16 +41,28 @@ void hmp_dump_guest_memory(Monitor *mon, const QDict *qdict)
>          dump_format = DUMP_GUEST_MEMORY_FORMAT_WIN_DMP;
>      }
>
> -    if (zlib) {
> -        dump_format = DUMP_GUEST_MEMORY_FORMAT_KDUMP_ZLIB;
> +    if (zlib && raw) {
> +        if (raw) {
> +            dump_format = DUMP_GUEST_MEMORY_FORMAT_KDUMP_RAW_ZLIB;
> +        } else {
> +            dump_format = DUMP_GUEST_MEMORY_FORMAT_KDUMP_ZLIB;

This is dead code, beacuse the outer conditional "(zlib && raw)"
ensures that raw can't be false here. What was the intention here?
(CID 1523841)

> +        }
>      }
>
>      if (lzo) {
> -        dump_format = DUMP_GUEST_MEMORY_FORMAT_KDUMP_LZO;
> +        if (raw) {
> +            dump_format = DUMP_GUEST_MEMORY_FORMAT_KDUMP_RAW_LZO;
> +        } else {
> +            dump_format = DUMP_GUEST_MEMORY_FORMAT_KDUMP_LZO;
> +        }
>      }
>
>      if (snappy) {
> -        dump_format = DUMP_GUEST_MEMORY_FORMAT_KDUMP_SNAPPY;
> +        if (raw) {
> +            dump_format = DUMP_GUEST_MEMORY_FORMAT_KDUMP_RAW_SNAPPY;
> +        } else {
> +            dump_format = DUMP_GUEST_MEMORY_FORMAT_KDUMP_SNAPPY;
> +        }
>      }
>
>      if (has_begin) {


> @@ -2166,6 +2190,10 @@ void qmp_dump_guest_memory(bool paging, const char *file,
>          error_setg(errp, QERR_INVALID_PARAMETER, "protocol");
>          return;
>      }
> +    if (kdump_raw && lseek(fd, 0, SEEK_CUR) == (off_t) -1) {
> +        error_setg(errp, "kdump-raw formats require a seekable file");
> +        return;
> +    }

This error-exit return path forgets to close(fd), so we leak
the file descriptor. (CID 1523842)

>
>      if (!dump_migration_blocker) {
>          error_setg(&dump_migration_blocker,

thanks
-- PMM

Re: [PULL 3/8] dump: Add command interface for kdump-raw formats

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Fri, 3 Nov 2023 at 07:02, <marcandre.lureau@redhat.com> wrote:
>>
>> From: Stephen Brennan <stephen.s.brennan@oracle.com>
>>
>> The QMP dump API represents the dump format as an enumeration. Add three
>> new enumerators, one for each supported kdump compression, each named
>> "kdump-raw-*".
>>
>> For the HMP command line, rather than adding a new flag corresponding to
>> each format, it seems more human-friendly to add a single flag "-R" to
>> switch the kdump formats to "raw" mode. The choice of "-R" also
>> correlates nicely to the "makedumpfile -R" option, which would serve to
>> reassemble a flattened vmcore.
>>
>> Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com>
>> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
>> [ Marc-André: replace loff_t with off_t, indent fixes ]
>> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> Message-Id: <20230918233233.1431858-4-stephen.s.brennan@oracle.com>
>
> Hi; Coverity points out some issues in this commit:

Proposed fixes:

    Subject: [PATCH 0/2] dump: Fix issues flagged by Coverity
    Message-ID: <20231108053731.112043-1-armbru@redhat.com>

Re: [PULL 3/8] dump: Add command interface for kdump-raw formats

[Stephen Brennan]

Markus Armbruster <armbru@redhat.com> writes:
> Peter Maydell <peter.maydell@linaro.org> writes:
>
>> On Fri, 3 Nov 2023 at 07:02, <marcandre.lureau@redhat.com> wrote:
>>>
>>> From: Stephen Brennan <stephen.s.brennan@oracle.com>
>>>
>>> The QMP dump API represents the dump format as an enumeration. Add three
>>> new enumerators, one for each supported kdump compression, each named
>>> "kdump-raw-*".
>>>
>>> For the HMP command line, rather than adding a new flag corresponding to
>>> each format, it seems more human-friendly to add a single flag "-R" to
>>> switch the kdump formats to "raw" mode. The choice of "-R" also
>>> correlates nicely to the "makedumpfile -R" option, which would serve to
>>> reassemble a flattened vmcore.
>>>
>>> Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com>
>>> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
>>> [ Marc-André: replace loff_t with off_t, indent fixes ]
>>> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>> Message-Id: <20230918233233.1431858-4-stephen.s.brennan@oracle.com>
>>
>> Hi; Coverity points out some issues in this commit:
>
> Proposed fixes:
>
>     Subject: [PATCH 0/2] dump: Fix issues flagged by Coverity
>     Message-ID: <20231108053731.112043-1-armbru@redhat.com>

Closing the loop on this, these fixes were exactly right. Sorry for the
extra work here folks.

Stephen

[PULL 4/8] dump: Rename qmp_dump_guest_memory() parameter to match QAPI schema

[marcandre.lureau]

From: Markus Armbruster <armbru@redhat.com>

The name of the second parameter differs between QAPI schema and C
implementation: it's @protocol in the former and @file in the latter.
Potentially confusing.  Change the C implementation to match the QAPI
schema.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Message-Id: <20231031104531.3169721-2-armbru@redhat.com>
---
 dump/dump.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/dump/dump.c b/dump/dump.c
index 44b27fef45..9cdb4a2bf8 100644
--- a/dump/dump.c
+++ b/dump/dump.c
@@ -2079,11 +2079,12 @@ DumpQueryResult *qmp_query_dump(Error **errp)
     return result;
 }
 
-void qmp_dump_guest_memory(bool paging, const char *file,
+void qmp_dump_guest_memory(bool paging, const char *protocol,
                            bool has_detach, bool detach,
-                           bool has_begin, int64_t begin, bool has_length,
-                           int64_t length, bool has_format,
-                           DumpGuestMemoryFormat format, Error **errp)
+                           bool has_begin, int64_t begin,
+                           bool has_length, int64_t length,
+                           bool has_format, DumpGuestMemoryFormat format,
+                           Error **errp)
 {
     ERRP_GUARD();
     const char *p;
@@ -2170,7 +2171,7 @@ void qmp_dump_guest_memory(bool paging, const char *file,
     }
 
 #if !defined(WIN32)
-    if (strstart(file, "fd:", &p)) {
+    if (strstart(protocol, "fd:", &p)) {
         fd = monitor_get_fd(monitor_cur(), p, errp);
         if (fd == -1) {
             return;
@@ -2178,7 +2179,7 @@ void qmp_dump_guest_memory(bool paging, const char *file,
     }
 #endif
 
-    if  (strstart(file, "file:", &p)) {
+    if  (strstart(protocol, "file:", &p)) {
         fd = qemu_open_old(p, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, S_IRUSR);
         if (fd < 0) {
             error_setg_file_open(errp, errno, p);
-- 
2.41.0

[PULL 5/8] dump: Fix g_array_unref(NULL) in dump-guest-memory

[marcandre.lureau]

From: Markus Armbruster <armbru@redhat.com>

When dump_init()'s check for non-zero @length fails, dump_cleanup()
passes null s->string_table_buf to g_array_unref(), which spews "GLib:
g_array_unref: assertion 'array' failed" to stderr.

Guard the g_array_unref().

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Message-Id: <20231031104531.3169721-3-armbru@redhat.com>
---
 dump/dump.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/dump/dump.c b/dump/dump.c
index 9cdb4a2bf8..24c829e705 100644
--- a/dump/dump.c
+++ b/dump/dump.c
@@ -100,7 +100,7 @@ static int dump_cleanup(DumpState *s)
     memory_mapping_list_free(&s->list);
     close(s->fd);
     g_free(s->guest_note);
-    g_array_unref(s->string_table_buf);
+    g_clear_pointer(&s->string_table_buf, g_array_unref);
     s->guest_note = NULL;
     if (s->resume) {
         if (s->detached) {
-- 
2.41.0

[PULL 6/8] dump: Recognize "fd:" protocols on Windows hosts

[marcandre.lureau]

From: Markus Armbruster <armbru@redhat.com>

A few QMP command can work with named file descriptors.

The only way to create a named file descriptor used to be QMP command
getfd, which only works on POSIX hosts.  Thus, named file descriptors
were actually usable only there.

They became usable on Windows hosts when we added QMP command
get-win32-socket (commit 4cda177c601 "qmp: add 'get-win32-socket'").

Except in dump-guest-memory, because qmp_dump_guest_memory() compiles
its named file descriptor code only #if !defined(WIN32).

Compile it unconditionally, like we do for the other commands
supporting them.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Message-Id: <20231031104531.3169721-4-armbru@redhat.com>
---
 dump/dump.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/dump/dump.c b/dump/dump.c
index 24c829e705..2d0b5bd22b 100644
--- a/dump/dump.c
+++ b/dump/dump.c
@@ -2170,14 +2170,12 @@ void qmp_dump_guest_memory(bool paging, const char *protocol,
         return;
     }
 
-#if !defined(WIN32)
     if (strstart(protocol, "fd:", &p)) {
         fd = monitor_get_fd(monitor_cur(), p, errp);
         if (fd == -1) {
             return;
         }
     }
-#endif
 
     if  (strstart(protocol, "file:", &p)) {
         fd = qemu_open_old(p, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, S_IRUSR);
-- 
2.41.0

[PULL 7/8] dump: Improve some dump-guest-memory error messages

[marcandre.lureau]

From: Markus Armbruster <armbru@redhat.com>

Zero @length is rejected with "Invalid parameter 'length'".  Improve
to "parameter 'length' expects a non-zero length".

qemu_open_old() is a wrapper around qemu_open_internal() that throws
away error information.  Switch to the wrapper that doesn't:
qemu_create().  Example improvement:

    (qemu) dump-guest-memory /dev/fdset/x 0 1
    Error: Could not open '/dev/fdset/x': Invalid argument

becomes

    Error: Could not parse fdset /dev/fdset/x

@protocol values not starting with "fd:" or "file:" are rejected with
"Invalid parameter 'protocol'".  Improve to "parameter 'protocol' must
start with 'file:' or 'fd:'".

While there, make the conditional checking @protocol a little more
obvious.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Message-Id: <20231031104531.3169721-5-armbru@redhat.com>
---
 dump/dump.c | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/dump/dump.c b/dump/dump.c
index 2d0b5bd22b..ec2cfcf9f7 100644
--- a/dump/dump.c
+++ b/dump/dump.c
@@ -1828,7 +1828,7 @@ static void dump_init(DumpState *s, int fd, bool has_format,
 
     s->fd = fd;
     if (has_filter && !length) {
-        error_setg(errp, QERR_INVALID_PARAMETER, "length");
+        error_setg(errp, "parameter 'length' expects a non-zero size");
         goto cleanup;
     }
     s->filter_area_begin = begin;
@@ -2088,7 +2088,7 @@ void qmp_dump_guest_memory(bool paging, const char *protocol,
 {
     ERRP_GUARD();
     const char *p;
-    int fd = -1;
+    int fd;
     DumpState *s;
     bool detach_p = false;
     bool kdump_raw = false;
@@ -2175,18 +2175,14 @@ void qmp_dump_guest_memory(bool paging, const char *protocol,
         if (fd == -1) {
             return;
         }
-    }
-
-    if  (strstart(protocol, "file:", &p)) {
-        fd = qemu_open_old(p, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, S_IRUSR);
+    } else if  (strstart(protocol, "file:", &p)) {
+        fd = qemu_create(p, O_WRONLY | O_TRUNC | O_BINARY, S_IRUSR, errp);
         if (fd < 0) {
-            error_setg_file_open(errp, errno, p);
             return;
         }
-    }
-
-    if (fd == -1) {
-        error_setg(errp, QERR_INVALID_PARAMETER, "protocol");
+    } else {
+        error_setg(errp,
+                   "parameter 'protocol' must start with 'file:' or 'fd:'");
         return;
     }
     if (kdump_raw && lseek(fd, 0, SEEK_CUR) == (off_t) -1) {
-- 
2.41.0

[PULL 8/8] dump: Drop redundant check for empty dump

[marcandre.lureau]

From: Markus Armbruster <armbru@redhat.com>

dump_init() first computes the size of the dump, taking the filter
area into account, and fails if its zero.  It then looks for memory in
the filter area, and fails if there is none.

This is redundant: if the size of the dump is zero, there is no
memory, and vice versa.  Delete this check.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Message-Id: <20231031104531.3169721-6-armbru@redhat.com>
---
 dump/dump.c | 26 --------------------------
 1 file changed, 26 deletions(-)

diff --git a/dump/dump.c b/dump/dump.c
index ec2cfcf9f7..1c304cadfd 100644
--- a/dump/dump.c
+++ b/dump/dump.c
@@ -1688,26 +1688,6 @@ static void create_kdump_vmcore(DumpState *s, Error **errp)
     }
 }
 
-static int validate_start_block(DumpState *s)
-{
-    GuestPhysBlock *block;
-
-    if (!dump_has_filter(s)) {
-        return 0;
-    }
-
-    QTAILQ_FOREACH(block, &s->guest_phys_blocks.head, next) {
-        /* This block is out of the range */
-        if (block->target_start >= s->filter_area_begin + s->filter_area_length ||
-            block->target_end <= s->filter_area_begin) {
-            continue;
-        }
-        return 0;
-   }
-
-    return -1;
-}
-
 static void get_max_mapnr(DumpState *s)
 {
     GuestPhysBlock *last_block;
@@ -1857,12 +1837,6 @@ static void dump_init(DumpState *s, int fd, bool has_format,
         goto cleanup;
     }
 
-    /* Is the filter filtering everything? */
-    if (validate_start_block(s) == -1) {
-        error_setg(errp, QERR_INVALID_PARAMETER, "begin");
-        goto cleanup;
-    }
-
     /* get dump info: endian, class and architecture.
      * If the target architecture is not supported, cpu_get_dump_info() will
      * return -1.
-- 
2.41.0

Re: [PULL 0/8] Dump patches

[Stefan Hajnoczi]

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

Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/8.2 for any user-visible changes.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]