Re: [Astlinux-devel] [Astlinux-commits] SF.net SVN: astlinux:[2059] trunk/target/generic/target_ske

Some code review comments inline...


dha...@us... wrote:
> Revision: 2059
>           http://astlinux.svn.sourceforge.net/astlinux/?rev=2059&view=rev
> Author:   dhartman
> Date:     2008-11-10 18:58:46 +0000 (Mon, 10 Nov 2008)
>
> Log Message:
> -----------
> add script to manage astlinux image upgrades from Lonnie's web interface
>
> Added Paths:
> -----------
>     trunk/target/generic/target_skeleton/usr/sbin/upgrade-run-image
>
> Added: trunk/target/generic/target_skeleton/usr/sbin/upgrade-run-image
> ===================================================================
> --- trunk/target/generic/target_skeleton/usr/sbin/upgrade-run-image	                        (rev 0)
> +++ trunk/target/generic/target_skeleton/usr/sbin/upgrade-run-image	2008-11-10 18:58:46 UTC (rev 2059)
> @@ -0,0 +1,325 @@
> +#!/bin/sh
> +# red color error message for web interface
> +RED=1
> +# orange color error message for web interface
> +ORANGE=2
> +# green color message for web interface
> +GREEN=0
> +
> +mount_oldroot_cdrom() {
> +  if `mount | grep -q "/oldroot/cdrom"`; then
> +    BASE=/oldroot/cdrom
> +  else
> +    if [ -x /sbin/findfs ]; then
> +      LABEL=`/sbin/findfs LABEL=RUNNIX 2>/dev/null`
> +      if [ -n "$LABEL" ]; then
> +        mount -t vfat "$LABEL" /oldroot/cdrom
> +      fi
> +    fi
> +    if `mount | grep -q "/oldroot/cdrom"`; then
> +      BASE=/oldroot/cdrom
> +    else
> +      return 1
> +    fi
> +  fi
> +  return 0
> +}
> +
> +unmount_oldroot_cdrom() {
> +  mount -o ro,remount /oldroot/cdrom >/dev/null 2>&1
>   

On commands that you're not expecting to ever fail, like this one, you 
really do want to see stderr when it does fail.

So I'd recommend using:

mount -o ro,remount /oldroot/cdrom >/dev/null

and keep any messages to stderr on console.  Leaving the `` out means 
"don't capture the output... just pay attention to the exit value".


> +}
> +
> +mount_tmpfs_up() {
> +  mkdir /tmp/up >/dev/null 2>&1
>   

Ditto.

> +
> +  if `mount -t tmpfs -o size=56m none /tmp/up >/dev/null 2>&1`; then
>   

Ditto.

Also, you can use $? to get the exit code, etc. i.e.

if ! mount -t tmpfs -o size=56m none /tmp/up >/dev/null; then

or:

mount -t tmpfs -o size=56m none /tmp/up >/dev/null
if [ $? -ne 0 ]; then
  rmdir /tmp/up
  return 1
fi


> +    :
> +  else
> +    rmdir /tmp/up
> +    return 1
> +  fi
> +  return 0
> +}
> +
> +unmount_tmpfs_up() {
> +  cd /tmp
> +  umount /tmp/up
> +  rmdir /tmp/up
> +}
> +
> +version_from_astimg() {
> +  RUN_VER=""
> +  for i in `cat /proc/cmdline`; do
> +    if `echo $i | grep -q "astimg="`; then
> +      RUN_VER=`echo $i | grep "astimg=" | sed -e 's/astimg=//' -e 's/.run$//'`
>   

... sed -e 's/astimg=\(.*\)\.run$/\1/'

means find the string that starts with "astimg=" and ends with "\.run$" 
and give me just the middle part.

I don't think we need to grep for astimg= twice, by the way.  The first 
one (in the "if") makes the second one in the RUN_VER= superfluous.


> +    fi
> +  done
> +}
> +
> +after_reboot() {
> +  AFTER_REBOOT=""
> +  if [ -n "$RUN_VER" ]; then
> +    REBOOT_VER=$RUN_VER
> +    if [ -f $BASE/os/ver ]; then
> +      REBOOT_VER=`cat $BASE/os/ver`
> +    fi
> +    if [ "$REBOOT_VER" != "$RUN_VER" ]; then
> +      AFTER_REBOOT=" [after reboot]"
> +    fi
> +  fi
> +}
> +
> +check() {
> +  for i in `cat /proc/cmdline`; do
> +    if `echo $i | grep -q "astlinux="`; then
> +      ARCH=`echo $i | grep "astlinux=" | sed -e 's/astlinux=//'`
>   

ditto on the double-grep.


> +    elif `echo $i | grep -q "console="`; then
> +      SERIAL=`echo $i | grep "console=" | sed -e 's/console=//'`
>   

ditto.


> +    fi
> +  done
> +
> +  if [ -z "$ARCH" ]; then
> +    echo "Unknown run image architecture type."
> +    exit $RED
> +  fi
> +
> +  if [ "$ARCH" = "via" -o "$ARCH" = "viac7" ]; then
> +    if [ -n "$SERIAL" ]; then
> +      ARCH="${ARCH}-serial"
> +    fi
> +  fi
> +
> +  URL="${SRC_URL}/${ARCH}"
> +
> +  version_from_astimg
> +
> +  mount_oldroot_cdrom
> +  if [ "$?" -ne 0 ]; then
>   

you never need quotes around $? ... it will always be numeric and non-empty.

> +    echo "Unable to find Runnix partition."
> +    exit $RED
> +  fi
> +
> +  if `wget -q -c -O /tmp/up_ver $URL/ver >/dev/null 2>&1`; then
>   

if wget -q -c -O /tmp/up_ver $URL/ver >/dev/null; then


> +    VER=`cat /tmp/up_ver`
> +    rm -f /tmp/up_ver
> +  else
> +    rm -f /tmp/up_ver
> +    echo "No version available."
> +    exit $RED
> +  fi
> +
> +  if [ -f $BASE/os/ver ]; then
> +    OVER=`cat $BASE/os/ver`
> +  else
> +    OVER=$RUN_VER
> +  fi
> +
> +  if [ -z "$OVER" ]; then
> +    echo "Unknown current version."
> +    exit $RED
> +  fi
> +
> +  if [ -f $BASE/os/Xver ]; then
> +    XVER=`cat $BASE/os/Xver`
> +  else
> +    XVER=""
> +  fi
> +  
> +  after_reboot
> +
> +  if [ "$VER" = "$OVER" ]; then
> +    echo "You are running the latest available version: ${VER}${AFTER_REBOOT}"
> +    exit $ORANGE
> +  fi
> +}
> +
> +upgrade() {
> +  if `mount -o rw,remount /oldroot/cdrom >/dev/null 2>&1`; then
>   

as above...


> +    :
> +  else
> +    echo "Unable to write to Runnix partition."
> +    exit $RED
> +  fi
> +
> +  mount_tmpfs_up
> +  if [ "$?" -ne 0 ]; then
> +    unmount_oldroot_cdrom
> +    echo "Error creating temporary filesystem."
> +    exit $RED
> +  fi
> +
> +  if `wget -q -c -O /tmp/up/$VER.tar.gz $URL/$VER.tar.gz >/dev/null 2>&1`; then
>   

as above.  And use ! to skip the "then" portion and move the "else" 
portion into the "then".

> +    :
> +  else
> +    unmount_tmpfs_up
> +    unmount_oldroot_cdrom
> +    echo "Firmware download failed."
> +    exit $RED
> +  fi
> +
> +  if `wget -q -c -O /tmp/up/$VER.tar.gz.sha1 $URL/$VER.tar.gz.sha1 >/dev/null 2>&1`; then
>   

Ditto.

> +    :
> +  else
> +    unmount_tmpfs_up
> +    unmount_oldroot_cdrom
> +    echo "Firmware sha1sum download failed."
> +    exit $RED
> +  fi
> +
> +  mkdir $BASE/tmp
> +  cd /tmp/up
> +
> +  if `sha1sum -cs $VER.tar.gz.sha1`; then
>   

Can we just use the exit value?


> +    if [ -n "$XVER" ]; then
> +      rm $BASE/os/$XVER.* >/dev/null 2>&1
> +      rm $BASE/os/Xver >/dev/null 2>&1
>   

If you're worried about these not existing, then use -f and don't throw 
away the stderr...

Most unix commands don't generate output on success anyway.


> +    fi
> +    tar -xzf /tmp/up/$VER.tar.gz -C $BASE/tmp/
> +    sync
> +  else
> +    unmount_tmpfs_up
> +    rmdir $BASE/tmp
> +    unmount_oldroot_cdrom
> +    echo "Firmware verification failed."
> +    exit $RED
> +  fi
> +
> +  cd $BASE/tmp/$VER
> +
> +  if `sha1sum -cs $VER.run.sha1`; then
>   

Ditto.


> +    mv $BASE/tmp/$VER/* $BASE/os/
> +    echo "$OVER" > $BASE/os/Xver
> +    echo "$VER" > $BASE/os/ver
> +    rm -r $BASE/tmp/
> +  else
> +    unmount_tmpfs_up
> +    rm -r $BASE/tmp/
> +    unmount_oldroot_cdrom
> +    echo "Firmware verification failed."
> +    exit $RED
> +  fi
> +  
> +  after_reboot
> +
> +  unmount_tmpfs_up
> +  unmount_oldroot_cdrom
> +}
> +
> +show() {
> +  version_from_astimg
> +
> +  mount_oldroot_cdrom
> +  if [ "$?" -ne 0 ]; then
>   

Ditto on the quotes.  You can also write this as:

if ! mount_oldroot_cdrom; then
...


> +    echo "Unable to find Runnix partition."
> +    exit $RED
> +  fi
> +
> +  if [ -f $BASE/os/ver ]; then
> +    VER=`cat $BASE/os/ver`
> +  else
> +    VER=$RUN_VER
> +  fi
> +
> +  if [ -z "$VER" ]; then
> +    echo "Unknown current version."
>   

Pipe to stderr via >&2 on non-zero exit.


> +    exit $RED
> +  fi
> +
> +  after_reboot
> +
> +  if [ -f $BASE/os/Xver ]; then
> +    XVER=`cat $BASE/os/Xver`
> +  else
> +    echo "Current version is: ${VER}${AFTER_REBOOT}, no previous saved version."
>   

Ditto.  Oh, wait... GREEN is 0...  never mind here.



> +    exit $GREEN
> +  fi
> +}
> +
> +revert() {
> +  version_from_astimg
> +
> +  mount_oldroot_cdrom
> +  if [ "$?" -ne 0 ]; then
>   

Ditto.


> +    echo "Unable to find Runnix partition."
>   

Ditto, and elsewhere.


> +    exit $RED
> +  fi
> +
> +  if [ -f $BASE/os/Xver ]; then
> +    XVER=`cat $BASE/os/Xver`
> +  else
> +    echo "Revert failed, there is no previous saved version."
> +    exit $RED
> +  fi
> +
> +  if [ -f $BASE/os/ver ]; then
> +    VER=`cat $BASE/os/ver`
> +  else
> +    echo "Unable to find the current version."
> +    exit $RED
> +  fi
> +
> +  if `mount -o rw,remount /oldroot/cdrom >/dev/null 2>&1`; then
>   

As above.


> +    echo $VER > $BASE/os/Xver
> +    echo $XVER > $BASE/os/ver
> +    mount -o ro,remount /oldroot/cdrom >/dev/null 2>&1
>   

As above.


> +  else
> +    echo "Revert failed, unable to change version files."
> +    exit $RED
> +  fi
> +  
> +  after_reboot
> +}
> +
> +# main
> +
> +if [ -d /tmp/up ]; then
> +  echo "Firmware Upgrade in Progress, please wait..."
> +  exit $ORANGE
> +fi
> +
> +case $1 in
> +
> +check)
> +  if [ -n "$2" ]; then
> +    SRC_URL=$2
> +  else
> +    echo "Usage: upgrade-run-image check firmware_repository_url"
>   

as above.


> +    exit 1
> +  fi
> +  check
> +  echo "Current version is: ${OVER}${AFTER_REBOOT},  Latest available version is: ${VER}"
> +  exit $GREEN
>   
> +  ;;
> +  
> +upgrade)
> +  if [ -n "$2" ]; then
> +    SRC_URL=$2
> +  else
> +    echo "Usage: upgrade-run-image upgrade firmware_repository_url"
> +    exit 1
>   

No color code?


> +  fi
> +  check
> +  upgrade
> +  echo "Successful upgrade to: ${VER}${AFTER_REBOOT}"
> +  exit $GREEN
> +  ;;
> +  
> +show)
> +  show
> +  echo "Current version is: ${VER}${AFTER_REBOOT}, Previous saved version is: ${XVER}"
> +  exit $GREEN
> +  ;;
> +  
> +revert)
> +  revert
> +  echo "Current version is now: ${XVER}${AFTER_REBOOT}"
> +  exit $GREEN
> +  ;;
> +  
> +*)
> +  echo "Usage: upgrade-run-image check|upgrade|show|revert firmware_repository_url"
> +  exit 1
> +  ;;
> +  
> +esac
> +
>