lvconvert: add lvconvert_to_pool_or_swap_metadatadev-dct-lvconvert_to_pool_or_swap_metadata

Give this syntax:

lvconvert --thinpool LV1 --poolmetadata LV2

To two different commands:

1. Convert LV1 to a pool when LV1 is not yet a pool.
   In this case the metadata LV is optional.

2. Swap the metadata LV with LV2 when LV1 is already a pool.
   In this case the metadata LV is required.

This makes the syntax ambiguous; we do not know by looking
at it what the command will do.  I believe it is very
unwise and unnecessary to make this syntax ambiguous.

Given that 1 is a routine command, and 2 is a dangerous
command that should not be used in general, and has been widely
frowned upon, conflating these two commands into one syntax
creates dangerous confusion to users.  There is really no need
for this to swap metadata LVs.  Both of the two operations
conflated have unique and umambiguous primary commands:

1. Primary form of creating a thin pool:
lvconvert --type thin-pool --poolmetadata LV2 LV1

2. Primary form of swapping metadata LV:
lvconvert --swapmetadata --poolmetadata LV2 LV1

So we're talking here about which of these primary commands
should be given the alternate syntax
(lvconvert --poolmetadata --thinpool), or whether the
alternate syntax should be given to both of them.

(The same case applies to lvconvert --cachepool which is also
included here.)

4 files changed, 126 insertions, 78 deletions

diff --git a/tools/command-lines.in b/tools/command-lines.in
index b6aa1d1f6..803315481 100644
--- a/tools/command-lines.in
+++ b/tools/command-lines.in
@@ -454,16 +454,39 @@ DESC: Convert LV to type thin-pool.
 RULE: all and lv_is_visible
 RULE: all not lv_is_locked lv_is_origin lv_is_merging_origin lv_is_external_origin lv_is_virtual
 
-# alternate form of lvconvert --type thin-pool
-# deprecated because of non-standard syntax (missing positional arg)
-# Commands in this form are converted to standard form so that
-# the validation of LV types and rules specified above will apply.
-lvconvert --thinpool LV_linear_striped_raid_cache
+# This command syntax has two different meanings depending on
+# whether the LV pos arg is already a thinpool or not.
+#
+# 1. When the LV arg is not a pool, this command converts
+# the LV into a pool, optionally using a specified meta LV.
+# Creating a pool is a routine operation.
+# This is an alternate form of the primary command:
+# lvconvert --type thin-pool LV
+#
+# 2. When the LV is is already a pool and a meta LV is specified,
+# the meta LV is swapped.  Swapping a meta LV is a very specialized
+# operation that users should never use.
+# This is an alternate form of the primary command:
+# lvconvert --swapmetadata --poolmetadata LV LV
+#
+# The command def cannot include --poolmetadata as a required
+# option, otherwise 1 would not pass, so the validation of
+# this option cannot be done by the command defs, but has to
+# be done ad hoc in the lvconvert implementation.
+#
+# Giving these two very different operations the identical syntax
+# is dangerously confusing.
+#
+# This command syntax is deprecated in general, for any purpose,
+# because of non-standard syntax (missing positional arg)
+#
+lvconvert --thinpool LV_linear_striped_raid_cache_thinpool
 OO: --type thin-pool, --stripes_long Number, --stripesize SizeKB,
 --discards Discards, --zero Bool, OO_LVCONVERT_POOL, OO_LVCONVERT
 OP: PV ...
-ID: lvconvert_to_thinpool_noarg
+ID: lvconvert_to_thinpool_or_swap_metadata
 DESC: Convert LV to type thin-pool (variant, use --type thin-pool).
+DESC: Swap metadata LV in a thin pool (variant, use --swapmetadata).
 FLAGS: SECONDARY_SYNTAX
 
 ---
@@ -475,16 +498,39 @@ OP: PV ...
 ID: lvconvert_to_cachepool
 DESC: Convert LV to type cache-pool.
 
