[PATCH 00/08] Add lvm lv properties for lvm2app, return struct.

[PATCH 00/08] Add lvm lv properties for lvm2app, return struct.

[Dave Wysochanski]

This is the modified patchset based on Petr's suggestion of changing
the return value from 'int' to struct lvm_property_value:
http://www.redhat.com/archives/lvm-devel/2010-October/msg00068.html

I agree it is a good approach and so am posting it for comparison
with the previous approach and final decision.

Patchset adds remaining lv properties for lvm2app, and adds
the lvm2app 'get' interfaces for vg, pv, and lv, and interactive
tests to exercise the interface.

Changes since last patchset:
1. Added readahead attributes (lv_read_ahead, lv_kernel_read_ahead)
2. Incorporated most all feedback comments, including renaming of the
fields, etc.  I did not change the 'get' function prototype to return
the property value rather than passing it in.
3. Updated comments describing new APIs in lvm2app.h
4. Changed return value from 'int' to lvm_property_value based on
Petr's suggestion.  Added a 'is_valid' flag, which must be checked
before using the value.
5. Simplified the common code in lvm2app into a local 'get_property'
function which allows for a single-line implementation of
lvm_{pv|vg|lv}_get_property

The only two lv properties not included now are still the float/percent
based properties, snap_percent and copy_percent.  These should be
refactored carefully and I did not want to delay these patches further.

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

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

[Dave Wysochanski]

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

diff --git a/lib/metadata/lv.c b/lib/metadata/lv.c
index f379c3e..bbaeb64 100644
--- a/lib/metadata/lv.c
+++ b/lib/metadata/lv.c
@@ -20,6 +20,11 @@
 #include "segtype.h"
 #include "str_list.h"
 
+char *lv_name_dup(struct dm_pool *mem, const struct logical_volume *lv)
+{
+	return dm_pool_strdup(mem, lv->name);
+}
+
 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
-- 
1.7.2.2

[PATCH 1/8] 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>

This can go straight in.

> diff --git a/lib/metadata/lv.c b/lib/metadata/lv.c
> index f379c3e..bbaeb64 100644
> --- a/lib/metadata/lv.c
> +++ b/lib/metadata/lv.c
> @@ -20,6 +20,11 @@
>  #include "segtype.h"
>  #include "str_list.h"
>  
> +char *lv_name_dup(struct dm_pool *mem, const struct logical_volume *lv)
> +{
> +	return dm_pool_strdup(mem, lv->name);
> +}
> +
>  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

[PATCH 2/8] 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     |    5 +++--
 4 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/lib/metadata/lv.c b/lib/metadata/lv.c
index bbaeb64..9cd5bf8 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)
 {
 	return dm_pool_strdup(mem, lv->name);
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 d155372..9d86b12 100644
--- a/lib/report/report.c
+++ b/lib/report/report.c
@@ -356,9 +356,10 @@ 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);
+	if ((name = lv_origin_dup(mem, lv)))
+		return dm_report_field_string(rh, field, &name);
 
 	dm_report_field_set_value(field, "", NULL);
 	return 1;
-- 
1.7.2.2

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

[Petr Rockai]

Dave Wysochanski <dwysocha@redhat.com> writes:

> Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>

> --- 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)
>  {
>  	return dm_pool_strdup(mem, lv->name);

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

> --- a/lib/report/report.c
> +++ b/lib/report/report.c
> @@ -356,9 +356,10 @@ 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);
> +	if ((name = lv_origin_dup(mem, lv)))
> +		return dm_report_field_string(rh, field, &name);
>  
>  	dm_report_field_set_value(field, "", NULL);
>  	return 1;

Now, this code is not equivalent for those cases where the origin is not
visible. I am not sure that can ever happen, though. I am also not sure
which behaviour is better. But it's something worth double-checking.

Other than this, looks OK.

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

Yours,
   Petr.

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

[Dave Wysochanski]

On Wed, 2010-10-20 at 23:41 +0200, Petr Rockai wrote:
> Dave Wysochanski <dwysocha@redhat.com> writes:
> 
> > Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
> 
> > --- 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)
> >  {
> >  	return dm_pool_strdup(mem, lv->name);
> 
> > --- 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
> 
> > --- a/lib/report/report.c
> > +++ b/lib/report/report.c
> > @@ -356,9 +356,10 @@ 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);
> > +	if ((name = lv_origin_dup(mem, lv)))
> > +		return dm_report_field_string(rh, field, &name);
> >  
> >  	dm_report_field_set_value(field, "", NULL);
> >  	return 1;
> 
> Now, this code is not equivalent for those cases where the origin is not
> visible. I am not sure that can ever happen, though. I am also not sure
> which behaviour is better. But it's something worth double-checking.
> 

This was another error in my patch - reporting commands need to retain
the same output, so origin_disp should not be changed IMO.

> Other than this, looks OK.
> 
> Reviewed-by: Petr Rockai <prockai@redhat.com>
> 
> Yours,
>    Petr.
> 
> --
> lvm-devel mailing list
> lvm-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/lvm-devel

[PATCH 3/8] Add lv_read_ahead and lv_kernel_read_ahead 'get' functions.

[Dave Wysochanski]

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

diff --git a/lib/metadata/lv.c b/lib/metadata/lv.c
index 9cd5bf8..b7a8700 100644
--- a/lib/metadata/lv.c
+++ b/lib/metadata/lv.c
@@ -20,6 +20,15 @@
 #include "segtype.h"
 #include "str_list.h"
 
+uint32_t lv_kernel_read_ahead(const struct logical_volume *lv)
+{
+	struct lvinfo info;
+
+	if (!lv_info(lv->vg->cmd, lv, 0, &info, 0, 1) || !info.exists)
+		return UINT32_MAX;
+	return info.read_ahead;
+}
+
 char *lv_origin_dup(struct dm_pool *mem, const struct logical_volume *lv)
 {
 	if (lv_is_cow(lv))
diff --git a/lib/metadata/lv.h b/lib/metadata/lv.h
index 1767551..d77c362 100644
--- a/lib/metadata/lv.h
+++ b/lib/metadata/lv.h
@@ -62,5 +62,6 @@ 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);
+uint32_t lv_kernel_read_ahead(const struct logical_volume *lv);
 
 #endif
diff --git a/lib/report/properties.c b/lib/report/properties.c
index 250c7c9..587f11d 100644
--- a/lib/report/properties.c
+++ b/lib/report/properties.c
@@ -109,13 +109,13 @@ GET_LV_NUM_PROPERTY_FN(lv_major, lv->major)
 #define _lv_major_set _not_implemented_set
 GET_LV_NUM_PROPERTY_FN(lv_minor, lv->minor)
 #define _lv_minor_set _not_implemented_set
-#define _lv_read_ahead_get _not_implemented_get
+GET_LV_NUM_PROPERTY_FN(lv_read_ahead, lv->read_ahead * SECTOR_SIZE)
 #define _lv_read_ahead_set _not_implemented_set
 GET_LV_NUM_PROPERTY_FN(lv_kernel_major, lv_kernel_major(lv))
 #define _lv_kernel_major_set _not_implemented_set
 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
