Re: CVS commit: src/etc

[Robert Elz <kre%munnari.OZ.AU@localhost>]

A couple of comments about your mount_critical_filesystems_zfs()
function in rc.subr

It starts:

	eval _fslist=\$critical_filesystems_zfs

I'm not sure what you're attempting to accomplish there.  The eval
command sees
	fslist=$critical_filesystems_zfs
(the \ having quoted the '$' preventing variable expansion, and
was then removed).

eval uses that as input, and runs it.   But if that was what you
wanted, why bother with eval at all, just write it as eval sees it.

eval is most useful when some of the arg string is to come from a
variable that is expanded before eval sees it, which isn't the case here.

Later:

	 _dataset=`

(followed by a lengthy command substitution).   Please don't use ``
command substitutions, they are fragile, and essentially obsolete.
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).   Perhaps "zfs mount" only ever
does exit(1) for any error, in which case it doesn't matter (1 from any
execution is the same thing) but if it returns different exit codes
depending upon the error, you usually want the first one, rather than
the last - particularly with filesystem mounting, it isn't unusual for
later mounts to fail if an earlier one did, if the earlier one was
intended to provide the mount points, for example - knowing that the
later mounts failed because the directory they intended to mount onto
wasn't present is fairly useless, what you want to know is why the
earlier one failed.

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 !=).

kre