[RFC,5/6] Do not reopen temporary files

Message ID 20180926111130.18956-6-tom@tromey.com
State New, archived
Headers

Commit Message

Tom Tromey Sept. 26, 2018, 11:11 a.m. UTC
  The current callers of mkostemp close the file descriptor and then
re-open it with fopen.  It seemed better to me to continue to use the
already-opened file descriptor, so this patch rearranges the code a
little in order to do so.  It takes care to ensure that the files are
only unlinked after the file descriptor in question is closed, as
before.

gdb/ChangeLog
2018-09-26  Tom Tromey  <tom@tromey.com>

	* unittests/scoped_fd-selftests.c (test_to_file): New function.
	(run_tests): Call test_to_file.
	* dwarf-index-write.c (write_psymtabs_to_index): Do not reopen
	temporary files.
	* common/scoped_fd.h (scoped_fd::to_file): New method.
---
 gdb/ChangeLog                       |  8 +++++
 gdb/common/scoped_fd.h              | 13 +++++++
 gdb/dwarf-index-write.c             | 55 ++++++++++++++---------------
 gdb/unittests/scoped_fd-selftests.c | 17 +++++++++
 4 files changed, 65 insertions(+), 28 deletions(-)
  

Comments

Eli Zaretskii Sept. 26, 2018, 2:11 p.m. UTC | #1
> From: Tom Tromey <tom@tromey.com>
> Cc: Tom Tromey <tom@tromey.com>
> Date: Wed, 26 Sep 2018 05:11:29 -0600
> 
> The current callers of mkostemp close the file descriptor and then
> re-open it with fopen.  It seemed better to me to continue to use the
> already-opened file descriptor, so this patch rearranges the code a
> little in order to do so.  It takes care to ensure that the files are
> only unlinked after the file descriptor in question is closed, as
> before.

Bother...

> -  FILE *out_file = gdb_fopen_cloexec (filename_temp.data (), "wb").release ();
> +  gdb_file_up out_file = out_file_fd.to_file ("wb");

There's a known bug in the MS-Windows (thus MinGW) implementation of
'fdopen': it ignores the 'b' and 't' flags.  So the preceding call to
'open' must itself specify the O_BINARY flag, or else this replacement
will introduce a subtle bug.

Thanks.
  
Tom Tromey Sept. 26, 2018, 9:44 p.m. UTC | #2
>>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:

>> -  FILE *out_file = gdb_fopen_cloexec (filename_temp.data (), "wb").release ();
>> +  gdb_file_up out_file = out_file_fd.to_file ("wb");

Eli> There's a known bug in the MS-Windows (thus MinGW) implementation of
Eli> 'fdopen': it ignores the 'b' and 't' flags.  So the preceding call to
Eli> 'open' must itself specify the O_BINARY flag, or else this replacement
Eli> will introduce a subtle bug.

Thanks.  I think I can pass O_BINARY to mkostemp.

Tom
  