+GET_LV_NUM_PROPERTY_FN(lv_kernel_read_ahead, lv_kernel_read_ahead(lv) * SECTOR_SIZE)
 #define _lv_kernel_read_ahead_set _not_implemented_set
 GET_LV_NUM_PROPERTY_FN(lv_size, lv->size * SECTOR_SIZE)
 #define _lv_size_set _not_implemented_set
diff --git a/lib/report/report.c b/lib/report/report.c
index 9d86b12..dbeef21 100644
--- a/lib/report/report.c
+++ b/lib/report/report.c
@@ -470,12 +470,12 @@ static int _lvkreadahead_disp(struct dm_report *rh, struct dm_pool *mem,
 			      void *private)
 {
 	const struct logical_volume *lv = (const struct logical_volume *) data;
-	struct lvinfo info;
+	uint32_t read_ahead;
 
-	if (!lv_info(lv->vg->cmd, lv, 0, &info, 0, 1) || !info.exists)
+	if ((read_ahead = lv_kernel_read_ahead(lv)) == UINT32_MAX)
 		return dm_report_field_int32(rh, field, &_minusone32);
 
-	return _size32_disp(rh, mem, field, &info.read_ahead, private);
+	return _size32_disp(rh, mem, field, &read_ahead, private);
 }
 
 static int _vgsize_disp(struct dm_report *rh, struct dm_pool *mem,
-- 
1.7.2.2

[PATCH 3/8] Add lv_read_ahead and lv_kernel_read_ahead '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>

Looks OK.

> diff --git a/lib/metadata/lv.c b/lib/metadata/lv.c
> index 9cd5bf8..b7a8700 100644
> --- a/lib/metadata/lv.c
> +++ b/lib/metadata/lv.c
> @@ -20,6 +20,15 @@
>  #include "segtype.h"
>  #include "str_list.h"
>  
> +uint32_t lv_kernel_read_ahead(const struct logical_volume *lv)
> +{
> +	struct lvinfo info;
> +
> +	if (!lv_info(lv->vg->cmd, lv, 0, &info, 0, 1) || !info.exists)
> +		return UINT32_MAX;
> +	return info.read_ahead;
> +}
> +
>  char *lv_origin_dup(struct dm_pool *mem, const struct logical_volume *lv)
>  {
>  	if (lv_is_cow(lv))
> diff --git a/lib/metadata/lv.h b/lib/metadata/lv.h
> index 1767551..d77c362 100644
> --- a/lib/metadata/lv.h
> +++ b/lib/metadata/lv.h
> @@ -62,5 +62,6 @@ 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);
> +uint32_t lv_kernel_read_ahead(const struct logical_volume *lv);
>  
>  #endif
> diff --git a/lib/report/properties.c b/lib/report/properties.c
> index 250c7c9..587f11d 100644
> --- a/lib/report/properties.c
> +++ b/lib/report/properties.c
> @@ -109,13 +109,13 @@ GET_LV_NUM_PROPERTY_FN(lv_major, lv->major)
>  #define _lv_major_set _not_implemented_set
>  GET_LV_NUM_PROPERTY_FN(lv_minor, lv->minor)
>  #define _lv_minor_set _not_implemented_set
> -#define _lv_read_ahead_get _not_implemented_get
> +GET_LV_NUM_PROPERTY_FN(lv_read_ahead, lv->read_ahead * SECTOR_SIZE)
>  #define _lv_read_ahead_set _not_implemented_set
>  GET_LV_NUM_PROPERTY_FN(lv_kernel_major, lv_kernel_major(lv))
>  #define _lv_kernel_major_set _not_implemented_set
>  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
> +GET_LV_NUM_PROPERTY_FN(lv_kernel_read_ahead, lv_kernel_read_ahead(lv) * SECTOR_SIZE)
>  #define _lv_kernel_read_ahead_set _not_implemented_set
>  GET_LV_NUM_PROPERTY_FN(lv_size, lv->size * SECTOR_SIZE)
>  #define _lv_size_set _not_implemented_set
> diff --git a/lib/report/report.c b/lib/report/report.c
> index 9d86b12..dbeef21 100644
> --- a/lib/report/report.c
> +++ b/lib/report/report.c
> @@ -470,12 +470,12 @@ static int _lvkreadahead_disp(struct dm_report *rh, struct dm_pool *mem,
>  			      void *private)
>  {
>  	const struct logical_volume *lv = (const struct logical_volume *) data;
> -	struct lvinfo info;
> +	uint32_t read_ahead;
>  
> -	if (!lv_info(lv->vg->cmd, lv, 0, &info, 0, 1) || !info.exists)
> +	if ((read_ahead = lv_kernel_read_ahead(lv)) == UINT32_MAX)
>  		return dm_report_field_int32(rh, field, &_minusone32);
>  
> -	return _size32_disp(rh, mem, field, &info.read_ahead, private);
> +	return _size32_disp(rh, mem, field, &read_ahead, private);
>  }
>  
>  static int _vgsize_disp(struct dm_report *rh, struct dm_pool *mem,

[PATCH 4/8] Rename fields in lvm_property_type.

[Dave Wysochanski]

Based on review comments, rename a few fields in lvm_property_type.
In particular, change 'is_writeable' to 'is_settable', which is
more intuitive to the intent of the bitfield (a 'set' function
exists for this field/property).  Also, remove the char array
for 'id' - unnecessary as we can just use the string passed in
to do the strcmp.  Finally rename the union members from n_val
to 'integer' and 's_val' to 'string'.

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

diff --git a/lib/report/properties.c b/lib/report/properties.c
index 587f11d..8f9272c 100644
--- a/lib/report/properties.c
+++ b/lib/report/properties.c
@@ -25,7 +25,7 @@ static int _ ## NAME ## _get (const void *obj, struct lvm_property_type *prop) \
 { \
 	const struct TYPE *VAR = (const struct TYPE *)obj; \
 \
-	prop->v.n_val = VALUE; \
+	prop->value.integer = VALUE; \
 	return 1; \
 }
 #define GET_VG_NUM_PROPERTY_FN(NAME, VALUE) \
@@ -40,7 +40,7 @@ static int _ ## NAME ## _get (const void *obj, struct lvm_property_type *prop) \
 { \
 	const struct TYPE *VAR = (const struct TYPE *)obj; \
 \
-	prop->v.s_val = (char *)VALUE;	\
+	prop->value.string = (char *)VALUE;	\
 	return 1; \
 }
 #define GET_VG_STR_PROPERTY_FN(NAME, VALUE) \
@@ -226,12 +226,12 @@ GET_VG_NUM_PROPERTY_FN(vg_mda_copies, (vg_mda_copies(vg)))
 
 #define STR DM_REPORT_FIELD_TYPE_STRING
 #define NUM DM_REPORT_FIELD_TYPE_NUMBER
-#define FIELD(type, strct, sorttype, head, field, width, fn, id, desc, writeable) \
-	{ type, #id, writeable, sorttype == STR, { .n_val = 0 }, _ ## id ## _get, _ ## id ## _set },
+#define FIELD(type, strct, sorttype, head, field, width, fn, id, desc, settable) \
+	{ type, #id, settable, sorttype == STR, { .integer = 0 }, _ ## id ## _get, _ ## id ## _set },
 
 struct lvm_property_type _properties[] = {
 #include "columns.h"
-	{ 0, "", 0, 0, { .n_val = 0 }, _not_implemented_get, _not_implemented_set },
+	{ 0, "", 0, 0, { .integer = 0 }, _not_implemented_get, _not_implemented_set },
 };
 
 #undef STR
diff --git a/lib/report/properties.h b/lib/report/properties.h
index 7398f2f..db4ae51 100644
--- a/lib/report/properties.h
+++ b/lib/report/properties.h
@@ -19,17 +19,15 @@
 #include "metadata.h"
 #include "report.h"
 
-#define LVM_PROPERTY_NAME_LEN DM_REPORT_FIELD_TYPE_ID_LEN
-
 struct lvm_property_type {
 	report_type_t type;
-	char id[LVM_PROPERTY_NAME_LEN];
-	unsigned is_writeable;
-	unsigned is_string;
+	const char *id;
+	unsigned is_settable:1;
+	unsigned is_string:1;
 	union {
-		char *s_val;
-		uint64_t n_val;
-	} v;
+		char *string;
+		uint64_t integer;
+	} value;
 	int (*get) (const void *obj, struct lvm_property_type *prop);
 	int (*set) (void *obj, struct lvm_property_type *prop);
 };
-- 
1.7.2.2

[PATCH 4/8] Rename fields in lvm_property_type.

[Petr Rockai]

Dave Wysochanski <dwysocha@redhat.com> writes:

> Based on review comments, rename a few fields in lvm_property_type.
> In particular, change 'is_writeable' to 'is_settable', which is
> more intuitive to the intent of the bitfield (a 'set' function
> exists for this field/property).  Also, remove the char array
> for 'id' - unnecessary as we can just use the string passed in
> to do the strcmp.  Finally rename the union members from n_val
> to 'integer' and 's_val' to 'string'.
>
> Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
Reviewed-by: Petr Rockai <prockai@redhat.com>

Looks good.

> diff --git a/lib/report/properties.c b/lib/report/properties.c
> index 587f11d..8f9272c 100644
> --- a/lib/report/properties.c
> +++ b/lib/report/properties.c
> @@ -25,7 +25,7 @@ static int _ ## NAME ## _get (const void *obj, struct lvm_property_type *prop) \
>  { \
>  	const struct TYPE *VAR = (const struct TYPE *)obj; \
>  \
> -	prop->v.n_val = VALUE; \
> +	prop->value.integer = VALUE; \
>  	return 1; \
>  }
>  #define GET_VG_NUM_PROPERTY_FN(NAME, VALUE) \
> @@ -40,7 +40,7 @@ static int _ ## NAME ## _get (const void *obj, struct lvm_property_type *prop) \
>  { \
>  	const struct TYPE *VAR = (const struct TYPE *)obj; \
>  \
> -	prop->v.s_val = (char *)VALUE;	\
> +	prop->value.string = (char *)VALUE;	\
>  	return 1; \
>  }
>  #define GET_VG_STR_PROPERTY_FN(NAME, VALUE) \
> @@ -226,12 +226,12 @@ GET_VG_NUM_PROPERTY_FN(vg_mda_copies, (vg_mda_copies(vg)))
>  
>  #define STR DM_REPORT_FIELD_TYPE_STRING
>  #define NUM DM_REPORT_FIELD_TYPE_NUMBER
> -#define FIELD(type, strct, sorttype, head, field, width, fn, id, desc, writeable) \
> -	{ type, #id, writeable, sorttype == STR, { .n_val = 0 }, _ ## id ## _get, _ ## id ## _set },
> +#define FIELD(type, strct, sorttype, head, field, width, fn, id, desc, settable) \
> +	{ type, #id, settable, sorttype == STR, { .integer = 0 }, _ ## id ## _get, _ ## id ## _set },
>  
>  struct lvm_property_type _properties[] = {
>  #include "columns.h"
> -	{ 0, "", 0, 0, { .n_val = 0 }, _not_implemented_get, _not_implemented_set },
> +	{ 0, "", 0, 0, { .integer = 0 }, _not_implemented_get, _not_implemented_set },
>  };
>  
>  #undef STR
> diff --git a/lib/report/properties.h b/lib/report/properties.h
> index 7398f2f..db4ae51 100644
> --- a/lib/report/properties.h
> +++ b/lib/report/properties.h
> @@ -19,17 +19,15 @@
>  #include "metadata.h"
>  #include "report.h"
>  
> -#define LVM_PROPERTY_NAME_LEN DM_REPORT_FIELD_TYPE_ID_LEN
> -
>  struct lvm_property_type {
>  	report_type_t type;
> -	char id[LVM_PROPERTY_NAME_LEN];
> -	unsigned is_writeable;
> -	unsigned is_string;
> +	const char *id;
> +	unsigned is_settable:1;
> +	unsigned is_string:1;
>  	union {
> -		char *s_val;
> -		uint64_t n_val;
> -	} v;
> +		char *string;
> +		uint64_t integer;
> +	} value;
>  	int (*get) (const void *obj, struct lvm_property_type *prop);
>  	int (*set) (void *obj, struct lvm_property_type *prop);
>  };

[PATCH 5/8] 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.
Rework lvm_vg_get_property to return lvm_property_value and require caller
to check 'is_valid' flag and lvm_errno() for API error.
Create a 'get_property' function, local to lvm2app, that factors out
most of the common code that copies the components of lvm_property_type
into lvm_property_value.  This allows for a 1-line function for each
of the generic property functions exported by lvm2app.

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 liblvm/lvm2app.h  |   62 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
 liblvm/lvm_misc.c |   34 +++++++++++++++++++++++++++++
 liblvm/lvm_misc.h |    2 +
 liblvm/lvm_vg.c   |    6 +++++
 4 files changed, 103 insertions(+), 1 deletions(-)

diff --git a/liblvm/lvm2app.h b/liblvm/lvm2app.h
index 47d3417..8b244fc 100644
--- a/liblvm/lvm2app.h
+++ b/liblvm/lvm2app.h
@@ -139,7 +139,7 @@ typedef struct physical_volume *pv_t;
 /**
  * Logical Volume object list.
  *
- * Lists of these structures are returned by lvm_vg_list_pvs().
+ * Lists of these structures are returned by lvm_vg_list_lvs().
  */
 typedef struct lvm_lv_list {
 	struct dm_list list;
@@ -168,6 +168,27 @@ 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().
+ *
+ * is_settable: indicates whether a 'set' function exists for this property
+ * is_string: indicates whether this property is a string (1) or integer (0)
+ * is_valid: indicates whether 'value' is valid (1) or not (0)
+ */
+typedef struct lvm_property_value {
+	unsigned is_settable:1;
+	unsigned is_string:1;
+	unsigned is_valid:1;
+	union {
+		const char *string;
+		uint64_t integer;
+	} value;
+} lvm_property_value_t;
+
 /*************************** generic lvm handling ***************************/
 /**
  * Create a LVM handle.
@@ -848,6 +869,45 @@ 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
+ *
+ * \param   vg
+ * VG handle obtained from lvm_vg_create() or lvm_vg_open().
+ *
+ * \param   name
+ * Name of property to query.  See vgs man page for full list of properties
+ * that may be queried.
+ *
+ * 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 v;
+ *      char *prop_name = "vg_mda_count";
+ *
+ *      v = lvm_vg_get_property(vg, prop_name);
+ *      if (lvm_errno(libh) || !v.is_valid) {
+ *           // handle error
+ *           printf("Invalid property name or unable to query"
+ *                  "'%s'.\n", prop_name);
+ *           return;
+ *      }
+ *      if (v.is_string)
+ *           printf(", value = %s\n", v.value.string);
+ *	else
+ *           printf(", value = %"PRIu64"\n", v.value.integer);
+ *
+ *
+ * \return
+ * lvm_property_value structure that will contain the current
+ * value of the property.  Caller should check lvm_errno() as well
+ * as 'is_valid' flag before using the value.
+ */
+struct lvm_property_value lvm_vg_get_property(const vg_t vg, const char *name);
+
 /************************** logical volume handling *************************/
 
 /**
diff --git a/liblvm/lvm_misc.c b/liblvm/lvm_misc.c
index e339faa..52f3636 100644
--- a/liblvm/lvm_misc.c
+++ b/liblvm/lvm_misc.c
@@ -15,6 +15,7 @@
 #include "lvm2app.h"
 #include "lvm_misc.h"
 #include "lib.h"
+#include "properties.h"
 
 struct dm_list *tag_list_copy(struct dm_pool *p, struct dm_list *tag_list)
 {
@@ -43,3 +44,36 @@ struct dm_list *tag_list_copy(struct dm_pool *p, struct dm_list *tag_list)
 	}
 	return list;
 }
+
+struct lvm_property_value get_property(const pv_t pv, const vg_t vg,
+				       const lv_t lv, const char *name)
+{
+	struct lvm_property_type prop;
+	struct lvm_property_value v;
+
+	prop.id = name;
+	if (pv) {
+		if (!pv_get_property(pv, &prop)) {
+			v.is_valid = 0;
+			return v;
+		}
+	} else if (vg) {
+		if (!vg_get_property(vg, &prop)) {
+			v.is_valid = 0;
+			return v;
+		}
+	} else if (lv) {
+		if (!lv_get_property(lv, &prop)) {
+			v.is_valid = 0;
+			return v;
+		}
+	}
+	v.is_settable = prop.is_settable;
+	v.is_string = prop.is_string;
+	if (v.is_string)
+		v.value.string = prop.value.string;
+	else
+		v.value.integer = prop.value.integer;
+	v.is_valid = 1;
+	return v;
+}
diff --git a/liblvm/lvm_misc.h b/liblvm/lvm_misc.h
index ced1f0f..b4e675e 100644
--- a/liblvm/lvm_misc.h
+++ b/liblvm/lvm_misc.h
@@ -17,5 +17,7 @@
 #include "libdevmapper.h"
 
 struct dm_list *tag_list_copy(struct dm_pool *p, struct dm_list *tag_list);
+struct lvm_property_value get_property(const pv_t pv, const vg_t vg,
+				       const lv_t lv, const char *name);
 
 #endif
diff --git a/liblvm/lvm_vg.c b/liblvm/lvm_vg.c
index a09208a..6970910 100644
--- a/liblvm/lvm_vg.c
+++ b/liblvm/lvm_vg.c
@@ -335,6 +335,12 @@ const char *lvm_vg_get_name(const vg_t vg)
 	return dm_pool_strndup(vg->vgmem, (const char *)vg->name, NAME_LEN+1);
 }
 
+
+struct lvm_property_value lvm_vg_get_property(const vg_t vg, const char *name)
+{
+	return get_property(NULL, vg, NULL, name);
+}
+
 struct dm_list *lvm_list_vg_names(lvm_t libh)
 {
 	return get_vgnames((struct cmd_context *)libh, 0);
-- 
1.7.2.2

[PATCH 5/8] Add lvm_vg_get_property() generic vg property function.

[Petr Rockai]

Dave Wysochanski <dwysocha@redhat.com> writes:

> Add a generic VG property function to lvm2app.  Call the internal library
> vg_get_property() function.  Strings are dup'd internally.
> Rework lvm_vg_get_property to return lvm_property_value and require caller
> to check 'is_valid' flag and lvm_errno() for API error.
> Create a 'get_property' function, local to lvm2app, that factors out
> most of the common code that copies the components of lvm_property_type
> into lvm_property_value.  This allows for a 1-line function for each
> of the generic property functions exported by lvm2app.
>
> Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
Reviewed-by: Petr Rockai <prockai@redhat.com>

I have some suggestions below, but I don't think any of them are
compulsory. I leave the final decision to your discretion. Other than
that, looks ready to go.

> --- a/liblvm/lvm2app.h
> +++ b/liblvm/lvm2app.h
> @@ -139,7 +139,7 @@ typedef struct physical_volume *pv_t;
>  /**
>   * Logical Volume object list.
>   *
> - * Lists of these structures are returned by lvm_vg_list_pvs().
> + * Lists of these structures are returned by lvm_vg_list_lvs().
>   */
>  typedef struct lvm_lv_list {
>  	struct dm_list list;
> @@ -168,6 +168,27 @@ 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().
> + *
> + * is_settable: indicates whether a 'set' function exists for this property
> + * is_string: indicates whether this property is a string (1) or integer (0)
> + * is_valid: indicates whether 'value' is valid (1) or not (0)
> + */
> +typedef struct lvm_property_value {
> +	unsigned is_settable:1;
> +	unsigned is_string:1;
> +	unsigned is_valid:1;
> +	union {
> +		const char *string;
> +		uint64_t integer;
> +	} value;
> +} lvm_property_value_t;

A is_integer might be useful here, for symmetry. In the client code, it
might not be obvious that !p.is_string means that it is an integer. It
would also improve API extensibility for the future, may we ever run
into a property that's neither string nor integer (although it won't
help with ABI).

> +
>  /*************************** generic lvm handling ***************************/
>  /**
>   * Create a LVM handle.
> @@ -848,6 +869,45 @@ 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
> + *
> + * \param   vg
> + * VG handle obtained from lvm_vg_create() or lvm_vg_open().
> + *
> + * \param   name
> + * Name of property to query.  See vgs man page for full list of properties
> + * that may be queried.
> + *
> + * 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 v;
> + *      char *prop_name = "vg_mda_count";
> + *
> + *      v = lvm_vg_get_property(vg, prop_name);
> + *      if (lvm_errno(libh) || !v.is_valid) {
> + *           // handle error
> + *           printf("Invalid property name or unable to query"
> + *                  "'%s'.\n", prop_name);
> + *           return;
> + *      }
> + *      if (v.is_string)
> + *           printf(", value = %s\n", v.value.string);
> + *	else
> + *           printf(", value = %"PRIu64"\n", v.value.integer);
> + *
> + *
> + * \return
> + * lvm_property_value structure that will contain the current
> + * value of the property.  Caller should check lvm_errno() as well
> + * as 'is_valid' flag before using the value.
> + */
> +struct lvm_property_value lvm_vg_get_property(const vg_t vg, const char *name);
> +
>  /************************** logical volume handling *************************/
>  
>  /**

> --- a/liblvm/lvm_misc.c
> +++ b/liblvm/lvm_misc.c
> @@ -15,6 +15,7 @@
>  #include "lvm2app.h"
>  #include "lvm_misc.h"
>  #include "lib.h"
> +#include "properties.h"
>  
>  struct dm_list *tag_list_copy(struct dm_pool *p, struct dm_list *tag_list)
>  {
> @@ -43,3 +44,36 @@ struct dm_list *tag_list_copy(struct dm_pool *p, struct dm_list *tag_list)
>  	}
>  	return list;
>  }
> +
> +struct lvm_property_value get_property(const pv_t pv, const vg_t vg,
> +				       const lv_t lv, const char *name)
> +{
> +	struct lvm_property_type prop;
> +	struct lvm_property_value v;
> +
> +	prop.id = name;
> +	if (pv) {
> +		if (!pv_get_property(pv, &prop)) {
> +			v.is_valid = 0;
> +			return v;
> +		}
> +	} else if (vg) {
> +		if (!vg_get_property(vg, &prop)) {
> +			v.is_valid = 0;
> +			return v;
> +		}
> +	} else if (lv) {
> +		if (!lv_get_property(lv, &prop)) {
> +			v.is_valid = 0;
> +			return v;
> +		}
> +	}
> +	v.is_settable = prop.is_settable;
> +	v.is_string = prop.is_string;
> +	if (v.is_string)
> +		v.value.string = prop.value.string;
> +	else
> +		v.value.integer = prop.value.integer;
> +	v.is_valid = 1;

Would it be more convenient to say v.is_valid = !lvm_errno(...) here?
That way, the client code would not need to check lvm_errno unless it
cared what went wrong. (The documentation above would need to be
updated, too.)

> +	return v;
> +}

> --- a/liblvm/lvm_misc.h
> +++ b/liblvm/lvm_misc.h
> @@ -17,5 +17,7 @@
>  #include "libdevmapper.h"
>  
>  struct dm_list *tag_list_copy(struct dm_pool *p, struct dm_list *tag_list);
> +struct lvm_property_value get_property(const pv_t pv, const vg_t vg,
> +				       const lv_t lv, const char *name);
>  
>  #endif
> diff --git a/liblvm/lvm_vg.c b/liblvm/lvm_vg.c
> index a09208a..6970910 100644
> --- a/liblvm/lvm_vg.c
> +++ b/liblvm/lvm_vg.c
> @@ -335,6 +335,12 @@ const char *lvm_vg_get_name(const vg_t vg)
>  	return dm_pool_strndup(vg->vgmem, (const char *)vg->name, NAME_LEN+1);
>  }
>  
> +
> +struct lvm_property_value lvm_vg_get_property(const vg_t vg, const char *name)
> +{
> +	return get_property(NULL, vg, NULL, name);
> +}
> +
>  struct dm_list *lvm_list_vg_names(lvm_t libh)
>  {
>  	return get_vgnames((struct cmd_context *)libh, 0);

Yours,
   Petr.

[PATCH 5/8] Add lvm_vg_get_property() generic vg property function.

[Dave Wysochanski]

On Wed, 2010-10-20 at 23:51 +0200, Petr Rockai wrote:
> Dave Wysochanski <dwysocha@redhat.com> writes:
> 
> > + * 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().
> > + *
> > + * is_settable: indicates whether a 'set' function exists for this property
> > + * is_string: indicates whether this property is a string (1) or integer (0)
> > + * is_valid: indicates whether 'value' is valid (1) or not (0)
> > + */
> > +typedef struct lvm_property_value {
> > +	unsigned is_settable:1;
> > +	unsigned is_string:1;
> > +	unsigned is_valid:1;
> > +	union {
> > +		const char *string;
> > +		uint64_t integer;
> > +	} value;
> > +} lvm_property_value_t;
> 
> A is_integer might be useful here, for symmetry. In the client code, it
> might not be obvious that !p.is_string means that it is an integer. It
> would also improve API extensibility for the future, may we ever run
> into a property that's neither string nor integer (although it won't
> help with ABI).
> 

Ok, I've implemented this - easy.  But...
Now that I think about it, rather than a bitfield and 'is_string' and
'is_integer", should we have a 'type' field and leave this an unsigned
(for example, #define LVM_PROPERTY_TYPE_NUM 1 and #define
LVM_PROPERTY_TYPE STR 2) or at least should we add padding to this
struct?  The struct might warrant a bit more thought, given how much
we've changed it recently, and given the ABI implications.


> > +		}
> > +	} else if (lv) {
> > +		if (!lv_get_property(lv, &prop)) {
> > +			v.is_valid = 0;
> > +			return v;
> > +		}
> > +	}
> > +	v.is_settable = prop.is_settable;
> > +	v.is_string = prop.is_string;
> > +	if (v.is_string)
> > +		v.value.string = prop.value.string;
> > +	else
> > +		v.value.integer = prop.value.integer;
> > +	v.is_valid = 1;
> 
> Would it be more convenient to say v.is_valid = !lvm_errno(...) here?
> That way, the client code would not need to check lvm_errno unless it
> cared what went wrong. (The documentation above would need to be
> updated, too.)
> 

As the code is, the client does not need to check lvm_errno() unless it
cares what went wrong.  In essence, v.is_valid "should =" !lvm_errno()
today, though there's no explicit assignment.  However, if we explicitly
assign v.is_valid = !lvm_errno(), then this assumes all error paths will
properly set lvm_errno().  Otherwise, we may end up telling the user the
value is valid when it may not be.  Now this case is clearly a bug but
keeping them separate avoids the potential bug.  It's fairly clear right
now that all error paths do properly set lvm_errno(), so this assignment
would be safe today.

I think I'm going to leave this one as is, though perhaps clarify the
comments.  One of the near-term TODOs for lvm2app should probably be
take a look a the error paths anyway, and make any adjustments and/or
cleanups.  Maybe you can tackle this specific item on that TODO, and
once further testing is in place.

[PATCH 5/8] Add lvm_vg_get_property() generic vg property function.

[Petr Rockai]

Hi,

Dave Wysochanski <dwysocha@redhat.com> writes:
>> A is_integer might be useful here, for symmetry. In the client code, it
>> might not be obvious that !p.is_string means that it is an integer. It
>> would also improve API extensibility for the future, may we ever run
>> into a property that's neither string nor integer (although it won't
>> help with ABI).
>> 
>
> Ok, I've implemented this - easy.  But...
> Now that I think about it, rather than a bitfield and 'is_string' and
> 'is_integer", should we have a 'type' field and leave this an unsigned
> (for example, #define LVM_PROPERTY_TYPE_NUM 1 and #define
> LVM_PROPERTY_TYPE STR 2) or at least should we add padding to this
> struct?  The struct might warrant a bit more thought, given how much
> we've changed it recently, and given the ABI implications.

The advantage of the bitfield is that it is much more convenient to use
(and the resulting code more concise and clearer). It also does not
impose any ABI problems as long as you keep the items in the right
order. What can be done to ensure forward compatibility is this:

uint32_t foo:1;
uint32_t bar:1;
uint32_t _padding:30;

And when you need a new flag, just reduce the padding by one and add
your new flag between the existing flags and the padding.

> As the code is, the client does not need to check lvm_errno() unless it
> cares what went wrong.  In essence, v.is_valid "should =" !lvm_errno()
> today, though there's no explicit assignment.  However, if we explicitly
> assign v.is_valid = !lvm_errno(), then this assumes all error paths will
> properly set lvm_errno().  Otherwise, we may end up telling the user the
> value is valid when it may not be.  Now this case is clearly a bug but
> keeping them separate avoids the potential bug.  It's fairly clear right
> now that all error paths do properly set lvm_errno(), so this assignment
> would be safe today.
Ok.

Yours,
   Petr.

[PATCH 5/8] Add lvm_vg_get_property() generic vg property function.

[Dave Wysochanski]

On Sun, 2010-10-24 at 14:04 +0200, Petr Rockai wrote:
> Hi,
> 
> Dave Wysochanski <dwysocha@redhat.com> writes:
> >> A is_integer might be useful here, for symmetry. In the client code, it
> >> might not be obvious that !p.is_string means that it is an integer. It
> >> would also improve API extensibility for the future, may we ever run
> >> into a property that's neither string nor integer (although it won't
> >> help with ABI).
> >> 
> >
> > Ok, I've implemented this - easy.  But...
> > Now that I think about it, rather than a bitfield and 'is_string' and
> > 'is_integer", should we have a 'type' field and leave this an unsigned
> > (for example, #define LVM_PROPERTY_TYPE_NUM 1 and #define
> > LVM_PROPERTY_TYPE STR 2) or at least should we add padding to this
> > struct?  The struct might warrant a bit more thought, given how much
> > we've changed it recently, and given the ABI implications.
> 
> The advantage of the bitfield is that it is much more convenient to use
> (and the resulting code more concise and clearer). It also does not
> impose any ABI problems as long as you keep the items in the right
> order. What can be done to ensure forward compatibility is this:
> 
> uint32_t foo:1;
> uint32_t bar:1;
> uint32_t _padding:30;
> 
> And when you need a new flag, just reduce the padding by one and add
> your new flag between the existing flags and the padding.
> 

