Re: [Crash-utility] Make note information human readable when help -D

----- Original Message ----- > Hello Dave, > > Thank you for suggestions. > I think I have further understanding of "keep it simple". > > This time, there are five functions in the patch. > They are: > display_ELF_note() > display_prstatus_x86_64() > display_prstatus_x86() > display_qemu_x86_64() > display_qemu_x86() > > It looks much better. > Previously, I tried to avoid duplicate code though it can bring > more complexity. Now, I think simple in logic is more important. > I do appreciate your teaching me so much. Hello Zhou, Thank you for tightening this up -- it is looking much better. But I still have a few more questions, suggestions, and further simplifications. In diskdump.c, you have this declaration: +void display_ELF_note(int, int, void *, char *); + Please move this declaration in defs.h with the other function prototypes under "netdump.c". You can also put the new PRSTATUS and QEMU #define's right underneath the function declaration instead of putting them at the bottom of defs.h. Also in diskdump.c: @@ -1736,10 +1738,18 @@ __diskdump_memory_dump(FILE *fp) dd->num_prstatus_notes); fprintf(fp, " notes_buf: %lx\n", (ulong)dd->notes_buf); + + char *l_buf = (char *)malloc(2 * BUFSIZE); for (i = 0; i< dd->num_prstatus_notes; i++) { fprintf(fp, " notes[%d]: %lx\n", i, (ulong)dd->nt_prstatus_percpu[i]); + + display_ELF_note(dd->machine_type, PRSTATUS, + dd->nt_prstatus_percpu[i],l_buf); + fprintf(fp, "%s", l_buf); + memset(l_buf, 0, 2 * BUFSIZE); } + free(l_buf); dump_nt_prstatus_offset(fp); } if (dh->header_version>= 5) { First, what's the reason for the memset() call after the data has been displayed? Aren't they always going to be the exact same, NULL-terminated, size? Secondly, the use of malloc() is discouraged during the runtime of a crash session, such as when "help -D" is entered. The reason for that is because: (1) a crash command may fail and call the error(FATAL) function for any number of reasons, or (2) CTRL-c may be entered in the middle of the command. In both cases, RESTART() is called, which does a longjmp() back to main(). So if malloc() had been called prior to either of those two occurances, the buffer would be leaked. That's the reason behind using the GETBUF()/FREEBUF facility, because buffers that GETBUF() returns are guaranteed to be freed prior to the next "crash>" prompt, regardless whether FREEBUF() had been called. However, since "crash -d1" will also call __diskdump_memory_dump() very early on before main() calls buf_init() -- which sets up the GETBUF()/FREEBUF() facility -- GETBUF()/FREEBUF() aren't available for use at that time. So to avoid having to choose between malloc() or GETBUF(), I would just use a char[BUFSIZE*2] buffer automatically declared on the stack in __diskdump_memory_dump(). In netdump.c, in the 3 places you call display_ELF_note() you qualify it by checking "if (nd->ofp)". I'm not sure why you do that? You should be able to just print the whole buffer by entering: netdump_print("%s", l_buf); Correct?

And like diskdump.c, the ELF dumping functions can be called immediately during initialization if "crash -d1" is invoked, so GETBUF() cannot be used, and using malloc() during runtime via "help -D" could cause a memory leak. SO again, please just declare a char[] buffer on the function stack. And lastly, you'll note that none of the crash command output ever use tabs as you have done. Can you replace them with spaces, or alternatively, use the space(count) function to return a pointer to a string with "count" number of spaces? Thanks, Dave