[PATCH 00/04] Add lvm lvseg properties for lvm2app.

[PATCH 00/04] Add lvm lvseg properties for lvm2app.

[Dave Wysochanski]

This patchset adds lvseg_t and lvm_lv_list_lvsegs() API to lvm2app,
then adds lvseg properties and API.  It was created on top of the
following patchset submitted earlier:
http://www.redhat.com/archives/lvm-devel/2010-October/msg00153.html

All lvseg properties are covered by this patch, with the exception of
'seg_pe_ranges' and 'devices'.

This patchset shows the basic approach to enumerating lvsegs, then
querying properties, very similar to the vg, lv, or pv.  Similar
APIs can be made to enumerate and query pvsegs, which would complete
the attribute patchset and allow all properties in the reports to
be queried.

[PATCH 1/4] Add lvseg_t to lvm2app and lvm_lv_list_lvsegs().

[Dave Wysochanski]

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 liblvm/lvm2app.h |   31 +++++++++++++++++++++++++++++++
 liblvm/lvm_lv.c  |   27 +++++++++++++++++++++++++++
 2 files changed, 58 insertions(+), 0 deletions(-)

diff --git a/liblvm/lvm2app.h b/liblvm/lvm2app.h
index e90adf8..f6d54f1 100644
--- a/liblvm/lvm2app.h
+++ b/liblvm/lvm2app.h
@@ -95,6 +95,7 @@ struct lvm;
 struct physical_volume;
 struct volume_group;
 struct logical_volume;
+struct lv_segment;
 
 /**
  * \class lvm_t
@@ -137,6 +138,13 @@ typedef struct logical_volume *lv_t;
 typedef struct physical_volume *pv_t;
 
 /**
+ * \class lvseg_t
+ *
+ * This lv segment object is bound to a lv_t.
+ */
+typedef struct lv_segment *lvseg_t;
+
+/**
  * Logical Volume object list.
  *
  * Lists of these structures are returned by lvm_vg_list_lvs().
@@ -147,6 +155,16 @@ typedef struct lvm_lv_list {
 } lv_list_t;
 
 /**
+ * Logical Volume object list.
+ *
+ * Lists of these structures are returned by lvm_lv_list_lvsegs().
+ */
+typedef struct lvm_lvseg_list {
+	struct dm_list list;
+	lvseg_t lvseg;
+} lvseg_list_t;
+
+/**
  * Physical volume object list.
  *
  * Lists of these structures are returned by lvm_vg_list_pvs().
@@ -933,6 +951,19 @@ struct lvm_property_value lvm_vg_get_property(const vg_t vg, const char *name);
 lv_t lvm_vg_create_lv_linear(vg_t vg, const char *name, uint64_t size);
 
 /**
+ * Return a list of lvseg handles for a given LV handle.
+ *
+ * \memberof lv_t
+ *
+ * \param   lv
+ * Logical volume handle.
+ *
+ * \return
+ * A list of lvm_lvseg_list structures containing lvseg handles for this lv.
+ */
+struct dm_list *lvm_lv_list_lvsegs(lv_t lv);
+
+/**
  * Activate a logical volume.
  *
  * \memberof lv_t
diff --git a/liblvm/lvm_lv.c b/liblvm/lvm_lv.c
index cef87f5..42982f9 100644
--- a/liblvm/lvm_lv.c
+++ b/liblvm/lvm_lv.c
@@ -216,6 +216,33 @@ int lvm_lv_deactivate(lv_t lv)
 	return 0;
 }
 
+struct dm_list *lvm_lv_list_lvsegs(lv_t lv)
+{
+	struct dm_list *list;
+	lvseg_list_t *lvseg;
+	struct lv_segment *lvl;
+
+	if (dm_list_empty(&lv->segments))
+		return NULL;
+
+	if (!(list = dm_pool_zalloc(lv->vg->vgmem, sizeof(*list)))) {
+		log_errno(ENOMEM, "Memory allocation fail for dm_list.");
+		return NULL;
+	}
+	dm_list_init(list);
+
+	dm_list_iterate_items(lvl, &lv->segments) {
+		if (!(lvseg = dm_pool_zalloc(lv->vg->vgmem, sizeof(*lvseg)))) {
+			log_errno(ENOMEM,
+				"Memory allocation fail for lvm_lvseg_list.");
+			return NULL;
+		}
+		lvseg->lvseg = lvl;
+		dm_list_add(list, &lvseg->list);
+	}
+	return list;
+}
+
 int lvm_lv_resize(const lv_t lv, uint64_t new_size)
 {
 	/* FIXME: add lv resize code here */
-- 
1.7.2.2

[PATCH 2/4] Add lvseg 'get' functions.

[Dave Wysochanski]

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

