[RFC,5/6] Do not reopen temporary files
Commit Message
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
> 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.
>>>>> "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
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
>>>>> "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
@@ -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;
@@ -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"));
}
@@ -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 */