Merge lp:~lool/linaro-image-tools/random-cleanups into lp:linaro-image-tools/11.11

[Loïc Minier]

A stupidly long collection of cleanup commits.

[James Westby (james-w)]

Hi,

18 -CODENAME=Chessy
19 +CODENAME="Chessy 10.05"

Where's that used? Shouldn't it be something else now?

126 -case $DEVNAME in
127 - beagle|igep|vexpress)
128 - ensure_command mkimage uboot-mkimage
129 - ;;
130 -esac

We don't need that anymore, or it is repeated elsewhere?

192 + -d "${DIR}/binary/${parts_dir}"/vmlinuz-*-linaro-omap \

Is that a little specific?

171 + mkdir -p ${BOOT_DISK}

While you are changing it you might as well quote "${BOOT_DISK}"

182 + ${BOOT_DISK}

ditto.

212 + sudo cp -v binary/usr/lib/u-boot/ca9x4_ct_vxp/u-boot.bin ${BOOT_DISK}

ditto

review: Approve

[Loïc Minier (lool)]

On Tue, Sep 28, 2010, James Westby wrote:
> 18 -CODENAME=Chessy
> 19 +CODENAME="Chessy 10.05"
> Where's that used? Shouldn't it be something else now?

 (It seems you're reading the diff as a whole; you might find it easier
 to read individual revisions to understand each change, especially
 since there are some isolated whitespace cleanup commits which clutter
 the diff a bit.)

 http://bazaar.launchpad.net/~lool/linaro-image-tools/random-cleanups/revision/107
 it was in the description of the boot script for beagles and igeps.

 I wish we'd remove this, I wanted to dump this first pass of
 mostly shell syntax improvements before the series would grow too large
 and I avoided controversial changes.

 But I could add a commit to drop this if you like; I think generated
 the boot script shouldn't say the release name. (I'm worried about
 other serious things with the boot script(s) though.)

> 126 -case $DEVNAME in
> 127 - beagle|igep|vexpress)
> 128 - ensure_command mkimage uboot-mkimage
> 129 - ;;
> 130 -esac
>
> We don't need that anymore, or it is repeated elsewhere?

 http://bazaar.launchpad.net/~lool/linaro-image-tools/random-cleanups/revision/117
 This test was broken (DEVNAME wasn't set in the script) and all the
 boards targets require mkimage, so I made it unconditional.

> 192 + -d "${DIR}/binary/${parts_dir}"/vmlinuz-*-linaro-omap \
> Is that a little specific?

 -vexpress was already enforcing linaro-vexpress on one file; I figured
 it was actually correct to do so, but it probably prevents installing
 relatively old Linaro images. I think that's ok.

> 171 + mkdir -p ${BOOT_DISK}
> While you are changing it you might as well quote "${BOOT_DISK}"
> 182 + ${BOOT_DISK}
> ditto.
> 212 + sudo cp -v binary/usr/lib/u-boot/ca9x4_ct_vxp/u-boot.bin ${BOOT_DISK}
> ditto

 Sure; I think I will do a quoting pass; it's just all over the place.
 I tried keeping each commit logical and not mixing two things in the
 same commit.

--
Loïc Minier

[James Westby (james-w)]

On Tue, 28 Sep 2010 17:13:43 -0000, Loïc Minier <email address hidden> wrote:
> On Tue, Sep 28, 2010, James Westby wrote:
> > 18 -CODENAME=Chessy
> > 19 +CODENAME="Chessy 10.05"
> > Where's that used? Shouldn't it be something else now?
>
> (It seems you're reading the diff as a whole; you might find it easier
> to read individual revisions to understand each change, especially
> since there are some isolated whitespace cleanup commits which clutter
> the diff a bit.)
>
> http://bazaar.launchpad.net/~lool/linaro-image-tools/random-cleanups/revision/107
> it was in the description of the boot script for beagles and igeps.
>
> I wish we'd remove this, I wanted to dump this first pass of
> mostly shell syntax improvements before the series would grow too large
> and I avoided controversial changes.
>
> But I could add a commit to drop this if you like; I think generated
> the boot script shouldn't say the release name. (I'm worried about
> other serious things with the boot script(s) though.)

A followup to use "Linaro 10.11" or something would be fine with me.

