From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 109531 invoked by alias); 31 Jul 2017 19:09:26 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 109497 invoked by uid 89); 31 Jul 2017 19:09:23 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-24.8 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 spammy=ssd, manages, ldt X-HELO: smtp.polymtl.ca Received: from smtp.polymtl.ca (HELO smtp.polymtl.ca) (132.207.4.11) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 31 Jul 2017 19:09:21 +0000 Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id v6VJ9EU6014127 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT) for ; Mon, 31 Jul 2017 15:09:19 -0400 Received: by simark.ca (Postfix, from userid 112) id C918F1EA01; Mon, 31 Jul 2017 15:09:14 -0400 (EDT) Received: from simark.ca (localhost [127.0.0.1]) by simark.ca (Postfix) with ESMTP id B36521E5B1; Mon, 31 Jul 2017 15:09:01 -0400 (EDT) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Date: Mon, 31 Jul 2017 19:09:00 -0000 From: Simon Marchi To: Tom Tromey Cc: gdb-patches@sourceware.org Subject: Re: [RFA v2 09/24] Remove close cleanup In-Reply-To: <20170725172107.9799-10-tom@tromey.com> References: <20170725172107.9799-1-tom@tromey.com> <20170725172107.9799-10-tom@tromey.com> Message-ID: <0f32812a9b3d7dc71fad1d28a5593bbe@polymtl.ca> X-Sender: simon.marchi@polymtl.ca User-Agent: Roundcube Webmail/1.3.0 X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Mon, 31 Jul 2017 19:09:15 +0000 X-IsSubscribed: yes X-SW-Source: 2017-07/txt/msg00452.txt.bz2 On 2017-07-25 19:20, Tom Tromey wrote: > make_cleanup_close calls close on a file descriptor. This patch > replaces it with a new RAII class, gdb::fd_closer. > > ChangeLog > 2017-07-25 Tom Tromey > > * source.c (get_filename_and_charpos, forward_search_command) > (reverse_search_command): Use fd_closer. > * procfs.c (load_syscalls, proc_get_LDT_entry) > (iterate_over_mappings): Use fd_closer. > * nto-procfs.c (procfs_open_1, procfs_pidlist): Use fd_closer. > * nat/linux-namespaces.c (linux_mntns_access_fs): Use fd_closer. > (close_saving_errno): New function. > * common/filestuff.h (gdb::fd_closer): New class. > * common/filestuff.c (do_close_cleanup, make_cleanup_close): > Remove. > --- > gdb/ChangeLog | 13 +++++++++++++ > gdb/common/filestuff.c | 21 --------------------- > gdb/common/filestuff.h | 41 > +++++++++++++++++++++++++++++++++++++++-- > gdb/nat/linux-namespaces.c | 43 > ++++++++++++++++++------------------------- > gdb/nto-procfs.c | 9 ++------- > gdb/procfs.c | 26 +++++++++----------------- > gdb/source.c | 13 ++++++------- > 7 files changed, 87 insertions(+), 79 deletions(-) > > diff --git a/gdb/ChangeLog b/gdb/ChangeLog > index 2bf549b..871b1f0 100644 > --- a/gdb/ChangeLog > +++ b/gdb/ChangeLog > @@ -1,5 +1,18 @@ > 2017-07-25 Tom Tromey > > + * source.c (get_filename_and_charpos, forward_search_command) > + (reverse_search_command): Use fd_closer. > + * procfs.c (load_syscalls, proc_get_LDT_entry) > + (iterate_over_mappings): Use fd_closer. > + * nto-procfs.c (procfs_open_1, procfs_pidlist): Use fd_closer. > + * nat/linux-namespaces.c (linux_mntns_access_fs): Use fd_closer. > + (close_saving_errno): New function. > + * common/filestuff.h (gdb::fd_closer): New class. > + * common/filestuff.c (do_close_cleanup, make_cleanup_close): > + Remove. > + > +2017-07-25 Tom Tromey > + > * compile/compile.c (cleanup_unlink_file): Remove. > (compile_to_object): Use gdb::unlinker. > (eval_compile_command): Likewise. > diff --git a/gdb/common/filestuff.c b/gdb/common/filestuff.c > index 4b05884..5a0dd65 100644 > --- a/gdb/common/filestuff.c > +++ b/gdb/common/filestuff.c > @@ -404,24 +404,3 @@ gdb_pipe_cloexec (int filedes[2]) > > return result; > } > - > -/* Helper function which does the work for make_cleanup_close. */ > - > -static void > -do_close_cleanup (void *arg) > -{ > - int *fd = (int *) arg; > - > - close (*fd); > -} > - > -/* See filestuff.h. */ > - > -struct cleanup * > -make_cleanup_close (int fd) > -{ > - int *saved_fd = XNEW (int); > - > - *saved_fd = fd; > - return make_cleanup_dtor (do_close_cleanup, saved_fd, xfree); > -} > diff --git a/gdb/common/filestuff.h b/gdb/common/filestuff.h > index 3cf2df6..ad34f27 100644 > --- a/gdb/common/filestuff.h > +++ b/gdb/common/filestuff.h > @@ -80,8 +80,45 @@ extern int gdb_socket_cloexec (int domain, int > style, int protocol); > > extern int gdb_pipe_cloexec (int filedes[2]); > > -/* Return a new cleanup that closes FD. */ > +namespace gdb > +{ > + > +/* A class that manages a file descriptor and closes it on > + destruction. This can be parameterized to use a different close > + function; the default is "close". */ > + > +template > +class fd_closer > +{ > +public: > + > + explicit fd_closer (int fd) > + : m_fd (fd) > + { > + } > + > + ~fd_closer () > + { > + if (m_fd != -1) > + CLOSER (m_fd); > + } > + > + fd_closer (const fd_closer &) = delete; > + fd_closer &operator= (const fd_closer &) = delete; > + > + /* Release the file descriptor to the caller. */ > + int release () > + { > + int result = m_fd; > + m_fd = -1; > + return result; > + } > + > +private: > + > + int m_fd; > +}; > > -extern struct cleanup *make_cleanup_close (int fd); > +} > > #endif /* FILESTUFF_H */ > diff --git a/gdb/nat/linux-namespaces.c b/gdb/nat/linux-namespaces.c > index 1df8f1b..fba4199 100644 > --- a/gdb/nat/linux-namespaces.c > +++ b/gdb/nat/linux-namespaces.c > @@ -20,6 +20,7 @@ > #include "common-defs.h" > #include "nat/linux-namespaces.h" > #include "filestuff.h" > +#include "scoped_restore.h" > #include > #include > #include > @@ -881,13 +882,21 @@ enum mnsh_fs_code > MNSH_FS_HELPER > }; > > +/* A function like "close" that saves and restores errno. */ > + > +static int > +close_saving_errno (int fd) > +{ > + scoped_restore save_errno = make_scoped_restore (&errno); > + return close (fd); > +} > + > /* Return a value indicating how the caller should access the > mount namespace of process PID. */ > > static enum mnsh_fs_code > linux_mntns_access_fs (pid_t pid) > { > - struct cleanup *old_chain; > struct linux_ns *ns; > struct stat sb; > struct linux_mnsh *helper; > @@ -901,27 +910,21 @@ linux_mntns_access_fs (pid_t pid) > if (ns == NULL) > return MNSH_FS_DIRECT; > > - old_chain = make_cleanup (null_cleanup, NULL); > - > fd = gdb_open_cloexec (linux_ns_filename (ns, pid), O_RDONLY, 0); > if (fd < 0) > - goto error; > + return MNSH_FS_ERROR; > > - make_cleanup_close (fd); > + gdb::fd_closer close_fd (fd); > > if (fstat (fd, &sb) != 0) > - goto error; > + return MNSH_FS_ERROR; > > if (sb.st_ino == ns->id) > - { > - do_cleanups (old_chain); > - > - return MNSH_FS_DIRECT; > - } > + return MNSH_FS_DIRECT; > > helper = linux_mntns_get_helper (); > if (helper == NULL) > - goto error; > + return MNSH_FS_ERROR; > > if (sb.st_ino != helper->nsid) > { > @@ -929,10 +932,10 @@ linux_mntns_access_fs (pid_t pid) > > size = mnsh_send_setns (helper, fd, 0); > if (size < 0) > - goto error; > + return MNSH_FS_ERROR; > > if (mnsh_recv_int (helper, &result, &error) != 0) > - goto error; > + return MNSH_FS_ERROR; > > if (result != 0) > { > @@ -945,23 +948,13 @@ linux_mntns_access_fs (pid_t pid) > error = ENOTSUP; > > errno = error; > - goto error; > + return MNSH_FS_ERROR; > } > > helper->nsid = sb.st_ino; > } > > - do_cleanups (old_chain); > - > return MNSH_FS_HELPER; > - > -error: > - saved_errno = errno; > - > - do_cleanups (old_chain); > - > - errno = saved_errno; > - return MNSH_FS_ERROR; > } > > /* See nat/linux-namespaces.h. */ > diff --git a/gdb/nto-procfs.c b/gdb/nto-procfs.c > index 7fb7095..63ef2ec 100644 > --- a/gdb/nto-procfs.c > +++ b/gdb/nto-procfs.c > @@ -115,7 +115,6 @@ procfs_open_1 (struct target_ops *ops, const char > *arg, int from_tty) > char buffer[50]; > int fd, total_size; > procfs_sysinfo *sysinfo; > - struct cleanup *cleanups; > char nto_procfs_path[PATH_MAX]; > > /* Offer to kill previous inferiors before opening this target. */ > @@ -165,7 +164,7 @@ procfs_open_1 (struct target_ops *ops, const char > *arg, int from_tty) > safe_strerror (errno)); > error (_("Invalid procfs arg")); > } > - cleanups = make_cleanup_close (fd); > + gdb::fd_closer close_fd (fd); > > sysinfo = (void *) buffer; > if (devctl (fd, DCMD_PROC_SYSINFO, sysinfo, sizeof buffer, 0) != > EOK) > @@ -201,7 +200,6 @@ procfs_open_1 (struct target_ops *ops, const char > *arg, int from_tty) > } > } > } > - do_cleanups (cleanups); > > inf_child_open_target (ops, arg, from_tty); > printf_filtered ("Debugging using %s\n", nto_procfs_path); > @@ -392,7 +390,6 @@ procfs_pidlist (char *args, int from_tty) > do > { > int fd; > - struct cleanup *inner_cleanup; > > /* Get the right pid and procfs path for the pid. */ > do > @@ -418,7 +415,7 @@ procfs_pidlist (char *args, int from_tty) > buf, errno, safe_strerror (errno)); > continue; > } > - inner_cleanup = make_cleanup_close (fd); > + gdb::fd_closer close_fd (fd); > > pidinfo = (procfs_info *) buf; > if (devctl (fd, DCMD_PROC_INFO, pidinfo, sizeof (buf), 0) != > EOK) > @@ -451,8 +448,6 @@ procfs_pidlist (char *args, int from_tty) > break; > } > } > - > - do_cleanups (inner_cleanup); > } > while (dirp != NULL); > > diff --git a/gdb/procfs.c b/gdb/procfs.c > index b03809c..a0d2270 100644 > --- a/gdb/procfs.c > +++ b/gdb/procfs.c > @@ -31,6 +31,7 @@ > #include "regcache.h" > #include "inf-child.h" > #include "filestuff.h" > +#include "common/filestuff.h" > > #if defined (NEW_PROC_API) > #define _STRUCTURED_PROC 1 /* Should be done by configure script. */ > @@ -883,7 +884,8 @@ load_syscalls (procinfo *pi) > { > error (_("load_syscalls: Can't open /proc/%d/sysent"), pi->pid); > } > - cleanups = make_cleanup_close (sysent_fd); > + > + gdb::fd_closer close_sysent_fd (sysent_fd); > > size = sizeof header - sizeof (prsyscall_t); > if (read (sysent_fd, &header, size) != size) > @@ -899,7 +901,7 @@ load_syscalls (procinfo *pi) > > size = header.pr_nsyscalls * sizeof (prsyscall_t); > syscalls = xmalloc (size); > - make_cleanup (free_current_contents, &syscalls); > + cleanups = make_cleanup (free_current_contents, &syscalls); > > if (read (sysent_fd, syscalls, size) != size) > error (_("load_syscalls: Error reading /proc/%d/sysent"), > pi->pid); > @@ -2484,7 +2486,6 @@ proc_get_LDT_entry (procinfo *pi, int key) > static struct ssd *ldt_entry = NULL; > #ifdef NEW_PROC_API > char pathname[MAX_PROC_NAME_SIZE]; > - struct cleanup *old_chain = NULL; > int fd; > > /* Allocate space for one LDT entry. > @@ -2499,8 +2500,8 @@ proc_get_LDT_entry (procinfo *pi, int key) > proc_warn (pi, "proc_get_LDT_entry (open)", __LINE__); > return NULL; > } > - /* Make sure it gets closed again! */ > - old_chain = make_cleanup_close (fd); > + > + gdb::fd_closer close_fd (fd); > > /* Now 'read' thru the table, find a match and return it. */ > while (read (fd, ldt_entry, sizeof (struct ssd)) == sizeof (struct > ssd)) > @@ -2512,13 +2513,9 @@ proc_get_LDT_entry (procinfo *pi, int key) > break; /* end of table */ > /* If key matches, return this entry. */ > if (ldt_entry->sel == key) > - { > - do_cleanups (old_chain); > - return ldt_entry; > - } > + return ldt_entry; > } > /* Loop ended, match not found. */ > - do_cleanups (old_chain); > return NULL; > #else > int nldt, i; > @@ -4917,7 +4914,6 @@ iterate_over_mappings (procinfo *pi, > find_memory_region_ftype child_func, > int funcstat; > int map_fd; > int nmap; > - struct cleanup *cleanups = make_cleanup (null_cleanup, NULL); > #ifdef NEW_PROC_API > struct stat sbuf; > #endif > @@ -4931,7 +4927,7 @@ iterate_over_mappings (procinfo *pi, > find_memory_region_ftype child_func, > proc_error (pi, "iterate_over_mappings (open)", __LINE__); > > /* Make sure it gets closed again. */ > - make_cleanup_close (map_fd); > + gdb::fd_closer close_map_fd (map_fd); > > /* Use stat to determine the file size, and compute > the number of prmap_t objects it contains. */ > @@ -4955,12 +4951,8 @@ iterate_over_mappings (procinfo *pi, > find_memory_region_ftype child_func, > > for (prmap = prmaps; nmap > 0; prmap++, nmap--) > if ((funcstat = (*func) (prmap, child_func, data)) != 0) > - { > - do_cleanups (cleanups); > - return funcstat; > - } > + return funcstat; > > - do_cleanups (cleanups); > return 0; > } > > diff --git a/gdb/source.c b/gdb/source.c > index 4cc862c..510f1c9 100644 > --- a/gdb/source.c > +++ b/gdb/source.c > @@ -1302,14 +1302,14 @@ get_filename_and_charpos (struct symtab *s, > char **fullname) > *fullname = NULL; > return 0; > } > - cleanups = make_cleanup_close (desc); > + > + gdb::fd_closer<> close_desc (desc); > if (fullname) > *fullname = s->fullname; > if (s->line_charpos == 0) > linenums_changed = 1; > if (linenums_changed) > find_source_lines (s, desc); > - do_cleanups (cleanups); > return linenums_changed; > } > > @@ -1627,7 +1627,6 @@ forward_search_command (char *regex, int > from_tty) > int desc; > int line; > char *msg; > - struct cleanup *cleanups; > > line = last_line_listed + 1; > > @@ -1641,7 +1640,7 @@ forward_search_command (char *regex, int > from_tty) > desc = open_source_file (current_source_symtab); > if (desc < 0) > perror_with_name (symtab_to_filename_for_display > (current_source_symtab)); > - cleanups = make_cleanup_close (desc); > + gdb::fd_closer<> close_desc (desc); > > if (current_source_symtab->line_charpos == 0) > find_source_lines (current_source_symtab, desc); > @@ -1652,7 +1651,7 @@ forward_search_command (char *regex, int > from_tty) > if (lseek (desc, current_source_symtab->line_charpos[line - 1], 0) < > 0) > perror_with_name (symtab_to_filename_for_display > (current_source_symtab)); > > - discard_cleanups (cleanups); > + close_desc.release (); > gdb_file_up stream (fdopen (desc, FDOPEN_MODE)); > clearerr (stream.get ()); > while (1) > @@ -1726,7 +1725,7 @@ reverse_search_command (char *regex, int > from_tty) > desc = open_source_file (current_source_symtab); > if (desc < 0) > perror_with_name (symtab_to_filename_for_display > (current_source_symtab)); > - cleanups = make_cleanup_close (desc); > + gdb::fd_closer<> close_desc (desc); > > if (current_source_symtab->line_charpos == 0) > find_source_lines (current_source_symtab, desc); > @@ -1737,7 +1736,7 @@ reverse_search_command (char *regex, int > from_tty) > if (lseek (desc, current_source_symtab->line_charpos[line - 1], 0) < > 0) > perror_with_name (symtab_to_filename_for_display > (current_source_symtab)); > > - discard_cleanups (cleanups); > + close_desc.release (); > gdb_file_up stream (fdopen (desc, FDOPEN_MODE)); > clearerr (stream.get ()); > while (line > 1) See response here: https://sourceware.org/ml/gdb-patches/2017-07/msg00451.html