cros_make_image_bootable: Do a final sanity check on the rootfs by running e2fsck

cros_make_image_bootable: Do a final sanity check on the rootfs by running e2fsck 

This is a complement to the proposed fix for http://crosbug.com/9447, and I am getting it out first since it is a relatively simple change.

This change ensures that the rootfs on the image output by cros_make_image_bootable wasn't corrupted accidentally (for example, no boot.desc was provided and the defaults were incorrect for the image). This should catch rootfs errors that we end up discovering at delta AU time.

BUG=chromium-os:9578
TEST= Ran cros_make_image_bootable manually on an existing image with and without boot.desc. As expected, without boot.desc the output image rootfs had errors.

Also ran ./build_image - which worked fine too.

Change-Id: I4ed3a56634f37ab7d5ff2dc052ad607740356a40

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

[sosa, 2010-11-20 02:22:00]

What happens if you take a prebuilt image from someone else's machine
and re-run cros_make_image_bootable?

On Fri, Nov 19, 2010 at 5:59 PM,  <gauravsh@chromium.org> wrote:
> Reviewers: Will Drewry, sosa,
>
> Description:
> Abort if boot.desc is now found.
>
> The command line parsing heuristics almost never work properly leading to a
> subtly corrupted image. This change makes the script abort if it can't read
> the
> boot parameters from the corresponding boot.desc file.
>
> BUG=chromium-os:9447
> TEST=Remove boot.desc file from image folder, run cros_make_image_bootable,
> script will abort. Also verified that build_image for an unmodified ToT
> checkout
> still works.
>
> Change-Id: I4ed3a56634f37ab7d5ff2dc052ad607740356a40
>
> Please review this at http://codereview.chromium.org/5216003/
>
> SVN Base: ssh://git@gitrw.chromium.org:9222/crosutils.git@master
>
> Affected files:
>  M bin/cros_make_image_bootable
>
>
> Index: bin/cros_make_image_bootable
> diff --git a/bin/cros_make_image_bootable b/bin/cros_make_image_bootable
> index
>
3517c85ec8cdbd38143b2784d3652815fd1977e2..9cec51a66347d830e61b9d6cdd299752aada3256
> 100755
> --- a/bin/cros_make_image_bootable
> +++ b/bin/cros_make_image_bootable
> @@ -36,9 +36,7 @@ shift
>  FLAG_OVERRIDES="${@}"
>
>  if [ ! -r "${BOOT_DESC_FILE}" ]; then
> -  warn "${BOOT_DESC_FILE} cannot be read!"
> -  warn "Falling back to command line parsing"
> -  BOOT_DESC="${@}"
> +  die "${BOOT_DESC_FILE} cannot be read!"
>  else
>   BOOT_DESC="$(cat ${BOOT_DESC_FILE} | tr -s '\n' ' ')"
>   info "Boot-time configuration for $(dirname ${IMAGE}): "
>
>
>

[gauravsh, 2010-11-20 02:29:31]

On Fri, Nov 19, 2010 at 6:21 PM, Chris Sosa <sosa@chromium.org> wrote:

> What happens if you take a prebuilt image from someone else's machine
> and re-run cros_make_image_bootable?
>

You need to have the boot.desc or the script will abort. The problem is that
without boot.desc, cros_make_image_bootable always ends up doing the wrong
thing (rootfs corruption for mario images, unbootable L13 images).

So, in the current state the script should abort instead of doing the wrong
thing and failing silently. Ideally, we should probably have boot.desc be
apart of the image (maybe under /boot in the rootfs) and
cros_make_image_bootable should just grab it from there. If that sounds like
a reasonable option to you, file a bug and assign it to me - I can have a
fix in a separate CL.


