[PATCH v4 0/2] dump: enhance win_dump_available to report properly

[PATCH v4 0/2] dump: enhance win_dump_available to report properly

[Nikolai Barybin]

Changes since last revision:
- Split in 2 patches

Nikolai Barybin (2):
  dump: enhance dump_state_prepare fd initialization
  dump: enhance win_dump_available to report properly

 dump/dump.c     | 138 ++++++++++++++++++++++++++++--------------------
 dump/win_dump.c |  23 ++++++--
 dump/win_dump.h |   2 +-
 3 files changed, 101 insertions(+), 62 deletions(-)

-- 
2.43.5

[PATCH v4 1/2] dump: enhance dump_state_prepare fd initialization

[Nikolai Barybin]

Initializing descriptor with zero is unsafe: during cleanup we risk to
unconditional close of fd == 0 in case dump state wasn't fully
initialized. Thus, let's init fd with -1 value and check its value
before closing it.

Signed-off-by: Nikolai Barybin <nikolai.barybin@virtuozzo.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
---
 dump/dump.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/dump/dump.c b/dump/dump.c
index 15bbcc0c61..b2f7ea7abd 100644
--- a/dump/dump.c
+++ b/dump/dump.c
@@ -103,7 +103,10 @@ static int dump_cleanup(DumpState *s)
 
     guest_phys_blocks_free(&s->guest_phys_blocks);
     memory_mapping_list_free(&s->list);
-    close(s->fd);
+    if (s->fd != -1) {
+        close(s->fd);
+    }
+    s->fd = -1;
     g_free(s->guest_note);
     g_clear_pointer(&s->string_table_buf, g_array_unref);
     s->guest_note = NULL;
@@ -1708,8 +1711,8 @@ static DumpState dump_state_global = { .status = DUMP_STATUS_NONE };
 
 static void dump_state_prepare(DumpState *s)
 {
-    /* zero the struct, setting status to active */
-    *s = (DumpState) { .status = DUMP_STATUS_ACTIVE };
+    /* zero the struct, setting status to active and fd to -1 */
+    *s = (DumpState) { .fd = -1, .status = DUMP_STATUS_ACTIVE };
 }
 
 bool qemu_system_dump_in_progress(void)
-- 
2.43.5

[PATCH v4 2/2] dump: enhance win_dump_available to report properly

[Nikolai Barybin]

QMP query-dump-guest-memory-capability reports win dump as available for
any x86 VM, which is false.

This patch implements proper query of vmcoreinfo and calculation of
guest note size. Based on that we can surely report whether win dump
available or not.

To perform this I suggest to split dump_init() into dump_preinit() and
dump_init_complete() to avoid exausting copypaste in
win_dump_available().

For further reference one may review this libvirt discussion:
https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/HJ3JRLWLGN3IKIC22OQ3PMZ4J3EFG5XB/#HJ3JRLWLGN3IKIC22OQ3PMZ4J3EFG5XB
[PATCH 0/4] Allow xml-configured coredump format on VM crash

Signed-off-by: Nikolai Barybin <nikolai.barybin@virtuozzo.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
---
 dump/dump.c     | 129 +++++++++++++++++++++++++++---------------------
 dump/win_dump.c |  23 +++++++--
 dump/win_dump.h |   2 +-
 3 files changed, 95 insertions(+), 59 deletions(-)

diff --git a/dump/dump.c b/dump/dump.c
index b2f7ea7abd..ce8b43f819 100644
--- a/dump/dump.c
+++ b/dump/dump.c
@@ -1780,10 +1780,7 @@ static void vmcoreinfo_update_phys_base(DumpState *s)
     g_strfreev(lines);
 }
 
