[RFA,09/23] Remove close cleanup

Message ID 20170503224626.2818-10-tom@tromey.com
State New, archived
Headers

Commit Message

Tom Tromey May 3, 2017, 10:46 p.m. UTC
  make_cleanup_close calls close on a file descriptor.  This patch
replaces it with a new RAII class, gdb::fd_closer.

2017-05-02  Tom Tromey  <tom@tromey.com>

	* source.c (get_filename_and_charpos, forward_search_command)
	(reverse_search_command): Use fd_closer.
	* procfs.c (load_syscalls, proc_get_LDT_entry)
	(iterate_over_mappings): Use fd_closer.
	* nto-procfs.c (procfs_open_1, procfs_pidlist): Use fd_closer.
	* nat/linux-namespaces.c (linux_mntns_access_fs): Use fd_closer.
	* common/filestuff.h (gdb::fd_closer): New class.
	* common/filestuff.c (do_close_cleanup, make_cleanup_close):
	Remove.
---
 gdb/ChangeLog              | 12 +++++++
 gdb/common/filestuff.c     | 21 ------------
 gdb/common/filestuff.h     | 39 ++++++++++++++++++++--
 gdb/nat/linux-namespaces.c | 80 +++++++++++++++++++++-------------------------
 gdb/nto-procfs.c           |  9 ++----
 gdb/procfs.c               | 26 ++++++---------
 gdb/source.c               | 13 ++++----
 7 files changed, 103 insertions(+), 97 deletions(-)
  

Comments

Pedro Alves June 2, 2017, 6:08 p.m. UTC | #1
Patch looks good.

The bit below made me stop for a second:

