vgrename: use process_each_vgdev-dct-vgrename-use-toollib

process_each_vg() locks and reads the old VG.

When real VG names are used (not a UUID in place of the
old name), the command still pre-locks the new name
(when strcmp wants it locked first), before calling
process_each_vg on the old name.

In the case where the old name is replaced with a UUID,
process_each_vg now translates that UUID into the real
VG name, which it locks and reads.  In this case, we
cannot do pre-locking to maintain lock ordering because
the old name is unknown.  So, in this case the strcmp
based lock ordering is suppressed and the old name is
always locked first.  This opens a remote chance for
lock ordering conflict between racing vgrenames between
two names where one or both commands use the UUID.

3 files changed, 150 insertions, 135 deletions

diff --git a/lib/cache/lvmcache.c b/lib/cache/lvmcache.c
index 799de6f6e..bcb346313 100644
--- a/lib/cache/lvmcache.c
+++ b/lib/cache/lvmcache.c
@@ -82,6 +82,7 @@ static int _has_scanned = 0;
 static int _vgs_locked = 0;
 static int _vg_global_lock_held = 0;	/* Global lock held when cache wiped? */
 static int _found_duplicate_pvs = 0;	/* If we never see a duplicate PV we can skip checking for them later. */
+static int _suppress_lock_ordering = 0;
 
 int lvmcache_init(void)
 {
@@ -371,6 +372,11 @@ static int _vgname_order_correct(const char *vgname1, const char *vgname2)
 	return 0;
 }
 
+void lvmcache_lock_ordering(int enable)
+{
+	_suppress_lock_ordering = enable;
+}
+
 /*
  * Ensure VG locks are acquired in alphabetical order.
  */
@@ -379,6 +385,9 @@ int lvmcache_verify_lock_order(const char *vgname)
 	struct dm_hash_node *n;
 	const char *vgname2;
 
+	if (_suppress_lock_ordering)
+		return 1;
+
 	if (!_lock_hash)
 		return_0;
 
