Re: [PATCH] Core file build-ids

[Nick Clifton <nickc at redhat dot com>]

Hi Keith,

  I do have a couple of comments on this patch:

> +void> +_bfd_elf_core_find_build_id (bfd *templ, bfd_vma offset)> +{> +  return (*get_elf_backend_data (templ)->elf_backend_core_find_build_id)> +    (templ, offset);
I would recommend being a little bit paranoid here and checking
that templ is an elf format bfd before attempting to access the
backend data.


> -      return _bfd_elf_make_section_from_phdr (abfd, hdr, hdr_index, "load");
> +      if (! _bfd_elf_make_section_from_phdr (abfd, hdr, hdr_index, "load"))
> +	return FALSE;
> +      if (bfd_get_format (abfd) == bfd_core && abfd->build_id == NULL)
> +	_bfd_elf_core_find_build_id (abfd, hdr->p_offset);
> +      return TRUE;

If _bfd_elf_core_find_build_id can fail (see above and below) then 
it would be worth checking the return value for an error result.


> +/* Attempt to find a build-id in a core file from the core file BFD.
> +   OFFSET is the file offset to a PT_LOAD segment that may contain
> +   the build-id note.  */
> +
> +void
> +NAME(_bfd_elf,core_find_build_id)

Given that things can wrong when attempting to find a build_id, I think
that this function should return a bfd_boolean.  I appreciate that the
caller could just check abfd->build_id != NULL, (assuming that it was NULL
to start with), but I think that it is cleaner that the function itself
should tell its caller whether ot not it succeeded.


> +  Elf_External_Ehdr x_ehdr;	/* Elf file header, external form */
> +  Elf_Internal_Ehdr i_ehdr;	/* Elf file header, internal form */

Minor formatting nit: comments should end with a full stop and two spaces.

Cheers
  Nick