[PATCH 00/12] Misc updates

[PATCH 00/12] Misc updates

[Zdenek Kabelac]

Some of patches that has accumulated in my tree
and they should be upstreamed and are unrelated
to thin coding.

Comments are enclosed with each patch.

"Do not zero full 16KB" should be examined closely.

"DEBUG Stack" is rather proposal not a final patch.


Zdenek Kabelac (12):
  Allow to activate snapshot
  Query before removing inactive snapshots
  Code reorder
  Unlock memory for vg_write
  Change default prefix.
  Do not zero full 16KB data
  Replace dynamic buffer allocations for PATH_MAX
  Check for DM_MAX_TYPE_NAME length
  Limit max stack size
  Remove constant expression check
  Remove fixmes
  DEBUG Stack detection code

 doc/example.conf.in           |    6 ++--
 lib/activate/activate.c       |    4 +-
 lib/activate/dev_manager.c    |   10 +++----
 lib/activate/fs.c             |   12 ++++----
 lib/commands/toolcontext.c    |    6 ++--
 lib/config/defaults.h         |    2 +-
 lib/filters/filter.c          |    3 --
 lib/format_text/format-text.c |    2 +-
 lib/metadata/lv_manip.c       |   12 ++++++++-
 lib/metadata/metadata.c       |   18 +++++++-----
 lib/mm/memlock.c              |   58 ++++++++++++++++++++++++++++++++++++++--
 libdm/ioctl/libdm-iface.c     |   43 ++++++++++++++++++++----------
 libdm/libdm-common.c          |    3 +-
 test/t-lvcreate-usage.sh      |    2 +-
 tools/lvchange.c              |   28 ++++++++++++++++---
 tools/toollib.c               |   30 +++++---------------
 16 files changed, 158 insertions(+), 81 deletions(-)

-- 
1.7.7.3

[PATCH 01/12] Allow to activate snapshot

[Zdenek Kabelac]

Add extra code to active and deactivate related
snapshots and origin when user specifies snapshot
volume as lvchange parameter.

Before patch:

$> lvs -a
  LV    VG   Attr     LSize  Pool Origin Snap%  Move Log Copy%  Convert
  lvol0 mvg  owi-a-s-  1.00k
  lvol1 mvg  swi-a-s- 16.00k      lvol0    0.00
  lvol2 mvg  swi-a-s- 16.00k      lvol0    0.00

$> lvchange -an mvg/lvol2; echo $?
  Can't change snapshot logical volume "lvol2".
5

After patch:

$> lvchange -an mvg/lvol2
Change of snapshot lvol2 will also change its origin lvol0 and 1 other
snapshot(s)? [y/n]: n
  Can't change snapshot logical volume "lvol2".

$> lvchange -y -an mvg/lvol2; echo $?
0

$> lvs -a
  LV    VG   Attr     LSize  Pool Origin Snap%  Move Log Copy%  Convert
  lvol0 mvg  owi---s-  1.00k
  lvol1 mvg  swi---s- 16.00k      lvol0
  lvol2 mvg  swi---s- 16.00k      lvol0

Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>
---
 tools/lvchange.c |   28 +++++++++++++++++++++++-----
 1 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/tools/lvchange.c b/tools/lvchange.c