On 05/03/2017 11:46 PM, Tom Tromey wrote:
> @@ -901,64 +900,59 @@ linux_mntns_access_fs (pid_t pid)
>    if (ns == NULL)
>      return MNSH_FS_DIRECT;
>  
> -  old_chain = make_cleanup (null_cleanup, NULL);
> -
>    fd = gdb_open_cloexec (linux_ns_filename (ns, pid), O_RDONLY, 0);
>    if (fd < 0)
> -    goto error;
> -
> -  make_cleanup_close (fd);
> +    return MNSH_FS_ERROR;
>  
> -  if (fstat (fd, &sb) != 0)
> -    goto error;
> +  /* Introduce a new scope here so we can reset errno after
> +     closing.  */
> +  {
> +    gdb::fd_closer close_fd (fd);
>  

Would the following be too clever?

    /* Restore errno on exit to the value saved by the
       last save() call.  Ctor saves.  */
    struct scoped_errno_restore
    {
      scoped_errno_restore () { save (); }
      ~scoped_errno_restore () { errno = m_value; }

      /* Take a snapshot of current errno.  */
      void save () { m_value = errno; }

    private:
      int m_value;
    };

    scoped_errno_restore restore_errno;
    gdb::fd_closer close_fd (fd);

    /* Save errno and return MNSH_FS_ERROR.  */
    auto fs_err = [&] ()
    {
       restore_errno.save ();
       return MNSH_FS_ERROR;
    };

    if (fstat (fd, &sb) != 0)
      return fs_err ();

    (...)

    helper = linux_mntns_get_helper ();
    if (helper == NULL)
      return fs_err ();

    size = mnsh_send_setns (helper, fd, 0);
    if (size < 0)
      return fs_err ();

[It gets rid of both the scope, and the gotos.]

Thanks,
Pedro Alves
  
Tom Tromey July 19, 2017, 10:27 p.m. UTC | #2
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> Patch looks good.
Pedro> The bit below made me stop for a second:
[...]


Pedro> Would the following be too clever?
Pedro>     /* Restore errno on exit to the value saved by the
Pedro>        last save() call.  Ctor saves.  */
Pedro>     struct scoped_errno_restore
[...]

Pedro>     /* Save errno and return MNSH_FS_ERROR.  */
Pedro>     auto fs_err = [&] ()
Pedro>     {
Pedro>        restore_errno.save ();
Pedro>        return MNSH_FS_ERROR;
Pedro>     };
[...]

Pedro> [It gets rid of both the scope, and the gotos.]

How about making fd_closer a template, like:

template<int (*CLOSER) (int) = close>
class fd_closer
...
  ~fd_closer ()
  {
    if (m_fd != -1)
      CLOSER (m_fd);
  }

Then in linux-namespaces.c:

/* A function like "close" that saves and restores errno.  */

static int
close_saving_fd (int fd)
{
  scoped_restore save_errno = make_scoped_restore (&errno);
  return close (fd);
}

...

  gdb::fd_closer<close_saving_errno> close_fd (fd);


This also removes the scope and the gotos.

The main plus is that it's simpler.  The main minus is that it's ad hoc.

Tom
  
Simon Marchi July 31, 2017, 7:08 p.m. UTC | #3
On 2017-07-20 00:27, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
> Pedro> Patch looks good.
> Pedro> The bit below made me stop for a second:
> [...]
> 
> 
> Pedro> Would the following be too clever?
> Pedro>     /* Restore errno on exit to the value saved by the
> Pedro>        last save() call.  Ctor saves.  */
> Pedro>     struct scoped_errno_restore
> [...]
> 
> Pedro>     /* Save errno and return MNSH_FS_ERROR.  */
> Pedro>     auto fs_err = [&] ()
> Pedro>     {
> Pedro>        restore_errno.save ();
> Pedro>        return MNSH_FS_ERROR;
> Pedro>     };
> [...]
> 
> Pedro> [It gets rid of both the scope, and the gotos.]
> 
> How about making fd_closer a template, like:
> 
> template<int (*CLOSER) (int) = close>
> class fd_closer
> ...
>   ~fd_closer ()
>   {
>     if (m_fd != -1)
>       CLOSER (m_fd);
>   }
> 
> Then in linux-namespaces.c:
> 
> /* A function like "close" that saves and restores errno.  */
> 
> static int
> close_saving_fd (int fd)
> {
>   scoped_restore save_errno = make_scoped_restore (&errno);
>   return close (fd);
> }
> 
> ...
> 
>   gdb::fd_closer<close_saving_errno> close_fd (fd);
> 
> 
> This also removes the scope and the gotos.
> 
> The main plus is that it's simpler.  The main minus is that it's ad 
> hoc.
> 
> Tom

(Responding here although I am reviewing v2)

Instead of finding a solution to preserve errno, I think we should 
return it explicitly when needed, it would be more robust and easier to 
follow the trail where the value comes from.  That would mean changing

static enum mnsh_fs_code
linux_mntns_access_fs (pid_t pid)

to

static enum mnsh_fs_code
linux_mntns_access_fs (pid_t pid, int *errnop)

and possibly others.  In that case, *ERRNOP would be set if 
MNSH_FS_ERROR is returned.

Simon
  
Tom Tromey Aug. 1, 2017, 9:41 p.m. UTC | #4
>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:

Simon> Instead of finding a solution to preserve errno, I think we should
Simon> return it explicitly when needed, it would be more robust and easier
Simon> to follow the trail where the value comes from.  That would mean
Simon> changing

Simon> static enum mnsh_fs_code
Simon> linux_mntns_access_fs (pid_t pid)

Simon> to

Simon> static enum mnsh_fs_code
Simon> linux_mntns_access_fs (pid_t pid, int *errnop)

Simon> and possibly others.  In that case, *ERRNOP would be set if
Simon> MNSH_FS_ERROR is returned.

I think somebody else should take this then.
I will see if I can drop this patch from the series.

Tom
  

Patch

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 2d0d71d..50d8794 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,17 @@ 
 2017-05-02  Tom Tromey  <tom@tromey.com>
 
+	* source.c (get_filename_and_charpos, forward_search_command)
+	(reverse_search_command): Use fd_closer.
+	* procfs.c (load_syscalls, proc_get_LDT_entry)
+	(iterate_over_mappings): Use fd_closer.
+	* nto-procfs.c (procfs_open_1, procfs_pidlist): Use fd_closer.
+	* nat/linux-namespaces.c (linux_mntns_access_fs): Use fd_closer.
+	* common/filestuff.h (gdb::fd_closer): New class.
+	* common/filestuff.c (do_close_cleanup, make_cleanup_close):
+	Remove.
+
+2017-05-02  Tom Tromey  <tom@tromey.com>
+
 	* compile/compile.c (cleanup_unlink_file): Remove.
 	(compile_to_object): Use gdb::unlinker.
 	(eval_compile_command): Likewise.
diff --git a/gdb/common/filestuff.c b/gdb/common/filestuff.c
index 4b05884..5a0dd65 100644
--- a/gdb/common/filestuff.c
+++ b/gdb/common/filestuff.c
@@ -404,24 +404,3 @@  gdb_pipe_cloexec (int filedes[2])
 
   return result;
 }
