[PATCH 0/3] Fix bugs for invalid regex input

[PATCH 0/3] Fix bugs for invalid regex input

[Zdenek Kabelac]

This small patchset fixes error messages shown to the user,
when he makes a mistake in regex input.

Zdenek Kabelac (3):
  Display errornous pattern to the user
  Release allocated pools in error path
  Fix internal error for errornous regex insert

 lib/cache/lvmcache.c       |    8 ++++----
 lib/commands/toolcontext.c |    9 +++++++--
 lib/filters/filter-regex.c |    4 ++--
 3 files changed, 13 insertions(+), 8 deletions(-)

[PATCH 1/3] Display errornous pattern to the user

[Zdenek Kabelac]

Show string with errornous pattern and start error message with
capital letter.

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

diff --git a/lib/filters/filter-regex.c b/lib/filters/filter-regex.c
index 1d415a4..c062402 100644
--- a/lib/filters/filter-regex.c
+++ b/lib/filters/filter-regex.c
@@ -103,7 +103,7 @@ static int _build_matcher(struct rfilter *rf, struct config_value *val)
 	 */
 	for (v = val; v; v = v->next) {
 		if (v->type != CFG_STRING) {
-			log_error("filter patterns must be enclosed in quotes");
+			log_error("Filter patterns must be enclosed in quotes.");
 			goto out;
 		}
 
@@ -128,7 +128,7 @@ static int _build_matcher(struct rfilter *rf, struct config_value *val)
 	 */
 	for (v = val, i = count - 1; v; v = v->next, i--)
 		if (!_extract_pattern(scratch, v->v.str, regex, rf->accept, i)) {
-			log_error("invalid filter pattern");
+			log_error("Invalid filter pattern \"%s\".", v->v.str);
 			goto out;
 		}
 
-- 
1.7.0.1

[PATCH 1/3] Display errornous pattern to the user

[Alasdair G Kergon]

On Fri, Apr 30, 2010 at 11:07:19AM +0200, Zdenek Kabelac wrote:
> -			log_error("invalid filter pattern");
> +			log_error("Invalid filter pattern \"%s\".", v->v.str);

No need to double the quotes there?
Just %s will do.
Ack.

Alasdair

[PATCH 1/3] Display errornous pattern to the user

[Zdenek Kabelac]

Dne 30.4.2010 12:55, Alasdair G Kergon napsal(a):
> On Fri, Apr 30, 2010 at 11:07:19AM +0200, Zdenek Kabelac wrote:
>> -			log_error("invalid filter pattern");
>> +			log_error("Invalid filter pattern \"%s\".", v->v.str);
> 
> No need to double the quotes there?
> Just %s will do.
> Ack.
> 

Well I wanted to make clear what is the expression - which is usually used
with quotes in lvm.conf. If the user uses spaces inside (or dots for sentence
end) it might look a bit confusing in the first sight. Thought the user will
usually immediatelly notice what is wrong - so plain '%s' would do the job as
well, but I would prefer quotes in this case.

Zdenek

[PATCH 1/3] Display errornous pattern to the user

[Alasdair G Kergon]

On Fri, Apr 30, 2010 at 01:30:46PM +0200, Zdenek Kabelac wrote:
> Well I wanted to make clear what is the expression - which is usually used
> with quotes in lvm.conf. If the user uses spaces inside (or dots for sentence
> end) it might look a bit confusing in the first sight. Thought the user will
> usually immediatelly notice what is wrong - so plain '%s' would do the job as
> well, but I would prefer quotes in this case.
 
Have you tested this?
I'm saying I prefer a message with "a|dev/loop|" to ""a|dev/loop\"".

Alasdair

[PATCH 1/3] Display errornous pattern to the user

[Alasdair G Kergon]

On Fri, Apr 30, 2010 at 12:46:47PM +0100, Alasdair G Kergon wrote:
> Have you tested this?
> I'm saying I prefer a message with "a|dev/loop|" to ""a|dev/loop\"".
 
Sorry, I chose a bad test pattern.  Your patch is correct.

Alasdair

[PATCH 2/3] Release allocated pools in error path

[Zdenek Kabelac]

Release pools for regex if there is error during processing
(so we do not get error messages about unreleased pools).

Do we want to really drop all filters here ?

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

diff --git a/lib/commands/toolcontext.c b/lib/commands/toolcontext.c
index ba99a8d..b4c2690 100644
--- a/lib/commands/toolcontext.c
+++ b/lib/commands/toolcontext.c
@@ -639,14 +639,14 @@ static struct dev_filter *_init_filter_components(struct cmd_context *cmd)
 
 	else if (!(filters[nr_filt++] = regex_filter_create(cn->v))) {
 		log_error("Failed to create regex device filter");
-		return NULL;
+		goto err;
 	}
 
 	/* device type filter. Required. */
 	cn = find_config_tree_node(cmd, "devices/types");
 	if (!(filters[nr_filt++] = lvm_type_filter_create(cmd->proc_dir, cn))) {
 		log_error("Failed to create lvm type filter");
-		return NULL;
+		goto err;
 	}
 
 	/* md component filter. Optional, non-critical. */
@@ -660,6 +660,11 @@ static struct dev_filter *_init_filter_components(struct cmd_context *cmd)
 	/* Only build a composite filter if we really need it. */
 	return (nr_filt == 1) ?
 	    filters[0] : composite_filter_create(nr_filt, filters);
+err:
+	nr_filt--; /* skip NULL */
+	while (nr_filt-- > 0)
+		 filters[nr_filt]->destroy(filters[nr_filt]);
+	return NULL;
 }
 
 static int _init_filters(struct cmd_context *cmd, unsigned load_persistent_cache)
