diff mbox

[v2] sotruss: convert from ksh to bash

Message ID 1513046.Pq6yaohzbx@vapier
State Committed
Headers show

Commit Message

Mike Frysinger March 13, 2014, 10:14 p.m. UTC
This script works fine under bash (which we already require), so tweak
the code to be pure bash.

Signed-off-by: Mike Frysinger <vapier@gentoo.org>

2014-01-23  Mike Frysinger  <vapier@gentoo.org>

	* elf/Makefile: Delete $(have-ksh) check.
	($(objpfx)sotruss): Change KSH to BASH.
	* elf/sotruss.ksh: Rename to ...
	* elf/sotruss.sh: ... this.  Change @KSH@ to @BASH@.  Change
	function style to match bash.
---
 elf/Makefile                    | 6 +++---
 elf/{sotruss.ksh => sotruss.sh} | 8 ++++----
 2 files changed, 7 insertions(+), 7 deletions(-)
 rename elf/{sotruss.ksh => sotruss.sh} (98%)

Comments

Roland McGrath March 13, 2014, 10:30 p.m. UTC | #1
> This script works fine under bash (which we already require), so tweak
> the code to be pure bash.

I don't think we've documented anywhere that our installed scripts require
bash.  Some of them already do, so this is not something you're changing
here.  But before "we already require foo" should generally be considered
an adequate rationale for a change by itself, it should be a requirement
that we have made explicit and documented, rather than just a de facto
requirement that might have snuck in without clear intent.

I think removing the ksh check is fine.

I also think the changes to the script are fine, but they are purely
cosmetic and wholly unnecessary.  Your "pure bash" and "to match bash"
comments seem to imply that "function foo {" style is somehow less
supported by bash than "foo() {", which AFAIK has never been the case.
Mike Frysinger March 13, 2014, 10:50 p.m. UTC | #2
On Thu 13 Mar 2014 15:30:05 Roland McGrath wrote:
> > This script works fine under bash (which we already require), so tweak
> > the code to be pure bash.
> 
> I don't think we've documented anywhere that our installed scripts require
> bash.  Some of them already do, so this is not something you're changing
> here.  But before "we already require foo" should generally be considered
> an adequate rationale for a change by itself, it should be a requirement
> that we have made explicit and documented, rather than just a de facto
> requirement that might have snuck in without clear intent.

sotruss itself doesn't work if you don't have bash or ksh.  so what i am 
changing is that ksh is no longer supported.

afaict, the only reason bash is required is for localization of the messages.  
but even that works w/POSIX shell, albeit with slightly funky display -- you 
get a literal $ in the output of each string.

> I also think the changes to the script are fine, but they are purely
> cosmetic and wholly unnecessary.  Your "pure bash" and "to match bash"
> comments seem to imply that "function foo {" style is somehow less
> supported by bash than "foo() {", which AFAIK has never been the case.

true, the current code works under bash unmodified.  really i'm fixing the 
code to be portable to pretty much any shell.  force of habit that i see this 
style and think "that's not valid".

so i guess i'll rewrite this to say "drop ksh support and add basic support 
for any POSIX shell".
-mike
Joseph Myers March 13, 2014, 11:36 p.m. UTC | #3
On Thu, 13 Mar 2014, Mike Frysinger wrote:

> afaict, the only reason bash is required is for localization of the messages.  
> but even that works w/POSIX shell, albeit with slightly funky display -- you 
> get a literal $ in the output of each string.

We know we want to avoid $"" (bug 13853) - some patches were posted in Nov 
2012.  I suppose those need revising to take account of any patch reviews 
and updating as needed.
Patrick 'P. J.' McDermott March 14, 2014, 12:31 a.m. UTC | #4
On 2014-03-13 19:36, Joseph S. Myers wrote:
> On Thu, 13 Mar 2014, Mike Frysinger wrote:
> 
>> afaict, the only reason bash is required is for localization of the messages.  
>> but even that works w/POSIX shell, albeit with slightly funky display -- you 
>> get a literal $ in the output of each string.
> 
> We know we want to avoid $"" (bug 13853) - some patches were posted in Nov 
> 2012.  I suppose those need revising to take account of any patch reviews 
> and updating as needed.