diff --git a/lib/metadata/lv.c b/lib/metadata/lv.c
index b7a8700..eca046f 100644
--- a/lib/metadata/lv.c
+++ b/lib/metadata/lv.c
@@ -20,6 +20,41 @@
 #include "segtype.h"
 #include "str_list.h"
 
+char *lvseg_tags_dup(const struct lv_segment *seg)
+{
+	return tags_format_and_copy(seg->lv->vg->vgmem, &seg->tags);
+}
+
+char *lvseg_segtype_dup(const struct lv_segment *seg)
+{
+	if (seg->area_count == 1) {
+		return (char *)"linear";
+	}
+
+	return dm_pool_strdup(seg->lv->vg->vgmem, seg->segtype->ops->name(seg));
+}
+
+uint64_t lvseg_chunksize(const struct lv_segment *seg)
+{
+	uint64_t size;
+
+	if (lv_is_cow(seg->lv))
+		size = (uint64_t) find_cow(seg->lv)->chunk_size;
+	else
+		size = UINT64_C(0);
+	return size;
+}
+
+uint64_t lvseg_start(const struct lv_segment *seg)
+{
+	return (uint64_t) seg->le * seg->lv->vg->extent_size;
+}
+
+uint64_t lvseg_size(const struct lv_segment *seg)
+{
+	return (uint64_t) seg->len * seg->lv->vg->extent_size;
+}
+
 uint32_t lv_kernel_read_ahead(const struct logical_volume *lv)
 {
 	struct lvinfo info;
diff --git a/lib/metadata/lv.h b/lib/metadata/lv.h
index d77c362..afe6da0 100644
--- a/lib/metadata/lv.h
+++ b/lib/metadata/lv.h
@@ -63,5 +63,10 @@ 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);
+uint64_t lvseg_start(const struct lv_segment *seg);
+uint64_t lvseg_size(const struct lv_segment *seg);
+uint64_t lvseg_chunksize(const struct lv_segment *seg);
+char *lvseg_segtype_dup(const struct lv_segment *seg);
+char *lvseg_tags_dup(const struct lv_segment *seg);
 
 #endif
diff --git a/lib/report/properties.c b/lib/report/properties.c
index c08ddfd..c3315b9 100644
--- a/lib/report/properties.c
+++ b/lib/report/properties.c
@@ -34,6 +34,8 @@ static int _ ## NAME ## _get (const void *obj, struct lvm_property_type *prop) \
 	GET_NUM_PROPERTY_FN(NAME, VALUE, physical_volume, pv)
 #define GET_LV_NUM_PROPERTY_FN(NAME, VALUE) \
 	GET_NUM_PROPERTY_FN(NAME, VALUE, logical_volume, lv)
+#define GET_LVSEG_NUM_PROPERTY_FN(NAME, VALUE) \
+	GET_NUM_PROPERTY_FN(NAME, VALUE, lv_segment, lvseg)
 
 #define GET_STR_PROPERTY_FN(NAME, VALUE, TYPE, VAR)			\
 static int _ ## NAME ## _get (const void *obj, struct lvm_property_type *prop) \
@@ -49,6 +51,8 @@ static int _ ## NAME ## _get (const void *obj, struct lvm_property_type *prop) \
 	GET_STR_PROPERTY_FN(NAME, VALUE, physical_volume, pv)
 #define GET_LV_STR_PROPERTY_FN(NAME, VALUE) \
 	GET_STR_PROPERTY_FN(NAME, VALUE, logical_volume, lv)
