[PATCH 0/6] Add pv->vg link, rework lvm2app string properties

[PATCH 0/6] Add pv->vg link, rework lvm2app string properties

[Dave Wysochanski]

This patchset adds a pv->vg link and reworks lvm2app string property
functions to use the vg handle's memory for allocation.

The first 3 patches are simple refactorings, the 4th patch adds the
pv->vg link, and the last 2 change lvm2app string functions and the
lvm2app version number.

[PATCH 1/6] Add pv to vg->pvs after check for maximum value of vg->extent_count.

[Dave Wysochanski]

In add_pv_to_vg(), we should only add the pv to vg->pvs after all
internal checks have passed.  The check for vg->extent_count exeeding
maximum was after we added the pv to the list, so this function could
return a state of vg->pvs that did not reflect other parameters such
as vg->pv_count.

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

diff --git a/lib/metadata/metadata.c b/lib/metadata/metadata.c
index a12a8c8..9e6d9ba 100644
--- a/lib/metadata/metadata.c
+++ b/lib/metadata/metadata.c
@@ -220,9 +220,6 @@ int add_pv_to_vg(struct volume_group *vg, const char *pv_name,
 	if (!alloc_pv_segment_whole_pv(mem, pv))
 		return_0;
 
-	pvl->pv = pv;
-	dm_list_add(&vg->pvs, &pvl->list);
-
 	if ((uint64_t) vg->extent_count + pv->pe_count > UINT32_MAX) {
 		log_error("Unable to add %s to %s: new extent count (%"
 			  PRIu64 ") exceeds limit (%" PRIu32 ").",
@@ -232,6 +229,9 @@ int add_pv_to_vg(struct volume_group *vg, const char *pv_name,
 		return 0;
 	}
 
+	pvl->pv = pv;
+	dm_list_add(&vg->pvs, &pvl->list);
+
 	vg->pv_count++;
 	vg->extent_count += pv->pe_count;
 	vg->free_count += pv->pe_count;
-- 
1.6.0.6

[PATCH 2/6] Refactor _read_pv() code adding pv to vg->pvs and updating vg counts.

[Dave Wysochanski]

Simple refactor to group code that adds a pv to the vg->vgs and updates
related vg counts.  No functional change.

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 lib/format_text/import_vsn1.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/lib/format_text/import_vsn1.c b/lib/format_text/import_vsn1.c
index ae9742a..cf2c9df 100644
--- a/lib/format_text/import_vsn1.c
+++ b/lib/format_text/import_vsn1.c
@@ -241,10 +241,6 @@ static int _read_pv(struct format_instance *fid, struct dm_pool *mem,
 		return 0;
 	}
 
-	/* adjust the volume group. */
-	vg->extent_count += pv->pe_count;
-	vg->free_count += pv->pe_count;
-
 	pv->pe_size = vg->extent_size;
 
 	pv->pe_alloc_count = 0;
@@ -273,6 +269,8 @@ static int _read_pv(struct format_instance *fid, struct dm_pool *mem,
 	if (!alloc_pv_segment_whole_pv(mem, pv))
 		return_0;
 
+	vg->extent_count += pv->pe_count;
+	vg->free_count += pv->pe_count;
 	vg->pv_count++;
 	dm_list_add(&vg->pvs, &pvl->list);
 
-- 
1.6.0.6

[PATCH 3/6] Add add_pvl_to_vgs() - helper function to add a pv to a vg list.

[Dave Wysochanski]

Small refactor of main places in the code where a pv is added to a
vg into a small function which adds the pv to the list and updates
the vg counts.

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 lib/format_text/import_vsn1.c |    5 +----
 lib/metadata/metadata.c       |   16 +++++++++++-----
 lib/metadata/metadata.h       |    1 +
 3 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/lib/format_text/import_vsn1.c b/lib/format_text/import_vsn1.c
index cf2c9df..48fa7f2 100644
--- a/lib/format_text/import_vsn1.c
+++ b/lib/format_text/import_vsn1.c
@@ -269,10 +269,7 @@ static int _read_pv(struct format_instance *fid, struct dm_pool *mem,
 	if (!alloc_pv_segment_whole_pv(mem, pv))
 		return_0;
 
-	vg->extent_count += pv->pe_count;
-	vg->free_count += pv->pe_count;
-	vg->pv_count++;
-	dm_list_add(&vg->pvs, &pvl->list);
+	add_pvl_to_vgs(vg, pvl);
 
 	return 1;
 }
diff --git a/lib/metadata/metadata.c b/lib/metadata/metadata.c
index 9e6d9ba..a60ea34 100644
--- a/lib/metadata/metadata.c
+++ b/lib/metadata/metadata.c
@@ -136,6 +136,16 @@ out:
 	return pv->pe_align_offset;
 }
 
+void add_pvl_to_vgs(struct volume_group *vg, struct pv_list *pvl)
+{
+	dm_list_add(&vg->pvs, &pvl->list);
+
+	vg->pv_count++;
+	vg->extent_count += pvl->pv->pe_count;
+	vg->free_count += pvl->pv->pe_count;
+}
+
+
 /**
  * add_pv_to_vg - Add a physical volume to a volume group
  * @vg - volume group to add to
@@ -230,11 +240,7 @@ int add_pv_to_vg(struct volume_group *vg, const char *pv_name,
 	}
 
 	pvl->pv = pv;
-	dm_list_add(&vg->pvs, &pvl->list);
-
-	vg->pv_count++;
-	vg->extent_count += pv->pe_count;
-	vg->free_count += pv->pe_count;
+	add_pvl_to_vgs(vg, pvl);
 
 	return 1;
 }
diff --git a/lib/metadata/metadata.h b/lib/metadata/metadata.h
index 782d300..1496252 100644
--- a/lib/metadata/metadata.h
+++ b/lib/metadata/metadata.h
@@ -377,5 +377,6 @@ struct id pv_vgid(const struct physical_volume *pv);
 struct physical_volume *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);
+void add_pvl_to_vgs(struct volume_group *vg, struct pv_list *pvl);
 
 #endif
-- 
1.6.0.6

[PATCH 4/6] Add pv->vg to solidify link between a pv and a vg.

[Dave Wysochanski]

lvm2app needs a link back to the vg in order to use the vg handle for
memory allocations as well as other things.  This patch adds the field
to struct physical_volume, and sets the field when returning from
vg_read() internal library call.

NOTE: Unfortunately in the case of pv_read() callers, we cannot set
this pv->vg field properly, since the vg it belongs to may not be
known by just reading a single device, and no VG is in existence at
the time of pv_read().  Significant refactorings related to
lvmcache, PVs with 0 mdas, etc, are required to set this field
for callers of pv_read().  For now we assume that if the field is
not set, the pv is not associated with a complete VG (as returned
by vg_read()).

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

diff --git a/lib/metadata/metadata-exported.h b/lib/metadata/metadata-exported.h
index b02a22c..d73dc06 100644
--- a/lib/metadata/metadata-exported.h
+++ b/lib/metadata/metadata-exported.h
@@ -183,7 +183,7 @@ struct physical_volume {
 	const struct format_type *fmt;
 	const char *vg_name;
 	struct id vgid;
-
+	struct volume_group *vg; /* FIXME: Only valid when used with lvm2app. */
 	uint64_t status;
 	uint64_t size;
 
diff --git a/lib/metadata/metadata.c b/lib/metadata/metadata.c
index a60ea34..6063744 100644
--- a/lib/metadata/metadata.c
+++ b/lib/metadata/metadata.c
@@ -143,6 +143,7 @@ void add_pvl_to_vgs(struct volume_group *vg, struct pv_list *pvl)
 	vg->pv_count++;
 	vg->extent_count += pvl->pv->pe_count;
 	vg->free_count += pvl->pv->pe_count;
+	pvl->pv->vg = vg;
 }
 
 
-- 
1.6.0.6

[PATCH 5/6] Use vg->vgmem to allocate vg/lv/pv string properties instead of dm_malloc/free.

[Dave Wysochanski]

Everywhere else in the API the caller can rely on lvm2app taking care of
memory allocation and free, so make the 'name' and 'uuid' properties of a
vg/lv/pv use the vg handle to allocate memory.

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 liblvm/lvm2app.h |   24 ++++++++++++------------
 liblvm/lvm_lv.c  |   10 +++-------
 liblvm/lvm_pv.c  |   10 +++-------
 liblvm/lvm_vg.c  |    9 ++-------
 4 files changed, 20 insertions(+), 33 deletions(-)

diff --git a/liblvm/lvm2app.h b/liblvm/lvm2app.h
index 61ffd87..dad92c0 100644
--- a/liblvm/lvm2app.h
+++ b/liblvm/lvm2app.h
@@ -642,12 +642,12 @@ uint64_t lvm_vg_is_partial(vg_t vg);
 uint64_t lvm_vg_get_seqno(const vg_t vg);
 
 /**
- * Get the current name of a volume group.
+ * Get the current uuid of a volume group.
  *
  * \memberof vg_t
  *
- * Memory is allocated using dm_malloc() and caller must free the memory
- * using dm_free().
+ * The memory allocated for the uuid is tied to the vg_t handle and will be
+ * released when lvm_vg_close() is called.
  *
  * \param   vg
  * VG handle obtained from lvm_vg_create or lvm_vg_open().
@@ -658,12 +658,12 @@ uint64_t lvm_vg_get_seqno(const vg_t vg);
 char *lvm_vg_get_uuid(const vg_t vg);
 
 /**
- * Get the current uuid of a volume group.
+ * Get the current name of a volume group.
  *
  * \memberof vg_t
  *
- * Memory is allocated using dm_malloc() and caller must free the memory
- * using dm_free().
+ * The memory allocated for the name is tied to the vg_t handle and will be
+ * released when lvm_vg_close() is called.
  *
  * \param   vg
  * VG handle obtained from lvm_vg_create or lvm_vg_open().
@@ -783,7 +783,7 @@ uint64_t lvm_vg_get_max_lv(const vg_t vg);
  * \memberof vg_t
  *
  * The memory allocated for the list is tied to the vg_t handle and will be
- * released when lvm_vg_close is called.
+ * released when lvm_vg_close() is called.
  *
  * To process the list, use the dm_list iterator functions.  For example:
  *      vg_t vg;
@@ -1001,7 +1001,7 @@ int lvm_lv_remove_tag(lv_t lv, const char *tag);
  * \memberof lv_t
  *
  * The memory allocated for the list is tied to the vg_t handle and will be
- * released when lvm_vg_close is called.
+ * released when lvm_vg_close() is called.
  *
  * To process the list, use the dm_list iterator functions.  For example:
  *      lv_t lv;
@@ -1057,8 +1057,8 @@ int lvm_lv_resize(const lv_t lv, uint64_t new_size);
  *
  * \memberof pv_t
  *
- * Memory is allocated using dm_malloc() and caller must free the memory
- * using dm_free().
+ * The memory allocated for the uuid is tied to the vg_t handle and will be
+ * released when lvm_vg_close() is called.
  *
  * \param   pv
  * Physical volume handle.
@@ -1073,8 +1073,8 @@ char *lvm_pv_get_uuid(const pv_t pv);
  *
  * \memberof pv_t
  *
- * Memory is allocated using dm_malloc() and caller must free the memory
- * using dm_free().
+ * The memory allocated for the uuid is tied to the vg_t handle and will be
+ * released when lvm_vg_close() is called.
  *
  * \param   pv
  * Physical volume handle.
diff --git a/liblvm/lvm_lv.c b/liblvm/lvm_lv.c
index d3bb6b8..1a6c6ce 100644
--- a/liblvm/lvm_lv.c
+++ b/liblvm/lvm_lv.c
@@ -47,17 +47,13 @@ char *lvm_lv_get_uuid(const lv_t lv)
 		log_error(INTERNAL_ERROR "unable to convert uuid");
 		return NULL;
 	}
-	return strndup((const char *)uuid, 64);
+	return dm_pool_strndup(lv->vg->vgmem, (const char *)uuid, 64);
 }
 
 char *lvm_lv_get_name(const lv_t lv)
 {
-	char *name;
-
-	name = dm_malloc(NAME_LEN + 1);
-	strncpy(name, (const char *)lv->name, NAME_LEN);
-	name[NAME_LEN] = '\0';
-	return name;
+	return dm_pool_strndup(lv->vg->vgmem, (const char *)lv->name,
+			       NAME_LEN+1);
 }
 
 uint64_t lvm_lv_is_active(const lv_t lv)
diff --git a/liblvm/lvm_pv.c b/liblvm/lvm_pv.c
index 894aa4b..033bc88 100644
--- a/liblvm/lvm_pv.c
+++ b/liblvm/lvm_pv.c
@@ -25,17 +25,13 @@ char *lvm_pv_get_uuid(const pv_t pv)
 		log_error(INTERNAL_ERROR "Unable to convert uuid");
 		return NULL;
 	}
-	return strndup((const char *)uuid, 64);
+	return dm_pool_strndup(pv->vg->vgmem, (const char *)uuid, 64);
 }
 
 char *lvm_pv_get_name(const pv_t pv)
 {
-	char *name;
-
-	name = dm_malloc(NAME_LEN + 1);
-	strncpy(name, (const char *)pv_dev_name(pv), NAME_LEN);
-	name[NAME_LEN] = '\0';
-	return name;
+	return dm_pool_strndup(pv->vg->vgmem,
+			       (const char *)pv_dev_name(pv), NAME_LEN + 1);
 }
 
 uint64_t lvm_pv_get_mda_count(const pv_t pv)
diff --git a/liblvm/lvm_vg.c b/liblvm/lvm_vg.c
index 4bb4cae..544cc56 100644
--- a/liblvm/lvm_vg.c
+++ b/liblvm/lvm_vg.c
@@ -336,17 +336,12 @@ char *lvm_vg_get_uuid(const vg_t vg)
 		log_error(INTERNAL_ERROR "Unable to convert uuid");
 		return NULL;
 	}
-	return strndup((const char *)uuid, 64);
+	return dm_pool_strndup(vg->vgmem, (const char *)uuid, 64);
 }
 
 char *lvm_vg_get_name(const vg_t vg)
 {
-	char *name;
-
-	name = dm_malloc(NAME_LEN + 1);
-	strncpy(name, (const char *)vg->name, NAME_LEN);
-	name[NAME_LEN] = '\0';
-	return name;
+	return dm_pool_strndup(vg->vgmem, (const char *)vg->name, NAME_LEN+1);
 }
 
 struct dm_list *lvm_list_vg_names(lvm_t libh)
-- 
1.6.0.6

[PATCH 6/6] RFC: Change lvm2app version number from 1 to 2.

[Dave Wysochanski]

This version number change reflects the memory handling change
for string-based pv/vg/lv string based attributes.
In addition, when adding support for tags, I forgot to increase
the version number.  Also, it might be nice to have a short list
of what has changed in the API since the last version, apart from
the WHATS_NEW lvm file.

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 VERSION |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/VERSION b/VERSION
index d96250e..204b86d 100644
--- a/VERSION
+++ b/VERSION
@@ -1 +1 @@
-2.02.63(1)-cvs (2010-03-09)
+2.02.63(2)-cvs (2010-03-09)
-- 
1.6.0.6

[PATCH 6/6] RFC: Change lvm2app version number from 1 to 2.

[Milan Broz]

On 04/01/2010 11:57 PM, Dave Wysochanski wrote:
> This version number change reflects the memory handling change
> for string-based pv/vg/lv string based attributes.

Maybe it is better idea to use VG mempool, maybe not for
VG attributes.

But change it now, it means that application using
calls which previously deallocates these attributes with dm_free
will now crash, because the memory returned is now owned
by library.

I really do not like changing API this way in every release...

I see three API possibilities

 - define buffer parameter, so all memory allocations are
in user application

 - return strings/parameters *CONST* and define that lvm2app
owns memory

 - use dm_malloc, and require dm_free in user aplications.

But please do not change this later!

It is extremely confusing for library users,
no warning that API is not stable will not help here...

Milan

[PATCH 6/6] RFC: Change lvm2app version number from 1 to 2.

[Dave Wysochanski]

On Fri, 2010-04-02 at 13:39 +0200, Milan Broz wrote:
> On 04/01/2010 11:57 PM, Dave Wysochanski wrote:
> > This version number change reflects the memory handling change
> > for string-based pv/vg/lv string based attributes.
> 
> Maybe it is better idea to use VG mempool, maybe not for
> VG attributes.
> 
> But change it now, it means that application using
> calls which previously deallocates these attributes with dm_free
> will now crash, because the memory returned is now owned
> by library.
> 

Right it's not a "nice" change right now but as far as I know we have
one library user and I've been in touch with him.


> I really do not like changing API this way in every release...
> 

Do you think it is better to leave things as they are?

It has not been changed until now.  Plus, there is already an
inconsistency in the way memory is handled with the APIs that return
lists owned by lvm2app and the attribute APIs return memory owned by the
application.  

With things as they are now, in addition to the inconsistency, an
application could easily create memory leaks if for example it just
wants to display attributes with something like this:

printf("vg_name = %s", lvm_vg_get_name(vg));

instead, the proper code is:

char *vgname;
vgname = lvm_vg_get_name(vg);
printf ("vg_name = %s", vgname);
dm_free(vgname);

And this must be done for _every_ attribute.


> I see three API possibilities
> 
>  - define buffer parameter, so all memory allocations are
> in user application
> 
>  - return strings/parameters *CONST* and define that lvm2app
> owns memory
> 
>  - use dm_malloc, and require dm_free in user aplications.
> 

Right, those are the possibilities.  I had discussed this with the David
Zeuthen a few weeks ago and he seemed to be fine with the change as this
is how one of his other APIs work.  I can forward you the discussion
and/or confirm with him again.

There are pros and cons of each approach.  


> But please do not change this later!
> 
> It is extremely confusing for library users,
> no warning that API is not stable will not help here...
> 

Agreed.  But do you feel we should leave the current inconsistency? 

Given that we are still early in the library development, and there are
few users, I felt it was not out of the question to propose a change.

[PATCH 4/6] Add pv->vg to solidify link between a pv and a vg.

[Alasdair G Kergon]

On Thu, Apr 01, 2010 at 05:57:30PM -0400, Dave Wysochanski wrote:
> lvm2app needs a link back to the vg in order to use the vg handle for
> memory allocations as well as other things.  This patch adds the field
> to struct physical_volume, and sets the field when returning from
> vg_read() internal library call.
 
> +	struct volume_group *vg; /* FIXME: Only valid when used with lvm2app. */

Please make it generally applicable and always internally consistent i.e.
always pointing back to the VG struct which contains the list it is on.

Also update vg_validate whenever extending the core metadata structs.

Alasdair

[PATCH 4/6] Add pv->vg to solidify link between a pv and a vg.

[Dave Wysochanski]

On Fri, 2010-04-02 at 02:23 +0100, Alasdair G Kergon wrote:
> On Thu, Apr 01, 2010 at 05:57:30PM -0400, Dave Wysochanski wrote:
> > lvm2app needs a link back to the vg in order to use the vg handle for
> > memory allocations as well as other things.  This patch adds the field
> > to struct physical_volume, and sets the field when returning from
> > vg_read() internal library call.
>  
> > +	struct volume_group *vg; /* FIXME: Only valid when used with lvm2app. */
> 
> Please make it generally applicable and always internally consistent i.e.
> always pointing back to the VG struct which contains the list it is on.
> 

Oh - this FIXME is an erroneous comment - sorry.  Originally I had
approached it this way, then changed it to be more general.  I could
have missed something though.

> Also update vg_validate whenever extending the core metadata structs.
> 

Will do so - thanks for the reminder.