[PATCH 00/19] Add lvm vg and pv properties for lvm2app.

[PATCH 00/19] Add lvm vg and pv properties for lvm2app.

[Dave Wysochanski]

This patchset is a repost and add onto the previous patchset
posted to lvm-devel on 9/9/2010.  It continues that patchset
and adds pv properties and addresses comments made on the vg
patchset.

Notable changes:
1. Simplified logic to create 'attr' strings based on review comment
2. Rework GET_*_PROPERTY_FN macro to allow for pv, vg, and lv
3. Renamed internal function vg_get_property to lvm_get_property
4. Add a lvm_pv_get_property function similar to the vg one
5. Various refactorings related to pv properties.

All vg and pv properties may now be queried with this patchset.
I will now move on to lv properties.

[PATCH 01/19] Add vg_attr() and lv_attr() functions.

[Dave Wysochanski]

Move the creating of the 'attr' strings into a common function so
they can be called from the 'disp' functions as well as the new
'get' property functions.

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 lib/metadata/metadata.c |  171 +++++++++++++++++++++++++++++++++++++++++++++++
 lib/metadata/metadata.h |    2 +
 lib/report/report.c     |  157 +------------------------------------------
 3 files changed, 175 insertions(+), 155 deletions(-)

diff --git a/lib/metadata/metadata.c b/lib/metadata/metadata.c
index d14c33c..094ffc1 100644
--- a/lib/metadata/metadata.c
+++ b/lib/metadata/metadata.c
@@ -4544,6 +4544,177 @@ uint32_t vg_mda_used_count(const struct volume_group *vg)
        return used_count;
 }
 
