* [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).