Hi Nikolai
On Wed, Sep 24, 2025 at 12:30 PM Marc-André Lureau
<[email protected]> wrote:
>
> Hi
>
> On Thu, Sep 11, 2025 at 4:37 PM Nikolai Barybin
> <[email protected]> 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/[email protected]/thread/HJ3JRLWLGN3IKIC22OQ3PMZ4J3EFG5XB/#HJ3JRLWLGN3IKIC22OQ3PMZ4J3EFG5XB
>> [PATCH 0/4] Allow xml-configured coredump format on VM crash
>>
>> Signed-off-by: Nikolai Barybin <[email protected]>
>> Reviewed-by: Daniel P. Berrangé <[email protected]>
>
>
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
