Xen-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] tools/configure: drop BASH configure variable
@ 2020-06-26 17:00 ` Andrew Cooper
  2020-06-29  7:41   ` Paul Durrant
                     ` (2 more replies)
  0 siblings, 3 replies; 13+ messages in thread
From: Andrew Cooper @ 2020-06-26 17:00 UTC (permalink / raw)
  To: Xen-devel
  Cc: Andrew Cooper, Daniel De Graaf, Paul Durrant, Wei Liu, Ian Jackson

This is a weird variable to have in the first place.  The only user of it is
XSM's CONFIG_SHELL, which opencodes a fallback to sh, and the only two scripts
run with this are shebang sh anyway, so don't need bash in the first place.

Make the mkflask.sh and mkaccess_vector.sh scripts executable, drop the
CONFIG_SHELL, and drop the $BASH variable to prevent further use.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Ian Jackson <Ian.Jackson@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Daniel De Graaf <dgdegra@tycho.nsa.gov>
CC: Paul Durrant <paul@xen.org>

./autogen.sh needs rerunning on commit.

RFC for 4.14.  This is a cleanup to the build system.

There is a separate check for [BASH] in the configure script, which is
checking the requirement for the Linux hotplug scripts.  Really, that is a
runtime requirement not a build time requirement, and it is rude to require
bash in a build environment on this basis.  IMO, that check wants dropping as
well.
---
 config/Tools.mk.in                      | 1 -
 tools/configure.ac                      | 1 -
 xen/xsm/flask/Makefile                  | 8 ++------
 xen/xsm/flask/policy/mkaccess_vector.sh | 0
 xen/xsm/flask/policy/mkflask.sh         | 0
 5 files changed, 2 insertions(+), 8 deletions(-)
 mode change 100644 => 100755 xen/xsm/flask/policy/mkaccess_vector.sh
 mode change 100644 => 100755 xen/xsm/flask/policy/mkflask.sh

diff --git a/config/Tools.mk.in b/config/Tools.mk.in
index 23df47af8d..48bd9ab731 100644
--- a/config/Tools.mk.in
+++ b/config/Tools.mk.in
@@ -12,7 +12,6 @@ PYTHON              := @PYTHON@
 PYTHON_PATH         := @PYTHONPATH@
 PY_NOOPT_CFLAGS     := @PY_NOOPT_CFLAGS@
 PERL                := @PERL@
-BASH                := @BASH@
 XGETTTEXT           := @XGETTEXT@
 AS86                := @AS86@
 LD86                := @LD86@
diff --git a/tools/configure.ac b/tools/configure.ac
index 9d126b7a14..6614a4f130 100644
--- a/tools/configure.ac
+++ b/tools/configure.ac
@@ -297,7 +297,6 @@ AC_ARG_VAR([PYTHON], [Path to the Python parser])
 AC_ARG_VAR([PERL], [Path to Perl parser])
 AC_ARG_VAR([BISON], [Path to Bison parser generator])
 AC_ARG_VAR([FLEX], [Path to Flex lexical analyser generator])
-AC_ARG_VAR([BASH], [Path to bash shell])
 AC_ARG_VAR([XGETTEXT], [Path to xgetttext tool])
 AC_ARG_VAR([AS86], [Path to as86 tool])
 AC_ARG_VAR([LD86], [Path to ld86 tool])
diff --git a/xen/xsm/flask/Makefile b/xen/xsm/flask/Makefile
index 07f36d075d..ba8f31a02c 100644
--- a/xen/xsm/flask/Makefile
+++ b/xen/xsm/flask/Makefile
@@ -8,10 +8,6 @@ CFLAGS-y += -I./include
 
 AWK = awk
 
-CONFIG_SHELL := $(shell if [ -x "$$BASH" ]; then echo $$BASH; \
-          else if [ -x /bin/bash ]; then echo /bin/bash; \
-          else echo sh; fi ; fi)
-
 FLASK_H_DEPEND = policy/security_classes policy/initial_sids
 AV_H_DEPEND = policy/access_vectors
 
@@ -24,14 +20,14 @@ extra-y += $(ALL_H_FILES)
 
 mkflask := policy/mkflask.sh
 quiet_cmd_mkflask = MKFLASK $@
-cmd_mkflask = $(CONFIG_SHELL) $(mkflask) $(AWK) include $(FLASK_H_DEPEND)
+cmd_mkflask = $(mkflask) $(AWK) include $(FLASK_H_DEPEND)
 
 $(subst include/,%/,$(FLASK_H_FILES)): $(FLASK_H_DEPEND) $(mkflask) FORCE
 	$(call if_changed,mkflask)
 
 mkaccess := policy/mkaccess_vector.sh
 quiet_cmd_mkaccess = MKACCESS VECTOR $@
-cmd_mkaccess = $(CONFIG_SHELL) $(mkaccess) $(AWK) $(AV_H_DEPEND)
+cmd_mkaccess = $(mkaccess) $(AWK) $(AV_H_DEPEND)
 
 $(subst include/,%/,$(AV_H_FILES)): $(AV_H_DEPEND) $(mkaccess) FORCE
 	$(call if_changed,mkaccess)
diff --git a/xen/xsm/flask/policy/mkaccess_vector.sh b/xen/xsm/flask/policy/mkaccess_vector.sh
old mode 100644
new mode 100755
diff --git a/xen/xsm/flask/policy/mkflask.sh b/xen/xsm/flask/policy/mkflask.sh
old mode 100644
new mode 100755
-- 
2.11.0



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

* RE: [PATCH] tools/configure: drop BASH configure variable
  2020-06-26 17:00 ` [PATCH] " Andrew Cooper
