----- Original Message -----
> At 2012-3-15 3:06, Dave Anderson wrote:
> >
> >
> > ----- 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.
>
> The patch is modified. I think I need to pay more attention to
> maintainable and thanks for your patience.
I reworded the help page a little, but other than that the patch
looks good, and is queued for crash-6.0.5.
Thanks,
Dave
--
Crash-utility mailing list
Crash-utility@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/crash-utility
