[PATCH 00/29] Fixes for analyzer problems

[PATCH 00/29] Fixes for analyzer problems

[Zdenek Kabelac]

Another patchset with fixes based on static analyzer.

Close inspection required for patches:

0010 - Add some backtrace - Attention please
0013 - Put some FIXME warnings in lvmcache_update_vg processing
0026 - Add stack traces for dev_set/close_immediate error path

It's not completly obvious what we should exactly do.

Zdenek Kabelac (29):
  Cleanup remove test for NULL
  Fix check for empty system_dir
  Remove printing of LCK_CACHE
  Reset vg pointer after release
  Test *buf for NULL
  Replace  snprintf -> dm_snprintf
  Test success from dm_poll_create
  Fix memory leak in error path
  Remove check for lv is NULL
  Add some backtrace - Attention please
  Add stack trace for error path
  Add test for 'read' result
  Put some FIXME warnings in lvmcache_update_vg processing
  Remove unneeded check for NULL pvd->system_id
  Modify test to catch passing NULL pointer
  Test uuid for NULL
  Optimize second call to strchr with same parameters
  Check result of vginfo_from_vgname
  Test for error status
  Add test for lv_name not NULL
  Instrument with nonnull  dev_manager_transient
  Ensure pointer first is notnull before dereference
  Add test and error message for failure case
  Test for str_list_add
  Check for unlink result
  Add stack traces for dev_set/close_immediate error path
  Add standard check for result of lv_info call
  Check type is not NULL before access
  Check for NULL pointer

 daemons/clvmd/clvmd-command.c   |   20 +++++++-------
 daemons/clvmd/clvmd.c           |    9 ++++--
 daemons/clvmd/lvm-functions.c   |    5 +--
 lib/activate/dev_manager.c      |   15 +++++-----
 lib/activate/dev_manager.h      |    2 +-
 lib/cache/lvmcache.c            |    6 ++++
 lib/commands/toolcontext.c      |    2 +-
 lib/config/config.c             |   10 +++++-
 lib/format1/disk-rep.c          |    2 +-
 lib/format1/format1.c           |    8 +++---
 lib/format1/import-export.c     |    2 +-
 lib/format_pool/import_export.c |   10 +++++-
 lib/format_text/archiver.c      |    4 ++-
 lib/format_text/export.c        |   56 ++++++++++++++++++--------------------
 lib/metadata/lv_manip.c         |    8 ++++-
 lib/metadata/metadata.c         |    6 +++-
 lib/mirror/mirrored.c           |   12 +++++++-
 libdm/ioctl/libdm-iface.c       |   15 +++++++---
 libdm/libdm-common.c            |   14 +++-------
 libdm/libdm-report.c            |    6 ++++
 tools/dmsetup.c                 |    4 ++-
 tools/lvmcmdline.c              |   12 +++++---
 tools/reporter.c                |    4 +++
 tools/toollib.c                 |    7 +++--
 24 files changed, 143 insertions(+), 96 deletions(-)

-- 
1.7.3.2

[PATCH 01/29] Cleanup remove test for NULL

[Zdenek Kabelac]

dm_free test for NULL itself.

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

diff --git a/libdm/libdm-common.c b/libdm/libdm-common.c
index b86739a..2dfa28f 100644
--- a/libdm/libdm-common.c
+++ b/libdm/libdm-common.c
@@ -250,10 +250,8 @@ int dm_task_set_name(struct dm_task *dmt, const char *name)
 	char path[PATH_MAX];
 	struct stat st1, st2;
 
-	if (dmt->dev_name) {
-		dm_free(dmt->dev_name);
-		dmt->dev_name = NULL;
-	}
+	dm_free(dmt->dev_name);
+	dmt->dev_name = NULL;
 
 	/*
 	 * Path supplied for existing device?
@@ -292,8 +290,7 @@ int dm_task_set_name(struct dm_task *dmt, const char *name)
 
 	if (strlen(name) >= DM_NAME_LEN) {
 		log_error("Name \"%s\" too long", name);
-		if (new_name)
-			dm_free(new_name);
+		dm_free(new_name);
 		return 0;
 	}
 
@@ -309,10 +306,7 @@ int dm_task_set_name(struct dm_task *dmt, const char *name)
 
 int dm_task_set_uuid(struct dm_task *dmt, const char *uuid)
 {
-	if (dmt->uuid) {
-		dm_free(dmt->uuid);
-		dmt->uuid = NULL;
-	}
+	dm_free(dmt->uuid);
 
 	if (!(dmt->uuid = dm_strdup(uuid))) {
 		log_error("dm_task_set_uuid: strdup(%s) failed", uuid);
-- 
1.7.3.2

[PATCH 01/29] Cleanup remove test for NULL

[Petr Rockai]

Zdenek Kabelac <zkabelac@redhat.com> writes:

> dm_free test for NULL itself.
Ack.

[PATCH 02/29] Fix check for empty system_dir

[Zdenek Kabelac]

Fixing most probably a typo - I assume original intention has been
to check for zero length string.

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

diff --git a/lib/commands/toolcontext.c b/lib/commands/toolcontext.c
index f714cdf..b8da161 100644
--- a/lib/commands/toolcontext.c
+++ b/lib/commands/toolcontext.c
@@ -1036,7 +1036,7 @@ static int _init_backup(struct cmd_context *cmd)
 	char default_dir[PATH_MAX];
 	const char *dir;
 
-	if (!cmd->system_dir) {
+	if (!*cmd->system_dir) {
 		log_warn("WARNING: Metadata changes will NOT be backed up");
 		backup_init(cmd, "", 0);
 		archive_init(cmd, "", 0, 0, 0);
-- 
1.7.3.2

[PATCH 02/29] Fix check for empty system_dir

[Petr Rockai]

Zdenek Kabelac <zkabelac@redhat.com> writes:

> Fixing most probably a typo - I assume original intention has been
> to check for zero length string.

In that case, please use !strlen(str), as that is much more readable.

> @@ -1036,7 +1036,7 @@ static int _init_backup(struct cmd_context *cmd)
>  	char default_dir[PATH_MAX];
>  	const char *dir;
>
> -	if (!cmd->system_dir) {
> +	if (!*cmd->system_dir) {
>  		log_warn("WARNING: Metadata changes will NOT be backed up");
>  		backup_init(cmd, "", 0);
>  		archive_init(cmd, "", 0, 0, 0);

[PATCH 02/29] Fix check for empty system_dir

[Zdenek Kabelac]

Dne 25.11.2010 18:19, Petr Rockai napsal(a):
> Zdenek Kabelac <zkabelac@redhat.com> writes:
> 
>> Fixing most probably a typo - I assume original intention has been
>> to check for zero length string.
> 
> In that case, please use !strlen(str), as that is much more readable.
> 

In this case I believe it's common practice not only in our source to check
the first char for '\0' when checking for empty string - so are we planning to
replace in other places in lvm code - then I'd propose probably some nice
macro  empty_string() or whatever name we could think of?


Zdenek

[PATCH 02/29] Fix check for empty system_dir

[Petr Rockai]

Zdenek Kabelac <zkabelac@redhat.com> writes:

> Dne 25.11.2010 18:19, Petr Rockai napsal(a):
>> Zdenek Kabelac <zkabelac@redhat.com> writes:
>> 
>>> Fixing most probably a typo - I assume original intention has been
>>> to check for zero length string.
>> 
>> In that case, please use !strlen(str), as that is much more readable.
>> 
>
> In this case I believe it's common practice not only in our source to check
> the first char for '\0' when checking for empty string - so are we planning to
> replace in other places in lvm code - then I'd propose probably some nice
> macro  empty_string() or whatever name we could think of?

Maybe go for !str[0] at least. But strempty sounds reasonable, IMHO. On
the other hand, I am not extremely happy with the use of ! instead of ==
0, either. I know, it's the preferred style in LVM...

Anyway, nevermind. Do as you think. :) The patch is technically OK and
it doesn't make sense to hold it back on style.

Yours,
   Petr.

[PATCH 02/29] Fix check for empty system_dir

[Alasdair G Kergon]

> > In that case, please use !strlen(str), as that is much more readable.
> In this case I believe it's common practice not only in our source to check
> the first char for '\0' when checking for empty string - so are we planning to
 
We use *str or str[0] in lots of places - it's pretty common practice I think
and shouldn't really surprise anyone.  !strlen(str) is also fine by me
where that adds clarity.

Alasdair

[PATCH 03/29] Remove printing of LCK_CACHE

[Zdenek Kabelac]

LCK_CACHE is defined as 0x100 so it cannot be passed through
unsigned char parameter - remove it from the sprintf code.

If LCK_CLUSTER would be desirable to be printed here - lot of code
would neet to be reworked.

Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>
---
 daemons/clvmd/lvm-functions.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/daemons/clvmd/lvm-functions.c b/daemons/clvmd/lvm-functions.c
index 20b25a6..51a4cdb 100644
--- a/daemons/clvmd/lvm-functions.c
+++ b/daemons/clvmd/lvm-functions.c
@@ -109,12 +109,11 @@ static const char *decode_locking_cmd(unsigned char cmdl)
 		break;
 	}
 
-	sprintf(buf, "0x%x %s (%s|%s%s%s%s%s%s)", cmdl, command, type, scope,
+	sprintf(buf, "0x%x %s (%s|%s%s%s%s%s)", cmdl, command, type, scope,
 		cmdl & LCK_NONBLOCK   ? "|NONBLOCK" : "",
 		cmdl & LCK_HOLD       ? "|HOLD" : "",
 		cmdl & LCK_LOCAL      ? "|LOCAL" : "",
-		cmdl & LCK_CLUSTER_VG ? "|CLUSTER_VG" : "",
-		cmdl & LCK_CACHE      ? "|CACHE" : "");
+		cmdl & LCK_CLUSTER_VG ? "|CLUSTER_VG" : "");
 
 	return buf;
 }
-- 
1.7.3.2

[PATCH 03/29] Remove printing of LCK_CACHE

[Petr Rockai]

Zdenek Kabelac <zkabelac@redhat.com> writes:

> LCK_CACHE is defined as 0x100 so it cannot be passed through
> unsigned char parameter - remove it from the sprintf code.

> If LCK_CLUSTER would be desirable to be printed here - lot of code
> would neet to be reworked.

Okey. It doesn't make me extremely happy to go this way, but I see how
it could be confusing as it is, and complicated to fix it properly. It
would be however desirable, at least, to add a comment to
decode_locking_cmd explaining the situation.

>
> Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>
> ---
>  daemons/clvmd/lvm-functions.c |    5 ++---
>  1 files changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/daemons/clvmd/lvm-functions.c b/daemons/clvmd/lvm-functions.c
> index 20b25a6..51a4cdb 100644
> --- a/daemons/clvmd/lvm-functions.c
> +++ b/daemons/clvmd/lvm-functions.c
> @@ -109,12 +109,11 @@ static const char *decode_locking_cmd(unsigned char cmdl)
>  		break;
>  	}
>  
> -	sprintf(buf, "0x%x %s (%s|%s%s%s%s%s%s)", cmdl, command, type, scope,
> +	sprintf(buf, "0x%x %s (%s|%s%s%s%s%s)", cmdl, command, type, scope,
>  		cmdl & LCK_NONBLOCK   ? "|NONBLOCK" : "",
>  		cmdl & LCK_HOLD       ? "|HOLD" : "",
>  		cmdl & LCK_LOCAL      ? "|LOCAL" : "",
> -		cmdl & LCK_CLUSTER_VG ? "|CLUSTER_VG" : "",
> -		cmdl & LCK_CACHE      ? "|CACHE" : "");
> +		cmdl & LCK_CLUSTER_VG ? "|CLUSTER_VG" : "");
>  
>  	return buf;
>  }

[PATCH 03/29] Remove printing of LCK_CACHE

[Alasdair G Kergon]

On Thu, Nov 25, 2010 at 11:55:07AM +0100, Zdenek Kabelac wrote:
> LCK_CACHE is defined as 0x100 so it cannot be passed through
> unsigned char parameter - remove it from the sprintf code.
 
> If LCK_CLUSTER would be desirable to be printed here - lot of code
> would neet to be reworked.
 
See follow up to commit message.  This one needs more work.

Alasdair

[PATCH 04/29] Reset vg pointer after release

[Zdenek Kabelac]

Set vg to NULL after releasing it as the following memlock() test may
lead to goto for the second call of vg_release() with the same vg pointer.

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

diff --git a/lib/metadata/metadata.c b/lib/metadata/metadata.c
index 780b806..743d633 100644
--- a/lib/metadata/metadata.c
+++ b/lib/metadata/metadata.c
@@ -3117,6 +3117,7 @@ static struct volume_group *_vg_read_by_vgid(struct cmd_context *cmd,
 			return vg;
 		}
 		vg_release(vg);
+		vg = NULL; /* reset so memlock goto out is safe */
 	}
 
 	/* Mustn't scan if memory locked: ensure cache gets pre-populated! */
