[PATCH] Another take on vg_read.

[PATCH] Another take on vg_read.

[Petr Rockai]

Hi,

I have done some more work on this series. I am resending everything, to avoid
sequencing problems like I introduced last time with a partial send.

Anyway, some news:
- the vg_check_status CLUSTERED fallthrough is fixed to return immediately
- cluster locking is now properly enforced by vg_read/vg_read_for_update
- process_each_pv is also ported over to new vg_read
- vg_read_internal is now private in metadata.c (static _vg_read_internal)
(plus, all patches should apply cleanly on top of today's CVS)

Hopefully, this moves things forward a little.

Yours,
   Petr.

[PATCH 1/14] Move vg_read out of the way, renaming it to vg_read_internal.

[Petr Rockai]

Mon Jan 12 17:38:59 CET 2009  Petr Rockai <me@mornfall.net>
  * Move vg_read out of the way, renaming it to vg_read_internal.
diff -rN -u -p old-temp.4430/daemons/clvmd/lvm-functions.c new-temp.4430/daemons/clvmd/lvm-functions.c
--- old-temp.4430/daemons/clvmd/lvm-functions.c	2009-01-22 11:02:32.294776955 +0100
+++ new-temp.4430/daemons/clvmd/lvm-functions.c	2009-01-22 11:02:32.322780654 +0100
@@ -711,7 +711,7 @@ void lvm_do_backup(const char *vgname)
 
 	DEBUGLOG("Triggering backup of VG metadata for %s. suspended=%d\n", vgname, suspended);
 
-	vg = vg_read(cmd, vgname, NULL /*vgid*/, &consistent);
+	vg = vg_read_internal(cmd, vgname, NULL /*vgid*/, &consistent);
 	if (vg) {
 		if (consistent)
 			check_current_backup(vg);
diff -rN -u -p old-temp.4430/lib/metadata/metadata.c new-temp.4430/lib/metadata/metadata.c
--- old-temp.4430/lib/metadata/metadata.c	2009-01-22 11:02:32.294776955 +0100
+++ new-temp.4430/lib/metadata/metadata.c	2009-01-22 11:02:32.342781520 +0100
@@ -224,8 +224,8 @@ int get_pv_from_vg_by_id(const struct fo
 	struct pv_list *pvl;
 	int consistent = 0;
 
-	if (!(vg = vg_read(fmt->cmd, vg_name, vgid, &consistent))) {
-		log_error("get_pv_from_vg_by_id: vg_read failed to read VG %s",
+	if (!(vg = vg_read_internal(fmt->cmd, vg_name, vgid, &consistent))) {
+		log_error("get_pv_from_vg_by_id: vg_read_internal failed to read VG %s",
 			  vg_name);
 		return 0;
 	}
@@ -503,7 +503,7 @@ struct volume_group *vg_create(struct cm
 		return_NULL;
 
 	/* is this vg name already in use ? */
-	if (vg_read(cmd, vg_name, NULL, &consistent)) {
+	if (vg_read_internal(cmd, vg_name, NULL, &consistent)) {
 		log_err("A volume group called '%s' already exists.", vg_name);
 		goto bad;
 	}
@@ -1684,12 +1684,12 @@ int vg_missing_pv_count(const vg_t *vg)
 	return ret;
 }
 
-/* Caller sets consistent to 1 if it's safe for vg_read to correct
+/* Caller sets consistent to 1 if it's safe for vg_read_internal to correct
  * inconsistent metadata on disk (i.e. the VG write lock is held).
  * This guarantees only consistent metadata is returned.
  * If consistent is 0, caller must check whether consistent == 1 on return
  * and take appropriate action if it isn't (e.g. abort; get write lock
- * and call vg_read again).
+ * and call vg_read_internal again).
  *
  * If precommitted is set, use precommitted metadata if present.
  *
@@ -1716,7 +1716,7 @@ static struct volume_group *_vg_read(str
 
 	if (is_orphan_vg(vgname)) {
 		if (use_precommitted) {
-			log_error("Internal error: vg_read requires vgname "
+			log_error("Internal error: vg_read_internal requires vgname "
 				  "with pre-commit.");
 			return NULL;
 		}
@@ -1974,7 +1974,7 @@ static struct volume_group *_vg_read(str
 	return correct_vg;
 }
 
-struct volume_group *vg_read(struct cmd_context *cmd, const char *vgname,
+struct volume_group *vg_read_internal(struct cmd_context *cmd, const char *vgname,
 			     const char *vgid, int *consistent)
 {
 	struct volume_group *vg;
@@ -2002,7 +2002,7 @@ struct volume_group *vg_read(struct cmd_
 
 /* This is only called by lv_from_lvid, which is only called from
  * activate.c so we know the appropriate VG lock is already held and
- * the vg_read is therefore safe.
+ * the vg_read_internal is therefore safe.
  */
 static struct volume_group *_vg_read_by_vgid(struct cmd_context *cmd,
 					    const char *vgid,
@@ -2034,7 +2034,7 @@ static struct volume_group *_vg_read_by_
 	if (memlock())
 		return NULL;
 
-	/* FIXME Need a genuine read by ID here - don't vg_read by name! */
+	/* FIXME Need a genuine read by ID here - don't vg_read_internal by name! */
 	/* FIXME Disabled vgrenames while active for now because we aren't
 	 *       allowed to do a full scan here any more. */
 
@@ -2218,7 +2218,7 @@ static int _get_pvs(struct cmd_context *
 			stack;
 			continue;
 		}
-		if (!(vg = vg_read(cmd, vgname, vgid, &consistent))) {
+		if (!(vg = vg_read_internal(cmd, vgname, vgid, &consistent))) {
 			stack;
 			continue;
 		}
@@ -2446,7 +2446,7 @@ vg_t *vg_lock_and_read(struct cmd_contex
 		return NULL;
 	}
 
-	if (!(vg = vg_read(cmd, vg_name, vgid, &consistent)) ||
+	if (!(vg = vg_read_internal(cmd, vg_name, vgid, &consistent)) ||
 	    ((misc_flags & FAIL_INCONSISTENT) && !consistent)) {
 		log_error("Volume group \"%s\" not found", vg_name);
 		unlock_vg(cmd, vg_name);
diff -rN -u -p old-temp.4430/lib/metadata/metadata-exported.h new-temp.4430/lib/metadata/metadata-exported.h
--- old-temp.4430/lib/metadata/metadata-exported.h	2009-01-22 11:02:32.294776955 +0100
+++ new-temp.4430/lib/metadata/metadata-exported.h	2009-01-22 11:02:32.342781520 +0100
@@ -76,7 +76,7 @@ struct pv_segment;
 						   written out in metadata*/
 
 //#define POSTORDER_FLAG	0x02000000U /* Not real flags, reserved for
-//#define POSTORDER_OPEN_FLAG	0x04000000U    temporary use inside vg_read. */
+//#define POSTORDER_OPEN_FLAG	0x04000000U    temporary use inside vg_read_internal. */
 
 #define LVM_READ              	0x00000100U	/* LV VG */
 #define LVM_WRITE             	0x00000200U	/* LV VG */
@@ -328,7 +328,7 @@ struct lv_list {
 int vg_write(struct volume_group *vg);
 int vg_commit(struct volume_group *vg);
 int vg_revert(struct volume_group *vg);
-struct volume_group *vg_read(struct cmd_context *cmd, const char *vg_name,
+struct volume_group *vg_read_internal(struct cmd_context *cmd, const char *vg_name,
 			     const char *vgid, int *consistent);
 struct physical_volume *pv_read(struct cmd_context *cmd, const char *pv_name,
 				struct dm_list *mdas, uint64_t *label_sector,
diff -rN -u -p old-temp.4430/lib/metadata/metadata.h new-temp.4430/lib/metadata/metadata.h
--- old-temp.4430/lib/metadata/metadata.h	2009-01-22 11:02:32.286779011 +0100
+++ new-temp.4430/lib/metadata/metadata.h	2009-01-22 11:02:32.342781520 +0100
@@ -79,7 +79,7 @@
 //						   written out in metadata*/
 
 #define POSTORDER_FLAG		0x02000000U /* Not real flags, reserved for  */
-#define POSTORDER_OPEN_FLAG	0x04000000U /* temporary use inside vg_read. */
+#define POSTORDER_OPEN_FLAG	0x04000000U /* temporary use inside vg_read_internal. */
 
 //#define LVM_READ              	0x00000100U	/* LV VG */
 //#define LVM_WRITE             	0x00000200U	/* LV VG */
diff -rN -u -p old-temp.4430/tools/pvdisplay.c new-temp.4430/tools/pvdisplay.c
--- old-temp.4430/tools/pvdisplay.c	2009-01-22 11:02:32.286779011 +0100
+++ new-temp.4430/tools/pvdisplay.c	2009-01-22 11:02:32.382780458 +0100
@@ -37,7 +37,7 @@ static int _pvdisplay_single(struct cmd_
 
 	 	/*
 		 * Replace possibly incomplete PV structure with new one
-		 * allocated in vg_read() path.
+		 * allocated in vg_read_internal() path.
 		 */
 		 if (!(pvl = find_pv_in_vg(vg, pv_name))) {
 			 log_error("Unable to find \"%s\" in volume group \"%s\"",
diff -rN -u -p old-temp.4430/tools/pvresize.c new-temp.4430/tools/pvresize.c
--- old-temp.4430/tools/pvresize.c	2009-01-22 11:02:32.286779011 +0100
+++ new-temp.4430/tools/pvresize.c	2009-01-22 11:02:32.382780458 +0100
@@ -62,7 +62,7 @@ static int _pv_resize_single(struct cmd_
 			return 0;
 		}
 
-		if (!(vg = vg_read(cmd, vg_name, NULL, &consistent))) {
+		if (!(vg = vg_read_internal(cmd, vg_name, NULL, &consistent))) {
 			unlock_vg(cmd, vg_name);
 			log_error("Unable to find volume group of \"%s\"",
 				  pv_name);
diff -rN -u -p old-temp.4430/tools/reporter.c new-temp.4430/tools/reporter.c
--- old-temp.4430/tools/reporter.c	2009-01-22 11:02:32.286779011 +0100
+++ new-temp.4430/tools/reporter.c	2009-01-22 11:02:32.382780458 +0100
@@ -136,7 +136,7 @@ static int _pvs_single(struct cmd_contex
 
 		/*
 		 * Replace possibly incomplete PV structure with new one
-		 * allocated in vg_read() path.
+		 * allocated in vg_read_internal() path.
 		*/
 		if (!(pvl = find_pv_in_vg(vg, pv_dev_name(pv)))) {
 			log_error("Unable to find \"%s\" in volume group \"%s\"",
diff -rN -u -p old-temp.4430/tools/toollib.c new-temp.4430/tools/toollib.c
--- old-temp.4430/tools/toollib.c	2009-01-22 11:02:32.286779011 +0100
+++ new-temp.4430/tools/toollib.c	2009-01-22 11:02:32.382780458 +0100
@@ -291,7 +291,7 @@ int process_each_lv(struct cmd_context *
 			consistent = 1;
 		else
 			consistent = 0;
-		if (!(vg = vg_read(cmd, vgname, NULL, &consistent)) || !consistent) {
+		if (!(vg = vg_read_internal(cmd, vgname, NULL, &consistent)) || !consistent) {
 			unlock_vg(cmd, vgname);
 			if (!vg)
 				log_error("Volume group \"%s\" "
@@ -433,7 +433,7 @@ static int _process_one_vg(struct cmd_co
 	}
 
 	log_verbose("Finding volume group \"%s\"", vg_name);
-	if (!(vg = vg_read(cmd, vg_name, vgid, &consistent))) {
+	if (!(vg = vg_read_internal(cmd, vg_name, vgid, &consistent))) {
 		log_error("Volume group \"%s\" not found", vg_name);
 		unlock_vg(cmd, vg_name);
 		return ECMD_FAILED;
@@ -720,7 +720,7 @@ int process_each_pv(struct cmd_context *
 					log_error("Can't lock %s: skipping", sll->str);
 					continue;
 				}
-				if (!(vg = vg_read(cmd, sll->str, NULL, &consistent))) {
+				if (!(vg = vg_read_internal(cmd, sll->str, NULL, &consistent))) {
 					log_error("Volume group \"%s\" not found", sll->str);
 					unlock_vg(cmd, sll->str);
 					ret_max = ECMD_FAILED;
@@ -1143,7 +1143,7 @@ struct volume_group *recover_vg(struct c
 		return NULL;
 	}
 
-	return vg_read(cmd, vgname, NULL, &consistent);
+	return vg_read_internal(cmd, vgname, NULL, &consistent);
 }
 
 int apply_lvname_restrictions(const char *name)
diff -rN -u -p old-temp.4430/tools/vgreduce.c new-temp.4430/tools/vgreduce.c
--- old-temp.4430/tools/vgreduce.c	2009-01-22 11:02:32.282780319 +0100
+++ new-temp.4430/tools/vgreduce.c	2009-01-22 11:02:32.386780408 +0100
@@ -426,7 +426,7 @@ static int _vgreduce_single(struct cmd_c
 	vg->free_count -= pv_pe_count(pv) - pv_pe_alloc_count(pv);
 	vg->extent_count -= pv_pe_count(pv);
 
-	if(!(orphan_vg = vg_read(cmd, vg->fid->fmt->orphan_vg_name, NULL, &consistent)) ||
+	if(!(orphan_vg = vg_read_internal(cmd, vg->fid->fmt->orphan_vg_name, NULL, &consistent)) ||
 	   !consistent) {
 		log_error("Unable to read existing orphan PVs");
 		unlock_vg(cmd, VG_ORPHANS);
@@ -520,7 +520,7 @@ int vgreduce(struct cmd_context *cmd, in
 		return ECMD_FAILED;
 	}
 
-	if ((!(vg = vg_read(cmd, vg_name, NULL, &consistent)) || !consistent)
+	if ((!(vg = vg_read_internal(cmd, vg_name, NULL, &consistent)) || !consistent)
 	    && !repairing) {
 		log_error("Volume group \"%s\" doesn't exist", vg_name);
 		unlock_vg(cmd, vg_name);
@@ -541,7 +541,7 @@ int vgreduce(struct cmd_context *cmd, in
 		}
 
 		consistent = !arg_count(cmd, force_ARG);
-		if (!(vg = vg_read(cmd, vg_name, NULL, &consistent))) {
+		if (!(vg = vg_read_internal(cmd, vg_name, NULL, &consistent))) {
 			log_error("Volume group \"%s\" not found", vg_name);
 			unlock_vg(cmd, vg_name);
 			return ECMD_FAILED;
diff -rN -u -p old-temp.4430/tools/vgrename.c new-temp.4430/tools/vgrename.c
--- old-temp.4430/tools/vgrename.c	2009-01-22 11:02:32.282780319 +0100
+++ new-temp.4430/tools/vgrename.c	2009-01-22 11:02:32.386780408 +0100
@@ -75,7 +75,7 @@ static int vg_rename_path(struct cmd_con
 		return 0;
 	}
 
-	if (!(vg = vg_read(cmd, vg_name_old, vgid, &consistent)) || !consistent) {
+	if (!(vg = vg_read_internal(cmd, vg_name_old, vgid, &consistent)) || !consistent) {
 		log_error("Volume group %s %s%s%snot found.", vg_name_old,
 		vgid ? "(" : "", vgid ? vgid : "", vgid ? ") " : "");
 		unlock_vg(cmd, vg_name_old);
@@ -107,7 +107,7 @@ static int vg_rename_path(struct cmd_con
 	}
 
 	consistent = 0;
-	if ((vg_new = vg_read(cmd, vg_name_new, NULL, &consistent))) {
+	if ((vg_new = vg_read_internal(cmd, vg_name_new, NULL, &consistent))) {
 		log_error("New volume group \"%s\" already exists",
 			  vg_name_new);
 		goto error;
diff -rN -u -p old-temp.4430/tools/vgsplit.c new-temp.4430/tools/vgsplit.c
--- old-temp.4430/tools/vgsplit.c	2009-01-22 11:02:32.282780319 +0100
+++ new-temp.4430/tools/vgsplit.c	2009-01-22 11:02:32.386780408 +0100
@@ -334,7 +334,7 @@ int vgsplit(struct cmd_context *cmd, int
 	}
 
 	consistent = 0;
-	if ((vg_to = vg_read(cmd, vg_name_to, NULL, &consistent))) {
+	if ((vg_to = vg_read_internal(cmd, vg_name_to, NULL, &consistent))) {
 		existing_vg = 1;
 		if (new_vg_option_specified(cmd)) {
 			log_error("Volume group \"%s\" exists, but new VG "
@@ -451,7 +451,7 @@ int vgsplit(struct cmd_context *cmd, int
 	 */
 	consistent = 1;
 	if (!test_mode() &&
-	    (!(vg_to = vg_read(cmd, vg_name_to, NULL, &consistent)) ||
+	    (!(vg_to = vg_read_internal(cmd, vg_name_to, NULL, &consistent)) ||
 	     !consistent)) {
 		log_error("Volume group \"%s\" became inconsistent: please "
 			  "fix manually", vg_name_to);

[PATCH 2/14] Provide _vg_lock_and_read, a new implementation of lock+vg_read_internal.

[Petr Rockai]

Thu Jan 22 10:36:41 CET 2009  Petr Rockai <me@mornfall.net>
  * Provide _vg_lock_and_read, a new implementation of lock+vg_read_internal.
  
  This is an internal function, to be later used by vg_read and
  vg_read_for_update. Not to be confused with the public function
  vg_lock_and_read, which will be removed later.
diff -rN -u -p old-temp.4430/lib/metadata/metadata.c new-temp.4430/lib/metadata/metadata.c
--- old-temp.4430/lib/metadata/metadata.c	2009-01-22 11:02:34.894778074 +0100
+++ new-temp.4430/lib/metadata/metadata.c	2009-01-22 11:02:34.942780194 +0100
@@ -2377,7 +2377,37 @@ int pv_analyze(struct cmd_context *cmd, 
 	return 1;
 }
 
+static uint32_t _vg_check_status(const struct volume_group *vg, uint32_t status)
+{
+	uint32_t failure = 0;
 
+	if ((status & CLUSTERED) &&
+	    (vg_is_clustered(vg)) && !locking_is_clustered() &&
+	    !lockingfailed()) {
+		log_error("Skipping clustered volume group %s", vg->name);
+		return FAILED_CLUSTERED;
+	}
+
+	if ((status & EXPORTED_VG) &&
+	    (vg->status & EXPORTED_VG)) {
+		log_error("Volume group %s is exported", vg->name);
+		failure |= FAILED_EXPORTED;
+	}
+
+	if ((status & LVM_WRITE) &&
+	    !(vg->status & LVM_WRITE)) {
+		log_error("Volume group %s is read-only", vg->name);
+		failure |= FAILED_READ_ONLY;
+	}
+
+	if ((status & RESIZEABLE_VG) &&
+	    !(vg->status & RESIZEABLE_VG)) {
+		log_error("Volume group %s is not resizeable.", vg->name);
+		failure |= FAILED_RESIZEABLE;
+	}
+
+	return failure;
+}
 
 /**
  * vg_check_status - check volume group status flags and log error
@@ -2462,6 +2492,133 @@ vg_t *vg_lock_and_read(struct cmd_contex
 }
 
 /*
+ * Create a (vg_t) volume group handle from a struct volume_group pointer and a
+ * possible failure code. Zero failure code = success.
+ */
+static vg_t *_vg_make_handle(struct cmd_context *cmd,
+			     struct volume_group *vg,
+			     uint32_t failure)
+{
+	if (!vg && !(vg = dm_pool_zalloc(cmd->mem, sizeof(*vg)))) {
+		log_error("Error allocating vg handle .");
+		return_NULL;
+	}
+	vg->read_failed = failure;
+	return (vg_t *) vg;
+}
+
+static vg_t *_recover_vg(struct cmd_context *cmd, const char *lock,
+			 const char *vg_name, const char *vgid,
+			 uint32_t lock_flags)
+{
+	int consistent = 1;
+	struct volume_group *vg;
+
+	lock_flags &= ~LCK_TYPE_MASK;
+	lock_flags |= LCK_WRITE;
+
+	unlock_vg(cmd, lock);
+
+	dev_close_all();
+
+	if (!lock_vol(cmd, lock, lock_flags))
+		return_NULL;
+
+	if (!(vg = vg_read_internal(cmd, vg_name, vgid, &consistent)))
+		return_NULL;
+	if (!consistent)
+		return_NULL;
+
+	return (vg_t *)vg;
+}
+
+/*
+ * _vg_lock_and_read - consolidate vg locking, reading, and status flag checking
+ * This is an internal function.
+ *
+ * misc_flags:
+ *  ALLOW_INCONSISTENT: disable autocorrection
+ *
+ * Setting ALLOW_INCONSISTENT might give you inconsistent metadata. You will
+ * *still* get FAILED_INCONSISTENT in case the metadata has *really* been
+ * inconsistent. However, you still get the latest version of metadata in VG.
+ *
+ * Returns:
+ * Use vg_read_error(vg) to determine the result. Nonzero vg_read_error(vg)
+ * means there were problems reading the volume group.
+ * Zero value means that the VG is open and appropriate locks are held.
+ */
+static vg_t *_vg_lock_and_read(struct cmd_context *cmd, const char *vg_name,
+			       const char *vgid,
+			       uint32_t lock_flags, uint32_t status_flags,
+			       uint32_t misc_flags)
+{
+	struct volume_group *vg = 0;
+	const char *lock;
+ 	int consistent = 1;
+	int consistent_in;
+	uint32_t failure = 0;
+
+	if (misc_flags & ALLOW_INCONSISTENT || !(lock_flags & LCK_WRITE))
+		consistent = 0;
+
+	if (!validate_name(vg_name) && !is_orphan_vg(vg_name)) {
+		log_error("Volume group name %s has invalid characters",
+			  vg_name);
+		return NULL;
+	}
+
+	lock = (misc_flags & ORPHAN_LOCK ? VG_ORPHANS : vg_name);
+	if (!(misc_flags & DISABLE_LOCK) && !lock_vol(cmd, lock, lock_flags)) {
+		log_error("Can't get lock for %s", vg_name);
+		return _vg_make_handle(cmd, vg, FAILED_LOCKING);
+	}
+
+	if (misc_flags & ORPHAN_LOCK)
+		status_flags &= ~LVM_WRITE;
+
+	if (misc_flags & EXISTENCE_CHECK)
+		consistent = 0;
+
+	consistent_in = consistent;
+
+	/* If consistent == 1, we get NULL here if correction fails. */
+	if (!(vg = vg_read_internal(cmd, vg_name, vgid, &consistent))) {
+		if (consistent_in && !consistent) {
+			log_error("Volume group \"%s\" inconsistent.", vg_name);
+			failure |= FAILED_INCONSISTENT;
+			goto_bad;
+		}
+		if (!(misc_flags & EXISTENCE_CHECK))
+			log_error("Volume group \"%s\" not found", vg_name);
+		failure |= FAILED_NOTFOUND | (misc_flags & EXISTENCE_CHECK);
+		goto_bad;
+	}
+
+	/* consistent == 0 when VG is not found, but failed == FAILED_NOTFOUND */
+	if (!consistent && !failure) {
+		if (!(vg = _recover_vg(cmd, lock, vg_name, vgid, lock_flags))) {
+			log_error("Recovery of volume group \"%s\" failed.",
+				  vg_name);
+			failure |= FAILED_INCONSISTENT;
+			goto_bad;
+		}
+	}
+
+	failure |= _vg_check_status(vg, status_flags);
+	if (failure)
+		goto_bad;
+
+	return _vg_make_handle(cmd, vg, failure);
+
+ bad:
+	if (failure != (FAILED_NOTFOUND | EXISTENCE_CHECK) &&
+	    !(misc_flags & KEEP_LOCK) && !(misc_flags & DISABLE_LOCK))
+			unlock_vg(cmd, lock);
+	return _vg_make_handle(cmd, vg, failure);
+}
+
+/*
  * Gets/Sets for external LVM library
  */
 struct id pv_id(const pv_t *pv)
diff -rN -u -p old-temp.4430/lib/metadata/metadata-exported.h new-temp.4430/lib/metadata/metadata-exported.h
--- old-temp.4430/lib/metadata/metadata-exported.h	2009-01-22 11:02:34.898782074 +0100
+++ new-temp.4430/lib/metadata/metadata-exported.h	2009-01-22 11:02:34.942780194 +0100
@@ -104,6 +104,43 @@ struct pv_segment;
 #define MIRROR_BY_LV		0x00000002U	/* mirror using whole mimage LVs */
 #define MIRROR_SKIP_INIT_SYNC	0x00000010U	/* skip initial sync */
 
+/* vg_read and vg_read_for_update flags */
+#define ALLOW_INCONSISTENT 0x1
+#define ALLOW_EXPORTED     0x2
+#define REQUIRE_RESIZEABLE 0x4
+#define EXISTENCE_CHECK    0x8
+
+/* FIXME these should be removed when we get better internal locking logic that
+   does not rely on caller to provide locking details. */
+#define NONBLOCKING_LOCK 0x100 /* Give up instead of waiting for a lock;
+                                  required when acquiring a second VG lock. */
+#define KEEP_LOCK        0x200 /* Do not unlock upon read failure. */
+
+/* FIXME the following flags should be removed in favour of better APIs
+   (vg_reread and vg_read_orphan?) */
+#define DISABLE_LOCK     0x400 /* Disable locking altogether. DANGEROUS. Only
+				  use when you already hold appropriate lock
+				  for this VG. */
+#define ORPHAN_LOCK      0x800 /* Use when you vg_read(_for_update) the orphan
+				  VG. */
+
+/* A meta-flag, useful with toollib for_each_* functions. */
+#define READ_FOR_UPDATE 0x1000
+
+/* vg's "read_failed" field */
+#define FAILED_INCONSISTENT 0x1
+#define FAILED_LOCKING      0x2
+#define FAILED_NOTFOUND     0x4
+/* #define EXISTENCE_CHECK  0x8 -- left out for EXISTENCE_CHECK above, as it can
+   be or'd into failure flags */
+
+#define FAILED_READ_ONLY   0x10
+#define FAILED_EXPORTED    0x20
+#define FAILED_RESIZEABLE  0x40
+#define FAILED_CLUSTERED   0x80
+
+#define FAILED_ALLOCATION 0x100
+
 /* Ordered list - see lv_manip.c */
 typedef enum {
 	ALLOC_INVALID,
@@ -196,6 +233,9 @@ struct volume_group {
 	char *name;
 	char *system_id;
 
+	uint32_t read_failed; /* 0 = no, everything OK. Otherwise uses FAILED_*
+				 flags as defined above. */
+
 	uint32_t status;
 	alloc_policy_t alloc;
 

[PATCH 3/14] Properly enforce cluster locking in _vg_lock_and_read.

[Petr Rockai]

Wed Jan 21 15:52:36 CET 2009  Petr Rockai <me@mornfall.net>
  * Properly enforce cluster locking in _vg_lock_and_read.
diff -rN -u -p old-temp.4430/lib/metadata/metadata.c new-temp.4430/lib/metadata/metadata.c
--- old-temp.4430/lib/metadata/metadata.c	2009-01-22 11:02:36.870777955 +0100
+++ new-temp.4430/lib/metadata/metadata.c	2009-01-22 11:02:36.918780355 +0100
@@ -2595,6 +2595,12 @@ static vg_t *_vg_lock_and_read(struct cm
 		goto_bad;
 	}
 
+	if (vg_is_clustered(vg) && !locking_is_clustered()) {
+		log_error("Skipping clustered volume group %s", vg->name);
+		failure |= FAILED_CLUSTERED;
+		goto_bad;
+	}
+
 	/* consistent == 0 when VG is not found, but failed == FAILED_NOTFOUND */
 	if (!consistent && !failure) {
 		if (!(vg = _recover_vg(cmd, lock, vg_name, vgid, lock_flags))) {
@@ -2605,7 +2611,7 @@ static vg_t *_vg_lock_and_read(struct cm
 		}
 	}
 
-	failure |= _vg_check_status(vg, status_flags);
+	failure |= _vg_check_status(vg, status_flags & ~CLUSTERED);
 	if (failure)
 		goto_bad;
 

[PATCH 4/14] Replace implementation of vg_check_status with a call to _vg_check_status.

[Petr Rockai]

Thu Jan 15 10:38:04 CET 2009  Petr Rockai <me@mornfall.net>
  * Replace implementation of vg_check_status with a call to _vg_check_status.
diff -rN -u -p old-temp.4430/lib/metadata/metadata.c new-temp.4430/lib/metadata/metadata.c
--- old-temp.4430/lib/metadata/metadata.c	2009-01-22 11:02:38.798779559 +0100
+++ new-temp.4430/lib/metadata/metadata.c	2009-01-22 11:02:38.854780880 +0100
@@ -2420,31 +2420,7 @@ static uint32_t _vg_check_status(const s
  */
 int vg_check_status(const struct volume_group *vg, uint32_t status)
 {
-	if ((status & CLUSTERED) &&
-	    (vg_is_clustered(vg)) && !locking_is_clustered() &&
-	    !lockingfailed()) {
-		log_error("Skipping clustered volume group %s", vg->name);
-		return 0;
-	}
-
-	if ((status & EXPORTED_VG) &&
-	    (vg->status & EXPORTED_VG)) {
-		log_error("Volume group %s is exported", vg->name);
-		return 0;
-	}
-
-	if ((status & LVM_WRITE) &&
-	    !(vg->status & LVM_WRITE)) {
-		log_error("Volume group %s is read-only", vg->name);
-		return 0;
-	}
-	if ((status & RESIZEABLE_VG) &&
-	    !(vg->status & RESIZEABLE_VG)) {
-		log_error("Volume group %s is not resizeable.", vg->name);
-		return 0;
-	}
-
-	return 1;
+	return !_vg_check_status(vg, status);
 }
 
 /*

[PATCH 5/14] Implement vg_read and vg_read_for_update.

[Petr Rockai]

Thu Jan 15 10:55:15 CET 2009  Petr Rockai <me@mornfall.net>
  * Implement vg_read and vg_read_for_update.
diff -rN -u -p old-temp.4430/lib/metadata/metadata.c new-temp.4430/lib/metadata/metadata.c
--- old-temp.4430/lib/metadata/metadata.c	2009-01-22 11:02:40.622781563 +0100
+++ new-temp.4430/lib/metadata/metadata.c	2009-01-22 11:02:40.670780470 +0100
@@ -2601,6 +2601,74 @@ static vg_t *_vg_lock_and_read(struct cm
 }
 
 /*
+ * vg_read: High-level volume group metadata read function.
+ *
+ * On failure, an error-indicating handle is returned. Users should use
+ * "vg_read_error" to check whether handle indicates an error or it points to
+ * valid volume group data. Moreover, vg_read_error will provide more details
+ * about the kind of failure:
+ *
+ *  - metadata inconsistent and automatic correction failed: FAILED_INCONSISTENT
+ *  - VG is read-only: FAILED_READ_ONLY
+ *  - VG is EXPORTED, unless flags has ALLOW_EXPORTED: FAILED_EXPORTED
+ *  - VG is not RESIZEABLE, unless flags has ALLOW_NONRESIZEABLE:
+ *    FAILED_RESIZEABLE
+ *  - locking failed: FAILED_LOCKING
+ *
+ * On failures, all locks are released, unless KEEP_LOCK has been supplied.
+ *
+ * By default, volume groups are opened read-only. To open a VG for updates,
+ * please use vg_read_for_update (see below).
+ *
+ * Checking for VG existence:
+ *
+ * If EXISTENCE_CHECK is set in flags, if the VG exists, a non-NULL struct
+ * volume_group will be returned every time, but if it has INCONSISTENT_VG set,
+ * the other fields will be uninitialized. You *have to* check for
+ * INCONSISTENT_VG if passing EXISTENCE_CHECK. You also *must not* use it if it
+ * has INCONSISTENT_VG set.
+ *
+ * FIXME: We want vg_read to attempt automatic recovery after acquiring a
+ * temporary write lock -- if that fails, we bail out as usual, with failed &
+ * FAILED_INCONSISTENT. If it works, we are good to go. Code that's been in
+ * toollib just set lock type to LCK_WRITE and called vg_read_internal with
+ * *consistent = 1.
+ */
+vg_t *vg_read(struct cmd_context *cmd, const char *vg_name,
+	      const char *vgid, uint32_t flags)
+{
+	uint32_t status = CLUSTERED;
+	uint32_t lock = LCK_VG_READ;
+
+	if (flags & READ_FOR_UPDATE) {
+		status |= EXPORTED_VG | LVM_WRITE;
+		lock = LCK_VG_WRITE;
+	}
+
+	if (flags & ALLOW_EXPORTED)
+		status &= ~EXPORTED_VG;
+
+	if (flags & REQUIRE_RESIZEABLE)
+		status |= RESIZEABLE_VG;
+
+	if (flags & NONBLOCKING_LOCK)
+		lock |= LCK_NONBLOCK;
+
+	return _vg_lock_and_read(cmd, vg_name, vgid, lock, status, flags);
+}
+
+/*
+ * A high-level volume group metadata reading function. Open a volume group for
+ * later update (this means the user code can change the metadata and later
+ * request the new metadata to be written and committed).
+ */
+vg_t *vg_read_for_update(struct cmd_context *cmd, const char *vg_name,
+			 const char *vgid, uint32_t flags)
+{
+	return vg_read(cmd, vg_name, vgid, flags | READ_FOR_UPDATE);
+}
+
+/*
  * Gets/Sets for external LVM library
  */
 struct id pv_id(const pv_t *pv)
diff -rN -u -p old-temp.4430/lib/metadata/metadata-exported.h new-temp.4430/lib/metadata/metadata-exported.h
--- old-temp.4430/lib/metadata/metadata-exported.h	2009-01-22 11:02:40.622781563 +0100
+++ new-temp.4430/lib/metadata/metadata-exported.h	2009-01-22 11:02:40.670780470 +0100
@@ -393,6 +393,12 @@ vg_t *vg_lock_and_read(struct cmd_contex
 		       uint32_t lock_flags, uint32_t status_flags,
 		       uint32_t misc_flags);
 
+/* Loading volume group metadata. */
+vg_t *vg_read(struct cmd_context *cmd, const char *vg_name,
+              const char *vgid, uint32_t flags);
+vg_t *vg_read_for_update(struct cmd_context *cmd, const char *vg_name,
+                         const char *vgid, uint32_t flags);
+
 /* pe_start and pe_end relate to any existing data so that new metadata
 * areas can avoid overlap */
 pv_t *pv_create(const struct cmd_context *cmd,

[PATCH 6/14] Add vg_read_error and vg_might_exist.

[Petr Rockai]

Thu Jan 15 10:55:43 CET 2009  Petr Rockai <me@mornfall.net>
  * Add vg_read_error and vg_might_exist.
diff -rN -u -p old-temp.4430/lib/metadata/metadata.c new-temp.4430/lib/metadata/metadata.c
--- old-temp.4430/lib/metadata/metadata.c	2009-01-22 11:02:42.462777778 +0100
+++ new-temp.4430/lib/metadata/metadata.c	2009-01-22 11:02:42.510780457 +0100
@@ -2669,6 +2669,40 @@ vg_t *vg_read_for_update(struct cmd_cont
 }
 
 /*
+ * Check whether a vg_read (or vg_read_for_update) operation resulted in an
+ * error, or completed successfully. If EXISTENCE_CHECK has been given to
+ * vg_read, we do not consider nonexistence of the volume group an error. The
+ * caller is responsible for using vg_might_exist:
+ * - !vg_read_error(vg) && vg_might_exist(vg) -> vg is a valid handle
+ * - vg_read_error(vg) && vg_might_exist(vg) -> vg is not valid, but the VG in
+ *   question probably exists, it just cannot be opened (consult vg_read_error
+ *   return code for reason).
+ * - !vg_read_error(vg) && !vg_might_exist(vg) -> the VG does not exist
+ * - vg_read_error(vg) && !vg_might_exist(vg) -> cannot happen
+ */
+uint32_t vg_read_error(vg_t *vg)
+{
+	if (!vg)
+		return FAILED_ALLOCATION;
+	if (vg->read_failed & EXISTENCE_CHECK)
+		return vg->read_failed & ~(EXISTENCE_CHECK | FAILED_NOTFOUND);
+	return vg->read_failed;
+}
+
+/*
+ * Returns true if the volume group already exists. If unsure, it will return
+ * true (it might exist, but we are not sure, as the read failed for some other
+ * reason).
+ */
+uint32_t vg_might_exist(vg_t *vg) {
+	if (!vg)
+		return 1;
+	if (vg->read_failed == (FAILED_NOTFOUND | EXISTENCE_CHECK))
+		return 0;
+	return 1;
+}
+
+/*
  * Gets/Sets for external LVM library
  */
 struct id pv_id(const pv_t *pv)
diff -rN -u -p old-temp.4430/lib/metadata/metadata-exported.h new-temp.4430/lib/metadata/metadata-exported.h
--- old-temp.4430/lib/metadata/metadata-exported.h	2009-01-22 11:02:42.462777778 +0100
+++ new-temp.4430/lib/metadata/metadata-exported.h	2009-01-22 11:02:42.510780457 +0100
@@ -399,6 +399,10 @@ vg_t *vg_read(struct cmd_context *cmd, c
 vg_t *vg_read_for_update(struct cmd_context *cmd, const char *vg_name,
                          const char *vgid, uint32_t flags);
 
+/* Queries on a (possibly error-indicating) VG handle. */
+uint32_t vg_read_error(vg_t *vg);
+uint32_t vg_might_exist(vg_t *vg);
+
 /* pe_start and pe_end relate to any existing data so that new metadata
 * areas can avoid overlap */
 pv_t *pv_create(const struct cmd_context *cmd,

[PATCH 7/14] Convert the straight instances of vg_lock_and_read to new vg_read(_for_update).

[Petr Rockai]

Fri Jan  9 15:21:07 CET 2009  Petr Rockai <me@mornfall.net>
  * Convert the straight instances of vg_lock_and_read to new vg_read(_for_update).
diff -rN -u -p old-temp.4430/tools/lvconvert.c new-temp.4430/tools/lvconvert.c
--- old-temp.4430/tools/lvconvert.c	2009-01-22 11:02:44.610781014 +0100
+++ new-temp.4430/tools/lvconvert.c	2009-01-22 11:02:44.694777025 +0100
@@ -240,10 +240,8 @@ static struct volume_group *_get_lvconve
 {
 	dev_close_all();
 
-        return vg_lock_and_read(cmd, extract_vgname(cmd, lv_name),
-				NULL, LCK_VG_WRITE,
- 				CLUSTERED | EXPORTED_VG | LVM_WRITE,
-				CORRECT_INCONSISTENT | FAIL_INCONSISTENT);
+        return vg_read_for_update(cmd, extract_vgname(cmd, lv_name),
+				  NULL, 0);
 }
 
 static struct logical_volume *_get_lvconvert_lv(struct cmd_context *cmd __attribute((unused)),
@@ -746,9 +744,8 @@ int lvconvert(struct cmd_context * cmd, 
 
 	log_verbose("Checking for existing volume group \"%s\"", lp.vg_name);
 
-	if (!(vg = vg_lock_and_read(cmd, lp.vg_name, NULL, LCK_VG_WRITE,
-				    CLUSTERED | EXPORTED_VG | LVM_WRITE,
-				    CORRECT_INCONSISTENT)))
+	vg = vg_read_for_update(cmd, lp.vg_name, NULL, 0);
+	if (vg_read_error(vg))
 		return ECMD_FAILED;
 
 	if (!(lvl = find_lv_in_vg(vg, lp.lv_name))) {
diff -rN -u -p old-temp.4430/tools/lvcreate.c new-temp.4430/tools/lvcreate.c
--- old-temp.4430/tools/lvcreate.c	2009-01-22 11:02:44.610781014 +0100
+++ new-temp.4430/tools/lvcreate.c	2009-01-22 11:02:44.694777025 +0100
@@ -896,9 +896,8 @@ int lvcreate(struct cmd_context *cmd, in
 		return EINVALID_CMD_LINE;
 
 	log_verbose("Finding volume group \"%s\"", lp.vg_name);
-	if (!(vg = vg_lock_and_read(cmd, lp.vg_name, NULL, LCK_VG_WRITE,
-				    CLUSTERED | EXPORTED_VG | LVM_WRITE,
-				    CORRECT_INCONSISTENT)))
+	vg = vg_read_for_update(cmd, lp.vg_name, NULL, 0);
+	if (vg_read_error(vg))
 		return ECMD_FAILED;
 
 	if (!_lvcreate(cmd, vg, &lp))
diff -rN -u -p old-temp.4430/tools/lvrename.c new-temp.4430/tools/lvrename.c
--- old-temp.4430/tools/lvrename.c	2009-01-22 11:02:44.610781014 +0100
+++ new-temp.4430/tools/lvrename.c	2009-01-22 11:02:44.698780397 +0100
@@ -101,9 +101,8 @@ int lvrename(struct cmd_context *cmd, in
 	}
 
 	log_verbose("Checking for existing volume group \"%s\"", vg_name);
-	if (!(vg = vg_lock_and_read(cmd, vg_name, NULL, LCK_VG_WRITE,
-				    CLUSTERED | EXPORTED_VG | LVM_WRITE,
-				    CORRECT_INCONSISTENT)))
+	vg = vg_read_for_update(cmd, vg_name, NULL, 0);
+	if (vg_read_error(vg))
 		return ECMD_FAILED;
 
 	if (!(lvl = find_lv_in_vg(vg, lv_name_old))) {
diff -rN -u -p old-temp.4430/tools/lvresize.c new-temp.4430/tools/lvresize.c
--- old-temp.4430/tools/lvresize.c	2009-01-22 11:02:44.610781014 +0100
+++ new-temp.4430/tools/lvresize.c	2009-01-22 11:02:44.698780397 +0100
@@ -661,9 +661,8 @@ int lvresize(struct cmd_context *cmd, in
 		return EINVALID_CMD_LINE;
 
 	log_verbose("Finding volume group %s", lp.vg_name);
-	if (!(vg = vg_lock_and_read(cmd, lp.vg_name, NULL, LCK_VG_WRITE,
-				    CLUSTERED | EXPORTED_VG | LVM_WRITE,
-				    CORRECT_INCONSISTENT))) {
+	vg = vg_read_for_update(cmd, lp.vg_name, NULL, 0);
+	if (vg_read_error(vg)) {
 		stack;
 		return ECMD_FAILED;
 	}
diff -rN -u -p old-temp.4430/tools/polldaemon.c new-temp.4430/tools/polldaemon.c
--- old-temp.4430/tools/polldaemon.c	2009-01-22 11:02:44.610781014 +0100
+++ new-temp.4430/tools/polldaemon.c	2009-01-22 11:02:44.698780397 +0100
@@ -147,7 +147,8 @@ static int _wait_for_single_mirror(struc
 		}
 
 		/* Locks the (possibly renamed) VG again */
-		if (!(vg = parms->poll_fns->get_copy_vg(cmd, name))) {
+		vg = parms->poll_fns->get_copy_vg(cmd, name);
+		if (vg_read_error(vg)) {
 			log_error("ABORTING: Can't reread VG for %s", name);
 			/* What more could we do here? */
 			return 0;
diff -rN -u -p old-temp.4430/tools/pvchange.c new-temp.4430/tools/pvchange.c
--- old-temp.4430/tools/pvchange.c	2009-01-22 11:02:44.610781014 +0100
+++ new-temp.4430/tools/pvchange.c	2009-01-22 11:02:44.698780397 +0100
@@ -56,9 +56,8 @@ static int _pvchange_single(struct cmd_c
 
 		log_verbose("Finding volume group %s of physical volume %s",
 			    vg_name, pv_name);
-		if (!(vg = vg_lock_and_read(cmd, vg_name, NULL, LCK_VG_WRITE,
-					    CLUSTERED | EXPORTED_VG | LVM_WRITE,
-					    CORRECT_INCONSISTENT)))
+		vg = vg_read_for_update(cmd, vg_name, NULL, 0);
+		if (vg_read_error(vg))
 			return_0;
 
 		if (!(pvl = find_pv_in_vg(vg, pv_name))) {
diff -rN -u -p old-temp.4430/tools/pvdisplay.c new-temp.4430/tools/pvdisplay.c
--- old-temp.4430/tools/pvdisplay.c	2009-01-22 11:02:44.610781014 +0100
+++ new-temp.4430/tools/pvdisplay.c	2009-01-22 11:02:44.698780397 +0100
@@ -28,8 +28,8 @@ static int _pvdisplay_single(struct cmd_
 
 	if (!is_orphan(pv) && !vg) {
 		vg_name = pv_vg_name(pv);
-		if (!(vg = vg_lock_and_read(cmd, vg_name, (char *)&pv->vgid,
-					    LCK_VG_READ, CLUSTERED, 0))) {
+		vg = vg_read(cmd, vg_name, (char *)&pv->vgid, 0);
+		if (vg_read_error(vg)) {
 		 	log_error("Skipping volume group %s", vg_name);
 			/* FIXME If CLUSTERED should return ECMD_PROCESSED here */
 		 	return ECMD_FAILED;
diff -rN -u -p old-temp.4430/tools/pvmove.c new-temp.4430/tools/pvmove.c
--- old-temp.4430/tools/pvmove.c	2009-01-22 11:02:44.606777572 +0100
+++ new-temp.4430/tools/pvmove.c	2009-01-22 11:02:44.698780397 +0100
@@ -89,16 +89,9 @@ static const char *_extract_lvname(struc
 
 static struct volume_group *_get_vg(struct cmd_context *cmd, const char *vgname)
 {
-	struct volume_group *vg;
-
 	dev_close_all();
 
-	if (!(vg = vg_lock_and_read(cmd, vgname, NULL, LCK_VG_WRITE,
-				    CLUSTERED | EXPORTED_VG | LVM_WRITE,
-				    CORRECT_INCONSISTENT | FAIL_INCONSISTENT)))
-		 return NULL;
-
-	return vg;
+	return vg_read_for_update(cmd, vgname, NULL, 0);
 }
 
 /* Create list of PVs for allocation of replacement extents */
@@ -382,7 +375,8 @@ static int _set_up_pvmove(struct cmd_con
 	/* Read VG */
 	log_verbose("Finding volume group \"%s\"", pv_vg_name(pv));
 
-	if (!(vg = _get_vg(cmd, pv_vg_name(pv)))) {
+	vg = _get_vg(cmd, pv_vg_name(pv));
+	if (vg_read_error(vg)) {
 		stack;
 		return ECMD_FAILED;
 	}
diff -rN -u -p old-temp.4430/tools/pvresize.c new-temp.4430/tools/pvresize.c
--- old-temp.4430/tools/pvresize.c	2009-01-22 11:02:44.606777572 +0100
+++ new-temp.4430/tools/pvresize.c	2009-01-22 11:02:44.698780397 +0100
@@ -29,7 +29,6 @@ static int _pv_resize_single(struct cmd_
 			     const uint64_t new_size)
 {
 	struct pv_list *pvl;
-	int consistent = 1;
 	uint64_t size = 0;
 	uint32_t new_pe_count = 0;
 	struct dm_list mdas;
@@ -57,22 +56,10 @@ static int _pv_resize_single(struct cmd_
 	} else {
 		vg_name = pv_vg_name(pv);
 
-		if (!lock_vol(cmd, vg_name, LCK_VG_WRITE)) {
-			log_error("Can't get lock for %s", pv_vg_name(pv));
-			return 0;
-		}
-
-		if (!(vg = vg_read_internal(cmd, vg_name, NULL, &consistent))) {
-			unlock_vg(cmd, vg_name);
-			log_error("Unable to find volume group of \"%s\"",
-				  pv_name);
-			return 0;
-		}
+		vg = vg_read_for_update(cmd, vg_name, NULL, 0);
 
-		if (!vg_check_status(vg, CLUSTERED | EXPORTED_VG | LVM_WRITE)) {
-			unlock_vg(cmd, vg_name);
+		if (vg_read_error(vg))
 			return 0;
-		}
 
 		if (!(pvl = find_pv_in_vg(vg, pv_name))) {
 			unlock_vg(cmd, vg_name);
diff -rN -u -p old-temp.4430/tools/reporter.c new-temp.4430/tools/reporter.c
--- old-temp.4430/tools/reporter.c	2009-01-22 11:02:44.606777572 +0100
+++ new-temp.4430/tools/reporter.c	2009-01-22 11:02:44.698780397 +0100
@@ -128,8 +128,8 @@ static int _pvs_single(struct cmd_contex
 	if (is_pv(pv) && !is_orphan(pv) && !vg) {
 		vg_name = pv_vg_name(pv);
 
-		if (!(vg = vg_lock_and_read(cmd, vg_name, (char *)&pv->vgid,
-					    LCK_VG_READ, CLUSTERED, 0))) {
+		vg = vg_read(cmd, vg_name, (char *)&pv->vgid, 0);
+		if (vg_read_error(vg)) {
 			log_error("Skipping volume group %s", vg_name);
 			return ECMD_FAILED;
 		}
diff -rN -u -p old-temp.4430/tools/vgextend.c new-temp.4430/tools/vgextend.c
--- old-temp.4430/tools/vgextend.c	2009-01-22 11:02:44.606777572 +0100
+++ new-temp.4430/tools/vgextend.c	2009-01-22 11:02:44.702779927 +0100
@@ -41,11 +41,10 @@ int vgextend(struct cmd_context *cmd, in
 	}
 
 	log_verbose("Checking for volume group \"%s\"", vg_name);
-	if (!(vg = vg_lock_and_read(cmd, vg_name, NULL, LCK_VG_WRITE | LCK_NONBLOCK,
-				    CLUSTERED | EXPORTED_VG |
-				    LVM_WRITE | RESIZEABLE_VG,
-				    CORRECT_INCONSISTENT | FAIL_INCONSISTENT))) {
-		 unlock_vg(cmd, VG_ORPHANS);
+	vg = vg_read_for_update(cmd, vg_name, NULL,
+				REQUIRE_RESIZEABLE | NONBLOCKING_LOCK);
+	if (vg_read_error(vg)) {
+		unlock_vg(cmd, VG_ORPHANS);
 		return ECMD_FAILED;
 	 }
 /********** FIXME
diff -rN -u -p old-temp.4430/tools/vgmerge.c new-temp.4430/tools/vgmerge.c
--- old-temp.4430/tools/vgmerge.c	2009-01-22 11:02:44.606777572 +0100
+++ new-temp.4430/tools/vgmerge.c	2009-01-22 11:02:44.702779927 +0100
@@ -27,16 +27,14 @@ static int _vgmerge_single(struct cmd_co
 	}
 
 	log_verbose("Checking for volume group \"%s\"", vg_name_to);
-	if (!(vg_to = vg_lock_and_read(cmd, vg_name_to, NULL, LCK_VG_WRITE,
-				       CLUSTERED | EXPORTED_VG | LVM_WRITE,
-				       CORRECT_INCONSISTENT | FAIL_INCONSISTENT)))
+	vg_to = vg_read_for_update(cmd, vg_name_to, NULL, 0);
+	if (vg_read_error(vg_to))
 		 return ECMD_FAILED;
 
 	log_verbose("Checking for volume group \"%s\"", vg_name_from);
-	if (!(vg_from = vg_lock_and_read(cmd, vg_name_from, NULL,
-					 LCK_VG_WRITE | LCK_NONBLOCK,
-					 CLUSTERED | EXPORTED_VG | LVM_WRITE,
-					 CORRECT_INCONSISTENT | FAIL_INCONSISTENT))) {
+	vg_from = vg_read_for_update(cmd, vg_name_from, NULL,
+				     NONBLOCKING_LOCK);
+	if (vg_read_error(vg_from)) {
 		unlock_vg(cmd, vg_name_to);
 		return ECMD_FAILED;
 	}

[PATCH 8/14] Convert vgreduce to use vg_read_for_update.

[Petr Rockai]

Fri Jan  9 15:38:20 CET 2009  Petr Rockai <me@mornfall.net>
  * Convert vgreduce to use vg_read_for_update.
diff -rN -u -p old-temp.4430/tools/vgreduce.c new-temp.4430/tools/vgreduce.c
--- old-temp.4430/tools/vgreduce.c	2009-01-22 11:02:46.762781688 +0100
+++ new-temp.4430/tools/vgreduce.c	2009-01-22 11:02:46.894781355 +0100
@@ -382,7 +382,6 @@ static int _vgreduce_single(struct cmd_c
 {
 	struct pv_list *pvl;
 	struct volume_group *orphan_vg;
-	int consistent = 1;
 	const char *name = pv_dev_name(pv);
 
 	if (pv_pe_alloc_count(pv)) {
@@ -396,17 +395,10 @@ static int _vgreduce_single(struct cmd_c
 		return ECMD_FAILED;
 	}
 
-	if (!lock_vol(cmd, VG_ORPHANS, LCK_VG_WRITE | LCK_NONBLOCK)) {
-		log_error("Can't get lock for orphan PVs");
-		return ECMD_FAILED;
-	}
-
 	pvl = find_pv_in_vg(vg, name);
 
-	if (!archive(vg)) {
-		unlock_vg(cmd, VG_ORPHANS);
+	if (!archive(vg))
 		return ECMD_FAILED;
-	}
 
 	log_verbose("Removing \"%s\" from volume group \"%s\"", name, vg->name);
 
@@ -426,12 +418,11 @@ static int _vgreduce_single(struct cmd_c
 	vg->free_count -= pv_pe_count(pv) - pv_pe_alloc_count(pv);
 	vg->extent_count -= pv_pe_count(pv);
 
-	if(!(orphan_vg = vg_read_internal(cmd, vg->fid->fmt->orphan_vg_name, NULL, &consistent)) ||
-	   !consistent) {
-		log_error("Unable to read existing orphan PVs");
-		unlock_vg(cmd, VG_ORPHANS);
+	orphan_vg = vg_read_for_update(cmd, vg->fid->fmt->orphan_vg_name,
+				       NULL, NONBLOCKING_LOCK | ORPHAN_LOCK);
+
+	if (vg_read_error(orphan_vg))
 		return ECMD_FAILED;
-	}
 
 	if (!vg_split_mdas(cmd, vg, orphan_vg) || !vg->pv_count) {
 		log_error("Cannot remove final metadata area on \"%s\" from \"%s\"",
@@ -468,7 +459,6 @@ int vgreduce(struct cmd_context *cmd, in
 	struct volume_group *vg;
 	char *vg_name;
 	int ret = 1;
-	int consistent = 1;
 	int fixed = 1;
 	int repairing = arg_count(cmd, removemissing_ARG);
 
@@ -515,41 +505,37 @@ int vgreduce(struct cmd_context *cmd, in
 	}
 
 	log_verbose("Finding volume group \"%s\"", vg_name);
-	if (!lock_vol(cmd, vg_name, LCK_VG_WRITE)) {
-		log_error("Can't get lock for %s", vg_name);
-		return ECMD_FAILED;
-	}
 
-	if ((!(vg = vg_read_internal(cmd, vg_name, NULL, &consistent)) || !consistent)
-	    && !repairing) {
-		log_error("Volume group \"%s\" doesn't exist", vg_name);
-		unlock_vg(cmd, vg_name);
+	vg = vg_read_for_update(cmd, vg_name, NULL, ALLOW_EXPORTED);
+	if (vg_read_error(vg) == FAILED_ALLOCATION ||
+	    vg_read_error(vg) == FAILED_NOTFOUND)
 		return ECMD_FAILED;
-	}
 
-	if (vg && !vg_check_status(vg, CLUSTERED)) {
-		unlock_vg(cmd, vg_name);
+	/* FIXME We want to allow read-only VGs to be changed here? */
+	if (vg_read_error(vg) && vg_read_error(vg) != FAILED_READ_ONLY
+	    && !arg_count(cmd, removemissing_ARG))
 		return ECMD_FAILED;
-	}
 
 	if (repairing) {
-		if (vg && consistent && !vg_missing_pv_count(vg)) {
+		if (!vg_read_error(vg) && !vg_missing_pv_count(vg)) {
 			log_error("Volume group \"%s\" is already consistent",
 				  vg_name);
 			unlock_vg(cmd, vg_name);
 			return ECMD_PROCESSED;
 		}
 
-		consistent = !arg_count(cmd, force_ARG);
-		if (!(vg = vg_read_internal(cmd, vg_name, NULL, &consistent))) {
-			log_error("Volume group \"%s\" not found", vg_name);
-			unlock_vg(cmd, vg_name);
-			return ECMD_FAILED;
-		}
-		if (!vg_check_status(vg, CLUSTERED)) {
+		log_verbose("Trying to open VG %s for recovery...", vg_name);
+
+		vg = vg_read_for_update(cmd, vg_name, NULL,
+					ALLOW_INCONSISTENT | DISABLE_LOCK
+					| ALLOW_EXPORTED);
+
+		if (vg_read_error(vg) && vg_read_error(vg) != FAILED_READ_ONLY
+		    && vg_read_error(vg) != FAILED_INCONSISTENT) {
 			unlock_vg(cmd, vg_name);
 			return ECMD_FAILED;
 		}
+
 		if (!archive(vg)) {
 			unlock_vg(cmd, vg_name);
 			return ECMD_FAILED;

[PATCH 9/14] Convert vgrename to vg_read_for_update.

[Petr Rockai]

Fri Jan  9 15:46:59 CET 2009  Petr Rockai <me@mornfall.net>
  * Convert vgrename to vg_read_for_update.
diff -rN -u -p old-temp.4430/tools/vgrename.c new-temp.4430/tools/vgrename.c
--- old-temp.4430/tools/vgrename.c	2009-01-22 11:02:48.830781112 +0100
+++ new-temp.4430/tools/vgrename.c	2009-01-22 11:02:48.922779187 +0100
@@ -20,7 +20,6 @@ static int vg_rename_path(struct cmd_con
 {
 	char *dev_dir;
 	struct id id;
-	int consistent = 1;
 	int match = 0;
 	int found_id = 0;
 	struct dm_list *vgids;
@@ -70,25 +69,11 @@ static int vg_rename_path(struct cmd_con
 	} else
 		vgid = NULL;
 
-	if (!lock_vol(cmd, vg_name_old, LCK_VG_WRITE)) {
-		log_error("Can't get lock for %s", vg_name_old);
-		return 0;
-	}
-
-	if (!(vg = vg_read_internal(cmd, vg_name_old, vgid, &consistent)) || !consistent) {
-		log_error("Volume group %s %s%s%snot found.", vg_name_old,
-		vgid ? "(" : "", vgid ? vgid : "", vgid ? ") " : "");
-		unlock_vg(cmd, vg_name_old);
-		return 0;
-	}
-
-	if (!vg_check_status(vg, CLUSTERED | LVM_WRITE)) {
-		unlock_vg(cmd, vg_name_old);
-		return 0;
-	}
-
-	/* Don't return failure for EXPORTED_VG */
-	vg_check_status(vg, EXPORTED_VG);
+	/* FIXME we used to print an error about EXPORTED, but proceeded
+	   nevertheless. */
+	vg = vg_read_for_update(cmd, vg_name_old, vgid, ALLOW_EXPORTED);
+	if (vg_read_error(vg))
+		return_0;
 
 	if (lvs_in_vg_activated_by_uuid_only(vg)) {
 		unlock_vg(cmd, vg_name_old);
@@ -100,14 +85,13 @@ static int vg_rename_path(struct cmd_con
 
 	log_verbose("Checking for new volume group \"%s\"", vg_name_new);
 
-	if (!lock_vol(cmd, vg_name_new, LCK_VG_WRITE | LCK_NONBLOCK)) {
-		unlock_vg(cmd, vg_name_old);
-		log_error("Can't get lock for %s", vg_name_new);
-		return 0;
-	}
+	vg_new = vg_read_for_update(cmd, vg_name_new, NULL, KEEP_LOCK |
+				    EXISTENCE_CHECK | NONBLOCKING_LOCK);
+
+	if (vg_read_error(vg_new))
+	    goto error;
 
-	consistent = 0;
-	if ((vg_new = vg_read_internal(cmd, vg_name_new, NULL, &consistent))) {
+	if (vg_might_exist(vg_new)) {
 		log_error("New volume group \"%s\" already exists",
 			  vg_name_new);
 		goto error;

[PATCH 10/14] Convert vgsplit to use vg_read_for_update.

[Petr Rockai]

Thu Jan 15 11:02:12 CET 2009  Petr Rockai <me@mornfall.net>
  * Convert vgsplit to use vg_read_for_update.
diff -rN -u -p old-temp.4430/tools/vgsplit.c new-temp.4430/tools/vgsplit.c
--- old-temp.4430/tools/vgsplit.c	2009-01-22 11:02:50.994777933 +0100
+++ new-temp.4430/tools/vgsplit.c	2009-01-22 11:02:51.178781976 +0100
@@ -289,7 +289,6 @@ int vgsplit(struct cmd_context *cmd, int
 	struct volume_group *vg_to, *vg_from;
 	int opt;
 	int existing_vg;
-	int consistent;
 	const char *lv_name;
 
 	if ((arg_count(cmd, name_ARG) + argc) < 3) {
@@ -320,21 +319,20 @@ int vgsplit(struct cmd_context *cmd, int
 	}
 
 	log_verbose("Checking for volume group \"%s\"", vg_name_from);
-	if (!(vg_from = vg_lock_and_read(cmd, vg_name_from, NULL, LCK_VG_WRITE,
-				       CLUSTERED | EXPORTED_VG |
-				       RESIZEABLE_VG | LVM_WRITE,
-				       CORRECT_INCONSISTENT | FAIL_INCONSISTENT)))
-		 return ECMD_FAILED;
+	vg_from = vg_read_for_update(cmd, vg_name_from, NULL,
+				     REQUIRE_RESIZEABLE);
+	if (vg_read_error(vg_from))
+		return ECMD_FAILED;
 
 	log_verbose("Checking for new volume group \"%s\"", vg_name_to);
-	if (!lock_vol(cmd, vg_name_to, LCK_VG_WRITE | LCK_NONBLOCK)) {
-		log_error("Can't get lock for %s", vg_name_to);
-		unlock_vg(cmd, vg_name_from);
-		return ECMD_FAILED;
-	}
+	vg_to = vg_read_for_update(cmd, vg_name_to, NULL, REQUIRE_RESIZEABLE |
+				   NONBLOCKING_LOCK | KEEP_LOCK |
+				   EXISTENCE_CHECK);
+
+	if (vg_read_error(vg_to))
+		goto_bad;
 
-	consistent = 0;
-	if ((vg_to = vg_read_internal(cmd, vg_name_to, NULL, &consistent))) {
+	if (vg_might_exist(vg_to)) {
 		existing_vg = 1;
 		if (new_vg_option_specified(cmd)) {
 			log_error("Volume group \"%s\" exists, but new VG "
@@ -449,13 +447,14 @@ int vgsplit(struct cmd_context *cmd, int
 	/*
 	 * Finally, remove the EXPORTED flag from the new VG and write it out.
 	 */
-	consistent = 1;
-	if (!test_mode() &&
-	    (!(vg_to = vg_read_internal(cmd, vg_name_to, NULL, &consistent)) ||
-	     !consistent)) {
-		log_error("Volume group \"%s\" became inconsistent: please "
-			  "fix manually", vg_name_to);
-		goto_bad;
+	if (!test_mode()) {
+		vg_to = vg_read_for_update(cmd, vg_name_to, NULL,
+					   DISABLE_LOCK | ALLOW_EXPORTED);
+		if (vg_read_error(vg_to)) {
+			log_error("Volume group \"%s\" became inconsistent: "
+				  "please fix manually", vg_name_to);
+			goto_bad;
+		}
 	}
 
 	vg_to->status &= ~EXPORTED_VG;

[PATCH 11/14] Rework the toollib interface (process_each_*) on top of new vg_read.

[Petr Rockai]

Fri Jan  9 15:34:41 CET 2009  Petr Rockai <me@mornfall.net>
  * Rework the toollib interface (process_each_*) on top of new vg_read.
  
  Since this makes an incompatible change to the interface of process_each_*
  functions, all of these need to go in at once.
diff -rN -u -p old-temp.4430/lib/metadata/metadata.c new-temp.4430/lib/metadata/metadata.c
--- old-temp.4430/lib/metadata/metadata.c	2009-01-22 11:02:53.130776363 +0100
+++ new-temp.4430/lib/metadata/metadata.c	2009-01-22 11:02:53.178781416 +0100
@@ -332,7 +332,7 @@ static int remove_lvs_in_vg(struct cmd_c
 
 /* FIXME: remove redundant vg_name */
 int vg_remove_single(struct cmd_context *cmd, const char *vg_name,
-		     struct volume_group *vg, int consistent,
+		     struct volume_group *vg,
 		     force_t force __attribute((unused)))
 {
 	struct physical_volume *pv;
@@ -340,7 +340,7 @@ int vg_remove_single(struct cmd_context 
 	unsigned lv_count;
 	int ret = 1;
 
-	if (!vg || !consistent || vg_missing_pv_count(vg)) {
+	if (vg_read_error(vg) || vg_missing_pv_count(vg)) {
 		log_error("Volume group \"%s\" not found, is inconsistent "
 			  "or has PVs missing.", vg_name);
 		log_error("Consider vgreduce --removemissing if metadata "
diff -rN -u -p old-temp.4430/lib/metadata/metadata-exported.h new-temp.4430/lib/metadata/metadata-exported.h
--- old-temp.4430/lib/metadata/metadata-exported.h	2009-01-22 11:02:53.130776363 +0100
+++ new-temp.4430/lib/metadata/metadata-exported.h	2009-01-22 11:02:53.178781416 +0100
@@ -428,7 +428,7 @@ struct volume_group *vg_create(struct cm
 			       int pv_count, char **pv_names);
 int vg_remove(struct volume_group *vg);
 int vg_remove_single(struct cmd_context *cmd, const char *vg_name,
-		     struct volume_group *vg, int consistent,
+		     struct volume_group *vg,
 		     force_t force);
 int vg_rename(struct cmd_context *cmd, struct volume_group *vg,
 	      const char *new_name);
diff -rN -u -p old-temp.4430/tools/lvchange.c new-temp.4430/tools/lvchange.c
--- old-temp.4430/tools/lvchange.c	2009-01-22 11:02:53.130776363 +0100
+++ new-temp.4430/tools/lvchange.c	2009-01-22 11:02:53.218779866 +0100
@@ -728,6 +728,6 @@ int lvchange(struct cmd_context *cmd, in
 		return EINVALID_CMD_LINE;
 	}
 
-	return process_each_lv(cmd, argc, argv, LCK_VG_WRITE, NULL,
+	return process_each_lv(cmd, argc, argv, READ_FOR_UPDATE, NULL,
 			       &lvchange_single);
 }
diff -rN -u -p old-temp.4430/tools/lvdisplay.c new-temp.4430/tools/lvdisplay.c
--- old-temp.4430/tools/lvdisplay.c	2009-01-22 11:02:53.130776363 +0100
+++ new-temp.4430/tools/lvdisplay.c	2009-01-22 11:02:53.218779866 +0100
@@ -54,6 +54,6 @@ int lvdisplay(struct cmd_context *cmd, i
 		return EINVALID_CMD_LINE;
 	}
 
-	return process_each_lv(cmd, argc, argv, LCK_VG_READ, NULL,
+	return process_each_lv(cmd, argc, argv, 0, NULL,
 			       &_lvdisplay_single);
 }
diff -rN -u -p old-temp.4430/tools/lvremove.c new-temp.4430/tools/lvremove.c
--- old-temp.4430/tools/lvremove.c	2009-01-22 11:02:53.130776363 +0100
+++ new-temp.4430/tools/lvremove.c	2009-01-22 11:02:53.218779866 +0100
@@ -33,6 +33,6 @@ int lvremove(struct cmd_context *cmd, in
 
 	cmd->handles_missing_pvs = 1;
 
-	return process_each_lv(cmd, argc, argv, LCK_VG_WRITE, NULL,
+	return process_each_lv(cmd, argc, argv, READ_FOR_UPDATE, NULL,
 			       &lvremove_single);
 }
diff -rN -u -p old-temp.4430/tools/lvscan.c new-temp.4430/tools/lvscan.c
--- old-temp.4430/tools/lvscan.c	2009-01-22 11:02:53.130776363 +0100
+++ new-temp.4430/tools/lvscan.c	2009-01-22 11:02:53.218779866 +0100
@@ -80,6 +80,6 @@ int lvscan(struct cmd_context *cmd, int 
 		return EINVALID_CMD_LINE;
 	}
 
-	return process_each_lv(cmd, argc, argv, LCK_VG_READ, NULL,
+	return process_each_lv(cmd, argc, argv, 0, NULL,
 			       &lvscan_single);
 }
diff -rN -u -p old-temp.4430/tools/polldaemon.c new-temp.4430/tools/polldaemon.c
--- old-temp.4430/tools/polldaemon.c	2009-01-22 11:02:53.130776363 +0100
+++ new-temp.4430/tools/polldaemon.c	2009-01-22 11:02:53.222781073 +0100
@@ -175,7 +175,7 @@ static int _wait_for_single_mirror(struc
 }
 
 static int _poll_vg(struct cmd_context *cmd, const char *vgname,
-		    struct volume_group *vg, int consistent, void *handle)
+		    struct volume_group *vg, void *handle)
 {
 	struct daemon_parms *parms = (struct daemon_parms *) handle;
 	struct lv_list *lvl;
@@ -183,18 +183,7 @@ static int _poll_vg(struct cmd_context *
 	const char *name;
 	int finished;
 
-	if (!vg) {
-		log_error("Couldn't read volume group %s", vgname);
-		return ECMD_FAILED;
-	}
-
-	if (!consistent) {
-		log_error("Volume Group %s inconsistent - skipping", vgname);
-		/* FIXME Should we silently recover it here or not? */
-		return ECMD_FAILED;
-	}
-
-	if (!vg_check_status(vg, EXPORTED_VG))
+	if (vg_read_error(vg))
 		return ECMD_FAILED;
 
 	dm_list_iterate_items(lvl, &vg->lvs) {
@@ -219,7 +208,7 @@ static void _poll_for_all_vgs(struct cmd
 {
 	while (1) {
 		parms->outstanding_count = 0;
-		process_each_vg(cmd, 0, NULL, LCK_VG_WRITE, 1, parms, _poll_vg);
+		process_each_vg(cmd, 0, NULL, READ_FOR_UPDATE, parms, _poll_vg);
 		if (!parms->outstanding_count)
 			break;
 		sleep(parms->interval);
diff -rN -u -p old-temp.4430/tools/reporter.c new-temp.4430/tools/reporter.c
--- old-temp.4430/tools/reporter.c	2009-01-22 11:02:53.130776363 +0100
+++ new-temp.4430/tools/reporter.c	2009-01-22 11:02:53.222781073 +0100
@@ -18,12 +18,10 @@
 
 static int _vgs_single(struct cmd_context *cmd __attribute((unused)),
 		       const char *vg_name, struct volume_group *vg,
-		       int consistent __attribute((unused)), void *handle)
+		       void *handle)
 {
-	if (!vg) {
-		log_error("Volume group %s not found", vg_name);
+	if (vg_read_error(vg))
 		return ECMD_FAILED;
-	}
 
 	if (!report_object(handle, vg, NULL, NULL, NULL, NULL))
 		return ECMD_FAILED;
@@ -160,26 +158,20 @@ out:
 
 static int _pvs_in_vg(struct cmd_context *cmd, const char *vg_name,
 		      struct volume_group *vg,
-		      int consistent __attribute((unused)),
 		      void *handle)
 {
-	if (!vg) {
-		log_error("Volume group %s not found", vg_name);
+	if (vg_read_error(vg))
 		return ECMD_FAILED;
-	}
 
 	return process_each_pv_in_vg(cmd, vg, NULL, handle, &_pvs_single);
 }
 
 static int _pvsegs_in_vg(struct cmd_context *cmd, const char *vg_name,
 			 struct volume_group *vg,
-			 int consistent __attribute((unused)),
 			 void *handle)
 {
-	if (!vg) {
-		log_error("Volume group %s not found", vg_name);
+	if (vg_read_error(vg))
 		return ECMD_FAILED;
-	}
 
 	return process_each_pv_in_vg(cmd, vg, NULL, handle, &_pvsegs_single);
 }
@@ -352,23 +344,23 @@ static int _report(struct cmd_context *c
 
 	switch (report_type) {
 	case LVS:
-		r = process_each_lv(cmd, argc, argv, LCK_VG_READ, report_handle,
+		r = process_each_lv(cmd, argc, argv, 0, report_handle,
 				    &_lvs_single);
 		break;
 	case VGS:
-		r = process_each_vg(cmd, argc, argv, LCK_VG_READ, 0,
-				    report_handle, &_vgs_single);
+		r = process_each_vg(cmd, argc, argv, 0, report_handle,
+				    &_vgs_single);
 		break;
 	case PVS:
 		if (args_are_pvs)
 			r = process_each_pv(cmd, argc, argv, NULL, LCK_VG_READ,
 					    report_handle, &_pvs_single);
 		else
-			r = process_each_vg(cmd, argc, argv, LCK_VG_READ, 0,
+			r = process_each_vg(cmd, argc, argv, 0,
 					    report_handle, &_pvs_in_vg);
 		break;
 	case SEGS:
-		r = process_each_lv(cmd, argc, argv, LCK_VG_READ, report_handle,
+		r = process_each_lv(cmd, argc, argv, 0, report_handle,
 				    &_lvsegs_single);
 		break;
 	case PVSEGS:
@@ -376,7 +368,7 @@ static int _report(struct cmd_context *c
 			r = process_each_pv(cmd, argc, argv, NULL, LCK_VG_READ,
 					    report_handle, &_pvsegs_single);
 		else
-			r = process_each_vg(cmd, argc, argv, LCK_VG_READ, 0,
+			r = process_each_vg(cmd, argc, argv, 0,
 					    report_handle, &_pvsegs_in_vg);
 		break;
 	}
diff -rN -u -p old-temp.4430/tools/toollib.c new-temp.4430/tools/toollib.c
--- old-temp.4430/tools/toollib.c	2009-01-22 11:02:53.126778997 +0100
+++ new-temp.4430/tools/toollib.c	2009-01-22 11:02:53.222781073 +0100
@@ -161,7 +161,7 @@ int process_each_lv_in_vg(struct cmd_con
 }
 
 int process_each_lv(struct cmd_context *cmd, int argc, char **argv,
-		    uint32_t lock_type, void *handle,
+		    uint32_t flags, void *handle,
 		    int (*process_single) (struct cmd_context * cmd,
 					   struct logical_volume * lv,
 					   void *handle))
@@ -169,7 +169,6 @@ int process_each_lv(struct cmd_context *
 	int opt = 0;
 	int ret_max = ECMD_PROCESSED;
 	int ret = 0;
-	int consistent;
 
 	struct dm_list *tags_arg;
 	struct dm_list *vgnames;	/* VGs to process */
@@ -283,38 +282,10 @@ int process_each_lv(struct cmd_context *
 		vgname = strl->str;
 		if (is_orphan_vg(vgname))
 			continue;	/* FIXME Unnecessary? */
-		if (!lock_vol(cmd, vgname, lock_type)) {
-			log_error("Can't lock %s: skipping", vgname);
-			continue;
-		}
-		if (lock_type & LCK_WRITE)
-			consistent = 1;
-		else
-			consistent = 0;
-		if (!(vg = vg_read_internal(cmd, vgname, NULL, &consistent)) || !consistent) {
-			unlock_vg(cmd, vgname);
-			if (!vg)
-				log_error("Volume group \"%s\" "
-					  "not found", vgname);
-			else {
-				if (!vg_check_status(vg, CLUSTERED)) {
-					if (ret_max < ECMD_FAILED)
-						ret_max = ECMD_FAILED;
-					continue;
-				}
-				log_error("Volume group \"%s\" "
-					  "inconsistent", vgname);
-			}
 
-			if (!vg || !(vg = recover_vg(cmd, vgname, lock_type))) {
-				if (ret_max < ECMD_FAILED)
-					ret_max = ECMD_FAILED;
-				continue;
-			}
-		}
+		vg = vg_read(cmd, vgname, NULL, flags);
 
-		if (!vg_check_status(vg, CLUSTERED)) {
-			unlock_vg(cmd, vgname);
+		if (vg_read_error(vg)) {
 			if (ret_max < ECMD_FAILED)
 				ret_max = ECMD_FAILED;
 			continue;
@@ -371,8 +342,8 @@ int process_each_segment_in_pv(struct cm
 	if (!vg && !is_orphan(pv)) {
 		vg_name = pv_vg_name(pv);
 
-		if (!(vg = vg_lock_and_read(cmd, vg_name, NULL, LCK_VG_READ,
-					    CLUSTERED, 0))) {
+		vg = vg_read(cmd, vg_name, NULL, 0);
+		if (vg_read_error(vg)) {
 			log_error("Skipping volume group %s", vg_name);
 			return ECMD_FAILED;
 		}
@@ -417,32 +388,18 @@ int process_each_segment_in_lv(struct cm
 static int _process_one_vg(struct cmd_context *cmd, const char *vg_name,
 			   const char *vgid,
 			   struct dm_list *tags, struct dm_list *arg_vgnames,
-			   uint32_t lock_type, int consistent, void *handle,
+			   uint32_t flags, void *handle,
 			   int ret_max,
 			   int (*process_single) (struct cmd_context * cmd,
 						  const char *vg_name,
 						  struct volume_group * vg,
-						  int consistent, void *handle))
+						  void *handle))
 {
 	struct volume_group *vg;
 	int ret = 0;
 
-	if (!lock_vol(cmd, vg_name, lock_type)) {
-		log_error("Can't lock volume group %s: skipping", vg_name);
-		return ret_max;
-	}
-
 	log_verbose("Finding volume group \"%s\"", vg_name);
-	if (!(vg = vg_read_internal(cmd, vg_name, vgid, &consistent))) {
-		log_error("Volume group \"%s\" not found", vg_name);
-		unlock_vg(cmd, vg_name);
-		return ECMD_FAILED;
-	}
-
-	if (!vg_check_status(vg, CLUSTERED)) {
-		unlock_vg(cmd, vg_name);
-		return ECMD_FAILED;
-	}
+	vg = vg_read(cmd, vg_name, vgid, flags);
 
 	if (!dm_list_empty(tags)) {
 		/* Only process if a tag matches or it's on arg_vgnames */
@@ -453,7 +410,7 @@ static int _process_one_vg(struct cmd_co
 		}
 	}
 
-	if ((ret = process_single(cmd, vg_name, vg, consistent,
+	if ((ret = process_single(cmd, vg_name, vg,
 				  handle)) > ret_max) {
 		ret_max = ret;
 	}
@@ -464,11 +421,11 @@ static int _process_one_vg(struct cmd_co
 }
 
 int process_each_vg(struct cmd_context *cmd, int argc, char **argv,
-		    uint32_t lock_type, int consistent, void *handle,
+		    uint32_t flags, void *handle,
 		    int (*process_single) (struct cmd_context * cmd,
 					   const char *vg_name,
 					   struct volume_group * vg,
-					   int consistent, void *handle))
+					   void *handle))
 {
 	int opt = 0;
 	int ret_max = ECMD_PROCESSED;
@@ -531,7 +488,7 @@ int process_each_vg(struct cmd_context *
 				continue;
 			ret_max = _process_one_vg(cmd, vg_name, vgid, &tags,
 						  &arg_vgnames,
-					  	  lock_type, consistent, handle,
+					  	  flags, handle,
 					  	  ret_max, process_single);
 			if (sigint_caught())
 				return ret_max;
@@ -543,7 +500,7 @@ int process_each_vg(struct cmd_context *
 				continue;	/* FIXME Unnecessary? */
 			ret_max = _process_one_vg(cmd, vg_name, NULL, &tags,
 						  &arg_vgnames,
-					  	  lock_type, consistent, handle,
+					  	  flags, handle,
 					  	  ret_max, process_single);
 			if (sigint_caught())
 				return ret_max;
@@ -1122,30 +1079,6 @@ struct dm_list *clone_pv_list(struct dm_
 	return r;
 }
 
-/*
- * Attempt metadata recovery
- */
-struct volume_group *recover_vg(struct cmd_context *cmd, const char *vgname,
-				uint32_t lock_type)
-{
-	int consistent = 1;
-
-	/* Don't attempt automatic recovery without proper locking */
-	if (lockingfailed())
-		return NULL;
-
-	lock_type &= ~LCK_TYPE_MASK;
-	lock_type |= LCK_WRITE;
-
-	if (!lock_vol(cmd, vgname, lock_type)) {
-		log_error("Can't lock %s for metadata recovery: skipping",
-			  vgname);
-		return NULL;
-	}
-
-	return vg_read_internal(cmd, vgname, NULL, &consistent);
-}
-
 int apply_lvname_restrictions(const char *name)
 {
 	if (!strncmp(name, "snapshot", 8)) {
diff -rN -u -p old-temp.4430/tools/toollib.h new-temp.4430/tools/toollib.h
--- old-temp.4430/tools/toollib.h	2009-01-22 11:02:53.126778997 +0100
+++ new-temp.4430/tools/toollib.h	2009-01-22 11:02:53.222781073 +0100
@@ -27,11 +27,11 @@ struct volume_group *recover_vg(struct c
 				uint32_t lock_type);
 
 int process_each_vg(struct cmd_context *cmd, int argc, char **argv,
-		    uint32_t lock_type, int consistent, void *handle,
+		    uint32_t flags, void *handle,
 		    int (*process_single) (struct cmd_context * cmd,
 					   const char *vg_name,
 					   struct volume_group * vg,
-					   int consistent, void *handle));
+					   void *handle));
 
 int process_each_pv(struct cmd_context *cmd, int argc, char **argv,
 		    struct volume_group *vg, uint32_t lock_type, void *handle,
@@ -49,7 +49,7 @@ int process_each_segment_in_pv(struct cm
 						      void *handle));
 
 int process_each_lv(struct cmd_context *cmd, int argc, char **argv,
-		    uint32_t lock_type, void *handle,
+		    uint32_t flags, void *handle,
 		    int (*process_single) (struct cmd_context * cmd,
 					   struct logical_volume * lv,
 					   void *handle));
diff -rN -u -p old-temp.4430/tools/vgcfgbackup.c new-temp.4430/tools/vgcfgbackup.c
--- old-temp.4430/tools/vgcfgbackup.c	2009-01-22 11:02:53.122780235 +0100
+++ new-temp.4430/tools/vgcfgbackup.c	2009-01-22 11:02:53.222781073 +0100
@@ -48,19 +48,14 @@ static char *_expand_filename(const char
 }
 
 static int vg_backup_single(struct cmd_context *cmd, const char *vg_name,
-			    struct volume_group *vg, int consistent,
+			    struct volume_group *vg,
 			    void *handle)
 {
 	char **last_filename = (char **)handle;
 	char *filename;
 
-	if (!vg) {
-		log_error("Volume group \"%s\" not found", vg_name);
+	if (vg_read_error(vg) && !vg_read_error(vg) == FAILED_INCONSISTENT)
 		return ECMD_FAILED;
-	}
-
-	if (!consistent)
-		log_error("WARNING: Volume group \"%s\" inconsistent", vg_name);
 
 	if (arg_count(cmd, file_ARG)) {
 		if (!(filename = _expand_filename(arg_value(cmd, file_ARG),
@@ -72,7 +67,7 @@ static int vg_backup_single(struct cmd_c
 		if (!backup_to_file(filename, vg->cmd->cmd_line, vg))
 			return ECMD_FAILED;
 	} else {
-		if (!consistent) {
+		if (vg_read_error(vg) == FAILED_INCONSISTENT) {
 			log_error("No backup taken: specify filename with -f "
 				  "to backup an inconsistent VG");
 			stack;
@@ -98,7 +93,7 @@ int vgcfgbackup(struct cmd_context *cmd,
 
 	init_pvmove(1);
 
-	ret = process_each_vg(cmd, argc, argv, LCK_VG_READ, 0, &last_filename,
+	ret = process_each_vg(cmd, argc, argv, 0, &last_filename,
 			      &vg_backup_single);
 
 	dm_free(last_filename);
diff -rN -u -p old-temp.4430/tools/vgchange.c new-temp.4430/tools/vgchange.c
--- old-temp.4430/tools/vgchange.c	2009-01-22 11:02:53.122780235 +0100
+++ new-temp.4430/tools/vgchange.c	2009-01-22 11:02:53.222781073 +0100
@@ -521,28 +521,13 @@ static int _vgchange_refresh(struct cmd_
 }
 
 static int vgchange_single(struct cmd_context *cmd, const char *vg_name,
-			   struct volume_group *vg, int consistent,
+			   struct volume_group *vg,
 			   void *handle __attribute((unused)))
 {
 	int r = ECMD_FAILED;
 
-	if (!vg) {
-		log_error("Unable to find volume group \"%s\"", vg_name);
+	if (vg_read_error(vg))
 		return ECMD_FAILED;
-	}
-
-	if (!consistent) {
-		unlock_vg(cmd, vg_name);
-		dev_close_all();
-		log_error("Volume group \"%s\" inconsistent", vg_name);
-		if (!(vg = recover_vg(cmd, vg_name, LCK_VG_WRITE)))
-			return ECMD_FAILED;
-	}
-
-	if (!(vg_status(vg) & LVM_WRITE) && !arg_count(cmd, available_ARG)) {
-		log_error("Volume group \"%s\" is read-only", vg->name);
-		return ECMD_FAILED;
-	}
 
 	if (vg_status(vg) & EXPORTED_VG) {
 		log_error("Volume group \"%s\" is exported", vg_name);
@@ -633,6 +618,6 @@ int vgchange(struct cmd_context *cmd, in
 
 	return process_each_vg(cmd, argc, argv,
 			       (arg_count(cmd, available_ARG)) ?
-			       LCK_VG_READ : LCK_VG_WRITE, 0, NULL,
+			       0 : READ_FOR_UPDATE, NULL,
 			       &vgchange_single);
 }
diff -rN -u -p old-temp.4430/tools/vgck.c new-temp.4430/tools/vgck.c
--- old-temp.4430/tools/vgck.c	2009-01-22 11:02:53.122780235 +0100
+++ new-temp.4430/tools/vgck.c	2009-01-22 11:02:53.222781073 +0100
@@ -17,27 +17,14 @@
 
 static int vgck_single(struct cmd_context *cmd __attribute((unused)),
 		       const char *vg_name,
-		       struct volume_group *vg, int consistent,
+		       struct volume_group *vg,
 		       void *handle __attribute((unused)))
 {
-	if (!vg) {
-		log_error("Volume group \"%s\" not found", vg_name);
-		return ECMD_FAILED;
-	}
-
-	if (!consistent) {
-		log_error("Volume group \"%s\" inconsistent", vg_name);
-		return ECMD_FAILED;
-	}
-
-	if (!vg_check_status(vg, EXPORTED_VG))
-		return ECMD_FAILED;
-
 	return ECMD_PROCESSED;
 }
 
 int vgck(struct cmd_context *cmd, int argc, char **argv)
 {
-	return process_each_vg(cmd, argc, argv, LCK_VG_READ, 0, NULL,
+	return process_each_vg(cmd, argc, argv, 0, NULL,
 			       &vgck_single);
 }
diff -rN -u -p old-temp.4430/tools/vgconvert.c new-temp.4430/tools/vgconvert.c
--- old-temp.4430/tools/vgconvert.c	2009-01-22 11:02:53.122780235 +0100
+++ new-temp.4430/tools/vgconvert.c	2009-01-22 11:02:53.222781073 +0100
@@ -16,7 +16,7 @@
 #include "tools.h"
 
 static int vgconvert_single(struct cmd_context *cmd, const char *vg_name,
-			    struct volume_group *vg, int consistent,
+			    struct volume_group *vg,
 			    void *handle __attribute((unused)))
 {
 	struct physical_volume *pv, *existing_pv;
@@ -32,20 +32,7 @@ static int vgconvert_single(struct cmd_c
 	struct lvinfo info;
 	int active = 0;
 
-	if (!vg) {
-		log_error("Unable to find volume group \"%s\"", vg_name);
-		return ECMD_FAILED;
-	}
-
-	if (!consistent) {
-		unlock_vg(cmd, vg_name);
-		dev_close_all();
-		log_error("Volume group \"%s\" inconsistent", vg_name);
-		if (!(vg = recover_vg(cmd, vg_name, LCK_VG_WRITE)))
-			return ECMD_FAILED;
-	}
-
-	if (!vg_check_status(vg, LVM_WRITE | EXPORTED_VG))
+	if (vg_read_error(vg))
 		return ECMD_FAILED;
 
 	if (vg->fid->fmt == cmd->fmt) {
@@ -233,6 +220,6 @@ int vgconvert(struct cmd_context *cmd, i
 		return EINVALID_CMD_LINE;
 	}
 
-	return process_each_vg(cmd, argc, argv, LCK_VG_WRITE, 0, NULL,
+	return process_each_vg(cmd, argc, argv, READ_FOR_UPDATE, NULL,
 			       &vgconvert_single);
 }
diff -rN -u -p old-temp.4430/tools/vgdisplay.c new-temp.4430/tools/vgdisplay.c
--- old-temp.4430/tools/vgdisplay.c	2009-01-22 11:02:53.122780235 +0100
+++ new-temp.4430/tools/vgdisplay.c	2009-01-22 11:02:53.222781073 +0100
@@ -16,17 +16,12 @@
 #include "tools.h"
 
 static int vgdisplay_single(struct cmd_context *cmd, const char *vg_name,
-			    struct volume_group *vg, int consistent,
+			    struct volume_group *vg,
 			    void *handle __attribute((unused)))
 {
 	/* FIXME Do the active check here if activevolumegroups_ARG ? */
-	if (!vg) {
-		log_error("Volume group \"%s\" doesn't exist", vg_name);
+	if (vg_read_error(vg))
 		return ECMD_FAILED;
-	}
-
-	if (!consistent)
-		log_error("WARNING: Volume group \"%s\" inconsistent", vg_name);
 
 	vg_check_status(vg, EXPORTED_VG);
 
@@ -98,8 +93,7 @@ int vgdisplay(struct cmd_context *cmd, i
 	}
 **********/
 
-	return process_each_vg(cmd, argc, argv, LCK_VG_READ, 0, NULL,
-			       vgdisplay_single);
+	return process_each_vg(cmd, argc, argv, 0, NULL, vgdisplay_single);
 
 /******** FIXME Need to count number processed
 	  Add this to process_each_vg if arg_count(cmd,activevolumegroups_ARG) ?
diff -rN -u -p old-temp.4430/tools/vgexport.c new-temp.4430/tools/vgexport.c
--- old-temp.4430/tools/vgexport.c	2009-01-22 11:02:53.122780235 +0100
+++ new-temp.4430/tools/vgexport.c	2009-01-22 11:02:53.222781073 +0100
@@ -17,25 +17,14 @@
 
 static int vgexport_single(struct cmd_context *cmd __attribute((unused)),
 			   const char *vg_name,
-			   struct volume_group *vg, int consistent,
+			   struct volume_group *vg,
 			   void *handle __attribute((unused)))
 {
 	struct pv_list *pvl;
 	struct physical_volume *pv;
 
-	if (!vg) {
-		log_error("Unable to find volume group \"%s\"", vg_name);
+	if (vg_read_error(vg))
 		goto error;
-	}
-
-	if (!consistent) {
-		log_error("Volume group %s inconsistent", vg_name);
-		goto error;
-	}
-
-	if (!vg_check_status(vg, EXPORTED_VG | LVM_WRITE)) {
-		goto error;
-	}
 
 	if (lvs_in_vg_activated(vg)) {
 		log_error("Volume group \"%s\" has active logical volumes",
@@ -78,6 +67,6 @@ int vgexport(struct cmd_context *cmd, in
 		return ECMD_FAILED;
 	}
 
-	return process_each_vg(cmd, argc, argv, LCK_VG_WRITE, 1, NULL,
+	return process_each_vg(cmd, argc, argv, READ_FOR_UPDATE, NULL,
 			       &vgexport_single);
 }
diff -rN -u -p old-temp.4430/tools/vgimport.c new-temp.4430/tools/vgimport.c
--- old-temp.4430/tools/vgimport.c	2009-01-22 11:02:53.122780235 +0100
+++ new-temp.4430/tools/vgimport.c	2009-01-22 11:02:53.222781073 +0100
@@ -17,17 +17,14 @@
 
 static int vgimport_single(struct cmd_context *cmd __attribute((unused)),
 			   const char *vg_name,
-			   struct volume_group *vg, int consistent,
+			   struct volume_group *vg,
 			   void *handle __attribute((unused)))
 {
 	struct pv_list *pvl;
 	struct physical_volume *pv;
 
-	if (!vg || !consistent) {
-		log_error("Unable to find exported volume group \"%s\"",
-			  vg_name);
+	if (vg_read_error(vg))
 		goto error;
-	}
 
 	if (!(vg_status(vg) & EXPORTED_VG)) {
 		log_error("Volume group \"%s\" is not exported", vg_name);
@@ -74,6 +71,7 @@ int vgimport(struct cmd_context *cmd, in
 		return ECMD_FAILED;
 	}
 
-	return process_each_vg(cmd, argc, argv, LCK_VG_WRITE, 1, NULL,
+	return process_each_vg(cmd, argc, argv,
+			       READ_FOR_UPDATE | ALLOW_EXPORTED, NULL,
 			       &vgimport_single);
 }
diff -rN -u -p old-temp.4430/tools/vgremove.c new-temp.4430/tools/vgremove.c
--- old-temp.4430/tools/vgremove.c	2009-01-22 11:02:53.118780145 +0100
+++ new-temp.4430/tools/vgremove.c	2009-01-22 11:02:53.222781073 +0100
@@ -16,10 +16,10 @@
 #include "tools.h"
 
 static int vgremove_single(struct cmd_context *cmd, const char *vg_name,
-			   struct volume_group *vg, int consistent,
+			   struct volume_group *vg,
 			   void *handle __attribute((unused)))
 {
-	if (!vg_remove_single(cmd, vg_name, vg, consistent,
+	if (!vg_remove_single(cmd, vg_name, vg,
 			      arg_count(cmd, force_ARG)))
 		return ECMD_FAILED;
 
@@ -41,7 +41,7 @@ int vgremove(struct cmd_context *cmd, in
 	}
 
 	ret = process_each_vg(cmd, argc, argv,
-			      LCK_VG_WRITE | LCK_NONBLOCK, 1,
+			      READ_FOR_UPDATE | NONBLOCKING_LOCK,
 			      NULL, &vgremove_single);
 
 	unlock_vg(cmd, VG_ORPHANS);
diff -rN -u -p old-temp.4430/tools/vgscan.c new-temp.4430/tools/vgscan.c
--- old-temp.4430/tools/vgscan.c	2009-01-22 11:02:53.118780145 +0100
+++ new-temp.4430/tools/vgscan.c	2009-01-22 11:02:53.222781073 +0100
@@ -16,22 +16,11 @@
 #include "tools.h"
 
 static int vgscan_single(struct cmd_context *cmd, const char *vg_name,
-			 struct volume_group *vg, int consistent,
+			 struct volume_group *vg,
 			 void *handle __attribute((unused)))
 {
-	if (!vg) {
-		log_error("Volume group \"%s\" not found", vg_name);
+	if (vg_read_error(vg))
 		return ECMD_FAILED;
-	}
-
-	if (!consistent) {
-		unlock_vg(cmd, vg_name);
-		dev_close_all();
-		log_error("Volume group \"%s\" inconsistent", vg_name);
-		/* Don't allow partial switch to this program */
-		if (!(vg = recover_vg(cmd, vg_name, LCK_VG_WRITE)))
-			return ECMD_FAILED;
-	}
 
 	log_print("Found %svolume group \"%s\" using metadata type %s",
 		  (vg_status(vg) & EXPORTED_VG) ? "exported " : "", vg_name,
@@ -61,8 +50,7 @@ int vgscan(struct cmd_context *cmd, int 
 
 	log_print("Reading all physical volumes.  This may take a while...");
 
-	maxret = process_each_vg(cmd, argc, argv, LCK_VG_READ, 0, NULL,
-				 &vgscan_single);
+	maxret = process_each_vg(cmd, argc, argv, 0, NULL, &vgscan_single);
 
 	if (arg_count(cmd, mknodes_ARG)) {
 		ret = vgmknodes(cmd, argc, argv);

[PATCH 12/14] Port process_each_pv to new vg_read.

[Petr Rockai]

Wed Jan 21 15:52:44 CET 2009  Petr Rockai <me@mornfall.net>
  * Port process_each_pv to new vg_read.
diff -rN -u -p old-temp.4430/tools/pvdisplay.c new-temp.4430/tools/pvdisplay.c
--- old-temp.4430/tools/pvdisplay.c	2009-01-22 11:02:54.986780827 +0100
+++ new-temp.4430/tools/pvdisplay.c	2009-01-22 11:02:55.074781398 +0100
@@ -110,6 +110,6 @@ int pvdisplay(struct cmd_context *cmd, i
 		return EINVALID_CMD_LINE;
 	}
 
-	return process_each_pv(cmd, argc, argv, NULL, LCK_VG_READ, NULL,
+	return process_each_pv(cmd, argc, argv, NULL, 0, NULL,
 			       _pvdisplay_single);
 }
diff -rN -u -p old-temp.4430/tools/pvresize.c new-temp.4430/tools/pvresize.c
--- old-temp.4430/tools/pvresize.c	2009-01-22 11:02:54.986780827 +0100
+++ new-temp.4430/tools/pvresize.c	2009-01-22 11:02:55.074781398 +0100
@@ -213,7 +213,7 @@ int pvresize(struct cmd_context *cmd, in
 	params.done = 0;
 	params.total = 0;
 
-	ret = process_each_pv(cmd, argc, argv, NULL, LCK_VG_WRITE, &params,
+	ret = process_each_pv(cmd, argc, argv, NULL, READ_FOR_UPDATE, &params,
 			      _pvresize_single);
 
 	log_print("%d physical volume(s) resized / %d physical volume(s) "
diff -rN -u -p old-temp.4430/tools/reporter.c new-temp.4430/tools/reporter.c
--- old-temp.4430/tools/reporter.c	2009-01-22 11:02:54.986780827 +0100
+++ new-temp.4430/tools/reporter.c	2009-01-22 11:02:55.074781398 +0100
@@ -353,7 +353,7 @@ static int _report(struct cmd_context *c
 		break;
 	case PVS:
 		if (args_are_pvs)
-			r = process_each_pv(cmd, argc, argv, NULL, LCK_VG_READ,
+			r = process_each_pv(cmd, argc, argv, NULL, 0,
 					    report_handle, &_pvs_single);
 		else
 			r = process_each_vg(cmd, argc, argv, 0,
@@ -365,7 +365,7 @@ static int _report(struct cmd_context *c
 		break;
 	case PVSEGS:
 		if (args_are_pvs)
-			r = process_each_pv(cmd, argc, argv, NULL, LCK_VG_READ,
+			r = process_each_pv(cmd, argc, argv, NULL, 0,
 					    report_handle, &_pvsegs_single);
 		else
 			r = process_each_vg(cmd, argc, argv, 0,
diff -rN -u -p old-temp.4430/tools/toollib.c new-temp.4430/tools/toollib.c
--- old-temp.4430/tools/toollib.c	2009-01-22 11:02:54.986780827 +0100
+++ new-temp.4430/tools/toollib.c	2009-01-22 11:02:55.074781398 +0100
@@ -578,7 +578,7 @@ static int _process_all_devs(struct cmd_
 }
 
 int process_each_pv(struct cmd_context *cmd, int argc, char **argv,
-		    struct volume_group *vg, uint32_t lock_type, void *handle,
+		    struct volume_group *vg, uint32_t flags, void *handle,
 		    int (*process_single) (struct cmd_context * cmd,
 					   struct volume_group * vg,
 					   struct physical_volume * pv,
@@ -673,25 +673,11 @@ int process_each_pv(struct cmd_context *
 		if (!dm_list_empty(&tags) && (vgnames = get_vgs(cmd, 0)) &&
 			   !dm_list_empty(vgnames)) {
 			dm_list_iterate_items(sll, vgnames) {
-				if (!lock_vol(cmd, sll->str, lock_type)) {
-					log_error("Can't lock %s: skipping", sll->str);
-					continue;
-				}
-				if (!(vg = vg_read_internal(cmd, sll->str, NULL, &consistent))) {
-					log_error("Volume group \"%s\" not found", sll->str);
-					unlock_vg(cmd, sll->str);
+				vg = vg_read(cmd, sll->str, NULL, flags);
+				if (vg_read_error(vg)) {
 					ret_max = ECMD_FAILED;
 					continue;
 				}
-				if (!consistent) {
-					unlock_vg(cmd, sll->str);
-					continue;
-				}
-
-				if (!vg_check_status(vg, CLUSTERED)) {
-					unlock_vg(cmd, sll->str);
-					continue;
-				}
 
 				ret = process_each_pv_in_vg(cmd, vg, &tags,
 							    handle,
diff -rN -u -p old-temp.4430/tools/vgreduce.c new-temp.4430/tools/vgreduce.c
--- old-temp.4430/tools/vgreduce.c	2009-01-22 11:02:54.986780827 +0100
+++ new-temp.4430/tools/vgreduce.c	2009-01-22 11:02:55.078780998 +0100
@@ -570,7 +570,7 @@ int vgreduce(struct cmd_context *cmd, in
 
 		/* FIXME: Pass private struct through to all these functions */
 		/* and update in batch here? */
-		ret = process_each_pv(cmd, argc, argv, vg, LCK_NONE, NULL,
+		ret = process_each_pv(cmd, argc, argv, vg, DISABLE_LOCK, NULL,
 				      _vgreduce_single);
 
 	}

[PATCH 13/14] Remove now-unused vg_lock_and_read.

[Petr Rockai]

Thu Jan 15 10:52:08 CET 2009  Petr Rockai <me@mornfall.net>
  * Remove now-unused vg_lock_and_read.
diff -rN -u -p old-temp.4430/lib/metadata/metadata.c new-temp.4430/lib/metadata/metadata.c
--- old-temp.4430/lib/metadata/metadata.c	2009-01-22 11:02:56.842777680 +0100
+++ new-temp.4430/lib/metadata/metadata.c	2009-01-22 11:02:56.890780708 +0100
@@ -2424,50 +2424,6 @@ int vg_check_status(const struct volume_
 }
 
 /*
- * vg_lock_and_read - consolidate vg locking, reading, and status flag checking
- *
- * Returns:
- * NULL - failure
- * non-NULL - success; volume group handle
- */
-vg_t *vg_lock_and_read(struct cmd_context *cmd, const char *vg_name,
-		       const char *vgid,
-		       uint32_t lock_flags, uint32_t status_flags,
-		       uint32_t misc_flags)
-{
-	struct volume_group *vg;
-	int consistent = 1;
-
-	if (!(misc_flags & CORRECT_INCONSISTENT))
-		consistent = 0;
-
-	if (!validate_name(vg_name)) {
-		log_error("Volume group name %s has invalid characters",
-			  vg_name);
-		return NULL;
-	}
-
-	if (!lock_vol(cmd, vg_name, lock_flags)) {
-		log_error("Can't get lock for %s", vg_name);
-		return NULL;
-	}
-
-	if (!(vg = vg_read_internal(cmd, vg_name, vgid, &consistent)) ||
-	    ((misc_flags & FAIL_INCONSISTENT) && !consistent)) {
-		log_error("Volume group \"%s\" not found", vg_name);
-		unlock_vg(cmd, vg_name);
-		return NULL;
-	}
-
-	if (!vg_check_status(vg, status_flags)) {
-		unlock_vg(cmd, vg_name);
-		return NULL;
-	}
-
-	return vg;
-}
-
-/*
  * Create a (vg_t) volume group handle from a struct volume_group pointer and a
  * possible failure code. Zero failure code = success.
  */
diff -rN -u -p old-temp.4430/lib/metadata/metadata-exported.h new-temp.4430/lib/metadata/metadata-exported.h
--- old-temp.4430/lib/metadata/metadata-exported.h	2009-01-22 11:02:56.842777680 +0100
+++ new-temp.4430/lib/metadata/metadata-exported.h	2009-01-22 11:02:56.890780708 +0100
@@ -95,10 +95,6 @@ struct pv_segment;
 #define FMT_UNLIMITED_STRIPESIZE 0x00000100U	/* Unlimited stripe size? */
 #define FMT_RESTRICTED_READAHEAD 0x00000200U	/* Readahead restricted to 2-120? */
 
-/* LVM2 external library flags */
-#define CORRECT_INCONSISTENT    0x00000001U /* Correct inconsistent metadata */
-#define FAIL_INCONSISTENT       0x00000002U /* Fail if metadata inconsistent */
-
 /* Mirror conversion type flags */
 #define MIRROR_BY_SEG		0x00000001U	/* segment-by-segment mirror */
 #define MIRROR_BY_LV		0x00000002U	/* mirror using whole mimage LVs */

[PATCH 14/14] Un-export vg_read_internal.

[Petr Rockai]

Thu Jan 22 10:56:49 CET 2009  Petr Rockai <me@mornfall.net>
  * Un-export vg_read_internal.
diff -rN -u -p old-temp.4430/lib/metadata/metadata.c new-temp.4430/lib/metadata/metadata.c
--- old-temp.4430/lib/metadata/metadata.c	2009-01-22 11:02:58.730781531 +0100
+++ new-temp.4430/lib/metadata/metadata.c	2009-01-22 11:02:58.778780229 +0100
@@ -31,6 +31,9 @@
 
 #include <sys/param.h>
 
+static struct volume_group *_vg_read_internal(struct cmd_context *cmd,
+					      const char *vgname,
+					      const char *vgid, int *consistent);
 /*
  * FIXME: Check for valid handle before dereferencing field or log error?
  */
@@ -224,8 +227,8 @@ int get_pv_from_vg_by_id(const struct fo
 	struct pv_list *pvl;
 	int consistent = 0;
 
-	if (!(vg = vg_read_internal(fmt->cmd, vg_name, vgid, &consistent))) {
-		log_error("get_pv_from_vg_by_id: vg_read_internal failed to read VG %s",
+	if (!(vg = _vg_read_internal(fmt->cmd, vg_name, vgid, &consistent))) {
+		log_error("get_pv_from_vg_by_id: _vg_read_internal failed to read VG %s",
 			  vg_name);
 		return 0;
 	}
@@ -503,7 +506,7 @@ struct volume_group *vg_create(struct cm
 		return_NULL;
 
 	/* is this vg name already in use ? */
-	if (vg_read_internal(cmd, vg_name, NULL, &consistent)) {
+	if (_vg_read_internal(cmd, vg_name, NULL, &consistent)) {
 		log_err("A volume group called '%s' already exists.", vg_name);
 		goto bad;
 	}
@@ -1716,7 +1719,7 @@ static struct volume_group *_vg_read(str
 
 	if (is_orphan_vg(vgname)) {
 		if (use_precommitted) {
-			log_error("Internal error: vg_read_internal requires vgname "
+			log_error("Internal error: _vg_read_internal requires vgname "
 				  "with pre-commit.");
 			return NULL;
 		}
@@ -1974,8 +1977,9 @@ static struct volume_group *_vg_read(str
 	return correct_vg;
 }
 
-struct volume_group *vg_read_internal(struct cmd_context *cmd, const char *vgname,
-			     const char *vgid, int *consistent)
+static struct volume_group *_vg_read_internal(struct cmd_context *cmd,
+					      const char *vgname,
+					      const char *vgid, int *consistent)
 {
 	struct volume_group *vg;
 	struct lv_list *lvl;
@@ -2002,7 +2006,7 @@ struct volume_group *vg_read_internal(st
 
 /* This is only called by lv_from_lvid, which is only called from
  * activate.c so we know the appropriate VG lock is already held and
- * the vg_read_internal is therefore safe.
+ * the _vg_read_internal is therefore safe.
  */
 static struct volume_group *_vg_read_by_vgid(struct cmd_context *cmd,
 					    const char *vgid,
@@ -2034,7 +2038,7 @@ static struct volume_group *_vg_read_by_
 	if (memlock())
 		return NULL;
 
-	/* FIXME Need a genuine read by ID here - don't vg_read_internal by name! */
+	/* FIXME Need a genuine read by ID here - don't _vg_read_internal by name! */
 	/* FIXME Disabled vgrenames while active for now because we aren't
 	 *       allowed to do a full scan here any more. */
 
@@ -2218,7 +2222,7 @@ static int _get_pvs(struct cmd_context *
 			stack;
 			continue;
 		}
-		if (!(vg = vg_read_internal(cmd, vgname, vgid, &consistent))) {
+		if (!(vg = _vg_read_internal(cmd, vgname, vgid, &consistent))) {
 			stack;
 			continue;
 		}
@@ -2456,7 +2460,7 @@ static vg_t *_recover_vg(struct cmd_cont
 	if (!lock_vol(cmd, lock, lock_flags))
 		return_NULL;
 
-	if (!(vg = vg_read_internal(cmd, vg_name, vgid, &consistent)))
+	if (!(vg = _vg_read_internal(cmd, vg_name, vgid, &consistent)))
 		return_NULL;
 	if (!consistent)
 		return_NULL;
@@ -2515,7 +2519,7 @@ static vg_t *_vg_lock_and_read(struct cm
 	consistent_in = consistent;
 
 	/* If consistent == 1, we get NULL here if correction fails. */
-	if (!(vg = vg_read_internal(cmd, vg_name, vgid, &consistent))) {
+	if (!(vg = _vg_read_internal(cmd, vg_name, vgid, &consistent))) {
 		if (consistent_in && !consistent) {
 			log_error("Volume group \"%s\" inconsistent.", vg_name);
 			failure |= FAILED_INCONSISTENT;
diff -rN -u -p old-temp.4430/lib/metadata/metadata-exported.h new-temp.4430/lib/metadata/metadata-exported.h
--- old-temp.4430/lib/metadata/metadata-exported.h	2009-01-22 11:02:58.730781531 +0100
+++ new-temp.4430/lib/metadata/metadata-exported.h	2009-01-22 11:02:58.778780229 +0100
@@ -364,8 +364,6 @@ struct lv_list {
 int vg_write(struct volume_group *vg);
 int vg_commit(struct volume_group *vg);
 int vg_revert(struct volume_group *vg);
-struct volume_group *vg_read_internal(struct cmd_context *cmd, const char *vg_name,
-			     const char *vgid, int *consistent);
 struct physical_volume *pv_read(struct cmd_context *cmd, const char *pv_name,
 				struct dm_list *mdas, uint64_t *label_sector,
 				int warnings);

[PATCH 14/14] Un-export vg_read_internal.

[Dave Wysochanski]

On Thu, 2009-01-22 at 11:10 +0100, Petr Rockai wrote:
> Thu Jan 22 10:56:49 CET 2009  Petr Rockai <me@mornfall.net>
>   * Un-export vg_read_internal.
> diff -rN -u -p old-temp.4430/lib/metadata/metadata.c new-temp.4430/lib/metadata/metadata.c
> --- old-temp.4430/lib/metadata/metadata.c	2009-01-22 11:02:58.730781531 +0100
> +++ new-temp.4430/lib/metadata/metadata.c	2009-01-22 11:02:58.778780229 +0100
> @@ -31,6 +31,9 @@
>  
>  #include <sys/param.h>
>  
> +static struct volume_group *_vg_read_internal(struct cmd_context *cmd,
> +					      const char *vgname,
> +					      const char *vgid, int *consistent);
>  /*
>   * FIXME: Check for valid handle before dereferencing field or log error?
>   */
> @@ -224,8 +227,8 @@ int get_pv_from_vg_by_id(const struct fo
>  	struct pv_list *pvl;
>  	int consistent = 0;
>  
> -	if (!(vg = vg_read_internal(fmt->cmd, vg_name, vgid, &consistent))) {
> -		log_error("get_pv_from_vg_by_id: vg_read_internal failed to read VG %s",
> +	if (!(vg = _vg_read_internal(fmt->cmd, vg_name, vgid, &consistent))) {
> +		log_error("get_pv_from_vg_by_id: _vg_read_internal failed to read VG %s",
>  			  vg_name);
>  		return 0;
>  	}
> @@ -503,7 +506,7 @@ struct volume_group *vg_create(struct cm
>  		return_NULL;
>  
>  	/* is this vg name already in use ? */
> -	if (vg_read_internal(cmd, vg_name, NULL, &consistent)) {
> +	if (_vg_read_internal(cmd, vg_name, NULL, &consistent)) {
>  		log_err("A volume group called '%s' already exists.", vg_name);
>  		goto bad;
>  	}
> @@ -1716,7 +1719,7 @@ static struct volume_group *_vg_read(str
>  
>  	if (is_orphan_vg(vgname)) {
>  		if (use_precommitted) {
> -			log_error("Internal error: vg_read_internal requires vgname "
> +			log_error("Internal error: _vg_read_internal requires vgname "
>  				  "with pre-commit.");
>  			return NULL;
>  		}
> @@ -1974,8 +1977,9 @@ static struct volume_group *_vg_read(str
>  	return correct_vg;
>  }
>  
> -struct volume_group *vg_read_internal(struct cmd_context *cmd, const char *vgname,
> -			     const char *vgid, int *consistent)
> +static struct volume_group *_vg_read_internal(struct cmd_context *cmd,
> +					      const char *vgname,
> +					      const char *vgid, int *consistent)
>  {
>  	struct volume_group *vg;
>  	struct lv_list *lvl;
> @@ -2002,7 +2006,7 @@ struct volume_group *vg_read_internal(st
>  
>  /* This is only called by lv_from_lvid, which is only called from
>   * activate.c so we know the appropriate VG lock is already held and
> - * the vg_read_internal is therefore safe.
> + * the _vg_read_internal is therefore safe.
>   */
>  static struct volume_group *_vg_read_by_vgid(struct cmd_context *cmd,
>  					    const char *vgid,
> @@ -2034,7 +2038,7 @@ static struct volume_group *_vg_read_by_
>  	if (memlock())
>  		return NULL;
>  
> -	/* FIXME Need a genuine read by ID here - don't vg_read_internal by name! */
> +	/* FIXME Need a genuine read by ID here - don't _vg_read_internal by name! */
>  	/* FIXME Disabled vgrenames while active for now because we aren't
>  	 *       allowed to do a full scan here any more. */
>  
> @@ -2218,7 +2222,7 @@ static int _get_pvs(struct cmd_context *
>  			stack;
>  			continue;
>  		}
> -		if (!(vg = vg_read_internal(cmd, vgname, vgid, &consistent))) {
> +		if (!(vg = _vg_read_internal(cmd, vgname, vgid, &consistent))) {
>  			stack;
>  			continue;
>  		}
> @@ -2456,7 +2460,7 @@ static vg_t *_recover_vg(struct cmd_cont
>  	if (!lock_vol(cmd, lock, lock_flags))
>  		return_NULL;
>  
> -	if (!(vg = vg_read_internal(cmd, vg_name, vgid, &consistent)))
> +	if (!(vg = _vg_read_internal(cmd, vg_name, vgid, &consistent)))
>  		return_NULL;
>  	if (!consistent)
>  		return_NULL;
> @@ -2515,7 +2519,7 @@ static vg_t *_vg_lock_and_read(struct cm
>  	consistent_in = consistent;
>  
>  	/* If consistent == 1, we get NULL here if correction fails. */
> -	if (!(vg = vg_read_internal(cmd, vg_name, vgid, &consistent))) {
> +	if (!(vg = _vg_read_internal(cmd, vg_name, vgid, &consistent))) {
>  		if (consistent_in && !consistent) {
>  			log_error("Volume group \"%s\" inconsistent.", vg_name);
>  			failure |= FAILED_INCONSISTENT;
> diff -rN -u -p old-temp.4430/lib/metadata/metadata-exported.h new-temp.4430/lib/metadata/metadata-exported.h
> --- old-temp.4430/lib/metadata/metadata-exported.h	2009-01-22 11:02:58.730781531 +0100
> +++ new-temp.4430/lib/metadata/metadata-exported.h	2009-01-22 11:02:58.778780229 +0100
> @@ -364,8 +364,6 @@ struct lv_list {
>  int vg_write(struct volume_group *vg);
>  int vg_commit(struct volume_group *vg);
>  int vg_revert(struct volume_group *vg);
> -struct volume_group *vg_read_internal(struct cmd_context *cmd, const char *vg_name,
> -			     const char *vgid, int *consistent);
>  struct physical_volume *pv_read(struct cmd_context *cmd, const char *pv_name,
>  				struct dm_list *mdas, uint64_t *label_sector,
>  				int warnings);
> 


You can't do this patch until you fix clvmd lvm-functions.c - still
calls vg_read_internal().

[PATCH 12/14] Port process_each_pv to new vg_read.

[Dave Wysochanski]

On Thu, 2009-01-22 at 11:10 +0100, Petr Rockai wrote:
>  
>  int process_each_pv(struct cmd_context *cmd, int argc, char **argv,
> -		    struct volume_group *vg, uint32_t lock_type, void *handle,
> +		    struct volume_group *vg, uint32_t flags, void *handle,
>  		    int (*process_single) (struct cmd_context * cmd,
>  					   struct volume_group * vg,
>  					   struct physical_volume * pv,
> @@ -673,25 +673,11 @@ int process_each_pv(struct cmd_context *
>  		if (!dm_list_empty(&tags) && (vgnames = get_vgs(cmd, 0)) &&
>  			   !dm_list_empty(vgnames)) {
>  			dm_list_iterate_items(sll, vgnames) {
> -				if (!lock_vol(cmd, sll->str, lock_type)) {
> -					log_error("Can't lock %s: skipping", sll->str);
> -					continue;
> -				}
> -				if (!(vg = vg_read_internal(cmd, sll->str, NULL, &consistent))) {
> -					log_error("Volume group \"%s\" not found", sll->str);
> -					unlock_vg(cmd, sll->str);
> +				vg = vg_read(cmd, sll->str, NULL, flags);
> +				if (vg_read_error(vg)) {
>  					ret_max = ECMD_FAILED;
>  					continue;
>  				}
> -				if (!consistent) {
> -					unlock_vg(cmd, sll->str);
> -					continue;
> -				}
> -
> -				if (!vg_check_status(vg, CLUSTERED)) {
> -					unlock_vg(cmd, sll->str);
> -					continue;
> -				}
>  
>  				ret = process_each_pv_in_vg(cmd, vg, &tags,
>  							    handle,

'consistent' declaration can be removed (saw it when I compiled).

Not critical to the patch though here's an updated patch with this one
line removed.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Port-process_each_pv-to-new-vg_read.patch
Type: application/mbox
Size: 4475 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/lvm-devel/attachments/20090122/c23defaa/attachment.mbox>

[PATCH updated] Convert vgreduce to use vg_read_for_update.

[Dave Wysochanski]

One line removed from original patch:
Remove incorrect unlock_vg() in vgreduce_single() error path.
If dev_get_size() fails we don't have the orphan lock yet so
don't try to unlock.

[PATCH] Convert vgreduce to use vg_read_for_update.

[Dave Wysochanski]

From: Petr Rockai <prockai@redhat.com>

Fri Jan  9 15:38:20 CET 2009  Petr Rockai <me@mornfall.net>
  * Convert vgreduce to use vg_read_for_update.

Remove incorrect unlock_vg() in vgreduce_single() error path.
If dev_get_size() fails we don't have the orphan lock yet so
don't try to unlock.

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 tools/vgreduce.c |   57 +++++++++++++++++++----------------------------------
 1 files changed, 21 insertions(+), 36 deletions(-)

diff --git a/tools/vgreduce.c b/tools/vgreduce.c
index 79bf3e0..09bbd09 100644
--- a/tools/vgreduce.c
+++ b/tools/vgreduce.c
@@ -382,7 +382,6 @@ static int _vgreduce_single(struct cmd_context *cmd, struct volume_group *vg,
 {
 	struct pv_list *pvl;
 	struct volume_group *orphan_vg;
-	int consistent = 1;
 	const char *name = pv_dev_name(pv);
 
 	if (pv_pe_alloc_count(pv)) {
@@ -396,17 +395,10 @@ static int _vgreduce_single(struct cmd_context *cmd, struct volume_group *vg,
 		return ECMD_FAILED;
 	}
 
-	if (!lock_vol(cmd, VG_ORPHANS, LCK_VG_WRITE | LCK_NONBLOCK)) {
-		log_error("Can't get lock for orphan PVs");
-		return ECMD_FAILED;
-	}
-
 	pvl = find_pv_in_vg(vg, name);
 
-	if (!archive(vg)) {
-		unlock_vg(cmd, VG_ORPHANS);
+	if (!archive(vg))
 		return ECMD_FAILED;
-	}
 
 	log_verbose("Removing \"%s\" from volume group \"%s\"", name, vg->name);
 
@@ -418,7 +410,6 @@ static int _vgreduce_single(struct cmd_context *cmd, struct volume_group *vg,
 
 	if (!dev_get_size(pv_dev(pv), &pv->size)) {
 		log_error("%s: Couldn't get size.", pv_dev_name(pv));
-		unlock_vg(cmd, VG_ORPHANS);
 		return ECMD_FAILED;
 	}
 
@@ -426,12 +417,11 @@ static int _vgreduce_single(struct cmd_context *cmd, struct volume_group *vg,
 	vg->free_count -= pv_pe_count(pv) - pv_pe_alloc_count(pv);
 	vg->extent_count -= pv_pe_count(pv);
 
-	if(!(orphan_vg = vg_read_internal(cmd, vg->fid->fmt->orphan_vg_name, NULL, &consistent)) ||
-	   !consistent) {
-		log_error("Unable to read existing orphan PVs");
-		unlock_vg(cmd, VG_ORPHANS);
+	orphan_vg = vg_read_for_update(cmd, vg->fid->fmt->orphan_vg_name,
+				       NULL, NONBLOCKING_LOCK | ORPHAN_LOCK);
+
+	if (vg_read_error(orphan_vg))
 		return ECMD_FAILED;
-	}
 
 	if (!vg_split_mdas(cmd, vg, orphan_vg) || !vg->pv_count) {
 		log_error("Cannot remove final metadata area on \"%s\" from \"%s\"",
@@ -468,7 +458,6 @@ int vgreduce(struct cmd_context *cmd, int argc, char **argv)
 	struct volume_group *vg;
 	char *vg_name;
 	int ret = 1;
-	int consistent = 1;
 	int fixed = 1;
 	int repairing = arg_count(cmd, removemissing_ARG);
 
@@ -515,41 +504,37 @@ int vgreduce(struct cmd_context *cmd, int argc, char **argv)
 	}
 
 	log_verbose("Finding volume group \"%s\"", vg_name);
-	if (!lock_vol(cmd, vg_name, LCK_VG_WRITE)) {
-		log_error("Can't get lock for %s", vg_name);
-		return ECMD_FAILED;
-	}
 
-	if ((!(vg = vg_read_internal(cmd, vg_name, NULL, &consistent)) || !consistent)
-	    && !repairing) {
-		log_error("Volume group \"%s\" doesn't exist", vg_name);
-		unlock_vg(cmd, vg_name);
+	vg = vg_read_for_update(cmd, vg_name, NULL, ALLOW_EXPORTED);
+	if (vg_read_error(vg) == FAILED_ALLOCATION ||
+	    vg_read_error(vg) == FAILED_NOTFOUND)
 		return ECMD_FAILED;
-	}
 
-	if (vg && !vg_check_status(vg, CLUSTERED)) {
-		unlock_vg(cmd, vg_name);
+	/* FIXME We want to allow read-only VGs to be changed here? */
+	if (vg_read_error(vg) && vg_read_error(vg) != FAILED_READ_ONLY
+	    && !arg_count(cmd, removemissing_ARG))
 		return ECMD_FAILED;
-	}
 
 	if (repairing) {
-		if (vg && consistent && !vg_missing_pv_count(vg)) {
+		if (!vg_read_error(vg) && !vg_missing_pv_count(vg)) {
 			log_error("Volume group \"%s\" is already consistent",
 				  vg_name);
 			unlock_vg(cmd, vg_name);
 			return ECMD_PROCESSED;
 		}
 
-		consistent = !arg_count(cmd, force_ARG);
-		if (!(vg = vg_read_internal(cmd, vg_name, NULL, &consistent))) {
-			log_error("Volume group \"%s\" not found", vg_name);
-			unlock_vg(cmd, vg_name);
-			return ECMD_FAILED;
-		}
-		if (!vg_check_status(vg, CLUSTERED)) {
+		log_verbose("Trying to open VG %s for recovery...", vg_name);
+
+		vg = vg_read_for_update(cmd, vg_name, NULL,
+					ALLOW_INCONSISTENT | DISABLE_LOCK
+					| ALLOW_EXPORTED);
+
+		if (vg_read_error(vg) && vg_read_error(vg) != FAILED_READ_ONLY
+		    && vg_read_error(vg) != FAILED_INCONSISTENT) {
 			unlock_vg(cmd, vg_name);
 			return ECMD_FAILED;
 		}
+
 		if (!archive(vg)) {
 			unlock_vg(cmd, vg_name);
 			return ECMD_FAILED;
-- 
1.5.5.1

[PATCH update2] Convert vgreduce to use vg_read_for_update.

[Dave Wysochanski]

Fix unlock_vg() in error paths.

[PATCH] Convert vgreduce to use vg_read_for_update.

[Dave Wysochanski]

From: Petr Rockai <prockai@redhat.com>

Fri Jan  9 15:38:20 CET 2009  Petr Rockai <me@mornfall.net>
  * Convert vgreduce to use vg_read_for_update.

Remove incorrect unlock_vg() in vgreduce_single() error path.
If dev_get_size() fails we don't have the orphan lock yet so
don't try to unlock.

Add unlock_vg() after vg_read_for_update() error path.
If vg_read_error() fails we need to unlock the orphan vg.

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 tools/vgreduce.c |   55 ++++++++++++++++++++---------------------------------
 1 files changed, 21 insertions(+), 34 deletions(-)

diff --git a/tools/vgreduce.c b/tools/vgreduce.c
index 79bf3e0..3742785 100644
--- a/tools/vgreduce.c
+++ b/tools/vgreduce.c
@@ -382,7 +382,6 @@ static int _vgreduce_single(struct cmd_context *cmd, struct volume_group *vg,
 {
 	struct pv_list *pvl;
 	struct volume_group *orphan_vg;
-	int consistent = 1;
 	const char *name = pv_dev_name(pv);
 
 	if (pv_pe_alloc_count(pv)) {
@@ -396,17 +395,10 @@ static int _vgreduce_single(struct cmd_context *cmd, struct volume_group *vg,
 		return ECMD_FAILED;
 	}
 
-	if (!lock_vol(cmd, VG_ORPHANS, LCK_VG_WRITE | LCK_NONBLOCK)) {
-		log_error("Can't get lock for orphan PVs");
-		return ECMD_FAILED;
-	}
-
 	pvl = find_pv_in_vg(vg, name);
 
-	if (!archive(vg)) {
-		unlock_vg(cmd, VG_ORPHANS);
+	if (!archive(vg))
 		return ECMD_FAILED;
-	}
 
 	log_verbose("Removing \"%s\" from volume group \"%s\"", name, vg->name);
 
@@ -418,7 +410,6 @@ static int _vgreduce_single(struct cmd_context *cmd, struct volume_group *vg,
 
 	if (!dev_get_size(pv_dev(pv), &pv->size)) {
 		log_error("%s: Couldn't get size.", pv_dev_name(pv));
-		unlock_vg(cmd, VG_ORPHANS);
 		return ECMD_FAILED;
 	}
 
@@ -426,9 +417,10 @@ static int _vgreduce_single(struct cmd_context *cmd, struct volume_group *vg,
 	vg->free_count -= pv_pe_count(pv) - pv_pe_alloc_count(pv);
 	vg->extent_count -= pv_pe_count(pv);
 
-	if(!(orphan_vg = vg_read_internal(cmd, vg->fid->fmt->orphan_vg_name, NULL, &consistent)) ||
-	   !consistent) {
-		log_error("Unable to read existing orphan PVs");
+	orphan_vg = vg_read_for_update(cmd, vg->fid->fmt->orphan_vg_name,
+				       NULL, NONBLOCKING_LOCK | ORPHAN_LOCK);
+
+	if (vg_read_error(orphan_vg)) {
 		unlock_vg(cmd, VG_ORPHANS);
 		return ECMD_FAILED;
 	}
@@ -468,7 +460,6 @@ int vgreduce(struct cmd_context *cmd, int argc, char **argv)
 	struct volume_group *vg;
 	char *vg_name;
 	int ret = 1;
-	int consistent = 1;
 	int fixed = 1;
 	int repairing = arg_count(cmd, removemissing_ARG);
 
@@ -515,41 +506,37 @@ int vgreduce(struct cmd_context *cmd, int argc, char **argv)
 	}
 
 	log_verbose("Finding volume group \"%s\"", vg_name);
-	if (!lock_vol(cmd, vg_name, LCK_VG_WRITE)) {
-		log_error("Can't get lock for %s", vg_name);
-		return ECMD_FAILED;
-	}
 
-	if ((!(vg = vg_read_internal(cmd, vg_name, NULL, &consistent)) || !consistent)
-	    && !repairing) {
-		log_error("Volume group \"%s\" doesn't exist", vg_name);
-		unlock_vg(cmd, vg_name);
+	vg = vg_read_for_update(cmd, vg_name, NULL, ALLOW_EXPORTED);
+	if (vg_read_error(vg) == FAILED_ALLOCATION ||
+	    vg_read_error(vg) == FAILED_NOTFOUND)
 		return ECMD_FAILED;
-	}
 
-	if (vg && !vg_check_status(vg, CLUSTERED)) {
-		unlock_vg(cmd, vg_name);
+	/* FIXME We want to allow read-only VGs to be changed here? */
+	if (vg_read_error(vg) && vg_read_error(vg) != FAILED_READ_ONLY
+	    && !arg_count(cmd, removemissing_ARG))
 		return ECMD_FAILED;
-	}
 
 	if (repairing) {
-		if (vg && consistent && !vg_missing_pv_count(vg)) {
+		if (!vg_read_error(vg) && !vg_missing_pv_count(vg)) {
 			log_error("Volume group \"%s\" is already consistent",
 				  vg_name);
 			unlock_vg(cmd, vg_name);
 			return ECMD_PROCESSED;
 		}
 
-		consistent = !arg_count(cmd, force_ARG);
-		if (!(vg = vg_read_internal(cmd, vg_name, NULL, &consistent))) {
-			log_error("Volume group \"%s\" not found", vg_name);
-			unlock_vg(cmd, vg_name);
-			return ECMD_FAILED;
-		}
-		if (!vg_check_status(vg, CLUSTERED)) {
+		log_verbose("Trying to open VG %s for recovery...", vg_name);
+
+		vg = vg_read_for_update(cmd, vg_name, NULL,
+					ALLOW_INCONSISTENT | DISABLE_LOCK
+					| ALLOW_EXPORTED);
+
+		if (vg_read_error(vg) && vg_read_error(vg) != FAILED_READ_ONLY
+		    && vg_read_error(vg) != FAILED_INCONSISTENT) {
 			unlock_vg(cmd, vg_name);
 			return ECMD_FAILED;
 		}
+
 		if (!archive(vg)) {
 			unlock_vg(cmd, vg_name);
 			return ECMD_FAILED;
-- 
1.5.5.1

[PATCH 3/14] Properly enforce cluster locking in _vg_lock_and_read.

[Dave Wysochanski]

On Thu, 2009-01-22 at 11:09 +0100, Petr Rockai wrote:
> Wed Jan 21 15:52:36 CET 2009  Petr Rockai <me@mornfall.net>
>   * Properly enforce cluster locking in _vg_lock_and_read.
> diff -rN -u -p old-temp.4430/lib/metadata/metadata.c new-temp.4430/lib/metadata/metadata.c
> --- old-temp.4430/lib/metadata/metadata.c	2009-01-22 11:02:36.870777955 +0100
> +++ new-temp.4430/lib/metadata/metadata.c	2009-01-22 11:02:36.918780355 +0100
> @@ -2595,6 +2595,12 @@ static vg_t *_vg_lock_and_read(struct cm
>  		goto_bad;
>  	}
>  
> +	if (vg_is_clustered(vg) && !locking_is_clustered()) {
> +		log_error("Skipping clustered volume group %s", vg->name);
> +		failure |= FAILED_CLUSTERED;
> +		goto_bad;
> +	}
> +
>  	/* consistent == 0 when VG is not found, but failed == FAILED_NOTFOUND */
>  	if (!consistent && !failure) {
>  		if (!(vg = _recover_vg(cmd, lock, vg_name, vgid, lock_flags))) {
> @@ -2605,7 +2611,7 @@ static vg_t *_vg_lock_and_read(struct cm
>  		}
>  	}
>  
> -	failure |= _vg_check_status(vg, status_flags);
> +	failure |= _vg_check_status(vg, status_flags & ~CLUSTERED);
>  	if (failure)
>  		goto_bad;
>  
> 


With this patch, is there any point in leaving that CLUSTERED flag check
in _vg_check_status()?  The clustered flag set in vg_read() is not
needed either (check is mandatory now - flag doesn't mean anything).

[PATCH 3/14] Properly enforce cluster locking in _vg_lock_and_read.

[Petr Rockai]

Hi,

Dave Wysochanski <dwysocha@redhat.com> writes:
> With this patch, is there any point in leaving that CLUSTERED flag check
> in _vg_check_status()?  The clustered flag set in vg_read() is not
> needed either (check is mandatory now - flag doesn't mean anything).
actually, it seems you are right. I left it in because there are calls to
vg_check_status scattered through the code -- I didn't realize that it is now
redundant. However, there are still users of that check that are removed by
subsequent patches in the series. So to keep things working without requiring
all of the series to be applied, it should stay there. I'll remove it in an
extra patch on top of the stack.

Yours,
   Petr.

-- 
Peter Rockai | me()mornfall!net | prockai()redhat!com
 http://blog.mornfall.net | http://web.mornfall.net

"In My Egotistical Opinion, most people's C programs should be
 indented six feet downward and covered with dirt."
     -- Blair P. Houghton on the subject of C program indentation

[PATCH 2/14] Provide _vg_lock_and_read, a new implementation of lock+vg_read_internal.

[Dave Wysochanski]

On Thu, 2009-01-22 at 11:09 +0100, Petr Rockai wrote:
> Thu Jan 22 10:36:41 CET 2009  Petr Rockai <me@mornfall.net>
>   * Provide _vg_lock_and_read, a new implementation of lock+vg_read_internal.
>   
>   This is an internal function, to be later used by vg_read and
>   vg_read_for_update. Not to be confused with the public function
>   vg_lock_and_read, which will be removed later.
> diff -rN -u -p old-temp.4430/lib/metadata/metadata.c new-temp.4430/lib/metadata/metadata.c
> --- old-temp.4430/lib/metadata/metadata.c	2009-01-22 11:02:34.894778074 +0100
> +++ new-temp.4430/lib/metadata/metadata.c	2009-01-22 11:02:34.942780194 +0100
> @@ -2377,7 +2377,37 @@ int pv_analyze(struct cmd_context *cmd, 
>  	return 1;
>  }
>  
> +static uint32_t _vg_check_status(const struct volume_group *vg, uint32_t status)
> +{
> +	uint32_t failure = 0;
>  
> +	if ((status & CLUSTERED) &&
> +	    (vg_is_clustered(vg)) && !locking_is_clustered() &&
> +	    !lockingfailed()) {
> +		log_error("Skipping clustered volume group %s", vg->name);
> +		return FAILED_CLUSTERED;
> +	}
> +
> +	if ((status & EXPORTED_VG) &&
> +	    (vg->status & EXPORTED_VG)) {
> +		log_error("Volume group %s is exported", vg->name);
> +		failure |= FAILED_EXPORTED;
> +	}
> +
> +	if ((status & LVM_WRITE) &&
> +	    !(vg->status & LVM_WRITE)) {
> +		log_error("Volume group %s is read-only", vg->name);
> +		failure |= FAILED_READ_ONLY;
> +	}
> +
> +	if ((status & RESIZEABLE_VG) &&
> +	    !(vg->status & RESIZEABLE_VG)) {
> +		log_error("Volume group %s is not resizeable.", vg->name);
> +		failure |= FAILED_RESIZEABLE;
> +	}
> +
> +	return failure;
> +}
>  
>  /**
>   * vg_check_status - check volume group status flags and log error
> @@ -2462,6 +2492,133 @@ vg_t *vg_lock_and_read(struct cmd_contex
>  }
>  
>  /*
> + * Create a (vg_t) volume group handle from a struct volume_group pointer and a
> + * possible failure code. Zero failure code = success.
> + */
> +static vg_t *_vg_make_handle(struct cmd_context *cmd,
> +			     struct volume_group *vg,
> +			     uint32_t failure)
> +{
> +	if (!vg && !(vg = dm_pool_zalloc(cmd->mem, sizeof(*vg)))) {
> +		log_error("Error allocating vg handle .");
> +		return_NULL;
> +	}
> +	vg->read_failed = failure;
> +	return (vg_t *) vg;
> +}
> +
> +static vg_t *_recover_vg(struct cmd_context *cmd, const char *lock,
> +			 const char *vg_name, const char *vgid,
> +			 uint32_t lock_flags)
> +{
> +	int consistent = 1;
> +	struct volume_group *vg;
> +
> +	lock_flags &= ~LCK_TYPE_MASK;
> +	lock_flags |= LCK_WRITE;
> +
> +	unlock_vg(cmd, lock);
> +
> +	dev_close_all();
> +
> +	if (!lock_vol(cmd, lock, lock_flags))
> +		return_NULL;
> +
> +	if (!(vg = vg_read_internal(cmd, vg_name, vgid, &consistent)))
> +		return_NULL;
> +	if (!consistent)
> +		return_NULL;
> +
> +	return (vg_t *)vg;
> +}
> +
> +/*
> + * _vg_lock_and_read - consolidate vg locking, reading, and status flag checking
> + * This is an internal function.
> + *
> + * misc_flags:
> + *  ALLOW_INCONSISTENT: disable autocorrection
> + *
> + * Setting ALLOW_INCONSISTENT might give you inconsistent metadata. You will
> + * *still* get FAILED_INCONSISTENT in case the metadata has *really* been
> + * inconsistent. However, you still get the latest version of metadata in VG.
> + *
> + * Returns:
> + * Use vg_read_error(vg) to determine the result. Nonzero vg_read_error(vg)
> + * means there were problems reading the volume group.
> + * Zero value means that the VG is open and appropriate locks are held.
> + */
> +static vg_t *_vg_lock_and_read(struct cmd_context *cmd, const char *vg_name,
> +			       const char *vgid,
> +			       uint32_t lock_flags, uint32_t status_flags,
> +			       uint32_t misc_flags)
> +{
> +	struct volume_group *vg = 0;
> +	const char *lock;
> + 	int consistent = 1;
> +	int consistent_in;
> +	uint32_t failure = 0;
> +
> +	if (misc_flags & ALLOW_INCONSISTENT || !(lock_flags & LCK_WRITE))
> +		consistent = 0;
> +
> +	if (!validate_name(vg_name) && !is_orphan_vg(vg_name)) {
> +		log_error("Volume group name %s has invalid characters",
> +			  vg_name);
> +		return NULL;
> +	}
> +
> +	lock = (misc_flags & ORPHAN_LOCK ? VG_ORPHANS : vg_name);
> +	if (!(misc_flags & DISABLE_LOCK) && !lock_vol(cmd, lock, lock_flags)) {
> +		log_error("Can't get lock for %s", vg_name);
> +		return _vg_make_handle(cmd, vg, FAILED_LOCKING);
> +	}

The above code doesn't work if vgid is given and vgname is NULL.
Name validation will fail - easy to fix.  But then the lock_vol fails.
I'm going to hack away at this and see how far I get.  We should
probably drop vgid from vg_read() and vg_read_for_update() for now
though.


> +
> +	if (misc_flags & ORPHAN_LOCK)
> +		status_flags &= ~LVM_WRITE;
> +
> +	if (misc_flags & EXISTENCE_CHECK)
> +		consistent = 0;
> +
> +	consistent_in = consistent;
> +
> +	/* If consistent == 1, we get NULL here if correction fails. */
> +	if (!(vg = vg_read_internal(cmd, vg_name, vgid, &consistent))) {
> +		if (consistent_in && !consistent) {
> +			log_error("Volume group \"%s\" inconsistent.", vg_name);
> +			failure |= FAILED_INCONSISTENT;
> +			goto_bad;
> +		}
> +		if (!(misc_flags & EXISTENCE_CHECK))
> +			log_error("Volume group \"%s\" not found", vg_name);
> +		failure |= FAILED_NOTFOUND | (misc_flags & EXISTENCE_CHECK);
> +		goto_bad;
> +	}
> +
> +	/* consistent == 0 when VG is not found, but failed == FAILED_NOTFOUND */
> +	if (!consistent && !failure) {
> +		if (!(vg = _recover_vg(cmd, lock, vg_name, vgid, lock_flags))) {
> +			log_error("Recovery of volume group \"%s\" failed.",
> +				  vg_name);
> +			failure |= FAILED_INCONSISTENT;
> +			goto_bad;
> +		}
> +	}
> +
> +	failure |= _vg_check_status(vg, status_flags);
> +	if (failure)
> +		goto_bad;
> +
> +	return _vg_make_handle(cmd, vg, failure);
> +
> + bad:
> +	if (failure != (FAILED_NOTFOUND | EXISTENCE_CHECK) &&
> +	    !(misc_flags & KEEP_LOCK) && !(misc_flags & DISABLE_LOCK))
> +			unlock_vg(cmd, lock);
> +	return _vg_make_handle(cmd, vg, failure);
> +}
> +
> +/*
>   * Gets/Sets for external LVM library
>   */
>  struct id pv_id(const pv_t *pv)
> diff -rN -u -p old-temp.4430/lib/metadata/metadata-exported.h new-temp.4430/lib/metadata/metadata-exported.h
> --- old-temp.4430/lib/metadata/metadata-exported.h	2009-01-22 11:02:34.898782074 +0100
> +++ new-temp.4430/lib/metadata/metadata-exported.h	2009-01-22 11:02:34.942780194 +0100
> @@ -104,6 +104,43 @@ struct pv_segment;
>  #define MIRROR_BY_LV		0x00000002U	/* mirror using whole mimage LVs */
>  #define MIRROR_SKIP_INIT_SYNC	0x00000010U	/* skip initial sync */
>  
> +/* vg_read and vg_read_for_update flags */
> +#define ALLOW_INCONSISTENT 0x1
> +#define ALLOW_EXPORTED     0x2
> +#define REQUIRE_RESIZEABLE 0x4
> +#define EXISTENCE_CHECK    0x8
> +
> +/* FIXME these should be removed when we get better internal locking logic that
> +   does not rely on caller to provide locking details. */
> +#define NONBLOCKING_LOCK 0x100 /* Give up instead of waiting for a lock;
> +                                  required when acquiring a second VG lock. */
> +#define KEEP_LOCK        0x200 /* Do not unlock upon read failure. */
> +
> +/* FIXME the following flags should be removed in favour of better APIs
> +   (vg_reread and vg_read_orphan?) */
> +#define DISABLE_LOCK     0x400 /* Disable locking altogether. DANGEROUS. Only
> +				  use when you already hold appropriate lock
> +				  for this VG. */
> +#define ORPHAN_LOCK      0x800 /* Use when you vg_read(_for_update) the orphan
> +				  VG. */
> +
> +/* A meta-flag, useful with toollib for_each_* functions. */
> +#define READ_FOR_UPDATE 0x1000
> +
> +/* vg's "read_failed" field */
> +#define FAILED_INCONSISTENT 0x1
> +#define FAILED_LOCKING      0x2
> +#define FAILED_NOTFOUND     0x4
> +/* #define EXISTENCE_CHECK  0x8 -- left out for EXISTENCE_CHECK above, as it can
> +   be or'd into failure flags */
> +
> +#define FAILED_READ_ONLY   0x10
> +#define FAILED_EXPORTED    0x20
> +#define FAILED_RESIZEABLE  0x40
> +#define FAILED_CLUSTERED   0x80
> +
> +#define FAILED_ALLOCATION 0x100
> +
>  /* Ordered list - see lv_manip.c */
>  typedef enum {
>  	ALLOC_INVALID,
> @@ -196,6 +233,9 @@ struct volume_group {
>  	char *name;
>  	char *system_id;
>  
> +	uint32_t read_failed; /* 0 = no, everything OK. Otherwise uses FAILED_*
> +				 flags as defined above. */
> +
>  	uint32_t status;
>  	alloc_policy_t alloc;
>  
> 
> --
> lvm-devel mailing list
> lvm-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/lvm-devel

[PATCH] Another take on vg_read.

[Dave Wysochanski]

I went through the list of earlier comments.  Here's what I found that
seems to not have been addressed:


Agk's earlier comments still unaddressed:
1. PATCH6 (old patch#4/11). vg_read_error().
use a typedef for the return value of vg_read_error(), or explain what the
uint32_t means 
https://www.redhat.com/archives/lvm-devel/2009-January/msg00039.html

2. PATCH2. flag naming/definition.
Use a common prefix so they get grouped together.  Line up the 0x0000 parts
vertically making their "single bit" nature more obvious.
https://www.redhat.com/archives/lvm-devel/2009-January/msg00036.html
 
3. PATCH2. EXISTENCE_CHECK definition.
Ambiguous meaning - which is it?  1. self-evident (delete).  2. reserved for
future (say 'reserved').
https://www.redhat.com/archives/lvm-devel/2009-January/msg00036.html

4. PATCH2. _vg_make_handle
naming and possible 'failure' typedef
https://www.redhat.com/archives/lvm-devel/2009-January/msg00037.html

5. PATCH2. _recover_vg()
[note to check call paths later if more than one VG lock is held simultaneously]
https://www.redhat.com/archives/lvm-devel/2009-January/msg00037.html

6. PATCH2. Flag definition.
Some flags (ALLOW_INCONSISTENT) still need comment definitions.
https://www.redhat.com/archives/lvm-devel/2009-January/msg00037.html

7. PATCH2. read_failed member
Let's add a brief comment explaining this one.
read_failed ? Yes or No.  Rename it perhaps?
I think it should be completely separate from 'status' and 'alloc'.
https://www.redhat.com/archives/lvm-devel/2009-January/msg00037.html

8. PATCH2 (old patch#3/11) _vg_check_status
typedef for the return value?
https://www.redhat.com/archives/lvm-devel/2009-January/msg00038.html


Future cleanup:
1. FAILED_READ_ONLY
Note only FAILED_READ_ONLY is used by any tool today - only vgreduce.
Further, FAILED_READ_ONLY can only occur with LVM1 metadata as far as I
can tell.

2. PATCH2. _vg_lock_and_read failure path with invalid name returns NULL.
Note that callling vg_read_error() after hitting this path will return
FAILED_ALLOCATION.  Do we need FAILED_INVALID_NAME or perhaps re-use
FAILED_NOT_FOUND?
https://www.redhat.com/archives/lvm-devel/2009-January/msg00040.html


On Thu, 2009-01-22 at 11:09 +0100, Petr Rockai wrote:
> Hi,
> 
> I have done some more work on this series. I am resending everything, to avoid
> sequencing problems like I introduced last time with a partial send.
> 
> Anyway, some news:
> - the vg_check_status CLUSTERED fallthrough is fixed to return immediately
> - cluster locking is now properly enforced by vg_read/vg_read_for_update
> - process_each_pv is also ported over to new vg_read
> - vg_read_internal is now private in metadata.c (static _vg_read_internal)
> (plus, all patches should apply cleanly on top of today's CVS)
> 
> Hopefully, this moves things forward a little.
> 
> Yours,
>    Petr.
> 
> --
> lvm-devel mailing list
> lvm-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/lvm-devel

[PATCH] Another take on vg_read.

[Petr Rockai]

Dave Wysochanski <dwysocha@redhat.com> writes:
> Agk's earlier comments still unaddressed:
> 1. PATCH6 (old patch#4/11). vg_read_error().
> use a typedef for the return value of vg_read_error(), or explain what the
> uint32_t means 
> https://www.redhat.com/archives/lvm-devel/2009-January/msg00039.html
Yes, I have deferred this.

> 2. PATCH2. flag naming/definition.
> Use a common prefix so they get grouped together.  Line up the 0x0000 parts
> vertically making their "single bit" nature more obvious.
> https://www.redhat.com/archives/lvm-devel/2009-January/msg00036.html
The 0x00 are hopefully lined up now and these look addressed to me. Common
prefix: why? They are far more readable this way and I don't know what prefix
to add, either.

> 3. PATCH2. EXISTENCE_CHECK definition.
> Ambiguous meaning - which is it?  1. self-evident (delete).  2. reserved for
> future (say 'reserved').
> https://www.redhat.com/archives/lvm-devel/2009-January/msg00036.html
This is explained in a comment, and I believe justified as it is.

> 4. PATCH2. _vg_make_handle
> naming and possible 'failure' typedef
> https://www.redhat.com/archives/lvm-devel/2009-January/msg00037.html
It is used to make non-error handles as well, so I don't think it should be
renamed. See the new comment above the function explaining it.

> 5. PATCH2. _recover_vg()
> [note to check call paths later if more than one VG lock is held simultaneously]
> https://www.redhat.com/archives/lvm-devel/2009-January/msg00037.html
This can probably be addressed by not doing any recovery in case DISABLE_LOCK
is given to vg_read?

> 6. PATCH2. Flag definition.
> Some flags (ALLOW_INCONSISTENT) still need comment definitions.
> https://www.redhat.com/archives/lvm-devel/2009-January/msg00037.html
Indeed. ALLOW_INCONSISTENT is explained in _vg_lock_and_read comment. It
probably deserves moving to vg_read description or to the flag definition
itself. (Most other flags are described in vg_read description as well.)

> 7. PATCH2. read_failed member
> Let's add a brief comment explaining this one.
> read_failed ? Yes or No.  Rename it perhaps?
> I think it should be completely separate from 'status' and 'alloc'.
> https://www.redhat.com/archives/lvm-devel/2009-January/msg00037.html
The added comment currently says what 0 and non-0 means and is separate from
both status and alloc. Something else that needs changing there?

> 8. PATCH2 (old patch#3/11) _vg_check_status
> typedef for the return value?
> https://www.redhat.com/archives/lvm-devel/2009-January/msg00038.html
Yes, typedef, see point 1.

> Future cleanup:
> 1. FAILED_READ_ONLY
> Note only FAILED_READ_ONLY is used by any tool today - only vgreduce.
> Further, FAILED_READ_ONLY can only occur with LVM1 metadata as far as I
> can tell.
>
> 2. PATCH2. _vg_lock_and_read failure path with invalid name returns NULL.
> Note that callling vg_read_error() after hitting this path will return
> FAILED_ALLOCATION.  Do we need FAILED_INVALID_NAME or perhaps re-use
> FAILED_NOT_FOUND?
> https://www.redhat.com/archives/lvm-devel/2009-January/msg00040.html
Noted.

Yours,
   Petr.

-- 
Peter Rockai | me()mornfall!net | prockai()redhat!com
 http://blog.mornfall.net | http://web.mornfall.net

"In My Egotistical Opinion, most people's C programs should be
 indented six feet downward and covered with dirt."
     -- Blair P. Houghton on the subject of C program indentation