Re: Create a hook for inspecting program headers during library load

[Roland McGrath <roland at hack dot frob dot com>]

> static inline bool
> elf_machine_phdr_check (const ElfW(Phdr) *phdr, ElfW(Half) phnum,
> 				   const char *buf, ssize_t len, int fd,
> 				   struct link_map *map)

Never use ssize_t in a case like this.  It is only for return values.  
Use size_t here.  Use uint_fast16_t in place of ElfW(Half).
The indentation of the later parameter lines is wrong.

Avoid an ambiguous name like "..._check".  It's better to pick a name
where the sense of the return value is unambiguous, by using a verb in
the name or using the "..._adjective_p" convention.

This has just one caller and that caller is a simple if statement, so it
is cleaner if the sense of the function's value is in the direction the
if test wants.  IOW, "if (__glibc_unlikely (...))" always looks better
than "if (!__glibc_likely (...))" (and avoids the discussion about
whether "if (__glibc_unlikely (!...))" is preferable).

> Does this prototype seem acceptable?

Not far off, anyway.

> Does the addition of the stub implementation to the generic architecture
> qualify as documentation, I can't see anywhere else like a 'new port'
> guide to update?

Yes, that's sufficient if the comments are thorough and clear.  We don't
have lots of internals documentation, and where/when we do it is not
usually worthwhile for it to be so specific when comments in code are
better-suited.

> What sort of testing is needed to commit this (I have only done
> x86_64-linux-gnu and mips*-linux-gnu)?

Ideally build-testing of the other machines you've touched.  But it's not
really necessary in a boilerplate case like this.

> I am happy to put the outcome of this thread on the wiki.

I'm not sure what addition you have in mind that would be useful.


Please avoid using attachments if you possibly can.  We really want just
plain text patches at the end of the plain text message.  (If you really
can't, then consider getting a MUA that doesn't constrain your composition
choices unreasonably.)


The approach you've taken is probably fine.  (However, it does concern me
that you added boilerplate to all the dl-machine.h files and made exactly
one nonidentical: the mips one is "static bool __attribute_used__" for no
apparent reason while the others are all "static inline bool".)  But when
you have to repeat a boilerplate definition a bunch of times, and
especially when it seems likely that most of those places will never
change, then it should be a red flag.  An alternative approach is to add
a new sysdeps header file just for the new purpose.  Then you not only do
not have to repeat the boilerplate (because you just write one
sysdeps/generic/--or in this case, perhaps just elf/--stub version that
everything uses), but you touch only "generic" code and so even the pro
forma ideal testing requirements are less.


Thanks,
Roland