Custom column names and scripts for zpool status/iostat -c #5852

tonyhutter · 2017-03-01T20:08:41Z

Description

This patch updates the zpool status/iostat -c option to only run "pre-baked" scripts from the /usr/local/etc/zfs/zpool.d directory (or wherever you install to). The scripts can only be run from -c as an unprivileged user. This was done mainly for security reasons. If you script needs to run a privileged command, consider adding the appropriate line in /etc/sudoers. See zpool(8) for an example of how to do this.

The patch also allows the scripts to output custom column names. If the script outputs a line like:

name=value

then "name" is used for the column name, and "value" is its value. Multiple columns can be specified by outputting multiple lines. Column names and values can have spaces. If the value is empty, a dash (-) is printed instead.

After all the "name=value" lines are read (if any), zpool will take the next the next line of output (if any) and print it without a column header. After that, no more lines will be processed. This can be useful for printing errors.

Example output of running four scripts:

$ zpool iostat -vc vendor,model,enc,slot
                         capacity     operations     bandwidth
pool                   alloc   free   read  write   read  write   vendor         model       enc  slot
---------------------  -----  -----  -----  -----  -----  -----  -------  ------------  --------  ----
tank                   20.4G  7.23T      0      8  44.6K  1020K
  mirror               20.4G  7.23T      0      8  44.6K  1020K
    24050438227969495      -      -      0      0      0      0  SEAGATE  ST8000NM0075   0:0:1:0     2
    U10                    -      -      0      0    566  13.1K  SEAGATE  ST8000NM0075   0:0:1:0    11
    U11                    -      -      0      0    913  13.1K  SEAGATE  ST8000NM0075   0:0:1:0    12
    U12                    -      -      0      0    486  13.1K  SEAGATE  ST8000NM0075   0:0:1:0    13
    U13                    -      -      0      0    504  13.1K  SEAGATE  ST8000NM0075   0:0:1:0    14
...

Lastly, this patch also disables the -c option with the latency and request size histograms, since it produced awkward output and made the code harder to maintain.

Motivation and Context

Make the -c option more secure.
Create pre-baked scripts to make looking up common disk stats easier

How Has This Been Tested?

Tested by hand and updated automated test scripts.

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)

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.
Change has been approved by a ZFS on Linux member.

mention-bot · 2017-03-01T20:08:46Z

@tonyhutter, thanks for your PR! By analyzing the history of the files in this pull request, we identified @behlendorf, @inkdot7 and @ryao to be potential reviewers.

tonyhutter · 2017-03-01T21:54:55Z

Lint is reporting some memory leaks, let me fix those.

behlendorf · 2017-03-01T22:26:12Z

You can fix the kernel.org build issues with a rebase on master too.

tonyhutter · 2017-03-02T16:52:52Z

Ok, I fixed the memory leak and rebased.

tonyhutter · 2017-03-02T19:13:59Z

Open question for everyone:
The proposed patch prints the user's column titles as-is, but the convention for zpool status is to capitalize column titles. Should zpool status capitalize titles and/or substitute column spaces for underscores? Examples:

Current patch:

$ zpool status -c 'echo col1=val1; echo vdev path=$VDEV_PATH'
...
NAME        STATE     READ WRITE CKSUM  col1  vdev path
mypool      ONLINE       0     0     0
  mirror-0  ONLINE       0     0     0
    sdb     ONLINE       0     0     0  val1   /dev/sdb
    sdc     ONLINE       0     0     0  val1   /dev/sdc

Capitalize column names:

$ zpool status -c 'echo col1=val1; echo vdev path=$VDEV_PATH'
...
NAME        STATE     READ WRITE CKSUM  COL1  VDEV PATH
mypool      ONLINE       0     0     0
  mirror-0  ONLINE       0     0     0
    sdb     ONLINE       0     0     0  val1   /dev/sdb
    sdc     ONLINE       0     0     0  val1   /dev/sdc

Capitalize w/underscores:

$ zpool status -c 'echo col1=val1; echo vdev path=$VDEV_PATH'
...
NAME        STATE     READ WRITE CKSUM  COL1  VDEV_PATH
mypool      ONLINE       0     0     0
  mirror-0  ONLINE       0     0     0
    sdb     ONLINE       0     0     0  val1   /dev/sdb
    sdc     ONLINE       0     0     0  val1   /dev/sdc

chrisrd · 2017-03-03T05:11:06Z

I think leave as-is, i.e. no title munging: if the user wants capitals etc. they can specify them.