Exactly.  Will add this now.


> > As the code is, the client does not need to check lvm_errno() unless it
> > cares what went wrong.  In essence, v.is_valid "should =" !lvm_errno()
> > today, though there's no explicit assignment.  However, if we explicitly
> > assign v.is_valid = !lvm_errno(), then this assumes all error paths will
> > properly set lvm_errno().  Otherwise, we may end up telling the user the
> > value is valid when it may not be.  Now this case is clearly a bug but
> > keeping them separate avoids the potential bug.  It's fairly clear right
> > now that all error paths do properly set lvm_errno(), so this assignment
> > would be safe today.
> Ok.
> 
> Yours,
>    Petr.
> 
> --
> lvm-devel mailing list
> lvm-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/lvm-devel

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

[Dave Wysochanski]

Add a generic PV property function to lvm2app, similar to VG function.
Return lvm_property_value and require caller to check 'is_valid' flag
and lvm_errno() for API error.

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

diff --git a/liblvm/lvm2app.h b/liblvm/lvm2app.h
index 8b244fc..ed22ed9 100644
--- a/liblvm/lvm2app.h
+++ b/liblvm/lvm2app.h
@@ -1240,6 +1240,44 @@ 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
+ *
+ * \param   pv
+ * Physical volume handle.
+ *
+ * \param   name
+ * Name of property to query.  See pvs man page for full list of properties
+ * that may be queried.
+ *
+ * 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;
+ *      char *prop_name = "pv_mda_count";
+ *
+ *      v = lvm_pv_get_property(pv, prop_name);
+ *      if (lvm_errno(libh) || !v.is_valid) {
+ *           // handle error
+ *           printf("Invalid property name or unable to query"
+ *                  "'%s'.\n", prop_name);
+ *           return;
+ *      }
+ *      if (v.is_string)
+ *           printf(", value = %s\n", v.value.string);
+ *	else
+ *           printf(", value = %"PRIu64"\n", v.value.integer);
+ *
+ * \return
+ * lvm_property_value structure that will contain the current
+ * value of the property.  Caller should check lvm_errno() as well
+ * as 'is_valid' flag before using the value.
+ */
+struct lvm_property_value lvm_pv_get_property(const pv_t pv, const char *name);
+
+/**
  * Resize physical volume to new_size bytes.
  *
  * \memberof pv_t
diff --git a/liblvm/lvm_pv.c b/liblvm/lvm_pv.c
index 0b6b6b1..efdccb5 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 "lvm_misc.h"
 
 const char *lvm_pv_get_uuid(const pv_t pv)
 {
@@ -48,6 +49,11 @@ uint64_t lvm_pv_get_free(const pv_t pv)
 	return (uint64_t) SECTOR_SIZE * pv_free(pv);
 }
 
+struct lvm_property_value lvm_pv_get_property(const pv_t pv, const char *name)
+{
+	return get_property(pv, NULL, NULL, name);
+}
+
 int lvm_pv_resize(const pv_t pv, uint64_t new_size)
 {
 	/* FIXME: add pv resize code here */