@ 2020-06-29  7:41   ` Paul Durrant
  2020-06-29  8:41   ` Jan Beulich
  2020-06-29 13:34   ` [PATCH] tools/configure: drop BASH configure variable Ian Jackson
  2 siblings, 0 replies; 13+ messages in thread
From: Paul Durrant @ 2020-06-29  7:41 UTC (permalink / raw)
  To: 'Andrew Cooper', 'Xen-devel'
  Cc: 'Ian Jackson', 'Daniel De Graaf', 'Wei Liu'

> -----Original Message-----
> From: Andrew Cooper <andrew.cooper3@citrix.com>
> Sent: 26 June 2020 18:01
> To: Xen-devel <xen-devel@lists.xenproject.org>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Wei Liu
> <wl@xen.org>; Daniel De Graaf <dgdegra@tycho.nsa.gov>; Paul Durrant <paul@xen.org>
> Subject: [PATCH] tools/configure: drop BASH configure variable
> 
> This is a weird variable to have in the first place.  The only user of it is
> XSM's CONFIG_SHELL, which opencodes a fallback to sh, and the only two scripts
> run with this are shebang sh anyway, so don't need bash in the first place.
> 
> Make the mkflask.sh and mkaccess_vector.sh scripts executable, drop the
> CONFIG_SHELL, and drop the $BASH variable to prevent further use.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Ian Jackson <Ian.Jackson@citrix.com>
> CC: Wei Liu <wl@xen.org>
> CC: Daniel De Graaf <dgdegra@tycho.nsa.gov>
> CC: Paul Durrant <paul@xen.org>
> 
> ./autogen.sh needs rerunning on commit.
> 
> RFC for 4.14.  This is a cleanup to the build system.
> 
> There is a separate check for [BASH] in the configure script, which is
> checking the requirement for the Linux hotplug scripts.  Really, that is a
> runtime requirement not a build time requirement, and it is rude to require
> bash in a build environment on this basis.  IMO, that check wants dropping as
> well.

That sounds quite reasonable.

Release-acked-by: Paul Durrant <paul@xen.org>

> ---
>  config/Tools.mk.in                      | 1 -
>  tools/configure.ac                      | 1 -
>  xen/xsm/flask/Makefile                  | 8 ++------
>  xen/xsm/flask/policy/mkaccess_vector.sh | 0
>  xen/xsm/flask/policy/mkflask.sh         | 0
>  5 files changed, 2 insertions(+), 8 deletions(-)
>  mode change 100644 => 100755 xen/xsm/flask/policy/mkaccess_vector.sh
>  mode change 100644 => 100755 xen/xsm/flask/policy/mkflask.sh
> 
> diff --git a/config/Tools.mk.in b/config/Tools.mk.in
> index 23df47af8d..48bd9ab731 100644
> --- a/config/Tools.mk.in
> +++ b/config/Tools.mk.in
> @@ -12,7 +12,6 @@ PYTHON              := @PYTHON@
>  PYTHON_PATH         := @PYTHONPATH@
>  PY_NOOPT_CFLAGS     := @PY_NOOPT_CFLAGS@
>  PERL                := @PERL@
> -BASH                := @BASH@
>  XGETTTEXT           := @XGETTEXT@
>  AS86                := @AS86@
>  LD86                := @LD86@
> diff --git a/tools/configure.ac b/tools/configure.ac
> index 9d126b7a14..6614a4f130 100644
> --- a/tools/configure.ac
> +++ b/tools/configure.ac
> @@ -297,7 +297,6 @@ AC_ARG_VAR([PYTHON], [Path to the Python parser])
>  AC_ARG_VAR([PERL], [Path to Perl parser])
>  AC_ARG_VAR([BISON], [Path to Bison parser generator])
>  AC_ARG_VAR([FLEX], [Path to Flex lexical analyser generator])
> -AC_ARG_VAR([BASH], [Path to bash shell])
>  AC_ARG_VAR([XGETTEXT], [Path to xgetttext tool])
>  AC_ARG_VAR([AS86], [Path to as86 tool])
>  AC_ARG_VAR([LD86], [Path to ld86 tool])
> diff --git a/xen/xsm/flask/Makefile b/xen/xsm/flask/Makefile
> index 07f36d075d..ba8f31a02c 100644
> --- a/xen/xsm/flask/Makefile
> +++ b/xen/xsm/flask/Makefile
> @@ -8,10 +8,6 @@ CFLAGS-y += -I./include
> 
>  AWK = awk
> 
> -CONFIG_SHELL := $(shell if [ -x "$$BASH" ]; then echo $$BASH; \
> -          else if [ -x /bin/bash ]; then echo /bin/bash; \
> -          else echo sh; fi ; fi)
> -
>  FLASK_H_DEPEND = policy/security_classes policy/initial_sids
>  AV_H_DEPEND = policy/access_vectors
> 
> @@ -24,14 +20,14 @@ extra-y += $(ALL_H_FILES)
> 
>  mkflask := policy/mkflask.sh
>  quiet_cmd_mkflask = MKFLASK $@
> -cmd_mkflask = $(CONFIG_SHELL) $(mkflask) $(AWK) include $(FLASK_H_DEPEND)
> +cmd_mkflask = $(mkflask) $(AWK) include $(FLASK_H_DEPEND)
> 
>  $(subst include/,%/,$(FLASK_H_FILES)): $(FLASK_H_DEPEND) $(mkflask) FORCE
>  	$(call if_changed,mkflask)
> 
>  mkaccess := policy/mkaccess_vector.sh
>  quiet_cmd_mkaccess = MKACCESS VECTOR $@
> -cmd_mkaccess = $(CONFIG_SHELL) $(mkaccess) $(AWK) $(AV_H_DEPEND)
> +cmd_mkaccess = $(mkaccess) $(AWK) $(AV_H_DEPEND)
> 
>  $(subst include/,%/,$(AV_H_FILES)): $(AV_H_DEPEND) $(mkaccess) FORCE
>  	$(call if_changed,mkaccess)
> diff --git a/xen/xsm/flask/policy/mkaccess_vector.sh b/xen/xsm/flask/policy/mkaccess_vector.sh
> old mode 100644
> new mode 100755
> diff --git a/xen/xsm/flask/policy/mkflask.sh b/xen/xsm/flask/policy/mkflask.sh
> old mode 100644
> new mode 100755
> --
> 2.11.0




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

* Re: [PATCH] tools/configure: drop BASH configure variable
  2020-06-26 17:00 ` [PATCH] " Andrew Cooper
  2020-06-29  7:41   ` Paul Durrant
@ 2020-06-29  8:41   ` Jan Beulich
  2020-06-29 12:05     ` Ian Jackson
  2020-06-29 13:34   ` [PATCH] tools/configure: drop BASH configure variable Ian Jackson
  2 siblings, 1 reply; 13+ messages in thread
