Fix bugs for invalid regex input

PATCH New Zdenek Kabelac 2 participants 10 messages Apr 30, 2010

lore

Share & tools

lore

Zdenek Kabelac

author

2010-04-30 09:07 UTC #0 lore raw

cover Fix bugs for invalid regex input

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(-)

Zdenek Kabelac

author

2010-04-30 09:07 UTC #1 lore raw

PATCH 1/3 Display errornous pattern to the user

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

Signed-off-by Zdenek Kabelac <[email protected]>

---
 lib/filters/filter-regex.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

lib/filters/filter-regex.c +2 -2

index 1d415a4..c062402 100644
@@ -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;
 		}

Signature · 1 line

1.7.0.1

Alasdair G Kergon

2010-04-30 10:55 UTC #2 lore raw

PATCH 1/3 Display errornous pattern to the user

lib/filters/filter-regex.c static int _build_matcher(struct rfilter *rf, struct config_value *val) · line 131

Quoted 2 lines · Zdenek Kabelac

lib/filters/filter-regex.c +1 -1

-			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

Zdenek Kabelac

author

2010-04-30 11:30 UTC #3 lore raw

PATCH 1/3 Display errornous pattern to the user

Dne 30.4.2010 12:55, Alasdair G Kergon napsal(a):

Quoted 8 lines · Zdenek Kabelac

lib/filters/filter-regex.c static int _build_matcher(struct rfilter *rf, struct config_value *val) · line 131
Quoted 2 lines
lib/filters/filter-regex.c +1 -1
-			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

Alasdair G Kergon

2010-04-30 11:46 UTC #4 lore raw

PATCH 1/3 Display errornous pattern to the user

Quoted 5 lines · Zdenek Kabelac

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

Alasdair G Kergon

2010-04-30 11:57 UTC #5 lore raw

PATCH 1/3 Display errornous pattern to the user

Quoted 2 lines · Alasdair G Kergon

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

Zdenek Kabelac

author

2010-04-30 09:07 UTC #6 lore raw

PATCH 2/3 Release allocated pools in error path

In reply to 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 <[email protected]>

---
 lib/commands/toolcontext.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

lib/commands/toolcontext.c +7 -2

index ba99a8d..b4c2690 100644
@@ -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)

Signature · 1 line

1.7.0.1

Alasdair G Kergon

2010-04-30 10:56 UTC #7 lore raw

PATCH 2/3 Release allocated pools in error path

Quoted 1 line · Zdenek Kabelac

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

Zdenek Kabelac

author

2010-04-30 09:07 UTC #8 lore raw

PATCH 3/3 Fix internal error for errornous regex insert

In reply to 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 <[email protected]>

---
 lib/cache/lvmcache.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

lib/cache/lvmcache.c +4 -4

index 73102ec..2c81e18 100644
@@ -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);
 }

Signature · 1 line

1.7.0.1

Alasdair G Kergon

2010-04-30 11:26 UTC #9 lore raw

PATCH 3/3 Fix internal error for errornous regex insert

Quoted 4 lines · 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.

If lvmcache_init is not called, neither should lvmcache_destroy be!

Alasdair