Add python style checking

Introduce a make recipe for flake8 to enable python
style checking. Ensure all python scripts pass flake8.
Return an error code of 0 for arcstat.py -v and
dbufstat.py -v.  Add test cases for python scripts.

Reviewed by: Richard Laager <rlaager@wiktel.com>
Reviewed-by: Richard Elling <Richard.Elling@RichardElling.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Ian Lee <IanLee1521@gmail.com>
Closes #5230

Makefile.am

@@ -39,7 +39,7 @@ dist-hook:
 	sed -i 's/Release:[[:print:]]*/Release:      $(RELEASE)/' \
 		$(distdir)/META
 
-checkstyle: cstyle shellcheck
+checkstyle: cstyle shellcheck flake8
 
 cstyle:
 	@find ${top_srcdir} -name '*.[hc]' ! -name 'zfs_config.*' \
@@ -62,6 +62,11 @@ cppcheck:
 		cppcheck --quiet --force --error-exitcode=2 ${top_srcdir}; \
 	fi
 
+flake8:
+	@if type flake8 > /dev/null 2>&1; then \
+		flake8 ${top_srcdir}; \
+	fi
+
 ctags:
 	$(RM) tags
 	find $(top_srcdir) -name .git -prune -o -name '*.[hc]' | xargs ctags

cmd/arc_summary/arc_summary.py

@@ -66,22 +66,6 @@ def get_Kstat():
             name, unused, value = kstat.split()
             Kstat[namespace + name] = D(value)
 
-    Kstats = [
-        "hw.pagesize",
-        "hw.physmem",
-        "kern.maxusers",
-        "vm.kmem_map_free",
-        "vm.kmem_map_size",
-        "vm.kmem_size",
-        "vm.kmem_size_max",
-        "vm.kmem_size_min",
-        "vm.kmem_size_scale",
-        "vm.stats",
-        "vm.swap_total",
-        "vm.swap_reserved",
-        "kstat.zfs",
-        "vfs.zfs"
-    ]
     Kstat = {}
     load_proc_kstats('/proc/spl/kstat/zfs/arcstats',
                      'kstat.zfs.misc.arcstats.')
@@ -92,6 +76,7 @@ def get_Kstat():
 
     return Kstat
 
+
 def div1():
     sys.stdout.write("\n")
     for i in range(18):
@@ -188,17 +173,17 @@ def get_arc_summary(Kstat):
 
     output['memory_throttle_count'] = fHits(memory_throttle_count)
 
-    ### ARC Misc. ###
+    # ARC Misc.
     deleted = Kstat["kstat.zfs.misc.arcstats.deleted"]
     mutex_miss = Kstat["kstat.zfs.misc.arcstats.mutex_miss"]
 
-    ### ARC Misc. ###
+    # ARC Misc.
     output["arc_misc"] = {}
     output["arc_misc"]["deleted"] = fHits(deleted)
     output["arc_misc"]['mutex_miss'] = fHits(mutex_miss)
     output["arc_misc"]['evict_skips'] = fHits(mutex_miss)
 
-    ### ARC Sizing ###
+    # ARC Sizing
     arc_size = Kstat["kstat.zfs.misc.arcstats.size"]
     mru_size = Kstat["kstat.zfs.misc.arcstats.p"]
     target_max_size = Kstat["kstat.zfs.misc.arcstats.c_max"]
@@ -207,7 +192,7 @@ def get_arc_summary(Kstat):
 
     target_size_ratio = (target_max_size / target_min_size)
 
-    ### ARC Sizing ###
+    # ARC Sizing
     output['arc_sizing'] = {}
     output['arc_sizing']['arc_size'] = {
         'per': fPerc(arc_size, target_max_size),
@@ -226,7 +211,7 @@ def get_arc_summary(Kstat):
         'num': fBytes(target_size),
     }
 
-    ### ARC Hash Breakdown ###
+    # ARC Hash Breakdown
     output['arc_hash_break'] = {}
     output['arc_hash_break']['hash_chain_max'] = Kstat[
         "kstat.zfs.misc.arcstats.hash_chain_max"
@@ -267,7 +252,7 @@ def get_arc_summary(Kstat):
             'num': fBytes(mfu_size),
         }
 
-    ### ARC Hash Breakdown ###
+    # ARC Hash Breakdown
     hash_chain_max = Kstat["kstat.zfs.misc.arcstats.hash_chain_max"]
     hash_chains = Kstat["kstat.zfs.misc.arcstats.hash_chains"]
     hash_collisions = Kstat["kstat.zfs.misc.arcstats.hash_collisions"]
@@ -288,7 +273,7 @@ def get_arc_summary(Kstat):
 
 
 def _arc_summary(Kstat):
-    ### ARC Sizing ###
+    # ARC Sizing
     arc = get_arc_summary(Kstat)
 
     sys.stdout.write("ARC Summary: (%s)\n" % arc['health'])
@@ -297,7 +282,7 @@ def _arc_summary(Kstat):
                      arc['memory_throttle_count'])
     sys.stdout.write("\n")
 
-    ### ARC Misc. ###
+    # ARC Misc.
     sys.stdout.write("ARC Misc:\n")
     sys.stdout.write("\tDeleted:\t\t\t\t%s\n" % arc['arc_misc']['deleted'])
     sys.stdout.write("\tMutex Misses:\t\t\t\t%s\n" %
@@ -306,7 +291,7 @@ def _arc_summary(Kstat):
                      arc['arc_misc']['mutex_miss'])
     sys.stdout.write("\n")
 
-    ### ARC Sizing ###
+    # ARC Sizing
     sys.stdout.write("ARC Size:\t\t\t\t%s\t%s\n" % (
         arc['arc_sizing']['arc_size']['per'],
         arc['arc_sizing']['arc_size']['num']
@@ -344,7 +329,7 @@ def _arc_summary(Kstat):
 
     sys.stdout.write("\n")
 
-    ### ARC Hash Breakdown ###
+    # ARC Hash Breakdown
     sys.stdout.write("ARC Hash Breakdown:\n")
     sys.stdout.write("\tElements Max:\t\t\t\t%s\n" %
                      arc['arc_hash_break']['elements_max'])
@@ -875,7 +860,8 @@ def _tunable_summary(Kstat):
 
     values = {}
     for name in names:
-        with open("/sys/module/zfs/parameters/" + name) as f: value = f.read()
+        with open("/sys/module/zfs/parameters/" + name) as f:
+            value = f.read()
         values[name] = value.strip()
 
     descriptions = {}
@@ -950,13 +936,15 @@ def usage():
     sys.stdout.write("\t -p PAGE, --page=PAGE : "
                      "Select a single output page to display,\n")
     sys.stdout.write("\t                        "
-            "should be an integer between 1 and " + str(len(unSub)) + "\n\n")
+                     "should be an integer between 1 and " +
+                     str(len(unSub)) + "\n\n")
     sys.stdout.write("Examples:\n")
     sys.stdout.write("\tarc_summary.py -a\n")
     sys.stdout.write("\tarc_summary.py -p 4\n")
     sys.stdout.write("\tarc_summary.py -ad\n")
     sys.stdout.write("\tarc_summary.py --page=2\n")
 
+
 def main():
     global show_tunable_descriptions
     global alternate_tunable_layout
@@ -987,7 +975,7 @@ def main():
     if 'p' in args:
         try:
             pages.append(unSub[int(args['p']) - 1])
-        except IndexError as e:
+        except IndexError:
             sys.stderr.write('the argument to -p must be between 1 and ' +
                              str(len(unSub)) + '\n')
             sys.exit()

cmd/arcstat/arcstat.py

@@ -122,7 +122,7 @@ def detailed_usage():
         sys.stderr.write("%11s : %s\n" % (key, cols[key][2]))
     sys.stderr.write("\n")
 
-    sys.exit(1)
+    sys.exit(0)
 
 
 def usage():
@@ -229,15 +229,19 @@ def print_header():
         sys.stdout.write("%*s%s" % (cols[col][0], col, sep))
     sys.stdout.write("\n")
 
+
 def get_terminal_lines():
     try:
-        import fcntl, termios, struct
+        import fcntl
+        import termios
+        import struct
         data = fcntl.ioctl(sys.stdout.fileno(), termios.TIOCGWINSZ, '1234')
         sz = struct.unpack('hh', data)
         return sz[0]
     except:
         pass
 
+
 def update_hdr_intr():
     global hdr_intr
 
@@ -245,6 +249,7 @@ def update_hdr_intr():
     if lines and lines > 3:
         hdr_intr = lines - 3
 
+
 def resize_handler(signum, frame):
     update_hdr_intr()
 

cmd/dbufstat/dbufstat.py

@@ -143,7 +143,7 @@ def detailed_usage():
         sys.stderr.write("%11s : %s\n" % (key, cols[key][2]))
     sys.stderr.write("\n")
 
-    sys.exit(1)
+    sys.exit(0)
 
 
 def usage():

tests/runfiles/linux.run

@@ -355,7 +355,8 @@ tests = ['zdb_001_neg', 'zfs_001_neg', 'zfs_allow_001_neg',
     'zpool_history_001_neg', 'zpool_import_001_neg', 'zpool_import_002_neg',
     'zpool_offline_001_neg', 'zpool_online_001_neg', 'zpool_remove_001_neg',
     'zpool_replace_001_neg', 'zpool_scrub_001_neg', 'zpool_set_001_neg',
-    'zpool_status_001_neg', 'zpool_upgrade_001_neg']
+    'zpool_status_001_neg', 'zpool_upgrade_001_neg', 'arcstat_001_pos',
+    'arc_summary_001_pos', 'dbufstat_001_pos']
 user =
 
 [tests/functional/cli_user/zfs_list]

