[RFC PATCH 0/3] kbuild: change the default shell to bash

[RFC PATCH 0/3] kbuild: change the default shell to bash

[Masahiro Yamada]

This is related to the discussion about the portability [1].

I wrote this code some time ago for some reason. My motivation at that
time was trap handling.

We use the EXIT trap in several places. The POSIX [2] mentions the 'EXIT'
as the trap condition, but nothing about the actual conditions that trigger
the EXIT trap. Bash invokes the EXIT trap when it exits after receiving
an unhandled signal, while dash does not.

I did not submit the patches because I lost the use-case of the EXIT trap
except cleaning temporary files. It is harmless to have temp files left over.

Recently, I saw a bug report regarding the portability of 'echo'
('echo' is a shell's built-in command. The behavior is different between
bash and dash).

I am sharing this patch set as RFC in case somebody might have interest or comment.
I am still wondering if this might be a big hammer, though.

[1]: https://lore.kernel.org/all/e902a360e3759c7f87d98d71d79a0d5cbe935e3e.camel@linuxfoundation.org/
[2]: https://pubs.opengroup.org/onlinepubs/009604599/utilities/trap.html



Masahiro Yamada (3):
  kconfig: move declarations for prepossessing to internal.h
  kconfig: allow to choose the shell for $(shell ) functions
  kbuild: use bash as the default shell for Make and Kconfig

 .../kbuild/kconfig-macro-language.rst         |  4 ++
 Makefile                                      |  7 ++
 scripts/Kconfig.include                       |  3 +
 scripts/kconfig/confdata.c                    |  1 +
 scripts/kconfig/internal.h                    | 18 +++++
 scripts/kconfig/lexer.l                       |  1 +
 scripts/kconfig/lkc_proto.h                   | 13 ----
 scripts/kconfig/parser.y                      |  1 +
 scripts/kconfig/preprocess.c                  | 66 +++++++++++++++----
 9 files changed, 87 insertions(+), 27 deletions(-)

-- 
2.34.1

[RFC PATCH 2/3] kconfig: allow to choose the shell for $(shell ) functions

[Masahiro Yamada]

GNU Make uses /bin/sh by default for running recipe lines and $(shell )
functions. You can change the shell by setting the 'SHELL' variable.
Unlike most variables, 'SHELL' is never set from the environment. [1]

Currently, Kconfig does not provide any way to change the default shell.
/bin/sh is always used for running $(shell,...) because do_shell() is
implemented by using popen(3).

This commit allows users to change the shell for Kconfig in a similar
way to GNU Make; you can set the 'SHELL' variable in a Kconfig file to
override the default shell. It is not taken from the environment. The
change is effective only for $(shell,...) invocations called after the
'SHELL' assignment.

[1]: https://www.gnu.org/software/make/manual/html_node/Choosing-the-Shell.html

Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---

 .../kbuild/kconfig-macro-language.rst         |  4 ++
 scripts/kconfig/internal.h                    |  1 +
 scripts/kconfig/parser.y                      |  1 +
 scripts/kconfig/preprocess.c                  | 65 +++++++++++++++----
 4 files changed, 57 insertions(+), 14 deletions(-)

diff --git a/Documentation/kbuild/kconfig-macro-language.rst b/Documentation/kbuild/kconfig-macro-language.rst
index 6163467f6ae4..fe8c6982179e 100644
--- a/Documentation/kbuild/kconfig-macro-language.rst
+++ b/Documentation/kbuild/kconfig-macro-language.rst
@@ -112,6 +112,10 @@ Kconfig currently supports the following built-in functions.
   replaced with a space. Any trailing newlines are deleted. The standard error
   is not returned, nor is any program exit status.
 
+  The program used as the shell is taken from the variable SHELL. If it is not
+  set anywhere in your Kconfig file, /bin/sh is used as the shell.
+  Unlike most variables, the variable SHELL is never set from the environment.
+
  - $(info,text)
 
   The "info" function takes a single argument and prints it to stdout.