behlendorf · 2017-03-03T00:23:46Z

cmd/zpool/zpool_iter.c

+	char *val = line;
+	char *equals;
+
+	if (!line)


nit: The preferred style is line == NULL, style guide page 25.

behlendorf · 2017-03-03T00:39:34Z

cmd/zpool/zpool_iter.c

+	line = NULL;
+	do {
+		/* Save the first line of output from the command */
+		if (getline(&line, &len, fp) != -1) {


getline was originally a GNU extension but it looks like it was added to POSIX.1-2008 and FreeBSD and Illumos both have an implementation so this shouldn't case any compatibility issues.

behlendorf · 2017-03-03T00:43:30Z

cmd/zpool/zpool_iter.c

+		if (data->lines == NULL)
+			return (0);
+
+


nit: extra newline

behlendorf · 2017-03-03T00:47:56Z

cmd/zpool/zpool_iter.c

+ * When running 'zpool iostat|status -c' the lines of output can either be
+ * in the form of:
+ *
+ * 	column_name=value


Extra leading space before tab

behlendorf · 2017-03-03T17:45:07Z

cmd/zpool/zpool_iter.c

+
+	if (val != NULL) {
+		data->lines = realloc(data->lines,
+		    (data->lines_cnt + 1) * sizeof (*data->lines));


Using realloc() in this way results in a memory leak on failure. When NULL is returned we'll overwrite the pointer to the allocated block and it won't be freed by realloc().

behlendorf · 2017-03-03T17:57:05Z

cmd/zpool/zpool_iter.c

@@ -504,12 +592,22 @@ all_pools_for_each_vdev_run(int argc, char **argv, char *cmd,
 void
 free_vdev_cmd_data_list(vdev_cmd_data_list_t *vcdl)
 {
-	int i;
+	int i, j;
 	for (i = 0; i < vcdl->count; i++) {


nit: We're now able to switch the c99 style usage here for the variables. It'd be nice to switch to this usage in functions touched by this patch.

for (int i = 0; i < vcdl->count; i++) {

behlendorf · 2017-03-03T18:01:59Z

man/man8/zpool.8

-vars are set before running each command:
+display the output in zpool iostat.  If the output is in the form of
+"name=value", then the column name is set to "name" and the value is set to
+"value".  Multiple lines can be use to output multiple columns.  If the


s/use/used/

behlendorf · 2017-03-03T18:02:23Z

man/man8/zpool.8

+output is not in this format, then the first line of output is displayed
+without a column title.
+
+The following environment vars are set before running each command:


s/vars/variables/

behlendorf · 2017-03-03T18:02:54Z

man/man8/zpool.8

-vars are set before running each command:
+display the output in zpool iostat.  If the output is in the form of
+"name=value", then the column name is set to "name" and the value is set to
+"value".  Multiple lines can be use to output multiple columns.  If the


s/use/used/

behlendorf · 2017-03-03T18:03:08Z

man/man8/zpool.8

+output is not in this format, then the first line of output is displayed
+without a column title.
+
+The following environment vars are set before running each command:


s/vars/variables/

behlendorf · 2017-03-03T18:05:05Z

I think as-is the right way to go here as well. This gives users the most flexibly to specify exactly what they want without us second guessing them.

tonyhutter · 2017-03-08T01:00:09Z

misc updates:

My latest push fixed all @behlendorf's comments
I need to deal with the issue where vdevs don't all output the same column names. This could happen if you're running smartctl on a pool with mixed disks, since the smartctl output is different for SATA vs SAS. Right now zpool blindly takes the column names from the 1st vdev's results:


zpool status -c 'if [ "$VDEV_UPATH" == "/dev/sdb" ] ; then echo "1st_vdev=1st" ; else echo "2nd_vdev=2nd" ; fi'
  pool: tank
 state: ONLINE
  scan: none requested
config:

    NAME        STATE     READ WRITE CKSUM  1st_vdev
    tank        ONLINE       0     0     0
      mirror-0  ONLINE       0     0     0
        sdb     ONLINE       0     0     0       1st
        sdc     ONLINE       0     0     0       2nd

What we really want:

    NAME        STATE     READ WRITE CKSUM  1st_vdev  2nd_vdev
    tank        ONLINE       0     0     0
      mirror-0  ONLINE       0     0     0
        sdb     ONLINE       0     0     0       1st
        sdc     ONLINE       0     0     0                 2nd

I'll fix this in the next push.

@chrisrd bought up some good points about potential security issues with -c (Add -c to zpool iostat & status to run command, remove libdevmapper requirement #5368 (comment)). I've been going over them in my head and have some thoughts on it:

We should never set SUID on the 'zpool' binary! If set, the user could run anything they want as root with -c. In fact, we should probably error out if someone trys to use -c with SUID set.

People need to sanitize input to zpool. If you have a webserver querying zpool status don't do this!:

zpool status $poolname_from_webserver

...because someone will inevitably set $poolname_from_webserver="-c 'rm -fr /'". You could sanitize it with:

zpool status -c true $poolname_from_webserver

...so you'd hit double -c's and error out. Currently, the double -c actually doesn't error out, and instead just uses the 2nd -c, so I need to fix this in my next push.

chrisrd · 2017-03-15T03:42:30Z

Maybe a -- to indicate "end of options", so you can zpool status -- "$poolname_from_webserver"? That has the advantage of being (semi-?)standard, e.g. man 3 getopt.

I'd still prefer running a single command(/script) via fork/exec rather than having arbitrary shell commands passed in on the command line - using either the hard-coded dir you spoke of in #5368 or my suggested ZFS_PATH/PATH version (where an suid zpool only looks at ZFS_PATH and non-suid looks at ZFS_PATH then PATH).

chrisrd · 2017-03-16T00:53:01Z

Sigh. What was I thinking?? Of course an suid zpool needs to use a hard-coded dir.

tonyhutter · 2017-03-18T00:24:15Z

@chrisrd I talked to @behlendorf offline and we decided to go the fork/exec route you suggested, for all the reasons you mentioned. So -c will only run scripts from /etc/zfs/zpool.d. In fact we're going to take it a step further and only allow non-privilaged users to run the scripts. If a script needs to run a privileged command (like smartctl) then admins will need to add smartctl -a to their /etc/sudoers for the user, like:

username ALL=NOPASSWD: /usr/sbin/smartctl -a /dev/sd[a-z]*, NOEXEC: /usr/sbin/smartctl -a /dev/sd[a-z]*

I'm not sure how I feel about having a $ZFS_PATH. I'll need to give it some thought. In general, I'd rather error on the side of paranoid just to get this code into the 0.7.0 release. After that, we can add features to it after we've convinced ourselves that they're safe.

I do like the -- idea though.

behlendorf · 2017-03-28T20:27:21Z

cmd/zpool/zpool.d/iostat

+		continue
+	fi
+	echo "${cols[$i]}=${vals[$i]}"
+done


Let's make sure these scripts pass shellcheck cleanly by adding this directory to the top level shellcheck make target. Running it locally does uncover a few small issues.

behlendorf · 2017-03-28T20:34:50Z

cmd/zpool/zpool_iter.c

+ * names and their widths.  When this function is done, vcdl->uniq_cols,
+ * vcdl->uniq_cols_cnt, and vcdl->uniq_cols_width will be filled in.
+ */
+static void process_unique_cmd_columns(vdev_cmd_data_list_t *vcdl)


[nit] static void should be on it's own line.

behlendorf · 2017-03-28T20:43:43Z

cmd/zpool/zpool_iter.c

+		close(link[1]);
+		if (asprintf(&env[0], "PATH=/bin:/sbin:/usr/bin:/usr/sbin")
+		    == -1)
+		goto end;


Let's use a local variable to hold the return value throughout this block of code to make it more readable. For example.

error = asprintf(&env[0], "PATH=/bin:/sbin:/usr/bin:/usr/sbin"); if (error == -1) goto end;

behlendorf · 2017-03-28T20:44:58Z

cmd/zpool/zpool_iter.c

+
+		if (asprintf(&env[1], "VDEV_PATH=%s",
+		    data->path ? data->path : "") == -1)
+			goto end;


[style] out, not end is the label most commonly used throughout the code base for this kind of cleanup. Let's switch to that.

behlendorf · 2017-03-28T21:18:55Z

cmd/zpool/zpool_iter.c

-	    data->cmd) >= sizeof (cmd)) {
-		/* Our string was truncated */
+	char *argv[2] = {cmd, 0};
+	char *env[5] = {0}; /* Four env vars + NULL */


You could define the hard coded path here which is static.

char *env[5] = { "PATH=/bin:/sbin:/usr/bin:/usr/sbin", NULL, NULL, NULL, NULL };

behlendorf · 2017-03-28T21:30:26Z

cmd/zpool/zpool_main.c

+				fprintf(stderr, gettext(
+				    "Missing script for -c.  Current scripts "
+				    "in %s:\n"),
+				    ZPOOL_SCRIPTS_DIR);


How about not framing this as an error and just printing the available scripts. It would be really convenient if we could provide a brief description extracted from the script, maybe by invoking it with a -h option. For example:

Available scripts: enc Print SCSI Enclosure Services (SES) info. iostat Display most relevant iostat bandwidth/latency numbers. ...

We should probably also restrict the entries returned to file which are executable.

behlendorf

The commit comment should reference the /etc/zfs/zpool.d directory which is what users will use.

behlendorf · 2017-04-10T18:06:16Z

cmd/zpool/zpool.d/ses

@@ -0,0 +1,51 @@
+#!/bin/sh


nit: You'll want to add an empty line here for consistency with the other scripts.

behlendorf · 2017-04-10T18:09:23Z

cmd/zpool/zpool_iter.c

+			for (k = 0; k < cnt; k++) {
+				if (strcmp(data->cols[j], uniq_cols[k]) == 0)
+					break; /* yes it is */
+


nit: You've got an extra empty line here which needs to be removed.

behlendorf · 2017-04-10T18:09:44Z

cmd/zpool/zpool_iter.c

+		data = &vcdl->data[i];
+		/* For each column the vdev reported */
+		for (int j = 0; j < data->cols_cnt; j++) {
+			/* Is this column in our list of uniq col names? */


Let's phrase this to avoid abbreviations.

s/uniq/unique
s/col/column

behlendorf · 2017-04-10T18:10:19Z

cmd/zpool/zpool_iter.c

+	}
+
+	/*
+	 * We now have a list of all the uniq col names.  Figure out the max


Let's phrase this to avoid abbreviations.

s/uniq/unique
s/col/column

behlendorf · 2017-04-10T18:14:35Z

cmd/zpool/zpool_iter.c

+ *
+ * Or just:
+ *
+ * 	value


Leading space before tab should be removed.

behlendorf · 2017-04-10T19:39:33Z

cmd/zpool/zpool_iter.c

@@ -463,6 +684,7 @@ all_pools_for_each_vdev_run_vcdl(vdev_cmd_data_list_t *vcdl)
 	taskq_wait(t);
 	taskq_destroy(t);
 	thread_fini();
+


nit: stray empty line

behlendorf · 2017-04-10T19:40:39Z

cmd/zpool/zpool_main.c

 #include <zfs_prop.h>
 #include <sys/fs/zfs.h>
 #include <sys/stat.h>
 #include <sys/fm/fs/zfs.h>
 #include <sys/fm/util.h>
 #include <sys/fm/protocol.h>
 #include <sys/zfs_ioctl.h>
+
 #include <math.h>


Let's move this up before the <sys/*> entries.

behlendorf · 2017-04-10T19:44:26Z

cmd/zpool/zpool_main.c

@@ -3963,11 +4069,89 @@ fsleep(float sec)
 	nanosleep(&req, NULL);
 }

+/* Print a "zpool status/iostat -c" script's help (-h) output. */
+static void
+print_zpool_script_help(char *name, char *path)


Let's make sure vdev_run_cmd() and print_zpool_script_help use the same shared function for fork/exec. Ideally, and updated/modified more generic version of the existing libzfs_run_process() would be nice.

behlendorf · 2017-04-10T19:47:06Z

cmd/zpool/zpool_main.c

+	DIR *dir;
+	struct dirent *ent;
+	char fullpath[MAXPATHLEN];
+	struct stat dir_stat;


nit: missing newline

behlendorf · 2017-04-10T19:49:15Z

cmd/zpool/zpool_main.c

+		printf("\n");
+		closedir(dir);
+	} else {
+		fprintf(stderr, gettext("ERROR: Can't open %s scripts dir\n"),


No need for the ERROR: here which differs from all the other error messages. Please remove it.

tonyhutter · 2017-04-11T00:58:43Z

My latest push fixes all the little stuff @behlendorf was looking for. I still need to ponder what to do about the common fork/exec code in print_zpool_script_help()/_zed_exec_fork_child()/vdev_run_cmd()/libzfs_run_process(). Seems like we should be able to consolidate those functions, but they're all subtly different...

dinatale2

I recommend adding the scripts in zpool.d directory to the shellcheck make recipe.

behlendorf · 2017-04-11T20:30:16Z

@tonyhutter regarding the fork/exec code, if we can't come up with a clean way to refactor it in this change I'm OK with leaving it for another day.

dinatale2

I like how this is shaping up! Good work @tonyhutter!

Only other note (which I already previously mentioned), add the scripts to the shellcheck recipe.

dinatale2 · 2017-04-11T20:59:55Z

cmd/zpool/zpool.d/iostat

+	y="-y"
+fi
+
+# Do a one second sample


nit: you're not always doing a 1 second sample here. You're doing either a 1s or 10s sample. Or when you're not running iostat-1s or iostat-10s, you're getting summary stats.

dinatale2 · 2017-04-11T23:15:14Z

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

@@ -45,12 +45,12 @@

 function check_pool_status
 {
-	RESULT=$(grep "pool:" /tmp/pool-status.$$)
+	RESULT=$($GREP "pool:" /tmp/pool-status.$$)


use grep directly

dinatale2 · 2017-04-11T23:15:25Z

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

 	if [ -z "$RESULT" ]
 	then
 		log_fail "No pool: string found in zpool status output!"
 	fi
-	rm /tmp/pool-status.$$
+	$RM /tmp/pool-status.$$


use rm directly

dinatale2 · 2017-04-11T23:19:27Z

cmd/zpool/zpool_main.c

@@ -337,8 +339,8 @@ get_usage(zpool_help_t idx)
 	case HELP_SCRUB:
 		return (gettext("\tscrub [-s] <pool> ...\n"));
 	case HELP_STATUS:
-		return (gettext("\tstatus [-c CMD] [-gLPvxD] [-T d|u] [pool]"
-		    " ... [interval [count]]\n"));
+		return (gettext("\tstatus [-c [script1,script2,...]] [-gLPvxD]"


should this be something like return (gettext("\tstatus [-c script1[,script2,...]] [-gLPvxD]"? You need to specify at least one script to run? The list of multiple scripts to run is optional part.

dinatale2 · 2017-04-11T23:20:30Z

cmd/zpool/zpool_main.c

-		    "[[-lq]|[-r|-w]]\n"
-		    "\t    [[pool ...]|[pool vdev ...]|[vdev ...]] "
-		    "[interval [count]]\n"));
+		return (gettext("\tiostat [[[-c [script1,script2,...]"


Should be return (gettext("\tiostat [[[-c script1[,script2,...]"? You need to specify at least one script to run, right?

You can run with just -c to get the list of the scripts:

$ zpool status -c Current scripts in /usr/local/etc/zfs/zpool.d: enc Show disk enclosure w:x:y:z value. encdev Show the /dev/sg* device for the enclosure associated with the disk slot. fault_led Show the value of the disk enclosure slot fault LED. iostat-1s Do a single 1-second iostat sample and show values. iostat-10s Do a single 10-second iostat sample and show values. label Show filesystem label. locate_led Show the value of the disk enclosure slot locate LED. lsblk Show the disk size, vendor, model number, and serial number. model Show disk model number. serial Show disk serial number. ses Show disk's enclosure, enclosure dev, slot number, and fault/locate LED values. size Show the disk capacity. slaves Show device mapper slave devices. slot Show disk slot number as reported by the enclosure. upath Show the underlying path for a device. vendor Show the disk vendor. iostat Show iostat values since boot (summary page).

ah, I missed that! Sorry!

dinatale2 · 2017-04-11T23:21:39Z

cmd/zpool/zpool_main.c

-			break;
+		if ((strcmp(vcdl->data[i].path, path) != 0) ||
+		    (strcmp(vcdl->data[i].pool, pool) != 0)) {
+			continue;	/* Not the vdev we're looking for */


nit: comment should be above the if

dinatale2 · 2017-04-11T23:24:57Z

cmd/zpool/zpool_main.c

@@ -6071,7 +6277,8 @@ status_callback(zpool_handle_t *zhp, void *data)
 }

 /*
- * zpool status [-c CMD] [-gLPvx] [-T d|u] [pool] ... [interval [count]]
+ * zpool status [-c [script1,script2,...]] [-gLPvx] [-T d|u] [pool] ...


* zpool status [-c script1[,script2,...]] [-gLPvx] [-T d|u] [pool] ...?

dinatale2 · 2017-04-11T23:25:52Z

man/man8/zpool.8

@@ -1542,14 +1542,31 @@ base 1024.  To get the raw values, use the \fB-p\fR flag.
 .sp
 .ne 2
 .na
-\fB\fB-c\fR \fBCMD\fR
+\fB\fB-c\fR \fB[SCRIPT1,SCRIPT2,...]\fR


ditto from above.

dinatale2 · 2017-04-11T23:26:18Z

man/man8/zpool.8

@@ -2103,7 +2120,7 @@ Sets the specified property for \fInewpool\fR. See the “Properties” section
 .sp
 .ne 2
 .na
-\fBzpool status\fR [\fB-c\fR \fBCMD\fR] [\fB-gLPvxD\fR] [\fB-T\fR d | u] [\fIpool\fR] ... [\fIinterval\fR [\fIcount\fR]]
+\fBzpool status\fR [\fB-c\fR \fB[SCRIPT1,SCRIPT2,...] \fR] [\fB-gLPvxD\fR] [\fB-T\fR d | u] [\fIpool\fR] ... [\fIinterval\fR [\fIcount\fR]]


ditto about the optional comma separated list of scripts to run

dinatale2 · 2017-04-11T23:26:32Z

man/man8/zpool.8

@@ -2114,14 +2131,31 @@ If a scrub or resilver is in progress, this command reports the percentage done
 .sp
 .ne 2
 .na
-\fB\fB-c\fR \fBCMD\fR
+\fB\fB-c\fR \fB[SCRIPT1,SCRIPT2,...]\fR


dinatale2 · 2017-04-13T00:28:29Z

Makefile.am

@@ -58,6 +58,7 @@ shellcheck:
 			scripts/zfs.sh \
 			scripts/commitcheck.sh \
 			$$(find cmd/zed/zed.d/*.sh -type f); \
+			$$(find cmd/zpool/zpool.d/* -executable); \


You need to remove the semicolon from the previous line. Your scripts aren't being passed to shellcheck atm.

Thanks, fixed in latest push

tonyhutter · 2017-04-18T16:09:19Z

@behlendorf I was able to refactor libzfs_run_process() to get it to work for the two -c cases. That code is in the latest push that is passing all the tests. I'm still working on the environment variable that you can set to run -c as root though. @dinatale2 all your fixes are in there as well.

dinatale2 · 2017-04-18T19:37:43Z

cmd/zpool/zpool.d/slaves

+# sddt  sdjw
+#
+
+if [ "$1" = "-h" ] ; then


nit: you should be consistent with your helpstr format all the way through these scripts.

Ah, I just realized what you were doing in all the other scripts... and it's really clever. It's still a nit in the sense that you have a helpstr in most of the other scripts. But, I'm not so bothered by this anymore. Feel free to ignore the nits related to this.

dinatale2 · 2017-04-18T19:38:04Z

cmd/zpool/zpool.d/upath

@@ -0,0 +1,7 @@
+#!/bin/sh
+if [ "$1" = "-h" ] ; then
+	echo "Show the underlying path for a device."


nit: ditto 'helpstr` format.

dinatale2 · 2017-04-18T19:40:05Z

cmd/zpool/zpool.d/ses

+ses:		Show disk's enclosure, enclosure dev, slot number, and fault/locate LED values."
+
+script=$(basename "$0")
+if [ "$1" = "-h" ] ; then


Related to the nit: about helpstr, would it be worth making a helper function and placing it in a lib style shell script? You can then define helpstr in each script and simply call a handle_helpstr function of some sort.

Actually, nevermind. I don't see any other things that you could pull into a lib style shell script. Just ignore this.

dinatale2 · 2017-04-18T21:22:56Z

lib/libzfs/libzfs_util.c

+ * Run a command and store its stdout lines in an array of strings (lines[]).
+ * lines[] is allocated and populated for you, and the number of lines is set in
+ * lines_cnt.  lines[] must be freed after use with libzfs_free_str_array().
+ * All newlines (\n) in lines[] are terminated for convenience.


Do you mean null terminated?

dinatale2 · 2017-04-18T21:24:55Z

man/man8/zpool.8

-The \fB-c\fR option allows you to run an arbitrary command on each vdev and
-display the first line of output in zpool iostat.  The following environment
-vars are set before running each command:
+The \fB-c\fR option allows you to run script(s) each vdev and display the


...run script(s) on each vdev...?

dinatale2 · 2017-04-18T21:30:47Z

man/man8/zpool.8

-display the first line of output in zpool iostat.  The following environment
-vars are set before running each command:
+The \fB-c\fR option allows you to run script(s) each vdev and display the
+output in zpool iostat.  For security reasons, only scripts in the


Might sound better as For security reasons, a user can only execute scripts found in the /<etc>/zfs/zpool.d directory as an unprivileged user. Thoughts?

dinatale2 · 2017-04-18T21:33:06Z

man/man8/zpool.8

+The \fB-c\fR option allows you to run script(s) each vdev and display the
+output in zpool iostat.  For security reasons, only scripts in the
+/<etc>/zfs/zpool.d directory can be run, and only as an unprivileged user.
+However, a privilaged user can run with \fB-c\fR if they have the


Privileged is misspelled. Though, the line may sound better as However, a user can run \fB-c\fR with elevated privileges if they have the ZPOOL_SCRIPTS_AS_ROOT environment variable set. Thoughts?

dinatale2 · 2017-04-18T21:36:19Z

man/man8/zpool.8

+/<etc>/zfs/zpool.d directory can be run, and only as an unprivileged user.
+However, a privilaged user can run with \fB-c\fR if they have the
+ZPOOL_SCRIPTS_AS_ROOT environment variable set.  If your script requires you
+to use a privileged command (like smartctl) then it's recommended you allow


Does If a script requires the use of a privileged command (such as smartctl), then it is recommended you allow... sound better?

dinatale2 · 2017-04-18T21:38:14Z

man/man8/zpool.8

-The \fB-c\fR option allows you to run an arbitrary command on each vdev and
-display the first line of output in zpool iostat.  The following environment
-vars are set before running each command:
+The \fB-c\fR option allows you to run script(s) each vdev and display the


ditto: same as the above comments for this same paragraph.

dinatale2 · 2017-04-18T23:48:03Z

Running zpool iostat -c <script> pool vdev results in the footer being printed incorrectly. The last line of dashes is not fully printed and no newline is printed.

Also, zpool iostat -c <script> doesn't display any extra information unless you use the -v flag. I understand that data won't be available because the scripts can only be run on leaf vdevs, but I feel like giving the user no feedback is confusing. It makes the user feel like the -c was ignored entirely. Maybe it would be a good idea to just print the full header so that way the user gets some visual feedback?

tonyhutter · 2017-04-19T17:45:15Z

@dinatale2 thanks for spotting the missing footer on the pool vdev case. I fixed that and the other things you were looking for in the latest push. Also, I made it so -c to automatically turns on verbose mode in zpool iostat, so you don't need to do -vc with it anymore.

tonyhutter · 2017-04-19T18:01:46Z

@behlendorf forgot to mention that the latest push also has the ZPOOL_SCRIPTS_AS_ROOT env var that allows you to run -c as root.

This patch updates the "zpool status/iostat -c" commands to only run "pre-baked" scripts from the /usr/local/etc/zfs/zpool.d directory (or wherever you install to). The scripts can only be run from -c as an unprivileged user (unless the ZPOOL_SCRIPTS_AS_ROOT environment var is set by root). This was done mainly for security reasons. If you script needs to run a privileged command, consider adding the appropriate line in /etc/sudoers. See zpool(8) for an example of how to do this. The patch also allows the scripts to output custom column names. If the script outputs a line like: name=value then "name" is used for the column name, and "value" is its value. Multiple columns can be specified by outputting multiple lines. Column names and values can have spaces. If the value is empty, a dash (-) is printed instead. After all the "name=value" lines are read (if any), zpool will take the next the next line of output (if any) and print it without a column header. After that, no more lines will be processed. This can be useful for printing errors. Lastly, this patch also disables the -c option with the latency and request size histograms, since it produced awkward output and made the code harder to maintain. Signed-off-by: Tony Hutter <[email protected]>

mgmartin · 2017-04-22T01:18:21Z

After pulling in this change, I'm seeing a minor formatting issue using zpool iostat -v with a cache device attached:

                                                capacity     operations     bandwidth 
pool                                           alloc   free   read  write   read  write
---------------------------------------------  -----  -----  -----  -----  -----  -----
array1                                         6.62T  2.44T  2.14K    113  44.2M   834K
  raidz1                                       6.62T  2.44T  2.14K    113  44.2M   834K
    wwn-0x50014ee05878a760                         -      -    441     23  8.90M   167K
    wwn-0x50014ee0adcd6257                         -      -    437     23  8.79M   167K
    wwn-0x50014ee003236b03                         -      -    437     22  8.77M   167K
    ata-WDC_WD2002FAEX-007BA0_WD-WMAY04091360      -      -    439     22  8.84M   167K
    wwn-0x50014ee05878abf1                         -      -    440     21  8.88M   167K
cache                                              -      -      -      -      -      -  loop0                                        3.34G  60.7G      3      9   110K   755K
---------------------------------------------  -----  -----  -----  -----  -----  -----

tonyhutter · 2017-04-24T16:24:07Z

@mgmartin thanks for reporting. I'll take a look.

tonyhutter · 2017-04-24T17:19:52Z

@mgmartin #6060

tonyhutter changed the title ~~Allow custom names in zpool status|iostat -c~~ Allow custom column names in zpool status|iostat -c Mar 1, 2017

tonyhutter force-pushed the col_names branch 2 times, most recently from 4dfda30 to 6cd5f63 Compare March 1, 2017 22:54

tonyhutter force-pushed the col_names branch from 6cd5f63 to 72a00d3 Compare March 2, 2017 22:36

chrisrd mentioned this pull request Mar 3, 2017

Add -c to zpool iostat & status to run command, remove libdevmapper requirement #5368

Merged

behlendorf requested changes Mar 3, 2017

View reviewed changes

tonyhutter force-pushed the col_names branch from 72a00d3 to 0bd3b4f Compare March 6, 2017 23:15

tonyhutter force-pushed the col_names branch 2 times, most recently from 3f09acd to 65f6fe0 Compare March 14, 2017 21:09

tonyhutter force-pushed the col_names branch 5 times, most recently from 0d83c7d to 0eff7fb Compare March 28, 2017 00:43

tonyhutter force-pushed the col_names branch from 0eff7fb to 19aec91 Compare March 28, 2017 01:01

behlendorf requested changes Mar 28, 2017

View reviewed changes

tonyhutter force-pushed the col_names branch 2 times, most recently from ce9fcd6 to a02a6ab Compare March 28, 2017 23:04

tonyhutter changed the title ~~Allow custom column names in zpool status|iostat -c~~ Custom column names and scripts for zpool status/iostat -c Mar 29, 2017

tonyhutter force-pushed the col_names branch 2 times, most recently from 4100fa3 to e1b2de1 Compare April 10, 2017 18:25

behlendorf requested changes Apr 10, 2017

View reviewed changes

tonyhutter force-pushed the col_names branch from e1b2de1 to cb17b76 Compare April 11, 2017 00:43

dinatale2 reviewed Apr 11, 2017

View reviewed changes

tonyhutter force-pushed the col_names branch from cb17b76 to 8ebb007 Compare April 11, 2017 17:01

dinatale2 suggested changes Apr 11, 2017

View reviewed changes

tonyhutter force-pushed the col_names branch 4 times, most recently from 66b50f8 to 4284379 Compare April 13, 2017 00:06

dinatale2 suggested changes Apr 13, 2017

View reviewed changes

tonyhutter force-pushed the col_names branch from 4284379 to 8af982b Compare April 13, 2017 00:31

tonyhutter force-pushed the col_names branch from 8af982b to dc507f6 Compare April 18, 2017 20:51

dinatale2 suggested changes Apr 18, 2017

View reviewed changes

tonyhutter force-pushed the col_names branch from dc507f6 to edc430a Compare April 18, 2017 23:06

tonyhutter force-pushed the col_names branch from edc430a to fb17fc1 Compare April 19, 2017 16:41

tonyhutter force-pushed the col_names branch from fb17fc1 to 350fa0b Compare April 21, 2017 00:39

behlendorf approved these changes Apr 21, 2017

View reviewed changes

behlendorf merged commit d6418de into openzfs:master Apr 21, 2017

Custom column names and scripts for zpool status/iostat -c #5852

Custom column names and scripts for zpool status/iostat -c #5852

Conversation

tonyhutter commented Mar 1, 2017 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

mention-bot commented Mar 1, 2017

tonyhutter commented Mar 1, 2017

behlendorf commented Mar 1, 2017

tonyhutter commented Mar 2, 2017

tonyhutter commented Mar 2, 2017

chrisrd commented Mar 3, 2017

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

behlendorf commented Mar 3, 2017 • edited Loading

tonyhutter commented Mar 8, 2017

chrisrd commented Mar 15, 2017

chrisrd commented Mar 16, 2017

tonyhutter commented Mar 18, 2017

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

behlendorf 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

tonyhutter commented Apr 11, 2017

dinatale2 left a comment

Choose a reason for hiding this comment

behlendorf commented Apr 11, 2017

dinatale2 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tonyhutter commented Apr 18, 2017 • 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

tonyhutter commented Mar 1, 2017 •

edited

Loading

behlendorf commented Mar 3, 2017 •

edited

Loading

tonyhutter commented Apr 18, 2017 •

edited

Loading