-- 
1.7.2.2

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

[Petr Rockai]

Dave Wysochanski <dwysocha@redhat.com> writes:

> Add a generic PV property function to lvm2app, similar to VG function.
> Return lvm_property_value and require caller to check 'is_valid' flag
> and lvm_errno() for API error.

If you go with my suggestion from previous patch about "including"
lvm_errno in is_valid, the docs in this patch will need to be changed
accordingly.

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

Other than that, good to go.

> diff --git a/liblvm/lvm2app.h b/liblvm/lvm2app.h
> index 8b244fc..ed22ed9 100644
> --- a/liblvm/lvm2app.h
> +++ b/liblvm/lvm2app.h
> @@ -1240,6 +1240,44 @@ 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
> + *
> + * \param   pv
> + * Physical volume handle.
> + *
> + * \param   name
> + * Name of property to query.  See pvs man page for full list of properties
> + * that may be queried.
> + *
> + * 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;
> + *      char *prop_name = "pv_mda_count";
> + *
> + *      v = lvm_pv_get_property(pv, prop_name);
> + *      if (lvm_errno(libh) || !v.is_valid) {
> + *           // handle error
> + *           printf("Invalid property name or unable to query"
> + *                  "'%s'.\n", prop_name);
> + *           return;
> + *      }
> + *      if (v.is_string)
> + *           printf(", value = %s\n", v.value.string);
> + *	else
> + *           printf(", value = %"PRIu64"\n", v.value.integer);
> + *
> + * \return
> + * lvm_property_value structure that will contain the current
> + * value of the property.  Caller should check lvm_errno() as well
> + * as 'is_valid' flag before using the value.
> + */
> +struct lvm_property_value lvm_pv_get_property(const pv_t pv, const char *name);
> +
> +/**
>   * Resize physical volume to new_size bytes.
>   *
>   * \memberof pv_t
> diff --git a/liblvm/lvm_pv.c b/liblvm/lvm_pv.c
> index 0b6b6b1..efdccb5 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 "lvm_misc.h"
>  
>  const char *lvm_pv_get_uuid(const pv_t pv)
>  {
> @@ -48,6 +49,11 @@ uint64_t lvm_pv_get_free(const pv_t pv)
>  	return (uint64_t) SECTOR_SIZE * pv_free(pv);
>  }
>  
> +struct lvm_property_value lvm_pv_get_property(const pv_t pv, const char *name)
> +{
> +	return get_property(pv, NULL, NULL, name);
> +}
> +
>  int lvm_pv_resize(const pv_t pv, uint64_t new_size)
>  {
>  	/* FIXME: add pv resize code here */

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

