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

[Dave Anderson]

----- Original Message -----
> At 2012-8-20 16:32, qiaonuohan wrote:
> >
> > I have modified the patches, and they are based on crash 6.0.9.
> 
> I find some mistakes in my former patches, please ignore them and refer
> to the attachments of this mail.
> 
> --
> --
> Regards
> Qiao Nuohan

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