Re: [PATCH 1/3]: Replace kernel/timeconst.pl with kernel/timeconst.sh

[Rob Landley <rob@landley.net>]

On Friday 02 January 2009 03:04:39 Sam Ravnborg wrote:
> Hi Rob.
>
> On Fri, Jan 02, 2009 at 02:13:30AM -0600, Rob Landley wrote:
> > From: Rob Landley <rob@landley.net>
> >
> > Replace kernel/timeconst.pl with kernel/timeconst.sh.  The new shell
> > script is much simpler, about 1/4 the size, and runs on Red Hat 9 from
> > 2003.
>
> This part of the changelog is OK except that is fails to
> address why we want to get away from perl.

You mean "The new shell script is much simpler, about 1/4 the size, runs on 
Red Hat 9 from 2003, and isn't perl?" :)

> Please drop the remining part of the changelog (but not the s-o-b).

ok.

> > Peter Anvin added this perl to 2.6.25.  Before that, the kernel had never
> > required perl to build.
> >
> > Signed-off-by: Rob Landley <rob@landley.net>
> > ---
> >
> >  kernel/Makefile     |    4
> >  kernel/timeconst.pl |  378 ------------------------------------------
> >  kernel/timeconst.sh |   91 ++++++++++
> >  3 files changed, 93 insertions(+), 380 deletions(-)
> >
> > diff -r d0c7611dcfd6 kernel/Makefile
> > --- a/kernel/Makefile	Tue Dec 30 17:48:25 2008 -0800
> > +++ b/kernel/Makefile	Fri Jan 02 00:10:44 2009 -0600
> > @@ -115,7 +115,7 @@
> >  $(obj)/time.o: $(obj)/timeconst.h
> >
> >  quiet_cmd_timeconst  = TIMEC   $@
> > -      cmd_timeconst  = $(PERL) $< $(CONFIG_HZ) > $@
> > +      cmd_timeconst  = $(CONFIG_SHELL) $< $(CONFIG_HZ) > $@
> >  targets += timeconst.h
> > -$(obj)/timeconst.h: $(src)/timeconst.pl FORCE
> > +$(obj)/timeconst.h: $(src)/timeconst.sh FORCE
> >  	$(call if_changed,timeconst)
>
> OK
>
> > --- /dev/null	1969-12-31 00:00:00.000000000 -0600
> > +++ hg/kernel/timeconst.sh	2009-01-01 23:53:17.000000000 -0600
> > @@ -0,0 +1,91 @@
> > +#!/bin/bash
> > +
> > +if [ $# -ne 1 ]
> > +then
> > +	echo "Usage: timeconst.sh HZ"
> > +	exit 1
> > +fi
>
> That usage is useless. Either extend it or spend a few lines
> in the shell script explainign what the shell script is supposed to do.

Do you mean something more like:

echo "Usage: timeconst.sh HZ"
echo
echo "Generates a header file of constants for converting between decimal"
echo "HZ timer ticks and milisecond or microsecond delays"

I'm happy turning it into a comment instead, I just found a quick check that 
I'd remembered to type an argument useful during debugging. :)

> > +
> > +HZ=$1
> > +
> > +# Sanity test: even the shell in Red Hat 9 (circa 2003) supported 64 bit
> > math. +
> > +[ $((1<<32)) -lt 0 ] && exit 1
> > +
>
> If it fails then add an error message explaining why. So if we get reports
> that this fails then we at least can see something like:
> "timeconst noticed that the shell did not support 64 bit math - stop"

Ok.

> > +# Output start of header file
> > +
> > +cat << EOF
> > +/* Automatically generated by kernel/timeconst.sh */
> > +/* Conversion constants for HZ == $HZ */
> > +
> > +#ifndef KERNEL_TIMECONST_H
> > +#define KERNEL_TIMECONST_H
>
> Please use __KERNEL_TIMECONST_H. The two underscores are standard.

Sure thing.  (I was just copying the perl there, I'll post an updated patch 
after I get some sleep.)

> > +
> > +#include <linux/param.h>
> > +#include <linux/types.h>
> > +
> > +#if HZ != $HZ
> > +#error "kernel/timeconst.h has the wrong HZ value!"
> > +#endif
> > +
> > +EOF
> > +
> > +# For both Miliseconds and Microseconds
> > +
> > +for i in "MSEC 1000" "USEC 1000000"
> > +do
> > +	NAME=$(echo $i | awk '{print $1}')
> > +	PERIOD=$(echo $i | awk '{print $2}')
> > +
> > +	# Find greatest common denominator (using Euclid's algorithm)
> > +
> > +	A=$HZ
> > +	B=$PERIOD
> > +
> > +	while [ $B -ne 0 ]
> > +	do
> > +		C=$(($A%$B))
> > +		A=$B
> > +		B=$C
> > +	done
> > +
> > +	GCD=$A
> > +
> > +	# Do this for each direction (HZ_TO_PERIOD and PERIOD_TO_HZ)
> > +
> > +	for DIRECTION in 0 1
> > +	do
> > +		if [ $DIRECTION -eq 0 ]
> > +		then
> > +			CONVERT="HZ_TO_${NAME}"
> > +			FROM=$HZ
> > +			TO=$PERIOD
> > +		else
> > +			CONVERT="${NAME}_TO_HZ"
> > +			FROM=$PERIOD
> > +			TO=$HZ
> > +		fi
> > +
> > +		# How many shift bits give 32 bits of significant data?
> > +
> > +		SHIFT=0
> > +		while [ $(( (($TO<<$SHIFT)+$FROM-1)/$FROM )) -lt $((1<<31)) ]
> > +		do
> > +			SHIFT=$(( $SHIFT+1 ))
> > +		done
> > +
> > +		MUL32=$(( (($TO<<$SHIFT)+$FROM-1)/$FROM ))
> > +		MUL32=$(printf %x $MUL32)
> > +		echo "#define ${CONVERT}_MUL32	U64_C(0x$MUL32)"
> > +		ADJ32=$(($FROM/$GCD))
> > +		ADJ32=$(( (($ADJ32-1)<<$SHIFT)/$ADJ32 ))
> > +		ADJ32=$(printf %x $ADJ32)
> > +		echo "#define ${CONVERT}_ADJ32	U64_C(0x$ADJ32)"
> > +		echo "#define ${CONVERT}_SHR32	$SHIFT"
> > +		echo "#define ${CONVERT}_NUM		U64_C($(($TO/$GCD)))"
> > +		echo "#define ${CONVERT}_DEN		U64_C($(($FROM/$GCD)))"
> > +	done
> > +done
>
> Is it a shell limitation that all spaces around operators are missing?
> Makes it hard to read..

No, I was just trying to make sure I didn't go over the 80 char limit.  
(Several temporary variables are there primarily because the style guide says 
indentation should be via 8 space tabs.)

I'll add some more spaces on the respin.

>
> You should use trap in your shell script so we do not end up with a
> partially generated file.
> Or you should change the rule in the Makefile to use
> a temperary file and then rename it.
>
> We have similar bugs in other places - but no need to add
> in more places.

Well, preserve in this case. :)

I can add an "EXIT" trap and then disarm it later, but the output is 
redirected in the makefile, so generating an error won't remove the output if 
you run again.  Perhaps the file to create should be fed in as a second 
argument to the script, so the trap can "rm" the output file if it triggers?

I'll poke at it in the morning.

Thanks for the feedback,

Rob