From: Jan Beulich @ 2020-06-29  8:41 UTC (permalink / raw)
  To: Andrew Cooper
  Cc: Xen-devel, Daniel De Graaf, Ian Jackson, Wei Liu, Paul Durrant

On 26.06.2020 19:00, Andrew Cooper wrote:
> @@ -24,14 +20,14 @@ extra-y += $(ALL_H_FILES)
>  
>  mkflask := policy/mkflask.sh
>  quiet_cmd_mkflask = MKFLASK $@
> -cmd_mkflask = $(CONFIG_SHELL) $(mkflask) $(AWK) include $(FLASK_H_DEPEND)
> +cmd_mkflask = $(mkflask) $(AWK) include $(FLASK_H_DEPEND)

This and ...

>  $(subst include/,%/,$(FLASK_H_FILES)): $(FLASK_H_DEPEND) $(mkflask) FORCE
>  	$(call if_changed,mkflask)
>  
>  mkaccess := policy/mkaccess_vector.sh
>  quiet_cmd_mkaccess = MKACCESS VECTOR $@
> -cmd_mkaccess = $(CONFIG_SHELL) $(mkaccess) $(AWK) $(AV_H_DEPEND)
> +cmd_mkaccess = $(mkaccess) $(AWK) $(AV_H_DEPEND)

... this should still use $(SHELL) though, as ...

> diff --git a/xen/xsm/flask/policy/mkaccess_vector.sh b/xen/xsm/flask/policy/mkaccess_vector.sh
> old mode 100644
> new mode 100755
> diff --git a/xen/xsm/flask/policy/mkflask.sh b/xen/xsm/flask/policy/mkflask.sh
> old mode 100644
> new mode 100755

... this may or may not take effect on the file system the sources
are stored on.

Jan


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

* Re: [PATCH] tools/configure: drop BASH configure variable
  2020-06-29  8:41   ` Jan Beulich
@ 2020-06-29 12:05     ` Ian Jackson
  2020-06-29 14:38       ` Jan Beulich
  0 siblings, 1 reply; 13+ messages in thread
From: Ian Jackson @ 2020-06-29 12:05 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Andrew Cooper, Daniel De Graaf, Paul Durrant, Wei Liu, Xen-devel

Jan Beulich writes ("Re: [PATCH] tools/configure: drop BASH configure variable"):
> On 26.06.2020 19:00, Andrew Cooper wrote:
> > diff --git a/xen/xsm/flask/policy/mkaccess_vector.sh b/xen/xsm/flask/policy/mkaccess_vector.sh
> > old mode 100644
> > new mode 100755
> > diff --git a/xen/xsm/flask/policy/mkflask.sh b/xen/xsm/flask/policy/mkflask.sh
> > old mode 100644
> > new mode 100755
> 
> ... this may or may not take effect on the file system the sources
> are stored on.

In what circumstances might this not take effect ?

IME all changes to files are properly replicated by git.  Tarball
distributions replicate the permissions of course.

The only difficulty would be if this change were to be carried as a
patch somewhere, by a downstream, but that seems unlikely, and can be
avoided by that downstream not taking this patch.

Ian.


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

* [PATCH] tools/configure: drop BASH configure variable
  2020-06-26 17:00 ` [PATCH] " Andrew Cooper
  2020-06-29  7:41   ` Paul Durrant
  2020-06-29  8:41   ` Jan Beulich
