This is the mail archive of the binutils@sources.redhat.com mailing list for the binutils 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: [rfa] Add the bfd_iovec


> -----Original Message-----
> From: binutils-owner  On Behalf Of Andrew Cagney
> Sent: 15 April 2004 20:28

> > Thanks.  I like it.
> > 
> > Does anybody dislike it?
> 
> I'll give it 24 hrs, in the mean time this has more doco fixes.
> 
> Andrew


  I like it too, it's clearly a good piece of interface abstraction, but I
have a couple of comments/queries:  first this, from bfdio.c
 
------->snip!<-------
@@ -121,25 +123,10 @@
       return get;
     }
 
-  nread = real_read (ptr, 1, (size_t) size, bfd_cache_lookup (abfd));
+  nread = abfd->iovec->bread (abfd, ptr, size);
   if (nread != (size_t) -1)
     abfd->where += nread;
 
-  /* Set bfd_error if we did not read as much data as we expected.
-
-     If the read failed due to an error set the bfd_error_system_call,
-     else set bfd_error_file_truncated.
-
-     A BFD backend may wish to override bfd_error_file_truncated to
-     provide something more useful (eg. no_symbols or wrong_format).  */
-  if (nread != size)
-    {
-      if (ferror (bfd_cache_lookup (abfd)))
-	bfd_set_error (bfd_error_system_call);
-      else
-	bfd_set_error (bfd_error_file_truncated);
-    }
-
   return nread;
 }
------->snip!<-------

  I don't understand why you removed the error handling here.  You haven't
done so for other functions.  Since you're providing an interface that
basically abstracts the standard FILE * functions, I think you should keep
the error handling as-is; I'm not sure if you were expecting it to move into
the iovec callback but I don't think that would make sense.  [ It occurs to
me that this code may simply have got lost by accident, in which case you
should certainly restore it before checking in. ]

------->snip!<-------
@@ -175,7 +162,7 @@
       return size;
     }
 
-  nwrote = fwrite (ptr, 1, (size_t) size, bfd_cache_lookup (abfd));
+  nwrote = abfd->iovec->bwrite (abfd, ptr, size);
   if (nwrote != (size_t) -1)
     abfd->where += nwrote;
   if (nwrote != size)
------->snip!<------- 

  From my reading of your code, you seem to have removed the caching
functionality from the bfd layer and moved it into a specialized iovec.  I
don't think this is the right decision.  Since we're making an abstract I/O
API that is modelled on the standard FILE * functions, and since the caching
functionality that already exists works using FILE * functions, it would
make more sense to me if the caching remained layered *over* the iovec
abstraction.  That way caching could be applied to any iovec regardless of
the underlying IO method, couldn't it?  The iovec should maybe contain a
flag to indicate to the caching layer whether bfd-fd-caching is a win or not
for the underlying IO method.

  I also want to talk about the approach to iovecs with limited
functionality - read-only, for instance, since you introduce one of those
later.  ISTM that there are three sensible ways of handling this situation: 

1) You can specify that every iovec has to have the function pointers set
for every callback, and supply dummy routines that return error indications
for functionality such as bwrite () that doesn't exist;

2) You can specify that if a function isn't provided the pointer in the
iovec should be NULL, and include a guard test at every site that calls
through the iovec table; or

3) Same as 2), but you provide a set of wrappers, so instead of calling

+  nwrote = abfd->iovec->bwrite (abfd, ptr, size);

you'd say

+  nwrote = bfd_iovec_bwrite (abfd->iovec, abfd, ptr, size);

and supply a wrapper function that incorporates the error checking from 1)

+file_ptr
+bfd_iovec_bwrite (struct iovec *vec, struct bfd *abfd,
+	      const void *where, file_ptr nbytes )
+{
+  if (vec->bwrite)
+    return vec->bwrite (abfd, where, nbytes);
+  return -1;
+}

thereby consolidating all the error checking for all iovecs with
unimplemented functions in one place.  I haven't verified if you'd ever need
to pass in an iovec that wasn't the one from abfd, or if you could even
simplify it to 

+  nwrote = bfd_iovec_bwrite (abfd, ptr, size);

+file_ptr
+bfd_iovec_bwrite (struct bfd *abfd,
+	      const void *where, file_ptr nbytes )
+{
+  if (abfd && abfd->vec && abfd->vec->bwrite)
+    return abfd->vec->bwrite (abfd, where, nbytes);
+  return -1;
+}

  (BTW, since this is an internal function, it should maybe actually be
called _bfd_iovec_bwrite instead.)

  I'm curious to know the reasoning behind your design decisions.  To me it
seems that 3) is the cleanest and most generalized abstraction, and probably
more future-proof / maintainable; the extra layer of wrappers may seem a bit
of an overhead, but IMO it's pretty trivial compared to the actual io
operation that's going to be taking place, and it provides a more robust
interface for future iovecs that people may implement.

  Finally,

------->snip!<------- 
@@ -415,7 +593,12 @@
   if (! BFD_SEND (abfd, _close_and_cleanup, (abfd)))
     return FALSE;
 
-  ret = bfd_cache_close (abfd);
+  /* FIXME: cagney/2004-02-15: Need to implement a BFD_IN_MEMORY io
+     vector.  */
+  if (!(abfd->flags & BFD_IN_MEMORY))
+    ret = abfd->iovec->bclose (abfd);
+  else
+    ret = 0;
------->snip!<------- 

  Isn't it likely to break something if you check this in without doing so? 

    cheers, 
      DaveK
-- 
Can't think of a witty .sigline today....


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