LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] tools: libsubcmd: Drop the less hack that was inherited from Git.
@ 2018-01-24  0:38 Arvind Sankar
  2018-01-24 20:54 ` Josh Poimboeuf
  0 siblings, 1 reply; 8+ messages in thread
From: Arvind Sankar @ 2018-01-24  0:38 UTC (permalink / raw)
  To: linux-kernel; +Cc: Greg Kroah-Hartman, Josh Poimboeuf

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>
---
 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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] tools: libsubcmd: Drop the less hack that was inherited from Git.
  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
  0 siblings, 1 reply; 8+ messages in thread
From: Josh Poimboeuf @ 2018-01-24 20:54 UTC (permalink / raw)
  To: Arvind Sankar
  Cc: linux-kernel, Greg Kroah-Hartman, Peter Zijlstra, Ingo Molnar,
	Arnaldo Carvalho de Melo

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?

> ---
>  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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] tools: libsubcmd: Drop the less hack that was inherited from Git.
  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-09  0:00     ` Arvind Sankar
  0 siblings, 2 replies; 8+ messages in thread
From: Arnaldo Carvalho de Melo @ 2018-01-25  8:16 UTC (permalink / raw)
  To: Josh Poimboeuf
  Cc: Arvind Sankar, linux-kernel, Greg Kroah-Hartman, Peter Zijlstra,
	Ingo Molnar

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] tools: libsubcmd: Drop the less hack that was inherited from Git.
  2018-01-25  8:16   ` Arnaldo Carvalho de Melo
@ 2018-01-25 13:24     ` Arvind Sankar
  2018-02-03  0:38       ` Arvind Sankar
  2018-02-09  0:00     ` Arvind Sankar
  1 sibling, 1 reply; 8+ messages in thread
From: Arvind Sankar @ 2018-01-25 13:24 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Josh Poimboeuf, linux-kernel, Greg Kroah-Hartman, Peter Zijlstra,
	Ingo Molnar

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] tools: libsubcmd: Drop the less hack that was inherited from Git.
  2018-01-25 13:24     ` Arvind Sankar
@ 2018-02-03  0:38       ` Arvind Sankar
  0 siblings, 0 replies; 8+ messages in thread
From: Arvind Sankar @ 2018-02-03  0:38 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Josh Poimboeuf, linux-kernel, Greg Kroah-Hartman, Peter Zijlstra,
	Ingo Molnar, Sergey Senozhatsky

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] tools: libsubcmd: Drop the less hack that was inherited from Git.
  2018-01-25  8:16   ` Arnaldo Carvalho de Melo
  2018-01-25 13:24     ` Arvind Sankar
@ 2018-02-09  0:00     ` Arvind Sankar
  2018-02-09  1:17       ` Arnaldo Carvalho de Melo
  1 sibling, 1 reply; 8+ messages in thread
From: Arvind Sankar @ 2018-02-09  0:00 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Josh Poimboeuf, linux-kernel, Greg Kroah-Hartman, Peter Zijlstra,
	Ingo Molnar

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>
---
 - Merge with commit ad343a98 (tools/lib/subcmd/pager.c: do not alias
   select() params) by Sergey

 tools/lib/subcmd/pager.c       | 20 --------------------
 tools/lib/subcmd/run-command.c |  2 --
 tools/lib/subcmd/run-command.h |  1 -
 3 files changed, 23 deletions(-)

diff --git a/tools/lib/subcmd/pager.c b/tools/lib/subcmd/pager.c
index 9997a8805a82..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,24 +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_set exception;
-
-	FD_ZERO(&in);
-	FD_ZERO(&exception);
-	FD_SET(0, &in);
-	FD_SET(0, &exception);
-	select(1, &in, NULL, &exception, NULL);
-
-	setenv("LESS", "FRSX", 0);
-}
-
 static const char *pager_argv[] = { "sh", "-c", NULL, NULL };
 static struct child_process pager_process;
 