@ 2020-06-29 13:34   ` Ian Jackson
  2020-06-29 13:51     ` Andrew Cooper
  2 siblings, 1 reply; 13+ messages in thread
From: Ian Jackson @ 2020-06-29 13:34 UTC (permalink / raw)
  To: Andrew Cooper; +Cc: Xen-devel, Daniel De Graaf, Wei Liu, Paul Durrant

Andrew Cooper writes ("[PATCH] tools/configure: drop BASH configure variable"):
> This is a weird variable to have in the first place.  The only user of it is
> XSM's CONFIG_SHELL, which opencodes a fallback to sh, and the only two scripts
> run with this are shebang sh anyway, so don't need bash in the first place.

Thanks for this cleanup.  I agree with the basic idea.

However, did you run these scripts with dash, or review them, to check
for bashisms ?  It is not unusual to find scripts which need bash but
which mistaken have #!/bin/sh - especially if the usual arrangements
for running them always use bash anyway.  So the presence of the
#!/bin/sh is only rather weak evidence that these scripts will be fine
when run with sh.

> Make the mkflask.sh and mkaccess_vector.sh scripts executable, drop the
> CONFIG_SHELL, and drop the $BASH variable to prevent further use.

Since the build currently uses bash for these, a more neutral change
would be to change to #!/bin/bash at the same time.

> RFC for 4.14.  This is a cleanup to the build system.

I see this already has a release-ack.  However, I would not have
recommended granting one at least on the basis of the description
above.

I agree that this is cleanup.  But the current situation is not buggy.
I'm not sure exactly what the release criteria are but ISTM that this
patch adds risk to the release rather than removing it.

Thanks for your attention.

Ian.


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

* Re: [PATCH] tools/configure: drop BASH configure variable
  2020-06-29 13:34   ` [PATCH] tools/configure: drop BASH configure variable Ian Jackson
@ 2020-06-29 13:51     ` Andrew Cooper
  2020-06-29 14:00       ` Paul Durrant
  0 siblings, 1 reply; 13+ messages in thread
From: Andrew Cooper @ 2020-06-29 13:51 UTC (permalink / raw)
  To: Ian Jackson; +Cc: Xen-devel, Daniel De Graaf, Wei Liu, Paul Durrant

On 29/06/2020 14:34, Ian Jackson wrote:
> Andrew Cooper writes ("[PATCH] tools/configure: drop BASH configure variable"):
>> This is a weird variable to have in the first place.  The only user of it is
>> XSM's CONFIG_SHELL, which opencodes a fallback to sh, and the only two scripts
>> run with this are shebang sh anyway, so don't need bash in the first place.
> Thanks for this cleanup.  I agree with the basic idea.
>
> However, did you run these scripts with dash, or review them, to check
> for bashisms ?

Yes, to all of the above.

They are both very thin wrappers (doing some argument shuffling) around
large AWK scripts.

>> Make the mkflask.sh and mkaccess_vector.sh scripts executable, drop the
>> CONFIG_SHELL, and drop the $BASH variable to prevent further use.
> Since the build currently uses bash for these, a more neutral change
> would be to change to #!/bin/bash at the same time.

That will break FreeBSD, which has no `bash` in sight.

>> RFC for 4.14.  This is a cleanup to the build system.
> I see this already has a release-ack.  However, I would not have
> recommended granting one at least on the basis of the description
> above.
>
> I agree that this is cleanup.  But the current situation is not buggy.
> I'm not sure exactly what the release criteria are but ISTM that this
> patch adds risk to the release rather than removing it.

I agree that the current state of play isn't a major issue, but having
./configure check for bash is buggy for both uses.

~Andrew


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

* RE: [PATCH] tools/configure: drop BASH configure variable
  2020-06-29 13:51     ` Andrew Cooper
