[WIP] Completely rework shell scripts #9182

c0d3z3r0 · 2019-08-19T22:45:09Z

THIS IS WIP - more changes will follow, as stated in #9182 (comment)

Motivation and Context

The shell scripts zfs-functions et al. have multiple problems:

Current shell code is hard to read and debug because of too much unoptimized code
Some bugs (like Fix bug in in_mtab() where the return value is always 1 #9168, zfs-functions: get_root_pool does not handle spaces in pool name #9183, and I guess some more but I did not search in the issues, yet)
Inconsistent code style
Use of upper case variables (very confusing at least for me, as only env vars or globals should be upper case (isn't this (un)written convention?)
Very much missing documentation which makes it hard for new devs to understand what SHOULD happen (which is not always what really happens :P)
use of evi... eval() where it is not needed at all, e.g.: eval export FSTAB_dev_$i="$fs"
duplicated functionality: in_mtab vs. is_mounted
inconsistent use of mount and cat/read /proc/self/mounts (which both return the same results. The only differene is that mount does some formatting (insert on and type, put options in brackets, omitting freq and pass)
Caching of fstab and mtab in env makes everything very, very complicated due to special character handling and (as far as I looked at the code, yet) is not needed at all. (MAYBE this is really needed and I did not get this far, yet. We'll see.) IMHO it really does not matter if we call grep multiple times but can completely drop any eval. The run time difference is negligible and should be in the range of some 10s or 100s of milliseconds.
... (more to follow)

This PR does a complete rework of all existing shell scripts to fix all those problems while keeping backwards compatibility.

Signed-off-by: Michael Niewöhner foss@mniewoehner.de

Description

How Has This Been Tested?

Currently only manual basic testing on function level;

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Performance enhancement (non-breaking change which improves efficiency)
Code cleanup (non-breaking change which makes code smaller or more readable)
Breaking change (fix or feature that would cause existing functionality to change)
Documentation (a change to man pages or other documentation)

Checklist:

My code follows the ZFS on Linux code style requirements.
I have updated the documentation accordingly.
I have read the contributing document.
I have added tests to cover my changes.
All new and existing tests passed.
All commit messages are properly formatted and contain Signed-off-by.

gdevenyi · 2019-08-19T23:16:30Z

Has shellcheck been used to check for issues?

c0d3z3r0 · 2019-08-19T23:30:34Z

Has shellcheck been used to check for issues?

not yet

ahrens · 2019-08-20T20:49:10Z

This PR does a complete rework of all existing shell scripts

It looks like the commit here is changing two .in files (which are processed to generate shell scripts). There are more shell scripts in zed.d/, scripts/, and many in the test suite. Do the problems you described apply to those as well, and are you planning to change them?

c0d3z3r0 · 2019-08-20T20:52:15Z

This PR does a complete rework of all existing shell scripts

It looks like the commit here is changing two .in files (which are processed to generate shell scripts). There are more shell scripts in zed.d/, scripts/, and many in the test suite. Do the problems you described apply to those as well, and are you planning to change them?

Yes, this is still WIP and I will push more changes over the next time, but it will take a bit because a) it's really much code and b) I don't want to break anything and c) want to keep backwards compatibility as good as possible

chrisrd

Huh. I could have sworn I posted a final comment previously. Oh well, it went something like this...

The processing of the mount tables definitely needs improvement, and more documentation is always appreciated.

I'm a bit equivocal about the style changes: I certainly don't dislike the new style as it matches my own when writing new scripts, however I don't find the existing style difficult, and bulk changes like this are a bit disruptive to the code base.

For the removal of mount table caching via environment variables (which is definitely flawed in the current code - I have discovered a truly marvelous fix of this, which this margin is too narrow to contain), can you characterise what the effect might be, e.g. for a system with, say, 30,000 live mounts?

I wonder if findmnt could be used rather than hand rolling our own mount table interpreter, whichever way (cache or not) we decide to go. I guess that depends on whether findmnt is available on all supported platforms.

In the original and new code, get_root_pool() and in_fstab() are unused. It looks like removal of these functions means the only caller of filter_tabfile() would be in_mtab() (via in_tabfile()). That would allow sigificant simplification of the filter_tabfile() - removal of the argument parsing etc.

chrisrd · 2019-08-19T23:25:55Z

etc/init.d/zfs-functions.in

 # Source function library
-if [ -f /etc/rc.d/init.d/functions ]; then
+if [ -e /etc/rc.d/init.d/functions ]; then


Why the change from -f to -e? E.g. what happens if /etc/rc.d/init.d/functions is a directory?

uhm. wtf did I do there...? I wanted to change -L to -f as this matches both symlink and file...

etc/init.d/zfs-functions.in

chrisrd · 2019-08-19T23:48:53Z

etc/init.d/zfs-functions.in


-	if type start-stop-daemon > /dev/null 2>&1 ; then
+	if do_noout type start-stop-daemon


Did you mean do_quiet here (and below)? The original code was dumping both stderr and stdout, new code is only dumping stdout.

IMHO we shouldn't quiet errors here because that would make debugging in case of errors very hard

It's testing if start-stop-daemon or daemon is present, which depends on the distribution. If it's a daemon type distribution they don't need to see "start-stop-daemon: not found".

Point for you, will fix that

chrisrd · 2019-08-19T23:50:17Z

etc/init.d/zfs-functions.in

 	else
 		# Unsupported
-		return 3
+		return -1


Why the change in return code?

context ;) I needed 3 for the return four lines above. Increasing to 4 didn't seem right as it is not related to 1-3 but a completely different error. Since -1 isn't posix this returns 255 now.

I'm wondering if the return codes have meaning within the init system they're called from? E.g. "3" might mean "unsupported" and changing the return code might change the behavior of the init system.

chrisrd · 2019-08-19T23:52:37Z

etc/init.d/zfs-functions.in

-		[ "$?" = 0 ] && rm -f "$PIDFILE"
+		start-stop-daemon --quiet --stop \
+		    --pidfile "${pidfile}" --name "${daemon_name}" \
+		    --retry=TERM/30/KILL/5


Losing removing the $pidfile?

will be deleted by the daemon(s), so wo do not need to do it manually

Are you sure? E.g.

$ man start-stop-daemon ... -p, --pidfile pid-file Check whether a process has created the file pid-file. Note: using this matching option alone might cause unintended processes to be acted on, if the old process terminated without being able to remove the pid-file. ...

Not start-stop-daemon but the daemon itself creates and deleetes the PID file.
Just look at zed for example :-) I checked that

chrisrd · 2019-08-20T00:26:54Z

etc/init.d/zfs-functions.in

+
+	local lines="$( \
+	    egrep -av '^#' "${tabfile}" | \
+    	    egrep -ai "^${dev:-[^ ]+} ${mntpt:-[^ ]+} ${type:-[^ ]+} ${opts:-[^ ]+}" | \


Note: /etc/fstab can contain arbitrary characters octal encoded as \NNN. E.g. a mount point of /foo could appear in /etc/fstab as \057\146\157\157. Your grep matching won't catch that. But to be fair, neither would the existing version, so we might not care.

Well, I did catch \040 and \011 as the are named in the man page. I wasn't aware of other possible values. Does anyone really use that?

It's a big world out there: someone is sure to be using it! 😄

Also, /proc/self/mounts encodes a \ as \134 so need to catch at least that as well. Here's some fun:

# mkdir '/tmp/foo' '/tmp/foo\t2\1 x\11' # ls -ld '/tmp/foo' '/tmp/foo\t2\1 x\11' drwxr-xr-x 2 root root 4096 Aug 21 12:19 /tmp/foo drwxr-xr-x 2 root root 4096 Aug 21 12:19 /tmp/foo\t2\1 x\11 # echo '/tmp/foo /tmp/foo\t2\1341\040x\11 none bind,noauto' >> /etc/fstab # mount '/tmp/foo\t2\1 x\11' # grep foo /proc/self/mounts /dev/mapper/ubuntu--vg-root /tmp/foo\134t2\1341\040x\13411 ext4 rw,relatime,errors=remount-ro,data=ordered 0 0

Holy.... \o/ ok... maybe findmnt will fix this all for us...

chrisrd · 2019-08-20T00:29:01Z

etc/init.d/zfs-functions.in

+	[ $(echo "${lines}" | wc -l) -gt 1 ] && \
+	    echo 'Warning: multiple matches; only first one returned.' >&2
+
+	printf "${lines}" | head -1


/etc/fstab does NOT use single-character escapes, e.g. \t is a literal "backslash t", but this printf will turn that into a tab character.

\t isn't allowed in fstab, is it?

Sure is - see above. Backslash only means something if followed by exactly 3 octal digits. The rest can be arbitrary bytes (but not null).

chrisrd · 2019-08-20T00:39:50Z

etc/init.d/zfs-functions.in

 	return 0
 }

-# first parameter is a regular expression that filters mtab
-read_mtab()


This is called from etc/init.d/zfs-mount.in, but that script isn't adjusted?

Oh guys, I'm repeating over and over again. This PR is still WIP. I will fix the others too, as promised but this will take some days ;-)

heh what!? no, Github, I don't want to start a review on my own code... wtf...

The problem is incomplete changes will ensure the github auto tests will fail.

Will be as soon as I have time

chrisrd · 2019-08-20T01:45:49Z

etc/init.d/zfs-functions.in

+# output:	matching mtab entry
+# returns:	boolean
+#
+get_fstab_entry()


Yes, because I did not rewrite the users of the env vars. This will be used once that is done

chrisrd · 2019-08-20T01:54:51Z

etc/init.d/zfs-functions.in

-		echo "$fs" | egrep -qE '^#|^$' && continue
-		echo "$mntpnt" | egrep -qE '^none|^swap' && continue
-		echo "$fstype" | egrep -qE '^swap' && continue
+	# backwards compatibility


Do we need backwards compatibility given we can fix the callers (zfs-mount etc.)?

I wanted to provide backwards compatibility to maybe exisiting user/distributions scripts but we can decide to not support any of them and cross fingers we don't break their systems... not sure and this is not something I can decide. @behlendorf @ahrens what do you say about compat for external users?

