From: Arvind Sankar <nivedita@alum.mit.edu>
To: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>,
linux-kernel@vger.kernel.org,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Peter Zijlstra <peterz@infradead.org>,
Ingo Molnar <mingo@redhat.com>,
Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
Subject: Re: [PATCH] tools: libsubcmd: Drop the less hack that was inherited from Git.
Date: Fri, 2 Feb 2018 19:38:51 -0500 [thread overview]
Message-ID: <20180203003850.GA62570@rani.riverdale> (raw)
In-Reply-To: <20180125132425.GA27283@rani.riverdale>
Hi, it looks like Sergey has put in a patch to fix the aliasing, looking
at the linux-next tree.
Are we still looking to remove this hack altogether?
Thanks
On Thu, Jan 25, 2018 at 08:24:26AM -0500, Arvind Sankar wrote:
> Thanks.
>
> This was found because gcc 8 appears to be enabling -Wrestrict in -Wall,
> so there is a build failure with mainline gcc.
>
> On Thu, Jan 25, 2018 at 05:16:52AM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Wed, Jan 24, 2018 at 02:54:11PM -0600, Josh Poimboeuf escreveu:
> > > On Tue, Jan 23, 2018 at 07:38:37PM -0500, Arvind Sankar wrote:
> > > > We inherited this hack with the original code from the Git project. The
> > > > select call is invalid as the two fd_set pointers should not be aliased.
> > > >
> > > > We could fix it, but the Git project removed this hack in 2012 in commit
> > > > e8320f3 (pager: drop "wait for output to run less" hack). The bug it
> > > > worked around was apparently fixed in less back in June 2007.
> > > >
> > > > So remove the hack from here as well.
> > > >
> > > > Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
> > >
> > > Looks good to me.
> > >
> > > Acked-by: Josh Poimboeuf <jpoimboe@redhat.com>
> > >
> > > Libsubcmd is used by perf and objtool, so adding the perf maintainers to
> > > CC. Arnaldo, do you want to pick this one up?
> >
> > Sure, I'll put it in my perf/core branch.
> >
> > - Arnaldo
> >
> > > > ---
> > > > tools/lib/subcmd/pager.c | 17 -----------------
> > > > tools/lib/subcmd/run-command.c | 2 --
> > > > tools/lib/subcmd/run-command.h | 1 -
> > > > 3 files changed, 20 deletions(-)
> > > >
> > > > diff --git a/tools/lib/subcmd/pager.c b/tools/lib/subcmd/pager.c
> > > > index 5ba754d17952..94d61d9b511f 100644
> > > > --- a/tools/lib/subcmd/pager.c
> > > > +++ b/tools/lib/subcmd/pager.c
> > > > @@ -1,5 +1,4 @@
> > > > // SPDX-License-Identifier: GPL-2.0
> > > > -#include <sys/select.h>
> > > > #include <stdlib.h>
> > > > #include <stdio.h>
> > > > #include <string.h>
> > > > @@ -23,21 +22,6 @@ void pager_init(const char *pager_env)
> > > > subcmd_config.pager_env = pager_env;
> > > > }
> > > >
> > > > -static void pager_preexec(void)
> > > > -{
> > > > - /*
> > > > - * Work around bug in "less" by not starting it until we
> > > > - * have real input
> > > > - */
> > > > - fd_set in;
> > > > -
> > > > - FD_ZERO(&in);
> > > > - FD_SET(0, &in);
> > > > - select(1, &in, NULL, &in, NULL);
> > > > -
> > > > - setenv("LESS", "FRSX", 0);
> > > > -}
> > > > -
> > > > static const char *pager_argv[] = { "sh", "-c", NULL, NULL };
> > > > static struct child_process pager_process;
> > > >
> > > > @@ -84,7 +68,6 @@ void setup_pager(void)
> > > > pager_argv[2] = pager;
> > > > pager_process.argv = pager_argv;
> > > > pager_process.in = -1;
> > > > - pager_process.preexec_cb = pager_preexec;
> > > >
> > > > if (start_command(&pager_process))
> > > > return;
> > > > diff --git a/tools/lib/subcmd/run-command.c b/tools/lib/subcmd/run-command.c
> > > > index 5cdac2162532..9e9dca717ed7 100644
> > > > --- a/tools/lib/subcmd/run-command.c
> > > > +++ b/tools/lib/subcmd/run-command.c
> > > > @@ -120,8 +120,6 @@ int start_command(struct child_process *cmd)
> > > > unsetenv(*cmd->env);
> > > > }
> > > > }
> > > > - if (cmd->preexec_cb)
> > > > - cmd->preexec_cb();
> > > > if (cmd->exec_cmd) {
> > > > execv_cmd(cmd->argv);
> > > > } else {
> > > > diff --git a/tools/lib/subcmd/run-command.h b/tools/lib/subcmd/run-command.h
> > > > index 17d969c6add3..6256268802b5 100644
> > > > --- a/tools/lib/subcmd/run-command.h
> > > > +++ b/tools/lib/subcmd/run-command.h
> > > > @@ -46,7 +46,6 @@ struct child_process {
> > > > unsigned no_stderr:1;
> > > > unsigned exec_cmd:1; /* if this is to be external sub-command */
> > > > unsigned stdout_to_stderr:1;
> > > > - void (*preexec_cb)(void);
> > > > };
> > > >
> > > > int start_command(struct child_process *);
> > > > --
> > > > 2.13.6
> > > >
> > >
> > > --
> > > Josh
next prev parent reply other threads:[~2018-02-03 0:38 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-01-24 0:38 [PATCH] tools: libsubcmd: Drop the less hack that was inherited from Git Arvind Sankar
2018-01-24 20:54 ` Josh Poimboeuf
2018-01-25 8:16 ` Arnaldo Carvalho de Melo
2018-01-25 13:24 ` Arvind Sankar
2018-02-03 0:38 ` Arvind Sankar [this message]
2018-02-09 0:00 ` Arvind Sankar
2018-02-09 1:17 ` Arnaldo Carvalho de Melo
2018-04-20 1:12 ` Arvind Sankar
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20180203003850.GA62570@rani.riverdale \
--to=nivedita@alum.mit.edu \
--cc=acme@kernel.org \
--cc=gregkh@linuxfoundation.org \
--cc=jpoimboe@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=mingo@redhat.com \
--cc=peterz@infradead.org \
--cc=sergey.senozhatsky@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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 NNTP newsgroup(s).