[PATCH] Encapsulate target_fileio file descriptors

[PATCH] Encapsulate target_fileio file descriptors

[Gary Benson]

Hi all,

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.

Built and regtested on RHEL 6.6 x86_64.

Ok to commit?

Thanks,
Gary

---
gdb/ChangeLog:

	* target.h (target_fileio_t): New typedef.
	(target_fileio_open): Changed signature, updated comment.
	(target_fileio_pread): Likewise.
	(target_fileio_pwrite): Likewise.
	(target_fileio_close): Likewise.
	* target.c (target_fileio_handle): New structure.
	(target_fileio_open): Updated to return a target_fileio_t
	instead of an integer file descriptor.  Updated targetdebug
	message to include the target shortname.  Updated comment.
	(target_fileio_pread): Take a target_fileio_t handle as the
	first argument instead of an integer file descriptor.  Updated
	targetdebug message to include the target shortname.  Updated
	comment.
	(target_fileio_pwrite): Likewise.
	(target_fileio_close): Likewise.
	(target_fileio_unlink): Updated comment.
	(target_fileio_readlink): Likewise.
	(target_fileio_close_cleanup): Take a target_fileio_t handle
	as the first argument instead of a pointer to an integer file
	descriptor.  Updated caller.
	(target_fileio_read_alloc_1): Use a target_fileio_t handle
	instead of an integer file descriptor.
---
 gdb/ChangeLog |   25 +++++++++
 gdb/target.c  |  164 ++++++++++++++++++++++++++------------------------------
 gdb/target.h  |   28 ++++++----
 3 files changed, 118 insertions(+), 99 deletions(-)

diff --git a/gdb/target.c b/gdb/target.c
index af94f48..7e04679 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -2684,10 +2684,20 @@ default_fileio_target (void)
     return find_default_run_target ("file I/O");
 }
 
-/* Open FILENAME on the target, using FLAGS and MODE.  Return a
-   target file descriptor, or -1 if an error occurs (and set
-   *TARGET_ERRNO).  */
-int
+/* File handle for target file operations.  */
+
+struct target_fileio_handle
+{
+  /* The target on which this file is open.  */
+  struct target_ops *t;
+
+  /* The file's descriptor on the target.  */
+  int fd;
+};
+
+/* See target.h.  */
+
+target_fileio_t
 target_fileio_open (const char *filename, int flags, int mode,
 		    int *target_errno)
 {
@@ -2698,107 +2708,86 @@ target_fileio_open (const char *filename, int flags, int mode,
       if (t->to_fileio_open != NULL)
 	{
 	  int fd = t->to_fileio_open (t, filename, flags, mode, target_errno);
+	  target_fileio_t fh = NULL;
 
 	  if (targetdebug)
 	    fprintf_unfiltered (gdb_stdlog,
-				"target_fileio_open (%s,0x%x,0%o) = %d (%d)\n",
-				filename, flags, mode,
+				"target_fileio_open (%s,0x%x,0%o)"
+				" = %s:%d (%d)\n",
+				filename, flags, mode, t->to_shortname,
 				fd, fd != -1 ? 0 : *target_errno);
-	  return fd;
+	  if (fd >= 0)
+	    {
+	      fh = XCNEW (struct target_fileio_handle);
+	      fh->t = t;
+	      fh->fd = fd;
+	    }
+
+	  return fh;
 	}
     }
 
   *target_errno = FILEIO_ENOSYS;
-  return -1;
+  return NULL;
 }
 
-/* Write up to LEN bytes from WRITE_BUF to FD on the target.
-   Return the number of bytes written, or -1 if an error occurs
-   (and set *TARGET_ERRNO).  */
+/* See target.h.  */
+
 int
-target_fileio_pwrite (int fd, const gdb_byte *write_buf, int len,
-		      ULONGEST offset, int *target_errno)
+target_fileio_pwrite (target_fileio_t fh, const gdb_byte *write_buf,
+		      int len, ULONGEST offset, int *target_errno)
 {
-  struct target_ops *t;
+  int ret = fh->t->to_fileio_pwrite (fh->t, fh->fd, write_buf, len,
+				     offset, target_errno);
 
-  for (t = default_fileio_target (); t != NULL; t = t->beneath)
-    {
-      if (t->to_fileio_pwrite != NULL)
-	{
-	  int ret = t->to_fileio_pwrite (t, fd, write_buf, len, offset,
-					 target_errno);
-
-	  if (targetdebug)
-	    fprintf_unfiltered (gdb_stdlog,
-				"target_fileio_pwrite (%d,...,%d,%s) "
-				"= %d (%d)\n",
-				fd, len, pulongest (offset),
-				ret, ret != -1 ? 0 : *target_errno);
-	  return ret;
-	}
-    }
-
-  *target_errno = FILEIO_ENOSYS;
-  return -1;
+  if (targetdebug)
+    fprintf_unfiltered (gdb_stdlog,
+			"target_fileio_pwrite (%s:%d,...,%d,%s)"
+			" = %d (%d)\n",
+			fh->t->to_shortname, fh->fd, len,
+			pulongest (offset), ret,
+			ret != -1 ? 0 : *target_errno);
+  return ret;
 }
 
-/* Read up to LEN bytes FD on the target into READ_BUF.
-   Return the number of bytes read, or -1 if an error occurs
-   (and set *TARGET_ERRNO).  */
+/* See target.h.  */
+
 int
