[lvm-devel] [PATCH 7/7] Add lvm_vg_create() - vg constructor API for new volume groups.

[Dave Wysochanski]

On Tue, 2009-07-07 at 12:34 +0200, Milan Broz wrote:
> Dave Wysochanski wrote:
> > vg_t *lvm_vg_create(struct cmd_context *cmd, const char *vg_name);
> > This is the first step towards the API called to create a VG.
> > Call vg_lock_newname() inside this function.  Use _vg_make_handle()
> > where possible.
> 
> There is too many changes in this patch...
> 
Ok, I will try to explain better and split if necessary.

> > Now we have 2 ways to construct a volume group:
> > 1) vg_read* APIs: Used when constructing an existing VG from disks
> > 2) lvm_vg_create: Used when constructing a new VG
> > Both of these interfaces obtain a lock, and return a vg_t *.
> > The usage of _vg_make_handle() inside lvm_vg_create() doesn't fit
> > perfectly but it's ok for now.  Needs some cleanup though and I've
> > noted "FIXME" in the code.
> 
> so no real difference from vg_read_* + vg_create, right?
> These functions just returns vg_t instead of vg now...
> 

Main difference is the original vg_create() took the initial vg
parameters.  In our discussions in Berlin about liblvm, we decided we
wanted a constructor with default parameters.  Now all along there has
been the requirement that the tools call the library.  So I decided to
rework the two places with this new constructor.  Because of this, there
are subtleties that I had to fix.  As you point out

> > Replace vg_create() with lvm_vg_create() plus vg 'set' functions
> > in the following tools:
> 
> so all lib functions will use lvm_* prefix?
> 

Anything we export has to - just as libdevmapper prefixes with dm_*.

> 
> > TODO in future patches:
> > 1. The VG_ORPHAN lock needs some thought.  We may want to treat
> > this as any other VG, and require the application to obtain a handle
> > and pass it to other API calls (for example, vg_extend).  Or,
> > we may find that hiding the VG_ORPHAN lock inside other APIs is
> > the way to go.  I thought of placing the VG_ORPHAN lock inside
> > lvm_vg_create() and tying it to the vg handle, but was not certain
> > this was the right approach.
> 
> I thought that we will want to deprecate orphan group, but it still
> need to be supported (old PVs without VG)...
> 
> But I think that application using liblvm
> should never use separate handle to VG_ORPHAN.
> 

That's what I thought too - it should be managed under the covers
somehow.


