From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 47891 invoked by alias); 19 Oct 2018 23:27:44 -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 47874 invoked by uid 89); 19 Oct 2018 23:27:44 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=237A, 7628, 0287, 31F4 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 19 Oct 2018 23:27:42 +0000 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 2E217309EFFC; Fri, 19 Oct 2018 23:27:41 +0000 (UTC) Received: from localhost (unused-10-15-17-196.yyz.redhat.com [10.15.17.196]) by smtp.corp.redhat.com (Postfix) with ESMTP id 024705D77E; Fri, 19 Oct 2018 23:27:40 +0000 (UTC) From: Sergio Durigan Junior To: Tom Tromey Cc: gdb-patches@sourceware.org Subject: Re: [PATCH v2 1/6] Unify shell-finding logic References: <20181018223100.20693-1-tom@tromey.com> <20181018223100.20693-2-tom@tromey.com> Date: Fri, 19 Oct 2018 23:27:00 -0000 In-Reply-To: <20181018223100.20693-2-tom@tromey.com> (Tom Tromey's message of "Thu, 18 Oct 2018 16:30:55 -0600") Message-ID: <87tvlhv7o3.fsf@redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes X-SW-Source: 2018-10/txt/msg00444.txt.bz2 On Thursday, October 18 2018, Tom Tromey wrote: > I noticed several places in gdb that were using getenv("SHELL") and > then falling back to "/bin/sh" if it returned NULL. This unifies > these into a single function. Not sure if I reviewed this one before, but it LGTM (with a very small nit below). It also can be pushed independently, IMO. Thanks, > gdb/ChangeLog > 2018-10-18 Tom Tromey > > * procfs.c (procfs_target::create_inferior): Use get_shell. > * cli/cli-cmds.c (shell_escape): Use get_shell. > * windows-nat.c (windows_nat_target::create_inferior): Use > get_shell. > * common/pathstuff.c (get_shell): New function. > * nat/fork-inferior.c (SHELL_FILE, get_startup_shell): Remove. > (fork_inferior): Use get_shell. > * common/pathstuff.h (get_shell): Declare. > --- > gdb/ChangeLog | 11 +++++++++++ > gdb/cli/cli-cmds.c | 6 ++---- > gdb/common/pathstuff.c | 12 ++++++++++++ > gdb/common/pathstuff.h | 4 ++++ > gdb/nat/fork-inferior.c | 21 ++------------------- > gdb/procfs.c | 4 ++-- > gdb/windows-nat.c | 5 ++--- > 7 files changed, 35 insertions(+), 28 deletions(-) > > diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c > index b871e476d3..135f550b80 100644 > --- a/gdb/cli/cli-cmds.c > +++ b/gdb/cli/cli-cmds.c > @@ -50,6 +50,7 @@ > #include "cli/cli-utils.h" > > #include "extension.h" > +#include "common/pathstuff.h" > > #ifdef TUI > #include "tui/tui.h" /* For tui_active et.al. */ > @@ -726,13 +727,10 @@ shell_escape (const char *arg, int from_tty) > > if ((pid = vfork ()) == 0) > { > - const char *p, *user_shell; > + const char *p, *user_shell = get_shell (); > > close_most_fds (); > > - if ((user_shell = getenv ("SHELL")) == NULL) > - user_shell = "/bin/sh"; > - > /* Get the name of the shell for arg0. */ > p = lbasename (user_shell); > > diff --git a/gdb/common/pathstuff.c b/gdb/common/pathstuff.c > index 82905c9e68..6d8e53f4e1 100644 > --- a/gdb/common/pathstuff.c > +++ b/gdb/common/pathstuff.c > @@ -190,3 +190,15 @@ get_standard_cache_dir () > > return {}; > } > + > +/* See common/pathstuff.h. */ > + > +const char * > +get_shell () > +{ > + const char *ret = getenv ("SHELL"); > + if (ret == NULL) > + ret = "/bin/sh"; > + > + return ret; > +} > diff --git a/gdb/common/pathstuff.h b/gdb/common/pathstuff.h > index a43b963651..40fbda8d85 100644 > --- a/gdb/common/pathstuff.h > +++ b/gdb/common/pathstuff.h > @@ -64,4 +64,8 @@ extern bool contains_dir_separator (const char *path); > > extern std::string get_standard_cache_dir (); > > +/* Return the file name of the user's shell. */ WDYT about explicitly mentioning here that "user's shell" means "the SHELL environment variable"? > + > +extern const char *get_shell (); > + > #endif /* PATHSTUFF_H */ > diff --git a/gdb/nat/fork-inferior.c b/gdb/nat/fork-inferior.c > index 40cd05a0f8..f1032b43c9 100644 > --- a/gdb/nat/fork-inferior.c > +++ b/gdb/nat/fork-inferior.c > @@ -24,16 +24,13 @@ > #include "target/target.h" > #include "common-inferior.h" > #include "common-gdbthread.h" > +#include "common/pathstuff.h" > #include "signals-state-save-restore.h" > #include "gdb_tilde_expand.h" > #include > > extern char **environ; > > -/* Default shell file to be used if 'startup-with-shell' is set but > - $SHELL is not. */ > -#define SHELL_FILE "/bin/sh" > - > /* Build the argument vector for execv(3). */ > > class execv_argv > @@ -265,20 +262,6 @@ execv_argv::init_for_shell (const char *exec_file, > m_argv.push_back (NULL); > } > > -/* Return the shell that must be used to startup the inferior. The > - first attempt is the environment variable SHELL; if it is not set, > - then we default to SHELL_FILE. */ > - > -static const char * > -get_startup_shell () > -{ > - const char *ret = getenv ("SHELL"); > - if (ret == NULL) > - ret = SHELL_FILE; > - > - return ret; > -} > - > /* See nat/fork-inferior.h. */ > > pid_t > @@ -316,7 +299,7 @@ fork_inferior (const char *exec_file_arg, const std::string &allargs, > > /* Figure out what shell to start up the user program under. */ > if (shell_file == NULL) > - shell_file = get_startup_shell (); > + shell_file = get_shell (); > > gdb_assert (shell_file != NULL); > } > diff --git a/gdb/procfs.c b/gdb/procfs.c > index 6ffe569e69..ca381a71ae 100644 > --- a/gdb/procfs.c > +++ b/gdb/procfs.c > @@ -3035,11 +3035,11 @@ procfs_target::create_inferior (const char *exec_file, > const std::string &allargs, > char **env, int from_tty) > { > - char *shell_file = getenv ("SHELL"); > + const char *shell_file = get_shell (); > char *tryname; > int pid; > > - if (shell_file != NULL && strchr (shell_file, '/') == NULL) > + if (strchr (shell_file, '/') == NULL) > { > > /* We will be looking down the PATH to find shell_file. If we > diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c > index 0047a26418..8292cf4212 100644 > --- a/gdb/windows-nat.c > +++ b/gdb/windows-nat.c > @@ -68,6 +68,7 @@ > #include "complaints.h" > #include "inf-child.h" > #include "gdb_tilde_expand.h" > +#include "common/pathstuff.h" > > #define AdjustTokenPrivileges dyn_AdjustTokenPrivileges > #define DebugActiveProcessStop dyn_DebugActiveProcessStop > @@ -2578,9 +2579,7 @@ windows_nat_target::create_inferior (const char *exec_file, > } > else > { > - sh = getenv ("SHELL"); > - if (!sh) > - sh = "/bin/sh"; > + sh = get_shell (); > if (cygwin_conv_path (CCP_POSIX_TO_WIN_W, sh, shell, __PMAX) < 0) > error (_("Error starting executable via shell: %d"), errno); > #ifdef __USEWIDE > -- > 2.17.1 -- Sergio GPG key ID: 237A 54B1 0287 28BF 00EF 31F4 D0EB 7628 65FC 5E36 Please send encrypted e-mail if possible http://sergiodj.net/