#1886 - LPAR/PPC64 bootlist is incorrectly set when having multiple 'prep' partitions #1887
Conversation
…le 'prep' partitions - Added handling of multiple 'prep' partitions - Enhanced handling of multiple 'prep' partitions and multipath
|
|
||
| if [[ ${#boot_list[@]} -gt 0 ]]; then | ||
| LogPrint "Set LPAR bootlist to '${boot_list[@]}'" | ||
| bootlist -m normal $boot_list | ||
| LogIfError "Unable to set bootlist. You will have to start in SMS to set it up manually." |
There was a problem hiding this comment.
Regadless that this is not part of this pull request but in general
I think LogIfError is useless to tell something to the user
because the LogIfError output appears only in the log file.
To show an error message to the user better use LogPrintIfError
or whatever appropriate function in lib/_input-output-functions.sh
|
@schabrolles |
|
@schabrolles |
|
@jsmeix, bootlist is responsible to set the boot devices order list for an LPAR in PowerVM. |
|
@schabrolles |
|
@jsmeix, for a plain looking code, this looks correct to me... |
|
@rmetrich @schabrolles |
|
|
||
| if [[ ${#boot_list[@]} -gt 0 ]]; then | ||
| LogPrint "Set LPAR bootlist to '${boot_list[@]}'" | ||
| bootlist -m normal $boot_list |
There was a problem hiding this comment.
Expanding $boot_list this way will return only the first element. Is it intended? Why is boot_list an array if it is intended - one element would be enough?
IIUC, this will lead to boot failure if the first path is disconnected.
There was a problem hiding this comment.
Ough, this is indeed broken!
| fi | ||
|
|
||
| if [[ ${#boot_list[@]} -gt 0 ]]; then | ||
| LogPrint "Set LPAR bootlist to '${boot_list[@]}'" |
There was a problem hiding this comment.
Using ${boot_list[@]} inside double quotes will cause it to expand to multiple strings, like this:
boot_list=( foo bar )
"Set LPAR bootlist to '${boot_list[@]}'" -> "Set LPAR bootlist to 'foo" "bar'"
LogPrint actually handles multiple arguments properly, so it is not a bug. But it is IMO better to do this in a less surprising way, because I don't think this kind of splitting into multiple argument was intended. And it can become a bug when this code gets cargo-culted to other places. And it is flagged by static analysis: https://github.com/koalaman/shellcheck/wiki/SC2145.
I propose:
LogPrint "Set LPAR bootlist to '${boot_list[*]}'"
(mentioned also in #2097 )
Relax-and-Recover (ReaR) Pull Request Template
Please fill in the following items before submitting a new pull request:
Pull Request Details:
Type: Bug Fix
Impact: Normal
Reference to related issue (URL): https://github.com/rear/rear/issues/1886
How was this pull request tested?
Tested on AIX/LPAR/PPC64le with 2 prep partitions, but no multipath on older ReaR 2.00 version
Brief description of the changes in this pull request: