[PATCH 00/12] Add lvm vg properties for lvm2app.

[PATCH 00/12] Add lvm vg properties for lvm2app.

[Dave Wysochanski]

This patchset is a continuation of the prior patchset to implement
generic functions to query a value of a field in an lvm report.
This patchset completes the vg-based fields of the report and adds
a generic function, lvm_vg_get_property() to lvm2app to query the
properties by name.

For the string-based properties, memory allocation is done inside
the individual supporting functions (i.e. vg_name(), vg_uuid(), etc).
I decided this to be the best approach for consistency sake, and
since some of the properties (for example, vg_attr, and vg_uuid)
required special formatting and thus required the allocation to be
done at the lower level.

I'm not entirely happy with this patchset but it is close enough.
In particular, some of the functions might be better placed inside
a separate file in lib/report and then shared between report.c
and properties.c.  For now I've placed most of the functions inside
metadata.c.

Once this patchset is accepted and committed, I shall continue with
the pv and lv properties using the same approach.

[PATCH 01/12] 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/12] 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/12] 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..6608473 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, 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..6c1169a 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, struct id *id);
+
 #endif
-- 
1.7.2.1

[PATCH 04/12] 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/12] 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.

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..6fb69dc 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(struct physical_volume *pv)
+{
+	return id_format_and_copy(pv->vg->vgmem, &pv->id);
+}
+
+char *vg_uuid(struct volume_group *vg)
+{
+	return id_format_and_copy(vg->vgmem, &vg->id);
+}
+
+char *lv_uuid(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..4eb826a 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(struct logical_volume *lv);
+char *vg_uuid(struct volume_group *vg);
+char *pv_uuid(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/12] 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 6fb69dc..ca2374d 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 4eb826a..82adf07 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(struct logical_volume *lv);
 char *vg_uuid(struct volume_group *vg);
 char *pv_uuid(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/12] 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 ca2374d..f418c18 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 82adf07..9c42276 100644
--- a/lib/metadata/metadata.h
+++ b/lib/metadata/metadata.h
@@ -426,5 +426,8 @@ char *lv_uuid(struct logical_volume *lv);
 char *vg_uuid(struct volume_group *vg);
 char *pv_uuid(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/12] 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 b99f630..20119b3 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/12] 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 20119b3..ce7b85f 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/12] 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 ce7b85f..c4bce08 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/12] 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/12] 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 01/12] Add vg_attr() and lv_attr() functions.

[Zdenek Kabelac]

Dne 9.9.2010 22:12, Dave Wysochanski napsal(a):
> 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

Yep - this file is already quite large (~4600 lines)

> @@ -4544,6 +4544,177 @@ uint32_t vg_mda_used_count
> +
> +	if (vg->status & LVM_WRITE)
> +		repstr[0] = 'w';
> +	else
> +		repstr[0] = 'r';

Shorter:

repstr[0] = (vg->status & LVM_WRITE) ? 'w' : 'r';

(similar to other places)


Zdenek

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

[Zdenek Kabelac]

Dne 9.9.2010 22:13, Dave Wysochanski napsal(a):
> 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..6608473 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, 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..6c1169a 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, struct id *id);

const struct id*

Please consider 'const' in case you do not plane to modify id inside this
function.

Zdenek

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

[Zdenek Kabelac]

Dne 9.9.2010 22:13, Dave Wysochanski napsal(a):
> Add supporting functions for pv_uuid, vg_uuid, and lv_uuid.
> Call new function id_format_and_copy.
> 
> 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..6fb69dc 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(struct physical_volume *pv)
> +{
> +	return id_format_and_copy(pv->vg->vgmem, &pv->id);
> +}
> +
> +char *vg_uuid(struct volume_group *vg)
> +{
> +	return id_format_and_copy(vg->vgmem, &vg->id);
> +}
> +
> +char *lv_uuid(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..4eb826a 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(struct logical_volume *lv);
> +char *vg_uuid(struct volume_group *vg);
> +char *pv_uuid(struct physical_volume *pv);

Again - 'const'for *lv, *vg, *pv  would fit here probably better.

>  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))) {


Probably not a big issue, but I can see some 'inconsistency' -

It looks like we have put 'const' on this return value to mark value as a
string which user should not try to 'free' - but this is somewhat missuse of
'const'  - there is probably no reason to avoid modification of copied string.
As we do not keep it internally - it just allocated string and user should be
able to do anything he wants to do with it.

Maybe we should reconsider and relax this return values to plain char*.

Zdenek

[PATCH] 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 f418c18..bbd450b 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 03/12] Add id_format_and_copy() uuid function to allocate and format a uuid.

[Dave Wysochanski]

On Fri, 2010-09-10 at 11:30 +0200, Zdenek Kabelac wrote:
> Dne 9.9.2010 22:13, Dave Wysochanski napsal(a):
> > 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..6608473 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, 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..6c1169a 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, struct id *id);
> 
> const struct id*
> 
> Please consider 'const' in case you do not plane to modify id inside this
> function.
> 
> Zdenek
> 

Updated in my patchset.

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

[Dave Wysochanski]

On Fri, 2010-09-10 at 11:38 +0200, Zdenek Kabelac wrote:
> Dne 9.9.2010 22:13, Dave Wysochanski napsal(a):
> > Add supporting functions for pv_uuid, vg_uuid, and lv_uuid.
> > Call new function id_format_and_copy.
> > 
> > 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..6fb69dc 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(struct physical_volume *pv)
> > +{
> > +	return id_format_and_copy(pv->vg->vgmem, &pv->id);
> > +}
> > +
> > +char *vg_uuid(struct volume_group *vg)
> > +{
> > +	return id_format_and_copy(vg->vgmem, &vg->id);
> > +}
> > +
> > +char *lv_uuid(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..4eb826a 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(struct logical_volume *lv);
> > +char *vg_uuid(struct volume_group *vg);
> > +char *pv_uuid(struct physical_volume *pv);
> 
> Again - 'const'for *lv, *vg, *pv  would fit here probably better.
> 

Updated in my patches - thanks.

> >  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))) {
> 
> 
> Probably not a big issue, but I can see some 'inconsistency' -
> 
> It looks like we have put 'const' on this return value to mark value as a
> string which user should not try to 'free' - but this is somewhat missuse of
> 'const'  - there is probably no reason to avoid modification of copied string.
> As we do not keep it internally - it just allocated string and user should be
> able to do anything he wants to do with it.
> 
> Maybe we should reconsider and relax this return values to plain char*.
> 

Well, I don't think we want users modifying the uuid's returned by these
functions.  I think const sends the right message.

Thanks for taking the time to review the patches.