[PATCH] tools: libsubcmd: Drop the less hack that was inherited from Git.

[PATCH] tools: libsubcmd: Drop the less hack that was inherited from Git.

[Arvind Sankar]

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

Re: [PATCH] tools: libsubcmd: Drop the less hack that was inherited from Git.

[Josh Poimboeuf]

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

Re: [PATCH] tools: libsubcmd: Drop the less hack that was inherited from Git.

[Arnaldo Carvalho de Melo]

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

Re: [PATCH] tools: libsubcmd: Drop the less hack that was inherited from Git.

[Arvind Sankar]

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

Re: [PATCH] tools: libsubcmd: Drop the less hack that was inherited from Git.

[Arvind Sankar]

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

Re: [PATCH] tools: libsubcmd: Drop the less hack that was inherited from Git.

[Arvind Sankar]

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

Re: [PATCH] tools: libsubcmd: Drop the less hack that was inherited from Git.

[Arnaldo Carvalho de Melo]

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

Re: [PATCH] tools: libsubcmd: Drop the less hack that was inherited from Git.

[Arvind Sankar]

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