Hello Wang,
I've re-studied this section, and I still have a few more issues
to resolve with this patch. But this should be the final review,
and I will wait for your v4 update before releasing 5.1.5.
> 1) Add a new option --no_elf_notes. If specified, it'll skip processing
> elf notes in kdump-compressed dump files.
OK good -- but please move the (dd->flags & NO_ELF_NOTES) check from
x86_process_elf_notes() into read_dump_header() so as to prevent the
allocation of any buffers if they're never going to be used, i.e.:
/* process elf notes data */
if (KDUMP_CMPRS_VALID() && !(dd->flags & NO_ELF_NOTES) &&
(dd->header->header_version >= 4) &&
And please make the --no_elf_notes option only applicable to x86 and x86_64
in order to prevent it from being applied to the s390x arch:
else if (STREQ(long_options[option_index].name, "no_elf_notes")) {
if (machine_type("X86") || machine_type("X86_64"))
*diskdump_flags |= NO_ELF_NOTES;
else
error(INFO,
"--no_elf_notes is only applicable to ",
"the X86 and X86_64 architectures.\n");
}
> 2) Dump notes related information including total number of notes, their
> buffers' pointer value and finally offset in the dump file.
> 3) Move nt_prstatus_percpu and nt_prstatus_percpu to diskdump_data struct.
> However, bt command may need sp/ip from the notes later, so I guess
> the buffer should not be freed after diskdump_read_header().
I wish I had understood the s390x implementation a bit better when I
reviewed your v2 patch. Currently read_dump_header() malloc's a
single "notes_buf" buffer and copies the complete ELF notes section
from the dumpfile into it. I believe (but am not completely sure) that
the s390x code references pieces of that buffer, and that's why they
originally kept the "notes_buf" buffer permanently allocated. That
being the case, the "notes_buf" pointer should be moved to the
diskdump header.
Anyway, in the case of x86/x86_64, your patch:
(1) malloc's dd->nt_prstatus_percpu as an array of pointers,
(2) malloc's a bunch of individual per-cpu ELF prstatus buffers, and
then copies the ELF data from the "notes_buf" buffer into
each per-cpu buffer.
(4) puts a pointer to each per-cpu buffer into each dd->nt_prstatus_percpu
pointer.
But given that the original "notes_buf" buffer is permanent, your patch
should only have to store per-cpu pointers into the original note_buf
buffer -- rather than redundantly malloc'ing a bunch of new buffers that
contain copies of what's already permanently available. So I would suggest
setting it up like this:
dd->nt_prstatus_percpu[cpu] => points into relevant "notes_buf" location
Also, even though the s390x will not utilize the dd->nt_prstatus_percpu[]
array, your dump_nt_prstatus_offset() function should be usable by
the s390x since it uses the same Elf64_Nhdr as x86_64.
And lastly, two other minor things:
Move the user_regs_struct_eip to the bottom of the offset_table.
The offset_table is exported to extension modules, so changing
members in the middle of the structure could easily break a module
that was compiled against an earlier version. Note that it has
this at the top:
struct offset_table { /* stash of commonly-used offsets */
long list_head_next; /* add new entries to end of table */
long list_head_prev;
long task_struct_pid;
long task_struct_state;
...
Finally, add a display of the user_regs_struct_eip offset_table entry value
into dump_offset_table(), which is used by "help -o". In that function you
can put the user_regs_struct_eip display next to its other user_regs_struct
partners.
Thanks,
Dave
--
Crash-utility mailing list
Crash-utility@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/crash-utility