Yes, I (the submitter of those patches) have been meaning to revisit
this.  I may post an RFC and/or patches for this later in the 2.20 or
2.21 cycle, unless someone beats me to it.

As Mike notes, scripts with `$""` already run fine on any shell, just
with `$` in the output.  (And as noted in BZ #13853, xgettext prints
lots of warnings.)

AFAIK sotruss is the last install-time or run-time script that requires
a specific shell (now that ldd, tzselect, and a couple of scripts under
sysdeps/unix/ have recently become more portable).  So with Mike's
patch, Bash is no longer in any way required to install or run glibc.
Andreas Schwab March 14, 2014, 6:51 a.m. UTC | #5
Roland McGrath <roland@hack.frob.com> writes:

> I also think the changes to the script are fine, but they are purely
> cosmetic and wholly unnecessary.  Your "pure bash" and "to match bash"
> comments seem to imply that "function foo {" style is somehow less
> supported by bash than "foo() {", which AFAIK has never been the case.

function foo is non-POSIX (POSIX reserves the function keyword, but
doesn't add any meaning to it).  Bash only supports function for
ksh-compatibility, but otherwise prefers the POSIX syntax.

Andreas.
diff mbox

Patch

diff --git a/elf/Makefile b/elf/Makefile
index 2db3c98..8abc60b 100644
--- a/elf/Makefile
+++ b/elf/Makefile
@@ -93,7 +93,7 @@  pldd-modules := xmalloc
 # To find xmalloc.c and xstrdup.c
 vpath %.c ../locale/programs
 
-ifeq ($(have-ksh)$(build-shared),yesyes)
+ifeq ($(build-shared),yes)
 extra-objs += sotruss-lib.os sotruss-lib.so
 install-others += $(inst_auditdir)/sotruss-lib.so
 install-bin-script += sotruss
@@ -104,8 +104,8 @@  $(objpfx)sotruss-lib.so: $(objpfx)sotruss-lib.os
 $(objpfx)sotruss-lib.so: $(common-objpfx)libc.so $(objpfx)ld.so \
 	$(common-objpfx)libc_nonshared.a
 
-$(objpfx)sotruss: sotruss.ksh $(common-objpfx)config.make
-	sed -e 's%@KSH@%$(KSH)%g' \
+$(objpfx)sotruss: sotruss.sh $(common-objpfx)config.make
+	sed -e 's%@BASH@%$(BASH)%g' \
 	    -e 's%@VERSION@%$(version)%g' \
 	    -e 's%@TEXTDOMAINDIR@%$(msgcatdir)%g' \
 	    -e 's%@PREFIX@%$(prefix)%g' \
diff --git a/elf/sotruss.ksh b/elf/sotruss.sh
similarity index 98%
rename from elf/sotruss.ksh
rename to elf/sotruss.sh
index 371a70b..a408b58 100755
--- a/elf/sotruss.ksh
+++ b/elf/sotruss.sh
@@ -1,4 +1,4 @@ 
-#! @KSH@
+#! @BASH@
 # Copyright (C) 2011-2014 Free Software Foundation, Inc.
 # This file is part of the GNU C Library.
 
@@ -28,7 +28,7 @@  unset SOTRUSS_NOINDENT
 SOTRUSS_WHICH=$$
 lib='@PREFIX@/$LIB/audit/sotruss-lib.so'
 
-function do_help {
+do_help() {
   echo $"Usage: sotruss [OPTION...] [--] EXECUTABLE [EXECUTABLE-OPTION...]
   -F, --from FROMLIST     Trace calls from objects on FROMLIST
   -T, --to TOLIST         Trace calls to objects on TOLIST
@@ -51,13 +51,13 @@  function do_help {
   exit 0
 }
 
-function do_missing_arg {
+do_missing_arg() {
   printf >&2 $"%s: option requires an argument -- '%s'\n" sotruss "$1"
   printf >&2 $"Try \`%s --help' or \`%s --usage' for more information.\n" sotruss sotruss
   exit 1
 }
 
-function do_ambiguous {
+do_ambiguous() {
   printf >&2 $"%s: option is ambiguous; possibilities:"
   while test $# -gt 0; do
     printf >&2 " '%s'" $1