-
-/* Helper function which does the work for make_cleanup_close.  */
-
-static void
-do_close_cleanup (void *arg)
-{
-  int *fd = (int *) arg;
-
-  close (*fd);
-}
-
-/* See filestuff.h.  */
-
-struct cleanup *
-make_cleanup_close (int fd)
-{
-  int *saved_fd = XNEW (int);
-
-  *saved_fd = fd;
-  return make_cleanup_dtor (do_close_cleanup, saved_fd, xfree);
-}
diff --git a/gdb/common/filestuff.h b/gdb/common/filestuff.h
index 3cf2df6..82eb5b3 100644
--- a/gdb/common/filestuff.h
+++ b/gdb/common/filestuff.h
@@ -80,8 +80,43 @@  extern int gdb_socket_cloexec (int domain, int style, int protocol);
 
 extern int gdb_pipe_cloexec (int filedes[2]);
 
-/* Return a new cleanup that closes FD.  */
+namespace gdb
+{
+
+/* A class that manages a file descriptor and closes it on
+   destruction.  */
+
+class fd_closer
+{
+public:
+
+  explicit fd_closer (int fd)
+    : m_fd (fd)
+  {
+  }
+
+  ~fd_closer ()
+  {
+    if (m_fd != -1)
+      close (m_fd);
+  }
+
+  fd_closer (const fd_closer &) = delete;
+  fd_closer &operator= (const fd_closer &) = delete;
+
+  /* Release the file descriptor to the caller.  */
+  int release ()
+  {
+    int result = m_fd;
+    m_fd = -1;
+    return result;
+  }
+
+private:
+
+  int m_fd;
+};
 
-extern struct cleanup *make_cleanup_close (int fd);
+}
 
 #endif /* FILESTUFF_H */
