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

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