+#define GET_LVSEG_STR_PROPERTY_FN(NAME, VALUE) \
+	GET_STR_PROPERTY_FN(NAME, VALUE, lv_segment, lvseg)
 
 static int _not_implemented_get(const void *obj, struct lvm_property_type *prop)
 {
@@ -187,29 +191,29 @@ GET_VG_NUM_PROPERTY_FN(vg_mda_copies, (vg_mda_copies(vg)))
 #define _vg_mda_copies_set _not_implemented_set
 
 /* LVSEG */
-#define _segtype_get _not_implemented_get
+GET_LVSEG_STR_PROPERTY_FN(segtype, lvseg_segtype_dup(lvseg))
 #define _segtype_set _not_implemented_set
-#define _stripes_get _not_implemented_get
+GET_LVSEG_NUM_PROPERTY_FN(stripes, lvseg->area_count)
 #define _stripes_set _not_implemented_set
-#define _stripesize_get _not_implemented_get
+GET_LVSEG_NUM_PROPERTY_FN(stripesize, lvseg->stripe_size)
 #define _stripesize_set _not_implemented_set
-#define _stripe_size_get _not_implemented_get
+GET_LVSEG_NUM_PROPERTY_FN(stripe_size, lvseg->stripe_size)
 #define _stripe_size_set _not_implemented_set
-#define _regionsize_get _not_implemented_get
+GET_LVSEG_NUM_PROPERTY_FN(regionsize, lvseg->region_size)
 #define _regionsize_set _not_implemented_set
-#define _region_size_get _not_implemented_get
+GET_LVSEG_NUM_PROPERTY_FN(region_size, lvseg->region_size)
 #define _region_size_set _not_implemented_set
-#define _chunksize_get _not_implemented_get
+GET_LVSEG_NUM_PROPERTY_FN(chunksize, lvseg_chunksize(lvseg))
 #define _chunksize_set _not_implemented_set
-#define _chunk_size_get _not_implemented_get
+GET_LVSEG_NUM_PROPERTY_FN(chunk_size, lvseg_chunksize(lvseg))
 #define _chunk_size_set _not_implemented_set
-#define _seg_start_get _not_implemented_get
+GET_LVSEG_NUM_PROPERTY_FN(seg_start, lvseg_start(lvseg))
 #define _seg_start_set _not_implemented_set
-#define _seg_start_pe_get _not_implemented_get
+GET_LVSEG_NUM_PROPERTY_FN(seg_start_pe, lvseg->le)
 #define _seg_start_pe_set _not_implemented_set
-#define _seg_size_get _not_implemented_get
+GET_LVSEG_NUM_PROPERTY_FN(seg_size, lvseg_size(lvseg))
 #define _seg_size_set _not_implemented_set
-#define _seg_tags_get _not_implemented_get
+GET_LVSEG_STR_PROPERTY_FN(seg_tags, lvseg_tags_dup(lvseg))
 #define _seg_tags_set _not_implemented_set
 #define _seg_pe_ranges_get _not_implemented_get
 #define _seg_pe_ranges_set _not_implemented_set
@@ -267,6 +271,12 @@ static int _get_property(const void *obj, struct lvm_property_type *prop,
 	return 1;
 }
 
+int lvseg_get_property(const struct lv_segment *lvseg,
+		       struct lvm_property_type *prop)
+{
+	return _get_property(lvseg, prop, SEGS);
+}
+
 int lv_get_property(const struct logical_volume *lv,
 		    struct lvm_property_type *prop)
 {
diff --git a/lib/report/properties.h b/lib/report/properties.h
index 18e5ab9..68d387f 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 lvseg_get_property(const struct lv_segment *lvseg,
+		       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,
diff --git a/lib/report/report.c b/lib/report/report.c
index dbeef21..c7f141f 100644
--- a/lib/report/report.c
+++ b/lib/report/report.c
@@ -279,12 +279,9 @@ static int _segtype_disp(struct dm_report *rh __attribute__((unused)),
 {
 	const struct lv_segment *seg = (const struct lv_segment *) data;
 
-	if (seg->area_count == 1) {
-		dm_report_field_set_value(field, "linear", NULL);
-		return 1;
-	}
-
-	dm_report_field_set_value(field, seg->segtype->ops->name(seg), NULL);
+	char *name;
+	name = lvseg_segtype_dup(seg);
+	dm_report_field_set_value(field, name, NULL);
 	return 1;
 }
 
@@ -497,7 +494,7 @@ static int _segstart_disp(struct dm_report *rh, struct dm_pool *mem,
 	const struct lv_segment *seg = (const struct lv_segment *) data;
 	uint64_t start;
 
-	start = (uint64_t) seg->le * seg->lv->vg->extent_size;
+	start = lvseg_start(seg);
 
 	return _size64_disp(rh, mem, field, &start, private);
 }
@@ -520,7 +517,7 @@ static int _segsize_disp(struct dm_report *rh, struct dm_pool *mem,
 	const struct lv_segment *seg = (const struct lv_segment *) data;
 	uint64_t size;
 
-	size = (uint64_t) seg->len * seg->lv->vg->extent_size;
+	size = lvseg_size(seg);
 
 	return _size64_disp(rh, mem, field, &size, private);
 }
@@ -532,10 +529,7 @@ static int _chunksize_disp(struct dm_report *rh, struct dm_pool *mem,
 	const struct lv_segment *seg = (const struct lv_segment *) data;
 	uint64_t size;
 
-	if (lv_is_cow(seg->lv))
-		size = (uint64_t) find_cow(seg->lv)->chunk_size;
-	else
-		size = UINT64_C(0);
+	size = lvseg_chunksize(seg);
 
 	return _size64_disp(rh, mem, field, &size, private);
 }
-- 
1.7.2.2

[PATCH 2/4] Add lvseg 'get' functions.

[Zdenek Kabelac]

Dne 19.10.2010 15:24, Dave Wysochanski napsal(a):
> 
> Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
> diff --git a/lib/metadata/lv.c b/lib/metadata/lv.c
> index b7a8700..eca046f 100644
> --- a/lib/metadata/lv.c
> +++ b/lib/metadata/lv.c
> @@ -20,6 +20,41 @@
>  #include "segtype.h"
>  #include "str_list.h"
>  
> +char *lvseg_tags_dup(const struct lv_segment *seg)
> +{
> +	return tags_format_and_copy(seg->lv->vg->vgmem, &seg->tags);
> +}
> +
> +char *lvseg_segtype_dup(const struct lv_segment *seg)
> +{
> +	if (seg->area_count == 1) {
> +		return (char *)"linear";

NACK!

> +	}
> +
> +	return dm_pool_strdup(seg->lv->vg->vgmem, seg->segtype->ops->name(seg));
> +}
> +
> +uint64_t lvseg_chunksize(const struct lv_segment *seg)
> +{
> +	uint64_t size;
> +
> +	if (lv_is_cow(seg->lv))
> +		size = (uint64_t) find_cow(seg->lv)->chunk_size;
> +	else
> +		size = UINT64_C(0);

if (lv_is_cow())
       return ...

return 0;


> diff --git a/lib/report/report.c b/lib/report/report.c
> index dbeef21..c7f141f 100644
> --- a/lib/report/report.c
> +++ b/lib/report/report.c
> @@ -279,12 +279,9 @@ static int _segtype_disp(struct dm_report *rh __attribute__((unused)),
>  {
>  	const struct lv_segment *seg = (const struct lv_segment *) data;
>  
> -	if (seg->area_count == 1) {
> -		dm_report_field_set_value(field, "linear", NULL);
> -		return 1;
> -	}
> -
> -	dm_report_field_set_value(field, seg->segtype->ops->name(seg), NULL);
> +	char *name;
> +	name = lvseg_segtype_dup(seg);
> +	dm_report_field_set_value(field, name, NULL);
>  	return 1;
>  }

I really think this API is wrong somewhere - there is way too many duplication
- this isn't going to be very efficient....
I do like the beauty of const strings....


Zdenek

[PATCH 2/4] Add lvseg 'get' functions.

[Petr Rockai]

Zdenek Kabelac <zkabelac@redhat.com> writes:
> I really think this API is wrong somewhere - there is way too many duplication
> - this isn't going to be very efficient....
> I do like the beauty of const strings....

I disagree. Having a consistent API is currently more important than
efficiency. We don't have any profile data, etc. So what you are asking
for is premature optimisation, IMO. We can ditch the duplication later
as needed. (Hopefully, we will be able to somewhat improve the general
memory allocation patterns used by LVM, independent of this. Presumably,
having better definitions of lifetimes of different things on a lower
layer would also make it feasible to rely on that lifetime in the
API. For now, I think it is a sound approach to just duplicate
everything to be on the safe side. Rule of thumb: get a working version
first, write tests, then worry about performance.)

Yours,
   Petr.

[PATCH 3/4] Add lvm_lvseg_get_property() function.

[Dave Wysochanski]

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 liblvm/lvm2app.h  |   39 +++++++++++++++++++++++++++++++++++++++
 liblvm/lvm_lv.c   |    8 +++++++-
 liblvm/lvm_misc.c |    8 +++++++-
 liblvm/lvm_misc.h |    3 ++-
 liblvm/lvm_pv.c   |    2 +-
 liblvm/lvm_vg.c   |    2 +-
 6 files changed, 57 insertions(+), 5 deletions(-)

diff --git a/liblvm/lvm2app.h b/liblvm/lvm2app.h
index f6d54f1..7e2eb69 100644
--- a/liblvm/lvm2app.h
+++ b/liblvm/lvm2app.h
@@ -1099,6 +1099,45 @@ uint64_t lvm_lv_get_size(const lv_t lv);
 struct lvm_property_value lvm_lv_get_property(const lv_t lv, const char *name);
 
 /**
+ * Get the value of a LV segment property
+ *
+ * \memberof lv_t
+ *
+ * \param   lvseg
+ * Logical volume segment 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_start_pe";
+ *
+ *      v = lvm_lvseg_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_lvseg_get_property(const lvseg_t lvseg,
+						 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 42982f9..5d6f110 100644
--- a/liblvm/lvm_lv.c
+++ b/liblvm/lvm_lv.c
@@ -50,7 +50,13 @@ const char *lvm_lv_get_name(const lv_t lv)
 
 struct lvm_property_value lvm_lv_get_property(const lv_t lv, const char *name)
 {
-	return get_property(NULL, NULL, lv, name);
+	return get_property(NULL, NULL, lv, NULL, name);
+}
+
+struct lvm_property_value lvm_lvseg_get_property(const lvseg_t lvseg,
+						 const char *name)
+{
+	return get_property(NULL, NULL, NULL, lvseg, name);
 }
 
 uint64_t lvm_lv_is_active(const lv_t lv)
diff --git a/liblvm/lvm_misc.c b/liblvm/lvm_misc.c
index 52f3636..7eab169 100644
--- a/liblvm/lvm_misc.c
+++ b/liblvm/lvm_misc.c
@@ -46,7 +46,8 @@ 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)
+				       const lv_t lv, const lvseg_t lvseg,
+				       const char *name)
 {
 	struct lvm_property_type prop;
 	struct lvm_property_value v;
@@ -67,6 +68,11 @@ struct lvm_property_value get_property(const pv_t pv, const vg_t vg,
 			v.is_valid = 0;
 			return v;
 		}
+	} else if (lvseg) {
+		if (!lvseg_get_property(lvseg, &prop)) {
+			v.is_valid = 0;
+			return v;
+		}
 	}
 	v.is_settable = prop.is_settable;
 	v.is_string = prop.is_string;
diff --git a/liblvm/lvm_misc.h b/liblvm/lvm_misc.h
index b4e675e..918482e 100644
--- a/liblvm/lvm_misc.h
+++ b/liblvm/lvm_misc.h
@@ -18,6 +18,7 @@
 
 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);
+				       const lv_t lv, const lvseg_t lvseg,
+				       const char *name);
 
 #endif
diff --git a/liblvm/lvm_pv.c b/liblvm/lvm_pv.c
index efdccb5..6705dd8 100644
--- a/liblvm/lvm_pv.c
+++ b/liblvm/lvm_pv.c
@@ -51,7 +51,7 @@ uint64_t lvm_pv_get_free(const pv_t pv)
 
 struct lvm_property_value lvm_pv_get_property(const pv_t pv, const char *name)
 {
-	return get_property(pv, NULL, NULL, name);
+	return get_property(pv, NULL, NULL, NULL, name);
 }
 
 int lvm_pv_resize(const pv_t pv, uint64_t new_size)
diff --git a/liblvm/lvm_vg.c b/liblvm/lvm_vg.c
index 6970910..bc92ca2 100644
--- a/liblvm/lvm_vg.c
+++ b/liblvm/lvm_vg.c
@@ -338,7 +338,7 @@ const char *lvm_vg_get_name(const vg_t vg)
 
 struct lvm_property_value lvm_vg_get_property(const vg_t vg, const char *name)
 {
-	return get_property(NULL, vg, NULL, name);
+	return get_property(NULL, vg, NULL, NULL, name);
 }
 
 struct dm_list *lvm_list_vg_names(lvm_t libh)
-- 
1.7.2.2

[PATCH 3/4] Add lvm_lvseg_get_property() function.

[Zdenek Kabelac]

Dne 19.10.2010 15:24, Dave Wysochanski napsal(a):
> 
> Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
> ---
> 
> diff --git a/liblvm/lvm2app.h b/liblvm/lvm2app.h
> index f6d54f1..7e2eb69 100644
> --- a/liblvm/lvm2app.h
> +++ b/liblvm/lvm2app.h
> @@ -1099,6 +1099,45 @@ uint64_t lvm_lv_get_size(const lv_t lv);
>  struct lvm_property_value lvm_lv_get_property(const lv_t lv, const char *name);
>  
>  /**
> + * Get the value of a LV segment property
> + *
> + * \memberof lv_t
> + *
> + * \param   lvseg
> + * Logical volume segment 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_start_pe";
> + *
> + *      v = lvm_lvseg_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;
> + *      }

That's IMHO not practical.

if (!lvm_lvseg_get_property(lv, prop_name, &v)) {
	/* error  path */
}

or whatever return error status we use, looks much more natural.


Zdenek

[PATCH 4/4] Update tests for lvseg apis.

[Dave Wysochanski]

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

diff --git a/test/api/test.c b/test/api/test.c
index fea3d0d..20f4210 100644
--- a/test/api/test.c
+++ b/test/api/test.c
@@ -69,6 +69,8 @@ static void _show_help(void)
 	       "List the PVs that exist in VG vgname\n");
 	printf("'vg_list_lvs vgname': "
 	       "List the LVs that exist in VG vgname\n");
+	printf("'lv_list_lvsegs vgname lvname': "
+	       "List the LV segments that exist in LV vgname/lvname\n");
 	printf("'vgs_open': "
 	       "List the VGs that are currently open\n");
 	printf("'vgs': "
@@ -569,12 +571,15 @@ 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)
+static void _print_property_value(const char *name,
+				  struct lvm_property_value v)
 {
-	if (value.is_string)
-		printf(", value = %s\n", value.value.string);
+	if (!v.is_valid)
+		printf("%s = INVALID\n", name);
+	else if (v.is_string)
+		printf("%s = %s\n", name, v.value.string);
 	else
-		printf(", value = %"PRIu64"\n", value.value.integer);
+		printf("%s = %"PRIu64"\n", name, v.value.integer);
 }
 
 static void _pv_get_property(char **argv, int argc)
@@ -589,17 +594,7 @@ static void _pv_get_property(char **argv, int argc)
 	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);
+	_print_property_value(argv[2], v);
 }
 
 static void _vg_get_property(char **argv, int argc)
@@ -614,17 +609,7 @@ static void _vg_get_property(char **argv, int argc)
 	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);
+	_print_property_value(argv[2], v);
 }
 
 static void _lv_get_property(char **argv, int argc)
