mod_image_for_recovery: keep the stateful partition around.

mod_image_for_recovery: keep the stateful partition around.

Developer images may have data on stateful they want to keep. In that case, just swap the kernels.

BUG=chromium-os:7451
TEST=tested with a --withdev build and stateful was intact and the recovery install worked.

Change-Id: I59d7f2e99892448d1eaf964e6292611accb2035c

[Will Drewry, 2010-11-04 00:23:50]

Fix reviewers. :/

[rochberg, 2010-11-04 00:43:20]

http://codereview.chromium.org/4418002/diff/3001/4001
File mod_image_for_recovery.sh (right):

http://codereview.chromium.org/4418002/diff/3001/4001#newcode283
mod_image_for_recovery.sh:283: if [ $FLAGS_preserve_stateful -eq $FLAGS_TRUE ];
then
I don't have much context here, but it seems like copying the entire original
image potentially has interesting side effects beyond just preserving the
stateful partition

[Will Drewry, 2010-11-04 01:49:26]

+hungte for FYI and comments :)

http://codereview.chromium.org/4418002/diff/3001/4001
File mod_image_for_recovery.sh (right):

http://codereview.chromium.org/4418002/diff/3001/4001#newcode283
mod_image_for_recovery.sh:283: if [ $FLAGS_preserve_stateful -eq $FLAGS_TRUE ];
then
On 2010/11/04 00:43:21, rochberg wrote:
> I don't have much context here, but it seems like copying the entire original
> image potentially has interesting side effects beyond just preserving the
> stateful partition


True - it means that if the image has more than just ROOT-A and KERN-A and
STATE, it will preserve that data too.

Perhaps this should simply be: --[no]minimize_image  .  Then builders can mod a
normal image for recovery but for dev and test images we pass --nominimize_image
and keep stateful and any other hacks.

Seem sane?

[Hung-Te, 2010-11-04 10:40:20]

seems fine to me.

[Will Drewry, 2010-11-04 14:59:21]

Thanks - any other thoughts?

David - if you're not comfortable LGTM'ing it, I can add someone else
who's read this script.
(And I'll certainly do a full test before push with a test image.)

On Thu, Nov 4, 2010 at 5:40 AM,  <hungte@chromium.org> wrote:
> seems fine to me.
>
> http://codereview.chromium.org/4418002/show
>

[rochberg_internal, 2010-11-04 15:24:26]

LGTM pending my comment, which you can choose to ignore.

http://codereview.chromium.org/4418002/diff/9001/10001
File mod_image_for_recovery.sh (right):

http://codereview.chromium.org/4418002/diff/9001/10001#newcode46
mod_image_for_recovery.sh:46: "Decides if the original image is used or a
minimal recovery image is \
DEFINE_boolean make_fresh_image $FLAGS_FALSE \
   "Build a fresh empty image.  If false, modifies the supplied image instead"


??

[Will Drewry, 2010-11-04 15:28:43]

Cool -- it'd save on disk space :) I think that'd be good in addition
to minimize_image.  Since it means we can swap the kernels in place,
copy the image and swap the kernels, or create a wholly new smaller
recovery image.

I'll add it and repost - thanks!


On Thu, Nov 4, 2010 at 10:24 AM,  <rochberg@google.com> wrote:
> LGTM pending my comment, which you can choose to ignore.
>
>
>
>
> http://codereview.chromium.org/4418002/diff/9001/10001
> File mod_image_for_recovery.sh (right):
>
> http://codereview.chromium.org/4418002/diff/9001/10001#newcode46
> mod_image_for_recovery.sh:46: "Decides if the original image is used or
> a minimal recovery image is \
> DEFINE_boolean make_fresh_image $FLAGS_FALSE \
>   "Build a fresh empty image.  If false, modifies the supplied image
> instead"
>
>
> ??
>
> http://codereview.chromium.org/4418002/show
>

[Will Drewry, 2010-11-04 19:09:12]

Added along with some verbose logging.  I also tested it and it worked fine with
a test image.

PTAL!

[sosa, 2010-11-05 03:12:04]

LGTM on new diff with two nits

http://codereview.chromium.org/4418002/diff/17001/18001
File mod_image_for_recovery.sh (right):

http://codereview.chromium.org/4418002/diff/17001/18001#newcode71
mod_image_for_recovery.sh:71: exec 2>/dev/null
Add a comment before die explaining you're redirecting it's output to stdout
rather than stderr to avoid the exec 2>/dev/null

http://codereview.chromium.org/4418002/diff/17001/18001#newcode345
mod_image_for_recovery.sh:345: if [ $FLAGS_image != $RECOVERY_IMAGE ]; then
quotes on both

[Will Drewry, 2010-11-05 03:16:06]

Done - thanks!

http://codereview.chromium.org/4418002/diff/17001/18001
File mod_image_for_recovery.sh (right):

http://codereview.chromium.org/4418002/diff/17001/18001#newcode71
mod_image_for_recovery.sh:71: exec 2>/dev/null
On 2010/11/05 03:12:09, sosa wrote:
> Add a comment before die explaining you're redirecting it's output to stdout
> rather than stderr to avoid the exec 2>/dev/null

Done.

http://codereview.chromium.org/4418002/diff/17001/18001#newcode345
mod_image_for_recovery.sh:345: if [ $FLAGS_image != $RECOVERY_IMAGE ]; then
On 2010/11/05 03:12:09, sosa wrote:
> quotes on both

Done.