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

["Zhou, Wenjian/周文剑"]

On 12/16/2014 05:50 AM, Dave Anderson wrote:
>
>
> ----- 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?
>
All have been addressed except this. Originally, I used the function
netdump_print(), but BUFSIZE in netdump_print() is not enough. So I
changed it to fprintf().

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


-- 
Thanks
Zhou Wenjian
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: 0001-Make-note-info-human-readable-when-help-D.patch
URL: <http://listman.redhat.com/archives/crash-utility/attachments/20141216/07ac19cb/attachment.ksh>