@@ -639,17 +624,7 @@ static void _lv_get_property(char **argv, int argc)
 	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);
+	_print_property_value(argv[3], v);
 }
 
 static void _lv_get_tags(char **argv, int argc)
@@ -742,6 +717,31 @@ static void _lvs_in_vg(char **argv, int argc)
 	}
 }
 
+static void _lvsegs_in_lv(char **argv, int argc)
+{
+	struct dm_list *lvsegs;
+	struct lvm_lvseg_list *lvl;
+	lv_t lv;
+
+	if (!(lv = _lookup_lv_by_name(argv[2])))
+		return;
+	lvsegs = lvm_lv_list_lvsegs(lv);
+	if (!lvsegs || dm_list_empty(lvsegs)) {
+		printf("No LV segments in lv %s\n", lvm_lv_get_name(lv));
+		return;
+	}
+	printf("LV segments in lv %s:\n", lvm_lv_get_name(lv));
+	dm_list_iterate_items(lvl, lvsegs) {
+		struct lvm_property_value v;
+		v = lvm_lvseg_get_property(lvl->lvseg, "segtype");
+		_print_property_value("segtype", v);
+		v = lvm_lvseg_get_property(lvl->lvseg, "seg_start_pe");
+		_print_property_value("seg_start_pe", v);
+		v = lvm_lvseg_get_property(lvl->lvseg, "seg_size");
+		_print_property_value("seg_size", v);
+	}
+}
+
 static void _lv_deactivate(char **argv, int argc)
 {
 	lv_t lv;
@@ -888,6 +888,8 @@ static int lvmapi_test_shell(lvm_t libh)
 			_pvs_in_vg(argv, argc);
 		} else if (!strcmp(argv[0], "vg_list_lvs")) {
 			_lvs_in_vg(argv, argc);
+		} else if (!strcmp(argv[0], "lv_list_lvsegs")) {
+			_lvsegs_in_lv(argv, argc);
 		} else if (!strcmp(argv[0], "list_vg_names")) {
 			_list_vg_names(libh);
 		} else if (!strcmp(argv[0], "list_vg_ids")) {
-- 
1.7.2.2

[PATCH 4/4] Update tests for lvseg apis.

[Zdenek Kabelac]

Dne 19.10.2010 15:24, Dave Wysochanski napsal(a):
> 
> Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
> ---
>  test/api/test.c |   76 ++++++++++++++++++++++++++++--------------------------
>  1 files changed, 39 insertions(+), 37 deletions(-)
> 
> diff --git a/test/api/test.c b/test/api/test.c
> index fea3d0d..20f4210 100644
> --- a/test/api/test.c
> +++ b/test/api/test.c
> @@ -69,6 +69,8 @@ static void _show_help(void)
>  	       "List the PVs that exist in VG vgname\n");
>  	printf("'vg_list_lvs vgname': "
>  	       "List the LVs that exist in VG vgname\n");
> +	printf("'lv_list_lvsegs vgname lvname': "
> +	       "List the LV segments that exist in LV vgname/lvname\n");
>  	printf("'vgs_open': "
>  	       "List the VGs that are currently open\n");
>  	printf("'vgs': "
> @@ -569,12 +571,15 @@ 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)
> +static void _print_property_value(const char *name,
> +				  struct lvm_property_value v)