diff --git a/gdb/nat/linux-namespaces.c b/gdb/nat/linux-namespaces.c
index 1df8f1b..fcc7ba3 100644
--- a/gdb/nat/linux-namespaces.c
+++ b/gdb/nat/linux-namespaces.c
@@ -887,7 +887,6 @@  enum mnsh_fs_code
 static enum mnsh_fs_code
 linux_mntns_access_fs (pid_t pid)
 {
-  struct cleanup *old_chain;
   struct linux_ns *ns;
   struct stat sb;
   struct linux_mnsh *helper;
@@ -901,64 +900,59 @@  linux_mntns_access_fs (pid_t pid)
   if (ns == NULL)
     return MNSH_FS_DIRECT;
 
-  old_chain = make_cleanup (null_cleanup, NULL);
-
   fd = gdb_open_cloexec (linux_ns_filename (ns, pid), O_RDONLY, 0);
   if (fd < 0)
-    goto error;
-
-  make_cleanup_close (fd);
+    return MNSH_FS_ERROR;
 
-  if (fstat (fd, &sb) != 0)
-    goto error;
+  /* Introduce a new scope here so we can reset errno after
+     closing.  */
+  {
+    gdb::fd_closer close_fd (fd);
 
-  if (sb.st_ino == ns->id)
-    {
-      do_cleanups (old_chain);
+    if (fstat (fd, &sb) != 0)
+      goto error;
 
+    if (sb.st_ino == ns->id)
       return MNSH_FS_DIRECT;
-    }
-
-  helper = linux_mntns_get_helper ();
-  if (helper == NULL)
-    goto error;
 
-  if (sb.st_ino != helper->nsid)
-    {
-      int result, error;
+    helper = linux_mntns_get_helper ();
+    if (helper == NULL)
+      goto error;
 
-      size = mnsh_send_setns (helper, fd, 0);
-      if (size < 0)
-	goto error;
+    if (sb.st_ino != helper->nsid)
+      {
+	int result, error;
 
-      if (mnsh_recv_int (helper, &result, &error) != 0)
-	goto error;
+	size = mnsh_send_setns (helper, fd, 0);
+	if (size < 0)
+	  goto error;
 
-      if (result != 0)
-	{
-	  /* ENOSYS indicates that an entire function is unsupported
-	     (it's not appropriate for our versions of open/unlink/
-	     readlink to sometimes return with ENOSYS depending on how
-	     they're called) so we convert ENOSYS to ENOTSUP if setns
-	     fails.  */
-	  if (error == ENOSYS)
-	    error = ENOTSUP;
-
-	  errno = error;
+	if (mnsh_recv_int (helper, &result, &error) != 0)
 	  goto error;
-	}
 
-      helper->nsid = sb.st_ino;
-    }
+	if (result != 0)
+	  {
+	    /* ENOSYS indicates that an entire function is unsupported
+	       (it's not appropriate for our versions of open/unlink/
+	       readlink to sometimes return with ENOSYS depending on how
+	       they're called) so we convert ENOSYS to ENOTSUP if setns
+	       fails.  */
+	    if (error == ENOSYS)
+	      error = ENOTSUP;
+
+	    errno = error;
+	    goto error;
+	  }
 
-  do_cleanups (old_chain);
+	helper->nsid = sb.st_ino;
+      }
 
-  return MNSH_FS_HELPER;
+    return MNSH_FS_HELPER;
 
-error:
-  saved_errno = errno;
+  error:
+    saved_errno = errno;
 
-  do_cleanups (old_chain);
+  }
 
   errno = saved_errno;
   return MNSH_FS_ERROR;
diff --git a/gdb/nto-procfs.c b/gdb/nto-procfs.c
index 7fb7095..63ef2ec 100644
--- a/gdb/nto-procfs.c
+++ b/gdb/nto-procfs.c
@@ -115,7 +115,6 @@  procfs_open_1 (struct target_ops *ops, const char *arg, int from_tty)
   char buffer[50];
   int fd, total_size;
   procfs_sysinfo *sysinfo;
-  struct cleanup *cleanups;
   char nto_procfs_path[PATH_MAX];
 
   /* Offer to kill previous inferiors before opening this target.  */
@@ -165,7 +164,7 @@  procfs_open_1 (struct target_ops *ops, const char *arg, int from_tty)
 		       safe_strerror (errno));
       error (_("Invalid procfs arg"));
     }
-  cleanups = make_cleanup_close (fd);
+  gdb::fd_closer close_fd (fd);
 
   sysinfo = (void *) buffer;
   if (devctl (fd, DCMD_PROC_SYSINFO, sysinfo, sizeof buffer, 0) != EOK)
@@ -201,7 +200,6 @@  procfs_open_1 (struct target_ops *ops, const char *arg, int from_tty)
 	    }
 	}
     }