> On Fri, Nov 19, 2010 at 5:59 PM,  <gauravsh@chromium.org> wrote:
> > Reviewers: Will Drewry, sosa,
> >
> > Description:
> > Abort if boot.desc is now found.
> >
> > The command line parsing heuristics almost never work properly leading to
> a
> > subtly corrupted image. This change makes the script abort if it can't
> read
> > the
> > boot parameters from the corresponding boot.desc file.
> >
> > BUG=chromium-os:9447
> > TEST=Remove boot.desc file from image folder, run
> cros_make_image_bootable,
> > script will abort. Also verified that build_image for an unmodified ToT
> > checkout
> > still works.
> >
> > Change-Id: I4ed3a56634f37ab7d5ff2dc052ad607740356a40
> >
> > Please review this at http://codereview.chromium.org/5216003/
> >
> > SVN Base: ssh://git@gitrw.chromium.org:9222/crosutils.git@master
> >
> > Affected files:
> >  M bin/cros_make_image_bootable
> >
> >
> > Index: bin/cros_make_image_bootable
> > diff --git a/bin/cros_make_image_bootable b/bin/cros_make_image_bootable
> > index
> >
>
3517c85ec8cdbd38143b2784d3652815fd1977e2..9cec51a66347d830e61b9d6cdd299752aada3256
> > 100755
> > --- a/bin/cros_make_image_bootable
> > +++ b/bin/cros_make_image_bootable
> > @@ -36,9 +36,7 @@ shift
> >  FLAG_OVERRIDES="${@}"
> >
> >  if [ ! -r "${BOOT_DESC_FILE}" ]; then
> > -  warn "${BOOT_DESC_FILE} cannot be read!"
> > -  warn "Falling back to command line parsing"
> > -  BOOT_DESC="${@}"
> > +  die "${BOOT_DESC_FILE} cannot be read!"
> >  else
> >   BOOT_DESC="$(cat ${BOOT_DESC_FILE} | tr -s '\n' ' ')"
> >   info "Boot-time configuration for $(dirname ${IMAGE}): "
> >
> >
> >
>



-- 
-g

[gauravsh, 2010-11-23 21:05:12]

ping?

[sosa, 2010-11-23 21:13:31]

Sorry about letting this drop over the weekend.

Anyway, I'm still confused why we need this because I'm not convinced that this
won't break the dev server work flow.  I've often taken builds without the
boot.desc ... changed some stuff on them ... and re-run
cros_make_image_bootable.  Won't this change break that workflow?

[sosa, 2010-11-24 18:16:15]

Nits

http://codereview.chromium.org/5216003/diff/7001/bin/cros_make_image_bootable
File bin/cros_make_image_bootable (right):

http://codereview.chromium.org/5216003/diff/7001/bin/cros_make_image_bootable...
bin/cros_make_image_bootable:259: trap "rm $rootfs_tmp_file" EXIT
Use {} around var names to be consistent with rest of script

http://codereview.chromium.org/5216003/diff/7001/bin/cros_make_image_bootable...
bin/cros_make_image_bootable:262: # This flips the read-inly compatibility flag,
so that
read-only*

http://codereview.chromium.org/5216003/diff/7001/bin/cros_make_image_bootable...
bin/cros_make_image_bootable:267: die "Root file system has errors, please
ensure boot.desc and/or \
Indent 2 here

[gauravsh, 2010-11-24 19:40:17]

http://codereview.chromium.org/5216003/diff/7001/bin/cros_make_image_bootable
File bin/cros_make_image_bootable (right):

http://codereview.chromium.org/5216003/diff/7001/bin/cros_make_image_bootable...
bin/cros_make_image_bootable:259: trap "rm $rootfs_tmp_file" EXIT
On 2010/11/24 18:16:15, sosa wrote:
> Use {} around var names to be consistent with rest of script

Done.

http://codereview.chromium.org/5216003/diff/7001/bin/cros_make_image_bootable...
bin/cros_make_image_bootable:262: # This flips the read-inly compatibility flag,
so that
On 2010/11/24 18:16:15, sosa wrote:
> read-only*

Done.

