[PATCH 00/14] Add lvm lv properties and lvm2app interfaces for pv/vg/lv.

[PATCH 00/14] Add lvm lv properties and lvm2app interfaces for pv/vg/lv.

[Dave Wysochanski]

This patchset is adds lv properties for lvm2app, and adds
the lvm2app interfaces and interactive tests to exercise
the interface.

A few attributes require more complex refactoring and are thus still
missing:
1. readahead: lv_read_ahead, lv_kernel_read_ahead
2. percents: snap_percent and copy_percent

All vg and pv properties and most lv properties may now be queried
with this patchset.

[PATCH 01/14] Add some lv 'get' functions that require no refactoring.

[Dave Wysochanski]

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

diff --git a/lib/report/properties.c b/lib/report/properties.c
index bea57b5..eec4c61 100644
--- a/lib/report/properties.c
+++ b/lib/report/properties.c
@@ -97,17 +97,17 @@ GET_PV_NUM_PROPERTY_FN(pv_mda_used_count, pv_mda_used_count(pv))
 #define _pv_mda_used_count_set _not_implemented_set
 
 /* LV */
-#define _lv_uuid_get _not_implemented_get
+GET_LV_STR_PROPERTY_FN(lv_uuid, lv_uuid_dup(lv))
 #define _lv_uuid_set _not_implemented_set
 #define _lv_name_get _not_implemented_get
 #define _lv_name_set _not_implemented_set
 #define _lv_path_get _not_implemented_get
 #define _lv_path_set _not_implemented_set
-#define _lv_attr_get _not_implemented_get
+GET_LV_STR_PROPERTY_FN(lv_attr, lv_attr_dup(lv->vg->vgmem, lv))
 #define _lv_attr_set _not_implemented_set
-#define _lv_major_get _not_implemented_get
+GET_LV_NUM_PROPERTY_FN(lv_major, lv->major)
 #define _lv_major_set _not_implemented_set
-#define _lv_minor_get _not_implemented_get
+GET_LV_NUM_PROPERTY_FN(lv_minor, lv->minor)
 #define _lv_minor_set _not_implemented_set
 #define _lv_read_ahead_get _not_implemented_get
 #define _lv_read_ahead_set _not_implemented_set
@@ -117,9 +117,9 @@ GET_PV_NUM_PROPERTY_FN(pv_mda_used_count, pv_mda_used_count(pv))
 #define _lv_kernel_minor_set _not_implemented_set
 #define _lv_kernel_read_ahead_get _not_implemented_get
 #define _lv_kernel_read_ahead_set _not_implemented_set
-#define _lv_size_get _not_implemented_get
+GET_LV_NUM_PROPERTY_FN(lv_size, lv->size * SECTOR_SIZE)
 #define _lv_size_set _not_implemented_set
-#define _seg_count_get _not_implemented_get
+GET_LV_NUM_PROPERTY_FN(seg_count, dm_list_size(&lv->segments))
 #define _seg_count_set _not_implemented_set
 #define _origin_get _not_implemented_get
 #define _origin_set _not_implemented_set
@@ -133,7 +133,7 @@ GET_PV_NUM_PROPERTY_FN(pv_mda_used_count, pv_mda_used_count(pv))
 #define _move_pv_set _not_implemented_set
 #define _convert_lv_get _not_implemented_get
 #define _convert_lv_set _not_implemented_set
-#define _lv_tags_get _not_implemented_get
+GET_LV_STR_PROPERTY_FN(lv_tags, lv_tags_dup(lv))
 #define _lv_tags_set _not_implemented_set
 #define _mirror_log_get _not_implemented_get
 #define _mirror_log_set _not_implemented_set
-- 
1.7.2.2

[PATCH 01/14] Add some lv 'get' functions that require no refactoring.

[Petr Rockai]

Dave Wysochanski <dwysocha@redhat.com> writes:

> Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
Adds lv_uuid, lv_attr, lv_major, lv_minor, lv_size, (assumed lv_)
seg_count, pv_mda_used_count, lv_tags.

Straightforward enough.

Reviewed-By: Petr Rockai <prockai@redhat.com>

> -#define _lv_uuid_get _not_implemented_get
> +GET_LV_STR_PROPERTY_FN(lv_uuid, lv_uuid_dup(lv))
>  #define _lv_uuid_set _not_implemented_set
>  #define _lv_name_get _not_implemented_get
>  #define _lv_name_set _not_implemented_set
>  #define _lv_path_get _not_implemented_get
>  #define _lv_path_set _not_implemented_set
> -#define _lv_attr_get _not_implemented_get
> +GET_LV_STR_PROPERTY_FN(lv_attr, lv_attr_dup(lv->vg->vgmem, lv))
>  #define _lv_attr_set _not_implemented_set
> -#define _lv_major_get _not_implemented_get
> +GET_LV_NUM_PROPERTY_FN(lv_major, lv->major)
>  #define _lv_major_set _not_implemented_set
> -#define _lv_minor_get _not_implemented_get
> +GET_LV_NUM_PROPERTY_FN(lv_minor, lv->minor)
>  #define _lv_minor_set _not_implemented_set
>  #define _lv_read_ahead_get _not_implemented_get
>  #define _lv_read_ahead_set _not_implemented_set
> @@ -117,9 +117,9 @@ GET_PV_NUM_PROPERTY_FN(pv_mda_used_count, pv_mda_used_count(pv))
>  #define _lv_kernel_minor_set _not_implemented_set
>  #define _lv_kernel_read_ahead_get _not_implemented_get
>  #define _lv_kernel_read_ahead_set _not_implemented_set
> -#define _lv_size_get _not_implemented_get
> +GET_LV_NUM_PROPERTY_FN(lv_size, lv->size * SECTOR_SIZE)
>  #define _lv_size_set _not_implemented_set
> -#define _seg_count_get _not_implemented_get
> +GET_LV_NUM_PROPERTY_FN(seg_count, dm_list_size(&lv->segments))
>  #define _seg_count_set _not_implemented_set
>  #define _origin_get _not_implemented_get
>  #define _origin_set _not_implemented_set
> @@ -133,7 +133,7 @@ GET_PV_NUM_PROPERTY_FN(pv_mda_used_count, pv_mda_used_count(pv))
>  #define _move_pv_set _not_implemented_set
>  #define _convert_lv_get _not_implemented_get
>  #define _convert_lv_set _not_implemented_set
> -#define _lv_tags_get _not_implemented_get
> +GET_LV_STR_PROPERTY_FN(lv_tags, lv_tags_dup(lv))
>  #define _lv_tags_set _not_implemented_set
>  #define _mirror_log_get _not_implemented_get
>  #define _mirror_log_set _not_implemented_set

[PATCH 02/14] Refactor and add code for (lv) 'lv_path' get function.

[Dave Wysochanski]

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

diff --git a/lib/metadata/lv.c b/lib/metadata/lv.c
index add0a4d..f750f54 100644
--- a/lib/metadata/lv.c
+++ b/lib/metadata/lv.c
@@ -16,6 +16,27 @@
 #include "lib.h"
 #include "metadata.h"
 #include "activate.h"
