Re: [PATCH v3 3/6] libdwelf: Add dwelf_dwarf_gnu_debugaltlink

[Mark Wielaard <mjw at redhat dot com>]

On Tue, 2014-04-15 at 14:49 +0200, Florian Weimer wrote:
> +2014-04-15  Florian Weimer  <fweimer@redhat.com>
> +
> +	* dwelf_dwarf_gnu_debugaltlink.c: New file.
> +	* Makefile.am (libdwelf_a_SOURCES): Add it.
> +	* libdwelf.h (dwelf_dwarf_gnu_debugaltlink): Declare new function.
> +	* libdwelfP.h (dwelf_dwarf_gnu_debugaltlink): Add internal
> +	declaration.

The code looks fine. But I still like the "tri-state" return more to
indicate why/how things went. Precisely because then you don't have to
do:

> +  Elf_Data *data = dwarf->sectiondata[IDX_gnu_debugaltlink];
> +  if (data == NULL)
> +    {
> +      /* Missing data is not actually an error. */
> +      __libdw_seterrno (0);
> +      return NULL;
> +    }
> [...]
> +/* Returns the name of the alternate debug information file from the
> +   .gnu_debugaltlink section if found in the ELF.  On success, writes
> +   a pointer to the build ID to *BUILD_IDP, and its length to
> +   *BUILD_ID_LENP.  Returns NULL if the ELF file didn't have a
> +   .gnu_debuglink section (and sets the dwarf_errcode error code to
> +   0), had malformed data in the section or some other error occured.
> +   The returned pointers are valid while the ELF handle is valid.  */
> +extern const char *dwelf_dwarf_gnu_debugaltlink (Dwarf *dwarf,
> +						 const void **build_idp,
> +						 size_t *build_id_lenp);

So I would write this as:

/* Returns the name and build_id of the alternate debug information file
   of the given DWARF if found through the .gnu_debugaltlink section. On
   success, writes a pointer to the name to *NAMEP and a pointer to the
   build ID to *BUILD_IDP and returns the length of the build_id.
   Returns zero if the DWARF didn't have an alternate debug information
   file in its ELF file .gnu_debuglink section. Returns -1 for errors
   and sets dwarf_errno.  The returned pointers are valid while the
   DWARF handle is valid.  */
extern int dwelf_dwarf_gnu_debugaltlink (Dwarf *dwarf,
                                         const char **namep,
                                         const void **build_idp);

> +2014-04-15  Florian Weimer  <fweimer@redhat.com>
> +
> +	* debugaltlink.c, run-debugaltlink.sh: New files.
> +	* Makefile.am (check_PROGRAMS): Add debugaltlink.
> +	(TESTS): Add run-debugaltlink.sh.
> +	(debugaltlink_LDADD): New variable.

Thanks for adding the testcase. run-debugaltlink.sh should also be added
to EXTRA_DIST (I also forgot that in my dwelf_elf_gnu_debuglink patch
(fixed on the mjw/dwelf branch).

Cheers,

Mark