This is the mail archive of the elfutils-devel@sourceware.org mailing list for the elfutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: Handling NT_SIGINFO, NT_FILE in core files


On Thu, 2013-09-26 at 21:41 +0200, Petr Machata wrote:
> Petr Machata <pmachata@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@lists.fedorahosted.org \
    master..pmachata/NT_SIGINFO

Looking at the branch:

commit 49a1a381a98f0bbe13014d572596055c90ef4b99
Author: Petr Machata <pmachata@redhat.com>
Date:   Mon Jun 17 23:52:27 2013 +0200

  Update elf.h from glibc
    
  Signed-off-by: Petr Machata <pmachata@redhat.com>

> +2013-06-17  Petr Machata  <pmachata@redhat.com>
> +
> +       * elf.h: Update from glibc.

This is fine.


commit 767633cf8b5bdc4b8a733b958e59e6750b40e33f
Author: Petr Machata <pmachata@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@redhat.com>

> +2013-09-26  Petr Machata  <pmachata@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@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@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@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@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@redhat.com>

> 2013-09-26  Petr Machata  <pmachata@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@redhat.com>
>  
> +       * run-readelf-mixed-corenote.sh: Update output of testfile71
> +       dump--readelf can newly decode the NT_FILE note.

Looks fine.

Thanks,

Mark


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]