[PATCH 0/4] Fixes and cleanups

[PATCH 0/4] Fixes and cleanups

[Zdenek Kabelac]

Zdenek Kabelac (4):
  Fix coredump and memory leak for 'dmsetup help -c'
  Fix unlocking for vgrelease for error paths
  Skip 'return' keyword for void() functions
  Call dm_pool_destroy on allocation error path

 lib/display/display.c           |   11 -----------
 lib/filters/filter-persistent.c |    2 --
 lib/filters/filter.c            |    1 -
 lib/format1/format1.c           |    1 -
 lib/format1/lvm1-label.c        |    1 -
 lib/format_pool/format_pool.c   |    1 -
 lib/format_pool/pool_label.c    |    1 -
 lib/format_text/format-text.c   |    1 -
 lib/locking/locking.c           |    4 ----
 lib/locking/no_locking.c        |    2 --
 lib/metadata/metadata.c         |    2 +-
 lib/misc/lvm-string.c           |    2 +-
 libdm/libdm-deptree.c           |    4 ++--
 tools/dmsetup.c                 |    7 ++++---
 tools/lvmcmdlib.c               |    2 --
 tools/pvresize.c                |    8 ++++++--
 tools/pvscan.c                  |    1 -
 tools/toollib.c                 |    6 +++---
 18 files changed, 17 insertions(+), 40 deletions(-)

[PATCH 1/4] Fix coredump and memory leak for 'dmsetup help -c'

[Zdenek Kabelac]

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

diff --git a/tools/dmsetup.c b/tools/dmsetup.c
index 888edff..b5568f0 100644
--- a/tools/dmsetup.c
+++ b/tools/dmsetup.c
@@ -2402,7 +2402,7 @@ static int _report_init(struct command *c)
 	size_t len = 0;
 	int r = 0;
 
-	if (!strcmp(c->name, "splitname"))
+	if (c && !strcmp(c->name, "splitname"))
 		options = (char *) splitname_report_options;
 
 	/* emulate old dmsetup behaviour */
@@ -2591,7 +2591,9 @@ static int _help(int argc __attribute((unused)),
 		_switches[OPTIONS_ARG] = 1;
 		_string_args[OPTIONS_ARG] = (char *) "help";
 		_switches[SORT_ARG] = 0;
-	
+
+		if (_report)
+			dm_report_free(_report);
 		(void) _report_init(NULL);
 	}
 
-- 
1.6.5.3

[PATCH 2/4] Fix unlocking for vgrelease for error paths

[Zdenek Kabelac]

Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>
---
 tools/pvresize.c |    8 ++++++--
 tools/toollib.c  |    6 +++---
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/tools/pvresize.c b/tools/pvresize.c
index 7a911ac..292e3cc 100644
--- a/tools/pvresize.c
+++ b/tools/pvresize.c
@@ -60,8 +60,12 @@ static int _pv_resize_single(struct cmd_context *cmd,
 
 		vg = vg_read_for_update(cmd, vg_name, NULL, 0);
 
-		if (vg_read_error(vg))
-			goto bad;
+		if (vg_read_error(vg)) {
+			vg_release(vg);
+			log_error("Unable to read volume group \"%s\".",
+				  vg_name);
+			return 0;
+		}
 
 		if (!(pvl = find_pv_in_vg(vg, pv_name))) {
 			log_error("Unable to find \"%s\" in volume group \"%s\"",
diff --git a/tools/toollib.c b/tools/toollib.c
index 33daf30..fce399b 100644
--- a/tools/toollib.c
+++ b/tools/toollib.c
@@ -315,7 +315,7 @@ int process_each_lv(struct cmd_context *cmd, int argc, char **argv,
 						  dm_pool_strdup(cmd->mem,
 							      lv_name + 1))) {
 					log_error("strlist allocation failed");
-					vg_release(vg);
+					unlock_and_release_vg(cmd, vg, vgname);
 					return ECMD_FAILED;
 				}
 			}
@@ -367,8 +367,8 @@ int process_each_segment_in_pv(struct cmd_context *cmd,
 		if (!(pvl = find_pv_in_vg(vg, pv_dev_name(pv)))) {
 			 log_error("Unable to find %s in volume group %s",
 				   pv_dev_name(pv), vg_name);
-			vg_release(vg);
-			return ECMD_FAILED;
+			 unlock_and_release_vg(cmd, vg, vg_name);
+			 return ECMD_FAILED;
 		}
 
 		pv = pvl->pv;
-- 
1.6.5.3

[PATCH 3/4] Skip 'return' keyword for void() functions

[Zdenek Kabelac]

Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>
---
 lib/display/display.c           |   11 -----------
 lib/filters/filter-persistent.c |    2 --
 lib/filters/filter.c            |    1 -
 lib/format1/format1.c           |    1 -
 lib/format1/lvm1-label.c        |    1 -
 lib/format_pool/format_pool.c   |    1 -
 lib/format_pool/pool_label.c    |    1 -
 lib/format_text/format-text.c   |    1 -
 lib/locking/locking.c           |    4 ----
 lib/locking/no_locking.c        |    2 --
 lib/misc/lvm-string.c           |    2 +-
 libdm/libdm-deptree.c           |    4 ++--
 tools/dmsetup.c                 |    1 -
 tools/lvmcmdlib.c               |    2 --
 tools/pvscan.c                  |    1 -
 15 files changed, 3 insertions(+), 32 deletions(-)

diff --git a/lib/display/display.c b/lib/display/display.c
index 691f1cf..a835327 100644
--- a/lib/display/display.c
+++ b/lib/display/display.c
@@ -327,8 +327,6 @@ void pvdisplay_colons(const struct physical_volume *pv)
 		  pv->pe_count,
 		  pv->pe_count - pv->pe_alloc_count,
 		  pv->pe_alloc_count, *uuid ? uuid : "none");
-
-	return;
 }
 
 void pvdisplay_segments(const struct physical_volume *pv)