-target_fileio_pread (int fd, gdb_byte *read_buf, int len,
+target_fileio_pread (target_fileio_t fh, gdb_byte *read_buf, int len,
 		     ULONGEST offset, int *target_errno)
 {
-  struct target_ops *t;
+  int ret = fh->t->to_fileio_pread (fh->t, fh->fd, read_buf, len,
+				    offset, target_errno);
 
-  for (t = default_fileio_target (); t != NULL; t = t->beneath)
-    {
-      if (t->to_fileio_pread != NULL)
-	{
-	  int ret = t->to_fileio_pread (t, fd, read_buf, len, offset,
-					target_errno);
-
-	  if (targetdebug)
-	    fprintf_unfiltered (gdb_stdlog,
-				"target_fileio_pread (%d,...,%d,%s) "
-				"= %d (%d)\n",
-				fd, len, pulongest (offset),
-				ret, ret != -1 ? 0 : *target_errno);
-	  return ret;
-	}
-    }
-
-  *target_errno = FILEIO_ENOSYS;
-  return -1;
+  if (targetdebug)
+    fprintf_unfiltered (gdb_stdlog,
+			"target_fileio_pread (%s:%d,...,%d,%s)"
+			" = %d (%d)\n",
+			fh->t->to_shortname, fh->fd, len,
+			pulongest (offset), ret,
+			ret != -1 ? 0 : *target_errno);
+  return ret;
 }
 
-/* Close FD on the target.  Return 0, or -1 if an error occurs
-   (and set *TARGET_ERRNO).  */
+/* See target.h.  */
+
 int
-target_fileio_close (int fd, int *target_errno)
+target_fileio_close (target_fileio_t fh, int *target_errno)
 {
-  struct target_ops *t;
+  int ret = fh->t->to_fileio_close (fh->t, fh->fd, target_errno);
 
-  for (t = default_fileio_target (); t != NULL; t = t->beneath)
-    {
-      if (t->to_fileio_close != NULL)
-	{
-	  int ret = t->to_fileio_close (t, fd, target_errno);
-
-	  if (targetdebug)
-	    fprintf_unfiltered (gdb_stdlog,
-				"target_fileio_close (%d) = %d (%d)\n",
-				fd, ret, ret != -1 ? 0 : *target_errno);
-	  return ret;
-	}
-    }
+  if (targetdebug)
+    fprintf_unfiltered (gdb_stdlog,
+			"target_fileio_close (%s:%d) = %d (%d)\n",
+			fh->t->to_shortname, fh->fd, ret,
+			ret != -1 ? 0 : *target_errno);
+  xfree (fh);
 
-  *target_errno = FILEIO_ENOSYS;
-  return -1;
+  return ret;
 }
 
-/* Unlink FILENAME on the target.  Return 0, or -1 if an error
-   occurs (and set *TARGET_ERRNO).  */
+/* See target.h.  */
+
 int
 target_fileio_unlink (const char *filename, int *target_errno)
 {
@@ -2822,9 +2811,8 @@ target_fileio_unlink (const char *filename, int *target_errno)
   return -1;
 }
 
-/* Read value of symbolic link FILENAME on the target.  Return a
-   null-terminated string allocated via xmalloc, or NULL if an error
-   occurs (and set *TARGET_ERRNO).  */
+/* See target.h.  */
+
 char *
 target_fileio_readlink (const char *filename, int *target_errno)
 {
@@ -2852,10 +2840,10 @@ target_fileio_readlink (const char *filename, int *target_errno)
 static void
 target_fileio_close_cleanup (void *opaque)
 {
-  int fd = *(int *) opaque;
+  target_fileio_t fh = (target_fileio_t) opaque;
   int target_errno;
 
-  target_fileio_close (fd, &target_errno);
+  target_fileio_close (fh, &target_errno);
 }
 
 /* Read target file FILENAME.  Store the result in *BUF_P and
@@ -2872,14 +2860,14 @@ target_fileio_read_alloc_1 (const char *filename,
   size_t buf_alloc, buf_pos;
   gdb_byte *buf;
   LONGEST n;
-  int fd;
+  target_fileio_t fh;
   int target_errno;
 
-  fd = target_fileio_open (filename, FILEIO_O_RDONLY, 0700, &target_errno);
-  if (fd == -1)
+  fh = target_fileio_open (filename, FILEIO_O_RDONLY, 0700, &target_errno);
+  if (fh == NULL)
     return -1;
 
-  close_cleanup = make_cleanup (target_fileio_close_cleanup, &fd);
+  close_cleanup = make_cleanup (target_fileio_close_cleanup, fh);
 
   /* Start by reading up to 4K at a time.  The target will throttle
      this number down if necessary.  */
@@ -2888,7 +2876,7 @@ target_fileio_read_alloc_1 (const char *filename,
   buf_pos = 0;
   while (1)
     {
-      n = target_fileio_pread (fd, &buf[buf_pos],
+      n = target_fileio_pread (fh, &buf[buf_pos],
 			       buf_alloc - buf_pos - padding, buf_pos,
 			       &target_errno);
       if (n < 0)
diff --git a/gdb/target.h b/gdb/target.h
index c95e1a4..1edee84 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -1915,27 +1915,33 @@ extern int target_search_memory (CORE_ADDR start_addr,
 
 /* Target file operations.  */
 
-/* Open FILENAME on the target, using FLAGS and MODE.  Return a
-   target file descriptor, or -1 if an error occurs (and set
-   *TARGET_ERRNO).  */
-extern int target_fileio_open (const char *filename, int flags, int mode,
-			       int *target_errno);
+/* File handle for target file operations.  */
+typedef struct target_fileio_handle *target_fileio_t;
 
-/* Write up to LEN bytes from WRITE_BUF to FD on the target.
+/* Open FILENAME on the target, using FLAGS and MODE.  Return a target
+   file handle, or NULL if an error occurs (and set *TARGET_ERRNO).  */
+extern target_fileio_t target_fileio_open (const char *filename,
+					   int flags, int mode,
+					   int *target_errno);
+
+/* Write up to LEN bytes from WRITE_BUF to FH on the target.
    Return the number of bytes written, or -1 if an error occurs
    (and set *TARGET_ERRNO).  */
-extern int target_fileio_pwrite (int fd, const gdb_byte *write_buf, int len,
+extern int target_fileio_pwrite (target_fileio_t fh,
+				 const gdb_byte *write_buf, int len,
 				 ULONGEST offset, int *target_errno);
 
-/* Read up to LEN bytes FD on the target into READ_BUF.
+/* Read up to LEN bytes from FH on the target into READ_BUF.
    Return the number of bytes read, or -1 if an error occurs
    (and set *TARGET_ERRNO).  */
-extern int target_fileio_pread (int fd, gdb_byte *read_buf, int len,
+extern int target_fileio_pread (target_fileio_t fh,
+				gdb_byte *read_buf, int len,
 				ULONGEST offset, int *target_errno);
 
-/* Close FD on the target.  Return 0, or -1 if an error occurs
+/* Close FH on the target.  Return 0, or -1 if an error occurs
    (and set *TARGET_ERRNO).  */
-extern int target_fileio_close (int fd, int *target_errno);
+extern int target_fileio_close (target_fileio_t fh,
+				int *target_errno);
 
 /* Unlink FILENAME on the target.  Return 0, or -1 if an error
    occurs (and set *TARGET_ERRNO).  */
-- 
1.7.1

Re: [PATCH] Encapsulate target_fileio file descriptors

[Pedro Alves]

On 03/18/2015 01:20 PM, Gary Benson wrote:
> Hi all,
> 
> 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.

Making the handle be something other than an integer itself
doesn't actually fix things -- e.g., if the patch only added
the target_fileio_t as a wrapper struct _without_ the target_ops
pointer.

The main issue is this :

/* Close FD on the target.  Return 0, or -1 if an error occurs
   (and set *TARGET_ERRNO).  */
int
target_fileio_close (int fd, int *target_errno)
{
  struct target_ops *t;

  for (t = default_fileio_target (); t != NULL; t = t->beneath)
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    {
      if (t->to_fileio_close != NULL)
	{
	  int ret = t->to_fileio_close (t, fd, target_errno);

	  if (targetdebug)
	    fprintf_unfiltered (gdb_stdlog,
				"target_fileio_close (%d) = %d (%d)\n",
				fd, ret, ret != -1 ? 0 : *target_errno);
	  return ret;
	}
    }

  *target_errno = FILEIO_ENOSYS;
  return -1;
}



Say that the file is originally opened with the remote target.  Then,
something goes wrong while the file is open, and the target
disconnects/closes, which pops the remote target off the stack.
Then, when later something tries to close the original
fileio file (e.g., target_fileio_close_cleanup, from
target_fileio_read_alloc_1), default_fileio_target is called to
find the target to apply the target method to.  This does:

static struct target_ops *
default_fileio_target (void)
{
  /* If we're already connected to something that can perform
     file I/O, use it. Otherwise, try using the native target.  */
  if (current_target.to_stratum >= process_stratum)
    return current_target.beneath;
  else
    return find_default_run_target ("file I/O");
}

Since the remote target is no longer in the target stack, the current
target won't have process_stratum stratum, and we'll reach
find_default_run_target.  And that, unless "set auto-connect-native-target"
is "off" (which is "on" by default), will return a pointer to
the native target.

The end result is that although the file descriptor only made
sense for the remote target, we call "t->to_fileio_close" in the
native target, which ends up in:

/* Close FD on the target.  Return 0, or -1 if an error occurs
   (and set *TARGET_ERRNO).  */
static int
inf_child_fileio_close (struct target_ops *self, int fd, int *target_errno)
{
  int ret;

  ret = close (fd);
        ^^^^^^^^^^

  if (ret == -1)
    *target_errno = inf_child_errno_to_fileio_error (errno);

  return ret;
}

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!

The fix for this is to always operate on the file
descriptor using the target that originally created it
in the first place.

> 
> Built and regtested on RHEL 6.6 x86_64.
> 
> Ok to commit?

Note that this solution of embedding the target_ops pointer in
the handle itself means that the targets themselves need to
be prepared to be called when they're already closed.

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.

We can cross that bridge when we get to it, but I thought I
should point it it out.

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

> gdb/ChangeLog:
> 
> 	* target.h (target_fileio_t): New typedef.

"target_fileio_t" doesn't really suggest to me that this is a
descriptor or a handle.  How about "target_fhandle_t" or "target_fildes_t"?

> 	(target_fileio_open): Changed signature, updated comment.

No past tense: "change", "update", throughout.

> 	(target_fileio_pread): Likewise.
> 	(target_fileio_pwrite): Likewise.
> 	(target_fileio_close): Likewise.
> 	* target.c (target_fileio_handle): New structure.
> 	(target_fileio_open): Updated to return a target_fileio_t
> 	instead of an integer file descriptor.  Updated targetdebug
> 	message to include the target shortname.  Updated comment.
> 	(target_fileio_pread): Take a target_fileio_t handle as the
> 	first argument instead of an integer file descriptor.  Updated
> 	targetdebug message to include the target shortname.  Updated
> 	comment.
> 	(target_fileio_pwrite): Likewise.
> 	(target_fileio_close): Likewise.
> 	(target_fileio_unlink): Updated comment.
> 	(target_fileio_readlink): Likewise.
> 	(target_fileio_close_cleanup): Take a target_fileio_t handle
> 	as the first argument instead of a pointer to an integer file
> 	descriptor.  Updated caller.
> 	(target_fileio_read_alloc_1): Use a target_fileio_t handle
> 	instead of an integer file descriptor.
> ---
>  gdb/ChangeLog |   25 +++++++++
>  gdb/target.c  |  164 ++++++++++++++++++++++++++------------------------------
>  gdb/target.h  |   28 ++++++----
>  3 files changed, 118 insertions(+), 99 deletions(-)
> 
> diff --git a/gdb/target.c b/gdb/target.c
> index af94f48..7e04679 100644
> --- a/gdb/target.c
> +++ b/gdb/target.c
> @@ -2684,10 +2684,20 @@ default_fileio_target (void)
>      return find_default_run_target ("file I/O");
>  }
>  
> -/* Open FILENAME on the target, using FLAGS and MODE.  Return a
> -   target file descriptor, or -1 if an error occurs (and set
> -   *TARGET_ERRNO).  */
> -int
> +/* File handle for target file operations.  */
> +
> +struct target_fileio_handle
> +{
> +  /* The target on which this file is open.  */
> +  struct target_ops *t;
> +
> +  /* The file's descriptor on the target.  */
> +  int fd;
> +};
> +
> +/* See target.h.  */
> +
> +target_fileio_t
>  target_fileio_open (const char *filename, int flags, int mode,
>  		    int *target_errno)
>  {
> @@ -2698,107 +2708,86 @@ target_fileio_open (const char *filename, int flags, int mode,
>        if (t->to_fileio_open != NULL)
>  	{
>  	  int fd = t->to_fileio_open (t, filename, flags, mode, target_errno);
> +	  target_fileio_t fh = NULL;
>  
>  	  if (targetdebug)
>  	    fprintf_unfiltered (gdb_stdlog,
> -				"target_fileio_open (%s,0x%x,0%o) = %d (%d)\n",
> -				filename, flags, mode,
> +				"target_fileio_open (%s,0x%x,0%o)"
> +				" = %s:%d (%d)\n",
> +				filename, flags, mode, t->to_shortname,
>  				fd, fd != -1 ? 0 : *target_errno);
> -	  return fd;
> +	  if (fd >= 0)
> +	    {
> +	      fh = XCNEW (struct target_fileio_handle);
> +	      fh->t = t;
> +	      fh->fd = fd;
> +	    }
> +
> +	  return fh;
>  	}
>      }
>  
>    *target_errno = FILEIO_ENOSYS;
> -  return -1;
> +  return NULL;
>  }
>  
> -/* Write up to LEN bytes from WRITE_BUF to FD on the target.
> -   Return the number of bytes written, or -1 if an error occurs
> -   (and set *TARGET_ERRNO).  */
> +/* See target.h.  */
> +
>  int
> -target_fileio_pwrite (int fd, const gdb_byte *write_buf, int len,
> -		      ULONGEST offset, int *target_errno)
> +target_fileio_pwrite (target_fileio_t fh, const gdb_byte *write_buf,
> +		      int len, ULONGEST offset, int *target_errno)
>  {
> -  struct target_ops *t;
> +  int ret = fh->t->to_fileio_pwrite (fh->t, fh->fd, write_buf, len,
> +				     offset, target_errno);
>  
> -  for (t = default_fileio_target (); t != NULL; t = t->beneath)
> -    {
> -      if (t->to_fileio_pwrite != NULL)
> -	{
> -	  int ret = t->to_fileio_pwrite (t, fd, write_buf, len, offset,
> -					 target_errno);
> -
> -	  if (targetdebug)
> -	    fprintf_unfiltered (gdb_stdlog,
> -				"target_fileio_pwrite (%d,...,%d,%s) "
> -				"= %d (%d)\n",
> -				fd, len, pulongest (offset),
> -				ret, ret != -1 ? 0 : *target_errno);
> -	  return ret;
> -	}
> -    }
> -
> -  *target_errno = FILEIO_ENOSYS;
> -  return -1;
> +  if (targetdebug)
> +    fprintf_unfiltered (gdb_stdlog,
> +			"target_fileio_pwrite (%s:%d,...,%d,%s)"
> +			" = %d (%d)\n",
> +			fh->t->to_shortname, fh->fd, len,
> +			pulongest (offset), ret,
> +			ret != -1 ? 0 : *target_errno);
> +  return ret;
>  }
>  
> -/* Read up to LEN bytes FD on the target into READ_BUF.
> -   Return the number of bytes read, or -1 if an error occurs
> -   (and set *TARGET_ERRNO).  */
> +/* See target.h.  */
> +
>  int
> -target_fileio_pread (int fd, gdb_byte *read_buf, int len,
> +target_fileio_pread (target_fileio_t fh, gdb_byte *read_buf, int len,
>  		     ULONGEST offset, int *target_errno)
>  {
> -  struct target_ops *t;
> +  int ret = fh->t->to_fileio_pread (fh->t, fh->fd, read_buf, len,
> +				    offset, target_errno);
>  
> -  for (t = default_fileio_target (); t != NULL; t = t->beneath)
> -    {
> -      if (t->to_fileio_pread != NULL)
> -	{
> -	  int ret = t->to_fileio_pread (t, fd, read_buf, len, offset,
> -					target_errno);
> -
> -	  if (targetdebug)
> -	    fprintf_unfiltered (gdb_stdlog,
> -				"target_fileio_pread (%d,...,%d,%s) "
> -				"= %d (%d)\n",
> -				fd, len, pulongest (offset),
> -				ret, ret != -1 ? 0 : *target_errno);
> -	  return ret;
> -	}
> -    }
> -
> -  *target_errno = FILEIO_ENOSYS;
> -  return -1;
> +  if (targetdebug)
> +    fprintf_unfiltered (gdb_stdlog,
> +			"target_fileio_pread (%s:%d,...,%d,%s)"
> +			" = %d (%d)\n",
> +			fh->t->to_shortname, fh->fd, len,
> +			pulongest (offset), ret,
> +			ret != -1 ? 0 : *target_errno);
> +  return ret;
>  }
>  
> -/* Close FD on the target.  Return 0, or -1 if an error occurs
> -   (and set *TARGET_ERRNO).  */
> +/* See target.h.  */
> +
>  int
> -target_fileio_close (int fd, int *target_errno)
> +target_fileio_close (target_fileio_t fh, int *target_errno)
>  {
> -  struct target_ops *t;
> +  int ret = fh->t->to_fileio_close (fh->t, fh->fd, target_errno);
>  
> -  for (t = default_fileio_target (); t != NULL; t = t->beneath)
> -    {
> -      if (t->to_fileio_close != NULL)
> -	{
> -	  int ret = t->to_fileio_close (t, fd, target_errno);
> -
> -	  if (targetdebug)
> -	    fprintf_unfiltered (gdb_stdlog,
> -				"target_fileio_close (%d) = %d (%d)\n",
> -				fd, ret, ret != -1 ? 0 : *target_errno);
> -	  return ret;
> -	}
> -    }
> +  if (targetdebug)
> +    fprintf_unfiltered (gdb_stdlog,
> +			"target_fileio_close (%s:%d) = %d (%d)\n",
> +			fh->t->to_shortname, fh->fd, ret,
> +			ret != -1 ? 0 : *target_errno);
> +  xfree (fh);
>  
> -  *target_errno = FILEIO_ENOSYS;
> -  return -1;
> +  return ret;
>  }
>  
> -/* Unlink FILENAME on the target.  Return 0, or -1 if an error
> -   occurs (and set *TARGET_ERRNO).  */
> +/* See target.h.  */
> +
>  int
>  target_fileio_unlink (const char *filename, int *target_errno)
>  {
> @@ -2822,9 +2811,8 @@ target_fileio_unlink (const char *filename, int *target_errno)
>    return -1;
>  }
>  
> -/* Read value of symbolic link FILENAME on the target.  Return a
> -   null-terminated string allocated via xmalloc, or NULL if an error
> -   occurs (and set *TARGET_ERRNO).  */
> +/* See target.h.  */
> +
>  char *
>  target_fileio_readlink (const char *filename, int *target_errno)
>  {
> @@ -2852,10 +2840,10 @@ target_fileio_readlink (const char *filename, int *target_errno)
>  static void
>  target_fileio_close_cleanup (void *opaque)
>  {
> -  int fd = *(int *) opaque;
> +  target_fileio_t fh = (target_fileio_t) opaque;
>    int target_errno;
>  
> -  target_fileio_close (fd, &target_errno);
> +  target_fileio_close (fh, &target_errno);
>  }
>  
>  /* Read target file FILENAME.  Store the result in *BUF_P and
> @@ -2872,14 +2860,14 @@ target_fileio_read_alloc_1 (const char *filename,
>    size_t buf_alloc, buf_pos;
>    gdb_byte *buf;
>    LONGEST n;
> -  int fd;
> +  target_fileio_t fh;
>    int target_errno;
>  
> -  fd = target_fileio_open (filename, FILEIO_O_RDONLY, 0700, &target_errno);
> -  if (fd == -1)
> +  fh = target_fileio_open (filename, FILEIO_O_RDONLY, 0700, &target_errno);
> +  if (fh == NULL)
>      return -1;
>  
> -  close_cleanup = make_cleanup (target_fileio_close_cleanup, &fd);
> +  close_cleanup = make_cleanup (target_fileio_close_cleanup, fh);
>  
>    /* Start by reading up to 4K at a time.  The target will throttle
>       this number down if necessary.  */
> @@ -2888,7 +2876,7 @@ target_fileio_read_alloc_1 (const char *filename,
>    buf_pos = 0;
>    while (1)
>      {
> -      n = target_fileio_pread (fd, &buf[buf_pos],
> +      n = target_fileio_pread (fh, &buf[buf_pos],
>  			       buf_alloc - buf_pos - padding, buf_pos,
>  			       &target_errno);
>        if (n < 0)
> diff --git a/gdb/target.h b/gdb/target.h
> index c95e1a4..1edee84 100644
> --- a/gdb/target.h
> +++ b/gdb/target.h
> @@ -1915,27 +1915,33 @@ extern int target_search_memory (CORE_ADDR start_addr,
>  
>  /* Target file operations.  */
>  
> -/* Open FILENAME on the target, using FLAGS and MODE.  Return a
> -   target file descriptor, or -1 if an error occurs (and set
> -   *TARGET_ERRNO).  */
> -extern int target_fileio_open (const char *filename, int flags, int mode,
> -			       int *target_errno);
> +/* File handle for target file operations.  */
> +typedef struct target_fileio_handle *target_fileio_t;
>  
> -/* Write up to LEN bytes from WRITE_BUF to FD on the target.
> +/* Open FILENAME on the target, using FLAGS and MODE.  Return a target
> +   file handle, or NULL if an error occurs (and set *TARGET_ERRNO).  */
> +extern target_fileio_t target_fileio_open (const char *filename,
> +					   int flags, int mode,
> +					   int *target_errno);
> +
> +/* Write up to LEN bytes from WRITE_BUF to FH on the target.
>     Return the number of bytes written, or -1 if an error occurs
>     (and set *TARGET_ERRNO).  */
> -extern int target_fileio_pwrite (int fd, const gdb_byte *write_buf, int len,
> +extern int target_fileio_pwrite (target_fileio_t fh,
> +				 const gdb_byte *write_buf, int len,
>  				 ULONGEST offset, int *target_errno);
>  
> -/* Read up to LEN bytes FD on the target into READ_BUF.
> +/* Read up to LEN bytes from FH on the target into READ_BUF.
>     Return the number of bytes read, or -1 if an error occurs
>     (and set *TARGET_ERRNO).  */
> -extern int target_fileio_pread (int fd, gdb_byte *read_buf, int len,
> +extern int target_fileio_pread (target_fileio_t fh,
> +				gdb_byte *read_buf, int len,
>  				ULONGEST offset, int *target_errno);
>  
> -/* Close FD on the target.  Return 0, or -1 if an error occurs
> +/* Close FH on the target.  Return 0, or -1 if an error occurs
>     (and set *TARGET_ERRNO).  */
> -extern int target_fileio_close (int fd, int *target_errno);
> +extern int target_fileio_close (target_fileio_t fh,
> +				int *target_errno);
>  
>  /* Unlink FILENAME on the target.  Return 0, or -1 if an error
>     occurs (and set *TARGET_ERRNO).  */
> 


Thanks,
Pedro Alves

Re: [PATCH] Encapsulate target_fileio file descriptors

[Gary Benson]

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/

[PATCH v2] Associate target_ops with target_fileio file descriptors

[Gary Benson]

Hi all,

Various target_fileio_* functions use integer file descriptors to
refer to open files.  File operation functions are looked up from
the target stack as they are used, which causes problems if the
target stack changes after the file is opened.

For example, if a file is opened on a remote target and the remote
target disconnects or closes the remote target will be popped off
the stack.  If target_fileio_close is then called on that file and
"set auto-connect-native-target" is "on" (the default) then the
native target's close method will be called.  If the file opened
on the remote happens to share the same number with a file open in
GDB then that file will be closed by mistake.

This commit changes target_fileio_open to store newly opened file
descriptors in a table together with the target_ops used to open
them.  The index into the table is returned and used as the file
descriptor argument to all target_fileio_* functions that accept
file descriptor arguments.

Built and regtested on RHEL 6.6 x86_64.

Ok to commit?

Thanks,
Gary

---
gdb/ChangeLog:

	* target.c (fileio_ft_t): New typedef, define object vector.
	(fileio_fhandles): New static variable.
	(is_closed_fileio_fh): New macro.
	(lowest_closed_fd): New static variable.
	(acquire_fileio_fd): New function.
	(release_fileio_fd): Likewise.
	(fileio_fd_to_fh): New macro.
	(target_fileio_open): Wrap the file descriptor on success.
	(target_fileio_pwrite): Updated to use wrapped file descriptor.
	(target_fileio_pread): Likewise.
	(target_fileio_close): Likewise.
---
 gdb/ChangeLog |   14 +++++
 gdb/target.c  |  178 ++++++++++++++++++++++++++++++++++++++++-----------------
 2 files changed, 140 insertions(+), 52 deletions(-)

diff --git a/gdb/target.c b/gdb/target.c
index af94f48..bb901b5 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -2684,6 +2684,87 @@ default_fileio_target (void)
     return find_default_run_target ("file I/O");
 }
 
+/* File handle for target file operations.  */
+
+typedef struct
+{
+  /* The target on which this file is open.  */
+  struct target_ops *t;
+
+  /* The file descriptor on the target.  */
+  int fd;
+} fileio_fh_t;
+
+DEF_VEC_O (fileio_fh_t);
+
+/* Vector of currently open file handles.  The value returned by
+   target_fileio_open and passed as the FD argument to other
+   target_fileio_* functions is an index into this vector.  This
+   vector's entries are never freed; instead, files are marked as
+   closed, and the handle becomes available for reuse.  */
+static VEC (fileio_fh_t) *fileio_fhandles;
+
+/* Macro to check whether a fileio_fh_t represents a closed file.  */
+#define is_closed_fileio_fh(fd) ((fd) < 0)
+
+/* Index into fileio_fhandles of the lowest handle that might
+   be closed, or -1 if no closed files currently exist.  This
+   permits handle reuse without searching the whole list each
+   time a new file is opened.  */
+static int lowest_closed_fd = -1;
+
+/* Acquire a target fileio file descriptor.  */
+
+static int
+acquire_fileio_fd (struct target_ops *t, int fd)
+{
+  int total = VEC_length (fileio_fh_t, fileio_fhandles);
+  int index = total;
+  fileio_fh_t *fh, buf;
+
+  gdb_assert (!is_closed_fileio_fh (fd));
+
+  /* Search for closed handles to reuse.  */
+  if (lowest_closed_fd >= 0)
+    {
+      for (index = lowest_closed_fd;
+	   VEC_iterate (fileio_fh_t, fileio_fhandles, index, fh);
+	   index++)
+	if (is_closed_fileio_fh (fh->fd))
+	  break;
+    }
+
+  /* Push a new handle if no closed handles were found.  */
+  if (index == total)
+    {
+      fh = VEC_safe_push (fileio_fh_t, fileio_fhandles, NULL);
+      lowest_closed_fd = -1;
+    }
+
+  /* Fill in the handle and return its index.  */
+  fh->t = t;
+  fh->fd = fd;
+
+  return index;
+}
+
+/* Release a target fileio file descriptor.  */
+
+static void
+release_fileio_fd (int fd, fileio_fh_t *fh)
+{
+  fh->fd = -1;
+  if (lowest_closed_fd == -1)
+    lowest_closed_fd = fd;
+  else
+    lowest_closed_fd = min (lowest_closed_fd, fd);
+}
+
+/* Return a pointer to the fileio_fhandle_t corresponding to FD.  */
+
+#define fileio_fd_to_fh(fd) \
+  VEC_index (fileio_fh_t, fileio_fhandles, (fd))
+
 /* Open FILENAME on the target, using FLAGS and MODE.  Return a
    target file descriptor, or -1 if an error occurs (and set
    *TARGET_ERRNO).  */
@@ -2699,6 +2780,11 @@ target_fileio_open (const char *filename, int flags, int mode,
 	{
 	  int fd = t->to_fileio_open (t, filename, flags, mode, target_errno);
 
+	  if (fd < 0)
+	    fd = -1;
+	  else
+	    fd = acquire_fileio_fd (t, fd);
+
 	  if (targetdebug)
 	    fprintf_unfiltered (gdb_stdlog,
 				"target_fileio_open (%s,0x%x,0%o) = %d (%d)\n",
@@ -2719,27 +2805,22 @@ int
 target_fileio_pwrite (int fd, const gdb_byte *write_buf, int len,
 		      ULONGEST offset, int *target_errno)
 {
-  struct target_ops *t;
-
-  for (t = default_fileio_target (); t != NULL; t = t->beneath)
-    {
-      if (t->to_fileio_pwrite != NULL)
-	{
-	  int ret = t->to_fileio_pwrite (t, fd, write_buf, len, offset,
-					 target_errno);
+  fileio_fh_t *fh = fileio_fd_to_fh (fd);
+  int ret = -1;
 
-	  if (targetdebug)
-	    fprintf_unfiltered (gdb_stdlog,
-				"target_fileio_pwrite (%d,...,%d,%s) "
-				"= %d (%d)\n",
-				fd, len, pulongest (offset),
-				ret, ret != -1 ? 0 : *target_errno);
-	  return ret;
-	}
-    }
+  if (is_closed_fileio_fh (fh->fd))
+    *target_errno = EBADF;
+  else
+    ret = fh->t->to_fileio_pwrite (fh->t, fh->fd, write_buf,
+				   len, offset, target_errno);
 
-  *target_errno = FILEIO_ENOSYS;
-  return -1;
+  if (targetdebug)
+    fprintf_unfiltered (gdb_stdlog,
+			"target_fileio_pwrite (%d,...,%d,%s) "
+			"= %d (%d)\n",
+			fd, len, pulongest (offset),
+			ret, ret != -1 ? 0 : *target_errno);
+  return ret;
 }
 
 /* Read up to LEN bytes FD on the target into READ_BUF.
@@ -2749,27 +2830,22 @@ int
 target_fileio_pread (int fd, gdb_byte *read_buf, int len,
 		     ULONGEST offset, int *target_errno)
 {
-  struct target_ops *t;
-
-  for (t = default_fileio_target (); t != NULL; t = t->beneath)
-    {
-      if (t->to_fileio_pread != NULL)
-	{
-	  int ret = t->to_fileio_pread (t, fd, read_buf, len, offset,
-					target_errno);
+  fileio_fh_t *fh = fileio_fd_to_fh (fd);
+  int ret = -1;
 
-	  if (targetdebug)
-	    fprintf_unfiltered (gdb_stdlog,
-				"target_fileio_pread (%d,...,%d,%s) "
-				"= %d (%d)\n",
-				fd, len, pulongest (offset),
-				ret, ret != -1 ? 0 : *target_errno);
-	  return ret;
-	}
-    }
+  if (is_closed_fileio_fh (fh->fd))
+    *target_errno = EBADF;
+  else
+    ret = fh->t->to_fileio_pread (fh->t, fh->fd, read_buf,
+				  len, offset, target_errno);
 
-  *target_errno = FILEIO_ENOSYS;
-  return -1;
+  if (targetdebug)
+    fprintf_unfiltered (gdb_stdlog,
+			"target_fileio_pread (%d,...,%d,%s) "
+			"= %d (%d)\n",
+			fd, len, pulongest (offset),
+			ret, ret != -1 ? 0 : *target_errno);
+  return ret;
 }
 
 /* Close FD on the target.  Return 0, or -1 if an error occurs
@@ -2777,24 +2853,22 @@ target_fileio_pread (int fd, gdb_byte *read_buf, int len,
 int
 target_fileio_close (int fd, int *target_errno)
 {
-  struct target_ops *t;
+  fileio_fh_t *fh = fileio_fd_to_fh (fd);
+  int ret = -1;
 
-  for (t = default_fileio_target (); t != NULL; t = t->beneath)
+  if (is_closed_fileio_fh (fh->fd))
+    *target_errno = EBADF;
+  else
     {
-      if (t->to_fileio_close != NULL)
-	{
-	  int ret = t->to_fileio_close (t, fd, target_errno);
-
-	  if (targetdebug)
-	    fprintf_unfiltered (gdb_stdlog,
-				"target_fileio_close (%d) = %d (%d)\n",
-				fd, ret, ret != -1 ? 0 : *target_errno);
-	  return ret;
-	}
+      ret = fh->t->to_fileio_close (fh->t, fh->fd, target_errno);
+      release_fileio_fd (fd, fh);
     }
 
-  *target_errno = FILEIO_ENOSYS;
-  return -1;
+  if (targetdebug)
+    fprintf_unfiltered (gdb_stdlog,
+			"target_fileio_close (%d) = %d (%d)\n",
+			fd, ret, ret != -1 ? 0 : *target_errno);
+  return ret;
 }
 
 /* Unlink FILENAME on the target.  Return 0, or -1 if an error
-- 
1.7.1

Re: [PATCH v2] Associate target_ops with target_fileio file descriptors

[Pedro Alves]

On 03/20/2015 04:06 PM, Gary Benson wrote:

> +
> +  /* Search for closed handles to reuse.  */
> +  if (lowest_closed_fd >= 0)
> +    {
> +      for (index = lowest_closed_fd;
> +	   VEC_iterate (fileio_fh_t, fileio_fhandles, index, fh);
> +	   index++)
> +	if (is_closed_fileio_fh (fh->fd))
> +	  break;

You don't really need 'index'.  If this walks using
lowest_closed_fd instead, like:

      for (;
	   VEC_iterate (fileio_fh_t, fileio_fhandles,
                        lowest_closed_fd, fh);
	   lowest_closed_fd++)
	if (is_closed_fileio_fh (fh->fd))
	  break;

Then the next time around we skip slots we already know are
open.

> +    }
> +
> +  /* Push a new handle if no closed handles were found.  */
> +  if (index == total)
> +    {
> +      fh = VEC_safe_push (fileio_fh_t, fileio_fhandles, NULL);
> +      lowest_closed_fd = -1;
> +    }
> +
> +  /* Fill in the handle and return its index.  */
> +  fh->t = t;
> +  fh->fd = fd;
> +
> +  return index;
> +}
> +

Otherwise looks good.  Please push.

Thanks,
Pedro Alves

Re: [PATCH v2] Associate target_ops with target_fileio file descriptors

[Gary Benson]

Pedro Alves wrote:
> On 03/20/2015 04:06 PM, Gary Benson wrote:
> > +
> > +  /* Search for closed handles to reuse.  */
> > +  if (lowest_closed_fd >= 0)
> > +    {
> > +      for (index = lowest_closed_fd;
> > +	   VEC_iterate (fileio_fh_t, fileio_fhandles, index, fh);
> > +	   index++)
> > +	if (is_closed_fileio_fh (fh->fd))
> > +	  break;
> 
> You don't really need 'index'.  If this walks using
> lowest_closed_fd instead, like:
> 
>       for (;
> 	   VEC_iterate (fileio_fh_t, fileio_fhandles,
>                         lowest_closed_fd, fh);
> 	   lowest_closed_fd++)
> 	if (is_closed_fileio_fh (fh->fd))
> 	  break;
> 
> Then the next time around we skip slots we already know are
> open.
> 
> > +    }
> > +
> > +  /* Push a new handle if no closed handles were found.  */
> > +  if (index == total)
> > +    {
> > +      fh = VEC_safe_push (fileio_fh_t, fileio_fhandles, NULL);
> > +      lowest_closed_fd = -1;
> > +    }
> > +
> > +  /* Fill in the handle and return its index.  */
> > +  fh->t = t;
> > +  fh->fd = fd;
> > +
> > +  return index;
> > +}

You do need another variable though.  The function returns the
index of the file handle it just used, so you need to save that
across the bit that sets lowest_closed_fd to -1.  I rearranged
things to this, but it's longer and I'd argue harder to follow:

  /* Acquire a target fileio file descriptor.  */
  
  static int
  acquire_fileio_fd (struct target_ops *t, int fd)
  {
    int total = VEC_length (fileio_fh_t, fileio_fhandles);
    int index = total;
    fileio_fh_t *fh, buf;
  
    gdb_assert (!is_closed_fileio_fh (fd));
  
    /* Search for closed handles to reuse.  */
    if (lowest_closed_fd >= 0)
      {
        for (;
             VEC_iterate (fileio_fh_t, fileio_fhandles,
                          lowest_closed_fd, fh);
             lowest_closed_fd++)
          if (is_closed_fileio_fh (fh->fd))
            break;
  
        index = lowest_closed_fd;
      }
  
    /* Push a new handle if no closed handles were found.  */
    if (index == total)
      {
        fh = VEC_safe_push (fileio_fh_t, fileio_fhandles, NULL);
        lowest_closed_fd = -1;
      }
  
    /* Fill in the handle and return its index.  */
    fh->t = t;
    fh->fd = fd;
  
    return index;
  }

Do you prefer that?

I noticed you can do "lowest_closed_fd++" if you don't create a new
handle, so whichever way you like I'll add that micro-optimization.

Cheers,
Gary

--
http://gbenson.net/

Re: [PATCH v2] Associate target_ops with target_fileio file descriptors

[Pedro Alves]

On 03/24/2015 04:17 PM, Gary Benson wrote:
> Pedro Alves wrote:
>> On 03/20/2015 04:06 PM, Gary Benson wrote:
>>> +
>>> +  /* Search for closed handles to reuse.  */
>>> +  if (lowest_closed_fd >= 0)
>>> +    {
>>> +      for (index = lowest_closed_fd;
>>> +	   VEC_iterate (fileio_fh_t, fileio_fhandles, index, fh);
>>> +	   index++)
>>> +	if (is_closed_fileio_fh (fh->fd))
>>> +	  break;
>>
>> You don't really need 'index'.  If this walks using
>> lowest_closed_fd instead, like:
>>
>>       for (;
>> 	   VEC_iterate (fileio_fh_t, fileio_fhandles,
>>                         lowest_closed_fd, fh);
>> 	   lowest_closed_fd++)
>> 	if (is_closed_fileio_fh (fh->fd))
>> 	  break;
>>
>> Then the next time around we skip slots we already know are
>> open.
>>
>>> +    }
>>> +
>>> +  /* Push a new handle if no closed handles were found.  */
>>> +  if (index == total)
>>> +    {
>>> +      fh = VEC_safe_push (fileio_fh_t, fileio_fhandles, NULL);
>>> +      lowest_closed_fd = -1;
>>> +    }
>>> +
>>> +  /* Fill in the handle and return its index.  */
>>> +  fh->t = t;
>>> +  fh->fd = fd;
>>> +
>>> +  return index;
>>> +}
> 
> You do need another variable though.  The function returns the
> index of the file handle it just used, so you need to save that
> across the bit that sets lowest_closed_fd to -1.  I rearranged
> things to this, but it's longer and I'd argue harder to follow:

We can get the index with VEC_address.  And if we get rid
of the -1 state, things get event simpler, and release_fileio_fd can
be simplified too, like this (untested):

/* Index into fileio_fhandles of the lowest handle that might be
   closed.  This permits handle reuse without searching the whole list
   each time a new file is opened.  */
static int lowest_closed_fd = 0;

static int
acquire_fileio_fd (struct target_ops *t, int fd)
{
  fileio_fh_t *fh, buf;

  gdb_assert (!is_closed_fileio_fh (fd));

  /* Search for closed handles to reuse.  */
  for (;
       VEC_iterate (fileio_fh_t, fileio_fhandles,
		    lowest_closed_fd, fh);
       lowest_closed_fd++)
    if (is_closed_fileio_fh (fh->fd))
      break;

  /* Push a new handle if no closed handles were found.  */
  if (lowest_closed_fd == VEC_length (fileio_fh_t, fileio_fhandles))
    {
      fh = VEC_safe_push (fileio_fh_t, fileio_fhandles, NULL);
      lowest_closed_fd++;
    }

  /* Fill in the handle.  */
  fh->t = t;
  fh->fd = fd;

  /* Return its index.  */
  return fh - VEC_address (fileio_fh_t, fileio_fhandles);
}

static void
release_fileio_fd (int fd, fileio_fh_t *fh)
{
  fh->fd = -1;
  lowest_closed_fd = min (lowest_closed_fd, fd);
}

Thanks,
Pedro Alves

Re: [PATCH v2] Associate target_ops with target_fileio file descriptors

[Pedro Alves]

On 03/24/2015 04:36 PM, Pedro Alves wrote:
>   /* Push a new handle if no closed handles were found.  */
>   if (lowest_closed_fd == VEC_length (fileio_fh_t, fileio_fhandles))
>     {
>       fh = VEC_safe_push (fileio_fh_t, fileio_fhandles, NULL);
>       lowest_closed_fd++;
>     }

Make this:

   /* Push a new handle if no closed handles were found.  */
   if (lowest_closed_fd == VEC_length (fileio_fh_t, fileio_fhandles))
     fh = VEC_safe_push (fileio_fh_t, fileio_fhandles, NULL);

   lowest_closed_fd++;

Because there's no point in starting the next lookup at the
handle we just opened if we're reusing a slot, either.  And then
that allows getting rid of the VEC_address, by simply doing
that increment at the end:

/* Index into fileio_fhandles of the lowest handle that might be
   closed.  This permits handle reuse without searching the whole list
   each time a new file is opened.  */
static int lowest_closed_fd = 0;

static int
acquire_fileio_fd (struct target_ops *t, int fd)
{
  fileio_fh_t *fh, buf;

  gdb_assert (!is_closed_fileio_fh (fd));

  /* Search for closed handles to reuse.  */
  for (;
       VEC_iterate (fileio_fh_t, fileio_fhandles,
		    lowest_closed_fd, fh);
       lowest_closed_fd++)
    if (is_closed_fileio_fh (fh->fd))
      break;

   /* Push a new handle if no closed handles were found.  */
   if (lowest_closed_fd == VEC_length (fileio_fh_t, fileio_fhandles))
     fh = VEC_safe_push (fileio_fh_t, fileio_fhandles, NULL);

  /* Fill in the handle.  */
  fh->t = t;
  fh->fd = fd;

  /* Return its index, and start the next lookup at
     the next index.  */
  return lowest_closed_fd++;
}

[pushed] [PATCH v2] Associate target_ops with target_fileio file descriptors

[Gary Benson]

Pedro Alves wrote:
> Because there's no point in starting the next lookup at the
> handle we just opened if we're reusing a slot, either.  And
> then that allows getting rid of the VEC_address, by simply
> doing that increment at the end:
> 
> /* Index into fileio_fhandles of the lowest handle that might be
>    closed.  This permits handle reuse without searching the whole list
>    each time a new file is opened.  */
> static int lowest_closed_fd = 0;
> 
> static int
> acquire_fileio_fd (struct target_ops *t, int fd)
> {
>   fileio_fh_t *fh, buf;
> 
>   gdb_assert (!is_closed_fileio_fh (fd));
> 
>   /* Search for closed handles to reuse.  */
>   for (;
>        VEC_iterate (fileio_fh_t, fileio_fhandles,
> 		    lowest_closed_fd, fh);
>        lowest_closed_fd++)
>     if (is_closed_fileio_fh (fh->fd))
>       break;
> 
>    /* Push a new handle if no closed handles were found.  */
>    if (lowest_closed_fd == VEC_length (fileio_fh_t, fileio_fhandles))
>      fh = VEC_safe_push (fileio_fh_t, fileio_fhandles, NULL);
> 
>   /* Fill in the handle.  */
>   fh->t = t;
>   fh->fd = fd;
> 
>   /* Return its index, and start the next lookup at
>      the next index.  */
>   return lowest_closed_fd++;
> }

That's nice, I pushed that one, thanks!

Gary

-- 
http://gbenson.net/