@ 2020-06-29 14:00       ` Paul Durrant
  0 siblings, 0 replies; 13+ messages in thread
From: Paul Durrant @ 2020-06-29 14:00 UTC (permalink / raw)
  To: 'Andrew Cooper', 'Ian Jackson'
  Cc: 'Xen-devel', 'Daniel De Graaf', 'Wei Liu'

> -----Original Message-----
> From: Andrew Cooper <andrew.cooper3@citrix.com>
> Sent: 29 June 2020 14:51
> To: Ian Jackson <ian.jackson@citrix.com>
> Cc: Xen-devel <xen-devel@lists.xenproject.org>; Wei Liu <wl@xen.org>; Daniel De Graaf
> <dgdegra@tycho.nsa.gov>; Paul Durrant <paul@xen.org>
> Subject: Re: [PATCH] tools/configure: drop BASH configure variable
> 
> On 29/06/2020 14:34, Ian Jackson wrote:
> > Andrew Cooper writes ("[PATCH] tools/configure: drop BASH configure variable"):
> >> This is a weird variable to have in the first place.  The only user of it is
> >> XSM's CONFIG_SHELL, which opencodes a fallback to sh, and the only two scripts
> >> run with this are shebang sh anyway, so don't need bash in the first place.
> > Thanks for this cleanup.  I agree with the basic idea.
> >
> > However, did you run these scripts with dash, or review them, to check
> > for bashisms ?
> 
> Yes, to all of the above.
> 
> They are both very thin wrappers (doing some argument shuffling) around
> large AWK scripts.
> 
> >> Make the mkflask.sh and mkaccess_vector.sh scripts executable, drop the
> >> CONFIG_SHELL, and drop the $BASH variable to prevent further use.
> > Since the build currently uses bash for these, a more neutral change
> > would be to change to #!/bin/bash at the same time.
> 
> That will break FreeBSD, which has no `bash` in sight.
> 
> >> RFC for 4.14.  This is a cleanup to the build system.
> > I see this already has a release-ack.  However, I would not have
> > recommended granting one at least on the basis of the description
> > above.
> >
> > I agree that this is cleanup.  But the current situation is not buggy.
> > I'm not sure exactly what the release criteria are but ISTM that this
> > patch adds risk to the release rather than removing it.
> 
> I agree that the current state of play isn't a major issue, but having
> ./configure check for bash is buggy for both uses.
> 

Even if it is not buggy, it is pointless complexity since FreeBSD would clearly have been broken all this time had there been bash-isms in the scripts.

  Paul

> ~Andrew



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

* Re: [PATCH] tools/configure: drop BASH configure variable
  2020-06-29 12:05     ` Ian Jackson
@ 2020-06-29 14:38       ` Jan Beulich
  2020-07-31 13:02         ` [PATCH] tools/configure: drop BASH configure variable [and 1 more messages] Ian Jackson
  0 siblings, 1 reply; 13+ messages in thread
From: Jan Beulich @ 2020-06-29 14:38 UTC (permalink / raw)
  To: Ian Jackson
  Cc: Andrew Cooper, Daniel De Graaf, Paul Durrant, Wei Liu, Xen-devel

On 29.06.2020 14:05, Ian Jackson wrote:
> Jan Beulich writes ("Re: [PATCH] tools/configure: drop BASH configure variable"):
>> On 26.06.2020 19:00, Andrew Cooper wrote:
>>> diff --git a/xen/xsm/flask/policy/mkaccess_vector.sh b/xen/xsm/flask/policy/mkaccess_vector.sh
>>> old mode 100644
>>> new mode 100755
>>> diff --git a/xen/xsm/flask/policy/mkflask.sh b/xen/xsm/flask/policy/mkflask.sh
>>> old mode 100644
>>> new mode 100755
>>
>> ... this may or may not take effect on the file system the sources
>> are stored on.
> 
> In what circumstances might this not take effect ?

When the file system is incapable of recording execute permissions?
It has been a common workaround for this in various projects that
I've worked with to use $(SHELL) to account for that, so the actual
permissions from the fs don't matter. (There may be mount options
to make everything executable on such file systems, but people may
be hesitant to use them.)

Jan

> IME all changes to files are properly replicated by git.  Tarball
> distributions replicate the permissions of course.
> 
> The only difficulty would be if this change were to be carried as a
> patch somewhere, by a downstream, but that seems unlikely, and can be
> avoided by that downstream not taking this patch.
> 
> Ian.
> 



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

