[lvm-devel] [PATCH 7/7] Add lvm_vg_create() - vg constructor API for new volume groups.
Milan Broz
mbroz at redhat.com
Tue Jul 7 10:34:27 UTC 2009
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...
> 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...
> Replace vg_create() with lvm_vg_create() plus vg 'set' functions
> in the following tools:
so all lib functions will use lvm_* prefix?
> 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.
> +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 ... ?
> + 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)
> + 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
> - 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?
> @@ -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)
> 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;
?
> @@ -2779,6 +2799,7 @@ static vg_t *_vg_make_handle(struct cmd_context *cmd,
> return_NULL;
> }
> vg->vgmem = vgmem;
> + vg->cmd = cmd;
another bugfix?
> @@ -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)
> + 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...
> - 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
More information about the lvm-devel
mailing list