Simon Marchi Sept. 29, 2018, 3:22 a.m. UTC | #3
On 2018-09-26 7:11 a.m., Tom Tromey wrote:
> diff --git a/gdb/dwarf-index-write.c b/gdb/dwarf-index-write.c
> index 7049e8968a..341e16bfc5 100644
> --- a/gdb/dwarf-index-write.c
> +++ b/gdb/dwarf-index-write.c
> @@ -1565,23 +1565,21 @@ write_psymtabs_to_index (struct dwarf2_per_objfile *dwarf2_per_objfile,
>  			   ? INDEX5_SUFFIX : INDEX4_SUFFIX));
>    gdb::char_vector filename_temp = make_temp_filename (filename);
>  
> -  gdb::optional<scoped_fd> out_file_fd
> -    (gdb::in_place, gdb_mkstemp_cloexec (filename_temp.data ()));
> -  if (out_file_fd->get () == -1)
> +  /* Order matters here; we want FILE to be closed before FILENAME_TEMP is
> +     unlinked, because on MS-Windows one cannot delete a file that is
> +     still open.  (Don't call anything here that might throw until
> +     file_closer is created.)  So, we wrap the unlinker in an optional

I just noticed this refers to file_closer, a variable that doesn't exist anymore
(or has been renamed).

Otherwise this LGTM.

Simon
  
Tom Tromey Oct. 1, 2018, 9:14 a.m. UTC | #4
>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

>> +  /* Order matters here; we want FILE to be closed before FILENAME_TEMP is
>> +     unlinked, because on MS-Windows one cannot delete a file that is
>> +     still open.  (Don't call anything here that might throw until
>> +     file_closer is created.)  So, we wrap the unlinker in an optional

Simon> I just noticed this refers to file_closer, a variable that doesn't exist anymore
Simon> (or has been renamed).

Thanks.  I've removed the parenthesized sentence, because it no longer
applies.

Tom
  

Patch

diff --git a/gdb/common/scoped_fd.h b/gdb/common/scoped_fd.h
index a6a8ab9a38..f37776ae21 100644
--- a/gdb/common/scoped_fd.h
+++ b/gdb/common/scoped_fd.h
@@ -25,6 +25,7 @@ 
 #ifdef HAVE_UNISTD_H
 
 #include <unistd.h>
+#include "filestuff.h"
 
 /* A smart-pointer-like class to automatically close a file descriptor.  */
 
@@ -47,6 +48,18 @@  public:
     return fd;
   }
 
+  /* Like release, but return a gdb_file_up that owns the file
+     descriptor.  On success, this scoped_fd will be released.  On
+     failure, return NULL and leave this scoped_fd in possession of
+     the fd.  */
+  gdb_file_up to_file (const char *mode) noexcept
+  {
+    gdb_file_up result (fdopen (m_fd, mode));
+    if (result != nullptr)
+      m_fd = -1;
+    return result;
+  }
+
   int get () const noexcept
   {
     return m_fd;
diff --git a/gdb/dwarf-index-write.c b/gdb/dwarf-index-write.c
index 7049e8968a..341e16bfc5 100644
--- a/gdb/dwarf-index-write.c
+++ b/gdb/dwarf-index-write.c
@@ -1565,23 +1565,21 @@  write_psymtabs_to_index (struct dwarf2_per_objfile *dwarf2_per_objfile,
 			   ? INDEX5_SUFFIX : INDEX4_SUFFIX));
   gdb::char_vector filename_temp = make_temp_filename (filename);
 
-  gdb::optional<scoped_fd> out_file_fd
-    (gdb::in_place, gdb_mkstemp_cloexec (filename_temp.data ()));
-  if (out_file_fd->get () == -1)
+  /* Order matters here; we want FILE to be closed before FILENAME_TEMP is
+     unlinked, because on MS-Windows one cannot delete a file that is
+     still open.  (Don't call anything here that might throw until
+     file_closer is created.)  So, we wrap the unlinker in an optional
+     and emplace it once we know the file name.  */
+  gdb::optional<gdb::unlinker> unlink_file;
+  scoped_fd out_file_fd (gdb_mkstemp_cloexec (filename_temp.data ()));
+  if (out_file_fd.get () == -1)
     perror_with_name (("mkstemp"));
 
-  FILE *out_file = gdb_fopen_cloexec (filename_temp.data (), "wb").release ();
+  gdb_file_up out_file = out_file_fd.to_file ("wb");
   if (out_file == nullptr)
     error (_("Can't open `%s' for writing"), filename_temp.data ());
 
