[PATCH 0/4] Alloc VG

[PATCH 0/4] Alloc VG

[Zdenek Kabelac]

Refactor allocation of volume group structure.

Patch is currently splitted into multiple files to make a review easier.

Things to check - why is pool_vg_read used  seqno == 1?
(To make it same for all - I'd leave to 0 ?)

Memory for import functions is always used from vg mempool.

Future updates -
It would be probably worth to switch vg name to  char[128] (max vg name len)
to simplify surrounding allocations...


Zdenek Kabelac (4):
  Refactor vg allocation code
  Use alloc_vg for _pool_vg_read
  Use allog_vg for _free_vg in _pvsegs_sub_single
  Use alloc_vg for forma1_vg_read

 lib/format1/format1.c         |   72 ++++++++++---------------------
 lib/format_pool/format_pool.c |   96 ++++++++++++++---------------------------
 lib/format_text/import_vsn1.c |   38 +++++-----------
 lib/metadata/metadata.c       |   75 ++++++-------------------------
 lib/metadata/vg.c             |   32 ++++++++++++++
 lib/metadata/vg.h             |    3 +
 tools/reporter.c              |   25 +++++------
 7 files changed, 128 insertions(+), 213 deletions(-)

-- 
1.7.4.1

[PATCH 1/4] Refactor vg allocation code

[Zdenek Kabelac]

Create new function to allocate VG structure.

Currently it takes pool_name (for easier debugging).
And also take vg_name to futher simplify code.

Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>
---
 lib/format_text/import_vsn1.c |   38 ++++++---------------
 lib/metadata/metadata.c       |   75 ++++++++--------------------------------
 lib/metadata/vg.c             |   32 +++++++++++++++++
 lib/metadata/vg.h             |    3 ++
 4 files changed, 61 insertions(+), 87 deletions(-)

diff --git a/lib/format_text/import_vsn1.c b/lib/format_text/import_vsn1.c
index 0ffe334..d75e7af 100644
--- a/lib/format_text/import_vsn1.c
+++ b/lib/format_text/import_vsn1.c
@@ -652,35 +652,25 @@ static struct volume_group *_read_vg(struct format_instance *fid,
 	const struct config_node *vgn, *cn;
 	struct volume_group *vg;
 	struct dm_hash_table *pv_hash = NULL, *lv_hash = NULL;
-	struct dm_pool *mem = dm_pool_create("lvm2 vg_read", VG_MEMPOOL_CHUNK);
 	unsigned scan_done_once = use_cached_pvs;
 
-	if (!mem)
-		return_NULL;
-
 	/* skip any top-level values */
 	for (vgn = cft->root; (vgn && vgn->v); vgn = vgn->sib)
 		;
 
 	if (!vgn) {
 		log_error("Couldn't find volume group in file.");
-		goto bad;
+		return NULL;
 	}
 
-	if (!(vg = dm_pool_zalloc(mem, sizeof(*vg))))
-		goto_bad;
-
-	vg->vgmem = mem;
-	vg->cmd = fid->fmt->cmd;
+	if (!(vg = alloc_vg("read_vg", fid->fmt->cmd, vgn->key)))
+		return_NULL;
 
 	/* FIXME Determine format type from file contents */
 	/* eg Set to instance of fmt1 here if reading a format1 backup? */
 	vg->fid = fid;
 
-	if (!(vg->name = dm_pool_strdup(mem, vgn->key)))
-		goto_bad;
-
-	if (!(vg->system_id = dm_pool_zalloc(mem, NAME_LEN)))
+	if (!(vg->system_id = dm_pool_zalloc(vg->vgmem, NAME_LEN)))
 		goto_bad;
 
 	vgn = vgn->child;
@@ -733,7 +723,6 @@ static struct volume_group *_read_vg(struct format_instance *fid,
 		goto bad;
 	}
 
-	vg->alloc = ALLOC_NORMAL;
 	if ((cn = find_config_node(vgn, "allocation_policy"))) {
 		const struct config_value *cv = cn->v;
 		if (!cv || !cv->v.str) {
@@ -761,21 +750,16 @@ static struct volume_group *_read_vg(struct format_instance *fid,
 		goto bad;
 	}
 
-	dm_list_init(&vg->pvs);
-	if (!_read_sections(fid, "physical_volumes", _read_pv, mem, vg,
+	if (!_read_sections(fid, "physical_volumes", _read_pv, vg->vgmem, vg,
 			    vgn, pv_hash, lv_hash, 0, &scan_done_once)) {
 		log_error("Couldn't find all physical volumes for volume "
 			  "group %s.", vg->name);
 		goto bad;
 	}
 
-	dm_list_init(&vg->lvs);
-	dm_list_init(&vg->tags);
-	dm_list_init(&vg->removed_pvs);
-
 	/* Optional tags */
 	if ((cn = find_config_node(vgn, "tags")) &&
-	    !(read_tags(mem, &vg->tags, cn->v))) {
+	    !(read_tags(vg->vgmem, &vg->tags, cn->v))) {
 		log_error("Couldn't read tags for volume group %s.", vg->name);
 		goto bad;
 	}
@@ -789,15 +773,15 @@ static struct volume_group *_read_vg(struct format_instance *fid,
 		goto bad;
 	}
 
-	if (!_read_sections(fid, "logical_volumes", _read_lvnames, mem, vg,
-			    vgn, pv_hash, lv_hash, 1, NULL)) {
+	if (!_read_sections(fid, "logical_volumes", _read_lvnames, vg->vgmem,
+			    vg, vgn, pv_hash, lv_hash, 1, NULL)) {
 		log_error("Couldn't read all logical volume names for volume "
 			  "group %s.", vg->name);
 		goto bad;
 	}
 
-	if (!_read_sections(fid, "logical_volumes", _read_lvsegs, mem, vg,
-			    vgn, pv_hash, lv_hash, 1, NULL)) {
+	if (!_read_sections(fid, "logical_volumes", _read_lvsegs, vg->vgmem,
+			    vg, vgn, pv_hash, lv_hash, 1, NULL)) {
 		log_error("Couldn't read all logical volumes for "
 			  "volume group %s.", vg->name);
 		goto bad;
@@ -824,7 +808,7 @@ static struct volume_group *_read_vg(struct format_instance *fid,
 	if (lv_hash)
 		dm_hash_destroy(lv_hash);
 
-	dm_pool_destroy(mem);
+	free_vg(vg);
 	return NULL;
 }
 
diff --git a/lib/metadata/metadata.c b/lib/metadata/metadata.c
index ff61cbb..f50ab2a 100644
--- a/lib/metadata/metadata.c
+++ b/lib/metadata/metadata.c
@@ -836,25 +836,16 @@ int vgcreate_params_validate(struct cmd_context *cmd,
  * possible failure code or zero for success.
  */
 static struct volume_group *_vg_make_handle(struct cmd_context *cmd,
-			     struct volume_group *vg,
-			     uint32_t failure)
+					    struct volume_group *vg,
+					    uint32_t failure)
 {
-	struct dm_pool *vgmem;
-
-	if (!vg) {
-		if (!(vgmem = dm_pool_create("lvm2 vg_handle", VG_MEMPOOL_CHUNK)) ||
-		    !(vg = dm_pool_zalloc(vgmem, sizeof(*vg)))) {
-			log_error("Error allocating vg handle.");
-			if (vgmem)
-				dm_pool_destroy(vgmem);
-			return_NULL;
-		}
-		vg->vgmem = vgmem;
-	}
+	if (!vg && !(vg = alloc_vg("vg_make_handle", cmd, NULL)))
+		return_NULL;
 
-	vg->read_status = failure;
+	if (vg->read_status != failure)
+		vg->read_status = failure;
 
-	return (struct volume_group *)vg;
+	return vg;
 }
 
 int lv_has_unknown_segments(const struct logical_volume *lv)
@@ -891,7 +882,6 @@ struct volume_group *vg_create(struct cmd_context *cmd, const char *vg_name)
 	struct volume_group *vg;
 	struct format_instance_ctx fic;
 	int consistent = 0;
-	struct dm_pool *mem;
 	uint32_t rc;
 
 	if (!validate_name(vg_name)) {
@@ -914,10 +904,10 @@ struct volume_group *vg_create(struct cmd_context *cmd, const char *vg_name)
 		return _vg_make_handle(cmd, NULL, FAILED_EXIST);
 	}
 
-	if (!(mem = dm_pool_create("lvm2 vg_create", VG_MEMPOOL_CHUNK)))
-		goto_bad;
+	/* Strip dev_dir if present */
+	vg_name = strip_dir(vg_name, cmd->dev_dir);
 
-	if (!(vg = dm_pool_zalloc(mem, sizeof(*vg))))
+	if (!(vg = alloc_vg("vg_create", cmd, vg_name)))
 		goto_bad;
 
 	if (!id_create(&vg->id)) {
@@ -926,19 +916,8 @@ struct volume_group *vg_create(struct cmd_context *cmd, const char *vg_name)
 		goto bad;
 	}
 
-	/* Strip dev_dir if present */
-	vg_name = strip_dir(vg_name, cmd->dev_dir);
-
-	vg->vgmem = mem;
-	vg->cmd = cmd;
-
-	if (!(vg->name = dm_pool_strdup(mem, vg_name)))
-		goto_bad;
-
-	vg->seqno = 0;
-
 	vg->status = (RESIZEABLE_VG | LVM_READ | LVM_WRITE);
-	if (!(vg->system_id = dm_pool_alloc(mem, NAME_LEN)))
+	if (!(vg->system_id = dm_pool_alloc(vg->vgmem, NAME_LEN)))
 		goto_bad;
 
 	*vg->system_id = '\0';
@@ -954,14 +933,6 @@ struct volume_group *vg_create(struct cmd_context *cmd, const char *vg_name)
 	vg->mda_copies = DEFAULT_VGMETADATACOPIES;
 
 	vg->pv_count = 0;
-	dm_list_init(&vg->pvs);
-
-	dm_list_init(&vg->lvs);
-
-	dm_list_init(&vg->tags);
-
-	/* initialize removed_pvs list */
-	dm_list_init(&vg->removed_pvs);
 
 	fic.type = FMT_INSTANCE_VG | FMT_INSTANCE_MDAS | FMT_INSTANCE_AUX_MDAS;
 	fic.context.vg_ref.vg_name = vg_name;
@@ -2614,31 +2585,15 @@ static struct volume_group *_vg_read_orphans(struct cmd_context *cmd,
 	struct pv_list *pvl;
 	struct volume_group *vg;
 	struct physical_volume *pv;
-	struct dm_pool *mem;
 
 	lvmcache_label_scan(cmd, 0);
 
 	if (!(vginfo = vginfo_from_vgname(orphan_vgname, NULL)))
 		return_NULL;
 
-	if (!(mem = dm_pool_create("vg_read orphan", VG_MEMPOOL_CHUNK)))
+	if (!(vg = alloc_vg("vg_read_orphans", cmd, orphan_vgname)))
 		return_NULL;
 
-	if (!(vg = dm_pool_zalloc(mem, sizeof(*vg)))) {
-		log_error("vg allocation failed");
-		goto bad;
-	}
-	dm_list_init(&vg->pvs);
-	dm_list_init(&vg->lvs);
-	dm_list_init(&vg->tags);
-	dm_list_init(&vg->removed_pvs);
-	vg->vgmem = mem;
-	vg->cmd = cmd;
-	if (!(vg->name = dm_pool_strdup(mem, orphan_vgname))) {
-		log_error("vg name allocation failed");
-		goto bad;
-	}
-
 	/* create format instance with appropriate metadata area */
 	fic.type = FMT_INSTANCE_VG | FMT_INSTANCE_AUX_MDAS;
 	fic.context.vg_ref.vg_name = orphan_vgname;
@@ -2649,11 +2604,11 @@ static struct volume_group *_vg_read_orphans(struct cmd_context *cmd,
 	}
 
 	dm_list_iterate_items(info, &vginfo->infos) {
-		if (!(pv = _pv_read(cmd, mem, dev_name(info->dev),
+		if (!(pv = _pv_read(cmd, vg->vgmem, dev_name(info->dev),
 				    vg->fid, NULL, warnings, 0))) {
 			continue;
 		}
-		if (!(pvl = dm_pool_zalloc(mem, sizeof(*pvl)))) {
+		if (!(pvl = dm_pool_zalloc(vg->vgmem, sizeof(*pvl)))) {
 			log_error("pv_list allocation failed");
 			goto bad;
 		}
@@ -2663,7 +2618,7 @@ static struct volume_group *_vg_read_orphans(struct cmd_context *cmd,
 
 	return vg;
 bad:
-	dm_pool_destroy(mem);
+	free_vg(vg);
 	return NULL;
 }
 
diff --git a/lib/metadata/vg.c b/lib/metadata/vg.c
index 9d0d5dd..5bd00ae 100644
--- a/lib/metadata/vg.c
+++ b/lib/metadata/vg.c
@@ -18,6 +18,38 @@
 #include "display.h"
 #include "activate.h"
 
+struct volume_group *alloc_vg(const char *pool_name, struct cmd_context *cmd,
+			      const char *vg_name)
+{
+	struct dm_pool *vgmem;
+	struct volume_group *vg;
+
+	if (!(vgmem = dm_pool_create(pool_name, VG_MEMPOOL_CHUNK)) ||
+	    !(vg = dm_pool_zalloc(vgmem, sizeof(*vg)))) {
+		log_error("Failed to allocate volume group structure");
+		if (vgmem)
+			dm_pool_destroy(vgmem);
+		return NULL;
+	}
+
+	if (vg_name && !(vg->name = dm_pool_strdup(vgmem, vg_name))) {
+		log_error("Failed to allocate VG name.");
+		dm_pool_destroy(vgmem);
+		return NULL;
+	}
+
+	vg->cmd = cmd;
+	vg->vgmem = vgmem;
+	vg->alloc = ALLOC_NORMAL;
+
+	dm_list_init(&vg->pvs);
+	dm_list_init(&vg->lvs);
+	dm_list_init(&vg->tags);
+	dm_list_init(&vg->removed_pvs);
+
+	return vg;
+}
+
 char *vg_fmt_dup(const struct volume_group *vg)
 {
 	if (!vg->fid || !vg->fid->fmt)
diff --git a/lib/metadata/vg.h b/lib/metadata/vg.h
index 448dcfe..2cc6047 100644
--- a/lib/metadata/vg.h
+++ b/lib/metadata/vg.h
@@ -95,6 +95,9 @@ struct volume_group {
 	uint32_t mda_copies; /* target number of mdas for this VG */
 };
 
+struct volume_group *alloc_vg(const char *pool_name, struct cmd_context *cmd,
+			      const char *vg_name);
+
 char *vg_fmt_dup(const struct volume_group *vg);
 char *vg_name_dup(const struct volume_group *vg);
 char *vg_system_id_dup(const struct volume_group *vg);
-- 
1.7.4.1

[PATCH 2/4] Use alloc_vg for _pool_vg_read

[Zdenek Kabelac]

For simplier review.

Move remainder of _build_vg_from_pds  to _pool_vg_read.
Use  vg memory pool for import functions.
(it's been using smem -> fid mempool -> cmd mempool)

CHECKME: Why is here seqno == 1 ?
It's seems like every other alloc_vg is happy with seqno == 0.
Or all the others are wrond and count should start with 1 ?
Seems to be there from import day.

Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>
---
 lib/format_pool/format_pool.c |   96 ++++++++++++++---------------------------
 1 files changed, 33 insertions(+), 63 deletions(-)

diff --git a/lib/format_pool/format_pool.c b/lib/format_pool/format_pool.c
index 4252dad..f37e1e3 100644
--- a/lib/format_pool/format_pool.c
+++ b/lib/format_pool/format_pool.c
@@ -98,93 +98,63 @@ static int _check_usp(const char *vgname, struct user_subpool *usp, int sp_count
 	return 1;
 }
 
-static struct volume_group *_build_vg_from_pds(struct format_instance
-					       *fid, struct dm_pool *mem,
-					       struct dm_list *pds)
+static struct volume_group *_pool_vg_read(struct format_instance *fid,
+					  const char *vg_name,
+					  struct metadata_area *mda __attribute__((unused)))
 {
-	struct dm_pool *smem = fid->fmt->cmd->mem;
-	struct volume_group *vg = NULL;
-	struct user_subpool *usp = NULL;
+	struct volume_group *vg;
+	struct user_subpool *usp;
 	int sp_count;
+	DM_LIST_INIT(pds);
 
-	if (!(vg = dm_pool_zalloc(smem, sizeof(*vg)))) {
-		log_error("Unable to allocate volume group structure");
-		return NULL;
-	}
+	/* We can safely ignore the mda passed in */
 
-	vg->cmd = fid->fmt->cmd;
-	vg->vgmem = mem;
-	vg->fid = fid;
-	vg->name = NULL;
-	vg->status = 0;
-	vg->extent_count = 0;
-	vg->pv_count = 0;
-	vg->seqno = 1;
-	vg->system_id = NULL;
-	dm_list_init(&vg->pvs);
-	dm_list_init(&vg->lvs);
-	dm_list_init(&vg->tags);
-	dm_list_init(&vg->removed_pvs);
-
-	if (!import_pool_vg(vg, smem, pds))
-		return_NULL;
+	/* Strip dev_dir if present */
+	vg_name = strip_dir(vg_name, fid->fmt->cmd->dev_dir);
 
-	if (!import_pool_pvs(fid->fmt, vg, smem, pds))
+	/* Set vg_name through read_pool_pds() (SIMPLIFY in alloc_vg) */
+	if (!(vg = alloc_vg("pool_vg_read", fid->fmt->cmd, NULL)))
 		return_NULL;
 
-	if (!import_pool_lvs(vg, smem, pds))
-		return_NULL;
+	/* Read all the pvs in the vg */
+	if (!read_pool_pds(fid->fmt, vg_name, vg->vgmem, &pds))
+		goto_bad;
+
+	vg->fid = fid;
+	vg->seqno = 1; /* FIXME: Why is used 1 here? */
+
+	if (!import_pool_vg(vg, vg->vgmem, &pds))
+		goto_bad;
+
+	if (!import_pool_pvs(fid->fmt, vg, vg->vgmem, &pds))
+		goto_bad;
+
+	if (!import_pool_lvs(vg, vg->vgmem, &pds))
+		goto_bad;
 
 	/*
 	 * I need an intermediate subpool structure that contains all the
 	 * relevant info for this.  Then i can iterate through the subpool
 	 * structures for checking, and create the segments
 	 */
-	if (!(usp = _build_usp(pds, mem, &sp_count)))
-		return_NULL;
+	if (!(usp = _build_usp(&pds, vg->vgmem, &sp_count)))
+		goto_bad;
 
 	/*
 	 * check the subpool structures - we can't handle partial VGs in
 	 * the pool format, so this will error out if we're missing PVs
 	 */
 	if (!_check_usp(vg->name, usp, sp_count))
-		return_NULL;
+		goto_bad;
 
-	if (!import_pool_segments(&vg->lvs, smem, usp, sp_count))
-		return_NULL;
+	if (!import_pool_segments(&vg->lvs, vg->vgmem, usp, sp_count))
+		goto_bad;
 
 	return vg;
-}
 
-static struct volume_group *_pool_vg_read(struct format_instance *fid,
-				     const char *vg_name,
-				     struct metadata_area *mda __attribute__((unused)))
-{
-	struct dm_pool *mem = dm_pool_create("pool vg_read", VG_MEMPOOL_CHUNK);
-	struct dm_list pds;
-	struct volume_group *vg = NULL;
-
-	dm_list_init(&pds);
-
-	/* We can safely ignore the mda passed in */
-
-	if (!mem)
-		return_NULL;
-
-	/* Strip dev_dir if present */
-	vg_name = strip_dir(vg_name, fid->fmt->cmd->dev_dir);
-
-	/* Read all the pvs in the vg */
-	if (!read_pool_pds(fid->fmt, vg_name, mem, &pds))
-		goto_out;
+bad:
+	free_vg(vg);
 
-	/* Do the rest of the vg stuff */
-	if (!(vg = _build_vg_from_pds(fid, mem, &pds)))
-		goto_out;
-
-	return vg;
-out:
-	dm_pool_destroy(mem);
 	return NULL;
 }
 
-- 
1.7.4.1

[PATCH 3/4] Use allog_vg for _free_vg in _pvsegs_sub_single

[Zdenek Kabelac]

For simplier review.

Switch to use alloc_vg function for allocation of VG struct.

Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>
---
 tools/reporter.c |   25 +++++++++++--------------
 1 files changed, 11 insertions(+), 14 deletions(-)

diff --git a/tools/reporter.c b/tools/reporter.c
index ebe85e7..b8f8e37 100644
--- a/tools/reporter.c
+++ b/tools/reporter.c
@@ -58,17 +58,9 @@ static int _pvsegs_sub_single(struct cmd_context *cmd,
 {
 	int ret = ECMD_PROCESSED;
 	struct lv_segment *seg = pvseg->lvseg;
-
-	struct volume_group _free_vg = {
-		.cmd = cmd,
-		.name = "",
-	};
-
-        if (!(_free_vg.vgmem = dm_pool_create("_free_vg", 10240)))
-		return ECMD_FAILED;
+	struct volume_group *_free_vg;
 
 	struct logical_volume _free_logical_volume = {
-		.vg = vg ?: &_free_vg,
 		.name = "",
 		.snapshot = NULL,
 		.status = VISIBLE_LV,
@@ -92,11 +84,15 @@ static int _pvsegs_sub_single(struct cmd_context *cmd,
 		.areas = NULL,
 	};
 
+	if (!(_free_vg = alloc_vg("free_vg", cmd, ""))) {
+		stack;
+		return ECMD_FAILED;
+	}
+
+	_free_logical_volume.vg = vg ?: _free_vg;
 	_free_lv_segment.segtype = get_segtype_from_string(cmd, "free");
 	_free_lv_segment.len = pvseg->len;
-	dm_list_init(&_free_vg.pvs);
-	dm_list_init(&_free_vg.lvs);
-	dm_list_init(&_free_vg.tags);
+
 	dm_list_init(&_free_lv_segment.tags);
 	dm_list_init(&_free_lv_segment.origin_list);
 	dm_list_init(&_free_logical_volume.tags);
@@ -109,8 +105,9 @@ static int _pvsegs_sub_single(struct cmd_context *cmd,
 		ret = ECMD_FAILED;
                 goto_out;
 	}
- out:
-	free_vg(&_free_vg);
+out:
+	free_vg(_free_vg);
+
 	return ret;
 }
 
-- 
1.7.4.1

[PATCH 4/4] Use alloc_vg for forma1_vg_read

[Zdenek Kabelac]

Update code to use alloc_vg.
Move remainder of the _build_vg to _format1_vg_read

Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>
---
 lib/format1/format1.c |   72 +++++++++++++++---------------------------------
 1 files changed, 23 insertions(+), 49 deletions(-)

diff --git a/lib/format1/format1.c b/lib/format1/format1.c
index 9011982..98c1916 100644
--- a/lib/format1/format1.c
+++ b/lib/format1/format1.c
@@ -177,84 +177,58 @@ out:
 	return 0;
 }
 
-static struct volume_group *_build_vg(struct format_instance *fid,
-				      struct dm_list *pvs,
-				      struct dm_pool *mem)
+static struct volume_group *_format1_vg_read(struct format_instance *fid,
+				     const char *vg_name,
+				     struct metadata_area *mda __attribute__((unused)))
 {
-	struct volume_group *vg = dm_pool_zalloc(mem, sizeof(*vg));
+	struct volume_group *vg;
 	struct disk_list *dl;
+	DM_LIST_INIT(pvs);
+
+	/* Strip dev_dir if present */
+	vg_name = strip_dir(vg_name, fid->fmt->cmd->dev_dir);
+
+	if (!(vg = alloc_vg("format1_vg_read", fid->fmt->cmd, NULL)))
+		return_NULL;
 
-	if (!vg)
+	if (!read_pvs_in_vg(fid->fmt, vg_name, fid->fmt->cmd->filter,
+			    vg->vgmem, &pvs))
 		goto_bad;
 
 	if (dm_list_empty(pvs))
 		goto_bad;
 
-	vg->cmd = fid->fmt->cmd;
-	vg->vgmem = mem;
 	vg->fid = fid;
-	vg->seqno = 0;
-	dm_list_init(&vg->pvs);
-	dm_list_init(&vg->lvs);
-	dm_list_init(&vg->tags);
-	dm_list_init(&vg->removed_pvs);
 
-	if (!_check_vgs(pvs, vg))
+	if (!_check_vgs(&pvs, vg))
 		goto_bad;
 
-	dl = dm_list_item(pvs->n, struct disk_list);
+	dl = dm_list_item(pvs.n, struct disk_list);
 
-	if (!import_vg(mem, vg, dl))
+	if (!import_vg(vg->vgmem, vg, dl))
 		goto_bad;
 
-	if (!import_pvs(fid->fmt, mem, vg, pvs))
+	if (!import_pvs(fid->fmt, vg->vgmem, vg, &pvs))
 		goto_bad;
 
-	if (!import_lvs(mem, vg, pvs))
+	if (!import_lvs(vg->vgmem, vg, &pvs))
 		goto_bad;
 
-	if (!import_extents(fid->fmt->cmd, vg, pvs))
+	if (!import_extents(fid->fmt->cmd, vg, &pvs))
 		goto_bad;
 
-	if (!import_snapshots(mem, vg, pvs))
+	if (!import_snapshots(vg->vgmem, vg, &pvs))
 		goto_bad;
 
 	/* Fix extents counts by adding missing PV if partial VG */
-	if ((vg->status & PARTIAL_VG) && !_fix_partial_vg(vg, pvs))
+	if ((vg->status & PARTIAL_VG) && !_fix_partial_vg(vg, &pvs))
 		goto_bad;
 
 	return vg;
 
-      bad:
-	dm_pool_free(mem, vg);
-	return NULL;
-}
-
-static struct volume_group *_format1_vg_read(struct format_instance *fid,
-				     const char *vg_name,
-				     struct metadata_area *mda __attribute__((unused)))
-{
-	struct dm_pool *mem = dm_pool_create("lvm1 vg_read", VG_MEMPOOL_CHUNK);
-	struct dm_list pvs;
-	struct volume_group *vg = NULL;
-	dm_list_init(&pvs);
-
-	if (!mem)
-		return_NULL;
-
-	/* Strip dev_dir if present */
-	vg_name = strip_dir(vg_name, fid->fmt->cmd->dev_dir);
-
-	if (!read_pvs_in_vg
-	    (fid->fmt, vg_name, fid->fmt->cmd->filter, mem, &pvs))
-		goto_bad;
-
-	if (!(vg = _build_vg(fid, &pvs, mem)))
-		goto_bad;
-
-	return vg;
 bad:
-	dm_pool_destroy(mem);
+	free(vg);
+
 	return NULL;
 }
 
-- 
1.7.4.1

[PATCH 3/4] Use allog_vg for _free_vg in _pvsegs_sub_single

[Alasdair G Kergon]

On Wed, Mar 02, 2011 at 07:20:25PM +0100, Zdenek Kabelac wrote:
> Switch to use alloc_vg function for allocation of VG struct.
 
This code looks wrong.  Maybe it suffered from some previous global change.

> -	struct volume_group _free_vg = {
> -		.cmd = cmd,
> -		.name = "",
> -	};

I think that should stay as it is.

> -        if (!(_free_vg.vgmem = dm_pool_create("_free_vg", 10240)))
> -		return ECMD_FAILED;

Why is vgmem being allocated?  If something really is using it, why?

Alasdair

[PATCH 3/4] Use allog_vg for _free_vg in _pvsegs_sub_single

[Zdenek Kabelac]

Dne 8.3.2011 18:09, Alasdair G Kergon napsal(a):
> On Wed, Mar 02, 2011 at 07:20:25PM +0100, Zdenek Kabelac wrote:
>> Switch to use alloc_vg function for allocation of VG struct.
>  
> This code looks wrong.  Maybe it suffered from some previous global change.
> 
>> -	struct volume_group _free_vg = {
>> -		.cmd = cmd,
>> -		.name = "",
>> -	};
> 
> I think that should stay as it is.

Peter already spend quite some time debuging bug in missing initialization (he
was not using this patchset in his code) of some VG list structure - because
there were 5 copies of VG alloc code and some of them were not all properly
setting all values to some defined state.

> 
>> -        if (!(_free_vg.vgmem = dm_pool_create("_free_vg", 10240)))
>> -		return ECMD_FAILED;
> 
> Why is vgmem being allocated?  If something really is using it, why?

I've in my tree already slightly better patch which allocates and free
_free_vg only if '!vg'.


The major purpose if the patch is to keep also reference count setting in one
place (needed in my vg sharing patch) - also whenever someone adds any new
member to VG structure - only one place needs to be updated for proper
initialization.  So I'd still prefer to have this code running through
standard allocation procedure.

Zdenek

[PATCH 3/4] Use allog_vg for _free_vg in _pvsegs_sub_single

[Alasdair G Kergon]

On Tue, Mar 08, 2011 at 11:30:16PM +0100, Zdenek Kabelac wrote:
> The major purpose if the patch is to keep also reference count setting in one
> place (needed in my vg sharing patch) - also whenever someone adds any new
> member to VG structure - only one place needs to be updated for proper
> initialization.  So I'd still prefer to have this code running through
> standard allocation procedure.
 
But the new function is not (yet?) initialising the whole structure.

The 'reporter' use is only as a read-only dummy structure to allow us
to share some code paths.  It should not need a vgmem (or reference
counting), I'd have thought.

Alasdair

[PATCH 3/4] Use allog_vg for _free_vg in _pvsegs_sub_single

[Zdenek Kabelac]

Dne 9.3.2011 04:01, Alasdair G Kergon napsal(a):
> On Tue, Mar 08, 2011 at 11:30:16PM +0100, Zdenek Kabelac wrote:
>> The major purpose if the patch is to keep also reference count setting in one
>> place (needed in my vg sharing patch) - also whenever someone adds any new
>> member to VG structure - only one place needs to be updated for proper
>> initialization.  So I'd still prefer to have this code running through
>> standard allocation procedure.
>  
> But the new function is not (yet?) initialising the whole structure.

Idea here was - to keep all dm_list initialization in one place - so whenever
some new list or some similar structure which required to have some non-null
value to be usable - it will be set in one place - so one doesn't need to
track all such places in the code and add dm_list_init in more then one place.
(object encapsulation).

> 
> The 'reporter' use is only as a read-only dummy structure to allow us
> to share some code paths.  It should not need a vgmem (or reference
> counting), I'd have thought.


I've been not checking all related code paths which may be executed from this
place - so it's been rather pure code refactoring.

It seems like the code below now should actually never allocate anything from
this pool (seem like after my recent fix for allocation from vg mem and using
reporter's mempool possibly fixed it?) and if it does some allocation
somewhere it's bug which should crash the appliction, but currently IMHO there
was no such restriction put or documented in the code.

Zdenek