Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6079004:
Preserves file system metadata between new build and latest shipping image. (Closed)
|
Created:
10 years ago by thieule Modified:
9 years, 4 months ago CC:
chromium-os-reviews_chromium.org, Randall Spangler, Luigi Semenzato, Bill Richardson, gauravsh Visibility:
Public. |
DescriptionPreserves file system metadata between new build and latest shipping image.
This script preserves the root file system metadata as much as possible
between the specified image and the latest shipping image. It preserves
the metadata by ensuring that the files reuse the same inodes and that they
are located at the same physical location on-disk. This leads to smaller
auto-update delta payload and less disk reshuffling, extending the life
of the SSD.
It is called before the image is signed during the stamping process.
Currently, this only supports x86-mario.
This is a continuation of a previous CL located at:
http://codereview.chromium.org/6058006/
BUG=chromium-os:10188
TEST=Build image, boot image, auto-update to new image, run suite_Smoke
Change-Id: I3270245dc15a074abb3bac250922c30e2e105f92
Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=ddc06e4
Patch Set 1 #
Total comments: 25
Patch Set 2 : Addressed code review feedbacks. #
Total comments: 3
Patch Set 3 : Removed sudo from clean up trap. #
Messages
Total messages: 12 (0 generated)
This is a continuation of http://codereview.chromium.org/6058006. PTAL
http://codereview.chromium.org/6079004/diff/1/scripts/image_signing/align_rootfs File scripts/image_signing/align_rootfs (right): http://codereview.chromium.org/6079004/diff/1/scripts/image_signing/align_roo... scripts/image_signing/align_rootfs:152: perform_latest_cleanup_action this is called twice here, but add_cleanup_action is called 4 times in this function http://codereview.chromium.org/6079004/diff/1/scripts/image_signing/common.sh File scripts/image_signing/common.sh (right): http://codereview.chromium.org/6079004/diff/1/scripts/image_signing/common.sh... scripts/image_signing/common.sh:55: V_VIDOFF='[m' should this be: \e[0m ?
Ping. On Thu, Dec 23, 2010 at 11:27 AM, <thieule@chromium.org> wrote: > Reviewers: adlr, petkov, gauravsh, > > Message: > This is a continuation of http://codereview.chromium.org/6058006. > > PTAL > > Description: > Preserves file system metadata between new build and latest shipping image. > > This script preserves the root file system metadata as much as possible > between the specified image and the latest shipping image. It preserves > the metadata by ensuring that the files reuse the same inodes and that they > are located at the same physical location on-disk. This leads to smaller > auto-update delta payload and less disk reshuffling, extending the life > of the SSD. > > It is called before the image is signed during the stamping process. > > Currently, this only supports x86-mario. > > This is a continuation of a previous CL located at: > http://codereview.chromium.org/6058006/ > > BUG=chromium-os:10188 > TEST=Build image, boot image, auto-update to new image, run suite_Smoke > > Change-Id: I3270245dc15a074abb3bac250922c30e2e105f92 > > Please review this at http://codereview.chromium.org/6079004/ > > SVN Base: http://git.chromium.org/git/vboot_reference.git@master > > Affected files: > A scripts/image_signing/align_rootfs > M scripts/image_signing/common.sh > > >
a few nits, lgtm otherwise. http://codereview.chromium.org/6079004/diff/1/scripts/image_signing/align_rootfs File scripts/image_signing/align_rootfs (right): http://codereview.chromium.org/6079004/diff/1/scripts/image_signing/align_roo... scripts/image_signing/align_rootfs:59: version="0.0.0.0" lang="en-US" track="dev-channel" you need to parametrize the channel at some point too to support beta-channel, right? http://codereview.chromium.org/6079004/diff/1/scripts/image_signing/align_roo... scripts/image_signing/align_rootfs:66: eval $1=$(wget -q --header="Content-Type: text/xml" \ i didn't even know you could do that... why don't you just print the version to stdout and then in the caller: version=$(get_latest_shipping_version) http://codereview.chromium.org/6079004/diff/1/scripts/image_signing/align_roo... scripts/image_signing/align_rootfs:67: --post-file="${au_request_file}" -O - ${auserver_url} | \ no need for \ at the end http://codereview.chromium.org/6079004/diff/1/scripts/image_signing/common.sh File scripts/image_signing/common.sh (right): http://codereview.chromium.org/6079004/diff/1/scripts/image_signing/common.sh... scripts/image_signing/common.sh:40: while [ $num_actions -gt 0 ]; do maybe propagate num_actions to avoid some code... while [ ${#cleanup_actions[*]} -gt 0 ]; do
http://codereview.chromium.org/6079004/diff/1/scripts/image_signing/align_rootfs File scripts/image_signing/align_rootfs (right): http://codereview.chromium.org/6079004/diff/1/scripts/image_signing/align_roo... scripts/image_signing/align_rootfs:1: #!/bin/bash slight nit: I know this is a bikeshedding issue and opinions vary but name this script as align_rootfs.sh to be consistent with the rest of the scripts in this directory. http://codereview.chromium.org/6079004/diff/1/scripts/image_signing/align_roo... scripts/image_signing/align_rootfs:3: # Copyright (c) 2010 The Chromium OS Authors. All rights reserved. 2011 http://codereview.chromium.org/6079004/diff/1/scripts/image_signing/align_roo... scripts/image_signing/align_rootfs:66: eval $1=$(wget -q --header="Content-Type: text/xml" \ On 2011/01/04 20:56:55, petkov wrote: > i didn't even know you could do that... why don't you just print the version to > stdout and then in the caller: > > version=$(get_latest_shipping_version) I would vote for something like that too - much cleaner IMO. http://codereview.chromium.org/6079004/diff/1/scripts/image_signing/align_roo... scripts/image_signing/align_rootfs:117: local src_root_fs_dir=$(mktemp -d "/tmp/align_root_fs_src_mount_dir.XXXX") just a suggestion: since you already including common.sh, you can use the tracked mounting (using make_temp_dir). This may not necessarily be what you want though, for example, if you'd like to control when the clean up happens (like you do at the end of this function). http://codereview.chromium.org/6079004/diff/1/scripts/image_signing/align_roo... scripts/image_signing/align_rootfs:175: trap cleanup INT TERM EXIT the trap also gets set in common.sh which gets overriden here - you could just change the line in common.sh to set the trap action for TERM and EXIT as well and get rid of this line here. This should be ok behavior for other scripts too. http://codereview.chromium.org/6079004/diff/1/scripts/image_signing/align_roo... scripts/image_signing/align_rootfs:186: if [ -z "${FLAGS_hardware_id}" ]; then since now you have a default value for the hardware id, you don't need this check. http://codereview.chromium.org/6079004/diff/1/scripts/image_signing/align_roo... scripts/image_signing/align_rootfs:238: trap - EXIT do you need these 2 lines - cleanup should happen automatically on exit, no?
I've addressed the code review feedbacks. PTAL http://codereview.chromium.org/6079004/diff/1/scripts/image_signing/align_rootfs File scripts/image_signing/align_rootfs (right): http://codereview.chromium.org/6079004/diff/1/scripts/image_signing/align_roo... scripts/image_signing/align_rootfs:1: #!/bin/bash On 2011/01/04 21:35:29, gauravsh wrote: > slight nit: I know this is a bikeshedding issue and opinions vary but name this > script as align_rootfs.sh to be consistent with the rest of the scripts in this > directory. Done. http://codereview.chromium.org/6079004/diff/1/scripts/image_signing/align_roo... scripts/image_signing/align_rootfs:3: # Copyright (c) 2010 The Chromium OS Authors. All rights reserved. On 2011/01/04 21:35:29, gauravsh wrote: > 2011 Done. http://codereview.chromium.org/6079004/diff/1/scripts/image_signing/align_roo... scripts/image_signing/align_rootfs:59: version="0.0.0.0" lang="en-US" track="dev-channel" On 2011/01/04 20:56:55, petkov wrote: > you need to parametrize the channel at some point too to support beta-channel, > right? Done. http://codereview.chromium.org/6079004/diff/1/scripts/image_signing/align_roo... scripts/image_signing/align_rootfs:66: eval $1=$(wget -q --header="Content-Type: text/xml" \ On 2011/01/04 20:56:55, petkov wrote: > i didn't even know you could do that... why don't you just print the version to > stdout and then in the caller: > > version=$(get_latest_shipping_version) Done. http://codereview.chromium.org/6079004/diff/1/scripts/image_signing/align_roo... scripts/image_signing/align_rootfs:67: --post-file="${au_request_file}" -O - ${auserver_url} | \ On 2011/01/04 20:56:55, petkov wrote: > no need for \ at the end > Done. http://codereview.chromium.org/6079004/diff/1/scripts/image_signing/align_roo... scripts/image_signing/align_rootfs:117: local src_root_fs_dir=$(mktemp -d "/tmp/align_root_fs_src_mount_dir.XXXX") On 2011/01/04 21:35:29, gauravsh wrote: > just a suggestion: since you already including common.sh, you can use the > tracked mounting (using make_temp_dir). > > This may not necessarily be what you want though, for example, if you'd like to > control when the clean up happens (like you do at the end of this function). > Unfortunately, in this case, I need to be able to dismount these file systems before exiting this function because I'll need to replace the rootfs later. Will keep in mind for future edits. http://codereview.chromium.org/6079004/diff/1/scripts/image_signing/align_roo... scripts/image_signing/align_rootfs:152: perform_latest_cleanup_action On 2011/01/04 19:04:48, adlr wrote: > this is called twice here, but add_cleanup_action is called 4 times in this > function Done. http://codereview.chromium.org/6079004/diff/1/scripts/image_signing/align_roo... scripts/image_signing/align_rootfs:175: trap cleanup INT TERM EXIT On 2011/01/04 21:35:29, gauravsh wrote: > the trap also gets set in common.sh which gets overriden here - you could just > change the line in common.sh to set the trap action for TERM and EXIT as well > and get rid of this line here. This should be ok behavior for other scripts too. Done. http://codereview.chromium.org/6079004/diff/1/scripts/image_signing/align_roo... scripts/image_signing/align_rootfs:186: if [ -z "${FLAGS_hardware_id}" ]; then On 2011/01/04 21:35:29, gauravsh wrote: > since now you have a default value for the hardware id, you don't need this > check. Done. http://codereview.chromium.org/6079004/diff/1/scripts/image_signing/align_roo... scripts/image_signing/align_rootfs:238: trap - EXIT On 2011/01/04 21:35:29, gauravsh wrote: > do you need these 2 lines - cleanup should happen automatically on exit, no? Done. http://codereview.chromium.org/6079004/diff/1/scripts/image_signing/common.sh File scripts/image_signing/common.sh (right): http://codereview.chromium.org/6079004/diff/1/scripts/image_signing/common.sh... scripts/image_signing/common.sh:40: while [ $num_actions -gt 0 ]; do On 2011/01/04 20:56:55, petkov wrote: > maybe propagate num_actions to avoid some code... > > while [ ${#cleanup_actions[*]} -gt 0 ]; do Done. http://codereview.chromium.org/6079004/diff/1/scripts/image_signing/common.sh... scripts/image_signing/common.sh:55: V_VIDOFF='[m' On 2011/01/04 19:04:48, adlr wrote: > should this be: > \e[0m > ? Done.
LGTM if others are happy
On 2011/01/04 23:59:39, adlr wrote: > LGTM if others are happy lgtm too
LGTM http://codereview.chromium.org/6079004/diff/13001/scripts/image_signing/align... File scripts/image_signing/align_rootfs.sh (right): http://codereview.chromium.org/6079004/diff/13001/scripts/image_signing/align... scripts/image_signing/align_rootfs.sh:54: trap "sudo rm -f \"${au_request_file}\"" INT TERM EXIT no need for sudo? (this trap seems evil...)
Thanks all :) http://codereview.chromium.org/6079004/diff/13001/scripts/image_signing/align... File scripts/image_signing/align_rootfs.sh (right): http://codereview.chromium.org/6079004/diff/13001/scripts/image_signing/align... scripts/image_signing/align_rootfs.sh:54: trap "sudo rm -f \"${au_request_file}\"" INT TERM EXIT On 2011/01/05 00:08:50, petkov wrote: > no need for sudo? > > (this trap seems evil...) Done.
http://codereview.chromium.org/6079004/diff/13001/scripts/image_signing/align... File scripts/image_signing/align_rootfs.sh (right): http://codereview.chromium.org/6079004/diff/13001/scripts/image_signing/align... scripts/image_signing/align_rootfs.sh:54: trap "sudo rm -f \"${au_request_file}\"" INT TERM EXIT On 2011/01/05 00:11:44, thieule wrote: > On 2011/01/05 00:08:50, petkov wrote: > > no need for sudo? > > > > (this trap seems evil...) > > Done. Wait, won't this ending up clobbering the previous trap you set up for performing cleanup?
Since it's a process substitution, a subprocess gets launched and the trap only applies to that subprocess. On Tue, Jan 4, 2011 at 4:15 PM, <gauravsh@chromium.org> wrote: > > > http://codereview.chromium.org/6079004/diff/13001/scripts/image_signing/align... > File scripts/image_signing/align_rootfs.sh (right): > > > http://codereview.chromium.org/6079004/diff/13001/scripts/image_signing/align... > scripts/image_signing/align_rootfs.sh:54: trap "sudo rm -f > \"${au_request_file}\"" INT TERM EXIT > On 2011/01/05 00:11:44, thieule wrote: > >> On 2011/01/05 00:08:50, petkov wrote: >> > no need for sudo? >> > >> > (this trap seems evil...) >> > > Done. >> > > Wait, won't this ending up clobbering the previous trap you set up for > performing cleanup? > > > http://codereview.chromium.org/6079004/ > |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
