This is the mail archive of the
binutils@sources.redhat.com
mailing list for the binutils project.
Re: [rfa] Add the bfd_iovec
- From: Andrew Cagney <cagney at gnu dot org>
- To: Dave Korn <dk at artimi dot com>
- Cc: binutils at sources dot redhat dot com
- Date: Fri, 16 Apr 2004 13:28:08 -0400
- Subject: Re: [rfa] Add the bfd_iovec
- References: <NUTMEGIf4RCBzdKKbpO000004a6@NUTMEG.CAM.ARTIMI.COM>
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