> > +vg_t *lvm_vg_create(struct cmd_context *cmd, const char *vg_name)
> >  {
> > -	struct volume_group *vg;
> > +	vg_t *vg;
> >  	int consistent = 0;
> >  	struct dm_pool *mem;
> > +	uint32_t rc;
> >  
> > +	if (!validate_name(vg_name)) {
> 
> I expect that this call is just moved here from ... ?
> 

Yes - you need to validate the name if you're going to export the
function.  The original vg_create() was called from tools who had
already validated the name, so no name validation was necessary.  We
cannot assume that an application has done that though.

> > +		log_error("Invalid vg name %s", vg_name);
> > +		/* FIXME: use _vg_make_handle() w/proper error code */
> > +		return NULL;
> > +	}
> > +
> > +	rc = vg_lock_newname(cmd, vg_name);
> > +	if (rc != SUCCESS) {
> > +		log_error("Unable to reserve vg name %s", vg_name);
> 
> - log_error("Can't get lock for %s", vp_new.vg_name);
> 
> "reserve" ? it means that vg already exist?
> (both messages are confusing to user imho)
> 

I could check for each error code (FAILED_LOCKING and FAILED_EXIST) and
print a specific message as is done in other places.


> > +		return _vg_make_handle(cmd, NULL, rc);
> > +	}
> > +
> > +	/* FIXME: Is this vg_read_internal necessary? Move it inside
> > +	   vg_lock_newname? */
> >  	/* is this vg name already in use ? */
> >  	if ((vg = vg_read_internal(cmd, vg_name, NULL, &consistent))) {
> >  		log_err("A volume group called '%s' already exists.", vg_name);
> 
> btw this should be log_error
> 

Right - I think that was an existing bug.

> > -		vg_release(vg);
> > -		return NULL;
> > +		unlock_and_release_vg(cmd, vg, vg_name);
> > +		return _vg_make_handle(cmd, NULL, FAILED_EXIST);
> 
> so now it calls unlock too... it is bugfix for some previous change?
> 

No.  Remember I am defining a new function.  If it fails I release the
lock.  The lock has been obtained earlier in vg_lock_newname() so I must
release it.

> > @@ -559,14 +583,14 @@ struct volume_group *vg_create(struct cmd_context *cmd, const char *vg_name,
> >  
> >  	*vg->system_id = '\0';
> >  
> > -	vg->extent_size = extent_size;
> > +	vg->extent_size = DEFAULT_EXTENT_SIZE * 2;
> 
> what these forced defaults mean here? just to overwrite them later?
> can we read them form command context instead of fixed values?
> 
> (not that user can change policy using --alloc for example)
> 

We could store them in cmd context.  Forced defaults should just be the
same as if the user did not give any values for them.

> >  	vg->extent_count = 0;
> >  	vg->free_count = 0;
> >  
> > -	vg->max_lv = max_lv;
> > -	vg->max_pv = max_pv;
> > +	vg->max_lv = 0;
> > +	vg->max_pv = 0;
> 
> ?
> 
> >  
> > -	vg->alloc = alloc;
> > +	vg->alloc = ALLOC_NORMAL;
> 
> ?
> 

These defaults were taken from the initialization code - when we're
parsing ARGS and getting default values if there are no arguments.


> > @@ -2779,6 +2799,7 @@ static vg_t *_vg_make_handle(struct cmd_context *cmd,
> >  			return_NULL;
> >  		}
> >  		vg->vgmem = vgmem;
> > +		vg->cmd = cmd;
> 
> another bugfix?
> 

Yes though could be solely the result of this refactoring, can't
remember.

> > @@ -322,23 +321,28 @@ int vgsplit(struct cmd_context *cmd, int argc, char **argv)
> >  		return ECMD_FAILED;
> >  	}
> >  
> > +	/* Set metadata format of original VG */
> > +	/* FIXME: need some common logic */
> > +	cmd->fmt = vg_from->fid->fmt;
> > +
> 
> and what happens after vg_release(vg_from) ? cmd->fmt will be undefined?
> (this is probably bug in current code though)
> 

This should never happen AFAIK.  cmd->fmt is set early on, and there is
a default.


> > +	vg_to = lvm_vg_create(cmd, vg_name_to);
> > +	if (vg_read_error(vg_to) == FAILED_LOCKING) {
> ...
> > +	if (vg_read_error(vg_to) == FAILED_EXIST) {
> ...
> > +	} else if (vg_read_error(vg_to) == SUCCESS) {
> ...
> > +		if (!vg_set_extent_size(vg_to, vp_new.extent_size) ||
> > +		    !vg_set_max_lv(vg_to, vp_new.max_lv) ||
> > +		    !vg_set_max_pv(vg_to, vp_new.max_pv) ||
> > +		    !vg_set_alloc(vg_to, vp_new.alloc))
> >  			goto_bad;
> 
> I like the old way much more better...
> 

So if we keep the old way, then I have a liblvm function that has a
default constructor and none of the below parameters.  So the existing
tools code cannot call it.

> > -		if (!(vg_to = vg_create(cmd, vg_name_to, vp_new.extent_size,
> > -					vp_new.max_pv, vp_new.max_lv,
> > -					vp_new.alloc, 0, NULL)))
> 
> 
> Milan
> 
> --
> lvm-devel mailing list
> lvm-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/lvm-devel