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

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.