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

["Daniel P. Berrangé" <berrange@redhat.com>]

On Sun, Aug 31, 2025 at 07:10:22PM +0300, Nikolai Barybin 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().
> 
> Also dump_state_prepare() and dump_cleanup() were modified to avoid
> unsafe close on fd == 0. Thus, it is initialized to -1.
> 
> 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>
> ---
>  dump/dump.c     | 138 ++++++++++++++++++++++++++++--------------------
>  dump/win_dump.c |  23 ++++++--
>  dump/win_dump.h |   2 +-
>  3 files changed, 101 insertions(+), 62 deletions(-)
> 
> diff --git a/dump/dump.c b/dump/dump.c
> index 15bbcc0c61..ce8b43f819 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 };
>  }
>


Can you do this as a separate patch, as this is a pre-existing bug that
ought to be cherry-picked to be fixed in stable too.

If this is split, then you can add

  Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

to both patches.

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