On Thu, 2013-09-26 at 21:41 +0200, Petr Machata wrote:
Petr Machata <pmachata(a)redhat.com> 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 --to=elfutils-devel(a)lists.fedorahosted.org \
master..pmachata/NT_SIGINFO
Looking at the branch:
commit 49a1a381a98f0bbe13014d572596055c90ef4b99
Author: Petr Machata <pmachata(a)redhat.com>
Date: Mon Jun 17 23:52:27 2013 +0200
Update elf.h from glibc
Signed-off-by: Petr Machata <pmachata(a)redhat.com>
+2013-06-17 Petr Machata <pmachata(a)redhat.com>
+
+ * elf.h: Update from glibc.
This is fine.
commit 767633cf8b5bdc4b8a733b958e59e6750b40e33f
Author: Petr Machata <pmachata(a)redhat.com>
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 <pmachata(a)redhat.com>
+2013-09-26 Petr Machata <pmachata(a)redhat.com>
+
+ * 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 <pmachata(a)redhat.com>
Date: Thu Sep 26 00:39:34 2013 +0200
Show contents NT_SIGINFO core note in readelf
Signed-off-by: Petr Machata <pmachata(a)redhat.com>
+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 <pmachata(a)redhat.com>
+
+ * 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 <pmachata(a)redhat.com>
Date: Thu Sep 26 21:02:22 2013 +0200
Show contents NT_FILE core note in readelf
Signed-off-by: Petr Machata <pmachata(a)redhat.com>
2013-09-26 Petr Machata <pmachata(a)redhat.com>
+ * 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 <pmachata(a)redhat.com>
+ * run-readelf-mixed-corenote.sh: Update output of testfile71
+ dump--readelf can newly decode the NT_FILE note.
Looks fine.
Thanks,
Mark