make_dev_ssd: new script to change SSD image to dev key

make_dev_ssd: new script to change SSD image to dev key

The make_dev_ssd.sh is made for devinstall shim to
change SSD kernels to be signed by dev keys.

 - Kernel A, B will be resigned with dev keys (ignore if A/B seems not bootable)
 - Adding param --remove_rootfs_verification can even disable rootfs hash check

This CL also includes some shared refine/fix to make_dev_firmware.sh

BUG=chrome-os-partner:1276
TEST=sudo ./make_dev_ssd.sh; (seeing Kernel A is resigned and B is ignored)
     then reboot without developer mode (OK),
          rootdev shows /dev/dm-0, rootdev -s shows /dev/sda3
     sudo ./make_dev_ssd.sh --remove_rootfs_verification;
     then reboot without developer mode (OK), rootdev shows /dev/sda3

Change-Id: Ic20f734b2af42e50a43c19a565a166a39d57a7fd

Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=20525b9

[Hung-Te, 2010-10-15 13:07:11]

wfrichar: please confirm if the behavior is what you expected
gauravsh: please review the script code itself
rspangler: FYI

[Bill Richardson, 2010-10-15 18:34:25]

LGTM.

[gauravsh, 2010-10-15 20:30:45]

http://codereview.chromium.org/3772013/diff/6001/7002
File scripts/image_signing/make_dev_ssd.sh (right):

http://codereview.chromium.org/3772013/diff/6001/7002#newcode41
scripts/image_signing/make_dev_ssd.sh:41: err_die() {
since this and a few other functions below are used in the other script - it's
fine to move it to common.sh

(However, I'd like to hold off on adding anything to common.sh that adds an
external dependency before discussing it first.)

http://codereview.chromium.org/3772013/diff/6001/7002#newcode58
scripts/image_signing/make_dev_ssd.sh:58: # Checks if the files given by
parameters all exist.
good candidate for moving to common. sh if you can make it shflags-independent

a better name for this may be ensure_files_exist or something like that  (to
make it clear what is being checked for existence)

http://codereview.chromium.org/3772013/diff/6001/7002#newcode92
scripts/image_signing/make_dev_ssd.sh:92: echo_kernel_name() {
seems rather overkill to have a function for this...

http://codereview.chromium.org/3772013/diff/6001/7002#newcode119
scripts/image_signing/make_dev_ssd.sh:119: local name="$(echo_kernel_name
$kernel_index)"
kernel_name

http://codereview.chromium.org/3772013/diff/6001/7002#newcode124
scripts/image_signing/make_dev_ssd.sh:124: local offset="$(cgpt show -b -i
$kernel_index $ssd_device)"
use partoffset here

http://codereview.chromium.org/3772013/diff/6001/7002#newcode125
scripts/image_signing/make_dev_ssd.sh:125: local size="$(cgpt show -s -i
$kernel_index $ssd_device)"
use partsize here

http://codereview.chromium.org/3772013/diff/6001/7002#newcode142
scripts/image_signing/make_dev_ssd.sh:142: if [ $(od -A n "$old_blob" | head |
wc -l) -lt 4 ]; then
this seems a bit hacky to me and I am not sure I understand the need for this?
Will dump_kernel_config not catch problems here? How about vbutil_kernel? 

If none of them complain, that's fine too - the end effect is the same.

http://codereview.chromium.org/3772013/diff/6001/7002#newcode157
scripts/image_signing/make_dev_ssd.sh:157: # TODO(hungte) $KERNEL_KEYBLOCK and
$new_kernel_config should also be
Is this a TODO? Or just a comment on why you don't quote?

http://codereview.chromium.org/3772013/diff/6001/7002#newcode184
scripts/image_signing/make_dev_ssd.sh:184: debug_msg "for debug purpose, check:
old_blob, new_blob, new_ken"
s/purpose/purposes/

