From a0a43a42af8228f1373650862051bb5c5294eb70 Mon Sep 17 00:00:00 2001
From: Darik Horn <dajhorn@vanadac.com>
Date: Tue, 22 Nov 2011 22:09:23 -0600
Subject: [PATCH 1/6] Invert the --skip-scrub test.

The $opt_skip_scrub option was tested incorrectly, which caused
the --skip-scrub option to operate opposite intent.
---
 src/zfs-auto-snapshot.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/zfs-auto-snapshot.sh b/src/zfs-auto-snapshot.sh
index 5ec98e8..d312e0a 100755
--- a/src/zfs-auto-snapshot.sh
+++ b/src/zfs-auto-snapshot.sh
@@ -346,7 +346,7 @@ do
 	done
 
 	# Exclude objects in scrubbing pools if the --skip-scrub flag is set.
-	test -z "$opt_skip_scrub" && for jj in $ZPOOLS_SCRUBBING
+	test -n "$opt_skip_scrub" && for jj in $ZPOOLS_SCRUBBING
 	do
 		# Ibid regarding iii.
 		jjj="$jj/"

From 249e6a4cb3e03ce0b74c88a4f434a540a83df536 Mon Sep 17 00:00:00 2001
From: Darik Horn <dajhorn@vanadac.com>
Date: Tue, 22 Nov 2011 23:27:24 -0600
Subject: [PATCH 2/6] Fold the main loops into a do_snapshots function.

The two primary `for` loops were identical except for the '-r' flag.
---
 src/zfs-auto-snapshot.sh | 167 ++++++++++++++++++---------------------
 1 file changed, 78 insertions(+), 89 deletions(-)

diff --git a/src/zfs-auto-snapshot.sh b/src/zfs-auto-snapshot.sh
index d312e0a..0f5f8ef 100755
--- a/src/zfs-auto-snapshot.sh
+++ b/src/zfs-auto-snapshot.sh
@@ -38,6 +38,14 @@ opt_syslog=''
 opt_skip_scrub=''
 opt_verbose=''
 
+# Global summary statistics.
+DESTRUCTION_COUNT='0'
+SNAPSHOT_COUNT='0'
+WARNING_COUNT='0'
+
+# Global variables.
+SNAPSHOTS_OLD=''
+
 
 print_usage ()
 {
@@ -108,7 +116,7 @@ print_log () # level, message, ...
 }
 
 
-do_run ()
+do_run () # [argv]
 {
 	if [ -n "$opt_dry_run" ]
 	then
@@ -127,11 +135,61 @@ do_run ()
 	return "$RC"
 }
 
+
+do_snapshots () # properties, flags, snapname, oldglob, [targets...]
+{
+	local PROPS="$1"
+	local FLAGS="$2"
+	local NAME="$3"
+	local GLOB="$4"
+	local TARGETS="$5"
+	local KEEP=''
+
+	# global DESTRUCTION_COUNT
+	# global SNAPSHOT_COUNT
+	# global WARNING_COUNT
+	# global SNAPSHOTS_OLD
+
+	for ii in $TARGETS
+	do
+		if do_run "zfs snapshot $PROPERTIES $FLAGS '$ii@$NAME'" 
+		then
+			SNAPSHOT_COUNT=$(( $SNAPSHOT_COUNT +1 ))
+		else
+			WARNING_COUNT=$(( $WARNING_COUNT +1 ))
+			continue
+		fi 
+
+		# Retain at most $opt_keep number of old snapshots of this filesystem,
+		# including the one that was just recently created.
+		test -z "$opt_keep" && continue
+		KEEP="$opt_keep"
+
+		# ASSERT: The old snapshot list is sorted by increasing age.
+		for jj in $SNAPSHOTS_OLD
+		do
+			# Check whether this is an old snapshot of the filesystem.
+			if [ -z "${jj#$ii@$GLOB}" ]
+			then
+				KEEP=$(( $KEEP - 1 ))
+				if [ "$KEEP" -le 0 ]
+				then
+					if do_run "zfs destroy $FLAGS '$jj'" 
+					then
+						DESTRUCTION_COUNT=$(( $DESTRUCTION_COUNT +1 ))
+					else
+						WARNING_COUNT=$(( $WARNING_COUNT + 1 ))
+					fi
+				fi
+			fi
+		done
+	done
+}
+
+
 # main ()
 # {
 
