From: Nikolai Barybin <nikolai.barybin@virtuozzo.com>
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>
Message-ID: <20250911123656.413160-3-nikolai.barybin@virtuozzo.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
dump/win_dump.h | 2 +-
dump/dump.c | 129 +++++++++++++++++++++++++++---------------------
dump/win_dump.c | 23 +++++++--
3 files changed, 95 insertions(+), 59 deletions(-)
diff --git a/dump/win_dump.h b/dump/win_dump.h
index 9d6cfa47c56..62e19c25273 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);
diff --git a/dump/dump.c b/dump/dump.c
index 016a7d7b970..89ce9bcd77e 100644
--- a/dump/dump.c
+++ b/dump/dump.c
@@ -1781,10 +1781,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();
@@ -1792,16 +1789,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;
@@ -1818,41 +1805,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,
@@ -1910,6 +1866,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);
@@ -1920,8 +1926,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;
@@ -2151,11 +2155,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) {
@@ -2194,9 +2193,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;
+ }
- dump_init(s, fd, has_format, format, paging, has_begin,
- begin, length, kdump_raw, errp);
+ if (has_format && format == DUMP_GUEST_MEMORY_FORMAT_WIN_DMP
+ && !win_dump_available(s, errp)) {
+ return;
+ }
+
+ 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;
@@ -2219,6 +2228,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);
@@ -2238,9 +2254,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 3162e8bd487..d42427feb22 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
--
2.52.0
Hi Nikolai,
On 8/1/26 17:12, Philippe Mathieu-Daudé wrote:
> From: Nikolai Barybin <nikolai.barybin@virtuozzo.com>
>
> 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().
This patch breaks CI (failure running tests/qtest/qmp-cmd-test.c).
Before:
{ "execute": "query-dump-guest-memory-capability" }
{"return": {"formats": ["elf", "kdump-zlib", "kdump-raw-zlib"]}}
After:
{ "execute": "query-dump-guest-memory-capability" }
{"error": {"class": "GenericError", "desc": "dumping guest memory is not
supported on this target"}}
CI output:
ERROR:../../tests/qtest/qmp-cmd-test.c:85:test_query: assertion failed:
(qdict_haskey(resp, "return"))
Bail out! ERROR:../../tests/qtest/qmp-cmd-test.c:85:test_query:
assertion failed: (qdict_haskey(resp, "return"))
Aborted (core dumped)
I'm dropping the entire series.
>
> 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>
> Message-ID: <20250911123656.413160-3-nikolai.barybin@virtuozzo.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> dump/win_dump.h | 2 +-
> dump/dump.c | 129 +++++++++++++++++++++++++++---------------------
> dump/win_dump.c | 23 +++++++--
> 3 files changed, 95 insertions(+), 59 deletions(-)
>
> diff --git a/dump/win_dump.h b/dump/win_dump.h
> index 9d6cfa47c56..62e19c25273 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);
>
> diff --git a/dump/dump.c b/dump/dump.c
> index 016a7d7b970..89ce9bcd77e 100644
> --- a/dump/dump.c
> +++ b/dump/dump.c
> @@ -1781,10 +1781,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();
> @@ -1792,16 +1789,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;
> @@ -1818,41 +1805,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,
> @@ -1910,6 +1866,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);
> @@ -1920,8 +1926,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;
> @@ -2151,11 +2155,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) {
> @@ -2194,9 +2193,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;
> + }
>
> - dump_init(s, fd, has_format, format, paging, has_begin,
> - begin, length, kdump_raw, errp);
> + if (has_format && format == DUMP_GUEST_MEMORY_FORMAT_WIN_DMP
> + && !win_dump_available(s, errp)) {
> + return;
> + }
> +
> + 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;
> @@ -2219,6 +2228,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);
>
> @@ -2238,9 +2254,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 3162e8bd487..d42427feb22 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
© 2016 - 2026 Red Hat, Inc.