| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <200901020600.47847.rob@landley.net>
Date: Fri, 2 Jan 2009 06:00:47 -0600
From: Rob Landley <rob@...dley.net>
To: Sam Ravnborg <sam@...nborg.org>
Cc: Embedded Linux mailing list <linux-embedded@...r.kernel.org>,
linux-kernel@...r.kernel.org,
Andrew Morton <akpm@...ux-foundation.org>,
"H. Peter Anvin" <hpa@...or.com>
Subject: Re: [PATCH 1/3]: Replace kernel/timeconst.pl with kernel/timeconst.sh
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@...dley.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@...dley.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists