[PATCH 1/4] Move guts of pvcreate into library area - prep for vgcreate.

[PATCH 1/4] Move guts of pvcreate into library area - prep for vgcreate.

[Dave Wysochanski]

Preparation for allowing vgcreate on devices which are not currently PVs.
Move necessary pvcreate code from tools subdir into library so we can call
this higher-level pvcreate code from the vgcreate path in the library.
This will also get us a little closer to where we should be with libLVM,
though there is some cleanup to do (pvcreate_single and pv_create should
be merged into a single function exported by the libLVM).

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 lib/metadata/metadata-exported.h |   18 +++
 lib/metadata/metadata.c          |  196 +++++++++++++++++++++++++++++++++++
 tools/pvcreate.c                 |  213 +-------------------------------------
 3 files changed, 215 insertions(+), 212 deletions(-)

diff --git a/lib/metadata/metadata-exported.h b/lib/metadata/metadata-exported.h
index d49eaba..829a894 100644
--- a/lib/metadata/metadata-exported.h
+++ b/lib/metadata/metadata-exported.h
@@ -353,6 +353,24 @@ vg_t *vg_lock_and_read(struct cmd_context *cmd, const char *vg_name,
 		       uint32_t lock_flags, uint32_t status_flags,
 		       uint32_t misc_flags);
 
+struct pvcreate_params {
+	int zero;
+	uint64_t size;
+	int pvmetadatacopies;
+	uint64_t pvmetadatasize;
+	int64_t labelsector;
+	struct id id; /* FIXME: redundant */
+	struct id *idp; /* 0 if no --uuid option */
+	uint64_t pe_start;
+	uint32_t extent_count;
+	uint32_t extent_size;
+	const char *restorefile; /* 0 if no --restorefile option */
+	force_t force;
+	unsigned yes;
+};
+
+int pvcreate_single(struct cmd_context *cmd, const char *pv_name,
+		    void *handle);
 /* 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,
diff --git a/lib/metadata/metadata.c b/lib/metadata/metadata.c
index e5a13e6..265b619 100644
--- a/lib/metadata/metadata.c
+++ b/lib/metadata/metadata.c
@@ -28,9 +28,13 @@
 #include "locking.h"
 #include "archiver.h"
 #include "defaults.h"
+#include "filter-persistent.h"
 
 #include <sys/param.h>
 
+const char _really_init[] =
+    "Really INITIALIZE physical volume \"%s\" of volume group \"%s\" [y/n]? ";
+
 /*
  * FIXME: Check for valid handle before dereferencing field or log error?
  */
@@ -879,6 +883,198 @@ static struct physical_volume *_pv_create(const struct format_type *fmt,
 	return NULL;
 }
 
+/*
+ * See if we may pvcreate on this device.
+ * 0 indicates we may not.
+ */
+static int pvcreate_check(struct cmd_context *cmd, const char *name,
+			  struct pvcreate_params *pp)
+{
+	struct physical_volume *pv;
+	struct device *dev;
+	uint64_t md_superblock;
+
+	/* FIXME Check partition type is LVM unless --force is given */
+
+	/* Is there a pv here already? */
+	pv = pv_read(cmd, name, NULL, NULL, 0);
+
+	/*
+	 * If a PV has no MDAs it may appear to be an orphan until the
+	 * metadata is read off another PV in the same VG.  Detecting
+	 * this means checking every VG by scanning every PV on the
+	 * system.
+	 */
+	if (pv && is_orphan(pv)) {
+		if (!scan_vgs_for_pvs(cmd))
+			return_0;
+		pv = pv_read(cmd, name, NULL, NULL, 0);
+	}
+
+	/* Allow partial & exported VGs to be destroyed. */
+	/* We must have -ff to overwrite a non orphan */
+	if (pv && !is_orphan(pv) && pp->force != DONT_PROMPT_OVERRIDE) {
+		log_error("Can't initialize physical volume \"%s\" of "
+			  "volume group \"%s\" without -ff", name, pv_vg_name(pv));
+		return 0;
+	}
+
+	/* prompt */
+	if (pv && !is_orphan(pv) && !pp->yes &&
+	    yes_no_prompt(_really_init, name, pv_vg_name(pv)) == 'n') {
+		log_print("%s: physical volume not initialized", name);
+		return 0;
+	}
+
+	if (sigint_caught())
+		return 0;
+
+	dev = dev_cache_get(name, cmd->filter);
+
+	/* Is there an md superblock here? */
+	if (!dev && md_filtering()) {
+		unlock_vg(cmd, VG_ORPHANS);
+
+		persistent_filter_wipe(cmd->filter);
+		lvmcache_destroy(cmd, 1);
+
+		init_md_filtering(0);
+		if (!lock_vol(cmd, VG_ORPHANS, LCK_VG_WRITE)) {
+			log_error("Can't get lock for orphan PVs");
+			init_md_filtering(1);
+			return 0;
+		}
+		dev = dev_cache_get(name, cmd->filter);
+		init_md_filtering(1);
+	}
+
+	if (!dev) {
+		log_error("Device %s not found (or ignored by filtering).", name);
+		return 0;
+	}
+
+	/*
+ 	 * This test will fail if the device belongs to an MD array.
+	 */
+	if (!dev_test_excl(dev)) {
+		/* FIXME Detect whether device-mapper itself is still using it */
+		log_error("Can't open %s exclusively.  Mounted filesystem?",
+			  name);
+		return 0;
+	}
+
+	/* Wipe superblock? */
+	if (dev_is_md(dev, &md_superblock) &&
+	    ((!pp->idp && !pp->restorefile) || pp->yes ||
+	     (yes_no_prompt("Software RAID md superblock "
+			    "detected on %s. Wipe it? [y/n] ", name) == 'y'))) {
+		log_print("Wiping software RAID md superblock on %s", name);
+		if (!dev_set(dev, md_superblock, 4, 0)) {
+			log_error("Failed to wipe RAID md superblock on %s",
+				  name);
+			return 0;
+		}
+	}
+
+	if (sigint_caught())
+		return 0;
+
+	if (pv && !is_orphan(pv) && pp->force) {
+		log_warn("WARNING: Forcing physical volume creation on "
+			  "%s%s%s%s", name,
+			  !is_orphan(pv) ? " of volume group \"" : "",
+			  !is_orphan(pv) ? pv_vg_name(pv) : "",
+			  !is_orphan(pv) ? "\"" : "");
+	}
+
+	return 1;
+}
+
+int pvcreate_single(struct cmd_context *cmd, const char *pv_name,
+		    void *handle)
+{
+	struct pvcreate_params *pp = (struct pvcreate_params *) handle;
+	void *pv;
+	struct device *dev;
+	struct dm_list mdas;
+
+	if (pp->idp) {
+		if ((dev = device_from_pvid(cmd, pp->idp)) &&
+		    (dev != dev_cache_get(pv_name, cmd->filter))) {
+			log_error("uuid %s already in use on \"%s\"",
+				  pp->idp->uuid, dev_name(dev));
+			return 0;
+		}
+	}
+
+	if (!lock_vol(cmd, VG_ORPHANS, LCK_VG_WRITE)) {
+		log_error("Can't get lock for orphan PVs");
+		return 0;
+	}
+
+	if (!pvcreate_check(cmd, pv_name, pp))
+		goto error;
+
+	if (sigint_caught())
+		goto error;
+
+	if (!(dev = dev_cache_get(pv_name, cmd->filter))) {
+		log_error("%s: Couldn't find device.  Check your filters?",
+			  pv_name);
+		goto error;
+	}
+
+	dm_list_init(&mdas);
+	if (!(pv = pv_create(cmd, dev, pp->idp, pp->size, pp->pe_start,
+			     pp->extent_count, pp->extent_size,
+			     pp->pvmetadatacopies,
+			     pp->pvmetadatasize,&mdas))) {
+		log_error("Failed to setup physical volume \"%s\"", pv_name);
+		goto error;
+	}
+
+	log_verbose("Set up physical volume for \"%s\" with %" PRIu64
+		    " available sectors", pv_name, pv_size(pv));
+
+	/* Wipe existing label first */
+	if (!label_remove(pv_dev(pv))) {
+		log_error("Failed to wipe existing label on %s", pv_name);
+		goto error;
+	}
+
+	if (pp->zero) {
+		log_verbose("Zeroing start of device %s", pv_name);
+		if (!dev_open_quiet(dev)) {
+			log_error("%s not opened: device not zeroed", pv_name);
+			goto error;
+		}
+
+		if (!dev_set(dev, UINT64_C(0), (size_t) 2048, 0)) {
+			log_error("%s not wiped: aborting", pv_name);
+			dev_close(dev);
+			goto error;
+		}
+		dev_close(dev);
+	}
+
+	log_very_verbose("Writing physical volume data to disk \"%s\"",
+			 pv_name);
+	if (!(pv_write(cmd, (struct physical_volume *)pv, &mdas,
+		       pp->labelsector))) {
+		log_error("Failed to write physical volume \"%s\"", pv_name);
+		goto error;
+	}
+
+	log_print("Physical volume \"%s\" successfully created", pv_name);
+
+	unlock_vg(cmd, VG_ORPHANS);
+	return 1;
+
+      error:
+	unlock_vg(cmd, VG_ORPHANS);
+	return 0;
+}
+
 /* FIXME: liblvm todo - make into function that returns handle */
 struct pv_list *find_pv_in_vg(const struct volume_group *vg,
 			      const char *pv_name)
