Move more parameter validation into the library.

Update vgrename to call validate_vg_rename_params().
Fix vgcreate and vgsplit default arguments by adding defaults parameter to
fill_vg_create_params().
Add t-vgrename-usage.sh test.
Bugzilla: bz251992
---
 tools/toollib.c  |   32 ++++++++------------------------
 tools/toollib.h  |    5 ++---
 tools/vgcreate.c |   35 +++++++++++++++++++++--------------
 tools/vgrename.c |   35 ++++++-----------------------------
 tools/vgsplit.c  |   21 ++++++++++++++-------
 5 files changed, 51 insertions(+), 77 deletions(-)

lib/metadata/metadata-exported.h

@@ -540,4 +540,7 @@ struct vgcreate_params {
 int validate_vg_create_params(struct cmd_context *cmd,
 			      struct vgcreate_params *vp);
 
+int validate_vg_rename_params(struct cmd_context *cmd,
+			      const char *vg_name_old,
+			      const char *vg_name_new);
 #endif

lib/metadata/metadata.c

@@ -18,6 +18,7 @@
 #include "metadata.h"
 #include "toolcontext.h"
 #include "lvm-string.h"
+#include "lvm-file.h"
 #include "lvmcache.h"
 #include "memlock.h"
 #include "str_list.h"
@@ -227,6 +228,54 @@ int get_pv_from_vg_by_id(const struct format_type *fmt, const char *vg_name,
 	return 0;
 }
 
+static int validate_new_vg_name(struct cmd_context *cmd, const char *vg_name)
+{
+	char vg_path[PATH_MAX];
+
+	if (!validate_name(vg_name))
+		return 0;
+
+	snprintf(vg_path, PATH_MAX, "%s%s", cmd->dev_dir, vg_name);
+	if (path_exists(vg_path)) {
+		log_error("%s: already exists in filesystem", vg_path);
+		return 0;
+	}
+
+	return 1;
+}
+
+
+int validate_vg_rename_params(struct cmd_context *cmd,
+			      const char *vg_name_old,
+			      const char *vg_name_new)
+{
+	unsigned length;
+	char *dev_dir;
+
+	dev_dir = cmd->dev_dir;
+	length = strlen(dev_dir);
+
+	/* Check sanity of new name */
+	if (strlen(vg_name_new) > NAME_LEN - length - 2) {
+		log_error("New volume group path exceeds maximum length "
+			  "of %d!", NAME_LEN - length - 2);
+		return 0;
+	}
+
+	if (!validate_new_vg_name(cmd, vg_name_new)) {
+		log_error("New volume group name \"%s\" is invalid",
+			  vg_name_new);
+		return 0;
+	}
+
+	if (!strcmp(vg_name_old, vg_name_new)) {
+		log_error("Old and new volume group names must differ");
+		return 0;
+	}
+
+	return 1;
+}
+
 int vg_rename(struct cmd_context *cmd, struct volume_group *vg,
 	      const char *new_name)
 {

test/t-vgrename-usage.sh

@@ -0,0 +1,46 @@
+#!/bin/sh
+# Copyright (C) 2007 Red Hat, Inc. All rights reserved.
+#
+# This copyrighted material is made available to anyone wishing to use,
+# modify, copy, or redistribute it subject to the terms and conditions
+# of the GNU General Public License v.2.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software Foundation,
+# Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+
+test_description='Exercise some vgrename diagnostics'
+privileges_required_=1
+
+. ./test-lib.sh
+
+cleanup_()
+{
+  test -n "$d1" && losetup -d "$d1"
+  test -n "$d2" && losetup -d "$d2"
+  test -n "$d3" && losetup -d "$d3"
+  test -n "$d4" && losetup -d "$d4"
+  rm -f "$f1" "$f2" "$f3" "$f4"
+}
+
+test_expect_success \
+  'set up temp files, loopback devices, PVs, vgnames' \
+  'f1=$(pwd)/1 && d1=$(loop_setup_ "$f1") &&
+   f2=$(pwd)/2 && d2=$(loop_setup_ "$f2") &&
+   f3=$(pwd)/3 && d3=$(loop_setup_ "$f3") &&
+   f4=$(pwd)/4 && d4=$(loop_setup_ "$f4") &&
+   vg1=$(this_test_)-1-$$          &&
+   vg2=$(this_test_)-2-$$          &&
+   pvcreate $d1 $d2 $d3 $d4'
+
+test_expect_success \
+  'vgrename normal operation - rename vg1 to vg2' \
+  'vgcreate $vg1 $d1 $d2 &&
+   vgrename $vg1 $vg2 &&
+   check_vg_field_ $vg2 vg_name $vg2 &&
+   vgremove $vg2'
+
+test_done
+# Local Variables:
+# indent-tabs-mode: nil
+# End:

tools/toollib.c

@@ -1218,23 +1218,6 @@ int apply_lvname_restrictions(const char *name)
 	return 1;
 }
 
-int validate_new_vg_name(struct cmd_context *cmd, const char *vg_name)
-{
-	char vg_path[PATH_MAX];
-
-	if (!validate_name(vg_name))
-		return 0;
-
-	snprintf(vg_path, PATH_MAX, "%s%s", cmd->dev_dir, vg_name);
-	if (path_exists(vg_path)) {
-		log_error("%s: already exists in filesystem", vg_path);
-		return 0;
-	}
-
-	return 1;
-}
-
-
 /*
  * Set members of struct vgcreate_params from cmdline.
  * Do preliminary validation with arg_*() interface.
@@ -1243,23 +1226,27 @@ int validate_new_vg_name(struct cmd_context *cmd, const char *vg_name)
  * validate_vgcreate_params() will be moved into the LVM library.
  */
 int fill_vg_create_params(struct cmd_context *cmd,
-			  char *vg_name, struct vgcreate_params *vp)
+			  char *vg_name, struct vgcreate_params *vp_new,
+			  struct vgcreate_params *vp_def)
 {
-	vp->vg_name = skip_dev_dir(cmd, vg_name, NULL);
+	vp_new->vg_name = skip_dev_dir(cmd, vg_name, NULL);
-	vp->max_lv = arg_uint_value(cmd, maxlogicalvolumes_ARG, 0);
+	vp_new->max_lv = arg_uint_value(cmd, maxlogicalvolumes_ARG,
-	vp->max_pv = arg_uint_value(cmd, maxphysicalvolumes_ARG, 0);
+					vp_def->max_lv);
-	vp->alloc = arg_uint_value(cmd, alloc_ARG, ALLOC_NORMAL);
+	vp_new->max_pv = arg_uint_value(cmd, maxphysicalvolumes_ARG,
+					vp_def->max_pv);
+	vp_new->alloc = arg_uint_value(cmd, alloc_ARG, vp_def->alloc);
 
 	/* Units of 512-byte sectors */
-	vp->extent_size =
+	vp_new->extent_size =
-	    arg_uint_value(cmd, physicalextentsize_ARG, DEFAULT_PE_SIZE);
+	    arg_uint_value(cmd, physicalextentsize_ARG, vp_def->extent_size);
 
 	if (arg_count(cmd, clustered_ARG))
-        	vp->clustered =
+        	vp_new->clustered =
-			!strcmp(arg_str_value(cmd, clustered_ARG, "n"), "y");
+			!strcmp(arg_str_value(cmd, clustered_ARG,
+					      vp_def->clustered ? "y":"n"), "y");
 	else
 		/* Default depends on current locking type */
-		vp->clustered = locking_is_clustered();
+		vp_new->clustered = locking_is_clustered();
 
 	if (arg_sign_value(cmd, physicalextentsize_ARG, 0) == SIGN_MINUS) {
 		log_error("Physical extent size may not be negative");
@@ -1278,4 +1265,3 @@ int fill_vg_create_params(struct cmd_context *cmd,
 
 	return 0;
 }
-

tools/toollib.h

@@ -96,8 +96,7 @@ struct list *clone_pv_list(struct dm_pool *mem, struct list *pvs);
 
 int apply_lvname_restrictions(const char *name);
 
-int validate_new_vg_name(struct cmd_context *cmd, const char *vg_name);
-
 int fill_vg_create_params(struct cmd_context *cmd,
-			  char *vg_name, struct vgcreate_params *vp);
+			  char *vg_name, struct vgcreate_params *vp_new,
+			  struct vgcreate_params *vp_def);
 #endif

tools/vgcreate.c

@@ -17,7 +17,8 @@
 
 int vgcreate(struct cmd_context *cmd, int argc, char **argv)
 {
-	struct vgcreate_params vp;
+	struct vgcreate_params vp_new;
+	struct vgcreate_params vp_def;
 	struct volume_group *vg;
 	const char *tag;
 
@@ -32,23 +33,29 @@ int vgcreate(struct cmd_context *cmd, int argc, char **argv)
 		return EINVALID_CMD_LINE;
 	}
 
-	if (fill_vg_create_params(cmd, argv[0], &vp))
+	vp_def.vg_name = NULL;
+	vp_def.extent_size = DEFAULT_PE_SIZE;
+	vp_def.max_pv = 0;
+	vp_def.max_lv = 0;
+	vp_def.alloc = ALLOC_NORMAL;
+	vp_def.clustered = 0;
+	if (fill_vg_create_params(cmd, argv[0], &vp_new, &vp_def))
 		return EINVALID_CMD_LINE;
 
-	if (validate_vg_create_params(cmd, &vp))
+	if (validate_vg_create_params(cmd, &vp_new))
 	    return EINVALID_CMD_LINE;
 
 	/* Create the new VG */
-	if (!(vg = vg_create(cmd, vp.vg_name, vp.extent_size, vp.max_pv,
+	if (!(vg = vg_create(cmd, vp_new.vg_name, vp_new.extent_size,
-			     vp.max_lv, vp.alloc,
+			     vp_new.max_pv, vp_new.max_lv, vp_new.alloc,
 			     argc - 1, argv + 1)))
 		return ECMD_FAILED;
 
-	if (vp.max_lv != vg->max_lv)
+	if (vp_new.max_lv != vg->max_lv)
 		log_warn("WARNING: Setting maxlogicalvolumes to %d "
 			 "(0 means unlimited)", vg->max_lv);
 
-	if (vp.max_pv != vg->max_pv)
+	if (vp_new.max_pv != vg->max_pv)
 		log_warn("WARNING: Setting maxphysicalvolumes to %d "
 			 "(0 means unlimited)", vg->max_pv);
 
@@ -65,13 +72,13 @@ int vgcreate(struct cmd_context *cmd, int argc, char **argv)
 
 		if (!str_list_add(cmd->mem, &vg->tags, tag)) {
 			log_error("Failed to add tag %s to volume group %s",
-				  tag, vp.vg_name);
+				  tag, vp_new.vg_name);
 			return ECMD_FAILED;
 		}
 	}
 
 	/* FIXME: move this inside vg_create? */
-	if (vp.clustered)
+	if (vp_new.clustered)
 		vg->status |= CLUSTERED;
 	else
 		vg->status &= ~CLUSTERED;
@@ -81,26 +88,26 @@ int vgcreate(struct cmd_context *cmd, int argc, char **argv)
 		return ECMD_FAILED;
 	}
 
-	if (!lock_vol(cmd, vp.vg_name, LCK_VG_WRITE | LCK_NONBLOCK)) {
+	if (!lock_vol(cmd, vp_new.vg_name, LCK_VG_WRITE | LCK_NONBLOCK)) {
-		log_error("Can't get lock for %s", vp.vg_name);
+		log_error("Can't get lock for %s", vp_new.vg_name);
 		unlock_vg(cmd, VG_ORPHANS);
 		return ECMD_FAILED;
 	}
 
 	if (!archive(vg)) {
-		unlock_vg(cmd, vp.vg_name);
+		unlock_vg(cmd, vp_new.vg_name);
 		unlock_vg(cmd, VG_ORPHANS);
 		return ECMD_FAILED;
 	}
 
 	/* Store VG on disk(s) */
 	if (!vg_write(vg) || !vg_commit(vg)) {
-		unlock_vg(cmd, vp.vg_name);
+		unlock_vg(cmd, vp_new.vg_name);
 		unlock_vg(cmd, VG_ORPHANS);
 		return ECMD_FAILED;
 	}
 
-	unlock_vg(cmd, vp.vg_name);
+	unlock_vg(cmd, vp_new.vg_name);
 	unlock_vg(cmd, VG_ORPHANS);
 
 	backup(vg);

tools/vgrename.c

@@ -19,7 +19,6 @@ static int vg_rename_path(struct cmd_context *cmd, const char *old_vg_path,
 			  const char *new_vg_path)
 {
 	char *dev_dir;
-	unsigned length;
 	struct id id;
 	int consistent = 1;
 	int match = 0;
@@ -35,25 +34,9 @@ static int vg_rename_path(struct cmd_context *cmd, const char *old_vg_path,
 	vg_name_new = skip_dev_dir(cmd, new_vg_path, NULL);
 
 	dev_dir = cmd->dev_dir;
-	length = strlen(dev_dir);
 
-	/* Check sanity of new name */
+	if (!validate_vg_rename_params(cmd, vg_name_old, vg_name_new))
-	if (strlen(vg_name_new) > NAME_LEN - length - 2) {
-		log_error("New volume group path exceeds maximum length "
-			  "of %d!", NAME_LEN - length - 2);
 		return 0;
-	}
-
-	if (!validate_new_vg_name(cmd, vg_name_new)) {
-		log_error("New volume group name \"%s\" is invalid",
-			  vg_name_new);
-		return 0;
-	}
-
-	if (!strcmp(vg_name_old, vg_name_new)) {
-		log_error("Old and new volume group names must differ");
-		return 0;
-	}
 
 	log_verbose("Checking for existing volume group \"%s\"", vg_name_old);
 

tools/vgsplit.c

@@ -212,7 +212,8 @@ static int _move_mirrors(struct volume_group *vg_from,
 
 int vgsplit(struct cmd_context *cmd, int argc, char **argv)
 {
-	struct vgcreate_params vp;
+	struct vgcreate_params vp_new;
+	struct vgcreate_params vp_def;
 	char *vg_name_from, *vg_name_to;
 	struct volume_group *vg_to, *vg_from;
 	int opt;
@@ -260,18 +261,24 @@ int vgsplit(struct cmd_context *cmd, int argc, char **argv)
 		/* FIXME: need some common logic */
 		cmd->fmt = vg_from->fid->fmt;
 
-		/* FIXME: original code took defaults from vg_from */
+		vp_def.vg_name = NULL;
-		if (fill_vg_create_params(cmd, vg_name_to, &vp))
+		vp_def.extent_size = vg_from->extent_size;
+		vp_def.max_pv = vg_from->max_pv;
+		vp_def.max_lv = vg_from->max_lv;
+		vp_def.alloc = vg_from->alloc;
+		vp_def.clustered = 0;
+
+		if (fill_vg_create_params(cmd, vg_name_to, &vp_new, &vp_def))
 			return EINVALID_CMD_LINE;
 
-		if (validate_vg_create_params(cmd, &vp))
+		if (validate_vg_create_params(cmd, &vp_new))
 			return EINVALID_CMD_LINE;
 
 		/* Create new VG structure */
 		/* FIXME: allow user input same params as to vgcreate tool */
-		if (!(vg_to = vg_create(cmd, vg_name_to, vp.extent_size,
+		if (!(vg_to = vg_create(cmd, vg_name_to, vp_new.extent_size,
-					vp.max_pv, vp.max_lv,
+					vp_new.max_pv, vp_new.max_lv,
-					vp.alloc, 0, NULL)))
+					vp_new.alloc, 0, NULL)))
 			goto error;
 
 		if (vg_from->status & CLUSTERED)