diff --git a/lib/cache/lvmcache.h b/lib/cache/lvmcache.h
index 509cfd6f6..731fddff8 100644
--- a/lib/cache/lvmcache.h
+++ b/lib/cache/lvmcache.h
@@ -197,4 +197,6 @@ void lvmcache_get_max_name_lengths(struct cmd_context *cmd,
 
 int lvmcache_vg_is_foreign(struct cmd_context *cmd, const char *vgname, const char *vgid);
 
+void lvmcache_lock_ordering(int enable);
+
 #endif
diff --git a/tools/vgrename.c b/tools/vgrename.c
index 9e6beec35..dd96fbf77 100644
--- a/tools/vgrename.c
+++ b/tools/vgrename.c
@@ -15,137 +15,90 @@
 
 #include "tools.h"
 
-static struct volume_group *_get_old_vg_for_rename(struct cmd_context *cmd,
-						   const char *vg_name_old,
-						   const char *vgid,
-						   uint32_t lockd_state)
-{
-	struct volume_group *vg;
-
-	/* FIXME we used to print an error about EXPORTED, but proceeded
-	   nevertheless. */
-	vg = vg_read_for_update(cmd, vg_name_old, vgid, READ_ALLOW_EXPORTED, lockd_state);
-	if (vg_read_error(vg)) {
-		release_vg(vg);
-		return_NULL;
-	}
-
-	return vg;
-}
+struct vgrename_params {
+	const char *vg_name_old;
+	const char *vg_name_new;
+	unsigned int old_name_is_uuid : 1;
+	unsigned int lock_vg_old_first : 1;
+	unsigned int unlock_new_name: 1;
+};
 
 static int _lock_new_vg_for_rename(struct cmd_context *cmd,
 				   const char *vg_name_new)
 {
-	int rc;
-
-	log_verbose("Checking for new volume group \"%s\"", vg_name_new);
-
-	rc = vg_lock_newname(cmd, vg_name_new);
-
-	if (rc == FAILED_LOCKING) {
+	if (!lock_vol(cmd, vg_name_new, LCK_VG_WRITE, NULL)) {
 		log_error("Can't get lock for %s", vg_name_new);
 		return 0;
 	}
 
-	if (rc == FAILED_EXIST) {
-		log_error("New volume group \"%s\" already exists",
-			  vg_name_new);
-		return 0;
-	}
 	return 1;
 }
 
-static int vg_rename_path(struct cmd_context *cmd, const char *old_vg_path,
-			  const char *new_vg_path)
+static int _vgrename_single(struct cmd_context *cmd, const char *vg_name,
+			    struct volume_group *vg, struct processing_handle *handle)
 {
+	struct vgrename_params *vp = (struct vgrename_params *) handle->custom_handle;
+	struct lvmcache_vginfo *vginfo;
+	char old_path[NAME_LEN];
+	char new_path[NAME_LEN];
 	char *dev_dir;
-	struct id id;
-	int match = 0;
-	int found_id = 0;
-	struct dm_list *vgids;
-	struct dm_str_list *sl;
-	const char *vg_name_new;
-	const char *vgid = NULL, *vg_name, *vg_name_old;
-	char old_path[NAME_LEN], new_path[NAME_LEN];
-	struct volume_group *vg = NULL;
-	uint32_t lockd_state = 0;
-	int lock_vg_old_first = 1;
-
-	vg_name_old = skip_dev_dir(cmd, old_vg_path, NULL);
-	vg_name_new = skip_dev_dir(cmd, new_vg_path, NULL);
-
-	dev_dir = cmd->dev_dir;
-
-	if (!validate_vg_rename_params(cmd, vg_name_old, vg_name_new))
-		return_0;
-
-	log_verbose("Checking for existing volume group \"%s\"", vg_name_old);
-
-	/* populate lvmcache */
-	if (!lvmetad_vg_list_to_lvmcache(cmd))
-		stack;
-
-	lvmcache_label_scan(cmd, 2);
-
-	/* Avoid duplicates */
-	if (!(vgids = get_vgids(cmd, 0)) || dm_list_empty(vgids)) {
-		log_error("No complete volume groups found");
-		return 0;
+	int r;
+
+	/*
+	 * vg_name_old may be a UUID which process_each
+	 * replaced with the real VG name.  In that case,
+	 * vp->vg_name_old will be the UUID and vg_name will be
+	 * the actual VG name.  Check again if the old and new
+	 * names match, using the real names.
+	 */
+	if (vp->old_name_is_uuid && !strcmp(vp->vg_name_new, vg_name)) {
+		log_error("New VG name must differ from the old VG name.");
+		return ECMD_FAILED;
 	}
 
-	dm_list_iterate_items(sl, vgids) {
-		vgid = sl->str;
-		if (!vgid || !(vg_name = lvmcache_vgname_from_vgid(NULL, vgid)))
-			continue;
-		if (!strcmp(vg_name, vg_name_old)) {
-			if (match) {
-				log_error("Found more than one VG called %s. "
-					  "Please supply VG uuid.", vg_name_old);
-				return 0;
-			}
-			match = 1;
-		}
+	/*
+	 * Check if a VG already exists with the new VG name.
+	 * (We could look for the new name in the list of all VGs
+	 * that process_each_vg created, but we don't have access
+	 * to that list here, so we have to look in lvmcache.
+	 * This requires populating lvmcache when using lvmetad.)
+	 */
+	lvmcache_seed_infos_from_lvmetad(cmd);
+
+	if ((vginfo = lvmcache_vginfo_from_vgname(vp->vg_name_new, NULL))) {
+		log_error("New volume group \"%s\" already exists", vp->vg_name_new);
+		return ECMD_FAILED;
 	}
 
-	found_id = id_read_format_try(&id, vg_name_old);
-
-	if (found_id && (vg_name = lvmcache_vgname_from_vgid(cmd->mem, (char *)id.uuid))) {
-		if (!strcmp(vg_name, vg_name_new)) {
-			log_error("New VG name must differ from the old VG name.");
-			return 0;
-		}
-
-		vg_name_old = vg_name;
-		vgid = (char *)id.uuid;
-	} else
-		vgid = NULL;
-
-	if (!lockd_vg(cmd, vg_name_old, "ex", 0, &lockd_state))
-		return_0;
-
-	if (strcmp(vg_name_new, vg_name_old) < 0)
-		lock_vg_old_first = 0;
-
-	if (lock_vg_old_first) {
-		vg = _get_old_vg_for_rename(cmd, vg_name_old, vgid, lockd_state);
-		if (!vg)
-			return_0;
+	/*
+	 * The lock ordering may want the new VG name locked here,
+	 * after the old VG name has been locked by process_each_vg.
+	 */
+	if (!vp->old_name_is_uuid && vp->lock_vg_old_first) {
+		if (!_lock_new_vg_for_rename(cmd, vp->vg_name_new))
+			return ECMD_FAILED;
+	}
 
-		if (!_lock_new_vg_for_rename(cmd, vg_name_new)) {
-			unlock_and_release_vg(cmd, vg, vg_name_old);
-			return_0;
-		}
-	} else {
-		if (!_lock_new_vg_for_rename(cmd, vg_name_new))
-			return_0;
-
-		vg = _get_old_vg_for_rename(cmd, vg_name_old, vgid, lockd_state);
-		if (!vg) {
-			unlock_vg(cmd, vg_name_new);
-			return_0;
-		}
+	/*
+	 * Pre-locking was skipped because the old VG name looked
+	 * like a UUID.  The real old VG name has now been locked by
+	 * process_each_vg and now we need to lock the new VG name.
+	 * The lock ordering may want the new name locked first,
+	 * but it's too late for that here, so we need to suppress
+	 * the lock order check.  (There's a remote chance of a
+	 * problem with racing vgrenames between two names, with
+	 * one or both using a UUID.)
+	 */
+	if (vp->old_name_is_uuid) {
+		lvmcache_lock_ordering(0);
+		r = _lock_new_vg_for_rename(cmd, vp->vg_name_new);
+		lvmcache_lock_ordering(1);
+		if (!r)
+			return ECMD_FAILED;
 	}
 
+	dev_dir = cmd->dev_dir;
+
 	if (!archive(vg))
 		goto error;
 
@@ -159,7 +112,7 @@ static int vg_rename_path(struct cmd_context *cmd, const char *old_vg_path,
 	}
 
 	/* Change the volume group name */
-	vg_rename(cmd, vg, vg_name_new);
+	vg_rename(cmd, vg, vp->vg_name_new);
 
 	/* store it on disks */
 	log_verbose("Writing out updated volume group");
@@ -167,8 +120,8 @@ static int vg_rename_path(struct cmd_context *cmd, const char *old_vg_path,
 		goto error;
 	}
 
-	sprintf(old_path, "%s%s", dev_dir, vg_name_old);
-	sprintf(new_path, "%s%s", dev_dir, vg_name_new);
+	sprintf(old_path, "%s%s", dev_dir, vg_name);
+	sprintf(new_path, "%s%s", dev_dir, vp->vg_name_new);
 
 	if (activation() && dir_exists(old_path)) {
 		log_verbose("Renaming \"%s\" to \"%s\"", old_path, new_path);
@@ -189,49 +142,100 @@ static int vg_rename_path(struct cmd_context *cmd, const char *old_vg_path,
 
 	if (!backup(vg))
 		stack;
-	if (!backup_remove(cmd, vg_name_old))
+	if (!backup_remove(cmd, vg_name))
 		stack;
 
-	unlock_vg(cmd, vg_name_new);
-	unlock_and_release_vg(cmd, vg, vg_name_old);
+	unlock_vg(cmd, vp->vg_name_new);
+	vp->unlock_new_name = 0;
 
 	log_print_unless_silent("Volume group \"%s\" successfully renamed to \"%s\"",
-				vg_name_old, vg_name_new);
-
-	/* FIXME lvmcache corruption - vginfo duplicated instead of renamed */
-	if (cmd->filter->wipe)
-		cmd->filter->wipe(cmd->filter);
-	lvmcache_destroy(cmd, 1, 0);
-
+				vp->vg_name_old, vp->vg_name_new);
 	return 1;
 
-      error:
+ error:
+	unlock_vg(cmd, vp->vg_name_new);
+	vp->unlock_new_name = 0;
+
 	lockd_rename_vg_final(cmd, vg, 0);
 
-	if (lock_vg_old_first) {
-		unlock_vg(cmd, vg_name_new);
-		unlock_and_release_vg(cmd, vg, vg_name_old);
-	} else {
-		unlock_and_release_vg(cmd, vg, vg_name_old);
-		unlock_vg(cmd, vg_name_new);
-	}
 	return 0;
 }
 
 int vgrename(struct cmd_context *cmd, int argc, char **argv)
 {
+	struct vgrename_params vp = { 0 };
+	struct processing_handle *handle;
+	const char *vg_name_new;
+	const char *vg_name_old;
+	struct id id;
+	int ret;
+
 	if (argc != 2) {
 		log_error("Old and new volume group names need specifying");
 		return EINVALID_CMD_LINE;
 	}
 
+	vg_name_old = skip_dev_dir(cmd, argv[0], NULL);
+	vg_name_new = skip_dev_dir(cmd, argv[1], NULL);
+
+	if (!validate_vg_rename_params(cmd, vg_name_old, vg_name_new))
+		return_0;
+
+	if (!(vp.vg_name_old = dm_pool_strdup(cmd->mem, vg_name_old)))
+		return_ECMD_FAILED;
+
+	if (!(vp.vg_name_new = dm_pool_strdup(cmd->mem, vg_name_new)))
+		return_ECMD_FAILED;
+
 	/* Needed change the global VG namespace. */
 	if (!lockd_gl(cmd, "ex", LDGL_UPDATE_NAMES))
 		return_ECMD_FAILED;
 
-	if (!vg_rename_path(cmd, argv[0], argv[1]))
-		return_ECMD_FAILED;
+	/*
+	 * Special case where vg_name_old may be a UUID:
+	 * If vg_name_old is a UUID, then process_each may
+	 * translate it to an actual VG name that we don't
+	 * yet know.  The lock ordering, and pre-locking,
+	 * needs to be done based on VG names.  When
+	 * vg_name_old is a UUID, do not do any pre-locking
+	 * based on it, since it's likely to be wrong, and
+	 * defer all the locking to the _single function.
+	 *
+	 * When it's not a UUID, we know the two VG names,
+	 * and we can pre-lock the new VG name if the lock
+	 * ordering wants it locked before the old VG name
+	 * which will be locked by process_each.  If lock
+	 * ordering wants the old name locked first, then
+	 * the _single function will lock the new VG name.
+	 */
+	if (!(vp.old_name_is_uuid = id_read_format_try(&id, vg_name_old))) {
+		if (strcmp(vg_name_new, vg_name_old) < 0) {
+			vp.lock_vg_old_first = 0;
+			vp.unlock_new_name = 1;
+
+			if (!_lock_new_vg_for_rename(cmd, vg_name_new))
+				return ECMD_FAILED;
+		} else {
+			vp.lock_vg_old_first = 1;
+		}
+	}
+
+	if (!(handle = init_processing_handle(cmd))) {
+		log_error("Failed to initialize processing handle.");
+		return ECMD_FAILED;
+	}
+
+	handle->custom_handle = &vp;
+
+	ret = process_each_vg(cmd, 0, NULL, vg_name_old,
+			      READ_FOR_UPDATE | READ_ALLOW_EXPORTED,
+			      handle, _vgrename_single);
+
+	/* Needed if process_each_vg returns error before calling _single. */
+	if (vp.unlock_new_name)
+		unlock_vg(cmd, vg_name_new);
 
-	return ECMD_PROCESSED;
+	destroy_processing_handle(cmd, handle);
+	return ret;
 }