mod_image_for_recovery: supprt new recovery model

mod_image_for_recovery: supprt new recovery model

With a change for chromeos-installer and http://codereview.chromium.org/4120005,
switched to a kernel image based approach.


BUG=chromium-os:7451
TEST= See http://codereview.chromium.org/4120005

[Will Drewry, 2010-10-29 20:17:58]

Fix reviewer. Doh!

[anush, 2010-11-01 17:51:45]

Few comments below. I understand this is inherited code but will be good to
error out where possible (without having to rewrite all the code).

http://codereview.chromium.org/4107003/diff/22001/23001
File make_developer_script_runner.sh (right):

http://codereview.chromium.org/4107003/diff/22001/23001#newcode20
make_developer_script_runner.sh:20: "Number of sectors to use for the stateful
filesystem"
nit: would be nice to take human readable sizes.

http://codereview.chromium.org/4107003/diff/22001/23001#newcode21
make_developer_script_runner.sh:21: # TODO: default to dev keys
Isn't this done below?

http://codereview.chromium.org/4107003/diff/22001/23001#newcode46
make_developer_script_runner.sh:46: if [ -z "$FLAGS_developer_script" -o \
Aren't the private key and keyblock) available as defaults? So should this only
check _script?

http://codereview.chromium.org/4107003/diff/22001/23001#newcode61
make_developer_script_runner.sh:61: seek=$((1 + max_kern_size + header_offset +
FLAGS_stateful_sectors))
do we want to die on failure ?

http://codereview.chromium.org/4107003/diff/22001/23001#newcode69
make_developer_script_runner.sh:69: seek=${FLAGS_statefulfs_sectors} count=0
Same die on failure ?

http://codereview.chromium.org/4107003/diff/22001/23001#newcode70
make_developer_script_runner.sh:70: /sbin/mkfs.ext3 -F -b 4096 $stateful_image
ditto - die on failure ?

http://codereview.chromium.org/4107003/diff/22001/23001#newcode98
make_developer_script_runner.sh:98: seek=$offset count=$FLAGS_statefulfs_sectors
exit on failures for the two commands above and following commands?

http://codereview.chromium.org/4107003/diff/22001/23003
File mod_image_for_recovery.sh (right):

http://codereview.chromium.org/4107003/diff/22001/23003#newcode3
mod_image_for_recovery.sh:3: # Copyright (c) 2009 The Chromium OS Authors. All
rights reserved.
as we chatted 2010.

http://codereview.chromium.org/4107003/diff/22001/23003#newcode125
mod_image_for_recovery.sh:125: WORKON="-g"
Maybe this should be called PORTAGE_FLAGS ?
I think we shouldn't use -g which downloads from the prebuilt server. And the
kernel in the prebuilt isnt built with initramfs.
http://build.chromium.org/mirror/chromiumos/prebuilt/board/x86-generic/Packages

http://codereview.chromium.org/4107003/diff/22001/23003#newcode128
mod_image_for_recovery.sh:128: fi
this should just be the default without anycheck to cros_workon

http://codereview.chromium.org/4107003/diff/22001/23003#newcode189
mod_image_for_recovery.sh:189: local kern_hash=$(busybox sha1sum "$kern_tmp" |
cut -f1 -d' ')
why busybox sha1sum and not just "sha1sum"

http://codereview.chromium.org/4107003/diff/22001/23003#newcode283
mod_image_for_recovery.sh:283: # TODO(wad) ignoring the dev payload for bow
now :)

[Will Drewry, 2010-11-01 18:23:19]

Thanks! PTAL :)

(Rerunning a base-case test now.)

http://codereview.chromium.org/4107003/diff/22001/23001
File make_developer_script_runner.sh (right):

http://codereview.chromium.org/4107003/diff/22001/23001#newcode20
make_developer_script_runner.sh:20: "Number of sectors to use for the stateful
filesystem"
On 2010/11/01 17:51:46, anush wrote:
> nit: would be nice to take human readable sizes. 

Done.

http://codereview.chromium.org/4107003/diff/22001/23001#newcode21
make_developer_script_runner.sh:21: # TODO: default to dev keys
On 2010/11/01 17:51:46, anush wrote:
> Isn't this done below? 