@@ -355,7 +353,6 @@ void pvdisplay_segments(const struct physical_volume *pv)
 	}
 
 	log_print(" ");
-	return;
 }
 
 /* FIXME Include label fields */
@@ -424,8 +421,6 @@ void pvdisplay_full(const struct cmd_context *cmd,
 	log_print("Allocated PE          %u", pv->pe_alloc_count);
 	log_print("PV UUID               %s", *uuid ? uuid : "none");
 	log_print(" ");
-
-	return;
 }
 
 int pvdisplay_short(const struct cmd_context *cmd __attribute((unused)),
@@ -470,7 +465,6 @@ void lvdisplay_colons(const struct logical_volume *lv)
 		  /* FIXME Add num allocated to struct! lv->lv_allocated_le, */
 		  (lv->alloc == ALLOC_CONTIGUOUS ? 2 : 0), lv->read_ahead,
 		  inkernel ? info.major : -1, inkernel ? info.minor : -1);
-	return;
 }
 
 int lvdisplay_full(struct cmd_context *cmd,
@@ -654,7 +648,6 @@ int lvdisplay_segments(const struct logical_volume *lv)
 
 void vgdisplay_extents(const struct volume_group *vg __attribute((unused)))
 {
-	return;
 }
 
 void vgdisplay_full(const struct volume_group *vg)
@@ -729,8 +722,6 @@ void vgdisplay_full(const struct volume_group *vg)
 
 	log_print("VG UUID               %s", uuid);
 	log_print(" ");
-
-	return;
 }
 
 void vgdisplay_colons(const struct volume_group *vg)
@@ -779,7 +770,6 @@ void vgdisplay_colons(const struct volume_group *vg)
 		vg->extent_count - vg->free_count,
 		vg->free_count,
 		uuid[0] ? uuid : "none");
-	return;
 }
 
 void vgdisplay_short(const struct volume_group *vg)
@@ -792,7 +782,6 @@ void vgdisplay_short(const struct volume_group *vg)
 			       ((uint64_t) vg->extent_count -
 				vg->free_count) * vg->extent_size),
 		  display_size(vg->cmd, vg_free(vg)));
-	return;
 }
 
 void display_formats(const struct cmd_context *cmd)
diff --git a/lib/filters/filter-persistent.c b/lib/filters/filter-persistent.c
index b24850b..d91ed13 100644
--- a/lib/filters/filter-persistent.c
+++ b/lib/filters/filter-persistent.c
@@ -171,8 +171,6 @@ static void _write_array(struct pfilter *pf, FILE *fp, const char *path,
 
 	if (!first)
 		fprintf(fp, "\n\t]\n");
-
-	return;
 }
 
 int persistent_filter_dump(struct dev_filter *f)
