Hi Aaron,
On Sun, Jun 22, 2025 at 07:02:04PM -0400, Aaron Merey wrote:
> Signed-off-by: Aaron Merey <ame...@redhat.com>
>
> ---
> v2: Clarify that the offset refers to the archive member header.
>
> > > +.SH RETURN VALUE
> > > +Return the file offset, in bytes, of the archive member referred to by
> > > +.IR elf .
> > > +If
> > > +.I elf
> > > +is NULL or is not a member of an archive,
> > > +return
> > > +.BR ELF_C_NULL .
> >
> > ehe, yes, that is true, although confusing...
> > It also isn't what other implementations seem to do, which return -1.
> > And it looks like we actually expect -1 ourselves in ar.c and ranlib.c
> > Groan :{
> > Might this really be a bug that nobody noticed before?
> > Should we fix it? Or is there a big risk we have users that rely on it
> > returning ELF_C_NULL instead of -1?
>
> IMO we should just leave it as is. elf_getaroff was added nearly 20
> years ago and I can't find any complaints about about this detail.
> But if we change this now we might get complaints.
But it doesn't really make sense. Our own code checks for -1 to detect
errors. Given it returns an offset (int64_t) it makes sense that an
error is represented as -1. It also seems to be what other
implementations do (only checked the mr511 implementation which seems
to be the only other one that implements elf_getaroff).
It also only matters for error management. Which might explain why
nobody noticed since you would normally call this just if you know you
have a valid ar member. Which seems to be what our own ar.c code does.
> doc/Makefile.am | 1 +
> doc/elf_getaroff.3 | 59 ++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 60 insertions(+)
> create mode 100644 doc/elf_getaroff.3
>
> diff --git a/doc/Makefile.am b/doc/Makefile.am
> index fbfebfe0..6451ffab 100644
> --- a/doc/Makefile.am
> +++ b/doc/Makefile.am
> @@ -55,6 +55,7 @@ notrans_dist_man3_MANS= elf32_checksum.3 \
> elf_errmsg.3 \
> elf_errno.3 \
> elf_fill.3 \
> + elf_getaroff.3 \
> elf_getbase.3 \
> elf_getdata.3 \
> elf_getscn.3 \
> diff --git a/doc/elf_getaroff.3 b/doc/elf_getaroff.3
> new file mode 100644
> index 00000000..3a393e5d
> --- /dev/null
> +++ b/doc/elf_getaroff.3
> @@ -0,0 +1,59 @@
> +.TH ELF_GETAROFF 3 2025-06-06 "Libelf" "Libelf Programmer's Manual"
> +
> +.SH NAME
> +elf_getaroff \- retrieve the offset of an archive member header
> +
> +.SH SYNOPSIS
> +.nf
> +.B #include <libelf.h>
> +
> +.BI "int64_t elf_getaroff(Elf *" elf ");"
> +.fi
> +.SH DESCRIPTION
> +Return the file offset, in bytes, of the archive member header currently
> +referred to by an ELF descriptor. This is the offset of the member header
> +in the parent archive file. This offset can be used with
> +.BR elf_rand .
> +
> +.SH PARAMETERS
> +.TP
> +.I elf
> +Elf descriptor referring to a member of an archive file header.
> +
> +.SH RETURN VALUE
> +Return the file offset, in bytes, of the archive member header referred
> +to by
> +.IR elf .
> +If
> +.I elf
> +is NULL or is not a member of an archive,
> +return
> +.BR ELF_C_NULL .
So I really think we should fix the implementation and return -1 on error.
> +.SH SEE ALSO
> +.BR elf_begin (3),
> +.BR elf_next (3),
> +.BR elf_rand (3),
> +.BR libelf (3),
> +.BR elf (5)
> +
> +.SH ATTRIBUTES
> +.TS
> +allbox;
> +lbx lb lb
> +l l l.
> +Interface Attribute Value
> +T{
> +.na
> +.nh
> +.BR elf_getaroff ()
> +T} Thread safety MT-Safe
> +.TE
> +
> +.SH REPORTING BUGS
> +Report bugs to <elfutils-devel@sourceware.org> or
> https://sourceware.org/bugzilla/.
> +
> +.SH HISTORY
> +.B elf_getaroff
> +first appeared in elfutils 0.114. This elfutils libelf function may not be
> +found in other libelf implementations.
Looks good otherwise.
Thanks,
Mark
