Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6246037:
Add sanity test for /etc/lsb-release file in CrOS images (Closed)
|
Created:
9 years, 10 months ago by jimhebert Modified:
9 years, 7 months ago CC:
chromium-os-reviews_chromium.org, Randall Spangler, gauravsh, Luigi Semenzato, Bill Richardson, Sumit Visibility:
Public. |
DescriptionAdd sanity test for /etc/lsb-release file in CrOS images
Change-Id: Ib8061ba35afd9681dc70fe1a1459ff9a00f74c3f
BUG=chrome-os-partner:2181
TEST=./ensure_sane_lsb-release.sh chromiumos_base_image.bin (passes)
Also tested each of the "test fail" possibilities:
* Tested by changing various values in the .config file to force each mismatch/failure
* Tested lsb_syntaxcheck by isolating it and having it check a mock bad lsb-release file
containing each of the possible syntax violations.
Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=6b2b81c
Patch Set 1 #
Total comments: 30
Patch Set 2 : Hopefully address all-except-indentation-related items #Patch Set 3 : Fix indentation/style #
Total comments: 2
Patch Set 4 : +gauravsh's suggested default config filename #Messages
Total messages: 6 (0 generated)
Thanks in advance for your review!
http://codereview.chromium.org/6246037/diff/1/scripts/image_signing/ensure_sa... File scripts/image_signing/ensure_sane_lsb-release.sh (right): http://codereview.chromium.org/6246037/diff/1/scripts/image_signing/ensure_sa... scripts/image_signing/ensure_sane_lsb-release.sh:123: extra newline http://codereview.chromium.org/6246037/diff/1/scripts/image_signing/ensure_sa... scripts/image_signing/ensure_sane_lsb-release.sh:133: for b in ${expected_boards[@]}; do shouldn't the board check happen before the eval on line 122, (in which case the app id check should also be skipped)? http://codereview.chromium.org/6246037/diff/1/scripts/image_signing/ensure_sa... scripts/image_signing/ensure_sane_lsb-release.sh:148: track_recognized=1 these 2 checks for CHROMEOS_RELEASE_BOARD and CHROMEOS_RELEASE_TRACK basically use the same code, can you re-factor them into a single function? (something like - check_keyval_in_list <lsbfile> <key> <listname>)
thanks much for adding this test. comments below. http://codereview.chromium.org/6246037/diff/1/scripts/image_signing/ensure_sa... File scripts/image_signing/ensure_sane_lsb-release.sh (right): http://codereview.chromium.org/6246037/diff/1/scripts/image_signing/ensure_sa... scripts/image_signing/ensure_sane_lsb-release.sh:16: echo "Usage $PROG image [config]" why 4-space indent? i think our style is 2 space. http://codereview.chromium.org/6246037/diff/1/scripts/image_signing/ensure_sa... scripts/image_signing/ensure_sane_lsb-release.sh:26: remove extra blank line http://codereview.chromium.org/6246037/diff/1/scripts/image_signing/ensure_sa... scripts/image_signing/ensure_sane_lsb-release.sh:59: badlines=$(grep -E -v '^[A-Z][A-Z_]*=[[:graph:]][[:print:]]*' "$lsbfile" |\ no need for trailing \ you could do this in a single grep, I think: ^[A-Z][A-Z_]*=([[:graph:]][[:print:]]*)?$ http://codereview.chromium.org/6246037/diff/1/scripts/image_signing/ensure_sa... scripts/image_signing/ensure_sane_lsb-release.sh:70: badlines=$(grep -E '^.{255}.{2}' "$lsbfile") stick with 255 for simplicity? :) http://codereview.chromium.org/6246037/diff/1/scripts/image_signing/ensure_sa... scripts/image_signing/ensure_sane_lsb-release.sh:85: remove extra blank line http://codereview.chromium.org/6246037/diff/1/scripts/image_signing/ensure_sa... scripts/image_signing/ensure_sane_lsb-release.sh:107: . "$configfile" you may want to print an info message showing which config file you're using. http://codereview.chromium.org/6246037/diff/1/scripts/image_signing/ensure_sa... scripts/image_signing/ensure_sane_lsb-release.sh:110: mount_image_partition_ro "$image" 3 "$rootfs" on error, nothing gets unmounted? and the temp dir remains? http://codereview.chromium.org/6246037/diff/1/scripts/image_signing/ensure_sa... scripts/image_signing/ensure_sane_lsb-release.sh:118: local board=$(lsbval $lsb CHROMEOS_RELEASE_BOARD | \ no need for trailing \ after | I think. also, I think style is each pipe on a separate line, i.e., cmd 1 | cmd 2 | cmd 3 http://codereview.chromium.org/6246037/diff/1/scripts/image_signing/ensure_sa... scripts/image_signing/ensure_sane_lsb-release.sh:119: cut -d = -f 2 | cut -d - -f 1,2) this may be a bit flaky but i don't have much better suggestions. i don't think there's a rule that the basic board name is id-id. somebody may come up with x86-mario-v2, for example. http://codereview.chromium.org/6246037/diff/1/scripts/image_signing/ensure_sa... scripts/image_signing/ensure_sane_lsb-release.sh:121: local boardvar=${board//-/_} i see why you're doing this but it's a bit flaky. it's ok as is, however, is there any way we can convert expected values to a proper map so that there's no need for the key to be a real identifier? maybe through an array indexed by looking up the board index in the expected boards array? http://codereview.chromium.org/6246037/diff/1/scripts/image_signing/ensure_sa... scripts/image_signing/ensure_sane_lsb-release.sh:158: main $@ add blank line before main http://codereview.chromium.org/6246037/diff/1/scripts/image_signing/ensure_sa... scripts/image_signing/ensure_sane_lsb-release.sh:158: main $@ "$@"
Agree with all the great feedback, thanks! New rev uploaded. See below. http://codereview.chromium.org/6246037/diff/1/scripts/image_signing/ensure_sa... File scripts/image_signing/ensure_sane_lsb-release.sh (right): http://codereview.chromium.org/6246037/diff/1/scripts/image_signing/ensure_sa... scripts/image_signing/ensure_sane_lsb-release.sh:16: echo "Usage $PROG image [config]" On 2011/02/02 15:57:12, petkov wrote: > why 4-space indent? i think our style is 2 space. Ack. I started out by copying the style from ensure_no_password.sh, guess I shoulda read the style guide instead. I'll submit an indentation fix by itself as the last patchkit to this CL review, but in the meantime to keep the diffs-between-patchkits readable I'll do a round of fixes for all of the other issues pointed out, maintaining the current indentation level. (Hope that makes sense: patchkit 2 fix all the issues, you get to read the diffs, then patchkit 3 fix indentation.) http://codereview.chromium.org/6246037/diff/1/scripts/image_signing/ensure_sa... scripts/image_signing/ensure_sane_lsb-release.sh:26: On 2011/02/02 15:57:12, petkov wrote: > remove extra blank line Done http://codereview.chromium.org/6246037/diff/1/scripts/image_signing/ensure_sa... scripts/image_signing/ensure_sane_lsb-release.sh:59: badlines=$(grep -E -v '^[A-Z][A-Z_]*=[[:graph:]][[:print:]]*' "$lsbfile" |\ On 2011/02/02 15:57:12, petkov wrote: > no need for trailing \ > > you could do this in a single grep, I think: > > ^[A-Z][A-Z_]*=([[:graph:]][[:print:]]*)?$ Done http://codereview.chromium.org/6246037/diff/1/scripts/image_signing/ensure_sa... scripts/image_signing/ensure_sane_lsb-release.sh:70: badlines=$(grep -E '^.{255}.{2}' "$lsbfile") On 2011/02/02 15:57:12, petkov wrote: > stick with 255 for simplicity? :) Done http://codereview.chromium.org/6246037/diff/1/scripts/image_signing/ensure_sa... scripts/image_signing/ensure_sane_lsb-release.sh:85: On 2011/02/02 15:57:12, petkov wrote: > remove extra blank line Done http://codereview.chromium.org/6246037/diff/1/scripts/image_signing/ensure_sa... scripts/image_signing/ensure_sane_lsb-release.sh:107: . "$configfile" On 2011/02/02 15:57:12, petkov wrote: > you may want to print an info message showing which config file you're using. Done http://codereview.chromium.org/6246037/diff/1/scripts/image_signing/ensure_sa... scripts/image_signing/ensure_sane_lsb-release.sh:110: mount_image_partition_ro "$image" 3 "$rootfs" On 2011/02/02 15:57:12, petkov wrote: > on error, nothing gets unmounted? and the temp dir remains? From my reading of common_minimal.sh, this is all handled automatically? Did I misunderstand? (trap "cleanup_temps_and_mounts" EXIT) http://codereview.chromium.org/6246037/diff/1/scripts/image_signing/ensure_sa... scripts/image_signing/ensure_sane_lsb-release.sh:118: local board=$(lsbval $lsb CHROMEOS_RELEASE_BOARD | \ On 2011/02/02 15:57:12, petkov wrote: > no need for trailing \ after | I think. also, I think style is each pipe on a > separate line, i.e., > > cmd 1 | > cmd 2 | > cmd 3 Done. http://codereview.chromium.org/6246037/diff/1/scripts/image_signing/ensure_sa... scripts/image_signing/ensure_sane_lsb-release.sh:121: local boardvar=${board//-/_} On 2011/02/02 15:57:12, petkov wrote: > i see why you're doing this but it's a bit flaky. it's ok as is, however, is > there any way we can convert expected values to a proper map so that there's no > need for the key to be a real identifier? maybe through an array indexed by > looking up the board index in the expected boards array? I agree with the sentiments and had the same thoughts when writing it. For me it came down to "well, if I want it that sophisticated, I ought to be writing this in python where I have dictionary types and such." ;-) With the limited data structures available, looking up the board offset in 1 array, then looking up an appid at the same offset in another array would be possible, though I fear it could become maintainer-unfriendly as the number of boards grows and things could get mis-correlated if some item gets added to the wrong place in the array. Hopefully, having moved the board-check to being above the eval, I've addressed most of the concern about flake - this should only ever eval to a small whitelist of things now. http://codereview.chromium.org/6246037/diff/1/scripts/image_signing/ensure_sa... scripts/image_signing/ensure_sane_lsb-release.sh:123: On 2011/02/02 01:33:42, gauravsh wrote: > extra newline Done http://codereview.chromium.org/6246037/diff/1/scripts/image_signing/ensure_sa... scripts/image_signing/ensure_sane_lsb-release.sh:133: for b in ${expected_boards[@]}; do On 2011/02/02 01:33:42, gauravsh wrote: > shouldn't the board check happen before the eval on line 122, (in which case the > app id check should also be skipped)? Done. http://codereview.chromium.org/6246037/diff/1/scripts/image_signing/ensure_sa... scripts/image_signing/ensure_sane_lsb-release.sh:148: track_recognized=1 On 2011/02/02 01:33:42, gauravsh wrote: > these 2 checks for CHROMEOS_RELEASE_BOARD and CHROMEOS_RELEASE_TRACK basically > use the same code, can you re-factor them into a single function? (something > like - check_keyval_in_list <lsbfile> <key> <listname>) Done. http://codereview.chromium.org/6246037/diff/1/scripts/image_signing/ensure_sa... scripts/image_signing/ensure_sane_lsb-release.sh:158: main $@ On 2011/02/02 15:57:12, petkov wrote: > "$@" Done http://codereview.chromium.org/6246037/diff/1/scripts/image_signing/ensure_sa... scripts/image_signing/ensure_sane_lsb-release.sh:158: main $@ On 2011/02/02 15:57:12, petkov wrote: > add blank line before main Done
... and now with indentation fixed. PTAL. On 2011/02/02 23:51:35, jimhebert wrote: > Agree with all the great feedback, thanks! New rev uploaded. See below. > > http://codereview.chromium.org/6246037/diff/1/scripts/image_signing/ensure_sa... > File scripts/image_signing/ensure_sane_lsb-release.sh (right): > > http://codereview.chromium.org/6246037/diff/1/scripts/image_signing/ensure_sa... > scripts/image_signing/ensure_sane_lsb-release.sh:16: echo "Usage $PROG image > [config]" > On 2011/02/02 15:57:12, petkov wrote: > > why 4-space indent? i think our style is 2 space. > > Ack. I started out by copying the style from ensure_no_password.sh, guess I > shoulda read the style guide instead. > > I'll submit an indentation fix by itself as the last patchkit to this CL review, > but in the meantime to keep the diffs-between-patchkits readable I'll do a round > of fixes for all of the other issues pointed out, maintaining the current > indentation level. (Hope that makes sense: patchkit 2 fix all the issues, you > get to read the diffs, then patchkit 3 fix indentation.) > > http://codereview.chromium.org/6246037/diff/1/scripts/image_signing/ensure_sa... > scripts/image_signing/ensure_sane_lsb-release.sh:26: > On 2011/02/02 15:57:12, petkov wrote: > > remove extra blank line > > Done > > http://codereview.chromium.org/6246037/diff/1/scripts/image_signing/ensure_sa... > scripts/image_signing/ensure_sane_lsb-release.sh:59: badlines=$(grep -E -v > '^[A-Z][A-Z_]*=[[:graph:]][[:print:]]*' "$lsbfile" |\ > On 2011/02/02 15:57:12, petkov wrote: > > no need for trailing \ > > > > you could do this in a single grep, I think: > > > > ^[A-Z][A-Z_]*=([[:graph:]][[:print:]]*)?$ > > Done > > http://codereview.chromium.org/6246037/diff/1/scripts/image_signing/ensure_sa... > scripts/image_signing/ensure_sane_lsb-release.sh:70: badlines=$(grep -E > '^.{255}.{2}' "$lsbfile") > On 2011/02/02 15:57:12, petkov wrote: > > stick with 255 for simplicity? :) > > Done > > http://codereview.chromium.org/6246037/diff/1/scripts/image_signing/ensure_sa... > scripts/image_signing/ensure_sane_lsb-release.sh:85: > On 2011/02/02 15:57:12, petkov wrote: > > remove extra blank line > > Done > > http://codereview.chromium.org/6246037/diff/1/scripts/image_signing/ensure_sa... > scripts/image_signing/ensure_sane_lsb-release.sh:107: . "$configfile" > On 2011/02/02 15:57:12, petkov wrote: > > you may want to print an info message showing which config file you're using. > > Done > > http://codereview.chromium.org/6246037/diff/1/scripts/image_signing/ensure_sa... > scripts/image_signing/ensure_sane_lsb-release.sh:110: mount_image_partition_ro > "$image" 3 "$rootfs" > On 2011/02/02 15:57:12, petkov wrote: > > on error, nothing gets unmounted? and the temp dir remains? > > From my reading of common_minimal.sh, this is all handled automatically? Did I > misunderstand? (trap "cleanup_temps_and_mounts" EXIT) > > http://codereview.chromium.org/6246037/diff/1/scripts/image_signing/ensure_sa... > scripts/image_signing/ensure_sane_lsb-release.sh:118: local board=$(lsbval $lsb > CHROMEOS_RELEASE_BOARD | \ > On 2011/02/02 15:57:12, petkov wrote: > > no need for trailing \ after | I think. also, I think style is each pipe on a > > separate line, i.e., > > > > cmd 1 | > > cmd 2 | > > cmd 3 > > Done. > > http://codereview.chromium.org/6246037/diff/1/scripts/image_signing/ensure_sa... > scripts/image_signing/ensure_sane_lsb-release.sh:121: local > boardvar=${board//-/_} > On 2011/02/02 15:57:12, petkov wrote: > > i see why you're doing this but it's a bit flaky. it's ok as is, however, is > > there any way we can convert expected values to a proper map so that there's > no > > need for the key to be a real identifier? maybe through an array indexed by > > looking up the board index in the expected boards array? > > I agree with the sentiments and had the same thoughts when writing it. For me it > came down to "well, if I want it that sophisticated, I ought to be writing this > in python where I have dictionary types and such." ;-) > > With the limited data structures available, looking up the board offset in 1 > array, then looking up an appid at the same offset in another array would be > possible, though I fear it could become maintainer-unfriendly as the number of > boards grows and things could get mis-correlated if some item gets added to the > wrong place in the array. > > Hopefully, having moved the board-check to being above the eval, I've addressed > most of the concern about flake - this should only ever eval to a small > whitelist of things now. > > http://codereview.chromium.org/6246037/diff/1/scripts/image_signing/ensure_sa... > scripts/image_signing/ensure_sane_lsb-release.sh:123: > On 2011/02/02 01:33:42, gauravsh wrote: > > extra newline > > Done > > http://codereview.chromium.org/6246037/diff/1/scripts/image_signing/ensure_sa... > scripts/image_signing/ensure_sane_lsb-release.sh:133: for b in > ${expected_boards[@]}; do > On 2011/02/02 01:33:42, gauravsh wrote: > > shouldn't the board check happen before the eval on line 122, (in which case > the > > app id check should also be skipped)? > > Done. > > http://codereview.chromium.org/6246037/diff/1/scripts/image_signing/ensure_sa... > scripts/image_signing/ensure_sane_lsb-release.sh:148: track_recognized=1 > On 2011/02/02 01:33:42, gauravsh wrote: > > these 2 checks for CHROMEOS_RELEASE_BOARD and CHROMEOS_RELEASE_TRACK basically > > use the same code, can you re-factor them into a single function? (something > > like - check_keyval_in_list <lsbfile> <key> <listname>) > > Done. > > http://codereview.chromium.org/6246037/diff/1/scripts/image_signing/ensure_sa... > scripts/image_signing/ensure_sane_lsb-release.sh:158: main $@ > On 2011/02/02 15:57:12, petkov wrote: > > "$@" > > Done > > http://codereview.chromium.org/6246037/diff/1/scripts/image_signing/ensure_sa... > scripts/image_signing/ensure_sane_lsb-release.sh:158: main $@ > On 2011/02/02 15:57:12, petkov wrote: > > add blank line before main > > Done
LGTM with 2 comments which you may choose to ignore or address in a separate CL. http://codereview.chromium.org/6246037/diff/1/scripts/image_signing/ensure_sa... File scripts/image_signing/ensure_sane_lsb-release.sh (right): http://codereview.chromium.org/6246037/diff/1/scripts/image_signing/ensure_sa... scripts/image_signing/ensure_sane_lsb-release.sh:119: cut -d = -f 2 | cut -d - -f 1,2) On 2011/02/02 15:57:12, petkov wrote: > this may be a bit flaky but i don't have much better suggestions. i don't think > there's a rule that the basic board name is id-id. somebody may come up with > x86-mario-v2, for example. Yes, I agree with this concern. The proper solution (IMO) would be to have a canonical board/platform name in lsb-release. Right now we overload that to indicate how the image is signed. http://codereview.chromium.org/6246037/diff/6001/scripts/image_signing/ensure... scripts/image_signing/ensure_sane_lsb-release.sh:50: # - Each line is a reasonable size (256 bytes). you could, I guess, also use an eval assuming the 3rd argument is the name of a list. not sure, if it will be necessarily cleaner, but worth a thought. http://codereview.chromium.org/6246037/diff/6001/scripts/image_signing/ensure... scripts/image_signing/ensure_sane_lsb-release.sh:122: eval "expected_appid=\"\$expected_appid_$boardvar\"" A thought: Since we will only keep the a default config file in here (and not the actual production config), you could replace this substitution with a file called ${SCRIPT_DIR}/default_lsb_release.config. This way the default config data file is not tied to the name of the script (which makes renaming easier). |
