* [PATCH 1/3] gdbsupport: change scoped_fd::to_file to a free function
@ 2021-07-22 19:37 Simon Marchi
2021-07-22 19:37 ` [PATCH 2/3] gdbsupport: make gdb_open_cloexec return scoped_fd Simon Marchi
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Simon Marchi @ 2021-07-22 19:37 UTC (permalink / raw)
To: gdb-patches
The following patch wants to chagne gdb_fopen_cloexec to return a
scoped_fd. Doing this causes a cyclic include between scoped_fd.h and
filestuff.h. scoped_fd.h includes filestuff.h because of the
scoped_fd::to_file method. To fix that, change scoped_fd::to_file to
be a free function in filestuff.h.
Change-Id: Ic82a48914b2aacee8f14af535b7469245f88b93d
---
gdb/dwarf2/index-write.c | 2 +-
gdb/source.c | 2 +-
gdb/unittests/scoped_fd-selftests.c | 2 +-
gdbsupport/filestuff.h | 17 +++++++++++++++++
gdbsupport/scoped_fd.h | 13 -------------
5 files changed, 20 insertions(+), 16 deletions(-)
diff --git a/gdb/dwarf2/index-write.c b/gdb/dwarf2/index-write.c
index 4e00c716d910..59a6aa4264d5 100644
--- a/gdb/dwarf2/index-write.c
+++ b/gdb/dwarf2/index-write.c
@@ -1544,7 +1544,7 @@ struct index_wip_file
if (out_file_fd.get () == -1)
perror_with_name (("mkstemp"));
- out_file = out_file_fd.to_file ("wb");
+ out_file = to_file (out_file_fd, "wb");
if (out_file == nullptr)
error (_("Can't open `%s' for writing"), filename_temp.data ());
diff --git a/gdb/source.c b/gdb/source.c
index 7d1934bd6c9b..0e3308048dc3 100644
--- a/gdb/source.c
+++ b/gdb/source.c
@@ -1620,7 +1620,7 @@ search_command_helper (const char *regex, int from_tty, bool forward)
if (lseek (desc.get (), (*offsets)[line - 1], 0) < 0)
perror_with_name (symtab_to_filename_for_display (loc->symtab ()));
- gdb_file_up stream = desc.to_file (FDOPEN_MODE);
+ gdb_file_up stream = to_file (desc, FDOPEN_MODE);
clearerr (stream.get ());
gdb::def_vector<char> buf;
diff --git a/gdb/unittests/scoped_fd-selftests.c b/gdb/unittests/scoped_fd-selftests.c
index d1803aaf8da2..1a361f00a2e3 100644
--- a/gdb/unittests/scoped_fd-selftests.c
+++ b/gdb/unittests/scoped_fd-selftests.c
@@ -76,7 +76,7 @@ test_to_file ()
unlink (filename);
- gdb_file_up file = sfd.to_file ("rw");
+ gdb_file_up file = to_file (sfd, "rw");
SELF_CHECK (file != nullptr);
SELF_CHECK (sfd.get () == -1);
}
diff --git a/gdbsupport/filestuff.h b/gdbsupport/filestuff.h
index 7f4cfb2388b1..658e1df4e558 100644
--- a/gdbsupport/filestuff.h
+++ b/gdbsupport/filestuff.h
@@ -21,6 +21,7 @@
#include <dirent.h>
#include <fcntl.h>
+#include "scoped_fd.h"
/* Note all the file descriptors which are open when this is called.
These file descriptors will not be closed by close_most_fds. */
@@ -96,6 +97,22 @@ gdb_fopen_cloexec (const std::string &filename, const char *opentype)
return gdb_fopen_cloexec (filename.c_str (), opentype);
}
+/* Converts FD into a gdb_file_up.
+
+ On success, the file descriptor owned by FD is released, meaning that FD no
+ longer contains nor owns it. */
+
+static inline
+gdb_file_up to_file (scoped_fd &fd, const char *mode) noexcept
+{
+ gdb_file_up result (fdopen (fd.get (), mode));
+
+ if (result != nullptr)
+ int unused ATTRIBUTE_UNUSED = fd.release ();
+
+ return result;
+}
+
/* Like 'socketpair', but ensures that the returned file descriptors
have the close-on-exec flag set. */
diff --git a/gdbsupport/scoped_fd.h b/gdbsupport/scoped_fd.h
index a1aad8493998..a571729c951c 100644
--- a/gdbsupport/scoped_fd.h
+++ b/gdbsupport/scoped_fd.h
@@ -21,7 +21,6 @@
#define COMMON_SCOPED_FD_H
#include <unistd.h>
-#include "filestuff.h"
/* A smart-pointer-like class to automatically close a file descriptor. */
@@ -63,18 +62,6 @@ class scoped_fd
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;
--
2.32.0
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 2/3] gdbsupport: make gdb_open_cloexec return scoped_fd
2021-07-22 19:37 [PATCH 1/3] gdbsupport: change scoped_fd::to_file to a free function Simon Marchi
@ 2021-07-22 19:37 ` Simon Marchi
2021-07-22 19:37 ` [PATCH 3/3] gdbsupport: make gdb_mkostemp_cloexec return a scoped_fd Simon Marchi
2021-07-30 13:47 ` [PATCH 1/3] gdbsupport: change scoped_fd::to_file to a free function Tom Tromey
2 siblings, 0 replies; 7+ messages in thread
From: Simon Marchi @ 2021-07-22 19:37 UTC (permalink / raw)
To: gdb-patches
Make gdb_open_cloexec return a scoped_fd, to encourage using automatic
management of the file descriptor closing. Except in the most trivial
cases, I changed the callers to just release the fd, which retains their
existing behavior. That will allow the transition to using scoped_fd
more to go gradually, one caller at a time.
Change-Id: Ife022b403f96e71d5ebb4f1056ef6251b30fe554
---
gdb/auxv.c | 14 ++++++--------
gdb/corelow.c | 2 +-
gdb/darwin-nat.c | 2 +-
gdb/gdb_bfd.c | 2 +-
gdb/inf-child.c | 2 +-
gdb/linux-nat.c | 2 +-
gdb/nat/linux-namespaces.c | 13 ++++---------
gdb/remote-fileio.c | 2 +-
gdb/ser-unix.c | 2 +-
gdb/solib.c | 7 ++++---
gdb/source.c | 15 +++++++--------
gdb/top.c | 8 +++-----
gdb/tracefile-tfile.c | 2 +-
gdbsupport/filestuff.cc | 8 ++++----
gdbsupport/filestuff.h | 6 +++---
gdbsupport/scoped_mmap.cc | 2 +-
16 files changed, 40 insertions(+), 49 deletions(-)
diff --git a/gdb/auxv.c b/gdb/auxv.c
index 2bcf9f452e3e..120e5c7cc21b 100644
--- a/gdb/auxv.c
+++ b/gdb/auxv.c
@@ -46,23 +46,21 @@ procfs_xfer_auxv (gdb_byte *readbuf,
ULONGEST len,
ULONGEST *xfered_len)
{
- int fd;
ssize_t l;
std::string pathname = string_printf ("/proc/%d/auxv", inferior_ptid.pid ());
- fd = gdb_open_cloexec (pathname, writebuf != NULL ? O_WRONLY : O_RDONLY, 0);
- if (fd < 0)
+ scoped_fd fd
+ = gdb_open_cloexec (pathname, writebuf != NULL ? O_WRONLY : O_RDONLY, 0);
+ if (fd.get () < 0)
return TARGET_XFER_E_IO;
if (offset != (ULONGEST) 0
- && lseek (fd, (off_t) offset, SEEK_SET) != (off_t) offset)
+ && lseek (fd.get (), (off_t) offset, SEEK_SET) != (off_t) offset)
l = -1;
else if (readbuf != NULL)
- l = read (fd, readbuf, (size_t) len);
+ l = read (fd.get (), readbuf, (size_t) len);
else
- l = write (fd, writebuf, (size_t) len);
-
- (void) close (fd);
+ l = write (fd.get (), writebuf, (size_t) len);
if (l < 0)
return TARGET_XFER_E_IO;
diff --git a/gdb/corelow.c b/gdb/corelow.c
index eb785a08633a..b2a40b1ed5c8 100644
--- a/gdb/corelow.c
+++ b/gdb/corelow.c
@@ -436,7 +436,7 @@ core_target_open (const char *arg, int from_tty)
flags |= O_RDWR;
else
flags |= O_RDONLY;
- scratch_chan = gdb_open_cloexec (filename.get (), flags, 0);
+ scratch_chan = gdb_open_cloexec (filename.get (), flags, 0).release ();
if (scratch_chan < 0)
perror_with_name (filename.get ());
diff --git a/gdb/darwin-nat.c b/gdb/darwin-nat.c
index a6790792fb65..eb3b6fa8ef4c 100644
--- a/gdb/darwin-nat.c
+++ b/gdb/darwin-nat.c
@@ -1822,7 +1822,7 @@ may_have_sip ()
static void
copy_shell_to_cache (const char *shell, const std::string &new_name)
{
- scoped_fd from_fd (gdb_open_cloexec (shell, O_RDONLY, 0));
+ scoped_fd from_fd = gdb_open_cloexec (shell, O_RDONLY, 0);
if (from_fd.get () < 0)
error (_("Could not open shell (%s) for reading: %s"),
shell, safe_strerror (errno));
diff --git a/gdb/gdb_bfd.c b/gdb/gdb_bfd.c
index 312442a466e2..c6ff409d49c6 100644
--- a/gdb/gdb_bfd.c
+++ b/gdb/gdb_bfd.c
@@ -541,7 +541,7 @@ gdb_bfd_open (const char *name, const char *target, int fd,
if (fd == -1)
{
- fd = gdb_open_cloexec (name, O_RDONLY | O_BINARY, 0);
+ fd = gdb_open_cloexec (name, O_RDONLY | O_BINARY, 0).release ();
if (fd == -1)
{
bfd_set_error (bfd_error_system_call);
diff --git a/gdb/inf-child.c b/gdb/inf-child.c
index f690aa77913f..5084f448c1e0 100644
--- a/gdb/inf-child.c
+++ b/gdb/inf-child.c
@@ -261,7 +261,7 @@ inf_child_target::fileio_open (struct inferior *inf, const char *filename,
return -1;
}
- fd = gdb_open_cloexec (filename, nat_flags, nat_mode);
+ fd = gdb_open_cloexec (filename, nat_flags, nat_mode).release ();
if (fd == -1)
*target_errno = host_to_fileio_error (errno);
diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index e4d0206eaac8..e4939fb65cbd 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -3848,7 +3848,7 @@ linux_proc_xfer_memory_partial_pid (ptid_t ptid,
"/proc/%d/task/%ld/mem", ptid.pid (), ptid.lwp ());
last_proc_mem_file.fd
- = gdb_open_cloexec (filename, O_RDWR | O_LARGEFILE, 0);
+ = gdb_open_cloexec (filename, O_RDWR | O_LARGEFILE, 0).release ();
if (last_proc_mem_file.fd == -1)
{
diff --git a/gdb/nat/linux-namespaces.c b/gdb/nat/linux-namespaces.c
index 0c4fadd49f7b..c5d5eb4ea8ce 100644
--- a/gdb/nat/linux-namespaces.c
+++ b/gdb/nat/linux-namespaces.c
@@ -520,13 +520,8 @@ static ssize_t
mnsh_handle_open (int sock, const char *filename,
int flags, mode_t mode)
{
- int fd = gdb_open_cloexec (filename, flags, mode);
- ssize_t result = mnsh_return_fd (sock, fd, errno);
-
- if (fd >= 0)
- close (fd);
-
- return result;
+ scoped_fd fd = gdb_open_cloexec (filename, flags, mode);
+ return mnsh_return_fd (sock, fd.get (), errno);
}
/* Handle a MNSH_REQ_UNLINK message. Must be async-signal-safe. */
@@ -901,7 +896,7 @@ linux_mntns_access_fs (pid_t pid)
if (ns == NULL)
return MNSH_FS_DIRECT;
- fd = gdb_open_cloexec (linux_ns_filename (ns, pid), O_RDONLY, 0);
+ fd = gdb_open_cloexec (linux_ns_filename (ns, pid), O_RDONLY, 0).release ();
if (fd < 0)
return MNSH_FS_ERROR;
@@ -968,7 +963,7 @@ linux_mntns_open_cloexec (pid_t pid, const char *filename,
return -1;
if (access == MNSH_FS_DIRECT)
- return gdb_open_cloexec (filename, flags, mode);
+ return gdb_open_cloexec (filename, flags, mode).release ();
gdb_assert (access == MNSH_FS_HELPER);
diff --git a/gdb/remote-fileio.c b/gdb/remote-fileio.c
index 9765093a7235..20ec0f215aae 100644
--- a/gdb/remote-fileio.c
+++ b/gdb/remote-fileio.c
@@ -425,7 +425,7 @@ remote_fileio_func_open (remote_target *remote, char *buf)
}
}
- fd = gdb_open_cloexec (pathname, flags, mode);
+ fd = gdb_open_cloexec (pathname, flags, mode).release ();
if (fd < 0)
{
remote_fileio_return_errno (remote, -1);
diff --git a/gdb/ser-unix.c b/gdb/ser-unix.c
index 96d024eea3d9..597032afe896 100644
--- a/gdb/ser-unix.c
+++ b/gdb/ser-unix.c
@@ -75,7 +75,7 @@ static int hardwire_setstopbits (struct serial *, int);
static int
hardwire_open (struct serial *scb, const char *name)
{
- scb->fd = gdb_open_cloexec (name, O_RDWR, 0);
+ scb->fd = gdb_open_cloexec (name, O_RDWR, 0).release ();
if (scb->fd < 0)
return -1;
diff --git a/gdb/solib.c b/gdb/solib.c
index 317f7eb485e2..a89ce6831e73 100644
--- a/gdb/solib.c
+++ b/gdb/solib.c
@@ -255,7 +255,8 @@ solib_find_1 (const char *in_pathname, int *fd, bool is_solib)
}
/* Now see if we can open it. */
- found_file = gdb_open_cloexec (temp_pathname.get (), O_RDONLY | O_BINARY, 0);
+ found_file = gdb_open_cloexec (temp_pathname.get (),
+ O_RDONLY | O_BINARY, 0).release ();
/* If the search in gdb_sysroot failed, and the path name has a
drive spec (e.g, c:/foo), try stripping ':' from the drive spec,
@@ -276,7 +277,7 @@ solib_find_1 (const char *in_pathname, int *fd, bool is_solib)
in_pathname + 2, (char *) NULL));
found_file = gdb_open_cloexec (temp_pathname.get (),
- O_RDONLY | O_BINARY, 0);
+ O_RDONLY | O_BINARY, 0).release ();
if (found_file < 0)
{
/* If the search in gdb_sysroot still failed, try fully
@@ -290,7 +291,7 @@ solib_find_1 (const char *in_pathname, int *fd, bool is_solib)
in_pathname + 2, (char *) NULL));
found_file = gdb_open_cloexec (temp_pathname.get (),
- O_RDONLY | O_BINARY, 0);
+ O_RDONLY | O_BINARY, 0).release ();
}
}
diff --git a/gdb/source.c b/gdb/source.c
index 0e3308048dc3..c57e96d49be7 100644
--- a/gdb/source.c
+++ b/gdb/source.c
@@ -818,7 +818,7 @@ openp (const char *path, openp_flags opts, const char *string,
{
filename = (char *) alloca (strlen (string) + 1);
strcpy (filename, string);
- fd = gdb_open_cloexec (filename, mode, 0);
+ fd = gdb_open_cloexec (filename, mode, 0).release ();
if (fd >= 0)
goto done;
last_errno = errno;
@@ -910,7 +910,7 @@ openp (const char *path, openp_flags opts, const char *string,
if (is_regular_file (filename, ®_file_errno))
{
- fd = gdb_open_cloexec (filename, mode, 0);
+ fd = gdb_open_cloexec (filename, mode, 0).release ();
if (fd >= 0)
break;
last_errno = errno;
@@ -1046,7 +1046,6 @@ find_and_open_source (const char *filename,
{
char *path = source_path;
const char *p;
- int result;
/* Quick way out if we already know its full name. */
@@ -1061,11 +1060,11 @@ find_and_open_source (const char *filename,
if (rewritten_fullname != NULL)
*fullname = std::move (rewritten_fullname);
- result = gdb_open_cloexec (fullname->get (), OPEN_MODE, 0);
- if (result >= 0)
+ scoped_fd result = gdb_open_cloexec (fullname->get (), OPEN_MODE, 0);
+ if (result.get () >= 0)
{
*fullname = gdb_realpath (fullname->get ());
- return scoped_fd (result);
+ return result;
}
/* Didn't work -- free old one, try again. */
@@ -1109,8 +1108,8 @@ find_and_open_source (const char *filename,
filename = rewritten_filename.get ();
/* Try to locate file using filename. */
- result = openp (path, OPF_SEARCH_IN_PATH | OPF_RETURN_REALPATH, filename,
- OPEN_MODE, fullname);
+ int result = openp (path, OPF_SEARCH_IN_PATH | OPF_RETURN_REALPATH, filename,
+ OPEN_MODE, fullname);
if (result < 0 && dirname != NULL)
{
/* Remove characters from the start of PATH that we don't need when
diff --git a/gdb/top.c b/gdb/top.c
index 6e0f43d2fd92..e2b00d1242a5 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -335,13 +335,11 @@ ui::~ui ()
static gdb_file_up
open_terminal_stream (const char *name)
{
- int fd;
-
- fd = gdb_open_cloexec (name, O_RDWR | O_NOCTTY, 0);
- if (fd < 0)
+ scoped_fd fd = gdb_open_cloexec (name, O_RDWR | O_NOCTTY, 0);
+ if (fd.get () < 0)
perror_with_name (_("opening terminal failed"));
- return gdb_file_up (fdopen (fd, "w+"));
+ return to_file (fd, "w+");
}
/* Implementation of the "new-ui" command. */
diff --git a/gdb/tracefile-tfile.c b/gdb/tracefile-tfile.c
index 33ce86bbe238..e1534826c5fe 100644
--- a/gdb/tracefile-tfile.c
+++ b/gdb/tracefile-tfile.c
@@ -475,7 +475,7 @@ tfile_target_open (const char *arg, int from_tty)
flags = O_BINARY | O_LARGEFILE;
flags |= O_RDONLY;
- scratch_chan = gdb_open_cloexec (filename.get (), flags, 0);
+ scratch_chan = gdb_open_cloexec (filename.get (), flags, 0).release ();
if (scratch_chan < 0)
perror_with_name (filename.get ());
diff --git a/gdbsupport/filestuff.cc b/gdbsupport/filestuff.cc
index 6ea2ad842d5d..2975a0e6a990 100644
--- a/gdbsupport/filestuff.cc
+++ b/gdbsupport/filestuff.cc
@@ -306,13 +306,13 @@ socket_mark_cloexec (int fd)
/* See filestuff.h. */
-int
+scoped_fd
gdb_open_cloexec (const char *filename, int flags, unsigned long mode)
{
- int fd = open (filename, flags | O_CLOEXEC, mode);
+ scoped_fd fd (open (filename, flags | O_CLOEXEC, mode));
- if (fd >= 0)
- maybe_mark_cloexec (fd);
+ if (fd.get () >= 0)
+ maybe_mark_cloexec (fd.get ());
return fd;
}
diff --git a/gdbsupport/filestuff.h b/gdbsupport/filestuff.h
index 658e1df4e558..a75312544191 100644
--- a/gdbsupport/filestuff.h
+++ b/gdbsupport/filestuff.h
@@ -47,8 +47,8 @@ extern void close_most_fds (void);
/* Like 'open', but ensures that the returned file descriptor has the
close-on-exec flag set. */
-extern int gdb_open_cloexec (const char *filename, int flags,
- /* mode_t */ unsigned long mode);
+extern scoped_fd gdb_open_cloexec (const char *filename, int flags,
+ /* mode_t */ unsigned long mode);
/* Like mkstemp, but ensures that the file descriptor is
close-on-exec. */
@@ -63,7 +63,7 @@ gdb_mkostemp_cloexec (char *name_template, int flags = 0)
/* Convenience wrapper for the above, which takes the filename as an
std::string. */
-static inline int
+static inline scoped_fd
gdb_open_cloexec (const std::string &filename, int flags,
/* mode_t */ unsigned long mode)
{
diff --git a/gdbsupport/scoped_mmap.cc b/gdbsupport/scoped_mmap.cc
index c76fb77b72d3..598b3280381f 100644
--- a/gdbsupport/scoped_mmap.cc
+++ b/gdbsupport/scoped_mmap.cc
@@ -27,7 +27,7 @@
scoped_mmap
mmap_file (const char *filename)
{
- scoped_fd fd (gdb_open_cloexec (filename, O_RDONLY, 0));
+ scoped_fd fd = gdb_open_cloexec (filename, O_RDONLY, 0);
if (fd.get () < 0)
perror_with_name (("open"));
--
2.32.0
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 3/3] gdbsupport: make gdb_mkostemp_cloexec return a scoped_fd
2021-07-22 19:37 [PATCH 1/3] gdbsupport: change scoped_fd::to_file to a free function Simon Marchi
2021-07-22 19:37 ` [PATCH 2/3] gdbsupport: make gdb_open_cloexec return scoped_fd Simon Marchi
@ 2021-07-22 19:37 ` Simon Marchi
2021-07-30 13:47 ` [PATCH 1/3] gdbsupport: change scoped_fd::to_file to a free function Tom Tromey
2 siblings, 0 replies; 7+ messages in thread
From: Simon Marchi @ 2021-07-22 19:37 UTC (permalink / raw)
To: gdb-patches
This encourages the callers to use automatic file descriptor management.
Change-Id: I137a81df6f3607b457e28c35aafde8ed6f3a3344
---
gdb/darwin-nat.c | 2 +-
gdb/dwarf2/index-write.c | 4 ++--
gdb/unittests/scoped_fd-selftests.c | 6 +++---
gdb/unittests/scoped_mmap-selftests.c | 9 +++++----
gdbsupport/filestuff.h | 4 ++--
5 files changed, 13 insertions(+), 12 deletions(-)
diff --git a/gdb/darwin-nat.c b/gdb/darwin-nat.c
index eb3b6fa8ef4c..a90ba8675b3c 100644
--- a/gdb/darwin-nat.c
+++ b/gdb/darwin-nat.c
@@ -1833,7 +1833,7 @@ copy_shell_to_cache (const char *shell, const std::string &new_name)
new_dir.c_str (), safe_strerror (errno));
gdb::char_vector temp_name = make_temp_filename (new_name);
- scoped_fd to_fd (gdb_mkostemp_cloexec (&temp_name[0]));
+ scoped_fd to_fd = gdb_mkostemp_cloexec (&temp_name[0]);
gdb::unlinker unlink_file_on_error (temp_name.data ());
if (to_fd.get () < 0)
diff --git a/gdb/dwarf2/index-write.c b/gdb/dwarf2/index-write.c
index 59a6aa4264d5..6cec5c25ef42 100644
--- a/gdb/dwarf2/index-write.c
+++ b/gdb/dwarf2/index-write.c
@@ -1539,8 +1539,8 @@ struct index_wip_file
filename_temp = make_temp_filename (filename);
- scoped_fd out_file_fd (gdb_mkostemp_cloexec (filename_temp.data (),
- O_BINARY));
+ scoped_fd out_file_fd = gdb_mkostemp_cloexec (filename_temp.data (),
+ O_BINARY);
if (out_file_fd.get () == -1)
perror_with_name (("mkstemp"));
diff --git a/gdb/unittests/scoped_fd-selftests.c b/gdb/unittests/scoped_fd-selftests.c
index 1a361f00a2e3..5aa0e4e7a198 100644
--- a/gdb/unittests/scoped_fd-selftests.c
+++ b/gdb/unittests/scoped_fd-selftests.c
@@ -32,7 +32,7 @@ static void
test_destroy ()
{
char filename[] = "scoped_fd-selftest-XXXXXX";
- int fd = gdb_mkostemp_cloexec (filename);
+ int fd = gdb_mkostemp_cloexec (filename).release ();
SELF_CHECK (fd >= 0);
unlink (filename);
@@ -51,7 +51,7 @@ static void
test_release ()
{
char filename[] = "scoped_fd-selftest-XXXXXX";
- int fd = gdb_mkostemp_cloexec (filename);
+ int fd = gdb_mkostemp_cloexec (filename).release ();
SELF_CHECK (fd >= 0);
unlink (filename);
@@ -71,7 +71,7 @@ test_to_file ()
{
char filename[] = "scoped_fd-selftest-XXXXXX";
- ::scoped_fd sfd (gdb_mkostemp_cloexec (filename));
+ ::scoped_fd sfd = gdb_mkostemp_cloexec (filename);
SELF_CHECK (sfd.get () >= 0);
unlink (filename);
diff --git a/gdb/unittests/scoped_mmap-selftests.c b/gdb/unittests/scoped_mmap-selftests.c
index 92a821d14b31..76d6c417ad05 100644
--- a/gdb/unittests/scoped_mmap-selftests.c
+++ b/gdb/unittests/scoped_mmap-selftests.c
@@ -89,11 +89,12 @@ static void
test_normal ()
{
char filename[] = "scoped_mmapped_file-selftest-XXXXXX";
- int fd = gdb_mkostemp_cloexec (filename);
- SELF_CHECK (fd >= 0);
+ {
+ scoped_fd fd = gdb_mkostemp_cloexec (filename);
+ SELF_CHECK (fd.get () >= 0);
- SELF_CHECK (write (fd, "Hello!", 7) == 7);
- close (fd);
+ SELF_CHECK (write (fd.get (), "Hello!", 7) == 7);
+ }
gdb::unlinker unlink_test_file (filename);
diff --git a/gdbsupport/filestuff.h b/gdbsupport/filestuff.h
index a75312544191..de71ec90647c 100644
--- a/gdbsupport/filestuff.h
+++ b/gdbsupport/filestuff.h
@@ -53,11 +53,11 @@ extern scoped_fd gdb_open_cloexec (const char *filename, int flags,
/* Like mkstemp, but ensures that the file descriptor is
close-on-exec. */
-static inline int
+static inline scoped_fd
gdb_mkostemp_cloexec (char *name_template, int flags = 0)
{
/* gnulib provides a mkostemp replacement if needed. */
- return mkostemp (name_template, flags | O_CLOEXEC);
+ return scoped_fd (mkostemp (name_template, flags | O_CLOEXEC));
}
/* Convenience wrapper for the above, which takes the filename as an
--
2.32.0
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/3] gdbsupport: change scoped_fd::to_file to a free function
2021-07-22 19:37 [PATCH 1/3] gdbsupport: change scoped_fd::to_file to a free function Simon Marchi
2021-07-22 19:37 ` [PATCH 2/3] gdbsupport: make gdb_open_cloexec return scoped_fd Simon Marchi
2021-07-22 19:37 ` [PATCH 3/3] gdbsupport: make gdb_mkostemp_cloexec return a scoped_fd Simon Marchi
@ 2021-07-30 13:47 ` Tom Tromey
2021-09-30 16:17 ` Simon Marchi
2 siblings, 1 reply; 7+ messages in thread
From: Tom Tromey @ 2021-07-30 13:47 UTC (permalink / raw)
To: Simon Marchi via Gdb-patches
Simon> The following patch wants to chagne gdb_fopen_cloexec to return a
"change"
Also I think that should be gdb_mkostemp_cloexec?
Simon> scoped_fd. Doing this causes a cyclic include between scoped_fd.h and
Simon> filestuff.h. scoped_fd.h includes filestuff.h because of the
Simon> scoped_fd::to_file method. To fix that, change scoped_fd::to_file to
Simon> be a free function in filestuff.h.
I guess that, while I don't really object, it seems like it would be
better to break the dependency some other way. For instance would
introducing a new header that only declares gdb_file_up work? The idea
behind this is not to let the oddities of C++ header file management
influence how the code itself is spelled -- that seems like a kind of
inversion to me. Going along with this is the idea that a method is the
clearest way to write to_file. These are all opinions of course, maybe
there's a reason to prefer a free function.
Tom
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/3] gdbsupport: change scoped_fd::to_file to a free function
2021-07-30 13:47 ` [PATCH 1/3] gdbsupport: change scoped_fd::to_file to a free function Tom Tromey
@ 2021-09-30 16:17 ` Simon Marchi
2021-09-30 17:48 ` Tom Tromey
0 siblings, 1 reply; 7+ messages in thread
From: Simon Marchi @ 2021-09-30 16:17 UTC (permalink / raw)
To: Tom Tromey, Simon Marchi via Gdb-patches
On 2021-07-30 09:47, Tom Tromey wrote:
> Simon> The following patch wants to chagne gdb_fopen_cloexec to return a
>
> "change"
> Also I think that should be gdb_mkostemp_cloexec?
Both, actually.
>
> Simon> scoped_fd. Doing this causes a cyclic include between scoped_fd.h and
> Simon> filestuff.h. scoped_fd.h includes filestuff.h because of the
> Simon> scoped_fd::to_file method. To fix that, change scoped_fd::to_file to
> Simon> be a free function in filestuff.h.
>
> I guess that, while I don't really object, it seems like it would be
> better to break the dependency some other way. For instance would
> introducing a new header that only declares gdb_file_up work? The idea
> behind this is not to let the oddities of C++ header file management
> influence how the code itself is spelled -- that seems like a kind of
> inversion to me. Going along with this is the idea that a method is the
> clearest way to write to_file. These are all opinions of course, maybe
> there's a reason to prefer a free function.
There's three way to do this:
- scoped_fd::to_file method
- gdb_file::from_fd static method
- a free function
I never know which is best, which object should know about the other
one, or if no object should no about the other (and hence use a free
function).
But I agree with you here. See the updated 1/3 patch below.
Patch 2 changes trivially to update the includes and a to_file call.
From 7a281de194629ba00546c2d34cc41d2f77ccc5b3 Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@polymtl.ca>
Date: Thu, 22 Jul 2021 11:34:57 -0400
Subject: [PATCH] gdbsupport: move gdb_file_up to its own file
The following patches wants to change gdb_fopen_cloexec and
gdb_mkostemp_cloexec to return a scoped_fd. Doing this causes a cyclic
include between scoped_fd.h and filestuff.h, that both want to include
each other. scoped_fd.h includes filestuff.h because of the
scoped_fd::to_file method's return value. filestuff.h would then
include scoped_fd.h for gdb_fopen_cloexec's and gdb_mkostemp_cloexec's
return values.
To fix that, move gdb_file_up to its own file, gdb_file.h.
Change-Id: Ic82a48914b2aacee8f14af535b7469245f88b93d
---
gdbsupport/filestuff.h | 13 +------------
gdbsupport/gdb_file.h | 37 +++++++++++++++++++++++++++++++++++++
gdbsupport/scoped_fd.h | 2 +-
3 files changed, 39 insertions(+), 13 deletions(-)
create mode 100644 gdbsupport/gdb_file.h
diff --git a/gdbsupport/filestuff.h b/gdbsupport/filestuff.h
index 7f4cfb2388b1..aa15f89aa920 100644
--- a/gdbsupport/filestuff.h
+++ b/gdbsupport/filestuff.h
@@ -21,6 +21,7 @@
#include <dirent.h>
#include <fcntl.h>
+#include "gdb_file.h"
/* Note all the file descriptors which are open when this is called.
These file descriptors will not be closed by close_most_fds. */
@@ -69,18 +70,6 @@ gdb_open_cloexec (const std::string &filename, int flags,
return gdb_open_cloexec (filename.c_str (), flags, mode);
}
-struct gdb_file_deleter
-{
- void operator() (FILE *file) const
- {
- fclose (file);
- }
-};
-
-/* A unique pointer to a FILE. */
-
-typedef std::unique_ptr<FILE, gdb_file_deleter> gdb_file_up;
-
/* Like 'fopen', but ensures that the returned file descriptor has the
close-on-exec flag set. */
diff --git a/gdbsupport/gdb_file.h b/gdbsupport/gdb_file.h
new file mode 100644
index 000000000000..2f5b399f6e87
--- /dev/null
+++ b/gdbsupport/gdb_file.h
@@ -0,0 +1,37 @@
+/* gdb_file_up, an RAII wrapper around FILE.
+ Copyright (C) 2021 Free Software Foundation, Inc.
+
+ This file is part of GDB.
+
+ This program is free software; you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation; either version 3 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with this program. If not, see <http://www.gnu.org/licenses/>. */
+
+#ifndef GDBSUPPORT_GDB_FILE
+#define GDBSUPPORT_GDB_FILE
+
+#include <memory>
+#include <stdio.h>
+
+struct gdb_file_deleter
+{
+ void operator() (FILE *file) const
+ {
+ fclose (file);
+ }
+};
+
+/* A unique pointer to a FILE. */
+
+typedef std::unique_ptr<FILE, gdb_file_deleter> gdb_file_up;
+
+#endif
diff --git a/gdbsupport/scoped_fd.h b/gdbsupport/scoped_fd.h
index a1aad8493998..f16e811a2cef 100644
--- a/gdbsupport/scoped_fd.h
+++ b/gdbsupport/scoped_fd.h
@@ -21,7 +21,7 @@
#define COMMON_SCOPED_FD_H
#include <unistd.h>
-#include "filestuff.h"
+#include "gdb_file.h"
/* A smart-pointer-like class to automatically close a file descriptor. */
--
2.33.0
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/3] gdbsupport: change scoped_fd::to_file to a free function
2021-09-30 16:17 ` Simon Marchi
@ 2021-09-30 17:48 ` Tom Tromey
2021-09-30 19:29 ` Simon Marchi
0 siblings, 1 reply; 7+ messages in thread
From: Tom Tromey @ 2021-09-30 17:48 UTC (permalink / raw)
To: Simon Marchi via Gdb-patches; +Cc: Tom Tromey, Simon Marchi
>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
Simon> But I agree with you here. See the updated 1/3 patch below.
Looks good to me, thank you.
Tom
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/3] gdbsupport: change scoped_fd::to_file to a free function
2021-09-30 17:48 ` Tom Tromey
@ 2021-09-30 19:29 ` Simon Marchi
0 siblings, 0 replies; 7+ messages in thread
From: Simon Marchi @ 2021-09-30 19:29 UTC (permalink / raw)
To: Tom Tromey, Simon Marchi via Gdb-patches
On 2021-09-30 13:48, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
>
> Simon> But I agree with you here. See the updated 1/3 patch below.
>
> Looks good to me, thank you.
>
> Tom
>
Thanks, pushed.
Simon
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-09-30 19:29 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-22 19:37 [PATCH 1/3] gdbsupport: change scoped_fd::to_file to a free function Simon Marchi
2021-07-22 19:37 ` [PATCH 2/3] gdbsupport: make gdb_open_cloexec return scoped_fd Simon Marchi
2021-07-22 19:37 ` [PATCH 3/3] gdbsupport: make gdb_mkostemp_cloexec return a scoped_fd Simon Marchi
2021-07-30 13:47 ` [PATCH 1/3] gdbsupport: change scoped_fd::to_file to a free function Tom Tromey
2021-09-30 16:17 ` Simon Marchi
2021-09-30 17:48 ` Tom Tromey
2021-09-30 19:29 ` Simon Marchi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).