-# alternate form of lvconvert --type cache-pool
-# deprecated because of non-standard syntax (missing positional arg)
-# Commands in this form are converted to standard form so that
-# the validation of LV types and rules specified above will apply.
-lvconvert --cachepool LV_linear_striped_raid
+# This command syntax has two different meanings depending on
+# whether the LV pos arg is already a cachepool or not.
+#
+# 1. When the LV arg is not a pool, this command converts
+# the LV into a pool, optionally using a specified meta LV.
+# Creating a pool is a routine operation.
+# This is an alternate form of the primary command:
+# lvconvert --type cache-pool LV
+#
+# 2. When the LV is is already a pool and a meta LV is specified,
+# the meta LV is swapped.  Swapping a meta LV is a very specialized
+# operation that users should never use.
+# This is an alternate form of the primary command:
+# lvconvert --swapmetadata --poolmetadata LV LV
+#
+# The command def cannot include --poolmetadata as a required
+# option, otherwise 1 would not pass, so the validation of
+# this option cannot be done by the command defs, but has to
+# be done ad hoc in the lvconvert implementation.
+#
+# Giving these two very different operations the identical syntax
+# is dangerously confusing.
+#
+# This command syntax is deprecated in general, for any purpose,
+# because of non-standard syntax (missing positional arg)
+#
+lvconvert --cachepool LV_linear_striped_raid_cachepool
 OO: --type cache-pool, OO_LVCONVERT_POOL, OO_LVCONVERT,
 --cachemode CacheMode, --cachepolicy String, --cachesettings String
 OP: PV ...
-ID: lvconvert_to_cachepool_noarg
+ID: lvconvert_to_cachepool_or_swap_metadata
 DESC: Convert LV to type cache-pool (variant, use --type cache-pool).
+DESC: Swap metadata LV in a cache pool (variant, use --swapmetadata).
 FLAGS: SECONDARY_SYNTAX
 
 ---
@@ -508,20 +554,6 @@ OO: --chunksize SizeKB, OO_LVCONVERT
 ID: lvconvert_swap_pool_metadata
 DESC: Swap metadata LV in a thin pool or cache pool (for repair only).
 
-lvconvert --poolmetadata LV --thinpool LV_thinpool
-OO: --chunksize SizeKB, OO_LVCONVERT
-ID: lvconvert_swap_thinpool_metadata
-DESC: Swap metadata LV in a thin pool (for repair only).
-DESC: (variant, use --swapmetadata).
-FLAGS: SECONDARY_SYNTAX
-
-lvconvert --poolmetadata LV --cachepool LV_cachepool
-OO: --chunksize SizeKB, OO_LVCONVERT
-ID: lvconvert_swap_cachepool_metadata
-DESC: Swap metadata LV in a cache pool (for repair only).
-DESC: (variant, use --swapmetadata).
-FLAGS: SECONDARY_SYNTAX
-
 ---
 
 # lvconvert --merge is an extremely ambiguous command.
diff --git a/tools/lvconvert.c b/tools/lvconvert.c
index 38976029d..ce2ef816d 100644
--- a/tools/lvconvert.c
+++ b/tools/lvconvert.c
@@ -3710,6 +3710,10 @@ int lvconvert_start_poll_cmd(struct cmd_context *cmd, int argc, char **argv)
 	return ret;
 }
 
+static int _lvconvert_swap_pool_metadata_single(struct cmd_context *cmd,
+					 struct logical_volume *lv,
+					 struct processing_handle *handle);
+
 static int _lvconvert_to_pool_single(struct cmd_context *cmd,
 					 struct logical_volume *lv,
 					 struct processing_handle *handle)
@@ -3753,38 +3757,84 @@ int lvconvert_to_pool_cmd(struct cmd_context *cmd, int argc, char **argv)
 			       NULL, NULL, &_lvconvert_to_pool_single);
 }
 