> > 126 -case $DEVNAME in
> > 127 - beagle|igep|vexpress)
> > 128 - ensure_command mkimage uboot-mkimage
> > 129 - ;;
> > 130 -esac
> >
> > We don't need that anymore, or it is repeated elsewhere?
>
> http://bazaar.launchpad.net/~lool/linaro-image-tools/random-cleanups/revision/117
> This test was broken (DEVNAME wasn't set in the script) and all the
> boards targets require mkimage, so I made it unconditional.

Isn't that unconditionally not there? Don't we want to check the command
is present and give the nice error?

> > 171 + mkdir -p ${BOOT_DISK}
> > While you are changing it you might as well quote "${BOOT_DISK}"
> > 182 + ${BOOT_DISK}
> > ditto.
> > 212 + sudo cp -v binary/usr/lib/u-boot/ca9x4_ct_vxp/u-boot.bin ${BOOT_DISK}
> > ditto
>
> Sure; I think I will do a quoting pass; it's just all over the place.
> I tried keeping each commit logical and not mixing two things in the
> same commit.

If you could do quoting as another pass that would be great, I've
reviewed all of this now, and pulling in all the results of quoting
would be a pain to re-review.

Thanks,

James

[Loïc Minier (lool)]

I pushed a couple of additional commits addressing your comments

[lp:~lool/linaro-image-tools/random-cleanups]

118. By Loïc Minier on 2010-09-28

Drop CODENAME and use "boot script" as the description of the boot script.

119. By Loïc Minier on 2010-09-28

Quote uses of BOOT_DISK and ROOT_DISK.

[Loïc Minier (lool)]

On Tue, Sep 28, 2010, James Westby wrote:
> A followup to use "Linaro 10.11" or something would be fine with me.

 I just dropped it in favor of "boot script" as a description.

> > http://bazaar.launchpad.net/~lool/linaro-image-tools/random-cleanups/revision/117
> > This test was broken (DEVNAME wasn't set in the script) and all the
> > boards targets require mkimage, so I made it unconditional.
>
> Isn't that unconditionally not there? Don't we want to check the command
> is present and give the nice error?

 I'm not sure what you mean; it's there around line 224:
ensure_command mkimage uboot-mkimage

> If you could do quoting as another pass that would be great, I've
> reviewed all of this now, and pulling in all the results of quoting
> would be a pain to re-review.

 Sorry; pushed that as my last commit now, but it's basically the latest
 commit.

--
Loïc Minier

[James Westby (james-w)]

On Tue, 28 Sep 2010 17:55:56 -0000, Loïc Minier <email address hidden> wrote:
> I'm not sure what you mean; it's there around line 224:
> ensure_command mkimage uboot-mkimage

Yep, don't know how I missed that, sorry.

Thanks,

James

[Steve Langasek (vorlon)]

On Tue, Sep 28, 2010 at 01:29:43PM -0000, Loïc Minier wrote:
> Loïc Minier has proposed merging lp:~lool/linaro-image-tools/random-cleanups into lp:linaro-image-tools.

> Requested reviews:
> Linaro Maintainers (linaro-maintainers)

> A stupidly long collection of cleanup commits.

I think this has collided with my refactoring of uboot installation in
l-m-c. Update & resubmit?

BTW, the whitespace changes here call attention to the many uses of sync in
this script. Are any of these *not* gratuitous? In the worst case, we
should call sync once before unmounting the filesystem; but even then that
would be a kernel bug if it fails to flush the data out to disk before
unmounting, and I'm not aware of any such bugs at present. I think this
script would run a bit faster, without negative consequences, if we were to
clear out all these 'sync' calls...

[Loïc Minier (lool)]

Steve, I resolved the conflict when merging already.

I did kill some sync calls, but not all of them. I wasn't sure why there were there, so I only killed the easy ones.

=== modified file 'linaro-media-create'
--- linaro-media-create	2010-09-23 20:53:10 +0000
+++ linaro-media-create	2010-09-28 17:47:01 +0000
@@ -19,9 +19,6 @@
 
 set -e
 
-MLO_FILE=usr/lib/x-loader-omap/MLO
-UBOOT_FILE=usr/lib/u-boot/omap3_beagle/u-boot.bin
-
 unset MMC MMC1 MMC2 MMC3 IMAGE_FILE HWPACK_FILE
 
 #Defaults
@@ -29,15 +26,14 @@
 BINARY_TARBALL='binary-tar.tar.gz'
 BOOT_LABEL=boot
 RFS_LABEL=rootfs
-CODENAME=Chessy
 IS_LIVE=
 FAT_SIZE=32
 IMAGE_SIZE=2G
 
 DIR=$PWD
 TMP_DIR=$(mktemp -d)
-BOOT_DISK=${DIR}/boot-disc
-ROOT_DISK=${DIR}/root-disc
+BOOT_DISK="${DIR}/boot-disc"
+ROOT_DISK="${DIR}/root-disc"
 
 ensure_command() {
   # ensure_command foo foo-package
@@ -51,7 +47,7 @@
   echo "usage: $(basename $0) --mmc /dev/sdd"
   echo "<or>"
   echo "usage: $(basename $0) --image_file mmc.img"
-cat <<EOF
+  cat <<EOF
 
 required options:
 --mmc </dev/sdX>
@@ -108,7 +104,7 @@
 --binary <filename>
     specify file used to create the bootable system. Default binary-tar.tar.gz
 EOF
-exit
+  exit
 }
 
 check_mmc() {
@@ -142,35 +138,14 @@
 }
 
 check_fs_type() {
-  IN_VALID_FS=1
-
-  if test "-$FS_TYPE-" = "-ext2-"
-  then
-    RFS=ext2
-    unset IN_VALID_FS
-  fi
-
-  if test "-$FS_TYPE-" = "-ext3-"
-  then
-    RFS=ext3
-    unset IN_VALID_FS
-  fi
-
-  if test "-$FS_TYPE-" = "-ext4-"
-  then
-    RFS=ext4
-    unset IN_VALID_FS
-  fi
-
-  if test "-$FS_TYPE-" = "-btrfs-"
-  then
-    RFS=btrfs
-    unset IN_VALID_FS
-  fi
-
-  if [ "$IN_VALID_FS" ] ; then
-    usage
-  fi
+  case "$RFS" in
+    ext2|ext3|ext4|btrfs)
+      :
+    ;;
+    *)
+      usage
+    ;;
+  esac
 }
 
 checkparm() {
@@ -187,7 +162,6 @@
   case $1 in
     -h|--help)
       usage
