Details
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 46462 Build 43351: arc lint + arc unit
Event Timeline
You could probably simplify it by just an if/elif/else instead of having to indent a separate if/else in the first else block, but, I don't think it is a big deal
Reviewed by: allanjude
| usr.sbin/bsdinstall/scripts/rootpass | ||
|---|---|---|
| 36–42 | This can just be if [ -z "$ROOTPASS_ENC" -a -z "$ROOTPASS_PLAIN" ]; then | |
Sorry for being late to the party
| usr.sbin/bsdinstall/scripts/rootpass | ||
|---|---|---|
| 36–42 | Actually, it can be just: if [ -z "$ROOTPASS_ENC$ROOTPASS_PLAIN" ]; then As an aside … You would want to avoid passing greater than 4 args to [ because many (all?) implementations only combinatorically guarantee syntactically correct results for handling up to 4 arguments (that for example passing 20 arguments, chaining many “-a” arguments, is not deterministically tested whereas there are test units for every possible combination up to — and including — 4 arguments). So using && was actually more correct from a portability standpoint, and I cannot condone the change to use -a. However, the whole discussion of && vs [ … -a … ] is registered moot when you realize that you can just expand both variables into a single string and then test the results. If either string is non-NULL, the desired results are achieved without having to resort to using either && or -a | |
| usr.sbin/bsdinstall/scripts/rootpass | ||
|---|---|---|
| 36–42 | Ooooh clever and optimized, I like it. | |
| usr.sbin/bsdinstall/scripts/rootpass | ||
|---|---|---|
| 36–42 | Also see the BUGS section of test(1).. I chose to go with the way Allan suggested since it looks cleaner to me. | |
LGTM, thanks!
One last nit that you may want to change is to use printf instead of echo. echo may behave in an undesired way if the root password is starting with a dash for example.
| usr.sbin/bsdinstall/scripts/rootpass | ||
|---|---|---|
| 37–39 | Perhaps this echo won't cause any problems (nobody sets their root password to -e and -n, and nobody is going to run this with a shell with different echo behaviour?) but something like printf '%s\n' "$ROOTPASS_PLAIN" is more obviously correct. An alternative is something like pw -R $BSDINSTALL_CHROOT usermod root -h 0 <<END $ROOTPASS_PLAIN END but that has the ugliness of a here-document. | |
| usr.sbin/bsdinstall/scripts/rootpass | ||
|---|---|---|
| 37–39 | I tried to break it, and could not: > cat test.sh #!/bin/sh echo -nooo > sh -x ./test.sh + echo -nooo -nooo Try #2: > cat test.sh #!/bin/sh echo -nooo lkjsldjf > sh -x ./test.sh + echo -nooo lkjsldjf -nooo lkjsldjf Anything else I should try? | |
FWIW, I'd switch to printf '%s' "$ROOTPASS" on general principles, to avoid the possibility of silliness.
*dons former infosec hat* Agree. Even if it looks OK now, who knows what you didn't consider or what someone will figure in 10 or 20 years?
| usr.sbin/bsdinstall/bsdinstall.8 | ||
|---|---|---|
| 346–349 | Need to say which is used if they're both present and give the format of ROOTPASS_ENC. Salted hash? | |
| usr.sbin/bsdinstall/bsdinstall.8 | ||
|---|---|---|
| 346–349 | Did the first half, but I am not sure this is the right place to do the latter half.. Perhaps best to just Xr pw(8)? | |
| usr.sbin/bsdinstall/bsdinstall.8 | ||
|---|---|---|
| 346–349 | If by that you mean something like in the format expected by .Xr pw 8 .Fl H Ar 0 (markup not guaranteed correct), WFM. | |