const struct if you do not modify it.
And I think using pointer would be wise decision here.


>  
>  static void _pv_get_property(char **argv, int argc)
> @@ -589,17 +594,7 @@ static void _pv_get_property(char **argv, int argc)

const array of const pointers ?

Zdenek

[PATCH 4/4] Update tests for lvseg apis.

[Petr Rockai]

Zdenek Kabelac <zkabelac@redhat.com> writes:
> Dne 19.10.2010 15:24, Dave Wysochanski napsal(a):
>> diff --git a/test/api/test.c b/test/api/test.c
>> index fea3d0d..20f4210 100644
>> --- a/test/api/test.c
>> +++ b/test/api/test.c
>> @@ -69,6 +69,8 @@ static void _show_help(void)
>>  	       "List the PVs that exist in VG vgname\n");
>>  	printf("'vg_list_lvs vgname': "
>>  	       "List the LVs that exist in VG vgname\n");
>> +	printf("'lv_list_lvsegs vgname lvname': "
>> +	       "List the LV segments that exist in LV vgname/lvname\n");
>>  	printf("'vgs_open': "
>>  	       "List the VGs that are currently open\n");
>>  	printf("'vgs': "
>> @@ -569,12 +571,15 @@ 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)
>> +static void _print_property_value(const char *name,
>> +				  struct lvm_property_value v)