-DATE=$(date +%F-%H%M)
-
 GETOPT=$(getopt \
   --longoptions=default-exclude,dry-run,skip-scrub,recursive \
   --longoptions=keep:,label:,prefix:,sep: \
@@ -405,20 +463,22 @@ do
 	TARGETS_RECURSIVE="${TARGETS_RECURSIVE:+$TARGETS_RECURSIVE	}$ii" # nb: \t
 done
 
-# Summary statistics.
-DESTRUCTION_COUNT='0'
-SNAPSHOT_COUNT='0'
-WARNING_COUNT='0'
 
 # Linux lacks SMF and the notion of an FMRI event.
 FMRI_EVENT='-'
 
-# Create the snapshot using these arguments.
-SNAPSHOT_PROPERTIES="-o com.sun:auto-snapshot-desc='$FMRI_EVENT'"
-SNAPSHOT_NAME="$opt_prefix${opt_label:+$opt_sep$opt_label-$DATE}"
+# Set this property because the SUNW program does.
+SNAPPROP="-o com.sun:auto-snapshot-desc='$FMRI_EVENT'"
 
-# The expression for old snapshots.                 -YYYY-MM-DD-HHMM
-SNAPSHOT_MATCH="$opt_prefix${opt_label:+?$opt_label}????????????????"
+# ISO style date; fifteen characters: YYYY-MM-DD-HHMM
+# On Solaris %H%M expands to 12h34.
+DATE=$(date +%F-%H%M)
+
+# The snapshot name after the @ symbol.
+SNAPNAME="$opt_prefix${opt_label:+$opt_sep$opt_label-$DATE}"
+
+# The expression for matching old snapshots.  -YYYY-MM-DD-HHMM
+SNAPGLOB="$opt_prefix${opt_label:+?$opt_label}????????????????"
 
 test -n "$TARGETS_REGULAR" \
   && print_log info "Doing regular snapshots of $TARGETS_REGULAR"
@@ -429,84 +489,13 @@ test -n "$TARGETS_RECURSIVE" \
 test -n "$opt_dry_run" \
   && print_log info "Doing a dry run. Not running these commands..."
 
+do_snapshots "$SNAPPROP" ""   "$SNAPNAME" "$SNAPGLOB" "$TARGETS_REGULAR"
+do_snapshots "$SNAPPROP" "-r" "$SNAPNAME" "$SNAPGLOB" "$TARGETS_RECURSIVE"
 
-for ii in $TARGETS_REGULAR
-do
-	if do_run "zfs snapshot $SNAPSHOT_PROPERTIES '$ii@$SNAPSHOT_NAME'"
-	then
-		SNAPSHOT_COUNT=$(( $SNAPSHOT_COUNT +1 ))
-	else
-		WARNING_COUNT=$(( $WARNING_COUNT +1 ))
-		continue
-	fi
-
-	# Retain at most $opt_keep number of old snapshots of this filesystem,
-	# including the one that was just recently created.
-	if [ -z "$opt_keep" ]
-	then
-		continue
-	fi
-	KEEP="$opt_keep"
-
-	for jj in $SNAPSHOTS_OLD
-	do
-		# Check whether this is an old snapshot of the filesystem.
-		if [ -z "${jj#$ii@$SNAPSHOT_MATCH}" ]
-		then
-			KEEP=$(( $KEEP - 1 ))
-			if [ "$KEEP" -le 0 ]
-			then
-				if do_run "zfs destroy '$jj'"
-				then
-					DESTRUCTION_COUNT=$(( $DESTRUCTION_COUNT +1 ))
-				else
-					WARNING_COUNT=$(( $WARNING_COUNT + 1 ))
-				fi
-			fi
-		fi
-	done
-done
-
-for ii in $TARGETS_RECURSIVE
-do
-	if do_run "zfs snapshot $SNAPSHOT_PROPERTIES -r '$ii@$SNAPSHOT_NAME'" 
-	then
-		SNAPSHOT_COUNT=$(( $SNAPSHOT_COUNT +1 ))
-	else
-		WARNING_COUNT=$(( $WARNING_COUNT +1 ))
-		continue
-	fi 
-
-	# Retain at most $opt_keep number of old snapshots of this filesystem,
-	# including the one that was just recently created.
-	if [ -z "$opt_keep" ]
-	then
-		continue
-	fi
-	KEEP="$opt_keep"
-
-	# ASSERT: The old snapshot list is sorted by increasing age.
-	for jj in $SNAPSHOTS_OLD
-	do
-		# Check whether this is an old snapshot of the filesystem.
-		if [ -z "${jj#$ii@$SNAPSHOT_MATCH}" ]
-		then
-			KEEP=$(( $KEEP - 1 ))
-			if [ "$KEEP" -le 0 ]
-			then
-				if do_run "zfs destroy -r '$jj'" 
-				then
-					DESTRUCTION_COUNT=$(( $DESTRUCTION_COUNT +1 ))
-				else
-					WARNING_COUNT=$(( $WARNING_COUNT + 1 ))
-				fi
-			fi
-		fi
-	done
-done
-
-print_log notice "@$SNAPSHOT_NAME, \
-$SNAPSHOT_COUNT created, $DESTRUCTION_COUNT destroyed, $WARNING_COUNT warnings."
+print_log notice "@$SNAPNAME," \
+  "$SNAPSHOT_COUNT created," \
+  "$DESTRUCTION_COUNT destroyed," \
+  "$WARNING_COUNT warnings."
 
 exit 0
 # }

From 4c14da41308465ac7ee7a78e994a3270ade0b0ed Mon Sep 17 00:00:00 2001
From: Darik Horn <dajhorn@vanadac.com>
Date: Tue, 22 Nov 2011 23:36:51 -0600
Subject: [PATCH 3/6] Consistently quote literal strings.

Apply some syntax hygiene. Quoting everything is a safe habit because
unquoted things can have subtle side-effects.
---
 src/zfs-auto-snapshot.sh | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/src/zfs-auto-snapshot.sh b/src/zfs-auto-snapshot.sh
index 0f5f8ef..2730518 100755
--- a/src/zfs-auto-snapshot.sh
+++ b/src/zfs-auto-snapshot.sh
@@ -43,7 +43,7 @@ DESTRUCTION_COUNT='0'
 SNAPSHOT_COUNT='0'
 WARNING_COUNT='0'
 
-# Global variables.
+# Other global variables.
 SNAPSHOTS_OLD=''
 
 
@@ -125,7 +125,7 @@ do_run () # [argv]
 	else
 		eval $*
 		RC="$?"
-		if [ "$RC" -eq 0 ]
+		if [ "$RC" -eq '0' ]
 		then
 			print_log debug "$*"
 		else
@@ -154,9 +154,9 @@ do_snapshots () # properties, flags, snapname, oldglob, [targets...]
 	do
 		if do_run "zfs snapshot $PROPERTIES $FLAGS '$ii@$NAME'" 
 		then
-			SNAPSHOT_COUNT=$(( $SNAPSHOT_COUNT +1 ))
+			SNAPSHOT_COUNT=$(( $SNAPSHOT_COUNT + 1 ))
 		else
-			WARNING_COUNT=$(( $WARNING_COUNT +1 ))
+			WARNING_COUNT=$(( $WARNING_COUNT + 1 ))
 			continue
 		fi 
 
@@ -172,11 +172,11 @@ do_snapshots () # properties, flags, snapname, oldglob, [targets...]
 			if [ -z "${jj#$ii@$GLOB}" ]
 			then
 				KEEP=$(( $KEEP - 1 ))
-				if [ "$KEEP" -le 0 ]
+				if [ "$KEEP" -le '0' ]
 				then
 					if do_run "zfs destroy $FLAGS '$jj'" 
 					then
-						DESTRUCTION_COUNT=$(( $DESTRUCTION_COUNT +1 ))
+						DESTRUCTION_COUNT=$(( $DESTRUCTION_COUNT + 1 ))
 					else
 						WARNING_COUNT=$(( $WARNING_COUNT + 1 ))
 					fi
@@ -200,13 +200,13 @@ GETOPT=$(getopt \
 
 eval set -- "$GETOPT"
 
-while [ "$#" -gt 0 ]
+while [ "$#" -gt '0' ]
 do
 	case "$1" in
 		(-d|--debug)
-			opt_debug=1
+			opt_debug='1'
 			opt_quiet=''
-			opt_verbose=1
+			opt_verbose='1'
 			shift 1
 			;;
 		(--default-exclude)
@@ -218,7 +218,7 @@ do
 			shift 1
 			;;
 		(-s|--skip-scrub)
-			opt_skip_scrub=1
+			opt_skip_scrub='1'
 			shift 1
 			;;
 		(-h|--help)
@@ -226,7 +226,7 @@ do
 			exit 0
 			;;
 		(-k|--keep)
-			if ! test "$2" -gt 0 2>/dev/null
+			if ! test "$2" -gt '0' 2>/dev/null
 			then
 				print_log error "The $1 parameter must be a positive integer."
 				exit 2
@@ -249,7 +249,7 @@ do
 			shift 1
 			;;
 		(-r|--recursive)
-			opt_recursive=1
+			opt_recursive='1'
 			shift 1
 			;;
 		(--sep)
@@ -264,18 +264,18 @@ do
 				(*)
 					print_log error "The $1 parameter must be one alphanumeric character."
 					exit 4
-				;;
+					;;
 			esac
 			opt_sep="$2"
 			shift 2
 			;;
 		(-g|--syslog)
-			opt_syslog=1
+			opt_syslog='1'
 			shift 1
 			;;
 		(-v|--verbose)
 			opt_quiet=''
-			opt_verbose=1
+			opt_verbose='1'
 			shift 1
 			;;
 		(--)
@@ -285,7 +285,7 @@ do
 	esac
 done
 
-if [ "$#" -eq 0 ]
+if [ "$#" -eq '0' ]
 then
 	print_log error "The filesystem argument list is empty."
 	exit 5
@@ -298,7 +298,7 @@ do
 	test "$ii" = '//' && SLASHIES=$(( $SLASHIES + 1 ))
 done
 
-if [ "$#" -gt 1 -a "$SLASHIES" -gt 0 ]
+if [ "$#" -gt '1' -a "$SLASHIES" -gt '0' ]
 then
 	print_log error "The // must be the only argument if it is given."
 	exit 6

From f7ceb28963f353beee7d13f87643944cb8ed10c2 Mon Sep 17 00:00:00 2001
From: Darik Horn <dajhorn@vanadac.com>
Date: Wed, 23 Nov 2011 10:00:24 -0600
Subject: [PATCH 4/6] Implement --prefix parameter checking.

Also add -p to the getopt list of short options.
---
 src/zfs-auto-snapshot.sh | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/src/zfs-auto-snapshot.sh b/src/zfs-auto-snapshot.sh
index 2730518..8a366bb 100755
--- a/src/zfs-auto-snapshot.sh
+++ b/src/zfs-auto-snapshot.sh
@@ -194,7 +194,7 @@ GETOPT=$(getopt \
   --longoptions=default-exclude,dry-run,skip-scrub,recursive \
   --longoptions=keep:,label:,prefix:,sep: \
   --longoptions=debug,help,quiet,syslog,verbose \
-  --options=dnshl:k:rs:qgv \
+  --options=dnshl:k:p:rs:qgv \
   -- "$@" ) \
   || exit 1
 
@@ -239,8 +239,19 @@ do
 			shift 2
 			;;
 		(-p|--prefix)
-			# @TODO: Parameter validation. See --sep below for the regex.
 			opt_prefix="$2"
+			while test "${#opt_prefix}" -gt '0'
+			do
+				case $opt_prefix in
+					([![:alnum:]_-.:\ ]*)
+						print_log error "The $1 parameter must be alphanumeric."
+						exit 44
+						;;
+				esac
+				opt_prefix="${opt_prefix#?}"
+			done
+			opt_prefix="$2"
+			shift 2
 			;;
 		(-q|--quiet)
 			opt_debug=''

From 2e26499f6a4931ff7e31038e95df5e7269935650 Mon Sep 17 00:00:00 2001
From: Darik Horn <dajhorn@vanadac.com>
Date: Wed, 23 Nov 2011 10:03:56 -0600
Subject: [PATCH 5/6] Rebase exit codes to above 127.

Most standard shell utilities return errors less than 128. Rebase the
exit codes of this script to reduce the liklihood of overlap.
---
 src/zfs-auto-snapshot.sh | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/src/zfs-auto-snapshot.sh b/src/zfs-auto-snapshot.sh
index 8a366bb..9e9f3f9 100755
--- a/src/zfs-auto-snapshot.sh
+++ b/src/zfs-auto-snapshot.sh
@@ -196,7 +196,7 @@ GETOPT=$(getopt \
   --longoptions=debug,help,quiet,syslog,verbose \
   --options=dnshl:k:p:rs:qgv \
   -- "$@" ) \
-  || exit 1
+  || exit 128
 
 eval set -- "$GETOPT"
 
@@ -229,7 +229,7 @@ do
 			if ! test "$2" -gt '0' 2>/dev/null
 			then
 				print_log error "The $1 parameter must be a positive integer."
-				exit 2
+				exit 129
 			fi
 			opt_keep="$2"
 			shift 2
@@ -245,7 +245,7 @@ do
 				case $opt_prefix in
 					([![:alnum:]_-.:\ ]*)
 						print_log error "The $1 parameter must be alphanumeric."
-						exit 44
+						exit 130
 						;;
 				esac
 				opt_prefix="${opt_prefix#?}"
@@ -270,11 +270,11 @@ do
 					;;
 				('')
 					print_log error "The $1 parameter must be non-empty."
-					exit 3
+					exit 131
 					;;
 				(*)
 					print_log error "The $1 parameter must be one alphanumeric character."
-					exit 4
+					exit 132
 					;;
 			esac
 			opt_sep="$2"
@@ -299,7 +299,7 @@ done
 if [ "$#" -eq '0' ]
 then
 	print_log error "The filesystem argument list is empty."
-	exit 5
+	exit 133
 fi 
 
 # Count the number of times '//' appears on the command line.
@@ -312,7 +312,7 @@ done
 if [ "$#" -gt '1' -a "$SLASHIES" -gt '0' ]
 then
 	print_log error "The // must be the only argument if it is given."
-	exit 6
+	exit 134
 fi
 
 # These are the only times that `zpool status` or `zfs list` are invoked, so
@@ -320,14 +320,14 @@ fi
 # Solaris implementation.
 
 ZPOOL_STATUS=$(env LC_ALL=C zpool status 2>&1 ) \
-  || { print_log error "zpool status $?: $ZPOOL_STATUS"; exit 7; }
+  || { print_log error "zpool status $?: $ZPOOL_STATUS"; exit 135; }
 
 ZFS_LIST=$(env LC_ALL=C zfs list -H -t filesystem,volume -s name \
   -o name,com.sun:auto-snapshot,com.sun:auto-snapshot:"$opt_label") \
-  || { print_log error "zfs list $?: $ZFS_LIST"; exit 8; }
+  || { print_log error "zfs list $?: $ZFS_LIST"; exit 136; }
 
 SNAPSHOTS_OLD=$(env LC_ALL=C zfs list -H -t snapshot -S creation -o name) \
-  || { print_log error "zfs list $?: $SNAPSHOTS_OLD"; exit 9; }
+  || { print_log error "zfs list $?: $SNAPSHOTS_OLD"; exit 137; }
 
 
 # Verify that each argument is a filesystem or volume.
@@ -341,7 +341,7 @@ do
 	$ZFS_LIST
 	HERE
 	print_log error "$ii is not a ZFS filesystem or volume."
-	exit 10
+	exit 138
 done
 
 # Get a list of pools that are being scrubbed.

From 5ce7a23384f566302f0e1ddba1b3752daae9e0df Mon Sep 17 00:00:00 2001
From: Darik Horn <dajhorn@vanadac.com>
Date: Wed, 23 Nov 2011 10:36:43 -0600
Subject: [PATCH 6/6] Implement the --event option for :auto-snap-desc.

Set the com.sun:auto-snap-desc property on each snapshot to an
arbitrary value.  On Solaris, this property is set to the dash
character by default -- which is convention for NULL -- but it
can contain an FMRI status comment or other user data.
---
 src/zfs-auto-snapshot.sh | 28 +++++++++++++++++++---------
 1 file changed, 19 insertions(+), 9 deletions(-)

diff --git a/src/zfs-auto-snapshot.sh b/src/zfs-auto-snapshot.sh
index 9e9f3f9..1b33c2d 100755
--- a/src/zfs-auto-snapshot.sh
+++ b/src/zfs-auto-snapshot.sh
@@ -28,6 +28,7 @@ opt_backup_full=''
 opt_backup_incremental=''
 opt_default_exclude=''
 opt_dry_run=''
+opt_event='-'
 opt_keep=''
 opt_label=''
 opt_prefix='zfs-auto-snap'
@@ -52,6 +53,7 @@ print_usage ()
 	echo "Usage: $0 [options] [-l label] <'//' | name [name...]>
   --default-exclude  Exclude objects if com.sun:auto-snapshot is unset.
   -d, --debug        Print debugging messages.
+  -e, --event=EVENT  Set the com.sun:auto-snapshot-desc property to EVENT.
   -n, --dry-run      Print actions without actually doing anything.
   -s, --skip-scrub   Do not snapshot filesystems in scrubbing pools.
   -h, --help         Print this usage message.
@@ -152,7 +154,7 @@ do_snapshots () # properties, flags, snapname, oldglob, [targets...]
 
 	for ii in $TARGETS
 	do
-		if do_run "zfs snapshot $PROPERTIES $FLAGS '$ii@$NAME'" 
+		if do_run "zfs snapshot $PROPS $FLAGS '$ii@$NAME'" 
 		then
 			SNAPSHOT_COUNT=$(( $SNAPSHOT_COUNT + 1 ))
 		else
@@ -192,9 +194,9 @@ do_snapshots () # properties, flags, snapname, oldglob, [targets...]
 
 GETOPT=$(getopt \
   --longoptions=default-exclude,dry-run,skip-scrub,recursive \
-  --longoptions=keep:,label:,prefix:,sep: \
+  --longoptions=event:,keep:,label:,prefix:,sep: \
   --longoptions=debug,help,quiet,syslog,verbose \
-  --options=dnshl:k:p:rs:qgv \
+  --options=dnshe:l:k:p:rs:qgv \
   -- "$@" ) \
   || exit 128
 
@@ -213,6 +215,17 @@ do
 			opt_default_exclude='1'
 			shift 1
 			;;
+		(-e|--event)
+			if [ "${#2}" -gt '1024' ]
+			then
+				print_log error "The $1 parameter must be less than 1025 characters."
+				exit 139
+			elif [ "${#2}" -gt '0' ]
+			then
+				opt_event="$2"
+			fi
+			shift 2
+			;;
 		(-n|--dry-run)
 			opt_dry_run='1'
 			shift 1
@@ -474,12 +487,9 @@ do
 	TARGETS_RECURSIVE="${TARGETS_RECURSIVE:+$TARGETS_RECURSIVE	}$ii" # nb: \t
 done
 
-
-# Linux lacks SMF and the notion of an FMRI event.
-FMRI_EVENT='-'
-
-# Set this property because the SUNW program does.
-SNAPPROP="-o com.sun:auto-snapshot-desc='$FMRI_EVENT'"
+# Linux lacks SMF and the notion of an FMRI event, but always set this property
+# because the SUNW program does. The dash character is the default.
+SNAPPROP="-o com.sun:auto-snapshot-desc='$opt_event'"
 
 # ISO style date; fifteen characters: YYYY-MM-DD-HHMM
 # On Solaris %H%M expands to 12h34.