-static void dump_init(DumpState *s, int fd, bool has_format,
-                      DumpGuestMemoryFormat format, bool paging, bool has_filter,
-                      int64_t begin, int64_t length, bool kdump_raw,
-                      Error **errp)
+static void dump_preinit(DumpState *s, Error **errp)
 {
     ERRP_GUARD();
     VMCoreInfoState *vmci = vmcoreinfo_find();
@@ -1791,16 +1788,6 @@ static void dump_init(DumpState *s, int fd, bool has_format,
     int nr_cpus;
     int ret;
 
-    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) {
-        assert(!paging && !has_filter);
-    }
-
     if (runstate_is_running()) {
         vm_stop(RUN_STATE_SAVE_VM);
         s->resume = true;
@@ -1817,41 +1804,10 @@ static void dump_init(DumpState *s, int fd, bool has_format,
         nr_cpus++;
     }
 
-    s->fd = fd;
-    if (has_filter && !length) {
-        error_setg(errp, "parameter 'length' expects a non-zero size");
-        goto cleanup;
-    }
-    s->filter_area_begin = begin;
-    s->filter_area_length = length;
-
-    /* First index is 0, it's the special null name */
-    s->string_table_buf = g_array_new(FALSE, TRUE, 1);
-    /*
-     * Allocate the null name, due to the clearing option set to true
-     * it will be 0.
-     */
-    g_array_set_size(s->string_table_buf, 1);
-
     memory_mapping_list_init(&s->list);
-
     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
 
-    /* it does not make sense to dump non-existent memory */
-    if (!s->total_size) {
-        error_setg(errp, "dump: no guest memory to dump");
-        goto cleanup;
-    }
-
-    /* get dump info: endian, class and architecture.
-     * If the target architecture is not supported, cpu_get_dump_info() will
-     * return -1.
-     */
     ret = cpu_get_dump_info(&s->dump_info, &s->guest_phys_blocks);
     if (ret < 0) {
         error_setg(errp,
@@ -1909,6 +1865,56 @@ static void dump_init(DumpState *s, int fd, bool has_format,
         }
     }
 
+    s->nr_cpus = nr_cpus;
+    return;
+cleanup:
+    dump_cleanup(s);
+}
+
+static void dump_init_complete(DumpState *s, int fd, bool has_format,
+                               DumpGuestMemoryFormat format, bool paging,
+                               bool has_filter, int64_t begin, int64_t length,
+                               bool kdump_raw, Error **errp)
+{
+    ERRP_GUARD();
+
+    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) {
+        assert(!paging && !has_filter);
+    }
+
+    s->fd = fd;
+    if (has_filter && !length) {
+        error_setg(errp, "parameter 'length' expects a non-zero size");
+        goto cleanup;
+    }
+    s->filter_area_begin = begin;
+    s->filter_area_length = length;
+
+    /* First index is 0, it's the special null name */
+    s->string_table_buf = g_array_new(FALSE, TRUE, 1);
+    /*
+     * Allocate the null name, due to the clearing option set to true
+     * it will be 0.
+     */
+    g_array_set_size(s->string_table_buf, 1);
+
+    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
+
+    /* it does not make sense to dump non-existent memory */
+    if (!s->total_size) {
+        error_setg(errp, "dump: no guest memory to dump");
+        goto cleanup;
+    }
+
     /* get memory mapping */
     if (paging) {
         qemu_get_guest_memory_mapping(&s->list, &s->guest_phys_blocks, errp);
@@ -1919,8 +1925,6 @@ static void dump_init(DumpState *s, int fd, bool has_format,
         qemu_get_guest_simple_memory_mapping(&s->list, &s->guest_phys_blocks);
     }
 
-    s->nr_cpus = nr_cpus;
-
     get_max_mapnr(s);
 
     uint64_t tmp;
@@ -2150,11 +2154,6 @@ void qmp_dump_guest_memory(bool paging, const char *protocol,
     }
 #endif
 
-    if (has_format && format == DUMP_GUEST_MEMORY_FORMAT_WIN_DMP
-        && !win_dump_available(errp)) {
-        return;
-    }
-
     if (strstart(protocol, "fd:", &p)) {
         fd = monitor_get_fd(monitor_cur(), p, errp);
         if (fd == -1) {
@@ -2193,9 +2192,19 @@ void qmp_dump_guest_memory(bool paging, const char *protocol,
 
     s = &dump_state_global;
     dump_state_prepare(s);
+    dump_preinit(s, errp);
+    if (*errp) {
+        qatomic_set(&s->status, DUMP_STATUS_FAILED);
+        return;
+    }
+
+    if (has_format && format == DUMP_GUEST_MEMORY_FORMAT_WIN_DMP
+        && !win_dump_available(s, errp)) {
+        return;
+    }
 
-    dump_init(s, fd, has_format, format, paging, has_begin,
-              begin, length, kdump_raw, errp);
+    dump_init_complete(s, fd, has_format, format, paging, has_begin,
+                       begin, length, kdump_raw, errp);
     if (*errp) {
         qatomic_set(&s->status, DUMP_STATUS_FAILED);
         return;
@@ -2218,6 +2227,13 @@ DumpGuestMemoryCapability *qmp_query_dump_guest_memory_capability(Error **errp)
                                   g_new0(DumpGuestMemoryCapability, 1);
     DumpGuestMemoryFormatList **tail = &cap->formats;
 
+    DumpState *s = g_new0(DumpState, 1);
+    dump_state_prepare(s);
+    dump_preinit(s, errp);
+    if (*errp) {
+        goto cleanup;
+    }
+
     /* elf is always available */
     QAPI_LIST_APPEND(tail, DUMP_GUEST_MEMORY_FORMAT_ELF);
 
@@ -2237,9 +2253,12 @@ DumpGuestMemoryCapability *qmp_query_dump_guest_memory_capability(Error **errp)
     QAPI_LIST_APPEND(tail, DUMP_GUEST_MEMORY_FORMAT_KDUMP_RAW_SNAPPY);
 #endif
 
-    if (win_dump_available(NULL)) {
+    if (win_dump_available(s, NULL)) {
         QAPI_LIST_APPEND(tail, DUMP_GUEST_MEMORY_FORMAT_WIN_DMP);
     }
 
+ cleanup:
+    dump_cleanup(s);
+    g_free(s);
     return cap;
 }
diff --git a/dump/win_dump.c b/dump/win_dump.c
index 3162e8bd48..d42427feb2 100644
--- a/dump/win_dump.c
+++ b/dump/win_dump.c
@@ -20,8 +20,25 @@
 
 #if defined(TARGET_X86_64)
 
-bool win_dump_available(Error **errp)
+static bool check_header(WinDumpHeader *h, bool *x64, Error **errp);
+
+bool win_dump_available(DumpState *s, Error **errp)
 {
+    WinDumpHeader *h = (void *)(s->guest_note + VMCOREINFO_ELF_NOTE_HDR_SIZE);
+    Error *local_err = NULL;
+    bool x64 = true;
+
+    if (s->guest_note_size != VMCOREINFO_WIN_DUMP_NOTE_SIZE32 &&
+            s->guest_note_size != VMCOREINFO_WIN_DUMP_NOTE_SIZE64) {
+        error_setg(errp, "win-dump: invalid vmcoreinfo note size");
+        return false;
+    }
+
+    if (!check_header(h, &x64, &local_err)) {
+        error_propagate(errp, local_err);
+        return false;
+    }
+
     return true;
 }
 
@@ -480,7 +497,7 @@ out_cr3:
 
 #else /* !TARGET_X86_64 */
 
-bool win_dump_available(Error **errp)
+bool win_dump_available(DumpState *s, Error **errp)
 {
     error_setg(errp, "Windows dump is only available for x86-64");
 
@@ -489,7 +506,7 @@ bool win_dump_available(Error **errp)
 
 void create_win_dump(DumpState *s, Error **errp)
 {
-    win_dump_available(errp);
+    win_dump_available(s, errp);
 }
 
 #endif
diff --git a/dump/win_dump.h b/dump/win_dump.h
index 9d6cfa47c5..62e19c2527 100644
--- a/dump/win_dump.h
+++ b/dump/win_dump.h
@@ -14,7 +14,7 @@
 #include "system/dump.h"
 
 /* Check Windows dump availability for the current target */
-bool win_dump_available(Error **errp);
+bool win_dump_available(DumpState *s, Error **errp);
 
 void create_win_dump(DumpState *s, Error **errp);
 
-- 
2.43.5

Re: [PATCH v4 2/2] dump: enhance win_dump_available to report properly

[Marc-André Lureau]

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

Hi

On Thu, Sep 11, 2025 at 4:37 PM Nikolai Barybin <
nikolai.barybin@virtuozzo.com> wrote:

> QMP query-dump-guest-memory-capability reports win dump as available for
> any x86 VM, which is false.
>
> This patch implements proper query of vmcoreinfo and calculation of
> guest note size. Based on that we can surely report whether win dump
> available or not.
>
> To perform this I suggest to split dump_init() into dump_preinit() and
> dump_init_complete() to avoid exausting copypaste in
> win_dump_available().
>
> For further reference one may review this libvirt discussion:
>
> https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/HJ3JRLWLGN3IKIC22OQ3PMZ4J3EFG5XB/#HJ3JRLWLGN3IKIC22OQ3PMZ4J3EFG5XB
> [PATCH 0/4] Allow xml-configured coredump format on VM crash
>
> Signed-off-by: Nikolai Barybin <nikolai.barybin@virtuozzo.com>
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
>

Could we split the patch for the generic dump refactoring and then the
windows specifics?


> ---
>  dump/dump.c     | 129 +++++++++++++++++++++++++++---------------------
>  dump/win_dump.c |  23 +++++++--
>  dump/win_dump.h |   2 +-
>  3 files changed, 95 insertions(+), 59 deletions(-)
>
> diff --git a/dump/dump.c b/dump/dump.c
> index b2f7ea7abd..ce8b43f819 100644
> --- a/dump/dump.c
> +++ b/dump/dump.c
> @@ -1780,10 +1780,7 @@ static void vmcoreinfo_update_phys_base(DumpState
> *s)
>      g_strfreev(lines);
>  }
>
> -static void dump_init(DumpState *s, int fd, bool has_format,
> -                      DumpGuestMemoryFormat format, bool paging, bool
> has_filter,
> -                      int64_t begin, int64_t length, bool kdump_raw,
> -                      Error **errp)
> +static void dump_preinit(DumpState *s, Error **errp)
>  {
>      ERRP_GUARD();
>      VMCoreInfoState *vmci = vmcoreinfo_find();
> @@ -1791,16 +1788,6 @@ static void dump_init(DumpState *s, int fd, bool
> has_format,
>      int nr_cpus;
>      int ret;
>
> -    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) {
> -        assert(!paging && !has_filter);
> -    }
> -
>      if (runstate_is_running()) {
>          vm_stop(RUN_STATE_SAVE_VM);
>          s->resume = true;
> @@ -1817,41 +1804,10 @@ static void dump_init(DumpState *s, int fd, bool
> has_format,
>          nr_cpus++;
>      }
>
> -    s->fd = fd;
> -    if (has_filter && !length) {
> -        error_setg(errp, "parameter 'length' expects a non-zero size");
> -        goto cleanup;
> -    }
> -    s->filter_area_begin = begin;
> -    s->filter_area_length = length;
> -
> -    /* First index is 0, it's the special null name */
> -    s->string_table_buf = g_array_new(FALSE, TRUE, 1);
> -    /*
> -     * Allocate the null name, due to the clearing option set to true
> -     * it will be 0.
> -     */
> -    g_array_set_size(s->string_table_buf, 1);
> -
>      memory_mapping_list_init(&s->list);
> -
>      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
>
> -    /* it does not make sense to dump non-existent memory */
> -    if (!s->total_size) {
> -        error_setg(errp, "dump: no guest memory to dump");
> -        goto cleanup;
> -    }
> -
> -    /* get dump info: endian, class and architecture.
> -     * If the target architecture is not supported, cpu_get_dump_info()
> will
> -     * return -1.
> -     */
>

Why drop that comment?


>      ret = cpu_get_dump_info(&s->dump_info, &s->guest_phys_blocks);
>      if (ret < 0) {
>          error_setg(errp,
> @@ -1909,6 +1865,56 @@ static void dump_init(DumpState *s, int fd, bool
> has_format,
>          }
>      }
>
> +    s->nr_cpus = nr_cpus;
> +    return;
> +cleanup:
> +    dump_cleanup(s);
>

Is it intentional and safe to call dump_cleanup() two times from
qmp_query_dump_guest_memory_capability()? Maybe it should only be called by
the one who allocated/init it, and thus dropped from this function.


> +}
> +
> +static void dump_init_complete(DumpState *s, int fd, bool has_format,
> +                               DumpGuestMemoryFormat format, bool paging,
> +                               bool has_filter, int64_t begin, int64_t
> length,
> +                               bool kdump_raw, Error **errp)
> +{
> +    ERRP_GUARD();
> +
> +    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) {
> +        assert(!paging && !has_filter);
> +    }
> +
> +    s->fd = fd;
> +    if (has_filter && !length) {
> +        error_setg(errp, "parameter 'length' expects a non-zero size");
> +        goto cleanup;
> +    }
> +    s->filter_area_begin = begin;
> +    s->filter_area_length = length;
> +
> +    /* First index is 0, it's the special null name */
> +    s->string_table_buf = g_array_new(FALSE, TRUE, 1);
> +    /*
> +     * Allocate the null name, due to the clearing option set to true
> +     * it will be 0.
> +     */
> +    g_array_set_size(s->string_table_buf, 1);
> +
> +    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
> +
> +    /* it does not make sense to dump non-existent memory */
> +    if (!s->total_size) {
> +        error_setg(errp, "dump: no guest memory to dump");
> +        goto cleanup;
> +    }
> +
>      /* get memory mapping */
>      if (paging) {
>          qemu_get_guest_memory_mapping(&s->list, &s->guest_phys_blocks,
> errp);
> @@ -1919,8 +1925,6 @@ static void dump_init(DumpState *s, int fd, bool
> has_format,
>          qemu_get_guest_simple_memory_mapping(&s->list,
> &s->guest_phys_blocks);
>      }
>
> -    s->nr_cpus = nr_cpus;
> -
>      get_max_mapnr(s);
>
>      uint64_t tmp;
> @@ -2150,11 +2154,6 @@ void qmp_dump_guest_memory(bool paging, const char
> *protocol,
>      }
>  #endif
>
> -    if (has_format && format == DUMP_GUEST_MEMORY_FORMAT_WIN_DMP
> -        && !win_dump_available(errp)) {
> -        return;
> -    }
> -
>      if (strstart(protocol, "fd:", &p)) {
>          fd = monitor_get_fd(monitor_cur(), p, errp);
>          if (fd == -1) {
> @@ -2193,9 +2192,19 @@ void qmp_dump_guest_memory(bool paging, const char
> *protocol,
>
>
It may be worth adding an ERRP_GUARD since you check *errp.

Why not make dump_preinit() return a bool, true on success, like
recommended by error API docs?



>      s = &dump_state_global;
>      dump_state_prepare(s);
> +    dump_preinit(s, errp);
> +    if (*errp) {
> +        qatomic_set(&s->status, DUMP_STATUS_FAILED);
> +        return;
> +    }
> +
> +    if (has_format && format == DUMP_GUEST_MEMORY_FORMAT_WIN_DMP
> +        && !win_dump_available(s, errp)) {
> +        return;
> +    }
>
> -    dump_init(s, fd, has_format, format, paging, has_begin,
> -              begin, length, kdump_raw, errp);
> +    dump_init_complete(s, fd, has_format, format, paging, has_begin,
> +                       begin, length, kdump_raw, errp);
>      if (*errp) {
>          qatomic_set(&s->status, DUMP_STATUS_FAILED);
>          return;
> @@ -2218,6 +2227,13 @@ DumpGuestMemoryCapability
> *qmp_query_dump_guest_memory_capability(Error **errp)
>                                    g_new0(DumpGuestMemoryCapability, 1);
>      DumpGuestMemoryFormatList **tail = &cap->formats;
>

Same for ERRP_GUARD


>
> +    DumpState *s = g_new0(DumpState, 1);
> +    dump_state_prepare(s);
> +    dump_preinit(s, errp);
> +    if (*errp) {
> +        goto cleanup;
> +    }
> +
>      /* elf is always available */
>      QAPI_LIST_APPEND(tail, DUMP_GUEST_MEMORY_FORMAT_ELF);
>
> @@ -2237,9 +2253,12 @@ DumpGuestMemoryCapability
> *qmp_query_dump_guest_memory_capability(Error **errp)
>      QAPI_LIST_APPEND(tail, DUMP_GUEST_MEMORY_FORMAT_KDUMP_RAW_SNAPPY);
>  #endif
>
> -    if (win_dump_available(NULL)) {
> +    if (win_dump_available(s, NULL)) {
>          QAPI_LIST_APPEND(tail, DUMP_GUEST_MEMORY_FORMAT_WIN_DMP);
>      }
>
> + cleanup:
> +    dump_cleanup(s);
> +    g_free(s);
>      return cap;
>  }
> diff --git a/dump/win_dump.c b/dump/win_dump.c
> index 3162e8bd48..d42427feb2 100644
> --- a/dump/win_dump.c
> +++ b/dump/win_dump.c
> @@ -20,8 +20,25 @@
>
>  #if defined(TARGET_X86_64)
>
> -bool win_dump_available(Error **errp)
> +static bool check_header(WinDumpHeader *h, bool *x64, Error **errp);
> +
> +bool win_dump_available(DumpState *s, Error **errp)
>  {
> +    WinDumpHeader *h = (void *)(s->guest_note +
> VMCOREINFO_ELF_NOTE_HDR_SIZE);
> +    Error *local_err = NULL;
> +    bool x64 = true;
> +
> +    if (s->guest_note_size != VMCOREINFO_WIN_DUMP_NOTE_SIZE32 &&
> +            s->guest_note_size != VMCOREINFO_WIN_DUMP_NOTE_SIZE64) {
> +        error_setg(errp, "win-dump: invalid vmcoreinfo note size");
> +        return false;
> +    }
> +
> +    if (!check_header(h, &x64, &local_err)) {
> +        error_propagate(errp, local_err);
> +        return false;
> +    }

+
>      return true;
>  }
>
> @@ -480,7 +497,7 @@ out_cr3:
>
>  #else /* !TARGET_X86_64 */
>
> -bool win_dump_available(Error **errp)
> +bool win_dump_available(DumpState *s, Error **errp)
>  {
>      error_setg(errp, "Windows dump is only available for x86-64");
>
> @@ -489,7 +506,7 @@ bool win_dump_available(Error **errp)
>
>  void create_win_dump(DumpState *s, Error **errp)
>  {
> -    win_dump_available(errp);
> +    win_dump_available(s, errp);
>  }
>
>  #endif
> diff --git a/dump/win_dump.h b/dump/win_dump.h
> index 9d6cfa47c5..62e19c2527 100644
> --- a/dump/win_dump.h
> +++ b/dump/win_dump.h
> @@ -14,7 +14,7 @@
>  #include "system/dump.h"
>
>  /* Check Windows dump availability for the current target */
> -bool win_dump_available(Error **errp);
> +bool win_dump_available(DumpState *s, Error **errp);
>
>  void create_win_dump(DumpState *s, Error **errp);
>
> --
> 2.43.5
>


Sadly we still don't have much testing for dump..

thanks

[-- Attachment #2: Type: text/html, Size: 13933 bytes --]

Re: [PATCH v4 2/2] dump: enhance win_dump_available to report properly

[Marc-André Lureau]

Hi Nikolai

On Wed, Sep 24, 2025 at 12:30 PM Marc-André Lureau
<marcandre.lureau@redhat.com> wrote:
>
> Hi
>
> On Thu, Sep 11, 2025 at 4:37 PM Nikolai Barybin <nikolai.barybin@virtuozzo.com> wrote:
>>
>> QMP query-dump-guest-memory-capability reports win dump as available for
>> any x86 VM, which is false.
>>
>> This patch implements proper query of vmcoreinfo and calculation of
>> guest note size. Based on that we can surely report whether win dump
>> available or not.
>>
>> To perform this I suggest to split dump_init() into dump_preinit() and
>> dump_init_complete() to avoid exausting copypaste in
>> win_dump_available().
>>
>> For further reference one may review this libvirt discussion:
>> https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/HJ3JRLWLGN3IKIC22OQ3PMZ4J3EFG5XB/#HJ3JRLWLGN3IKIC22OQ3PMZ4J3EFG5XB
>> [PATCH 0/4] Allow xml-configured coredump format on VM crash
>>
>> Signed-off-by: Nikolai Barybin <nikolai.barybin@virtuozzo.com>
>> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
>
>

Have you looked at my review & updated the patches? thanks

> Could we split the patch for the generic dump refactoring and then the windows specifics?
>
>>
>> ---
>>  dump/dump.c     | 129 +++++++++++++++++++++++++++---------------------
>>  dump/win_dump.c |  23 +++++++--
>>  dump/win_dump.h |   2 +-
>>  3 files changed, 95 insertions(+), 59 deletions(-)
>>
>> diff --git a/dump/dump.c b/dump/dump.c
>> index b2f7ea7abd..ce8b43f819 100644
>> --- a/dump/dump.c
>> +++ b/dump/dump.c
>> @@ -1780,10 +1780,7 @@ static void vmcoreinfo_update_phys_base(DumpState *s)
>>      g_strfreev(lines);
>>  }
>>
>> -static void dump_init(DumpState *s, int fd, bool has_format,
>> -                      DumpGuestMemoryFormat format, bool paging, bool has_filter,
>> -                      int64_t begin, int64_t length, bool kdump_raw,
>> -                      Error **errp)
>> +static void dump_preinit(DumpState *s, Error **errp)
>>  {
>>      ERRP_GUARD();
>>      VMCoreInfoState *vmci = vmcoreinfo_find();
>> @@ -1791,16 +1788,6 @@ static void dump_init(DumpState *s, int fd, bool has_format,
>>      int nr_cpus;
>>      int ret;
>>
>> -    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) {
>> -        assert(!paging && !has_filter);
>> -    }
>> -
>>      if (runstate_is_running()) {
>>          vm_stop(RUN_STATE_SAVE_VM);
>>          s->resume = true;
>> @@ -1817,41 +1804,10 @@ static void dump_init(DumpState *s, int fd, bool has_format,
>>          nr_cpus++;
>>      }
>>
>> -    s->fd = fd;
>> -    if (has_filter && !length) {
>> -        error_setg(errp, "parameter 'length' expects a non-zero size");
>> -        goto cleanup;
>> -    }
>> -    s->filter_area_begin = begin;
>> -    s->filter_area_length = length;
>> -
>> -    /* First index is 0, it's the special null name */
>> -    s->string_table_buf = g_array_new(FALSE, TRUE, 1);
>> -    /*
>> -     * Allocate the null name, due to the clearing option set to true
>> -     * it will be 0.
>> -     */
>> -    g_array_set_size(s->string_table_buf, 1);
>> -
>>      memory_mapping_list_init(&s->list);
>> -
>>      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
>>
>> -    /* it does not make sense to dump non-existent memory */
>> -    if (!s->total_size) {
>> -        error_setg(errp, "dump: no guest memory to dump");
>> -        goto cleanup;
>> -    }
>> -
>> -    /* get dump info: endian, class and architecture.
>> -     * If the target architecture is not supported, cpu_get_dump_info() will
>> -     * return -1.
>> -     */
>
>
> Why drop that comment?
>
>>
>>      ret = cpu_get_dump_info(&s->dump_info, &s->guest_phys_blocks);
>>      if (ret < 0) {
>>          error_setg(errp,
>> @@ -1909,6 +1865,56 @@ static void dump_init(DumpState *s, int fd, bool has_format,
>>          }
>>      }
>>
>> +    s->nr_cpus = nr_cpus;
>> +    return;
>> +cleanup:
>> +    dump_cleanup(s);
>
>
> Is it intentional and safe to call dump_cleanup() two times from qmp_query_dump_guest_memory_capability()? Maybe it should only be called by the one who allocated/init it, and thus dropped from this function.
>
>>
>> +}
>> +
>> +static void dump_init_complete(DumpState *s, int fd, bool has_format,
>> +                               DumpGuestMemoryFormat format, bool paging,
>> +                               bool has_filter, int64_t begin, int64_t length,
>> +                               bool kdump_raw, Error **errp)
>> +{
>> +    ERRP_GUARD();
>> +
>> +    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) {
>> +        assert(!paging && !has_filter);
>> +    }
>> +
>> +    s->fd = fd;
>> +    if (has_filter && !length) {
>> +        error_setg(errp, "parameter 'length' expects a non-zero size");
>> +        goto cleanup;
>> +    }
>> +    s->filter_area_begin = begin;
>> +    s->filter_area_length = length;
>> +
>> +    /* First index is 0, it's the special null name */
>> +    s->string_table_buf = g_array_new(FALSE, TRUE, 1);
>> +    /*
>> +     * Allocate the null name, due to the clearing option set to true
>> +     * it will be 0.
>> +     */
>> +    g_array_set_size(s->string_table_buf, 1);
>> +
>> +    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
>> +
>> +    /* it does not make sense to dump non-existent memory */
>> +    if (!s->total_size) {
>> +        error_setg(errp, "dump: no guest memory to dump");
>> +        goto cleanup;
>> +    }
>> +
>>      /* get memory mapping */
>>      if (paging) {
>>          qemu_get_guest_memory_mapping(&s->list, &s->guest_phys_blocks, errp);
>> @@ -1919,8 +1925,6 @@ static void dump_init(DumpState *s, int fd, bool has_format,
>>          qemu_get_guest_simple_memory_mapping(&s->list, &s->guest_phys_blocks);
>>      }
>>
>> -    s->nr_cpus = nr_cpus;
>> -
>>      get_max_mapnr(s);
>>
>>      uint64_t tmp;
>> @@ -2150,11 +2154,6 @@ void qmp_dump_guest_memory(bool paging, const char *protocol,
>>      }
>>  #endif
>>
>> -    if (has_format && format == DUMP_GUEST_MEMORY_FORMAT_WIN_DMP
>> -        && !win_dump_available(errp)) {
>> -        return;
>> -    }
>> -
>>      if (strstart(protocol, "fd:", &p)) {
>>          fd = monitor_get_fd(monitor_cur(), p, errp);
>>          if (fd == -1) {
>> @@ -2193,9 +2192,19 @@ void qmp_dump_guest_memory(bool paging, const char *protocol,
>>
>
> It may be worth adding an ERRP_GUARD since you check *errp.
>
> Why not make dump_preinit() return a bool, true on success, like recommended by error API docs?
>
>
>>
>>      s = &dump_state_global;
>>      dump_state_prepare(s);
>> +    dump_preinit(s, errp);
>> +    if (*errp) {
>> +        qatomic_set(&s->status, DUMP_STATUS_FAILED);
>> +        return;
>> +    }
>> +
>> +    if (has_format && format == DUMP_GUEST_MEMORY_FORMAT_WIN_DMP
>> +        && !win_dump_available(s, errp)) {
>> +        return;
>> +    }
>>
>> -    dump_init(s, fd, has_format, format, paging, has_begin,
>> -              begin, length, kdump_raw, errp);
>> +    dump_init_complete(s, fd, has_format, format, paging, has_begin,
>> +                       begin, length, kdump_raw, errp);
>>      if (*errp) {
>>          qatomic_set(&s->status, DUMP_STATUS_FAILED);
>>          return;
>> @@ -2218,6 +2227,13 @@ DumpGuestMemoryCapability *qmp_query_dump_guest_memory_capability(Error **errp)
>>                                    g_new0(DumpGuestMemoryCapability, 1);
>>      DumpGuestMemoryFormatList **tail = &cap->formats;
>
>
> Same for ERRP_GUARD
>
>>
>>
>> +    DumpState *s = g_new0(DumpState, 1);
>> +    dump_state_prepare(s);
>> +    dump_preinit(s, errp);
>> +    if (*errp) {
>> +        goto cleanup;
>> +    }
>> +
>>      /* elf is always available */
>>      QAPI_LIST_APPEND(tail, DUMP_GUEST_MEMORY_FORMAT_ELF);
>>
>> @@ -2237,9 +2253,12 @@ DumpGuestMemoryCapability *qmp_query_dump_guest_memory_capability(Error **errp)
>>      QAPI_LIST_APPEND(tail, DUMP_GUEST_MEMORY_FORMAT_KDUMP_RAW_SNAPPY);
>>  #endif
>>
>> -    if (win_dump_available(NULL)) {
>> +    if (win_dump_available(s, NULL)) {
>>          QAPI_LIST_APPEND(tail, DUMP_GUEST_MEMORY_FORMAT_WIN_DMP);
>>      }
>>
>> + cleanup:
>> +    dump_cleanup(s);
>> +    g_free(s);
>>      return cap;
>>  }
>> diff --git a/dump/win_dump.c b/dump/win_dump.c
>> index 3162e8bd48..d42427feb2 100644
>> --- a/dump/win_dump.c
>> +++ b/dump/win_dump.c
>> @@ -20,8 +20,25 @@
>>
>>  #if defined(TARGET_X86_64)
>>
>> -bool win_dump_available(Error **errp)
>> +static bool check_header(WinDumpHeader *h, bool *x64, Error **errp);
>> +
>> +bool win_dump_available(DumpState *s, Error **errp)
>>  {
>> +    WinDumpHeader *h = (void *)(s->guest_note + VMCOREINFO_ELF_NOTE_HDR_SIZE);
>> +    Error *local_err = NULL;
>> +    bool x64 = true;
>> +
>> +    if (s->guest_note_size != VMCOREINFO_WIN_DUMP_NOTE_SIZE32 &&
>> +            s->guest_note_size != VMCOREINFO_WIN_DUMP_NOTE_SIZE64) {
>> +        error_setg(errp, "win-dump: invalid vmcoreinfo note size");
>> +        return false;
>> +    }
>> +
>> +    if (!check_header(h, &x64, &local_err)) {
>> +        error_propagate(errp, local_err);
>> +        return false;
>> +    }
>>
>> +
>>      return true;
>>  }
>>
>> @@ -480,7 +497,7 @@ out_cr3:
>>
>>  #else /* !TARGET_X86_64 */
>>
>> -bool win_dump_available(Error **errp)
>> +bool win_dump_available(DumpState *s, Error **errp)
>>  {
>>      error_setg(errp, "Windows dump is only available for x86-64");
>>
>> @@ -489,7 +506,7 @@ bool win_dump_available(Error **errp)
>>
>>  void create_win_dump(DumpState *s, Error **errp)
>>  {
>> -    win_dump_available(errp);
>> +    win_dump_available(s, errp);
>>  }
>>
>>  #endif
>> diff --git a/dump/win_dump.h b/dump/win_dump.h
>> index 9d6cfa47c5..62e19c2527 100644
>> --- a/dump/win_dump.h
>> +++ b/dump/win_dump.h
>> @@ -14,7 +14,7 @@
>>  #include "system/dump.h"
>>
>>  /* Check Windows dump availability for the current target */
>> -bool win_dump_available(Error **errp);
>> +bool win_dump_available(DumpState *s, Error **errp);
>>
>>  void create_win_dump(DumpState *s, Error **errp);
>>
>> --
>> 2.43.5
>>
>
>
> Sadly we still don't have much testing for dump..
>
> thanks



-- 
Marc-André Lureau

Re: [PATCH v4 2/2] dump: enhance win_dump_available to report properly

[Denis V. Lunev]

On 1/8/26 09:11, Marc-André Lureau wrote:
> Hi Nikolai
>
> On Wed, Sep 24, 2025 at 12:30 PM Marc-André Lureau
> <marcandre.lureau@redhat.com> wrote:
>> Hi
>>
>> On Thu, Sep 11, 2025 at 4:37 PM Nikolai Barybin <nikolai.barybin@virtuozzo.com> wrote:
>>> QMP query-dump-guest-memory-capability reports win dump as available for
>>> any x86 VM, which is false.
>>>
>>> This patch implements proper query of vmcoreinfo and calculation of
>>> guest note size. Based on that we can surely report whether win dump
>>> available or not.
>>>
>>> To perform this I suggest to split dump_init() into dump_preinit() and
>>> dump_init_complete() to avoid exausting copypaste in
>>> win_dump_available().
>>>
>>> For further reference one may review this libvirt discussion:
>>> https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/HJ3JRLWLGN3IKIC22OQ3PMZ4J3EFG5XB/#HJ3JRLWLGN3IKIC22OQ3PMZ4J3EFG5XB
>>> [PATCH 0/4] Allow xml-configured coredump format on VM crash
>>>
>>> Signed-off-by: Nikolai Barybin <nikolai.barybin@virtuozzo.com>
>>> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
>>
> Have you looked at my review & updated the patches? thanks
I will send updated version next week after vacation. Nikolai has
left Virtuozzo.

Thank you for the reminder and best regards,
    Den

Re: [PATCH v4 2/2] dump: enhance win_dump_available to report properly

[Daniel P. Berrangé]

On Thu, Jan 08, 2026 at 10:30:29AM +0100, Denis V. Lunev wrote:
> On 1/8/26 09:11, Marc-André Lureau wrote:
> > Hi Nikolai
> >
> > On Wed, Sep 24, 2025 at 12:30 PM Marc-André Lureau
> > <marcandre.lureau@redhat.com> wrote:
> >> Hi
> >>
> >> On Thu, Sep 11, 2025 at 4:37 PM Nikolai Barybin <nikolai.barybin@virtuozzo.com> wrote:
> >>> QMP query-dump-guest-memory-capability reports win dump as available for
> >>> any x86 VM, which is false.
> >>>
> >>> This patch implements proper query of vmcoreinfo and calculation of
> >>> guest note size. Based on that we can surely report whether win dump
> >>> available or not.
> >>>
> >>> To perform this I suggest to split dump_init() into dump_preinit() and
> >>> dump_init_complete() to avoid exausting copypaste in
> >>> win_dump_available().
> >>>
> >>> For further reference one may review this libvirt discussion:
> >>> https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/HJ3JRLWLGN3IKIC22OQ3PMZ4J3EFG5XB/#HJ3JRLWLGN3IKIC22OQ3PMZ4J3EFG5XB
> >>> [PATCH 0/4] Allow xml-configured coredump format on VM crash
> >>>
> >>> Signed-off-by: Nikolai Barybin <nikolai.barybin@virtuozzo.com>
> >>> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> >>
> > Have you looked at my review & updated the patches? thanks
> I will send updated version next week after vacation. Nikolai has
> left Virtuozzo.
> 
> Thank you for the reminder and best regards,

I don't recall seeing a followup v5 for this ? Were you still
interested in progressing this work ?

With regards,
Daniel
-- 
|: https://berrange.com       ~~        https://hachyderm.io/@berrange :|
|: https://libvirt.org          ~~          https://entangle-photo.org :|
|: https://pixelfed.art/berrange   ~~    https://fstop138.berrange.com :|

Re: [PATCH v4 2/2] dump: enhance win_dump_available to report properly

[Denis V. Lunev]

On 6/5/26 18:10, Daniel P. Berrangé wrote:
> On Thu, Jan 08, 2026 at 10:30:29AM +0100, Denis V. Lunev wrote:
>> On 1/8/26 09:11, Marc-André Lureau wrote:
>>> Hi Nikolai
>>>
>>> On Wed, Sep 24, 2025 at 12:30 PM Marc-André Lureau
>>> <marcandre.lureau@redhat.com> wrote:
>>>> Hi
>>>>
>>>> On Thu, Sep 11, 2025 at 4:37 PM Nikolai Barybin <nikolai.barybin@virtuozzo.com> wrote:
>>>>> QMP query-dump-guest-memory-capability reports win dump as available for
>>>>> any x86 VM, which is false.
>>>>>
>>>>> This patch implements proper query of vmcoreinfo and calculation of
>>>>> guest note size. Based on that we can surely report whether win dump
>>>>> available or not.
>>>>>
>>>>> To perform this I suggest to split dump_init() into dump_preinit() and
>>>>> dump_init_complete() to avoid exausting copypaste in
>>>>> win_dump_available().
>>>>>
>>>>> For further reference one may review this libvirt discussion:
>>>>> https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/HJ3JRLWLGN3IKIC22OQ3PMZ4J3EFG5XB/#HJ3JRLWLGN3IKIC22OQ3PMZ4J3EFG5XB
>>>>> [PATCH 0/4] Allow xml-configured coredump format on VM crash
>>>>>
>>>>> Signed-off-by: Nikolai Barybin <nikolai.barybin@virtuozzo.com>
>>>>> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
>>> Have you looked at my review & updated the patches? thanks
>> I will send updated version next week after vacation. Nikolai has
>> left Virtuozzo.
>>
>> Thank you for the reminder and best regards,
> I don't recall seeing a followup v5 for this ? Were you still
> interested in progressing this work ?

Nikolay has left the team and that piece falls on me.
This is still in my list and I have motivation to
finish but need some breath.

I home to come back again soon.

Den

Re: [PATCH v4 0/2] dump: enhance win_dump_available to report properly

[Daniel P. Berrangé]

Marc-André, are these patches good with you ? From my POV they're
ready to be queued.

On Thu, Sep 11, 2025 at 03:36:54PM +0300, Nikolai Barybin wrote:
> Changes since last revision:
> - Split in 2 patches
> 
> Nikolai Barybin (2):
>   dump: enhance dump_state_prepare fd initialization
>   dump: enhance win_dump_available to report properly
> 
>  dump/dump.c     | 138 ++++++++++++++++++++++++++++--------------------
>  dump/win_dump.c |  23 ++++++--
>  dump/win_dump.h |   2 +-
>  3 files changed, 101 insertions(+), 62 deletions(-)
> 
> -- 
> 2.43.5
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|