http://codereview.chromium.org/3772013/diff/6001/7002#newcode218
scripts/image_signing/make_dev_ssd.sh:218: sync; sync; sync
why this sync-fest? a comment here, maybe?

http://codereview.chromium.org/3772013/diff/6001/7002#newcode234
scripts/image_signing/make_dev_ssd.sh:234: debug_msg "Pre-requisition check"
Prerequisite check

http://codereview.chromium.org/3772013/diff/6001/7002#newcode246
scripts/image_signing/make_dev_ssd.sh:246: echo "Successfully resigned
$num_signed kernel(s) on device $FLAGS_image".
re-signed

http://codereview.chromium.org/3772013/diff/6001/7002#newcode248
scripts/image_signing/make_dev_ssd.sh:248: err_die "Failed resigning kernels."
re-signing

[Hung-Te, 2010-10-16 00:12:27]

Thanks for the review. Comments are addressed with fixes:

http://codereview.chromium.org/3772013/diff/6001/7002
File scripts/image_signing/make_dev_ssd.sh (right):

http://codereview.chromium.org/3772013/diff/6001/7002#newcode41
scripts/image_signing/make_dev_ssd.sh:41: err_die() {
I was not sure if we're going to change the layout/folder of these files so I
didn't make more things go into common.sh.

is_debug_mode and debug_msg seems also like good candidates to common.sh (may
even err_die). If you're  OK with them, we can create another CL to isolate the
common libraries.

http://codereview.chromium.org/3772013/diff/6001/7002#newcode58
scripts/image_signing/make_dev_ssd.sh:58: # Checks if the files given by
parameters all exist.
Actually the logic is a little different now in these two files. One allows
devices file because it works on ssd. However I think we can allow them to check
only exist (-f).

I've changed the function name and moved to common.sh.

http://codereview.chromium.org/3772013/diff/6001/7002#newcode92
scripts/image_signing/make_dev_ssd.sh:92: echo_kernel_name() {
At first I was expecting something like ${part_name${index}}, but bash/dash does
not allow that. Then I think, we may have some more names to display (ex,
rootfs) in the future, so it's fine to make it a function.

http://codereview.chromium.org/3772013/diff/6001/7002#newcode119
scripts/image_signing/make_dev_ssd.sh:119: local name="$(echo_kernel_name
$kernel_index)"
done as "cros_kernel_name"

http://codereview.chromium.org/3772013/diff/6001/7002#newcode124
scripts/image_signing/make_dev_ssd.sh:124: local offset="$(cgpt show -b -i
$kernel_index $ssd_device)"
On 2010/10/15 20:30:45, gauravsh wrote:
> use partoffset here

Done.

http://codereview.chromium.org/3772013/diff/6001/7002#newcode125
scripts/image_signing/make_dev_ssd.sh:125: local size="$(cgpt show -s -i
$kernel_index $ssd_device)"
On 2010/10/15 20:30:45, gauravsh wrote:
> use partsize here

Done.

