Hi Robert,
Robert Elz wrote:
> A couple of comments about your mount_critical_filesystems_zfs()
> function in rc.subr
Thank you for reviewing my code!
> It starts:
>
> eval _fslist=\$critical_filesystems_zfs
>
> I'm not sure what you're attempting to accomplish there.
I copied this line from mount_critical_filesystems:
eval _fslist=\$critical_filesystems_${1}
and changed ${1} to zfs without realising that I don't need eval
anymore.
>...
> _dataset=`
>
> (followed by a lengthy command substitution). Please don't use ``
> command substitutions, they are fragile, and essentially obsolete.
Noted.
I don't like the approach I took but I'm not very comfortable in
a bare shell when tools in /usr aren't (yet) available and this
giant $() was all I could think of.
> The only excuse for ever using them in anything modern is if the
> script might need to be run by a truly ancient shell. Just use $( )
> instead, the semantics are much cleaner.
>
> And perhaps more important, near the bottom of the big loop:
>
> if [ "$_mount_es" != 0 ]; then
> _mountcrit_es="$_mount_es"
> fi
>
> which causes the exit status of the function (_mountcrit_es) to be the
> status of the last mount that failed for some reason, rather than the
> first (which tends to be more common).
These lines is a copy/paste from mount_critical_filesystems and
your comment apply to that function too.
>...
> One additional minor point, it might be better to use -ne there instead
> of != since the values are intended to be integers, rather than strings.
> But that doesn't really matter (unless $_mount_es might be "" in which
> case using != is better - that would be 0 for -ne, but not 0 for !=).
Yes, it's intended to be integers.
--
Alex
