This is the mail archive of the elfutils-devel@sourceware.org mailing list for the elfutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: elf_cntl(ELF_C_FDREAD) breaks elf64_getshdr() with non-mmap()ed ELF files


Please put parens around & inside || and the like.

This seems vaguely reasonable to me, but OTOH since in the mmap case we do
eagerly set up the shdr pointers in file_read_elf, I think it makes sense
to be consistent and do that in __libelf_readall too.  It should also save
a little memory to not malloc the ehdr and shdr copies.  That means that
__libelf_readall really ought to free the old ehdr copy (which is always
cached) and the shdr copies if they were cached before.  Most of the code
can just be broken out of file_read_elf into a subroutine,
e.g. __libelf_maybe_precache_headers.  __libelf_readall will just need to
free the old state and clear the shdr_malloced flag.

Let's start with just putting in your test case, which will fail today.
Never use -1 as the exit status.  The norm is 2 for user error and 1 for
failure.

+  int fd;
+  if ((fd = open (argv[2], O_RDONLY)) < 0)

Only use an assignment inside a conditional when it's really the most
natural and least repetitive thing.  Here you can do the assignment
as the initializer in the definition instead.



Thanks,
Roland


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]