-  do_cleanups (cleanups);
 
   inf_child_open_target (ops, arg, from_tty);
   printf_filtered ("Debugging using %s\n", nto_procfs_path);
@@ -392,7 +390,6 @@  procfs_pidlist (char *args, int from_tty)
   do
     {
       int fd;
-      struct cleanup *inner_cleanup;
 
       /* Get the right pid and procfs path for the pid.  */
       do
@@ -418,7 +415,7 @@  procfs_pidlist (char *args, int from_tty)
 			      buf, errno, safe_strerror (errno));
 	  continue;
 	}
-      inner_cleanup = make_cleanup_close (fd);
+      gdb::fd_closer close_fd (fd);
 
       pidinfo = (procfs_info *) buf;
       if (devctl (fd, DCMD_PROC_INFO, pidinfo, sizeof (buf), 0) != EOK)
@@ -451,8 +448,6 @@  procfs_pidlist (char *args, int from_tty)
 	      break;
 	    }
 	}
-
-      do_cleanups (inner_cleanup);
     }
   while (dirp != NULL);
 
diff --git a/gdb/procfs.c b/gdb/procfs.c
index 5d940dd..2aaee07 100644
--- a/gdb/procfs.c
+++ b/gdb/procfs.c
@@ -31,6 +31,7 @@ 
 #include "regcache.h"
 #include "inf-child.h"
 #include "filestuff.h"
+#include "common/filestuff.h"
 
 #if defined (NEW_PROC_API)
 #define _STRUCTURED_PROC 1	/* Should be done by configure script.  */
@@ -883,7 +884,8 @@  load_syscalls (procinfo *pi)
     {
       error (_("load_syscalls: Can't open /proc/%d/sysent"), pi->pid);
     }
-  cleanups = make_cleanup_close (sysent_fd);
+
+  gdb::fd_closer close_sysent_fd (sysent_fd);
 
   size = sizeof header - sizeof (prsyscall_t);
   if (read (sysent_fd, &header, size) != size)
@@ -899,7 +901,7 @@  load_syscalls (procinfo *pi)
 
   size = header.pr_nsyscalls * sizeof (prsyscall_t);
   syscalls = xmalloc (size);
-  make_cleanup (free_current_contents, &syscalls);
+  cleanups = make_cleanup (free_current_contents, &syscalls);
 
   if (read (sysent_fd, syscalls, size) != size)
     error (_("load_syscalls: Error reading /proc/%d/sysent"), pi->pid);
@@ -2484,7 +2486,6 @@  proc_get_LDT_entry (procinfo *pi, int key)
   static struct ssd *ldt_entry = NULL;
 #ifdef NEW_PROC_API
   char pathname[MAX_PROC_NAME_SIZE];
-  struct cleanup *old_chain = NULL;
   int  fd;
 
   /* Allocate space for one LDT entry.
@@ -2499,8 +2500,8 @@  proc_get_LDT_entry (procinfo *pi, int key)
       proc_warn (pi, "proc_get_LDT_entry (open)", __LINE__);
       return NULL;
     }
-  /* Make sure it gets closed again!  */
-  old_chain = make_cleanup_close (fd);
+
+  gdb::fd_closer close_fd (fd);
 
   /* Now 'read' thru the table, find a match and return it.  */
   while (read (fd, ldt_entry, sizeof (struct ssd)) == sizeof (struct ssd))
@@ -2512,13 +2513,9 @@  proc_get_LDT_entry (procinfo *pi, int key)
 	break;	/* end of table */
       /* If key matches, return this entry.  */
       if (ldt_entry->sel == key)
-	{
-	  do_cleanups (old_chain);
-	  return ldt_entry;
-	}
+	return ldt_entry;
     }
   /* Loop ended, match not found.  */
-  do_cleanups (old_chain);
   return NULL;
 #else
   int nldt, i;