http://codereview.chromium.org/3772013/diff/6001/7002#newcode142
scripts/image_signing/make_dev_ssd.sh:142: if [ $(od -A n "$old_blob" | head |
wc -l) -lt 4 ]; then
The old_kernel_config was not always checked when I first write this part. (But
since we have it now, I think it's OK to remove it.) Thanks for finding this.
Done.

http://codereview.chromium.org/3772013/diff/6001/7002#newcode157
scripts/image_signing/make_dev_ssd.sh:157: # TODO(hungte) $KERNEL_KEYBLOCK and
$new_kernel_config should also be
I'd comment more for what to do. Done

http://codereview.chromium.org/3772013/diff/6001/7002#newcode184
scripts/image_signing/make_dev_ssd.sh:184: debug_msg "for debug purpose, check:
old_blob, new_blob, new_ken"
On 2010/10/15 20:30:45, gauravsh wrote:
> s/purpose/purposes/

Done.

http://codereview.chromium.org/3772013/diff/6001/7002#newcode218
scripts/image_signing/make_dev_ssd.sh:218: sync; sync; sync
On 2010/10/15 20:30:45, gauravsh wrote:
> why this sync-fest? a comment here, maybe?

Done.

http://codereview.chromium.org/3772013/diff/6001/7002#newcode234
scripts/image_signing/make_dev_ssd.sh:234: debug_msg "Pre-requisition check"
On 2010/10/15 20:30:45, gauravsh wrote:
> Prerequisite check

Done.

http://codereview.chromium.org/3772013/diff/6001/7002#newcode246
scripts/image_signing/make_dev_ssd.sh:246: echo "Successfully resigned
$num_signed kernel(s) on device $FLAGS_image".
On 2010/10/15 20:30:45, gauravsh wrote:
> re-signed

Done.

http://codereview.chromium.org/3772013/diff/6001/7002#newcode248
scripts/image_signing/make_dev_ssd.sh:248: err_die "Failed resigning kernels."
On 2010/10/15 20:30:45, gauravsh wrote:
> re-signing

Done.

[Hung-Te, 2010-10-16 00:13:53]

http://codereview.chromium.org/3772013/diff/6001/7002
File scripts/image_signing/make_dev_ssd.sh (right):

http://codereview.chromium.org/3772013/diff/6001/7002#newcode58
scripts/image_signing/make_dev_ssd.sh:58: # Checks if the files given by
parameters all exist.
sorry, I mean we can always allow -f and -b

[gauravsh, 2010-10-16 00:29:10]

LGTM with some comments

I'd like to keep any dependencies on shflags as restricted as possible, mainly
because of the re-factoring I have in mind. Basically, I'd like to keep the
shflags dependency only on scripts that are run from the live image or within
chroot. having the shflags dependency makes it hard to just move and run scripts
to a different host without copying the whole image_signing/ folder which is a
problem for scripts that will be used in the signing server.

http://codereview.chromium.org/3772013/diff/6001/7002
File scripts/image_signing/make_dev_ssd.sh (right):

http://codereview.chromium.org/3772013/diff/6001/7002#newcode41
scripts/image_signing/make_dev_ssd.sh:41: err_die() {
On 2010/10/16 00:12:27, Hung-Te wrote:
> I was not sure if we're going to change the layout/folder of these files so I
> didn't make more things go into common.sh.
> 
> is_debug_mode and debug_msg seems also like good candidates to common.sh (may
> even err_die). If you're  OK with them, we can create another CL to isolate
the
> common libraries.

only issue I have is with is_debug_mode() - I would prefer to  keep any
functions that depend on shflags to not be in common.sh. I will have a separate
discussion with you about why prefer keeping the shflags dependency only on
as-needed basis

http://codereview.chromium.org/3772013/diff/14001/15001
File scripts/image_signing/common.sh (right):

http://codereview.chromium.org/3772013/diff/14001/15001#newcode154
scripts/image_signing/common.sh:154: local filename is_success=$FLAGS_TRUE
do you mind making this independent of shflags?

[Hung-Te, 2010-10-16 01:14:53]

http://codereview.chromium.org/3772013/diff/6001/7002
File scripts/image_signing/make_dev_ssd.sh (right):

http://codereview.chromium.org/3772013/diff/6001/7002#newcode41
scripts/image_signing/make_dev_ssd.sh:41: err_die() {
OK, let's do this in some other CL if we decide to go for that. In the meantime
I'll remove all dependency of shflags from common.sh

http://codereview.chromium.org/3772013/diff/14001/15001
File scripts/image_signing/common.sh (right):

http://codereview.chromium.org/3772013/diff/14001/15001#newcode154
scripts/image_signing/common.sh:154: local filename is_success=$FLAGS_TRUE
On 2010/10/16 00:29:10, gauravsh wrote:
> do you mind making this independent of shflags? 

Done.

[gauravsh, 2010-10-16 01:19:52]

LGTM