-- 
1.7.3.2

[PATCH 04/29] Reset vg pointer after release

[Petr Rockai]

Zdenek Kabelac <zkabelac@redhat.com> writes:

> Set vg to NULL after releasing it as the following memlock() test may
> lead to goto for the second call of vg_release() with the same vg pointer.
Ack.

> Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>
> ---
>  lib/metadata/metadata.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/lib/metadata/metadata.c b/lib/metadata/metadata.c
> index 780b806..743d633 100644
> --- a/lib/metadata/metadata.c
> +++ b/lib/metadata/metadata.c
> @@ -3117,6 +3117,7 @@ static struct volume_group *_vg_read_by_vgid(struct cmd_context *cmd,
>  			return vg;
>  		}
>  		vg_release(vg);
> +		vg = NULL; /* reset so memlock goto out is safe */
>  	}
>  
>  	/* Mustn't scan if memory locked: ensure cache gets pre-populated! */

[PATCH 05/29] Test *buf for NULL

[Zdenek Kabelac]

As we reallocate *buf in CLVMD_CMD_TEST we need to test for NULL
to print status to avoid printing to NULL buffer.
(realloc() == NULL & status != 0)

CHECKME:
General question - what is supposed to be written in *retlen for  *buf == NULL?

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

diff --git a/daemons/clvmd/clvmd-command.c b/daemons/clvmd/clvmd-command.c
index 3500ca5..8d1dcff 100644
--- a/daemons/clvmd/clvmd-command.c
+++ b/daemons/clvmd/clvmd-command.c
@@ -169,7 +169,8 @@ int do_command(struct local_client *client, struct clvm_header *msg, int msglen,
 
 	/* Check the status of the command and return the error text */
 	if (status) {
-		*retlen = 1 + snprintf(*buf, buflen, "%s", strerror(status));
+		*retlen = 1 + (*buf) ? snprintf(*buf, buflen, "%s",
+						strerror(status)) : -1;
 	}
 
 	return status;
-- 
1.7.3.2

[PATCH 05/29] Test *buf for NULL

[Alasdair G Kergon]

On Thu, Nov 25, 2010 at 11:55:09AM +0100, Zdenek Kabelac wrote:
> As we reallocate *buf in CLVMD_CMD_TEST we need to test for NULL
> to print status to avoid printing to NULL buffer.
> (realloc() == NULL & status != 0)

Ack for now.

> CHECKME:
> General question - what is supposed to be written in *retlen for  *buf == NULL?
 
The callers initialise it to 0 so it should stay 0 and the callers ought to
cope...

Alasdair

[PATCH 06/29] Replace snprintf -> dm_snprintf

[Zdenek Kabelac]

Use dm_snprintf with known error case behavior - return -1.

Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>
---
 daemons/clvmd/clvmd-command.c |   21 ++++++++++-----------
 1 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/daemons/clvmd/clvmd-command.c b/daemons/clvmd/clvmd-command.c
index 8d1dcff..915fcc7 100644
--- a/daemons/clvmd/clvmd-command.c
+++ b/daemons/clvmd/clvmd-command.c
@@ -97,10 +97,10 @@ int do_command(struct local_client *client, struct clvm_header *msg, int msglen,
 		}
 		if (*buf) {
 			uname(&nodeinfo);
-			*retlen = 1 + snprintf(*buf, buflen,
-					       "TEST from %s: %s v%s",
-					       nodeinfo.nodename, args,
-					       nodeinfo.release);
+			*retlen = 1 + dm_snprintf(*buf, buflen,
+						  "TEST from %s: %s v%s",
+						  nodeinfo.nodename, args,
+						  nodeinfo.release);
 		}
 		break;
 
@@ -121,9 +121,8 @@ int do_command(struct local_client *client, struct clvm_header *msg, int msglen,
 		status = do_lock_lv(lock_cmd, lock_flags, lockname);
 		/* Replace EIO with something less scary */
 		if (status == EIO) {
-			*retlen =
-			    1 + snprintf(*buf, buflen, "%s",
-					 get_last_lvm_error());
+			*retlen = 1 + dm_snprintf(*buf, buflen, "%s",
+						  get_last_lvm_error());
 			return EIO;
 		}
 		break;
@@ -133,7 +132,7 @@ int do_command(struct local_client *client, struct clvm_header *msg, int msglen,
 		if (buflen < 3)
 			return EIO;
 		if ((locktype = do_lock_query(lockname)))
-			*retlen = 1 + snprintf(*buf, buflen, "%s", locktype);
+			*retlen = 1 + dm_snprintf(*buf, buflen, "%s", locktype);
 		break;
 
 	case CLVMD_CMD_REFRESH:
@@ -169,8 +168,8 @@ int do_command(struct local_client *client, struct clvm_header *msg, int msglen,
 
 	/* Check the status of the command and return the error text */
 	if (status) {
-		*retlen = 1 + (*buf) ? snprintf(*buf, buflen, "%s",
-						strerror(status)) : -1;
+		*retlen = 1 + (*buf) ? dm_snprintf(*buf, buflen, "%s",
+						   strerror(status)) : -1;
 	}
 
 	return status;
@@ -377,7 +376,7 @@ static int restart_clvmd(void)
 	/* Propogate debug options */
 	if (debug) {
 		if (!(debug_arg = malloc(16)) ||
-		    snprintf(debug_arg, 16, "-d%d", (int)debug) < 0)
+		    dm_snprintf(debug_arg, 16, "-d%d", (int)debug) < 0)
 			goto_out;
 		argv[argc++] = debug_arg;
 	}
-- 
1.7.3.2

[PATCH 06/29] Replace snprintf -> dm_snprintf

[Alasdair G Kergon]

On Thu, Nov 25, 2010 at 11:55:10AM +0100, Zdenek Kabelac wrote:
> Use dm_snprintf with known error case behavior - return -1.
 
Ack.

Alasdair

[PATCH 07/29] Test success from dm_poll_create

[Zdenek Kabelac]

Add testing for NULL return value from dm_poll_create.

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

diff --git a/lib/config/config.c b/lib/config/config.c
index c10c78e..4ecec94 100644
--- a/lib/config/config.c
+++ b/lib/config/config.c
@@ -511,7 +511,10 @@ int write_config_node(const struct config_node *cn, putline_fn putline, void *ba
 {
 	struct output_line outline;
 	outline.fp = NULL;
-	outline.mem = dm_pool_create("config_line", 1024);
+	if (!(outline.mem = dm_pool_create("config_line", 1024))) {
+		log_error("Failed to allocate config_line pool.");
+		return 0;
+	}
 	outline.putline = putline;
 	outline.putline_baton = baton;
 	if (!_write_config(cn, 0, &outline, 0)) {
@@ -538,7 +541,10 @@ int write_config_file(struct config_tree *cft, const char *file,
 		return 0;
 	}
 
-	outline.mem = dm_pool_create("config_line", 1024);
+	if (!(outline.mem = dm_pool_create("config_line", 1024))) {
+		log_error("Failed to allocate config_line pool.");
+		return 0;
+	}
 
 	log_verbose("Dumping configuration to %s", file);
 	if (!argc) {
-- 
1.7.3.2

[PATCH 07/29] Test success from dm_poll_create

[Alasdair G Kergon]

On Thu, Nov 25, 2010 at 11:55:11AM +0100, Zdenek Kabelac wrote:
> Add testing for NULL return value from dm_poll_create.
 
Ack.

Alasdair

[PATCH 08/29] Fix memory leak in error path

[Zdenek Kabelac]

Nicely hidden memory leak in error path.
outf() is macro which uses out_text() and does automagical return_0.
This would leak tag_buffer.

As there was a repeating code for tags output - create _out_tags().

Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>
---
 lib/format_text/export.c |   56 ++++++++++++++++++++++-----------------------
 1 files changed, 27 insertions(+), 29 deletions(-)

diff --git a/lib/format_text/export.c b/lib/format_text/export.c
index d49090c..8ddfa85 100644
--- a/lib/format_text/export.c
+++ b/lib/format_text/export.c
@@ -363,10 +363,27 @@ static int _print_flag_config(struct formatter *f, uint64_t status, int type)
 	return 1;
 }
 
+
+static int _out_tags(struct formatter *f, struct dm_list *tags)
+{
+	char *tag_buffer;
+
+	if (!dm_list_empty(tags)) {
+		if (!(tag_buffer = alloc_printed_tags(tags)))
+			return_0;
+		if (!out_text(f, "tags = %s", tag_buffer)) {
+			dm_free(tag_buffer);
+			return_0;
+		}
+		dm_free(tag_buffer);
+	}
+
+	return 1;
+}
+
 static int _print_vg(struct formatter *f, struct volume_group *vg)
 {
 	char buffer[4096];
-	char *tag_buffer = NULL;
 
 	if (!id_write_format(&vg->id, buffer, sizeof(buffer)))
 		return_0;
@@ -378,12 +395,8 @@ static int _print_vg(struct formatter *f, struct volume_group *vg)
 	if (!_print_flag_config(f, vg->status, VG_FLAGS))
 		return_0;
 
-	if (!dm_list_empty(&vg->tags)) {
-		if (!(tag_buffer = alloc_printed_tags(&vg->tags)))
-			return_0;
-		outf(f, "tags = %s", tag_buffer);
-		dm_free(tag_buffer);
-	}
+	if (!_out_tags(f, &vg->tags))
+		return_0;
 
 	if (vg->system_id && *vg->system_id)
 		outf(f, "system_id = \"%s\"", vg->system_id);
@@ -428,7 +441,7 @@ static int _print_pvs(struct formatter *f, struct volume_group *vg)
 	struct pv_list *pvl;
 	struct physical_volume *pv;
 	char buffer[4096];
-	char *buf, *tag_buffer = NULL;
+	char *buf;
 	const char *name;
 
 	outf(f, "physical_volumes {");
@@ -462,12 +475,8 @@ static int _print_pvs(struct formatter *f, struct volume_group *vg)
 		if (!_print_flag_config(f, pv->status, PV_FLAGS))
 			return_0;
 
-		if (!dm_list_empty(&pv->tags)) {
-			if (!(tag_buffer = alloc_printed_tags(&pv->tags)))
-				return_0;
-			outf(f, "tags = %s", tag_buffer);
-			dm_free(tag_buffer);
-		}
+		if (!_out_tags(f, &pv->tags))
+			return_0;
 
 		outsize(f, pv->size, "dev_size = %" PRIu64, pv->size);
 
@@ -487,8 +496,6 @@ static int _print_pvs(struct formatter *f, struct volume_group *vg)
 static int _print_segment(struct formatter *f, struct volume_group *vg,
 			  int count, struct lv_segment *seg)
 {
-	char *tag_buffer = NULL;
-
 	outf(f, "segment%u {", count);
 	_inc_indent(f);
 
@@ -499,12 +506,8 @@ static int _print_segment(struct formatter *f, struct volume_group *vg,
 	outnl(f);
 	outf(f, "type = \"%s\"", seg->segtype->name);
 
-	if (!dm_list_empty(&seg->tags)) {
-		if (!(tag_buffer = alloc_printed_tags(&seg->tags)))
-			return_0;
-		outf(f, "tags = %s", tag_buffer);
-		dm_free(tag_buffer);
-	}
+	if (!_out_tags(f, &seg->tags))
+		return_0;
 
 	if (seg->segtype->ops->text_export &&
 	    !seg->segtype->ops->text_export(seg, f))
@@ -557,7 +560,6 @@ static int _print_lv(struct formatter *f, struct logical_volume *lv)
 {
 	struct lv_segment *seg;
 	char buffer[4096];
-	char *tag_buffer = NULL;
 	int seg_count;
 
 	outnl(f);
@@ -573,12 +575,8 @@ static int _print_lv(struct formatter *f, struct logical_volume *lv)
 	if (!_print_flag_config(f, lv->status, LV_FLAGS))
 		return_0;
 
-	if (!dm_list_empty(&lv->tags)) {
-		if (!(tag_buffer = alloc_printed_tags(&lv->tags)))
-			return_0;
-		outf(f, "tags = %s", tag_buffer);
-		dm_free(tag_buffer);
-	}
+	if (!_out_tags(f, &lv->tags))
+		return_0;
 
 	if (lv->alloc != ALLOC_INHERIT)
 		outf(f, "allocation_policy = \"%s\"",
-- 
1.7.3.2

[PATCH 08/29] Fix memory leak in error path

[Petr Rockai]

Zdenek Kabelac <zkabelac@redhat.com> writes:

> Nicely hidden memory leak in error path.
> outf() is macro which uses out_text() and does automagical return_0.
> This would leak tag_buffer.
>
> As there was a repeating code for tags output - create _out_tags().
>
> Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>
Ack.

(I haven't tried the patch, but as long as it compiles and passes
testsuite, it should be good to go.)

> --- a/lib/format_text/export.c
> +++ b/lib/format_text/export.c
> @@ -363,10 +363,27 @@ static int _print_flag_config(struct formatter *f, uint64_t status, int type)
>  	return 1;
>  }
>  
> +
> +static int _out_tags(struct formatter *f, struct dm_list *tags)
> +{
> +	char *tag_buffer;
> +
> +	if (!dm_list_empty(tags)) {
> +		if (!(tag_buffer = alloc_printed_tags(tags)))
> +			return_0;
> +		if (!out_text(f, "tags = %s", tag_buffer)) {
> +			dm_free(tag_buffer);
> +			return_0;
> +		}
> +		dm_free(tag_buffer);
> +	}
> +
> +	return 1;
> +}
> +
>  static int _print_vg(struct formatter *f, struct volume_group *vg)
>  {
>  	char buffer[4096];
> -	char *tag_buffer = NULL;
>  
>  	if (!id_write_format(&vg->id, buffer, sizeof(buffer)))
>  		return_0;
> @@ -378,12 +395,8 @@ static int _print_vg(struct formatter *f, struct volume_group *vg)
>  	if (!_print_flag_config(f, vg->status, VG_FLAGS))
>  		return_0;
>  
> -	if (!dm_list_empty(&vg->tags)) {
> -		if (!(tag_buffer = alloc_printed_tags(&vg->tags)))
> -			return_0;
> -		outf(f, "tags = %s", tag_buffer);
> -		dm_free(tag_buffer);
> -	}
> +	if (!_out_tags(f, &vg->tags))
> +		return_0;
>  
>  	if (vg->system_id && *vg->system_id)
>  		outf(f, "system_id = \"%s\"", vg->system_id);
> @@ -428,7 +441,7 @@ static int _print_pvs(struct formatter *f, struct volume_group *vg)
>  	struct pv_list *pvl;
>  	struct physical_volume *pv;
>  	char buffer[4096];
> -	char *buf, *tag_buffer = NULL;
> +	char *buf;
>  	const char *name;
>  
>  	outf(f, "physical_volumes {");
> @@ -462,12 +475,8 @@ static int _print_pvs(struct formatter *f, struct volume_group *vg)
>  		if (!_print_flag_config(f, pv->status, PV_FLAGS))
>  			return_0;
>  
> -		if (!dm_list_empty(&pv->tags)) {
> -			if (!(tag_buffer = alloc_printed_tags(&pv->tags)))
> -				return_0;
> -			outf(f, "tags = %s", tag_buffer);
> -			dm_free(tag_buffer);
> -		}
> +		if (!_out_tags(f, &pv->tags))
> +			return_0;
>  
>  		outsize(f, pv->size, "dev_size = %" PRIu64, pv->size);
>  
> @@ -487,8 +496,6 @@ static int _print_pvs(struct formatter *f, struct volume_group *vg)
>  static int _print_segment(struct formatter *f, struct volume_group *vg,
>  			  int count, struct lv_segment *seg)
>  {
> -	char *tag_buffer = NULL;
> -
>  	outf(f, "segment%u {", count);
>  	_inc_indent(f);
>  
> @@ -499,12 +506,8 @@ static int _print_segment(struct formatter *f, struct volume_group *vg,
>  	outnl(f);
>  	outf(f, "type = \"%s\"", seg->segtype->name);
>  
> -	if (!dm_list_empty(&seg->tags)) {
> -		if (!(tag_buffer = alloc_printed_tags(&seg->tags)))
> -			return_0;
> -		outf(f, "tags = %s", tag_buffer);
> -		dm_free(tag_buffer);
> -	}
> +	if (!_out_tags(f, &seg->tags))
> +		return_0;
>  
>  	if (seg->segtype->ops->text_export &&
>  	    !seg->segtype->ops->text_export(seg, f))
> @@ -557,7 +560,6 @@ static int _print_lv(struct formatter *f, struct logical_volume *lv)
>  {
>  	struct lv_segment *seg;
>  	char buffer[4096];
> -	char *tag_buffer = NULL;
>  	int seg_count;
>  
>  	outnl(f);
> @@ -573,12 +575,8 @@ static int _print_lv(struct formatter *f, struct logical_volume *lv)
>  	if (!_print_flag_config(f, lv->status, LV_FLAGS))
>  		return_0;
>  
> -	if (!dm_list_empty(&lv->tags)) {
> -		if (!(tag_buffer = alloc_printed_tags(&lv->tags)))
> -			return_0;
> -		outf(f, "tags = %s", tag_buffer);
> -		dm_free(tag_buffer);
> -	}
> +	if (!_out_tags(f, &lv->tags))
> +		return_0;
>  
>  	if (lv->alloc != ALLOC_INHERIT)
>  		outf(f, "allocation_policy = \"%s\"",

[PATCH 09/29] Remove check for lv is NULL

[Zdenek Kabelac]

'lv' is deferenced in the begining of the function so any check
later is not helpful.

Parameters for dev_manager_transien() will be marked as nonnull.

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

diff --git a/lib/activate/dev_manager.c b/lib/activate/dev_manager.c
index cbb5378..cbd55f2 100644
--- a/lib/activate/dev_manager.c
+++ b/lib/activate/dev_manager.c
@@ -614,14 +614,13 @@ int dev_manager_transient(struct dev_manager *dm, struct logical_volume *lv)
 	do {
 		next = dm_get_next_target(dmt, next, &start, &length, &type,
 					  &params);
-		if (lv) {
-			if (!(segh = dm_list_next(&lv->segments, segh))) {
-				log_error("Number of segments in active LV %s "
-					  "does not match metadata", lv->name);
-				goto out;
-			}
-			seg = dm_list_item(segh, struct lv_segment);
+
+		if (!(segh = dm_list_next(&lv->segments, segh))) {
+		    log_error("Number of segments in active LV %s "
+			      "does not match metadata", lv->name);
+		    goto out;
 		}
+		seg = dm_list_item(segh, struct lv_segment);
 
 		if (!type || !params)
 			continue;
@@ -632,7 +631,7 @@ int dev_manager_transient(struct dev_manager *dm, struct logical_volume *lv)
 
 	} while (next);
 
-	if (lv && (segh = dm_list_next(&lv->segments, segh))) {
+	if (segh = dm_list_next(&lv->segments, segh)) {
 		log_error("Number of segments in active LV %s does not "
 			  "match metadata", lv->name);
 		goto out;
-- 
1.7.3.2

[PATCH 09/29] Remove check for lv is NULL

[Alasdair G Kergon]

On Thu, Nov 25, 2010 at 11:55:13AM +0100, Zdenek Kabelac wrote:
> 'lv' is deferenced in the begining of the function so any check
> later is not helpful.
 
Caller always supplies it these days, ack.

Alasdair

[PATCH 10/29] Add some backtrace - Attention please

[Zdenek Kabelac]

ATTENTION

It is a bit unclear what should happen on error path.

My idea:
 - skip to the next node if the set_name() fails.
 - print the backtrace for all error cases.

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

diff --git a/libdm/ioctl/libdm-iface.c b/libdm/ioctl/libdm-iface.c
index 8c0115a..147eec2 100644
--- a/libdm/ioctl/libdm-iface.c
+++ b/libdm/ioctl/libdm-iface.c
@@ -1594,8 +1594,12 @@ static int _process_mapper_dir(struct dm_task *dmt)
 		    !strcmp(dirent->d_name, "..") ||
 		    !strcmp(dirent->d_name, "control"))
 			continue;
-		dm_task_set_name(dmt, dirent->d_name);
-		dm_task_run(dmt);
+		if (!dm_task_set_name(dmt, dirent->d_name)) {
+			stack;
+			continue; /* try next name */
+		}
+		if (!dm_task_run(dmt))
+                        stack;  /* keep going */
 	}
 
 	if (closedir(d))
-- 
1.7.3.2

[PATCH 10/29] Add some backtrace - Attention please

[Alasdair G Kergon]

On Thu, Nov 25, 2010 at 11:55:14AM +0100, Zdenek Kabelac wrote:
> -		dm_task_set_name(dmt, dirent->d_name);
> -		dm_task_run(dmt);
> +		if (!dm_task_set_name(dmt, dirent->d_name)) {
> +			stack;
> +			continue; /* try next name */
> +		}
> +		if (!dm_task_run(dmt))
> +                        stack;  /* keep going */

Yes, best endeavours makes sense in this (repair) function.
but add some 'r = 0' to return 0 not 1 if any of them failed.

Ack.

Alasdair

[PATCH 11/29] Add stack trace for error path

[Zdenek Kabelac]

If dm_task_set_cookie fail - print stack trace, but keep going on.

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

diff --git a/libdm/ioctl/libdm-iface.c b/libdm/ioctl/libdm-iface.c
index 147eec2..431aa8f 100644
--- a/libdm/ioctl/libdm-iface.c
+++ b/libdm/ioctl/libdm-iface.c
@@ -1776,9 +1776,10 @@ static int _create_and_load_v4(struct dm_task *dmt)
 	if (dmt->cookie_set) {
 		cookie = (dmt->event_nr & ~DM_UDEV_FLAGS_MASK) |
 			 (DM_COOKIE_MAGIC << DM_UDEV_FLAGS_SHIFT);
-		dm_task_set_cookie(dmt, &cookie,
-				   (dmt->event_nr & DM_UDEV_FLAGS_MASK) >>
-				    DM_UDEV_FLAGS_SHIFT);
+		if (!dm_task_set_cookie(dmt, &cookie,
+					(dmt->event_nr & DM_UDEV_FLAGS_MASK) >>
+					DM_UDEV_FLAGS_SHIFT))
+                        stack; /* keep going */
 	}
 
 	if (!dm_task_run(dmt))
-- 
1.7.3.2

[PATCH 11/29] Add stack trace for error path

[Alasdair G Kergon]

On Thu, Nov 25, 2010 at 11:55:15AM +0100, Zdenek Kabelac wrote:
> If dm_task_set_cookie fail - print stack trace, but keep going on.

Ack.
(Function can't currently fail.)

Alasdair

[PATCH 12/29] Add test for 'read' result

[Zdenek Kabelac]

Static analyzer complains when read result is ignored.

So let's add some small check even when there is very low probablity to
fail in this place.

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

diff --git a/tools/lvmcmdline.c b/tools/lvmcmdline.c
index 192531d..f599613 100644
--- a/tools/lvmcmdline.c
+++ b/tools/lvmcmdline.c
@@ -1191,15 +1191,17 @@ static const char *_get_cmdline(pid_t pid)
 {
 	static char _proc_cmdline[32];
 	char buf[256];
-	int fd;
+	int fd, n = 0;
 
 	snprintf(buf, sizeof(buf), DEFAULT_PROC_DIR "/%u/cmdline", pid);
 	if ((fd = open(buf, O_RDONLY)) > 0) {
-		read(fd, _proc_cmdline, sizeof(_proc_cmdline) - 1);
-		_proc_cmdline[sizeof(_proc_cmdline) - 1] = '\0';
+		if ((n = read(fd, _proc_cmdline, sizeof(_proc_cmdline) - 1)) < 0) {
+			log_sys_error("read", buf);
+			n = 0;
+		}
 		close(fd);
-	} else
-		_proc_cmdline[0] = '\0';
+	}
+	_proc_cmdline[n] = '\0';
 
 	return _proc_cmdline;
 }
-- 
1.7.3.2

[PATCH 12/29] Add test for 'read' result

[Alasdair G Kergon]

On Thu, Nov 25, 2010 at 11:55:16AM +0100, Zdenek Kabelac wrote:
> Static analyzer complains when read result is ignored.
 
Compiler should have complained too! Ack.

>  	snprintf(buf, sizeof(buf), DEFAULT_PROC_DIR "/%u/cmdline", pid);

/* FIXME Use generic read code. */

>  	if ((fd = open(buf, O_RDONLY)) > 0) {
> -		read(fd, _proc_cmdline, sizeof(_proc_cmdline) - 1);


Alasdair

[PATCH 13/29] Put some FIXME warnings in lvmcache_update_vg processing

[Zdenek Kabelac]

It's not clear how this code is supposed to work.
For now we never set INCONSISTENT_VG flag - so marking expression with
big FIXME as it always gives true.
Remove extra paramater from lvmcache_update_vg call - as it effectively
produced always 'false' - so reverting to previous version - where it has been
possible to get also 'true' - use case is probably only in cluster
environment - some testcase is needed.

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

diff --git a/lib/metadata/metadata.c b/lib/metadata/metadata.c
index 743d633..fd5d024 100644
--- a/lib/metadata/metadata.c
+++ b/lib/metadata/metadata.c
@@ -2679,6 +2679,7 @@ static struct volume_group *_vg_read(struct cmd_context *cmd,
 	 * the missing PV logic below.
 	 */
 	if ((correct_vg = lvmcache_get_vg(vgid, precommitted)) &&
+	    /* FIXME: Expression is always TRUE - as we never set INCONSISTENT_VG! */
 	    (use_precommitted || !*consistent || !(correct_vg->status & INCONSISTENT_VG))) {
 		if (!(correct_vg->status & INCONSISTENT_VG))
 			*consistent = 1;
@@ -2935,8 +2936,8 @@ static struct volume_group *_vg_read(struct cmd_context *cmd,
 	 * If there is no precommitted metadata, committed metadata
 	 * is read and stored in the cache even if use_precommitted is set
 	 */
-	lvmcache_update_vg(correct_vg, correct_vg->status & PRECOMMITTED &
-			   (inconsistent ? INCONSISTENT_VG : 0));
+	/* FIXME: How to handle INCONSISTENT_VG? */
+	lvmcache_update_vg(correct_vg, correct_vg->status & PRECOMMITTED);
 
 	if (inconsistent) {
 		/* FIXME Test should be if we're *using* precommitted metadata not if we were searching for it */
-- 
1.7.3.2

[PATCH 13/29] Put some FIXME warnings in lvmcache_update_vg processing

[Zdenek Kabelac]

Dne 25.11.2010 11:55, Zdenek Kabelac napsal(a):
> It's not clear how this code is supposed to work.
> For now we never set INCONSISTENT_VG flag - so marking expression with
> big FIXME as it always gives true.
> Remove extra paramater from lvmcache_update_vg call - as it effectively
> produced always 'false' - so reverting to previous version - where it has been
> possible to get also 'true' - use case is probably only in cluster
> environment - some testcase is needed.
> 
> Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>
> ---
>  lib/metadata/metadata.c |    5 +++--
>  1 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/metadata/metadata.c b/lib/metadata/metadata.c
> index 743d633..fd5d024 100644
> --- a/lib/metadata/metadata.c
> +++ b/lib/metadata/metadata.c
> @@ -2679,6 +2679,7 @@ static struct volume_group *_vg_read(struct cmd_context *cmd,
>  	 * the missing PV logic below.
>  	 */
>  	if ((correct_vg = lvmcache_get_vg(vgid, precommitted)) &&
> +	    /* FIXME: Expression is always TRUE - as we never set INCONSISTENT_VG! */
>  	    (use_precommitted || !*consistent || !(correct_vg->status & INCONSISTENT_VG))) {
>  		if (!(correct_vg->status & INCONSISTENT_VG))
>  			*consistent = 1;
> @@ -2935,8 +2936,8 @@ static struct volume_group *_vg_read(struct cmd_context *cmd,
>  	 * If there is no precommitted metadata, committed metadata
>  	 * is read and stored in the cache even if use_precommitted is set
>  	 */
> -	lvmcache_update_vg(correct_vg, correct_vg->status & PRECOMMITTED &
> -			   (inconsistent ? INCONSISTENT_VG : 0));
> +	/* FIXME: How to handle INCONSISTENT_VG? */
> +	lvmcache_update_vg(correct_vg, correct_vg->status & PRECOMMITTED);
>  
>  	if (inconsistent) {
>  		/* FIXME Test should be if we're *using* precommitted metadata not if we were searching for it */


Any opinion about this patch ?
(missed in review)


Zdenek

[PATCH 14/29] Remove unneeded check for NULL pvd->system_id

[Zdenek Kabelac]

Remove check is for system_id defined as int8_t[].

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

diff --git a/lib/format1/import-export.c b/lib/format1/import-export.c
index e261737..796aa29 100644
--- a/lib/format1/import-export.c
+++ b/lib/format1/import-export.c
@@ -185,7 +185,7 @@ int export_pv(struct cmd_context *cmd, struct dm_pool *mem __attribute__((unused
 	}
 
 	/* Generate system_id if PV is in VG */
-	if (!pvd->system_id || !*pvd->system_id)
+	if (!*pvd->system_id)
 		if (!_system_id(cmd, (char *)pvd->system_id, ""))
 			return_0;
 
-- 
1.7.3.2

[PATCH 14/29] Remove unneeded check for NULL pvd->system_id

[Alasdair G Kergon]

On Thu, Nov 25, 2010 at 11:55:18AM +0100, Zdenek Kabelac wrote:
> Remove check is for system_id defined as int8_t[].
 
Ack.

Alasdair

[PATCH 15/29] Modify test to catch passing NULL pointer

[Zdenek Kabelac]

Reason for this change is to avoid passing NULL 'data' to
_add_pv_to_list().

'data' is initialized as NULL and if 'dev' would be NULL from info->dev
'data' assignement would skipped and NULL would be passed through.
Also it would not make big sence to pass 'data' pointer twice in case
'dev' would be NULL.

Another fix could be that 'dev' could never be NULL in this place - so
the test could be to completely removed?

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

diff --git a/lib/format1/disk-rep.c b/lib/format1/disk-rep.c
index bc58744..071b39d 100644
--- a/lib/format1/disk-rep.c
+++ b/lib/format1/disk-rep.c
@@ -471,7 +471,7 @@ int read_pvs_in_vg(const struct format_type *fmt, const char *vg_name,
 	    vginfo->infos.n) {
 		dm_list_iterate_items(info, &vginfo->infos) {
 			dev = info->dev;
-			if (dev && !(data = read_disk(fmt, dev, mem, vg_name)))
+			if (!dev || !(data = read_disk(fmt, dev, mem, vg_name)))
 				break;
 			_add_pv_to_list(head, data);
 		}
-- 
1.7.3.2

[PATCH 15/29] Modify test to catch passing NULL pointer

[Alasdair G Kergon]

On Thu, Nov 25, 2010 at 11:55:19AM +0100, Zdenek Kabelac wrote:
> 'data' is initialized as NULL and if 'dev' would be NULL from info->dev

If we hit that condition we must do a scan.

> Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>
> ---
>  lib/format1/disk-rep.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/lib/format1/disk-rep.c b/lib/format1/disk-rep.c
> index bc58744..071b39d 100644
> --- a/lib/format1/disk-rep.c
> +++ b/lib/format1/disk-rep.c
> @@ -471,7 +471,7 @@ int read_pvs_in_vg(const struct format_type *fmt, const char *vg_name,
>  	    vginfo->infos.n) {
>  		dm_list_iterate_items(info, &vginfo->infos) {
>  			dev = info->dev;
> -			if (dev && !(data = read_disk(fmt, dev, mem, vg_name)))
> +			if (!dev || !(data = read_disk(fmt, dev, mem, vg_name)))
>  				break;
>  			_add_pv_to_list(head, data);

This makes it fall through to the scan.
Ack.

Alasdair

[PATCH 16/29] Test uuid for NULL

[Zdenek Kabelac]

This one again makes static analysis easier.

Though in this case when uuid is NULL len is also 0 so no bytes are
copied from the src address so no dereference happens - the function
memcpy itself is declared as the function which doesn't take
NULL as either src or dst parameter - let's be complaint.

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

diff --git a/tools/dmsetup.c b/tools/dmsetup.c
index 241fa74..1a56d24 100644
--- a/tools/dmsetup.c
+++ b/tools/dmsetup.c
@@ -343,7 +343,9 @@ static char *_extract_uuid_prefix(const char *uuid, const int separator)
 		return NULL;
 	}
 
-	memcpy(uuid_prefix, uuid, len);
+	if (uuid)
+		memcpy(uuid_prefix, uuid, len);
+
 	uuid_prefix[len] = '\0';
 
 	return uuid_prefix;
-- 
1.7.3.2

[PATCH 16/29] Test uuid for NULL

[Alasdair G Kergon]

On Thu, Nov 25, 2010 at 11:55:20AM +0100, Zdenek Kabelac wrote:
> memcpy itself is declared as the function which doesn't take
> NULL as either src or dst parameter - let's be complaint.
 
Ack.

Alasdair

[PATCH 17/29] Optimize second call to strchr with same parameters

[Zdenek Kabelac]

Add tempory char* to keep result of strchr call.
Make also static analysis happier as lv_name is now tested for nonnull.

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

diff --git a/tools/toollib.c b/tools/toollib.c
index 9af1630..f6ba45b 100644
--- a/tools/toollib.c
+++ b/tools/toollib.c
@@ -214,6 +214,7 @@ int process_each_lv(struct cmd_context *cmd, int argc, char **argv,
 
 		for (; opt < argc; opt++) {
 			const char *lv_name = argv[opt];
+			const char *tmp_lv_name;
 			char *vgname_def;
 			unsigned dev_dir_found = 0;
 
@@ -246,9 +247,9 @@ int process_each_lv(struct cmd_context *cmd, int argc, char **argv,
 				continue;
 			}
 			lv_name = vgname;
-			if (strchr(vgname, '/')) {
+			if ((tmp_lv_name = strchr(vgname, '/'))) {
 				/* Must be an LV */
-				lv_name = strchr(vgname, '/');
+				lv_name = tmp_lv_name;
 				while (*lv_name == '/')
 					lv_name++;
 				if (!(vgname = extract_vgname(cmd, vgname))) {
-- 
1.7.3.2

[PATCH 17/29] Optimize second call to strchr with same parameters

[Alasdair G Kergon]

On Thu, Nov 25, 2010 at 11:55:21AM +0100, Zdenek Kabelac wrote:
> Add tempory char* to keep result of strchr call.

> Make also static analysis happier as lv_name is now tested for nonnull.

As you know, I don't accept 'make static analysis happier' as a valid reason
for making changes:)  File bugs against the static analyser instead if it's
giving false positives.
 
So I'll ack that for the optimisation reason only.

Alasdair

[PATCH 18/29] Check result of vginfo_from_vgname

[Zdenek Kabelac]

Keep static code analyzers happier. In this case there is a very small
chance bug could appear (memory fault?). But as we check result from
vginfo_from_vgname in other cases - do it here as well.

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

diff --git a/lib/cache/lvmcache.c b/lib/cache/lvmcache.c
index 60edcec..6fb0900 100644
--- a/lib/cache/lvmcache.c
+++ b/lib/cache/lvmcache.c
@@ -1083,6 +1083,12 @@ static int _lvmcache_update_vgname(struct lvmcache_info *info,
 		       _scanning_in_progress && _vginfo_is_invalid(primary_vginfo))
 			dm_list_iterate_items_safe(info2, info3, &primary_vginfo->infos) {
 				orphan_vginfo = vginfo_from_vgname(primary_vginfo->fmt->orphan_vg_name, NULL);
+				if (!orphan_vginfo) {
+					log_error(INTERNAL_ERROR "vginfo for orphan_vg_name failed.");
+					dm_free(vginfo->vgname);
+					dm_free(vginfo);
+					return 0;
+				}
 				_drop_vginfo(info2, primary_vginfo);	
 				_vginfo_attach_info(orphan_vginfo, info2);
 				if (info2->mdas.n)
-- 
1.7.3.2

[PATCH 18/29] Check result of vginfo_from_vgname

[Alasdair G Kergon]

On Thu, Nov 25, 2010 at 11:55:22AM +0100, Zdenek Kabelac wrote:
> Keep static code analyzers happier. In this case there is a very small
> chance bug could appear (memory fault?). 

Our code doesn't attempt to protect against memory faults!

We are entitled to assume the orphan VGs are always there, so if they
aren't it's an internal error, yes.

>  			dm_list_iterate_items_safe(info2, info3, &primary_vginfo->infos) {
>  				orphan_vginfo = vginfo_from_vgname(primary_vginfo->fmt->orphan_vg_name, NULL);
> +				if (!orphan_vginfo) {
> +					log_error(INTERNAL_ERROR "vginfo for orphan_vg_name failed.");

Which orphan VG?  If this failed, I'd want to know what it was trying to look up!
INTERNAL_ERROR "Orphan vginfo %s lost from cache."

Ack with a more-useful error message.

Alasdair

[PATCH 19/29] Test for error status

[Zdenek Kabelac]

Check for errors from write and close.

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

diff --git a/daemons/clvmd/clvmd.c b/daemons/clvmd/clvmd.c
index 1a9d40e..ec98c3d 100644
--- a/daemons/clvmd/clvmd.c
+++ b/daemons/clvmd/clvmd.c
@@ -181,8 +181,10 @@ static void usage(char *prog, FILE *file)
 static void child_init_signal(int status)
 {
         if (child_pipe[1]) {
-	        write(child_pipe[1], &status, sizeof(status));
-		close(child_pipe[1]);
+		if (write(child_pipe[1], &status, sizeof(status)) < 0)
+			log_sys_error("write", "child_pipe");
+		if (close(child_pipe[1]))
+			log_sys_error("close", "child_pipe");
 	}
 }
 
-- 
1.7.3.2

[PATCH 19/29] Test for error status

[Alasdair G Kergon]

On Thu, Nov 25, 2010 at 11:55:23AM +0100, Zdenek Kabelac wrote:
> +		if (write(child_pipe[1], &status, sizeof(status)) < 0)
> +			log_sys_error("write", "child_pipe");

Still incomplete - add FIXME to use a proper wrapper around write().

> +		if (close(child_pipe[1]))
> +			log_sys_error("close", "child_pipe");

Ack.

Alasdair

[PATCH 20/29] Add test for lv_name not NULL

[Zdenek Kabelac]

This test is in fact not really needed - as the calculation of the
strlen(vgname) will be different from (lv_name - vg_name) when lv_name is
NULL - but IMHO it's logical to not make this evaluation -
and also static analysis is more straighforward when lv_name pointer
is guaranteed to be non NULL.

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

diff --git a/tools/toollib.c b/tools/toollib.c
index f6ba45b..9be9936 100644
--- a/tools/toollib.c
+++ b/tools/toollib.c
@@ -331,7 +331,7 @@ int process_each_lv(struct cmd_context *cmd, int argc, char **argv,
 				tags_arg = NULL;
 				dm_list_init(&lvnames);
 				break;
-			} else if (!strncmp(vg_name, vgname, strlen(vgname)) &&
+			} else if (!strncmp(vg_name, vgname, strlen(vgname)) && lv_name &&
 				   strlen(vgname) == (size_t) (lv_name - vg_name)) {
 				if (!str_list_add(cmd->mem, &lvnames,
 						  dm_pool_strdup(cmd->mem,
-- 
1.7.3.2

[PATCH 20/29] Add test for lv_name not NULL

[Alasdair G Kergon]

On Thu, Nov 25, 2010 at 11:55:24AM +0100, Zdenek Kabelac wrote:
> -			} else if (!strncmp(vg_name, vgname, strlen(vgname)) &&
> +			} else if (!strncmp(vg_name, vgname, strlen(vgname)) && lv_name &&
>  				   strlen(vgname) == (size_t) (lv_name - vg_name)) {

Ack

Alasdair

[PATCH 21/29] Instrument with nonnull dev_manager_transient

[Zdenek Kabelac]

As the function directly dereferencies its parameters mark them as non-null.

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

diff --git a/lib/activate/dev_manager.h b/lib/activate/dev_manager.h
index be6de51..72ca7f9 100644
--- a/lib/activate/dev_manager.h
+++ b/lib/activate/dev_manager.h
@@ -58,7 +58,7 @@ int dev_manager_activate(struct dev_manager *dm, struct logical_volume *lv, unsi
 int dev_manager_preload(struct dev_manager *dm, struct logical_volume *lv,
 			unsigned origin_only, int *flush_required);
 int dev_manager_deactivate(struct dev_manager *dm, struct logical_volume *lv);
-int dev_manager_transient(struct dev_manager *dm, struct logical_volume *lv);
+int dev_manager_transient(struct dev_manager *dm, struct logical_volume *lv) __attribute__((nonnull(1, 2)));
 
 int dev_manager_mknodes(const struct logical_volume *lv);
 
-- 
1.7.3.2

[PATCH 21/29] Instrument with nonnull dev_manager_transient

[Alasdair G Kergon]

On Thu, Nov 25, 2010 at 11:55:25AM +0100, Zdenek Kabelac wrote:
> +int dev_manager_transient(struct dev_manager *dm, struct logical_volume *lv) __attribute__((nonnull(1, 2)));

Ack.

Alasdair

[PATCH 22/29] Ensure pointer first is notnull before dereference

[Zdenek Kabelac]

Modify code to put check into local function.

Old code expected  _check_vgs had been give  non empty pvs list which
should ensure some 'first' pointer is defined.

New code remove check for empty pvs list before call of _check_vgs
and leave this case to be handle inside this function - this
makes static analyzer less confused.

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

diff --git a/lib/format1/format1.c b/lib/format1/format1.c
index 0dcec05..0745890 100644
--- a/lib/format1/format1.c
+++ b/lib/format1/format1.c
@@ -102,7 +102,10 @@ static int _check_vgs(struct dm_list *pvs, struct volume_group *vg)
 		pv_count++;
 	}
 
-	/* On entry to fn, list known to be non-empty */
+	/* Empty pvs list is not valid */
+	if (!first)
+		return 0;
+
 	if (pv_count != first->vgd.pv_cur) {
 		log_error("%d PV(s) found for VG %s: expected %d",
 			  pv_count, first->pvd.vg_name, first->vgd.pv_cur);
@@ -187,9 +190,6 @@ static struct volume_group *_build_vg(struct format_instance *fid,
 	if (!vg)
 		goto_bad;
 
-	if (dm_list_empty(pvs))
-		goto_bad;
-
 	vg->cmd = fid->fmt->cmd;
 	vg->vgmem = mem;
 	vg->fid = fid;
-- 
1.7.3.2

[PATCH 22/29] Ensure pointer first is notnull before dereference

[Alasdair G Kergon]

On Thu, Nov 25, 2010 at 11:55:26AM +0100, Zdenek Kabelac wrote:
> Modify code to put check into local function.
 
I find the proposed version more obscure and prefer it the way it is now, where
we do an explicit check up-front instead of burying it inside another function,
seemingly as a side-effect.

Alasdair

[PATCH 23/29] Add test and error message for failure case

[Zdenek Kabelac]

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

diff --git a/daemons/clvmd/clvmd.c b/daemons/clvmd/clvmd.c
index ec98c3d..06be3c2 100644
--- a/daemons/clvmd/clvmd.c
+++ b/daemons/clvmd/clvmd.c
@@ -288,7 +288,8 @@ static const char *decode_cmd(unsigned char cmdl)
 
 static void remove_lockfile(void)
 {
-	unlink(CLVMD_PIDFILE);
+	if (unlink(CLVMD_PIDFILE))
+		log_sys_error("unlink", CLVMD_PIDFILE);
 }
 
 /*
-- 
1.7.3.2

[PATCH 23/29] Add test and error message for failure case

[Alasdair G Kergon]

On Thu, Nov 25, 2010 at 11:55:27AM +0100, Zdenek Kabelac wrote:
> +	if (unlink(CLVMD_PIDFILE))
> +		log_sys_error("unlink", CLVMD_PIDFILE);

Ack.

Alasdair

[PATCH 24/29] Test for str_list_add

[Zdenek Kabelac]

Check for success and report error on failure case.

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

diff --git a/lib/format_pool/import_export.c b/lib/format_pool/import_export.c
index 07f67fd..7884247 100644
--- a/lib/format_pool/import_export.c
+++ b/lib/format_pool/import_export.c
@@ -206,7 +206,10 @@ static int _add_stripe_seg(struct dm_pool *mem,
 			return_0;
 
 	/* add the subpool type to the segment tag list */
-	str_list_add(mem, &seg->tags, _cvt_sptype(usp->type));
+	if (!str_list_add(mem, &seg->tags, _cvt_sptype(usp->type))) {
+		log_error("Allocation failed for str_list.");
+		return 0;
+	}
 
 	dm_list_add(&lv->segments, &seg->list);
 
@@ -240,7 +243,10 @@ static int _add_linear_seg(struct dm_pool *mem,
 		}
 
 		/* add the subpool type to the segment tag list */
-		str_list_add(mem, &seg->tags, _cvt_sptype(usp->type));
+		if (!str_list_add(mem, &seg->tags, _cvt_sptype(usp->type))) {
+			log_error("Allocation failed for str_list.");
+			return 0;
+		}
 
 		if (!set_lv_segment_area_pv(seg, 0, usp->devs[j].pv, 0))
 			return_0;
-- 
1.7.3.2

[PATCH 24/29] Test for str_list_add

[Alasdair G Kergon]

On Thu, Nov 25, 2010 at 11:55:28AM +0100, Zdenek Kabelac wrote:
> Check for success and report error on failure case.
 
Ack.

Alasdair

[PATCH 25/29] Check for unlink result

[Zdenek Kabelac]

Even if we intentionally wish to silently ignore any error from
unlink in this place - it might be still good idea to put at least
a debug message to get the reason for failure.

Test makes static analyzer happier about checked unlink result.

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

diff --git a/lib/format_text/archiver.c b/lib/format_text/archiver.c
index 3ace628..e0efafb 100644
--- a/lib/format_text/archiver.c
+++ b/lib/format_text/archiver.c
@@ -261,7 +261,9 @@ int backup_remove(struct cmd_context *cmd, const char *vg_name)
 	/*
 	 * Let this fail silently.
 	 */
-	unlink(path);
+	if (unlink(path))
+		log_debug("Unlink %s failed: %s", path, strerror(errno));
+
 	return 1;
 }
 
-- 
1.7.3.2

[PATCH 25/29] Check for unlink result

[Alasdair G Kergon]

On Thu, Nov 25, 2010 at 11:55:29AM +0100, Zdenek Kabelac wrote:
> Even if we intentionally wish to silently ignore any error from
> unlink in this place - it might be still good idea to put at least
> a debug message to get the reason for failure.

Don't see why not.
Ack.
 
> Test makes static analyzer happier about checked unlink result.

It's a project standard that all return values should be checked
(or explicitly discarded with a cast to void).
 
Alasdair

[PATCH 25/29] Check for unlink result

[Alasdair G Kergon]

On Thu, Nov 25, 2010 at 11:55:29AM +0100, Zdenek Kabelac wrote:
> +	if (unlink(path))
> +		log_debug("Unlink %s failed: %s", path, strerror(errno));

log_sys_debug ?

Alasdair

[PATCH 25/29] Check for unlink result

[Zdenek Kabelac]

Dne 29.11.2010 22:51, Alasdair G Kergon napsal(a):
> On Thu, Nov 25, 2010 at 11:55:29AM +0100, Zdenek Kabelac wrote:
>> +	if (unlink(path))
>> +		log_debug("Unlink %s failed: %s", path, strerror(errno));
> 
> log_sys_debug ?
> 


At this place it's valid the file in the moment of unlink doesn't exist.
There is no explicit test for existence of backup file - so I've used only
debug to print reason for failure at least in debug log.
(Assuming 'Let this fail silently.' has this meaning)

Zdenek

[PATCH 26/29] Add stack traces for dev_set/close_immediate error path

[Zdenek Kabelac]

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

diff --git a/lib/metadata/lv_manip.c b/lib/metadata/lv_manip.c
index 31f1ff3..6f553cd 100644
--- a/lib/metadata/lv_manip.c
+++ b/lib/metadata/lv_manip.c
@@ -3033,9 +3033,13 @@ int set_lv(struct cmd_context *cmd, struct logical_volume *lv,
 	if (sectors > lv->size)
 		sectors = lv->size;
 
-	dev_set(dev, UINT64_C(0), (size_t) sectors << SECTOR_SHIFT, value);
+	if (!dev_set(dev, UINT64_C(0), (size_t) sectors << SECTOR_SHIFT, value))
+		stack;
+
 	dev_flush(dev);
-	dev_close_immediate(dev);
+
+	if (!dev_close_immediate(dev))
+                stack;
 
 	return 1;
 }
-- 
1.7.3.2

[PATCH 26/29] Add stack traces for dev_set/close_immediate error path

[Alasdair G Kergon]

On Thu, Nov 25, 2010 at 11:55:30AM +0100, Zdenek Kabelac wrote:
> +	if (!dev_set(dev, UINT64_C(0), (size_t) sectors << SECTOR_SHIFT, value))
> +		stack;
> +
>  	dev_flush(dev);
> -	dev_close_immediate(dev);
> +
> +	if (!dev_close_immediate(dev))
> +                stack;

Ack.

Alasdair

[PATCH 27/29] Add standard check for result of lv_info call

[Zdenek Kabelac]

Also add log_error message in case info would not be found.

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

diff --git a/lib/mirror/mirrored.c b/lib/mirror/mirrored.c
index da9bdba..98aaa0d 100644
--- a/lib/mirror/mirrored.c
+++ b/lib/mirror/mirrored.c
@@ -296,7 +296,11 @@ static int _mirrored_transient_status(struct lv_segment *seg, char *params)
 	if (!strcmp(log_args[0], "disk")) {
 		char buf[32];
 		log = first_seg(lv)->log_lv;
-		lv_info(lv->vg->cmd, log, 0, &info, 0, 0);
+		if (!lv_info(lv->vg->cmd, log, 0, &info, 0, 0)) {
+			log_error("Check for existence of mirror log '%s' failed.",
+				  log->name);
+			return 0;
+		}
 		log_debug("Found mirror log at %d:%d", info.major, info.minor);
 		sprintf(buf, "%d:%d", info.major, info.minor);
 		if (strcmp(buf, log_args[1])) {
@@ -316,7 +320,11 @@ static int _mirrored_transient_status(struct lv_segment *seg, char *params)
 
 	for (i = 0; i < seg->area_count; ++i) {
 		char buf[32];
-		lv_info(lv->vg->cmd, seg_lv(seg, i), 0, &info, 0, 0);
+		if (!lv_info(lv->vg->cmd, seg_lv(seg, i), 0, &info, 0, 0)) {
+			log_error("Check for existence of mirror leg '%s' failed.",
+				  log->name);
+			return 0;
+		}
 		log_debug("Found mirror leg at %d:%d", info.major, info.minor);
 		sprintf(buf, "%d:%d", info.major, info.minor);
 		for (j = 0; j < num_devs; ++j) {
-- 
1.7.3.2

[PATCH 27/29] Add standard check for result of lv_info call

[Zdenek Kabelac]

Dne 25.11.2010 11:55, Zdenek Kabelac napsal(a):
> Also add log_error message in case info would not be found.
> 
> Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>
> ---
>  lib/mirror/mirrored.c |   12 ++++++++++--
>  1 files changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/mirror/mirrored.c b/lib/mirror/mirrored.c
> index da9bdba..98aaa0d 100644
> --- a/lib/mirror/mirrored.c
> +++ b/lib/mirror/mirrored.c
> @@ -296,7 +296,11 @@ static int _mirrored_transient_status(struct lv_segment *seg, char *params)
>  	if (!strcmp(log_args[0], "disk")) {
>  		char buf[32];
>  		log = first_seg(lv)->log_lv;
> -		lv_info(lv->vg->cmd, log, 0, &info, 0, 0);
> +		if (!lv_info(lv->vg->cmd, log, 0, &info, 0, 0)) {
> +			log_error("Check for existence of mirror log '%s' failed.",
> +				  log->name);
> +			return 0;
> +		}
>  		log_debug("Found mirror log at %d:%d", info.major, info.minor);
>  		sprintf(buf, "%d:%d", info.major, info.minor);
>  		if (strcmp(buf, log_args[1])) {
> @@ -316,7 +320,11 @@ static int _mirrored_transient_status(struct lv_segment *seg, char *params)
>  
>  	for (i = 0; i < seg->area_count; ++i) {
>  		char buf[32];
> -		lv_info(lv->vg->cmd, seg_lv(seg, i), 0, &info, 0, 0);
> +		if (!lv_info(lv->vg->cmd, seg_lv(seg, i), 0, &info, 0, 0)) {
> +			log_error("Check for existence of mirror leg '%s' failed.",
> +				  log->name);

Ooops - forget one merge commit in my local branch to fix this copy&paste.

Here instead of 'log->name'     'seg_lv(seg, i)->name'  has to be used.

Zdenek

[PATCH 27/29] Add standard check for result of lv_info call

[Alasdair G Kergon]

On Thu, Nov 25, 2010 at 11:55:31AM +0100, Zdenek Kabelac wrote:
> Also add log_error message in case info would not be found.
 
Ack.

> +			log_error("Check for existence of mirror log '%s' failed.",

I generally don't use any quotes around names.
Heinz uses \".

> +			log_error("Check for existence of mirror leg '%s' failed.",

Rightly or wrongly, we decided never to use 'leg' in user-visible error messages
or documentation (in favour of 'image').

Alasdair

[PATCH 28/29] Check type is not NULL before access

[Zdenek Kabelac]

clang Logic error Dereference of null pointer

Check type before dereference and report internal error in case
it's undefined and return from the function.

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

diff --git a/libdm/libdm-report.c b/libdm/libdm-report.c
index 9b8e3c1..6d8c8bb 100644
--- a/libdm/libdm-report.c
+++ b/libdm/libdm-report.c
@@ -273,6 +273,12 @@ static void _display_fields(struct dm_report *rh)
 			log_warn("%*.*s", (int) strlen(desc) + 7,
 				 (int) strlen(desc) + 7,
 				 "-------------------------------------------------------------------------------");
+
+			if (!type) {
+				log_error(INTERNAL_ERROR "Type is not defined.");
+				return;
+			}
+
 			log_warn("  %sall%-*s - %s", type->prefix,
 				 (int) (id_len - 3 - strlen(type->prefix)), "",
 				 "All fields in this section.");
-- 
1.7.3.2

[PATCH 28/29] Check type is not NULL before access

[Alasdair G Kergon]

On Thu, Nov 25, 2010 at 11:55:32AM +0100, Zdenek Kabelac wrote:
> Check type before dereference and report internal error in case
> it's undefined and return from the function.
 
But that does *not* look to be an internal dm library error at the moment.

It's just a parameter to dm_report_init().

Alasdair

[PATCH 29/29] Check for NULL pointer

[Zdenek Kabelac]

clangs is happier and check for non NULL options value.
Report internal error in this case.

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

diff --git a/tools/reporter.c b/tools/reporter.c
index 40d3c9a..16c19e2 100644
--- a/tools/reporter.c
+++ b/tools/reporter.c
@@ -328,6 +328,10 @@ static int _report(struct cmd_context *cmd, int argc, char **argv,
 			return EINVALID_CMD_LINE;
 		}
 		if (*opts == '+') {
+			if (!options) {
+				log_error(INTERNAL_ERROR "Missing options.");
+				return EINVALID_CMD_LINE;
+			}
 			if (!(str = dm_pool_alloc(cmd->mem,
 					 strlen(options) + strlen(opts) + 1))) {
 				log_error("options string allocation failed");
-- 
1.7.3.2

[PATCH 29/29] Check for NULL pointer

[Zdenek Kabelac]

Dne 25.11.2010 11:55, Zdenek Kabelac napsal(a):
> clangs is happier and check for non NULL options value.
> Report internal error in this case.
> 
> Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>
> ---
>  tools/reporter.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/tools/reporter.c b/tools/reporter.c
> index 40d3c9a..16c19e2 100644
> --- a/tools/reporter.c
> +++ b/tools/reporter.c
> @@ -328,6 +328,10 @@ static int _report(struct cmd_context *cmd, int argc, char **argv,
>  			return EINVALID_CMD_LINE;
>  		}
>  		if (*opts == '+') {
> +			if (!options) {
> +				log_error(INTERNAL_ERROR "Missing options.");
> +				return EINVALID_CMD_LINE;
> +			}
>  			if (!(str = dm_pool_alloc(cmd->mem,
>  					 strlen(options) + strlen(opts) + 1))) {
>  				log_error("options string allocation failed");

Ok - here is 'smarter' way how to avoid NULL 'options' path:

It's more or less assert - but since we are not using asserts.

As with current code 'default:' could never be reached - so it doesn't put any
extra check into the original code path.

diff --git a/tools/reporter.c b/tools/reporter.c
index 40d3c9a..358c404 100644
--- a/tools/reporter.c
+++ b/tools/reporter.c
@@ -318,6 +318,9 @@ static int _report(struct cmd_context *cmd, int argc, char
**argv,
 						  "report/pvsegs_cols_verbose",
 						  DEFAULT_PVSEGS_COLS_VERB);
 		break;
+	default:
+		log_error(INTERNAL_ERROR "Unknown report type.");
+		return ECMD_FAILED;
 	}

 	/* If -o supplied use it, else use default for report_type */

[PATCH 29/29] Check for NULL pointer

[Alasdair G Kergon]

On Fri, Nov 26, 2010 at 12:02:33AM +0100, Zdenek Kabelac wrote:
> +	default:
> +		log_error(INTERNAL_ERROR "Unknown report type.");
> +		return ECMD_FAILED;

It's an enum where we cover every value.  An assertion is frankly
pointless.

I could equally perform my own 'static analysis' and deduce that the
default case there is redundant and delete it:)

Alasdair

[PATCH 29/29] Check for NULL pointer

[Zdenek Kabelac]

Dne 29.11.2010 23:47, Alasdair G Kergon napsal(a):
> On Fri, Nov 26, 2010 at 12:02:33AM +0100, Zdenek Kabelac wrote:
>> +	default:
>> +		log_error(INTERNAL_ERROR "Unknown report type.");
>> +		return ECMD_FAILED;
> 
> It's an enum where we cover every value.  An assertion is frankly
> pointless.
> 
> I could equally perform my own 'static analysis' and deduce that the
> default case there is redundant and delete it:)
> 


Of course for our code all cases are covered - and as I've mentioned  -
sometimes the analyzer is not yet skilled enough to recognize that all
use-cases of the function are already covered and there cannot be any misusage
as the function is invoked only locally (static) - but usually all 'asserts()'
are about something which should never happen :)

Anyway I'll open another Clang bug for this case.

(as you may see I'm usually being suggested to cover the problematic case by
adding asserts (i.e. see http://llvm.org/bugs/show_bug.cgi?id=8697))

It's about whether we want to get all these 'easy-to-hide' false positives
again with next round check - so anyone who will look into the result of
analyzer will need to go through the list of 'old known' problems - or just
directly see a new problem - i.e. having multiple pages of issues where some
of them are known false positives - currently with my local tree I get just
23 reports - which is pretty small list)

It's not a problem to keep these little quirks only in my local tree (except
we wont get cleaner Coverity reports as that one is made with CVS tree).

Zdenek

[PATCH 29/29] Check for NULL pointer

[Alasdair G Kergon]

On Tue, Nov 30, 2010 at 01:33:55PM +0100, Zdenek Kabelac wrote:
> It's not a problem to keep these little quirks only in my local tree (except
> we wont get cleaner Coverity reports as that one is made with CVS tree).
 
Coverity's web interface handles it nicely - you just mark it as a false
report and it ignores it next time.  Coverity scans broke down in February.

Amazingly, just today, a new scan has appeared with an updated web interface.
They normally ignore emails I send them, but let's see if they respond to a
request to give you access.

Alasdair

[PATCH 29/29] Check for NULL pointer

[Alasdair G Kergon]

On Thu, Nov 25, 2010 at 11:55:33AM +0100, Zdenek Kabelac wrote:
> clangs is happier and check for non NULL options value.
> Report internal error in this case.
 
That one looks completely pointless to me.
(And if you did have it, the right place for it would be after the switch that
always sets it.)

Nack.

Alasdair