27.08.2025 20:14, Daniel P. Berrangé wrote:
On Wed, Aug 27, 2025 at 04:15:27PM +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().
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]>
---
During first series discussion Den mentions that that code will not work
on 32bit guest with more than 4Gb RAM on i386.
This issue required even more code duplication between dump_init() and
win_dump_available() which we'd like to avoid as mentioned by Daniel.
Hence I present 2nd version of this fix:
- split dump_init() into dump_preinit() and dump_init_complete()
- pass pre-inited dump structure with calculated guest note size to
win_dump_available()
- call header check and guest note size check inside
win_dump_available()
---
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
15bbcc0c6192822cf920fcb7d60eb7d2cfad0952..19341fa42feef4d1c50dbb3a892ded59a3468d20
100644
--- a/dump/dump.c
+++ b/dump/dump.c
@@ -1777,10 +1777,7 @@ static void vmcoreinfo_update_phys_base(DumpState *s)
g_strfreev(lines);
}
[...]
+ s->nr_cpus = nr_cpus;
+ return;
+cleanup:
+ dump_cleanup(s);
+}
The 'dump_cleanup' call is unsafe.
In qmp_query_dump_guest_memory_capability we initialize 's' using
'dump_state_prepare' which just zero's the struct, aside from
the 'status' field.
Meanwhile 'dump_cleanup' will unconditionally do:
close(s->fd);
and 'fd' will be 0, as in stdin, so we break any usage of stdin
that QEMU has. Then some other unlucky part of QEMU will open a
FD and get given FD == 0, making things potentially even worse.
We need 'dump_state_prepare' to set 's->fd = -1', and in
dump_cleanup we should check for s->fd != -1, and after
closing it, must set it back to '-1'.
In fact, I think even the existing dump code is broken in
this respect, and so this should likely be a separate fix
we can send to stable.
I think the 'migrate_del_blocker' call in dump_cleanup
is potentially unsafe too, as it might try to delete a
blocker that is not registered.
I'm not sure about that. Dump blocker variable is defined as global
static and is zeroed by default:
static Error *dump_migration_blocker;
And even if we tried to delete unregistered blocker it would do nothing:
migrate_del_blocker(&dump_migration_blocker);
void migrate_del_blocker(Error **reasonp)
{
if (*reasonp) { <--- NULL-ptr check
for (MigMode mode = 0; mode < MIG_MODE__MAX; mode++) {
migration_blockers[mode] =
g_slist_remove(migration_blockers[mode],
*reasonp);
}
error_free(*reasonp);
*reasonp = NULL;
}
}
But maybe I'm losing something, correct me if I'm wrong.
[...]
With regards,
Daniel