@@ -4912,7 +4909,6 @@  iterate_over_mappings (procinfo *pi, find_memory_region_ftype child_func,
   int funcstat;
   int map_fd;
   int nmap;
-  struct cleanup *cleanups = make_cleanup (null_cleanup, NULL);
 #ifdef NEW_PROC_API
   struct stat sbuf;
 #endif
@@ -4926,7 +4922,7 @@  iterate_over_mappings (procinfo *pi, find_memory_region_ftype child_func,
     proc_error (pi, "iterate_over_mappings (open)", __LINE__);
 
   /* Make sure it gets closed again.  */
-  make_cleanup_close (map_fd);
+  gdb::fd_closer close_map_fd (map_fd);
 
   /* Use stat to determine the file size, and compute
      the number of prmap_t objects it contains.  */
@@ -4950,12 +4946,8 @@  iterate_over_mappings (procinfo *pi, find_memory_region_ftype child_func,
 
   for (prmap = prmaps; nmap > 0; prmap++, nmap--)
     if ((funcstat = (*func) (prmap, child_func, data)) != 0)
-      {
-	do_cleanups (cleanups);
-        return funcstat;
-      }
+      return funcstat;
 
-  do_cleanups (cleanups);
   return 0;
 }
 
diff --git a/gdb/source.c b/gdb/source.c
index 4cc862c..3d477ad 100644
--- a/gdb/source.c
+++ b/gdb/source.c
@@ -1302,14 +1302,14 @@  get_filename_and_charpos (struct symtab *s, char **fullname)
 	*fullname = NULL;
       return 0;
     }
-  cleanups = make_cleanup_close (desc);
+
+  gdb::fd_closer close_desc (desc);
   if (fullname)
     *fullname = s->fullname;
   if (s->line_charpos == 0)
     linenums_changed = 1;
   if (linenums_changed)
     find_source_lines (s, desc);
-  do_cleanups (cleanups);
   return linenums_changed;
 }
 
@@ -1627,7 +1627,6 @@  forward_search_command (char *regex, int from_tty)
   int desc;
   int line;
   char *msg;
-  struct cleanup *cleanups;
 
   line = last_line_listed + 1;
 
@@ -1641,7 +1640,7 @@  forward_search_command (char *regex, int from_tty)
   desc = open_source_file (current_source_symtab);
   if (desc < 0)
     perror_with_name (symtab_to_filename_for_display (current_source_symtab));
-  cleanups = make_cleanup_close (desc);
+  gdb::fd_closer close_desc (desc);
 
   if (current_source_symtab->line_charpos == 0)
     find_source_lines (current_source_symtab, desc);
@@ -1652,7 +1651,7 @@  forward_search_command (char *regex, int from_tty)
   if (lseek (desc, current_source_symtab->line_charpos[line - 1], 0) < 0)
     perror_with_name (symtab_to_filename_for_display (current_source_symtab));
 
-  discard_cleanups (cleanups);
+  close_desc.release ();
   gdb_file_up stream (fdopen (desc, FDOPEN_MODE));
   clearerr (stream.get ());
   while (1)
@@ -1726,7 +1725,7 @@  reverse_search_command (char *regex, int from_tty)
   desc = open_source_file (current_source_symtab);
   if (desc < 0)
     perror_with_name (symtab_to_filename_for_display (current_source_symtab));
-  cleanups = make_cleanup_close (desc);
+  gdb::fd_closer close_desc (desc);
 
   if (current_source_symtab->line_charpos == 0)
     find_source_lines (current_source_symtab, desc);
@@ -1737,7 +1736,7 @@  reverse_search_command (char *regex, int from_tty)
   if (lseek (desc, current_source_symtab->line_charpos[line - 1], 0) < 0)
     perror_with_name (symtab_to_filename_for_display (current_source_symtab));
 
-  discard_cleanups (cleanups);
+  close_desc.release ();
   gdb_file_up stream (fdopen (desc, FDOPEN_MODE));
   clearerr (stream.get ());
   while (line > 1)