diff --git a/lib/filters/filter.c b/lib/filters/filter.c
index 889b416..b33d099 100644
--- a/lib/filters/filter.c
+++ b/lib/filters/filter.c
@@ -331,5 +331,4 @@ struct dev_filter *lvm_type_filter_create(const char *proc,
 void lvm_type_filter_destroy(struct dev_filter *f)
 {
 	dm_free(f);
-	return;
 }
diff --git a/lib/format1/format1.c b/lib/format1/format1.c
index 887906c..b6a2c15 100644
--- a/lib/format1/format1.c
+++ b/lib/format1/format1.c
@@ -480,7 +480,6 @@ static struct format_instance *_format1_create_instance(const struct format_type
 
 static void _format1_destroy_instance(struct format_instance *fid __attribute((unused)))
 {
-	return;
 }
 
 static void _format1_destroy(const struct format_type *fmt)
diff --git a/lib/format1/lvm1-label.c b/lib/format1/lvm1-label.c
index eeb929c..bf25ce0 100644
--- a/lib/format1/lvm1-label.c
+++ b/lib/format1/lvm1-label.c
@@ -96,7 +96,6 @@ static int _lvm1_initialise_label(struct labeller *l __attribute((unused)), stru
 
 static void _lvm1_destroy_label(struct labeller *l __attribute((unused)), struct label *label __attribute((unused)))
 {
-	return;
 }
 
 static void _lvm1_destroy(struct labeller *l)
diff --git a/lib/format_pool/format_pool.c b/lib/format_pool/format_pool.c
index 0d3f918..26321c1 100644
--- a/lib/format_pool/format_pool.c
+++ b/lib/format_pool/format_pool.c
@@ -281,7 +281,6 @@ static struct format_instance *_pool_create_instance(const struct format_type *f
 
 static void _pool_destroy_instance(struct format_instance *fid __attribute((unused)))
 {
-	return;
 }
 
 static void _pool_destroy(const struct format_type *fmt)
diff --git a/lib/format_pool/pool_label.c b/lib/format_pool/pool_label.c
index 5b43375..7b4cfac 100644
--- a/lib/format_pool/pool_label.c
+++ b/lib/format_pool/pool_label.c
@@ -73,7 +73,6 @@ static int _pool_initialise_label(struct labeller *l __attribute((unused)), stru
 
 static void _pool_destroy_label(struct labeller *l __attribute((unused)), struct label *label __attribute((unused)))
 {
-	return;
 }
 
 static void _label_pool_destroy(struct labeller *l)
diff --git a/lib/format_text/format-text.c b/lib/format_text/format-text.c
index 5dacb21..2c75054 100644
--- a/lib/format_text/format-text.c
+++ b/lib/format_text/format-text.c
@@ -1593,7 +1593,6 @@ static int _text_pv_read(const struct format_type *fmt, const char *pv_name,
 
 static void _text_destroy_instance(struct format_instance *fid __attribute((unused)))
 {
-	return;
 }
 
 static void _free_dirs(struct dm_list *dir_list)
diff --git a/lib/locking/locking.c b/lib/locking/locking.c
index 7bf3f78..ad1c2db 100644
--- a/lib/locking/locking.c
+++ b/lib/locking/locking.c
@@ -145,8 +145,6 @@ static void _block_signals(uint32_t flags __attribute((unused)))
 	}
 
 	_signals_blocked = 1;
-
-	return;
 }
 
 static void _unblock_signals(void)
@@ -161,8 +159,6 @@ static void _unblock_signals(void)
 	}
 
 	_signals_blocked = 0;
-
-	return;
 }
 
 static void _lock_memory(lv_operation_t lv_op)
diff --git a/lib/locking/no_locking.c b/lib/locking/no_locking.c
index f4a14ad..85374b3 100644
--- a/lib/locking/no_locking.c
+++ b/lib/locking/no_locking.c
@@ -27,12 +27,10 @@
 
 static void _no_fin_locking(void)
 {
-	return;
 }
 
 static void _no_reset_locking(void)
 {
-	return;
 }
 
 static int _no_lock_resource(struct cmd_context *cmd, const char *resource,
diff --git a/lib/misc/lvm-string.c b/lib/misc/lvm-string.c
index c75a9a7..5a7de96 100644
--- a/lib/misc/lvm-string.c
+++ b/lib/misc/lvm-string.c
@@ -125,7 +125,7 @@ static void _unquote_characters(char *src, const int orig_char,
  */
 static void _quote_hyphens(char **out, const char *src)
 {
-	return _quote_characters(out, src, '-', '-', 0);
+	_quote_characters(out, src, '-', '-', 0);
 }
 
 /*
diff --git a/libdm/libdm-deptree.c b/libdm/libdm-deptree.c
index a596c83..2691c38 100644
--- a/libdm/libdm-deptree.c
+++ b/libdm/libdm-deptree.c
@@ -268,7 +268,7 @@ static int _add_to_toplevel(struct dm_tree_node *node)
 
 static void _remove_from_toplevel(struct dm_tree_node *node)
 {
-	return _unlink_nodes(&node->dtree->root, node);
+	_unlink_nodes(&node->dtree->root, node);
 }
 
 static int _add_to_bottomlevel(struct dm_tree_node *node)
@@ -278,7 +278,7 @@ static int _add_to_bottomlevel(struct dm_tree_node *node)
 
 static void _remove_from_bottomlevel(struct dm_tree_node *node)
 {
-	return _unlink_nodes(node, &node->dtree->root);
+	_unlink_nodes(node, &node->dtree->root);
 }
 
 static int _link_tree_nodes(struct dm_tree_node *parent, struct dm_tree_node *child)
diff --git a/tools/dmsetup.c b/tools/dmsetup.c
index b5568f0..3232b86 100644
--- a/tools/dmsetup.c
+++ b/tools/dmsetup.c
@@ -2571,7 +2571,6 @@ static void _usage(FILE *out)
 	fprintf(out, "Tree options are: ascii, utf, vt100; compact, inverted, notrunc;\n"
 		     "                  [no]device, active, open, rw and uuid.\n");
 	fprintf(out, "\n");
-	return;
 }
 
 static void _losetup_usage(FILE *out)
diff --git a/tools/lvmcmdlib.c b/tools/lvmcmdlib.c
index bbdf08c..580aad9 100644
--- a/tools/lvmcmdlib.c
+++ b/tools/lvmcmdlib.c
@@ -102,8 +102,6 @@ void lvm2_log_level(void *handle, int level)
 	struct cmd_context *cmd = (struct cmd_context *) handle;
 
 	cmd->default_settings.verbose = level - VERBOSE_BASE_LEVEL;
-
-	return;
 }
 
 void lvm2_log_fn(lvm2_log_fn_t log_fn)
diff --git a/tools/pvscan.c b/tools/pvscan.c
index 775ba29..cbeccb7 100644
--- a/tools/pvscan.c
+++ b/tools/pvscan.c
@@ -94,7 +94,6 @@ static void _pvscan_display_single(struct cmd_context *cmd,
 		  display_size(cmd, (uint64_t) (pv_pe_count(pv) -
 						pv_pe_alloc_count(pv)) *
 					   pv_pe_size(pv)));
-	return;
 }
 
 int pvscan(struct cmd_context *cmd, int argc __attribute((unused)),
-- 
1.6.5.3

[PATCH 4/4] Call dm_pool_destroy on allocation error path

[Zdenek Kabelac]

Do not leave allocated mem pool in error path

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

diff --git a/lib/metadata/metadata.c b/lib/metadata/metadata.c
index 44a8a0a..65c11c4 100644
--- a/lib/metadata/metadata.c
+++ b/lib/metadata/metadata.c
@@ -2380,7 +2380,7 @@ static struct volume_group *_vg_read_orphans(struct cmd_context *cmd,
 
 	if (!(vg = dm_pool_zalloc(mem, sizeof(*vg)))) {
 		log_error("vg allocation failed");
-		return NULL;
+		goto bad;
 	}
 	dm_list_init(&vg->pvs);
 	dm_list_init(&vg->lvs);
-- 
1.6.5.3

Re: [PATCH 1/4] Fix coredump and memory leak for 'dmsetup help -c'

[Zdenek Kabelac]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Dne 10.12.2009 13:53, Zdenek Kabelac napsal(a):
> Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>
> ---
>  tools/dmsetup.c |    6 ++++--
>  1 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/dmsetup.c b/tools/dmsetup.c
> index 888edff..b5568f0 100644
> --- a/tools/dmsetup.c
> +++ b/tools/dmsetup.c
> @@ -2402,7 +2402,7 @@ static int _report_init(struct command *c)
>  	size_t len = 0;
>  	int r = 0;
>  
> -	if (!strcmp(c->name, "splitname"))
> +	if (c && !strcmp(c->name, "splitname"))
>  		options = (char *) splitname_report_options;
>  
>  	/* emulate old dmsetup behaviour */
> @@ -2591,7 +2591,9 @@ static int _help(int argc __attribute((unused)),
>  		_switches[OPTIONS_ARG] = 1;
>  		_string_args[OPTIONS_ARG] = (char *) "help";
>  		_switches[SORT_ARG] = 0;
> -	
> +
> +		if (_report)
> +			dm_report_free(_report);
>  		(void) _report_init(NULL);
>  	}
>  

To stay safe even in case of memory failure inside of _report_init:
- --
- -	
+
+		if (_report) {
+			dm_report_free(_report);
+			_report = NULL;
+		}
- --

Zdenek


-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.13 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iQIcBAEBAgAGBQJLIPiQAAoJEBiFrTh9QvFuPIwQALCWEqesTlha+wVDWzXY4We5
Xr6DRXZwYZpFQJS8fJWceOiU1QBHAArPizrpASLWLnC4vvVjqP4vAk+ifSE5mktz
qEDu+rSbJW2GBvEsyo2C3dm3tIqvd9GN7Z5OQ8ECgLqdqeN+oz4WUf8Jk/s5RYvI
Y8eIC2UVrPEEE0MjGgogPc5nH8mrBr27IPmqa7TXOLFht9RajTfcJkFlrilVnaSs
vLPpj2w4xWMcNzqHDlvV+AHU92KZA8fFq72dgzNEsRuXFxpfMKMXALT9pMLE1oAv
5KOMWdSsG+qtoyr3Ivb1YFQUgHdOLhLJIz0RbTd51K6NH8NIdmCxeuyjCBErS/Yo
L5AbO6VlhNyx7BWwFaOP6s3MVBkLgmo62KsAy1cJoBqFc/KbgSvZLWNbnwP0DqkS
jHZ4vhEU907A+99h1HM0Oze40ngNqUbSIunA+/pyKuqCjDjYoQNYMPHNYaZCMHb7
rS/r+RWU5zJAqXipXXD0YbMEb0CFRJqjTQ4YTUoIVULZ5hNRKOs2dO0ARh/C8lx/
lLy2eRz1ROb8GR4De76bCec4TUVBQSoqqLXwBU2Jy/XLw3vZB+ve66VYXBtoqsj1
4hbjSOzqduixO2mL7Ll3KQVJL66ocKmLOgNlbLJcgLYJwPw9Pw/bvxvk4VZ8eS2H
mAF1XWnA8IavteF8wVlF
=7rFB
-----END PGP SIGNATURE-----

[PATCH 0/4] Fixes and cleanups

[Milan Broz]

Well, these are all cleanup patches + segfault fix,
ACK.

Milan


On 12/10/2009 01:53 PM, Zdenek Kabelac wrote:
> Zdenek Kabelac (4):
>   Fix coredump and memory leak for 'dmsetup help -c'
>   Fix unlocking for vgrelease for error paths
>   Skip 'return' keyword for void() functions
>   Call dm_pool_destroy on allocation error path
> 
>  lib/display/display.c           |   11 -----------
>  lib/filters/filter-persistent.c |    2 --
>  lib/filters/filter.c            |    1 -
>  lib/format1/format1.c           |    1 -
>  lib/format1/lvm1-label.c        |    1 -
>  lib/format_pool/format_pool.c   |    1 -
>  lib/format_pool/pool_label.c    |    1 -
>  lib/format_text/format-text.c   |    1 -
>  lib/locking/locking.c           |    4 ----
>  lib/locking/no_locking.c        |    2 --
>  lib/metadata/metadata.c         |    2 +-
>  lib/misc/lvm-string.c           |    2 +-
>  libdm/libdm-deptree.c           |    4 ++--
>  tools/dmsetup.c                 |    7 ++++---
>  tools/lvmcmdlib.c               |    2 --
>  tools/pvresize.c                |    8 ++++++--
>  tools/pvscan.c                  |    1 -
>  tools/toollib.c                 |    6 +++---
>  18 files changed, 17 insertions(+), 40 deletions(-)