Switch to GPT-format disk images.

Switch to GPT-format disk images.

This changes the disk image for both USB keys and hard-drive installs to use
the EFI GUID Partition Tables. This is a prequisite for booting with an EFI
BIOS. This change does not use the EFI Boot protocol; it still boots using
Legacy BIOS. The PMBR contains syslinux' gptmbr.bin, which searches the GPT
for a specified partition's GUID to boot.

I've tested it on my EeePC. The USB image works, chromeos-install works, and
the reimaged hard drive works. I have not yet tested the memento_updater.sh
script, but I wanted to start the review without waiting until it's all perfect.
I will also be refactoring build_gpt.sh and chromeos-install to share common
logic.

It's almost certain that all existing dogfood machines will need to be
reimaged from a USB key. We could probably figure out a way to upgrade
automatically, but not quickly or without risk.

In addition to the GPT formatting, the build_image script has changed to
emit a single usb.bin file. This can be copied directly onto a USB key and
booted. Installation of dev tools needs to happen with build_image, not
image_to_usb. I have not yet looked at the other image_to_* scripts.

[sosa, 2010-03-18 21:30:22]

Mostly questions and nits

http://codereview.chromium.org/1100001/diff/1/2
File src/platform/init/chromeos_startup (right):

http://codereview.chromium.org/1100001/diff/1/2#newcode40
src/platform/init/chromeos_startup:40: ROOT_DEV=$(rootdev)
Where is rootdev set?

http://codereview.chromium.org/1100001/diff/1/3
File src/platform/installer/chromeos-install (right):

http://codereview.chromium.org/1100001/diff/1/3#newcode1
src/platform/installer/chromeos-install:1: #!/bin/bash -xeu
Bash is part of chromeos-dev not chromeos base.  Will this script work without
bash?

http://codereview.chromium.org/1100001/diff/1/3#newcode12
src/platform/installer/chromeos-install:12: if [ $(id -u) == "0" ]
Why did you rewrite this?  Is there a particular reason besides cleanup?  Also
why -ne 1000 vs. == 0.

http://codereview.chromium.org/1100001/diff/1/3#newcode28
src/platform/installer/chromeos-install:28: DST=${1:-/dev/sda}
Add comments to be clear what this is.  It's not as easy to read as the old
format even though it's shorter

http://codereview.chromium.org/1100001/diff/1/3#newcode30
src/platform/installer/chromeos-install:30:
SKIP_CHECK_SOURCE_REMOVABLE=${2:-false}
Ditto.

http://codereview.chromium.org/1100001/diff/1/3#newcode34
src/platform/installer/chromeos-install:34: # Base device
Period at the end of comment

http://codereview.chromium.org/1100001/diff/1/3#newcode84
src/platform/installer/chromeos-install:84: 
Be consistent with the number of lines between functions

http://codereview.chromium.org/1100001/diff/1/3#newcode90
src/platform/installer/chromeos-install:90: echo > /dev/null
Why are you echoing to dev null here?