-  /* Order matters here; we want FILE to be closed before FILENAME_TEMP is
-     unlinked, because on MS-Windows one cannot delete a file that is
-     still open.  (Don't call anything here that might throw until
-     file_closer is created.)  We don't need OUT_FILE_FD anymore, so we might
-     as well close it now.  */
-  out_file_fd.reset ();
-  gdb::unlinker unlink_file (filename_temp.data ());
-  gdb_file_up close_out_file (out_file);
+  unlink_file.emplace (filename_temp.data ());
 
   if (index_kind == dw_index_kind::DEBUG_NAMES)
     {
@@ -1589,44 +1587,45 @@  write_psymtabs_to_index (struct dwarf2_per_objfile *dwarf2_per_objfile,
 				+ basename + DEBUG_STR_SUFFIX);
       gdb::char_vector filename_str_temp = make_temp_filename (filename_str);
 
-      gdb::optional<scoped_fd> out_file_str_fd
-	(gdb::in_place, gdb_mkstemp_cloexec (filename_str_temp.data ()));
-      if (out_file_str_fd->get () == -1)
+      /* As above, arrange to unlink the file only after the file
+	 descriptor has been closed.  */
+      gdb::optional<gdb::unlinker> unlink_file_str;
+      scoped_fd out_file_str_fd
+	(gdb_mkstemp_cloexec (filename_str_temp.data ()));
+      if (out_file_str_fd.get () == -1)
         perror_with_name (("mkstemp"));
 
-      FILE *out_file_str
-	= gdb_fopen_cloexec (filename_str_temp.data (), "wb").release ();
+      gdb_file_up out_file_str = out_file_str_fd.to_file ("wb");
       if (out_file_str == nullptr)
 	error (_("Can't open `%s' for writing"), filename_str_temp.data ());
 
-      out_file_str_fd.reset ();
-      gdb::unlinker unlink_file_str (filename_str_temp.data ());
-      gdb_file_up close_out_file_str (out_file_str);
+      unlink_file_str.emplace (filename_str_temp.data ());
 
       const size_t total_len
-	= write_debug_names (dwarf2_per_objfile, out_file, out_file_str);
-      assert_file_size (out_file, filename_temp.data (), total_len);
+	= write_debug_names (dwarf2_per_objfile, out_file.get (),
+			     out_file_str.get ());
+      assert_file_size (out_file.get (), filename_temp.data (), total_len);
 
       /* We want to keep the file .debug_str file too.  */
-      unlink_file_str.keep ();
+      unlink_file_str->keep ();
 
       /* Close and move the str file in place.  */
-      close_out_file_str.reset ();
+      out_file_str.reset ();
       if (rename (filename_str_temp.data (), filename_str.c_str ()) != 0)
 	perror_with_name (("rename"));
     }
   else
     {
       const size_t total_len
-	= write_gdbindex (dwarf2_per_objfile, out_file);
-      assert_file_size (out_file, filename_temp.data (), total_len);
+	= write_gdbindex (dwarf2_per_objfile, out_file.get ());
+      assert_file_size (out_file.get (), filename_temp.data (), total_len);
     }
 
   /* We want to keep the file.  */
-  unlink_file.keep ();
+  unlink_file->keep ();
 
   /* Close and move the file in place.  */
-  close_out_file.reset ();
+  out_file.reset ();
   if (rename (filename_temp.data (), filename.c_str ()) != 0)
 	perror_with_name (("rename"));
 }
diff --git a/gdb/unittests/scoped_fd-selftests.c b/gdb/unittests/scoped_fd-selftests.c
index 4320b540cd..4690ae79f5 100644
--- a/gdb/unittests/scoped_fd-selftests.c
+++ b/gdb/unittests/scoped_fd-selftests.c
@@ -68,12 +68,29 @@  test_release ()
   SELF_CHECK (close (fd) == 0 || errno != EBADF);
 }
 
+/* Test that the file descriptor can be converted to a FILE *.  */
+static void
+test_to_file ()
+{
+  char filename[] = "scoped_fd-selftest-XXXXXX";
+
+  ::scoped_fd sfd (gdb_mkstemp_cloexec (filename));
+  SELF_CHECK (sfd.get () >= 0);
+
+  unlink (filename);
+  
+  gdb_file_up file = sfd.to_file ("rw");
+  SELF_CHECK (file != nullptr);
+  SELF_CHECK (sfd.get () == -1);
+}
+
 /* Run selftests.  */
 static void
 run_tests ()
 {
   test_destroy ();
   test_release ();
+  test_to_file ();
 }
 
 } /* namespace scoped_fd */