+static int _lvconvert_to_pool_or_swap_metadata_single(struct cmd_context *cmd,
+					 struct logical_volume *lv,
+					 struct processing_handle *handle)
+{
+	struct dm_list *use_pvh = NULL;
+	int to_thinpool = 0;
+	int to_cachepool = 0;
+
+	switch (cmd->command->command_enum) {
+	case lvconvert_to_thinpool_or_swap_metadata_CMD:
+		to_thinpool = 1;
+		break;
+	case lvconvert_to_cachepool_or_swap_metadata_CMD:
+		to_cachepool = 1;
+		break;
+	default:
+		log_error(INTERNAL_ERROR "Invalid lvconvert pool command");
+		return 0;
+	};
+
+	if (cmd->position_argc > 1) {
+		/* First pos arg is required LV, remaining are optional PVs. */
+		if (!(use_pvh = create_pv_list(cmd->mem, lv->vg, cmd->position_argc - 1, cmd->position_argv + 1, 0)))
+			return_ECMD_FAILED;
+	} else
+		use_pvh = &lv->vg->pvs;
+
+	/*
+	 * We can finally determine if this command is supposed to create
+	 * a pool or swap the metadata in an existing pool.
+	 *
+	 * This is an ugly hack that allows the ambiguous command:
+	 * 'lvconvert --thinpool LV1 --poolmetadata LV2' to mean either:
+	 * 1. convert LV2 to a pool using the specified meta LV2
+	 * 2. swap the meta lv in LV1 with LV2
+	 *
+	 * In case 2, the poolmetadata option is required, but in case 1
+	 * it is optional.  So, the command def is not able to validate
+	 * the correct syntax in this case, and we have to check here
+	 * for missing poolmetadata in case 2.
+	 */
+	if (lv_is_pool(lv)) {
+		if (!arg_is_set(cmd, poolmetadata_ARG)) {
+			log_error("The --poolmetadata option is required to swap metadata.");
+			return ECMD_FAILED;
+		}
+		return _lvconvert_swap_pool_metadata_single(cmd, lv, handle);
+	}
+
+	if (!_lvconvert_to_pool(cmd, lv, to_thinpool, to_cachepool, use_pvh))
+		return_ECMD_FAILED;
+
+	return ECMD_PROCESSED;
+}
+
 /*
- * Reformats non-standard command form into standard command form.
- *
  * In the command variants with no position LV arg, the LV arg is taken from
  * the --thinpool/--cachepool arg, and the position args are modified to match
  * the standard command form.
  */
 
-int lvconvert_to_pool_noarg_cmd(struct cmd_context *cmd, int argc, char **argv)
+int lvconvert_to_pool_or_swap_metadata_cmd(struct cmd_context *cmd, int argc, char **argv)
 {
-	struct command *new_command;
 	char *pool_data_name;
 	int i, p;
 
 	switch (cmd->command->command_enum) {
-	case lvconvert_to_thinpool_noarg_CMD:
+	case lvconvert_to_thinpool_or_swap_metadata_CMD:
 		pool_data_name = (char *)arg_str_value(cmd, thinpool_ARG, NULL);
-		new_command = get_command(lvconvert_to_thinpool_CMD);
 		break;
-	case lvconvert_to_cachepool_noarg_CMD:
+	case lvconvert_to_cachepool_or_swap_metadata_CMD:
 		pool_data_name = (char *)arg_str_value(cmd, cachepool_ARG, NULL);
-		new_command = get_command(lvconvert_to_cachepool_CMD);
 		break;
 	default:
 		log_error(INTERNAL_ERROR "Unknown pool conversion.");
 		return 0;
 	};
 
-	log_debug("Changing command %d:%s to standard form %d:%s",
-		  cmd->command->command_index, cmd->command->command_id,
-		  new_command->command_index, new_command->command_id);
-
 	/* Make the LV the first position arg. */
 
 	p = cmd->position_argc;
@@ -3793,9 +3843,9 @@ int lvconvert_to_pool_noarg_cmd(struct cmd_context *cmd, int argc, char **argv)
 
 	cmd->position_argv[0] = pool_data_name;
 	cmd->position_argc++;
-	cmd->command = new_command;
 
-	return lvconvert_to_pool_cmd(cmd, argc, argv);
+	return process_each_lv(cmd, 1, cmd->position_argv, NULL, NULL, READ_FOR_UPDATE,
+			       NULL, NULL, &_lvconvert_to_pool_or_swap_metadata_single);
 }
 
 static int _lvconvert_to_cache_vol_single(struct cmd_context *cmd,
@@ -4040,38 +4090,6 @@ int lvconvert_swap_pool_metadata_cmd(struct cmd_context *cmd, int argc, char **a
 			       NULL, NULL, &_lvconvert_swap_pool_metadata_single);
 }
 
