[LU-6056] sanity-hsm uses bash's local built-in incorrectly Created: 19/Dec/14 Updated: 05/Jun/15 Resolved: 16/Jan/15 |
|
| Status: | Resolved |
| Project: | Lustre |
| Component/s: | None |
| Affects Version/s: | Lustre 2.7.0 |
| Fix Version/s: | Lustre 2.7.0 |
| Type: | Bug | Priority: | Minor |
| Reporter: | John Hammond | Assignee: | Bruno Faccini (Inactive) |
| Resolution: | Fixed | Votes: | 0 |
| Labels: | hsm, tests | ||
| Severity: | 3 |
| Rank (Obsolete): | 16868 |
| Description |
|
sanity-hsm (and probably all test scripts) contains several instances of the following local fid=$(make_large_for_striping $f)
[ $? != 0 ] && skip "not enough free space" && return
This is wrong since the exit status of make_large_for_striping gets discarded when used with the local built-in. If this is rewritten as local fid
fid=$(make_large_for_striping $f)
[ $? != 0 ] && skip "not enough free space" && return
then it will work as intended. From the Google Bash Style Guide "Declaration and assignment must be separate statements when the assignment value is provided by a command substitution; as the 'local' builtin does not propagate the exit code from the command substitution." |
| Comments |
| Comment by Gerrit Updater [ 19/Dec/14 ] |
|
Faccini Bruno (bruno.faccini@intel.com) uploaded a new patch: http://review.whamcloud.com/13159 |
| Comment by Bruno Faccini (Inactive) [ 19/Dec/14 ] |
|
Wow, thanks to have found this John! |
| Comment by John Hammond [ 20/Dec/14 ] |
|
> Concerning the other possible occurrences of such bug in our other scripts and since it does not look really easy to be automatically detected, I have a question, did you find it just by reading the script or have used some code browser/tool that could also be widely run on all our scripts? I only noticed because (while running locally) I could see that make_large_for_cancel() was failing but that the [ $? != 0 ] test was not detecting the failure. This reminded me of the strange behavior of local. About auditing the other scripts, maybe something like the following could be used: awk '
/local/ {
last_local_nr = NR;
last_local_line = $0;
}
/\$\?/ {
if (last_local_nr == NR - 1) {
printf "%4d:%s\n", last_local_nr, last_local_line
printf "%4d:%s\n", NR, $0;
printf "----\n"
}
}
' lustre/tests/sanity-hsm.sh
Obviously it doesn't catch everything. 535: local st=$($LFS hsm_state $f) 536: [[ $? == 0 ]] || error "$LFS hsm_state $f failed" ---- 1155: local fid=$(make_large_for_striping $f) 1156: [ $? != 0 ] && skip "not enough free space" && return ---- 2032: local fid=$(make_large_for_progress $f) 2033: [ $? != 0 ] && skip "not enough free space" && return ---- 2071: local fid=$(make_large_for_progress $f) 2072: [ $? != 0 ] && skip "not enough free space" && return ---- 2092: local fid=$(make_large_for_progress $f) 2093: [ $? != 0 ] && skip "not enough free space" && return ---- 2272: local fid=$(make_large_for_progress $f) 2273: [ $? != 0 ] && skip "not enough free space" && return ---- 2295: local fid=$(make_large_for_progress_aligned $f) 2296: [ $? != 0 ] && skip "not enough free space" && return ---- 2318: local fid=$(make_large_for_progress $f) 2319: [ $? != 0 ] && skip "not enough free space" && return ---- 2384: local fid=$(make_large_for_progress $f) 2385: [ $? != 0 ] && skip "not enough free space" && return ---- 2419: local fid=$(make_large_for_progress $f) 2420: [ $? != 0 ] && skip "not enough free space" && return ---- 2457: local fid=$(make_large_for_progress $f) 2458: [ $? != 0 ] && skip "not enough free space" && return ---- 2652: local fid=$(make_large_for_progress $f) 2653: [ $? != 0 ] && skip "not enough free space" && return ---- 2775: local fid=$(make_large_for_progress $f) 2776: [ $? != 0 ] && skip "not enough free space" && return ---- 2883: local fid=$(make_large_for_progress $f) 2884: [ $? != 0 ] && skip "not enough free space" && return ---- 3160: local fid=$(make_large_for_progress $f) 3161: [ $? != 0 ] && skip "not enough free space" && return ---- 3473: local fid=$(make_large_for_cancel $f) 3474: [ $? != 0 ] && skip "not enough free space" && return ---- 3516: local fid=$(make_large_for_progress $f) 3517: [ $? != 0 ] && skip "not enough free space" && return ---- 3563: local fid=$(make_large_for_cancel $f) 3564: [ $? != 0 ] && skip "not enough free space" && return ---- 3671: local fid=$(make_large_for_progress $f) 3672: [ $? != 0 ] && skip "not enough free space" && return ---- 3732: local fid=$(make_large_for_progress $f) 3733: [ $? != 0 ] && skip "not enough free space" && return ---- 3936: local fid=$(make_large_for_cancel $f) 3937: [ $? != 0 ] && skip "not enough free space" && return ---- |
| Comment by Gerrit Updater [ 05/Jan/15 ] |
|
Oleg Drokin (oleg.drokin@intel.com) merged in patch http://review.whamcloud.com/13159/ |
| Comment by Bruno Faccini (Inactive) [ 10/Jan/15 ] |
|
Oops, just discovered that my change title had a typo, missing the "sanity" pattern in front of "-hsm" ... |
| Comment by John Hammond [ 12/Jan/15 ] |
|
> John, do you agree that we can close this ticket now? Yes. |
| Comment by Jodi Levi (Inactive) [ 16/Jan/15 ] |
|
Patch landed to Master. |