Re: How to get file descriptor from abfd?

[Dave Korn <dave.korn.cygwin@googlemail.com>]

Paul Pluzhnikov wrote:
> On Mon, Jun 1, 2009 at 10:54 PM, Paul Pluzhnikov<ppluzhnikov@google.com> wrote:
> 
>> Would attached patch be acceptable?
> 
> Ping?

  I can't approve this as it's outside my area.  I have verified that it
applies cleanly and builds without failures on i686-pc-cygwin (although that
doesn't support --enable-mmap currently for reasons I have yet to discover)
and have just a couple of minor formatting comments on the patch:

>  Index: bfdio.c
> ===================================================================
> RCS file: /cvs/src/src/bfd/bfdio.c,v
> retrieving revision 1.21
> diff -u -p -u -r1.21 bfdio.c
> --- bfdio.c	24 May 2009 11:47:27 -0000	1.21
> +++ bfdio.c	2 Jun 2009 05:30:15 -0000
> @@ -158,6 +158,8 @@ DESCRIPTION
>  .  int (*bclose) (struct bfd *abfd);
>  .  int (*bflush) (struct bfd *abfd);
>  .  int (*bstat) (struct bfd *abfd, struct stat *sb);
> +.  void (*bmmap) (struct bfd *abfd, void *addr, bfd_size_type len,
> +.                 int prot, int flags, file_ptr offset);

  The return type here should be void*, not plain void.

> +static void *
> +cache_bmmap (struct bfd *abfd ATTRIBUTE_UNUSED,
> +	     void *addr ATTRIBUTE_UNUSED,
> +	     bfd_size_type len ATTRIBUTE_UNUSED,
> +	     int prot ATTRIBUTE_UNUSED,
> +	     int flags ATTRIBUTE_UNUSED,
> +	     file_ptr offset ATTRIBUTE_UNUSED)
> +{
> +  void *ret = (void *)-1;

  Gnu coding standards request a space between the cast and the -1.  This also
applies to several other casts throughout the patch.

> +
> +  if ((abfd->flags & BFD_IN_MEMORY) != 0)
> +    abort ();
> +#ifdef HAVE_MMAP
> +  else
> +    {
> +      FILE *f = bfd_cache_lookup (abfd, CACHE_NO_SEEK_ERROR);
> +      if (f == NULL)
> +	return ret;
> +
> +      ret = mmap (addr, len, prot, flags, fileno (f), offset);
> +      if (ret == (void *)-1)
> +	bfd_set_error (bfd_error_system_call);
> +    }
> +#endif
> +
> +  return ret;
> +

  Since abort() doesn't return, the else clause and extra level of nested
braces is superfluous here.  Straight-line code would be cleaner IMO.

  Anyway, that's minor stuff, as I said; the patch looks fundamentally sound
to me, but I can't OK it for you.

    cheers,
      DaveK