+static char _alloc_policy_char(alloc_policy_t alloc)
+{
+	switch (alloc) {
+	case ALLOC_CONTIGUOUS:
+		return 'c';
+	case ALLOC_CLING:
+		return 'l';
+	case ALLOC_NORMAL:
+		return 'n';
+	case ALLOC_ANYWHERE:
+		return 'a';
+	default:
+		return 'i';
+	}
+}
+
+char *vg_attr(struct dm_pool *mem, const struct volume_group *vg)
+{
+	char *repstr;
+
+	if (!(repstr = dm_pool_zalloc(mem, 7))) {
+		log_error("dm_pool_alloc failed");
+		return NULL;
+	}
+
+	if (vg->status & LVM_WRITE)
+		repstr[0] = 'w';
+	else
+		repstr[0] = 'r';
+
+	if (vg_is_resizeable(vg))
+		repstr[1] = 'z';
+	else
+		repstr[1] = '-';
+
+	if (vg_is_exported(vg))
+		repstr[2] = 'x';
+	else
+		repstr[2] = '-';
+
+	if (vg_missing_pv_count(vg))
+		repstr[3] = 'p';
+	else
+		repstr[3] = '-';
+
+	repstr[4] = _alloc_policy_char(vg->alloc);
+
+	if (vg_is_clustered(vg))
+		repstr[5] = 'c';
+	else
+		repstr[5] = '-';
+	return repstr;
+}
+
+static int _lv_mimage_in_sync(const struct logical_volume *lv)
+{
+	float percent;
+	percent_range_t percent_range;
+	struct lv_segment *mirror_seg = find_mirror_seg(first_seg(lv));
+
+	if (!(lv->status & MIRROR_IMAGE) || !mirror_seg)
+		return_0;
+
+	if (!lv_mirror_percent(lv->vg->cmd, mirror_seg->lv, 0, &percent,
+			       &percent_range, NULL))
+		return_0;
+
+	return (percent_range == PERCENT_100) ? 1 : 0;
+}
+
+char *lv_attr(struct dm_pool *mem, const struct logical_volume *lv)
+{
+	float snap_percent;
+	percent_range_t percent_range;
+	struct lvinfo info;
+	char *repstr;
+
+	if (!(repstr = dm_pool_zalloc(mem, 7))) {
+		log_error("dm_pool_alloc failed");
+		return 0;
+	}
+
+	/* Blank if this is a "free space" LV. */
+	if (!*lv->name)
+		goto out;
+
+	if (lv->status & PVMOVE)
+		repstr[0] = 'p';
+	else if (lv->status & CONVERTING)
+		repstr[0] = 'c';
+	else if (lv->status & VIRTUAL)
+		repstr[0] = 'v';
+	/* Origin takes precedence over Mirror */
+	else if (lv_is_origin(lv)) {
+		if (lv_is_merging_origin(lv))
+			repstr[0] = 'O';
+		else
+			repstr[0] = 'o';
+	}
+	else if (lv->status & MIRRORED) {
+		if (lv->status & MIRROR_NOTSYNCED)
+			repstr[0] = 'M';
+		else
+			repstr[0] = 'm';
+	}else if (lv->status & MIRROR_IMAGE)
+		if (_lv_mimage_in_sync(lv))
+			repstr[0] = 'i';
+		else
+			repstr[0] = 'I';
+	else if (lv->status & MIRROR_LOG)
+		repstr[0] = 'l';
+	else if (lv_is_cow(lv)) {
+		if (lv_is_merging_cow(lv))
+			repstr[0] = 'S';
+		else
+			repstr[0] = 's';
+	} else
+		repstr[0] = '-';
+
+	if (lv->status & PVMOVE)
+		repstr[1] = '-';
+	else if (lv->status & LVM_WRITE)
+		repstr[1] = 'w';
+	else if (lv->status & LVM_READ)
+		repstr[1] = 'r';
+	else
+		repstr[1] = '-';
+
+	repstr[2] = _alloc_policy_char(lv->alloc);
+
+	if (lv->status & LOCKED)
+		repstr[2] = toupper(repstr[2]);
+
+	if (lv->status & FIXED_MINOR)
+		repstr[3] = 'm';	/* Fixed Minor */
+	else
+		repstr[3] = '-';
+
+	if (lv_info(lv->vg->cmd, lv, 0, &info, 1, 0) && info.exists) {
+		if (info.suspended)
+			repstr[4] = 's';	/* Suspended */
+		else if (info.live_table)
+			repstr[4] = 'a';	/* Active */
+		else if (info.inactive_table)
+			repstr[4] = 'i';	/* Inactive with table */
+		else
+			repstr[4] = 'd';	/* Inactive without table */
+
+		/* Snapshot dropped? */
+		if (info.live_table && lv_is_cow(lv) &&
+		    (!lv_snapshot_percent(lv, &snap_percent, &percent_range) ||
+		     percent_range == PERCENT_INVALID)) {
+			repstr[0] = toupper(repstr[0]);
+			if (info.suspended)
+				repstr[4] = 'S'; /* Susp Inv snapshot */
+			else
+				repstr[4] = 'I'; /* Invalid snapshot */
+		}
+
+		if (info.open_count)
+			repstr[5] = 'o';	/* Open */
+		else
+			repstr[5] = '-';
+	} else {
+		repstr[4] = '-';
+		repstr[5] = '-';
+	}
+out:
+	return repstr;
+}
+
 uint64_t lv_size(const struct logical_volume *lv)
 {
 	return lv->size;
diff --git a/lib/metadata/metadata.h b/lib/metadata/metadata.h
index b570cee..f3e268c 100644
--- a/lib/metadata/metadata.h
+++ b/lib/metadata/metadata.h
@@ -419,5 +419,7 @@ int is_mirror_image_removable(struct logical_volume *mimage_lv, void *baton);
 uint64_t find_min_mda_size(struct dm_list *mdas);
 uint64_t vg_mda_size(const struct volume_group *vg);
 uint64_t vg_mda_free(const struct volume_group *vg);
+char *vg_attr(struct dm_pool *mem, const struct volume_group *vg);
+char *lv_attr(struct dm_pool *mem, const struct logical_volume *lv);
 
 #endif
diff --git a/lib/report/report.c b/lib/report/report.c
index 703060b..2de78a1 100644
--- a/lib/report/report.c
+++ b/lib/report/report.c
@@ -34,22 +34,6 @@ struct lvm_report_object {
 	struct pv_segment *pvseg;
 };
 
-static char _alloc_policy_char(alloc_policy_t alloc)
-{
-	switch (alloc) {
-	case ALLOC_CONTIGUOUS:
-		return 'c';
-	case ALLOC_CLING:
-		return 'l';
-	case ALLOC_NORMAL:
-		return 'n';
-	case ALLOC_ANYWHERE:
-		return 'a';
-	default:
-		return 'i';
-	}
-}
-
 static const uint64_t _minusone64 = UINT64_C(-1);
 static const int32_t _minusone32 = INT32_C(-1);
 
@@ -264,124 +248,16 @@ static int _lvkmin_disp(struct dm_report *rh, struct dm_pool *mem __attribute__(
 	return dm_report_field_int32(rh, field, &_minusone32);
 }
 
-static int _lv_mimage_in_sync(const struct logical_volume *lv)
-{
-	float percent;
-	percent_range_t percent_range;
-	struct lv_segment *mirror_seg = find_mirror_seg(first_seg(lv));
-
-	if (!(lv->status & MIRROR_IMAGE) || !mirror_seg)
-		return_0;
-
-	if (!lv_mirror_percent(lv->vg->cmd, mirror_seg->lv, 0, &percent,
-			       &percent_range, NULL))
-		return_0;
-
-	return (percent_range == PERCENT_100) ? 1 : 0;
-}
-
 static int _lvstatus_disp(struct dm_report *rh __attribute__((unused)), struct dm_pool *mem,
 			  struct dm_report_field *field,
 			  const void *data, void *private __attribute__((unused)))
 {
 	const struct logical_volume *lv = (const struct logical_volume *) data;
-	struct lvinfo info;
 	char *repstr;
-	float snap_percent;
-	percent_range_t percent_range;
 
-	if (!(repstr = dm_pool_zalloc(mem, 7))) {
-		log_error("dm_pool_alloc failed");
+	if (!(repstr = lv_attr(mem, lv)))
 		return 0;
-	}
-
-	/* Blank if this is a "free space" LV. */
-	if (!*lv->name)
-		goto out;
-
-	if (lv->status & PVMOVE)
-		repstr[0] = 'p';
-	else if (lv->status & CONVERTING)
-		repstr[0] = 'c';
-	else if (lv->status & VIRTUAL)
-		repstr[0] = 'v';
-	/* Origin takes precedence over Mirror */
-	else if (lv_is_origin(lv)) {
-		if (lv_is_merging_origin(lv))
-			repstr[0] = 'O';
-		else
-			repstr[0] = 'o';
-	}
-	else if (lv->status & MIRRORED) {
-		if (lv->status & MIRROR_NOTSYNCED)
-			repstr[0] = 'M';
-		else
-			repstr[0] = 'm';
-	}else if (lv->status & MIRROR_IMAGE)
-		if (_lv_mimage_in_sync(lv))
-			repstr[0] = 'i';
-		else
-			repstr[0] = 'I';
-	else if (lv->status & MIRROR_LOG)
-		repstr[0] = 'l';
-	else if (lv_is_cow(lv)) {
-		if (lv_is_merging_cow(lv))
-			repstr[0] = 'S';
-		else
-			repstr[0] = 's';
-	} else
-		repstr[0] = '-';
 
-	if (lv->status & PVMOVE)
-		repstr[1] = '-';
-	else if (lv->status & LVM_WRITE)
-		repstr[1] = 'w';
-	else if (lv->status & LVM_READ)
-		repstr[1] = 'r';
-	else
-		repstr[1] = '-';
-
-	repstr[2] = _alloc_policy_char(lv->alloc);
-
-	if (lv->status & LOCKED)
-		repstr[2] = toupper(repstr[2]);
-
-	if (lv->status & FIXED_MINOR)
-		repstr[3] = 'm';	/* Fixed Minor */
-	else
-		repstr[3] = '-';
-
-	if (lv_info(lv->vg->cmd, lv, 0, &info, 1, 0) && info.exists) {
-		if (info.suspended)
-			repstr[4] = 's';	/* Suspended */
-		else if (info.live_table)
-			repstr[4] = 'a';	/* Active */
-		else if (info.inactive_table)
-			repstr[4] = 'i';	/* Inactive with table */
-		else
-			repstr[4] = 'd';	/* Inactive without table */
-
-		/* Snapshot dropped? */
-		if (info.live_table && lv_is_cow(lv) &&
-		    (!lv_snapshot_percent(lv, &snap_percent, &percent_range) ||
-		     percent_range == PERCENT_INVALID)) {
-			repstr[0] = toupper(repstr[0]);
-			if (info.suspended)
-				repstr[4] = 'S'; /* Susp Inv snapshot */
-			else
-				repstr[4] = 'I'; /* Invalid snapshot */
-		}
-
-		if (info.open_count)
-			repstr[5] = 'o';	/* Open */
-		else
-			repstr[5] = '-';
-	} else {
-		repstr[4] = '-';
-		repstr[5] = '-';
-	}
-
-out:
 	dm_report_field_set_value(field, repstr, NULL);
 	return 1;
 }
@@ -419,37 +295,8 @@ static int _vgstatus_disp(struct dm_report *rh __attribute__((unused)), struct d
 	const struct volume_group *vg = (const struct volume_group *) data;
 	char *repstr;
 
-	if (!(repstr = dm_pool_zalloc(mem, 7))) {
-		log_error("dm_pool_alloc failed");
+	if (!(repstr = vg_attr(mem, vg)))
 		return 0;
-	}
-
-	if (vg->status & LVM_WRITE)
-		repstr[0] = 'w';
-	else
-		repstr[0] = 'r';
-
-	if (vg_is_resizeable(vg))
-		repstr[1] = 'z';
-	else
-		repstr[1] = '-';
-
-	if (vg_is_exported(vg))
-		repstr[2] = 'x';
-	else
-		repstr[2] = '-';
-
-	if (vg_missing_pv_count(vg))
-		repstr[3] = 'p';
-	else
-		repstr[3] = '-';
-
-	repstr[4] = _alloc_policy_char(vg->alloc);
-
-	if (vg_is_clustered(vg))
-		repstr[5] = 'c';
-	else
-		repstr[5] = '-';
 
 	dm_report_field_set_value(field, repstr, NULL);
 	return 1;
-- 
1.7.2.1

[PATCH 02/19] Refactor pvstatus_disp to take pv argument and call common pv_attr function.

[Dave Wysochanski]

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 lib/metadata/metadata.c |   21 +++++++++++++++++++++
 lib/metadata/metadata.h |    1 +
 lib/report/columns.h    |    2 +-
 lib/report/report.c     |   17 +++--------------
 4 files changed, 26 insertions(+), 15 deletions(-)

diff --git a/lib/metadata/metadata.c b/lib/metadata/metadata.c
index 094ffc1..86f07e7 100644
--- a/lib/metadata/metadata.c
+++ b/lib/metadata/metadata.c
@@ -4598,6 +4598,27 @@ char *vg_attr(struct dm_pool *mem, const struct volume_group *vg)
 	return repstr;
 }
 
+char *pv_attr(struct dm_pool *mem, const struct physical_volume *pv)
+{
+	char *repstr;
+
+	if (!(repstr = dm_pool_zalloc(mem, 3))) {
+		log_error("dm_pool_alloc failed");
+		return NULL;
+	}
+
+	if (pv->status & ALLOCATABLE_PV)
+		repstr[0] = 'a';
+	else
+		repstr[0] = '-';
+
+	if (pv->status & EXPORTED_VG)
+		repstr[1] = 'x';
+	else
+		repstr[1] = '-';
+	return repstr;
+}
+
 static int _lv_mimage_in_sync(const struct logical_volume *lv)
 {
 	float percent;
diff --git a/lib/metadata/metadata.h b/lib/metadata/metadata.h
index f3e268c..1749f85 100644
--- a/lib/metadata/metadata.h
+++ b/lib/metadata/metadata.h
@@ -419,6 +419,7 @@ int is_mirror_image_removable(struct logical_volume *mimage_lv, void *baton);
 uint64_t find_min_mda_size(struct dm_list *mdas);
 uint64_t vg_mda_size(const struct volume_group *vg);
 uint64_t vg_mda_free(const struct volume_group *vg);
+char *pv_attr(struct dm_pool *mem, const struct physical_volume *pv);
 char *vg_attr(struct dm_pool *mem, const struct volume_group *vg);
 char *lv_attr(struct dm_pool *mem, const struct logical_volume *lv);
 
diff --git a/lib/report/columns.h b/lib/report/columns.h
index 95ad578..689f6a5 100644
--- a/lib/report/columns.h
+++ b/lib/report/columns.h
@@ -93,7 +93,7 @@ FIELD(PVS, pv, NUM, "1st PE", pe_start, 7, size64, pe_start, "Offset to the star
 FIELD(PVS, pv, NUM, "PSize", id, 5, pvsize, pv_size, "Size of PV in current units.", 0)
 FIELD(PVS, pv, NUM, "PFree", id, 5, pvfree, pv_free, "Total amount of unallocated space in current units.", 0)
 FIELD(PVS, pv, NUM, "Used", id, 4, pvused, pv_used, "Total amount of allocated space in current units.", 0)
-FIELD(PVS, pv, STR, "Attr", status, 4, pvstatus, pv_attr, "Various attributes - see man page.", 0)
+FIELD(PVS, pv, STR, "Attr", id, 4, pvstatus, pv_attr, "Various attributes - see man page.", 0)
 FIELD(PVS, pv, NUM, "PE", pe_count, 3, uint32, pv_pe_count, "Total number of Physical Extents.", 0)
 FIELD(PVS, pv, NUM, "Alloc", pe_alloc_count, 5, uint32, pv_pe_alloc_count, "Total number of allocated Physical Extents.", 0)
 FIELD(PVS, pv, STR, "PV Tags", tags, 7, tags, pv_tags, "Tags, if any.", 0)
diff --git a/lib/report/report.c b/lib/report/report.c
index 2de78a1..b7612a1 100644
--- a/lib/report/report.c
+++ b/lib/report/report.c
@@ -266,23 +266,12 @@ static int _pvstatus_disp(struct dm_report *rh __attribute__((unused)), struct d
 			  struct dm_report_field *field,
 			  const void *data, void *private __attribute__((unused)))
 {
-	const uint32_t status = *(const uint32_t *) data;
+	const struct physical_volume *pv =
+	    (const struct physical_volume *) data;
 	char *repstr;
 
-	if (!(repstr = dm_pool_zalloc(mem, 3))) {
-		log_error("dm_pool_alloc failed");
+	if (!(repstr = pv_attr(mem, pv)))
 		return 0;
-	}
-
-	if (status & ALLOCATABLE_PV)
-		repstr[0] = 'a';
-	else
-		repstr[0] = '-';
-
-	if (status & EXPORTED_VG)
-		repstr[1] = 'x';
-	else
-		repstr[1] = '-';
 
 	dm_report_field_set_value(field, repstr, NULL);
 	return 1;
-- 
1.7.2.1

[PATCH 03/19] Add id_format_and_copy() uuid function to allocate and format a uuid.

[Dave Wysochanski]

Add supporting uuid function to allocate memory and call id_write_format.
Will be used from reporting functions as well as property functions.

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

diff --git a/lib/uuid/uuid.c b/lib/uuid/uuid.c
index de3f0cd..e85e852 100644
--- a/lib/uuid/uuid.c
+++ b/lib/uuid/uuid.c
@@ -206,3 +206,18 @@ int id_read_format(struct id *id, const char *buffer)
 
 	return id_valid(id);
 }
+
+char *id_format_and_copy(struct dm_pool *mem, const struct id *id)
+{
+	char *repstr = NULL;
+
+	if (!(repstr = dm_pool_alloc(mem, 40))) {
+		log_error("dm_pool_alloc failed");
+		return NULL;
+	}
+
+	if (!id_write_format(id, repstr, 40))
+		return_NULL;
+
+	return repstr;
+}
diff --git a/lib/uuid/uuid.h b/lib/uuid/uuid.h
index 0029639..5c8382d 100644
--- a/lib/uuid/uuid.h
+++ b/lib/uuid/uuid.h
@@ -54,4 +54,6 @@ int id_write_format(const struct id *id, char *buffer, size_t size);
  */
 int id_read_format(struct id *id, const char *buffer);
 
+char *id_format_and_copy(struct dm_pool *mem, const struct id *id);
+
 #endif
-- 
1.7.2.1

[PATCH 04/19] Call id_format_and_copy from _uuid_disp.

[Dave Wysochanski]

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

diff --git a/lib/report/report.c b/lib/report/report.c
index b7612a1..940ee25 100644
--- a/lib/report/report.c
+++ b/lib/report/report.c
@@ -677,13 +677,8 @@ static int _uuid_disp(struct dm_report *rh __attribute__((unused)), struct dm_po
 {
 	char *repstr = NULL;
 
-	if (!(repstr = dm_pool_alloc(mem, 40))) {
-		log_error("dm_pool_alloc failed");
+	if (!(repstr = id_format_and_copy(mem, (struct id *)data)))
 		return 0;
-	}
-
-	if (!id_write_format((const struct id *) data, repstr, 40))
-		return_0;
 
 	dm_report_field_set_value(field, repstr, NULL);
 	return 1;
-- 
1.7.2.1

[PATCH 05/19] Add pv_uuid, vg_uuid, and lv_uuid, and call id_format_and_copy.

[Dave Wysochanski]

Add supporting functions for pv_uuid, vg_uuid, and lv_uuid.
Call new function id_format_and_copy.  Use 'const' where appropriate.

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 lib/metadata/metadata.c |   15 +++++++++++++++
 lib/metadata/metadata.h |    3 +++
 liblvm/lvm_lv.c         |   10 ++--------
 liblvm/lvm_pv.c         |   10 ++--------
 liblvm/lvm_vg.c         |    8 +-------
 5 files changed, 23 insertions(+), 23 deletions(-)

diff --git a/lib/metadata/metadata.c b/lib/metadata/metadata.c
index 86f07e7..cb49d0f 100644
--- a/lib/metadata/metadata.c
+++ b/lib/metadata/metadata.c
@@ -4619,6 +4619,21 @@ char *pv_attr(struct dm_pool *mem, const struct physical_volume *pv)
 	return repstr;
 }
 
+char *pv_uuid(const struct physical_volume *pv)
+{
+	return id_format_and_copy(pv->vg->vgmem, &pv->id);
+}
+
+char *vg_uuid(const struct volume_group *vg)
+{
+	return id_format_and_copy(vg->vgmem, &vg->id);
+}
+
+char *lv_uuid(const struct logical_volume *lv)
+{
+	return id_format_and_copy(lv->vg->vgmem, &lv->lvid.id[1]);
+}
+
 static int _lv_mimage_in_sync(const struct logical_volume *lv)
 {
 	float percent;
diff --git a/lib/metadata/metadata.h b/lib/metadata/metadata.h
index 1749f85..6e6d4c5 100644
--- a/lib/metadata/metadata.h
+++ b/lib/metadata/metadata.h
@@ -422,5 +422,8 @@ uint64_t vg_mda_free(const struct volume_group *vg);
 char *pv_attr(struct dm_pool *mem, const struct physical_volume *pv);
 char *vg_attr(struct dm_pool *mem, const struct volume_group *vg);
 char *lv_attr(struct dm_pool *mem, const struct logical_volume *lv);
+char *lv_uuid(const struct logical_volume *lv);
+char *vg_uuid(const struct volume_group *vg);
+char *pv_uuid(const struct physical_volume *pv);
 
 #endif
diff --git a/liblvm/lvm_lv.c b/liblvm/lvm_lv.c
index 4519a7b..2cc7530 100644
--- a/liblvm/lvm_lv.c
+++ b/liblvm/lvm_lv.c
@@ -14,7 +14,7 @@
 
 #include "lib.h"
 #include "lvm2app.h"
-#include "metadata-exported.h"
+#include "metadata.h"
 #include "lvm-string.h"
 #include "defaults.h"
 #include "segtype.h"
@@ -39,13 +39,7 @@ uint64_t lvm_lv_get_size(const lv_t lv)
 
 const char *lvm_lv_get_uuid(const lv_t lv)
 {
-	char uuid[64] __attribute__((aligned(8)));
-
-	if (!id_write_format(&lv->lvid.id[1], uuid, sizeof(uuid))) {
-		log_error(INTERNAL_ERROR "unable to convert uuid");
-		return NULL;
-	}
-	return dm_pool_strndup(lv->vg->vgmem, (const char *)uuid, 64);
+	return lv_uuid(lv);
 }
 
 const char *lvm_lv_get_name(const lv_t lv)
diff --git a/liblvm/lvm_pv.c b/liblvm/lvm_pv.c
index db2383c..e3962e4 100644
--- a/liblvm/lvm_pv.c
+++ b/liblvm/lvm_pv.c
@@ -14,18 +14,12 @@
 
 #include "lib.h"
 #include "lvm2app.h"
-#include "metadata-exported.h"
+#include "metadata.h"
 #include "lvm-string.h"
 
 const char *lvm_pv_get_uuid(const pv_t pv)
 {
-	char uuid[64] __attribute__((aligned(8)));
-
-	if (!id_write_format(&pv->id, uuid, sizeof(uuid))) {
-		log_error(INTERNAL_ERROR "Unable to convert uuid");
-		return NULL;
-	}
-	return dm_pool_strndup(pv->vg->vgmem, (const char *)uuid, 64);
+	return pv_uuid(pv);
 }
 
 const char *lvm_pv_get_name(const pv_t pv)
diff --git a/liblvm/lvm_vg.c b/liblvm/lvm_vg.c
index a75652d..a2f8e31 100644
--- a/liblvm/lvm_vg.c
+++ b/liblvm/lvm_vg.c
@@ -327,13 +327,7 @@ uint64_t lvm_vg_get_max_lv(const vg_t vg)
 
 const char *lvm_vg_get_uuid(const vg_t vg)
 {
-	char uuid[64] __attribute__((aligned(8)));
-
-	if (!id_write_format(&vg->id, uuid, sizeof(uuid))) {
-		log_error(INTERNAL_ERROR "Unable to convert uuid");
-		return NULL;
-	}
-	return dm_pool_strndup(vg->vgmem, (const char *)uuid, 64);
+	return vg_uuid(vg);
 }
 
 const char *lvm_vg_get_name(const vg_t vg)
-- 
1.7.2.1

[PATCH 06/19] Add tags_format_and_copy() common function to format tags strings.

[Dave Wysochanski]

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

diff --git a/lib/metadata/metadata.c b/lib/metadata/metadata.c
index cb49d0f..7e8d8f1 100644
--- a/lib/metadata/metadata.c
+++ b/lib/metadata/metadata.c
@@ -4751,6 +4751,30 @@ out:
 	return repstr;
 }
 
+char *tags_format_and_copy(struct dm_pool *mem, const struct dm_list *tags)
+{
+	struct str_list *sl;
+
+	if (!dm_pool_begin_object(mem, 256)) {
+		log_error("dm_pool_begin_object failed");
+		return NULL;
+	}
+
+	dm_list_iterate_items(sl, tags) {
+		if (!dm_pool_grow_object(mem, sl->str, strlen(sl->str)) ||
+		    (sl->list.n != tags && !dm_pool_grow_object(mem, ",", 1))) {
+			log_error("dm_pool_grow_object failed");
+			return NULL;
+		}
+	}
+
+	if (!dm_pool_grow_object(mem, "\0", 1)) {
+		log_error("dm_pool_grow_object failed");
+		return NULL;
+	}
+	return dm_pool_end_object(mem);
+}
+
 uint64_t lv_size(const struct logical_volume *lv)
 {
 	return lv->size;
diff --git a/lib/metadata/metadata.h b/lib/metadata/metadata.h
index 6e6d4c5..64da490 100644
--- a/lib/metadata/metadata.h
+++ b/lib/metadata/metadata.h
@@ -425,5 +425,6 @@ char *lv_attr(struct dm_pool *mem, const struct logical_volume *lv);
 char *lv_uuid(const struct logical_volume *lv);
 char *vg_uuid(const struct volume_group *vg);
 char *pv_uuid(const struct physical_volume *pv);
+char *tags_format_and_copy(struct dm_pool *mem, const struct dm_list *tags);
 
 #endif
diff --git a/lib/report/report.c b/lib/report/report.c
index 940ee25..ed53fac 100644
--- a/lib/report/report.c
+++ b/lib/report/report.c
@@ -150,27 +150,12 @@ static int _tags_disp(struct dm_report *rh __attribute__((unused)), struct dm_po
 		      const void *data, void *private __attribute__((unused)))
 {
 	const struct dm_list *tags = (const struct dm_list *) data;
-	struct str_list *sl;
+	char *tags_str;
 
-	if (!dm_pool_begin_object(mem, 256)) {
-		log_error("dm_pool_begin_object failed");
-		return 0;
-	}
-
-	dm_list_iterate_items(sl, tags) {
-		if (!dm_pool_grow_object(mem, sl->str, strlen(sl->str)) ||
-		    (sl->list.n != tags && !dm_pool_grow_object(mem, ",", 1))) {
-			log_error("dm_pool_grow_object failed");
-			return 0;
-		}
-	}
-
-	if (!dm_pool_grow_object(mem, "\0", 1)) {
-		log_error("dm_pool_grow_object failed");
+	if (!(tags_str = tags_format_and_copy(mem, tags)))
 		return 0;
-	}
 
-	dm_report_field_set_value(field, dm_pool_end_object(mem), NULL);
+	dm_report_field_set_value(field, tags_str, NULL);
 
 	return 1;
 }
-- 
1.7.2.1

[PATCH 07/19] Add pv_tags, vg_tags, lv_tags functions that call tags_format_and_copy.

[Dave Wysochanski]

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

diff --git a/lib/metadata/metadata.c b/lib/metadata/metadata.c
index 7e8d8f1..704e77b 100644
--- a/lib/metadata/metadata.c
+++ b/lib/metadata/metadata.c
@@ -4775,6 +4775,21 @@ char *tags_format_and_copy(struct dm_pool *mem, const struct dm_list *tags)
 	return dm_pool_end_object(mem);
 }
 
+char *pv_tags(const struct physical_volume *pv)
+{
+	return tags_format_and_copy(pv->vg->vgmem, &pv->tags);
+}
+
+char *vg_tags(const struct volume_group *vg)
+{
+	return tags_format_and_copy(vg->vgmem, &vg->tags);
+}
+
+char *lv_tags(const struct logical_volume *lv)
+{
+	return tags_format_and_copy(lv->vg->vgmem, &lv->tags);
+}
+
 uint64_t lv_size(const struct logical_volume *lv)
 {
 	return lv->size;
diff --git a/lib/metadata/metadata.h b/lib/metadata/metadata.h
index 64da490..47fcd08 100644
--- a/lib/metadata/metadata.h
+++ b/lib/metadata/metadata.h
@@ -426,5 +426,8 @@ char *lv_uuid(const struct logical_volume *lv);
 char *vg_uuid(const struct volume_group *vg);
 char *pv_uuid(const struct physical_volume *pv);
 char *tags_format_and_copy(struct dm_pool *mem, const struct dm_list *tags);
+char *pv_tags(const struct physical_volume *pv);
+char *vg_tags(const struct volume_group *vg);
+char *lv_tags(const struct logical_volume *lv);
 
 #endif
-- 
1.7.2.1

[PATCH 08/19] Add GET_STR_PROPERTY_FN macro.

[Dave Wysochanski]

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 lib/report/properties.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/lib/report/properties.c b/lib/report/properties.c
index 9e3cad3..5f8f2ee 100644
--- a/lib/report/properties.c
+++ b/lib/report/properties.c
@@ -29,6 +29,15 @@ static int _ ## NAME ## _get (void *obj, struct lvm_property_type *prop) \
 	return 1; \
 }
 
+#define GET_STR_PROPERTY_FN(NAME, VALUE) \
+static int _ ## NAME ## _get (void *obj, struct lvm_property_type *prop) \
+{ \
+	struct volume_group *vg = (struct volume_group *)obj; \
+\
+	prop->v.s_val = (char *)VALUE;	\
+	return 1; \
+}
+
 static int _not_implemented(void *obj, struct lvm_property_type *prop)
 {
 	log_errno(ENOSYS, "Function not implemented");
-- 
1.7.2.1

[PATCH 09/19] Add 'get' functions for a few vg string fields, vg_name, vg_fmt, vg_sysid.

[Dave Wysochanski]

Add _vg_name_get, _vg_fmt_get, and _vg_sysid_get functions.
Place the static functions inside properties.c to avoid namespace conflict
with vg_name(), and as there are no other callers.

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

diff --git a/lib/report/properties.c b/lib/report/properties.c
index 5f8f2ee..fe867c0 100644
--- a/lib/report/properties.c
+++ b/lib/report/properties.c
@@ -44,6 +44,23 @@ static int _not_implemented(void *obj, struct lvm_property_type *prop)
 	return 0;
 }
 
+static char *vg_fmt(const struct volume_group *vg)
+{
+	if (!vg->fid || !vg->fid->fmt)
+		return NULL;
+	return dm_pool_strdup(vg->vgmem, vg->fid->fmt->name);
+}
+
+static char *vg_name(const struct volume_group *vg)
+{
+	return dm_pool_strdup(vg->vgmem, vg->name);
+}
+
+static char *vg_system_id(const struct volume_group *vg)
+{
+	return dm_pool_strdup(vg->vgmem, vg->system_id);
+}
+
 /* PV */
 #define _pv_fmt_get _not_implemented
 #define _pv_fmt_set _not_implemented
@@ -123,11 +140,11 @@ static int _not_implemented(void *obj, struct lvm_property_type *prop)
 #define _modules_set _not_implemented
 
 /* VG */
-#define _vg_fmt_get _not_implemented
+GET_STR_PROPERTY_FN(vg_fmt, vg_fmt(vg))
 #define _vg_fmt_set _not_implemented
 #define _vg_uuid_get _not_implemented
 #define _vg_uuid_set _not_implemented
-#define _vg_name_get _not_implemented
+GET_STR_PROPERTY_FN(vg_name, vg_name(vg))
 #define _vg_name_set _not_implemented
 #define _vg_attr_get _not_implemented
 #define _vg_attr_set _not_implemented
@@ -135,7 +152,7 @@ GET_NUM_PROPERTY_FN(vg_size, (SECTOR_SIZE * vg_size(vg)))
 #define _vg_size_set _not_implemented
 GET_NUM_PROPERTY_FN(vg_free, (SECTOR_SIZE * vg_free(vg)))
 #define _vg_free_set _not_implemented
-#define _vg_sysid_get _not_implemented
+GET_STR_PROPERTY_FN(vg_sysid, vg_system_id(vg))
 #define _vg_sysid_set _not_implemented
 GET_NUM_PROPERTY_FN(vg_extent_size, vg->extent_size)
 #define _vg_extent_size_set _not_implemented
-- 
1.7.2.1

[PATCH 10/19] Add vg_uuid, vg_attr, vg_tags 'get' functions.

[Dave Wysochanski]

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

diff --git a/lib/report/properties.c b/lib/report/properties.c
index fe867c0..1880ef7 100644
--- a/lib/report/properties.c
+++ b/lib/report/properties.c
@@ -142,11 +142,11 @@ static char *vg_system_id(const struct volume_group *vg)
 /* VG */
 GET_STR_PROPERTY_FN(vg_fmt, vg_fmt(vg))
 #define _vg_fmt_set _not_implemented
-#define _vg_uuid_get _not_implemented
+GET_STR_PROPERTY_FN(vg_uuid, vg_uuid(vg))
 #define _vg_uuid_set _not_implemented
 GET_STR_PROPERTY_FN(vg_name, vg_name(vg))
 #define _vg_name_set _not_implemented
-#define _vg_attr_get _not_implemented
+GET_STR_PROPERTY_FN(vg_attr, vg_attr(vg->vgmem, vg))
 #define _vg_attr_set _not_implemented
 GET_NUM_PROPERTY_FN(vg_size, (SECTOR_SIZE * vg_size(vg)))
 #define _vg_size_set _not_implemented
@@ -172,7 +172,7 @@ GET_NUM_PROPERTY_FN(snap_count, (snapshot_count(vg)))
 #define _snap_count_set _not_implemented
 GET_NUM_PROPERTY_FN(vg_seqno, vg->seqno)
 #define _vg_seqno_set _not_implemented
-#define _vg_tags_get _not_implemented
+GET_STR_PROPERTY_FN(vg_tags, vg_tags(vg))
 #define _vg_tags_set _not_implemented
 GET_NUM_PROPERTY_FN(vg_mda_count, (vg_mda_count(vg)))
 #define _vg_mda_count_set _not_implemented
-- 
1.7.2.1

[PATCH 11/19] Add lvm_vg_get_property() generic vg property function.

[Dave Wysochanski]

Add a generic VG property function to lvm2app.  Call the internal library
vg_get_property() function.  Strings are dup'd internally.

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 liblvm/lvm2app.h |   42 ++++++++++++++++++++++++++++++++++++++++++
 liblvm/lvm_vg.c  |   19 +++++++++++++++++++
 2 files changed, 61 insertions(+), 0 deletions(-)

diff --git a/liblvm/lvm2app.h b/liblvm/lvm2app.h
index 47d3417..cc839a6 100644
--- a/liblvm/lvm2app.h
+++ b/liblvm/lvm2app.h
@@ -168,6 +168,22 @@ typedef struct lvm_str_list {
 	const char *str;
 } lvm_str_list_t;
 
+/**
+ * Property Value
+ *
+ * This structure defines a single LVM property value for an LVM object.
+ * The structures are returned by functions such as
+ * lvm_vg_get_property() and lvm_vg_set_property().
+ */
+typedef struct lvm_property_value {
+	unsigned is_writeable;
+	unsigned is_string;
+	union {
+		char *s_val;
+		uint64_t n_val;
+	} v;
+} lvm_property_value_t;
+
 /*************************** generic lvm handling ***************************/
 /**
  * Create a LVM handle.
@@ -848,6 +864,32 @@ uint64_t lvm_vg_get_max_lv(const vg_t vg);
  */
 struct dm_list *lvm_vg_get_tags(const vg_t vg);
 
+/**
+ * Get the value of a VG property
+ *
+ * \memberof vg_t
+ *
+ * The memory allocated for a string property value is tied to the vg_t
+ * handle and will be released when lvm_vg_close() is called.
+ *
+ * Example:
+ *      lvm_property_value value;
+ *
+ *      if (lvm_vg_get_property(vg, "vg_mda_count", &value) < 0) {
+ *              // handle error
+ *      }
+ *      if (value.is_string)
+ *           printf(", value = %s\n", value.u.s_val);
+ *	else
+ *           printf(", value = %"PRIu64"\n", value.u.n_val);
+ *
+ *
+ * \return
+ * 0 (success) or -1 (failure).
+ */
+int lvm_vg_get_property(vg_t vg, const char *name,
+			struct lvm_property_value *value);
+
 /************************** logical volume handling *************************/
 
 /**
diff --git a/liblvm/lvm_vg.c b/liblvm/lvm_vg.c
index a2f8e31..9a72bec 100644
--- a/liblvm/lvm_vg.c
+++ b/liblvm/lvm_vg.c
@@ -20,6 +20,7 @@
 #include "locking.h"
 #include "lvmcache.h"
 #include "lvm_misc.h"
+#include "properties.h"
 
 int lvm_vg_add_tag(vg_t vg, const char *tag)
 {
@@ -335,6 +336,24 @@ const char *lvm_vg_get_name(const vg_t vg)
 	return dm_pool_strndup(vg->vgmem, (const char *)vg->name, NAME_LEN+1);
 }
 
+
+int lvm_vg_get_property(vg_t vg, const char *name,
+			struct lvm_property_value *value)
+{
+	struct lvm_property_type prop;
+
+	strncpy(prop.id, name, LVM_PROPERTY_NAME_LEN);
+	if (!vg_get_property(vg, &prop))
+		return -1;
+	value->is_writeable = prop.is_writeable;
+	value->is_string = prop.is_string;
+	if (value->is_string)
+		value->v.s_val = prop.v.s_val;
+	else
+		value->v.n_val = prop.v.n_val;
+	return 0;
+}
+
 struct dm_list *lvm_list_vg_names(lvm_t libh)
 {
 	return get_vgnames((struct cmd_context *)libh, 0);
-- 
1.7.2.1

[PATCH 12/19] Add tests for lvm_vg_get_property().

[Dave Wysochanski]

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 test/api/test.c |   33 +++++++++++++++++++++++++++++++++
 1 files changed, 33 insertions(+), 0 deletions(-)

diff --git a/test/api/test.c b/test/api/test.c
index f89840c..4560655 100644
--- a/test/api/test.c
+++ b/test/api/test.c
@@ -91,6 +91,8 @@ static void _show_help(void)
 	       "Issue a lvm_config_override() with accept device filter\n");
 	printf("'vg_get_tags vgname': "
 	       "List the tags of a VG\n");
+	printf("'vg_get_property vgname property_name': "
+	       "Display the value of VG property\n");
 	printf("'lv_get_tags vgname lvname': "
 	       "List the tags of a LV\n");
 	printf("'vg_{add|remove}_tag vgname tag': "
@@ -546,6 +548,35 @@ static void _vg_tag(char **argv, int argc, int add)
 	       add ? "adding":"removing", argv[2], argv[1]);
 }
 
+static void _vg_get_property(char **argv, int argc)
+{
+	vg_t vg;
+	struct lvm_property_value value;
+	int rc;
+
+	if (argc < 3) {
+		printf("Please enter vgname, field_id\n");
+		return;
+	}
+	if (!(vg = _lookup_vg_by_name(argv, argc)))
+		return;
+	rc = lvm_vg_get_property(vg, argv[2], &value);
+	if (rc)
+		printf("Error ");
+	else
+		printf("Success ");
+	printf("Obtaining value of property %s in VG %s",
+	       argv[2], argv[1]);
+	if (rc) {
+		printf("\n");
+		return;
+	}
+	if (value.is_string)
+		printf(", value = %s\n", value.v.s_val);
+	else
+		printf(", value = %"PRIu64"\n", value.v.n_val);
+}
+
 static void _lv_get_tags(char **argv, int argc)
 {
 	lv_t lv;
@@ -796,6 +827,8 @@ static int lvmapi_test_shell(lvm_t libh)
 			_vg_tag(argv, argc, 0);
 		} else if (!strcmp(argv[0], "vg_get_tags")) {
 			_vg_get_tags(argv, argc);
+		} else if (!strcmp(argv[0], "vg_get_property")) {
+			_vg_get_property(argv, argc);
 		} else if (!strcmp(argv[0], "lv_add_tag")) {
 			_lv_tag(argv, argc, 1);
 		} else if (!strcmp(argv[0], "lv_remove_tag")) {
-- 
1.7.2.1

[PATCH 13/19] Simplify logic to create 'attr' strings.

[Dave Wysochanski]

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

diff --git a/lib/metadata/metadata.c b/lib/metadata/metadata.c
index 704e77b..de878c3 100644
--- a/lib/metadata/metadata.c
+++ b/lib/metadata/metadata.c
@@ -4569,32 +4569,12 @@ char *vg_attr(struct dm_pool *mem, const struct volume_group *vg)
 		return NULL;
 	}
 
-	if (vg->status & LVM_WRITE)
-		repstr[0] = 'w';
-	else
-		repstr[0] = 'r';
-
-	if (vg_is_resizeable(vg))
-		repstr[1] = 'z';
-	else
-		repstr[1] = '-';
-
-	if (vg_is_exported(vg))
-		repstr[2] = 'x';
-	else
-		repstr[2] = '-';
-
-	if (vg_missing_pv_count(vg))
-		repstr[3] = 'p';
-	else
-		repstr[3] = '-';
-
+	repstr[0] = (vg->status & LVM_WRITE) ? 'w' : 'r';
+	repstr[1] = (vg_is_resizeable(vg)) ? 'z' : '-';
+	repstr[2] = (vg_is_exported(vg)) ? 'x' : '-';
+	repstr[3] = (vg_missing_pv_count(vg)) ? 'p' : '-';
 	repstr[4] = _alloc_policy_char(vg->alloc);
-
-	if (vg_is_clustered(vg))
-		repstr[5] = 'c';
-	else
-		repstr[5] = '-';
+	repstr[5] = (vg_is_clustered(vg)) ? 'c' : '-';
 	return repstr;
 }
 
@@ -4607,15 +4587,8 @@ char *pv_attr(struct dm_pool *mem, const struct physical_volume *pv)
 		return NULL;
 	}
 
-	if (pv->status & ALLOCATABLE_PV)
-		repstr[0] = 'a';
-	else
-		repstr[0] = '-';
-
-	if (pv->status & EXPORTED_VG)
-		repstr[1] = 'x';
-	else
-		repstr[1] = '-';
+	repstr[0] = (pv->status & ALLOCATABLE_PV) ? 'a' : '-';
+	repstr[1] = (pv->status & EXPORTED_VG) ? 'x' : '-';
 	return repstr;
 }
 
@@ -4674,28 +4647,16 @@ char *lv_attr(struct dm_pool *mem, const struct logical_volume *lv)
 		repstr[0] = 'v';
 	/* Origin takes precedence over Mirror */
 	else if (lv_is_origin(lv)) {
-		if (lv_is_merging_origin(lv))
-			repstr[0] = 'O';
-		else
-			repstr[0] = 'o';
+		repstr[0] = (lv_is_merging_origin(lv)) ? 'O' : 'o';
 	}
 	else if (lv->status & MIRRORED) {
-		if (lv->status & MIRROR_NOTSYNCED)
-			repstr[0] = 'M';
-		else
-			repstr[0] = 'm';
+		repstr[0] = (lv->status & MIRROR_NOTSYNCED) ? 'M' : 'm';
 	}else if (lv->status & MIRROR_IMAGE)
-		if (_lv_mimage_in_sync(lv))
-			repstr[0] = 'i';
-		else
-			repstr[0] = 'I';
+		repstr[0] = (_lv_mimage_in_sync(lv)) ? 'i' : 'I';
 	else if (lv->status & MIRROR_LOG)
 		repstr[0] = 'l';
 	else if (lv_is_cow(lv)) {
-		if (lv_is_merging_cow(lv))
-			repstr[0] = 'S';
-		else
-			repstr[0] = 's';
+		repstr[0] = (lv_is_merging_cow(lv)) ? 'S' : 's';
 	} else
 		repstr[0] = '-';
 
@@ -4713,10 +4674,7 @@ char *lv_attr(struct dm_pool *mem, const struct logical_volume *lv)
 	if (lv->status & LOCKED)
 		repstr[2] = toupper(repstr[2]);
 
-	if (lv->status & FIXED_MINOR)
-		repstr[3] = 'm';	/* Fixed Minor */
-	else
-		repstr[3] = '-';
+	repstr[3] = (lv->status & FIXED_MINOR) ? 'm' : '-';
 
 	if (lv_info(lv->vg->cmd, lv, 0, &info, 1, 0) && info.exists) {
 		if (info.suspended)
@@ -4739,10 +4697,7 @@ char *lv_attr(struct dm_pool *mem, const struct logical_volume *lv)
 				repstr[4] = 'I'; /* Invalid snapshot */
 		}
 
-		if (info.open_count)
-			repstr[5] = 'o';	/* Open */
-		else
-			repstr[5] = '-';
+		repstr[5] = (info.open_count) ? 'o' : '-';
 	} else {
 		repstr[4] = '-';
 		repstr[5] = '-';
-- 
1.7.2.1

[PATCH 14/19] Make generic GET_*_PROPERTY_FN macros and define secondary macro for vg, pv, lv.

[Dave Wysochanski]

Will need similar macros for VG, PV and LV, so define a generic one, and just
pass in the struct name and variable name for the specific macro.

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 lib/report/properties.c |   64 ++++++++++++++++++++++++++++-------------------
 1 files changed, 38 insertions(+), 26 deletions(-)

diff --git a/lib/report/properties.c b/lib/report/properties.c
index 1880ef7..0352489 100644
--- a/lib/report/properties.c
+++ b/lib/report/properties.c
@@ -20,23 +20,35 @@
 #include "lvm-types.h"
 #include "metadata.h"
 
-#define GET_NUM_PROPERTY_FN(NAME, VALUE) \
+#define GET_NUM_PROPERTY_FN(NAME, VALUE, STRUCT, VAR)			\
 static int _ ## NAME ## _get (void *obj, struct lvm_property_type *prop) \
 { \
-	struct volume_group *vg = (struct volume_group *)obj; \
+	struct STRUCT *VAR = (struct STRUCT *)obj; \
 \
 	prop->v.n_val = VALUE; \
 	return 1; \
 }
+#define GET_VG_NUM_PROPERTY_FN(NAME, VALUE) \
+	GET_NUM_PROPERTY_FN(NAME, VALUE, volume_group, vg)
+#define GET_PV_NUM_PROPERTY_FN(NAME, VALUE) \
+	GET_NUM_PROPERTY_FN(NAME, VALUE, physical_volume, pv)
+#define GET_LV_NUM_PROPERTY_FN(NAME, VALUE) \
+	GET_NUM_PROPERTY_FN(NAME, VALUE, logical_volume, lv)
 
-#define GET_STR_PROPERTY_FN(NAME, VALUE) \
+#define GET_STR_PROPERTY_FN(NAME, VALUE, STRUCT, VAR)			\
 static int _ ## NAME ## _get (void *obj, struct lvm_property_type *prop) \
 { \
-	struct volume_group *vg = (struct volume_group *)obj; \
+	struct STRUCT *VAR = (struct STRUCT *)obj; \
 \
 	prop->v.s_val = (char *)VALUE;	\
 	return 1; \
 }
+#define GET_VG_STR_PROPERTY_FN(NAME, VALUE) \
+	GET_STR_PROPERTY_FN(NAME, VALUE, volume_group, vg)
+#define GET_PV_STR_PROPERTY_FN(NAME, VALUE) \
+	GET_STR_PROPERTY_FN(NAME, VALUE, physical_volume, pv)
+#define GET_LV_STR_PROPERTY_FN(NAME, VALUE) \
+	GET_STR_PROPERTY_FN(NAME, VALUE, logical_volume, lv)
 
 static int _not_implemented(void *obj, struct lvm_property_type *prop)
 {
@@ -140,49 +152,49 @@ static char *vg_system_id(const struct volume_group *vg)
 #define _modules_set _not_implemented
 
 /* VG */
-GET_STR_PROPERTY_FN(vg_fmt, vg_fmt(vg))
+GET_VG_STR_PROPERTY_FN(vg_fmt, vg_fmt(vg))
 #define _vg_fmt_set _not_implemented
-GET_STR_PROPERTY_FN(vg_uuid, vg_uuid(vg))
+GET_VG_STR_PROPERTY_FN(vg_uuid, vg_uuid(vg))
 #define _vg_uuid_set _not_implemented
-GET_STR_PROPERTY_FN(vg_name, vg_name(vg))
+GET_VG_STR_PROPERTY_FN(vg_name, vg_name(vg))
 #define _vg_name_set _not_implemented
-GET_STR_PROPERTY_FN(vg_attr, vg_attr(vg->vgmem, vg))
+GET_VG_STR_PROPERTY_FN(vg_attr, vg_attr(vg->vgmem, vg))
 #define _vg_attr_set _not_implemented
-GET_NUM_PROPERTY_FN(vg_size, (SECTOR_SIZE * vg_size(vg)))
+GET_VG_NUM_PROPERTY_FN(vg_size, (SECTOR_SIZE * vg_size(vg)))
 #define _vg_size_set _not_implemented
-GET_NUM_PROPERTY_FN(vg_free, (SECTOR_SIZE * vg_free(vg)))
+GET_VG_NUM_PROPERTY_FN(vg_free, (SECTOR_SIZE * vg_free(vg)))
 #define _vg_free_set _not_implemented
-GET_STR_PROPERTY_FN(vg_sysid, vg_system_id(vg))
+GET_VG_STR_PROPERTY_FN(vg_sysid, vg_system_id(vg))
 #define _vg_sysid_set _not_implemented
-GET_NUM_PROPERTY_FN(vg_extent_size, vg->extent_size)
+GET_VG_NUM_PROPERTY_FN(vg_extent_size, vg->extent_size)
 #define _vg_extent_size_set _not_implemented
-GET_NUM_PROPERTY_FN(vg_extent_count, vg->extent_count)
+GET_VG_NUM_PROPERTY_FN(vg_extent_count, vg->extent_count)
 #define _vg_extent_count_set _not_implemented
-GET_NUM_PROPERTY_FN(vg_free_count, vg->free_count)
+GET_VG_NUM_PROPERTY_FN(vg_free_count, vg->free_count)
 #define _vg_free_count_set _not_implemented
-GET_NUM_PROPERTY_FN(max_lv, vg->max_lv)
+GET_VG_NUM_PROPERTY_FN(max_lv, vg->max_lv)
 #define _max_lv_set _not_implemented
-GET_NUM_PROPERTY_FN(max_pv, vg->max_pv)
+GET_VG_NUM_PROPERTY_FN(max_pv, vg->max_pv)
 #define _max_pv_set _not_implemented
-GET_NUM_PROPERTY_FN(pv_count, vg->pv_count)
+GET_VG_NUM_PROPERTY_FN(pv_count, vg->pv_count)
 #define _pv_count_set _not_implemented
-GET_NUM_PROPERTY_FN(lv_count, (vg_visible_lvs(vg)))
+GET_VG_NUM_PROPERTY_FN(lv_count, (vg_visible_lvs(vg)))
 #define _lv_count_set _not_implemented
-GET_NUM_PROPERTY_FN(snap_count, (snapshot_count(vg)))
+GET_VG_NUM_PROPERTY_FN(snap_count, (snapshot_count(vg)))
 #define _snap_count_set _not_implemented
-GET_NUM_PROPERTY_FN(vg_seqno, vg->seqno)
+GET_VG_NUM_PROPERTY_FN(vg_seqno, vg->seqno)
 #define _vg_seqno_set _not_implemented
-GET_STR_PROPERTY_FN(vg_tags, vg_tags(vg))
+GET_VG_STR_PROPERTY_FN(vg_tags, vg_tags(vg))
 #define _vg_tags_set _not_implemented
-GET_NUM_PROPERTY_FN(vg_mda_count, (vg_mda_count(vg)))
+GET_VG_NUM_PROPERTY_FN(vg_mda_count, (vg_mda_count(vg)))
 #define _vg_mda_count_set _not_implemented
-GET_NUM_PROPERTY_FN(vg_mda_used_count, (vg_mda_used_count(vg)))
+GET_VG_NUM_PROPERTY_FN(vg_mda_used_count, (vg_mda_used_count(vg)))
 #define _vg_mda_used_count_set _not_implemented
-GET_NUM_PROPERTY_FN(vg_mda_free, (SECTOR_SIZE * vg_mda_free(vg)))
+GET_VG_NUM_PROPERTY_FN(vg_mda_free, (SECTOR_SIZE * vg_mda_free(vg)))
 #define _vg_mda_free_set _not_implemented
-GET_NUM_PROPERTY_FN(vg_mda_size, (SECTOR_SIZE * vg_mda_size(vg)))
+GET_VG_NUM_PROPERTY_FN(vg_mda_size, (SECTOR_SIZE * vg_mda_size(vg)))
 #define _vg_mda_size_set _not_implemented
-GET_NUM_PROPERTY_FN(vg_mda_copies, (vg_mda_copies(vg)))
+GET_VG_NUM_PROPERTY_FN(vg_mda_copies, (vg_mda_copies(vg)))
 #define _vg_mda_copies_set _not_implemented
 
 /* LVSEG */
-- 
1.7.2.1

[PATCH 15/19] Add pv_mda_size, pv_mda_free, and pv_used functions.

[Dave Wysochanski]

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 lib/metadata/metadata.c |   44 ++++++++++++++++++++++++++++++++++++++++++++
 lib/metadata/metadata.h |    3 +++
 lib/report/report.c     |   34 +++++++++-------------------------
 3 files changed, 56 insertions(+), 25 deletions(-)

diff --git a/lib/metadata/metadata.c b/lib/metadata/metadata.c
index de878c3..62dbfde 100644
--- a/lib/metadata/metadata.c
+++ b/lib/metadata/metadata.c
@@ -1277,6 +1277,39 @@ uint64_t vg_mda_free(const struct volume_group *vg)
 	return freespace;
 }
 
+uint64_t pv_mda_size(const struct physical_volume *pv)
+{
+	struct lvmcache_info *info;
+	uint64_t min_mda_size = 0;
+	const char *pvid = (const char *)(&pv->id.uuid);
+
+	/* PVs could have 2 mdas of different sizes (rounding effect) */
+	if ((info = info_from_pvid(pvid, 0)))
+		min_mda_size = find_min_mda_size(&info->mdas);
+	return min_mda_size;
+}
+
+uint64_t pv_mda_free(const struct physical_volume *pv)
+{
+	struct lvmcache_info *info;
+	uint64_t freespace = UINT64_MAX, mda_free;
+	const char *pvid = (const char *)&pv->id.uuid;
+	struct metadata_area *mda;
+
+	if ((info = info_from_pvid(pvid, 0)))
+		dm_list_iterate_items(mda, &info->mdas) {
+			if (!mda->ops->mda_free_sectors)
+				continue;
+			mda_free = mda->ops->mda_free_sectors(mda);
+			if (mda_free < freespace)
+				freespace = mda_free;
+		}
+
+	if (freespace == UINT64_MAX)
+		freespace = UINT64_C(0);
+	return freespace;
+}
+
 int vg_set_extent_size(struct volume_group *vg, uint32_t new_size)
 {
 	uint32_t old_size = vg->extent_size;
@@ -4205,6 +4238,17 @@ uint64_t pv_free(const struct physical_volume *pv)
 	return freespace;
 }
 
+uint64_t pv_used(const struct physical_volume *pv)
+{
+	uint64_t used;
+
+	if (!pv->pe_count)
+		used = 0LL;
+	else
+		used = (uint64_t) pv->pe_alloc_count * pv->pe_size;
+	return used;
+}
+
 uint64_t pv_status(const struct physical_volume *pv)
 {
 	return pv_field(pv, status);
diff --git a/lib/metadata/metadata.h b/lib/metadata/metadata.h
index 47fcd08..2a40d8a 100644
--- a/lib/metadata/metadata.h
+++ b/lib/metadata/metadata.h
@@ -419,6 +419,9 @@ int is_mirror_image_removable(struct logical_volume *mimage_lv, void *baton);
 uint64_t find_min_mda_size(struct dm_list *mdas);
 uint64_t vg_mda_size(const struct volume_group *vg);
 uint64_t vg_mda_free(const struct volume_group *vg);
+uint64_t pv_mda_size(const struct physical_volume *pv);
+uint64_t pv_mda_free(const struct physical_volume *pv);
+uint64_t pv_used(const struct physical_volume *pv);
 char *pv_attr(struct dm_pool *mem, const struct physical_volume *pv);
 char *vg_attr(struct dm_pool *mem, const struct volume_group *vg);
 char *lv_attr(struct dm_pool *mem, const struct logical_volume *lv);
diff --git a/lib/report/report.c b/lib/report/report.c
index ed53fac..4861f95 100644
--- a/lib/report/report.c
+++ b/lib/report/report.c
@@ -597,10 +597,7 @@ static int _pvused_disp(struct dm_report *rh, struct dm_pool *mem,
 	    (const struct physical_volume *) data;
 	uint64_t used;
 
-	if (!pv->pe_count)
-		used = 0LL;
-	else
-		used = (uint64_t) pv->pe_alloc_count * pv->pe_size;
+	used = pv_used(pv);
 
 	return _size64_disp(rh, mem, field, &used, private);
 }
@@ -754,22 +751,11 @@ static int _pvmdafree_disp(struct dm_report *rh, struct dm_pool *mem,
 			   struct dm_report_field *field,
 			   const void *data, void *private)
 {
-	struct lvmcache_info *info;
-	uint64_t freespace = UINT64_MAX, mda_free;
-	const char *pvid = (const char *)(&((const struct id *) data)->uuid);
-	struct metadata_area *mda;
-
-	if ((info = info_from_pvid(pvid, 0)))
-		dm_list_iterate_items(mda, &info->mdas) {
-			if (!mda->ops->mda_free_sectors)
-				continue;
-			mda_free = mda->ops->mda_free_sectors(mda);
-			if (mda_free < freespace)
-				freespace = mda_free;
-		}
+	const struct physical_volume *pv =
+	    (const struct physical_volume *) data;
+	uint64_t freespace;
 
-	if (freespace == UINT64_MAX)
-		freespace = UINT64_C(0);
+	freespace = pv_mda_free(pv);
 
 	return _size64_disp(rh, mem, field, &freespace, private);
 }
@@ -778,13 +764,11 @@ static int _pvmdasize_disp(struct dm_report *rh, struct dm_pool *mem,
 			   struct dm_report_field *field,
 			   const void *data, void *private)
 {
-	struct lvmcache_info *info;
-	uint64_t min_mda_size = 0;
-	const char *pvid = (const char *)(&((const struct id *) data)->uuid);
+	const struct physical_volume *pv =
+	    (const struct physical_volume *) data;
+	uint64_t min_mda_size;
 
-	/* PVs could have 2 mdas of different sizes (rounding effect) */
-	if ((info = info_from_pvid(pvid, 0)))
-		min_mda_size = find_min_mda_size(&info->mdas);
+	min_mda_size = pv_mda_size(pv);
 
 	return _size64_disp(rh, mem, field, &min_mda_size, private);
 }
-- 
1.7.2.1

[PATCH 16/19] Add pv 'get' functions for all pv properties.

[Dave Wysochanski]

Add 'get' functions for all pv properties.
Multiply by SECTOR_SIZE for pv properties pv_mda_free, pv_mda_size,
pe_start, pv_size, pv_free, pv_used.

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 lib/report/properties.c |   44 ++++++++++++++++++++++++++++----------------
 1 files changed, 28 insertions(+), 16 deletions(-)

diff --git a/lib/report/properties.c b/lib/report/properties.c
index 0352489..0b80593 100644
--- a/lib/report/properties.c
+++ b/lib/report/properties.c
@@ -56,6 +56,13 @@ static int _not_implemented(void *obj, struct lvm_property_type *prop)
 	return 0;
 }
 
+static char *pv_fmt(const struct physical_volume *pv)
+{
+	if (!pv->fmt)
+		return NULL;
+	return dm_pool_strdup(pv->vg->vgmem, pv->fmt->name);
+}
+
 static char *vg_fmt(const struct volume_group *vg)
 {
 	if (!vg->fid || !vg->fid->fmt)
@@ -63,6 +70,11 @@ static char *vg_fmt(const struct volume_group *vg)
 	return dm_pool_strdup(vg->vgmem, vg->fid->fmt->name);
 }
 
+static char *pv_name(const struct physical_volume *pv)
+{
+	return dm_pool_strdup(pv->vg->vgmem, dev_name(pv->dev));
+}
+
 static char *vg_name(const struct volume_group *vg)
 {
 	return dm_pool_strdup(vg->vgmem, vg->name);
@@ -74,37 +86,37 @@ static char *vg_system_id(const struct volume_group *vg)
 }
 
 /* PV */
-#define _pv_fmt_get _not_implemented
+GET_PV_STR_PROPERTY_FN(pv_fmt, pv_fmt(pv))
 #define _pv_fmt_set _not_implemented
-#define _pv_uuid_get _not_implemented
+GET_PV_STR_PROPERTY_FN(pv_uuid, pv_uuid(pv))
 #define _pv_uuid_set _not_implemented
-#define _dev_size_get _not_implemented
+GET_PV_NUM_PROPERTY_FN(dev_size, SECTOR_SIZE * pv_dev_size(pv))
 #define _dev_size_set _not_implemented
-#define _pv_name_get _not_implemented
+GET_PV_STR_PROPERTY_FN(pv_name, pv_name(pv))
 #define _pv_name_set _not_implemented
-#define _pv_mda_free_get _not_implemented
+GET_PV_NUM_PROPERTY_FN(pv_mda_free, SECTOR_SIZE * pv_mda_free(pv))
 #define _pv_mda_free_set _not_implemented
-#define _pv_mda_size_get _not_implemented
+GET_PV_NUM_PROPERTY_FN(pv_mda_size, SECTOR_SIZE * pv_mda_size(pv))
 #define _pv_mda_size_set _not_implemented
-#define _pe_start_get _not_implemented
+GET_PV_NUM_PROPERTY_FN(pe_start, SECTOR_SIZE * pv->pe_start)
 #define _pe_start_set _not_implemented
-#define _pv_size_get _not_implemented
+GET_PV_NUM_PROPERTY_FN(pv_size, SECTOR_SIZE * pv_size_field(pv))
 #define _pv_size_set _not_implemented
-#define _pv_free_get _not_implemented
+GET_PV_NUM_PROPERTY_FN(pv_free, SECTOR_SIZE * pv_free(pv))
 #define _pv_free_set _not_implemented
-#define _pv_used_get _not_implemented
+GET_PV_NUM_PROPERTY_FN(pv_used, SECTOR_SIZE * pv_used(pv))
 #define _pv_used_set _not_implemented
-#define _pv_attr_get _not_implemented
+GET_PV_STR_PROPERTY_FN(pv_attr, pv_attr(pv->vg->vgmem, pv))
 #define _pv_attr_set _not_implemented
-#define _pv_pe_count_get _not_implemented
+GET_PV_NUM_PROPERTY_FN(pv_pe_count, pv->pe_count)
 #define _pv_pe_count_set _not_implemented
-#define _pv_pe_alloc_count_get _not_implemented
+GET_PV_NUM_PROPERTY_FN(pv_pe_alloc_count, pv->pe_alloc_count)
 #define _pv_pe_alloc_count_set _not_implemented
-#define _pv_tags_get _not_implemented
+GET_PV_STR_PROPERTY_FN(pv_tags, pv_tags(pv))
 #define _pv_tags_set _not_implemented
-#define _pv_mda_count_get _not_implemented
+GET_PV_NUM_PROPERTY_FN(pv_mda_count, pv_mda_count(pv))
 #define _pv_mda_count_set _not_implemented
-#define _pv_mda_used_count_get _not_implemented
+GET_PV_NUM_PROPERTY_FN(pv_mda_used_count, pv_mda_used_count(pv))
 #define _pv_mda_used_count_set _not_implemented
 
 /* LV */
-- 
1.7.2.1

[PATCH 17/19] Rename internal vg_get_property to more generic lvm_get_property.

[Dave Wysochanski]

We need to use a similar function for pv and lv properties, so just make
the internal library function take a void *.  This makes the interface more
consistent as the lvm_property_type struct contains 'get' and 'set'
methods taking a void *.  The disambiguation occurs when looking up the
field name - see columns.h and the requirement that field names be unique.

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 lib/report/properties.c |    4 ++--
 lib/report/properties.h |    2 +-
 liblvm/lvm_vg.c         |    2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/lib/report/properties.c b/lib/report/properties.c
index 0b80593..95da8ab 100644
--- a/lib/report/properties.c
+++ b/lib/report/properties.c
@@ -262,7 +262,7 @@ struct lvm_property_type _properties[] = {
 #undef FIELD
 
 
-int vg_get_property(struct volume_group *vg, struct lvm_property_type *prop)
+int lvm_get_property(void *obj, struct lvm_property_type *prop)
 {
 	struct lvm_property_type *p;
 
@@ -278,7 +278,7 @@ int vg_get_property(struct volume_group *vg, struct lvm_property_type *prop)
 	}
 
 	*prop = *p;
-	if (!p->get((void *)vg, prop)) {
+	if (!p->get(obj, prop)) {
 		return 0;
 	}
 	return 1;
diff --git a/lib/report/properties.h b/lib/report/properties.h
index 2e1381d..0a13f39 100644
--- a/lib/report/properties.h
+++ b/lib/report/properties.h
@@ -32,6 +32,6 @@ struct lvm_property_type {
 	int (*set) (void *obj, struct lvm_property_type *prop);
 };
 
-int vg_get_property(struct volume_group *vg, struct lvm_property_type *prop);
+int lvm_get_property(void *obj, struct lvm_property_type *prop);
 
 #endif
diff --git a/liblvm/lvm_vg.c b/liblvm/lvm_vg.c
index 9a72bec..98070dd 100644
--- a/liblvm/lvm_vg.c
+++ b/liblvm/lvm_vg.c
@@ -343,7 +343,7 @@ int lvm_vg_get_property(vg_t vg, const char *name,
 	struct lvm_property_type prop;
 
 	strncpy(prop.id, name, LVM_PROPERTY_NAME_LEN);
-	if (!vg_get_property(vg, &prop))
+	if (!lvm_get_property((void *)vg, &prop))
 		return -1;
 	value->is_writeable = prop.is_writeable;
 	value->is_string = prop.is_string;
-- 
1.7.2.1

[PATCH 18/19] Add lvm_pv_get_property() generic function to obtain value of any pv property.

[Dave Wysochanski]

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 liblvm/lvm2app.h |   26 ++++++++++++++++++++++++++
 liblvm/lvm_pv.c  |   18 ++++++++++++++++++
 2 files changed, 44 insertions(+), 0 deletions(-)

diff --git a/liblvm/lvm2app.h b/liblvm/lvm2app.h
index cc839a6..634394a 100644
--- a/liblvm/lvm2app.h
+++ b/liblvm/lvm2app.h
@@ -1222,6 +1222,32 @@ uint64_t lvm_pv_get_size(const pv_t pv);
 uint64_t lvm_pv_get_free(const pv_t pv);
 
 /**
+ * Get the value of a PV property
+ *
+ * \memberof pv_t
+ *
+ * The memory allocated for a string property value is tied to the vg_t
+ * handle and will be released when lvm_vg_close() is called.
+ *
+ * Example:
+ *      lvm_property_value value;
+ *
+ *      if (lvm_pv_get_property(pv, "pv_mda_count", &value) < 0) {
+ *              // handle error
+ *      }
+ *      if (value.is_string)
+ *           printf(", value = %s\n", value.u.s_val);
+ *	else
+ *           printf(", value = %"PRIu64"\n", value.u.n_val);
+ *
+ *
+ * \return
+ * 0 (success) or -1 (failure).
+ */
+int lvm_pv_get_property(pv_t pv, const char *name,
+			struct lvm_property_value *value);
+
+/**
  * Resize physical volume to new_size bytes.
  *
  * \memberof pv_t
diff --git a/liblvm/lvm_pv.c b/liblvm/lvm_pv.c
index e3962e4..5fc73ba 100644
--- a/liblvm/lvm_pv.c
+++ b/liblvm/lvm_pv.c
@@ -16,6 +16,7 @@
 #include "lvm2app.h"
 #include "metadata.h"
 #include "lvm-string.h"
+#include "properties.h"
 
 const char *lvm_pv_get_uuid(const pv_t pv)
 {
@@ -48,6 +49,23 @@ uint64_t lvm_pv_get_free(const pv_t pv)
 	return (uint64_t) SECTOR_SIZE * pv_free(pv);
 }
 
+int lvm_pv_get_property(pv_t pv, const char *name,
+			struct lvm_property_value *value)
+{
+	struct lvm_property_type prop;
+
+	strncpy(prop.id, name, LVM_PROPERTY_NAME_LEN);
+	if (!lvm_get_property((void *)pv, &prop))
+		return -1;
+	value->is_writeable = prop.is_writeable;
+	value->is_string = prop.is_string;
+	if (value->is_string)
+		value->v.s_val = prop.v.s_val;
+	else
+		value->v.n_val = prop.v.n_val;
+	return 0;
+}
+
 int lvm_pv_resize(const pv_t pv, uint64_t new_size)
 {
 	/* FIXME: add pv resize code here */
-- 
1.7.2.1

[PATCH 19/19] Add pv_get_property to interactive test.

[Dave Wysochanski]

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 test/api/test.c |   52 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 51 insertions(+), 1 deletions(-)

diff --git a/test/api/test.c b/test/api/test.c
index 4560655..2579ff0 100644
--- a/test/api/test.c
+++ b/test/api/test.c
@@ -93,6 +93,8 @@ static void _show_help(void)
 	       "List the tags of a VG\n");
 	printf("'vg_get_property vgname property_name': "
 	       "Display the value of VG property\n");
+	printf("'pv_get_property pvname property_name': "
+	       "Display the value of PV property\n");
 	printf("'lv_get_tags vgname lvname': "
 	       "List the tags of a LV\n");
 	printf("'vg_{add|remove}_tag vgname tag': "
@@ -182,6 +184,23 @@ static vg_t _lookup_vg_by_name(char **argv, int argc)
 	}
 	return vg;
 }
+
+static pv_t _lookup_pv_by_name(char **argv, int argc)
+{
+	pv_t pv;
+
+	if (argc < 2) {
+		printf ("Please enter vg_name\n");
+		return NULL;
+	}
+	if (!(pv = dm_hash_lookup(_pvname_hash, argv[1]))) {
+		printf ("Can't find %s in open PVs - run vg_open first\n",
+			argv[1]);
+		return NULL;
+	}
+	return pv;
+}
+
 static void _add_lvs_to_lvname_hash(struct dm_list *lvs)
 {
 	struct lvm_lv_list *lvl;
@@ -548,6 +567,35 @@ static void _vg_tag(char **argv, int argc, int add)
 	       add ? "adding":"removing", argv[2], argv[1]);
 }
 
+static void _pv_get_property(char **argv, int argc)
+{
+	pv_t pv;
+	struct lvm_property_value value;
+	int rc;
+
+	if (argc < 3) {
+		printf("Please enter vgname, field_id\n");
+		return;
+	}
+	if (!(pv = _lookup_pv_by_name(argv, argc)))
+		return;
+	rc = lvm_pv_get_property(pv, argv[2], &value);
+	if (rc)
+		printf("Error ");
+	else
+		printf("Success ");
+	printf("obtaining value of property %s in VG %s",
+	       argv[2], argv[1]);
+	if (rc) {
+		printf("\n");
+		return;
+	}
+	if (value.is_string)
+		printf(", value = %s\n", value.v.s_val);
+	else
+		printf(", value = %"PRIu64"\n", value.v.n_val);
+}
+
 static void _vg_get_property(char **argv, int argc)
 {
 	vg_t vg;
@@ -565,7 +613,7 @@ static void _vg_get_property(char **argv, int argc)
 		printf("Error ");
 	else
 		printf("Success ");
-	printf("Obtaining value of property %s in VG %s",
+	printf("obtaining value of property %s in VG %s",
 	       argv[2], argv[1]);
 	if (rc) {
 		printf("\n");
@@ -829,6 +877,8 @@ static int lvmapi_test_shell(lvm_t libh)
 			_vg_get_tags(argv, argc);
 		} else if (!strcmp(argv[0], "vg_get_property")) {
 			_vg_get_property(argv, argc);
+		} else if (!strcmp(argv[0], "pv_get_property")) {
+			_pv_get_property(argv, argc);
 		} else if (!strcmp(argv[0], "lv_add_tag")) {
 			_lv_tag(argv, argc, 1);
 		} else if (!strcmp(argv[0], "lv_remove_tag")) {
-- 
1.7.2.1

[PATCH 17/19] Rename internal vg_get_property to more generic lvm_get_property.

[Zdenek Kabelac]

Dne 15.9.2010 17:36, Dave Wysochanski napsal(a):
>  lib/report/properties.c |    4 ++--
>  lib/report/properties.h |    2 +-
>  liblvm/lvm_vg.c         |    2 +-
>  3 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/report/properties.c b/lib/report/properties.c
> index 0b80593..95da8ab 100644
> --- a/lib/report/properties.c
> +++ b/lib/report/properties.c
> @@ -262,7 +262,7 @@ struct lvm_property_type _properties[] = {
>  #undef FIELD
>  
>  
> -int vg_get_property(struct volume_group *vg, struct lvm_property_type *prop)
> +int lvm_get_property(void *obj, struct lvm_property_type *prop)

Hope vg_get_property has been marked as unstable API ?


>  {
>  	struct lvm_property_type *p;
>  
> @@ -278,7 +278,7 @@ int vg_get_property(struct volume_group *vg, struct lvm_property_type *prop)
>  	}
>  
>  	*prop = *p;
> -	if (!p->get((void *)vg, prop)) {
> +	if (!p->get(obj, prop)) {
>  		return 0;
>  	}
>  	return 1;
> diff --git a/lib/report/properties.h b/lib/report/properties.h
> index 2e1381d..0a13f39 100644
> --- a/lib/report/properties.h
> +++ b/lib/report/properties.h
> @@ -32,6 +32,6 @@ struct lvm_property_type {
>  	int (*set) (void *obj, struct lvm_property_type *prop);
>  };
>  
> -int vg_get_property(struct volume_group *vg, struct lvm_property_type *prop);
> +int lvm_get_property(void *obj, struct lvm_property_type *prop);
>  
>  #endif
> diff --git a/liblvm/lvm_vg.c b/liblvm/lvm_vg.c
> index 9a72bec..98070dd 100644
> --- a/liblvm/lvm_vg.c
> +++ b/liblvm/lvm_vg.c
> @@ -343,7 +343,7 @@ int lvm_vg_get_property(vg_t vg, const char *name,
>  	struct lvm_property_type prop;
>  
>  	strncpy(prop.id, name, LVM_PROPERTY_NAME_LEN);
> -	if (!vg_get_property(vg, &prop))
> +	if (!lvm_get_property((void *)vg, &prop))

No need to add cast to (void*) - it's C not C++...

Zdenek

[PATCH 02/19] Refactor pvstatus_disp to take pv argument and call common pv_attr function.

[Zdenek Kabelac]

Dne 15.9.2010 17:35, Dave Wysochanski napsal(a):
> 
> Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
> ---
>  lib/metadata/metadata.c |   21 +++++++++++++++++++++
>  lib/metadata/metadata.h |    1 +
>  lib/report/columns.h    |    2 +-
>  lib/report/report.c     |   17 +++--------------
>  4 files changed, 26 insertions(+), 15 deletions(-)
> 
> diff --git a/lib/metadata/metadata.c b/lib/metadata/metadata.c
> index 094ffc1..86f07e7 100644
> --- a/lib/metadata/metadata.c
> +++ b/lib/metadata/metadata.c
> @@ -4598,6 +4598,27 @@ char *vg_attr(struct dm_pool *mem, const struct volume_group *vg)
>  	return repstr;
>  }
>  
> +char *pv_attr(struct dm_pool *mem, const struct physical_volume *pv)
> +{
> +	char *repstr;
> +
> +	if (!(repstr = dm_pool_zalloc(mem, 3))) {
> +		log_error("dm_pool_alloc failed");
> +		return NULL;
> +	}
> +
> +	if (pv->status & ALLOCATABLE_PV)
> +		repstr[0] = 'a';
> +	else
> +		repstr[0] = '-';
> +
> +	if (pv->status & EXPORTED_VG)
> +		repstr[1] = 'x';
> +	else
> +		repstr[1] = '-';
> +	return repstr;

when you are moving this code maybe again little shortening could be made:
c = (x) ? a : b;

Zdenek

[PATCH 02/19] Refactor pvstatus_disp to take pv argument and call common pv_attr function.

[Zdenek Kabelac]

Dne 15.9.2010 17:35, Dave Wysochanski napsal(a):
> 
> Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
> ---
>  lib/metadata/metadata.c |   21 +++++++++++++++++++++
>  lib/metadata/metadata.h |    1 +
>  lib/report/columns.h    |    2 +-
>  lib/report/report.c     |   17 +++--------------
>  4 files changed, 26 insertions(+), 15 deletions(-)
> 
> diff --git a/lib/metadata/metadata.c b/lib/metadata/metadata.c
> index 094ffc1..86f07e7 100644
> --- a/lib/metadata/metadata.c
> +++ b/lib/metadata/metadata.c
> @@ -4598,6 +4598,27 @@ char *vg_attr(struct dm_pool *mem, const struct volume_group *vg)
>  	return repstr;
>  }
>  
> +char *pv_attr(struct dm_pool *mem, const struct physical_volume *pv)
> +{
> +	char *repstr;
> +
> +	if (!(repstr = dm_pool_zalloc(mem, 3))) {
> +		log_error("dm_pool_alloc failed");
> +		return NULL;
> +	}
> +
> +	if (pv->status & ALLOCATABLE_PV)
> +		repstr[0] = 'a';
> +	else
> +		repstr[0] = '-';
> +
> +	if (pv->status & EXPORTED_VG)
> +		repstr[1] = 'x';
> +	else
> +		repstr[1] = '-';
> +	return repstr;

when you are moving this code maybe again little shortening could be made:
c = (x) ? a : b;

Zdenek

[PATCH 04/19] Call id_format_and_copy from _uuid_disp.

[Zdenek Kabelac]

Dne 15.9.2010 17:35, Dave Wysochanski napsal(a):
> 
> Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
> ---
>  lib/report/report.c |    7 +------
>  1 files changed, 1 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/report/report.c b/lib/report/report.c
> index b7612a1..940ee25 100644
> --- a/lib/report/report.c
> +++ b/lib/report/report.c
> @@ -677,13 +677,8 @@ static int _uuid_disp(struct dm_report *rh __attribute__((unused)), struct dm_po
>  {
>  	char *repstr = NULL;
>  
> -	if (!(repstr = dm_pool_alloc(mem, 40))) {
> -		log_error("dm_pool_alloc failed");
> +	if (!(repstr = id_format_and_copy(mem, (struct id *)data)))
>  		return 0;
> -	}
> -
> -	if (!id_write_format((const struct id *) data, repstr, 40))
> -		return_0;

return_0 should be used after NULL detection from id_format_and_copy()

Zdenek

[PATCH 17/19] Rename internal vg_get_property to more generic lvm_get_property.

[Zdenek Kabelac]

Dne 16.9.2010 10:41, Zdenek Kabelac napsal(a):
> Dne 15.9.2010 17:36, Dave Wysochanski napsal(a):
>>  lib/report/properties.c |    4 ++--
>>  lib/report/properties.h |    2 +-
>>  liblvm/lvm_vg.c         |    2 +-
>>  3 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/lib/report/properties.c b/lib/report/properties.c
>> index 0b80593..95da8ab 100644
>> --- a/lib/report/properties.c
>> +++ b/lib/report/properties.c
>> @@ -262,7 +262,7 @@ struct lvm_property_type _properties[] = {
>>  #undef FIELD
>>  
>>  
>> -int vg_get_property(struct volume_group *vg, struct lvm_property_type *prop)
>> +int lvm_get_property(void *obj, struct lvm_property_type *prop)
> 
> Hope vg_get_property has been marked as unstable API ?
> 
> 
>>  {
>>  	struct lvm_property_type *p;
>>  
>> @@ -278,7 +278,7 @@ int vg_get_property(struct volume_group *vg, struct lvm_property_type *prop)
>>  	}
>>  
>>  	*prop = *p;
>> -	if (!p->get((void *)vg, prop)) {
>> +	if (!p->get(obj, prop)) {
>>  		return 0;
>>  	}
>>  	return 1;
>> diff --git a/lib/report/properties.h b/lib/report/properties.h
>> index 2e1381d..0a13f39 100644
>> --- a/lib/report/properties.h
>> +++ b/lib/report/properties.h
>> @@ -32,6 +32,6 @@ struct lvm_property_type {
>>  	int (*set) (void *obj, struct lvm_property_type *prop);
>>  };
>>  
>> -int vg_get_property(struct volume_group *vg, struct lvm_property_type *prop);
>> +int lvm_get_property(void *obj, struct lvm_property_type *prop);
>>  
>>  #endif
>> diff --git a/liblvm/lvm_vg.c b/liblvm/lvm_vg.c
>> index 9a72bec..98070dd 100644
>> --- a/liblvm/lvm_vg.c
>> +++ b/liblvm/lvm_vg.c
>> @@ -343,7 +343,7 @@ int lvm_vg_get_property(vg_t vg, const char *name,
>>  	struct lvm_property_type prop;
>>  
>>  	strncpy(prop.id, name, LVM_PROPERTY_NAME_LEN);
>> -	if (!vg_get_property(vg, &prop))
>> +	if (!lvm_get_property((void *)vg, &prop))
> 
> No need to add cast to (void*) - it's C not C++...
> 

In fact - do we really need this functionality ?? It's quite dangerous to
allow passing any pointer type to such global API function.

I would vote for only using separate functionality and using right types and
right function names - this C++ polymorphism in C code leads to error which
are hard to catch as compiler will not give any warning about missused typed
structure pointers.

Zdenek

[PATCH 17/19] Rename internal vg_get_property to more generic lvm_get_property.

[Dave Wysochanski]

On Thu, 2010-09-16 at 10:41 +0200, Zdenek Kabelac wrote:
> Dne 15.9.2010 17:36, Dave Wysochanski napsal(a):
> >  lib/report/properties.c |    4 ++--
> >  lib/report/properties.h |    2 +-
> >  liblvm/lvm_vg.c         |    2 +-
> >  3 files changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/lib/report/properties.c b/lib/report/properties.c
> > index 0b80593..95da8ab 100644
> > --- a/lib/report/properties.c
> > +++ b/lib/report/properties.c
> > @@ -262,7 +262,7 @@ struct lvm_property_type _properties[] = {
> >  #undef FIELD
> >  
> >  
> > -int vg_get_property(struct volume_group *vg, struct lvm_property_type *prop)
> > +int lvm_get_property(void *obj, struct lvm_property_type *prop)
> 
> Hope vg_get_property has been marked as unstable API ?
> 

Oh this was not exported - it is in the internal library so it does
not matter.  So far I've only committed refactorings and changes to
internal library, no new lvm2app symbols.

Perhaps the name should be changed from "lvm_*" but wasn't sure
what to make it - perhaps just "get_property".

> 
> >  {
> >  	struct lvm_property_type *p;
> >  
> > @@ -278,7 +278,7 @@ int vg_get_property(struct volume_group *vg, struct lvm_property_type *prop)
> >  	}
> >  
> >  	*prop = *p;
> > -	if (!p->get((void *)vg, prop)) {
> > +	if (!p->get(obj, prop)) {
> >  		return 0;
> >  	}
> >  	return 1;
> > diff --git a/lib/report/properties.h b/lib/report/properties.h
> > index 2e1381d..0a13f39 100644
> > --- a/lib/report/properties.h
> > +++ b/lib/report/properties.h
> > @@ -32,6 +32,6 @@ struct lvm_property_type {
> >  	int (*set) (void *obj, struct lvm_property_type *prop);
> >  };
> >  
> > -int vg_get_property(struct volume_group *vg, struct lvm_property_type *prop);
> > +int lvm_get_property(void *obj, struct lvm_property_type *prop);
> >  
> >  #endif
> > diff --git a/liblvm/lvm_vg.c b/liblvm/lvm_vg.c
> > index 9a72bec..98070dd 100644
> > --- a/liblvm/lvm_vg.c
> > +++ b/liblvm/lvm_vg.c
> > @@ -343,7 +343,7 @@ int lvm_vg_get_property(vg_t vg, const char *name,
> >  	struct lvm_property_type prop;
> >  
> >  	strncpy(prop.id, name, LVM_PROPERTY_NAME_LEN);
> > -	if (!vg_get_property(vg, &prop))
> > +	if (!lvm_get_property((void *)vg, &prop))
> 
> No need to add cast to (void*) - it's C not C++...
> 

Ok.

[PATCH 02/19] Refactor pvstatus_disp to take pv argument and call common pv_attr function.

[Dave Wysochanski]

On Thu, 2010-09-16 at 10:44 +0200, Zdenek Kabelac wrote:
> Dne 15.9.2010 17:35, Dave Wysochanski napsal(a):
> > 
> > Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
> > ---
> >  lib/metadata/metadata.c |   21 +++++++++++++++++++++
> >  lib/metadata/metadata.h |    1 +
> >  lib/report/columns.h    |    2 +-
> >  lib/report/report.c     |   17 +++--------------
> >  4 files changed, 26 insertions(+), 15 deletions(-)
> > 
> > diff --git a/lib/metadata/metadata.c b/lib/metadata/metadata.c
> > index 094ffc1..86f07e7 100644
> > --- a/lib/metadata/metadata.c
> > +++ b/lib/metadata/metadata.c
> > @@ -4598,6 +4598,27 @@ char *vg_attr(struct dm_pool *mem, const struct volume_group *vg)
> >  	return repstr;
> >  }
> >  
> > +char *pv_attr(struct dm_pool *mem, const struct physical_volume *pv)
> > +{
> > +	char *repstr;
> > +
> > +	if (!(repstr = dm_pool_zalloc(mem, 3))) {
> > +		log_error("dm_pool_alloc failed");
> > +		return NULL;
> > +	}
> > +
> > +	if (pv->status & ALLOCATABLE_PV)
> > +		repstr[0] = 'a';
> > +	else
> > +		repstr[0] = '-';
> > +
> > +	if (pv->status & EXPORTED_VG)
> > +		repstr[1] = 'x';
> > +	else
> > +		repstr[1] = '-';
> > +	return repstr;
> 
> when you are moving this code maybe again little shortening could be made:
> c = (x) ? a : b;
> 
> Zdenek

This was done in a later patch - did you see patch 13?
I did in a later patch to make it easier to spot potential errors.

[PATCH 17/19] Rename internal vg_get_property to more generic lvm_get_property.

[Dave Wysochanski]

On Thu, 2010-09-16 at 10:57 +0200, Zdenek Kabelac wrote:
> Dne 16.9.2010 10:41, Zdenek Kabelac napsal(a):
> > Dne 15.9.2010 17:36, Dave Wysochanski napsal(a):
> >>  lib/report/properties.c |    4 ++--
> >>  lib/report/properties.h |    2 +-
> >>  liblvm/lvm_vg.c         |    2 +-
> >>  3 files changed, 4 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/lib/report/properties.c b/lib/report/properties.c
> >> index 0b80593..95da8ab 100644
> >> --- a/lib/report/properties.c
> >> +++ b/lib/report/properties.c
> >> @@ -262,7 +262,7 @@ struct lvm_property_type _properties[] = {
> >>  #undef FIELD
> >>  
> >>  
> >> -int vg_get_property(struct volume_group *vg, struct lvm_property_type *prop)
> >> +int lvm_get_property(void *obj, struct lvm_property_type *prop)
> > 
> > Hope vg_get_property has been marked as unstable API ?
> > 
> > 
> >>  {
> >>  	struct lvm_property_type *p;
> >>  
> >> @@ -278,7 +278,7 @@ int vg_get_property(struct volume_group *vg, struct lvm_property_type *prop)
> >>  	}
> >>  
> >>  	*prop = *p;
> >> -	if (!p->get((void *)vg, prop)) {
> >> +	if (!p->get(obj, prop)) {
> >>  		return 0;
> >>  	}
> >>  	return 1;
> >> diff --git a/lib/report/properties.h b/lib/report/properties.h
> >> index 2e1381d..0a13f39 100644
> >> --- a/lib/report/properties.h
> >> +++ b/lib/report/properties.h
> >> @@ -32,6 +32,6 @@ struct lvm_property_type {
> >>  	int (*set) (void *obj, struct lvm_property_type *prop);
> >>  };
> >>  
> >> -int vg_get_property(struct volume_group *vg, struct lvm_property_type *prop);
> >> +int lvm_get_property(void *obj, struct lvm_property_type *prop);
> >>  
> >>  #endif
> >> diff --git a/liblvm/lvm_vg.c b/liblvm/lvm_vg.c
> >> index 9a72bec..98070dd 100644
> >> --- a/liblvm/lvm_vg.c
> >> +++ b/liblvm/lvm_vg.c
> >> @@ -343,7 +343,7 @@ int lvm_vg_get_property(vg_t vg, const char *name,
> >>  	struct lvm_property_type prop;
> >>  
> >>  	strncpy(prop.id, name, LVM_PROPERTY_NAME_LEN);
> >> -	if (!vg_get_property(vg, &prop))
> >> +	if (!lvm_get_property((void *)vg, &prop))
> > 
> > No need to add cast to (void*) - it's C not C++...
> > 
> 
> In fact - do we really need this functionality ?? It's quite dangerous to
> allow passing any pointer type to such global API function.
> 

Let's see what our options are to clean this up.

It's either void * or duplicate the lvm_get_property() function for each
object (vg, lv, pv) if you don't like void.  If void * is ok, I could
add a magic number to the head of each pointer and do a check inside the
lvm_get_property().  (BTW, In my patchset, I've renamed
lvm_get_property() to just get_property() for now.)


> I would vote for only using separate functionality and using right types and
> right function names - this C++ polymorphism in C code leads to error which
> are hard to catch as compiler will not give any warning about missused typed
> structure pointers.
> 

Ok, so you're saying something like this in properties.h:

int vg_get_property(struct volume_group *vg, struct lvm_property_type *prop);
int pv_get_property(struct physical_volume *vg, struct lvm_property_type *prop);
int lv_get_property(struct logical_volume *lv, struct lvm_property_type *prop);

and maybe make a generic _get_property() inside properties.c, 
called from each of the above, like this:

static int _get_property(void *obj, struct lvm_property_type *prop)
{
	struct lvm_property_type *p;

	p = _properties;
	while (p->id[0]) {
		if (!strcmp(p->id, prop->id))
			break;
		p++;
	}
	if (!p->id[0]) {
		log_errno(EINVAL, "Invalid property name %s", prop->id);
		return 0;
	}

	*prop = *p;
	if (!p->get(obj, prop)) {
		return 0;
	}
	return 1;
}


Keep in mind I'm trying to extend the columns.h / report.c existing structure,
so I've got this _properties array similar to the _fields array in report.c.
In this array, I look up based on field id, then need to call some 'get' or
'set' function tied to the field_id / property name.  The 'get' or 'set'
functions defined inside struct lvm_property_type needed to have one prototype,
so it was either void * or I guess I could do a union.  Would you prefer a
union there?  Maybe I should be using lvm_report_object * instead of
void * for 'obj'?

Let me see if I can rework to be more type-safe.

[PATCH 17/19] Rename internal vg_get_property to more generic lvm_get_property.

[Dave Wysochanski]

On Thu, 2010-09-16 at 10:57 +0200, Zdenek Kabelac wrote:
> Dne 16.9.2010 10:41, Zdenek Kabelac napsal(a):
> > Dne 15.9.2010 17:36, Dave Wysochanski napsal(a):
> >>  lib/report/properties.c |    4 ++--
> >>  lib/report/properties.h |    2 +-
> >>  liblvm/lvm_vg.c         |    2 +-
> >>  3 files changed, 4 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/lib/report/properties.c b/lib/report/properties.c
> >> index 0b80593..95da8ab 100644
> >> --- a/lib/report/properties.c
> >> +++ b/lib/report/properties.c
> >> @@ -262,7 +262,7 @@ struct lvm_property_type _properties[] = {
> >>  #undef FIELD
> >>  
> >>  
> >> -int vg_get_property(struct volume_group *vg, struct lvm_property_type *prop)
> >> +int lvm_get_property(void *obj, struct lvm_property_type *prop)
> > 
> > Hope vg_get_property has been marked as unstable API ?
> > 
> > 
> >>  {
> >>  	struct lvm_property_type *p;
> >>  
> >> @@ -278,7 +278,7 @@ int vg_get_property(struct volume_group *vg, struct lvm_property_type *prop)
> >>  	}
> >>  
> >>  	*prop = *p;
> >> -	if (!p->get((void *)vg, prop)) {
> >> +	if (!p->get(obj, prop)) {
> >>  		return 0;
> >>  	}
> >>  	return 1;
> >> diff --git a/lib/report/properties.h b/lib/report/properties.h
> >> index 2e1381d..0a13f39 100644
> >> --- a/lib/report/properties.h
> >> +++ b/lib/report/properties.h
> >> @@ -32,6 +32,6 @@ struct lvm_property_type {
> >>  	int (*set) (void *obj, struct lvm_property_type *prop);
> >>  };
> >>  
> >> -int vg_get_property(struct volume_group *vg, struct lvm_property_type *prop);
> >> +int lvm_get_property(void *obj, struct lvm_property_type *prop);
> >>  
> >>  #endif
> >> diff --git a/liblvm/lvm_vg.c b/liblvm/lvm_vg.c
> >> index 9a72bec..98070dd 100644
> >> --- a/liblvm/lvm_vg.c
> >> +++ b/liblvm/lvm_vg.c
> >> @@ -343,7 +343,7 @@ int lvm_vg_get_property(vg_t vg, const char *name,
> >>  	struct lvm_property_type prop;
> >>  
> >>  	strncpy(prop.id, name, LVM_PROPERTY_NAME_LEN);
> >> -	if (!vg_get_property(vg, &prop))
> >> +	if (!lvm_get_property((void *)vg, &prop))
> > 
> > No need to add cast to (void*) - it's C not C++...
> > 
> 
> In fact - do we really need this functionality ?? It's quite dangerous to
> allow passing any pointer type to such global API function.
> 
> I would vote for only using separate functionality and using right types and
> right function names - this C++ polymorphism in C code leads to error which
> are hard to catch as compiler will not give any warning about missused typed
> structure pointers.
> 

How about the attached patch in place of this one.  I've left
vg_get_property() alone and added pv_get_property().  Also, I add the
report 'type' to lvm_property_type and then check inside the static
_get_property() function to ensure the correct function is called for
the given 'id' string.


>From 63e74cff2f9db5606b48d64e6ecf53935dd66839 Mon Sep 17 00:00:00 2001
From: Dave Wysochanski <dwysocha@redhat.com>
Date: Fri, 17 Sep 2010 13:16:53 -0400
Subject: [PATCH] Add pv_get_property and create generic internal _get_property function.

We need to use a similar function for pv and lv properties, so just make
a generic _get_property() function that contains most of the required
functionality.  Also, add a check to ensure the field name matches the
object passed in by re-using report_type_t enum.  For pv properties,
the report_type might be either PVS or LABEL.
---
 lib/report/properties.c |   23 +++++++++++++++++++----
 lib/report/properties.h |    3 +++
 2 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/lib/report/properties.c b/lib/report/properties.c
index 0b80593..52657c0 100644
--- a/lib/report/properties.c
+++ b/lib/report/properties.c
@@ -250,11 +250,11 @@ GET_VG_NUM_PROPERTY_FN(vg_mda_copies, (vg_mda_copies(vg)))
 #define STR DM_REPORT_FIELD_TYPE_STRING
 #define NUM DM_REPORT_FIELD_TYPE_NUMBER
 #define FIELD(type, strct, sorttype, head, field, width, fn, id, desc, writeable) \
-	{ #id, writeable, sorttype == STR, { .n_val = 0 }, _ ## id ## _get, _ ## id ## _set },
+	{ type, #id, writeable, sorttype == STR, { .n_val = 0 }, _ ## id ## _get, _ ## id ## _set },
 
 struct lvm_property_type _properties[] = {
 #include "columns.h"
-	{ "", 0, 0, { .n_val = 0 }, _not_implemented, _not_implemented },
+	{ 0, "", 0, 0, { .n_val = 0 }, _not_implemented, _not_implemented },
 };
 
 #undef STR
@@ -262,7 +262,7 @@ struct lvm_property_type _properties[] = {
 #undef FIELD
 
 
-int vg_get_property(struct volume_group *vg, struct lvm_property_type *prop)
+static int _get_property(void *obj, struct lvm_property_type *prop, report_type_t type)
 {
 	struct lvm_property_type *p;
 
@@ -276,10 +276,25 @@ int vg_get_property(struct volume_group *vg, struct lvm_property_type *prop)
 		log_errno(EINVAL, "Invalid property name %s", prop->id);
 		return 0;
 	}
+	if (!(p->type & type)) {
+		log_errno(EINVAL, "Property name %s does not match type %d",
+			  prop->id, p->type);
+		return 0;
+	}
 
 	*prop = *p;
-	if (!p->get((void *)vg, prop)) {
+	if (!p->get(obj, prop)) {
 		return 0;
 	}
 	return 1;
 }
+
+int vg_get_property(struct volume_group *vg, struct lvm_property_type *prop)
+{
+	return _get_property(vg, prop, VGS);
+}
+
+int pv_get_property(struct physical_volume *pv, struct lvm_property_type *prop)
+{
+	return _get_property(pv, prop, PVS | LABEL);
+}
diff --git a/lib/report/properties.h b/lib/report/properties.h
index 2e1381d..1a1a439 100644
--- a/lib/report/properties.h
+++ b/lib/report/properties.h
@@ -17,10 +17,12 @@
 #include "libdevmapper.h"
 #include "lvm-types.h"
 #include "metadata.h"
+#include "report.h"
 
 #define LVM_PROPERTY_NAME_LEN DM_REPORT_FIELD_TYPE_ID_LEN
 
 struct lvm_property_type {
+	report_type_t type;
 	char id[LVM_PROPERTY_NAME_LEN];
 	unsigned is_writeable;
 	unsigned is_string;
@@ -33,5 +35,6 @@ struct lvm_property_type {
 };
 
 int vg_get_property(struct volume_group *vg, struct lvm_property_type *prop);
+int pv_get_property(struct physical_volume *pv, struct lvm_property_type *prop);
 
 #endif
-- 
1.7.2.2

[PATCH 17/19] Rename internal vg_get_property to more generic lvm_get_property.

[Zdenek Kabelac]

Dne 20.9.2010 18:49, Dave Wysochanski napsal(a):
> On Thu, 2010-09-16 at 10:57 +0200, Zdenek Kabelac wrote:
>> Dne 16.9.2010 10:41, Zdenek Kabelac napsal(a):
>>> Dne 15.9.2010 17:36, Dave Wysochanski napsal(a):
>>>>  lib/report/properties.c |    4 ++--
>>>>  lib/report/properties.h |    2 +-
>>>>  liblvm/lvm_vg.c         |    2 +-
>>>>  3 files changed, 4 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/lib/report/properties.c b/lib/report/properties.c
>>>> index 0b80593..95da8ab 100644
>>>> --- a/lib/report/properties.c
>>>> +++ b/lib/report/properties.c
> +
> +int vg_get_property(struct volume_group *vg, struct lvm_property_type *prop)

This function should be able to operate with const* vg - or do we need to
modify vg with some get functionality ?

> +int pv_get_property(struct physical_volume *pv, struct lvm_property_type *prop)

const* pv


Zdenek