Re: [PATCH] Encapsulate target_fileio file descriptors

[Gary Benson <gbenson at redhat dot com>]

Pedro Alves wrote:
> On 03/18/2015 01:20 PM, Gary Benson wrote:
> > Various target_fileio_* functions use integer file descriptors to
> > refer to open files, which can cause problems if the target stack
> > changes between the file being opened and other file operations
> > being performed on that file.  This commit makes the
> > target_fileio_* functions that use file descriptors use an opaque
> > target_fileio_t handle instead that encapsulates both the file
> > descriptor itself and the target_ops vector that should be used
> > to access it.
> 
> Thanks Gary.
> 
> I think this deserves an expanded explanation, for posterity.
...
> And obviously, if FD happens to be a file descriptor number that
> happens to be open in GDB as well, we'll close it by mistake.
> That's the sort of random issue that wouldn't be very fun to track
> down!

Yeah, like GDB printing 350MB of poll errors because it closed one
of its readline pipes :)

> It works today, though I suspect that it may cause problems in
> the future.  When we get to multi-target, and target_ops instances
> become objects instantiated on the heap, we'll have the problem that
> the target_ops pointer in the handle becomes invalid as soon as the
> target_ops object is closed/destroyed/released (unless the target
> object is refcounted, which isn't clear it should).
> 
> A table-based solution, where target.c maintains (something
> conceptually like) a table of "target file descriptor + target_ops"
> elements, and passes an index into that table as file handle to
> target_fileio_ callers, would make it possible to invalidate the
> file handles directly in the table when a target is closed, so
> that e.g., target_fileio_close (etc.) could detect that the file
> handle is now invalid, and return error with errno=EBADF _without_
> calling through the now invalid target_ops pointer.

The files could be closed when the table entry is invalidated too.
I'll reimplement this and post a new patch tomorrow or Friday.

> Speaking of EBADF, I noticed that remote_hostio_send_command
> fails with ENOSYS if the remote connection is closed, which
> seems wrong to me.

That seems intuitive for the ones that take file descriptor
arguments (remote_hostio_{pread,write,fstat,close}).  Not sure
if that makes sense for the ones that take filenames (open,
readlink, unlink).  ENOSYS isn't really correct for those
either--they are implemented, you just can't do it right now.
ECOMM?  ECONNABORTED?  ENOTCONN?  EPIPE?  EREMOTEIO?
I'm open to suggestions :)

Cheers,
Gary

-- 
http://gbenson.net/