Change lvm2app memory allocation for pv/vg/lv properties
cover Change lvm2app memory allocation for pv/vg/lv properties
This patchset changes memory allocation for pv/vg/lv properties in lvm2app. Unfortunately, this is a change to the existing API, but has the following benefits:
1) gives us consistency in memory handling across the API (lvm2app allocs/frees memory for strings and lists).
2) allows for simpler application code
A few downsides of this approach:
1) an application that repeatedly calls a 'get' property function will generate repeated allocations and could eventually trigger an OOM, as the only way to release memory is to release the vg handle. Possible fixes to this situation include the application release and reacquire the vg handle, or lvm2app provide another API to release object property memory tied to a vg handle (internally this would require a separate memory pool for the attribute memory). In any case, there are workarounds possible for this downside.
2) An application may hold a vg handle open longer than it would previously. This would mean an application would hold a lock longer than it might have had lvm2app given the application control of the memory. This is really an application issue though, since the application can simply copy memory properties it might need and release the vg handle.
PATCH 1/2 Use vg->vgmem to allocate vg/lv/pv string properties instead of dm_malloc/free.
Everywhere else in the API the caller can rely on lvm2app taking care of memory allocation and free, so make the 'name' and 'uuid' properties of a vg/lv/pv use the vg handle to allocate memory.
--- liblvm/lvm2app.h | 24 ++++++++++++------------ liblvm/lvm_lv.c | 10 +++------- liblvm/lvm_pv.c | 10 +++------- liblvm/lvm_vg.c | 9 ++------- 4 files changed, 20 insertions(+), 33 deletions(-)
liblvm/lvm2app.h +12 -12
@@ -642,12 +642,12 @@ uint64_t lvm_vg_is_partial(vg_t vg);uint64_t lvm_vg_get_seqno (const vg_t vg);/** -* Get the current name of a volume group. +* Get the current uuid of a volume group. * * \memberof vg_t * -* Memory is allocated using dm_malloc() and caller must free the memory -* using dm_free(). +* The memory allocated for the uuid is tied to the vg_t handle and will be +* released when lvm_vg_close() is called. * * \param vg * VG handle obtained from lvm_vg_create or lvm_vg_open().
@@ -658,12 +658,12 @@ uint64_t lvm_vg_get_seqno(const vg_t vg);char *lvm_vg_get_uuid (const vg_t vg);/** -* Get the current uuid of a volume group. +* Get the current name of a volume group. * * \memberof vg_t * -* Memory is allocated using dm_malloc() and caller must free the memory -* using dm_free(). +* The memory allocated for the name is tied to the vg_t handle and will be +* released when lvm_vg_close() is called. * * \param vg * VG handle obtained from lvm_vg_create or lvm_vg_open().
@@ -783,7 +783,7 @@ uint64_t lvm_vg_get_max_lv(const vg_t vg);* \memberof vg_t * * The memory allocated for the list is tied to the vg_t handle and will be -* released when lvm_vg_close is called. +* released when lvm_vg_close() is called. * * To process the list, use the dm_list iterator functions. For example: * vg_t vg;
@@ -1001,7 +1001,7 @@ int lvm_lv_remove_tag(lv_t lv, const char *tag);* \memberof lv_t * * The memory allocated for the list is tied to the vg_t handle and will be -* released when lvm_vg_close is called. +* released when lvm_vg_close() is called. * * To process the list, use the dm_list iterator functions. For example: * lv_t lv;
@@ -1057,8 +1057,8 @@ int lvm_lv_resize(const lv_t lv, uint64_t new_size);* * \memberof pv_t * -* Memory is allocated using dm_malloc() and caller must free the memory -* using dm_free(). +* The memory allocated for the uuid is tied to the vg_t handle and will be +* released when lvm_vg_close() is called. * * \param pv * Physical volume handle.
@@ -1073,8 +1073,8 @@ char *lvm_pv_get_uuid(const pv_t pv);* * \memberof pv_t * -* Memory is allocated using dm_malloc() and caller must free the memory -* using dm_free(). +* The memory allocated for the uuid is tied to the vg_t handle and will be +* released when lvm_vg_close() is called. * * \param pv * Physical volume handle.
liblvm/lvm_lv.c +3 -7
@@ -47,17 +47,13 @@ char *lvm_lv_get_uuid(const lv_t lv)log_error (INTERNAL_ERROR "unable to convert uuid" );return NULL ; } -return strndup ((const char *)uuid,64 ); +return dm_pool_strndup (lv->vg->vgmem, (const char *)uuid,64 ); }char *lvm_lv_get_name (const lv_t lv) { -char *name; - - name =dm_malloc (NAME_LEN +1 ); -strncpy (name, (const char *)lv->name,NAME_LEN ); - name[NAME_LEN ] ='\0' ; -return name; +return dm_pool_strndup (lv->vg->vgmem, (const char *)lv->name, +NAME_LEN +1 ); }uint64_t lvm_lv_is_active (const lv_t lv)
liblvm/lvm_pv.c +3 -7
@@ -25,17 +25,13 @@ char *lvm_pv_get_uuid(const pv_t pv)log_error (INTERNAL_ERROR "Unable to convert uuid" );return NULL ; } -return strndup ((const char *)uuid,64 ); +return dm_pool_strndup (pv->vg->vgmem, (const char *)uuid,64 ); }char *lvm_pv_get_name (const pv_t pv) { -char *name; - - name =dm_malloc (NAME_LEN +1 ); -strncpy (name, (const char *)pv_dev_name (pv),NAME_LEN ); - name[NAME_LEN ] ='\0' ; -return name; +return dm_pool_strndup (pv->vg->vgmem, + (const char *)pv_dev_name (pv),NAME_LEN +1 ); }uint64_t lvm_pv_get_mda_count (const pv_t pv)
liblvm/lvm_vg.c +2 -7
@@ -336,17 +336,12 @@ char *lvm_vg_get_uuid(const vg_t vg)log_error (INTERNAL_ERROR "Unable to convert uuid" );return NULL ; } -return strndup ((const char *)uuid,64 ); +return dm_pool_strndup (vg->vgmem, (const char *)uuid,64 ); }char *lvm_vg_get_name (const vg_t vg) { -char *name; - - name =dm_malloc (NAME_LEN +1 ); -strncpy (name, (const char *)vg->name,NAME_LEN ); - name[NAME_LEN ] ='\0' ; -return name; +return dm_pool_strndup (vg->vgmem, (const char *)vg->name,NAME_LEN +1 ); }struct dm_list *lvm_list_vg_names (lvm_t libh)
Signature · 1 line
1.6.0.6
PATCH 2/2 Change lvm2app version number from 1 to 2.
This version number change reflects the memory handling change for string-based pv/vg/lv string based attributes.
In addition, when adding support for tags, I forgot to increase the version number.
--- VERSION | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
VERSION +1 -1
@@ -1 +1 @@ -2.02.63(1)-cvs (2010-03-09) +2.02.63(2)-cvs (2010-03-09)
Signature · 1 line
1.6.0.6
PATCH 1/2 Use vg->vgmem to allocate vg/lv/pv string properties instead of dm_malloc/free.
In reply to Dave WysochanskiQuoted 126 lines · Dave Wysochanski
Everywhere else in the API the caller can rely on lvm2app taking care of memory allocation and free, so make the 'name' and 'uuid' properties of a vg/lv/pv use the vg handle to allocate memory.
Signed-off-by Dave Wysochanski <[email protected]>--- liblvm/lvm2app.h | 24 ++++++++++++------------ liblvm/lvm_lv.c | 10 +++------- liblvm/lvm_pv.c | 10 +++------- liblvm/lvm_vg.c | 9 ++------- 4 files changed, 20 insertions(+), 33 deletions(-)liblvm/lvm2app.h +12
-12@@ -642,12 +642,12 @@ uint64_t lvm_vg_is_partial(vg_t vg);uint64_t lvm_vg_get_seqno (const vg_t vg);/** -* Get the current name of a volume group. +* Get the current uuid of a volume group. * * \memberof vg_t * -* Memory is allocated using dm_malloc() and caller must free the memory -* using dm_free(). +* The memory allocated for the uuid is tied to the vg_t handle and will be +* released when lvm_vg_close() is called. * * \param vg * VG handle obtained from lvm_vg_create or lvm_vg_open(). @@ -658,12 +658,12 @@ uint64_t lvm_vg_get_seqno(const vg_t vg);char *lvm_vg_get_uuid (const vg_t vg);/** -* Get the current uuid of a volume group. +* Get the current name of a volume group. * * \memberof vg_t * -* Memory is allocated using dm_malloc() and caller must free the memory -* using dm_free(). +* The memory allocated for the name is tied to the vg_t handle and will be +* released when lvm_vg_close() is called. * * \param vg * VG handle obtained from lvm_vg_create or lvm_vg_open(). @@ -783,7 +783,7 @@ uint64_t lvm_vg_get_max_lv(const vg_t vg);* \memberof vg_t * * The memory allocated for the list is tied to the vg_t handle and will be -* released when lvm_vg_close is called. +* released when lvm_vg_close() is called. * * To process the list, use the dm_list iterator functions. For example: * vg_t vg; @@ -1001,7 +1001,7 @@ int lvm_lv_remove_tag(lv_t lv, const char *tag);* \memberof lv_t * * The memory allocated for the list is tied to the vg_t handle and will be -* released when lvm_vg_close is called. +* released when lvm_vg_close() is called. * * To process the list, use the dm_list iterator functions. For example: * lv_t lv; @@ -1057,8 +1057,8 @@ int lvm_lv_resize(const lv_t lv, uint64_t new_size);* * \memberof pv_t * -* Memory is allocated using dm_malloc() and caller must free the memory -* using dm_free(). +* The memory allocated for the uuid is tied to the vg_t handle and will be +* released when lvm_vg_close() is called. * * \param pv * Physical volume handle. @@ -1073,8 +1073,8 @@ char *lvm_pv_get_uuid(const pv_t pv);* * \memberof pv_t * -* Memory is allocated using dm_malloc() and caller must free the memory -* using dm_free(). +* The memory allocated for the uuid is tied to the vg_t handle and will be +* released when lvm_vg_close() is called. * * \param pv * Physical volume handle. liblvm/lvm_lv.c +3
-7@@ -47,17 +47,13 @@ char *lvm_lv_get_uuid(const lv_t lv)log_error (INTERNAL_ERROR "unable to convert uuid" );return NULL ; } -return strndup ((const char *)uuid,64 ); +return dm_pool_strndup (lv->vg->vgmem, (const char *)uuid,64 ); }char *lvm_lv_get_name (const lv_t lv) { -char *name; - - name =dm_malloc (NAME_LEN +1 ); -strncpy (name, (const char *)lv->name,NAME_LEN ); - name[NAME_LEN ] ='\0' ; -return name; +return dm_pool_strndup (lv->vg->vgmem, (const char *)lv->name, +NAME_LEN +1 ); }uint64_t lvm_lv_is_active (const lv_t lv)liblvm/lvm_pv.c +1
-1@@ -25,17 +25,13 @@ char *lvm_pv_get_uuid(const pv_t pv)log_error (INTERNAL_ERROR "Unable to convert uuid" );return NULL ; } -return strndup ((const char *)uuid,64 ); +return dm_pool_strndup (pv->vg->vgmem, (const char *)uuid,64 ); }char *lvm_pv_get_name (const pv_t pv)
liblvm/lvm_pv.c char *lvm_pv_get_uuid(const pv_t pv) · line 35
Quoted 10 lines
liblvm/lvm_pv.c +2
-6{ -char *name; - - name =dm_malloc (NAME_LEN +1 ); -strncpy (name, (const char *)pv_dev_name (pv),NAME_LEN ); - name[NAME_LEN ] ='\0' ; -return name; +return dm_pool_strndup (pv->vg->vgmem, + (const char *)pv_dev_name (pv),NAME_LEN +1 ); }
Why not return const char* for given PV - if user needs local copy, he may do a local copy himself calling strdup - doing this all the time is not really effective.
Zdenek
PATCH 1/2 Use vg->vgmem to allocate vg/lv/pv string properties instead of dm_malloc/free.
Quoted 126 lines · Zdenek Kabelac
Quoted 125 lines · Dave Wysochanski
Everywhere else in the API the caller can rely on lvm2app taking care of memory allocation and free, so make the 'name' and 'uuid' properties of a vg/lv/pv use the vg handle to allocate memory.
Signed-off-by Dave Wysochanski <[email protected]>--- liblvm/lvm2app.h | 24 ++++++++++++------------ liblvm/lvm_lv.c | 10 +++------- liblvm/lvm_pv.c | 10 +++------- liblvm/lvm_vg.c | 9 ++------- 4 files changed, 20 insertions(+), 33 deletions(-)liblvm/lvm2app.h +12
-12@@ -642,12 +642,12 @@ uint64_t lvm_vg_is_partial(vg_t vg);uint64_t lvm_vg_get_seqno (const vg_t vg);/** -* Get the current name of a volume group. +* Get the current uuid of a volume group. * * \memberof vg_t * -* Memory is allocated using dm_malloc() and caller must free the memory -* using dm_free(). +* The memory allocated for the uuid is tied to the vg_t handle and will be +* released when lvm_vg_close() is called. * * \param vg * VG handle obtained from lvm_vg_create or lvm_vg_open(). @@ -658,12 +658,12 @@ uint64_t lvm_vg_get_seqno(const vg_t vg);char *lvm_vg_get_uuid (const vg_t vg);/** -* Get the current uuid of a volume group. +* Get the current name of a volume group. * * \memberof vg_t * -* Memory is allocated using dm_malloc() and caller must free the memory -* using dm_free(). +* The memory allocated for the name is tied to the vg_t handle and will be +* released when lvm_vg_close() is called. * * \param vg * VG handle obtained from lvm_vg_create or lvm_vg_open(). @@ -783,7 +783,7 @@ uint64_t lvm_vg_get_max_lv(const vg_t vg);* \memberof vg_t * * The memory allocated for the list is tied to the vg_t handle and will be -* released when lvm_vg_close is called. +* released when lvm_vg_close() is called. * * To process the list, use the dm_list iterator functions. For example: * vg_t vg; @@ -1001,7 +1001,7 @@ int lvm_lv_remove_tag(lv_t lv, const char *tag);* \memberof lv_t * * The memory allocated for the list is tied to the vg_t handle and will be -* released when lvm_vg_close is called. +* released when lvm_vg_close() is called. * * To process the list, use the dm_list iterator functions. For example: * lv_t lv; @@ -1057,8 +1057,8 @@ int lvm_lv_resize(const lv_t lv, uint64_t new_size);* * \memberof pv_t * -* Memory is allocated using dm_malloc() and caller must free the memory -* using dm_free(). +* The memory allocated for the uuid is tied to the vg_t handle and will be +* released when lvm_vg_close() is called. * * \param pv * Physical volume handle. @@ -1073,8 +1073,8 @@ char *lvm_pv_get_uuid(const pv_t pv);* * \memberof pv_t * -* Memory is allocated using dm_malloc() and caller must free the memory -* using dm_free(). +* The memory allocated for the uuid is tied to the vg_t handle and will be +* released when lvm_vg_close() is called. * * \param pv * Physical volume handle. liblvm/lvm_lv.c +3
-7@@ -47,17 +47,13 @@ char *lvm_lv_get_uuid(const lv_t lv)log_error (INTERNAL_ERROR "unable to convert uuid" );return NULL ; } -return strndup ((const char *)uuid,64 ); +return dm_pool_strndup (lv->vg->vgmem, (const char *)uuid,64 ); }char *lvm_lv_get_name (const lv_t lv) { -char *name; - - name =dm_malloc (NAME_LEN +1 ); -strncpy (name, (const char *)lv->name,NAME_LEN ); - name[NAME_LEN ] ='\0' ; -return name; +return dm_pool_strndup (lv->vg->vgmem, (const char *)lv->name, +NAME_LEN +1 ); }uint64_t lvm_lv_is_active (const lv_t lv)liblvm/lvm_pv.c +1
-1@@ -25,17 +25,13 @@ char *lvm_pv_get_uuid(const pv_t pv)log_error (INTERNAL_ERROR "Unable to convert uuid" );return NULL ; } -return strndup ((const char *)uuid,64 ); +return dm_pool_strndup (pv->vg->vgmem, (const char *)uuid,64 ); }
Quoted 16 lines
liblvm/lvm_pv.c char *lvm_pv_get_uuid(const pv_t pv) · line 35
Quoted 11 lines
liblvm/lvm_pv.c +2
-6char *lvm_pv_get_name (const pv_t pv) { -char *name; - - name =dm_malloc (NAME_LEN +1 ); -strncpy (name, (const char *)pv_dev_name (pv),NAME_LEN ); - name[NAME_LEN ] ='\0' ; -return name; +return dm_pool_strndup (pv->vg->vgmem, + (const char *)pv_dev_name (pv),NAME_LEN +1 ); }Why not return const char* for given PV - if user needs local copy, he may do a local copy himself calling strdup - doing this all the time is not really effective.
Right - const. I forgot that yet again - I will update the patch.
The memory copy on the attributes is for safety. The question is, what is the consequence of allowing an application the possibility to change an attribute value without using a 'set' or 'change' function. Using const, you do get a compile warning, but you can still change it if for example a non-const pointer is used as the lvalue for assignment to the function return. For things like vgnames, uuids, we definitely would not want to allow the possibility. For other things, maybe the consequences would not be as great but I guess I'd err on the side of safety instead of efficiency. This is one area where an internal library may differ from an external/application library though.