ports-mgmt/rc-subr-jail: Shell library to help writing jailed rc services.

[arrowd]

Summary

A review from someone knowledgeable in shell would be really appreciated.

Test Plan

I rewrote net-p2p/cardano-node rc script using these functions:

https://github.com/freebsd/freebsd-ports-haskell/commit/c8f879f6c5d43152164d85b13978ec075e075375

[arrowd]

• Add NO_MTREE knob.

[flo_purplekraken.com]

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.

[arrowd]

• Address comments.

[jlduran]

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

8

: "${jail_copy_resolv_conf:='no'}" and the following knobs?
Or are these defined somewhere else?

46

67

80

Can you test(1) the return of check_kern_features directly?

if check_kern_features inet; then
     ...
fi

Else, per test(1), you should check both integers (the return code $?, and 0), using -eq

84

Same here

91

Prefer read -r so backslashes do not act as escape characters (in this case, it's mostly to appease ShellCheck).

122

Same here (read -r)?

[arrowd]

ports-mgmt/rc-subr-jail/files/rc.subr.jail

8

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.

[jlduran]

ports-mgmt/rc-subr-jail/files/rc.subr.jail

8

OK, how about using checkyesno?

[arrowd]

• Address comments.

[arrowd]

ports-mgmt/rc-subr-jail/files/rc.subr.jail

8

checkyesno prints a warning if the variable isn't defined. We don't want this, as these knobs are optional.

[mat]

It feels like something like this should be in base, not in ports.

[arrowd]

In D38394#879861, @mat wrote:

It feels like something like this should be in base, not in ports.

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.