[PATCH 0/7]: Implement lvm_vg_create() and 4 required vg_set*() APIs

[PATCH 0/7]: Implement lvm_vg_create() and 4 required vg_set*() APIs

[Dave Wysochanski]

The following 7 patches implement the beginning of lvm_vg_create() liblvm API.
This API will create a VG with default parameters and return a vg_t.  Later,
if any parameters of the VG need set, they are done with a separate 'set' call.
This is the approach that was discussed for liblvm, and required some refactoring
of the code, including vgcreate and vgsplit.  The reason for the refactoring of
vgcreate and vgsplit is that the existing vg_create() constructor did not
take this approach, but rather took input parameters such as extent_size, max_pv,
max_lv, etc.  Thus, to implement what was decided, I had to pull out all the
validity checks into a separate 'set' function.

In addition, lvm_vg_create() now takes the place of vg_lock_newname() in both
vgcreate and vgsplit.  I believe that down the road we may not have to export
vg_lock_newname() but can use lvm_vg_create() for the existence check, and as
a complement to the vg_read() APIs.  The only remaining use of vg_lock_newname()
is in vgrename, and this use can be hidden inside a vgrename API.

Note that in a few cases, for the 'set' functions, the return codes have been
modified slightly.  The cases are things like EINVALID_CMD_LINE becomes
ECMD_FAILED, and setting the same value returning SUCCESS or FAILURE (this
was inconsistent in the code - sometimes it was an error, sometimes it was
success - I made it always a success).

Passes the testsuite, and should apply cleanly on:
commit ea47a922568e19202ac365b5eb00bf55b4b6d8a2
Author: Dave Wysochanski <dwysocha@redhat.com>
Date:   Mon Jul 6 21:42:25 2009 -0400

    Fix compile warning in lvmcmdline.c - use C99 PRIu64 for uint64_t.

[PATCH 1/7] Update vg_change_pesize() to contain all validity checks.

[Dave Wysochanski]

Update vg_change_pesize() to contain all validity checks.
It would be nice to have one function that does all the validation
and setting of the VG's pesize.  However, currently some checks
are in the higher-level function _vgchange_pesize(), and some
checks are in the lower function vg_change_pesize().
This patch moves most of the higher-level checks inside
vg_change_pesize.  In one case a failure return code is
changed from ECMD_FAILED to EINVALID_CMD_LINE.

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 lib/metadata/metadata.c |   30 ++++++++++++++++++++++++++++++
 tools/vgchange.c        |   27 ++-------------------------
 2 files changed, 32 insertions(+), 25 deletions(-)

