Add pv->vg link, rework lvm2app string properties
cover Add pv->vg link, rework lvm2app string properties
This patchset adds a pv->vg link and reworks lvm2app string property functions to use the vg handle's memory for allocation.
The first 3 patches are simple refactorings, the 4th patch adds the pv->vg link, and the last 2 change lvm2app string functions and the lvm2app version number.
PATCH 1/6 Add pv to vg->pvs after check for maximum value of vg->extent_count.
In add_pv_to_vg(), we should only add the pv to vg->pvs after all internal checks have passed. The check for vg->extent_count exeeding maximum was after we added the pv to the list, so this function could return a state of vg->pvs that did not reflect other parameters such as vg->pv_count.
--- lib/metadata/metadata.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-)
lib/metadata/metadata.c +3 -3
@@ -220,9 +220,6 @@ int add_pv_to_vg(struct volume_group *vg, const char *pv_name,if (!alloc_pv_segment_whole_pv (mem, pv)) return_0; - pvl->pv = pv; -dm_list_add (&vg->pvs, &pvl->list); -if ((uint64_t ) vg->extent_count + pv->pe_count >UINT32_MAX ) { log_error("Unable to add %s to %s: new extent count (%" PRIu64 ") exceeds limit (%" PRIu32 ").",
@@ -232,6 +229,9 @@ int add_pv_to_vg(struct volume_group *vg, const char *pv_name,return 0 ; } + pvl->pv = pv; +dm_list_add (&vg->pvs, &pvl->list); + vg->pv_count++; vg->extent_count += pv->pe_count; vg->free_count += pv->pe_count;
Signature · 1 line
1.6.0.6
PATCH 2/6 Refactor _read_pv() code adding pv to vg->pvs and updating vg counts.
Simple refactor to group code that adds a pv to the vg->vgs and updates related vg counts. No functional change.
--- lib/format_text/import_vsn1.c | 6 ++---- 1 files changed, 2 insertions(+), 4 deletions(-)
lib/format_text/import_vsn1.c +2 -4
@@ -241,10 +241,6 @@ static int _read_pv(struct format_instance *fid, struct dm_pool *mem,return 0 ; } -/* adjust the volume group. */ - vg->extent_count += pv->pe_count; - vg->free_count += pv->pe_count; - pv->pe_size = vg->extent_size; pv->pe_alloc_count =0 ;
@@ -273,6 +269,8 @@ static int _read_pv(struct format_instance *fid, struct dm_pool *mem,if (!alloc_pv_segment_whole_pv (mem, pv)) return_0; + vg->extent_count += pv->pe_count; + vg->free_count += pv->pe_count; vg->pv_count++;dm_list_add (&vg->pvs, &pvl->list);
Signature · 1 line
1.6.0.6
PATCH 3/6 Add add_pvl_to_vgs() - helper function to add a pv to a vg list.
Small refactor of main places in the code where a pv is added to a vg into a small function which adds the pv to the list and updates the vg counts.
--- lib/format_text/import_vsn1.c | 5 +---- lib/metadata/metadata.c | 16 +++++++++++----- lib/metadata/metadata.h | 1 + 3 files changed, 13 insertions(+), 9 deletions(-)
lib/format_text/import_vsn1.c +1 -4
@@ -269,10 +269,7 @@ static int _read_pv(struct format_instance *fid, struct dm_pool *mem,if (!alloc_pv_segment_whole_pv (mem, pv)) return_0; - vg->extent_count += pv->pe_count; - vg->free_count += pv->pe_count; - vg->pv_count++; -dm_list_add (&vg->pvs, &pvl->list); +add_pvl_to_vgs (vg, pvl);return 1 ; }
lib/metadata/metadata.c +11 -5
@@ -136,6 +136,16 @@ out:return pv->pe_align_offset; } +void add_pvl_to_vgs (struct volume_group *vg,struct pv_list *pvl) +{ +dm_list_add (&vg->pvs, &pvl->list); + + vg->pv_count++; + vg->extent_count += pvl->pv->pe_count; + vg->free_count += pvl->pv->pe_count; +} + +/** * add_pv_to_vg - Add a physical volume to a volume group * @vg - volume group to add to
@@ -230,11 +240,7 @@ int add_pv_to_vg(struct volume_group *vg, const char *pv_name, } pvl->pv = pv; -dm_list_add (&vg->pvs, &pvl->list); - - vg->pv_count++; - vg->extent_count += pv->pe_count; - vg->free_count += pv->pe_count; +add_pvl_to_vgs (vg, pvl);return 1 ; }
lib/metadata/metadata.h +1 -0
@@ -377,5 +377,6 @@ struct id pv_vgid(const struct physical_volume *pv);struct physical_volume *pv_by_path (struct cmd_context *cmd,const char *pv_name);int add_pv_to_vg (struct volume_group *vg,const char *pv_name,struct physical_volume *pv); +void add_pvl_to_vgs (struct volume_group *vg,struct pv_list *pvl);#endif
Signature · 1 line
1.6.0.6
PATCH 4/6 Add pv->vg to solidify link between a pv and a vg.
lvm2app needs a link back to the vg in order to use the vg handle for memory allocations as well as other things. This patch adds the field to struct physical_volume, and sets the field when returning from vg_read() internal library call.
NOTE: Unfortunately in the case of pv_read() callers, we cannot set this pv->vg field properly, since the vg it belongs to may not be known by just reading a single device, and no VG is in existence at the time of pv_read(). Significant refactorings related to lvmcache, PVs with 0 mdas, etc, are required to set this field for callers of pv_read(). For now we assume that if the field is not set, the pv is not associated with a complete VG (as returned by vg_read()).
--- lib/metadata/metadata-exported.h | 2 +- lib/metadata/metadata.c | 1 + 2 files changed, 2 insertions(+), 1 deletions(-)
lib/metadata/metadata-exported.h +1 -1
@@ -183,7 +183,7 @@ struct physical_volume {const struct format_type *fmt;const char *vg_name;struct id vgid; - +struct volume_group *vg;/* FIXME: Only valid when used with lvm2app. */ uint64_t status;uint64_t size;
lib/metadata/metadata.c +1 -0
@@ -143,6 +143,7 @@ void add_pvl_to_vgs(struct volume_group *vg, struct pv_list *pvl) vg->pv_count++; vg->extent_count += pvl->pv->pe_count; vg->free_count += pvl->pv->pe_count; + pvl->pv->vg = vg; }
Signature · 1 line
1.6.0.6
PATCH 5/6 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 6/6 RFC: 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. Also, it might be nice to have a short list of what has changed in the API since the last version, apart from the WHATS_NEW lvm file.
--- 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 6/6 RFC: Change lvm2app version number from 1 to 2.
Quoted 2 lines · Dave Wysochanski
This version number change reflects the memory handling change for string-based pv/vg/lv string based attributes.
Maybe it is better idea to use VG mempool, maybe not for VG attributes.
But change it now, it means that application using calls which previously deallocates these attributes with dm_free will now crash, because the memory returned is now owned by library.
I really do not like changing API this way in every release...
I see three API possibilities
- define buffer parameter, so all memory allocations are
in user application
- return strings/parameters *CONST* and define that lvm2app
owns memory
- use dm_malloc, and require dm_free in user aplications.
But please do not change this later!
It is extremely confusing for library users, no warning that API is not stable will not help here...
Milan
PATCH 6/6 RFC: Change lvm2app version number from 1 to 2.
Quoted 4 lines · Milan Broz
Quoted 2 lines · Dave Wysochanski
This version number change reflects the memory handling change for string-based pv/vg/lv string based attributes.
Quoted 8 lines
Maybe it is better idea to use VG mempool, maybe not for VG attributes.
But change it now, it means that application using calls which previously deallocates these attributes with
dm_freewill now crash, because the memory returned is now owned by library.
Right it's not a "nice" change right now but as far as I know we have one library user and I've been in touch with him.
Quoted 2 lines
I really do not like changing API this way in every release...
Do you think it is better to leave things as they are?
It has not been changed until now. Plus, there is already an inconsistency in the way memory is handled with the APIs that return lists owned by lvm2app and the attribute APIs return memory owned by the application.
With things as they are now, in addition to the inconsistency, an application could easily create memory leaks if for example it just wants to display attributes with something like this:
printf("vg_name = %s", lvm_vg_get_name(vg));
instead, the proper code is:
char *vgname;
vgname = lvm_vg_get_name(vg);
printf ("vg_name = %s", vgname);dm_free(vgname);
And this must be done for _every_ attribute.
Quoted 10 lines
I see three API possibilities
- define buffer parameter, so all memory allocations are
in user application- return strings/parameters *CONST* and define that lvm2app
owns memory- use
dm_malloc, and requiredm_freein user aplications.
Right, those are the possibilities. I had discussed this with the David Zeuthen a few weeks ago and he seemed to be fine with the change as this is how one of his other APIs work. I can forward you the discussion and/or confirm with him again.
There are pros and cons of each approach.
Quoted 5 lines
But please do not change this later!
It is extremely confusing for library users, no warning that API is not stable will not help here...
Agreed. But do you feel we should leave the current inconsistency?
Given that we are still early in the library development, and there are few users, I felt it was not out of the question to propose a change.
PATCH 4/6 Add pv->vg to solidify link between a pv and a vg.
In reply to Dave Wysochanskilib/metadata/metadata-exported.h struct physical_volume { · line 186
Quoted 6 lines · Dave Wysochanski
lvm2app needs a link back to the vg in order to use the vg handle for memory allocations as well as other things. This patch adds the field to struct
physical_volume, and sets the field when returning fromvg_read()internal library call.lib/metadata/metadata-exported.h +1
-0+struct volume_group *vg;/* FIXME: Only valid when used with lvm2app. */
Please make it generally applicable and always internally consistent i.e. always pointing back to the VG struct which contains the list it is on.
Also update vg_validate whenever extending the core metadata structs.
Alasdair
PATCH 4/6 Add pv->vg to solidify link between a pv and a vg.
Quoted 11 lines · Alasdair G Kergon
lib/metadata/metadata-exported.h struct physical_volume { · line 186
Quoted 6 lines · Dave Wysochanski
lvm2app needs a link back to the vg in order to use the vg handle for memory allocations as well as other things. This patch adds the field to struct
physical_volume, and sets the field when returning fromvg_read()internal library call.lib/metadata/metadata-exported.h +1
-0+struct volume_group *vg;/* FIXME: Only valid when used with lvm2app. */ Please make it generally applicable and always internally consistent i.e. always pointing back to the VG struct which contains the list it is on.
Oh - this FIXME is an erroneous comment - sorry. Originally I had approached it this way, then changed it to be more general. I could have missed something though.
Quoted 2 lines
Also update
vg_validatewhenever extending the core metadata structs.
Will do so - thanks for the reminder.