* [PATCH] lvm2app: Add thin and thin pool lv creation V2
@ 2013-05-03 16:08 Tony Asleson
2013-05-06 13:26 ` M. Mohan Kumar
0 siblings, 1 reply; 6+ messages in thread
From: Tony Asleson @ 2013-05-03 16:08 UTC (permalink / raw)
To: lvm-devel
From: "M. Mohan Kumar" <mohan@in.ibm.com>
Add thin and thin pool lv creation support to lvm library
This is Mohan's patch, re-worked to include suggestions
from Zdenek.
V2: Remove const lvm_lv_params_create_thin
Add const lvm_lv_params_skip_zero_get
Signed-off-by: Tony Asleson <tasleson@redhat.com>
---
liblvm/lvm2app.h | 116 +++++++++++++++++++++++++++++++
liblvm/lvm_lv.c | 205 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 321 insertions(+)
diff --git a/liblvm/lvm2app.h b/liblvm/lvm2app.h
index 93a78c3..0a00425 100644
--- a/liblvm/lvm2app.h
+++ b/liblvm/lvm2app.h
@@ -97,6 +97,7 @@ struct volume_group;
struct logical_volume;
struct lv_segment;
struct pv_segment;
+struct lvm_lv_create_params;
/**
* \class lvm_t
@@ -153,6 +154,14 @@ typedef struct lv_segment *lvseg_t;
typedef struct pv_segment *pvseg_t;
/**
+ * \class lv_create_params
+ *
+ * This lv_create_params represents the plethora of available options when
+ * creating a logical volume
+ */
+typedef struct lvm_lv_create_params *lv_create_params_t;
+
+/**
* Logical Volume object list.
*
* Lists of these structures are returned by lvm_vg_list_lvs().
@@ -1400,6 +1409,113 @@ int lvm_lv_resize(const lv_t lv, uint64_t new_size);
*/
lv_t lvm_lv_snapshot(const lv_t lv, const char *snap_name, uint64_t max_snap_size);
+/**
+ * Thin provisioning discard policies
+ */
+typedef enum {
+ LVM_THIN_DISCARDS_IGNORE,
+ LVM_THIN_DISCARDS_NO_PASSDOWN,
+ LVM_THIN_DISCARDS_PASSDOWN,
+} lvm_thin_discards_t;
+
+/**
+ * Create a thinpool parameter passing object for the specified VG
+ *
+ * \param vg
+ * Volume Group handle.
+ *
+ * \param pool_name
+ * Name of the pool.
+ *
+ * \param size
+ * size of the pool
+ *
+ * \param chunk_size
+ * data block size of the pool
+ * Default value is DEFAULT_THIN_POOL_CHUNK_SIZE * 2 when 0 passed as chunk_size
+ * DM_THIN_MIN_DATA_BLOCK_SIZE < chunk_size < DM_THIN_MAX_DATA_BLOCK_SIZE
+ *
+ * \param meta_size
+ * Size of thin pool's metadata logical volume. Allowed range is 2MB-16GB.
+ * Default value (ie if 0) pool size / pool chunk size * 64
+ *
+ * \param discard
+ * Thin discard policy
+ * Note: THIN_DISCARDS_PASSDOWN is the default.
+ *
+ * \return
+ * Valid lv_create_params pointer on success, else NULL on error.
+ * Note: Memory is associated with the vg, it will get reclaimed when vg is
+ * closed.
+ *
+ */
+lv_create_params_t lvm_lv_params_create_thin_pool(vg_t vg,
+ const char *pool_name, uint64_t size, uint32_t chunk_size,
+ uint64_t meta_size, lvm_thin_discards_t discard);
+
+#define lvm_lv_params_create_thin_pool_default(vg, pool_name, size) \
+ lvm_lv_params_create_thin_pool((vg), (pool_name), (size), 0, 0, \
+ LVM_THIN_DISCARDS_PASSDOWN)
+
+
+/**
+ * Set the value of zero skip for the lv_create_params
+ *
+ * \param params
+ * lv_create_prams to change
+ *
+ * \param skip
+ * Value of skip zero to be set (0 == zero, 1 == skip zero)
+ *
+ * \return
+ * 0 on success, else -1
+ */
+int lvm_lv_params_skip_zero_set(lv_create_params_t params, int skip);
+
+/**
+ * Get the value of zero skip for the lv_create_params
+ *
+ * \param params
+ * lv_create_prams to query
+ *
+ * \return
+ * -1 on error, else 0 == zero, 1 == skip zero
+ */
+int lvm_lv_params_skip_zero_get(const lv_create_params_t params);
+
+/**
+ * Create a thin LV creation parameters in a given VG & thin pool
+ *
+ * \param vg
+ * Volume Group handle.
+ *
+ * \param pool_name
+ * Name of the pool.
+ *
+ * \param lvname
+ * Name of the LV to create
+ *
+ * \param size
+ * Size of logical volume
+ *
+ * \return
+ * Valid lv_create_params pointer on success, else NULL on error.
+ * Note: Memory is associated with the vg, it will get reclaimed when vg is
+ * closed.
+ *
+ */
+lv_create_params_t lvm_lv_params_create_thin(const vg_t vg, const char *pool_name,
+ const char *lvname, uint64_t size);
+/**
+ * Create the actual logical volume.
+ *
+ * \param params The parameters object for lv creation
+ *
+ * \return
+ * Valid lv pointer on success, else NULL on error.
+ */
+lv_t lvm_lv_create(lv_create_params_t params);
+
/************************** physical volume handling ************************/
/**
diff --git a/liblvm/lvm_lv.c b/liblvm/lvm_lv.c
index 91948a6..e1b76be 100644
--- a/liblvm/lvm_lv.c
+++ b/liblvm/lvm_lv.c
@@ -22,6 +22,15 @@
#include "lvm_misc.h"
#include "lvm2app.h"
+struct lvm_lv_create_params
+{
+ uint32_t magic;
+ vg_t vg;
+ struct lvcreate_params lvp;
+};
+
+#define LV_CREATE_PARAMS_MAGIC 0xFEED0001
+
static int _lv_check_handle(const lv_t lv, const int vg_writeable)
{
if (!lv || !lv->vg || vg_read_error(lv->vg))
@@ -350,3 +359,199 @@ lv_t lvm_lv_snapshot(const lv_t lv, const char *snap_name, uint64_t max_snap_siz
return NULL;
return (lv_t) lvl->lv;
}
+
+/* Set defaults for thin pool specific LV parameters */
+static void _lv_set_pool_params(struct lvcreate_params *lp,
+ vg_t vg, const char *pool,
+ uint64_t extents, uint64_t meta_size)
+{
+ _lv_set_default_params(lp, vg, NULL, extents);
+
+ lp->pool = pool;
+
+ lp->create_thin_pool = 1;
+ lp->segtype = get_segtype_from_string(vg->cmd, "thin-pool");
+ lp->stripes = 1;
+
+ if (!meta_size) {
+ lp->poolmetadatasize = extents * vg->extent_size /
+ (lp->chunk_size * (SECTOR_SIZE / 64));
+ while ((lp->poolmetadatasize >
+ (2 * DEFAULT_THIN_POOL_OPTIMAL_SIZE / SECTOR_SIZE)) &&
+ lp->chunk_size < DM_THIN_MAX_DATA_BLOCK_SIZE) {
+ lp->chunk_size <<= 1;
+ lp->poolmetadatasize >>= 1;
+ }
+ } else
+ lp->poolmetadatasize = meta_size;
+
+ if (lp->poolmetadatasize % vg->extent_size)
+ lp->poolmetadatasize +=
+ vg->extent_size - lp->poolmetadatasize % vg->extent_size;
+
+ lp->poolmetadataextents =
+ extents_from_size(vg->cmd, lp->poolmetadatasize / SECTOR_SIZE,
+ vg->extent_size);
+}
+
+lv_create_params_t lvm_lv_params_create_thin_pool(vg_t vg,
+ const char *pool_name, uint64_t size, uint32_t chunk_size,
+ uint64_t meta_size, lvm_thin_discards_t discard)
+{
+ uint64_t extents = 0;
+ struct lvm_lv_create_params *lvcp = NULL;
+
+ if (meta_size > (2 * DEFAULT_THIN_POOL_MAX_METADATA_SIZE)) {
+ log_error("Invalid metadata size");
+ return NULL;
+ }
+
+ if (meta_size &&
+ meta_size < (2 * DEFAULT_THIN_POOL_MIN_METADATA_SIZE)) {
+ log_error("Invalid metadata size");
+ return NULL;
+ }
+
+ if (vg_read_error(vg))
+ return NULL;
+
+ if (!vg_check_write_mode(vg))
+ return NULL;
+
+ if (pool_name == NULL || !strlen(pool_name)) {
+ log_error("pool_name invalid");
+ return NULL;
+ }
+
+ if (!(extents = extents_from_size(vg->cmd, size / SECTOR_SIZE,
+ vg->extent_size))) {
+ log_error("Unable to create LV thin pool without size.");
+ return NULL;
+ }
+
+ lvcp = dm_pool_zalloc(vg->vgmem, sizeof (struct lvm_lv_create_params));
+
+ if (lvcp) {
+ lvcp->vg = vg;
+ lvcp->lvp.discards = discard;
+
+ if (chunk_size)
+ lvcp->lvp.chunk_size = chunk_size;
+ else
+ lvcp->lvp.chunk_size = DEFAULT_THIN_POOL_CHUNK_SIZE * 2;
+
+ if (lvcp->lvp.chunk_size < DM_THIN_MIN_DATA_BLOCK_SIZE ||
+ lvcp->lvp.chunk_size > DM_THIN_MAX_DATA_BLOCK_SIZE) {
+ log_error("Invalid chunk_size");
+ return NULL;
+ }
+
+ _lv_set_pool_params(&lvcp->lvp, vg, pool_name, extents, meta_size);
+
+ lvcp->magic = LV_CREATE_PARAMS_MAGIC;
+ }
+ return lvcp;
+}
+
+/* Set defaults for thin LV specific parameters */
+static void _lv_set_thin_params(struct lvcreate_params *lp,
+ vg_t vg, const char *pool,
+ const char *lvname,
+ uint64_t extents)
+{
+ _lv_set_default_params(lp, vg, lvname, extents);
+
+ lp->thin = 1;
+ lp->pool = pool;
+ lp->segtype = get_segtype_from_string(vg->cmd, "thin");
+
+ lp->voriginsize = extents * vg->extent_size;
+ lp->voriginextents = extents_from_size(vg->cmd, lp->voriginsize,
+ vg->extent_size);
+
+ lp->stripes = 1;
+}
+
+lv_create_params_t lvm_lv_params_create_thin(const vg_t vg, const char *pool_name,
+ const char *lvname, uint64_t size)
+{
+ struct lvm_lv_create_params *lvcp = NULL;
+ uint64_t extents = 0;
+
+ /* precondition checks */
+ if (vg_read_error(vg))
+ return NULL;
+
+ if (!vg_check_write_mode(vg))
+ return NULL;
+
+ if (pool_name == NULL || !strlen(pool_name)) {
+ log_error("pool_name invalid");
+ return NULL;
+ }
+
+ if (lvname == NULL || !strlen(lvname)) {
+ log_error("lvname invalid");
+ return NULL;
+ }
+
+ if (!(extents = extents_from_size(vg->cmd, size / SECTOR_SIZE,
+ vg->extent_size))) {
+ log_error("Unable to create thin LV without size.");
+ return NULL;
+ }
+
+ lvcp = dm_pool_zalloc(vg->vgmem, sizeof (struct lvm_lv_create_params));
+ if (lvcp) {
+ lvcp->vg = vg;
+ _lv_set_thin_params(&lvcp->lvp, vg, pool_name, lvname, extents);
+ lvcp->magic = LV_CREATE_PARAMS_MAGIC;
+ }
+
+ return lvcp;
+}
+
+int lvm_lv_params_skip_zero_set(lv_create_params_t params, int skip)
+{
+ int rc = -1;
+
+ if (skip == 0 || skip == 1) {
+ if (params && params->magic == LV_CREATE_PARAMS_MAGIC) {
+ params->lvp.zero = skip;
+ rc = 0;
+ } else {
+ log_error("Invalid lv_create_params parameter");
+ }
+ } else {
+ log_error("Invalid skip value");
+ }
+ return rc;
+}
+
+int lvm_lv_params_skip_zero_get(const lv_create_params_t params)
+{
+ if (params && params->magic == LV_CREATE_PARAMS_MAGIC) {
+ return params->lvp.zero;
+ }
+ log_error("Invalid lv_create_params parameter");
+ return -1;
+}
+
+lv_t lvm_lv_create(lv_create_params_t params)
+{
+ struct lv_list *lvl = NULL;
+
+ if (params && params->magic == LV_CREATE_PARAMS_MAGIC) {
+ if (!params->lvp.segtype) {
+ log_error("segtype parameter is NULL");
+ return_NULL;
+ }
+ if (!lv_create_single(params->vg, ¶ms->lvp))
+ return_NULL;
+ if (!(lvl = find_lv_in_vg(params->vg, params->lvp.pool)))
+ return_NULL;
+ return (lv_t) lvl->lv;
+ }
+ log_error("Invalid lv_create_params parameter");
+ return NULL;
+}
--
1.8.1.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH] lvm2app: Add thin and thin pool lv creation V2
2013-05-03 16:08 [PATCH] lvm2app: Add thin and thin pool lv creation V2 Tony Asleson
@ 2013-05-06 13:26 ` M. Mohan Kumar
2013-05-06 15:37 ` Tony Asleson
0 siblings, 1 reply; 6+ messages in thread
From: M. Mohan Kumar @ 2013-05-06 13:26 UTC (permalink / raw)
To: lvm-devel
Tony Asleson <tasleson@redhat.com> writes:
> + *
> + */
> +lv_create_params_t lvm_lv_params_create_thin_pool(vg_t vg,
> + const char *pool_name, uint64_t size, uint32_t chunk_size,
> + uint64_t meta_size, lvm_thin_discards_t discard);
> +
> +#define lvm_lv_params_create_thin_pool_default(vg, pool_name, size) \
> + lvm_lv_params_create_thin_pool((vg), (pool_name), (size), 0, 0, \
> + LVM_THIN_DISCARDS_PASSDOWN)
> +
> +
> +/**
> + * Set the value of zero skip for the lv_create_params
> + *
> + * \param params
> + * lv_create_prams to change
> + *
> + * \param skip
> + * Value of skip zero to be set (0 == zero, 1 == skip zero)
> + *
> + * \return
> + * 0 on success, else -1
> + */
> +int lvm_lv_params_skip_zero_set(lv_create_params_t params, int skip);
Are we going to provide as many helper functions to get/set parameters
in lv_create_params structure? For example if an user does wants a new
LV to be created in deactivation state, should we provide a helper
something like this?
int lvm_lv_params_activation_set(lv_create_params_t params, int
activate);
In this case lvm-api wont be flooded with lots of helper functions?
Instead of individual helper function how about a common helper function
to take care of this?
int lvm_lv_params_set(lv_create_params_t params, enum lv_create_param_type,
type, void *data);
lvm_lv_params_set function will validate 'data' based on the
lv_create_param_type and will return error if 'data' does not match
given enum (resource)?
enum lv_create_param_type {
LV_CR_INVALID,
LV_CR_ZERO,
LV_CR_ACTIVATION,
LV_CR_MAX_PARAM,
};
In above zero case, this function will be invoked like this:
lvm_lv_params_set(params, LV_CR_ZERO, &zero);
lvm_lv_params_set will do:
switch (type) {
case LV_CR_ZERO:
memcpy (&zero_skip, data, sizeof(int));
break;
case LV_CR_ACTIVATION:
memcpy (&activation, data, sizeof(int));
if (activation < CHANGE_AY && activation >
CHANGE_AAY) {
log_error();
return -1;
}
}
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] lvm2app: Add thin and thin pool lv creation V2
2013-05-06 13:26 ` M. Mohan Kumar
@ 2013-05-06 15:37 ` Tony Asleson
2013-05-06 15:47 ` Zdenek Kabelac
2013-05-06 15:48 ` M. Mohan Kumar
0 siblings, 2 replies; 6+ messages in thread
From: Tony Asleson @ 2013-05-06 15:37 UTC (permalink / raw)
To: lvm-devel
On 05/06/2013 08:26 AM, M. Mohan Kumar wrote:
> Tony Asleson <tasleson@redhat.com> writes:
>
>> + *
>> + */
>> +lv_create_params_t lvm_lv_params_create_thin_pool(vg_t vg,
>> + const char *pool_name, uint64_t size, uint32_t chunk_size,
>> + uint64_t meta_size, lvm_thin_discards_t discard);
>> +
>> +#define lvm_lv_params_create_thin_pool_default(vg, pool_name, size) \
>> + lvm_lv_params_create_thin_pool((vg), (pool_name), (size), 0, 0, \
>> + LVM_THIN_DISCARDS_PASSDOWN)
>> +
>> +
>> +/**
>> + * Set the value of zero skip for the lv_create_params
>> + *
>> + * \param params
>> + * lv_create_prams to change
>> + *
>> + * \param skip
>> + * Value of skip zero to be set (0 == zero, 1 == skip zero)
>> + *
>> + * \return
>> + * 0 on success, else -1
>> + */
>> +int lvm_lv_params_skip_zero_set(lv_create_params_t params, int skip);
>
> Are we going to provide as many helper functions to get/set parameters
> in lv_create_params structure? For example if an user does wants a new
> LV to be created in deactivation state, should we provide a helper
> something like this?
>
> int lvm_lv_params_activation_set(lv_create_params_t params, int
> activate);
>
> In this case lvm-api wont be flooded with lots of helper functions?
> Instead of individual helper function how about a common helper function
> to take care of this?
>
> int lvm_lv_params_set(lv_create_params_t params, enum lv_create_param_type,
> type, void *data);
>
> lvm_lv_params_set function will validate 'data' based on the
> lv_create_param_type and will return error if 'data' does not match
> given enum (resource)?
>
> enum lv_create_param_type {
> LV_CR_INVALID,
> LV_CR_ZERO,
> LV_CR_ACTIVATION,
>
> LV_CR_MAX_PARAM,
> };
>
> In above zero case, this function will be invoked like this:
>
> lvm_lv_params_set(params, LV_CR_ZERO, &zero);
>
> lvm_lv_params_set will do:
> switch (type) {
> case LV_CR_ZERO:
> memcpy (&zero_skip, data, sizeof(int));
> break;
> case LV_CR_ACTIVATION:
> memcpy (&activation, data, sizeof(int));
> if (activation < CHANGE_AY && activation >
> CHANGE_AAY) {
> log_error();
> return -1;
> }
> }
>
>
Yeah, this is probably a better approach to minimize API explosion. The
only thing I would like to add is a size_t data_len field. This way we
can accommodate variable size data for same operation.
Does that sound reasonable? I guess if we only support simple fixed
type sizes it would be redundant.
Thanks,
Tony
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] lvm2app: Add thin and thin pool lv creation V2
2013-05-06 15:37 ` Tony Asleson
@ 2013-05-06 15:47 ` Zdenek Kabelac
2013-05-06 17:17 ` Tony Asleson
2013-05-06 15:48 ` M. Mohan Kumar
1 sibling, 1 reply; 6+ messages in thread
From: Zdenek Kabelac @ 2013-05-06 15:47 UTC (permalink / raw)
To: lvm-devel
Dne 6.5.2013 17:37, Tony Asleson napsal(a):
> On 05/06/2013 08:26 AM, M. Mohan Kumar wrote:
>> Tony Asleson <tasleson@redhat.com> writes:
>>
>>> + *
>>> + */
>>> +lv_create_params_t lvm_lv_params_create_thin_pool(vg_t vg,
>>> + const char *pool_name, uint64_t size, uint32_t chunk_size,
>>> + uint64_t meta_size, lvm_thin_discards_t discard);
>>> +
>>> +#define lvm_lv_params_create_thin_pool_default(vg, pool_name, size) \
>>> + lvm_lv_params_create_thin_pool((vg), (pool_name), (size), 0, 0, \
>>> + LVM_THIN_DISCARDS_PASSDOWN)
>>> +
>>> +
>>> +/**
>>> + * Set the value of zero skip for the lv_create_params
>>> + *
>>> + * \param params
>>> + * lv_create_prams to change
>>> + *
>>> + * \param skip
>>> + * Value of skip zero to be set (0 == zero, 1 == skip zero)
>>> + *
>>> + * \return
>>> + * 0 on success, else -1
>>> + */
>>> +int lvm_lv_params_skip_zero_set(lv_create_params_t params, int skip);
>>
>> Are we going to provide as many helper functions to get/set parameters
>> in lv_create_params structure? For example if an user does wants a new
>> LV to be created in deactivation state, should we provide a helper
>> something like this?
>>
>> int lvm_lv_params_activation_set(lv_create_params_t params, int
>> activate);
>>
>> In this case lvm-api wont be flooded with lots of helper functions?
>> Instead of individual helper function how about a common helper function
>> to take care of this?
>>
>> int lvm_lv_params_set(lv_create_params_t params, enum lv_create_param_type,
>> type, void *data);
>>
>> lvm_lv_params_set function will validate 'data' based on the
>> lv_create_param_type and will return error if 'data' does not match
>> given enum (resource)?
>>
>> enum lv_create_param_type {
>> LV_CR_INVALID,
>> LV_CR_ZERO,
>> LV_CR_ACTIVATION,
>>
>> LV_CR_MAX_PARAM,
>> };
>>
>> In above zero case, this function will be invoked like this:
>>
>> lvm_lv_params_set(params, LV_CR_ZERO, &zero);
>>
>> lvm_lv_params_set will do:
>> switch (type) {
>> case LV_CR_ZERO:
>> memcpy (&zero_skip, data, sizeof(int));
>> break;
>> case LV_CR_ACTIVATION:
>> memcpy (&activation, data, sizeof(int));
>> if (activation < CHANGE_AY && activation >
>> CHANGE_AAY) {
>> log_error();
>> return -1;
>> }
>> }
>>
>>
>
> Yeah, this is probably a better approach to minimize API explosion. The
> only thing I would like to add is a size_t data_len field. This way we
> can accommodate variable size data for same operation.
>
> Does that sound reasonable? I guess if we only support simple fixed
> type sizes it would be redundant.
We already have the API 'explosion' for lvs/pvs/vgs properties.
So I think it could probably share the same principal for lvcreate attrs.
I liked the 'X protocol' way of using string named fields - but then
you lose compile time check whether you pass valid arguments...
Zdenek
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] lvm2app: Add thin and thin pool lv creation V2
2013-05-06 15:37 ` Tony Asleson
2013-05-06 15:47 ` Zdenek Kabelac
@ 2013-05-06 15:48 ` M. Mohan Kumar
1 sibling, 0 replies; 6+ messages in thread
From: M. Mohan Kumar @ 2013-05-06 15:48 UTC (permalink / raw)
To: lvm-devel
Tony Asleson <tasleson@redhat.com> writes:
> On 05/06/2013 08:26 AM, M. Mohan Kumar wrote:
>> Tony Asleson <tasleson@redhat.com> writes:
>>
>>> + *
>>> + */
>>> +lv_create_params_t lvm_lv_params_create_thin_pool(vg_t vg,
>>> + const char *pool_name, uint64_t size, uint32_t chunk_size,
>>> + uint64_t meta_size, lvm_thin_discards_t discard);
>>> +
>>> +#define lvm_lv_params_create_thin_pool_default(vg, pool_name, size) \
>>> + lvm_lv_params_create_thin_pool((vg), (pool_name), (size), 0, 0, \
>>> + LVM_THIN_DISCARDS_PASSDOWN)
>>> +
>>> +
>>> +/**
>>> + * Set the value of zero skip for the lv_create_params
>>> + *
>>> + * \param params
>>> + * lv_create_prams to change
>>> + *
>>> + * \param skip
>>> + * Value of skip zero to be set (0 == zero, 1 == skip zero)
>>> + *
>>> + * \return
>>> + * 0 on success, else -1
>>> + */
>>> +int lvm_lv_params_skip_zero_set(lv_create_params_t params, int skip);
>>
>> Are we going to provide as many helper functions to get/set parameters
>> in lv_create_params structure? For example if an user does wants a new
>> LV to be created in deactivation state, should we provide a helper
>> something like this?
>>
>> int lvm_lv_params_activation_set(lv_create_params_t params, int
>> activate);
>>
>> In this case lvm-api wont be flooded with lots of helper functions?
>> Instead of individual helper function how about a common helper function
>> to take care of this?
>>
>> int lvm_lv_params_set(lv_create_params_t params, enum lv_create_param_type,
>> type, void *data);
>>
>> lvm_lv_params_set function will validate 'data' based on the
>> lv_create_param_type and will return error if 'data' does not match
>> given enum (resource)?
>>
>> enum lv_create_param_type {
>> LV_CR_INVALID,
>> LV_CR_ZERO,
>> LV_CR_ACTIVATION,
>>
>> LV_CR_MAX_PARAM,
>> };
>>
>> In above zero case, this function will be invoked like this:
>>
>> lvm_lv_params_set(params, LV_CR_ZERO, &zero);
>>
>> lvm_lv_params_set will do:
>> switch (type) {
>> case LV_CR_ZERO:
>> memcpy (&zero_skip, data, sizeof(int));
>> break;
>> case LV_CR_ACTIVATION:
>> memcpy (&activation, data, sizeof(int));
>> if (activation < CHANGE_AY && activation >
>> CHANGE_AAY) {
>> log_error();
>> return -1;
>> }
>> }
>>
>>
>
> Yeah, this is probably a better approach to minimize API explosion. The
> only thing I would like to add is a size_t data_len field. This way we
> can accommodate variable size data for same operation.
>
> Does that sound reasonable? I guess if we only support simple fixed
> type sizes it would be redundant.
>
+1
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] lvm2app: Add thin and thin pool lv creation V2
2013-05-06 15:47 ` Zdenek Kabelac
@ 2013-05-06 17:17 ` Tony Asleson
0 siblings, 0 replies; 6+ messages in thread
From: Tony Asleson @ 2013-05-06 17:17 UTC (permalink / raw)
To: lvm-devel
On 05/06/2013 10:47 AM, Zdenek Kabelac wrote:
> We already have the API 'explosion' for lvs/pvs/vgs properties.
> So I think it could probably share the same principal for lvcreate attrs.
For API consistency we probably should use the same technique for
get/set proprieties like we do in other parts of the API, for some
reason when you mentioned more C++ like I went with a separate get/set
functions. I've mentioned before that I don't like returning structures
by value because there is a potential link issues if the library and
client code compiled with different options:
-fpcc-struct-return/-freg-struct-return.
> I liked the 'X protocol' way of using string named fields - but then
> you lose compile time check whether you pass valid arguments...
We are going to lose compile time enforcement regardless if we want
generic get/set functionality.
Regards,
Tony
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-05-06 17:17 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-05-03 16:08 [PATCH] lvm2app: Add thin and thin pool lv creation V2 Tony Asleson
2013-05-06 13:26 ` M. Mohan Kumar
2013-05-06 15:37 ` Tony Asleson
2013-05-06 15:47 ` Zdenek Kabelac
2013-05-06 17:17 ` Tony Asleson
2013-05-06 15:48 ` M. Mohan Kumar
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.