Re: [PATCH/RFC] eradicate bashisms in scripts/patch-kernel

[Randy Dunlap <rdunlap@xenotime.net>]

On Thu, 1 Nov 2007 23:16:06 +0100 Andreas Mohr wrote:

[add Herbert]

> Hi,
> 
> On Thu, Nov 01, 2007 at 08:24:57AM -0700, Randy Dunlap wrote:
> > On Thu, 1 Nov 2007 13:11:33 +0100 Andreas Mohr wrote:
> > > I'll think a bit more about these couple changed places (and whether
> > > this still truly works as intended) and mail a patch then.
> > > 
> > > (and a big NOTE: I'm no POSIX vs. non-POSIX shell guru at all, only a
> > > semi-versed shell script writer, thus these changes should be reviewed
> > > quite thoroughly)
> > 
> > Neither am I.  I read those web pages quickly yesterday, so after
> > you read them, we can discuss more and/or review more patches.
> 
> OK, next iteration (v2).
> 
> 
> Make the patch-kernel shell script sufficiently compatible with POSIX shells.
> 
> 
> Changes since v1:
> - don't actually change string quoting
> - prepend ./ at mktemp step already
> - remove added superfluous spaces in arithmetic expression
> - don't remove double braces in STOPSUBLEVEL evaluation,
>   since these are integer values to be compared
> 
> Full ChangeLog:
> - replaced non-standard "==" by standard "="
OK

> - replaced non-standard "source" statement by POSIX "dot" statement
OK

> - POSIX shell local file lookup needs ./ prepended, thus have mktemp
>   use this from the beginning and comment it properly

I would like the patch description to say something like:

Use ./ as a prefix on the TEMP filename so that the current search
PATH will not be searched and thus another file with this same
(pseudo random) name won't be used accidentally.

> - replace non-standard bash string parsing by sed expression
>   (is the sed syntax ok? correct? strict enough?)

I think that this is the part that bothers me.  I can't find
anything at
http://www.opengroup.org/onlinepubs/000095399/utilities/xcu_chap02.html
that says that this:
	EXTRAVER=${EXTRAVER%%[[:punct:]]*}

is invalid or even optional syntax.  OTOH, it does list such syntax,
so why are we working around this syntax?
Please point out to me what I am missing...

> - added missing $ signs to shell variable names
OK


> About the missing $ signs:
> http://www.opengroup.org/onlinepubs/009695399/utilities/xcu_chap02.html#tag_02_14
> says:
> "If the shell variable x contains a value that forms a valid integer
> constant, then the arithmetic expansions
>  "$((x))" and "$(($x))" shall return the same value."
> 
> Hmm, well, seems dash doesn't... (syntax error).
> Thus I still needed to add the $ signs despite opengroup.org specifying
> it differently.

Herbert?

> Updated version verified again to now work with both bash and dash
> on Debian stable.
> 
> Patch intended for inclusion in -mm, once it has survived some reviews.
> 
> Thanks.
> 
> Signed-off-by: Andreas Mohr <andi@lisas.de>
> 
> --- linux-2.6.23/scripts/patch-kernel.orig	2007-11-01 22:51:34.000000000 +0100
> +++ linux-2.6.23/scripts/patch-kernel	2007-11-01 22:10:14.000000000 +0100
> @@ -65,7 +65,7 @@
>  patchdir=${2-.}
>  stopvers=${3-default}
>  
> -if [ "$1" == -h -o "$1" == --help -o ! -r "$sourcedir/Makefile" ]; then
> +if [ "$1" = -h -o "$1" = --help -o ! -r "$sourcedir/Makefile" ]; then
>  cat << USAGE
>  usage: $PNAME [-h] [ sourcedir [ patchdir [ stopversion ] [ -acxx ] ] ]
>    source directory defaults to /usr/src/linux,
> @@ -182,10 +182,12 @@
>  }
>  
>  # set current VERSION, PATCHLEVEL, SUBLEVEL, EXTRAVERSION
> -TMPFILE=`mktemp .tmpver.XXXXXX` || { echo "cannot make temp file" ; exit 1; }
> +# sourcing $TMPFILE.1 below needs lookup in local directory in a POSIX shell,
> +# thus prepend ./ to mktemp argument
> +TMPFILE=`mktemp ./.tmpver.XXXXXX` || { echo "cannot make temp file" ; exit 1; }
>  grep -E "^(VERSION|PATCHLEVEL|SUBLEVEL|EXTRAVERSION)" $sourcedir/Makefile > $TMPFILE
>  tr -d [:blank:] < $TMPFILE > $TMPFILE.1
> -source $TMPFILE.1
> +. $TMPFILE.1
>  rm -f $TMPFILE*
>  if [ -z "$VERSION" -o -z "$PATCHLEVEL" -o -z "$SUBLEVEL" ]
>  then
> @@ -202,13 +204,7 @@
>  EXTRAVER=
>  if [ x$EXTRAVERSION != "x" ]
>  then
> -	if [ ${EXTRAVERSION:0:1} == "." ]; then
> -		EXTRAVER=${EXTRAVERSION:1}
> -	else
> -		EXTRAVER=$EXTRAVERSION
> -	fi
> -	EXTRAVER=${EXTRAVER%%[[:punct:]]*}
> -	#echo "$PNAME: changing EXTRAVERSION from $EXTRAVERSION to $EXTRAVER"
> +	EXTRAVER=`echo $EXTRAVERSION|sed -s 's/^[\.]\?\([^[:punct:]]*\).*/\1/'`
>  fi
>  
>  #echo "stopvers=$stopvers"
> @@ -251,16 +247,16 @@
>  do
>      CURRENTFULLVERSION="$VERSION.$PATCHLEVEL.$SUBLEVEL"
>      EXTRAVER=
> -    if [ $stopvers == $CURRENTFULLVERSION ]; then
> +    if [ $stopvers = $CURRENTFULLVERSION ]; then
>          echo "Stopping at $CURRENTFULLVERSION base as requested."
>          break
>      fi
>  
> -    SUBLEVEL=$((SUBLEVEL + 1))
> +    SUBLEVEL=$(($SUBLEVEL + 1))
>      FULLVERSION="$VERSION.$PATCHLEVEL.$SUBLEVEL"
>      #echo "#___ trying $FULLVERSION ___"
>  
> -    if [ $((SUBLEVEL)) -gt $((STOPSUBLEVEL)) ]; then
> +    if [ $(($SUBLEVEL)) -gt $(($STOPSUBLEVEL)) ]; then
>  	echo "Stopping since sublevel ($SUBLEVEL) is beyond stop-sublevel ($STOPSUBLEVEL)"
>  	exit 1
>      fi
> @@ -297,7 +293,7 @@
>  if [ x$gotac != x ]; then
>    # Out great user wants the -ac patches
>  	# They could have done -ac (get latest) or -acxx where xx=version they want
> -	if [ $gotac == "-ac" ]; then
> +	if [ $gotac = "-ac" ]; then
>  	  # They want the latest version
>  		HIGHESTPATCH=0
>  		for PATCHNAMES in $patchdir/patch-${CURRENTFULLVERSION}-ac*\.*
> -

Thanks,
---
~Randy