On Thu, 2013-09-26 at 21:41 +0200, Petr Machata wrote: > Petr Machata <[email protected]> writes: > > > I haven't had a chance to look at NT_FILE yet, but I intend to. > > pmachata/NT_SIGINFO now includes code for handling NT_FILE as well. > I changed the READ_INT etc. macros to functions (they can be reused for > NT_FILE) and rewrote the branch history with this change in.
I do prefer reviewing patches on the list. You can easily sent patches from the branch to the list for review with something like: $ git send-email [email protected] \ master..pmachata/NT_SIGINFO Looking at the branch: commit 49a1a381a98f0bbe13014d572596055c90ef4b99 Author: Petr Machata <[email protected]> Date: Mon Jun 17 23:52:27 2013 +0200 Update elf.h from glibc Signed-off-by: Petr Machata <[email protected]> > +2013-06-17 Petr Machata <[email protected]> > + > + * elf.h: Update from glibc. This is fine. commit 767633cf8b5bdc4b8a733b958e59e6750b40e33f Author: Petr Machata <[email protected]> Date: Thu Sep 26 00:38:37 2013 +0200 Recognize names of some new core note types in ebl_core_note_type_name Signed-off-by: Petr Machata <[email protected]> > +2013-09-26 Petr Machata <[email protected]> > + > + * eblcorenotetypename.c: Handle NT_ARM_TLS, NT_ARM_HW_BREAK, > + NT_ARM_HW_WATCH, NT_SIGINFO, NT_FILE. Looks fine too. commit c5fce1f17fae87f07a3f880ced819c6fe7987fcc Author: Petr Machata <[email protected]> Date: Thu Sep 26 00:39:34 2013 +0200 Show contents NT_SIGINFO core note in readelf Signed-off-by: Petr Machata <[email protected]> > +static int > +buf_has_data (unsigned char const *ptr, unsigned char const *end, size_t sz) > +{ > + return ptr < end && (size_t)(end - ptr) >= sz; ^ Missing space after closing bracket. > +static int > +buf_read_int (Elf *core, unsigned char const **ptrp, unsigned char const > *end) > +{ > + if (!buf_has_data (*ptrp, end, 4)) ^ I like a space after the ! to make the negation to stand out. But admittedly the file isn't consistent. > + return printf ("<buffer overrun>"), 0; ehe, OK, I admit I didn't know you could do that with a return statement. I assume it returns zero? I would have added int *val as argument and let the function return a boolean or int to signal failure to read the int value. Then in the caller (handle_siginfo_note) just check the return value of buf_read_int and print the error there. Then you can also abort early instead of keeping printing error messages. > +static uint64_t > +buf_read_ulong (Elf *core, unsigned char const **ptrp, unsigned char const > *end) Same comment as above. > +static void > +handle_siginfo_note (Elf *core, GElf_Word descsz, GElf_Off desc_pos) > +{ > + if (descsz != 128) > + return; The magic value needs a comment. Should it be < 128 (to allow expansion of the note content later?) Also don't you want to print some error/unexpected message here? > + /* Siginfo head is three ints: signal number, error number, origin > + code. */ > + int si_signo = buf_read_int (core, &ptr, end); > + int si_errno = buf_read_int (core, &ptr, end); > + int si_code = buf_read_int (core, &ptr, end); With the suggestion above this would become: int si_signo, si_errno, si_code; if (! buf_read_int (core, &ptr, end, &si_signo) || ! buf_read_int (core, &ptr, end, &si_errno) || ! buf_read_int (core, &ptr, end, &si_code)) { printf ("<buffer overrun>"); return; } > + printf (" si_signo:%d, si_errno:%d, si_code:%d\n", > + si_signo, si_errno, si_code); Does it make sense to translate the si_code and si_node into something human readable in the switch case below? > +2013-09-26 Petr Machata <[email protected]> > + > + * Makefile.am (EXTRA_DIST): Add testfile71.bz2. > + * run-readelf-mixed-corenote.sh: New test for this file. > + * testfile71.bz2: New file. Looks fine. But I would like a little note in run-readelf-mixed-corenote.sh how the test file was created just in case someone wants to replicate it later in a different environment. commit 648831244b406446c726584cd09e131f561bc444 Author: Petr Machata <[email protected]> Date: Thu Sep 26 21:02:22 2013 +0200 Show contents NT_FILE core note in readelf Signed-off-by: Petr Machata <[email protected]> > 2013-09-26 Petr Machata <[email protected]> > > + * readelf.c (handle_file_note): New function. > + (handle_notes_data): Call it to handle NT_FILE notes. > + uint64_t count = buf_read_ulong (core, &ptr, end); > + uint64_t page_size = buf_read_ulong (core, &ptr, end); Depending on whether you take the suggestion on the previous patch, these would change a little. There are a couple more. > + char const *fptr = (char *)fstart; ^ Missing space. > + const char *fnext = memchr (fptr, '\0', (char *)end - fptr); ^ Likewise. > 2013-09-26 Petr Machata <[email protected]> > > + * run-readelf-mixed-corenote.sh: Update output of testfile71 > + dump--readelf can newly decode the NT_FILE note. Looks fine. Thanks, Mark