-- 
1.7.0.1

[PATCH 2/3] Release allocated pools in error path

[Alasdair G Kergon]

On Fri, Apr 30, 2010 at 11:07:20AM +0200, Zdenek Kabelac wrote:
> Do we want to really drop all filters here ?
 
If we can't set up all filters, it needs to be a fatal error.
(Otherwise the tools might operate on devices they shouldn't.)

Ack.

Alasdair

[PATCH 3/3] Fix internal error for errornous regex insert

[Zdenek Kabelac]

Fix error message shown for incorrect regex in config file:

'Internal error: _vginfos list should be empty'

When regex parser fails lvmcache_init() is not invoked - thus
_vginfos list is not initiliased.

To fix this problem  '_vgname_hash' pointer is reused as a sign of properly
processed initializion and only in this case list is checked for
being empty and updated error message is printed in this case.

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

diff --git a/lib/cache/lvmcache.c b/lib/cache/lvmcache.c
index 73102ec..2c81e18 100644
--- a/lib/cache/lvmcache.c
+++ b/lib/cache/lvmcache.c
@@ -1356,6 +1356,10 @@ void lvmcache_destroy(struct cmd_context *cmd, int retain_orphans)
 		_pvid_hash = NULL;
 	}
 
+	if (_vgname_hash && !dm_list_empty(&_vginfos))
+		log_error(INTERNAL_ERROR "List _vginfos should be empty.");
+	dm_list_init(&_vginfos);
+
 	if (_vgname_hash) {
 		dm_hash_iter(_vgname_hash,
 			  (dm_hash_iterate_fn) _lvmcache_destroy_vgnamelist);
@@ -1370,10 +1374,6 @@ void lvmcache_destroy(struct cmd_context *cmd, int retain_orphans)
 		_lock_hash = NULL;
 	}
 
-	if (!dm_list_empty(&_vginfos))
-		log_error(INTERNAL_ERROR "_vginfos list should be empty");
-	dm_list_init(&_vginfos);
-
 	if (retain_orphans)
 		init_lvmcache_orphans(cmd);
 }
-- 
1.7.0.1

[PATCH 3/3] Fix internal error for errornous regex insert

[Alasdair G Kergon]

On Fri, Apr 30, 2010 at 11:07:21AM +0200, Zdenek Kabelac wrote:
> Fix error message shown for incorrect regex in config file:
> 'Internal error: _vginfos list should be empty'
> When regex parser fails lvmcache_init() is not invoked - thus
> _vginfos list is not initiliased.
 
If lvmcache_init is not called, neither should lvmcache_destroy be!

Alasdair