lvconvert: remove unused calls for snapshots

snapshot commands are no longer called from the
monolithic lvconvert code, so remove the unused code.

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 --mergesnapshot $vg1/$(snap_lv_name_ $lv1)
+lvconvert --merge $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 --mergesnapshot $vg/$lv2
+	lvconvert -b --merge $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 --mergesnapshot requires explicit LV listing
-not lvconvert --mergesnapshot
-lvconvert --mergesnapshot $vg/$(snap_lv_name_ $lv1)
+# make sure lvconvert --merge requires explicit LV listing
+not lvconvert --merge
+lvconvert --merge $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 --mergesnapshot --background $vg/$(snap_lv_name_ $lv1)
+lvconvert -i+100 --merge --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 --mergesnapshot $vg/$(snap_lv_name_ $lv1)
+lvconvert --merge $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 --mergesnapshot $vg/oldsnapof_${lv1}
+lvconvert --merge $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

@@ -518,32 +518,24 @@ 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
+RULE: all not lv_is_locked lv_is_pvmove lv_is_origin lv_is_external_origin lv_is_merging_cow lv_is_vg_writable
 
 ---
 
-# 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.
+# NB: an unsual use of position args here
 
 # alternate form of lvconvert --snapshot
-lvconvert --type snapshot LV LV_linear
+lvconvert --type snapshot LV_linear_striped_raid LV_snapshot
 OO: --snapshot, --chunksize SizeKB, --zero Bool, OO_LVCONVERT
 ID: lvconvert_combine_split_snapshot
-DESC: Combine a former COW snapshot (second arg) with a former
-DESC: origin LV (first arg) to reverse a splitsnapshot command.
+DESC: Combine LV with a previously split snapshot LV.
 FLAGS: SECONDARY_SYNTAX
 RULE: all not lv_is_locked lv_is_pvmove
 
-lvconvert --snapshot LV LV_linear
+lvconvert --snapshot LV_linear_striped_raid LV_snapshot
 OO: --type snapshot, --chunksize SizeKB, --zero Bool, OO_LVCONVERT
 ID: lvconvert_combine_split_snapshot
-DESC: Combine a former COW snapshot (second arg) with a former
-DESC: origin LV (first arg) to reverse a splitsnapshot command.
+DESC: Combine LV with a previously split snapshot LV.
 RULE: all not lv_is_locked lv_is_pvmove
 
 ---

tools/lvchange.c

@@ -1178,9 +1178,7 @@ static int _lvchange_refresh_check(struct cmd_context *cmd,
 {
 	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 1;
 		return 0;
 	}
 

tools/lvconvert.c

@@ -2076,25 +2076,12 @@ static int _lvconvert_snapshot(struct cmd_context *cmd,
 		return_0;
 
 	/*
-	 * check_lv_rules() checks cannot be done via command definition
-	 * rules because this LV is not processed by process_each_lv.
+	 * lv_is_prop checks here cannot be automated by command definition
+	 * rules because they are not done on the LV being processed.
 	 */
 	if (lv_is_locked(org) || lv_is_pvmove(org)) {
-		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));
+		log_error("Unable to convert an LV into a snapshot of a %s LV.",
+			  lv_is_locked(org) ? "locked" : "pvmove");
 		return 0;
 	}
 
@@ -4391,36 +4378,6 @@ 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.
  */
@@ -4626,7 +4583,7 @@ 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_generic_check, &_lvconvert_repair_pvs_or_thinpool_single);
+			      handle, NULL, &_lvconvert_repair_pvs_or_thinpool_single);
 
 	init_ignore_suspended_devices(saved_ignore_suspended_devices);
 
@@ -4703,7 +4660,7 @@ 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_generic_check, &_lvconvert_replace_pv_single);
+			      handle, NULL, &_lvconvert_replace_pv_single);
 
 	destroy_processing_handle(cmd, handle);
 
@@ -4741,20 +4698,6 @@ 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;
@@ -4772,12 +4715,12 @@ 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_check, &_lvconvert_merge_snapshot_single);
+			      handle, NULL, &_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), 1, 0);
+						arg_is_set(cmd, background_ARG), 0, 0);
 			if (poll_ret > ret)
 				ret = poll_ret;
 		}
@@ -4808,47 +4751,12 @@ 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_generic_check, &_lvconvert_split_snapshot_single);
+			       NULL, NULL, &_lvconvert_split_snapshot_single);
 }
 
 /*
- * 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.
+ * Combine an origin LV with a snapshot LV (cow) that was previously split from
+ * the origin using --splitsnapshot.
  */
 
 static int _lvconvert_combine_split_snapshot_single(struct cmd_context *cmd,
@@ -4869,7 +4777,15 @@ 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)
 {
+	/*
+	 * 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_generic_check, &_lvconvert_combine_split_snapshot_single);
+			       NULL, NULL, &_lvconvert_combine_split_snapshot_single);
 }
 

tools/toollib.c

@@ -2582,6 +2582,7 @@ static int _get_lvt_enum(struct logical_volume *lv)
 	if (!strcmp(seg->segtype->name, SEG_TYPE_NAME_ZERO))
 		return zero_LVT;
 
+	log_error(INTERNAL_ERROR "unknown lv type for %s", display_lvname(lv));
 	return 0;
 }
 
@@ -2677,29 +2678,35 @@ static int _lv_props_match(struct cmd_context *cmd, struct logical_volume *lv, u
 	return !found_a_mismatch;
 }
 
-static int _check_lv_types(struct cmd_context *cmd, struct logical_volume *lv, int pos)
+/*
+ * 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)
 {
 	int ret = 1;
 
-	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");
+	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");
+		}
 	}
 
 	return ret;
@@ -2863,59 +2870,6 @@ 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,
@@ -2935,7 +2889,6 @@ int process_each_lv_in_vg(struct cmd_context *cmd, struct volume_group *vg,
 	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;
@@ -3095,32 +3048,42 @@ int process_each_lv_in_vg(struct cmd_context *cmd, struct volume_group *vg,
 
 		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, lv_arg_pos)) {
+		if (!_check_lv_types(cmd, lvl->lv)) {
 			/* FIXME: include this result in report log? */
+			/* FIXME: avoid duplicating message for each level */
+
 			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 (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;
 		}