On Wed, 2010-07-07 at 14:03 +1000, Allan McRae wrote:
> Here is a quick review on all these patches. I recommend that the lvm
> and crypttab changes get a decent amount of testing before these go live
> as they are the biggest changes being done.
>
>
> Tighten up the console size finding code a bit.
> Add some white space in test construct:
> if ((STAT_COL==0)); then
> if (( STAT_COL == 0 )); then
> and throughout these patches
Maybe when all the other bugs are fixed. :)
> Simplify the code that clears USECOLOR.
> The following condition is removed with no commit message to explain why
> if [ $? = 3 ]; then
> TERM_COLOURS=8
An exitval of 3 means tput has no idea what terminal type we are running
on, and I figured that it is after to default to not using colorized
output in that case.
> Replace trivial use of grep with bash regex conditional.
> - if [ -n "$CONSOLEMAP" ] && echo "$LOCALE" | /bin/grep -qi utf ; then
> + [[ $CONSOLEMAP && $LOCALE =~ UTF|utf ]] && CONSOLEMAP=""
>
> Use ... && ${LOCALE,,} == utf ]] to accurately replicate the grep
Hmmm... I had not seen that parameter expansion before. New to bash
4.1?
> Replace slightly too long echo staement with a here document.
> ^^ typo
> I actually find the echo more readable
Well, then Thomas can keep it or drop it as he prefers.
> Change the daemon runnign loop to use a case statement.
> Quote daemon names.
> Merge these commits
>
>
> Both rc.single and rc.shutdown use the same code to kill everything.
> + # $1 = where we are being called from.
> + # This is used to determine which hooks to run.
> -> Add separater line here...
> + # Find daemons NOT in the DAEMONS array. Shut these down first
>
> Why has this been removed:
> -if [ -x /etc/rc.local.shutdown ]; then
> - /etc/rc.local.shutdown
> -fi
> Ah... it has been moved to another place in another commit. Please
> document these sorts of changes in your commit message and preferably do
> the entire move in one commit.
Will fix.
> Flatten LVM deactivation if block in rc.shutdown.
> This change does not do the same thing and I do not see where it gets
> replicated
> -if [ -f /etc/crypttab -a -n "$(/bin/grep -v ^# /etc/crypttab |
> /bin/grep -v ^$)" ]; then
> +if [[ -f /etc/crypttab ]]; then
All the second bit of that test does is to see if there is actual
content in /etc/crypttab. I handle that in the read loop by zapping
comments and blank lines with parameter expansion and conditional checks
instead -- the greps end up reading the whole file anyways.
Replicated via parameter expansion and conditionals:
[[ $line && ${line:0:1} != '#' ]] || continue
eval nspo=("${line%#*}")
> Also another:
> +if [[ $USELVM =~ yes|YES
> -> ${USELVM,,} == yes
>
>
> bashify bringing up the loopback adaptor.
> Add a commit message as that is doing a lot more than bashifing.
Already fixed in the last round of rebasing.
> Bashify locale setting.
> $LOCALE =~ utf|UTF
>
>
> Allan
--
Victor Lowther
LPIC2 UCP RHCE
