From 36033ef57cd048588f9a3d5523712147066421f2 Mon Sep 17 00:00:00 2001 From: Tom Tromey Date: Thu, 20 Sep 2018 16:30:47 -0600 Subject: [PATCH] Do not reopen temporary files 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-10-27 Tom Tromey * 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 | 56 ++++++++++++++--------------- gdb/unittests/scoped_fd-selftests.c | 17 +++++++++ 4 files changed, 65 insertions(+), 29 deletions(-) diff --git a/gdb/ChangeLog b/gdb/ChangeLog index c6a563573e..346d9c6c52 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,11 @@ +2018-10-27 Tom Tromey + + * 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. + 2018-10-27 Tom Tromey * unittests/scoped_mmap-selftests.c (test_normal): Use diff --git a/gdb/common/scoped_fd.h b/gdb/common/scoped_fd.h index c2125bd1af..d20e18a2c0 100644 --- a/gdb/common/scoped_fd.h +++ b/gdb/common/scoped_fd.h @@ -21,6 +21,7 @@ #define SCOPED_FD_H #include +#include "filestuff.h" /* A smart-pointer-like class to automatically close a file descriptor. */ @@ -43,6 +44,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 e07bda9c08..8a4c1c7ea4 100644 --- a/gdb/dwarf-index-write.c +++ b/gdb/dwarf-index-write.c @@ -1566,23 +1566,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 out_file_fd - (gdb::in_place, gdb_mkostemp_cloexec (filename_temp.data (), O_BINARY)); - 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. So, we wrap the unlinker in an + optional and emplace it once we know the file name. */ + gdb::optional unlink_file; + scoped_fd out_file_fd (gdb_mkostemp_cloexec (filename_temp.data (), + O_BINARY)); + 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) { @@ -1590,45 +1588,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 out_file_str_fd - (gdb::in_place, gdb_mkostemp_cloexec (filename_str_temp.data (), - O_BINARY)); - if (out_file_str_fd->get () == -1) + /* As above, arrange to unlink the file only after the file + descriptor has been closed. */ + gdb::optional unlink_file_str; + scoped_fd out_file_str_fd + (gdb_mkostemp_cloexec (filename_str_temp.data (), O_BINARY)); + 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 fb6a0d675d..6a9c727477 100644 --- a/gdb/unittests/scoped_fd-selftests.c +++ b/gdb/unittests/scoped_fd-selftests.c @@ -65,12 +65,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_mkostemp_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 */