@@ -87,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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] tools: libsubcmd: Drop the less hack that was inherited from Git.
  2018-02-09  0:00     ` Arvind Sankar
@ 2018-02-09  1:17       ` Arnaldo Carvalho de Melo
  2018-04-20  1:12         ` Arvind Sankar
  0 siblings, 1 reply; 8+ messages in thread
From: Arnaldo Carvalho de Melo @ 2018-02-09  1:17 UTC (permalink / raw)
  To: Arvind Sankar
  Cc: Josh Poimboeuf, linux-kernel, Greg Kroah-Hartman, Peter Zijlstra,
	Ingo Molnar

Em Thu, Feb 08, 2018 at 07:00:10PM -0500, Arvind Sankar escreveu:
> 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.

Ok, so e8320f3 was merged directly by Linus, I'll wait till perf/urgent
pulls from upstream and then will merge this,

Thanks,

- Arnaldo
 
> Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
> ---
>  - Merge with commit ad343a98 (tools/lib/subcmd/pager.c: do not alias
>    select() params) by Sergey
> 
>  tools/lib/subcmd/pager.c       | 20 --------------------
>  tools/lib/subcmd/run-command.c |  2 --
>  tools/lib/subcmd/run-command.h |  1 -
>  3 files changed, 23 deletions(-)
> 
> diff --git a/tools/lib/subcmd/pager.c b/tools/lib/subcmd/pager.c
> index 9997a8805a82..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,24 +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_set exception;
> -
> -	FD_ZERO(&in);
> -	FD_ZERO(&exception);
> -	FD_SET(0, &in);
> -	FD_SET(0, &exception);
> -	select(1, &in, NULL, &exception, NULL);
> -
> -	setenv("LESS", "FRSX", 0);
> -}
> -
>  static const char *pager_argv[] = { "sh", "-c", NULL, NULL };
>  static struct child_process pager_process;
>  
> @@ -87,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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] tools: libsubcmd: Drop the less hack that was inherited from Git.
  2018-02-09  1:17       ` Arnaldo Carvalho de Melo
@ 2018-04-20  1:12         ` Arvind Sankar
  0 siblings, 0 replies; 8+ messages in thread
From: Arvind Sankar @ 2018-04-20  1:12 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Josh Poimboeuf, linux-kernel, Greg Kroah-Hartman, Peter Zijlstra,
	Ingo Molnar

[-- Attachment #1: Type: text/plain, Size: 3535 bytes --]

Hey did this ever make it in?

On Thu, Feb 8, 2018 at 8:17 PM, Arnaldo Carvalho de Melo <acme@kernel.org>
wrote:

> Em Thu, Feb 08, 2018 at 07:00:10PM -0500, Arvind Sankar escreveu:
> > 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.
>
> Ok, so e8320f3 was merged directly by Linus, I'll wait till perf/urgent
> pulls from upstream and then will merge this,
>
> Thanks,
>
> - Arnaldo
>
> > Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
> > ---
> >  - Merge with commit ad343a98 (tools/lib/subcmd/pager.c: do not alias
> >    select() params) by Sergey
> >
> >  tools/lib/subcmd/pager.c       | 20 --------------------
> >  tools/lib/subcmd/run-command.c |  2 --
> >  tools/lib/subcmd/run-command.h |  1 -
> >  3 files changed, 23 deletions(-)
> >
> > diff --git a/tools/lib/subcmd/pager.c b/tools/lib/subcmd/pager.c
> > index 9997a8805a82..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,24 +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_set exception;
> > -
> > -     FD_ZERO(&in);
> > -     FD_ZERO(&exception);
> > -     FD_SET(0, &in);
> > -     FD_SET(0, &exception);
> > -     select(1, &in, NULL, &exception, NULL);
> > -
> > -     setenv("LESS", "FRSX", 0);
> > -}
> > -
> >  static const char *pager_argv[] = { "sh", "-c", NULL, NULL };
> >  static struct child_process pager_process;
> >
> > @@ -87,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
>

[-- Attachment #2: Type: text/html, Size: 4951 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2018-04-20  1:12 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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
2018-02-09  0:00     ` Arvind Sankar
2018-02-09  1:17       ` Arnaldo Carvalho de Melo
2018-04-20  1:12         ` Arvind Sankar

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