On Tue, 2014-04-15 at 16:58 +0200, Florian Weimer wrote:
> This change also adds more error checking and reporting.
>
> +	* allfcts.c (setup_alt): New function.
> +	(main): Call it.  Implementation additional error checking and
> +	reporting.

> +static Dwarf *
> +setup_alt (Dwarf *main)
> +{
> +  const void *build_id;
> +  size_t build_id_len;
> +  const char *alt_name = dwelf_dwarf_gnu_debugaltlink
> +    (main, &build_id, &build_id_len);
> +  if (alt_name == NULL)
> +    return NULL;
> +  int fd = open (alt_name, O_RDONLY);
> +  if (fd < 0)
> +    err (1, "open (%s)", alt_name);
> +  Dwarf *dbg_alt = dwarf_begin (fd, DWARF_C_READ);
> +  close (fd);

I am slightly surprised this actually works. Normally you cannot just
close the fd of the underlying Dwarf (or Elf). For an Elf you can if you
force it to have been read completely into memory first by doing
elf_cntl (elf, ELF_C_FDREAD) as is currently done in try_debugaltlink.
Which you could do here (on the Elf and then call dwarf_begin_elf) if
you want to make sure to not leak the fd. But otherwise you need to keep
the fd around and close it after calling dwarf_end.

>  		  doff = dwarf_getfuncs (die, cb, NULL, doff);
> +		  if (dwarf_errno () != 0)
> +		    errx (1, "dwarf_getfuncs (%s): %s",
> +			  argv[i], dwarf_errmsg (-1));

I think this works fine, and the original code did this, but in a less
convenient way (you wouldn't get any error reported). So keep it as is.

But the way dwarf_getfuncs returns an error state is by returning -1.
Which isn't really documented and somewhat awkward, so what you do is
nicer. It is just that theoretically a callback could trigger
dwarf_errno being set and ignore that as being fine, which might be
silly, but "legal". You would then pick up a non-fatal dwarf_errno.

Cheers,

Mark