[Crash-utility] Make note information human readable when help -D
"Zhou, Wenjian/周文剑"
zhouwj-fnst at cn.fujitsu.com
Fri Dec 12 04:02:14 UTC 2014
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.
On 12/12/2014 01:06 AM, Dave Anderson wrote:
>
>
> ----- Original Message -----
>> On 12/11/2014 06:27 AM, Dave Anderson wrote:
>>
>>> First, please address all of these warnings:
>>>
>>> $ make warn
>>> ... [ cut ] ...
>>> cc -c -g -DX86_64 -DLZO -DSNAPPY -DGDB_7_6 netdump.c -Wall -O2
>>> -Wstrict-prototypes -Wmissing-prototypes -fstack-protector
>>> -Wformat-security
>>> netdump.c: In function 'dump_Elf32_Nhdr':
>>> netdump.c:1987:4: warning: format not a string literal and no format
>>> arguments [-Wformat-security]
>>> netdump.c: In function 'dump_Elf64_Nhdr':
>>> netdump.c:2272:4: warning: format not a string literal and no format
>>> arguments [-Wformat-security]
>>> netdump.c:2303:4: warning: format not a string literal and no format
>>> arguments [-Wformat-security]
>>> ...
>>>
>>
>> Hello Dave,
>>
>> These warnings have been addressed.
>>
>>> Secondly, for compressed kdumps in diskdump.c, you have this construct:
>>>
>>> if (dd->machine_type == EM_386)
>>> display_note_elf32(dd->nt_prstatus_percpu[i],l_buf);
>>> else if (dd->machine_type == EM_X86_64)
>>> display_note_elf64(dd->nt_prstatus_percpu[i],l_buf);
>>>
>>> But for ELF kdumps in netdump.c, display_note_elf32() are display_note_elf64()
>>> look to be called unconditionally. What about the other architectures?
>>>
>>
>> I distinguish the architectures by the struct size in display_note(). I wonder
>> if it is needed to add other conditions like machine_type.
>>
>>
>> display_note(void *note_ptr, char *buf, int descsz)
>> {
>> if (descsz == (2 * sizeof(struct x86_64_prstatus)))
>> display_prstatus_elf64(note_ptr, buf);
>> else if (descsz == sizeof(struct x86_prstatus))
>> display_prstatus_elf32(note_ptr, buf);
>> else if (descsz == (2 * sizeof(QEMUCPUState)))
>> display_qemu_elf64(note_ptr, buf);
>> else if (descsz == sizeof(QEMUCPUState))
>> display_qemu_elf32(note_ptr, buf);
>> }
>
> Yes, it should definitely take the machine type into account, given
> that it's not guaranteed to work with any other architectures without
> knowing exactly what the size of their notes would be.
>
> In fact this whole patchset needs to be made more machine-type specific.
> The myriad of new functions you've added to netdump.c are generally confusing
> to me because several of their names imply that they are generic to ELF,
> and not specific to x86 and x86_64. Yes, the data being displayed is contained
> within an ELF note, but more importantly, the data itself is machine-specific.
>
> So can we please simplify/clarify things a bit?
>
> Here's what I suggest. Given the fact that display_prstatus_el64()
> and display_prstatus_el32 are coming from two different dumpfile locations
> (and perhaps more in the future?), why not add machine type and note type
> arguments? To reduce the number of functions, create a general purpose
> "display_ELF_note()" function that can distribute the work appropriately?
>
> So diskdump.c can be simplified from:
>
> if (dd->machine_type == EM_386)
> display_note_elf32(dd->nt_prstatus_percpu[i],l_buf);
> else if (dd->machine_type == EM_X86_64)
> display_note_elf64(dd->nt_prstatus_percpu[i],l_buf);
> to just:
>
> display_ELF_note(dd->machine_type, PRSTATUS, dd->nt_prstatus_percpu[i], l_buf);
>
> And the 3 netdump.c invocations would be changed from:
>
> display_note_elf32(note, l_buf);
> display_note_elf32(note, l_buf);
> display_note_el64((note, l_buf);
>
> to:
> display_ELF_note(EM_386, qemuinfo ? QEMU : PRSTATUS, note, l_buf);
> display_ELF_note(EM_386, PRSTATUS, note, l_buf);
> display_ELF_note(EM_X86_64, qemuinfo ? QEMU : PRSTATUS, note, l_buf);
>
> Then the general-purpose display_ELF_note() function can directly call
> the appropriate function. And those architecture-specific functions
> should be should be renamed to append "_x86" or "_x86_64" (as you have
> done with "x86_prstatus" and "x86_64_prstatus" structures).
>
> So the display_ELF_note() function could look something like:
>
> void
> display_ELF_note(int machine, int type, void *note, char *buf)
> {
> switch (machine)
> {
> case EM_386:
> switch (type)
> {
> case PRSTATUS:
> display_prstatus_x86(note, buf);
> break;
> case QEMU:
> display_qemu_x86(note, buf);
> break;
> }
> break;
>
> case EM_X86_64:
> switch (type)
> {
> case PRSTATUS:
> display_prstatus_x86_64(note, buf);
> break;
> case QEMU:
> display_qemu_x86_64(note, buf);
> break;
> }
> break;
>
> default:
> return;
> }
> }
>
> And in the future, functions can simply be added for other architectures.
>
> Now, I understand that your display_qemu_elf32() and display_qemu_elf64()
> functions call into your single display_qemu_elf() function because there
> is common code. That could be handled in few different manners.
>
> You could consolidate my suggested display_qemu_x86() and display_qemu_x86_64()
> functions into a "common" function that is named display_qemu_x86_32_64(), or
> display_qemu_intel(), or whatever (?).
>
> Alternatively, you could have display_ELF_note() call an architecture-neutral
> display_qemu() function, which can set up things based upon the machine-type,
> and then call the architecture-specific function -- which in this case would
> be the common function used by both x86 and x86_64.
>
> Or your display_qemu_elf() function could be broken into two separate
> functions, which would contain some common/duplicate code. That's also
> fine with me.
>
> However you do it, all I ask is that if a function only applies to a specific
> architecture, then please follow the structure-naming convention so that there
> is no question.
>
> 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/20141212/be6a6936/attachment.ksh>
More information about the Crash-utility
mailing list