http://codereview.chromium.org/1100001/diff/1/3#newcode93
src/platform/installer/chromeos-install:93: mymount() {
Document what this function is and change to my_mount

http://codereview.chromium.org/1100001/diff/1/3#newcode94
src/platform/installer/chromeos-install:94: local DST_DEV=$1
local variables should be all lower case to be consistent with other scripts

http://codereview.chromium.org/1100001/diff/1/3#newcode115
src/platform/installer/chromeos-install:115: local SRC_PARTITION=$1
lowercase locals

http://codereview.chromium.org/1100001/diff/1/3#newcode126
src/platform/installer/chromeos-install:126: sync
Why?  Is this just extra?

http://codereview.chromium.org/1100001/diff/1/3#newcode132
src/platform/installer/chromeos-install:132: # NOTE: This hack won't work if we
stop using an ext-based rootfs
Period at the end of line

http://codereview.chromium.org/1100001/diff/1/3#newcode143
src/platform/installer/chromeos-install:143: # Copy the new label superblock to
the dest
period

http://codereview.chromium.org/1100001/diff/1/3#newcode146
src/platform/installer/chromeos-install:146: # Copy the rest of the source to
the dest
period

http://codereview.chromium.org/1100001/diff/1/3#newcode153
src/platform/installer/chromeos-install:153: # Functions to round 512-byte
blocks up to 4096-byte blocks
doc args

http://codereview.chromium.org/1100001/diff/1/3#newcode164
src/platform/installer/chromeos-install:164: rounddown() {
Rounds down to?

http://codereview.chromium.org/1100001/diff/1/3#newcode175
src/platform/installer/chromeos-install:175: 
Too many lines between

http://codereview.chromium.org/1100001/diff/1/3#newcode182
src/platform/installer/chromeos-install:182: 
Too many lines between

http://codereview.chromium.org/1100001/diff/1/3#newcode190
src/platform/installer/chromeos-install:190: 
Same

http://codereview.chromium.org/1100001/diff/1/3#newcode198
src/platform/installer/chromeos-install:198: #   Primary GPT Table (16K)
It makes more sense to order these comments by partition #

http://codereview.chromium.org/1100001/diff/1/3#newcode201
src/platform/installer/chromeos-install:201: #   non-synced system settings     
                partition 6
What is this partition?  What does this mean now?

http://codereview.chromium.org/1100001/diff/1/3#newcode213
src/platform/installer/chromeos-install:213: # same, but they'll be as small as
possible.
Are we also leaving off partitions 6,7 and 8?  Is there a doc describing all
these partitions ... I didn't realize we were going up to 8

http://codereview.chromium.org/1100001/diff/1/3#newcode217
src/platform/installer/chromeos-install:217: # upwards towards the rootfs images
(and they can expand downwards). The
We are allowing the Rootfs to expand?

http://codereview.chromium.org/1100001/diff/1/3#newcode233
src/platform/installer/chromeos-install:233: # Here's how much we're going to
reserve on the dest device
period

http://codereview.chromium.org/1100001/diff/1/3#newcode233
src/platform/installer/chromeos-install:233: # Here's how much we're going to
reserve on the dest device
What implications will reserving have?  It seems we should probably reserve a
bit more for the stateful partition i.e. 1GB

http://codereview.chromium.org/1100001/diff/1/3#newcode241
src/platform/installer/chromeos-install:241: 
Too many lines

http://codereview.chromium.org/1100001/diff/1/5
File src/platform/memento_softwareupdate/memento_updater.sh (right):

http://codereview.chromium.org/1100001/diff/1/5#newcode108
src/platform/memento_softwareupdate/memento_updater.sh:108: # figure out which
partition I'm on, and which to download to.
F

http://codereview.chromium.org/1100001/diff/1/5#newcode120
src/platform/memento_softwareupdate/memento_updater.sh:120: # our device
O w/ period at end

http://codereview.chromium.org/1100001/diff/1/6
File src/scripts/build_gpt.sh (right):

http://codereview.chromium.org/1100001/diff/1/6#newcode47
src/scripts/build_gpt.sh:47: if [[ -z "$1" || -z "$2" ]] ; then
You should probably verify $1 is a directory and $2 is a device

http://codereview.chromium.org/1100001/diff/1/6#newcode50
src/scripts/build_gpt.sh:50: fi
Add line

http://codereview.chromium.org/1100001/diff/1/6#newcode51
src/scripts/build_gpt.sh:51: IMAGEDIR="$1"
IMAGE_DIR

http://codereview.chromium.org/1100001/diff/1/6#newcode52
src/scripts/build_gpt.sh:52: OUTDEV="$2"
OUT_DEV

http://codereview.chromium.org/1100001/diff/1/6#newcode54
src/scripts/build_gpt.sh:54: if [[ -n "$FLAGS_arch" ]]; then
Why do you need another variable?  Just use FLAGS_arch

http://codereview.chromium.org/1100001/diff/1/6#newcode73
src/scripts/build_gpt.sh:73: if [[ -z "$FLAGS_board_root" ]]; then
Why do you need flags board and board_root?  Why would they be different?

http://codereview.chromium.org/1100001/diff/1/6#newcode83
src/scripts/build_gpt.sh:83: # Check for missing parts
period

http://codereview.chromium.org/1100001/diff/1/6#newcode136
src/scripts/build_gpt.sh:136: #
Do we really have to duplicate this comment?  Can you just put it in a readme in
the scripts directory?

http://codereview.chromium.org/1100001/diff/1/6#newcode280
src/scripts/build_gpt.sh:280: # Now populate the partitions
Shouldn't be plural

http://codereview.chromium.org/1100001/diff/1/7
File src/scripts/build_image (right):

http://codereview.chromium.org/1100001/diff/1/7#newcode324
src/scripts/build_image:324: sudo cp -f ${ROOT_FS_DIR}/boot/vmlinuz
${OUTPUT_DIR}/vmlinuz.image
Do we need two copies of the kernel on the system?

http://codereview.chromium.org/1100001/diff/1/8
File src/scripts/image_to_usb.sh (right):

http://codereview.chromium.org/1100001/diff/1/8#newcode72
src/scripts/image_to_usb.sh:72: echo "Just use $IMG_SRC directly. It's ready to
boot."
There use to be instructions of how to get this on an img.  Can we still dd this
straight onto a usb key?  You may want to add some more warning info saying you
can't copy to usb from within chroot

http://codereview.chromium.org/1100001/diff/1/8#newcode123
src/scripts/image_to_usb.sh:123: echo "Just copy ${IMG_SRC} to whatever filename
you want. It's ready to boot."
Combine this with the chroot error

[adlr, 2010-03-18 21:32:19]

http://codereview.chromium.org/1100001/diff/1/2
File src/platform/init/chromeos_startup (right):

http://codereview.chromium.org/1100001/diff/1/2#newcode41
src/platform/init/chromeos_startup:41: STATE_DEV=${ROOT_DEV%%[0-9]*}1
should we put the kernel partitions first? is there a doc that says what # each
partition is and in what order they are laid out on disk? That would be helpful.

http://codereview.chromium.org/1100001/diff/1/3
File src/platform/installer/chromeos-install (right):

http://codereview.chromium.org/1100001/diff/1/3#newcode1
src/platform/installer/chromeos-install:1: #!/bin/bash -xeu
can we put this back to /bin/sh? we want this to run on the device and we want
to get to point where we don't have bash on the machine.

http://codereview.chromium.org/1100001/diff/1/3#newcode33
src/platform/installer/chromeos-install:33: ROOTDEV=$(rootdev)
maybe i'm missing something, but where is the rootdev command?

http://codereview.chromium.org/1100001/diff/1/3#newcode90
src/platform/installer/chromeos-install:90: echo > /dev/null
why this line?

http://codereview.chromium.org/1100001/diff/1/3#newcode93
src/platform/installer/chromeos-install:93: mymount() {
perhaps call it something like "safe_loop_mount"?

http://codereview.chromium.org/1100001/diff/1/3#newcode104
src/platform/installer/chromeos-install:104: trap do_cleanup EXIT
can you move this outside the function? it's a little weird to set a trap inside
a function, but remove it outside. it's nice to keep them paired at the same
level.

http://codereview.chromium.org/1100001/diff/1/3#newcode206
src/platform/installer/chromeos-install:206: #   Rootfs A                       
                partition 3
why does A come after B for rootfs, but before B for kernel? Seems
counter-intuitive.

Also, should we allocate numbers (even if no disk space) to a third
kerne/rootfs? I think Randall and I would like that for future growth.

http://codereview.chromium.org/1100001/diff/1/3#newcode211
src/platform/installer/chromeos-install:211: # towards the inside, so we can
leave image B off for USB keys. We'll still
Couldn't we still leave image B off the usb image if it were on the outside of
the image?

http://codereview.chromium.org/1100001/diff/1/3#newcode233
src/platform/installer/chromeos-install:233: # Here's how much we're going to
reserve on the dest device
can we explicitly set "BLOCK_SIZE=4096 #bytes" here, and then use that below?

http://codereview.chromium.org/1100001/diff/1/3#newcode244
src/platform/installer/chromeos-install:244: NUM_PMBR_BLOCKS=1
rather than call these blocks, since they seem to be 512 bytes in size, can we
call them sectors? Also, note in a comment that a sector is assumed to be 512
bytes.

http://codereview.chromium.org/1100001/diff/1/3#newcode269
src/platform/installer/chromeos-install:269: 
it might be a good check to make sure that no two partitions overlap. or maybe
the gpt tool already does that? (i have no idea if it does, but i could imagine
a partitioning tool doing that check)

http://codereview.chromium.org/1100001/diff/1/3#newcode272
src/platform/installer/chromeos-install:272: PMBRCODE=/tmp/gptmbr.bin
is there any reason to copy this to a temp file? You could just pull it from
$SRC and put it directly on $DST right? (and save 512bytes of memory...). yeah
pretty minor point...

http://codereview.chromium.org/1100001/diff/1/3#newcode280
src/platform/installer/chromeos-install:280: dd if=/dev/null of=${OUTDEV} bs=512
count=1 seek=$(($IMAGE_SIZE - 1))
where is IMAGE_SIZE set?

http://codereview.chromium.org/1100001/diff/1/3#newcode287
src/platform/installer/chromeos-install:287: seek=${START_GPT_FOOTER}
count=$(($NUM_GPT_HDR_BLOCKS + $NUM_GPT_TABLE_BLOCKS))
wrap to 80 cols

http://codereview.chromium.org/1100001/diff/1/3#newcode294
src/platform/installer/chromeos-install:294: sudo gpt add -b ${START_STATEFUL}
-s ${NUM_STATEFUL_BLOCKS} -t ${STATEFUL_GUID} ${OUTDEV}
wrap to 80 cols

http://codereview.chromium.org/1100001/diff/1/3#newcode300
src/platform/installer/chromeos-install:300: sudo gpt add -b ${START_ROOTFS_A}
-s ${NUM_ROOTFS_BLOCKS} -t ${ROOTFS_GUID} ${OUTDEV}
wrap to 80 cols

http://codereview.chromium.org/1100001/diff/1/3#newcode300
src/platform/installer/chromeos-install:300: sudo gpt add -b ${START_ROOTFS_A}
-s ${NUM_ROOTFS_BLOCKS} -t ${ROOTFS_GUID} ${OUTDEV}
is it okay to give multiple partitions the same GUID? it would mean that we
can't look by guid right?

http://codereview.chromium.org/1100001/diff/1/3#newcode306
src/platform/installer/chromeos-install:306: sudo gpt add -b ${START_ROOTFS_B}
-s ${NUM_ROOTFS_BLOCKS} -t ${ROOTFS_GUID} ${OUTDEV}
wrap to 80 cols

http://codereview.chromium.org/1100001/diff/1/3#newcode310
src/platform/installer/chromeos-install:310: sudo gpt boot -i 3 -b ${PMBRCODE}
${OUTDEV}
there are lots of magic numbers here ("-i #"). can we replace them with human
readable variables?

http://codereview.chromium.org/1100001/diff/1/3#newcode310
src/platform/installer/chromeos-install:310: sudo gpt boot -i 3 -b ${PMBRCODE}
${OUTDEV}
also, why does the PMBR, which is at the very first sector of the USB key, go
inside the rootfs?

http://codereview.chromium.org/1100001/diff/1/3#newcode334
src/platform/installer/chromeos-install:334: sudo ${TMPMNT}/postinst ${DST}3
again, '3' is a magic number. same w/ '5' below.

Also, since the new partition table hasn't been loaded by the kernel yet, might
the postinst script, which writes to ${DST}3, be writing to some random place on
the hard drive, as dictated by the old partition table?

http://codereview.chromium.org/1100001/diff/1/3#newcode345
src/platform/installer/chromeos-install:345: LOOP_DEV=$(sudo losetup -f)
call it STATE_LOOP_DEV?

http://codereview.chromium.org/1100001/diff/1/4
File src/platform/installer/chromeos-postinst (right):

http://codereview.chromium.org/1100001/diff/1/4#newcode23
src/platform/installer/chromeos-postinst:23: dd if=${INSTALL_ROOT}/boot/vmlinuz
of=${KINSTALL_DEV}
do we actually use this when booting? Our new firmware will use the kernel
partition, but won't it have to be formatted differently that this does (ie,
doesn't it contain some configuration file saying how long the kernel is?).

http://codereview.chromium.org/1100001/diff/1/5
File src/platform/memento_softwareupdate/memento_updater.sh (right):

http://codereview.chromium.org/1100001/diff/1/5#newcode109
src/platform/memento_softwareupdate/memento_updater.sh:109: LOCAL_DEV=$(rootdev)
again, where does rootdev come from?

http://codereview.chromium.org/1100001/diff/1/5#newcode118
src/platform/memento_softwareupdate/memento_updater.sh:118: KINSTALL_DEV=$(echo
$LOCAL_DEV | tr '35' '42')
do you mean "tr '35' '24'"?

http://codereview.chromium.org/1100001/diff/1/5#newcode274
src/platform/memento_softwareupdate/memento_updater.sh:274: gpt boot -i
$NEW_PART_NUM -b /tmp/oldpmbr.bin ${OUTDEV} 2>&1 | cat >> "$MEMENTO_AU_LOG"
hm, why are we installing the pmbr somewhere?

http://codereview.chromium.org/1100001/diff/1/6
File src/scripts/build_gpt.sh (right):

http://codereview.chromium.org/1100001/diff/1/6#newcode48
src/scripts/build_gpt.sh:48: flags_help
is this a command?

http://codereview.chromium.org/1100001/diff/1/6#newcode80
src/scripts/build_gpt.sh:80: set -u
what does -u do?

http://codereview.chromium.org/1100001/diff/1/6#newcode115
src/scripts/build_gpt.sh:115:
STATEFUL_GUID='ebd0a0a2-b9e5-4433-87c0-68b6b72699c7'
are these duplicated? if so, move them to somewhere common

http://codereview.chromium.org/1100001/diff/1/6#newcode122
src/scripts/build_gpt.sh:122: roundup() {
move this somewhere common?

http://codereview.chromium.org/1100001/diff/1/6#newcode133
src/scripts/build_gpt.sh:133: # The gpt tool requires a fixed-size target to
work on, so we may have to create
move this somewhere common

http://codereview.chromium.org/1100001/diff/1/6#newcode173
src/scripts/build_gpt.sh:173: # Figure out how much space we need, in blocks.
same here. let's aim for zero code duplication

http://codereview.chromium.org/1100001/diff/1/6#newcode199
src/scripts/build_gpt.sh:199: IMAGE_SIZE=$(($START_GPT_FOOTER +
$NUM_GPT_TABLE_BLOCKS + $NUM_GPT_HDR_BLOCKS))
80 cols

http://codereview.chromium.org/1100001/diff/1/6#newcode240
src/scripts/build_gpt.sh:240: hexdump -v -C "$MBR_IMG"
is this just for debugging?

http://codereview.chromium.org/1100001/diff/1/6#newcode253
src/scripts/build_gpt.sh:253: if [[ ! -e ${OUTDEV} ]]; then
avoid duplication, and where is IMAGE_SIZE set?

http://codereview.chromium.org/1100001/diff/1/6#newcode257
src/scripts/build_gpt.sh:257: # Zap any old partitions (otherwise gpt complains)
you could create a common function to erase all partition tables from a device
and avoid duplication

http://codereview.chromium.org/1100001/diff/1/6#newcode263
src/scripts/build_gpt.sh:263: # Create the new GPT partitions. The order
determines the partition number. The
same here about avoiding duplication and same comments from the installer file

http://codereview.chromium.org/1100001/diff/1/7
File src/scripts/build_image (right):

http://codereview.chromium.org/1100001/diff/1/7#newcode360
src/scripts/build_image:360: echo "Done.  Image created in ${OUTPUT_DIR}"
can you make sure these instructions still work?

http://codereview.chromium.org/1100001/diff/1/8
File src/scripts/image_to_usb.sh (left):

http://codereview.chromium.org/1100001/diff/1/8#oldcode187
src/scripts/image_to_usb.sh:187: ! sudo umount "$i"
why get rid of the '!'?

[sosa, 2010-03-18 23:15:43]

One thing I forgot.

This change breaks mod_image_for_test breaking out autotest mechanisms.  Either
you need to add changes to mod_image_for_test or there would have to be a change
immediately after

[Bill Richardson, 2010-03-18 23:45:28]

I'll look into it. Thanks for all the quick feedback. I'm still hoping to
have all the questions addressed by tomorrow afternoon so it can get into
the R6 snapshot.

On Thu, Mar 18, 2010 at 4:15 PM, <sosa@chromium.org> wrote:

> One thing I forgot.
>
> This change breaks mod_image_for_test breaking out autotest mechanisms.
>  Either
> you need to add changes to mod_image_for_test or there would have to be a
> change
> immediately after
>
>
> http://codereview.chromium.org/1100001
>

[adlr, 2010-03-27 03:25:24]

Great progress. A lot of these comments are nits that should be quick to fix.

I'm excited for GPT!!

http://codereview.chromium.org/1100001/diff/1/3
File src/platform/installer/chromeos-install (right):

http://codereview.chromium.org/1100001/diff/1/3#newcode198
src/platform/installer/chromeos-install:198: #   Primary GPT Table (16K)
On 2010/03/18 21:30:22, sosa wrote:
> It makes more sense to order these comments by partition #

Sosa: see
https://docs.google.com/a/google.com/Doc?docid=0AWhi6uVK10xEZGQ5azlobnRfN2Nic...
for info

http://codereview.chromium.org/1100001/diff/14001/15002
File src/platform/installer/chromeos-common.sh (right):

http://codereview.chromium.org/1100001/diff/14001/15002#newcode26
src/platform/installer/chromeos-common.sh:26: function numsectors {
change to more generic "numsectors() {"

http://codereview.chromium.org/1100001/diff/14001/15002#newcode83
src/platform/installer/chromeos-common.sh:83: GPT=$(which gpt 2>/dev/null)
add "|| true" to the end of the line, since if which fails to find GPT, it will
return 1, which will cause the shell to exit if "set -e" is set.

http://codereview.chromium.org/1100001/diff/14001/15002#newcode85
src/platform/installer/chromeos-common.sh:85: if [ -x
${DEFAULT_CHROOT_DIR}/usr/bin/gpt ]; then
put ${DEFAULT_CHROOT_DIR}/usr/bin/gpt in quotes

http://codereview.chromium.org/1100001/diff/14001/15002#newcode99
src/platform/installer/chromeos-common.sh:99: # Return: nothing useful
change to "Return: nothing"?

http://codereview.chromium.org/1100001/diff/14001/15002#newcode105
src/platform/installer/chromeos-common.sh:105: #   START_KERN_A
what units are these in? sectors?

http://codereview.chromium.org/1100001/diff/14001/15002#newcode176
src/platform/installer/chromeos-common.sh:176: local image_size=$(numsectors
$outdev)
s/image_size/image_sector_size/

http://codereview.chromium.org/1100001/diff/14001/15002#newcode212
src/platform/installer/chromeos-common.sh:212: dd if=/dev/null of=${outdev}
bs=512 count=1 seek=$(($image_size - 1))
b/c reads from /dev/null don't return any data, this will create a file one
sector size too short.

In general we've set if=/dev/zero to avoid this.

http://codereview.chromium.org/1100001/diff/14001/15002#newcode285
src/platform/installer/chromeos-common.sh:285: [[ -n "$X" ]] && echo $X
replace with simply "echo $X"?

http://codereview.chromium.org/1100001/diff/14001/15003
File src/platform/installer/chromeos-install (right):

http://codereview.chromium.org/1100001/diff/14001/15003#newcode1
src/platform/installer/chromeos-install:1: #!/bin/bash -eu
can you switch back to /bin/sh?

http://codereview.chromium.org/1100001/diff/14001/15003#newcode38
src/platform/installer/chromeos-install:38: ROOTDEV=$(rootdev)
maybe i'm missing something, but where is the rootdev command?

http://codereview.chromium.org/1100001/diff/14001/15003#newcode73
src/platform/installer/chromeos-install:73: sudo -K  # Force them to enter a
password (well, it used to matter)
"(well, it used to matter)" seems out of place here. can we remove it? What is
it in reference to?

http://codereview.chromium.org/1100001/diff/14001/15003#newcode91
src/platform/installer/chromeos-install:91: # Remember the loop device using the
global variable LOOP_DEV.
you're using a lot of globals here (LOOP_DEV, TMPMNT). Would it be much trouble
to convert them to locals and pass them in/out. One way to return a value from a
bash function is to echo it in the function. E.g.

get_message() {
  echo "Hello, $1"
}

MESSAGE=$(get_message "World")

echo $MESSAGE
Hello, World

http://codereview.chromium.org/1100001/diff/14001/15003#newcode94
src/platform/installer/chromeos-install:94: my_loop() {
perhaps call it something like "offset_loop_setup"?

http://codereview.chromium.org/1100001/diff/14001/15003#newcode109
src/platform/installer/chromeos-install:109: my_uloop() {
perhaps call it something like "offset_loop_cleanup"?

http://codereview.chromium.org/1100001/diff/14001/15003#newcode118
src/platform/installer/chromeos-install:118: my_mount() {
if you use locals, you can probably get rid of this one-liner. If you keep it,
at least pick a less generic name.

http://codereview.chromium.org/1100001/diff/14001/15003#newcode123
src/platform/installer/chromeos-install:123: my_umount() {
also:

if you use locals, you can probably get rid of this one-liner. If you keep it,
at least pick a less generic name.

http://codereview.chromium.org/1100001/diff/14001/15003#newcode135
src/platform/installer/chromeos-install:135: # Invoke as: command
Do you really invoke it as "command"?

http://codereview.chromium.org/1100001/diff/14001/15003#newcode232
src/platform/installer/chromeos-install:232: sudo mkfs.ext3 -F -b 4096 -L
"H-STATE" ${LOOP_DEV} $(($NUM_STATEFUL_SECTORS / 8))
80 cols

http://codereview.chromium.org/1100001/diff/14001/15004
File src/platform/installer/chromeos-postinst (right):

http://codereview.chromium.org/1100001/diff/14001/15004#newcode25
src/platform/installer/chromeos-postinst:25: dd if=${INSTALL_ROOT}/boot/vmlinuz
of=${KINSTALL_DEV}
# TODO: install both the kernel and kernel config file into kernel partition

http://codereview.chromium.org/1100001/diff/14001/15005
File src/platform/memento_softwareupdate/memento_updater.sh (right):

http://codereview.chromium.org/1100001/diff/14001/15005#newcode236
src/platform/memento_softwareupdate/memento_updater.sh:236:
"$MOUNTPOINT"/postinst "$INSTALL_DEV" "$KINSTALL_DEV" 2>&1 | cat >>
"$MEMENTO_AU_LOG"; \
80 cols

http://codereview.chromium.org/1100001/diff/14001/15005#newcode274
src/platform/memento_softwareupdate/memento_updater.sh:274: gpt -S boot -i
$NEW_PART_NUM -b /tmp/oldpmbr.bin ${ROOT_DEV} 2>&1 | cat >> "$MEMENTO_AU_LOG"
80 cols.

also, what does this command do?

http://codereview.chromium.org/1100001/diff/14001/15009
File src/scripts/image_to_usb.sh (right):

http://codereview.chromium.org/1100001/diff/14001/15009#newcode95
src/scripts/image_to_usb.sh:95: cp -f ${SRC_IMAGE}
"${FLAGS_from}/chromiumos_test_image.bin"
quotes around ${SRC_IMAGE}

http://codereview.chromium.org/1100001/diff/14001/15009#newcode99
src/scripts/image_to_usb.sh:99: ${SCRIPTS_DIR}/mod_image_for_test.sh --image
${SRC_IMAGE}
quotes around ${SCRIPTS_DIR}/mod_image_for_test.sh and ${SRC_IMAGE}

http://codereview.chromium.org/1100001/diff/14001/15009#newcode119
src/scripts/image_to_usb.sh:119: offset=$(partoffset ${SRC_IMAGE} 1)
quotes around ${SRC_IMAGE}

http://codereview.chromium.org/1100001/diff/14001/15009#newcode130
src/scripts/image_to_usb.sh:130: sudo losetup -o $(( $offset * 512 ))
${stateful_loop_dev} ${SRC_IMAGE}
quotes around ${stateful_loop_dev} and ${SRC_IMAGE}

http://codereview.chromium.org/1100001/diff/14001/15009#newcode136
src/scripts/image_to_usb.sh:136: sudo cp -fpru ${AUTOTEST_SRC}/client/* \
not your code, but change ${AUTOTEST_SRC}/client/* to "${AUTOTEST_SRC}/client"/*
(notice the quotes)

http://codereview.chromium.org/1100001/diff/14001/15009#newcode141
src/scripts/image_to_usb.sh:141: sudo umount ${STATEFUL_DIR}
again, not your code, but quotes around ${STATEFUL_DIR}

http://codereview.chromium.org/1100001/diff/14001/15009#newcode159
src/scripts/image_to_usb.sh:159: if echo ${FLAGS_to} | grep -q '[0-9]$'; then
quotes around ${FLAGS_to}

http://codereview.chromium.org/1100001/diff/14001/15009#newcode160
src/scripts/image_to_usb.sh:160: local drive=$(echo ${FLAGS_to} | sed -re
's/[0-9]+$//')
quotes around ${FLAGS_to}

http://codereview.chromium.org/1100001/diff/14001/15009#newcode185
src/scripts/image_to_usb.sh:185: for i in $(mount | grep ^${FLAGS_to} | awk
'{print $1'})
s/$1'})/$1}')/

http://codereview.chromium.org/1100001/diff/14001/15009#newcode192
src/scripts/image_to_usb.sh:192: sudo dd if=${SRC_IMAGE} of=${FLAGS_to} bs=4M
quotes around SRC_IMAGE, FLAGS_to

http://codereview.chromium.org/1100001/diff/14001/15009#newcode198
src/scripts/image_to_usb.sh:198: cp -f ${SRC_IMAGE} ${FLAGS_to}
quotes around these two, too

http://codereview.chromium.org/1100001/diff/14001/15009#newcode205
src/scripts/image_to_usb.sh:205: foo=$(basename ${FLAGS_to})
pick a better variable name

http://codereview.chromium.org/1100001/diff/14001/15010
File src/scripts/mod_image_for_test.sh (right):

http://codereview.chromium.org/1100001/diff/14001/15010#newcode99
src/scripts/mod_image_for_test.sh:99: ROOT_FS_DIR=$(dirname
${FLAGS_image})/rootfs
quotes around ${FLAGS_image}

http://codereview.chromium.org/1100001/diff/14001/15010#newcode104
src/scripts/mod_image_for_test.sh:104: offset=$(partoffset ${FLAGS_image} 3)
quotes around ${FLAGS_image}

http://codereview.chromium.org/1100001/diff/14001/15010#newcode107
src/scripts/mod_image_for_test.sh:107: sudo losetup -o $(( $offset * 512 ))
"${LOOP_DEV}" "${FLAGS_image}"
if [ -z "$LOOP_DEV" ]; then
  echo "No free loop device"
  exit 1
fi

[Bill Richardson, 2010-03-29 18:55:09]

Okay

http://codereview.chromium.org/1100001/diff/14001/15002
File src/platform/installer/chromeos-common.sh (right):

http://codereview.chromium.org/1100001/diff/14001/15002#newcode212
src/platform/installer/chromeos-common.sh:212: dd if=/dev/null of=${outdev}
bs=512 count=1 seek=$(($image_size - 1))
On 2010/03/27 03:25:25, adlr wrote:
> b/c reads from /dev/null don't return any data, this will create a file one
> sector size too short.
> 
> In general we've set if=/dev/zero to avoid this.

So do I. This is a typo.

http://codereview.chromium.org/1100001/diff/14001/15002#newcode285
src/platform/installer/chromeos-common.sh:285: [[ -n "$X" ]] && echo $X
On 2010/03/27 03:25:25, adlr wrote:
> replace with simply "echo $X"?

No, because we want the function to fail if X is empty.

http://codereview.chromium.org/1100001/diff/14001/15003
File src/platform/installer/chromeos-install (right):

http://codereview.chromium.org/1100001/diff/14001/15003#newcode1
src/platform/installer/chromeos-install:1: #!/bin/bash -eu
On 2010/03/27 03:25:25, adlr wrote:
> can you switch back to /bin/sh?

Bleah. Fine.

http://codereview.chromium.org/1100001/diff/14001/15003#newcode38
src/platform/installer/chromeos-install:38: ROOTDEV=$(rootdev)
On 2010/03/27 03:25:25, adlr wrote:
> maybe i'm missing something, but where is the rootdev command?

/usr/bin/rootdev.

Source in src/third_party/rootdev/

http://codereview.chromium.org/1100001/diff/14001/15003#newcode73
src/platform/installer/chromeos-install:73: sudo -K  # Force them to enter a
password (well, it used to matter)
On 2010/03/27 03:25:25, adlr wrote:
> "(well, it used to matter)" seems out of place here. can we remove it? What is
> it in reference to?

As of Feb 1, /etc/sudoers now has this in it:

chronos ALL=NOPASSWD: ALL

so no password is needed for sudo. I didn't want to completely remove this test,
in case this turned out to be a mistake.

http://codereview.chromium.org/1100001/diff/14001/15003#newcode91
src/platform/installer/chromeos-install:91: # Remember the loop device using the
global variable LOOP_DEV.
On 2010/03/27 03:25:25, adlr wrote:
> you're using a lot of globals here (LOOP_DEV, TMPMNT). Would it be much
trouble
> to convert them to locals and pass them in/out. One way to return a value from
a
> bash function is to echo it in the function. E.g.
> 
> get_message() {
>   echo "Hello, $1"
> }
> 
> MESSAGE=$(get_message "World")
> 
> echo $MESSAGE
> Hello, World

Yes, it's ugly, but we have to use globals so that the cleanup functions (which
are triggered on errors) know what to clean. I think it's slightly easier to
read if you see the globals being assigned in the setup function. Otherwise, you
have to go looking for the invocation of the setup functions, where the caller
has to remember to use the correct global name for the returned value. Setting
the correct global variable is a mandatory side-effect, or the cleanup won't
work.

http://codereview.chromium.org/1100001/diff/14001/15003#newcode94
src/platform/installer/chromeos-install:94: my_loop() {
On 2010/03/27 03:25:25, adlr wrote:
> perhaps call it something like "offset_loop_setup"?

Good suggestion.

http://codereview.chromium.org/1100001/diff/14001/15003#newcode135
src/platform/installer/chromeos-install:135: # Invoke as: command
On 2010/03/27 03:25:25, adlr wrote:
> Do you really invoke it as "command"?

Yes. There are two ways to invoke shell functions. As a command:

  foo arg arg arg

or as a subshell:

  $(foo arg arg arg)

Commands can read and set global variables, but can't return any values.
Subshells can read global variables but can't set them in the caller's
environment, and can return values with "echo".

My convention is that if I want it to return a value directly, it needs to be
invoked as a subshell. Otherwise, it should be invoked as a command.

http://codereview.chromium.org/1100001/diff/14001/15009
File src/scripts/image_to_usb.sh (right):

http://codereview.chromium.org/1100001/diff/14001/15009#newcode95
src/scripts/image_to_usb.sh:95: cp -f ${SRC_IMAGE}
"${FLAGS_from}/chromiumos_test_image.bin"
On 2010/03/27 03:25:25, adlr wrote:
> quotes around ${SRC_IMAGE}

Ugh. I need to do this in lots of places. The shell expands it okay without the
quotes, but won't use it properly if it has spaces in it. Thanks.

http://codereview.chromium.org/1100001/diff/14001/15009#newcode185
src/scripts/image_to_usb.sh:185: for i in $(mount | grep ^${FLAGS_to} | awk
'{print $1'})
On 2010/03/27 03:25:25, adlr wrote:
> s/$1'})/$1}')/

Good catch. Thanks.

[adlr, 2010-03-30 19:50:20]

LGTM w/ small comment

Awesome work!

http://codereview.chromium.org/1100001/diff/14001/15002
File src/platform/installer/chromeos-common.sh (right):

http://codereview.chromium.org/1100001/diff/14001/15002#newcode285
src/platform/installer/chromeos-common.sh:285: [[ -n "$X" ]] && echo $X
On 2010/03/29 18:55:09, Bill Richardson wrote:
> On 2010/03/27 03:25:25, adlr wrote:
> > replace with simply "echo $X"?
> 
> No, because we want the function to fail if X is empty.

the function doesn't seem to fail if X is empty:

$ bash
$ set -e
$ X=""
$ INNER_SHELL=1
$ [ -n "$X" ] && echo $X
$ echo $INNER_SHELL
1

you probably want:

  ...
  test -n "$X"
  echo "$X"
}

http://codereview.chromium.org/1100001/diff/30003/31002#newcode297
src/platform/installer/chromeos-common.sh:297: }
same here?

http://codereview.chromium.org/1100001/diff/30003/31002#newcode308
src/platform/installer/chromeos-common.sh:308: }
same here?

[sosa, 2010-03-30 20:11:32]

LGTM from the testing perspective.  Thanks for doing those!

[robotboy, 2010-03-30 20:14:34]

LGTM, we can make ARM use this once it's in and working for x86.  It looks like
you could remove the kernel_fetcher that I wrote as well with this.