+#include "toolcontext.h"
+
+char *lv_path_dup(struct dm_pool *mem, const struct logical_volume *lv)
+{
+	char *repstr;
+	size_t len;
+
+	len = strlen(lv->vg->cmd->dev_dir);
+	len += strlen(lv->vg->name) + strlen(lv->name) + 2;
+	if (!(repstr = dm_pool_zalloc(mem, len))) {
+		log_error("dm_pool_alloc failed");
+		return 0;
+	}
+
+	if (dm_snprintf(repstr, len, "%s%s/%s",
+			lv->vg->cmd->dev_dir, lv->vg->name, lv->name) < 0) {
+		log_error("lvpath snprintf failed");
+		return 0;
+	}
+	return repstr;
+}
 
 char *lv_uuid_dup(const struct logical_volume *lv)
 {
diff --git a/lib/metadata/lv.h b/lib/metadata/lv.h
index 54ed7e0..eee2811 100644
--- a/lib/metadata/lv.h
+++ b/lib/metadata/lv.h
@@ -52,5 +52,6 @@ uint64_t lv_size(const struct logical_volume *lv);
 char *lv_attr_dup(struct dm_pool *mem, const struct logical_volume *lv);
 char *lv_uuid_dup(const struct logical_volume *lv);
 char *lv_tags_dup(const struct logical_volume *lv);
+char *lv_path_dup(struct dm_pool *mem, const struct logical_volume *lv);
 
 #endif
diff --git a/lib/report/properties.c b/lib/report/properties.c
index eec4c61..8890e27 100644
--- a/lib/report/properties.c
+++ b/lib/report/properties.c
@@ -101,7 +101,7 @@ GET_LV_STR_PROPERTY_FN(lv_uuid, lv_uuid_dup(lv))
 #define _lv_uuid_set _not_implemented_set
 #define _lv_name_get _not_implemented_get
 #define _lv_name_set _not_implemented_set
-#define _lv_path_get _not_implemented_get
+GET_LV_STR_PROPERTY_FN(lv_path, lv_path_dup(lv->vg->vgmem, lv))
 #define _lv_path_set _not_implemented_set
 GET_LV_STR_PROPERTY_FN(lv_attr, lv_attr_dup(lv->vg->vgmem, lv))
 #define _lv_attr_set _not_implemented_set
diff --git a/lib/report/report.c b/lib/report/report.c
index 95f4550..32559b8 100644
--- a/lib/report/report.c
+++ b/lib/report/report.c
@@ -350,18 +350,9 @@ static int _lvpath_disp(struct dm_report *rh, struct dm_pool *mem,
 {
 	const struct logical_volume *lv = (const struct logical_volume *) data;
 	char *repstr;
-	size_t len;
 
-	len = strlen(lv->vg->cmd->dev_dir) + strlen(lv->vg->name) + strlen(lv->name) + 2;
-	if (!(repstr = dm_pool_zalloc(mem, len))) {
-		log_error("dm_pool_alloc failed");
+	if (!(repstr = lv_path_dup(mem, lv)))
 		return 0;
-	}
-
-	if (dm_snprintf(repstr, len, "%s%s/%s", lv->vg->cmd->dev_dir, lv->vg->name, lv->name) < 0) {
-		log_error("lvpath snprintf failed");
-		return 0;
-	}
 
 	dm_report_field_set_value(field, repstr, NULL);
 
-- 
1.7.2.2

[PATCH 02/14] Refactor and add code for (lv) 'lv_path' get function.

[Petr Rockai]

Dave Wysochanski <dwysocha@redhat.com> writes:

> Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
Reviewed-By: Petr Rockai <prockai@redhat.com>

(But! See inline comments below. It would make sense to have another go
at this one.)

> ---
>  lib/metadata/lv.c       |   21 +++++++++++++++++++++
>  lib/metadata/lv.h       |    1 +
>  lib/report/properties.c |    2 +-
>  lib/report/report.c     |   11 +----------
>  4 files changed, 24 insertions(+), 11 deletions(-)
>
> diff --git a/lib/metadata/lv.c b/lib/metadata/lv.c
> index add0a4d..f750f54 100644
> --- a/lib/metadata/lv.c
> +++ b/lib/metadata/lv.c
> @@ -16,6 +16,27 @@
>  #include "lib.h"
>  #include "metadata.h"
>  #include "activate.h"
> +#include "toolcontext.h"
> +
> +char *lv_path_dup(struct dm_pool *mem, const struct logical_volume *lv)
> +{
> +	char *repstr;
> +	size_t len;
> +
> +	len = strlen(lv->vg->cmd->dev_dir);
> +	len += strlen(lv->vg->name) + strlen(lv->name) + 2;
Can you just make that a single size_t len = foo + bar + ...; line? That
would make it clear there are no side effects involved...

> +	if (!(repstr = dm_pool_zalloc(mem, len))) {
> +		log_error("dm_pool_alloc failed");
> +		return 0;
> +	}
> +
> +	if (dm_snprintf(repstr, len, "%s%s/%s",
> +			lv->vg->cmd->dev_dir, lv->vg->name, lv->name) < 0) {
> +		log_error("lvpath snprintf failed");
> +		return 0;
> +	}
^^ Should the above dm_pool_zalloc/dm_snprintf go into some
dm_pool_asprintf, in fact? That would also remove that "len" computation
and make this function completely trivial. We already have dm_asprintf
(which uses dm_malloc). You could probably parametrise dm_asprintf with
the strdup function into dm_generic_asprintf and have both dm_asprintf
and dm_pool_asprintf implemented in terms of that.

> +	return repstr;
> +}

Also, the log_error bits aren't that spectacular... Could you make them
a bit clearer, or even omit them? The caller has to check the result and
error out anyway...

>  char *lv_uuid_dup(const struct logical_volume *lv)
>  {
> diff --git a/lib/metadata/lv.h b/lib/metadata/lv.h
> index 54ed7e0..eee2811 100644
> --- a/lib/metadata/lv.h
> +++ b/lib/metadata/lv.h
> @@ -52,5 +52,6 @@ uint64_t lv_size(const struct logical_volume *lv);
>  char *lv_attr_dup(struct dm_pool *mem, const struct logical_volume *lv);
>  char *lv_uuid_dup(const struct logical_volume *lv);
>  char *lv_tags_dup(const struct logical_volume *lv);
> +char *lv_path_dup(struct dm_pool *mem, const struct logical_volume *lv);
>  
>  #endif
> diff --git a/lib/report/properties.c b/lib/report/properties.c
> index eec4c61..8890e27 100644
> --- a/lib/report/properties.c
> +++ b/lib/report/properties.c
> @@ -101,7 +101,7 @@ GET_LV_STR_PROPERTY_FN(lv_uuid, lv_uuid_dup(lv))
>  #define _lv_uuid_set _not_implemented_set
>  #define _lv_name_get _not_implemented_get
>  #define _lv_name_set _not_implemented_set
> -#define _lv_path_get _not_implemented_get
> +GET_LV_STR_PROPERTY_FN(lv_path, lv_path_dup(lv->vg->vgmem, lv))
>  #define _lv_path_set _not_implemented_set
>  GET_LV_STR_PROPERTY_FN(lv_attr, lv_attr_dup(lv->vg->vgmem, lv))
>  #define _lv_attr_set _not_implemented_set
> diff --git a/lib/report/report.c b/lib/report/report.c
> index 95f4550..32559b8 100644
> --- a/lib/report/report.c
> +++ b/lib/report/report.c
> @@ -350,18 +350,9 @@ static int _lvpath_disp(struct dm_report *rh, struct dm_pool *mem,
>  {
>  	const struct logical_volume *lv = (const struct logical_volume *) data;
>  	char *repstr;
> -	size_t len;
>  
> -	len = strlen(lv->vg->cmd->dev_dir) + strlen(lv->vg->name) + strlen(lv->name) + 2;
> -	if (!(repstr = dm_pool_zalloc(mem, len))) {
> -		log_error("dm_pool_alloc failed");
> +	if (!(repstr = lv_path_dup(mem, lv)))
>  		return 0;
> -	}
> -
> -	if (dm_snprintf(repstr, len, "%s%s/%s", lv->vg->cmd->dev_dir, lv->vg->name, lv->name) < 0) {
> -		log_error("lvpath snprintf failed");
> -		return 0;
> -	}
>  
>  	dm_report_field_set_value(field, repstr, NULL);
Ok.

Yours,
   Petr.

[PATCH 02/14] Refactor and add code for (lv) 'lv_path' get function.

[Dave Wysochanski]

On Mon, 2010-10-11 at 20:09 +0200, Petr Rockai wrote:
> Dave Wysochanski <dwysocha@redhat.com> writes:
> 
> > Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
> Reviewed-By: Petr Rockai <prockai@redhat.com>
> 
> (But! See inline comments below. It would make sense to have another go
> at this one.)
> 

Ok.


> > +
> > +char *lv_path_dup(struct dm_pool *mem, const struct logical_volume *lv)
> > +{
> > +	char *repstr;
> > +	size_t len;
> > +
> > +	len = strlen(lv->vg->cmd->dev_dir);
> > +	len += strlen(lv->vg->name) + strlen(lv->name) + 2;
> Can you just make that a single size_t len = foo + bar + ...; line? That
> would make it clear there are no side effects involved...
> 

done.


> > +	if (!(repstr = dm_pool_zalloc(mem, len))) {
> > +		log_error("dm_pool_alloc failed");
> > +		return 0;
> > +	}
> > +
> > +	if (dm_snprintf(repstr, len, "%s%s/%s",
> > +			lv->vg->cmd->dev_dir, lv->vg->name, lv->name) < 0) {
> > +		log_error("lvpath snprintf failed");
> > +		return 0;
> > +	}
> ^^ Should the above dm_pool_zalloc/dm_snprintf go into some
> dm_pool_asprintf, in fact? That would also remove that "len" computation
> and make this function completely trivial. We already have dm_asprintf
> (which uses dm_malloc). You could probably parametrise dm_asprintf with
> the strdup function into dm_generic_asprintf and have both dm_asprintf
> and dm_pool_asprintf implemented in terms of that.
> 
> > +	return repstr;
> > +}
> 
> Also, the log_error bits aren't that spectacular... Could you make them
> a bit clearer, or even omit them? The caller has to check the result and
> error out anyway...
> 

If it's ok with you, I may defer these to a couple cleanup patches.
They are good suggestions but I'm mostly focused on straight moving and
refactoring for this patch set.

There's probably other places in the code that could benefit from such a
dm_pool_asprintf.  So when that is proposed as a patch, all code should
get cleaned up.

The error paths in the attribute and lvm2app code need a cleanup pass -
for example some places I'm using log_errno(ENOMEM, ....), others it
seems we're just doing return_NULL, and still others is this method.

I've placed these two items into the bz for now:
https://bugzilla.redhat.com/show_bug.cgi?id=614049

[PATCH 02/14] Refactor and add code for (lv) 'lv_path' get function.

[Petr Rockai]

Dave Wysochanski <dwysocha@redhat.com> writes:

>> Should the above dm_pool_zalloc/dm_snprintf go into some
>> dm_pool_asprintf, in fact? That would also remove that "len"
>> computation and make this function completely trivial. We already
>> have dm_asprintf (which uses dm_malloc). You could probably
>> parametrise dm_asprintf with the strdup function into
>> dm_generic_asprintf and have both dm_asprintf and dm_pool_asprintf
>> implemented in terms of that.

> If it's ok with you, I may defer these to a couple cleanup patches.
> They are good suggestions but I'm mostly focused on straight moving
> and refactoring for this patch set.

Sure, no problem. That can be done later. The patch is, logic-wise, in
order, as far as I can tell. The suggestions are quite optional, and of
course there's always a tradeoff. Ok to check in, I'll look at factoring
in dm_pool_asprintf later.

Yours,
   Petr.

[PATCH 03/14] Refactor and add code for (lv) 'origin_size' get function.

[Dave Wysochanski]

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 lib/metadata/lv.c       |   13 +++++++++++++
 lib/metadata/lv.h       |    1 +
 lib/report/properties.c |    2 +-
 lib/report/report.c     |    7 +------
 4 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/lib/metadata/lv.c b/lib/metadata/lv.c
index f750f54..ae8a4fb 100644
--- a/lib/metadata/lv.c
+++ b/lib/metadata/lv.c
@@ -18,6 +18,19 @@
 #include "activate.h"
 #include "toolcontext.h"
 
+uint64_t lv_origin_size(const struct logical_volume *lv)
+{
+	uint64_t size;
+
+	if (lv_is_cow(lv))
+		size = (uint64_t) find_cow(lv)->len * lv->vg->extent_size;
+	else if (lv_is_origin(lv))
+		size = lv->size;
+	else
+		size = UINT64_C(0);
+	return size;
+}
+
 char *lv_path_dup(struct dm_pool *mem, const struct logical_volume *lv)
 {
 	char *repstr;
diff --git a/lib/metadata/lv.h b/lib/metadata/lv.h
index eee2811..69c4077 100644
--- a/lib/metadata/lv.h
+++ b/lib/metadata/lv.h
@@ -53,5 +53,6 @@ char *lv_attr_dup(struct dm_pool *mem, const struct logical_volume *lv);
 char *lv_uuid_dup(const struct logical_volume *lv);
 char *lv_tags_dup(const struct logical_volume *lv);
 char *lv_path_dup(struct dm_pool *mem, const struct logical_volume *lv);
+uint64_t lv_origin_size(const struct logical_volume *lv);
 
 #endif
diff --git a/lib/report/properties.c b/lib/report/properties.c
index 8890e27..27503a9 100644
--- a/lib/report/properties.c
+++ b/lib/report/properties.c
@@ -123,7 +123,7 @@ GET_LV_NUM_PROPERTY_FN(seg_count, dm_list_size(&lv->segments))
 #define _seg_count_set _not_implemented_set
 #define _origin_get _not_implemented_get
 #define _origin_set _not_implemented_set
-#define _origin_size_get _not_implemented_get
+GET_LV_NUM_PROPERTY_FN(origin_size, lv_origin_size(lv))
 #define _origin_size_set _not_implemented_set
 #define _snap_percent_get _not_implemented_get
 #define _snap_percent_set _not_implemented_set
diff --git a/lib/report/report.c b/lib/report/report.c
index 32559b8..83216b2 100644
--- a/lib/report/report.c
+++ b/lib/report/report.c
@@ -570,12 +570,7 @@ static int _originsize_disp(struct dm_report *rh, struct dm_pool *mem,
 	const struct logical_volume *lv = (const struct logical_volume *) data;
 	uint64_t size;
 
-	if (lv_is_cow(lv))
-		size = (uint64_t) find_cow(lv)->len * lv->vg->extent_size;
-	else if (lv_is_origin(lv))
-		size = lv->size;
-	else
-		size = UINT64_C(0);
+	size = lv_origin_size(lv);
 
 	return _size64_disp(rh, mem, field, &size, private);
 }
-- 
1.7.2.2

[PATCH 03/14] Refactor and add code for (lv) 'origin_size' get function.

[Petr Rockai]

Dave Wysochanski <dwysocha@redhat.com> writes:

> Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
Reviewed-By: Petr Rockai <prockai@redhat.com>

> --- a/lib/metadata/lv.c
> +++ b/lib/metadata/lv.c
> @@ -18,6 +18,19 @@
>  #include "activate.h"
>  #include "toolcontext.h"
>  
> +uint64_t lv_origin_size(const struct logical_volume *lv)
> +{
> +	uint64_t size;
> +
> +	if (lv_is_cow(lv))
> +		size = (uint64_t) find_cow(lv)->len * lv->vg->extent_size;
> +	else if (lv_is_origin(lv))
> +		size = lv->size;
> +	else
> +		size = UINT64_C(0);
> +	return size;
> +}
You don't need the UINT64_C there. size = 0 will work, you know. :)
Also, I would be inclined to write instead:

uint64_t lv_origin_size(const struct logical_volume *lv)
{
	if (lv_is_cow(lv))
		return (uint64_t) find_cow(lv)->len * lv->vg->extent_size;
	if (lv_is_origin(lv))
		return lv->size;
	return 0;
}

which has the same effect.

[snip]

> --- a/lib/report/properties.c
> +++ b/lib/report/properties.c
> @@ -123,7 +123,7 @@ GET_LV_NUM_PROPERTY_FN(seg_count, dm_list_size(&lv->segments))
>  #define _seg_count_set _not_implemented_set
>  #define _origin_get _not_implemented_get
>  #define _origin_set _not_implemented_set
> -#define _origin_size_get _not_implemented_get
> +GET_LV_NUM_PROPERTY_FN(origin_size, lv_origin_size(lv))
>  #define _origin_size_set _not_implemented_set
>  #define _snap_percent_get _not_implemented_get
>  #define _snap_percent_set _not_implemented_set
> diff --git a/lib/report/report.c b/lib/report/report.c
> index 32559b8..83216b2 100644
> --- a/lib/report/report.c
> +++ b/lib/report/report.c
> @@ -570,12 +570,7 @@ static int _originsize_disp(struct dm_report *rh, struct dm_pool *mem,
>  	const struct logical_volume *lv = (const struct logical_volume *) data;
>  	uint64_t size;
>  
> -	if (lv_is_cow(lv))
> -		size = (uint64_t) find_cow(lv)->len * lv->vg->extent_size;
> -	else if (lv_is_origin(lv))
> -		size = lv->size;
> -	else
> -		size = UINT64_C(0);
> +	size = lv_origin_size(lv);
>  
>  	return _size64_disp(rh, mem, field, &size, private);
>  }
Ok.

Yours,
   Petr.

[PATCH 03/14] Refactor and add code for (lv) 'origin_size' get function.

[Dave Wysochanski]

On Mon, 2010-10-11 at 20:13 +0200, Petr Rockai wrote:
> Dave Wysochanski <dwysocha@redhat.com> writes:
> 
> > Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
> Reviewed-By: Petr Rockai <prockai@redhat.com>
> 
> >  
> > +uint64_t lv_origin_size(const struct logical_volume *lv)
> > +{
> > +	uint64_t size;
> > +
> > +	if (lv_is_cow(lv))
> > +		size = (uint64_t) find_cow(lv)->len * lv->vg->extent_size;
> > +	else if (lv_is_origin(lv))
> > +		size = lv->size;
> > +	else
> > +		size = UINT64_C(0);
> > +	return size;
> > +}
> You don't need the UINT64_C there. size = 0 will work, you know. :)
> Also, I would be inclined to write instead:
> 
> uint64_t lv_origin_size(const struct logical_volume *lv)
> {
> 	if (lv_is_cow(lv))
> 		return (uint64_t) find_cow(lv)->len * lv->vg->extent_size;
> 	if (lv_is_origin(lv))
> 		return lv->size;
> 	return 0;
> }
> 
> which has the same effect.
> 

Very good.  An excellent cleanup - thanks.

[PATCH 04/14] Refactor and add code for (lv) 'move_pv' get function.

[Dave Wysochanski]

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 lib/metadata/lv.c       |   15 +++++++++++++++
 lib/metadata/lv.h       |    1 +
 lib/report/properties.c |    2 +-
 lib/report/report.c     |   11 +++--------
 4 files changed, 20 insertions(+), 9 deletions(-)

diff --git a/lib/metadata/lv.c b/lib/metadata/lv.c
index ae8a4fb..8f4c95a 100644
--- a/lib/metadata/lv.c
+++ b/lib/metadata/lv.c
@@ -18,6 +18,21 @@
 #include "activate.h"
 #include "toolcontext.h"
 
+char *lv_move_pv_dup(struct dm_pool *mem, const struct logical_volume *lv)
+{
+	const char *name;
+	struct lv_segment *seg;
+
+	dm_list_iterate_items(seg, &lv->segments) {
+		if (!(seg->status & PVMOVE))
+			continue;
+		name = dev_name(seg_dev(seg, 0));
+	}
+	if (name)
+		return dm_pool_strndup(mem, name, strlen(name) + 1);
+	return NULL;
+}
+
 uint64_t lv_origin_size(const struct logical_volume *lv)
 {
 	uint64_t size;
diff --git a/lib/metadata/lv.h b/lib/metadata/lv.h
index 69c4077..bfb38f9 100644
--- a/lib/metadata/lv.h
+++ b/lib/metadata/lv.h
@@ -54,5 +54,6 @@ char *lv_uuid_dup(const struct logical_volume *lv);
 char *lv_tags_dup(const struct logical_volume *lv);
 char *lv_path_dup(struct dm_pool *mem, const struct logical_volume *lv);
 uint64_t lv_origin_size(const struct logical_volume *lv);
+char *lv_move_pv_dup(struct dm_pool *mem, const struct logical_volume *lv);
 
 #endif
diff --git a/lib/report/properties.c b/lib/report/properties.c
index 27503a9..bcec0c8 100644
--- a/lib/report/properties.c
+++ b/lib/report/properties.c
@@ -129,7 +129,7 @@ GET_LV_NUM_PROPERTY_FN(origin_size, lv_origin_size(lv))
 #define _snap_percent_set _not_implemented_set
 #define _copy_percent_get _not_implemented_get
 #define _copy_percent_set _not_implemented_set
-#define _move_pv_get _not_implemented_get
+GET_LV_STR_PROPERTY_FN(move_pv, lv_move_pv_dup(lv->vg->vgmem, lv))
 #define _move_pv_set _not_implemented_set
 #define _convert_lv_get _not_implemented_get
 #define _convert_lv_set _not_implemented_set
diff --git a/lib/report/report.c b/lib/report/report.c
index 83216b2..26ff879 100644
--- a/lib/report/report.c
+++ b/lib/report/report.c
@@ -378,16 +378,11 @@ static int _movepv_disp(struct dm_report *rh, struct dm_pool *mem __attribute__(
 {
 	const struct logical_volume *lv = (const struct logical_volume *) data;
 	const char *name;
-	struct lv_segment *seg;
 
-	dm_list_iterate_items(seg, &lv->segments) {
-		if (!(seg->status & PVMOVE))
-			continue;
-		name = dev_name(seg_dev(seg, 0));
+	if (!(name = lv_move_pv_dup(mem, lv)))
+		dm_report_field_set_value(field, "", NULL);
+	else
 		return dm_report_field_string(rh, field, &name);
-	}
-
-	dm_report_field_set_value(field, "", NULL);
 	return 1;
 }
 
-- 
1.7.2.2

[PATCH 04/14] Refactor and add code for (lv) 'move_pv' get function.

[Petr Rockai]

Dave Wysochanski <dwysocha@redhat.com> writes:

> Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
Reviewed-By: Petr Rockai <prockai@redhat.com>

> diff --git a/lib/metadata/lv.c b/lib/metadata/lv.c
> index ae8a4fb..8f4c95a 100644
> --- a/lib/metadata/lv.c
> +++ b/lib/metadata/lv.c
> @@ -18,6 +18,21 @@
>  #include "activate.h"
>  #include "toolcontext.h"
>  
> +char *lv_move_pv_dup(struct dm_pool *mem, const struct logical_volume *lv)
> +{
> +	const char *name;
> +	struct lv_segment *seg;
> +
> +	dm_list_iterate_items(seg, &lv->segments) {
> +		if (!(seg->status & PVMOVE))
> +			continue;
> +		name = dev_name(seg_dev(seg, 0));
> +	}
> +	if (name)
> +		return dm_pool_strndup(mem, name, strlen(name) + 1);
> +	return NULL;
> +}

I believe that dev_name can't give you NULL and that there is only ever
a single matching device (even if there wasn't, giving first or last
shouldn't make any difference). So you can remove the "name" temporary
and just say

if (seg->status & PVMOVE)
   return dm_pool_strdup(mem, dev_name(seg_dev(seg, 0)))

in the loop, and return NULL after the loop. (Even if you prefer the one
you already have, changing strndup to strdup is a good idea, since doing
the above comes with no benefits, just downsides.)

> --- a/lib/metadata/lv.h
> +++ b/lib/metadata/lv.h
> @@ -54,5 +54,6 @@ char *lv_uuid_dup(const struct logical_volume *lv);
>  char *lv_tags_dup(const struct logical_volume *lv);
>  char *lv_path_dup(struct dm_pool *mem, const struct logical_volume *lv);
>  uint64_t lv_origin_size(const struct logical_volume *lv);
> +char *lv_move_pv_dup(struct dm_pool *mem, const struct logical_volume *lv);
>  
>  #endif

> --- a/lib/report/properties.c
> +++ b/lib/report/properties.c
> @@ -129,7 +129,7 @@ GET_LV_NUM_PROPERTY_FN(origin_size, lv_origin_size(lv))
>  #define _snap_percent_set _not_implemented_set
>  #define _copy_percent_get _not_implemented_get
>  #define _copy_percent_set _not_implemented_set
> -#define _move_pv_get _not_implemented_get
> +GET_LV_STR_PROPERTY_FN(move_pv, lv_move_pv_dup(lv->vg->vgmem, lv))
>  #define _move_pv_set _not_implemented_set
>  #define _convert_lv_get _not_implemented_get
>  #define _convert_lv_set _not_implemented_set

> --- a/lib/report/report.c
> +++ b/lib/report/report.c
> @@ -378,16 +378,11 @@ static int _movepv_disp(struct dm_report *rh, struct dm_pool *mem __attribute__(
>  {
>  	const struct logical_volume *lv = (const struct logical_volume *) data;
>  	const char *name;
> -	struct lv_segment *seg;
>  
> -	dm_list_iterate_items(seg, &lv->segments) {
> -		if (!(seg->status & PVMOVE))
> -			continue;
> -		name = dev_name(seg_dev(seg, 0));
> +	if (!(name = lv_move_pv_dup(mem, lv)))
> +		dm_report_field_set_value(field, "", NULL);
> +	else
>  		return dm_report_field_string(rh, field, &name);
> -	}
> -
> -	dm_report_field_set_value(field, "", NULL);
>  	return 1;
>  }
Ok.

[PATCH 04/14] Refactor and add code for (lv) 'move_pv' get function.

[Dave Wysochanski]

On Mon, 2010-10-11 at 20:21 +0200, Petr Rockai wrote:
> Dave Wysochanski <dwysocha@redhat.com> writes:
> 
> > Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
> Reviewed-By: Petr Rockai <prockai@redhat.com>
> 
> > diff --git a/lib/metadata/lv.c b/lib/metadata/lv.c
> > index ae8a4fb..8f4c95a 100644
> > --- a/lib/metadata/lv.c
> > +++ b/lib/metadata/lv.c
> > @@ -18,6 +18,21 @@
> >  #include "activate.h"
> >  #include "toolcontext.h"
> >  
> > +char *lv_move_pv_dup(struct dm_pool *mem, const struct logical_volume *lv)
> > +{
> > +	const char *name;
> > +	struct lv_segment *seg;
> > +
> > +	dm_list_iterate_items(seg, &lv->segments) {
> > +		if (!(seg->status & PVMOVE))
> > +			continue;
> > +		name = dev_name(seg_dev(seg, 0));
> > +	}
> > +	if (name)
> > +		return dm_pool_strndup(mem, name, strlen(name) + 1);
> > +	return NULL;
> > +}
> 
> I believe that dev_name can't give you NULL and that there is only ever
> a single matching device (even if there wasn't, giving first or last
> shouldn't make any difference). So you can remove the "name" temporary
> and just say
> 
> if (seg->status & PVMOVE)
>    return dm_pool_strdup(mem, dev_name(seg_dev(seg, 0)))
> 
> in the loop, and return NULL after the loop. (Even if you prefer the one
> you already have, changing strndup to strdup is a good idea, since doing
> the above comes with no benefits, just downsides.)
> 

Ok, I've made these cleanups as well.  Another good simplification.

[PATCH 05/14] Refactor and add code for (lv) 'convert_lv' get function.

[Dave Wysochanski]

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

diff --git a/lib/metadata/lv.c b/lib/metadata/lv.c
index 8f4c95a..fd6d36d 100644
--- a/lib/metadata/lv.c
+++ b/lib/metadata/lv.c
@@ -18,6 +18,26 @@
 #include "activate.h"
 #include "toolcontext.h"
 
+char *lv_convert_lv_dup(struct dm_pool *mem, const struct logical_volume *lv)
+{
+	struct lv_segment *seg;
+	const char *name = NULL;
+
+	if (lv->status & CONVERTING) {
+		if (lv->status & MIRRORED) {
+			seg = first_seg(lv);
+
+			/* Temporary mirror is always area_num == 0 */
+			if (seg_type(seg, 0) == AREA_LV &&
+			    is_temporary_mirror_layer(seg_lv(seg, 0)))
+				name = seg_lv(seg, 0)->name;
+		}
+	}
+	if (name)
+		return dm_pool_strndup(mem, name, strlen(name) + 1);
+	return NULL;
+}
+
 char *lv_move_pv_dup(struct dm_pool *mem, const struct logical_volume *lv)
 {
 	const char *name;
diff --git a/lib/metadata/lv.h b/lib/metadata/lv.h
index bfb38f9..1ba932b 100644
--- a/lib/metadata/lv.h
+++ b/lib/metadata/lv.h
@@ -55,5 +55,6 @@ char *lv_tags_dup(const struct logical_volume *lv);
 char *lv_path_dup(struct dm_pool *mem, const struct logical_volume *lv);
 uint64_t lv_origin_size(const struct logical_volume *lv);
 char *lv_move_pv_dup(struct dm_pool *mem, const struct logical_volume *lv);
+char *lv_convert_lv_dup(struct dm_pool *mem, const struct logical_volume *lv);
 
 #endif
diff --git a/lib/report/properties.c b/lib/report/properties.c
index bcec0c8..db02683 100644
--- a/lib/report/properties.c
+++ b/lib/report/properties.c
@@ -131,7 +131,7 @@ GET_LV_NUM_PROPERTY_FN(origin_size, lv_origin_size(lv))
 #define _copy_percent_set _not_implemented_set
 GET_LV_STR_PROPERTY_FN(move_pv, lv_move_pv_dup(lv->vg->vgmem, lv))
 #define _move_pv_set _not_implemented_set
-#define _convert_lv_get _not_implemented_get
+GET_LV_STR_PROPERTY_FN(convert_lv, lv_convert_lv_dup(lv->vg->vgmem, lv))
 #define _convert_lv_set _not_implemented_set
 GET_LV_STR_PROPERTY_FN(lv_tags, lv_tags_dup(lv))
 #define _lv_tags_set _not_implemented_set
diff --git a/lib/report/report.c b/lib/report/report.c
index 26ff879..bb2d4ce 100644
--- a/lib/report/report.c
+++ b/lib/report/report.c
@@ -392,19 +392,8 @@ static int _convertlv_disp(struct dm_report *rh, struct dm_pool *mem __attribute
 {
 	const struct logical_volume *lv = (const struct logical_volume *) data;
 	const char *name = NULL;
-	struct lv_segment *seg;
-
-	if (lv->status & CONVERTING) {
-		if (lv->status & MIRRORED) {
-			seg = first_seg(lv);
-
-			/* Temporary mirror is always area_num == 0 */
-			if (seg_type(seg, 0) == AREA_LV &&
-			    is_temporary_mirror_layer(seg_lv(seg, 0)))
-				name = seg_lv(seg, 0)->name;
-		}
-	}
 
+	name = lv_convert_lv_dup(mem, lv);
 	if (name)
 		return dm_report_field_string(rh, field, &name);
 
-- 
1.7.2.2

[PATCH 05/14] Refactor and add code for (lv) 'convert_lv' get function.

[Petr Rockai]

Dave Wysochanski <dwysocha@redhat.com> writes:

> Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
Reviewed-By: Petr Rockai <prockai@redhat.com>

> --- a/lib/metadata/lv.c
> +++ b/lib/metadata/lv.c
> @@ -18,6 +18,26 @@
>  #include "activate.h"
>  #include "toolcontext.h"
>  
> +char *lv_convert_lv_dup(struct dm_pool *mem, const struct logical_volume *lv)
> +{
> +	struct lv_segment *seg;
> +	const char *name = NULL;
> +
> +	if (lv->status & CONVERTING) {
> +		if (lv->status & MIRRORED) {
> +			seg = first_seg(lv);
> +
> +			/* Temporary mirror is always area_num == 0 */
> +			if (seg_type(seg, 0) == AREA_LV &&
> +			    is_temporary_mirror_layer(seg_lv(seg, 0)))
> +				name = seg_lv(seg, 0)->name;
> +		}
> +	}
> +	if (name)
> +		return dm_pool_strndup(mem, name, strlen(name) + 1);
> +	return NULL;
> +}
strndup/strdup again, use of "name" can be eliminated as well...

> +
>  char *lv_move_pv_dup(struct dm_pool *mem, const struct logical_volume *lv)
>  {
>  	const char *name;

> --- a/lib/metadata/lv.h
> +++ b/lib/metadata/lv.h
> @@ -55,5 +55,6 @@ char *lv_tags_dup(const struct logical_volume *lv);
>  char *lv_path_dup(struct dm_pool *mem, const struct logical_volume *lv);
>  uint64_t lv_origin_size(const struct logical_volume *lv);
>  char *lv_move_pv_dup(struct dm_pool *mem, const struct logical_volume *lv);
> +char *lv_convert_lv_dup(struct dm_pool *mem, const struct logical_volume *lv);
>  
>  #endif

> --- a/lib/report/properties.c
> +++ b/lib/report/properties.c
> @@ -131,7 +131,7 @@ GET_LV_NUM_PROPERTY_FN(origin_size, lv_origin_size(lv))
>  #define _copy_percent_set _not_implemented_set
>  GET_LV_STR_PROPERTY_FN(move_pv, lv_move_pv_dup(lv->vg->vgmem, lv))
>  #define _move_pv_set _not_implemented_set
> -#define _convert_lv_get _not_implemented_get
> +GET_LV_STR_PROPERTY_FN(convert_lv, lv_convert_lv_dup(lv->vg->vgmem, lv))
>  #define _convert_lv_set _not_implemented_set
>  GET_LV_STR_PROPERTY_FN(lv_tags, lv_tags_dup(lv))
>  #define _lv_tags_set _not_implemented_set

> --- a/lib/report/report.c
> +++ b/lib/report/report.c
> @@ -392,19 +392,8 @@ static int _convertlv_disp(struct dm_report *rh, struct dm_pool *mem __attribute
>  {
>  	const struct logical_volume *lv = (const struct logical_volume *) data;
>  	const char *name = NULL;
> -	struct lv_segment *seg;
> -
> -	if (lv->status & CONVERTING) {
> -		if (lv->status & MIRRORED) {
> -			seg = first_seg(lv);
> -
> -			/* Temporary mirror is always area_num == 0 */
> -			if (seg_type(seg, 0) == AREA_LV &&
> -			    is_temporary_mirror_layer(seg_lv(seg, 0)))
> -				name = seg_lv(seg, 0)->name;
> -		}
> -	}
>  
> +	name = lv_convert_lv_dup(mem, lv);
>  	if (name)
>  		return dm_report_field_string(rh, field, &name);

Ok. Although we don't need a string duplicate here. On the other hand,
having a non-dup version of the above code and the dup version
implemented in terms of that would be overkill, I guess...

Yours,
   Petr.

[PATCH 05/14] Refactor and add code for (lv) 'convert_lv' get function.

[Dave Wysochanski]

On Mon, 2010-10-11 at 20:25 +0200, Petr Rockai wrote:
> Dave Wysochanski <dwysocha@redhat.com> writes:
> 
> > Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
> Reviewed-By: Petr Rockai <prockai@redhat.com>
> 
> > --- a/lib/metadata/lv.c
> > +++ b/lib/metadata/lv.c
> > @@ -18,6 +18,26 @@
> >  #include "activate.h"
> >  #include "toolcontext.h"
> >  
> > +char *lv_convert_lv_dup(struct dm_pool *mem, const struct logical_volume *lv)
> > +{
> > +	struct lv_segment *seg;
> > +	const char *name = NULL;
> > +
> > +	if (lv->status & CONVERTING) {
> > +		if (lv->status & MIRRORED) {
> > +			seg = first_seg(lv);
> > +
> > +			/* Temporary mirror is always area_num == 0 */
> > +			if (seg_type(seg, 0) == AREA_LV &&
> > +			    is_temporary_mirror_layer(seg_lv(seg, 0)))
> > +				name = seg_lv(seg, 0)->name;
> > +		}
> > +	}
> > +	if (name)
> > +		return dm_pool_strndup(mem, name, strlen(name) + 1);
> > +	return NULL;
> > +}
> strndup/strdup again, use of "name" can be eliminated as well...
> 

Done.  I also simplified the lv->status checks:

+char *lv_convert_lv_dup(struct dm_pool *mem, const struct logical_volume *lv)
+{
+       struct lv_segment *seg;
+
+       if (lv->status & (CONVERTING|MIRRORED)) {
+               seg = first_seg(lv);
+
+               /* Temporary mirror is always area_num == 0 */
+               if (seg_type(seg, 0) == AREA_LV &&
+                   is_temporary_mirror_layer(seg_lv(seg, 0)))
+                       return dm_pool_strdup(mem, seg_lv(seg, 0)->name);
+       }
+       return NULL;
+}


> > +
> >  char *lv_move_pv_dup(struct dm_pool *mem, const struct logical_volume *lv)
> >  {
> >  	const char *name;
> 
> > --- a/lib/metadata/lv.h
> > +++ b/lib/metadata/lv.h
> > @@ -55,5 +55,6 @@ char *lv_tags_dup(const struct logical_volume *lv);
> >  char *lv_path_dup(struct dm_pool *mem, const struct logical_volume *lv);
> >  uint64_t lv_origin_size(const struct logical_volume *lv);
> >  char *lv_move_pv_dup(struct dm_pool *mem, const struct logical_volume *lv);
> > +char *lv_convert_lv_dup(struct dm_pool *mem, const struct logical_volume *lv);
> >  
> >  #endif
> 
> > --- a/lib/report/properties.c
> > +++ b/lib/report/properties.c
> > @@ -131,7 +131,7 @@ GET_LV_NUM_PROPERTY_FN(origin_size, lv_origin_size(lv))
> >  #define _copy_percent_set _not_implemented_set
> >  GET_LV_STR_PROPERTY_FN(move_pv, lv_move_pv_dup(lv->vg->vgmem, lv))
> >  #define _move_pv_set _not_implemented_set
> > -#define _convert_lv_get _not_implemented_get
> > +GET_LV_STR_PROPERTY_FN(convert_lv, lv_convert_lv_dup(lv->vg->vgmem, lv))
> >  #define _convert_lv_set _not_implemented_set
> >  GET_LV_STR_PROPERTY_FN(lv_tags, lv_tags_dup(lv))
> >  #define _lv_tags_set _not_implemented_set
> 
> > --- a/lib/report/report.c
> > +++ b/lib/report/report.c
> > @@ -392,19 +392,8 @@ static int _convertlv_disp(struct dm_report *rh, struct dm_pool *mem __attribute
> >  {
> >  	const struct logical_volume *lv = (const struct logical_volume *) data;
> >  	const char *name = NULL;
> > -	struct lv_segment *seg;
> > -
> > -	if (lv->status & CONVERTING) {
> > -		if (lv->status & MIRRORED) {
> > -			seg = first_seg(lv);
> > -
> > -			/* Temporary mirror is always area_num == 0 */
> > -			if (seg_type(seg, 0) == AREA_LV &&
> > -			    is_temporary_mirror_layer(seg_lv(seg, 0)))
> > -				name = seg_lv(seg, 0)->name;
> > -		}
> > -	}
> >  
> > +	name = lv_convert_lv_dup(mem, lv);
> >  	if (name)
> >  		return dm_report_field_string(rh, field, &name);
> 
> Ok. Although we don't need a string duplicate here. On the other hand,
> having a non-dup version of the above code and the dup version
> implemented in terms of that would be overkill, I guess...
> 
> Yours,
>    Petr.
> 
> --
> lvm-devel mailing list
> lvm-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/lvm-devel

[PATCH 05/14] Refactor and add code for (lv) 'convert_lv' get function.

[Petr Rockai]

Hi,

Dave Wysochanski <dwysocha@redhat.com> writes:

>> > +char *lv_convert_lv_dup(struct dm_pool *mem, const struct logical_volume *lv)
>> > +{
>> > +	struct lv_segment *seg;
>> > +	const char *name = NULL;
>> > +
>> > +	if (lv->status & CONVERTING) {
>> > +		if (lv->status & MIRRORED) {
>> > +			seg = first_seg(lv);
>> > +
>> > +			/* Temporary mirror is always area_num == 0 */
>> > +			if (seg_type(seg, 0) == AREA_LV &&
>> > +			    is_temporary_mirror_layer(seg_lv(seg, 0)))
>> > +				name = seg_lv(seg, 0)->name;
>> > +		}
>> > +	}
>> > +	if (name)
>> > +		return dm_pool_strndup(mem, name, strlen(name) + 1);
>> > +	return NULL;
>> > +}

> Done.  I also simplified the lv->status checks:
>
> +char *lv_convert_lv_dup(struct dm_pool *mem, const struct logical_volume *lv)
> +{
> +       struct lv_segment *seg;
> +
> +       if (lv->status & (CONVERTING|MIRRORED)) {
> +               seg = first_seg(lv);
> +
> +               /* Temporary mirror is always area_num == 0 */
> +               if (seg_type(seg, 0) == AREA_LV &&
> +                   is_temporary_mirror_layer(seg_lv(seg, 0)))
> +                       return dm_pool_strdup(mem, seg_lv(seg, 0)->name);
> +       }
> +       return NULL;
> +}
>

Looks OK to me. So the first ~half of the patches should be ready to go
in. I haven't noticed any forward dependencies, so you can check in what
we have (unless I have missed something), I'll get through more of the
review later today.

Yours,
   Petr.

[PATCH 05/14] Refactor and add code for (lv) 'convert_lv' get function.

[Dave Wysochanski]

On Tue, 2010-10-12 at 17:55 +0200, Petr Rockai wrote:
> Hi,
> 
> Dave Wysochanski <dwysocha@redhat.com> writes:
> 
> >> > +char *lv_convert_lv_dup(struct dm_pool *mem, const struct logical_volume *lv)
> >> > +{
> >> > +	struct lv_segment *seg;
> >> > +	const char *name = NULL;
> >> > +
> >> > +	if (lv->status & CONVERTING) {
> >> > +		if (lv->status & MIRRORED) {
> >> > +			seg = first_seg(lv);
> >> > +
> >> > +			/* Temporary mirror is always area_num == 0 */
> >> > +			if (seg_type(seg, 0) == AREA_LV &&
> >> > +			    is_temporary_mirror_layer(seg_lv(seg, 0)))
> >> > +				name = seg_lv(seg, 0)->name;
> >> > +		}
> >> > +	}
> >> > +	if (name)
> >> > +		return dm_pool_strndup(mem, name, strlen(name) + 1);
> >> > +	return NULL;
> >> > +}
> 
> > Done.  I also simplified the lv->status checks:
> >
> > +char *lv_convert_lv_dup(struct dm_pool *mem, const struct logical_volume *lv)
> > +{
> > +       struct lv_segment *seg;
> > +
> > +       if (lv->status & (CONVERTING|MIRRORED)) {
> > +               seg = first_seg(lv);
> > +
> > +               /* Temporary mirror is always area_num == 0 */
> > +               if (seg_type(seg, 0) == AREA_LV &&
> > +                   is_temporary_mirror_layer(seg_lv(seg, 0)))
> > +                       return dm_pool_strdup(mem, seg_lv(seg, 0)->name);
> > +       }
> > +       return NULL;
> > +}
> >
> 
> Looks OK to me. So the first ~half of the patches should be ready to go
> in. I haven't noticed any forward dependencies, so you can check in what
> we have (unless I have missed something), I'll get through more of the
> review later today.
> 

Ok - checking in through patches #1-8.

[PATCH 06/14] Refactor and add code for (lv) 'lv_kernel_{major|minor}' get functions.

[Dave Wysochanski]

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 lib/metadata/lv.c       |   17 +++++++++++++++++
 lib/metadata/lv.h       |    2 ++
 lib/report/properties.c |    4 ++--
 lib/report/report.c     |   12 ++++++------
 4 files changed, 27 insertions(+), 8 deletions(-)

diff --git a/lib/metadata/lv.c b/lib/metadata/lv.c
index fd6d36d..af2b418 100644
--- a/lib/metadata/lv.c
+++ b/lib/metadata/lv.c
@@ -18,6 +18,23 @@
 #include "activate.h"
 #include "toolcontext.h"
 
+int lv_kernel_minor(const struct logical_volume *lv)
+{
+	struct lvinfo info;
+
+	if (lv_info(lv->vg->cmd, lv, 0, &info, 0, 0) && info.exists)
+		return info.minor;
+	return -1;
+}
+
+int lv_kernel_major(const struct logical_volume *lv)
+{
+	struct lvinfo info;
+	if (lv_info(lv->vg->cmd, lv, 0, &info, 0, 0) && info.exists)
+		return info.major;
+	return -1;
+}
+
 char *lv_convert_lv_dup(struct dm_pool *mem, const struct logical_volume *lv)
 {
 	struct lv_segment *seg;
diff --git a/lib/metadata/lv.h b/lib/metadata/lv.h
index 1ba932b..557f54a 100644
--- a/lib/metadata/lv.h
+++ b/lib/metadata/lv.h
@@ -56,5 +56,7 @@ char *lv_path_dup(struct dm_pool *mem, const struct logical_volume *lv);
 uint64_t lv_origin_size(const struct logical_volume *lv);
 char *lv_move_pv_dup(struct dm_pool *mem, const struct logical_volume *lv);
 char *lv_convert_lv_dup(struct dm_pool *mem, const struct logical_volume *lv);
+int lv_kernel_major(const struct logical_volume *lv);
+int lv_kernel_minor(const struct logical_volume *lv);
 
 #endif
diff --git a/lib/report/properties.c b/lib/report/properties.c
index db02683..5e42014 100644
--- a/lib/report/properties.c
+++ b/lib/report/properties.c
@@ -111,9 +111,9 @@ GET_LV_NUM_PROPERTY_FN(lv_minor, lv->minor)
 #define _lv_minor_set _not_implemented_set
 #define _lv_read_ahead_get _not_implemented_get
 #define _lv_read_ahead_set _not_implemented_set
-#define _lv_kernel_major_get _not_implemented_get
+GET_LV_NUM_PROPERTY_FN(lv_kernel_major, lv_kernel_major(lv))
 #define _lv_kernel_major_set _not_implemented_set
-#define _lv_kernel_minor_get _not_implemented_get
+GET_LV_NUM_PROPERTY_FN(lv_kernel_minor, lv_kernel_minor(lv))
 #define _lv_kernel_minor_set _not_implemented_set
 #define _lv_kernel_read_ahead_get _not_implemented_get
 #define _lv_kernel_read_ahead_set _not_implemented_set
diff --git a/lib/report/report.c b/lib/report/report.c
index bb2d4ce..f3e3e41 100644
--- a/lib/report/report.c
+++ b/lib/report/report.c
@@ -212,10 +212,10 @@ static int _lvkmaj_disp(struct dm_report *rh, struct dm_pool *mem __attribute__(
 			const void *data, void *private __attribute__((unused)))
 {
 	const struct logical_volume *lv = (const struct logical_volume *) data;
-	struct lvinfo info;
+	int major;
 
-	if (lv_info(lv->vg->cmd, lv, 0, &info, 0, 0) && info.exists)
-		return dm_report_field_int(rh, field, &info.major);
+	if ((major = lv_kernel_major(lv)) >= 0)
+		return dm_report_field_int(rh, field, &major);
 
 	return dm_report_field_int32(rh, field, &_minusone32);
 }
@@ -225,10 +225,10 @@ static int _lvkmin_disp(struct dm_report *rh, struct dm_pool *mem __attribute__(
 			const void *data, void *private __attribute__((unused)))
 {
 	const struct logical_volume *lv = (const struct logical_volume *) data;
-	struct lvinfo info;
+	int minor;
 
-	if (lv_info(lv->vg->cmd, lv, 0, &info, 0, 0) && info.exists)
-		return dm_report_field_int(rh, field, &info.minor);
+	if ((minor = lv_kernel_minor(lv)) >= 0)
+		return dm_report_field_int(rh, field, &minor);
 
 	return dm_report_field_int32(rh, field, &_minusone32);
 }
-- 
1.7.2.2

[PATCH 06/14] Refactor and add code for (lv) 'lv_kernel_{major|minor}' get functions.

[Petr Rockai]

Dave Wysochanski <dwysocha@redhat.com> writes:

> Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
Reviewed-By: Petr Rockai <prockai@redhat.com>

> --- a/lib/metadata/lv.c
> +++ b/lib/metadata/lv.c
> @@ -18,6 +18,23 @@
>  #include "activate.h"
>  #include "toolcontext.h"
>  
> +int lv_kernel_minor(const struct logical_volume *lv)
> +{
> +	struct lvinfo info;
> +
> +	if (lv_info(lv->vg->cmd, lv, 0, &info, 0, 0) && info.exists)
> +		return info.minor;
> +	return -1;
> +}
> +
> +int lv_kernel_major(const struct logical_volume *lv)
> +{
> +	struct lvinfo info;
> +	if (lv_info(lv->vg->cmd, lv, 0, &info, 0, 0) && info.exists)
> +		return info.major;
> +	return -1;
> +}
> +

> --- a/lib/report/report.c
> +++ b/lib/report/report.c
> @@ -212,10 +212,10 @@ static int _lvkmaj_disp(struct dm_report *rh, struct dm_pool *mem __attribute__(
>  			const void *data, void *private __attribute__((unused)))
>  {
>  	const struct logical_volume *lv = (const struct logical_volume *) data;
> -	struct lvinfo info;
> +	int major;
>  
> -	if (lv_info(lv->vg->cmd, lv, 0, &info, 0, 0) && info.exists)
> -		return dm_report_field_int(rh, field, &info.major);
> +	if ((major = lv_kernel_major(lv)) >= 0)
> +		return dm_report_field_int(rh, field, &major);
>  
>  	return dm_report_field_int32(rh, field, &_minusone32);
>  }
> @@ -225,10 +225,10 @@ static int _lvkmin_disp(struct dm_report *rh, struct dm_pool *mem __attribute__(
>  			const void *data, void *private __attribute__((unused)))
>  {
>  	const struct logical_volume *lv = (const struct logical_volume *) data;
> -	struct lvinfo info;
> +	int minor;
>  
> -	if (lv_info(lv->vg->cmd, lv, 0, &info, 0, 0) && info.exists)
> -		return dm_report_field_int(rh, field, &info.minor);
> +	if ((minor = lv_kernel_minor(lv)) >= 0)
> +		return dm_report_field_int(rh, field, &minor);
>  
>  	return dm_report_field_int32(rh, field, &_minusone32);
>  }

OK.

Yours,
   Petr.

[PATCH 07/14] Refactor and add code for (lv) 'mirror_log' get function.

[Dave Wysochanski]

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 lib/metadata/lv.c       |   16 ++++++++++++++++
 lib/metadata/lv.h       |    1 +
 lib/report/properties.c |    2 +-
 lib/report/report.c     |   11 ++++-------
 4 files changed, 22 insertions(+), 8 deletions(-)

diff --git a/lib/metadata/lv.c b/lib/metadata/lv.c
index af2b418..4fad293 100644
--- a/lib/metadata/lv.c
+++ b/lib/metadata/lv.c
@@ -17,6 +17,22 @@
 #include "metadata.h"
 #include "activate.h"
 #include "toolcontext.h"
+#include "segtype.h"
+
+char *lv_mirror_log_dup(struct dm_pool *mem, const struct logical_volume *lv)
+{
+	struct lv_segment *seg;
+	const char *name = NULL;
+
+	dm_list_iterate_items(seg, &lv->segments) {
+		if (!seg_is_mirrored(seg) || !seg->log_lv)
+			continue;
+		name = seg->log_lv->name;
+	}
+	if (name)
+		return dm_pool_strndup(mem, name, strlen(name) + 1);
+	return NULL;
+}
 
 int lv_kernel_minor(const struct logical_volume *lv)
 {
diff --git a/lib/metadata/lv.h b/lib/metadata/lv.h
index 557f54a..8d069af 100644
--- a/lib/metadata/lv.h
+++ b/lib/metadata/lv.h
@@ -58,5 +58,6 @@ char *lv_move_pv_dup(struct dm_pool *mem, const struct logical_volume *lv);
 char *lv_convert_lv_dup(struct dm_pool *mem, const struct logical_volume *lv);
 int lv_kernel_major(const struct logical_volume *lv);
 int lv_kernel_minor(const struct logical_volume *lv);
+char *lv_mirror_log_dup(struct dm_pool *mem, const struct logical_volume *lv);
 
 #endif
diff --git a/lib/report/properties.c b/lib/report/properties.c
index 5e42014..eba2b59 100644
--- a/lib/report/properties.c
+++ b/lib/report/properties.c
@@ -135,7 +135,7 @@ GET_LV_STR_PROPERTY_FN(convert_lv, lv_convert_lv_dup(lv->vg->vgmem, lv))
 #define _convert_lv_set _not_implemented_set
 GET_LV_STR_PROPERTY_FN(lv_tags, lv_tags_dup(lv))
 #define _lv_tags_set _not_implemented_set
-#define _mirror_log_get _not_implemented_get
+GET_LV_STR_PROPERTY_FN(mirror_log, lv_mirror_log_dup(lv->vg->vgmem, lv))
 #define _mirror_log_set _not_implemented_set
 #define _modules_get _not_implemented_get
 #define _modules_set _not_implemented_set
diff --git a/lib/report/report.c b/lib/report/report.c
index f3e3e41..65becaf 100644
--- a/lib/report/report.c
+++ b/lib/report/report.c
@@ -297,14 +297,11 @@ static int _loglv_disp(struct dm_report *rh, struct dm_pool *mem __attribute__((
 		       const void *data, void *private __attribute__((unused)))
 {
 	const struct logical_volume *lv = (const struct logical_volume *) data;
-	struct lv_segment *seg;
+	const char *name;
 
-	dm_list_iterate_items(seg, &lv->segments) {
-		if (!seg_is_mirrored(seg) || !seg->log_lv)
-			continue;
-		return dm_report_field_string(rh, field,
-					      (const char **) &seg->log_lv->name);
-	}
+	name = lv_mirror_log_dup(mem, lv);
+	if (name)
+		return dm_report_field_string(rh, field, &name);
 
 	dm_report_field_set_value(field, "", NULL);
 	return 1;
-- 
1.7.2.2

[PATCH 07/14] Refactor and add code for (lv) 'mirror_log' get function.

[Petr Rockai]

Dave Wysochanski <dwysocha@redhat.com> writes:

> Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
Reviewed-By: Petr Rockai <prockai@redhat.com>

> --- a/lib/metadata/lv.c
> +++ b/lib/metadata/lv.c
> @@ -17,6 +17,22 @@
>  #include "metadata.h"
>  #include "activate.h"
>  #include "toolcontext.h"
> +#include "segtype.h"
> +
> +char *lv_mirror_log_dup(struct dm_pool *mem, const struct logical_volume *lv)
> +{
> +	struct lv_segment *seg;
> +	const char *name = NULL;
> +
> +	dm_list_iterate_items(seg, &lv->segments) {
> +		if (!seg_is_mirrored(seg) || !seg->log_lv)
> +			continue;
> +		name = seg->log_lv->name;
> +	}
> +	if (name)
> +		return dm_pool_strndup(mem, name, strlen(name) + 1);
> +	return NULL;
> +}
strndup & unnecessary temporary again

> --- a/lib/report/properties.c
> +++ b/lib/report/properties.c
> @@ -135,7 +135,7 @@ GET_LV_STR_PROPERTY_FN(convert_lv, lv_convert_lv_dup(lv->vg->vgmem, lv))
>  #define _convert_lv_set _not_implemented_set
>  GET_LV_STR_PROPERTY_FN(lv_tags, lv_tags_dup(lv))
>  #define _lv_tags_set _not_implemented_set
> -#define _mirror_log_get _not_implemented_get
> +GET_LV_STR_PROPERTY_FN(mirror_log, lv_mirror_log_dup(lv->vg->vgmem, lv))
>  #define _mirror_log_set _not_implemented_set
>  #define _modules_get _not_implemented_get
>  #define _modules_set _not_implemented_set

> --- a/lib/report/report.c
> +++ b/lib/report/report.c
> @@ -297,14 +297,11 @@ static int _loglv_disp(struct dm_report *rh, struct dm_pool *mem __attribute__((
>  		       const void *data, void *private __attribute__((unused)))
>  {
>  	const struct logical_volume *lv = (const struct logical_volume *) data;
> -	struct lv_segment *seg;
> +	const char *name;
>  
> -	dm_list_iterate_items(seg, &lv->segments) {
> -		if (!seg_is_mirrored(seg) || !seg->log_lv)
> -			continue;
> -		return dm_report_field_string(rh, field,
> -					      (const char **) &seg->log_lv->name);
> -	}
> +	name = lv_mirror_log_dup(mem, lv);
> +	if (name)
> +		return dm_report_field_string(rh, field, &name);
>  
>  	dm_report_field_set_value(field, "", NULL);
>  	return 1;

OK. Your other patches use if ((name = ...)) style, maybe use it here as
well? (Or change the others to use this style instead?)

Yours,
   Petr.

[PATCH 07/14] Refactor and add code for (lv) 'mirror_log' get function.

[Dave Wysochanski]

On Mon, 2010-10-11 at 20:29 +0200, Petr Rockai wrote:
> Dave Wysochanski <dwysocha@redhat.com> writes:
> 
> > Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
> Reviewed-By: Petr Rockai <prockai@redhat.com>
> 
> > +		name = seg->log_lv->name;
> > +	}
> > +	if (name)
> > +		return dm_pool_strndup(mem, name, strlen(name) + 1);
> > +	return NULL;
> > +}
> strndup & unnecessary temporary again
> 

Fixed.

> > -		if (!seg_is_mirrored(seg) || !seg->log_lv)
> > -			continue;
> > -		return dm_report_field_string(rh, field,
> > -					      (const char **) &seg->log_lv->name);
> > -	}
> > +	name = lv_mirror_log_dup(mem, lv);
> > +	if (name)
> > +		return dm_report_field_string(rh, field, &name);
> >  
> >  	dm_report_field_set_value(field, "", NULL);
> >  	return 1;
> 
> OK. Your other patches use if ((name = ...)) style, maybe use it here as
> well? (Or change the others to use this style instead?)
> 

Done.

[PATCH 08/14] Refactor and add code for (lv) 'modules' get function.

[Dave Wysochanski]

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

diff --git a/lib/metadata/lv.c b/lib/metadata/lv.c
index 4fad293..329a3be 100644
--- a/lib/metadata/lv.c
+++ b/lib/metadata/lv.c
@@ -18,6 +18,21 @@
 #include "activate.h"
 #include "toolcontext.h"
 #include "segtype.h"
+#include "str_list.h"
+
+char *lv_modules_dup(struct dm_pool *mem, const struct logical_volume *lv)
+{
+	struct dm_list *modules;
+
+	if (!(modules = str_list_create(mem))) {
+		log_error("modules str_list allocation failed");
+		return NULL;
+	}
+
+	if (!list_lv_modules(mem, lv, modules))
+		return_NULL;
+	return tags_format_and_copy(lv->vg->vgmem, modules);
+}
 
 char *lv_mirror_log_dup(struct dm_pool *mem, const struct logical_volume *lv)
 {
diff --git a/lib/metadata/lv.h b/lib/metadata/lv.h
index 8d069af..2f0ebee 100644
--- a/lib/metadata/lv.h
+++ b/lib/metadata/lv.h
@@ -59,5 +59,6 @@ char *lv_convert_lv_dup(struct dm_pool *mem, const struct logical_volume *lv);
 int lv_kernel_major(const struct logical_volume *lv);
 int lv_kernel_minor(const struct logical_volume *lv);
 char *lv_mirror_log_dup(struct dm_pool *mem, const struct logical_volume *lv);
+char *lv_modules_dup(struct dm_pool *mem, const struct logical_volume *lv);
 
 #endif
diff --git a/lib/report/properties.c b/lib/report/properties.c
index eba2b59..ce1d2f7 100644
--- a/lib/report/properties.c
+++ b/lib/report/properties.c
@@ -137,7 +137,7 @@ GET_LV_STR_PROPERTY_FN(lv_tags, lv_tags_dup(lv))
 #define _lv_tags_set _not_implemented_set
 GET_LV_STR_PROPERTY_FN(mirror_log, lv_mirror_log_dup(lv->vg->vgmem, lv))
 #define _mirror_log_set _not_implemented_set
-#define _modules_get _not_implemented_get
+GET_LV_STR_PROPERTY_FN(modules, lv_modules_dup(lv->vg->vgmem, lv))
 #define _modules_set _not_implemented_set
 
 /* VG */
diff --git a/lib/report/report.c b/lib/report/report.c
index 65becaf..012659e 100644
--- a/lib/report/report.c
+++ b/lib/report/report.c
@@ -165,17 +165,13 @@ static int _modules_disp(struct dm_report *rh, struct dm_pool *mem,
 			 const void *data, void *private)
 {
 	const struct logical_volume *lv = (const struct logical_volume *) data;
-	struct dm_list *modules;
+	char *modules_str;
 
-	if (!(modules = str_list_create(mem))) {
-		log_error("modules str_list allocation failed");
+	if (!(modules_str = lv_modules_dup(mem, lv)))
 		return 0;
-	}
-
-	if (!list_lv_modules(mem, lv, modules))
-		return_0;
 
-	return _tags_disp(rh, mem, field, modules, private);
+	dm_report_field_set_value(field, modules_str, NULL);
+	return 1;
 }
 
 static int _vgfmt_disp(struct dm_report *rh, struct dm_pool *mem,
-- 
1.7.2.2

[PATCH 09/14] Refactor and add code for (lv) 'lv_name' get function.

[Dave Wysochanski]

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 lib/metadata/lv.c       |   27 +++++++++++++++++++++++++++
 lib/metadata/lv.h       |    1 +
 lib/report/properties.c |    2 +-
 lib/report/report.c     |   28 +++++-----------------------
 4 files changed, 34 insertions(+), 24 deletions(-)

diff --git a/lib/metadata/lv.c b/lib/metadata/lv.c
index 329a3be..63851f6 100644
--- a/lib/metadata/lv.c
+++ b/lib/metadata/lv.c
@@ -20,6 +20,33 @@
 #include "segtype.h"
 #include "str_list.h"
 
+char *lv_name_dup(struct dm_pool *mem, const struct logical_volume *lv)
+{
+	char *repstr, *lvname;
+	size_t len;
+
+	if (lv_is_visible(lv)) {
+		goto dup;
+	}
+
+	len = strlen(lv->name) + 3;
+	if (!(repstr = dm_pool_zalloc(mem, len))) {
+		log_error("dm_pool_alloc failed");
+		return NULL;
+	}
+
+	if (dm_snprintf(repstr, len, "[%s]", lv->name) < 0) {
+		log_error("lvname snprintf failed");
+		return NULL;
+	}
+dup:
+	if (!(lvname = dm_pool_strdup(mem, lv->name))) {
+		log_error("dm_pool_strdup failed");
+		return NULL;
+	}
+	return lvname;
+}
+
 char *lv_modules_dup(struct dm_pool *mem, const struct logical_volume *lv)
 {
 	struct dm_list *modules;
diff --git a/lib/metadata/lv.h b/lib/metadata/lv.h
index 2f0ebee..f80d2d5 100644
--- a/lib/metadata/lv.h
+++ b/lib/metadata/lv.h
@@ -60,5 +60,6 @@ int lv_kernel_major(const struct logical_volume *lv);
 int lv_kernel_minor(const struct logical_volume *lv);
 char *lv_mirror_log_dup(struct dm_pool *mem, const struct logical_volume *lv);
 char *lv_modules_dup(struct dm_pool *mem, const struct logical_volume *lv);
+char *lv_name_dup(struct dm_pool *mem, const struct logical_volume *lv);
 
 #endif
diff --git a/lib/report/properties.c b/lib/report/properties.c
index ce1d2f7..8228193 100644
--- a/lib/report/properties.c
+++ b/lib/report/properties.c
@@ -99,7 +99,7 @@ GET_PV_NUM_PROPERTY_FN(pv_mda_used_count, pv_mda_used_count(pv))
 /* LV */
 GET_LV_STR_PROPERTY_FN(lv_uuid, lv_uuid_dup(lv))
 #define _lv_uuid_set _not_implemented_set
-#define _lv_name_get _not_implemented_get
+GET_LV_STR_PROPERTY_FN(lv_name, lv_name_dup(lv->vg->vgmem, lv))
 #define _lv_name_set _not_implemented_set
 GET_LV_STR_PROPERTY_FN(lv_path, lv_path_dup(lv->vg->vgmem, lv))
 #define _lv_path_set _not_implemented_set
diff --git a/lib/report/report.c b/lib/report/report.c
index 012659e..ffca851 100644
--- a/lib/report/report.c
+++ b/lib/report/report.c
@@ -308,31 +308,13 @@ static int _lvname_disp(struct dm_report *rh, struct dm_pool *mem,
 			const void *data, void *private __attribute__((unused)))
 {
 	const struct logical_volume *lv = (const struct logical_volume *) data;
-	char *repstr, *lvname;
-	size_t len;
-
-	if (lv_is_visible(lv)) {
-		repstr = lv->name;
-		return dm_report_field_string(rh, field, (const char **) &repstr);
-	}
-
-	len = strlen(lv->name) + 3;
-	if (!(repstr = dm_pool_zalloc(mem, len))) {
-		log_error("dm_pool_alloc failed");
-		return 0;
-	}
-
-	if (dm_snprintf(repstr, len, "[%s]", lv->name) < 0) {
-		log_error("lvname snprintf failed");
-		return 0;
-	}
+	const char *name;
 
-	if (!(lvname = dm_pool_strdup(mem, lv->name))) {
-		log_error("dm_pool_strdup failed");
-		return 0;
-	}
+	name = lv_name_dup(mem, lv);
+	if (name)
+		return dm_report_field_string(rh, field, &name);
 
-	dm_report_field_set_value(field, repstr, lvname);
+	dm_report_field_set_value(field, "", NULL);
 
 	return 1;
 }
-- 
1.7.2.2

[PATCH 09/14] Refactor and add code for (lv) 'lv_name' get function.

[Petr Rockai]

Dave Wysochanski <dwysocha@redhat.com> writes:

> Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
Reviewed-By: Petr Rockai <prockai@redhat.com>

> --- a/lib/metadata/lv.c
> +++ b/lib/metadata/lv.c
> @@ -20,6 +20,33 @@
>  #include "segtype.h"
>  #include "str_list.h"
>  
> +char *lv_name_dup(struct dm_pool *mem, const struct logical_volume *lv)
> +{
> +	char *repstr, *lvname;
> +	size_t len;
> +
> +	if (lv_is_visible(lv)) {
> +		goto dup;
> +	}
> +
> +	len = strlen(lv->name) + 3;
> +	if (!(repstr = dm_pool_zalloc(mem, len))) {
> +		log_error("dm_pool_alloc failed");
> +		return NULL;
> +	}
> +
> +	if (dm_snprintf(repstr, len, "[%s]", lv->name) < 0) {
> +		log_error("lvname snprintf failed");
> +		return NULL;
> +	}
> +dup:
> +	if (!(lvname = dm_pool_strdup(mem, lv->name))) {
> +		log_error("dm_pool_strdup failed");
> +		return NULL;
> +	}
> +	return lvname;
> +}
Using (proposed) dm_pool_asprintf would simplify this a lot, too.

> --- a/lib/metadata/lv.h
> +++ b/lib/metadata/lv.h
> @@ -60,5 +60,6 @@ int lv_kernel_major(const struct logical_volume *lv);
>  int lv_kernel_minor(const struct logical_volume *lv);
>  char *lv_mirror_log_dup(struct dm_pool *mem, const struct logical_volume *lv);
>  char *lv_modules_dup(struct dm_pool *mem, const struct logical_volume *lv);
> +char *lv_name_dup(struct dm_pool *mem, const struct logical_volume *lv);
>  
>  #endif

> --- a/lib/report/properties.c
> +++ b/lib/report/properties.c
> @@ -99,7 +99,7 @@ GET_PV_NUM_PROPERTY_FN(pv_mda_used_count, pv_mda_used_count(pv))
>  /* LV */
>  GET_LV_STR_PROPERTY_FN(lv_uuid, lv_uuid_dup(lv))
>  #define _lv_uuid_set _not_implemented_set
> -#define _lv_name_get _not_implemented_get
> +GET_LV_STR_PROPERTY_FN(lv_name, lv_name_dup(lv->vg->vgmem, lv))
>  #define _lv_name_set _not_implemented_set
>  GET_LV_STR_PROPERTY_FN(lv_path, lv_path_dup(lv->vg->vgmem, lv))
>  #define _lv_path_set _not_implemented_set

> --- a/lib/report/report.c
> +++ b/lib/report/report.c
> @@ -308,31 +308,13 @@ static int _lvname_disp(struct dm_report *rh, struct dm_pool *mem,
>  			const void *data, void *private __attribute__((unused)))
>  {
>  	const struct logical_volume *lv = (const struct logical_volume *) data;
> -	char *repstr, *lvname;
> -	size_t len;
> -
> -	if (lv_is_visible(lv)) {
> -		repstr = lv->name;
> -		return dm_report_field_string(rh, field, (const char **) &repstr);
> -	}
> -
> -	len = strlen(lv->name) + 3;
> -	if (!(repstr = dm_pool_zalloc(mem, len))) {
> -		log_error("dm_pool_alloc failed");
> -		return 0;
> -	}
> -
> -	if (dm_snprintf(repstr, len, "[%s]", lv->name) < 0) {
> -		log_error("lvname snprintf failed");
> -		return 0;
> -	}
> +	const char *name;
>  
> -	if (!(lvname = dm_pool_strdup(mem, lv->name))) {
> -		log_error("dm_pool_strdup failed");
> -		return 0;
> -	}
> +	name = lv_name_dup(mem, lv);
> +	if (name)
> +		return dm_report_field_string(rh, field, &name);
if ((name = ...)) vs ... ; if (name) ... again

>  
> -	dm_report_field_set_value(field, repstr, lvname);
> +	dm_report_field_set_value(field, "", NULL);
>  
>  	return 1;
>  }

OK.

Yours,
   Petr.

[PATCH 09/14] Refactor and add code for (lv) 'lv_name' get function.

[Dave Wysochanski]

On Mon, 2010-10-11 at 20:36 +0200, Petr Rockai wrote:
> Dave Wysochanski <dwysocha@redhat.com> writes:
> 
> > Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
> Reviewed-By: Petr Rockai <prockai@redhat.com>
> 
> > --- a/lib/metadata/lv.c
> > +++ b/lib/metadata/lv.c
> > @@ -20,6 +20,33 @@
> >  #include "segtype.h"
> >  #include "str_list.h"
> >  
> > +char *lv_name_dup(struct dm_pool *mem, const struct logical_volume *lv)
> > +{
> > +	char *repstr, *lvname;
> > +	size_t len;
> > +
> > +	if (lv_is_visible(lv)) {
> > +		goto dup;
> > +	}
> > +
> > +	len = strlen(lv->name) + 3;
> > +	if (!(repstr = dm_pool_zalloc(mem, len))) {
> > +		log_error("dm_pool_alloc failed");
> > +		return NULL;
> > +	}
> > +
> > +	if (dm_snprintf(repstr, len, "[%s]", lv->name) < 0) {
> > +		log_error("lvname snprintf failed");
> > +		return NULL;
> > +	}
> > +dup:
> > +	if (!(lvname = dm_pool_strdup(mem, lv->name))) {
> > +		log_error("dm_pool_strdup failed");
> > +		return NULL;
> > +	}
> > +	return lvname;
> > +}
> Using (proposed) dm_pool_asprintf would simplify this a lot, too.
> 

Ok.  Actually this patch is wrong anyway.
The 'dup' function should dup the name.
The code that adds the '[' and ']' should be left inside
the 'disp' function.


> > --- a/lib/metadata/lv.h
> > +++ b/lib/metadata/lv.h
> >  
> > -	if (!(lvname = dm_pool_strdup(mem, lv->name))) {
> > -		log_error("dm_pool_strdup failed");
> > -		return 0;
> > -	}
> > +	name = lv_name_dup(mem, lv);
> > +	if (name)
> > +		return dm_report_field_string(rh, field, &name);
> if ((name = ...)) vs ... ; if (name) ... again
> 

Ok.

[PATCH 10/14] Refactor and add code for (lv) 'lv_origin' get function.

[Dave Wysochanski]

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

diff --git a/lib/metadata/lv.c b/lib/metadata/lv.c
index 63851f6..593d37a 100644
--- a/lib/metadata/lv.c
+++ b/lib/metadata/lv.c
@@ -20,6 +20,13 @@
 #include "segtype.h"
 #include "str_list.h"
 
+char *lv_origin_dup(struct dm_pool *mem, const struct logical_volume *lv)
+{
+	if (lv_is_cow(lv))
+		return lv_name_dup(mem, origin_from_cow(lv));
+	return NULL;
+}
+
 char *lv_name_dup(struct dm_pool *mem, const struct logical_volume *lv)
 {
 	char *repstr, *lvname;
diff --git a/lib/metadata/lv.h b/lib/metadata/lv.h
index f80d2d5..1767551 100644
--- a/lib/metadata/lv.h
+++ b/lib/metadata/lv.h
@@ -61,5 +61,6 @@ int lv_kernel_minor(const struct logical_volume *lv);
 char *lv_mirror_log_dup(struct dm_pool *mem, const struct logical_volume *lv);
 char *lv_modules_dup(struct dm_pool *mem, const struct logical_volume *lv);
 char *lv_name_dup(struct dm_pool *mem, const struct logical_volume *lv);
+char *lv_origin_dup(struct dm_pool *mem, const struct logical_volume *lv);
 
 #endif
diff --git a/lib/report/properties.c b/lib/report/properties.c
index 8228193..250c7c9 100644
--- a/lib/report/properties.c
+++ b/lib/report/properties.c
@@ -121,7 +121,7 @@ GET_LV_NUM_PROPERTY_FN(lv_size, lv->size * SECTOR_SIZE)
 #define _lv_size_set _not_implemented_set
 GET_LV_NUM_PROPERTY_FN(seg_count, dm_list_size(&lv->segments))
 #define _seg_count_set _not_implemented_set
-#define _origin_get _not_implemented_get
+GET_LV_STR_PROPERTY_FN(origin, lv_origin_dup(lv->vg->vgmem, lv))
 #define _origin_set _not_implemented_set
 GET_LV_NUM_PROPERTY_FN(origin_size, lv_origin_size(lv))
 #define _origin_size_set _not_implemented_set
diff --git a/lib/report/report.c b/lib/report/report.c
index ffca851..522abd4 100644
--- a/lib/report/report.c
+++ b/lib/report/report.c
@@ -339,9 +339,11 @@ static int _origin_disp(struct dm_report *rh, struct dm_pool *mem,
 			const void *data, void *private)
 {
 	const struct logical_volume *lv = (const struct logical_volume *) data;
+	const char *name;
 
-	if (lv_is_cow(lv))
-		return _lvname_disp(rh, mem, field, origin_from_cow(lv), private);
+	name = lv_origin_dup(mem, lv);
+	if (name)
+		return dm_report_field_string(rh, field, &name);
 
 	dm_report_field_set_value(field, "", NULL);
 	return 1;
-- 
1.7.2.2

[PATCH 11/14] 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..b5008c1 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(const 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 a09208a..848a497 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(const 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.2

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

[Zdenek Kabelac]

Dne 11.10.2010 17:14, Dave Wysochanski napsal(a):
> 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..b5008c1 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;

are we going to support return of modifiable strings - or const would fit here
?  (IMHO I still think, we are duplicating way too many things on return...)

> +		uint64_t n_val;
> +	} v;
> +} lvm_property_value_t;
> +


> +int lvm_vg_get_property(const vg_t vg, const char *name,
> +			struct lvm_property_value *value)
> +{
> +	struct lvm_property_type prop;
> +
> +	strncpy(prop.id, name, LVM_PROPERTY_NAME_LEN);

Hmmm why doing a copy here instead of passing/assigning  'name' ptr somewhere?


> +	if (!vg_get_property(vg, &prop))
> +		return -1;

As this is public interface - I'd add check for valid 'value' pointer.

if (value) return 0;   // might indicate, property exists and is queriable,
but user is not interested in the result.


> +	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);


Zdenek

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

[Petr Rockai]

Zdenek Kabelac <zkabelac@redhat.com> writes:

>> +/**
>> + * 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;
>
> are we going to support return of modifiable strings - or const would fit here
> ?  (IMHO I still think, we are duplicating way too many things on return...)

I don't think duplication is the issue. We are stuck in C, which means
that memory management has to be explicit (no RAII here). So going for
consistency is a good goal, since we can't really win and have a neat
API. We can still have one that comes with as few surprises as possible,
though.

With the approach Dave is using, we can guarantee lifetime of objects
related to a VG to be the same as that of the VG handle. The other
consistent option is to not use pools at all, and allocate everything on
heap. Then, the objects can be explicitly cleaned up by the caller. This
would be a lot better for memory scalability.

Never allocating anything at all can't work, since some of the strings
that we return are not persistently stored in the VG structures. There
is the option to take a buffer/length parameter, but that makes for very
tedious client code.

>
>> +		uint64_t n_val;
>> +	} v;
>> +} lvm_property_value_t;
>> +
>
>
>> +int lvm_vg_get_property(const vg_t vg, const char *name,
>> +			struct lvm_property_value *value)
>> +{
>> +	struct lvm_property_type prop;
>> +
>> +	strncpy(prop.id, name, LVM_PROPERTY_NAME_LEN);
>
> Hmmm why doing a copy here instead of passing/assigning  'name' ptr somewhere?
>
>
>> +	if (!vg_get_property(vg, &prop))
>> +		return -1;
>
> As this is public interface - I'd add check for valid 'value' pointer.
>
> if (value) return 0;   // might indicate, property exists and is queriable,
> but user is not interested in the result.
You mean if (!value)?

>> +	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;
>> +}

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

[Petr Rockai]

Zdenek Kabelac <zkabelac@redhat.com> writes:
>> +	struct lvm_property_type prop;
>> +
>> +	strncpy(prop.id, name, LVM_PROPERTY_NAME_LEN);
>
> Hmmm why doing a copy here instead of passing/assigning  'name' ptr somewhere?

Because lvm_property_type says: char id[LVM_PROPERTY_NAME_LEN];

Yours,
   Petr.

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

[Dave Wysochanski]

On Tue, 2010-10-12 at 13:05 +0200, Petr Rockai wrote:
> Zdenek Kabelac <zkabelac@redhat.com> writes:
> >> +	struct lvm_property_type prop;
> >> +
> >> +	strncpy(prop.id, name, LVM_PROPERTY_NAME_LEN);
> >
> > Hmmm why doing a copy here instead of passing/assigning  'name' ptr somewhere?
> 
> Because lvm_property_type says: char id[LVM_PROPERTY_NAME_LEN];
> 
> Yours,
>    Petr.
> 

Thanks both of you for the feedback.  I am reworking this based on all
your comments and will re-post soon.

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

[Petr Rockai]

Hi,

Dave Wysochanski <dwysocha@redhat.com> writes:
> --- 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;
^^ Can you make this a bitfield? Allocating 4 bytes for a single-bit
value is a bit wasteful.

What does "is_writeable" mean? That a corresponding lvm_vg_set_property
makes sense? Should go into the docstring.

> +	union {
> +		char *s_val;
> +		uint64_t n_val;
> +	} v;
> +} lvm_property_value_t;
(Sadly, no anonymous unions in standard C...) Would it make the client
code more readable to say

union {
      char *string;
      uint64_t integer;
} value;

though?

We'd then get foo.value.integer instead of foo.v.n_val; Contracting
value to val may be acceptable.

> +/**
> + * 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) {
^^ can we strip the vg_ prefix in the vg_mda_count? It would make a
slightly less repetitive API. Alternatively, accept both.

> + *              // 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(const vg_t vg, const char *name,
> +			struct lvm_property_value *value);
^^ As for the signature of this function, would it make sense to just
make this a composite return, instead of taking a value pointer? What is
the tradeoff?

lvm_property_value v = lvm_vg_get_property(vg, "foo");
if (v.valid) {
   // ... use stuff in v
}

lvm_property_value v;
if (lvm_vg_get_property(vg, "foo", &v)) {
  // ... use stuff in v
}

I probably slightly prefer the former, for situations like those, too:

lvm_property_value foo = lvm_vg_get_property(vg, "foo"),
                   bar = lvm_vg_get_property(vg, "bar");
if (foo.valid && bar.valid) {
   // ...
}

Moreover, this would make it possible to have an universal and
unconditional release function, if we ever decide to go for explicit
memory management (for scalability reasons):

void lvm_property_value_release(lvm_property_value v) {
    if (!v.valid)
        return;
    // ... free up resources
}

>  /************************** logical volume handling *************************/
>  
>  /**
> diff --git a/liblvm/lvm_vg.c b/liblvm/lvm_vg.c
> index a09208a..848a497 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(const 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);

Yours,
   Petr.

[PATCH 12/14] 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 b5008c1..b6cc729 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(const 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 0b6b6b1..2e3c3e9 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(const 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 (!pv_get_property(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.2

[PATCH 13/14] Add lvm_lv_get_property() generic function to obtain value of any lv property.

[Dave Wysochanski]

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 lib/report/properties.c |    6 ++++++
 lib/report/properties.h |    2 ++
 liblvm/lvm2app.h        |   26 ++++++++++++++++++++++++++
 liblvm/lvm_lv.c         |   18 ++++++++++++++++++
 4 files changed, 52 insertions(+), 0 deletions(-)

diff --git a/lib/report/properties.c b/lib/report/properties.c
index 250c7c9..56372da 100644
--- a/lib/report/properties.c
+++ b/lib/report/properties.c
@@ -267,6 +267,12 @@ static int _get_property(const void *obj, struct lvm_property_type *prop,
 	return 1;
 }
 
+int lv_get_property(const struct logical_volume *lv,
+		    struct lvm_property_type *prop)
+{
+	return _get_property(lv, prop, LVS);
+}
+
 int vg_get_property(const struct volume_group *vg,
 		    struct lvm_property_type *prop)
 {
diff --git a/lib/report/properties.h b/lib/report/properties.h
index 7398f2f..5956604 100644
--- a/lib/report/properties.h
+++ b/lib/report/properties.h
@@ -34,6 +34,8 @@ struct lvm_property_type {
 	int (*set) (void *obj, struct lvm_property_type *prop);
 };
 
+int lv_get_property(const struct logical_volume *lv,
+		    struct lvm_property_type *prop);
 int vg_get_property(const struct volume_group *vg,
 		    struct lvm_property_type *prop);
 int pv_get_property(const struct physical_volume *pv,
diff --git a/liblvm/lvm2app.h b/liblvm/lvm2app.h
index b6cc729..126700a 100644
--- a/liblvm/lvm2app.h
+++ b/liblvm/lvm2app.h
@@ -1012,6 +1012,32 @@ const char *lvm_lv_get_name(const lv_t lv);
 uint64_t lvm_lv_get_size(const lv_t lv);
 
 /**
+ * Get the value of a LV 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_lv_get_property(pv, "seg_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_lv_get_property(const lv_t lv, const char *name,
+			struct lvm_property_value *value);
+
+/**
  * Get the current activation state of a logical volume.
  *
  * \memberof lv_t
diff --git a/liblvm/lvm_lv.c b/liblvm/lvm_lv.c
index 7bdafe0..76e8691 100644
--- a/liblvm/lvm_lv.c
+++ b/liblvm/lvm_lv.c
@@ -21,6 +21,7 @@
 #include "locking.h"
 #include "activate.h"
 #include "lvm_misc.h"
+#include "properties.h"
 
 static int _lv_check_handle(const lv_t lv, const int vg_writeable)
 {
@@ -48,6 +49,23 @@ const char *lvm_lv_get_name(const lv_t lv)
 			       NAME_LEN+1);
 }
 
+int lvm_lv_get_property(const lv_t lv, const char *name,
+			struct lvm_property_value *value)
+{
+	struct lvm_property_type prop;
+
+	strncpy(prop.id, name, LVM_PROPERTY_NAME_LEN);
+	if (!lv_get_property(lv, &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;
+}
+
 uint64_t lvm_lv_is_active(const lv_t lv)
 {
 	struct lvinfo info;
-- 
1.7.2.2

[PATCH 14/14] Add interactive tests for lvm_{pv|vg|lv}_get_property().

[Dave Wysochanski]

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

diff --git a/test/api/test.c b/test/api/test.c
index f89840c..1461de5 100644
--- a/test/api/test.c
+++ b/test/api/test.c
@@ -91,6 +91,12 @@ 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("'lv_get_property vgname lvname property_name': "
+	       "Display the value of LV property\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': "
@@ -180,6 +186,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;
@@ -546,6 +569,93 @@ 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;
+	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_property(char **argv, int argc)
+{
+	lv_t lv;
+	struct lvm_property_value value;
+	int rc;
+
+	if (argc < 4) {
+		printf("Please enter vgname, lvname, field_id\n");
+		return;
+	}
+	if (!(lv = _lookup_lv_by_name(argv[2])))
+		return;
+	rc = lvm_lv_get_property(lv, argv[3], &value);
+	if (rc)
+		printf("Error ");
+	else
+		printf("Success ");
+	printf("obtaining value of property %s in LV %s",
+	       argv[3], argv[2]);
+	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 +906,12 @@ 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], "lv_get_property")) {
+			_lv_get_property(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.2