-      MMC=1
       ;;
     --hwpack)
       checkparm $2
@@ -208,7 +182,7 @@
       ;;
     --rootfs)
       checkparm $2
-      FS_TYPE="$2"
+      RFS="$2"
       check_fs_type
       ;;
     --boot_label)
@@ -247,27 +221,20 @@
   shift
 done
 
+ensure_command mkimage uboot-mkimage
 ensure_command uuidgen uuid-runtime
 ensure_command parted parted
 ensure_command wget wget
 ensure_command md5sum coreutils
 ensure_command realpath realpath
-case $RFS in
-  ext3)
-    ensure_command mkfs.ext3 e2fsprogs
-    ;;
-  ext4)
-    ensure_command mkfs.ext4 e2fsprogs
+case "$RFS" in
+  ext2|ext3|ext4)
+    ensure_command "mkfs.$RFS" e2fsprogs
     ;;
   btrfs)
     ensure_command mkfs.btrfs btrfs-tools
     ;;
 esac
-case $DEVNAME in
-  beagle|igep|vexpress)
-    ensure_command mkimage uboot-mkimage
-    ;;
-esac
 
 RFS_UUID=`uuidgen -r`
 
@@ -365,19 +332,19 @@
 
 cleanup_sd() {
 
- echo ""
- echo "Umounting Partitions"
- echo ""
+  echo ""
+  echo "Umounting Partitions"
+  echo ""
 
- if test -n "$MMC1"; then
-   sudo umount ${MMC1} &> /dev/null || true
- fi
- if test -n "$MMC2"; then
-   sudo umount ${MMC2} &> /dev/null || true
- fi
- if [ "${MMC}" ]; then
-   sudo parted -s ${MMC} mklabel msdos
- fi
+  if test -n "$MMC1"; then
+    sudo umount ${MMC1} &> /dev/null || true
+  fi
+  if test -n "$MMC2"; then
+    sudo umount ${MMC2} &> /dev/null || true
+  fi
+  if [ "${MMC}" ]; then
+    sudo parted -s ${MMC} mklabel msdos
+  fi
 }
 
 create_partitions() {
@@ -441,54 +408,53 @@
     parts_dir=boot
   fi
  
-  mkdir -p ${BOOT_DISK} || true
-  sudo mount ${MMC1} ${BOOT_DISK}
+  mkdir -p "${BOOT_DISK}"
+  sudo mount ${MMC1} "${BOOT_DISK}"
   case "$DEVIMAGE" in
     beagle|igep)
       if [ "$DEVIMAGE" = "beagle" ]; then
