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

Dave Anderson anderson at redhat.com
Mon Dec 15 21:50:55 UTC 2014



----- 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





More information about the Crash-utility mailing list