* LVM2/liblvm lvm2app.h lvm_lv.c lvm_pv.c lvm_vg.c
@ 2010-04-19 15:22 wysochanski
2010-04-19 15:34 ` Zdenek Kabelac
0 siblings, 1 reply; 6+ messages in thread
From: wysochanski @ 2010-04-19 15:22 UTC (permalink / raw)
To: lvm-devel
CVSROOT: /cvs/lvm2
Module name: LVM2
Changes by: wysochanski at sourceware.org 2010-04-19 15:22:24
Modified files:
liblvm : lvm2app.h lvm_lv.c lvm_pv.c lvm_vg.c
Log message:
Use vg->vgmem to allocate vg/lv/pv string properties instead of dm_malloc/fr
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 <dwysocha@redhat.com>
Patches:
http://sourceware.org/cgi-bin/cvsweb.cgi/LVM2/liblvm/lvm2app.h.diff?cvsroot=lvm2&r1=1.14&r2=1.15
http://sourceware.org/cgi-bin/cvsweb.cgi/LVM2/liblvm/lvm_lv.c.diff?cvsroot=lvm2&r1=1.21&r2=1.22
http://sourceware.org/cgi-bin/cvsweb.cgi/LVM2/liblvm/lvm_pv.c.diff?cvsroot=lvm2&r1=1.10&r2=1.11
http://sourceware.org/cgi-bin/cvsweb.cgi/LVM2/liblvm/lvm_vg.c.diff?cvsroot=lvm2&r1=1.39&r2=1.40
--- LVM2/liblvm/lvm2app.h 2010/02/24 18:16:44 1.14
+++ LVM2/liblvm/lvm2app.h 2010/04/19 15:22:24 1.15
@@ -642,12 +642,12 @@
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().
@@ -655,15 +655,15 @@
* \return
* Copy of the uuid string.
*/
-char *lvm_vg_get_uuid(const vg_t vg);
+const 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().
@@ -671,7 +671,7 @@
* \return
* Copy of the name.
*/
-char *lvm_vg_get_name(const vg_t vg);
+const char *lvm_vg_get_name(const vg_t vg);
/**
* Get the current size in bytes of a volume group.
@@ -783,7 +783,7 @@
* \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;
@@ -896,7 +896,7 @@
* \return
* Copy of the uuid string.
*/
-char *lvm_lv_get_uuid(const lv_t lv);
+const char *lvm_lv_get_uuid(const lv_t lv);
/**
* Get the current uuid of a logical volume.
@@ -912,7 +912,7 @@
* \return
* Copy of the name.
*/
-char *lvm_lv_get_name(const lv_t lv);
+const char *lvm_lv_get_name(const lv_t lv);
/**
* Get the current size in bytes of a logical volume.
@@ -1001,7 +1001,7 @@
* \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 @@
*
* \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.
@@ -1066,15 +1066,15 @@
* \return
* Copy of the uuid string.
*/
-char *lvm_pv_get_uuid(const pv_t pv);
+const char *lvm_pv_get_uuid(const pv_t pv);
/**
* Get the current name of a physical volume.
*
* \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.
@@ -1082,7 +1082,7 @@
* \return
* Copy of the name.
*/
-char *lvm_pv_get_name(const pv_t pv);
+const char *lvm_pv_get_name(const pv_t pv);
/**
* Get the current number of metadata areas in the physical volume.
--- LVM2/liblvm/lvm_lv.c 2010/03/25 18:22:05 1.21
+++ LVM2/liblvm/lvm_lv.c 2010/04/19 15:22:24 1.22
@@ -39,7 +39,7 @@
return SECTOR_SIZE * lv_size(lv);
}
-char *lvm_lv_get_uuid(const lv_t lv)
+const char *lvm_lv_get_uuid(const lv_t lv)
{
char uuid[64] __attribute((aligned(8)));
@@ -47,17 +47,13 @@
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)
+const 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)
--- LVM2/liblvm/lvm_pv.c 2010/03/25 18:22:05 1.10
+++ LVM2/liblvm/lvm_pv.c 2010/04/19 15:22:24 1.11
@@ -17,7 +17,7 @@
#include "metadata-exported.h"
#include "lvm-string.h"
-char *lvm_pv_get_uuid(const pv_t pv)
+const char *lvm_pv_get_uuid(const pv_t pv)
{
char uuid[64] __attribute((aligned(8)));
@@ -25,17 +25,13 @@
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)
+const 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)
--- LVM2/liblvm/lvm_vg.c 2010/03/25 18:22:05 1.39
+++ LVM2/liblvm/lvm_vg.c 2010/04/19 15:22:24 1.40
@@ -328,7 +328,7 @@
return vg_max_lv(vg);
}
-char *lvm_vg_get_uuid(const vg_t vg)
+const char *lvm_vg_get_uuid(const vg_t vg)
{
char uuid[64] __attribute((aligned(8)));
@@ -336,17 +336,12 @@
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)
+const 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)
^ permalink raw reply [flat|nested] 6+ messages in thread
* LVM2/liblvm lvm2app.h lvm_lv.c lvm_pv.c lvm_vg.c
2010-04-19 15:22 LVM2/liblvm lvm2app.h lvm_lv.c lvm_pv.c lvm_vg.c wysochanski
@ 2010-04-19 15:34 ` Zdenek Kabelac
2010-04-19 17:06 ` Alasdair G Kergon
2010-04-19 19:34 ` Dave Wysochanski
0 siblings, 2 replies; 6+ messages in thread
From: Zdenek Kabelac @ 2010-04-19 15:34 UTC (permalink / raw)
To: lvm-devel
On 19.4.2010 17:22, wysochanski at sourceware.org wrote:
> CVSROOT: /cvs/lvm2
> Module name: LVM2
> Changes by: wysochanski at sourceware.org 2010-04-19 15:22:24
>
> Modified files:
> liblvm : lvm2app.h lvm_lv.c lvm_pv.c lvm_vg.c
>
> Log message:
> Use vg->vgmem to allocate vg/lv/pv string properties instead of dm_malloc/fr
>
> 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 <dwysocha@redhat.com>
> -char *lvm_vg_get_name(const vg_t vg)
> +const 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);
> }
Either we should return const char* return vg->name;
or we should char* return strdup(vg->name) and user could do whatever he
wants to do with the string.
I'd prefer 1.) for efficiency, but I don't see any point in current code.
Usually user wants to make a quick check for something - he will use const
char*, or he want to keep it for later and he fill duplicate string with his
own memory allocation routines (i.e. C++ new.... )
Zdenek
^ permalink raw reply [flat|nested] 6+ messages in thread
* LVM2/liblvm lvm2app.h lvm_lv.c lvm_pv.c lvm_vg.c
2010-04-19 15:34 ` Zdenek Kabelac
@ 2010-04-19 17:06 ` Alasdair G Kergon
2010-04-19 18:38 ` Zdenek Kabelac
2010-04-19 19:34 ` Dave Wysochanski
1 sibling, 1 reply; 6+ messages in thread
From: Alasdair G Kergon @ 2010-04-19 17:06 UTC (permalink / raw)
To: lvm-devel
On Mon, Apr 19, 2010 at 05:34:04PM +0200, Zdenek Kabelac wrote:
> Either we should return const char* return vg->name;
But then we have to guarantee that vg->name remains const - which we are
unable to do. (We can't impose limits on vgrename.)
Alasdair
^ permalink raw reply [flat|nested] 6+ messages in thread
* LVM2/liblvm lvm2app.h lvm_lv.c lvm_pv.c lvm_vg.c
2010-04-19 17:06 ` Alasdair G Kergon
@ 2010-04-19 18:38 ` Zdenek Kabelac
2010-04-19 18:47 ` Alasdair G Kergon
0 siblings, 1 reply; 6+ messages in thread
From: Zdenek Kabelac @ 2010-04-19 18:38 UTC (permalink / raw)
To: lvm-devel
On 19.4.2010 19:06, Alasdair G Kergon wrote:
> On Mon, Apr 19, 2010 at 05:34:04PM +0200, Zdenek Kabelac wrote:
>> Either we should return const char* return vg->name;
>
> But then we have to guarantee that vg->name remains const - which we are
> unable to do. (We can't impose limits on vgrename.)
>
I think we may state, that until next call of any lvm library function pointer
remains valid. Thus if user needs it for something later - he needs to make a
local copy.
We may be nicer if we list functions which could change this pointer (i.e. I
wouldn't expect any *_get_* function could ever change it) - but I think it's
not needed - major use case would be to check value for something
and eventually copy somewhere - thus const char* is perfectly usable in this
case. So I think any const char* is valid between lvm calls - and if they are
valid after - it's just pure luck....
Zdenek
^ permalink raw reply [flat|nested] 6+ messages in thread
* LVM2/liblvm lvm2app.h lvm_lv.c lvm_pv.c lvm_vg.c
2010-04-19 18:38 ` Zdenek Kabelac
@ 2010-04-19 18:47 ` Alasdair G Kergon
0 siblings, 0 replies; 6+ messages in thread
From: Alasdair G Kergon @ 2010-04-19 18:47 UTC (permalink / raw)
To: lvm-devel
On Mon, Apr 19, 2010 at 08:38:16PM +0200, Zdenek Kabelac wrote:
> wouldn't expect any *_get_* function could ever change it
Yes it could - the vgrename might have been run on a different machine in the
cluster.
Alasdair
^ permalink raw reply [flat|nested] 6+ messages in thread
* LVM2/liblvm lvm2app.h lvm_lv.c lvm_pv.c lvm_vg.c
2010-04-19 15:34 ` Zdenek Kabelac
2010-04-19 17:06 ` Alasdair G Kergon
@ 2010-04-19 19:34 ` Dave Wysochanski
1 sibling, 0 replies; 6+ messages in thread
From: Dave Wysochanski @ 2010-04-19 19:34 UTC (permalink / raw)
To: lvm-devel
On Mon, 2010-04-19 at 17:34 +0200, Zdenek Kabelac wrote:
> On 19.4.2010 17:22, wysochanski at sourceware.org wrote:
> > CVSROOT: /cvs/lvm2
> > Module name: LVM2
> > Changes by: wysochanski at sourceware.org 2010-04-19 15:22:24
> >
> > Modified files:
> > liblvm : lvm2app.h lvm_lv.c lvm_pv.c lvm_vg.c
> >
> > Log message:
> > Use vg->vgmem to allocate vg/lv/pv string properties instead of dm_malloc/fr
> >
> > 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 <dwysocha@redhat.com>
>
> > -char *lvm_vg_get_name(const vg_t vg)
> > +const 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);
> > }
>
>
> Either we should return const char* return vg->name;
> or we should char* return strdup(vg->name) and user could do whatever he
> wants to do with the string.
>
> I'd prefer 1.) for efficiency, but I don't see any point in current code.
> Usually user wants to make a quick check for something - he will use const
> char*, or he want to keep it for later and he fill duplicate string with his
> own memory allocation routines (i.e. C++ new.... )
>
I thought I answered this in my other mail, but you never responded.
I guess you are not as concerned about a misbehaving application as I
am. Returning pointers to internal LVM data seems dangerous to me, but
perhaps I'm overly paranoid.
Indeed, 'const' should be enough to tell the application "this is a
snapshot of the present state, don't modify it". But what if he does -
then we have a potential for corrupt metadata. I'd like to take #1
approach, but it just doesn't seem safe.
If you copy memory but don't use 'const', then I would think this sends
the wrong message to applications using the API.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2010-04-19 19:34 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-04-19 15:22 LVM2/liblvm lvm2app.h lvm_lv.c lvm_pv.c lvm_vg.c wysochanski
2010-04-19 15:34 ` Zdenek Kabelac
2010-04-19 17:06 ` Alasdair G Kergon
2010-04-19 18:38 ` Zdenek Kabelac
2010-04-19 18:47 ` Alasdair G Kergon
2010-04-19 19:34 ` Dave Wysochanski
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.