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



  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.

Only one IOVEC required the mechanism, and there, for a very specalized reason (as you note) - a shortage of file descriptors. By moving that complexity into the FILE IOVEC, BFD proper was simplified.


When it turns out that a generalized FD-cache mechanism is needed then, the person needing the mechanism can add it :-) (at present we've only memory, FILE and GDB-inferior IOVECs and only FILE has the problems).

Anyway, and regardless, I think that such a mechanism belongs in the lower layers (IOVEC) and should not be directly tied to BFD. By keeping it separate from BFD clients (i.e., GDB) use the underlying FD-cache to manage other open files - the source code.

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:

ISTM?


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;
+}

As you note, each has advantages and disadvantages. Where the interface external I'd have done what you suggest. It isn't so I made it as simple and sparse as possible.


Also, one of the long ago original threads expressed concern at the call overhead (Ok I don't be believe it to be significant), but hey this looks leaner :-)

  (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?

I'm lost. It's the memmory code, that's going to need many changes.


Andrew




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