[lvm-devel] [PATCH 6/6] RFC: Change lvm2app version number from 1 to 2.

[Dave Wysochanski]

On Fri, 2010-04-02 at 13:39 +0200, Milan Broz wrote:
> On 04/01/2010 11:57 PM, Dave Wysochanski wrote:
> > 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.
> 

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.


> 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.


> 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.
> 

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.  


> 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.