-        if [ -e binary/${MLO_FILE} ] && [ -e binary/${UBOOT_FILE} ]; then
-          sudo cp -v binary/${MLO_FILE}   ${BOOT_DISK}/MLO
-          sudo cp -v binary/${UBOOT_FILE} ${BOOT_DISK}/u-boot.bin
-        fi
+        sudo cp -v binary/usr/lib/x-loader-omap/MLO \
+                   binary/usr/lib/u-boot/omap3_beagle/u-boot.bin \
+                   "${BOOT_DISK}"
       fi
       sync
-      cd ${DIR}
       echo "done"
- 
+
       sudo mkimage -A arm -O linux -T kernel -C none -a 0x80008000 \
-           -e 0x80008000 -n Linux -d "${DIR}/binary/${parts_dir}"/vmlinuz-*omap \
+           -e 0x80008000 -n Linux \
+           -d "${DIR}/binary/${parts_dir}"/vmlinuz-*-linaro-omap \
            "${BOOT_DISK}/uImage"
- 
       sudo mkimage -A arm -O linux -T ramdisk -C none -a 0 \
-           -e 0 -n initramfs -d "${DIR}/binary/${parts_dir}"/initrd.img-*-omap \
+           -e 0 -n initramfs \
+           -d "${DIR}/binary/${parts_dir}"/initrd.img-*-linaro-omap \
            "${BOOT_DISK}/uInitrd"
- 
       sudo mkimage -A arm -O linux -T script -C none -a 0 \
-           -e 0 -n "$CODENAME 10.05" -d "${TMP_DIR}/boot.cmd" \
+           -e 0 -n "boot script" -d "${TMP_DIR}/boot.cmd" \
            "${BOOT_DISK}/boot.scr"
-      sudo cp -v ${BOOT_DISK}/boot.scr ${BOOT_DISK}/boot.ini
+      sudo cp -v "${BOOT_DISK}/boot.scr" "${BOOT_DISK}/boot.ini"
       ;;
+
     vexpress)
-      cp binary/usr/lib/u-boot/ca9x4_ct_vxp/u-boot.bin ${BOOT_DISK}/
-      sudo mkimage -A arm -O linux -T kernel -C none -a 0x60008000 -e 0x60008000 \
-           -n "Linux" -d ${DIR}/binary/${parts_dir}/vmlinuz*-vexpress ${BOOT_DISK}/uImage
+      sudo cp -v binary/usr/lib/u-boot/ca9x4_ct_vxp/u-boot.bin "${BOOT_DISK}"
+      sudo mkimage -A arm -O linux -T kernel -C none -a 0x60008000 \
+           -e 0x60008000 -n Linux \
+           -d "${DIR}/binary/${parts_dir}"/vmlinuz-*-linaro-vexpress \
+           "${BOOT_DISK}/uImage"
       sudo mkimage -A arm -O linux -T ramdisk -C none -a 0x81000000 \
-           -e 0x81000000 -n "initramfs" -d \
-           ${DIR}/binary/${parts_dir}/initrd.img-*-linaro-vexpress \
-           "${BOOT_DISK}"/uInitrd
+           -e 0x81000000 -n initramfs \
+           -d "${DIR}/binary/${parts_dir}"/initrd.img-*-linaro-vexpress \
+           "${BOOT_DISK}/uInitrd"
       ;;
+
     *)
-      echo "Set --dev parameter: --dev <beagle|igep|vexpress>" 1>&2
-      exit
+      echo "Internal error; missing support for $DEVIMAGE" >&2
+      exit 1
       ;;
   esac