diff --git a/scripts/kconfig/internal.h b/scripts/kconfig/internal.h
index 8e0e6d315b6c..c18017650e54 100644
--- a/scripts/kconfig/internal.h
+++ b/scripts/kconfig/internal.h
@@ -19,6 +19,7 @@ enum variable_flavor {
 void env_write_dep(FILE *f, const char *auto_conf_name);
 void variable_add(const char *name, const char *value,
 		  enum variable_flavor flavor);
+void variable_init(void);
 void variable_all_del(void);
 char *expand_dollar(const char **str);
 char *expand_one_token(const char **str);
diff --git a/scripts/kconfig/parser.y b/scripts/kconfig/parser.y
index 2af7ce4e1531..436afaef9228 100644
--- a/scripts/kconfig/parser.y
+++ b/scripts/kconfig/parser.y
@@ -483,6 +483,7 @@ void conf_parse(const char *name)
 	zconf_initscan(name);
 
 	_menu_init();
+	variable_init();
 
 	if (getenv("ZCONF_DEBUG"))
 		yydebug = 1;
diff --git a/scripts/kconfig/preprocess.c b/scripts/kconfig/preprocess.c
index aeb3fe362c04..608a84e7f5d6 100644
--- a/scripts/kconfig/preprocess.c
+++ b/scripts/kconfig/preprocess.c
@@ -8,6 +8,7 @@
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
+#include <unistd.h>
 
 #include "list.h"
 #include "lkc.h"
@@ -141,24 +142,59 @@ static char *do_lineno(int argc, char *argv[])
 
 static char *do_shell(int argc, char *argv[])
 {
-	FILE *p;
+	int pipefd[2];
+	pid_t pid;
 	char buf[4096];
-	char *cmd;
-	size_t nread;
+	ssize_t nread;
 	int i;
 
-	cmd = argv[0];
-
-	p = popen(cmd, "r");
-	if (!p) {
-		perror(cmd);
+	if (pipe(pipefd) < 0) {
+		perror("pipe");
+		exit(1);
+	}
+	pid = fork();
+	if (pid < -1) {
+		perror("fork");
 		exit(1);
 	}
 
-	nread = fread(buf, 1, sizeof(buf), p);
+	if (pid == 0) { /* child */
+		char *shell;
+
+		/* duplicate the write end to stdout */
+		if (dup2(pipefd[1], STDOUT_FILENO) < 0) {
+			perror("dup2");
+			_exit(1);
+		}
+
+		/*
+		 * Do not leak file descriptors to the child process
+		 * (including the read end of the pipe).
+		 * Closing up to 15 is enough for us?
+		 */
+		for (i = STDERR_FILENO + 1; i < 16; i++)
+			close(i);
+
+		shell = expand_string("$(SHELL)");
+
+		execl(shell, shell, "-c", argv[0], NULL);
+		perror("execl");
+
+		free(shell);
+
+		_exit(1);
+	}
+
+	/* parent */
+
+	close(pipefd[1]);	/* the write end is unneeded */
+
+	nread = read(pipefd[0], buf, sizeof(buf));
 	if (nread == sizeof(buf))
 		nread--;
 
+	close(pipefd[0]);	/* now close the read end */
+
 	/* remove trailing new lines */
 	while (nread > 0 && buf[nread - 1] == '\n')
 		nread--;
@@ -171,11 +207,6 @@ static char *do_shell(int argc, char *argv[])
 			buf[i] = ' ';
 	}
 
-	if (pclose(p) == -1) {
-		perror(cmd);
-		exit(1);
-	}
-
 	return xstrdup(buf);
 }
 
@@ -330,6 +361,12 @@ static void variable_del(struct variable *v)
 	free(v);
 }
 
+void variable_init(void)
+{
+	/* Set the default shell */
+	variable_add("SHELL", "/bin/sh", VAR_RECURSIVE);
+}
+
 void variable_all_del(void)
 {
 	struct variable *v, *tmp;
-- 
2.34.1

Re: [RFC PATCH 2/3] kconfig: allow to choose the shell for $(shell ) functions

[Bagas Sanjaya]

On 8/19/22 13:56, Masahiro Yamada wrote:
> GNU Make uses /bin/sh by default for running recipe lines and $(shell )
> functions. You can change the shell by setting the 'SHELL' variable.
> Unlike most variables, 'SHELL' is never set from the environment. [1]
> 
> Currently, Kconfig does not provide any way to change the default shell.
> /bin/sh is always used for running $(shell,...) because do_shell() is
> implemented by using popen(3).
> 
> This commit allows users to change the shell for Kconfig in a similar
> way to GNU Make; you can set the 'SHELL' variable in a Kconfig file to
> override the default shell. It is not taken from the environment. The
> change is effective only for $(shell,...) invocations called after the
> 'SHELL' assignment.
> 

Hmmm...

Can we say that if we run SHELL=/bin/bash make nconfig, Kconfig will use
$SHELL but we can't set it as environment variable?

-- 
An old man doll... just what I always wanted! - Clara

RE: [RFC PATCH 2/3] kconfig: allow to choose the shell for $(shell ) functions

[David Laight]

From: Bagas Sanjaya
> Sent: 19 August 2022 09:59
> 
> On 8/19/22 13:56, Masahiro Yamada wrote:
> > GNU Make uses /bin/sh by default for running recipe lines and $(shell )
> > functions. You can change the shell by setting the 'SHELL' variable.
> > Unlike most variables, 'SHELL' is never set from the environment. [1]
> >
> > Currently, Kconfig does not provide any way to change the default shell.
> > /bin/sh is always used for running $(shell,...) because do_shell() is
> > implemented by using popen(3).
> >
> > This commit allows users to change the shell for Kconfig in a similar
> > way to GNU Make; you can set the 'SHELL' variable in a Kconfig file to
> > override the default shell. It is not taken from the environment. The
> > change is effective only for $(shell,...) invocations called after the
> > 'SHELL' assignment.
> >
> 
> Hmmm...
> 
> Can we say that if we run SHELL=/bin/bash make nconfig, Kconfig will use
> $SHELL but we can't set it as environment variable?

That just puts it into the environment for the single command.
You'd need to pass it as a command line argument to make.

(Or pass it in a different environment variable and then assign
it within the makefile.)

Or just remove the crappy bashisms and write portable shell scripts.
Just be glad you're not trying to use the SYSV /bin/sh.
(And avoid the other bugs that make dash fail to run most of the
scripts I write.)

Compex echo requests can be replaced by printf (without penalty
since it will be a shell builtin).

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)