lvconvert: add hidden lv access restrictions

This copies the previous logic which is probably
not correct.

test/shell/lvmetad-pvscan-autoactivation-polling.sh

@@ -54,7 +54,7 @@ mkdir test_mnt
 
 setup_merge_ $vg1 $lv1
 mount "$(lvdev_ $vg1 $lv1)" test_mnt
-lvconvert --merge $vg1/$(snap_lv_name_ $lv1)
+lvconvert --mergesnapshot $vg1/$(snap_lv_name_ $lv1)
 umount test_mnt
 vgchange -an $vg1
 

test/shell/snapshot-merge-stack.sh

@@ -34,7 +34,7 @@ snap_and_merge() {
 	SLEEP_PID=$!
 
 	# initiate background merge
-	lvconvert -b --merge $vg/$lv2
+	lvconvert -b --mergesnapshot $vg/$lv2
 
 	lvs -a -o+lv_merging,lv_merge_failed $vg
 	kill $SLEEP_PID

test/shell/snapshot-merge.sh

@@ -51,15 +51,15 @@ mkdir test_mnt
 # test full merge of a single LV
 setup_merge_ $vg $lv1
 
-# make sure lvconvert --merge requires explicit LV listing
-not lvconvert --merge
-lvconvert --merge $vg/$(snap_lv_name_ $lv1)
+# make sure lvconvert --mergesnapshot requires explicit LV listing
+not lvconvert --mergesnapshot
+lvconvert --mergesnapshot $vg/$(snap_lv_name_ $lv1)
 lvremove -f $vg/$lv1
 
 
 # test that an actively merging snapshot may not be removed
 setup_merge_ $vg $lv1
-lvconvert -i+100 --merge --background $vg/$(snap_lv_name_ $lv1)
+lvconvert -i+100 --mergesnapshot --background $vg/$(snap_lv_name_ $lv1)
 not lvremove -f $vg/$(snap_lv_name_ $lv1)
 lvremove -f $vg/$lv1
 
@@ -67,7 +67,7 @@ lvremove -f $vg/$lv1
 # "onactivate merge" test
 setup_merge_ $vg $lv1
 mount "$(lvdev_ $vg $lv1)" test_mnt
-lvconvert --merge $vg/$(snap_lv_name_ $lv1)
+lvconvert --mergesnapshot $vg/$(snap_lv_name_ $lv1)
 # -- refresh LV while FS is still mounted (merge must not start),
 #    verify 'snapshot-origin' target is still being used
 lvchange --refresh $vg/$lv1

test/shell/thin-merge.sh

@@ -102,7 +102,7 @@ lvcreate -s -n snap $vg/$lv1
 lvcreate -s -L10 -n oldsnapof_${lv1} $vg/$lv1
 not lvconvert --merge $vg/snap
 $MKFS "$DM_DEV_DIR/$vg/oldsnapof_${lv1}"
-lvconvert --merge $vg/oldsnapof_${lv1}
+lvconvert --mergesnapshot $vg/oldsnapof_${lv1}
 fsck -n "$DM_DEV_DIR/$vg/$lv1"
 check lv_not_exists $vg oldsnapof_${lv1}
 # Add old snapshot to thin snapshot

tools/command-lines.in

@@ -240,7 +240,7 @@ OO_LVCHANGE_META: --addtag Tag, --deltag Tag,
 --writebehind Number, --writemostly WriteMostlyPV, --persistent n
 
 lvchange OO_LVCHANGE_META VG|LV|Tag|Select ...
-OO: OO_LVCHANGE
+OO: --activate Active, OO_LVCHANGE
 ID: lvchange_properties
 DESC: Change a general LV property.
 RULE: all not lv_is_pvmove lv_is_mirror_log lv_is_mirror_image
@@ -275,15 +275,14 @@ ID: lvchange_rebuild
 DESC: Reconstruct data on specific PVs of a raid LV.
 RULE: all not LV_raid0
 
-# try removing the META change options from here?
 lvchange --activate Active VG|LV|Tag|Select ...
 OO: --activationmode ActivationMode, --partial, --ignoreactivationskip,
---ignorelockingfailure, --sysinit, OO_LVCHANGE_META, OO_LVCHANGE
+--ignorelockingfailure, --sysinit, OO_LVCHANGE
 ID: lvchange_activate
 DESC: Activate or deactivate an LV.
 
 lvchange --refresh VG|LV|Tag|Select ...
-OO: --partial, OO_LVCHANGE
+OO: --partial, --poll Bool, OO_LVCHANGE
 ID: lvchange_refresh
 DESC: Reactivate an LV using the latest metadata.
 
@@ -470,9 +469,6 @@ FLAGS: SECONDARY_SYNTAX
 
 ---
 
-# lvconvert utilities related to snapshots and repair.
-# Create a new command set for these and migrate them out of lvconvert?
-
 # FIXME: lvconvert --merge is an extremely ambiguous command.
 # It can do very different operations, but which one depends
 # on knowing the LV type.  So, the command doesn't know what
@@ -503,34 +499,58 @@ ID: lvconvert_merge
 DESC: Merge LV that was previously split from a mirror.
 DESC: Merge thin LV into its origin LV.
 DESC: Merge COW snapshot LV into its origin.
-RULE: all not lv_is_merging_origin lv_is_virtual_origin lv_is_external_origin lv_is_merging_cow
+RULE: all not lv_is_locked lv_is_pvmove lv_is_merging_origin lv_is_virtual_origin lv_is_external_origin lv_is_merging_cow
+
+---
+
+# lvconvert snapshot-related utilities
+# Create a new command set for these and migrate them out of lvconvert?
 
 lvconvert --mergesnapshot LV_snapshot ...
 OO: --background, --interval Number, OO_LVCONVERT
 ID: lvconvert_merge_snapshot
 DESC: Merge LV that was previously split from a mirror.
-RULE: all not lv_is_merging_origin lv_is_virtual_origin lv_is_external_origin lv_is_merging_cow
+RULE: all not lv_is_locked lv_is_pvmove lv_is_merging_origin lv_is_virtual_origin lv_is_external_origin lv_is_merging_cow
 
 ---
 
-# NB: an unsual use of position args here, where
-# the second position LV is the only one processed
-# by process_each_lv.
+lvconvert --splitsnapshot LV_snapshot
+OO: OO_LVCONVERT
+ID: lvconvert_split_cow_snapshot
+DESC: Separate a COW snapshot from its origin LV.
+RULE: all not lv_is_locked lv_is_pvmove lv_is_origin lv_is_external_origin lv_is_merging_cow
+
+---
+
+# NB: an unsual use of position args here, the first pos arg
+# (will become origin LV) is not passed to process_each,
+# the second pos arg (will become cow LV) is given to
+# process_each.  Because the first pos LV is not handled
+# by process_each_lv, it cannot be checked against this
+# command def, so a specific LV type in the first pos
+# will not be checked.
 
 # alternate form of lvconvert --snapshot
-lvconvert --type snapshot LV_linear_striped_raid LV_snapshot
+lvconvert --type snapshot LV LV_linear
 OO: --snapshot, --chunksize SizeKB, --zero Bool, OO_LVCONVERT
 ID: lvconvert_combine_split_snapshot
-DESC: Combine LV with a previously split snapshot LV.
+DESC: Combine a former COW snapshot (second arg) with a former
+DESC: origin LV (first arg) to reverse a splitsnapshot command.
 FLAGS: SECONDARY_SYNTAX
+RULE: all not lv_is_locked lv_is_pvmove
 
-lvconvert --snapshot LV_linear_striped_raid LV_snapshot
+lvconvert --snapshot LV LV_linear
 OO: --type snapshot, --chunksize SizeKB, --zero Bool, OO_LVCONVERT
 ID: lvconvert_combine_split_snapshot
-DESC: Combine LV with a previously split snapshot LV.
+DESC: Combine a former COW snapshot (second arg) with a former
+DESC: origin LV (first arg) to reverse a splitsnapshot command.
+RULE: all not lv_is_locked lv_is_pvmove
 
 ---
 
+# lvconvert repair/replace utilitiles
+# Create a new command set for these and migrate them out of lvconvert?
+
 # FIXME: use specific option names to distinguish these two
 # very different commands, e.g.
 #
@@ -551,22 +571,14 @@ OP: PV ...
 ID: lvconvert_repair_pvs_or_thinpool
 DESC: Replace failed PVs in a raid or mirror LV.
 DESC: Repair a thin pool.
-
----
+RULE: all not lv_is_locked lv_is_pvmove
 
 lvconvert --replace PV LV_raid
 OO: OO_LVCONVERT
 OP: PV ...
 ID: lvconvert_replace_pv
 DESC: Replace specific PV(s) in a raid* LV with another PV.
-
----
-
-lvconvert --splitsnapshot LV_snapshot
-OO: OO_LVCONVERT
-ID: lvconvert_split_cow_snapshot
-DESC: Separate a COW snapshot from its origin LV.
-RULE: all not lv_is_origin lv_is_external_origin lv_is_merging_cow lv_is_vg_writable lv_is_pvmove
+RULE: all not lv_is_locked lv_is_pvmove
 
 ---
 

tools/lv_types.h

@@ -28,5 +28,7 @@ lvt(raid4_LVT, "raid4", NULL)
 lvt(raid5_LVT, "raid5", NULL)
 lvt(raid6_LVT, "raid6", NULL)
 lvt(raid10_LVT, "raid10", NULL)
+lvt(error_LVT, "error", NULL)
+lvt(zero_LVT, "zero", NULL)
 lvt(LVT_COUNT, "", NULL)
 

tools/lvchange.c

@@ -1009,14 +1009,56 @@ static int _lvchange_properties_single(struct cmd_context *cmd,
 	return ECMD_PROCESSED;
 }
 
+static int _lvchange_properties_check(struct cmd_context *cmd,
+				     struct logical_volume *lv,
+				     struct processing_handle *handle,
+				     int lv_is_named_arg)
+{
+	if (!lv_is_visible(lv)) {
+		if (lv_is_named_arg)
+			log_error("Operation not permitted (%s %d) on hidden LV %s.",
+				  cmd->command->command_line_id, cmd->command->command_line_enum,
+				  display_lvname(lv));
+		return 0;
+	}
+
+	return 1;
+}
+
 int lvchange_properties_cmd(struct cmd_context *cmd, int argc, char **argv)
 {
+	int ret;
+
 	/*
 	 * A command def rule allows only some options when LV is partial,
 	 * so handles_missing_pvs will only affect those.
 	 */
 	cmd->handles_missing_pvs = 1;
-	return process_each_lv(cmd, argc, argv, NULL, NULL, READ_FOR_UPDATE, NULL, _lvchange_properties_single);
+	ret = process_each_lv(cmd, argc, argv, NULL, NULL, READ_FOR_UPDATE,
+			       NULL, &_lvchange_properties_check, &_lvchange_properties_single);
+
+
+	/*
+	 * Unfortunately, lvchange has previously allowed changing an LV
+	 * property *and* changing LV activation in a single command.  This was
+	 * not a good idea because the behavior/results are hard to predict and
+	 * not possible to sensibly describe.  It's also unnecessary.  So, this
+	 * is here for the sake of compatibility.
+	 *
+	 * This is extremely ugly; activation should always be done separately.
+	 * This is not the full-featured lvchange capability, just the basic
+	 * (the advanced activate options are not provided.)
+	 *
+	 * FIXME: wrap this in a config setting that we can disable by default
+	 * to phase this out?
+	 */
+	if (arg_is_set(cmd, activate_ARG)) {
+		log_warn("WARNING: Combined property change and activation change is not advised.");
+		ret = lvchange_activate_cmd(cmd, argc, argv);
+	}
+
+
+	return ret;
 }
 
 static int _lvchange_activate_single(struct cmd_context *cmd,
@@ -1073,6 +1115,22 @@ static int _lvchange_activate_single(struct cmd_context *cmd,
 	return ECMD_PROCESSED;
 }
 
+static int _lvchange_activate_check(struct cmd_context *cmd,
+				     struct logical_volume *lv,
+				     struct processing_handle *handle,
+				     int lv_is_named_arg)
+{
+	if (!lv_is_visible(lv)) {
+		if (lv_is_named_arg)
+			log_error("Operation not permitted (%s %d) on hidden LV %s.",
+				  cmd->command->command_line_id, cmd->command->command_line_enum,
+				  display_lvname(lv));
+		return 0;
+	}
+
+	return 1;
+}
+
 int lvchange_activate_cmd(struct cmd_context *cmd, int argc, char **argv)
 {
 	cmd->handles_missing_pvs = 1;
@@ -1089,7 +1147,8 @@ int lvchange_activate_cmd(struct cmd_context *cmd, int argc, char **argv)
 	if (is_change_activating((activation_change_t)arg_uint_value(cmd, activate_ARG, CHANGE_AY)))
 		cmd->lockd_vg_enforce_sh = 1;
 
-	return process_each_lv(cmd, argc, argv, NULL, NULL, 0, NULL, _lvchange_activate_single);
+	return process_each_lv(cmd, argc, argv, NULL, NULL, 0,
+			       NULL, &_lvchange_activate_check, &_lvchange_activate_single);
 }
 
 static int _lvchange_refresh_single(struct cmd_context *cmd,
@@ -1101,15 +1160,40 @@ static int _lvchange_refresh_single(struct cmd_context *cmd,
 	if (!lv_refresh(cmd, lv))
 		return_ECMD_FAILED;
 
+	/*
+	 * FIXME: In some cases, the lv_refresh() starts polling without
+	 * checking poll arg.  Pull that out of lv_refresh.
+	 */
+	if (arg_is_set(cmd, poll_ARG) &&
+	    !_lvchange_background_polling(cmd, lv))
+		return_ECMD_FAILED;
+
 	return ECMD_PROCESSED;
 }
 
+static int _lvchange_refresh_check(struct cmd_context *cmd,
+				       struct logical_volume *lv,
+				       struct processing_handle *handle,
+				       int lv_is_named_arg)
+{
+	if (!lv_is_visible(lv)) {
+		if (lv_is_named_arg)
+			log_error("Operation not permitted (%s %d) on hidden LV %s.",
+				  cmd->command->command_line_id, cmd->command->command_line_enum,
+				  display_lvname(lv));
+		return 0;
+	}
+
+	return 1;
+}
+
 int lvchange_refresh_cmd(struct cmd_context *cmd, int argc, char **argv)
 {
 	cmd->handles_missing_pvs = 1;
 	cmd->lockd_vg_default_sh = 1;
 
-	return process_each_lv(cmd, argc, argv, NULL, NULL, 0, NULL, _lvchange_refresh_single);
+	return process_each_lv(cmd, argc, argv, NULL, NULL, 0,
+			       NULL, &_lvchange_refresh_check, &_lvchange_refresh_single);
 }
 
 static int _lvchange_resync_single(struct cmd_context *cmd,
@@ -1122,9 +1206,24 @@ static int _lvchange_resync_single(struct cmd_context *cmd,
 	return ECMD_PROCESSED;
 }
 
+static int _lvchange_resync_check(struct cmd_context *cmd,
+				       struct logical_volume *lv,
+				       struct processing_handle *handle,
+				       int lv_is_named_arg)
+{
+	if (!lv_is_visible(lv)) {
+		if (lv_is_named_arg)
+			return 1;
+		return 0;
+	}
+
+	return 1;
+}
+
 int lvchange_resync_cmd(struct cmd_context *cmd, int argc, char **argv)
 {
-	return process_each_lv(cmd, argc, argv, NULL, NULL, READ_FOR_UPDATE, NULL, _lvchange_resync_single);
+	return process_each_lv(cmd, argc, argv, NULL, NULL, READ_FOR_UPDATE,
+			       NULL, &_lvchange_resync_check, &_lvchange_resync_single);
 }
 
 static int _lvchange_syncaction_single(struct cmd_context *cmd,
@@ -1137,9 +1236,24 @@ static int _lvchange_syncaction_single(struct cmd_context *cmd,
 	return ECMD_PROCESSED;
 }
 
+static int _lvchange_syncaction_check(struct cmd_context *cmd,
+				       struct logical_volume *lv,
+				       struct processing_handle *handle,
+				       int lv_is_named_arg)
+{
+	if (!lv_is_visible(lv)) {
+		if (lv_is_named_arg)
+			return 1;
+		return 0;
+	}
+
+	return 1;
+}
+
 int lvchange_syncaction_cmd(struct cmd_context *cmd, int argc, char **argv)
 {
-	return process_each_lv(cmd, argc, argv, NULL, NULL, READ_FOR_UPDATE, NULL, _lvchange_syncaction_single);
+	return process_each_lv(cmd, argc, argv, NULL, NULL, READ_FOR_UPDATE,
+			       NULL, &_lvchange_syncaction_check, &_lvchange_syncaction_single);
 }
 
 static int _lvchange_rebuild_single(struct cmd_context *cmd,
@@ -1152,9 +1266,24 @@ static int _lvchange_rebuild_single(struct cmd_context *cmd,
 	return ECMD_PROCESSED;
 }
 
+static int _lvchange_rebuild_check(struct cmd_context *cmd,
+				     struct logical_volume *lv,
+				     struct processing_handle *handle,
+				     int lv_is_named_arg)
+{
+	if (!lv_is_visible(lv)) {
+		if (lv_is_named_arg)
+			return 1;
+		return 0;
+	}
+
+	return 1;
+}
+
 int lvchange_rebuild_cmd(struct cmd_context *cmd, int argc, char **argv)
 {
-	return process_each_lv(cmd, argc, argv, NULL, NULL, READ_FOR_UPDATE, NULL, _lvchange_rebuild_single);
+	return process_each_lv(cmd, argc, argv, NULL, NULL, READ_FOR_UPDATE,
+			       NULL, &_lvchange_rebuild_check, &_lvchange_rebuild_single);
 }
 
 static int _lvchange_monitor_poll_single(struct cmd_context *cmd,
@@ -1172,10 +1301,25 @@ static int _lvchange_monitor_poll_single(struct cmd_context *cmd,
 	return ECMD_PROCESSED;
 }
 
+static int _lvchange_monitor_poll_check(struct cmd_context *cmd,
+				     struct logical_volume *lv,
+				     struct processing_handle *handle,
+				     int lv_is_named_arg)
+{
+	if (!lv_is_visible(lv)) {
+		if (lv_is_named_arg)
+			return 1;
+		return 0;
+	}
+
+	return 1;
+}
+
 int lvchange_monitor_poll_cmd(struct cmd_context *cmd, int argc, char **argv)
 {
 	cmd->handles_missing_pvs = 1;
-	return process_each_lv(cmd, argc, argv, NULL, NULL, 0, NULL, _lvchange_monitor_poll_single);
+	return process_each_lv(cmd, argc, argv, NULL, NULL, 0,
+			       NULL, &_lvchange_monitor_poll_check, &_lvchange_monitor_poll_single);
 }
 
 static int _lvchange_persistent_single(struct cmd_context *cmd,
@@ -1188,10 +1332,27 @@ static int _lvchange_persistent_single(struct cmd_context *cmd,
 	return ECMD_PROCESSED;
 }
 
+static int _lvchange_persistent_check(struct cmd_context *cmd,
+				     struct logical_volume *lv,
+				     struct processing_handle *handle,
+				     int lv_is_named_arg)
+{
+	if (!lv_is_visible(lv)) {
+		if (lv_is_named_arg)
+			log_error("Operation not permitted (%s %d) on hidden LV %s.",
+				  cmd->command->command_line_id, cmd->command->command_line_enum,
+				  display_lvname(lv));
+		return 0;
+	}
+
+	return 1;
+}
+
 int lvchange_persistent_cmd(struct cmd_context *cmd, int argc, char **argv)
 {
 	cmd->handles_missing_pvs = 1;
-	return process_each_lv(cmd, argc, argv, NULL, NULL, READ_FOR_UPDATE, NULL, _lvchange_persistent_single);
+	return process_each_lv(cmd, argc, argv, NULL, NULL, READ_FOR_UPDATE,
+			       NULL, &_lvchange_persistent_check, &_lvchange_persistent_single);
 }
 
 int lvchange(struct cmd_context *cmd, int argc, char **argv)

tools/lvconvert.c

@@ -41,20 +41,17 @@ typedef enum {
 	/* Merge:
 	 *   If merge_snapshot is also set:
 	 *     merge thin snapshot LV into its origin
-	 *     merge old snapshot COW into its origin
 	 *   or if merge_mirror is also set
 	 *     merge LV previously split from mirror back into mirror
 	 */
 	CONV_MERGE = 1,
 
 	/* Split:
-	 *   For a snapshot, split it apart into COW and origin for future recombination
 	 *   For a cached LV, split it apart into the cached LV and its pool
 	 *   For a mirrored or raid LV, split mirror into two mirrors, optionally tracking
 	 *     future changes to the main mirror to allow future recombination.
 	 */
 	CONV_SPLIT = 2,
-	CONV_SPLIT_SNAPSHOT = 3,
 	CONV_SPLIT_CACHE = 4,
 	CONV_SPLIT_MIRRORS = 5,
 
@@ -64,9 +61,6 @@ typedef enum {
 	/* Destroy the cache attached to a cached LV */
 	CONV_UNCACHE = 7,
 
-	/* Reconstruct a snapshot from its origin and COW */
-	CONV_SNAPSHOT = 8,
-
 	/* Convert normal LV into one in a thin pool */
 	CONV_THIN = 11,
 
@@ -78,19 +72,16 @@ struct lvconvert_params {
 	/* Exactly one of these 12 command categories is determined */
 	int merge;		/* 1 */
 	int split;		/* 2 */
-	int splitsnapshot;	/* 3 */
 	int splitcache;		/* 4 */
 	int keep_mimages;	/* 5 */	/* --splitmirrors */
 	int cache;		/* 6 */
 	int uncache;		/* 7 */
-	int snapshot;		/* 8 */
 	int thin;		/* 11 */
 	/* other */		/* 12 */
 
 	/* FIXME Eliminate all cases where more than one of the above are set then use conv_type instead */
 	conversion_type_t	conv_type;
 
-	int merge_snapshot;	/* CONV_MERGE is set */
 	int merge_mirror;	/* CONV_MERGE is set */
 
 	int track_changes;	/* CONV_SPLIT_MIRRORS is set */
@@ -202,13 +193,6 @@ static int _lvconvert_name_params(struct lvconvert_params *lp,
 			return 0;
 		}
 
-		if (!strstr((*pargv)[0], "_rimage_")) {	/* Snapshot */
-			lp->type_str = SEG_TYPE_NAME_SNAPSHOT;
-			lp->merge_snapshot = 1;
-
-			return 1;
-		}
-
 		/* Mirror */
 		lp->merge_mirror = 1;
 	}
@@ -223,11 +207,6 @@ static int _lvconvert_name_params(struct lvconvert_params *lp,
 				  "the external origin.");
 			return 0;
 		}
-		if (lp->snapshot) {
-			log_error("Please specify a logical volume to act as "
-				  "the snapshot exception store.");
-			return 0;
-		}
 		if (lp->split) {
 			log_error("Logical volume for split is missing.");
 			return 0;
@@ -296,14 +275,6 @@ static int _lvconvert_name_params(struct lvconvert_params *lp,
 		return_0;
 
 	if (*pargc) {
-		if (lp->snapshot) {
-			log_error("Too many arguments provided for snapshots.");
-			return 0;
-		}
-		if (lp->splitsnapshot) {
-			log_error("Too many arguments provided with --splitsnapshot.");
-			return 0;
-		}
 		if (lp->splitcache) {
 			log_error("Too many arguments provided with --splitcache.");
 			return 0;
@@ -328,12 +299,6 @@ static int _lvconvert_name_params(struct lvconvert_params *lp,
 	return 1;
 }
 
-/* -s/--snapshot and --type snapshot are synonyms */
-static int _snapshot_type_requested(struct cmd_context *cmd, const char *type_str)
-{
-	return (arg_is_set(cmd, snapshot_ARG) || !strcmp(type_str, SEG_TYPE_NAME_SNAPSHOT));
-}
-
 static int _raid0_type_requested(const char *type_str)
 {
 	return (!strcmp(type_str, SEG_TYPE_NAME_RAID0) || !strcmp(type_str, SEG_TYPE_NAME_RAID0_META));
@@ -531,14 +496,6 @@ static int _read_params(struct cmd_context *cmd, int argc, char **argv,
 			return_0;
 		lp->splitcache = 1;
 		_set_conv_type(lp, CONV_SPLIT_CACHE);
-	} else if (arg_is_set(cmd, splitsnapshot_ARG)) {
-		if (arg_outside_list_is_set(cmd, "cannot be used with --splitsnapshot",
-					    splitsnapshot_ARG,
-					    force_ARG, noudevsync_ARG, test_ARG,
-					    -1))
-			return_0;
-		lp->splitsnapshot = 1;
-		_set_conv_type(lp, CONV_SPLIT_SNAPSHOT);
 	} else if (arg_is_set(cmd, uncache_ARG)) {
 		if (arg_outside_list_is_set(cmd, "cannot be used with --uncache",
 					    uncache_ARG,
@@ -585,15 +542,6 @@ static int _read_params(struct cmd_context *cmd, int argc, char **argv,
 	if (!_read_pool_params(cmd, &argc, &argv, lp))
 		return_0;
 
-	if (_snapshot_type_requested(cmd, lp->type_str)) {
-		if (lp->merge) {
-			log_error("--snapshot and --merge are mutually exclusive.");
-			return 0;
-		}
-		lp->snapshot = 1;
-		_set_conv_type(lp, CONV_SNAPSHOT);
-	}
-
 	if (lp->split) {
 		lp->lv_split_name = arg_str_value(cmd, name_ARG, NULL);
 
@@ -632,17 +580,17 @@ static int _read_params(struct cmd_context *cmd, int argc, char **argv,
 	}
 
 	/* If no other case was identified, then use of --stripes means --type striped */
-	if (!arg_is_set(cmd, type_ARG) && !*lp->type_str && !lp->merge && !lp->splitsnapshot &&
-	    !lp->splitcache && !lp->split && !lp->snapshot && !lp->uncache && !lp->cache && !lp->thin &&
+	if (!arg_is_set(cmd, type_ARG) && !*lp->type_str && !lp->merge &&
+	    !lp->splitcache && !lp->split && !lp->uncache && !lp->cache && !lp->thin &&
 	    !lp->mirrorlog && !lp->corelog &&
 	    (arg_is_set(cmd, stripes_long_ARG) || arg_is_set(cmd, stripesize_ARG)))
 		lp->type_str = SEG_TYPE_NAME_STRIPED;
 
-	if ((_snapshot_type_requested(cmd, lp->type_str) || lp->merge) &&
+	if (lp->merge &&
 	    (lp->mirrorlog || _mirror_or_raid_type_requested(cmd, lp->type_str) ||
 	     arg_is_set(cmd, thinpool_ARG) || _raid0_type_requested(lp->type_str) ||
 	     _striped_type_requested(lp->type_str))) {
-		log_error("--snapshot/--type snapshot or --merge argument "
+		log_error("--merge argument "
 			  "cannot be mixed with --mirrors/--type mirror/--type raid*/--stripes/--type striped/--type linear, "
 			  "--mirrorlog, --repair or --thinpool.");
 		return 0;
@@ -666,8 +614,8 @@ static int _read_params(struct cmd_context *cmd, int argc, char **argv,
 	lp->alloc = (alloc_policy_t) arg_uint_value(cmd, alloc_ARG, ALLOC_INHERIT);
 
 	/* We should have caught all these cases already. */
-	if (lp->merge + lp->splitsnapshot + lp->splitcache + lp->split + lp->uncache +
-	    lp->cache + lp->thin + lp->keep_mimages + lp->snapshot > 1) {
+	if (lp->merge + lp->splitcache + lp->split + lp->uncache +
+	    lp->cache + lp->thin + lp->keep_mimages > 1) {
 		log_error(INTERNAL_ERROR "Unexpected combination of incompatible options selected.");
 		return 0;
 	}
@@ -676,14 +624,12 @@ static int _read_params(struct cmd_context *cmd, int argc, char **argv,
 	/*
 	 * Final checking of each case:
 	 *   lp->merge
-	 *   lp->splitsnapshot
 	 *   lp->splitcache
 	 *   lp->split
 	 *   lp->uncache
 	 *   lp->cache
 	 *   lp->thin
 	 *   lp->keep_mimages
-	 *   lp->snapshot
 	 *   --type mirror|raid  lp->mirrorlog lp->corelog
 	 *   --type raid0|striped
 	 */
@@ -692,46 +638,10 @@ static int _read_params(struct cmd_context *cmd, int argc, char **argv,
 	case CONV_SPLIT:
 	case CONV_SPLIT_CACHE:
 	case CONV_SPLIT_MIRRORS:
-	case CONV_SPLIT_SNAPSHOT:	/* Destroy snapshot retaining cow as separate LV */
 	case CONV_CACHE:
 	case CONV_UNCACHE:
 	case CONV_THIN:
                 break;
-	case CONV_SNAPSHOT:	/* Snapshot creation from pre-existing cow */
-		if (!argc) {
-			log_error("Please provide logical volume path for snapshot origin.");
-			return 0;
-		}
-		lp->origin_name = argv[0];
-		argv++, argc--;
-
-		if (arg_is_set(cmd, regionsize_ARG)) {
-			log_error("--regionsize is only available with mirrors");
-			return 0;
-		}
-
-		if (arg_is_set(cmd, stripesize_ARG) || arg_is_set(cmd, stripes_long_ARG)) {
-			log_error("--stripes and --stripesize are only available with striped mirrors");
-			return 0;
-		}
-
-		if (arg_is_set(cmd, chunksize_ARG) &&
-		    (arg_sign_value(cmd, chunksize_ARG, SIGN_NONE) == SIGN_MINUS)) {
-			log_error("Negative chunk size is invalid.");
-			return 0;
-		}
-
-		lp->chunk_size = arg_uint_value(cmd, chunksize_ARG, 8);
-		if (lp->chunk_size < 8 || lp->chunk_size > 1024 ||
-		    !is_power_of_2(lp->chunk_size)) {
-			log_error("Chunk size must be a power of 2 in the "
-				  "range 4K to 512K");
-			return 0;
-		}
-		log_verbose("Setting chunk size to %s.", display_size(cmd, lp->chunk_size));
-
-		lp->type_str = SEG_TYPE_NAME_SNAPSHOT;
-		break;
 
 	case CONV_OTHER:
 		if (_mirror_or_raid_type_requested(cmd, lp->type_str) ||
@@ -2166,12 +2076,25 @@ static int _lvconvert_snapshot(struct cmd_context *cmd,
 		return_0;
 
 	/*
-	 * lv_is_prop checks here cannot be automated by command definition
-	 * rules because they are not done on the LV being processed.
+	 * check_lv_rules() checks cannot be done via command definition
+	 * rules because this LV is not processed by process_each_lv.
 	 */
 	if (lv_is_locked(org) || lv_is_pvmove(org)) {
-		log_error("Unable to convert an LV into a snapshot of a %s LV.",
-			  lv_is_locked(org) ? "locked" : "pvmove");
+		log_error("Unable to use LV %s as snapshot origin: LV is %s.",
+			  display_lvname(lv), lv_is_locked(org) ? "locked" : "pvmove");
+		return 0;
+	}
+
+	/*
+	 * check_lv_types() checks cannot be done via command definition
+	 * LV_foo specification because this LV is not processed by process_each_lv.
+	 */
+	if (lv_is_cache_type(org) ||
+	    lv_is_thin_type(org) ||
+	    lv_is_mirrored(org) ||
+	    lv_is_cow(org)) {
+		log_error("Unable to use LV %s as snapshot origin: invald LV type.",
+			  display_lvname(lv));
 		return 0;
 	}
 
@@ -3363,26 +3286,6 @@ static int _lvconvert_cache(struct cmd_context *cmd,
  * moved into the _convert_lvtype_operation() functions below.
  */
 
-/*
- * Separate a COW snapshot LV from its origin.
- * lvconvert --splitsnapshot LV
- */
-static int _convert_cow_snapshot_splitsnapshot(struct cmd_context *cmd, struct logical_volume *lv,
-					       struct lvconvert_params *lp)
-{
-	return _lvconvert_splitsnapshot(cmd, lv);
-}
-
-/*
- * Merge a COW snapshot LV into its origin.
- * lvconvert --merge LV
- */
-static int _convert_cow_snapshot_merge(struct cmd_context *cmd, struct logical_volume *lv,
-				       struct lvconvert_params *lp)
-{
-	/* return _lvconvert_merge_old_snapshot(cmd, lv, lp); */
-}
-
 /*
  * Merge a snapshot thin LV into its origin.
  * lvconvert --merge LV
@@ -3663,20 +3566,6 @@ static int _convert_raid_merge(struct cmd_context *cmd, struct logical_volume *l
 	return _lvconvert_raid(lv, lp);
 }
 
-/*
- * Combine a raid* LV with a snapshot LV that was previously
- * split from the raid* LV using --splitsnapshot.
- * lvconvert --type snapshot LV SnapshotLV
- *
- * Alternate syntax:
- * lvconvert --snapshot LV SnapshotLV
- */
-static int _convert_raid_snapshot(struct cmd_context *cmd, struct logical_volume *lv,
-				  struct lvconvert_params *lp)
-{
-	return _lvconvert_snapshot(cmd, lv, lp->origin_name);
-}
-
 /*
  * Convert a raid* LV to a thin LV with an external origin.
  * lvconvert --type thin LV
@@ -3802,20 +3691,6 @@ static int _convert_striped_merge(struct cmd_context *cmd, struct logical_volume
 	return _lvconvert_raid(lv, lp);
 }
 
-/*
- * Combine a linear/striped LV with a snapshot LV that was previously
- * split from the linear/striped LV using --splitsnapshot.
- * lvconvert --type snapshot LV SnapshotLV
- *
- * Alternate syntax:
- * lvconvert --snapshot LV SnapshotLV
- */
-static int _convert_striped_snapshot(struct cmd_context *cmd, struct logical_volume *lv,
-				     struct lvconvert_params *lp)
-{
-	return _lvconvert_snapshot(cmd, lv, lp->origin_name);
-}
-
 /*
  * Convert a striped/linear LV to a thin LV with an external origin.
  * lvconvert --type thin LV
@@ -3930,22 +3805,6 @@ static int _convert_striped_raid(struct cmd_context *cmd, struct logical_volume
  *
  * _convert_<lvtype>
  */
-static int _convert_cow_snapshot(struct cmd_context *cmd, struct logical_volume *lv,
-				 struct lvconvert_params *lp)
-{
-	if (lp->splitsnapshot)
-		return _convert_cow_snapshot_splitsnapshot(cmd, lv, lp);
-
-	/* FIXME: add --merge-snapshot to make this distinct from --merge-mirror. */
-	if (lp->merge)
-		return _convert_cow_snapshot_merge(cmd, lv, lp);
-
-	log_error("Operation not permitted on COW snapshot LV %s.", display_lvname(lv));
-	log_error("Operations permitted on a COW snapshot LV are:\n"
-		  "  --splitsnapshot\n"
-		  "  --merge\n");
-	return 0;
-}
 
 static int _convert_thin_volume(struct cmd_context *cmd, struct logical_volume *lv,
 				struct lvconvert_params *lp)
@@ -4147,9 +4006,6 @@ static int _convert_raid(struct cmd_context *cmd, struct logical_volume *lv,
 		if (lp->merge)
 			return _convert_raid_merge(cmd, lv, lp);
 
-		if (!strcmp(lp->type_str, SEG_TYPE_NAME_SNAPSHOT) || arg_is_set(cmd, snapshot_ARG))
-			return _convert_raid_snapshot(cmd, lv, lp);
-
 		if (lp->thin)
 			return _convert_raid_thin(cmd, lv, lp);
 
@@ -4181,9 +4037,6 @@ static int _convert_striped(struct cmd_context *cmd, struct logical_volume *lv,
 	if (lp->merge)
 		return _convert_striped_merge(cmd, lv, lp);
 
-	if (lp->snapshot || !strcmp(lp->type_str, SEG_TYPE_NAME_SNAPSHOT))
-		return _convert_striped_snapshot(cmd, lv, lp);
-
 	if (lp->thin)
 		return _convert_striped_thin(cmd, lv, lp);
 
@@ -4322,18 +4175,10 @@ static int _lvconvert(struct cmd_context *cmd, struct logical_volume *lv,
 				lp->stripes = 0;
 	}
 
-	if (lp->snapshot)
-		lp->zero = (lp->segtype->flags & SEG_CANNOT_BE_ZEROED) ? 0 : arg_int_value(cmd, zero_ARG, 1);
-
 	/*
 	 * Each LV type that can be converted.
 	 * (The existing type of the LV, not a requested type.)
 	 */
-	if (lv_is_cow(lv)) {
-		ret = _convert_cow_snapshot(cmd, lv, lp);
-		goto out;
-	}
-
 	if (lv_is_thin_volume(lv)) {
 		ret = _convert_thin_volume(cmd, lv, lp);
 		goto out;
@@ -4490,7 +4335,7 @@ int lvconvert(struct cmd_context * cmd, int argc, char **argv)
 
 	if (lp.merge) {
 		ret = process_each_lv(cmd, argc, argv, NULL, NULL,
-				      READ_FOR_UPDATE, handle, &_lvconvert_merge_single);
+				      READ_FOR_UPDATE, handle, NULL, &_lvconvert_merge_single);
 	} else {
 		int saved_ignore_suspended_devices = ignore_suspended_devices();
 
@@ -4500,7 +4345,7 @@ int lvconvert(struct cmd_context * cmd, int argc, char **argv)
 		}
 
 		ret = process_each_lv(cmd, 0, NULL, lp.vg_name, lp.lv_name,
-				      READ_FOR_UPDATE, handle, &_lvconvert_single);
+				      READ_FOR_UPDATE, handle, NULL, &_lvconvert_single);
 
 		init_ignore_suspended_devices(saved_ignore_suspended_devices);
 	}
@@ -4546,6 +4391,36 @@ out:
  */
 
 
+/*
+ * FIXME: it's very unlikely that the same !visible exceptions apply to every
+ * lvconvert command.  Add specific !visible exceptions in command-specific
+ * check functions.
+ */
+
+static int _lvconvert_generic_check(struct cmd_context *cmd, struct logical_volume *lv,
+			struct processing_handle *handle,
+			int lv_is_named_arg)
+{
+	if (!lv_is_visible(lv)) {
+		if (lv_is_cache_pool_metadata(lv) ||
+		    lv_is_cache_pool_data(lv) ||
+		    lv_is_thin_pool_metadata(lv) ||
+		    lv_is_thin_pool_data(lv) ||
+		    lv_is_used_cache_pool(lv) ||
+		    lv_is_mirrored(lv) ||
+		    lv_is_raid(lv)) {
+			return 1;
+		}
+
+		log_error("Operation not permitted (%s %d) on hidden LV %s.",
+			  cmd->command->command_line_id, cmd->command->command_line_enum,
+			  display_lvname(lv));
+		return 0;
+	}
+
+	return 1;
+}
+
 /*
  * Data/results accumulated during processing.
  */
@@ -4750,8 +4625,8 @@ int lvconvert_repair_pvs_or_thinpool_cmd(struct cmd_context *cmd, int argc, char
 
 	cmd->handles_missing_pvs = 1;
 
-	ret = process_each_lv(cmd, 1, cmd->position_argv, NULL, NULL, READ_FOR_UPDATE, handle,
-			      &_lvconvert_repair_pvs_or_thinpool_single);
+	ret = process_each_lv(cmd, 1, cmd->position_argv, NULL, NULL, READ_FOR_UPDATE,
+			      handle, &_lvconvert_generic_check, &_lvconvert_repair_pvs_or_thinpool_single);
 
 	init_ignore_suspended_devices(saved_ignore_suspended_devices);
 
@@ -4827,8 +4702,8 @@ int lvconvert_replace_pv_cmd(struct cmd_context *cmd, int argc, char **argv)
 
 	handle->custom_handle = &lr;
 
-	ret = process_each_lv(cmd, 1, cmd->position_argv, NULL, NULL, READ_FOR_UPDATE, handle,
-			      &_lvconvert_replace_pv_single);
+	ret = process_each_lv(cmd, 1, cmd->position_argv, NULL, NULL, READ_FOR_UPDATE,
+			      handle, &_lvconvert_generic_check, &_lvconvert_replace_pv_single);
 
 	destroy_processing_handle(cmd, handle);
 
@@ -4857,7 +4732,7 @@ static int _lvconvert_merge_snapshot_single(struct cmd_context *cmd,
 		return_ECMD_FAILED;
 
 	if (lv_to_poll) {
-		if (!(idl = _convert_poll_id_list_create(cmd, lv)))
+		if (!(idl = _convert_poll_id_list_create(cmd, lv_to_poll)))
 			return_ECMD_FAILED;
 		dm_list_add(&lr->poll_idls, &idl->list);
 		lr->need_polling = 1;
@@ -4866,6 +4741,20 @@ static int _lvconvert_merge_snapshot_single(struct cmd_context *cmd,
 	return ECMD_PROCESSED;
 }
 
+static int _lvconvert_merge_snapshot_check(struct cmd_context *cmd, struct logical_volume *lv,
+			struct processing_handle *handle,
+			int lv_is_named_arg)
+{
+	if (!lv_is_visible(lv)) {
+		log_error("Operation not permitted (%s %d) on hidden LV %s.",
+			  cmd->command->command_line_id, cmd->command->command_line_enum,
+			  display_lvname(lv));
+		return 0;
+	}
+
+	return 1;
+}
+
 int lvconvert_merge_snapshot_cmd(struct cmd_context *cmd, int argc, char **argv)
 {
 	struct processing_handle *handle;
@@ -4882,13 +4771,13 @@ int lvconvert_merge_snapshot_cmd(struct cmd_context *cmd, int argc, char **argv)
 
 	handle->custom_handle = &lr;
 
-	ret = process_each_lv(cmd, cmd->position_argc, cmd->position_argv, NULL, NULL, READ_FOR_UPDATE, handle,
-			      _lvconvert_merge_snapshot_single);
+	ret = process_each_lv(cmd, cmd->position_argc, cmd->position_argv, NULL, NULL, READ_FOR_UPDATE,
+			      handle, &_lvconvert_merge_snapshot_check, &_lvconvert_merge_snapshot_single);
 
 	if (lr.need_polling) {
 		dm_list_iterate_items(idl, &lr.poll_idls) {
 			poll_ret = _lvconvert_poll_by_id(cmd, idl->id,
-						arg_is_set(cmd, background_ARG), 0, 0);
+						arg_is_set(cmd, background_ARG), 1, 0);
 			if (poll_ret > ret)
 				ret = poll_ret;
 		}
@@ -4906,11 +4795,6 @@ int lvconvert_merge_snapshot_cmd(struct cmd_context *cmd, int argc, char **argv)
  * lvconvert_split_cow_snapshot
  */
 
-/*
- * TODO: ensure that LV_snapshot implies that the LV cannot be thinpool,
- * mirror_type, raid_type, thin_type
- */
-
 static int _lvconvert_split_snapshot_single(struct cmd_context *cmd,
                                        struct logical_volume *lv,
                                        struct processing_handle *handle)
@@ -4923,25 +4807,61 @@ static int _lvconvert_split_snapshot_single(struct cmd_context *cmd,
 
 int lvconvert_split_snapshot_cmd(struct cmd_context *cmd, int argc, char **argv)
 {
-	return process_each_lv(cmd, 1, cmd->position_argv, NULL, NULL, READ_FOR_UPDATE, NULL,
-			       _lvconvert_split_snapshot_single);
+	return process_each_lv(cmd, 1, cmd->position_argv, NULL, NULL, READ_FOR_UPDATE,
+			       NULL, &_lvconvert_generic_check, &_lvconvert_split_snapshot_single);
 }
 
 /*
- * Combine an origin LV with a snapshot LV (cow) that was previously split from
- * the origin using --splitsnapshot.
- */
-
-/*
- * TODO: ensure that LV_snapshot implies that the LV cannot be other types
- * that were checked for: lv_is_cache_type lv_is_mirrored lv_is_pool lv_is_thick_origin
+ * Combine two LVs that were once an origin/cow pair of LVs, were then
+ * separated with --splitsnapshot, and now with this command are combined again
+ * into the origin/cow pair.
+ *
+ * This is an obscure command that has little to no real uses.
+ *
+ * The command has unusual handling of position args.  The first position arg
+ * will become the origin LV, and is not processed by process_each_lv.  The
+ * second position arg will become the cow LV and is processed by
+ * process_each_lv.
+ *
+ * The single function can grab the origin LV from position_argv[0].
+ *
+ * begin with an ordinary LV foo:
+ * lvcreate -n foo -L 1 vg
+ *
+ * create a cow snapshot of foo named foosnap:
+ * lvcreate -s -L 1 -n foosnap vg/foo
+ *
+ * now, foo is an "origin LV" and foosnap is a "cow LV"
+ * (foosnap matches LV_snapshot aka lv_is_cow)
+ *
+ * split the two LVs apart:
+ * lvconvert --splitsnapshot vg/foosnap
+ *
+ * now, foo is *not* an origin LV and foosnap is *not* a cow LV
+ * (foosnap does not match LV_snapshot)
+ *
+ * now, combine the two LVs again:
+ * lvconvert --snapshot vg/foo vg/foosnap
+ *
+ * after this, foosnap will match LV_snapshot again.
+ *
+ * FIXME: when splitsnapshot is run, the previous cow LV should be
+ * flagged in the metadata somehow, and then that flag should be
+ * required here.  As it is now, the first and second args
+ * (origin and cow) can be swapped and nothing catches it.
  */
 
 static int _lvconvert_combine_split_snapshot_single(struct cmd_context *cmd,
                                        struct logical_volume *lv,
                                        struct processing_handle *handle)
 {
-	if (!_lvconvert_snapshot(cmd, lv, cmd->position_argv[0]))
+	const char *origin_name = cmd->position_argv[0];
+
+	/* If origin_name includes VG name, the VG name is removed. */
+	if (!validate_lvname_param(cmd, &lv->vg->name, &origin_name))
+		return_ECMD_FAILED;
+
+	if (!_lvconvert_snapshot(cmd, lv, origin_name))
 		return_ECMD_FAILED;
 
 	return ECMD_PROCESSED;
@@ -4949,20 +4869,7 @@ static int _lvconvert_combine_split_snapshot_single(struct cmd_context *cmd,
 
 int lvconvert_combine_split_snapshot_cmd(struct cmd_context *cmd, int argc, char **argv)
 {
-	const char *origin_name;
-
-	origin_name = cmd->position_argv[0];
-	/* TODO: validate origin_name, see validate_restricted_lvname_param */
-
-	/*
-	 * The command has unusual handling of position args.  The first
-	 * position arg is the origin name, and that LV is not processed by
-	 * process_each_lv.  The second position arg is the snapshot LV that's
-	 * processed by process_each_lv.  The single function can grab the
-	 * origin LV from position_argv[0].
-	 */
-
-	return process_each_lv(cmd, 1, cmd->position_argv + 1, NULL, NULL, READ_FOR_UPDATE, NULL,
-			       _lvconvert_combine_split_snapshot_single);
+	return process_each_lv(cmd, 1, cmd->position_argv + 1, NULL, NULL, READ_FOR_UPDATE,
+			       NULL, &_lvconvert_generic_check, &_lvconvert_combine_split_snapshot_single);
 }
 

tools/lvdisplay.c

@@ -58,5 +58,5 @@ int lvdisplay(struct cmd_context *cmd, int argc, char **argv)
 		return EINVALID_CMD_LINE;
 	}
 
-	return process_each_lv(cmd, argc, argv, NULL, NULL, 0, NULL, &_lvdisplay_single);
+	return process_each_lv(cmd, argc, argv, NULL, NULL, 0, NULL, NULL, &_lvdisplay_single);
 }

tools/lvremove.c

@@ -27,5 +27,5 @@ int lvremove(struct cmd_context *cmd, int argc, char **argv)
 	cmd->include_historical_lvs = 1;
 
 	return process_each_lv(cmd, argc, argv, NULL, NULL, READ_FOR_UPDATE, NULL,
-			       &lvremove_single);
+			       NULL, &lvremove_single);
 }

tools/lvscan.c

@@ -119,5 +119,5 @@ int lvscan(struct cmd_context *cmd, int argc, char **argv)
 		 */
 	}
 
-	return process_each_lv(cmd, argc, argv, NULL, NULL, 0, NULL, &lvscan_single);
+	return process_each_lv(cmd, argc, argv, NULL, NULL, 0, NULL, NULL, &lvscan_single);
 }

tools/reporter.c

@@ -517,14 +517,14 @@ static int _report_all_in_vg(struct cmd_context *cmd, struct processing_handle *
 			r = _vgs_single(cmd, vg->name, vg, handle);
 			break;
 		case LVS:
-			r = process_each_lv_in_vg(cmd, vg, NULL, NULL, 0, handle,
+			r = process_each_lv_in_vg(cmd, vg, NULL, NULL, 0, handle, NULL,
 						  do_lv_info && !do_lv_seg_status ? &_lvs_with_info_single :
 						  !do_lv_info && do_lv_seg_status ? &_lvs_with_status_single :
 						  do_lv_info && do_lv_seg_status ? &_lvs_with_info_and_status_single :
 										   &_lvs_single);
 			break;
 		case SEGS:
-			r = process_each_lv_in_vg(cmd, vg, NULL, NULL, 0, handle,
+			r = process_each_lv_in_vg(cmd, vg, NULL, NULL, 0, handle, NULL,
 						  do_lv_info && !do_lv_seg_status ? &_lvsegs_with_info_single :
 						  !do_lv_info && do_lv_seg_status ? &_lvsegs_with_status_single :
 						  do_lv_info && do_lv_seg_status ? &_lvsegs_with_info_and_status_single :
@@ -1099,7 +1099,7 @@ static int _do_report(struct cmd_context *cmd, struct processing_handle *handle,
 			if (args->full_report_vg)
 				r = _report_all_in_vg(cmd, handle, args->full_report_vg, LVS, lv_info_needed, lv_segment_status_needed);
 			else
-				r = process_each_lv(cmd, args->argc, args->argv, NULL, NULL, 0, handle,
+				r = process_each_lv(cmd, args->argc, args->argv, NULL, NULL, 0, handle, NULL,
 						    lv_info_needed && !lv_segment_status_needed ? &_lvs_with_info_single :
 						    !lv_info_needed && lv_segment_status_needed ? &_lvs_with_status_single :
 						    lv_info_needed && lv_segment_status_needed ? &_lvs_with_info_and_status_single :
@@ -1133,7 +1133,7 @@ static int _do_report(struct cmd_context *cmd, struct processing_handle *handle,
 			if (args->full_report_vg)
 				r = _report_all_in_vg(cmd, handle, args->full_report_vg, SEGS, lv_info_needed, lv_segment_status_needed);
 			else
-				r = process_each_lv(cmd, args->argc, args->argv, NULL, NULL, 0, handle,
+				r = process_each_lv(cmd, args->argc, args->argv, NULL, NULL, 0, handle, NULL,
 						    lv_info_needed && !lv_segment_status_needed ? &_lvsegs_with_info_single :
 						    !lv_info_needed && lv_segment_status_needed ? &_lvsegs_with_status_single :
 						    lv_info_needed && lv_segment_status_needed ? &_lvsegs_with_info_and_status_single :

tools/toollib.c

@@ -2529,6 +2529,10 @@ static int _lv_is_type(struct cmd_context *cmd, struct logical_volume *lv, int l
 #endif
 	case raid10_LVT:
 		return seg_is_raid10(seg);
+	case error_LVT:
+		return !strcmp(seg->segtype->name, SEG_TYPE_NAME_ERROR);
+	case zero_LVT:
+		return !strcmp(seg->segtype->name, SEG_TYPE_NAME_ZERO);
 	default:
 		log_error(INTERNAL_ERROR "unknown lv type value lvt_enum %d", lvt_enum);
 	}
@@ -2573,7 +2577,11 @@ static int _get_lvt_enum(struct logical_volume *lv)
 	if (seg_is_raid10(seg))
 		return raid10_LVT;
 
-	log_error(INTERNAL_ERROR "unknown lv type for %s", display_lvname(lv));
+	if (!strcmp(seg->segtype->name, SEG_TYPE_NAME_ERROR))
+		return error_LVT;
+	if (!strcmp(seg->segtype->name, SEG_TYPE_NAME_ZERO))
+		return zero_LVT;
+
 	return 0;
 }
 
@@ -2669,35 +2677,29 @@ static int _lv_props_match(struct cmd_context *cmd, struct logical_volume *lv, u
 	return !found_a_mismatch;
 }
 
-/*
- * If the command definition specifies one required positional
- * LV (possibly repeatable), and specifies accepted LV types,
- * then verify that the LV being processed matches one of those
- * types.
- *
- * process_each_lv() can only be used for commands that have
- * one positional LV arg (optionally repeating, where each is
- * processed independently.)  It cannot work for commands that
- * have different required LVs in designated positions, like
- * 'lvrename LV1 LV2', where each LV is not processed
- * independently.  That means that this LV type check only
- * needs to check the lv_type of the first positional arg.
- */
-
-static int _check_lv_types(struct cmd_context *cmd, struct logical_volume *lv)
+static int _check_lv_types(struct cmd_context *cmd, struct logical_volume *lv, int pos)
 {
 	int ret = 1;
 
-	if ((cmd->command->rp_count == 1) &&
-	    val_bit_is_set(cmd->command->required_pos_args[0].def.val_bits, lv_VAL) &&
-	    cmd->command->required_pos_args[0].def.lvt_bits) {
-		ret = _lv_types_match(cmd, lv, cmd->command->required_pos_args[0].def.lvt_bits, NULL, NULL);
-		if (!ret) {
-			int lvt_enum = _get_lvt_enum(lv);
-			struct lv_types *type = get_lv_type(lvt_enum);
-			log_warn("Operation on LV %s which has invalid type %s.",
-				 display_lvname(lv), type ? type->name : "unknown");
-		}
+	if (!pos)
+		return 1;
+
+	if (!cmd->command->required_pos_args[pos-1].def.lvt_bits)
+		return 1;
+
+	if (!val_bit_is_set(cmd->command->required_pos_args[pos-1].def.val_bits, lv_VAL)) {
+		log_error(INTERNAL_ERROR "Command (%s %d) arg position %d does not permit an LV (%llx)",
+			  cmd->command->command_line_id, cmd->command->command_line_enum,
+			  pos, (unsigned long long)cmd->command->required_pos_args[pos-1].def.val_bits);
+		return 0;
+	}
+
+	ret = _lv_types_match(cmd, lv, cmd->command->required_pos_args[pos-1].def.lvt_bits, NULL, NULL);
+	if (!ret) {
+		int lvt_enum = _get_lvt_enum(lv);
+		struct lv_types *type = get_lv_type(lvt_enum);
+		log_warn("Operation on LV %s which has invalid type %s.",
+			 display_lvname(lv), type ? type->name : "unknown");
 	}
 
 	return ret;
@@ -2861,10 +2863,64 @@ static int _check_lv_rules(struct cmd_context *cmd, struct logical_volume *lv)
 	return ret;
 }
 
+/*
+ * Return which arg position the given LV is at,
+ * where 1 represents the first position arg.
+ * When the first position arg is repeatable,
+ * return 1 for all.
+ *
+ * Return 0 when the command has no required
+ * position args. (optional position args are
+ * not considered.)
+ */
+
+static int _find_lv_arg_position(struct cmd_context *cmd, struct logical_volume *lv)
+{
+	const char *sep, *lvname;
+	int i;
+
+	if (cmd->command->rp_count == 0)
+		return 0;
+
+	if (cmd->command->rp_count == 1)
+		return 1;
+
+	for (i = 0; i < cmd->position_argc; i++) {
+		if (i == cmd->command->rp_count)
+			break;
+
+		if (!val_bit_is_set(cmd->command->required_pos_args[i].def.val_bits, lv_VAL))
+			continue;
+
+		if ((sep = strstr(cmd->position_argv[i], "/")))
+			lvname = sep + 1;
+		else
+			lvname = cmd->position_argv[i];
+
+		if (!strcmp(lvname, lv->name))
+			return i + 1;
+	}
+
+	/*
+	 * If the last position arg is an LV and this
+	 * arg is beyond that position, then the last
+	 * LV position arg is repeatable, so return
+	 * that position.
+	 */
+	if (i == cmd->command->rp_count) {
+		int last_pos = cmd->command->rp_count;
+		if (val_bit_is_set(cmd->command->required_pos_args[last_pos-1].def.val_bits, lv_VAL))
+			return last_pos;
+	}
+
+	return 0;
+}
+
 int process_each_lv_in_vg(struct cmd_context *cmd, struct volume_group *vg,
 			  struct dm_list *arg_lvnames, const struct dm_list *tags_in,
 			  int stop_on_error,
 			  struct processing_handle *handle,
+			  check_single_lv_fn_t check_single_lv,
 			  process_single_lv_fn_t process_single_lv)
 {
 	log_report_t saved_log_report_state = log_get_report_state();
@@ -2874,10 +2930,12 @@ int process_each_lv_in_vg(struct cmd_context *cmd, struct volume_group *vg,
 	int ret = 0;
 	int whole_selected = 0;
 	int handle_supplied = handle != NULL;
+	int lv_is_named_arg;
 	unsigned process_lv;
 	unsigned process_all = 0;
 	unsigned tags_supplied = 0;
 	unsigned lvargs_supplied = 0;
+	int lv_arg_pos;
 	struct lv_list *lvl;
 	struct dm_str_list *sl;
 	struct dm_list final_lvs;
@@ -3035,42 +3093,40 @@ int process_each_lv_in_vg(struct cmd_context *cmd, struct volume_group *vg,
 		if (lv_is_removed(lvl->lv))
 			continue;
 
+		lv_is_named_arg = str_list_match_item(&found_arg_lvnames, lvl->lv->name);
+
+		lv_arg_pos = _find_lv_arg_position(cmd, lvl->lv);
+
 		/*
 		 * The command definition may include restrictions on the
 		 * types and properties of LVs that can be processed.
 		 */
 
-		if (!_check_lv_types(cmd, lvl->lv)) {
+		if (!_check_lv_types(cmd, lvl->lv, lv_arg_pos)) {
 			/* FIXME: include this result in report log? */
-			/* FIXME: avoid duplicating message for each level */
-
-			if (str_list_match_item(&found_arg_lvnames, lvl->lv->name)) {
+			if (lv_is_named_arg) {
 				log_error("Operation not permitted (%s %d) on LV %s.",
 					  cmd->command->command_line_id, cmd->command->command_line_enum,
 					  display_lvname(lvl->lv));
 				ret_max = ECMD_FAILED;
-			} else {
-				log_warn("Operation not permitted (%s %d) on LV %s.",
-					 cmd->command->command_line_id, cmd->command->command_line_enum,
-					 display_lvname(lvl->lv));
 			}
 			continue;
 		}
 
 		if (!_check_lv_rules(cmd, lvl->lv)) {
 			/* FIXME: include this result in report log? */
-			/* FIXME: avoid duplicating message for each level */
-
-			if (str_list_match_item(&found_arg_lvnames, lvl->lv->name)) {
+			if (lv_is_named_arg) {
 				log_error("Operation not permitted (%s %d) on LV %s.",
 					  cmd->command->command_line_id, cmd->command->command_line_enum,
 					  display_lvname(lvl->lv));
 				ret_max = ECMD_FAILED;
-			} else {
-				log_warn("Operation not permitted (%s %d) on LV %s.",
-					 cmd->command->command_line_id, cmd->command->command_line_enum,
-					 display_lvname(lvl->lv));
-			}
+			} 
+			continue;
+		}
+
+		if (check_single_lv && !check_single_lv(cmd, lvl->lv, handle, lv_is_named_arg)) {
+			if (lv_is_named_arg)
+				ret_max = ECMD_FAILED;
 			continue;
 		}
 
@@ -3310,6 +3366,7 @@ static int _process_lv_vgnameid_list(struct cmd_context *cmd, uint32_t read_flag
 				     struct dm_list *arg_lvnames,
 				     struct dm_list *arg_tags,
 				     struct processing_handle *handle,
+				     check_single_lv_fn_t check_single_lv,
 				     process_single_lv_fn_t process_single_lv)
 {
 	log_report_t saved_log_report_state = log_get_report_state();
@@ -3402,7 +3459,7 @@ static int _process_lv_vgnameid_list(struct cmd_context *cmd, uint32_t read_flag
 			goto endvg;
 
 		ret = process_each_lv_in_vg(cmd, vg, &lvnames, tags_arg, 0,
-					    handle, process_single_lv);
+					    handle, check_single_lv, process_single_lv);
 		if (ret != ECMD_PROCESSED)
 			stack;
 		report_log_ret_code(ret);
@@ -3433,6 +3490,7 @@ int process_each_lv(struct cmd_context *cmd,
 		    const char *one_vgname, const char *one_lvname,
 		    uint32_t read_flags,
 		    struct processing_handle *handle,
+		    check_single_lv_fn_t check_single_lv,
 		    process_single_lv_fn_t process_single_lv)
 {
 	log_report_t saved_log_report_state = log_get_report_state();
@@ -3548,7 +3606,7 @@ int process_each_lv(struct cmd_context *cmd,
 		_choose_vgs_to_process(cmd, &arg_vgnames, &vgnameids_on_system, &vgnameids_to_process);
 
 	ret = _process_lv_vgnameid_list(cmd, read_flags, &vgnameids_to_process, &arg_vgnames, &arg_lvnames,
-					&arg_tags, handle, process_single_lv);
+					&arg_tags, handle, check_single_lv, process_single_lv);
 
 	if (ret > ret_max)
 		ret_max = ret;

tools/toollib.h

@@ -98,6 +98,18 @@ typedef int (*process_single_pvseg_fn_t) (struct cmd_context * cmd,
 					  struct pv_segment * pvseg,
 					  struct processing_handle *handle);
 
+/*
+ * Called prior to process_single_lv() to decide if the LV should be
+ * processed.  If this returns 0, the LV is not processed.
+ *
+ * This can evaluate the combination of command definition and
+ * the LV object to decide if the combination is allowed.
+ */
+typedef int (*check_single_lv_fn_t) (struct cmd_context *cmd,
+				     struct logical_volume *lv,
+				     struct processing_handle *handle,
+				     int lv_is_named_arg);
+
 int process_each_vg(struct cmd_context *cmd,
 	            int argc, char **argv,
 		    const char *one_vgname,
@@ -125,6 +137,7 @@ int process_each_segment_in_pv(struct cmd_context *cmd,
 int process_each_lv(struct cmd_context *cmd, int argc, char **argv,
 		    const char *one_vgname, const char *one_lvname,
 		    uint32_t flags, struct processing_handle *handle,
+		    check_single_lv_fn_t check_single_lv,
 		    process_single_lv_fn_t process_single_lv);
 
 
@@ -141,6 +154,7 @@ int process_each_pv_in_vg(struct cmd_context *cmd, struct volume_group *vg,
 int process_each_lv_in_vg(struct cmd_context *cmd, struct volume_group *vg,
 			  struct dm_list *arg_lvnames, const struct dm_list *tagsl,
 			  int stop_on_error, struct processing_handle *handle,
+			  check_single_lv_fn_t check_single_lv,
 			  process_single_lv_fn_t process_single_lv);
 
 struct processing_handle *init_processing_handle(struct cmd_context *cmd, struct processing_handle *parent_handle);

tools/vgdisplay.c

@@ -38,7 +38,7 @@ static int vgdisplay_single(struct cmd_context *cmd, const char *vg_name,
 		vgdisplay_extents(vg);
 
 		process_each_lv_in_vg(cmd, vg, NULL, NULL, 0, NULL,
-				      (process_single_lv_fn_t)lvdisplay_full);
+				      NULL, (process_single_lv_fn_t)lvdisplay_full);
 
 		log_print("--- Physical volumes ---");
 		process_each_pv_in_vg(cmd, vg, NULL,

tools/vgmknodes.c

@@ -33,5 +33,5 @@ int vgmknodes(struct cmd_context *cmd, int argc, char **argv)
 	if (!lv_mknodes(cmd, NULL))
 		return_ECMD_FAILED;
 
-	return process_each_lv(cmd, argc, argv, NULL, NULL, LCK_VG_READ, NULL, &_vgmknodes_single);
+	return process_each_lv(cmd, argc, argv, NULL, NULL, LCK_VG_READ, NULL, NULL, &_vgmknodes_single);
 }

tools/vgremove.c

@@ -62,7 +62,7 @@ static int vgremove_single(struct cmd_context *cmd, const char *vg_name,
 		}
 
 		if ((ret = process_each_lv_in_vg(cmd, vg, NULL, NULL, 1, &void_handle,
-						 (process_single_lv_fn_t)lvremove_single)) != ECMD_PROCESSED) {
+						 NULL, (process_single_lv_fn_t)lvremove_single)) != ECMD_PROCESSED) {
 			stack;
 			return ret;
 		}