* [PATCH v2] tools/configure: drop BASH configure variable
@ 2020-07-22 11:32 Andrew Cooper
  2020-06-26 17:00 ` [PATCH] " Andrew Cooper
  0 siblings, 1 reply; 13+ messages in thread
From: Andrew Cooper @ 2020-07-22 11:32 UTC (permalink / raw)
  To: Xen-devel; +Cc: Andrew Cooper, Daniel De Graaf, Wei Liu, Ian Jackson

This is a weird variable to have in the first place.  The only user of it is
XSM's CONFIG_SHELL, which opencodes a fallback to sh, and the only two scripts
run with this are shebang sh anyway, so don't need bash in the first place.

Make the mkflask.sh and mkaccess_vector.sh scripts executable, drop the
CONFIG_SHELL, and drop the $BASH variable to prevent further use.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Ian Jackson <Ian.Jackson@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Daniel De Graaf <dgdegra@tycho.nsa.gov>

./autogen.sh needs rerunning on commit.

v2:
 * Use $(SHELL)

There is a separate check for [BASH] in the configure script, which is
checking the requirement for the Linux hotplug scripts.  Really, that is a
runtime requirement not a build time requirement, and it is rude to require
bash in a build environment on this basis.  IMO, that check wants dropping as
well.
---
 config/Tools.mk.in                      | 1 -
 tools/configure.ac                      | 1 -
 xen/xsm/flask/Makefile                  | 8 ++------
 xen/xsm/flask/policy/mkaccess_vector.sh | 0
 xen/xsm/flask/policy/mkflask.sh         | 0
 5 files changed, 2 insertions(+), 8 deletions(-)
 mode change 100644 => 100755 xen/xsm/flask/policy/mkaccess_vector.sh
 mode change 100644 => 100755 xen/xsm/flask/policy/mkflask.sh

diff --git a/config/Tools.mk.in b/config/Tools.mk.in
index 23df47af8d..48bd9ab731 100644
--- a/config/Tools.mk.in
+++ b/config/Tools.mk.in
@@ -12,7 +12,6 @@ PYTHON              := @PYTHON@
 PYTHON_PATH         := @PYTHONPATH@
 PY_NOOPT_CFLAGS     := @PY_NOOPT_CFLAGS@
 PERL                := @PERL@
-BASH                := @BASH@
 XGETTTEXT           := @XGETTEXT@
 AS86                := @AS86@
 LD86                := @LD86@
diff --git a/tools/configure.ac b/tools/configure.ac
index 9d126b7a14..6614a4f130 100644
--- a/tools/configure.ac
+++ b/tools/configure.ac
@@ -297,7 +297,6 @@ AC_ARG_VAR([PYTHON], [Path to the Python parser])
 AC_ARG_VAR([PERL], [Path to Perl parser])
 AC_ARG_VAR([BISON], [Path to Bison parser generator])
 AC_ARG_VAR([FLEX], [Path to Flex lexical analyser generator])
-AC_ARG_VAR([BASH], [Path to bash shell])
 AC_ARG_VAR([XGETTEXT], [Path to xgetttext tool])
 AC_ARG_VAR([AS86], [Path to as86 tool])
 AC_ARG_VAR([LD86], [Path to ld86 tool])
diff --git a/xen/xsm/flask/Makefile b/xen/xsm/flask/Makefile
index 07f36d075d..50bec20a1e 100644
--- a/xen/xsm/flask/Makefile
+++ b/xen/xsm/flask/Makefile
@@ -8,10 +8,6 @@ CFLAGS-y += -I./include
 
 AWK = awk
 
-CONFIG_SHELL := $(shell if [ -x "$$BASH" ]; then echo $$BASH; \
-          else if [ -x /bin/bash ]; then echo /bin/bash; \
-          else echo sh; fi ; fi)
-
 FLASK_H_DEPEND = policy/security_classes policy/initial_sids
 AV_H_DEPEND = policy/access_vectors
 
@@ -24,14 +20,14 @@ extra-y += $(ALL_H_FILES)
 
 mkflask := policy/mkflask.sh
 quiet_cmd_mkflask = MKFLASK $@
-cmd_mkflask = $(CONFIG_SHELL) $(mkflask) $(AWK) include $(FLASK_H_DEPEND)
+cmd_mkflask = $(SHELL) $(mkflask) $(AWK) include $(FLASK_H_DEPEND)
 
 $(subst include/,%/,$(FLASK_H_FILES)): $(FLASK_H_DEPEND) $(mkflask) FORCE
 	$(call if_changed,mkflask)
 
 mkaccess := policy/mkaccess_vector.sh
 quiet_cmd_mkaccess = MKACCESS VECTOR $@
-cmd_mkaccess = $(CONFIG_SHELL) $(mkaccess) $(AWK) $(AV_H_DEPEND)
+cmd_mkaccess = $(SHELL) $(mkaccess) $(AWK) $(AV_H_DEPEND)
 
 $(subst include/,%/,$(AV_H_FILES)): $(AV_H_DEPEND) $(mkaccess) FORCE
 	$(call if_changed,mkaccess)
diff --git a/xen/xsm/flask/policy/mkaccess_vector.sh b/xen/xsm/flask/policy/mkaccess_vector.sh
old mode 100644
new mode 100755
diff --git a/xen/xsm/flask/policy/mkflask.sh b/xen/xsm/flask/policy/mkflask.sh
old mode 100644
new mode 100755
-- 
2.11.0



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

* Re: [PATCH] tools/configure: drop BASH configure variable [and 1 more messages]
  2020-06-29 14:38       ` Jan Beulich
@ 2020-07-31 13:02         ` Ian Jackson
  2020-07-31 13:30           ` Jan Beulich
  0 siblings, 1 reply; 13+ messages in thread
From: Ian Jackson @ 2020-07-31 13:02 UTC (permalink / raw)
  To: Andrew Cooper, Jan Beulich
  Cc: Xen-devel, Daniel De Graaf, Wei Liu, Paul Durrant