> const struct if you do not modify it.
Whether or not you modify it, the change cannot be reflected in the
caller. This is call by value.

> And I think using pointer would be wise decision here.
Care to elaborate? What's wise about using a pointer here?

[PATCH 4/4] Update tests for lvseg apis.

[Zdenek Kabelac]

Dne 21.10.2010 18:38, Petr Rockai napsal(a):
> Zdenek Kabelac <zkabelac@redhat.com> writes:
>> Dne 19.10.2010 15:24, Dave Wysochanski napsal(a):
>>> diff --git a/test/api/test.c b/test/api/test.c
>>> index fea3d0d..20f4210 100644
>>> --- a/test/api/test.c
>>> +++ b/test/api/test.c
>>> @@ -69,6 +69,8 @@ static void _show_help(void)
>>>  	       "List the PVs that exist in VG vgname\n");
>>>  	printf("'vg_list_lvs vgname': "
>>>  	       "List the LVs that exist in VG vgname\n");
>>> +	printf("'lv_list_lvsegs vgname lvname': "
>>> +	       "List the LV segments that exist in LV vgname/lvname\n");
>>>  	printf("'vgs_open': "
>>>  	       "List the VGs that are currently open\n");
>>>  	printf("'vgs': "
>>> @@ -569,12 +571,15 @@ 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)
>>> +static void _print_property_value(const char *name,
>>> +				  struct lvm_property_value v)
> 
>> const struct if you do not modify it.
> Whether or not you modify it, the change cannot be reflected in the
> caller. This is call by value.



