Re: [Crash-utility] [PATCH] add support to "virsh dump-guest-memory"(qemu memory dump)

----- Original Message ----- Hello Qiao, Thanks for consolidating the common SADUMP/QEMU_MEM_DUMP code into kdump_backup_region_init(). The patch works well, at least on the two sample QEMU_MEM_DUMP dumpfiles I have. Did you also verify this patch on an SADUMP dumpfile? (I don't have one available). I'd like to make a few small changes/additions that don't affect the functionality of the patch, but are more for aesthetics/maintainability. You do *not* have to resubmit the patch again! FYI, here's what I'm changing: You created new get_vmcore_data() and get_sadump_data() functions to get pointers to the vmcore_data and sadump_data structures, but you have them returning a void *: void * get_vmcore_data(void) { return nd; } void * get_sadump_data(void) { return sd; } But your get_vmcore_data() is essentially a duplicate of the existing get_kdump_vmcore_data(), which returns a pointer to the same structure: struct vmcore_data * get_kdump_vmcore_data(void) { if (!VMCORE_VALID() || !KDUMP_DUMPFILE()) return NULL; return &vmcore_data; } Anyway, in kdump_backup_region_init(), you call the two functions to set "vd": void *vd; ... if (SADUMP_DUMPFILE()) { vd = get_sadump_data(); ... } else if (pc->flags2 & QEMU_MEM_DUMP) { vd = get_vmcore_data(); And then you have constructs such as these where you have to cast "vd" as the relevant structure: if (SADUMP_DUMPFILE()) { ((struct sadump_data *)vd)->flags |= SADUMP_KDUMP_BACKUP; ((struct sadump_data *)vd)->backup_src_start = backup_src_start; ((struct sadump_data *)vd)->backup_src_size = backup_src_size; ((struct sadump_data *)vd)->backup_offset = backup_offset; } else if (pc->flags2 & QEMU_MEM_DUMP) { ((struct vmcore_data *)vd)->flags |= QEMU_MEM_DUMP_KDUMP_BACKUP; ((struct vmcore_data *)vd)->backup_src_start = backup_src_start; ((struct vmcore_data *)vd)->backup_src_size = backup_src_size; ((struct vmcore_data *)vd)->backup_offset = backup_offset; } Kind of ugly, no? So I'm going to do it like this, so that the constructs above can be expressed more clearly: struct vmcore_data *vd; struct sadump_data *sd; ... if (SADUMP_DUMPFILE()) { sd = get_sadump_data(); ... } else if (pc->flags2 & QEMU_MEM_DUMP) { vd = get_kdump_vmcore_data(); .... if (SADUMP_DUMPFILE()) { sd->flags |= SADUMP_KDUMP_BACKUP; sd->backup_src_start = backup_src_start; sd->backup_src_size = backup_src_size; sd->backup_offset = backup_offset; } else if (pc->flags2 & QEMU_MEM_DUMP) { vd->flags |= QEMU_MEM_DUMP_KDUMP_BACKUP; vd->backup_src_start = backup_src_start; vd->backup_src_size = backup_src_size; vd->backup_offset = backup_offset; } Also, I don't like this declaration/naming convention: int archflag; /* indicate the arch of core file: 1 -> 32bit */ and then all of the subsequent "if (archflag)" checks in the function. Since it's not really an "arch", but rather whether it's a 32-bit dumpfile, I've changed its name to "is_32_bit" to clarify the subsequent usage. I also updated the "help" help page itself, and changed the option from "help -q" to "help -r". That way, if in the future somebody wants to dump the registers from some other dumpfile type, it can be done from there. Anyway, thanks again for all the work you've put into this effort. Dave