tests/test-runner/cmd/test-runner.py

@@ -127,7 +127,7 @@ class Cmd(object):
         self.killed = False
         self.result = Result()
 
-        if self.timeout == None:
+        if self.timeout is None:
             self.timeout = 60
 
     def __str__(self):
@@ -309,7 +309,8 @@ class Test(Cmd):
         if len(self.post_user):
             post_user = ' (as %s)' % (self.post_user)
         return "Pathname: %s\nOutputdir: %s\nTimeout: %d\nPre: %s%s\nPost: " \
-               "%s%s\nUser: %s\n" % (self.pathname, self.outputdir,
+               "%s%s\nUser: %s\n" % (
+                   self.pathname, self.outputdir,
                    self.timeout, self.pre, pre_user, self.post, post_user,
                    self.user)
 
@@ -384,9 +385,9 @@ class TestGroup(Test):
         if len(self.post_user):
             post_user = ' (as %s)' % (self.post_user)
         return "Pathname: %s\nOutputdir: %s\nTests: %s\nTimeout: %d\n" \
-               "Pre: %s%s\nPost: %s%s\nUser: %s\n" % (self.pathname,
+            "Pre: %s%s\nPost: %s%s\nUser: %s\n" % (
-                self.outputdir, self.tests, self.timeout, self.pre, pre_user,
+                self.pathname, self.outputdir, self.tests, self.timeout,
-                self.post, post_user, self.user)
+                self.pre, pre_user, self.post, post_user, self.user)
 
     def verify(self, logger):
         """
@@ -428,8 +429,8 @@ class TestGroup(Test):
             if not verify_file(os.path.join(self.pathname, test)):
                 del self.tests[self.tests.index(test)]
                 logger.info("Warning: Test '%s' removed from TestGroup '%s' "
-                            "because it failed verification." % (test,
+                            "because it failed verification." %
-                            self.pathname))
+                            (test, self.pathname))
 
         return len(self.tests) is not 0
 
@@ -634,7 +635,7 @@ class TestRun(object):
             components -= 1
             for testfile in tmp_dict.keys():
                 uniq = '/'.join(testfile.split('/')[components:]).lstrip('/')
-                if not uniq in l:
+                if uniq not in l:
                     l.append(uniq)
                     tmp_dict[testfile].outputdir = os.path.join(base, uniq)
                 else:
@@ -705,8 +706,8 @@ class TestRun(object):
         m, s = divmod(time() - self.starttime, 60)
         h, m = divmod(m, 60)
         print '\nRunning Time:\t%02d:%02d:%02d' % (h, m, s)
-        print 'Percent passed:\t%.1f%%' % ((float(Result.runresults['PASS']) /
+        print 'Percent passed:\t%.1f%%' % (
-               float(Result.total)) * 100)
+            (float(Result.runresults['PASS']) / float(Result.total)) * 100)
         print 'Log directory:\t%s' % self.outputdir
 
 
@@ -717,9 +718,9 @@ def verify_file(pathname):
     if os.path.isdir(pathname) or os.path.islink(pathname):
         return False
 
-    if (os.path.isfile(pathname) and os.access(pathname, os.X_OK)) or \
+    for ext in '', '.ksh', '.sh':
-            (os.path.isfile(pathname+'.ksh') and os.access(pathname+'.ksh', os.X_OK)) or \
+        script_path = pathname + ext
-            (os.path.isfile(pathname+'.sh') and os.access(pathname+'.sh', os.X_OK)):
+        if os.path.isfile(script_path) and os.access(script_path, os.X_OK):
             return True
 
     return False
@@ -731,15 +732,13 @@ def verify_user(user, logger):
     sudo without being prompted for a password.
     """
     testcmd = [SUDO, '-n', '-u', user, TRUE]
-    can_sudo = exists = True
 
     if user in Cmd.verified_users:
         return True
 
     try:
-        _ = getpwnam(user)
+        getpwnam(user)
     except KeyError:
-        exists = False
         logger.info("Warning: user '%s' does not exist.", user)
         return False
 

tests/zfs-tests/include/default.cfg.in

@@ -43,6 +43,9 @@ export ZPOOL=${ZPOOL:-${sbindir}/zpool}
 export ZTEST=${ZTEST:-${sbindir}/ztest}
 export ZPIOS=${ZPIOS:-${sbindir}/zpios}
 export RAIDZ_TEST=${RAIDZ_TEST:-${bindir}/raidz_test}
+export ARC_SUMMARY=${ARC_SUMMARY:-${bindir}/arc_summary.py}
+export ARCSTAT=${ARCSTAT:-${bindir}/arcstat.py}
+export DBUFSTAT=${DBUFSTAT:-${bindir}/dbufstat.py}
 
 . $STF_SUITE/include/libtest.shlib
 

tests/zfs-tests/tests/functional/cli_user/misc/Makefile.am

@@ -43,4 +43,7 @@ dist_pkgdata_SCRIPTS = \
 	zpool_scrub_001_neg.ksh \
 	zpool_set_001_neg.ksh \
 	zpool_status_001_neg.ksh \
-	zpool_upgrade_001_neg.ksh
+	zpool_upgrade_001_neg.ksh \
+	arcstat_001_pos.ksh \
+	arc_summary_001_pos.ksh \
+	dbufstat_001_pos.ksh

tests/zfs-tests/tests/functional/cli_user/misc/arc_summary_001_pos.ksh

@@ -0,0 +1,40 @@
+#! /bin/ksh -p
+#
+# CDDL HEADER START
+#
+# The contents of this file are subject to the terms of the
+# Common Development and Distribution License (the "License").
+# You may not use this file except in compliance with the License.
+#
+# You can obtain a copy of the license at usr/src/OPENSOLARIS.LICENSE
+# or http://www.opensolaris.org/os/licensing.
+# See the License for the specific language governing permissions
+# and limitations under the License.
+#
+# When distributing Covered Code, include this CDDL HEADER in each
+# file and include the License file at usr/src/OPENSOLARIS.LICENSE.
+# If applicable, add the following below this CDDL HEADER, with the
+# fields enclosed by brackets "[]" replaced with your own identifying
+# information: Portions Copyright [yyyy] [name of copyright owner]
+#
+# CDDL HEADER END
+#
+
+#
+# Copyright (c) 2015 by Lawrence Livermore National Security, LLC.
+# All rights reserved.
+#
+
+. $STF_SUITE/include/libtest.shlib
+
+set -A args  "" "-a" "-d" "-p 1"
+
+log_assert "arc_summary.py generates output and doesn't return an error code"
+
+typeset -i i=0
+while [[ $i -lt ${#args[*]} ]]; do
+        log_must eval "$ARC_SUMMARY ${args[i]} > /dev/null"
+        ((i = i + 1))
+done
+
+log_pass "arc_summary.py generates output and doesn't return an error code"

tests/zfs-tests/tests/functional/cli_user/misc/arcstat_001_pos.ksh

@@ -0,0 +1,41 @@
+#! /bin/ksh -p
+#
+# CDDL HEADER START
+#
+# The contents of this file are subject to the terms of the
+# Common Development and Distribution License (the "License").
+# You may not use this file except in compliance with the License.
+#
+# You can obtain a copy of the license at usr/src/OPENSOLARIS.LICENSE
+# or http://www.opensolaris.org/os/licensing.
+# See the License for the specific language governing permissions
+# and limitations under the License.
+#
+# When distributing Covered Code, include this CDDL HEADER in each
+# file and include the License file at usr/src/OPENSOLARIS.LICENSE.
+# If applicable, add the following below this CDDL HEADER, with the
+# fields enclosed by brackets "[]" replaced with your own identifying
+# information: Portions Copyright [yyyy] [name of copyright owner]
+#
+# CDDL HEADER END
+#
+
+#
+# Copyright (c) 2015 by Lawrence Livermore National Security, LLC.
+# All rights reserved.
+#
+
+. $STF_SUITE/include/libtest.shlib
+
+set -A args  "" "-s \",\"" "-x" "-v" \
+    "-f time,hit%,dh%,ph%,mh%"
+
+log_assert "arcstat.py generates output and doesn't return an error code"
+
+typeset -i i=0
+while [[ $i -lt ${#args[*]} ]]; do
+        log_must eval "$ARCSTAT ${args[i]} > /dev/null"
+        ((i = i + 1))
+done
+log_pass "arcstat.py generates output and doesn't return an error code"
+

tests/zfs-tests/tests/functional/cli_user/misc/dbufstat_001_pos.ksh

@@ -0,0 +1,40 @@
+#! /bin/ksh -p
+#
+# CDDL HEADER START
+#
+# The contents of this file are subject to the terms of the
+# Common Development and Distribution License (the "License").
+# You may not use this file except in compliance with the License.
+#
+# You can obtain a copy of the license at usr/src/OPENSOLARIS.LICENSE
+# or http://www.opensolaris.org/os/licensing.
+# See the License for the specific language governing permissions
+# and limitations under the License.
+#
+# When distributing Covered Code, include this CDDL HEADER in each
+# file and include the License file at usr/src/OPENSOLARIS.LICENSE.
+# If applicable, add the following below this CDDL HEADER, with the
+# fields enclosed by brackets "[]" replaced with your own identifying
+# information: Portions Copyright [yyyy] [name of copyright owner]
+#
+# CDDL HEADER END
+#
+
+#
+# Copyright (c) 2015 by Lawrence Livermore National Security, LLC.
+# All rights reserved.
+#
+
+. $STF_SUITE/include/libtest.shlib
+
+set -A args  "" "-b" "-d" "-r" "-v" "-s \",\"" "-x"
+
+log_assert "dbufstat.py generates output and doesn't return an error code"
+
+typeset -i i=0
+while [[ $i -lt ${#args[*]} ]]; do
+        log_must eval "$DBUFSTAT ${args[i]} > /dev/null"
+        ((i = i + 1))
+done
+
+log_pass "dbufstat.py generates output and doesn't return an error code"

zfs-script-config.sh.in

@@ -29,6 +29,9 @@ export ZPOOL=${CMDDIR}/zpool/zpool
 export ZTEST=${CMDDIR}/ztest/ztest
 export ZPIOS=${CMDDIR}/zpios/zpios
 export RAIDZ_TEST=${CMDDIR}/raidz_test/raidz_test
+export ARC_SUMMARY=${CMDDIR}/arc_summary/arc_summary.py
+export ARCSTAT=${CMDDIR}/arcstat/arcstat.py
+export DBUFSTAT=${CMDDIR}/dbufstat/dbufstat.py
 
 export COMMON_SH=${SCRIPTDIR}/common.sh
 export ZFS_SH=${SCRIPTDIR}/zfs.sh