> 
>> And I think using pointer would be wise decision here.
> Care to elaborate? What's wise about using a pointer here?

(const struct lvm_proper_value *v)

avoid doing local copy of the 'v' structure - currently it's not a big
difference, but as you pointed out in other emails - it could be extended in
future. And I think we are using  pointers in other calls as well - so we
should stay consistent also internally - and just pass pointers all the time -
instead of thinking when to use struct and when the pointer should be passed.

Zdenek

[PATCH 4/4] Update tests for lvseg apis.

[Petr Rockai]

Zdenek Kabelac <zkabelac@redhat.com> writes:
>>> And I think using pointer would be wise decision here.
>> Care to elaborate? What's wise about using a pointer here?
>
> (const struct lvm_proper_value *v)
>
> avoid doing local copy of the 'v' structure - currently it's not a big
> difference, but as you pointed out in other emails - it could be extended in
> future. And I think we are using  pointers in other calls as well - so we
> should stay consistent also internally - and just pass pointers all the time -
> instead of thinking when to use struct and when the pointer should be passed.

Unlike object lifetime, this is something that the compiler can check
statically. Moreover, it may or may not be more efficient to pass a
pointer. Usually it's not, unless the structure is big and/or the
compiler optimisations poor.

Passing a value is prone to fewer errors. I vote for a value. (We should
aim to pass values whenever possible, IMHO. Those vg_t/pv_t/lv_t behave
as values as well, in this respect.)

Yours,
   Petr.

[PATCH 4/4] Update tests for lvseg apis.

[Zdenek Kabelac]

Dne 22.10.2010 10:59, Petr Rockai napsal(a):
> Zdenek Kabelac <zkabelac@redhat.com> writes:
>>>> And I think using pointer would be wise decision here.
>>> Care to elaborate? What's wise about using a pointer here?
>>
>> (const struct lvm_proper_value *v)
>>
>> avoid doing local copy of the 'v' structure - currently it's not a big
>> difference, but as you pointed out in other emails - it could be extended in
>> future. And I think we are using  pointers in other calls as well - so we
>> should stay consistent also internally - and just pass pointers all the time -
>> instead of thinking when to use struct and when the pointer should be passed.
> 
> Unlike object lifetime, this is something that the compiler can check
> statically. Moreover, it may or may not be more efficient to pass a
> pointer. Usually it's not, unless the structure is big and/or the
> compiler optimisations poor.
> 
> Passing a value is prone to fewer errors. I vote for a value. (We should
> aim to pass values whenever possible, IMHO. Those vg_t/pv_t/lv_t behave
> as values as well, in this respect.)
> 

I'm not going to argue whether it's more or less efficient as in this case it
will make no difference.  But passing things by value is simply very hardly
supportable by dso libraries - and usually require complete rebuild of binary
to use updated library. Also you would need to remember where are you using
passing by value - and in case structure size grows - rewrite many functions
to switch to pointers - why not do that right from the beginning? -  I'd pass
by value only the language atomic/basic types - definitely not any structure
where even you are pointing out future extensions in other post.