Jan Beulich writes ("Re: [PATCH] tools/configure: drop BASH configure variable"):
> On 29.06.2020 14:05, Ian Jackson wrote:
> > Jan Beulich writes ("Re: [PATCH] tools/configure: drop BASH configure variable"):
> >> On 26.06.2020 19:00, Andrew Cooper wrote:
> >> ... this may or may not take effect on the file system the sources
> >> are stored on.
> > 
> > In what circumstances might this not take effect ?
> 
> When the file system is incapable of recording execute permissions?
> It has been a common workaround for this in various projects that
> I've worked with to use $(SHELL) to account for that, so the actual
> permissions from the fs don't matter. (There may be mount options
> to make everything executable on such file systems, but people may
> be hesitant to use them.)

I don't think we support building from sources which have been
unpacked onto such filesystems.  Other projects which might actually
need to build on Windows or something do do this $(SHELL) thing or an
equivalent, but I don't think that's us.

But obviously that opinion of mine is not a blocker for Andy's patch
since Andy is going in the right direction, regardless.

Andrew Cooper writes ("[PATCH v2] tools/configure: drop BASH configure variable"):
> This is a weird variable to have in the first place.  The only user of it is
> XSM's CONFIG_SHELL, which opencodes a fallback to sh, and the only two scripts
> run with this are shebang sh anyway, so don't need bash in the first place.
> 
> Make the mkflask.sh and mkaccess_vector.sh scripts executable, drop the
> CONFIG_SHELL, and drop the $BASH variable to prevent further use.

In response to this commit message, I wrote:

> > Andrew Cooper writes ("[PATCH] tools/configure: drop BASH configure variable"):
> > Thanks for this cleanup.  I agree with the basic idea.
> >
> > However, did you run these scripts with dash, or review them, to check
> > for bashisms ?

And you replied:

> Yes, to all of the above.
> 
> They are both very thin wrappers (doing some argument shuffling) around
> large AWK scripts.

Can you please put this information in the commit message where it
belongs ?  As a rule we should know in future what we were thinking
when a change was made, and as I say "are shebang sh anyway, so don't
need bash in the first place" is weak evidence.

With that change,

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Thanks,
Ian.


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

* Re: [PATCH] tools/configure: drop BASH configure variable [and 1 more messages]
  2020-07-31 13:02         ` [PATCH] tools/configure: drop BASH configure variable [and 1 more messages] Ian Jackson
@ 2020-07-31 13:30           ` Jan Beulich
  2020-07-31 13:46             ` Andrew Cooper
  0 siblings, 1 reply; 13+ messages in thread
From: Jan Beulich @ 2020-07-31 13:30 UTC (permalink / raw)
  To: Ian Jackson
  Cc: Andrew Cooper, Daniel De Graaf, Paul Durrant, Wei Liu, Xen-devel

On 31.07.2020 15:02, Ian Jackson wrote:
> Jan Beulich writes ("Re: [PATCH] tools/configure: drop BASH configure variable"):
>> On 29.06.2020 14:05, Ian Jackson wrote:
>>> Jan Beulich writes ("Re: [PATCH] tools/configure: drop BASH configure variable"):
>>>> On 26.06.2020 19:00, Andrew Cooper wrote:
>>>> ... this may or may not take effect on the file system the sources
>>>> are stored on.
>>>
>>> In what circumstances might this not take effect ?
>>
>> When the file system is incapable of recording execute permissions?
>> It has been a common workaround for this in various projects that
>> I've worked with to use $(SHELL) to account for that, so the actual
>> permissions from the fs don't matter. (There may be mount options
>> to make everything executable on such file systems, but people may
>> be hesitant to use them.)
> 
> I don't think we support building from sources which have been
> unpacked onto such filesystems.  Other projects which might actually
> need to build on Windows or something do do this $(SHELL) thing or an
> equivalent, but I don't think that's us.

It's not unexpected that you think of Windows here, but my thoughts
were more towards building from sources on a CD or DVD, where iirc
execute permissions also don't exist. The latest when we have
out-of-tree builds fully working, this ought to be something that
people should be able to do, imo. (Even without out-of-tree builds,
my "next best" alternative of using a tree of symlinks to build in
would similarly have an issue with the links pointing at a mounted
CD/DVD, if the $(SHELL) wasn't present.)

Jan


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

* Re: [PATCH] tools/configure: drop BASH configure variable [and 1 more messages]
  2020-07-31 13:30           ` Jan Beulich
@ 2020-07-31 13:46             ` Andrew Cooper
  2020-07-31 13:47               ` Jan Beulich
  0 siblings, 1 reply; 13+ messages in thread
From: Andrew Cooper @ 2020-07-31 13:46 UTC (permalink / raw)
  To: Jan Beulich, Ian Jackson
  Cc: Xen-devel, Daniel De Graaf, Wei Liu, Paul Durrant

