A review from someone knowledgeable in shell would be really appreciated.
Details
- Reviewers
- None
- Group Reviewers
Contributor Reviewers (ports) Jails - Commits
- R11:44b00ffe5fb0: ports-mgmt/rc-subr-jail: + Shell library to help writing jailed rc services.
I rewrote net-p2p/cardano-node rc script using these functions:
https://github.com/freebsd/freebsd-ports-haskell/commit/c8f879f6c5d43152164d85b13978ec075e075375
Diff Detail
- Repository
- rP FreeBSD ports repository
- Lint
No Lint Coverage - Unit
No Test Coverage - Build Status
Buildable 49609 Build 46499: arc lint + arc unit
Event Timeline
| ports-mgmt/rc-subr-jail/files/rc.subr.jail | ||
|---|---|---|
| 43 | Can we be sure that the argument will never include spaces? Otherwise we need to quote it. | |
I do knot fully understand what is the purpose of this script, if it is what I think it is, then this seems like a nice addition.
Anyhow, here are a few basic ShellCheck suggestions.
| ports-mgmt/rc-subr-jail/files/rc.subr.jail | ||
|---|---|---|
| 7 | : "${jail_copy_resolv_conf:='no'}" and the following knobs? | |
| 45 | ||
| 66 | ||
| 79 | Can you test(1) the return of check_kern_features directly? if check_kern_features inet; then
...
fiElse, per test(1), you should check both integers (the return code $?, and 0), using -eq | |
| 83 | Same here | |
| 90 | Prefer read -r so backslashes do not act as escape characters (in this case, it's mostly to appease ShellCheck). | |
| 121 | Same here (read -r)? | |
| ports-mgmt/rc-subr-jail/files/rc.subr.jail | ||
|---|---|---|
| 7 | I just thought there is no need in providing default value, since I check it the following way if [ "$jail_copy_resolv_conf" = "yes" ]; then If the variable isn't defined, this will turn into [ "" = "yes"] and the conditional will not be executed. | |
| ports-mgmt/rc-subr-jail/files/rc.subr.jail | ||
|---|---|---|
| 7 | OK, how about using checkyesno? | |
| ports-mgmt/rc-subr-jail/files/rc.subr.jail | ||
|---|---|---|
| 7 | checkyesno prints a warning if the variable isn't defined. We don't want this, as these knobs are optional. | |
Yes, I agree, but this would greatly delay updates to this library. At its current state the library will certainly require more changes as more ports begin to use it. I think it is better to keep it in Ports until it is polished enough.