-int lvconvert_swap_pool_metadata_noarg_cmd(struct cmd_context *cmd, int argc, char **argv)
-{
-	struct command *new_command;
-	char *pool_name;
-
-	switch (cmd->command->command_enum) {
-	case lvconvert_swap_thinpool_metadata_CMD:
-		pool_name = (char *)arg_str_value(cmd, thinpool_ARG, NULL);
-		break;
-	case lvconvert_swap_cachepool_metadata_CMD:
-		pool_name = (char *)arg_str_value(cmd, cachepool_ARG, NULL);
-		break;
-	default:
-		log_error(INTERNAL_ERROR "Unknown pool conversion.");
-		return 0;
-	};
-
-	new_command = get_command(lvconvert_swap_pool_metadata_CMD);
-
-	log_debug("Changing command %d:%s to standard form %d:%s",
-		  cmd->command->command_index, cmd->command->command_id,
-		  new_command->command_index, new_command->command_id);
-
-	/* Make the LV the first position arg. */
-
-	cmd->position_argv[0] = pool_name;
-	cmd->position_argc++;
-	cmd->command = new_command;
-
-	return lvconvert_swap_pool_metadata_cmd(cmd, argc, argv);
-}
-
 static int _lvconvert_merge_thin_single(struct cmd_context *cmd,
 					 struct logical_volume *lv,
 					 struct processing_handle *handle)
diff --git a/tools/lvmcmdline.c b/tools/lvmcmdline.c
index 65f80c800..07d420370 100644
--- a/tools/lvmcmdline.c
+++ b/tools/lvmcmdline.c
@@ -135,14 +135,12 @@ struct command_function command_functions[CMD_COUNT] = {
 
 	/* lvconvert utilities for creating/maintaining thin and cache objects. */
 	{ lvconvert_to_thinpool_CMD,			lvconvert_to_pool_cmd },
-	{ lvconvert_to_thinpool_noarg_CMD,		lvconvert_to_pool_noarg_cmd },
 	{ lvconvert_to_cachepool_CMD,			lvconvert_to_pool_cmd },
-	{ lvconvert_to_cachepool_noarg_CMD,		lvconvert_to_pool_noarg_cmd },
+	{ lvconvert_swap_pool_metadata_CMD,		lvconvert_swap_pool_metadata_cmd },
+	{ lvconvert_to_thinpool_or_swap_metadata_CMD,   lvconvert_to_pool_or_swap_metadata_cmd },
+	{ lvconvert_to_cachepool_or_swap_metadata_CMD,  lvconvert_to_pool_or_swap_metadata_cmd },
 	{ lvconvert_to_thin_with_external_CMD,		lvconvert_to_thin_with_external_cmd },
 	{ lvconvert_to_cache_vol_CMD,			lvconvert_to_cache_vol_cmd },
-	{ lvconvert_swap_pool_metadata_CMD,		lvconvert_swap_pool_metadata_cmd },
-	{ lvconvert_swap_thinpool_metadata_CMD,		lvconvert_swap_pool_metadata_noarg_cmd },
-	{ lvconvert_swap_cachepool_metadata_CMD,	lvconvert_swap_pool_metadata_noarg_cmd },
 	{ lvconvert_merge_thin_CMD,			lvconvert_merge_thin_cmd },
 	{ lvconvert_split_and_keep_cachepool_CMD,	lvconvert_split_cachepool_cmd },
 	{ lvconvert_split_and_remove_cachepool_CMD,	lvconvert_split_cachepool_cmd },
diff --git a/tools/tools.h b/tools/tools.h
index 2f2a0fd7c..a520c62f3 100644
--- a/tools/tools.h
+++ b/tools/tools.h
@@ -278,7 +278,7 @@ int lvconvert_to_pool_noarg_cmd(struct cmd_context *cmd, int argc, char **argv);
 int lvconvert_to_cache_vol_cmd(struct cmd_context *cmd, int argc, char **argv);
 int lvconvert_to_thin_with_external_cmd(struct cmd_context *cmd, int argc, char **argv);
 int lvconvert_swap_pool_metadata_cmd(struct cmd_context *cmd, int argc, char **argv);
-int lvconvert_swap_pool_metadata_noarg_cmd(struct cmd_context *cmd, int argc, char **argv);
+int lvconvert_to_pool_or_swap_metadata_cmd(struct cmd_context *cmd, int argc, char **argv);
 int lvconvert_merge_thin_cmd(struct cmd_context *cmd, int argc, char **argv);
 int lvconvert_split_cachepool_cmd(struct cmd_context *cmd, int argc, char **argv);