On 31/07/2020 14:30, Jan Beulich wrote:
> On 31.07.2020 15:02, Ian Jackson wrote:
>> Jan Beulich writes ("Re: [PATCH] tools/configure: drop BASH configure variable"):
>>> On 29.06.2020 14:05, Ian Jackson wrote:
>>>> Jan Beulich writes ("Re: [PATCH] tools/configure: drop BASH configure variable"):
>>>>> On 26.06.2020 19:00, Andrew Cooper wrote:
>>>>> ... this may or may not take effect on the file system the sources
>>>>> are stored on.
>>>> In what circumstances might this not take effect ?
>>> When the file system is incapable of recording execute permissions?
>>> It has been a common workaround for this in various projects that
>>> I've worked with to use $(SHELL) to account for that, so the actual
>>> permissions from the fs don't matter. (There may be mount options
>>> to make everything executable on such file systems, but people may
>>> be hesitant to use them.)
>> I don't think we support building from sources which have been
>> unpacked onto such filesystems.  Other projects which might actually
>> need to build on Windows or something do do this $(SHELL) thing or an
>> equivalent, but I don't think that's us.
> It's not unexpected that you think of Windows here, but my thoughts
> were more towards building from sources on a CD or DVD, where iirc
> execute permissions also don't exist. The latest when we have
> out-of-tree builds fully working, this ought to be something that
> people should be able to do, imo. (Even without out-of-tree builds,
> my "next best" alternative of using a tree of symlinks to build in
> would similarly have an issue with the links pointing at a mounted
> CD/DVD, if the $(SHELL) wasn't present.)

See v2.  I put $(SHELL) in, because it isn't a worthwhile use of our
time to continue arguing over this point.

~Andrew


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

* Re: [PATCH] tools/configure: drop BASH configure variable [and 1 more messages]
  2020-07-31 13:46             ` Andrew Cooper
@ 2020-07-31 13:47               ` Jan Beulich
  0 siblings, 0 replies; 13+ messages in thread
From: Jan Beulich @ 2020-07-31 13:47 UTC (permalink / raw)
  To: Andrew Cooper
  Cc: Ian Jackson, Daniel De Graaf, Paul Durrant, Wei Liu, Xen-devel

On 31.07.2020 15:46, Andrew Cooper wrote:
> On 31/07/2020 14:30, Jan Beulich wrote:
>> On 31.07.2020 15:02, Ian Jackson wrote:
>>> Jan Beulich writes ("Re: [PATCH] tools/configure: drop BASH configure variable"):
>>>> On 29.06.2020 14:05, Ian Jackson wrote:
>>>>> Jan Beulich writes ("Re: [PATCH] tools/configure: drop BASH configure variable"):
>>>>>> On 26.06.2020 19:00, Andrew Cooper wrote:
>>>>>> ... this may or may not take effect on the file system the sources
>>>>>> are stored on.
>>>>> In what circumstances might this not take effect ?
>>>> When the file system is incapable of recording execute permissions?
>>>> It has been a common workaround for this in various projects that
>>>> I've worked with to use $(SHELL) to account for that, so the actual
>>>> permissions from the fs don't matter. (There may be mount options
>>>> to make everything executable on such file systems, but people may
>>>> be hesitant to use them.)
>>> I don't think we support building from sources which have been
>>> unpacked onto such filesystems.  Other projects which might actually
>>> need to build on Windows or something do do this $(SHELL) thing or an
>>> equivalent, but I don't think that's us.
>> It's not unexpected that you think of Windows here, but my thoughts
>> were more towards building from sources on a CD or DVD, where iirc
>> execute permissions also don't exist. The latest when we have
>> out-of-tree builds fully working, this ought to be something that
>> people should be able to do, imo. (Even without out-of-tree builds,
>> my "next best" alternative of using a tree of symlinks to build in
>> would similarly have an issue with the links pointing at a mounted
>> CD/DVD, if the $(SHELL) wasn't present.)
> 
> See v2.  I put $(SHELL) in, because it isn't a worthwhile use of our
> time to continue arguing over this point.

I had seen you did; I was merely replying back to Ian's comments.

Jan


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

end of thread, back to index

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-22 11:32 [PATCH v2] tools/configure: drop BASH configure variable Andrew Cooper
2020-06-26 17:00 ` [PATCH] " Andrew Cooper
2020-06-29  7:41   ` Paul Durrant
2020-06-29  8:41   ` Jan Beulich
2020-06-29 12:05     ` Ian Jackson
2020-06-29 14:38       ` Jan Beulich
2020-07-31 13:02         ` [PATCH] tools/configure: drop BASH configure variable [and 1 more messages] Ian Jackson
2020-07-31 13:30           ` Jan Beulich
2020-07-31 13:46             ` Andrew Cooper
2020-07-31 13:47               ` Jan Beulich
2020-06-29 13:34   ` [PATCH] tools/configure: drop BASH configure variable Ian Jackson
2020-06-29 13:51     ` Andrew Cooper
2020-06-29 14:00       ` Paul Durrant

Xen-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/xen-devel/0 xen-devel/git/0.git
	git clone --mirror https://lore.kernel.org/xen-devel/1 xen-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 xen-devel xen-devel/ https://lore.kernel.org/xen-devel \
		xen-devel@lists.xenproject.org xen-devel@lists.xen.org
	public-inbox-index xen-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.xenproject.lists.xen-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git