[PATCH 1/2] Always query device by uuid only.

[PATCH 1/2] Always query device by uuid only.

[Milan Broz]

lvm2 devices have always UUID set even if imported from lvm1 metadata.

Patch removes name argument from dev_manager_info call and converts
all activation related calls to use query by UUID.

Also it simplifies mknode call (which is the only user on mknodes parameter).
---
 lib/activate/activate.c    |   48 ++++++++++++++-----------------------------
 lib/activate/dev_manager.c |   33 ++++++++++++++++++++----------
 lib/activate/dev_manager.h |    3 +-
 3 files changed, 39 insertions(+), 45 deletions(-)

diff --git a/lib/activate/activate.c b/lib/activate/activate.c
index 433c8cc..3c7105b 100644
--- a/lib/activate/activate.c
+++ b/lib/activate/activate.c
@@ -440,28 +440,18 @@ int target_present(struct cmd_context *cmd, const char *target_name,
 /*
  * Returns 1 if info structure populated, else 0 on failure.
  */
-static int _lv_info(struct cmd_context *cmd, const struct logical_volume *lv, int with_mknodes,
-		    struct lvinfo *info, int with_open_count, int with_read_ahead, unsigned by_uuid_only)
+int lv_info(struct cmd_context *cmd, const struct logical_volume *lv,
+	    struct lvinfo *info, int with_open_count, int with_read_ahead)
 {
 	struct dm_info dminfo;
-	char *name = NULL;
 
 	if (!activation())
 		return 0;
 
-	if (!by_uuid_only &&
-	    !(name = build_dm_name(cmd->mem, lv->vg->name, lv->name, NULL)))
+	if (!dev_manager_info(lv->vg->cmd->mem, lv, 0, with_open_count,
+			      with_read_ahead, &dminfo, &info->read_ahead))
 		return_0;
 
-	log_debug("Getting device info for %s", name);
-	if (!dev_manager_info(lv->vg->cmd->mem, name, lv, with_mknodes,
-			      with_open_count, with_read_ahead, &dminfo,
-			      &info->read_ahead)) {
-		if (name)
-			dm_pool_free(cmd->mem, name);
-		return_0;
-	}
-
 	info->exists = dminfo.exists;
 	info->suspended = dminfo.suspended;
 	info->open_count = dminfo.open_count;
@@ -471,18 +461,9 @@ static int _lv_info(struct cmd_context *cmd, const struct logical_volume *lv, in
 	info->live_table = dminfo.live_table;
 	info->inactive_table = dminfo.inactive_table;
 
-	if (name)
-		dm_pool_free(cmd->mem, name);
-
 	return 1;
 }
 
-int lv_info(struct cmd_context *cmd, const struct logical_volume *lv, struct lvinfo *info,
-	    int with_open_count, int with_read_ahead)
-{
-	return _lv_info(cmd, lv, 0, info, with_open_count, with_read_ahead, 0);
-}
-
 int lv_info_by_lvid(struct cmd_context *cmd, const char *lvid_s,
 		    struct lvinfo *info, int with_open_count, int with_read_ahead)
 {
@@ -492,7 +473,7 @@ int lv_info_by_lvid(struct cmd_context *cmd, const char *lvid_s,
 	if (!(lv = lv_from_lvid(cmd, lvid_s, 0)))
 		return 0;
 
-	r = _lv_info(cmd, lv, 0, info, with_open_count, with_read_ahead, 1);
+	r = lv_info(cmd, lv, info, with_open_count, with_read_ahead);
 	vg_release(lv->vg);
 
 	return r;
@@ -558,12 +539,11 @@ int lv_mirror_percent(struct cmd_context *cmd, struct logical_volume *lv,
 	return r;
 }
 
-static int _lv_active(struct cmd_context *cmd, struct logical_volume *lv,
-		      unsigned by_uuid_only)
+static int _lv_active(struct cmd_context *cmd, struct logical_volume *lv)
 {
 	struct lvinfo info;
 
-	if (!_lv_info(cmd, lv, 0, &info, 0, 0, by_uuid_only)) {
+	if (!lv_info(cmd, lv, &info, 0, 0)) {
 		stack;
 		return -1;
 	}
@@ -657,7 +637,7 @@ static int _lvs_in_vg_activated(struct volume_group *vg, unsigned by_uuid_only)
 
 	dm_list_iterate_items(lvl, &vg->lvs) {
 		if (lv_is_visible(lvl->lv))
-			count += (_lv_active(vg->cmd, lvl->lv, by_uuid_only) == 1);
+			count += (_lv_active(vg->cmd, lvl->lv) == 1);
 	}
 
 	return count;
@@ -700,7 +680,7 @@ int lv_is_active(struct logical_volume *lv)
 {
 	int ret;
 
-	if (_lv_active(lv->vg->cmd, lv, 0))
+	if (_lv_active(lv->vg->cmd, lv))
 		return 1;
 
 	if (!vg_is_clustered(lv->vg))
@@ -1182,7 +1162,7 @@ int lv_activate_with_filter(struct cmd_context *cmd, const char *lvid_s, int exc
 
 int lv_mknodes(struct cmd_context *cmd, const struct logical_volume *lv)
 {
-	struct lvinfo info;
+	struct dm_info dminfo;
 	int r = 1;
 
 	if (!lv) {
@@ -1191,10 +1171,14 @@ int lv_mknodes(struct cmd_context *cmd, const struct logical_volume *lv)
 		return r;
 	}
 
-	if (!_lv_info(cmd, lv, 1, &info, 0, 0, 0))
+	if (!activation())
+		return 0;
+
+	if (!dev_manager_info(lv->vg->cmd->mem, lv, 1,
+			      0, 0, &dminfo, NULL))
 		return_0;
 
-	if (info.exists) {
+	if (dminfo.exists) {
 		if (lv_is_visible(lv))
 			r = dev_manager_lv_mknodes(lv);
 	} else
diff --git a/lib/activate/dev_manager.c b/lib/activate/dev_manager.c
index bbf95a3..84f94ff 100644
--- a/lib/activate/dev_manager.c
+++ b/lib/activate/dev_manager.c
@@ -123,12 +123,14 @@ static int _info_run(const char *name, const char *dlid, struct dm_info *info,
 {
 	int r = 0;
 	struct dm_task *dmt;
-	int dmtask;
 
-	dmtask = mknodes ? DM_DEVICE_MKNODES : DM_DEVICE_INFO;
-
-	if (!(dmt = _setup_task(name, dlid, 0, dmtask, major, minor)))
-		return_0;
+	if (mknodes) {
+		if (!(dmt = _setup_task(name, dlid, 0, DM_DEVICE_MKNODES, major, minor)))
+			return_0;
+	} else {
+		if (!(dmt = _setup_task(NULL, dlid, 0, DM_DEVICE_INFO, major, minor)))
+			return_0;
+	}
 
 	if (!with_open_count)
 		if (!dm_task_no_open_count(dmt))
@@ -235,20 +237,29 @@ static int _info_by_dev(uint32_t major, uint32_t minor, struct dm_info *info)
 	return _info_run(NULL, NULL, info, NULL, 0, 0, 0, major, minor);
 }
 
-int dev_manager_info(struct dm_pool *mem, const char *name,
-		     const struct logical_volume *lv, int with_mknodes,
-		     int with_open_count, int with_read_ahead,
+int dev_manager_info(struct dm_pool *mem, const struct logical_volume *lv,
+		     int with_mknodes, int with_open_count, int with_read_ahead,
 		     struct dm_info *info, uint32_t *read_ahead)
 {
-	const char *dlid;
+	const char *dlid, *name;
+	int r;
+
+	if (!(name = build_dm_name(mem, lv->vg->name, lv->name, NULL))) {
+		log_error("name build failed for %s", lv->name);
+		return 0;
+	}
 
 	if (!(dlid = _build_dlid(mem, lv->lvid.s, NULL))) {
 		log_error("dlid build failed for %s", lv->name);
 		return 0;
 	}
 
-	return _info(name, dlid, with_mknodes, with_open_count, with_read_ahead,
-		     info, read_ahead);
+	log_debug("Getting device info for %s", name);
+	r = _info(NULL, dlid, with_mknodes, with_open_count,
+		  with_read_ahead, info, read_ahead);
+
+	dm_pool_free(mem, (char*)name);
+	return r;
 }
 
 static const struct dm_info *_cached_info(struct dm_pool *mem,
diff --git a/lib/activate/dev_manager.h b/lib/activate/dev_manager.h
index 5333544..86c978f 100644
--- a/lib/activate/dev_manager.h
+++ b/lib/activate/dev_manager.h
@@ -40,8 +40,7 @@ void dev_manager_exit(void);
  * (eg, an origin is created before its snapshot, but is not
  * unsuspended until the snapshot is also created.)
  */
-int dev_manager_info(struct dm_pool *mem, const char *name,
-		     const struct logical_volume *lv,
+int dev_manager_info(struct dm_pool *mem, const struct logical_volume *lv,
 		     int mknodes, int with_open_count, int with_read_ahead,
 		     struct dm_info *info, uint32_t *read_ahead);
 int dev_manager_snapshot_percent(struct dev_manager *dm,
-- 
1.7.0

[PATCH 2/2] Remove lvs_in_vg_activated_by_uuid_only call.

[Milan Broz]

THere is no difference from lvs_in_vg_activated now,
convert all users to this call.
---
 lib/activate/activate.c |   16 +---------------
 lib/activate/activate.h |    1 -
 tools/vgrename.c        |    4 ++--
 3 files changed, 3 insertions(+), 18 deletions(-)

diff --git a/lib/activate/activate.c b/lib/activate/activate.c
index 3c7105b..dcdad73 100644
--- a/lib/activate/activate.c
+++ b/lib/activate/activate.c
@@ -171,10 +171,6 @@ int lvs_in_vg_activated(struct volume_group *vg)
 {
 	return 0;
 }
-int lvs_in_vg_activated_by_uuid_only(struct volume_group *vg)
-{
-	return 0;
-}
 int lvs_in_vg_opened(struct volume_group *vg)
 {
 	return 0;
@@ -627,7 +623,7 @@ static int _lv_suspend_lv(struct logical_volume *lv, int lockfs, int flush_requi
  * These two functions return the number of visible LVs in the state,
  * or -1 on error.
  */
-static int _lvs_in_vg_activated(struct volume_group *vg, unsigned by_uuid_only)
+int lvs_in_vg_activated(struct volume_group *vg)
 {
 	struct lv_list *lvl;
 	int count = 0;
@@ -643,16 +639,6 @@ static int _lvs_in_vg_activated(struct volume_group *vg, unsigned by_uuid_only)
 	return count;
 }
 
-int lvs_in_vg_activated_by_uuid_only(struct volume_group *vg)
-{
-	return _lvs_in_vg_activated(vg, 1);
-}
-
-int lvs_in_vg_activated(struct volume_group *vg)
-{
-	return _lvs_in_vg_activated(vg, 0);
-}
-
 int lvs_in_vg_opened(const struct volume_group *vg)
 {
 	const struct lv_list *lvl;
diff --git a/lib/activate/activate.h b/lib/activate/activate.h
index ba70372..32d9a5c 100644
--- a/lib/activate/activate.h
+++ b/lib/activate/activate.h
@@ -91,7 +91,6 @@ int lv_mirror_percent(struct cmd_context *cmd, struct logical_volume *lv,
  * Return number of LVs in the VG that are active.
  */
 int lvs_in_vg_activated(struct volume_group *vg);
-int lvs_in_vg_activated_by_uuid_only(struct volume_group *vg);
 int lvs_in_vg_opened(const struct volume_group *vg);
 
 int lv_is_active(struct logical_volume *lv);
diff --git a/tools/vgrename.c b/tools/vgrename.c
index faf20bc..a63a626 100644
--- a/tools/vgrename.c
+++ b/tools/vgrename.c
@@ -29,7 +29,7 @@ static struct volume_group *vg_rename_old(struct cmd_context *cmd,
 		return_NULL;
 	}
 
-	if (lvs_in_vg_activated_by_uuid_only(vg)) {
+	if (lvs_in_vg_activated(vg)) {
 		unlock_and_release_vg(cmd, vg, vg_name_old);
 		log_error("Volume group \"%s\" still has active LVs",
 			  vg_name_old);
@@ -161,7 +161,7 @@ static int vg_rename_path(struct cmd_context *cmd, const char *old_vg_path,
 		if (test_mode())
 			log_verbose("Test mode: Skipping rename.");
 
-		else if (lvs_in_vg_activated_by_uuid_only(vg)) {
+		else if (lvs_in_vg_activated(vg)) {
 			if (!vg_refresh_visible(cmd, vg)) {
 				log_error("Renaming \"%s\" to \"%s\" failed", 
 					old_path, new_path);
-- 
1.7.0

[PATCH 1/2] Always query device by uuid only.

[Mike Snitzer]

On Tue, Feb 23 2010 at  8:46am -0500,
Milan Broz <mbroz@redhat.com> wrote:

> lvm2 devices have always UUID set even if imported from lvm1 metadata.
> 
> Patch removes name argument from dev_manager_info call and converts
> all activation related calls to use query by UUID.
> 
> Also it simplifies mknode call (which is the only user on mknodes parameter).

Looks good, I have few questions though.

> diff --git a/lib/activate/dev_manager.c b/lib/activate/dev_manager.c
> index bbf95a3..84f94ff 100644
> --- a/lib/activate/dev_manager.c
> +++ b/lib/activate/dev_manager.c
> @@ -235,20 +237,29 @@ static int _info_by_dev(uint32_t major, uint32_t minor, struct dm_info *info)
>  	return _info_run(NULL, NULL, info, NULL, 0, 0, 0, major, minor);
>  }
>  
> -int dev_manager_info(struct dm_pool *mem, const char *name,
> -		     const struct logical_volume *lv, int with_mknodes,
> -		     int with_open_count, int with_read_ahead,
> +int dev_manager_info(struct dm_pool *mem, const struct logical_volume *lv,
> +		     int with_mknodes, int with_open_count, int with_read_ahead,
>  		     struct dm_info *info, uint32_t *read_ahead)
>  {
> -	const char *dlid;
> +	const char *dlid, *name;
> +	int r;
> +
> +	if (!(name = build_dm_name(mem, lv->vg->name, lv->name, NULL))) {
> +		log_error("name build failed for %s", lv->name);
> +		return 0;
> +	}
>  
>  	if (!(dlid = _build_dlid(mem, lv->lvid.s, NULL))) {
>  		log_error("dlid build failed for %s", lv->name);
>  		return 0;
>  	}
>  
> -	return _info(name, dlid, with_mknodes, with_open_count, with_read_ahead,
> -		     info, read_ahead);
> +	log_debug("Getting device info for %s", name);
> +	r = _info(NULL, dlid, with_mknodes, with_open_count,
> +		  with_read_ahead, info, read_ahead);
> +
> +	dm_pool_free(mem, (char*)name);
> +	return r;
>  }

So the debugging benefit of having 'name' here outweighs the (small)
cost of build_dm_name/dm_pool_free?  Really not a big deal but figured
I'd ask.

Also, _add_dev_to_dtree() is the only remaining caller of _info() that
passes 'name'.  Passing a NULL 'name' to _info() from _add_dev_to_dtree()
still allows the testsuite to pass.  What are the benefit(s) of
preserving _info()'s info-by-name fallback for _add_dev_to_dtree()?

Mike

[PATCH 1/2] Always query device by uuid only.

[Milan Broz]

On 02/23/2010 09:39 PM, Mike Snitzer wrote:

> Also, _add_dev_to_dtree() is the only remaining caller of _info() that
> passes 'name'.  Passing a NULL 'name' to _info() from _add_dev_to_dtree()
> still allows the testsuite to pass.  What are the benefit(s) of
> preserving _info()'s info-by-name fallback for _add_dev_to_dtree()?

user for name paramater is lv_mknodes() and my patch was wrong here,
name must be still passed into that function.

_add_dev_to_dtree() should use NULL for name...

Thanks.

Milan

---
 lib/activate/dev_manager.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/activate/dev_manager.c b/lib/activate/dev_manager.c
index 84f94ff..2ec9b6f 100644
--- a/lib/activate/dev_manager.c
+++ b/lib/activate/dev_manager.c
@@ -254,8 +254,8 @@ int dev_manager_info(struct dm_pool *mem, const struct logical_volume *lv,
 		return 0;
 	}
 
-	log_debug("Getting device info for %s", name);
-	r = _info(NULL, dlid, with_mknodes, with_open_count,
+	log_debug("Getting device info for %s [%s]", name, dlid);
+	r = _info(name, dlid, with_mknodes, with_open_count,
 		  with_read_ahead, info, read_ahead);
 
 	dm_pool_free(mem, (char*)name);
@@ -785,7 +785,7 @@ static int _add_dev_to_dtree(struct dev_manager *dm, struct dm_tree *dtree,
 		return_0;
 
 	log_debug("Getting device info for %s [%s]", name, dlid);
-	if (!_info(name, dlid, 0, 1, 0, &info, NULL)) {
+	if (!_info(NULL, dlid, 0, 1, 0, &info, NULL)) {
 		log_error("Failed to get info for %s [%s].", name, dlid);
 		return 0;
 	}

[PATCH 1/2] Always query device by uuid only.

[Milan Broz]

On 02/24/2010 12:46 PM, Milan Broz wrote:
> -	log_debug("Getting device info for %s", name);
> -	r = _info(NULL, dlid, with_mknodes, with_open_count,
> +	log_debug("Getting device info for %s [%s]", name, dlid);
> +	r = _info(name, dlid, with_mknodes, with_open_count,

ok, so I did already three times the same mistake and re-introduced
fallback to name query. Name query should be only in mknodes now...

sigh. Either I am idiot or split of mknodes and info dm calls
in _info_run is confusing (or both).

I'll post another patch...

Milan

[PATCH 1/2] Always query device by uuid only.

[Milan Broz]

> sigh. Either I am idiot or split of mknodes and info dm calls
> in _info_run is confusing (or both).
>
>   
I would prefer something like this (on top of my previous 2 patches):

Separate mknodes to dev_manager_mknodes function.

All info calls uses now uuid only.

Remove confusing with_mknodes paramater.

Move all nodes handling logic inside dev_manager.
---
 lib/activate/activate.c    |   13 +--------
 lib/activate/dev_manager.c |   59 ++++++++++++++++++++++++++------------------
 lib/activate/dev_manager.h |    5 +--
 3 files changed, 39 insertions(+), 38 deletions(-)

diff --git a/lib/activate/activate.c b/lib/activate/activate.c
index dcdad73..4e43131 100644
--- a/lib/activate/activate.c
+++ b/lib/activate/activate.c
@@ -444,7 +444,7 @@ int lv_info(struct cmd_context *cmd, const struct logical_volume *lv,
 	if (!activation())
 		return 0;
 
-	if (!dev_manager_info(lv->vg->cmd->mem, lv, 0, with_open_count,
+	if (!dev_manager_info(lv->vg->cmd->mem, lv, with_open_count,
 			      with_read_ahead, &dminfo, &info->read_ahead))
 		return_0;
 
@@ -1148,7 +1148,6 @@ int lv_activate_with_filter(struct cmd_context *cmd, const char *lvid_s, int exc
 
 int lv_mknodes(struct cmd_context *cmd, const struct logical_volume *lv)
 {
-	struct dm_info dminfo;
 	int r = 1;
 
 	if (!lv) {
@@ -1160,15 +1159,7 @@ int lv_mknodes(struct cmd_context *cmd, const struct logical_volume *lv)
 	if (!activation())
 		return 0;
 
-	if (!dev_manager_info(lv->vg->cmd->mem, lv, 1,
-			      0, 0, &dminfo, NULL))
-		return_0;
-
-	if (dminfo.exists) {
-		if (lv_is_visible(lv))
-			r = dev_manager_lv_mknodes(lv);
-	} else
-		r = dev_manager_lv_rmnodes(lv);
+	r = dev_manager_mknodes(lv);
 
 	fs_unlock();
 
diff --git a/lib/activate/dev_manager.c b/lib/activate/dev_manager.c
index 84f94ff..70c0b04 100644
--- a/lib/activate/dev_manager.c
+++ b/lib/activate/dev_manager.c
@@ -207,27 +207,18 @@ int device_is_usable(dev_t dev)
 	return r;
 }
 
-static int _info(const char *name, const char *dlid, int mknodes,
-		 int with_open_count, int with_read_ahead,
+static int _info(const char *dlid, int with_open_count, int with_read_ahead,
 		 struct dm_info *info, uint32_t *read_ahead)
 {
 	int r = 0;
 
-	if (!mknodes && dlid && *dlid) {
-		if ((r = _info_run(NULL, dlid, info, read_ahead, 0, with_open_count,
-			      with_read_ahead, 0, 0)) &&
-	    	    info->exists)
-			return 1;
-		else if ((r = _info_run(NULL, dlid + sizeof(UUID_PREFIX) - 1, info,
-				   read_ahead, 0, with_open_count,
-				   with_read_ahead, 0, 0)) &&
-			 info->exists)
-			return 1;
-	}
-
-	if (name)
-		return _info_run(name, NULL, info, read_ahead, mknodes,
-				 with_open_count, with_read_ahead, 0, 0);
+	if ((r = _info_run(NULL, dlid, info, read_ahead, 0, with_open_count,
+			   with_read_ahead, 0, 0)) && info->exists)
+		return 1;
+	else if ((r = _info_run(NULL, dlid + sizeof(UUID_PREFIX) - 1, info,
+				read_ahead, 0, with_open_count,
+				with_read_ahead, 0, 0)) && info->exists)
+		return 1;
 
 	return r;
 }
@@ -238,7 +229,7 @@ static int _info_by_dev(uint32_t major, uint32_t minor, struct dm_info *info)
 }
 
 int dev_manager_info(struct dm_pool *mem, const struct logical_volume *lv,
-		     int with_mknodes, int with_open_count, int with_read_ahead,
+		     int with_open_count, int with_read_ahead,
 		     struct dm_info *info, uint32_t *read_ahead)
 {
 	const char *dlid, *name;
@@ -254,9 +245,8 @@ int dev_manager_info(struct dm_pool *mem, const struct logical_volume *lv,
 		return 0;
 	}
 
-	log_debug("Getting device info for %s", name);
-	r = _info(NULL, dlid, with_mknodes, with_open_count,
-		  with_read_ahead, info, read_ahead);
+	log_debug("Getting device info for %s [%s]", name, dlid);
+	r = _info(dlid, with_open_count, with_read_ahead, info, read_ahead);
 
 	dm_pool_free(mem, (char*)name);
 	return r;
@@ -755,7 +745,7 @@ static int _belong_to_vg(const char *vgname, const char *name)
 /*  NEW CODE STARTS HERE */
 /*************************/
 
-int dev_manager_lv_mknodes(const struct logical_volume *lv)
+static int dev_manager_lv_mknodes(const struct logical_volume *lv)
 {
 	char *name;
 
@@ -766,11 +756,32 @@ int dev_manager_lv_mknodes(const struct logical_volume *lv)
 	return fs_add_lv(lv, name);
 }
 
-int dev_manager_lv_rmnodes(const struct logical_volume *lv)
+static int dev_manager_lv_rmnodes(const struct logical_volume *lv)
 {
 	return fs_del_lv(lv);
 }
 
+int dev_manager_mknodes(const struct logical_volume *lv)
+{
+	struct dm_info dminfo;
+	const char *name;
+	int r = 0;
+
+	if (!(name = build_dm_name(lv->vg->cmd->mem, lv->vg->name, lv->name, NULL)))
+		return_0;
+
+	if ((r = _info_run(name, NULL, &dminfo, NULL, 1, 0, 0, 0, 0))) {
+		if (dminfo.exists) {
+			if (lv_is_visible(lv))
+				r = dev_manager_lv_mknodes(lv);
+		} else
+			r = dev_manager_lv_rmnodes(lv);
+	}
+
+	dm_pool_free(lv->vg->cmd->mem, (char*)name);
+	return r;
+}
+
 static int _add_dev_to_dtree(struct dev_manager *dm, struct dm_tree *dtree,
 			       struct logical_volume *lv, const char *layer)
 {
@@ -785,7 +796,7 @@ static int _add_dev_to_dtree(struct dev_manager *dm, struct dm_tree *dtree,
 		return_0;
 
 	log_debug("Getting device info for %s [%s]", name, dlid);
-	if (!_info(name, dlid, 0, 1, 0, &info, NULL)) {
+	if (!_info(dlid, 1, 0, &info, NULL)) {
 		log_error("Failed to get info for %s [%s].", name, dlid);
 		return 0;
 	}
diff --git a/lib/activate/dev_manager.h b/lib/activate/dev_manager.h
index 86c978f..e946aaf 100644
--- a/lib/activate/dev_manager.h
+++ b/lib/activate/dev_manager.h
@@ -41,7 +41,7 @@ void dev_manager_exit(void);
  * unsuspended until the snapshot is also created.)
  */
 int dev_manager_info(struct dm_pool *mem, const struct logical_volume *lv,
-		     int mknodes, int with_open_count, int with_read_ahead,
+		     int with_open_count, int with_read_ahead,
 		     struct dm_info *info, uint32_t *read_ahead);
 int dev_manager_snapshot_percent(struct dev_manager *dm,
 				 const struct logical_volume *lv,
@@ -58,8 +58,7 @@ int dev_manager_preload(struct dev_manager *dm, struct logical_volume *lv,
 			int *flush_required);
 int dev_manager_deactivate(struct dev_manager *dm, struct logical_volume *lv);
 
-int dev_manager_lv_mknodes(const struct logical_volume *lv);
-int dev_manager_lv_rmnodes(const struct logical_volume *lv);
+int dev_manager_mknodes(const struct logical_volume *lv);
 
 /*
  * Put the desired changes into effect.

[PATCH 1/2] Always query device by uuid only.

[Mike Snitzer]

On Wed, Feb 24 2010 at  7:55am -0500,
Milan Broz <mbroz@redhat.com> wrote:

> 
> > sigh. Either I am idiot or split of mknodes and info dm calls
> > in _info_run is confusing (or both).
> >
> >   
> I would prefer something like this (on top of my previous 2 patches):
> 
> Separate mknodes to dev_manager_mknodes function.
> 
> All info calls uses now uuid only.
> 
> Remove confusing with_mknodes paramater.
> 
> Move all nodes handling logic inside dev_manager.

Cleans things up nicely, thanks.

Acked-by: Mike Snitzer <snitzer@redhat.com>

[PATCH 1/2] Always query device by uuid only.

[Petr Rockai]

Milan Broz <mbroz@redhat.com> writes:
> --- a/lib/activate/dev_manager.c
> +++ b/lib/activate/dev_manager.c
> @@ -123,12 +123,14 @@ static int _info_run(const char *name, const char *dlid, struct dm_info *info,
>  {
>  	int r = 0;
>  	struct dm_task *dmt;
> -	int dmtask;
>  
> -	dmtask = mknodes ? DM_DEVICE_MKNODES : DM_DEVICE_INFO;
> -
> -	if (!(dmt = _setup_task(name, dlid, 0, dmtask, major, minor)))
> -		return_0;
> +	if (mknodes) {
> +		if (!(dmt = _setup_task(name, dlid, 0, DM_DEVICE_MKNODES, major, minor)))
> +			return_0;
> +	} else {
> +		if (!(dmt = _setup_task(NULL, dlid, 0, DM_DEVICE_INFO, major, minor)))
> +			return_0;
> +	}

Can we instead have

if (!(dmt = _setup_task(mknodes ? name : NULL, dlid, 0, dmtask, major, minor)))
	return_0;

please?

Otherwise, ACK.

[PATCH 1/2] Always query device by uuid only.

[Petr Rockai]

Hi,

Milan Broz <mbroz@redhat.com> writes:
> diff --git a/lib/activate/activate.c b/lib/activate/activate.c
> index dcdad73..4e43131 100644
> --- a/lib/activate/activate.c
> +++ b/lib/activate/activate.c
> @@ -444,7 +444,7 @@ int lv_info(struct cmd_context *cmd, const struct logical_volume *lv,
>  	if (!activation())
>  		return 0;
>  
> -	if (!dev_manager_info(lv->vg->cmd->mem, lv, 0, with_open_count,
> +	if (!dev_manager_info(lv->vg->cmd->mem, lv, with_open_count,
>  			      with_read_ahead, &dminfo, &info->read_ahead))
>  		return_0;
>  
> @@ -1148,7 +1148,6 @@ int lv_activate_with_filter(struct cmd_context *cmd, const char *lvid_s, int exc
>  
>  int lv_mknodes(struct cmd_context *cmd, const struct logical_volume *lv)
>  {
> -	struct dm_info dminfo;
>  	int r = 1;
>  
>  	if (!lv) {
> @@ -1160,15 +1159,7 @@ int lv_mknodes(struct cmd_context *cmd, const struct logical_volume *lv)
>  	if (!activation())
>  		return 0;
>  
> -	if (!dev_manager_info(lv->vg->cmd->mem, lv, 1,
> -			      0, 0, &dminfo, NULL))
> -		return_0;
> -
> -	if (dminfo.exists) {
> -		if (lv_is_visible(lv))
> -			r = dev_manager_lv_mknodes(lv);
> -	} else
> -		r = dev_manager_lv_rmnodes(lv);
> +	r = dev_manager_mknodes(lv);
>  
>  	fs_unlock();
OK.

> diff --git a/lib/activate/dev_manager.c b/lib/activate/dev_manager.c
> index 84f94ff..70c0b04 100644
> --- a/lib/activate/dev_manager.c
> +++ b/lib/activate/dev_manager.c
> @@ -207,27 +207,18 @@ int device_is_usable(dev_t dev)
>  	return r;
>  }
>  
> -static int _info(const char *name, const char *dlid, int mknodes,
> -		 int with_open_count, int with_read_ahead,
> +static int _info(const char *dlid, int with_open_count, int with_read_ahead,
>  		 struct dm_info *info, uint32_t *read_ahead)
>  {
>  	int r = 0;
>  
> -	if (!mknodes && dlid && *dlid) {
> -		if ((r = _info_run(NULL, dlid, info, read_ahead, 0, with_open_count,
> -			      with_read_ahead, 0, 0)) &&
> -	    	    info->exists)
> -			return 1;
> -		else if ((r = _info_run(NULL, dlid + sizeof(UUID_PREFIX) - 1, info,
> -				   read_ahead, 0, with_open_count,
> -				   with_read_ahead, 0, 0)) &&
> -			 info->exists)
> -			return 1;
> -	}
> -
> -	if (name)
> -		return _info_run(name, NULL, info, read_ahead, mknodes,
> -				 with_open_count, with_read_ahead, 0, 0);
> +	if ((r = _info_run(NULL, dlid, info, read_ahead, 0, with_open_count,
> +			   with_read_ahead, 0, 0)) && info->exists)
> +		return 1;
> +	else if ((r = _info_run(NULL, dlid + sizeof(UUID_PREFIX) - 1, info,
> +				read_ahead, 0, with_open_count,
> +				with_read_ahead, 0, 0)) && info->exists)
> +		return 1;
>  
>  	return r;
>  }
OK.

> @@ -238,7 +229,7 @@ static int _info_by_dev(uint32_t major, uint32_t minor, struct dm_info *info)
>  }
>  
>  int dev_manager_info(struct dm_pool *mem, const struct logical_volume *lv,
> -		     int with_mknodes, int with_open_count, int with_read_ahead,
> +		     int with_open_count, int with_read_ahead,
>  		     struct dm_info *info, uint32_t *read_ahead)
>  {
>  	const char *dlid, *name;
> @@ -254,9 +245,8 @@ int dev_manager_info(struct dm_pool *mem, const struct logical_volume *lv,
>  		return 0;
>  	}
>  
> -	log_debug("Getting device info for %s", name);
> -	r = _info(NULL, dlid, with_mknodes, with_open_count,
> -		  with_read_ahead, info, read_ahead);
> +	log_debug("Getting device info for %s [%s]", name, dlid);
> +	r = _info(dlid, with_open_count, with_read_ahead, info, read_ahead);
>  
>  	dm_pool_free(mem, (char*)name);
>  	return r;
> @@ -755,7 +745,7 @@ static int _belong_to_vg(const char *vgname, const char *name)
>  /*  NEW CODE STARTS HERE */
>  /*************************/
>  
> -int dev_manager_lv_mknodes(const struct logical_volume *lv)
> +static int dev_manager_lv_mknodes(const struct logical_volume *lv)
_dev_manager_lv_mknodes (due to static)?

> @@ -766,11 +756,32 @@ int dev_manager_lv_mknodes(const struct logical_volume *lv)
>  	return fs_add_lv(lv, name);
>  }
>  
> -int dev_manager_lv_rmnodes(const struct logical_volume *lv)
> +static int dev_manager_lv_rmnodes(const struct logical_volume *lv)
Same as above.

> +int dev_manager_mknodes(const struct logical_volume *lv)
> +{
> +	struct dm_info dminfo;
> +	const char *name;
> +	int r = 0;
> +
> +	if (!(name = build_dm_name(lv->vg->cmd->mem, lv->vg->name, lv->name, NULL)))
> +		return_0;
> +
> +	if ((r = _info_run(name, NULL, &dminfo, NULL, 1, 0, 0, 0, 0))) {
> +		if (dminfo.exists) {
> +			if (lv_is_visible(lv))
> +				r = dev_manager_lv_mknodes(lv);
> +		} else
> +			r = dev_manager_lv_rmnodes(lv);
> +	}
> +
> +	dm_pool_free(lv->vg->cmd->mem, (char*)name);
> +	return r;
> +}
> +
>  static int _add_dev_to_dtree(struct dev_manager *dm, struct dm_tree *dtree,
>  			       struct logical_volume *lv, const char *layer)
>  {
> @@ -785,7 +796,7 @@ static int _add_dev_to_dtree(struct dev_manager *dm, struct dm_tree *dtree,
>  		return_0;
>  
>  	log_debug("Getting device info for %s [%s]", name, dlid);
> -	if (!_info(name, dlid, 0, 1, 0, &info, NULL)) {
> +	if (!_info(dlid, 1, 0, &info, NULL)) {
>  		log_error("Failed to get info for %s [%s].", name, dlid);
>  		return 0;
>  	}
> diff --git a/lib/activate/dev_manager.h b/lib/activate/dev_manager.h
> index 86c978f..e946aaf 100644
> --- a/lib/activate/dev_manager.h
> +++ b/lib/activate/dev_manager.h
> @@ -41,7 +41,7 @@ void dev_manager_exit(void);
>   * unsuspended until the snapshot is also created.)
>   */
>  int dev_manager_info(struct dm_pool *mem, const struct logical_volume *lv,
> -		     int mknodes, int with_open_count, int with_read_ahead,
> +		     int with_open_count, int with_read_ahead,
>  		     struct dm_info *info, uint32_t *read_ahead);
>  int dev_manager_snapshot_percent(struct dev_manager *dm,
>  				 const struct logical_volume *lv,
> @@ -58,8 +58,7 @@ int dev_manager_preload(struct dev_manager *dm, struct logical_volume *lv,
>  			int *flush_required);
>  int dev_manager_deactivate(struct dev_manager *dm, struct logical_volume *lv);
>  
> -int dev_manager_lv_mknodes(const struct logical_volume *lv);
> -int dev_manager_lv_rmnodes(const struct logical_volume *lv);
> +int dev_manager_mknodes(const struct logical_volume *lv);
>  

Other than above, ACK.

Yours,
   Petr.