diff mbox

[v2] Associate target_ops with target_fileio file descriptors

Message ID 1426867612-32278-1-git-send-email-gbenson@redhat.com
State Committed
Headers

Commit Message

Gary Benson March 20, 2015, 4:06 p.m. UTC 
  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(-)

Comments

Pedro Alves March 24, 2015, 1:22 p.m. UTC | #1 
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
Gary Benson March 24, 2015, 4:17 p.m. UTC | #2 
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/
Pedro Alves March 24, 2015, 4:36 p.m. UTC | #3 
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
Pedro Alves March 24, 2015, 4:55 p.m. UTC | #4 
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++;
}
Gary Benson March 25, 2015, 11:32 a.m. UTC | #5 
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
diff mbox

Patch

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