MAKEDEV (and code highlighted by PR bin/52280)

[Robert Elz <kre%munnari.OZ.AU@localhost>]

The recent PR bin/52280 which discovered a piece of lack of thought in
my sh $LINENO code, was triggered by this section of code in MAKEDEV ...

        # Now we need exactly one node-creation method.
        case $(( $($do_mtree && echo 1 || echo 0) + \
                $($do_pax && echo 1 || echo 0) + \ 
                $($do_mknod && echo 1 || echo 0) + \
                $($do_specfile && echo 1 || echo 0) ))
        in
        1)      : OK ;;
        *)      die "-m, -p, -s, and -t options are mutually exclusive" ;;
        esac

First, let me say that there's nothing technically wrong with that code,
and I wouldn't change it before the bug was fixed (though any of a number
of changes could have hidden the bug, so it is good that it was written
just this way).

But now the bug is fixed ...

#1:   the \'s at end of the lines there are totally unnecessary (those were,
leaving aside my role in writing the code, the primary bug trigger, dealing
with line counting and elided newlines, accurately, is tricky.)

The general rule is that you need a \ at end of line in sh if it is possible
that the command could (correctly) end at that point.

Since here we're in the middle of a $(( )) expression, nothing "ends" until
we get the )) so we can never need a \ newline in there

The same applies in a $( ) command substitution which won't end until the )
is seen, though there a command being executed might need to be continued
over multiple lines, and then that might need a \ newline.

Similarly after any of the | || && operators, which need a subsequent
command to be complete, and after most reserved words (if while... even !)
some other command is required, so no \ is needed before a newline.


#2:   running 4 command substitutions in order to generate an arith expression
seems excessive, if we wanted to do it that way, something more like

        case $(( $( echo 0
                    $do_mtree && echo + 1
                    $do_pax && echo + 1
                    $do_mknod && echo + 1
                    $do_specfile && echo + 1
                  )
              ))
        in

would be better, just one cmdsub to achieve the same result.
[Irrelevant aside, in general it is safer to use printf(1) than echo(1)
 but here it doesn't matter.]


#3:   all this looks like an attempt to program sh with pseudo-C code,
so rather than either of those:

        case ",${do_mtree},${do_pax},${do_mknod},$do_specfile)," in
        ( *,true,*,true,* ) 
                die "-m, -p, -s, and -t options are mutually exclusive"
                ;;
        esac

looks more like the sh way.   Here the quotes around the case word look
unneeded (and it is correct that the effect would be the same without them
as each of those variables is just "true" or "false") it is better to
include them, not only "just in case" the variable happens to contain white
space or a meta-character ('*' etc), neither of which is going to happen
here, and we know that - but also because when you use the "" you are telling
the shell that you do not want filename, or field splitting to happen, and
with that knowledge, the shell has less work to do and is more efficient.
So, do always quote things unless you really need the unquoted result, adding
quotes does not make the shell work harder, it reduces the workload.

In this, earlier code has already assured that at least one of the
do_xxx's is true), but:


#4:  And then speaking of that earlier code, it is...

        if ! ( $do_mtree || $do_pax || $do_mknod || $do_specfile ); then
		# code to set one of them to true
	fi

In that the grouping is needed (though we could apply De Morgan's law to
remove it) but it doesn't need to be a sub-shell, better would be ...

        if ! { $do_mtree || $do_pax || $do_mknod || $do_specfile ;}; then

That is, in sh, use { } where you would use ( ) in C or mathematics, but
just remember that while ( and ) are operators, { and } are just reserved
words, so need more care in how they are placed.

But even better, we can combine that test into the test for more than
one option set, by adding...

        ( ,false,false,false,false, )
		# code to set one of them
		;;

to the case statement above.  Again, that's the sh way.


#5:  But now writing about unnecessary sub-shells, a little later we have this:

        # do_mtree or do_pax internally implies do_specfile.
        # This happens after checking for mutually-exclusive options.
        if ($do_mtree || $do_pax) && ! $do_specfile; then
                do_specfile=true
                opts="${opts} -s"
        fi

where again a sub-shell is not needed, and we could use

        if { $do_mtree || $do_pax; } && ! $do_specfile; then

(note the change of spacing and the extra ';')   But that's still C programming
in sh, in sh the operator precedence rules are different than C, and the
grouping is not needed here at all, just

        if $do_mtree || $do_pax && ! $do_specfile; then

works fine - sh evaluates and-or lists left to right, making a decision
at each operator whether the next command (pipeline) needs to be evaluated
or not.  So if do_mtree is true we do not evaluate do_pax, and proceed to
! do_specfile.  If do_mtree is false, we evaluate do_pax, and if that is true,
we do the do_specfile case (and invert the result).   If both do_mtree and
do_pax are both false then at the && we skip the do_spacfile expansion
(and its !).

Since in any of those cases we are then at the end of the and-or list
whatever status we're left with from the last command evaluated is the result.

That's not how C would evaluate it, but this is not C.

So, given all that, does anyone object if I commit the following patch
(to src/etc/MAKEDEV.tmpl) ?

kre

Index: MAKEDEV.tmpl
===================================================================
RCS file: /cvsroot/src/etc/MAKEDEV.tmpl,v
retrieving revision 1.183
diff -u -r1.183 MAKEDEV.tmpl
--- MAKEDEV.tmpl	8 Sep 2016 15:00:08 -0000	1.183
+++ MAKEDEV.tmpl	7 Jun 2017 18:04:54 -0000
@@ -567,7 +567,9 @@
 	# mknod is just very slow, because the shell has to fork for
 	# each device node.
 	#
-	if ! ( $do_mtree || $do_pax || $do_mknod || $do_specfile ); then
+
+	case ",${do_mtree},${do_pax},${do_mknod},$do_specfile)," in
+	( ,false,false,false,false, )
 		if check_mtree "${TOOL_MTREE}"; then
 			do_mtree=true
 		elif check_pax "${TOOL_PAX}"; then
@@ -575,16 +577,10 @@
 		else
 			do_mknod=true
 		fi
-	fi
-
-	# Now we need exactly one node-creation method.
-	case $(( $($do_mtree && echo 1 || echo 0) + \
-		$($do_pax && echo 1 || echo 0) + \
-		$($do_mknod && echo 1 || echo 0) + \
-		$($do_specfile && echo 1 || echo 0) ))
-	in
-	1)	: OK ;;
-	*)	die "-m, -p, -s, and -t options are mutually exclusive" ;;
+		;;
+	( *,true,*,true,* )
+		die "-m, -p, -s, and -t options are mutually exclusive"
+		;;
 	esac
 
 	# If we are using mknod, then decide what options to pass it.
@@ -599,7 +595,7 @@
 
 	# do_mtree or do_pax internally implies do_specfile.
 	# This happens after checking for mutually-exclusive options.
-	if ($do_mtree || $do_pax) && ! $do_specfile; then
+	if $do_mtree || $do_pax && ! $do_specfile; then
 		do_specfile=true
 		opts="${opts} -s"
 	fi