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

[Dave Anderson]

----- Original Message -----
> At 2012-8-21 23:18, Dave Anderson wrote:
> > Can you change your patch so that they are the same?
> >
> > And would you like to work on creating the new common
> > kdump_backup_region_init() function that can handle both
> > vmcore_data and sadump_data structures?
> >
> 
> Please refer to the attachments. The first two are used to support qemu
> memory dump file. And I made the 4th patch to rewrite function
> kdump_backup_region_init() based on HATAYAMA's patch(the 3rd patch).
> 
> --
> --
> Regards
> Qiao Nuohan

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