[Dave Wysochanski]

Add a generic LV property function to lvm2app, similar to VG function.
Return lvm_property_value and require caller to check 'is_valid' flag
and lvm_errno() for API error.

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

diff --git a/lib/report/properties.c b/lib/report/properties.c
index 8f9272c..c08ddfd 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 db4ae51..18e5ab9 100644
--- a/lib/report/properties.h
+++ b/lib/report/properties.h
@@ -32,6 +32,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 ed22ed9..e90adf8 100644
--- a/liblvm/lvm2app.h
+++ b/liblvm/lvm2app.h
@@ -1030,6 +1030,44 @@ 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 lv_t
+ *
+ * \param   lv
+ * Logical volume handle.
+ *
+ * \param   name
+ * Name of property to query.  See lvs man page for full list of properties
+ * that may be queried.
+ *
+ * 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 v;
+ *      char *prop_name = "seg_count";
+ *
+ *      v = lvm_lv_get_property(lv, prop_name);
+ *      if (lvm_errno(libh) || !v.is_valid) {
+ *           // handle error
+ *           printf("Invalid property name or unable to query"
+ *                  "'%s'.\n", prop_name);
+ *           return;
+ *      }
+ *      if (v.is_string)
+ *           printf(", value = %s\n", v.value.string);
+ *	else
+ *           printf(", value = %"PRIu64"\n", v.value.integer);
+ *
+ * \return
+ * lvm_property_value structure that will contain the current
+ * value of the property.  Caller should check lvm_errno() as well
+ * as 'is_valid' flag before using the value.
+ */
+struct lvm_property_value lvm_lv_get_property(const lv_t lv, const char *name);
+
+/**
  * 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..cef87f5 100644
--- a/liblvm/lvm_lv.c
+++ b/liblvm/lvm_lv.c
@@ -48,6 +48,11 @@ const char *lvm_lv_get_name(const lv_t lv)
 			       NAME_LEN+1);
 }
 
+struct lvm_property_value lvm_lv_get_property(const lv_t lv, const char *name)
+{
+	return get_property(NULL, NULL, lv, name);
+}
+
 uint64_t lvm_lv_is_active(const lv_t lv)
 {
 	struct lvinfo info;
-- 
1.7.2.2

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

[Petr Rockai]

Dave Wysochanski <dwysocha@redhat.com> writes:

> Add a generic LV property function to lvm2app, similar to VG function.
> Return lvm_property_value and require caller to check 'is_valid' flag
> and lvm_errno() for API error.
Same as the previous one.

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

> diff --git a/lib/report/properties.c b/lib/report/properties.c
> index 8f9272c..c08ddfd 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 db4ae51..18e5ab9 100644
> --- a/lib/report/properties.h
> +++ b/lib/report/properties.h
> @@ -32,6 +32,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 ed22ed9..e90adf8 100644
> --- a/liblvm/lvm2app.h
> +++ b/liblvm/lvm2app.h
> @@ -1030,6 +1030,44 @@ 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 lv_t
> + *
> + * \param   lv
> + * Logical volume handle.
> + *
> + * \param   name
> + * Name of property to query.  See lvs man page for full list of properties
> + * that may be queried.
> + *
> + * 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 v;
> + *      char *prop_name = "seg_count";
> + *
> + *      v = lvm_lv_get_property(lv, prop_name);
> + *      if (lvm_errno(libh) || !v.is_valid) {
> + *           // handle error
> + *           printf("Invalid property name or unable to query"
> + *                  "'%s'.\n", prop_name);
> + *           return;
> + *      }
> + *      if (v.is_string)
> + *           printf(", value = %s\n", v.value.string);
> + *	else
> + *           printf(", value = %"PRIu64"\n", v.value.integer);
> + *
> + * \return
> + * lvm_property_value structure that will contain the current
> + * value of the property.  Caller should check lvm_errno() as well
> + * as 'is_valid' flag before using the value.
> + */
> +struct lvm_property_value lvm_lv_get_property(const lv_t lv, const char *name);
> +
> +/**
>   * 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..cef87f5 100644
> --- a/liblvm/lvm_lv.c
> +++ b/liblvm/lvm_lv.c
> @@ -48,6 +48,11 @@ const char *lvm_lv_get_name(const lv_t lv)
>  			       NAME_LEN+1);
>  }
>  
> +struct lvm_property_value lvm_lv_get_property(const lv_t lv, const char *name)
> +{
> +	return get_property(NULL, NULL, lv, name);
> +}
> +
>  uint64_t lvm_lv_is_active(const lv_t lv)
>  {
>  	struct lvinfo info;

Yours,
   Petr.

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

[Dave Wysochanski]

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

diff --git a/test/api/test.c b/test/api/test.c
index f89840c..fea3d0d 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,89 @@ static void _vg_tag(char **argv, int argc, int add)
 	       add ? "adding":"removing", argv[2], argv[1]);
 }
 
+static void _print_property_value(struct lvm_property_value value)
+{
+	if (value.is_string)
+		printf(", value = %s\n", value.value.string);
+	else
+		printf(", value = %"PRIu64"\n", value.value.integer);
+}
+
+static void _pv_get_property(char **argv, int argc)
+{
+	pv_t pv;
+	struct lvm_property_value v;
+
+	if (argc < 3) {
+		printf("Please enter pvname, field_id\n");
+		return;
+	}
+	if (!(pv = _lookup_pv_by_name(argv, argc)))
+		return;
+	v = lvm_pv_get_property(pv, argv[2]);
+	if (!v.is_valid)
+		printf("Error ");
+	else
+		printf("Success ");
+	printf("obtaining value of property %s in PV %s",
+	       argv[2], argv[1]);
+	if (!v.is_valid) {
+		printf("\n");
+		return;
+	}
+	_print_property_value(v);
+}
+
+static void _vg_get_property(char **argv, int argc)
+{
+	vg_t vg;
+	struct lvm_property_value v;
+
+	if (argc < 3) {
+		printf("Please enter vgname, field_id\n");
+		return;
+	}
+	if (!(vg = _lookup_vg_by_name(argv, argc)))
+		return;
+	v =  lvm_vg_get_property(vg, argv[2]);
+	if (!v.is_valid)
+		printf("Error ");
+	else
+		printf("Success ");
+	printf("obtaining value of property %s in VG %s",
+	       argv[2], argv[1]);
+	if (!v.is_valid) {
+		printf("\n");
+		return;
+	}
+	_print_property_value(v);
+}
+
+static void _lv_get_property(char **argv, int argc)
+{
+	lv_t lv;
+	struct lvm_property_value v;
+
+	if (argc < 4) {
+		printf("Please enter vgname, lvname, field_id\n");
+		return;
+	}
+	if (!(lv = _lookup_lv_by_name(argv[2])))
+		return;
+	v = lvm_lv_get_property(lv, argv[3]);
+	if (!v.is_valid)
+		printf("Error ");
+	else
+		printf("Success ");
+	printf("obtaining value of property %s in LV %s",
+	       argv[3], argv[2]);
+	if (!v.is_valid) {
+		printf("\n");
+		return;
+	}
+	_print_property_value(v);
+}
+
 static void _lv_get_tags(char **argv, int argc)
 {
 	lv_t lv;
@@ -796,6 +902,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

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

[Petr Rockai]

Dave Wysochanski <dwysocha@redhat.com> writes:

> Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>

I take your word for this one (looks OK after a quick scan).

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

> diff --git a/test/api/test.c b/test/api/test.c
> index f89840c..fea3d0d 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,89 @@ static void _vg_tag(char **argv, int argc, int add)
>  	       add ? "adding":"removing", argv[2], argv[1]);
>  }
>  
> +static void _print_property_value(struct lvm_property_value value)
> +{
> +	if (value.is_string)
> +		printf(", value = %s\n", value.value.string);
> +	else
> +		printf(", value = %"PRIu64"\n", value.value.integer);
> +}
> +
> +static void _pv_get_property(char **argv, int argc)
> +{
> +	pv_t pv;
> +	struct lvm_property_value v;
> +
> +	if (argc < 3) {
> +		printf("Please enter pvname, field_id\n");
> +		return;
> +	}
> +	if (!(pv = _lookup_pv_by_name(argv, argc)))
> +		return;
> +	v = lvm_pv_get_property(pv, argv[2]);
> +	if (!v.is_valid)
> +		printf("Error ");
> +	else
> +		printf("Success ");
> +	printf("obtaining value of property %s in PV %s",
> +	       argv[2], argv[1]);
> +	if (!v.is_valid) {
> +		printf("\n");
> +		return;
> +	}
> +	_print_property_value(v);
> +}
> +
> +static void _vg_get_property(char **argv, int argc)
> +{
> +	vg_t vg;
> +	struct lvm_property_value v;
> +
> +	if (argc < 3) {
> +		printf("Please enter vgname, field_id\n");
> +		return;
> +	}
> +	if (!(vg = _lookup_vg_by_name(argv, argc)))
> +		return;
> +	v =  lvm_vg_get_property(vg, argv[2]);
> +	if (!v.is_valid)
> +		printf("Error ");
> +	else
> +		printf("Success ");
> +	printf("obtaining value of property %s in VG %s",
> +	       argv[2], argv[1]);
> +	if (!v.is_valid) {
> +		printf("\n");
> +		return;
> +	}
> +	_print_property_value(v);
> +}
> +
> +static void _lv_get_property(char **argv, int argc)
> +{
> +	lv_t lv;
> +	struct lvm_property_value v;
> +
> +	if (argc < 4) {
> +		printf("Please enter vgname, lvname, field_id\n");
> +		return;
> +	}
> +	if (!(lv = _lookup_lv_by_name(argv[2])))
> +		return;
> +	v = lvm_lv_get_property(lv, argv[3]);
> +	if (!v.is_valid)
> +		printf("Error ");
> +	else
> +		printf("Success ");
> +	printf("obtaining value of property %s in LV %s",
> +	       argv[3], argv[2]);
> +	if (!v.is_valid) {
> +		printf("\n");
> +		return;
> +	}
> +	_print_property_value(v);
> +}
> +
>  static void _lv_get_tags(char **argv, int argc)
>  {
>  	lv_t lv;
> @@ -796,6 +902,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")) {

[PATCH 00/08] Add lvm lv properties for lvm2app.

[Dave Wysochanski]

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

Changes since last patchset:
1. Added readahead attributes (lv_read_ahead, lv_kernel_read_ahead)
2. Incorporated most all Petr and Kabi's feedback comments, including
renaming of the fields, etc.  I did not change the 'get' function
prototype to return the property value rather than passing it in.
I will do this as a second patchset.
3. Updated comments describing new APIs in lvm2app.h

The only two lv properties not included now are still the float/percent
based properties, snap_percent and copy_percent.  These should be
refactored carefully and I did not want to delay these patches further.

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

[PATCH 5/8] 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 |   66 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
 liblvm/lvm_vg.c  |   21 +++++++++++++++++
 2 files changed, 86 insertions(+), 1 deletions(-)

diff --git a/liblvm/lvm2app.h b/liblvm/lvm2app.h
index 47d3417..32a138b 100644
--- a/liblvm/lvm2app.h
+++ b/liblvm/lvm2app.h
@@ -139,7 +139,7 @@ typedef struct physical_volume *pv_t;
 /**
  * Logical Volume object list.
  *
- * Lists of these structures are returned by lvm_vg_list_pvs().
+ * Lists of these structures are returned by lvm_vg_list_lvs().
  */
 typedef struct lvm_lv_list {
 	struct dm_list list;
@@ -168,6 +168,25 @@ 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().
+ *
+ * is_settable: indicates whether a 'set' function exists for this property
+ * is_string: indicates whether this property is a string (1) or integer (0)
+ */
+typedef struct lvm_property_value {
+	unsigned is_settable:1;
+	unsigned is_string:1;
+	union {
+		const char *string;
+		uint64_t integer;
+	} value;
+} lvm_property_value_t;
+
 /*************************** generic lvm handling ***************************/
 /**
  * Create a LVM handle.
@@ -848,6 +867,51 @@ 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
+ *
+ * \param   vg
+ * VG handle obtained from lvm_vg_create() or lvm_vg_open().
+ *
+ * \param   name
+ * Name of property to query.  See vgs man page for full list of properties
+ * that may be queried.
+ *
+ * \param   value
+ * Pointer to an lvm_property_value structure that will contain the current
+ * value of the property.  The pointer may be NULL if just checking the
+ * name of the property.
+ *
+ * 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;
+ *      char *prop_name = "vg_mda_count";
+ *
+ *      if (lvm_vg_get_property(vg, prop_name, NULL)) {
+ *           // handle error
+ *           printf("Invalid property name or unable to query"
+ *                  "'%s'.\n", prop_name);
+ *           return;
+ *      }
+ *      if (lvm_vg_get_property(vg, prop_name, &value) < 0) {
+ *              // handle error
+ *      }
+ *      if (value.is_string)
+ *           printf(", value = %s\n", value.value.string);
+ *	else
+ *           printf(", value = %"PRIu64"\n", value.value.integer);
+ *
+ *
+ * \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..94b3723 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,26 @@ 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;
+
+	prop.id = name;
+	if (!vg_get_property(vg, &prop))
+		return -1;
+	if (!value)
+		return 0;
+	value->is_settable = prop.is_settable;
+	value->is_string = prop.is_string;
+	if (value->is_string)
+		value->value.string = prop.value.string;
+	else
+		value->value.integer = prop.value.integer;
+	return 0;
+}
+
 struct dm_list *lvm_list_vg_names(lvm_t libh)
 {
 	return get_vgnames((struct cmd_context *)libh, 0);
-- 
1.7.2.2