Done.

http://codereview.chromium.org/4107003/diff/22001/23001#newcode46
make_developer_script_runner.sh:46: if [ -z "$FLAGS_developer_script" -o \
On 2010/11/01 17:51:46, anush wrote:
> Aren't the private key and keyblock) available as defaults? So should this
only
> check _script?

WFM :) Done!

http://codereview.chromium.org/4107003/diff/22001/23001#newcode61
make_developer_script_runner.sh:61: seek=$((1 + max_kern_size + header_offset +
FLAGS_stateful_sectors))
On 2010/11/01 17:51:46, anush wrote:
> do we want to die on failure ? 

Done.

http://codereview.chromium.org/4107003/diff/22001/23001#newcode69
make_developer_script_runner.sh:69: seek=${FLAGS_statefulfs_sectors} count=0
On 2010/11/01 17:51:46, anush wrote:
> Same die on failure ?

Done.

http://codereview.chromium.org/4107003/diff/22001/23001#newcode70
make_developer_script_runner.sh:70: /sbin/mkfs.ext3 -F -b 4096 $stateful_image
On 2010/11/01 17:51:46, anush wrote:
> ditto - die on failure ?

Done.

http://codereview.chromium.org/4107003/diff/22001/23001#newcode98
make_developer_script_runner.sh:98: seek=$offset count=$FLAGS_statefulfs_sectors
On 2010/11/01 17:51:46, anush wrote:
> exit on failures for the two commands above and following commands?

Done.

http://codereview.chromium.org/4107003/diff/22001/23003
File mod_image_for_recovery.sh (right):

http://codereview.chromium.org/4107003/diff/22001/23003#newcode3
mod_image_for_recovery.sh:3: # Copyright (c) 2009 The Chromium OS Authors. All
rights reserved.
On 2010/11/01 17:51:46, anush wrote:
> as we chatted 2010. 

Done.

http://codereview.chromium.org/4107003/diff/22001/23003#newcode3
mod_image_for_recovery.sh:3: # Copyright (c) 2009 The Chromium OS Authors. All
rights reserved.
On 2010/11/01 17:51:46, anush wrote:
> as we chatted 2010. 

Done.

http://codereview.chromium.org/4107003/diff/22001/23003#newcode125
mod_image_for_recovery.sh:125: WORKON="-g"
On 2010/11/01 17:51:46, anush wrote:
> Maybe this should be called PORTAGE_FLAGS ?
> I think we shouldn't use -g which downloads from the prebuilt server. And the
> kernel in the prebuilt isnt built with initramfs.
>
http://build.chromium.org/mirror/chromiumos/prebuilt/board/x86-generic/Packages

Done.

http://codereview.chromium.org/4107003/diff/22001/23003#newcode128
mod_image_for_recovery.sh:128: fi
On 2010/11/01 17:51:46, anush wrote:
> this should just be the default without anycheck to cros_workon

Done!  That's how I had it at first but I figured that I could reuse a
pre-existing package if there was one from the factory image generation.

http://codereview.chromium.org/4107003/diff/22001/23003#newcode189
mod_image_for_recovery.sh:189: local kern_hash=$(busybox sha1sum "$kern_tmp" |
cut -f1 -d' ')
On 2010/11/01 17:51:46, anush wrote:
> why busybox sha1sum and not just "sha1sum"

I just wanted to ensure it behaved exactly as the initramfs does.  I'll just use
the normal unless we witness a recovery image not booting.

http://codereview.chromium.org/4107003/diff/22001/23003#newcode283
mod_image_for_recovery.sh:283: # TODO(wad) ignoring the dev payload for bow
Even better - there is no more "dev payload" case! Done.

[anush, 2010-11-01 22:29:47]

LGTM - with ignorable nit :)

http://codereview.chromium.org/4107003/diff/28001/3002
File make_developer_script_runner.sh (right):

http://codereview.chromium.org/4107003/diff/28001/3002#newcode20
make_developer_script_runner.sh:20: "Number of mebibytes to use for the stateful
filesystem"
Nit: mebibyte - does it come after the yottabyte :)