index 6cb9185..2cd4ba3 100644
--- a/tools/lvchange.c
+++ b/tools/lvchange.c
@@ -1,6 +1,6 @@
 /*
  * Copyright (C) 2001-2004 Sistina Software, Inc. All rights reserved.
- * Copyright (C) 2004-2007 Red Hat, Inc. All rights reserved.
+ * Copyright (C) 2004-2011 Red Hat, Inc. All rights reserved.
  *
  * This file is part of LVM2.
  *
@@ -125,6 +125,9 @@ static int lvchange_availability(struct cmd_context *cmd,
 
 	activate = arg_uint_value(cmd, available_ARG, 0);
 
+	if (lv_is_cow(lv) && !lv_is_virtual_origin(origin_from_cow(lv)))
+		lv = origin_from_cow(lv);
+
 	if (activate == CHANGE_ALN) {
 		log_verbose("Deactivating logical volume \"%s\" locally",
 			    lv->name);
@@ -515,6 +518,7 @@ static int lvchange_single(struct cmd_context *cmd, struct logical_volume *lv,
 	int doit = 0, docmds = 0;
 	int dmeventd_mode, archived = 0;
 	struct logical_volume *origin;
+	char snaps_msg[128];
 
 	if (!(lv->vg->status & LVM_WRITE) &&
 	    (arg_count(cmd, contiguous_ARG) || arg_count(cmd, permission_ARG) ||
@@ -534,11 +538,25 @@ static int lvchange_single(struct cmd_context *cmd, struct logical_volume *lv,
 		return ECMD_FAILED;
 	}
 
-	if (lv_is_cow(lv) && !lv_is_virtual_origin(origin_from_cow(lv)) &&
+	if (lv_is_cow(lv) && !lv_is_virtual_origin(origin = origin_from_cow(lv)) &&
 	    arg_count(cmd, available_ARG)) {
-		log_error("Can't change snapshot logical volume \"%s\"",
-			  lv->name);
-		return ECMD_FAILED;
+		if (origin->origin_count < 2)
+			snaps_msg[0] = '\0';
+		else if (dm_snprintf(snaps_msg, sizeof(snaps_msg),
+				     " and %u other snapshot(s)",
+				     origin->origin_count - 1) < 0) {
+			log_error("Failed to prepare message.");
+			return ECMD_FAILED;
+		}
+
+		if (!arg_count(cmd, yes_ARG) &&
+		    (yes_no_prompt("Change of snapshot %s will also change its "
+				   "origin %s%s? [y/n]: ", lv->name,
+				   origin->name, snaps_msg) == 'n')) {
+			log_error("Can't change snapshot logical volume \"%s\".",
+				  lv->name);
+			return ECMD_FAILED;
+		}
 	}
 
 	if (lv->status & PVMOVE) {
-- 
1.7.7.3

[PATCH 02/12] Query before removing inactive snapshots

[Zdenek Kabelac]

If the origin is not active - removal of such origin removes
also all connected snapshots.

When we now support 'old' external snapshots with thin volumes,
removal of pool will not only drop all thin volumes, but as
a consequence also all snapshots - which might be seen a bit
unexpected for the user - so add a query to confirm such action.

lvremove -f will skip the prompt.

Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>
---
 lib/metadata/lv_manip.c  |   12 +++++++++++-
 test/t-lvcreate-usage.sh |    2 +-
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/lib/metadata/lv_manip.c b/lib/metadata/lv_manip.c
index c323921..77ea830 100644
--- a/lib/metadata/lv_manip.c
+++ b/lib/metadata/lv_manip.c
@@ -3248,7 +3248,17 @@ int lv_remove_with_dependencies(struct cmd_context *cmd, struct logical_volume *
 	}
 
 	if (lv_is_origin(lv)) {
-		/* remove snapshot LVs first */
+		/* Remove snapshot LVs first */
+		if ((force == PROMPT) &&
+		    /* Active snapshot already needs to confirm each active LV */
+		    !lv_is_active(lv) &&
+		    yes_no_prompt("Removing origin %s will also remove %u "
+				  "snapshots(s). OK? [y/n]: ",
+				  lv->name, lv->origin_count) == 'n') {
+			log_error("Logical volume %s not removed.", lv->name);
+			return 0;
+		}
+
 		dm_list_iterate_safe(snh, snht, &lv->snapshot_segs)
 			if (!lv_remove_with_dependencies(cmd, dm_list_struct_base(snh, struct lv_segment,
 										  origin_list)->cow,
diff --git a/test/t-lvcreate-usage.sh b/test/t-lvcreate-usage.sh
index 595e1ad..b8e6967 100755
--- a/test/t-lvcreate-usage.sh
+++ b/test/t-lvcreate-usage.sh
@@ -122,7 +122,7 @@ lvcreate -s --virtualoriginsize 64m -L 32m -n $lv1 $vg
 lvrename $vg/$lv1 $vg/$lv2
 lvcreate -s --virtualoriginsize 64m -L 32m -n $lv1 $vg
 lvchange -a n $vg/$lv1
-lvremove $vg/$lv1
+lvremove -ff $vg/$lv1
 lvremove -ff $vg
 
 # readahead default (auto), none, #, auto
-- 
1.7.7.3

[PATCH 03/12] Code reorder

[Zdenek Kabelac]

Boolean algebra changes:

1st.
Drop process_lv variable since it's not needed.

2nd.
process_lv was always initilized to 0 - so condition was always true.
It the condition (!tags_supplied && !lvargs_supplied) evaluates to
true, process_all is already set to 1, so skip vg tags evaluation.

3rd.
Move check for matching lv name in front of lv tags check
since this check can't be skipped for lvargs_matched counter.
If this filter evaluates to true, skip lv tags evaluation.

Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>
---
 tools/toollib.c |   30 ++++++++----------------------
 1 files changed, 8 insertions(+), 22 deletions(-)

diff --git a/tools/toollib.c b/tools/toollib.c
index 1e13961..8030f8e 100644
--- a/tools/toollib.c
+++ b/tools/toollib.c
@@ -93,7 +93,6 @@ int process_each_lv_in_vg(struct cmd_context *cmd,
 	int ret_max = ECMD_PROCESSED;
 	int ret = 0;
 	unsigned process_all = 0;
-	unsigned process_lv = 0;
 	unsigned tags_supplied = 0;
 	unsigned lvargs_supplied = 0;
 	unsigned lvargs_matched = 0;
@@ -112,12 +111,10 @@ int process_each_lv_in_vg(struct cmd_context *cmd,
 	/* Process all LVs in this VG if no restrictions given */
 	if (!tags_supplied && !lvargs_supplied)
 		process_all = 1;
-
 	/* Or if VG tags match */
-	if (!process_lv && tags_supplied &&
-	    str_list_match_list(tags, &vg->tags, NULL)) {
+	else if (tags_supplied &&
+		 str_list_match_list(tags, &vg->tags, NULL))
 		process_all = 1;
-	}
 
 	dm_list_iterate_items(lvl, &vg->lvs) {
 		if (lvl->lv->status & SNAPSHOT)
@@ -139,26 +136,15 @@ int process_each_lv_in_vg(struct cmd_context *cmd,
 		if (!lvargs_supplied && !lv_is_visible(lvl->lv) && !arg_count(cmd, all_ARG))
 			continue;
 
-		/* Should we process this LV? */
-		if (process_all)
-			process_lv = 1;
-		else
-			process_lv = 0;
-
-		/* LV tag match? */
-		if (!process_lv && tags_supplied &&
-		    str_list_match_list(tags, &lvl->lv->tags, NULL)) {
-			process_lv = 1;
-		}
-
 		/* LV name match? */
 		if (lvargs_supplied &&
-		    str_list_match_item(arg_lvnames, lvl->lv->name)) {
-			process_lv = 1;
+		    str_list_match_item(arg_lvnames, lvl->lv->name))
+			/* Check even when process_all for counter */
 			lvargs_matched++;
-		}
-
-		if (!process_lv)
+		/* LV tag match?   skip test, when process_all */
+		else if (!process_all &&
+			 (!tags_supplied ||
+			  !str_list_match_list(tags, &lvl->lv->tags, NULL)))
 			continue;
 
 		lvl->lv->vg->cmd_missing_vgs = 0;
-- 
1.7.7.3

[PATCH 04/12] Unlock memory for vg_write

[Zdenek Kabelac]

For write we do not need to hold memory locked.
We should never write metadata in locked state.
This relaxes meny conditions and avoid problems when allocating
a lot of memory for writting metadata buffers.
(In case of huge MDA size this would lead to mismatch between
locked and unlocked memory size).

Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>
---
 lib/metadata/metadata.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/lib/metadata/metadata.c b/lib/metadata/metadata.c
index 4037c0e..f9742e7 100644
--- a/lib/metadata/metadata.c
+++ b/lib/metadata/metadata.c
@@ -2573,6 +2573,8 @@ int vg_write(struct volume_group *vg)
 		return 0;
 	}
 
+	/* Unlock memory if possible */
+	memlock_unlock(vg->cmd);
 	vg->seqno++;
 
         dm_list_iterate_items(pv_to_create, &vg->pvs_to_create) {
-- 
1.7.7.3

[PATCH 05/12] Change default prefix.

[Zdenek Kabelac]

Use ""  instead of "  ".
So commands like lvs print from the begining of the line.
(no need to trim space around).

Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>
---
 doc/example.conf.in   |    6 +++---
 lib/config/defaults.h |    2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/doc/example.conf.in b/doc/example.conf.in
index 2dd1808..1bc6df7 100644
--- a/doc/example.conf.in
+++ b/doc/example.conf.in
@@ -258,9 +258,9 @@ log {
     command_names = 0
 
     # A prefix to use before the message text (but after the command name,
-    # if selected).  Default is two spaces, so you can see/grep the severity
-    # of each message.
-    prefix = "  "
+    # if selected). Set to "  " to revert to the default behaviour
+    # prior to version 2.02.89.
+    prefix = ""
 
     # To make the messages look similar to the original LVM tools use:
     #   indent = 0
diff --git a/lib/config/defaults.h b/lib/config/defaults.h
index 9b91aea..5691d56 100644
--- a/lib/config/defaults.h
+++ b/lib/config/defaults.h
@@ -96,7 +96,7 @@
 #define DEFAULT_MAXIMISE_CLING 1
 #define DEFAULT_CLUSTERED 0
 
-#define DEFAULT_MSG_PREFIX "  "
+#define DEFAULT_MSG_PREFIX ""
 #define DEFAULT_CMD_NAME 0
 #define DEFAULT_OVERWRITE 0
 
-- 
1.7.7.3

[PATCH 06/12] Do not zero full 16KB data

[Zdenek Kabelac]

Could be a bit dangerous - so careful review is needed here.

When buffer is passed into ioctl - we always zero at least 16KB with
each ioctl - but it seems only 2 * sizeof(struct dm_ioctl) is
actually needed - this safe quite a few wasted CPU cycles spent
on many ioctl operations.
(could be seen as  micro-optimalization, since the real effect in CPU
 is below reliable measurable treshold for normal use cases.)

Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>
---
 libdm/ioctl/libdm-iface.c |   11 +++++++++--
 1 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/libdm/ioctl/libdm-iface.c b/libdm/ioctl/libdm-iface.c
index a0fa640..cddd84a 100644
--- a/libdm/ioctl/libdm-iface.c
+++ b/libdm/ioctl/libdm-iface.c
@@ -960,6 +960,7 @@ static struct dm_ioctl *_flatten(struct dm_task *dmt, unsigned repeat_count)
 	size_t len = sizeof(struct dm_ioctl);
 	char *b, *e;
 	int count = 0;
+	size_t min_zero_len = 2 * len;
 
 	for (t = dmt->head; t; t = t->next) {
 		len += sizeof(struct dm_target_spec);
@@ -1011,6 +1012,9 @@ static struct dm_ioctl *_flatten(struct dm_task *dmt, unsigned repeat_count)
 	if (dmt->geometry)
 		len += strlen(dmt->geometry) + 1;
 
+	if (min_zero_len < len)
+		min_zero_len = len;
+
 	/*
 	 * Give len a minimum size so that we have space to store
 	 * dependencies or status information.
@@ -1022,10 +1026,13 @@ static struct dm_ioctl *_flatten(struct dm_task *dmt, unsigned repeat_count)
 	while (repeat_count--)
 		len *= 2;
 
-	if (!(dmi = dm_malloc(len)))
+	if (!(dmi = dm_malloc(len))) {
+		log_error("Failed to allocate ioctl buffer.");
 		return NULL;
+	}
 
-	memset(dmi, 0, len);
+	/* Zero@least 2 * sizeof(struct dm_ioctl) */
+	memset(dmi, 0, min_zero_len);
 
 	version = &_cmd_data_v4[dmt->type].version;
 
-- 
1.7.7.3

[PATCH 07/12] Replace dynamic buffer allocations for PATH_MAX

[Zdenek Kabelac]

Use static buffer instead of stack allocated buffer.
This reduces stack size usage of lvm tool and the
change is very simple.

Since the whole library is not thread safe - it should not
add any extra problems - and if there will be some convertion
it's easy to convert this to use some preallocated buffer.

Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>
---
 lib/activate/activate.c       |    4 ++--
 lib/activate/fs.c             |   12 ++++++------
 lib/commands/toolcontext.c    |    6 +++---
 lib/format_text/format-text.c |    2 +-
 lib/metadata/metadata.c       |   16 ++++++++--------
 5 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/lib/activate/activate.c b/lib/activate/activate.c
index 0541c55..c0e8bea 100644
--- a/lib/activate/activate.c
+++ b/lib/activate/activate.c
@@ -38,7 +38,7 @@
 
 int lvm1_present(struct cmd_context *cmd)
 {
-	char path[PATH_MAX];
+	static char path[PATH_MAX];
 
 	if (dm_snprintf(path, sizeof(path), "%s/lvm/global", cmd->proc_dir)
 	    < 0) {
@@ -294,7 +294,7 @@ static int _passes_activation_filter(struct cmd_context *cmd,
 	const struct dm_config_node *cn;
 	const struct dm_config_value *cv;
 	const char *str;
-	char path[PATH_MAX];
+	static char path[PATH_MAX];
 
 	if (!(cn = find_config_tree_node(cmd, "activation/volume_list"))) {
 		log_verbose("activation/volume_list configuration setting "
diff --git a/lib/activate/fs.c b/lib/activate/fs.c
index f9eb280..2edd81e 100644
--- a/lib/activate/fs.c
+++ b/lib/activate/fs.c
@@ -36,7 +36,7 @@ static int _fs_create = 0;
 
 static int _mk_dir(const char *dev_dir, const char *vg_name)
 {
-	char vg_path[PATH_MAX];
+	static char vg_path[PATH_MAX];
 	mode_t old_umask;
 
 	if (dm_snprintf(vg_path, sizeof(vg_path), "%s%s",
@@ -67,7 +67,7 @@ static int _mk_dir(const char *dev_dir, const char *vg_name)
 
 static int _rm_dir(const char *dev_dir, const char *vg_name)
 {
-	char vg_path[PATH_MAX];
+	static char vg_path[PATH_MAX];
 
 	if (dm_snprintf(vg_path, sizeof(vg_path), "%s%s",
 			 dev_dir, vg_name) == -1) {
@@ -87,7 +87,7 @@ static int _rm_dir(const char *dev_dir, const char *vg_name)
 static void _rm_blks(const char *dir)
 {
 	const char *name;
-	char path[PATH_MAX];
+	static char path[PATH_MAX];
 	struct dirent *dirent;
 	struct stat buf;
 	DIR *d;
@@ -124,8 +124,8 @@ static void _rm_blks(const char *dir)
 static int _mk_link(const char *dev_dir, const char *vg_name,
 		    const char *lv_name, const char *dev, int check_udev)
 {
-	char lv_path[PATH_MAX], link_path[PATH_MAX], lvm1_group_path[PATH_MAX];
-	char vg_path[PATH_MAX];
+	static char lv_path[PATH_MAX], link_path[PATH_MAX], lvm1_group_path[PATH_MAX];
+	static char vg_path[PATH_MAX];
 	struct stat buf, buf_lp;
 
 	if (dm_snprintf(vg_path, sizeof(vg_path), "%s%s",
@@ -226,7 +226,7 @@ static int _rm_link(const char *dev_dir, const char *vg_name,
 		    const char *lv_name, int check_udev)
 {
 	struct stat buf;
-	char lv_path[PATH_MAX];
+	static char lv_path[PATH_MAX];
 
 	if (dm_snprintf(lv_path, sizeof(lv_path), "%s%s/%s",
 			 dev_dir, vg_name, lv_name) == -1) {
diff --git a/lib/commands/toolcontext.c b/lib/commands/toolcontext.c
index ec6e400..8b6ad3b 100644
--- a/lib/commands/toolcontext.c
+++ b/lib/commands/toolcontext.c
@@ -477,7 +477,7 @@ static int _init_tags(struct cmd_context *cmd, struct dm_config_tree *cft)
 
 static int _load_config_file(struct cmd_context *cmd, const char *tag)
 {
-	char config_file[PATH_MAX] = "";
+	static char config_file[PATH_MAX] = "";
 	const char *filler = "";
 	struct stat info;
 	struct config_tree_list *cfl;
@@ -786,10 +786,10 @@ err:
 
 static int _init_filters(struct cmd_context *cmd, unsigned load_persistent_cache)
 {
+	static char cache_file[PATH_MAX];
 	const char *dev_cache = NULL, *cache_dir, *cache_file_prefix;
 	struct dev_filter *f3, *f4;
 	struct stat st;
-	char cache_file[PATH_MAX];
 
 	cmd->dump_filter = 0;
 
@@ -1138,8 +1138,8 @@ static int _init_hostname(struct cmd_context *cmd)
 
 static int _init_backup(struct cmd_context *cmd)
 {
+	static char default_dir[PATH_MAX];
 	uint32_t days, min;
-	char default_dir[PATH_MAX];
 	const char *dir;
 
 	if (!cmd->system_dir[0]) {
diff --git a/lib/format_text/format-text.c b/lib/format_text/format-text.c
index 5e1db36..df3c870 100644
--- a/lib/format_text/format-text.c
+++ b/lib/format_text/format-text.c
@@ -1791,6 +1791,7 @@ static void *_create_text_context(struct dm_pool *mem, struct text_context *tc)
 static int _create_vg_text_instance(struct format_instance *fid,
                                     const struct format_instance_ctx *fic)
 {
+	static char path[PATH_MAX];
 	uint32_t type = fic->type;
 	struct text_fid_context *fidtc;
 	struct metadata_area *mda;
@@ -1798,7 +1799,6 @@ static int _create_vg_text_instance(struct format_instance *fid,
 	struct dir_list *dl;
 	struct raw_list *rl;
 	struct dm_list *dir_list, *raw_list;
-	char path[PATH_MAX];
 	struct text_context tc;
 	struct lvmcache_vginfo *vginfo;
 	struct lvmcache_info *info;
diff --git a/lib/metadata/metadata.c b/lib/metadata/metadata.c
index f9742e7..500732d 100644
--- a/lib/metadata/metadata.c
+++ b/lib/metadata/metadata.c
@@ -452,12 +452,12 @@ int move_pvs_used_by_lv(struct volume_group *vg_from,
 
 static int validate_new_vg_name(struct cmd_context *cmd, const char *vg_name)
 {
-	char vg_path[PATH_MAX];
+	static char vg_path[PATH_MAX];
 
 	if (!validate_name(vg_name))
 		return_0;
 
-	snprintf(vg_path, PATH_MAX, "%s%s", cmd->dev_dir, vg_name);
+	snprintf(vg_path, sizeof(vg_path), "%s%s", cmd->dev_dir, vg_name);
 	if (path_exists(vg_path)) {
 		log_error("%s: already exists in filesystem", vg_path);
 		return 0;
@@ -4197,7 +4197,7 @@ static int _convert_key_to_string(const char *key, size_t key_len,
 int fid_add_mda(struct format_instance *fid, struct metadata_area *mda,
 		 const char *key, size_t key_len, const unsigned sub_key)
 {
-	char full_key[PATH_MAX];
+	static char full_key[PATH_MAX];
 	dm_list_add(mda_is_ignored(mda) ? &fid->metadata_areas_ignored :
 					  &fid->metadata_areas_in_use, &mda->list);
 
@@ -4208,7 +4208,7 @@ int fid_add_mda(struct format_instance *fid, struct metadata_area *mda,
 	/* Add metadata area to index. */
 	if (fid->type & FMT_INSTANCE_VG) {
 		if (!_convert_key_to_string(key, key_len, sub_key,
-					    full_key, PATH_MAX))
+					    full_key, sizeof(full_key)))
 		return_0;
 
 		dm_hash_insert(fid->metadata_areas_index.hash,
@@ -4242,13 +4242,13 @@ struct metadata_area *fid_get_mda_indexed(struct format_instance *fid,
 					  const char *key, size_t key_len,
 					  const unsigned sub_key)
 {
-	char full_key[PATH_MAX];
+	static char full_key[PATH_MAX];
 	struct metadata_area *mda = NULL;
 
 
 	if (fid->type & FMT_INSTANCE_VG) {
 		if (!_convert_key_to_string(key, key_len, sub_key,
-					    full_key, PATH_MAX))
+					    full_key, sizeof(full_key)))
 			return_NULL;
 		mda = (struct metadata_area *) dm_hash_lookup(fid->metadata_areas_index.hash,
 							      full_key);
@@ -4262,8 +4262,8 @@ struct metadata_area *fid_get_mda_indexed(struct format_instance *fid,
 int fid_remove_mda(struct format_instance *fid, struct metadata_area *mda,
 		   const char *key, size_t key_len, const unsigned sub_key)
 {
+	static char full_key[PATH_MAX];
 	struct metadata_area *mda_indexed = NULL;
-	char full_key[PATH_MAX];
 
 	/* At least one of mda or key must be specified. */
 	if (!mda && !key)
@@ -4283,7 +4283,7 @@ int fid_remove_mda(struct format_instance *fid, struct metadata_area *mda,
 
 		if (fid->type & FMT_INSTANCE_VG) {
 			if (!_convert_key_to_string(key, key_len, sub_key,
-					    full_key, PATH_MAX))
+					    full_key, sizeof(full_key)))
 				return_0;
 
 			dm_hash_remove(fid->metadata_areas_index.hash, full_key);
-- 
1.7.7.3

[PATCH 08/12] Check for DM_MAX_TYPE_NAME length

[Zdenek Kabelac]

Avoid creation of target when it's longer then DM_MAX_TYPE_NAME
(noticed by static analyzer where the sp.target_type
might be missing '\0' at the end.)

Before patch:

$> dmsetup create long
0 1000 looooooooooooooooooooooooooong
^D
device-mapper: reload ioctl failed: Invalid argument

After patch:

$> dmsetup create xxx
0 1000 looooooooooooooooooooooooooong
Target type name looooooooooooooooooooooooooong is too long.
Command failed

Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>
---
 libdm/ioctl/libdm-iface.c |   32 ++++++++++++++++++++------------
 libdm/libdm-common.c      |    3 +--
 2 files changed, 21 insertions(+), 14 deletions(-)

diff --git a/libdm/ioctl/libdm-iface.c b/libdm/ioctl/libdm-iface.c
index cddd84a..725bd83 100644
--- a/libdm/ioctl/libdm-iface.c
+++ b/libdm/ioctl/libdm-iface.c
@@ -853,9 +853,14 @@ int dm_task_set_event_nr(struct dm_task *dmt, uint32_t event_nr)
 struct target *create_target(uint64_t start, uint64_t len, const char *type,
 			     const char *params)
 {
-	struct target *t = dm_zalloc(sizeof(*t));
+	struct target *t;
+
+	if (strlen(type) >= DM_MAX_TYPE_NAME) {
+		log_error("Target type name %s is too long.", type);
+		return NULL;
+	}
 
-	if (!t) {
+	if (!(t = dm_zalloc(sizeof(*t)))) {
 		log_error("create_target: malloc(%" PRIsize_t ") failed",
 			  sizeof(*t));
 		return NULL;
@@ -889,19 +894,24 @@ static char *_add_target(struct target *t, char *out, char *end)
 	size_t sp_size = sizeof(struct dm_target_spec);
 	int len;
 
-	out += sp_size;
-	if (out >= end)
-		return_NULL;
+	if (strlen(t->type) >= sizeof(sp.target_type)) {
+		log_error("Target type name %s is too long.", t->type);
+		return NULL;
+	}
 
 	sp.status = 0;
 	sp.sector_start = t->start;
 	sp.length = t->length;
-	strncpy(sp.target_type, t->type, sizeof(sp.target_type));
+	strncpy(sp.target_type, t->type, sizeof(sp.target_type) - 1);
+	sp.target_type[sizeof(sp.target_type) - 1] = '\0';
 
+	out += sp_size;
 	len = strlen(t->params);
 
-	if ((out + len + 1) >= end)
-		return_NULL;
+	if ((out >= end) || (out + len + 1) >= end) {
+		log_error("Ran out of memory building ioctl parameter");
+		return NULL;
+	}
 
 	strcpy(out, t->params);
 	out += len + 1;
@@ -1117,10 +1127,8 @@ static struct dm_ioctl *_flatten(struct dm_task *dmt, unsigned repeat_count)
 	e = (char *) dmi + len;
 
 	for (t = dmt->head; t; t = t->next)
-		if (!(b = _add_target(t, b, e))) {
-			log_error("Ran out of memory building ioctl parameter");
-			goto bad;
-		}
+		if (!(b = _add_target(t, b, e)))
+			goto_bad;
 
 	if (dmt->newname)
 		strcpy(b, dmt->newname);
diff --git a/libdm/libdm-common.c b/libdm/libdm-common.c
index 23539d7..4f438bd 100644
--- a/libdm/libdm-common.c
+++ b/libdm/libdm-common.c
@@ -407,9 +407,8 @@ int dm_task_add_target(struct dm_task *dmt, uint64_t start, uint64_t size,
 		       const char *ttype, const char *params)
 {
 	struct target *t = create_target(start, size, ttype, params);
-
 	if (!t)
-		return 0;
+		return_0;
 
 	if (!dmt->head)
 		dmt->head = dmt->tail = t;
-- 
1.7.7.3

[PATCH 09/12] Limit max stack size

[Zdenek Kabelac]

For now CLMVD allocates 128KiB for each thread which is
twice the DEFAULT_RESERVED_STACK that happens to be 64KiB so
we cannot support to prealllocate the arbitrary large stack size.

With this whole patchset applied it could be seen, the whole test-suite
is passing even with just 32KiB preallocated stack with still some KiB
left. So there is probably no point to allow allocation of more then
64KiB.

We can have number of solutions here - ideally I'd drop the whole
configurability of stack prealloction and leave it for internal code
to know better what the stack size is needed.

Without this patch every CLVMD user which have activation/reserved_stack
set to 256KiB (which was default prior 2.02.89) will get clvmd crash
after start.

IMHO the worst option is to implement lvm.conf parsing into
clvmd, to find out reserved_size and update pthread stack size to
accomodate this value (since currently clvmd does not use lvm.conf -
only its lvm threads).

Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>
---
 lib/mm/memlock.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/lib/mm/memlock.c b/lib/mm/memlock.c
index 8bf9f22..b9fc430 100644
--- a/lib/mm/memlock.c
+++ b/lib/mm/memlock.c
@@ -446,8 +446,13 @@ void memlock_dec_daemon(struct cmd_context *cmd)
 void memlock_init(struct cmd_context *cmd)
 {
 	_size_stack = find_config_tree_int(cmd,
-				      "activation/reserved_stack",
-				      DEFAULT_RESERVED_STACK) * 1024;
+					   "activation/reserved_stack",
+					   DEFAULT_RESERVED_STACK) * 1024;
+	if (_size_stack > (DEFAULT_RESERVED_STACK * 1024)) {
+		_size_stack = DEFAULT_RESERVED_STACK * 1024;
+		log_verbose("Using maximum supported prealloc stack size %uKB",
+			    DEFAULT_RESERVED_STACK);
+	}
 	_size_malloc_tmp = find_config_tree_int(cmd,
 					   "activation/reserved_memory",
 					   DEFAULT_RESERVED_MEMORY) * 1024;
-- 
1.7.7.3

[PATCH 10/12] Remove constant expression check

[Zdenek Kabelac]

"result_independent_of_operands: ((dev->dev & 0xfff00UL) >> 8) ==
18446744073709551615UL /* -1 */ is always false regardless of the values
of its operands (logical operand of if)."

'dev->dev' is set in dev-cache.c _insert() and it's not expectable
st_rdev would have '-1'

Thought even if it would - it'd be still missed it by macro
transformation.

My proposal is to remove this check competely.

For linux it's noop anyway.

Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>
---
 lib/filters/filter.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/lib/filters/filter.c b/lib/filters/filter.c
index c66939a..4c3d3b7 100644
--- a/lib/filters/filter.c
+++ b/lib/filters/filter.c
@@ -64,9 +64,6 @@ int dev_subsystem_part_major(const struct device *dev)
 {
 	dev_t primary_dev;
 
-	if (MAJOR(dev->dev) == -1)
-		return 0;
-
 	if (MAJOR(dev->dev) == _md_major)
 		return 1;
 
-- 
1.7.7.3

[PATCH 11/12] Remove fixmes

[Zdenek Kabelac]

Remove FIXMES - there should not be any pool free call since
the memory pool is from device manager, and pool is detroyed
after the operation, so doing extra free here would not help here.

However lv_has_target_type() is using cmd mempool so here the extra
call for dm_pool_free makes sence.

Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>
---
 lib/activate/dev_manager.c |   10 ++++------
 1 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/lib/activate/dev_manager.c b/lib/activate/dev_manager.c
index 4f7e6ac..fdeb967 100644
--- a/lib/activate/dev_manager.c
+++ b/lib/activate/dev_manager.c
@@ -411,7 +411,7 @@ int lv_has_target_type(struct dm_pool *mem, struct logical_volume *lv,
 
 	if (!(dmt = _setup_task(NULL, dlid, 0,
 				DM_DEVICE_STATUS, 0, 0)))
-		return_0;
+		goto_bad;
 
 	if (!dm_task_no_open_count(dmt))
 		log_error("Failed to disable open_count");
@@ -433,8 +433,10 @@ int lv_has_target_type(struct dm_pool *mem, struct logical_volume *lv,
 		}
 	} while (next);
 
- out:
+out:
 	dm_task_destroy(dmt);
+	dm_pool_free(mem, dlid);
+bad:
 	return r;
 }
 
@@ -737,8 +739,6 @@ int dev_manager_snapshot_percent(struct dev_manager *dm,
 		       NULL, fail_if_percent_unsupported)))
 		return_0;
 
-	/* FIXME dm_pool_free ? */
-
 	/* If the snapshot isn't available, percent will be -1 */
 	return 1;
 }
@@ -760,8 +760,6 @@ int dev_manager_mirror_percent(struct dev_manager *dm,
 	if (!(name = dm_build_dm_name(dm->mem, lv->vg->name, lv->name, layer)))
 		return_0;
 
-	/* FIXME dm_pool_free ? */
-
 	if (!(dlid = build_dm_uuid(dm->mem, lv->lvid.s, layer))) {
 		log_error("dlid build failed for %s", lv->name);
 		return 0;
-- 
1.7.7.3

[PATCH 12/12] DEBUG Stack detection code

[Zdenek Kabelac]

This patch is rather just an idea we might deploy into the source
code in some further modifed version.

It prefills allocated stack size with some pattern and check
whether pattern is still present when leaving critical section
and prints the amount of unmodified data.

Final version might probably mark just bottom 32? bytes
and check if they are still there ?
(Something like we are doing for memory locked & unlocked size).

Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>
---
 lib/mm/memlock.c |   49 ++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 48 insertions(+), 1 deletions(-)

diff --git a/lib/mm/memlock.c b/lib/mm/memlock.c
index b9fc430..0f705c0 100644
--- a/lib/mm/memlock.c
+++ b/lib/mm/memlock.c
@@ -26,6 +26,10 @@
 #include <sys/time.h>
 #include <sys/resource.h>
 
+#ifdef VALGRIND_POOL
+#include "valgrind/memcheck.h"
+#endif
+
 #ifndef DEVMAPPER_SUPPORT
 
 void memlock_inc_daemon(struct cmd_context *cmd)
@@ -121,12 +125,54 @@ static void _touch_memory(void *mem, size_t size)
 	}
 }
 
+static long *_stack_lowest = 0;
+static void _touch_stack(void *mem, size_t size)
+{
+	long *pos = _stack_lowest = (long *) mem;
+	long *end = (long *) ((char *) pos + size - sizeof(long));
+	while (pos < end) {
+		*pos = (long) pos;
+		pos++;
+	}
+}
+
+static void _test_stack(void)
+{
+	long *pos = NULL;
+	long *end = (long*)&end;
+
+	/* Tets works only for downward growing stack */
+	if (&end > &pos)
+		return;
+
+	if (end <= _stack_lowest)
+		goto bad;
+
+	pos = alloca((end - _stack_lowest) * sizeof(long));
+#ifdef VALGRIND_POOL
+	VALGRIND_MAKE_MEM_DEFINED(pos, (end - _stack_lowest) * sizeof(long));
+#endif
+
+	pos = _stack_lowest;
+
+	while (pos < end && (*pos == (long) pos))
+		pos++;
+
+	log_verbose("Stack has not used %" PRIsize_t " reserved bytes.",
+		    sizeof(long) * (pos - _stack_lowest));
+
+	if ((pos - _stack_lowest) > 0)
+		return;
+bad:
+	log_error(INTERNAL_ERROR "Reserved stack has been too small.");
+}
+
 static void _allocate_memory(void)
 {
 	void *stack_mem, *temp_malloc_mem;
 
 	if ((stack_mem = alloca(_size_stack)))
-		_touch_memory(stack_mem, _size_stack);
+		_touch_stack(stack_mem, _size_stack);
 
 	if ((temp_malloc_mem = malloc(_size_malloc_tmp)))
 		_touch_memory(temp_malloc_mem, _size_malloc_tmp);
@@ -140,6 +186,7 @@ static void _allocate_memory(void)
 static void _release_memory(void)
 {
 	free(_malloc_mem);
+	_test_stack();
 }
 
 /*
-- 
1.7.7.3