[Crash-utility] [PATCH] add -s option to vm to display one vma at a time

[Dave Anderson]

----- Original Message -----
> At 2012-3-14 4:45, Dave Anderson wrote:
> >
> >
> > ----- Original Message -----
> >> At 2012-3-13 17:56, qiaonuohan wrote:
> >>
> >> Hello Dave,
> >>
> >> When I am using "vm -p" command, I feel it is chaotic when too
> >> much data is printed. I think it is clear to display one vma
> >> each time.
> >>
> >> In the patch, I compare all vmas with the argument of -s option.
> >> If an equal vma is found, it will be printed like below.
> >>
> >> crash>  vm -ps ffff88028ff7d3a0
> >>        VMA           START       END     FLAGS FILE
> >> ffff88028ff7d3a0 7fff29b71000 7fff29b87000 100173
> >> VIRTUAL     PHYSICAL
> >> 7fff29b71000  (not mapped)
> >> 7fff29b72000  (not mapped)
> >> 7fff29b73000  (not mapped)
> >> 7fff29b74000  (not mapped)
> >> 7fff29b75000  (not mapped)
> >> 7fff29b76000  (not mapped)
> >> 7fff29b77000  27faad000
> >> 7fff29b78000  2807aa000
> >> 7fff29b79000  280781000
> >> 7fff29b7a000  280787000
> >> 7fff29b7b000  280776000
> >> 7fff29b7c000  280786000
> >> 7fff29b7d000  27f2df000
> >> 7fff29b7e000  27f2e0000
> >> 7fff29b7f000  27f2e1000
> >> 7fff29b80000  27f2d7000
> >> 7fff29b81000  28b1ac000
> >> 7fff29b82000  28ecc1000
> >> 7fff29b83000  28c5c2000
> >> 7fff29b84000  28aaf4000
> >> 7fff29b85000  28aaf9000
> >> 7fff29b86000  289566000
> >> crash>
> >
> > Seems like a reasonable request.
> >
> > However, I don't like your mixing it with the "-R reference" usage,
> > because it confuses things like this simple example:
> >
> >    crash>  vm -p -s ffff810ec9f57818
> >          VMA           START       END     FLAGS FILE
> >    ffff810ec9f57818   4010d000   40110000 101877 /usr/local/java/jdk1.5.0_19/bin/java
> >    VIRTUAL     PHYSICAL
> >    4010d000    ec89b1000
> >    4010e000    FILE: /usr/local/java/jdk1.5.0_19/bin/java  OFFSET: e000
> >    4010f000    e99803000
> >    crash>
> >
> > If the command above were to be qualified with a "-R 4010e000" reference
> > check, it results in a nonsensical error message:
> >
> >    crash>  vm -p -s ffff810ec9f57818 -R 4010e000
> >    vm: only one -R option allowed
> >    Usage:
> >      vm [-p | -v | -m | [-R reference] | [-f vm_flags]] [pid |
> >      taskp] ...
> >    Enter "help vm" for details.
> >    crash>
> >
> > A few suggestions:
> >
> > (1) Don't even bother to tie it in with the "vm -v" option, because if
> >      you already know the vm_area_struct address, then just print it with
> >      "vm_area_struct <address>".
> >
> > (2) Then, since it *only* applies with the -p functionality, make it
> >      a new "-P<vmaddr>" option, where the help page explains that the
> >      <vmaddr>  argument must be a vm_area_struct of the current context:
> >
> >      Usage:
> >        vm [-p | -P vmaddr | -v | -m | [-R reference] | [-f
> >        vm_flags]] [pid | taskp] ...
> >
> > (3) Make -P mutually exclusive with all of the other options.
> >
> > (4) Do not use the reference structure for this feature.  Just use your new
> >      flag, and pass the vma address in the 3rd "vaddr" argument to vm_area_dump(),
> >      since it's not even being used by cmd_vm().
> 
> I have modified the patch, please check.

OK, these are my suggestions to finalize this patch.  First, fix this:

  # make warn
  ...
  cc -c -g -DX86_64  -DGDB_7_3_1  memory.c -Wall -O2 -Wstrict-prototypes -Wmissing-prototypes -fstack-protector 
  memory.c: In function "vm_area_dump":
  memory.c:3503:7: warning: "single_vma" may be used uninitialized in this function [-Wuninitialized]
  ...

Secondly, this command is no different from the other context-sensitive
"vm" command options, so please always print the task header like they
do.  Just remove the three restrictions in cmd_vm() that do this:

  -               if (!ref)
  +               if (!(ref || (flag & PRINT_SINGLE_VMA)))
                        print_task_header(fp, CURRENT_CONTEXT(), 0);

Third, the option is either going to work OK or fail to find the VMA.
Your patch does this if an invalid vm_area_struct address is entered:

  crash> vm -P ffff880617e7af44
        VMA           START       END     FLAGS FILE
  crash>

Regardless of success or failure, the print_task_header() call in cmd_vm()
should always show the task information.  Then, if you actually find the VMA, 
print the "VMA  START ..." header along with its data:
  
  crash> vm -P ffff880284cf87e0
  PID: 13318  TASK: ffff8806294f2690  CPU: 11  COMMAND: "crash"
        VMA           START       END     FLAGS FILE
  ffff880284cf87e0     400000     9fa000 8001875 /root/crash-6.0.4/crash
  VIRTUAL     PHYSICAL 
  400000      28c7fa000
  401000      29470a000
  402000      29470b000
  403000      29470c000
  404000      29470d000
  405000      29470e000
  406000      29470f000
  ...

But if the command fails to find the VMA, then indicate that under
the task header:

  crash> vm -P ffff880284cf87e8
  PID: 13318  TASK: ffff8806294f2690  CPU: 11  COMMAND: "crash"
  (not found)
  crash> 
  
Lastly, please don't use subterfuge to accomplish the task at
hand.  The setting of PHYSADDR in the vm_area_dump() function 
makes no sense at all:

  +       single_vma_header = 0;
  +       if (flag & PRINT_SINGLE_VMA) {
  +               flag |= PHYSADDR;
  +               single_vma = vaddr;
  +               vaddr = 0;
  +       }

It may "work" by doing the above, but I'm sure you can accomplish 
it just as easily using your PRINT_SINGLE_VMA flag and/or your new
local variables, making it both easier to understand and more 
maintainable.   

Thanks,
  Dave