BTW: I don't take vg_t/pv_t/lv_t as an an argument for passing things by value
as it's nothing else that syntactical sugar for pointers - and in fact I'd not
any objections against exactly same strategy for properties - making property
object completely private structure to lvm and give the API user only handle
lvm_property_t and set of function for this handle (C++ in C :))
(i.e.
lvm_property_is_string/get_string/is_integer/get_integer(lvm_property_t...)

Zdenek

[PATCH 4/4] Update tests for lvseg apis.

[Petr Rockai]

Zdenek Kabelac <zkabelac@redhat.com> writes:
> I'm not going to argue whether it's more or less efficient as in this case it
> will make no difference.  But passing things by value is simply very hardly
> supportable by dso libraries - and usually require complete rebuild of binary
> to use updated library. Also you would need to remember where are you using
> passing by value - and in case structure size grows - rewrite many functions
> to switch to pointers - why not do that right from the beginning? -  I'd pass
> by value only the language atomic/basic types - definitely not any structure
> where even you are pointing out future extensions in other post.

But there is a difference in adding bitfield values / union members and
adding new members to the structure! Both mentioned extensions are
ABI-compatible (within limits, see my other mails).

> BTW: I don't take vg_t/pv_t/lv_t as an an argument for passing things by value
> as it's nothing else that syntactical sugar for pointers - and in fact I'd not
> any objections against exactly same strategy for properties - making property
> object completely private structure to lvm and give the API user only handle
> lvm_property_t and set of function for this handle (C++ in C :))
> (i.e.
> lvm_property_is_string/get_string/is_integer/get_integer(lvm_property_t...)

That's both inefficient and extremely tedious to write and read. You
have to make some compromises, C is a low level language, and by no
means supports encapsulation or (gods forbid) OO. I will rather freeze
the structure ABI-wise than have to write

lvm_property_t p = lvm_get_property(...)
if (lvm_property_is_valid(p)) {
    if (lvm_property_is_string(p)) {
       const char *str = lvm_property_get_string(p);
       ...
    } else if (lvm_property_is_integer(p)) {
       int i = lvm_property_get_integer(p);
       ...
    }
}

In fact, I'd rather go for C++ than for Glib, if we want to have fancy
things in the API.

Yours,
   Petr.

[PATCH 4/4] Update tests for lvseg apis.

[Zdenek Kabelac]

Dne 24.10.2010 23:06, Petr Rockai napsal(a):
> Zdenek Kabelac <zkabelac@redhat.com> writes:
>> I'm not going to argue whether it's more or less efficient as in this case it
>> will make no difference.  But passing things by value is simply very hardly
>> supportable by dso libraries - and usually require complete rebuild of binary
>> to use updated library. Also you would need to remember where are you using
>> passing by value - and in case structure size grows - rewrite many functions
>> to switch to pointers - why not do that right from the beginning? -  I'd pass
>> by value only the language atomic/basic types - definitely not any structure
>> where even you are pointing out future extensions in other post.
> 
> But there is a difference in adding bitfield values / union members and
> adding new members to the structure! Both mentioned extensions are
> ABI-compatible (within limits, see my other mails).
> 
>> BTW: I don't take vg_t/pv_t/lv_t as an an argument for passing things by value
>> as it's nothing else that syntactical sugar for pointers - and in fact I'd not
>> any objections against exactly same strategy for properties - making property
>> object completely private structure to lvm and give the API user only handle
>> lvm_property_t and set of function for this handle (C++ in C :))
>> (i.e.
>> lvm_property_is_string/get_string/is_integer/get_integer(lvm_property_t...)
> 
> That's both inefficient and extremely tedious to write and read. You
> have to make some compromises, C is a low level language, and by no
> means supports encapsulation or (gods forbid) OO. I will rather freeze
> the structure ABI-wise than have to write
> 
> lvm_property_t p = lvm_get_property(...)
> if (lvm_property_is_valid(p)) {
>     if (lvm_property_is_string(p)) {
>        const char *str = lvm_property_get_string(p);
>        ...
>     } else if (lvm_property_is_integer(p)) {
>        int i = lvm_property_get_integer(p);
>        ...
>     }
> }
> 

If don't see all that big difference in:

(p.is_valid)   and  (lvm_property_is_valid(p))

and as the common pattern would be to query for some known property it should
be all handled by lines like this:

if (lvm_property_get_string(p, &prop_char))

which keeps all those  'is_valid() & is_string' internal - IMHO there won't be
many tools interested in detailed diagnostic in their code....
This gives you nicer and user code that you can get with plain struct...
(IMHO everyone would write such wrapper for struct anyway...)

Exposing structures to the 'outside' world if you already think about future
extensions should be rather avoided.

> In fact, I'd rather go for C++ than for Glib, if we want to have fancy
> things in the API.

Plain C is still fancy and usable ;)

Zdenek