Re: CVS commit: src/etc

[Alexander Nasonov <alnsn%yandex.ru@localhost>]

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