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

----- Original Message ----- The patch is looking better, but a few issues still remain to be cleaned up. I'm wondering about the correctness of this addition to read_netdump() for 32-bit dumpfiles: int read_netdump(int fd, void *bufptr, int cnt, ulong addr, physaddr_t paddr) { off_t offset; struct pt_load_segment *pls; int i; + if (nd->flags & QEMU_MEM_DUMP_KDUMP_BACKUP && + paddr >= nd->backup_src_start && + paddr < nd->backup_src_start + nd->backup_src_size) { + ulong orig_paddr; + + orig_paddr = paddr; + paddr += nd->backup_offset - nd->backup_src_start; + + if (CRASHDEBUG(1)) + error(INFO, + "qemu_mem_dump: kdump backup region: %#lx => %#lx\n", + orig_paddr, paddr); + } The incoming "paddr" parameter is type physaddr_t, which is declared as: typedef uint64_t physaddr_t; I see that you've pretty much copied the "if" statement from read_sadump(), but I'm not sure whether SADUMP supports 32-bit dumpfiles? Since nd->backup_src_start is a physical address, maybe it should be declared as a physaddr_t as well? Or if the incoming paddr is 4GB or greater, then perhaps it shouldn't be checked against nd->backup_src_start. In other words, I'm not sure what happens when you do this: paddr >= nd->backup_src_start When running on a 32-bit machine, does paddr get truncated to a 32-bit value, or does nd->backup_src_start get promoted to a 64-bit value? In any case, whatever you decide, please move the complete "if" statement above from read_netdump() into read_kdump(). It makes much more sense for it to be located in read_kdump(), where for example, the incoming "paddr" is also modified for Xen ELF core files. You can also use the "paddr_in" that is declared there instead of the 32-bit-invalid "ulong orig_paddr" that you currently use within your "if" statement. Lastly, "help -n" should show the new QEMU_MEM_DUMP_KDUMP_BACKUP vmcore_data.flag, as well as the 3 new "backup_xxx" fields that you have added to the vmcore_data structure. Other than that, it looks pretty good. Thanks, Dave