http://codereview.chromium.org/5216003/diff/7001/bin/cros_make_image_bootable...
bin/cros_make_image_bootable:267: die "Root file system has errors, please
ensure boot.desc and/or \
On 2010/11/24 18:16:15, sosa wrote:
> Indent 2 here

Done.

[sosa, 2010-11-24 19:58:07]

LGTM

[petkov, 2010-11-24 20:08:23]

i think you have a typo. lgtm otherwise.

http://codereview.chromium.org/5216003/diff/16001/bin/cros_make_image_bootable
File bin/cros_make_image_bootable (right):

http://codereview.chromium.org/5216003/diff/16001/bin/cros_make_image_bootabl...
bin/cros_make_image_bootable:259: trap "rm ${rootfs_tmp_file|" EXIT
what's with the |?

[gauravsh, 2010-11-24 20:40:05]

On Wed, Nov 24, 2010 at 12:08 PM, <petkov@chromium.org> wrote:

> i think you have a typo. lgtm otherwise.
>

Yeah, discovered 2 other typos while testing the change again. Uploaded a
new patchset after testing.


>
>
>
>
> http://codereview.chromium.org/5216003/diff/16001/bin/cros_make_image_bootable
>
> File bin/cros_make_image_bootable (right):
>
>
>
http://codereview.chromium.org/5216003/diff/16001/bin/cros_make_image_bootabl...
>
> bin/cros_make_image_bootable:259: trap "rm ${rootfs_tmp_file|" EXIT
> what's with the |?
>
>
> http://codereview.chromium.org/5216003/
>



-- 
-g

[sosa, 2010-11-24 20:42:17]

LGTM w/ nits

http://codereview.chromium.org/5216003/diff/20001/bin/cros_make_image_bootable
File bin/cros_make_image_bootable (right):

http://codereview.chromium.org/5216003/diff/20001/bin/cros_make_image_bootabl...
bin/cros_make_image_bootable:264: enable_rw_mount ${rootfs_tmp_file}
Want quotes around this since you put it elsewhere for same var

http://codereview.chromium.org/5216003/diff/20001/bin/cros_make_image_bootabl...
bin/cros_make_image_bootable:286: make_image_bootable ${IMAGE}
Note sure why calls to both of these aren't quoted

[gauravsh, 2010-11-24 20:46:46]

http://codereview.chromium.org/5216003/diff/20001/bin/cros_make_image_bootable
File bin/cros_make_image_bootable (right):

http://codereview.chromium.org/5216003/diff/20001/bin/cros_make_image_bootabl...
bin/cros_make_image_bootable:264: enable_rw_mount ${rootfs_tmp_file}
On 2010/11/24 20:42:17, sosa wrote:
> Want quotes around this since you put it elsewhere for same var

Done.

http://codereview.chromium.org/5216003/diff/20001/bin/cros_make_image_bootabl...
bin/cros_make_image_bootable:286: make_image_bootable ${IMAGE}
On 2010/11/24 20:42:17, sosa wrote:
> Note sure why calls to both of these aren't quoted

Done.

[gauravsh, 2010-11-24 20:47:19]

On 2010/11/24 20:46:46, gauravsh wrote:
> http://codereview.chromium.org/5216003/diff/20001/bin/cros_make_image_bootable
> File bin/cros_make_image_bootable (right):
> 
>
http://codereview.chromium.org/5216003/diff/20001/bin/cros_make_image_bootabl...
> bin/cros_make_image_bootable:264: enable_rw_mount ${rootfs_tmp_file}
> On 2010/11/24 20:42:17, sosa wrote:
> > Want quotes around this since you put it elsewhere for same var
> 
> Done.
> 
>
http://codereview.chromium.org/5216003/diff/20001/bin/cros_make_image_bootabl...
> bin/cros_make_image_bootable:286: make_image_bootable ${IMAGE}
> On 2010/11/24 20:42:17, sosa wrote:
> > Note sure why calls to both of these aren't quoted
> 
> Done.

Pushing...