[lvm-devel] [PATCH 05/12] Add pv_uuid, vg_uuid, and lv_uuid, and call id_format_and_copy.

[Dave Wysochanski]

On Fri, 2010-09-10 at 11:38 +0200, Zdenek Kabelac wrote:
> Dne 9.9.2010 22:13, Dave Wysochanski napsal(a):
> > Add supporting functions for pv_uuid, vg_uuid, and lv_uuid.
> > Call new function id_format_and_copy.
> > 
> > Signed-off-by: Dave Wysochanski <dwysocha at redhat.com>
> > ---
> >  lib/metadata/metadata.c |   15 +++++++++++++++
> >  lib/metadata/metadata.h |    3 +++
> >  liblvm/lvm_lv.c         |   10 ++--------
> >  liblvm/lvm_pv.c         |   10 ++--------
> >  liblvm/lvm_vg.c         |    8 +-------
> >  5 files changed, 23 insertions(+), 23 deletions(-)
> > 
> > diff --git a/lib/metadata/metadata.c b/lib/metadata/metadata.c
> > index 86f07e7..6fb69dc 100644
> > --- a/lib/metadata/metadata.c
> > +++ b/lib/metadata/metadata.c
> > @@ -4619,6 +4619,21 @@ char *pv_attr(struct dm_pool *mem, const struct physical_volume *pv)
> >  	return repstr;
> >  }
> >  
> > +char *pv_uuid(struct physical_volume *pv)
> > +{
> > +	return id_format_and_copy(pv->vg->vgmem, &pv->id);
> > +}
> > +
> > +char *vg_uuid(struct volume_group *vg)
> > +{
> > +	return id_format_and_copy(vg->vgmem, &vg->id);
> > +}
> > +
> > +char *lv_uuid(struct logical_volume *lv)
> > +{
> > +	return id_format_and_copy(lv->vg->vgmem, &lv->lvid.id[1]);
> > +}
> > +
> >  static int _lv_mimage_in_sync(const struct logical_volume *lv)
> >  {
> >  	float percent;
> > diff --git a/lib/metadata/metadata.h b/lib/metadata/metadata.h
> > index 1749f85..4eb826a 100644
> > --- a/lib/metadata/metadata.h
> > +++ b/lib/metadata/metadata.h
> > @@ -422,5 +422,8 @@ uint64_t vg_mda_free(const struct volume_group *vg);
> >  char *pv_attr(struct dm_pool *mem, const struct physical_volume *pv);
> >  char *vg_attr(struct dm_pool *mem, const struct volume_group *vg);
> >  char *lv_attr(struct dm_pool *mem, const struct logical_volume *lv);
> > +char *lv_uuid(struct logical_volume *lv);
> > +char *vg_uuid(struct volume_group *vg);
> > +char *pv_uuid(struct physical_volume *pv);
> 
> Again - 'const'for *lv, *vg, *pv  would fit here probably better.
> 

Updated in my patches - thanks.

> >  const char *lvm_lv_get_uuid(const lv_t lv)
> >  {
> > -	char uuid[64] __attribute__((aligned(8)));
> > -
> > -	if (!id_write_format(&lv->lvid.id[1], uuid, sizeof(uuid))) {
> 
> 
> Probably not a big issue, but I can see some 'inconsistency' -
> 
> It looks like we have put 'const' on this return value to mark value as a
> string which user should not try to 'free' - but this is somewhat missuse of
> 'const'  - there is probably no reason to avoid modification of copied string.
> As we do not keep it internally - it just allocated string and user should be
> able to do anything he wants to do with it.
> 
> Maybe we should reconsider and relax this return values to plain char*.
> 

Well, I don't think we want users modifying the uuid's returned by these
functions.  I think const sends the right message.

Thanks for taking the time to review the patches.