The shell scripts zfs-functions et al. have multiple problems: - Current shell code is hard to read and debug because of too much unoptimized code - Some bugs (like openzfs#9168, openzfs#9183, and I guess some more but I did not search in the issues, yet) - Inconsistent code style - Use of upper case variables (very confusing at least for me, as only env vars or globals should be upper case (isn't this (un)written convention?) - Very much missing documentation which makes it hard for new devs to understand what SHOULD happen (which is not always what really happens) - use of evi... eval() where it is not needed at all, e.g.: `eval export FSTAB_dev_$i="$fs"` - duplicated functionality: `in_mtab` vs. `is_mounted` - inconsistent use of `mount` and `cat/read /proc/self/mounts` (which both return the same results. The only differene is that mount does some formatting (insert `on` and `type`, put `options` in brackets, omitting `freq` and `pass`) - Caching of fstab and mtab in env makes everything very, very complicated due to special character handling and (as far as I looked at the code, yet) is not needed at all. (MAYBE this is really needed and I did not get this far, yet. We'll see.) IMHO it really does not matter if we call grep multiple times but can completely drop any `eval`. The run time difference is negligible and should be in the range of some 10s or 100s of milliseconds. - ... (more to follow) This PR does a complete rework of all existing shell scripts to fix all those problems while keeping backwards compatibility. Signed-off-by: Michael Niewöhner <foss@mniewoehner.de>

c0d3z3r0 · 2019-08-21T01:24:06Z

@chrisrd Thanks for looking at this!

Huh. I could have sworn I posted a final comment previously. Oh well, it went something like this...

The processing of the mount tables definitely needs improvement, and more documentation is always appreciated.

I'm a bit equivocal about the style changes: I certainly don't dislike the new style as it matches my own when writing new scripts, however I don't find the existing style difficult, and bulk changes like this are a bit disruptive to the code base.

The problem I see is that there is no style. There are two or three, which makes it difficult and error-prone.

For the removal of mount table caching via environment variables (which is definitely flawed in the current code - I have discovered a truly marvelous fix of this, which this margin is too narrow to contain), can you characterise what the effect might be, e.g. for a system with, say, 30,000 live mounts?

Haha! I knew there must be a reason for that :D No, I absolutely can not... I will look at this an do some simulations with both codes so we can see the difference.

I wonder if findmnt could be used rather than hand rolling our own mount table interpreter, whichever way (cache or not) we decide to go. I guess that depends on whether findmnt is available on all supported platforms.

Oh, this is brilliant! This would save us a LOT of converting/guessing/crystal-ball-handling!
I guess it's available by default on any distribution (I need to verify this and do some tests first, though).

In the original and new code, get_root_pool() and in_fstab() are unused. It looks like removal of these functions means the only caller of filter_tabfile() would be in_mtab() (via in_tabfile()). That would allow sigificant simplification of the filter_tabfile() - removal of the argument parsing etc.

Here the same applies as for ... uhm, I forgot the function name... look in one of my comments above... they will (maybe) be needed later when I touch the other scripts. If we like to, we can drop any leftovers then.

codecov · 2019-08-21T06:01:19Z

Codecov Report

Merging #9182 into master will decrease coverage by 12.54%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           master    #9182       +/-   ##
===========================================
- Coverage   79.31%   66.77%   -12.55%     
===========================================
  Files         400      325       -75     
  Lines      122000   104611    -17389     
===========================================
- Hits        96765    69850    -26915     
- Misses      25235    34761     +9526

Flag	Coverage Δ
#kernel	`?`
#user	`66.77% <ø> (-0.52%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c759b33...75d2194. Read the comment docs.

gdevenyi · 2019-08-21T14:03:04Z

Current state of shellcheck on this PR
ShellCheck - shell script analysis tool
version: 0.7.0
license: GNU General Public License, version 3
website: https://www.shellcheck.net

I hacked the include paths so that shellcheck could include them properly.

Question:
Are these scripts supposed to be POSIX? Based on pointing at /bin/sh I assumed so.

> shellcheck -s sh -x *in
In zfs-functions.in line 37:
	. /etc/rc.d/init.d/functions
          ^------------------------^ SC1091: Not following: /etc/rc.d/init.d/functions: openBinaryFile: does not exist (No such file or directory)


In zfs-functions.in line 40:
	. /etc/init.d/functions.sh
          ^----------------------^ SC1091: Not following: /etc/init.d/functions.sh: openBinaryFile: does not exist (No such file or directory)


In zfs-functions.in line 68:
		local tIFS="${1}"
                ^--------^ SC2039: In POSIX sh, 'local' is undefined.


In zfs-functions.in line 77:
	zfs_log_begin_msg() { echo -n "${1} "; }
                                   ^-- SC2039: In POSIX sh, echo flags are undefined.


In zfs-functions.in line 80:
		[ "${1}" -eq 0 ] && success || failure
                                 ^-- SC2015: Note that A && B || C is not if-then-else. C may run when A is true.


In zfs-functions.in line 90:
	zfs_log_progress_msg() { echo -n $"${1}"; }  # TODO: does not work!?
                                      ^-- SC2039: In POSIX sh, echo flags are undefined.
                                         ^-----^ SC2039: In POSIX sh, $".." is undefined.


In zfs-functions.in line 99:
	zfs_log_progress_msg() { echo -n; }
                                      ^-- SC2039: In POSIX sh, echo flags are undefined.


In zfs-functions.in line 103:
	zfs_log_begin_msg() { echo -n "${1}"; }
                                   ^-- SC2039: In POSIX sh, echo flags are undefined.


In zfs-functions.in line 105:
		local ret=${1}
                ^-------^ SC2039: In POSIX sh, 'local' is undefined.


In zfs-functions.in line 110:
	zfs_log_progress_msg() { echo -n "${1}"; }
                                      ^-- SC2039: In POSIX sh, echo flags are undefined.


In zfs-functions.in line 140:
	local argname="${1}"
        ^-----------^ SC2039: In POSIX sh, 'local' is undefined.


In zfs-functions.in line 154:
	local msg="${1}";	shift
        ^-------^ SC2039: In POSIX sh, 'local' is undefined.


In zfs-functions.in line 155:
	local cmd="$*"
        ^-------^ SC2039: In POSIX sh, 'local' is undefined.


In zfs-functions.in line 156:
	local ret
        ^-------^ SC2039: In POSIX sh, 'local' is undefined.


In zfs-functions.in line 162:
	[ "${ret}" -eq 0 ] && zfs_log_end_msg ${ret} || \
                           ^-- SC2015: Note that A && B || C is not if-then-else. C may run when A is true.


In zfs-functions.in line 176:
	local pidfile="${1}";		shift
        ^-----------^ SC2039: In POSIX sh, 'local' is undefined.


In zfs-functions.in line 177:
	local daemon_bin="${1}";	shift
        ^--------------^ SC2039: In POSIX sh, 'local' is undefined.


In zfs-functions.in line 178:
	local daemon_args="$*"
        ^---------------^ SC2039: In POSIX sh, 'local' is undefined.


In zfs-functions.in line 195:
		[ -f "${pidfile}" -a -d /run/sendsigs.omit.d ] && \
                                  ^-- SC2166: Prefer [ p ] && [ q ] as [ p -a q ] is not well defined.


In zfs-functions.in line 221:
	local pidfile="${1}";		shift
        ^-----------^ SC2039: In POSIX sh, 'local' is undefined.


In zfs-functions.in line 222:
	local daemon_bin="${1}";	shift
        ^--------------^ SC2039: In POSIX sh, 'local' is undefined.


In zfs-functions.in line 223:
	local daemon_name="${1}"
        ^---------------^ SC2039: In POSIX sh, 'local' is undefined.


In zfs-functions.in line 246:
	local pidfile="${1}";		shift
        ^-----------^ SC2039: In POSIX sh, 'local' is undefined.


In zfs-functions.in line 247:
	local daemon_bin="${1}";	shift
        ^--------------^ SC2039: In POSIX sh, 'local' is undefined.


In zfs-functions.in line 248:
	local daemon_name="${1}"
        ^---------------^ SC2039: In POSIX sh, 'local' is undefined.


In zfs-functions.in line 270:
	local pidfile="${1}";	shift
        ^-----------^ SC2039: In POSIX sh, 'local' is undefined.


In zfs-functions.in line 271:
	local daemon_name="${1}"
        ^---------------^ SC2039: In POSIX sh, 'local' is undefined.


In zfs-functions.in line 297:
	local cmd=${1}
        ^-------^ SC2039: In POSIX sh, 'local' is undefined.


In zfs-functions.in line 355:
		[ -n "${init}" ] && exit 3
                      ^-----^ SC2154: init is referenced but not assigned.


In zfs-functions.in line 396:
	local var="${1}"
        ^-------^ SC2039: In POSIX sh, 'local' is undefined.


In zfs-functions.in line 411:
	local module="${1}"
        ^----------^ SC2039: In POSIX sh, 'local' is undefined.


In zfs-functions.in line 426:
	local module="${1}"
        ^----------^ SC2039: In POSIX sh, 'local' is undefined.


In zfs-functions.in line 450:
	local tabfile="${1}"; shift
        ^-----------^ SC2039: In POSIX sh, 'local' is undefined.


In zfs-functions.in line 452:
	local dev="$(parsearg dev "$@")"
        ^-------^ SC2039: In POSIX sh, 'local' is undefined.
              ^-^ SC2155: Declare and assign separately to avoid masking return values.


In zfs-functions.in line 453:
	local mntpt="$(parsearg mntpt "$@")"
        ^---------^ SC2039: In POSIX sh, 'local' is undefined.
              ^---^ SC2155: Declare and assign separately to avoid masking return values.


In zfs-functions.in line 454:
	local type="$(parsearg type "$@")"
        ^--------^ SC2039: In POSIX sh, 'local' is undefined.
              ^--^ SC2155: Declare and assign separately to avoid masking return values.


In zfs-functions.in line 455:
	local opts="$(parsearg opts "$@")"
        ^--------^ SC2039: In POSIX sh, 'local' is undefined.
              ^--^ SC2155: Declare and assign separately to avoid masking return values.


In zfs-functions.in line 463:
	local lines="$( \
        ^---------^ SC2039: In POSIX sh, 'local' is undefined.
              ^---^ SC2155: Declare and assign separately to avoid masking return values.


In zfs-functions.in line 469:
	[ $(printf "%s" "${lines}" | wc -l) -gt 1 ] && \
          ^-- SC2046: Quote this to prevent word splitting.


In zfs-functions.in line 512:
	local tabfile="${1}";	shift
        ^-----------^ SC2039: In POSIX sh, 'local' is undefined.


In zfs-functions.in line 513:
	local compatfilter
        ^----------------^ SC2039: In POSIX sh, 'local' is undefined.


In zfs-functions.in line 561:
	local mntpt="${1}"
        ^---------^ SC2039: In POSIX sh, 'local' is undefined.


In zfs-import.in line 1:
#!@SHELL@
^-------^ SC2239: Ensure the shebang uses an absolute path to the interpreter.


In zfs-import.in line 57:
	local cmd="$*"
        ^-------^ SC2039: In POSIX sh, 'local' is undefined.


In zfs-import.in line 59:
	do_noerr ${cmd} | egrep "pool:|^[a-zA-Z0-9]" | \
                 ^----^ SC2086: Double quote to prevent globbing and word splitting.
                          ^---^ SC2196: egrep is non-standard and deprecated. Use grep -E instead.

Did you mean: 
	do_noerr "${cmd}" | egrep "pool:|^[a-zA-Z0-9]" | \


In zfs-import.in line 67:
	local already_imported available_pools pool npools
        ^-- SC2039: In POSIX sh, 'local' is undefined.


In zfs-import.in line 68:
	local exception dir ZPOOL_IMPORT_PATH RET=0 r=1
        ^-- SC2039: In POSIX sh, 'local' is undefined.


In zfs-import.in line 74:
	if [ -n "$USE_DISK_BY_ID" -a "$USE_DISK_BY_ID" != 'yes' ]
                                  ^-- SC2166: Prefer [ p ] && [ q ] as [ p -a q ] is not well defined.


In zfs-import.in line 116:
		local found=""
                ^---------^ SC2039: In POSIX sh, 'local' is undefined.


In zfs-import.in line 117:
		local apools=""
                ^----------^ SC2039: In POSIX sh, 'local' is undefined.


In zfs-import.in line 147:
	if [ -n "$USE_DISK_BY_ID" -a -z "$ZPOOL_IMPORT_PATH" ]
                                  ^-- SC2166: Prefer [ p ] && [ q ] as [ p -a q ] is not well defined.


In zfs-import.in line 149:
		local dirs
                ^--------^ SC2039: In POSIX sh, 'local' is undefined.


In zfs-import.in line 156:
			echo -n "$dir:"
                             ^-- SC2039: In POSIX sh, echo flags are undefined.


In zfs-import.in line 225:
		if [ "$r" -gt 0 -a -f "$ZPOOL_CACHE" ]
                                ^-- SC2166: Prefer [ p ] && [ q ] as [ p -a q ] is not well defined.


In zfs-import.in line 248:
	[ -n "$already_imported" -a -z "$available_pools" ] && return 0
                                 ^-- SC2166: Prefer [ p ] && [ q ] as [ p -a q ] is not well defined.


In zfs-import.in line 255:
	check_boolean "${ZPOOL_IMPORT_ALL_VISIBLE}" && \
                                                    ^-- SC2015: Note that A && B || C is not if-then-else. C may run when A is true.


In zfs.in line 7:
ZFS_MOUNT='yes'
^-------^ SC2034: ZFS_MOUNT appears unused. Verify use (or export if used externally).


In zfs.in line 10:
ZFS_UNMOUNT='yes'
^---------^ SC2034: ZFS_UNMOUNT appears unused. Verify use (or export if used externally).


In zfs.in line 14:
ZFS_SHARE='yes'
^-------^ SC2034: ZFS_SHARE appears unused. Verify use (or export if used externally).


In zfs.in line 17:
ZFS_UNSHARE='yes'
^---------^ SC2034: ZFS_UNSHARE appears unused. Verify use (or export if used externally).


In zfs.in line 35:
ZPOOL_IMPORT_ALL_VISIBLE='no'
^----------------------^ SC2034: ZPOOL_IMPORT_ALL_VISIBLE appears unused. Verify use (or export if used externally).


In zfs.in line 58:
VERBOSE_MOUNT='no'
^-----------^ SC2034: VERBOSE_MOUNT appears unused. Verify use (or export if used externally).


In zfs.in line 63:
DO_OVERLAY_MOUNTS='no'
^---------------^ SC2034: DO_OVERLAY_MOUNTS appears unused. Verify use (or export if used externally).


In zfs.in line 70:
ZPOOL_IMPORT_OPTS=""
^---------------^ SC2034: ZPOOL_IMPORT_OPTS appears unused. Verify use (or export if used externally).


In zfs.in line 88:
MOUNT_EXTRA_OPTIONS=""
^-----------------^ SC2034: MOUNT_EXTRA_OPTIONS appears unused. Verify use (or export if used externally).


In zfs.in line 92:
ZFS_DKMS_ENABLE_DEBUG='no'
^-------------------^ SC2034: ZFS_DKMS_ENABLE_DEBUG appears unused. Verify use (or export if used externally).


In zfs.in line 96:
ZFS_DKMS_DISABLE_STRIP='no'
^--------------------^ SC2034: ZFS_DKMS_DISABLE_STRIP appears unused. Verify use (or export if used externally).


In zfs.in line 101:
ZFS_INITRD_PRE_MOUNTROOT_SLEEP='0'
^----------------------------^ SC2034: ZFS_INITRD_PRE_MOUNTROOT_SLEEP appears unused. Verify use (or export if used externally).


In zfs.in line 108:
ZFS_INITRD_POST_MODPROBE_SLEEP='0'
^----------------------------^ SC2034: ZFS_INITRD_POST_MODPROBE_SLEEP appears unused. Verify use (or export if used externally).


In zfs-mount.in line 1:
#!@SHELL@
^-------^ SC2239: Ensure the shebang uses an absolute path to the interpreter.


In zfs-mount.in line 37:
	while read line; do
              ^--^ SC2162: read without -r will mangle backslashes.


In zfs-mount.in line 38:
		set -- $line
                       ^---^ SC2086: Double quote to prevent globbing and word splitting.

Did you mean: 
		set -- "$line"


In zfs-mount.in line 68:
	local verbose overlay i mntpt val
        ^-- SC2039: In POSIX sh, 'local' is undefined.
                                      ^-^ SC2034: val appears unused. Verify use (or export if used externally).


In zfs-mount.in line 90:
		dev=$(eval echo "$"FSTAB_dev_$i)
                                 ^-- SC1135: Prefer escape over ending quote to make $ literal. Instead of "It costs $"5, use "It costs \$5".


In zfs-mount.in line 126:
	local i var mntpt
        ^---------------^ SC2039: In POSIX sh, 'local' is undefined.


In zfs-mount.in line 143:
		dev=$(eval echo "$"FSTAB_dev_$i)
                                 ^-- SC1135: Prefer escape over ending quote to make $ literal. Instead of "It costs $"5, use "It costs \$5".


In zfs-share.in line 1:
#!@SHELL@
^-------^ SC2239: Ensure the shebang uses an absolute path to the interpreter.


In zfs-zed.in line 1:
#!@SHELL@
^-------^ SC2239: Ensure the shebang uses an absolute path to the interpreter.


In zfs-zed.in line 35:
extra_started_commands="reload"
^--------------------^ SC2034: extra_started_commands appears unused. Verify use (or export if used externally).


In zfs-zed.in line 60:
	local pools RET
        ^-------------^ SC2039: In POSIX sh, 'local' is undefined.
                    ^-^ SC2034: RET appears unused. Verify use (or export if used externally).


In zfs-zed.in line 65:
	if [ "$?" -eq "0" ]
             ^--^ SC2181: Check exit code directly with e.g. 'if mycmd;', not indirectly with $?.

For more information:
  https://www.shellcheck.net/wiki/SC2239 -- Ensure the shebang uses an absolu...
  https://www.shellcheck.net/wiki/SC2034 -- DO_OVERLAY_MOUNTS appears unused....
  https://www.shellcheck.net/wiki/SC2039 -- In POSIX sh, $".." is undefined.

c0d3z3r0 · 2019-08-21T14:27:43Z

Current state of shellcheck on this PR
In zfs-zed.in line 1:

Can you wait with this until I have more stuff ready, please? This is WIP (work in progress).... I just finished zfs-functions but there is work to be done as you can see at the conversation above.

Fyi I ran shellcheck yesterday and already applied some stuff to zfs-functions in my last push.

In zfs-functions.in line 1:

This is a script with common functions etc used by zfs-import, zfs-mount,

^-- SC2148: Tips depend on target shell and yours is unknown. Add a shebang.

No. This script will NEVER run on it's own, so we do NOT wan't a shebang.

In zfs-functions.in line 29:
. @initconfdir@/zfs
^-- SC1091: Not following: @initconfdir@/zfs was not specified as input (see shellcheck -x).

In zfs-functions.in line 37:
. /etc/rc.d/init.d/functions
^-- SC1091: Not following: /etc/rc.d/init.d/functions was not specified as input (see shellcheck -x).

In zfs-functions.in line 40:
. /etc/init.d/functions.sh
^-- SC1091: Not following: /etc/init.d/functions.sh was not specified as input (see shellcheck -x).

In zfs-functions.in line 43:
. /lib/lsb/init-functions
^-- SC1091: Not following: /lib/lsb/init-functions was not specified as input (see shellcheck -x).

Shellcheck default... scripts will be check one-for-one.

In zfs-functions.in line 80:
[ "${1}" -eq 0 ] && success || failure
^-- SC2015: Note that A && B || C is not if-then-else. C may run when A is true.

In zfs-functions.in line 162:
[ "${ret}" -eq 0 ] && zfs_log_end_msg ${ret} ||
^-- SC2015: Note that A && B || C is not if-then-else. C may run when A is true.

Does not matter here.

In zfs-functions.in line 195:
[ -f "${pidfile}" -a -d /run/sendsigs.omit.d ] &&
^-- SC2166: Prefer [ p ] && [ q ] as [ p -a q ] is not well defined.

Not changed. Worked before and should be ok (I never new -a was not well defined anywhere. This may apply to vanilla POSIX only.

In zfs-functions.in line 355:
[ -n "${init}" ] && exit 3
^-- SC2154: init is referenced but not assigned.

Not sure but I guess init is defined elsewhere but it should be upercase then. Have to check.

In zfs-functions.in line 383:
IFS="\0" read -r dev _;
^-- SC1117: Backslash is literal in "\0". Prefer explicit escaping: "\0".

Huh? Yes, of course. This shall be \0.

In zfs-functions.in line 452:
local dev="$(parsearg dev "$@")"
^-- SC2155: Declare and assign separately to avoid masking return values.

In zfs-functions.in line 453:
local mntpt="$(parsearg mntpt "$@")"
^-- SC2155: Declare and assign separately to avoid masking return values.

In zfs-functions.in line 454:
local type="$(parsearg type "$@")"
^-- SC2155: Declare and assign separately to avoid masking return values.

In zfs-functions.in line 455:
local opts="$(parsearg opts "$@")"
^-- SC2155: Declare and assign separately to avoid masking return values.

In zfs-functions.in line 463:
local lines="$(
^-- SC2155: Declare and assign separately to avoid masking return values.

I guess shellcheck is confused because of using opts as string here.

In zfs-functions.in line 459:
echo "Invalid filter: '$*'" >&2
^-- SC1117: Backslash is literal in "'". Prefer explicit escaping: "\'".
^-- SC1117: Backslash is literal in "'". Prefer explicit escaping: "\'".

Nope. We do mean that literal.

Same as

In zfs-functions.in line 469:
[ $(printf "%s" "${lines}" | wc -l) -gt 1 ] &&
^-- SC2046: Quote this to prevent word splitting.

Does not matter here

gdevenyi · 2019-08-21T14:50:18Z

@c0d3z3r0 sorry, I updated my post to change how shellcheck is run, to enforce POSIXness and to allow it to import the other function files. I did grab your latest push as of when the check was run.

The shell this code is run against is /bin/sh which I understand could be dash or sh, but I think we won't know which, so we should stick to POSIX? Especially if we are integrating the BSDs into the repo eventually.

c0d3z3r0 · 2019-08-21T15:06:55Z

@c0d3z3r0 sorry, I updated my post to change how shellcheck is run, to enforce POSIXness and to allow it to import the other function files. I did grab your latest push as of when the check was run.

The shell this code is run against is /bin/sh which I understand could be dash or sh, but I think we won't know which, so we should stick to POSIX? Especially if we are integrating the BSDs into the repo eventually.

You can select the shell for shellcheck with -s. I guess we should check them all, even bash since some distros (arch linux for example) use bash in posix compatibility mode (calling it as "sh").
I am not sure if ksh is used anywhere is /bin/sh.

TL;DR yes we should stick to POSIX (but even POSIX compatible shells are not always implemented that strict in the shells; see the -a "problem" above).

gdevenyi · 2019-08-21T15:12:42Z

Great, the current status of my original comment includes an explicit "-s sh" in the shellcheck call, so it is enforcing strict POSIX.

PrivatePuffin · 2019-11-13T14:12:43Z

@gdevenyi would it be an idea to add shellcheck to the general test suit anyway?
In that case we could makeup an issue for it.

gdevenyi · 2019-11-13T14:42:14Z

@Ornias1993 if the rework makes it in, adding shellcheck to the testsuite could prevent bugs/inconsistencies creeping back in

PrivatePuffin · 2019-11-13T14:50:05Z

@gdevenyi Wouldn't shellcheck be a good thing in the test suite, regardless if this PR is merged or another form of rework is done in the future?

gdevenyi · 2019-11-13T15:07:22Z

@Ornias1993 yes, but its likely to throw a bunch of errors if added now, since the scripts haven't been cleaned up. Typically I've seen PRs for such checks held back until the scripts pass and then merged in together.

c0d3z3r0 force-pushed the for-upstream/shell_cleanup branch 3 times, most recently from 7755d81 to 424c55f Compare August 19, 2019 23:05

c0d3z3r0 changed the title ~~Completely rework shell scripts~~ [WIP] Completely rework shell scripts Aug 19, 2019

behlendorf added the Type: Defect Incorrect behavior (e.g. crash, hang) label Aug 20, 2019

ahrens added the Status: Code Review Needed Ready for review and testing label Aug 20, 2019

c0d3z3r0 force-pushed the for-upstream/shell_cleanup branch from 424c55f to 2570b92 Compare August 20, 2019 21:06

behlendorf mentioned this pull request Aug 20, 2019

Fix bug in in_mtab() where the return value is always 1 #9168

Merged

12 tasks

chrisrd reviewed Aug 21, 2019

View reviewed changes

behlendorf added Status: Work in Progress Not yet ready for general review and removed Status: Code Review Needed Ready for review and testing labels Aug 21, 2019

c0d3z3r0 force-pushed the for-upstream/shell_cleanup branch from 2570b92 to fa56501 Compare August 21, 2019 01:01

c0d3z3r0 force-pushed the for-upstream/shell_cleanup branch from fa56501 to 75d2194 Compare August 21, 2019 01:12

ahrens requested a review from jwk404 August 21, 2019 16:43

c0d3z3r0 closed this May 1, 2020


		if type start-stop-daemon > /dev/null 2>&1 ; then
		if do_noout type start-stop-daemon

[WIP] Completely rework shell scripts #9182

[WIP] Completely rework shell scripts #9182

Conversation

c0d3z3r0 commented Aug 19, 2019 • edited Loading

THIS IS WIP - more changes will follow, as stated in #9182 (comment)

Motivation and Context

Description

How Has This Been Tested?

Types of changes

Checklist:

gdevenyi commented Aug 19, 2019

c0d3z3r0 commented Aug 19, 2019

ahrens commented Aug 20, 2019

c0d3z3r0 commented Aug 20, 2019

chrisrd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisrd Aug 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

c0d3z3r0 commented Aug 21, 2019 • edited Loading

codecov bot commented Aug 21, 2019 • edited Loading

Codecov Report

gdevenyi commented Aug 21, 2019 • edited Loading

c0d3z3r0 commented Aug 21, 2019

This is a script with common functions etc used by zfs-import, zfs-mount,

gdevenyi commented Aug 21, 2019

c0d3z3r0 commented Aug 21, 2019

gdevenyi commented Aug 21, 2019

PrivatePuffin commented Nov 13, 2019

gdevenyi commented Nov 13, 2019

PrivatePuffin commented Nov 13, 2019

gdevenyi commented Nov 13, 2019

c0d3z3r0 commented Aug 19, 2019 •

edited

Loading

chrisrd Aug 21, 2019 •

edited

Loading

c0d3z3r0 commented Aug 21, 2019 •

edited

Loading

codecov bot commented Aug 21, 2019 •

edited

Loading

gdevenyi commented Aug 21, 2019 •

edited

Loading