diff --git a/tools/pvcreate.c b/tools/pvcreate.c
index db1a0e2..f40cd2c 100644
--- a/tools/pvcreate.c
+++ b/tools/pvcreate.c
@@ -14,218 +14,7 @@
  */
 
 #include "tools.h"
-#include "metadata.h"
-
-struct pvcreate_params {
-	int zero;
-	uint64_t size;
-	int pvmetadatacopies;
-	uint64_t pvmetadatasize;
-	int64_t labelsector;
-	struct id id; /* FIXME: redundant */
-	struct id *idp; /* 0 if no --uuid option */
-	uint64_t pe_start;
-	uint32_t extent_count;
-	uint32_t extent_size;
-	const char *restorefile; /* 0 if no --restorefile option */
-	force_t force;
-	unsigned yes;
-};
-
-const char _really_init[] =
-    "Really INITIALIZE physical volume \"%s\" of volume group \"%s\" [y/n]? ";
-
-/*
- * See if we may pvcreate on this device.
- * 0 indicates we may not.
- */
-static int pvcreate_check(struct cmd_context *cmd, const char *name,
-			  struct pvcreate_params *pp)
-{
-	struct physical_volume *pv;
-	struct device *dev;
-	uint64_t md_superblock;
-
-	/* FIXME Check partition type is LVM unless --force is given */
-
-	/* Is there a pv here already? */
-	pv = pv_read(cmd, name, NULL, NULL, 0);
-
-	/*
-	 * If a PV has no MDAs it may appear to be an orphan until the
-	 * metadata is read off another PV in the same VG.  Detecting
-	 * this means checking every VG by scanning every PV on the
-	 * system.
-	 */
-	if (pv && is_orphan(pv)) {
-		if (!scan_vgs_for_pvs(cmd))
-			return_0;
-		pv = pv_read(cmd, name, NULL, NULL, 0);
-	}
-
-	/* Allow partial & exported VGs to be destroyed. */
-	/* We must have -ff to overwrite a non orphan */
-	if (pv && !is_orphan(pv) && pp->force != DONT_PROMPT_OVERRIDE) {
-		log_error("Can't initialize physical volume \"%s\" of "
-			  "volume group \"%s\" without -ff", name, pv_vg_name(pv));
-		return 0;
-	}
-
-	/* prompt */
-	if (pv && !is_orphan(pv) && !pp->yes &&
-	    yes_no_prompt(_really_init, name, pv_vg_name(pv)) == 'n') {
-		log_print("%s: physical volume not initialized", name);
-		return 0;
-	}
-
-	if (sigint_caught())
-		return 0;
-
-	dev = dev_cache_get(name, cmd->filter);
-
-	/* Is there an md superblock here? */
-	if (!dev && md_filtering()) {
-		unlock_vg(cmd, VG_ORPHANS);
-
-		persistent_filter_wipe(cmd->filter);
-		lvmcache_destroy(cmd, 1);
-
-		init_md_filtering(0);
-		if (!lock_vol(cmd, VG_ORPHANS, LCK_VG_WRITE)) {
-			log_error("Can't get lock for orphan PVs");
-			init_md_filtering(1);
-			return 0;
-		}
-		dev = dev_cache_get(name, cmd->filter);
-		init_md_filtering(1);
-	}
-
-	if (!dev) {
-		log_error("Device %s not found (or ignored by filtering).", name);
-		return 0;
-	}
-
-	/*
- 	 * This test will fail if the device belongs to an MD array.
-	 */
-	if (!dev_test_excl(dev)) {
-		/* FIXME Detect whether device-mapper itself is still using it */
-		log_error("Can't open %s exclusively.  Mounted filesystem?",
-			  name);
-		return 0;
-	}
-
-	/* Wipe superblock? */
-	if (dev_is_md(dev, &md_superblock) &&
-	    ((!pp->idp && !pp->restorefile) || pp->yes ||
-	     (yes_no_prompt("Software RAID md superblock "
-			    "detected on %s. Wipe it? [y/n] ", name) == 'y'))) {
-		log_print("Wiping software RAID md superblock on %s", name);
-		if (!dev_set(dev, md_superblock, 4, 0)) {
-			log_error("Failed to wipe RAID md superblock on %s",
-				  name);
-			return 0;
-		}
-	}
-
-	if (sigint_caught())
-		return 0;
-
-	if (pv && !is_orphan(pv) && pp->force) {
-		log_warn("WARNING: Forcing physical volume creation on "
-			  "%s%s%s%s", name,
-			  !is_orphan(pv) ? " of volume group \"" : "",
-			  !is_orphan(pv) ? pv_vg_name(pv) : "",
-			  !is_orphan(pv) ? "\"" : "");
-	}
-
-	return 1;
-}
-
-static int pvcreate_single(struct cmd_context *cmd, const char *pv_name,
-			   void *handle)
-{
-	struct pvcreate_params *pp = (struct pvcreate_params *) handle;
-	void *pv;
-	struct device *dev;
-	struct dm_list mdas;
-
-	if (pp->idp) {
-		if ((dev = device_from_pvid(cmd, pp->idp)) &&
-		    (dev != dev_cache_get(pv_name, cmd->filter))) {
-			log_error("uuid %s already in use on \"%s\"",
-				  pp->idp->uuid, dev_name(dev));
-			return 0;
-		}
-	}
-
-	if (!lock_vol(cmd, VG_ORPHANS, LCK_VG_WRITE)) {
-		log_error("Can't get lock for orphan PVs");
-		return 0;
-	}
-
-	if (!pvcreate_check(cmd, pv_name, pp))
-		goto error;
-
-	if (sigint_caught())
-		goto error;
-
-	if (!(dev = dev_cache_get(pv_name, cmd->filter))) {
-		log_error("%s: Couldn't find device.  Check your filters?",
-			  pv_name);
-		goto error;
-	}
-
-	dm_list_init(&mdas);
-	if (!(pv = pv_create(cmd, dev, pp->idp, pp->size, pp->pe_start,
-			     pp->extent_count, pp->extent_size,
-			     pp->pvmetadatacopies,
-			     pp->pvmetadatasize,&mdas))) {
-		log_error("Failed to setup physical volume \"%s\"", pv_name);
-		goto error;
-	}
-
-	log_verbose("Set up physical volume for \"%s\" with %" PRIu64
-		    " available sectors", pv_name, pv_size(pv));
-
-	/* Wipe existing label first */
-	if (!label_remove(pv_dev(pv))) {
-		log_error("Failed to wipe existing label on %s", pv_name);
-		goto error;
-	}
-
-	if (pp->zero) {
-		log_verbose("Zeroing start of device %s", pv_name);
-		if (!dev_open_quiet(dev)) {
-			log_error("%s not opened: device not zeroed", pv_name);
-			goto error;
-		}
-
-		if (!dev_set(dev, UINT64_C(0), (size_t) 2048, 0)) {
-			log_error("%s not wiped: aborting", pv_name);
-			dev_close(dev);
-			goto error;
-		}
-		dev_close(dev);
-	}
-
-	log_very_verbose("Writing physical volume data to disk \"%s\"",
-			 pv_name);
-	if (!(pv_write(cmd, (struct physical_volume *)pv, &mdas,
-		       pp->labelsector))) {
-		log_error("Failed to write physical volume \"%s\"", pv_name);
-		goto error;
-	}
-
-	log_print("Physical volume \"%s\" successfully created", pv_name);
-
-	unlock_vg(cmd, VG_ORPHANS);
-	return 1;
-
-      error:
-	unlock_vg(cmd, VG_ORPHANS);
-	return 0;
-}
+#include "metadata-exported.h"
 
 /*
  * Intial sanity checking of command-line arguments and fill in 'pp' fields.
-- 
1.5.5.1

[PATCH 2/4] Allow pvcreate_single() to be called with NULL for pvcreate parameters.

[Dave Wysochanski]

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 lib/metadata/metadata.c |   20 +++++++++++++++++++-
 1 files changed, 19 insertions(+), 1 deletions(-)

diff --git a/lib/metadata/metadata.c b/lib/metadata/metadata.c
index 265b619..09c345f 100644
--- a/lib/metadata/metadata.c
+++ b/lib/metadata/metadata.c
@@ -993,10 +993,28 @@ static int pvcreate_check(struct cmd_context *cmd, const char *name,
 int pvcreate_single(struct cmd_context *cmd, const char *pv_name,
 		    void *handle)
 {
-	struct pvcreate_params *pp = (struct pvcreate_params *) handle;
+	struct pvcreate_params *pp;
 	void *pv;
 	struct device *dev;
 	struct dm_list mdas;
+	struct pvcreate_params default_pp = { 0, /* zero Y/N */
+					      0, /* size */
+					      DEFAULT_PVMETADATACOPIES,
+					      DEFAULT_PVMETADATASIZE,
+					      DEFAULT_LABELSECTOR,
+					      { { 0 } }, /* id */
+					      0, /* idp */
+					      0, /* pe_start */
+					      0, /* extent_count */
+					      0, /* extent_size */
+					      0, /* restorefile */
+					      PROMPT, /* force option */
+					      0 /* yes */};
+
+	if (!handle)
+		pp = &default_pp;
+	else
+		pp = (struct pvcreate_params *) handle;
 
 	if (pp->idp) {
 		if ((dev = device_from_pvid(cmd, pp->idp)) &&
-- 
1.5.5.1

[PATCH 3/4] Change pvcreate_single() to return pv_t * in preparation for other callers.

[Dave Wysochanski]

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 lib/metadata/metadata-exported.h |    4 ++--
 lib/metadata/metadata.c          |   12 ++++++------
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/lib/metadata/metadata-exported.h b/lib/metadata/metadata-exported.h
index 829a894..c77ebdd 100644
--- a/lib/metadata/metadata-exported.h
+++ b/lib/metadata/metadata-exported.h
@@ -369,8 +369,8 @@ struct pvcreate_params {
 	unsigned yes;
 };
 
-int pvcreate_single(struct cmd_context *cmd, const char *pv_name,
-		    void *handle);
+pv_t *pvcreate_single(struct cmd_context *cmd, const char *pv_name,
+		      void *handle);
 /* 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,
diff --git a/lib/metadata/metadata.c b/lib/metadata/metadata.c
index 09c345f..4b231d3 100644
--- a/lib/metadata/metadata.c
+++ b/lib/metadata/metadata.c
@@ -990,8 +990,8 @@ static int pvcreate_check(struct cmd_context *cmd, const char *name,
 	return 1;
 }
 
-int pvcreate_single(struct cmd_context *cmd, const char *pv_name,
-		    void *handle)
+pv_t *pvcreate_single(struct cmd_context *cmd, const char *pv_name,
+		      void *handle)
 {
 	struct pvcreate_params *pp;
 	void *pv;
@@ -1021,13 +1021,13 @@ int pvcreate_single(struct cmd_context *cmd, const char *pv_name,
 		    (dev != dev_cache_get(pv_name, cmd->filter))) {
 			log_error("uuid %s already in use on \"%s\"",
 				  pp->idp->uuid, dev_name(dev));
-			return 0;
+			return NULL;
 		}
 	}
 
 	if (!lock_vol(cmd, VG_ORPHANS, LCK_VG_WRITE)) {
 		log_error("Can't get lock for orphan PVs");
-		return 0;
+		return NULL;
 	}
 
 	if (!pvcreate_check(cmd, pv_name, pp))
@@ -1086,11 +1086,11 @@ int pvcreate_single(struct cmd_context *cmd, const char *pv_name,
 	log_print("Physical volume \"%s\" successfully created", pv_name);
 
 	unlock_vg(cmd, VG_ORPHANS);
-	return 1;
+	return pv;
 
       error:
 	unlock_vg(cmd, VG_ORPHANS);
-	return 0;
+	return NULL;
 }
 
 /* FIXME: liblvm todo - make into function that returns handle */
-- 
1.5.5.1

[PATCH 4/4] Update vgcreate and vgextend to allow uninitialized devices as input.

[Dave Wysochanski]

Note that this changes the behavior of vgcreate and vgextend when non-PV
devices are given.  Previously this would result in vgextend or vgcreate
printing an error message and exiting.  Now these tools will prompt the
user.

With this update, the user may opt to skip pvcreate, provided the default
values of pvcreate are adequate.  If the user wants some non-default value
in the PV (e.g. 0 metadatacopies, etc), pvcreate must be used prior to
vgcreate/vgextend.

Update man pages to clarify the new behavior.

Modify pvcreate_single() to first check whether the VG_ORPHANS lock is
held before trying to obtain it.  This is necessary for the vgexten
code path, which must obtain VG_ORPHANS before obtaining the appropriate
VG lock, and befroe calling vg_extend().  The other paths calling
pvcreate_single() are pvcreate() and vgcreate(), and neither of these
paths hold VG_ORPHANS.  Without this modification vgextend() will hang
trying to obtain VG_ORPHANS which it already holds.

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 WHATS_NEW               |    1 +
 lib/metadata/metadata.c |   32 ++++++++++++++++++++++++++++----
 man/vgcreate.8.in       |   15 +++++++++------
 man/vgextend.8.in       |    6 ++++++
 4 files changed, 44 insertions(+), 10 deletions(-)

diff --git a/WHATS_NEW b/WHATS_NEW
index 459533a..03ec238 100644
--- a/WHATS_NEW
+++ b/WHATS_NEW
@@ -1,5 +1,6 @@
 Version 2.02.44 - 
 ====================================
+  Update vgcreate and vgextend to allow uninitialized devices as input.
   Don't skip updating pvid hash when lvmcache_info struct got swapped.
   Add tinfo to termcap search path for pld-linux.
   Fix startup race in clvmd.
diff --git a/lib/metadata/metadata.c b/lib/metadata/metadata.c
index 4b231d3..4e375f5 100644
--- a/lib/metadata/metadata.c
+++ b/lib/metadata/metadata.c
@@ -422,9 +422,22 @@ int vg_extend(struct volume_group *vg, int pv_count, char **pv_names)
 	/* attach each pv */
 	for (i = 0; i < pv_count; i++) {
 		if (!(pv = pv_by_path(vg->fid->fmt->cmd, pv_names[i]))) {
-			log_error("%s not identified as an existing "
-				  "physical volume", pv_names[i]);
-			goto bad;
+			if (yes_no_prompt("%s not identified as an existing "
+					  "physical volume.\nInitialize "
+					  "device and add to volume "
+					  "group %s? [y/n]: ", pv_names[i],
+					  vg->name) == 'n') {
+				goto bad;
+			} else {
+				pv = pvcreate_single(vg->cmd, pv_names[i],
+						     NULL);
+				if (!pv) {
+					log_error("Failed to setup physical "
+						  "volume \"%s\"",
+						  pv_names[i]);
+					goto bad;
+				}
+			}
 		}
 		
 		if (!add_pv_to_vg(vg, pv_names[i], pv))
@@ -990,6 +1003,16 @@ static int pvcreate_check(struct cmd_context *cmd, const char *name,
 	return 1;
 }
 
+/*
+ * pvcreate_single() - initialize a device with PV label and metadata
+ *
+ * Locking: Ok (but not required) for caller to hold VG_ORPHAN lock
+ * before calling
+ *
+ * Parameters:
+ * - pv_name: device path to initialize
+ * - handle: options to pass to pv_create; NULL indicates use defaults
+ */
 pv_t *pvcreate_single(struct cmd_context *cmd, const char *pv_name,
 		      void *handle)
 {
@@ -1025,7 +1048,8 @@ pv_t *pvcreate_single(struct cmd_context *cmd, const char *pv_name,
 		}
 	}
 
-	if (!lock_vol(cmd, VG_ORPHANS, LCK_VG_WRITE)) {
+	if (!vgname_is_locked(VG_ORPHANS) &&
+	    (!lock_vol(cmd, VG_ORPHANS, LCK_VG_WRITE))) {
 		log_error("Can't get lock for orphan PVs");
 		return NULL;
 	}
diff --git a/man/vgcreate.8.in b/man/vgcreate.8.in
index a53197e..0e8baa2 100644
--- a/man/vgcreate.8.in
+++ b/man/vgcreate.8.in
@@ -21,16 +21,19 @@ vgcreate \- create a volume group
 .RB [ \-t | \-\-test ]
 .RB [ \-v | \-\-verbose ]
 .RB [ \-\-version ]
-.I VolumeGroupName PhysicalVolumePath
-.RI [ PhysicalVolumePath ...]
+.I VolumeGroupName PhysicalDevicePath
+.RI [ PhysicalDevicePath ...]
 .SH DESCRIPTION
 .B vgcreate
 creates a new volume group called
 .I VolumeGroupName
-using the block special device
-.IR PhysicalVolumePath
-previously configured for LVM with
-.BR pvcreate (8).
+using the block special device \fIPhysicalDevicePath\fP.
+If \fIPhysicalDevicePath\fP was not previously configured for LVM with
+\fBpvcreate (8)\fP, a prompt will be given to confirm initialization of
+the device.  If confirmed, the device will be initialized with the same
+default values used with \fBpvcreate\fP.  If non-default 
+\fPpvcreate\fP values are are desired, \fBpvcreate\fP should be explicitly
+called prior to calling \fBvgcreate\fP.
 .SH OPTIONS
 See \fBlvm\fP for common options.
 .TP
diff --git a/man/vgextend.8.in b/man/vgextend.8.in
index 682cc5a..1fee769 100644
--- a/man/vgextend.8.in
+++ b/man/vgextend.8.in
@@ -11,6 +11,12 @@ VolumeGroupName PhysicalDevicePath [PhysicalDevicePath...]
 vgextend allows you to add one or more initialized physical volumes ( see
 .B pvcreate(8)
 ) to an existing volume group to extend it in size.
+If \fIPhysicalDevicePath\fP was not previously configured for LVM with
+\fBpvcreate (8)\fP, a prompt will be given to confirm initialization of
+the device.  If confirmed, the device will be initialized with the same
+default values used with \fBpvcreate\fP.  If non-default 
+\fPpvcreate\fP values are are desired, \fBpvcreate\fP should be explicitly
+called prior to calling \fBvgextend\fP.
 .SH OPTIONS
 See \fBlvm\fP for common options.
 .SH Examples
-- 
1.5.5.1

[PATCH 4/4] Update vgcreate and vgextend to allow uninitialized devices as input.

[Dave Wysochanski]

On Sun, 2008-11-30 at 04:18 +0000, Alasdair G Kergon wrote:
> Why not just add all the relevant pvcreate parameters to those commands?

I thought it would clutter the commands unnecessarily for options that
were seldom used.  I will work on a patch for adding the parameters
though since it sounds like that is what you would like.

> And any prompting should be identical to how pvcreate itself behaves.
> - If you want a prompt to say 'we are running pvcreate implcitly here'
> until people get used to the new behaviour, then make it a config option that
> people can disable.
> 
So you don't think there is danger in just switching to the new behavior
(used to be an error to vgcreate/vgextend with non-initialized disk, now
will initialized the disk) without a transition period?  The case I was
thinking of was a user that inadvertently specified the wrong disk.  The
pvcreate code will catch at least some of the cases but perhaps not
others.  It is now a destructive operation whereas before it was not.

> This *will* be the default in future - most people will not use pvcreate
> at all.  Think of it from the point of view of a newcomer to lvm - no need
> to learn about PVs in order to set up and use lvm.
> 
Right.  I was just wondering about the transition period and whether we
would deprecate pvcreate in the future.  If not, then why not just leave
vgextend/vgcreate without the pvcreate options and make the less-likely
user pay the penalty?

Re: [PATCH 4/4] Update vgcreate and vgextend to allow uninitialized devices as input.

[Dave Wysochanski]

On Sat, 2008-11-29 at 16:02 -0500, Dave Wysochanski wrote:

> Modify pvcreate_single() to first check whether the VG_ORPHANS lock is
> held before trying to obtain it.  This is necessary for the vgexten
> code path, which must obtain VG_ORPHANS before obtaining the appropriate
> VG lock, and befroe calling vg_extend().  The other paths calling
> pvcreate_single() are pvcreate() and vgcreate(), and neither of these
> paths hold VG_ORPHANS.  Without this modification vgextend() will hang
> trying to obtain VG_ORPHANS which it already holds.
> 
> Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
> ---
>  WHATS_NEW               |    1 +
>  lib/metadata/metadata.c |   32 ++++++++++++++++++++++++++++----
>  man/vgcreate.8.in       |   15 +++++++++------
>  man/vgextend.8.in       |    6 ++++++
>  4 files changed, 44 insertions(+), 10 deletions(-)
> 
> diff --git a/WHATS_NEW b/WHATS_NEW
> index 459533a..03ec238 100644
> --- a/WHATS_NEW
> +++ b/WHATS_NEW
> @@ -1,5 +1,6 @@
>  Version 2.02.44 - 
>  ====================================
> +  Update vgcreate and vgextend to allow uninitialized devices as input.
>    Don't skip updating pvid hash when lvmcache_info struct got swapped.
>    Add tinfo to termcap search path for pld-linux.
>    Fix startup race in clvmd.
> diff --git a/lib/metadata/metadata.c b/lib/metadata/metadata.c
> index 4b231d3..4e375f5 100644
> --- a/lib/metadata/metadata.c
> +++ b/lib/metadata/metadata.c
> @@ -422,9 +422,22 @@ int vg_extend(struct volume_group *vg, int pv_count, char **pv_names)
>  	/* attach each pv */
>  	for (i = 0; i < pv_count; i++) {
>  		if (!(pv = pv_by_path(vg->fid->fmt->cmd, pv_names[i]))) {
> -			log_error("%s not identified as an existing "
> -				  "physical volume", pv_names[i]);
> -			goto bad;
> +			if (yes_no_prompt("%s not identified as an existing "
> +					  "physical volume.\nInitialize "
> +					  "device and add to volume "
> +					  "group %s? [y/n]: ", pv_names[i],
> +					  vg->name) == 'n') {
> +				goto bad;
> +			} else {
> +				pv = pvcreate_single(vg->cmd, pv_names[i],
> +						     NULL);
> +				if (!pv) {
> +					log_error("Failed to setup physical "
> +						  "volume \"%s\"",
> +						  pv_names[i]);
> +					goto bad;
> +				}
> +			}
>  		}
>  		
>  		if (!add_pv_to_vg(vg, pv_names[i], pv))
> @@ -990,6 +1003,16 @@ static int pvcreate_check(struct cmd_context *cmd, const char *name,
>  	return 1;
>  }
>  
> +/*
> + * pvcreate_single() - initialize a device with PV label and metadata
> + *
> + * Locking: Ok (but not required) for caller to hold VG_ORPHAN lock
> + * before calling
> + *
> + * Parameters:
> + * - pv_name: device path to initialize
> + * - handle: options to pass to pv_create; NULL indicates use defaults
> + */
>  pv_t *pvcreate_single(struct cmd_context *cmd, const char *pv_name,
>  		      void *handle)
>  {
> @@ -1025,7 +1048,8 @@ pv_t *pvcreate_single(struct cmd_context *cmd, const char *pv_name,
>  		}
>  	}
>  
> -	if (!lock_vol(cmd, VG_ORPHANS, LCK_VG_WRITE)) {
> +	if (!vgname_is_locked(VG_ORPHANS) &&
> +	    (!lock_vol(cmd, VG_ORPHANS, LCK_VG_WRITE))) {
>  		log_error("Can't get lock for orphan PVs");
>  		return NULL;
>  	}

Upon further consideration, I'm not sure putting the pvcreate_single()
calls into vg_extend() is the best way to do this since it causes
complications with VG_ORPHANS lock in pvcreate_single().  Even with the
above code to check the lock before obtaining, pvcreate_single() will
release VG_ORPHANS lock in a different sequence than was obtained (in
vgextend case).  Might be better to place this logic higher up in
vgcreate() and vgextend(), or try to refactor.  I will work on another
patch.

[PATCH 1/4] Move guts of pvcreate into library area - prep for vgcreate.

[Dave Wysochanski]

On Sun, 2008-11-30 at 04:26 +0000, Alasdair G Kergon wrote:
> I'm a bit bothered that too much code may be getting dumped into the library here
> wholesale rather than getting broken up into separate calls.
> 
> - I don't think the library should be prompting, that should remain at tools level
> and the interface should allow for that.
> -- maybe yes_no_prompt needs some global mode (anaconda would set) to control
> whether it does anything or simply defaults to the value given in an additional
> parameter 
> 

Why global mode - why not just set 'force' and '-y' for the API call?  I
think this was similar to Thomas's argument - he always envisioned '-ff'
and '-y' on things like pvcreate.

To be specific, in pvcreate we prompt if:
1) the device is already a PV
2) we find an MD superblock
(and of course the user has not given the '-ff' and '-y')

So if we don't want the yes_no_prompt() in the library, either the
library calls do not do these checks at all, or we duplicate the code in
the library.  Otherwise, we leave the yes_no_prompt() code in the
library and just set the options for the API call.

Note that we currently have yes_no_prompt() in the library for lvremove
and vgremove code paths.  The pvcreate/vgremove/lvremove paths might
also be a good area to talk about the error codes/handling and what the
API should look like.

[PATCH] Update vgcreate and vgextend to allow uninitialized devices as input.

[Dave Wysochanski]

Note that this changes the behavior of vgcreate and vgextend when non-PV
devices are given.  Previously this would result in vgextend or vgcreate
printing an error message and exiting.  Now these tools will implicitly
pvcreate on any devices given on the commandline that are not already
PVs.  All devices given on the commandline are first checked if they are
an existing PV before attempting to initialize, so there should be little/no
risk with this change.

With this update, the user may opt to skip pvcreate, provided the default
values of pvcreate are adequate.  If the user wants some non-default value
in the PV (e.g. 0 metadatacopies, etc), pvcreate must be used prior to
vgcreate/vgextend.

Update man pages to clarify the new behavior.

Add pvcreate_devices() helper function inside toollib which loops through
a set of device names and runs pvcreate on each devices if it is not already
a PV.
---
 WHATS_NEW                        |    1 +
 lib/metadata/metadata-exported.h |    1 +
 lib/metadata/metadata.c          |    7 +++++++
 lib/metadata/metadata.h          |    1 -
 man/vgcreate.8.in                |   14 ++++++++------
 man/vgextend.8.in                |    5 +++++
 tools/toollib.c                  |   23 +++++++++++++++++++++++
 tools/toollib.h                  |    2 ++
 tools/vgcreate.c                 |    6 ++++++
 tools/vgextend.c                 |    6 ++++++
 10 files changed, 59 insertions(+), 7 deletions(-)

diff --git a/WHATS_NEW b/WHATS_NEW
index 459533a..03ec238 100644
--- a/WHATS_NEW
+++ b/WHATS_NEW
@@ -1,5 +1,6 @@
 Version 2.02.44 - 
 ====================================
+  Update vgcreate and vgextend to allow uninitialized devices as input.
   Don't skip updating pvid hash when lvmcache_info struct got swapped.
   Add tinfo to termcap search path for pld-linux.
   Fix startup race in clvmd.
diff --git a/lib/metadata/metadata-exported.h b/lib/metadata/metadata-exported.h
index c77ebdd..71dfe36 100644
--- a/lib/metadata/metadata-exported.h
+++ b/lib/metadata/metadata-exported.h
@@ -588,6 +588,7 @@ uint32_t pv_pe_size(const pv_t *pv);
 uint64_t pv_pe_start(const pv_t *pv);
 uint32_t pv_pe_count(const pv_t *pv);
 uint32_t pv_pe_alloc_count(const pv_t *pv);
+pv_t *pv_by_path(struct cmd_context *cmd, const char *pv_name);
 
 int vg_missing_pv_count(const vg_t *vg);
 uint32_t vg_status(const vg_t *vg);
diff --git a/lib/metadata/metadata.c b/lib/metadata/metadata.c
index 4b231d3..8afa322 100644
--- a/lib/metadata/metadata.c
+++ b/lib/metadata/metadata.c
@@ -990,6 +990,13 @@ static int pvcreate_check(struct cmd_context *cmd, const char *name,
 	return 1;
 }
 
+/*
+ * pvcreate_single() - initialize a device with PV label and metadata
+ *
+ * Parameters:
+ * - pv_name: device path to initialize
+ * - handle: options to pass to pv_create; NULL indicates use defaults
+ */
 pv_t *pvcreate_single(struct cmd_context *cmd, const char *pv_name,
 		      void *handle)
 {
diff --git a/lib/metadata/metadata.h b/lib/metadata/metadata.h
index 3d9fd8d..7ce2c62 100644
--- a/lib/metadata/metadata.h
+++ b/lib/metadata/metadata.h
@@ -343,7 +343,6 @@ struct id pv_id(const pv_t *pv);
 const struct format_type *pv_format_type(const pv_t *pv);
 struct id pv_vgid(const pv_t *pv);
 
-pv_t *pv_by_path(struct cmd_context *cmd, const char *pv_name);
 int add_pv_to_vg(struct volume_group *vg, const char *pv_name,
 		 struct physical_volume *pv);
 
diff --git a/man/vgcreate.8.in b/man/vgcreate.8.in
index a53197e..54276dd 100644
--- a/man/vgcreate.8.in
+++ b/man/vgcreate.8.in
@@ -21,16 +21,18 @@ vgcreate \- create a volume group
 .RB [ \-t | \-\-test ]
 .RB [ \-v | \-\-verbose ]
 .RB [ \-\-version ]
-.I VolumeGroupName PhysicalVolumePath
-.RI [ PhysicalVolumePath ...]
+.I VolumeGroupName PhysicalDevicePath
+.RI [ PhysicalDevicePath ...]
 .SH DESCRIPTION
 .B vgcreate
 creates a new volume group called
 .I VolumeGroupName
-using the block special device
-.IR PhysicalVolumePath
-previously configured for LVM with
-.BR pvcreate (8).
+using the block special device \fIPhysicalDevicePath\fP.
+If \fIPhysicalDevicePath\fP was not previously configured for LVM with
+\fBpvcreate (8)\fP, the device will be initialized with the same
+default values used with \fBpvcreate\fP.  If non-default 
+\fPpvcreate\fP values are are desired, \fBpvcreate\fP should be explicitly
+called prior to calling \fBvgcreate\fP.
 .SH OPTIONS
 See \fBlvm\fP for common options.
 .TP
diff --git a/man/vgextend.8.in b/man/vgextend.8.in
index 682cc5a..3ba44a7 100644
--- a/man/vgextend.8.in
+++ b/man/vgextend.8.in
@@ -11,6 +11,11 @@ VolumeGroupName PhysicalDevicePath [PhysicalDevicePath...]
 vgextend allows you to add one or more initialized physical volumes ( see
 .B pvcreate(8)
 ) to an existing volume group to extend it in size.
+If \fIPhysicalDevicePath\fP was not previously configured for LVM with
+\fBpvcreate (8)\fP, the device will be initialized with the same
+default values used with \fBpvcreate\fP.  If non-default 
+\fPpvcreate\fP values are are desired, \fBpvcreate\fP should be explicitly
+called prior to calling \fBvgcreate\fP.
 .SH OPTIONS
 See \fBlvm\fP for common options.
 .SH Examples
diff --git a/tools/toollib.c b/tools/toollib.c
index ef4ee0e..2f0713e 100644
--- a/tools/toollib.c
+++ b/tools/toollib.c
@@ -1288,3 +1288,26 @@ int fill_vg_create_params(struct cmd_context *cmd,
 
 	return 0;
 }
+
+/*
+ * Run pvcreate on a set of devices if they are not currently initialized.
+ */
+int pvcreate_devices(struct cmd_context *cmd, int pv_count, char **pv_names)
+{
+	int i;
+	pv_t *pv;
+
+	for (i = 0; i < pv_count; i++) {
+		if (!(pv = pv_by_path(cmd, pv_names[i]))) {
+			log_verbose("Running pvcreate on %s", pv_names[i]);
+			if (!pvcreate_single(cmd, pv_names[i], NULL)) {
+				log_error("Failed to setup physical "
+					  "volume \"%s\"",
+					  pv_names[i]);
+				return 0;
+			}
+		}
+	}
+
+	return 1;
+}
diff --git a/tools/toollib.h b/tools/toollib.h
index f002d60..72d13a3 100644
--- a/tools/toollib.h
+++ b/tools/toollib.h
@@ -100,4 +100,6 @@ int is_reserved_lvname(const char *name);
 int fill_vg_create_params(struct cmd_context *cmd,
 			  char *vg_name, struct vgcreate_params *vp_new,
 			  struct vgcreate_params *vp_def);
+int pvcreate_devices(struct cmd_context *cmd, int pv_count, char **pv_names);
+
 #endif
diff --git a/tools/vgcreate.c b/tools/vgcreate.c
index 62c957a..d579ea9 100644
--- a/tools/vgcreate.c
+++ b/tools/vgcreate.c
@@ -46,6 +46,12 @@ int vgcreate(struct cmd_context *cmd, int argc, char **argv)
 	if (validate_vg_create_params(cmd, &vp_new))
 	    return EINVALID_CMD_LINE;
 
+	/*
+	 * Check whether any devices need initialized
+	 */
+	if (!pvcreate_devices(cmd, argc - 1, argv + 1))
+		return ECMD_FAILED;
+
 	/* Create the new VG */
 	if (!(vg = vg_create(cmd, vp_new.vg_name, vp_new.extent_size,
 			     vp_new.max_pv, vp_new.max_lv, vp_new.alloc,
diff --git a/tools/vgextend.c b/tools/vgextend.c
index 089c44e..154fbf6 100644
--- a/tools/vgextend.c
+++ b/tools/vgextend.c
@@ -35,6 +35,12 @@ int vgextend(struct cmd_context *cmd, int argc, char **argv)
 	argc--;
 	argv++;
 
+	/*
+	 * Check whether any devices need initialized
+	 */
+	if (!pvcreate_devices(cmd, argc, argv))
+		return ECMD_FAILED;
+
 	if (!lock_vol(cmd, VG_ORPHANS, LCK_VG_WRITE)) {
 		log_error("Can't get lock for orphan PVs");
 		return ECMD_FAILED;
-- 
1.5.5.1

[PATCH] Update vgcreate and vgextend to allow uninitialized devices as input.

[Alasdair G Kergon]

On Sun, Nov 30, 2008 at 02:11:13PM -0500, Dave Wysochanski wrote:
> +int pvcreate_devices(struct cmd_context *cmd, int pv_count, char **pv_names)

Can that hook instead into vg_extend() to replace the error message?
Current explicit PV functionality needs to become handled implicitly
throughout the code, rather than "do all the PV things" then "do all the
VG things".

Currently:
  vgcreate = { create empty vg; vgextend; }

int vg_extend(struct volume_group *vg, int pv_count, char **pv_names)
{
...
        /* attach each pv */
        for (i = 0; i < pv_count; i++) {
                if (!(pv = pv_by_path(vg->fid->fmt->cmd, pv_names[i]))) {
                        log_error("%s not identified as an existing "
                                  "physical volume", pv_names[i]);
                        goto bad;
                }
                
Alasdair
-- 
agk at redhat.com

[PATCH] Update vgcreate and vgextend to allow uninitialized devices as input.

[Alasdair G Kergon]

On Sun, Nov 30, 2008 at 02:11:13PM -0500, Dave Wysochanski wrote:
> With this update, the user may opt to skip pvcreate, provided the default
> values of pvcreate are adequate.  If the user wants some non-default value
> in the PV (e.g. 0 metadatacopies, etc), pvcreate must be used prior to
> vgcreate/vgextend.
 
Look at each of the pvcreate command line options and try to offer them as part
of vgcreate and vgextend.  They will only be used for devices that are not
already PVs and inside the VG.

vgcreate
        [--addtag Tag]   
	  - will continue to add a VG tag not a PV tag (because the command
	    name starts with vg)
	If there's a need for both in future we can add --addpvtag and --addvgtag.

pvcreate args:
    Add to both vgcreate & vgextend:
        [-f[f]|--force [--force]] 
        [-y|--yes]
	    - trivial

        [--labelsector sector] 
        [--metadatasize MetadataSize[kKmMgGtTpPeE]]
	    - unambiguous

        [--metadatacopies #copies]
	    - ambiguous: Does it mean number of copies on each disk or number
	      of copies in total in the VG?
	Solution: add --pvmetadatacopies to pvcreate, vgcreate & vgextend 
	In future we can add --vgmetadatacopies to vgcreate & vgextend.
	--metadatacopies will be the same as --pvmetadatacopies in pv* commands and
	--vgmetadatacopies in vg* commands.  Code it ready to work like that, i.e.
	change the existing code to use pvmetadatacopies_ARG everywhere
	internally, and use _merge_synonym() based on the first 2 chars of the
	command (hard-coded).

        [-Z|--zero {y|n}]
	   - potentially ambiguous: Does it mean zero the VG itself?
	But it's already ambiguous and the only way to find out what it does is to
	read the man page definition.  Zeroing the VG would have no impact or use
	within LVM, so we can leave it doing just what it does today to new PVs.

    Don't add to vgcreate & vgextend - leave these as pv-only for now:  (We can add
    them in an extended way eventually.)
        [--setphysicalvolumesize PhysicalVolumeSize[kKmMgGtTpPeE]
	    - main use is for testing so leave as pvcreate only

        [--restorefile file]
        [-u|--uuid uuid] 
	    - only required in recovery situations


After these changes, the only necessary use of pvcreate will be in recovery
situations.  In future, we'll sort out those remaining recovery options,
then require a vgname (defaulting to an explicit per-metadata-type orphan vg)
always to be given with pvcreate so that every PV belongs to a VG and make
pvcreate a synonym for vgcreate.


Alasdair
-- 
agk at redhat.com

[PATCH 1/4] Move guts of pvcreate into library area - prep for vgcreate.

[Alasdair G Kergon]

On Sun, Nov 30, 2008 at 09:57:47AM -0500, Dave Wysochanski wrote:
> On Sun, 2008-11-30 at 04:26 +0000, Alasdair G Kergon wrote:
> > I'm a bit bothered that too much code may be getting dumped into the library here
> > wholesale rather than getting broken up into separate calls.
> > 
> > - I don't think the library should be prompting, that should remain at tools level
> > and the interface should allow for that.
> > -- maybe yes_no_prompt needs some global mode (anaconda would set) to control
> > whether it does anything or simply defaults to the value given in an additional
> > parameter 
 
> Why global mode - why not just set 'force' and '-y' for the API call?  I
> think this was similar to Thomas's argument - he always envisioned '-ff'
> and '-y' on things like pvcreate.
> To be specific, in pvcreate we prompt if:
> 1) the device is already a PV
> 2) we find an MD superblock
> (and of course the user has not given the '-ff' and '-y')

To clarify: I mentioned two possible ways to proceed there.

1) no prompting in the library (which was my original intention)
=> library calls are split at appropriate places
In your example above, this would mean separate calls for the validation
functions (which anaconda would also call) prior to the call
that actually makes the change (which would not repeat the validation).

2) prompting within the library, with a guaranteed way for a caller
to avoid it (e.g. a global setting that defaults to returning with
an error whenever there would otherwise have been a prompt).
In the pvcreate example, it would avoid returning with an error because when
the equivalent of -ff and -y are given there would not be a prompt.
 
Alasdair
-- 
agk at redhat.com

[PATCH] Update vgcreate and vgextend to allow uninitialized devices as input.

[Dave Wysochanski]

On Sun, 2008-11-30 at 19:40 +0000, Alasdair G Kergon wrote:
> On Sun, Nov 30, 2008 at 02:11:13PM -0500, Dave Wysochanski wrote:
> > +int pvcreate_devices(struct cmd_context *cmd, int pv_count, char **pv_names)
> 
> Can that hook instead into vg_extend() to replace the error message?
> Current explicit PV functionality needs to become handled implicitly
> throughout the code, rather than "do all the PV things" then "do all the
> VG things".
> 
> Currently:
>   vgcreate = { create empty vg; vgextend; }
> 
> int vg_extend(struct volume_group *vg, int pv_count, char **pv_names)
> {
> ...
>         /* attach each pv */
>         for (i = 0; i < pv_count; i++) {
>                 if (!(pv = pv_by_path(vg->fid->fmt->cmd, pv_names[i]))) {
>                         log_error("%s not identified as an existing "
>                                   "physical volume", pv_names[i]);
>                         goto bad;
>                 }
>                 

I tried that in my first patch and ran into an issue with VG_ORPHAN
lock.  
http://www.redhat.com/archives/lvm-devel/2008-November/msg00075.html

vgextend() obtains VG_ORPHAN, then the lock on the vg before calling
vg_extend().  So if I put pvcreate_devices() logic in there, that calls
down into pvcreate_single(), which also tries to obtain / release
VG_ORPHAN.  I could put logic into pvcreate_single() to check to see if
lock is already obtained (original patch has that) and not try to
reaquire / release VG_ORPHAN (original patch missed release part), but
that looked more hacky than the solution of first calling the logic at
the higher level in the tool.  Would you prefer this first approach and
detecting whether VG_ORPHAN was obtained?

[PATCH] Update vgcreate and vgextend to allow uninitialized devices as input.

[Alasdair G Kergon]

On Mon, Dec 01, 2008 at 09:19:57AM -0500, Dave Wysochanski wrote:
> vgextend() obtains VG_ORPHAN, then the lock on the vg before calling
> vg_extend().  

Makes sense.

> So if I put pvcreate_devices() logic in there, that calls
> down into pvcreate_single(), which also tries to obtain / release
> VG_ORPHAN.  

So you have two options:
1. an extra parameter on the function to say whether or not the lock
is already held
2. require the lock always to be held before calling that function.

Think of VG_ORPHAN as if it was a real volume group - pvcreate is the
same as vgextend on that volume group and should be calling into
the same code and should already hold the VG lock.

But then you see this:
                unlock_vg(cmd, VG_ORPHANS);

                persistent_filter_wipe(cmd->filter);
                lvmcache_destroy(cmd, 1);

                init_md_filtering(0);
                if (!lock_vol(cmd, VG_ORPHANS, LCK_VG_WRITE)) {

and need to worry about *why* the code drops the lock there.

It's probably just for accounting reasons - the cache knows something about
held locks.  IOW (I think) no locks can be held when lvmcache_destroy is
called.  Check that (look at other instances where it's called) then look for
some workaround - a less-drastic destroy function that preserves the state,
perhaps, or splitting the function.

Also note that VG_ORPHANS is set to disappear, replaced by ORPHAN_VG_NAME(fmt).

Alasdair
-- 
agk at redhat.com

[PATCH 1/4] Move guts of pvcreate into library area - prep for vgcreate.

[Dave Wysochanski]

On Mon, 2008-12-01 at 13:59 +0000, Alasdair G Kergon wrote:
> On Sun, Nov 30, 2008 at 09:57:47AM -0500, Dave Wysochanski wrote:
> > On Sun, 2008-11-30 at 04:26 +0000, Alasdair G Kergon wrote:
> > > I'm a bit bothered that too much code may be getting dumped into the library here
> > > wholesale rather than getting broken up into separate calls.
> > > 
> > > - I don't think the library should be prompting, that should remain at tools level
> > > and the interface should allow for that.
> > > -- maybe yes_no_prompt needs some global mode (anaconda would set) to control
> > > whether it does anything or simply defaults to the value given in an additional
> > > parameter 
>  
> > Why global mode - why not just set 'force' and '-y' for the API call?  I
> > think this was similar to Thomas's argument - he always envisioned '-ff'
> > and '-y' on things like pvcreate.
> > To be specific, in pvcreate we prompt if:
> > 1) the device is already a PV
> > 2) we find an MD superblock
> > (and of course the user has not given the '-ff' and '-y')
> 
> To clarify: I mentioned two possible ways to proceed there.
> 
> 1) no prompting in the library (which was my original intention)
> => library calls are split at appropriate places
> In your example above, this would mean separate calls for the validation
> functions (which anaconda would also call) prior to the call
> that actually makes the change (which would not repeat the validation).
> 
I don't see how this is possible.  Are you suggesting we split the
validation into a separate API call(s), and let an app decide to call it
or not?

If you're not saying let the app decide, then if we split the validation
and the create APIs, the only way I can think of ensuring the app has
called the validation before calling the create is to somehow store a
"passed_validation" flag somewhere (maybe within struct cmd_context).
Seems kinda messy though since the "passed_validation" would depend on
the create parameters.

Maybe there is another way but I don't see it.


> 2) prompting within the library, with a guaranteed way for a caller
> to avoid it (e.g. a global setting that defaults to returning with
> an error whenever there would otherwise have been a prompt).
> In the pvcreate example, it would avoid returning with an error because when
> the equivalent of -ff and -y are given there would not be a prompt.
>  
This might be the only reasonable way to proceed.  This would be the
equivalent of a "--no" option without prompting I guess.

[PATCH 1/4] Move guts of pvcreate into library area - prep for vgcreate.

[Alasdair G Kergon]

On Mon, Dec 01, 2008 at 09:26:08PM -0500, Dave Wysochanski wrote:
> I don't see how this is possible.  Are you suggesting we split the
> validation into a separate API call(s), and let an app decide to call it
> or not?
 
There are two sorts of validation.

Optional validation and compulsory validation.

Compulsory validation covers anything that is required for internal LVM
consistency.  This must be built into the library calls and impossible
to bypass.

Optional validation covers everything else: the client of the library
is free to bypass it if it believes it knows what it is doing and
doesn't require it.  Roughly, this is what you get today with -ff -y.

Two current examples from pvcreate:

  Compulsory 
  - The device must not be in use currently (ie we can open it exclusively)

  Optional 
  - PV must not belong to an existing VG (ie we can destroy a VG if want to)

Alasdair
-- 
agk at redhat.com

[PATCH 1/4] Move guts of pvcreate into library area - prep for vgcreate.

[Alasdair G Kergon]

On Mon, Dec 01, 2008 at 09:26:08PM -0500, Dave Wysochanski wrote:
> If you're not saying let the app decide, then if we split the validation
> and the create APIs, the only way I can think of ensuring the app has
> called the validation before calling the create is to somehow store a
> "passed_validation" flag somewhere (maybe within struct cmd_context).
> Seems kinda messy though since the "passed_validation" would depend on
> the create parameters.
> Maybe there is another way but I don't see it.
 
Split the validation into optional validation and compulsory validation.
Offer separate library call(s) for the optional validation, but build
the compulsory validation into the main call(s).

Alasdair
-- 
agk at redhat.com

[PATCH 1/4] Move guts of pvcreate into library area - prep for vgcreate.

[Alasdair G Kergon]

On Tue, Dec 02, 2008 at 02:43:13AM +0000, Alasdair G Kergon wrote:
> Split the validation into optional validation and compulsory validation.
> Offer separate library call(s) for the optional validation, but build
> the compulsory validation into the main call(s).
 
So looking at pvcreate, most of pvcreate_check() is optional and depending
on error handling could be done as one piece or broken into multiple functions.
The few bits of compulsory validation move out of there into the actual 
pvcreate call (which is performed via a vgextend() interface now).

Alasdair
-- 
agk at redhat.com

[PATCH 1/4] Move guts of pvcreate into library area - prep for vgcreate.

[Dave Wysochanski]

On Tue, 2008-12-02 at 02:48 +0000, Alasdair G Kergon wrote:
> On Tue, Dec 02, 2008 at 02:43:13AM +0000, Alasdair G Kergon wrote:
> > Split the validation into optional validation and compulsory validation.
> > Offer separate library call(s) for the optional validation, but build
> > the compulsory validation into the main call(s).
>  
> So looking at pvcreate, most of pvcreate_check() is optional and depending
> on error handling could be done as one piece or broken into multiple functions.
> The few bits of compulsory validation move out of there into the actual 
> pvcreate call (which is performed via a vgextend() interface now).
> 

Ok, I did not understand that we are going to allow an APP the option to
bypass what was in pvcreate_check() (except the exclusive device open
check).