- 
-  cd ${BOOT_DISK}/
-  sync
-  sync
-  cd ${DIR}/
- 
-  sudo umount ${BOOT_DISK} || true
+
+  sync
+  sync
+  sudo umount "${BOOT_DISK}" || true
 }
 
 populate_rootfs() {
@@ -496,13 +462,13 @@
   echo "Populating rootfs Partition"
   echo "Be patient, this may take a few minutes"
   echo ""
-  mkdir -p ${ROOT_DISK}
-  sudo mount ${MMC2} ${ROOT_DISK}
+  mkdir -p "${ROOT_DISK}"
+  sudo mount ${MMC2} "${ROOT_DISK}"
 
-  sudo mv ${DIR}/binary/* $ROOT_DISK
+  sudo mv ${DIR}/binary/* "$ROOT_DISK"
 
   # add fstab entry for rootfs and boot
-  echo "UUID=${RFS_UUID} / ${RFS}  errors=remount-ro 0 1 " | sudo tee -a ${ROOT_DISK}/etc/fstab
+  echo "UUID=${RFS_UUID} / ${RFS}  errors=remount-ro 0 1 " | sudo tee -a "${ROOT_DISK}/etc/fstab"
 
   if [ "$CREATE_SWAP" ] ; then
 
@@ -510,14 +476,14 @@
     echo "Creating SWAP File"
     echo ""
 
-    SPACE_LEFT=$(df ${ROOT_DISK} | grep ${MMC2} | awk '{print $4}')
+    SPACE_LEFT=$(df "${ROOT_DISK}" | grep ${MMC2} | awk '{print $4}')
 
     let SIZE=$SWAP_SIZE*1024
 
     if [ $SPACE_LEFT -ge $SIZE ] ; then
-      sudo dd if=/dev/zero of=${ROOT_DISK}/SWAP.swap bs=1M count=$SWAP_SIZE
-      sudo mkswap ${ROOT_DISK}/SWAP.swap
-      echo "/SWAP.swap  none  swap  sw  0 0" | sudo tee -a ${ROOT_DISK}/etc/fstab
+      sudo dd if=/dev/zero "of=${ROOT_DISK}/SWAP.swap" bs=1M count=$SWAP_SIZE
+      sudo mkswap "${ROOT_DISK}/SWAP.swap"
+      echo "/SWAP.swap  none  swap  sw  0 0" | sudo tee -a "${ROOT_DISK}/etc/fstab"
     else
       echo "SWAP file bigger then whats left on partition"
     fi
@@ -527,14 +493,11 @@
   echo "Creating /etc/flash-kernel.conf"
   echo ""
 
-  echo "UBOOT_PART=/dev/mmcblk0p1" | sudo tee ${ROOT_DISK}/etc/flash-kernel.conf >/dev/null
-
-  cd ${ROOT_DISK}
-  sync
-  sync
-  cd ${DIR}/
-
-  sudo umount ${ROOT_DISK} || true
+  echo "UBOOT_PART=/dev/mmcblk0p1" | sudo tee "${ROOT_DISK}/etc/flash-kernel.conf" >/dev/null
+
+  sync
+  sync
+  sudo umount "${ROOT_DISK}" || true
 }
 
 calculatesize() {
@@ -585,6 +548,20 @@
   fi
 fi
 
+case "$DEVIMAGE" in
+  beagle|igep|vexpress)
+    :
+  ;;
+  "")
+    echo "Please set some target device with --dev" >&2
+    usage
+  ;;
+  *)
+    echo "Unknown target device $DEVIMAGE" >&2
+    usage
+  ;;
+esac
+
 if [ "$DEVIMAGE" ]; then
   case "$DEVIMAGE" in
     beagle|igep)
@@ -603,7 +580,8 @@
       FAT_SIZE=16
       ;;
     *)
-      echo "unknown --dev paramater: $DEVIMAGE" 1>&2
+      echo "Internal error; missing support for $DEVIMAGE" >&2
+      exit 1
       ;;
   esac
 else
@@ -616,31 +594,31 @@
   usage
 fi
 
- unpack_binary_tarball
- if [ "$HWPACK_FILE" ]; then
-   install_hwpack
- fi
- create_boot_cmd
- get_mmcs_by_id
- cleanup_sd
- create_partitions
- echo -n "waiting for partitioning to settle ..."
- sync
- sleep 3
- echo "done."
- get_mmcs_by_id
- if test -z "$MMC1" -o -z "$MMC2"; then
-   echo "MMC1: $MMC1 nor MMC2: $MMC2 must be empty"
-   exit 2
- fi
- prepare_partitions
- populate_boot
- populate_rootfs
+unpack_binary_tarball
+if [ "$HWPACK_FILE" ]; then
+  install_hwpack
+fi
+create_boot_cmd
+get_mmcs_by_id
+cleanup_sd
+create_partitions
+echo -n "waiting for partitioning to settle ..."
+sync
+sleep 3
+echo "done."
+get_mmcs_by_id
+if test -z "$MMC1" -o -z "$MMC2"; then
+  echo "MMC1: $MMC1 nor MMC2: $MMC2 must be empty"
+  exit 2
+fi
+prepare_partitions
+populate_boot
+populate_rootfs
 
- if [ "${IMAGE_FILE}" ]; then
+if [ "${IMAGE_FILE}" ]; then
   echo "Create ${IMAGE_FILE}.gz"
   gzip -f ${IMAGE_FILE} > ${IMAGE_FILE}.gz
- fi
+fi
 
- remove_binary_dir
+remove_binary_dir