diff --git a/lib/metadata/metadata.c b/lib/metadata/metadata.c
index fda07be..c042d60 100644
--- a/lib/metadata/metadata.c
+++ b/lib/metadata/metadata.c
@@ -636,6 +636,36 @@ int vg_change_pesize(struct cmd_context *cmd __attribute((unused)),
 	struct pv_segment *pvseg;
 	uint32_t s;
 
+	if (!(vg_status(vg) & RESIZEABLE_VG)) {
+		log_error("Volume group \"%s\" must be resizeable "
+			  "to change PE size", vg->name);
+		return 0;
+	}
+
+	if (!new_size) {
+		log_error("Physical extent size may not be zero");
+		return 0;
+	}
+
+	if (new_size == vg->extent_size) {
+		log_error("Physical extent size of VG %s is already %s",
+			  vg->name, display_size(vg->cmd, (uint64_t) new_size));
+		return 1;
+	}
+
+	if (new_size & (new_size - 1)) {
+		log_error("Physical extent size must be a power of 2.");
+		return 0;
+	}
+
+	if (new_size > vg->extent_size) {
+		if ((uint64_t) vg->extent_size * vg->extent_count % new_size) {
+			/* FIXME Adjust used PV sizes instead */
+			log_error("New extent size is not a perfect fit");
+			return 0;
+		}
+	}
+
 	vg->extent_size = new_size;
 
 	if (vg->fid->fmt->ops->vg_setup &&
diff --git a/tools/vgchange.c b/tools/vgchange.c
index a5121fc..1609e01 100644
--- a/tools/vgchange.c
+++ b/tools/vgchange.c
@@ -381,48 +381,25 @@ static int _vgchange_pesize(struct cmd_context *cmd, struct volume_group *vg)
 {
 	uint32_t extent_size;
 
-	if (!(vg_status(vg) & RESIZEABLE_VG)) {
-		log_error("Volume group \"%s\" must be resizeable "
-			  "to change PE size", vg->name);
-		return ECMD_FAILED;
-	}
-
 	if (arg_sign_value(cmd, physicalextentsize_ARG, 0) == SIGN_MINUS) {
 		log_error("Physical extent size may not be negative");
 		return EINVALID_CMD_LINE;
 	}
 
 	extent_size = arg_uint_value(cmd, physicalextentsize_ARG, 0);
-	if (!extent_size) {
-		log_error("Physical extent size may not be zero");
-		return EINVALID_CMD_LINE;
-	}
-
+	/* FIXME: remove check - redundant with vg_change_pesize */
 	if (extent_size == vg->extent_size) {
 		log_error("Physical extent size of VG %s is already %s",
 			  vg->name, display_size(cmd, (uint64_t) extent_size));
 		return ECMD_PROCESSED;
 	}
 
-	if (extent_size & (extent_size - 1)) {
-		log_error("Physical extent size must be a power of 2.");
-		return EINVALID_CMD_LINE;
-	}
-
-	if (extent_size > vg->extent_size) {
-		if ((uint64_t) vg->extent_size * vg->extent_count % extent_size) {
-			/* FIXME Adjust used PV sizes instead */
-			log_error("New extent size is not a perfect fit");
-			return EINVALID_CMD_LINE;
-		}
-	}
-
 	if (!archive(vg))
 		return ECMD_FAILED;
 
 	if (!vg_change_pesize(cmd, vg, extent_size)) {
 		stack;
-		return ECMD_FAILED;
+		return EINVALID_CMD_LINE;
 	}
 
 	if (!vg_write(vg) || !vg_commit(vg))
-- 
1.6.0.6

[PATCH 2/7] Remove unused 'cmd' from vg_change_pesize().

[Dave Wysochanski]

Remove unused 'cmd' from vg_change_pesize().

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

diff --git a/lib/metadata/metadata-exported.h b/lib/metadata/metadata-exported.h
index b9e9591..b103518 100644
--- a/lib/metadata/metadata-exported.h
+++ b/lib/metadata/metadata-exported.h
@@ -440,8 +440,7 @@ int vg_remove_single(struct cmd_context *cmd, const char *vg_name,
 int vg_rename(struct cmd_context *cmd, struct volume_group *vg,
 	      const char *new_name);
 int vg_extend(struct volume_group *vg, int pv_count, char **pv_names);
-int vg_change_pesize(struct cmd_context *cmd, struct volume_group *vg,
-		     uint32_t new_extent_size);
+int vg_change_pesize(struct volume_group *vg, uint32_t new_extent_size);
 int vg_split_mdas(struct cmd_context *cmd, struct volume_group *vg_from,
 		  struct volume_group *vg_to);
 
diff --git a/lib/metadata/metadata.c b/lib/metadata/metadata.c
index c042d60..b0c83cc 100644
--- a/lib/metadata/metadata.c
+++ b/lib/metadata/metadata.c
@@ -624,8 +624,7 @@ static int _recalc_extents(uint32_t *extents, const char *desc1,
 	return 1;
 }
 
-int vg_change_pesize(struct cmd_context *cmd __attribute((unused)),
-		     struct volume_group *vg, uint32_t new_size)
+int vg_change_pesize(struct volume_group *vg, uint32_t new_size)
 {
 	uint32_t old_size = vg->extent_size;
 	struct pv_list *pvl;
diff --git a/tools/vgchange.c b/tools/vgchange.c
index 1609e01..ade0735 100644
--- a/tools/vgchange.c
+++ b/tools/vgchange.c
@@ -397,7 +397,7 @@ static int _vgchange_pesize(struct cmd_context *cmd, struct volume_group *vg)
 	if (!archive(vg))
 		return ECMD_FAILED;
 
-	if (!vg_change_pesize(cmd, vg, extent_size)) {
+	if (!vg_change_pesize(vg, extent_size)) {
 		stack;
 		return EINVALID_CMD_LINE;
 	}
-- 
1.6.0.6

[PATCH 3/7] Rename vg_change_pesize to vg_set_extent_size and use vg_t.

[Dave Wysochanski]

Rename vg_change_pesize to vg_set_extent_size and use vg_t.
In liblvm, we will reserve the word 'change' to mean an API that
both sets one or more values, and commits to disk.  This will be
consistent with the LVM commandline.  The existing vg_change_pesize()
function does not commit to disk, but just changes the extent_size
and ensures all internal structures are updated.  This logic should
be contained in a function that sets the value.

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

diff --git a/lib/metadata/metadata-exported.h b/lib/metadata/metadata-exported.h
index b103518..70c24dd 100644
--- a/lib/metadata/metadata-exported.h
+++ b/lib/metadata/metadata-exported.h
@@ -440,7 +440,7 @@ int vg_remove_single(struct cmd_context *cmd, const char *vg_name,
 int vg_rename(struct cmd_context *cmd, struct volume_group *vg,
 	      const char *new_name);
 int vg_extend(struct volume_group *vg, int pv_count, char **pv_names);
-int vg_change_pesize(struct volume_group *vg, uint32_t new_extent_size);
+int vg_set_extent_size(vg_t *vg, uint32_t new_extent_size);
 int vg_split_mdas(struct cmd_context *cmd, struct volume_group *vg_from,
 		  struct volume_group *vg_to);
 
diff --git a/lib/metadata/metadata.c b/lib/metadata/metadata.c
index b0c83cc..2237f83 100644
--- a/lib/metadata/metadata.c
+++ b/lib/metadata/metadata.c
@@ -624,7 +624,7 @@ static int _recalc_extents(uint32_t *extents, const char *desc1,
 	return 1;
 }
 
-int vg_change_pesize(struct volume_group *vg, uint32_t new_size)
+int vg_set_extent_size(vg_t *vg, uint32_t new_size)
 {
 	uint32_t old_size = vg->extent_size;
 	struct pv_list *pvl;
diff --git a/tools/vgchange.c b/tools/vgchange.c
index ade0735..a2b8aa9 100644
--- a/tools/vgchange.c
+++ b/tools/vgchange.c
@@ -397,7 +397,7 @@ static int _vgchange_pesize(struct cmd_context *cmd, struct volume_group *vg)
 	if (!archive(vg))
 		return ECMD_FAILED;
 
-	if (!vg_change_pesize(vg, extent_size)) {
+	if (!vg_set_extent_size(vg, extent_size)) {
 		stack;
 		return EINVALID_CMD_LINE;
 	}
-- 
1.6.0.6

[PATCH 4/7] Add vg_set_max_lv() liblvm function and move vgchange logic inside.

[Dave Wysochanski]

Add vg_set_max_lv() liblvm function and move vgchange logic inside.

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 lib/metadata/metadata-exported.h |    1 +
 lib/metadata/metadata.c          |   28 ++++++++++++++++++++++++++++
 tools/vgchange.c                 |   25 ++-----------------------
 3 files changed, 31 insertions(+), 23 deletions(-)

diff --git a/lib/metadata/metadata-exported.h b/lib/metadata/metadata-exported.h
index 70c24dd..b817f5f 100644
--- a/lib/metadata/metadata-exported.h
+++ b/lib/metadata/metadata-exported.h
@@ -441,6 +441,7 @@ int vg_rename(struct cmd_context *cmd, struct volume_group *vg,
 	      const char *new_name);
 int vg_extend(struct volume_group *vg, int pv_count, char **pv_names);
 int vg_set_extent_size(vg_t *vg, uint32_t new_extent_size);
+int vg_set_max_lv(vg_t *vg, uint32_t max_lv);
 int vg_split_mdas(struct cmd_context *cmd, struct volume_group *vg_from,
 		  struct volume_group *vg_to);
 
diff --git a/lib/metadata/metadata.c b/lib/metadata/metadata.c
index 2237f83..ab9a72b 100644
--- a/lib/metadata/metadata.c
+++ b/lib/metadata/metadata.c
@@ -774,6 +774,34 @@ int vg_set_extent_size(vg_t *vg, uint32_t new_size)
 	return 1;
 }
 
+int vg_set_max_lv(vg_t *vg, uint32_t max_lv)
+{
+	if (!(vg_status(vg) & RESIZEABLE_VG)) {
+		log_error("Volume group \"%s\" must be resizeable "
+			  "to change MaxLogicalVolume", vg->name);
+		return 0;
+	}
+
+	if (!(vg->fid->fmt->features & FMT_UNLIMITED_VOLS)) {
+		if (!max_lv)
+			max_lv = 255;
+		else if (max_lv > 255) {
+			log_error("MaxLogicalVolume limit is 255");
+			return 0;
+		}
+	}
+
+	if (max_lv && max_lv < vg_visible_lvs(vg)) {
+		log_error("MaxLogicalVolume is less than the current number "
+			  "%d of LVs for %s", vg_visible_lvs(vg),
+			  vg->name);
+		return 0;
+	}
+	vg->max_lv = max_lv;
+
+	return 1;
+}
+
 /*
  * Separate metadata areas after splitting a VG.
  * Also accepts orphan VG as destination (for vgreduce).
diff --git a/tools/vgchange.c b/tools/vgchange.c
index a2b8aa9..6bb837e 100644
--- a/tools/vgchange.c
+++ b/tools/vgchange.c
@@ -293,32 +293,11 @@ static int _vgchange_logicalvolume(struct cmd_context *cmd,
 {
 	uint32_t max_lv = arg_uint_value(cmd, logicalvolume_ARG, 0);
 
-	if (!(vg_status(vg) & RESIZEABLE_VG)) {
-		log_error("Volume group \"%s\" must be resizeable "
-			  "to change MaxLogicalVolume", vg->name);
-		return ECMD_FAILED;
-	}
-
-	if (!(vg->fid->fmt->features & FMT_UNLIMITED_VOLS)) {
-		if (!max_lv)
-			max_lv = 255;
-		else if (max_lv > 255) {
-			log_error("MaxLogicalVolume limit is 255");
-			return ECMD_FAILED;
-		}
-	}
-
-	if (max_lv && max_lv < vg_visible_lvs(vg)) {
-		log_error("MaxLogicalVolume is less than the current number "
-			  "%d of LVs for %s", vg_visible_lvs(vg),
-			  vg->name);
-		return ECMD_FAILED;
-	}
-
 	if (!archive(vg))
 		return ECMD_FAILED;
 
-	vg->max_lv = max_lv;
+	if (!vg_set_max_lv(vg, max_lv))
+		return ECMD_FAILED;
 
 	if (!vg_write(vg) || !vg_commit(vg))
 		return ECMD_FAILED;
-- 
1.6.0.6

[PATCH 5/7] Add vg_set_max_pv() liblvm function and move vgchange logic inside.

[Dave Wysochanski]

Add vg_set_max_pv() liblvm function and move vgchange logic inside.

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 lib/metadata/metadata-exported.h |    1 +
 lib/metadata/metadata.c          |   27 +++++++++++++++++++++++++++
 tools/vgchange.c                 |   25 ++-----------------------
 3 files changed, 30 insertions(+), 23 deletions(-)

diff --git a/lib/metadata/metadata-exported.h b/lib/metadata/metadata-exported.h
index b817f5f..c95f5f3 100644
--- a/lib/metadata/metadata-exported.h
+++ b/lib/metadata/metadata-exported.h
@@ -442,6 +442,7 @@ int vg_rename(struct cmd_context *cmd, struct volume_group *vg,
 int vg_extend(struct volume_group *vg, int pv_count, char **pv_names);
 int vg_set_extent_size(vg_t *vg, uint32_t new_extent_size);
 int vg_set_max_lv(vg_t *vg, uint32_t max_lv);
+int vg_set_max_pv(vg_t *vg, uint32_t max_pv);
 int vg_split_mdas(struct cmd_context *cmd, struct volume_group *vg_from,
 		  struct volume_group *vg_to);
 
diff --git a/lib/metadata/metadata.c b/lib/metadata/metadata.c
index ab9a72b..5b563e3 100644
--- a/lib/metadata/metadata.c
+++ b/lib/metadata/metadata.c
@@ -802,6 +802,33 @@ int vg_set_max_lv(vg_t *vg, uint32_t max_lv)
 	return 1;
 }
 
+int vg_set_max_pv(vg_t *vg, uint32_t max_pv)
+{
+	if (!(vg_status(vg) & RESIZEABLE_VG)) {
+		log_error("Volume group \"%s\" must be resizeable "
+			  "to change MaxPhysicalVolumes", vg->name);
+		return 0;
+	}
+
+	if (!(vg->fid->fmt->features & FMT_UNLIMITED_VOLS)) {
+		if (!max_pv)
+			max_pv = 255;
+		else if (max_pv > 255) {
+			log_error("MaxPhysicalVolume limit is 255");
+			return 0;
+		}
+	}
+
+	if (max_pv && max_pv < vg->pv_count) {
+		log_error("MaxPhysicalVolumes is less than the current number "
+			  "%d of PVs for \"%s\"", vg->pv_count,
+			  vg->name);
+		return 0;
+	}
+	vg->max_pv = max_pv;
+	return 1;
+}
+
 /*
  * Separate metadata areas after splitting a VG.
  * Also accepts orphan VG as destination (for vgreduce).
diff --git a/tools/vgchange.c b/tools/vgchange.c
index 6bb837e..a095d9e 100644
--- a/tools/vgchange.c
+++ b/tools/vgchange.c
@@ -314,37 +314,16 @@ static int _vgchange_physicalvolumes(struct cmd_context *cmd,
 {
 	uint32_t max_pv = arg_uint_value(cmd, maxphysicalvolumes_ARG, 0);
 
-	if (!(vg_status(vg) & RESIZEABLE_VG)) {
-		log_error("Volume group \"%s\" must be resizeable "
-			  "to change MaxPhysicalVolumes", vg->name);
-		return ECMD_FAILED;
-	}
-
 	if (arg_sign_value(cmd, maxphysicalvolumes_ARG, 0) == SIGN_MINUS) {
 		log_error("MaxPhysicalVolumes may not be negative");
 		return EINVALID_CMD_LINE;
 	}
 
-	if (!(vg->fid->fmt->features & FMT_UNLIMITED_VOLS)) {
-		if (!max_pv)
-			max_pv = 255;
-		else if (max_pv > 255) {
-			log_error("MaxPhysicalVolume limit is 255");
-			return ECMD_FAILED;
-		}
-	}
-
-	if (max_pv && max_pv < vg->pv_count) {
-		log_error("MaxPhysicalVolumes is less than the current number "
-			  "%d of PVs for \"%s\"", vg->pv_count,
-			  vg->name);
-		return ECMD_FAILED;
-	}
-
 	if (!archive(vg))
 		return ECMD_FAILED;
 
-	vg->max_pv = max_pv;
+	if (!vg_set_max_pv(vg, max_pv))
+		return ECMD_FAILED;
 
 	if (!vg_write(vg) || !vg_commit(vg))
 		return ECMD_FAILED;
-- 
1.6.0.6

[PATCH 6/7] Add vg_set_alloc() liblvm function and move vgchange logic inside.

[Dave Wysochanski]

Add vg_set_alloc() liblvm function and move vgchange logic inside.
NOTE: We also change a failure to success in one case - setting
an alloc value that is the same as the current value.  This should
not be an error, and is inconsistent throughout the code (see
vg_set_extent_size)

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 lib/metadata/metadata-exported.h |    1 +
 lib/metadata/metadata.c          |   18 ++++++++++++++++++
 tools/vgchange.c                 |   15 ++-------------
 3 files changed, 21 insertions(+), 13 deletions(-)

diff --git a/lib/metadata/metadata-exported.h b/lib/metadata/metadata-exported.h
index c95f5f3..1b7eee3 100644
--- a/lib/metadata/metadata-exported.h
+++ b/lib/metadata/metadata-exported.h
@@ -443,6 +443,7 @@ int vg_extend(struct volume_group *vg, int pv_count, char **pv_names);
 int vg_set_extent_size(vg_t *vg, uint32_t new_extent_size);
 int vg_set_max_lv(vg_t *vg, uint32_t max_lv);
 int vg_set_max_pv(vg_t *vg, uint32_t max_pv);
+int vg_set_alloc(vg_t *vg, alloc_policy_t alloc);
 int vg_split_mdas(struct cmd_context *cmd, struct volume_group *vg_from,
 		  struct volume_group *vg_to);
 
diff --git a/lib/metadata/metadata.c b/lib/metadata/metadata.c
index 5b563e3..077159b 100644
--- a/lib/metadata/metadata.c
+++ b/lib/metadata/metadata.c
@@ -829,6 +829,24 @@ int vg_set_max_pv(vg_t *vg, uint32_t max_pv)
 	return 1;
 }
 
+int vg_set_alloc(vg_t *vg, alloc_policy_t alloc)
+{
+	if (alloc == ALLOC_INHERIT) {
+		log_error("Volume Group allocation policy cannot inherit "
+			  "from anything");
+		return 0;
+	}
+
+	if (alloc == vg->alloc) {
+		log_error("Volume group allocation policy is already %s",
+			  get_alloc_string(vg->alloc));
+		return 1;
+	}
+	vg->alloc = alloc;
+	return 1;
+}
+
+
 /*
  * Separate metadata areas after splitting a VG.
  * Also accepts orphan VG as destination (for vgreduce).
diff --git a/tools/vgchange.c b/tools/vgchange.c
index a095d9e..f99290a 100644
--- a/tools/vgchange.c
+++ b/tools/vgchange.c
@@ -179,22 +179,11 @@ static int _vgchange_alloc(struct cmd_context *cmd, struct volume_group *vg)
 
 	alloc = arg_uint_value(cmd, alloc_ARG, ALLOC_NORMAL);
 
-	if (alloc == ALLOC_INHERIT) {
-		log_error("Volume Group allocation policy cannot inherit "
-			  "from anything");
-		return EINVALID_CMD_LINE;
-	}
-
-	if (alloc == vg->alloc) {
-		log_error("Volume group allocation policy is already %s",
-			  get_alloc_string(vg->alloc));
-		return ECMD_FAILED;
-	}
-
 	if (!archive(vg))
 		return ECMD_FAILED;
 
-	vg->alloc = alloc;
+	if (!vg_set_alloc(vg, alloc))
+		return ECMD_FAILED;
 
 	if (!vg_write(vg) || !vg_commit(vg))
 		return ECMD_FAILED;
-- 
1.6.0.6

[PATCH 7/7] Add lvm_vg_create() - vg constructor API for new volume groups.

[Dave Wysochanski]

vg_t *lvm_vg_create(struct cmd_context *cmd, const char *vg_name);
This is the first step towards the API called to create a VG.
Call vg_lock_newname() inside this function.  Use _vg_make_handle()
where possible.
Now we have 2 ways to construct a volume group:
1) vg_read* APIs: Used when constructing an existing VG from disks
2) lvm_vg_create: Used when constructing a new VG
Both of these interfaces obtain a lock, and return a vg_t *.
The usage of _vg_make_handle() inside lvm_vg_create() doesn't fit
perfectly but it's ok for now.  Needs some cleanup though and I've
noted "FIXME" in the code.

Replace vg_create() with lvm_vg_create() plus vg 'set' functions
in the following tools:
- vgcreate: Fairly straightforward refactoring.  We just moved
vg_lock_newname inside lvm_vg_create so we check the return via
vg_read_error.
- vgsplit: The refactoring here is a bit more tricky.  Originally
we called vg_lock_newname and depending on the error code, we either
read the existing vg or created the new one.  Now lvm_vg_create()
calls vg_lock_newname, so we first try to create the VG.  If this
fails with FAILED_EXIST, we can then do the vg_read.  If the
create succeeds, we check the input parameters and set any new
values on the VG.

TODO in future patches:
1. The VG_ORPHAN lock needs some thought.  We may want to treat
this as any other VG, and require the application to obtain a handle
and pass it to other API calls (for example, vg_extend).  Or,
we may find that hiding the VG_ORPHAN lock inside other APIs is
the way to go.  I thought of placing the VG_ORPHAN lock inside
lvm_vg_create() and tying it to the vg handle, but was not certain
this was the right approach.
2. Cleanup error paths. Integrate vg_read_error() with lvm_vg_create and
vg_read* error codes.

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 lib/metadata/metadata-exported.h |    5 +--
 lib/metadata/metadata.c          |   63 +++++++++++++++++++++++++------------
 tools/vgcreate.c                 |   24 ++++++++------
 tools/vgsplit.c                  |   33 +++++++++++---------
 4 files changed, 75 insertions(+), 50 deletions(-)

diff --git a/lib/metadata/metadata-exported.h b/lib/metadata/metadata-exported.h
index 1b7eee3..3eae89b 100644
--- a/lib/metadata/metadata-exported.h
+++ b/lib/metadata/metadata-exported.h
@@ -429,10 +429,7 @@ int pv_analyze(struct cmd_context *cmd, const char *pv_name,
 /* FIXME: move internal to library */
 uint32_t pv_list_extents_free(const struct dm_list *pvh);
 
-struct volume_group *vg_create(struct cmd_context *cmd, const char *name,
-			       uint32_t extent_size, uint32_t max_pv,
-			       uint32_t max_lv, alloc_policy_t alloc,
-			       int pv_count, char **pv_names);
+vg_t *lvm_vg_create(struct cmd_context *cmd, const char *vg_name);
 int vg_remove(struct volume_group *vg);
 int vg_remove_single(struct cmd_context *cmd, const char *vg_name,
 		     struct volume_group *vg,
diff --git a/lib/metadata/metadata.c b/lib/metadata/metadata.c
index 077159b..9939ac3 100644
--- a/lib/metadata/metadata.c
+++ b/lib/metadata/metadata.c
@@ -1,6 +1,6 @@
 /*
  * Copyright (C) 2001-2004 Sistina Software, Inc. All rights reserved.
- * Copyright (C) 2004-2007 Red Hat, Inc. All rights reserved.
+ * Copyright (C) 2004-2009 Red Hat, Inc. All rights reserved.
  *
  * This file is part of LVM2.
  *
@@ -67,6 +67,10 @@ static struct pv_list *_find_pv_in_vg(const struct volume_group *vg,
 static struct physical_volume *_find_pv_in_vg_by_uuid(const struct volume_group *vg,
 						      const struct id *id);
 
+static vg_t *_vg_make_handle(struct cmd_context *cmd,
+			     struct volume_group *vg,
+			     uint32_t failure);
+
 unsigned long set_pe_align(struct physical_volume *pv, unsigned long data_alignment)
 {
 	if (pv->pe_align)
@@ -515,24 +519,44 @@ int validate_vg_create_params(struct cmd_context *cmd,
 	return 0;
 }
 
-struct volume_group *vg_create(struct cmd_context *cmd, const char *vg_name,
-			       uint32_t extent_size, uint32_t max_pv,
-			       uint32_t max_lv, alloc_policy_t alloc,
-			       int pv_count, char **pv_names)
+/*
+ * Create a VG with default parameters.
+ * Returns:
+ * - vg_t* with SUCCESS code: VG structure created
+ * - NULL or vg_t* with FAILED_* code: error creating VG structure
+ * Use vg_read_error() to determine success or failure.
+ * FIXME: cleanup usage of _vg_make_handle()
+ */
+vg_t *lvm_vg_create(struct cmd_context *cmd, const char *vg_name)
 {
-	struct volume_group *vg;
+	vg_t *vg;
 	int consistent = 0;
 	struct dm_pool *mem;
+	uint32_t rc;
 
+	if (!validate_name(vg_name)) {
+		log_error("Invalid vg name %s", vg_name);
+		/* FIXME: use _vg_make_handle() w/proper error code */
+		return NULL;
+	}
+
+	rc = vg_lock_newname(cmd, vg_name);
+	if (rc != SUCCESS) {
+		log_error("Unable to reserve vg name %s", vg_name);
+		return _vg_make_handle(cmd, NULL, rc);
+	}
+
+	/* FIXME: Is this vg_read_internal necessary? Move it inside
+	   vg_lock_newname? */
 	/* is this vg name already in use ? */
 	if ((vg = vg_read_internal(cmd, vg_name, NULL, &consistent))) {
 		log_err("A volume group called '%s' already exists.", vg_name);
-		vg_release(vg);
-		return NULL;
+		unlock_and_release_vg(cmd, vg, vg_name);
+		return _vg_make_handle(cmd, NULL, FAILED_EXIST);
 	}
 
 	if (!(mem = dm_pool_create("lvm2 vg_create", VG_MEMPOOL_CHUNK)))
-		return_NULL;
+		goto_bad;
 
 	if (!(vg = dm_pool_zalloc(mem, sizeof(*vg))))
 		goto_bad;
@@ -559,14 +583,14 @@ struct volume_group *vg_create(struct cmd_context *cmd, const char *vg_name,
 
 	*vg->system_id = '\0';
 
-	vg->extent_size = extent_size;
+	vg->extent_size = DEFAULT_EXTENT_SIZE * 2;
 	vg->extent_count = 0;
 	vg->free_count = 0;
 
-	vg->max_lv = max_lv;
-	vg->max_pv = max_pv;
+	vg->max_lv = 0;
+	vg->max_pv = 0;
 
-	vg->alloc = alloc;
+	vg->alloc = ALLOC_NORMAL;
 
 	vg->pv_count = 0;
 	dm_list_init(&vg->pvs);
@@ -587,15 +611,11 @@ struct volume_group *vg_create(struct cmd_context *cmd, const char *vg_name,
 			  vg_name);
 		goto bad;
 	}
+	return _vg_make_handle(cmd, vg, SUCCESS);
 
-	/* attach the pv's */
-	if (!vg_extend(vg, pv_count, pv_names))
-		goto_bad;
-
-	return vg;
-
-      bad:
-	dm_pool_destroy(mem);
+bad:
+	unlock_and_release_vg(cmd, vg, vg_name);
+	/* FIXME: use _vg_make_handle() w/proper error code */
 	return NULL;
 }
 
@@ -2779,6 +2799,7 @@ static vg_t *_vg_make_handle(struct cmd_context *cmd,
 			return_NULL;
 		}
 		vg->vgmem = vgmem;
+		vg->cmd = cmd;
 	}
 
 	vg->read_status = failure;
diff --git a/tools/vgcreate.c b/tools/vgcreate.c
index 232b570..edc8c62 100644
--- a/tools/vgcreate.c
+++ b/tools/vgcreate.c
@@ -46,22 +46,26 @@ int vgcreate(struct cmd_context *cmd, int argc, char **argv)
 	if (validate_vg_create_params(cmd, &vp_new))
 	    return EINVALID_CMD_LINE;
 
+	/* FIXME: orphan lock needs tied to vg handle or inside library call */
 	if (!lock_vol(cmd, VG_ORPHANS, LCK_VG_WRITE)) {
 		log_error("Can't get lock for orphan PVs");
 		return ECMD_FAILED;
 	}
 
-	if (vg_lock_newname(cmd, vp_new.vg_name) != SUCCESS) {
-		log_error("Can't get lock for %s", vp_new.vg_name);
-		unlock_vg(cmd, VG_ORPHANS);
-		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,
-			     argc - 1, argv + 1)))
-		goto bad;
+	vg = lvm_vg_create(cmd, vp_new.vg_name);
+	if (vg_read_error(vg))
+		goto_bad;
+
+	if (!vg_set_extent_size(vg, vp_new.extent_size) ||
+	    !vg_set_max_lv(vg, vp_new.max_lv) ||
+	    !vg_set_max_pv(vg, vp_new.max_pv) ||
+	    !vg_set_alloc(vg, vp_new.alloc))
+		goto_bad;
+
+	/* attach the pv's */
+	if (!vg_extend(vg, argc - 1, argv + 1))
+		goto_bad;
 
 	if (vp_new.max_lv != vg->max_lv)
 		log_warn("WARNING: Setting maxlogicalvolumes to %d "
diff --git a/tools/vgsplit.c b/tools/vgsplit.c
index 9cd90b8..1bbda29 100644
--- a/tools/vgsplit.c
+++ b/tools/vgsplit.c
@@ -284,7 +284,6 @@ int vgsplit(struct cmd_context *cmd, int argc, char **argv)
 	int existing_vg = 0;
 	int r = ECMD_FAILED;
 	const char *lv_name;
-	uint32_t rc;
 
 	if ((arg_count(cmd, name_ARG) + argc) < 3) {
 		log_error("Existing VG, new VG and either physical volumes "
@@ -322,23 +321,28 @@ int vgsplit(struct cmd_context *cmd, int argc, char **argv)
 		return ECMD_FAILED;
 	}
 
+	/* Set metadata format of original VG */
+	/* FIXME: need some common logic */
+	cmd->fmt = vg_from->fid->fmt;
+
 	log_verbose("Checking for new volume group \"%s\"", vg_name_to);
 	/*
-	 * Try to lock the name of the new VG.  If we cannot reserve it,
-	 * then we assume it exists, and we will not be holding a lock.
-	 * We then try to read it - the vgsplit will be into an existing VG.
+	 * First try to create a new VG.  If we cannot create it,
+	 * and we get FAILED_EXIST (we will not be holding a lock),
+	 * a VG must already exist with this name.  We then try to
+	 * read the existing VG - the vgsplit will be into an existing VG.
 	 *
 	 * Otherwise, if the lock was successful, it must be the case that
 	 * we obtained a WRITE lock and could not find the vgname in the
 	 * system.  Thus, the split will be into a new VG.
 	 */
-	rc = vg_lock_newname(cmd, vg_name_to);
-	if (rc == FAILED_LOCKING) {
+	vg_to = lvm_vg_create(cmd, vg_name_to);
+	if (vg_read_error(vg_to) == FAILED_LOCKING) {
 		log_error("Can't get lock for %s", vg_name_to);
 		unlock_and_release_vg(cmd, vg_from, vg_name_from);
 		return ECMD_FAILED;
 	}
-	if (rc == FAILED_EXIST) {
+	if (vg_read_error(vg_to) == FAILED_EXIST) {
 		existing_vg = 1;
 		vg_to = vg_read_for_update(cmd, vg_name_to, NULL,
 					   READ_REQUIRE_RESIZEABLE |
@@ -355,13 +359,9 @@ int vgsplit(struct cmd_context *cmd, int argc, char **argv)
 		}
 		if (!vgs_are_compatible(cmd, vg_from,vg_to))
 			goto_bad;
-	} else if (rc == SUCCESS) {
+	} else if (vg_read_error(vg_to) == SUCCESS) {
 		existing_vg = 0;
 
-		/* Set metadata format of original VG */
-		/* FIXME: need some common logic */
-		cmd->fmt = vg_from->fid->fmt;
-
 		vp_def.vg_name = NULL;
 		vp_def.extent_size = vg_from->extent_size;
 		vp_def.max_pv = vg_from->max_pv;
@@ -379,9 +379,10 @@ int vgsplit(struct cmd_context *cmd, int argc, char **argv)
 			goto bad;
 		}
 
-		if (!(vg_to = vg_create(cmd, vg_name_to, vp_new.extent_size,
-					vp_new.max_pv, vp_new.max_lv,
-					vp_new.alloc, 0, NULL)))
+		if (!vg_set_extent_size(vg_to, vp_new.extent_size) ||
+		    !vg_set_max_lv(vg_to, vp_new.max_lv) ||
+		    !vg_set_max_pv(vg_to, vp_new.max_pv) ||
+		    !vg_set_alloc(vg_to, vp_new.alloc))
 			goto_bad;
 
 		if (vg_is_clustered(vg_from))
@@ -460,6 +461,8 @@ int vgsplit(struct cmd_context *cmd, int argc, char **argv)
 	 * Finally, remove the EXPORTED flag from the new VG and write it out.
 	 */
 	if (!test_mode()) {
+		/* FIXME: why are we reading again ?*/
+		vg_release(vg_to);
 		vg_to = vg_read_for_update(cmd, vg_name_to, NULL,
 					   READ_ALLOW_EXPORTED);
 		if (vg_read_error(vg_to)) {
-- 
1.6.0.6

[PATCH 7/7] Add lvm_vg_create() - vg constructor API for new volume groups.

[Milan Broz]

Dave Wysochanski wrote:
> vg_t *lvm_vg_create(struct cmd_context *cmd, const char *vg_name);
> This is the first step towards the API called to create a VG.
> Call vg_lock_newname() inside this function.  Use _vg_make_handle()
> where possible.

There is too many changes in this patch...

> Now we have 2 ways to construct a volume group:
> 1) vg_read* APIs: Used when constructing an existing VG from disks
> 2) lvm_vg_create: Used when constructing a new VG
> Both of these interfaces obtain a lock, and return a vg_t *.
> The usage of _vg_make_handle() inside lvm_vg_create() doesn't fit
> perfectly but it's ok for now.  Needs some cleanup though and I've
> noted "FIXME" in the code.

so no real difference from vg_read_* + vg_create, right?
These functions just returns vg_t instead of vg now...

> Replace vg_create() with lvm_vg_create() plus vg 'set' functions
> in the following tools:

so all lib functions will use lvm_* prefix?


> TODO in future patches:
> 1. The VG_ORPHAN lock needs some thought.  We may want to treat
> this as any other VG, and require the application to obtain a handle
> and pass it to other API calls (for example, vg_extend).  Or,
> we may find that hiding the VG_ORPHAN lock inside other APIs is
> the way to go.  I thought of placing the VG_ORPHAN lock inside
> lvm_vg_create() and tying it to the vg handle, but was not certain
> this was the right approach.

I thought that we will want to deprecate orphan group, but it still
need to be supported (old PVs without VG)...

But I think that application using liblvm
should never use separate handle to VG_ORPHAN.

> +vg_t *lvm_vg_create(struct cmd_context *cmd, const char *vg_name)
>  {
> -	struct volume_group *vg;
> +	vg_t *vg;
>  	int consistent = 0;
>  	struct dm_pool *mem;
> +	uint32_t rc;
>  
> +	if (!validate_name(vg_name)) {

I expect that this call is just moved here from ... ?

> +		log_error("Invalid vg name %s", vg_name);
> +		/* FIXME: use _vg_make_handle() w/proper error code */
> +		return NULL;
> +	}
> +
> +	rc = vg_lock_newname(cmd, vg_name);
> +	if (rc != SUCCESS) {
> +		log_error("Unable to reserve vg name %s", vg_name);

- log_error("Can't get lock for %s", vp_new.vg_name);

"reserve" ? it means that vg already exist?
(both messages are confusing to user imho)

> +		return _vg_make_handle(cmd, NULL, rc);
> +	}
> +
> +	/* FIXME: Is this vg_read_internal necessary? Move it inside
> +	   vg_lock_newname? */
>  	/* is this vg name already in use ? */
>  	if ((vg = vg_read_internal(cmd, vg_name, NULL, &consistent))) {
>  		log_err("A volume group called '%s' already exists.", vg_name);

btw this should be log_error

> -		vg_release(vg);
> -		return NULL;
> +		unlock_and_release_vg(cmd, vg, vg_name);
> +		return _vg_make_handle(cmd, NULL, FAILED_EXIST);

so now it calls unlock too... it is bugfix for some previous change?

> @@ -559,14 +583,14 @@ struct volume_group *vg_create(struct cmd_context *cmd, const char *vg_name,
>  
>  	*vg->system_id = '\0';
>  
> -	vg->extent_size = extent_size;
> +	vg->extent_size = DEFAULT_EXTENT_SIZE * 2;

what these forced defaults mean here? just to overwrite them later?
can we read them form command context instead of fixed values?

(not that user can change policy using --alloc for example)

>  	vg->extent_count = 0;
>  	vg->free_count = 0;
>  
> -	vg->max_lv = max_lv;
> -	vg->max_pv = max_pv;
> +	vg->max_lv = 0;
> +	vg->max_pv = 0;

?

>  
> -	vg->alloc = alloc;
> +	vg->alloc = ALLOC_NORMAL;

?

> @@ -2779,6 +2799,7 @@ static vg_t *_vg_make_handle(struct cmd_context *cmd,
>  			return_NULL;
>  		}
>  		vg->vgmem = vgmem;
> +		vg->cmd = cmd;

another bugfix?

> @@ -322,23 +321,28 @@ int vgsplit(struct cmd_context *cmd, int argc, char **argv)
>  		return ECMD_FAILED;
>  	}
>  
> +	/* Set metadata format of original VG */
> +	/* FIXME: need some common logic */
> +	cmd->fmt = vg_from->fid->fmt;
> +

and what happens after vg_release(vg_from) ? cmd->fmt will be undefined?
(this is probably bug in current code though)

> +	vg_to = lvm_vg_create(cmd, vg_name_to);
> +	if (vg_read_error(vg_to) == FAILED_LOCKING) {
...
> +	if (vg_read_error(vg_to) == FAILED_EXIST) {
...
> +	} else if (vg_read_error(vg_to) == SUCCESS) {
...
> +		if (!vg_set_extent_size(vg_to, vp_new.extent_size) ||
> +		    !vg_set_max_lv(vg_to, vp_new.max_lv) ||
> +		    !vg_set_max_pv(vg_to, vp_new.max_pv) ||
> +		    !vg_set_alloc(vg_to, vp_new.alloc))
>  			goto_bad;

I like the old way much more better...

> -		if (!(vg_to = vg_create(cmd, vg_name_to, vp_new.extent_size,
> -					vp_new.max_pv, vp_new.max_lv,
> -					vp_new.alloc, 0, NULL)))


Milan

[PATCH 7/7] Add lvm_vg_create() - vg constructor API for new volume groups.

[Dave Wysochanski]

On Tue, 2009-07-07 at 12:34 +0200, Milan Broz wrote:
> Dave Wysochanski wrote:
> > vg_t *lvm_vg_create(struct cmd_context *cmd, const char *vg_name);
> > This is the first step towards the API called to create a VG.
> > Call vg_lock_newname() inside this function.  Use _vg_make_handle()
> > where possible.
> 
> There is too many changes in this patch...
> 
Ok, I will try to explain better and split if necessary.

> > Now we have 2 ways to construct a volume group:
> > 1) vg_read* APIs: Used when constructing an existing VG from disks
> > 2) lvm_vg_create: Used when constructing a new VG
> > Both of these interfaces obtain a lock, and return a vg_t *.
> > The usage of _vg_make_handle() inside lvm_vg_create() doesn't fit
> > perfectly but it's ok for now.  Needs some cleanup though and I've
> > noted "FIXME" in the code.
> 
> so no real difference from vg_read_* + vg_create, right?
> These functions just returns vg_t instead of vg now...
> 

Main difference is the original vg_create() took the initial vg
parameters.  In our discussions in Berlin about liblvm, we decided we
wanted a constructor with default parameters.  Now all along there has
been the requirement that the tools call the library.  So I decided to
rework the two places with this new constructor.  Because of this, there
are subtleties that I had to fix.  As you point out

> > Replace vg_create() with lvm_vg_create() plus vg 'set' functions
> > in the following tools:
> 
> so all lib functions will use lvm_* prefix?
> 

Anything we export has to - just as libdevmapper prefixes with dm_*.

> 
> > TODO in future patches:
> > 1. The VG_ORPHAN lock needs some thought.  We may want to treat
> > this as any other VG, and require the application to obtain a handle
> > and pass it to other API calls (for example, vg_extend).  Or,
> > we may find that hiding the VG_ORPHAN lock inside other APIs is
> > the way to go.  I thought of placing the VG_ORPHAN lock inside
> > lvm_vg_create() and tying it to the vg handle, but was not certain
> > this was the right approach.
> 
> I thought that we will want to deprecate orphan group, but it still
> need to be supported (old PVs without VG)...
> 
> But I think that application using liblvm
> should never use separate handle to VG_ORPHAN.
> 

That's what I thought too - it should be managed under the covers
somehow.


> > +vg_t *lvm_vg_create(struct cmd_context *cmd, const char *vg_name)
> >  {
> > -	struct volume_group *vg;
> > +	vg_t *vg;
> >  	int consistent = 0;
> >  	struct dm_pool *mem;
> > +	uint32_t rc;
> >  
> > +	if (!validate_name(vg_name)) {
> 
> I expect that this call is just moved here from ... ?
> 

Yes - you need to validate the name if you're going to export the
function.  The original vg_create() was called from tools who had
already validated the name, so no name validation was necessary.  We
cannot assume that an application has done that though.

> > +		log_error("Invalid vg name %s", vg_name);
> > +		/* FIXME: use _vg_make_handle() w/proper error code */
> > +		return NULL;
> > +	}
> > +
> > +	rc = vg_lock_newname(cmd, vg_name);
> > +	if (rc != SUCCESS) {
> > +		log_error("Unable to reserve vg name %s", vg_name);
> 
> - log_error("Can't get lock for %s", vp_new.vg_name);
> 
> "reserve" ? it means that vg already exist?
> (both messages are confusing to user imho)
> 

I could check for each error code (FAILED_LOCKING and FAILED_EXIST) and
print a specific message as is done in other places.


> > +		return _vg_make_handle(cmd, NULL, rc);
> > +	}
> > +
> > +	/* FIXME: Is this vg_read_internal necessary? Move it inside
> > +	   vg_lock_newname? */
> >  	/* is this vg name already in use ? */
> >  	if ((vg = vg_read_internal(cmd, vg_name, NULL, &consistent))) {
> >  		log_err("A volume group called '%s' already exists.", vg_name);
> 
> btw this should be log_error
> 

Right - I think that was an existing bug.

> > -		vg_release(vg);
> > -		return NULL;
> > +		unlock_and_release_vg(cmd, vg, vg_name);
> > +		return _vg_make_handle(cmd, NULL, FAILED_EXIST);
> 
> so now it calls unlock too... it is bugfix for some previous change?
> 

No.  Remember I am defining a new function.  If it fails I release the
lock.  The lock has been obtained earlier in vg_lock_newname() so I must
release it.

> > @@ -559,14 +583,14 @@ struct volume_group *vg_create(struct cmd_context *cmd, const char *vg_name,
> >  
> >  	*vg->system_id = '\0';
> >  
> > -	vg->extent_size = extent_size;
> > +	vg->extent_size = DEFAULT_EXTENT_SIZE * 2;
> 
> what these forced defaults mean here? just to overwrite them later?
> can we read them form command context instead of fixed values?
> 
> (not that user can change policy using --alloc for example)
> 

We could store them in cmd context.  Forced defaults should just be the
same as if the user did not give any values for them.

> >  	vg->extent_count = 0;
> >  	vg->free_count = 0;
> >  
> > -	vg->max_lv = max_lv;
> > -	vg->max_pv = max_pv;
> > +	vg->max_lv = 0;
> > +	vg->max_pv = 0;
> 
> ?
> 
> >  
> > -	vg->alloc = alloc;
> > +	vg->alloc = ALLOC_NORMAL;
> 
> ?
> 

These defaults were taken from the initialization code - when we're
parsing ARGS and getting default values if there are no arguments.


> > @@ -2779,6 +2799,7 @@ static vg_t *_vg_make_handle(struct cmd_context *cmd,
> >  			return_NULL;
> >  		}
> >  		vg->vgmem = vgmem;
> > +		vg->cmd = cmd;
> 
> another bugfix?
> 

Yes though could be solely the result of this refactoring, can't
remember.

> > @@ -322,23 +321,28 @@ int vgsplit(struct cmd_context *cmd, int argc, char **argv)
> >  		return ECMD_FAILED;
> >  	}
> >  
> > +	/* Set metadata format of original VG */
> > +	/* FIXME: need some common logic */
> > +	cmd->fmt = vg_from->fid->fmt;
> > +
> 
> and what happens after vg_release(vg_from) ? cmd->fmt will be undefined?
> (this is probably bug in current code though)
> 

This should never happen AFAIK.  cmd->fmt is set early on, and there is
a default.


> > +	vg_to = lvm_vg_create(cmd, vg_name_to);
> > +	if (vg_read_error(vg_to) == FAILED_LOCKING) {
> ...
> > +	if (vg_read_error(vg_to) == FAILED_EXIST) {
> ...
> > +	} else if (vg_read_error(vg_to) == SUCCESS) {
> ...
> > +		if (!vg_set_extent_size(vg_to, vp_new.extent_size) ||
> > +		    !vg_set_max_lv(vg_to, vp_new.max_lv) ||
> > +		    !vg_set_max_pv(vg_to, vp_new.max_pv) ||
> > +		    !vg_set_alloc(vg_to, vp_new.alloc))
> >  			goto_bad;
> 
> I like the old way much more better...
> 

So if we keep the old way, then I have a liblvm function that has a
default constructor and none of the below parameters.  So the existing
tools code cannot call it.

> > -		if (!(vg_to = vg_create(cmd, vg_name_to, vp_new.extent_size,
> > -					vp_new.max_pv, vp_new.max_lv,
> > -					vp_new.alloc, 0, NULL)))
> 
> 
> Milan
> 
> --
> lvm-devel mailing list
> lvm-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/lvm-devel

[PATCH 0/7]: Implement lvm_vg_create() and 4 required vg_set*() APIs

[Alasdair G Kergon]

On Tue, Jul 07, 2009 at 03:50:01AM -0400, Dave Wysochanski wrote:
> The following 7 patches implement the beginning of lvm_vg_create() liblvm API.
> This API will create a VG with default parameters and return a vg_t.  Later,
> if any parameters of the VG need set, they are done with a separate 'set' call.

The distinction we made at LinuxTag was between parameters that would normally
need to be set (like the name of the VG) and parameters that normally do not need
to be (like max_lv).  (This is judged from the perspective of a typical library
user - not necessarily the lvm2 tools themselves.)

Alasdair

[PATCH 6/7] Add vg_set_alloc() liblvm function and move vgchange logic inside.

[Alasdair G Kergon]

On Tue, Jul 07, 2009 at 03:50:07AM -0400, Dave Wysochanski wrote:
> Add vg_set_alloc() liblvm function and move vgchange logic inside.

For the interface, we should probably say 'set_alloc_policy' rather
than just 'set_alloc'.

> +	if (alloc == vg->alloc) {
> +		log_error("Volume group allocation policy is already %s",
> +			  get_alloc_string(vg->alloc));
> +		return 1;
> +	}

It's back to 'change' vs. 'set' again:-)
If this input is acceptable now, either go for WARNING like we do in other
places where we proceed, or alternatively just reduce the message to
log_verbose if you don't consider it important.

Alasdair

[PATCH 7/7] Add lvm_vg_create() - vg constructor API for new volume groups.

[Alasdair G Kergon]

On Tue, Jul 07, 2009 at 03:50:08AM -0400, Dave Wysochanski wrote:
> 1. The VG_ORPHAN lock needs some thought.  

Outside the scope of the new library interface: it will not need to
deal with orphans, as discussed at LinuxTag.

Alasdair

[PATCH 7/7] Add lvm_vg_create() - vg constructor API for new volume groups.

[Alasdair G Kergon]

On Tue, Jul 07, 2009 at 12:34:27PM +0200, Milan Broz wrote:
> > +		if (!vg_set_extent_size(vg_to, vp_new.extent_size) ||
> > +		    !vg_set_max_lv(vg_to, vp_new.max_lv) ||
> > +		    !vg_set_max_pv(vg_to, vp_new.max_pv) ||
> > +		    !vg_set_alloc(vg_to, vp_new.alloc))
> >  			goto_bad;
 
> I like the old way much more better...
 
Applications using the defaults won't need to call any of those, and
eventually lvm2 tools should call them while processing any
corresponding cmdline args where non-default values are requested.

Alasdair

[PATCH 7/7] Add lvm_vg_create() - vg constructor API for new volume groups.

[Alasdair G Kergon]

On Tue, Jul 07, 2009 at 03:50:08AM -0400, Dave Wysochanski wrote:
> -	vg->extent_size = extent_size;
> +	vg->extent_size = DEFAULT_EXTENT_SIZE * 2;

I suggest introducing similar DEFAULT_* for other settable parameters.

Alasdair

[PATCH 0/7]: Implement lvm_vg_create() and 4 required vg_set*() APIs

[Alasdair G Kergon]

On Tue, Jul 07, 2009 at 03:50:01AM -0400, Dave Wysochanski wrote:
> The following 7 patches implement the beginning of lvm_vg_create() liblvm API.

Ack to the principles within.
(I've not done a line-by-line review of correctness.)

Alasdair