[PATCH 00/30] Fixes from clang

[PATCH 00/30] Fixes from clang

[Zdenek Kabelac]

Patchset combines code cleanups for compiler warnings
and code fixes found by static analyzer scan-build.

Potentional NULL pointer dereferencies are mostly
in very unusual code execution paths. I've tried to
order them on their importance - the later ones are not
event trigerable as surrounding code should not be to
pass problematic values - but checks seems to be simple
enough to keep static analysis happy.

Last patches in this set add some instrumentation to
function calls that might allow better code optimization
or better static analysis.

Zdenek Kabelac (30):
  Fix clang warning for ntohl(*((uint32_t *)buf))
  Use dm_pool_grow_object embeded strlen for 0 delta
  Check (type) is not NULL before access
  Fix NULL pointer dereference
  Print vg_name and do not to access vg->name
  Ensure we always have origin defined
  Ensure seg is nonnull
  Ensure  first is not NULL
  Reuse result of previous strchr.
  Fix potential NULL pointer dereference
  Fix theoretical usage of NULL pointer dereference
  Check for NULL pointer
  Eliminate uninitialized_var
  Fix missing initilisation to 0
  Function pull_state cannot work with NULL buffer
  Fix  void* pointer arithmetic
  Use const pvid for device_from_pvid()
  Use const pointer for return value of dm_basename
  Fix constness warning
  Fix constness warning
  Fix constness warning
  Fix constness warning
  Using const config node
  bufused is assigned 0 in preceding source line
  Proper C declaration
  Use static
  Instrument compiler about code unreachability
  Skip uninitialized macro for scan-build
  Add some __attribute__ instrumentation
  __attribute__((nonnull(1)))

 daemons/clvmd/clvmd.c                 |    7 +++++--
 daemons/cmirrord/cluster.c            |    2 +-
 daemons/cmirrord/functions.c          |    4 +++-
 daemons/dmeventd/dmeventd.c           |   21 +++++++++++----------
 daemons/dmeventd/libdevmapper-event.c |   20 ++++++++++----------
 lib/activate/dev_manager.c            |    2 +-
 lib/cache/lvmcache.c                  |   10 +++++-----
 lib/cache/lvmcache.h                  |    2 +-
 lib/config/config.c                   |    2 +-
 lib/device/dev-cache.h                |    2 +-
 lib/device/dev-md.c                   |    3 ++-
 lib/format1/format1.c                 |    2 +-
 lib/format_text/archive.c             |    4 ++--
 lib/format_text/format-text.c         |    2 +-
 lib/log/log.c                         |    2 +-
 lib/metadata/lv_manip.c               |    2 +-
 lib/metadata/metadata.c               |    6 +++---
 lib/misc/crc.c                        |    6 +++---
 lib/misc/util.h                       |    4 ++++
 lib/mm/memlock.c                      |    4 ++--
 lib/report/report.c                   |    6 +++---
 lib/striped/striped.c                 |    2 +-
 libdm/libdevmapper.h                  |   28 ++++++++++++++--------------
 libdm/libdm-report.c                  |    7 ++++---
 libdm/libdm-string.c                  |    6 +++---
 libdm/mm/pool-fast.c                  |    8 ++++----
 libdm/regex/ttree.c                   |    7 +++++--
 tools/dmsetup.c                       |    2 +-
 tools/lvcreate.c                      |    4 ++++
 tools/lvm.c                           |    6 +++---
 tools/lvmcmdline.c                    |    2 +-
 tools/pvresize.c                      |    2 +-
 tools/reporter.c                      |    2 +-
 tools/toollib.c                       |    2 +-
 34 files changed, 105 insertions(+), 86 deletions(-)

--
1.7.3.1

[PATCH 01/30] Fix clang warning for ntohl(*((uint32_t *)buf))

[Zdenek Kabelac]

We cast (char*) to (uint32_t*) that changes alignment requierements.
For our case the code has been correct as alloca() returns properly
aligned buffer, however this patch make it cleaner and more readable
and avoids warning generation.

FIXME: code duplication

Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>
---
 daemons/dmeventd/dmeventd.c           |   17 +++++++++--------
 daemons/dmeventd/libdevmapper-event.c |   20 ++++++++++----------
 2 files changed, 19 insertions(+), 18 deletions(-)

diff --git a/daemons/dmeventd/dmeventd.c b/daemons/dmeventd/dmeventd.c
index 639f200..d179879 100644
--- a/daemons/dmeventd/dmeventd.c
+++ b/daemons/dmeventd/dmeventd.c
@@ -1300,9 +1300,9 @@ static int _client_read(struct dm_event_fifos *fifos,
 	unsigned bytes = 0;
 	int ret = 0;
 	fd_set fds;
-	int header = 1;
 	size_t size = 2 * sizeof(uint32_t);	/* status + size */
-	char *buf = alloca(size);
+	uint32_t *header = alloca(size);
+	char *buf = (char *)header;
 
 	msg->data = NULL;
 
@@ -1326,9 +1326,9 @@ static int _client_read(struct dm_event_fifos *fifos,
 
 		ret = read(fifos->client, buf + bytes, size - bytes);
 		bytes += ret > 0 ? ret : 0;
-		if (bytes == 2 * sizeof(uint32_t) && header) {
-			msg->cmd = ntohl(*((uint32_t *) buf));
-			msg->size = ntohl(*((uint32_t *) buf + 1));
+		if (header && (bytes == 2 * sizeof(uint32_t))) {
+			msg->cmd = ntohl(header[0]);
+			msg->size = ntohl(header[1]);
 			buf = msg->data = dm_malloc(msg->size);
 			size = msg->size;
 			bytes = 0;
@@ -1356,10 +1356,11 @@ static int _client_write(struct dm_event_fifos *fifos,
 	fd_set fds;
 
 	size_t size = 2 * sizeof(uint32_t) + msg->size;
-	char *buf = alloca(size);
+	uint32_t *header = alloca(size);
+	char *buf = (char *)header;
 
-	*((uint32_t *)buf) = htonl(msg->cmd);
-	*((uint32_t *)buf + 1) = htonl(msg->size);
+	header[0] = htonl(msg->cmd);
+	header[1] = htonl(msg->size);
 	if (msg->data)
 		memcpy(buf + 2 * sizeof(uint32_t), msg->data, msg->size);
 
diff --git a/daemons/dmeventd/libdevmapper-event.c b/daemons/dmeventd/libdevmapper-event.c
index abbb968..bc8ad99 100644
--- a/daemons/dmeventd/libdevmapper-event.c
+++ b/daemons/dmeventd/libdevmapper-event.c
@@ -230,8 +230,8 @@ static int _daemon_read(struct dm_event_fifos *fifos,
 	fd_set fds;
 	struct timeval tval = { 0, 0 };
 	size_t size = 2 * sizeof(uint32_t);	/* status + size */
-	char *buf = alloca(size);
-	int header = 1;
+	uint32_t *header = alloca(size);
+	char *buf = (char *)header;
 
 	while (bytes < size) {
 		for (i = 0, ret = 0; (i < 20) && (ret < 1); i++) {
@@ -262,9 +262,9 @@ static int _daemon_read(struct dm_event_fifos *fifos,
 		}
 
 		bytes += ret;
-		if (bytes == 2 * sizeof(uint32_t) && header) {
-			msg->cmd = ntohl(*((uint32_t *)buf));
-			msg->size = ntohl(*((uint32_t *)buf + 1));
+		if (header && (bytes == 2 * sizeof(uint32_t))) {
+			msg->cmd = ntohl(header[0]);
+			msg->size = ntohl(header[1]);
 			buf = msg->data = dm_malloc(msg->size);
 			size = msg->size;
 			bytes = 0;
@@ -288,12 +288,13 @@ static int _daemon_write(struct dm_event_fifos *fifos,
 	fd_set fds;
 
 	size_t size = 2 * sizeof(uint32_t) + msg->size;
-	char *buf = alloca(size);
+	uint32_t *header = alloca(size);
+	char *buf = (char *)header;
 	char drainbuf[128];
 	struct timeval tval = { 0, 0 };
 
-	*((uint32_t *)buf) = htonl(msg->cmd);
-	*((uint32_t *)buf + 1) = htonl(msg->size);
+	header[0] = htonl(msg->cmd);
+	header[1] = htonl(msg->size);
 	memcpy(buf + 2 * sizeof(uint32_t), msg->data, msg->size);
 
 	/* drain the answer fifo */
@@ -323,8 +324,7 @@ static int _daemon_write(struct dm_event_fifos *fifos,
 			}
 		} while (ret < 1);
 
-		ret = write(fifos->client, ((char *) buf) + bytes,
-			    size - bytes);
+		ret = write(fifos->client, buf + bytes, size - bytes);
 		if (ret < 0) {
 			if ((errno == EINTR) || (errno == EAGAIN))
 				continue;
-- 
1.7.3.1

[PATCH 02/30] Use dm_pool_grow_object embeded strlen for 0 delta

[Zdenek Kabelac]

Use internal strlen for 0

Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>
---
 lib/config/config.c       |    2 +-
 lib/format_text/archive.c |    4 ++--
 lib/metadata/lv_manip.c   |    2 +-
 lib/metadata/metadata.c   |    4 ++--
 lib/report/report.c       |    6 +++---
 tools/lvmcmdline.c        |    2 +-
 6 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/lib/config/config.c b/lib/config/config.c
index 2b38663..e64ec86 100644
--- a/lib/config/config.c
+++ b/lib/config/config.c
@@ -387,7 +387,7 @@ static int _line_append(struct output_line *outline, const char *fmt, ...)
 	}
 	va_end(ap);
 
-	if (!dm_pool_grow_object(outline->mem, &buf[0], strlen(buf))) {
+	if (!dm_pool_grow_object(outline->mem, buf, 0)) {
 		log_error("dm_pool_grow_object failed for config line");
 		return 0;
 	}
diff --git a/lib/format_text/archive.c b/lib/format_text/archive.c
index bf16da5..246350b 100644
--- a/lib/format_text/archive.c
+++ b/lib/format_text/archive.c
@@ -110,9 +110,9 @@ static void _insert_archive_file(struct dm_list *head, struct archive_file *b)
 static char *_join_file_to_dir(struct dm_pool *mem, const char *dir, const char *name)
 {
 	if (!dm_pool_begin_object(mem, 32) ||
-	    !dm_pool_grow_object(mem, dir, strlen(dir)) ||
+	    !dm_pool_grow_object(mem, dir, 0) ||
 	    !dm_pool_grow_object(mem, "/", 1) ||
-	    !dm_pool_grow_object(mem, name, strlen(name)) ||
+	    !dm_pool_grow_object(mem, name, 0) ||
 	    !dm_pool_grow_object(mem, "\0", 1))
 		return_NULL;
 
diff --git a/lib/metadata/lv_manip.c b/lib/metadata/lv_manip.c
index e4349cc..0ea7dcf 100644
--- a/lib/metadata/lv_manip.c
+++ b/lib/metadata/lv_manip.c
@@ -665,7 +665,7 @@ static int _log_parallel_areas(struct dm_pool *mem, struct dm_list *parallel_are
 		}
 
 		dm_list_iterate_items(pvl, &spvs->pvs) {
-			if (!dm_pool_grow_object(mem, pv_dev_name(pvl->pv), strlen(pv_dev_name(pvl->pv)))) {
+			if (!dm_pool_grow_object(mem, pv_dev_name(pvl->pv), 0)) {
 				log_error("dm_pool_grow_object failed");
 				dm_pool_abandon_object(mem);
 				return 0;
diff --git a/lib/metadata/metadata.c b/lib/metadata/metadata.c
index 4d11ac4..2da4085 100644
--- a/lib/metadata/metadata.c
+++ b/lib/metadata/metadata.c
@@ -1078,7 +1078,7 @@ static dm_bitset_t _bitset_with_random_bits(struct dm_pool *mem, uint32_t num_bi
 			dm_pool_free(mem, bs);
                 	return NULL;
 		}
-		if (!dm_pool_grow_object(mem, buf, strlen(buf))) {
+		if (!dm_pool_grow_object(mem, buf, 0)) {
 			log_error("Failed to generate list of random bits.");
 			dm_pool_free(mem, bs);
                 	return NULL;
@@ -3970,7 +3970,7 @@ char *tags_format_and_copy(struct dm_pool *mem, const struct dm_list *tags)
 	}
 
 	dm_list_iterate_items(sl, tags) {
-		if (!dm_pool_grow_object(mem, sl->str, strlen(sl->str)) ||
+		if (!dm_pool_grow_object(mem, sl->str, 0) ||
 		    (sl->list.n != tags && !dm_pool_grow_object(mem, ",", 1))) {
 			log_error("dm_pool_grow_object failed");
 			return NULL;
diff --git a/lib/report/report.c b/lib/report/report.c
index 7a90b4d..b26a1e3 100644
--- a/lib/report/report.c
+++ b/lib/report/report.c
@@ -85,7 +85,7 @@ static int _format_pvsegs(struct dm_pool *mem, struct dm_report_field *field,
 			extent = 0;
 		}
 
-		if (!dm_pool_grow_object(mem, name, strlen(name))) {
+		if (!dm_pool_grow_object(mem, name, 0)) {
 			log_error("dm_pool_grow_object failed");
 			return 0;
 		}
@@ -97,7 +97,7 @@ static int _format_pvsegs(struct dm_pool *mem, struct dm_report_field *field,
 			log_error("Extent number dm_snprintf failed");
 			return 0;
 		}
-		if (!dm_pool_grow_object(mem, extent_str, strlen(extent_str))) {
+		if (!dm_pool_grow_object(mem, extent_str, 0)) {
 			log_error("dm_pool_grow_object failed");
 			return 0;
 		}
@@ -108,7 +108,7 @@ static int _format_pvsegs(struct dm_pool *mem, struct dm_report_field *field,
 				log_error("Extent number dm_snprintf failed");
 				return 0;
 			}
-			if (!dm_pool_grow_object(mem, extent_str, strlen(extent_str))) {
+			if (!dm_pool_grow_object(mem, extent_str, 0)) {
 				log_error("dm_pool_grow_object failed");
 				return 0;
 			}
diff --git a/tools/lvmcmdline.c b/tools/lvmcmdline.c
index b29acfb..06873a0 100644
--- a/tools/lvmcmdline.c
+++ b/tools/lvmcmdline.c
@@ -986,7 +986,7 @@ static const char *_copy_command_line(struct cmd_context *cmd, int argc, char **
 		if (space && !dm_pool_grow_object(cmd->mem, "'", 1))
 			goto_bad;
 
-		if (!dm_pool_grow_object(cmd->mem, argv[i], strlen(argv[i])))
+		if (!dm_pool_grow_object(cmd->mem, argv[i], 0))
 			goto_bad;
 
 		if (space && !dm_pool_grow_object(cmd->mem, "'", 1))
-- 
1.7.3.1

[PATCH 03/30] Check (type) is not NULL before access

[Zdenek Kabelac]

clang Logic error	Dereference of null pointer

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

diff --git a/libdm/libdm-report.c b/libdm/libdm-report.c
index 7631e21..28b27b2 100644
--- a/libdm/libdm-report.c
+++ b/libdm/libdm-report.c
@@ -273,9 +273,10 @@ static void _display_fields(struct dm_report *rh)
 			log_warn("%*.*s", (int) strlen(desc) + 7,
 				 (int) strlen(desc) + 7,
 				 "-------------------------------------------------------------------------------");
-			log_warn("  %sall%-*s - %s", type->prefix,
-				 (int) (id_len - 3 - strlen(type->prefix)), "",
-				 "All fields in this section.");
+			if (type)
+				log_warn("  %sall%-*s - %s", type->prefix,
+					 (int) (id_len - 3 - strlen(type->prefix)), "",
+					 "All fields in this section.");
 		}
 
 		/* FIXME Add line-wrapping at terminal width (or 80 cols) */
-- 
1.7.3.1

[PATCH 04/30] Fix NULL pointer dereference

[Zdenek Kabelac]

vg pointer is not yet initialized in this place and vgname must be used.

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

diff --git a/lib/format_text/format-text.c b/lib/format_text/format-text.c
index eee0114..0102a15 100644
--- a/lib/format_text/format-text.c
+++ b/lib/format_text/format-text.c
@@ -507,7 +507,7 @@ static struct volume_group *_vg_read_raw_area(struct format_instance *fid,
 
 	if (wrap > rlocn->offset) {
 		log_error("VG %s metadata too large for circular buffer",
-			  vg->name);
+			  vgname);
 		goto out;
 	}
 
-- 
1.7.3.1

[PATCH 05/30] Print vg_name and do not to access vg->name

[Zdenek Kabelac]

clang Logic error Dereference of null pointer
Replacing pointer access with vg_name.

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

diff --git a/tools/pvresize.c b/tools/pvresize.c
index ccd025a..af499e5 100644
--- a/tools/pvresize.c
+++ b/tools/pvresize.c
@@ -148,7 +148,7 @@ static int _pv_resize_single(struct cmd_context *cmd,
 	if (!is_orphan_vg(vg_name)) {
 		if (!vg_write(vg) || !vg_commit(vg)) {
 			log_error("Failed to store physical volume \"%s\" in "
-				  "volume group \"%s\"", pv_name, vg->name);
+				  "volume group \"%s\"", pv_name, vg_name);
 			goto out;
 		}
 		backup(vg);
-- 
1.7.3.1

[PATCH 06/30] Ensure we always have origin defined

[Zdenek Kabelac]

clang Logic error	Dereference of null pointer
Ensure the code path could not use origin == NULL because of internal
error in code.

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

diff --git a/tools/lvcreate.c b/tools/lvcreate.c
index de5b59c..a9e3c2d 100644
--- a/tools/lvcreate.c
+++ b/tools/lvcreate.c
@@ -179,6 +179,10 @@ static int _update_extents_params(struct volume_group *vg,
 					  lp->origin);
 				return 0;
 			}
+			if (!origin) {
+				log_error(INTERNAL_ERROR "Couldn't find origin volume.");
+				return 0;
+			}
 			lp->extents = lp->extents * origin->le_count / 100;
 			break;
 		case PERCENT_NONE:
-- 
1.7.3.1

[PATCH 07/30] Ensure seg is nonnull

[Zdenek Kabelac]

clang Logic error	Dereference of null pointer
Make sure we do not try to use NULL seg pointer.

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

diff --git a/lib/activate/dev_manager.c b/lib/activate/dev_manager.c
index aab0c9a..216b042 100644
--- a/lib/activate/dev_manager.c
+++ b/lib/activate/dev_manager.c
@@ -615,7 +615,7 @@ int dev_manager_transient(struct dev_manager *dm, struct logical_volume *lv)
 		if (!type || !params)
 			continue;
 
-		if (seg->segtype->ops->check_transient_status &&
+		if (seg && seg->segtype->ops->check_transient_status &&
 		    !seg->segtype->ops->check_transient_status(seg, params))
 			goto_out;
 
-- 
1.7.3.1

[PATCH 08/30] Ensure first is not NULL

[Zdenek Kabelac]

clang Logic error	Dereference of null pointer

For empty lists make sure the NULL first pointer is not dereferenced.

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

diff --git a/lib/format1/format1.c b/lib/format1/format1.c
index 0dcec05..0efde31 100644
--- a/lib/format1/format1.c
+++ b/lib/format1/format1.c
@@ -103,7 +103,7 @@ static int _check_vgs(struct dm_list *pvs, struct volume_group *vg)
 	}
 
 	/* On entry to fn, list known to be non-empty */
-	if (pv_count != first->vgd.pv_cur) {
+	if (first && (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);
 		vg->status |= PARTIAL_VG;
-- 
1.7.3.1

[PATCH 09/30] Reuse result of previous strchr.

[Zdenek Kabelac]

clang Argument with 'nonnull' attribute passed null
Reuse result of the last strchr call - it also makes sure,
'st' point is nonnull for next strchr call.

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 02c4962..5adc160 100644
--- a/tools/toollib.c
+++ b/tools/toollib.c
@@ -869,7 +869,7 @@ const char *extract_vgname(struct cmd_context *cmd, const char *lv_name)
 			while (*st == '/')
 				st++;
 
-		if (!strchr(vg_name, '/') || strchr(st, '/')) {
+		if (!st || strchr(st, '/')) {
 			log_error("\"%s\": Invalid path for Logical Volume",
 				  lv_name);
 			return 0;
-- 
1.7.3.1

[PATCH 10/30] Fix potential NULL pointer dereference

[Zdenek Kabelac]

Makes clang happier as it covers all code paths and avoids NULL pointer
dereference through com pointer (which is NULL by default static
initialisation).

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

diff --git a/tools/lvm.c b/tools/lvm.c
index 60f9a79..9cac23e 100644
--- a/tools/lvm.c
+++ b/tools/lvm.c
@@ -85,11 +85,11 @@ static char *_list_args(const char *text, int state)
 				break;
 			}
 		}
-
-		if (!com)
-			return NULL;
 	}
 
+	if (!com)
+		return NULL;
+
 	/* Short form arguments */
 	if (len < 3) {
 		while (match_no < com->num_args) {
-- 
1.7.3.1

[PATCH 11/30] Fix theoretical usage of NULL pointer dereference

[Zdenek Kabelac]

clang seems to be happier with this check.

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

diff --git a/libdm/regex/ttree.c b/libdm/regex/ttree.c
index ec97c98..0ad40bd 100644
--- a/libdm/regex/ttree.c
+++ b/libdm/regex/ttree.c
@@ -97,7 +97,9 @@ int ttree_insert(struct ttree *tt, unsigned int *key, void *data)
 			}
 		}
 	}
-	(*c)->data = data;
+
+	if (*c)
+		(*c)->data = data;
 
 	return 1;
 }
-- 
1.7.3.1

[PATCH 12/30] Check for NULL pointer

[Zdenek Kabelac]

clangs is happier

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

diff --git a/tools/reporter.c b/tools/reporter.c
index f39d23f..eb0a857 100644
--- a/tools/reporter.c
+++ b/tools/reporter.c
@@ -323,7 +323,7 @@ static int _report(struct cmd_context *cmd, int argc, char **argv,
 			log_error("Invalid options string: %s", opts);
 			return EINVALID_CMD_LINE;
 		}
-		if (*opts == '+') {
+		if (*opts == '+' && options) {
 			if (!(str = dm_pool_alloc(cmd->mem,
 					 strlen(options) + strlen(opts) + 1))) {
 				log_error("options string allocation failed");
-- 
1.7.3.1

[PATCH 13/30] Eliminate uninitialized_var

[Zdenek Kabelac]

Use some default value instead of a random number for unknown
minor_version.

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

diff --git a/lib/device/dev-md.c b/lib/device/dev-md.c
index 89b9341..cb9feae 100644
--- a/lib/device/dev-md.c
+++ b/lib/device/dev-md.c
@@ -59,7 +59,7 @@ typedef enum {
 
 static uint64_t _v1_sb_offset(uint64_t size, md_minor_version_t minor_version)
 {
-	uint64_t uninitialized_var(sb_offset);
+	uint64_t sb_offset;
 
 	switch(minor_version) {
 	case MD_MINOR_V0:
@@ -69,6 +69,7 @@ static uint64_t _v1_sb_offset(uint64_t size, md_minor_version_t minor_version)
 		sb_offset = 0;
 		break;
 	case MD_MINOR_V2:
+	default:
 		sb_offset = 4 * 2;
 		break;
 	}
-- 
1.7.3.1

[PATCH 14/30] Fix missing initilisation to 0

[Zdenek Kabelac]

Missing init value for 'found' which is later tested and may
containe garbage value.

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

diff --git a/daemons/cmirrord/cluster.c b/daemons/cmirrord/cluster.c
index dddaec9..b6c925a 100644
--- a/daemons/cmirrord/cluster.c
+++ b/daemons/cmirrord/cluster.c
@@ -129,7 +129,7 @@ int cluster_send(struct clog_request *rq)
 {
 	int r;
 	int count=0;
-	int found;
+	int found = 0;
 	struct iovec iov;
 	struct clog_cpg *entry;
 
-- 
1.7.3.1

[PATCH 15/30] Function pull_state cannot work with NULL buffer

[Zdenek Kabelac]

In practice this situation should not happen in current lvm code.

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

diff --git a/daemons/cmirrord/functions.c b/daemons/cmirrord/functions.c
index e5da757..58e76ac 100644
--- a/daemons/cmirrord/functions.c
+++ b/daemons/cmirrord/functions.c
@@ -1809,8 +1809,10 @@ int pull_state(const char *uuid, uint64_t luid,
 	int bitset_size;
 	struct log_c *lc;
 
-	if (!buf)
+	if (!buf) {
 		LOG_ERROR("pull_state: buf == NULL");
+		return -EINVAL;
+	}
 
 	lc = get_log(uuid, luid);
 	if (!lc) {
-- 
1.7.3.1

[PATCH 16/30] Fix void* pointer arithmetic

[Zdenek Kabelac]

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

diff --git a/lib/mm/memlock.c b/lib/mm/memlock.c
index 3de8cc1..1e3065c 100644
--- a/lib/mm/memlock.c
+++ b/lib/mm/memlock.c
@@ -87,8 +87,8 @@ static size_t _mstats; /* statistic for maps locking */
 static void _touch_memory(void *mem, size_t size)
 {
 	size_t pagesize = lvm_getpagesize();
-	void *pos = mem;
-	void *end = mem + size - sizeof(long);
+	char *pos = mem;
+	char *end = pos + size - sizeof(long);
 
 	while (pos < end) {
 		*(long *) pos = 1;
-- 
1.7.3.1

[PATCH 17/30] Use const pvid for device_from_pvid()

[Zdenek Kabelac]

Update interface for device_from_pvid and use const pvid.

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

diff --git a/lib/cache/lvmcache.c b/lib/cache/lvmcache.c
index 35d6f76..83ac1a5 100644
--- a/lib/cache/lvmcache.c
+++ b/lib/cache/lvmcache.c
@@ -733,7 +733,7 @@ struct dm_list *lvmcache_get_pvids(struct cmd_context *cmd, const char *vgname,
 	return pvids;
 }
 
-struct device *device_from_pvid(struct cmd_context *cmd, struct id *pvid,
+struct device *device_from_pvid(struct cmd_context *cmd, const struct id *pvid,
 				unsigned *scan_done_once)
 {
 	struct label *label;
diff --git a/lib/cache/lvmcache.h b/lib/cache/lvmcache.h
index 4966a5c..28f8541 100644
--- a/lib/cache/lvmcache.h
+++ b/lib/cache/lvmcache.h
@@ -94,7 +94,7 @@ struct lvmcache_vginfo *vginfo_from_vgname(const char *vgname,
 struct lvmcache_vginfo *vginfo_from_vgid(const char *vgid);
 struct lvmcache_info *info_from_pvid(const char *pvid, int valid_only);
 const char *vgname_from_vgid(struct dm_pool *mem, const char *vgid);
-struct device *device_from_pvid(struct cmd_context *cmd, struct id *pvid,
+struct device *device_from_pvid(struct cmd_context *cmd, const struct id *pvid,
 				unsigned *scan_done_once);
 const char *pvid_from_devname(struct cmd_context *cmd,
 			      const char *dev_name);
-- 
1.7.3.1

[PATCH 18/30] Use const pointer for return value of dm_basename

[Zdenek Kabelac]

Fix pointer created from passed input const pointer.

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

diff --git a/libdm/libdevmapper.h b/libdm/libdevmapper.h
index 7ed60be..e2f2673 100644
--- a/libdm/libdevmapper.h
+++ b/libdm/libdevmapper.h
@@ -955,7 +955,7 @@ int dm_snprintf(char *buf, size_t bufsize, const char *format, ...)
 /*
  * Returns pointer to the last component of the path.
  */
-char *dm_basename(const char *path);
+const char *dm_basename(const char *path);
 
 /**************************
  * file/stream manipulation
diff --git a/libdm/libdm-string.c b/libdm/libdm-string.c
index ce05a10..d8fca1a 100644
--- a/libdm/libdm-string.c
+++ b/libdm/libdm-string.c
@@ -123,11 +123,11 @@ int dm_snprintf(char *buf, size_t bufsize, const char *format, ...)
 	return n;
 }
 
-char *dm_basename(const char *path)
+const char *dm_basename(const char *path)
 {
-	char *p = strrchr(path, '/');
+	const char *p = strrchr(path, '/');
 
-	return p ? p + 1 : (char *) path;
+	return p ? p + 1 : path;
 }
 
 int dm_asprintf(char **result, const char *format, ...)
-- 
1.7.3.1

[PATCH 19/30] Fix constness warning

[Zdenek Kabelac]

After update of device_from_pvid API fix few constness warnings

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 83ac1a5..60edcec 100644
--- a/lib/cache/lvmcache.c
+++ b/lib/cache/lvmcache.c
@@ -514,7 +514,7 @@ char *lvmcache_vgname_from_pvid(struct cmd_context *cmd, const char *pvid)
 	struct lvmcache_info *info;
 	char *vgname;
 
-	if (!device_from_pvid(cmd, (struct id *)pvid, NULL)) {
+	if (!device_from_pvid(cmd, (const struct id *)pvid, NULL)) {
 		log_error("Couldn't find device with uuid %s.", pvid);
 		return NULL;
 	}
@@ -740,7 +740,7 @@ struct device *device_from_pvid(struct cmd_context *cmd, const struct id *pvid,
 	struct lvmcache_info *info;
 
 	/* Already cached ? */
-	if ((info = info_from_pvid((char *) pvid, 0))) {
+	if ((info = info_from_pvid((const char *) pvid, 0))) {
 		if (label_read(info->dev, &label, UINT64_C(0))) {
 			info = (struct lvmcache_info *) label->info;
 			if (id_equal(pvid, (struct id *) &info->dev->pvid))
@@ -751,7 +751,7 @@ struct device *device_from_pvid(struct cmd_context *cmd, const struct id *pvid,
 	lvmcache_label_scan(cmd, 0);
 
 	/* Try again */
-	if ((info = info_from_pvid((char *) pvid, 0))) {
+	if ((info = info_from_pvid((const char *) pvid, 0))) {
 		if (label_read(info->dev, &label, UINT64_C(0))) {
 			info = (struct lvmcache_info *) label->info;
 			if (id_equal(pvid, (struct id *) &info->dev->pvid))
@@ -767,7 +767,7 @@ struct device *device_from_pvid(struct cmd_context *cmd, const struct id *pvid,
 		*scan_done_once = 1;
 
 	/* Try again */
-	if ((info = info_from_pvid((char *) pvid, 0))) {
+	if ((info = info_from_pvid((const char *) pvid, 0))) {
 		if (label_read(info->dev, &label, UINT64_C(0))) {
 			info = (struct lvmcache_info *) label->info;
 			if (id_equal(pvid, (struct id *) &info->dev->pvid))
-- 
1.7.3.1

[PATCH 20/30] Fix constness warning

[Zdenek Kabelac]

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 2da4085..77fdd02 100644
--- a/lib/metadata/metadata.c
+++ b/lib/metadata/metadata.c
@@ -3141,7 +3141,7 @@ struct logical_volume *lv_from_lvid(struct cmd_context *cmd, const char *lvid_s,
 	lvid = (const union lvid *) lvid_s;
 
 	log_very_verbose("Finding volume group for uuid %s", lvid_s);
-	if (!(vg = _vg_read_by_vgid(cmd, (char *)lvid->id[0].uuid, precommitted))) {
+	if (!(vg = _vg_read_by_vgid(cmd, (const char *)lvid->id[0].uuid, precommitted))) {
 		log_error("Volume group for uuid not found: %s", lvid_s);
 		return NULL;
 	}
-- 
1.7.3.1

[PATCH 21/30] Fix constness warning

[Zdenek Kabelac]

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

diff --git a/lib/misc/crc.c b/lib/misc/crc.c
index f2f2f06..9f95c37 100644
--- a/lib/misc/crc.c
+++ b/lib/misc/crc.c
@@ -59,8 +59,8 @@ static uint32_t _calc_crc_new(uint32_t initial, const uint8_t *buf, uint32_t siz
 		0xbdbdf21c, 0xcabac28a, 0x53b39330, 0x24b4a3a6, 0xbad03605, 0xcdd70693, 0x54de5729, 0x23d967bf,
 		0xb3667a2e, 0xc4614ab8, 0x5d681b02, 0x2a6f2b94, 0xb40bbe37, 0xc30c8ea1, 0x5a05df1b, 0x2d02ef8d,
 	};
-	uint32_t *start = (uint32_t *) buf;
-	uint32_t *end = (uint32_t *) (buf + (size & 0xfffffffc));
+	const uint32_t *start = (const uint32_t *) buf;
+	const uint32_t *end = (const uint32_t *) (buf + (size & 0xfffffffc));
 	uint32_t crc = initial;
    
 	/* Process 4 bytes per iteration */
@@ -73,7 +73,7 @@ static uint32_t _calc_crc_new(uint32_t initial, const uint8_t *buf, uint32_t siz
 	}
 
 	/* Process any bytes left over */
-	buf = (uint8_t *) start;
+	buf = (const uint8_t *) start;
 	size = size & 0x3;
 	while (size--) {
 		crc = crc ^ *buf++;
-- 
1.7.3.1

[PATCH 22/30] Fix constness warning

[Zdenek Kabelac]

Fix usage of const 'data' pointer and also assign void* directly without
uneeded cast for C.

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

diff --git a/tools/dmsetup.c b/tools/dmsetup.c
index a38fc4c..88291f7 100644
--- a/tools/dmsetup.c
+++ b/tools/dmsetup.c
@@ -2399,7 +2399,7 @@ static int _dm_deps_disp(struct dm_report *rh, struct dm_pool *mem,
 			 struct dm_report_field *field, const void *data,
 			 void *private)
 {
-	struct dm_deps *deps = (struct dm_deps *) data;
+	const struct dm_deps *deps = data;
 	int i;
 	char buf[DM_MAX_TYPE_NAME], *repstr;
 
-- 
1.7.3.1

[PATCH 23/30] Using const config node

[Zdenek Kabelac]

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

diff --git a/lib/striped/striped.c b/lib/striped/striped.c
index 01fbc1e..44f9774 100644
--- a/lib/striped/striped.c
+++ b/lib/striped/striped.c
@@ -71,7 +71,7 @@ static int _striped_text_import_area_count(struct config_node *sn, uint32_t *are
 static int _striped_text_import(struct lv_segment *seg, const struct config_node *sn,
 			struct dm_hash_table *pv_hash)
 {
-	struct config_node *cn;
+	const struct config_node *cn;
 
 	if ((seg->area_count != 1) &&
 	    !get_config_uint32(sn, "stripe_size", &seg->stripe_size)) {
-- 
1.7.3.1

[PATCH 24/30] bufused is assigned 0 in preceding source line

[Zdenek Kabelac]

clang Idempotent operation

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

diff --git a/lib/log/log.c b/lib/log/log.c
index 5c49586..86b4988 100644
--- a/lib/log/log.c
+++ b/lib/log/log.c
@@ -359,7 +359,7 @@ void print_log(int level, const char *file, int line, int dm_errno,
 		_already_logging = 1;
 		memset(&buf, ' ', sizeof(buf));
 		bufused = 0;
-		if ((n = dm_snprintf(buf, sizeof(buf) - bufused - 1,
+		if ((n = dm_snprintf(buf, sizeof(buf) - 1,
 				      "%s:%d %s%s", file, line, log_command_name(),
 				      _msg_prefix)) == -1)
 			goto done;
-- 
1.7.3.1

[PATCH 25/30] Proper C declaration

[Zdenek Kabelac]

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

diff --git a/daemons/dmeventd/dmeventd.c b/daemons/dmeventd/dmeventd.c
index d179879..bbe6e10 100644
--- a/daemons/dmeventd/dmeventd.c
+++ b/daemons/dmeventd/dmeventd.c
@@ -1477,7 +1477,7 @@ static void _process_request(struct dm_event_fifos *fifos)
 	dm_free(msg.data);
 }
 
-static void _process_initial_registrations()
+static void _process_initial_registrations(void)
 {
 	int i = 0;
 	char *reg;
@@ -1697,7 +1697,7 @@ static void _daemonize(void)
 	setsid();
 }
 
-static void restart()
+static void restart(void)
 {
 	struct dm_event_fifos fifos;
 	struct dm_event_daemon_message msg = { 0, 0, NULL };
-- 
1.7.3.1

[PATCH 26/30] Use static

[Zdenek Kabelac]

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

diff --git a/libdm/mm/pool-fast.c b/libdm/mm/pool-fast.c
index d9030cf..ebd982e 100644
--- a/libdm/mm/pool-fast.c
+++ b/libdm/mm/pool-fast.c
@@ -33,8 +33,8 @@ struct dm_pool {
 	unsigned object_alignment;
 };
 
-void _align_chunk(struct chunk *c, unsigned alignment);
-struct chunk *_new_chunk(struct dm_pool *p, size_t s);
+static void _align_chunk(struct chunk *c, unsigned alignment);
+static struct chunk *_new_chunk(struct dm_pool *p, size_t s);
 static void _free_chunk(struct chunk *c);
 
 /* by default things come out aligned for doubles */
@@ -240,12 +240,12 @@ void dm_pool_abandon_object(struct dm_pool *p)
 	p->object_alignment = DEFAULT_ALIGNMENT;
 }
 
-void _align_chunk(struct chunk *c, unsigned alignment)
+static void _align_chunk(struct chunk *c, unsigned alignment)
 {
 	c->begin += alignment - ((unsigned long) c->begin & (alignment - 1));
 }
 
-struct chunk *_new_chunk(struct dm_pool *p, size_t s)
+static struct chunk *_new_chunk(struct dm_pool *p, size_t s)
 {
 	struct chunk *c;
 
-- 
1.7.3.1

[PATCH 27/30] Instrument compiler about code unreachability

[Zdenek Kabelac]

Clang needs some instrumentation for code analysis.
It might probably help also for gcc.

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

diff --git a/daemons/clvmd/clvmd.c b/daemons/clvmd/clvmd.c
index 57f7ffe..f9096cc 100644
--- a/daemons/clvmd/clvmd.c
+++ b/daemons/clvmd/clvmd.c
@@ -491,6 +491,7 @@ int main(int argc, char *argv[])
 		DEBUGLOG("Can't initialise cluster interface\n");
 		log_error("Can't initialise cluster interface\n");
 		child_init_signal(DFAIL_CLUSTER_IF);
+		__builtin_unreachable();
 	}
 	DEBUGLOG("Cluster ready, doing some more initialisation\n");
 
@@ -505,8 +506,10 @@ int main(int argc, char *argv[])
 
 	/* Add the local socket to the list */
 	newfd = malloc(sizeof(struct local_client));
-	if (!newfd)
-	        child_init_signal(DFAIL_MALLOC);
+	if (!newfd) {
+		child_init_signal(DFAIL_MALLOC);
+		__builtin_unreachable();
+	}
 
 	newfd->fd = local_sock;
 	newfd->removeme = 0;
-- 
1.7.3.1

[PATCH 28/30] Skip uninitialized macro for scan-build

[Zdenek Kabelac]

scan-build reports assigment of ununitialised values.

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

diff --git a/lib/misc/util.h b/lib/misc/util.h
index fcda369..59e6980 100644
--- a/lib/misc/util.h
+++ b/lib/misc/util.h
@@ -25,7 +25,11 @@
 		     (void) (&_a == &_b); \
 		     _a > _b ? _a : _b; })
 
+#ifndef  __clang__
 #define uninitialized_var(x) x = x
+#else
+#define uninitialized_var(x) x
+#endif
 
 #define KERNEL_VERSION(major, minor, release) (((major) << 16) + ((minor) << 8) + (release))
 
-- 
1.7.3.1

[PATCH 29/30] Add some __attribute__ instrumentation

[Zdenek Kabelac]

Add some malloc intrumentation and warning attributes.

Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>
---
 libdm/libdevmapper.h |   26 +++++++++++++-------------
 1 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/libdm/libdevmapper.h b/libdm/libdevmapper.h
index e2f2673..9ae8bbd 100644
--- a/libdm/libdevmapper.h
+++ b/libdm/libdevmapper.h
@@ -510,13 +510,13 @@ uint32_t dm_tree_get_cookie(struct dm_tree_node *node);
  * Memory management
  *******************/
 
-void *dm_malloc_aux(size_t s, const char *file, int line);
-void *dm_malloc_aux_debug(size_t s, const char *file, int line);
-void *dm_zalloc_aux(size_t s, const char *file, int line);
-void *dm_zalloc_aux_debug(size_t s, const char *file, int line);
-char *dm_strdup_aux(const char *str, const char *file, int line);
+void *dm_malloc_aux(size_t s, const char *file, int line) __attribute__((malloc)) __attribute__((__warn_unused_result__));
+void *dm_malloc_aux_debug(size_t s, const char *file, int line) __attribute__((__warn_unused_result__));
+void *dm_zalloc_aux(size_t s, const char *file, int line)  __attribute__((malloc)) __attribute__((__warn_unused_result__));
+void *dm_zalloc_aux_debug(size_t s, const char *file, int line) __attribute__((__warn_unused_result__));
+char *dm_strdup_aux(const char *str, const char *file, int line) __attribute__((malloc)) __attribute__((__warn_unused_result__));
 void dm_free_aux(void *p);
-void *dm_realloc_aux(void *p, unsigned int s, const char *file, int line);
+void *dm_realloc_aux(void *p, unsigned int s, const char *file, int line) __attribute__((__warn_unused_result__));
 int dm_dump_memory_debug(void);
 void dm_bounds_check_debug(void);
 
@@ -579,12 +579,12 @@ void dm_bounds_check_debug(void);
 struct dm_pool;
 
 /* constructor and destructor */
-struct dm_pool *dm_pool_create(const char *name, size_t chunk_hint);
+struct dm_pool *dm_pool_create(const char *name, size_t chunk_hint) __attribute__((__warn_unused_result__));
 void dm_pool_destroy(struct dm_pool *p);
 
 /* simple allocation/free routines */
-void *dm_pool_alloc(struct dm_pool *p, size_t s);
-void *dm_pool_alloc_aligned(struct dm_pool *p, size_t s, unsigned alignment);
+void *dm_pool_alloc(struct dm_pool *p, size_t s) __attribute__((__warn_unused_result__));
+void *dm_pool_alloc_aligned(struct dm_pool *p, size_t s, unsigned alignment) __attribute__((__warn_unused_result__));
 void dm_pool_empty(struct dm_pool *p);
 void dm_pool_free(struct dm_pool *p, void *ptr);
 
@@ -639,9 +639,9 @@ void *dm_pool_end_object(struct dm_pool *p);
 void dm_pool_abandon_object(struct dm_pool *p);
 
 /* utilities */
-char *dm_pool_strdup(struct dm_pool *p, const char *str);
-char *dm_pool_strndup(struct dm_pool *p, const char *str, size_t n);
-void *dm_pool_zalloc(struct dm_pool *p, size_t s);
+char *dm_pool_strdup(struct dm_pool *p, const char *str) __attribute__((__warn_unused_result__));
+char *dm_pool_strndup(struct dm_pool *p, const char *str, size_t n) __attribute__((__warn_unused_result__));
+void *dm_pool_zalloc(struct dm_pool *p, size_t s) __attribute__((__warn_unused_result__));
 
 /******************
  * bitset functions
@@ -699,7 +699,7 @@ struct dm_hash_node;
 
 typedef void (*dm_hash_iterate_fn) (void *data);
 
-struct dm_hash_table *dm_hash_create(unsigned size_hint);
+struct dm_hash_table *dm_hash_create(unsigned size_hint) __attribute__((__warn_unused_result__));
 void dm_hash_destroy(struct dm_hash_table *t);
 void dm_hash_wipe(struct dm_hash_table *t);
 
-- 
1.7.3.1

[PATCH 30/30] __attribute__((nonnull(1)))

[Zdenek Kabelac]

Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>
---
 lib/device/dev-cache.h |    2 +-
 libdm/regex/ttree.c    |    3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/lib/device/dev-cache.h b/lib/device/dev-cache.h
index c1c86d6..772d28e 100644
--- a/lib/device/dev-cache.h
+++ b/lib/device/dev-cache.h
@@ -41,7 +41,7 @@ int dev_cache_has_scanned(void);
 
 int dev_cache_add_dir(const char *path);
 int dev_cache_add_loopfile(const char *path);
-struct device *dev_cache_get(const char *name, struct dev_filter *f);
+struct device *dev_cache_get(const char *name, struct dev_filter *f) __attribute__((nonnull(1)));
 
 void dev_set_preferred_name(struct str_list *sl, struct device *dev);
 
diff --git a/libdm/regex/ttree.c b/libdm/regex/ttree.c
index 0ad40bd..fc24c96 100644
--- a/libdm/regex/ttree.c
+++ b/libdm/regex/ttree.c
@@ -28,7 +28,8 @@ struct ttree {
 	struct node *root;
 };
 
-static struct node **_lookup_single(struct node **c, unsigned int k)
+static __attribute__((nonnull (1)))
+struct node **_lookup_single(struct node **c, unsigned int k)
 {
 	while (*c) {
 		if (k < (*c)->k)
-- 
1.7.3.1

[PATCH 00/30] Fixes from clang

[Jim Meyering]

Zdenek Kabelac wrote:
> Patchset combines code cleanups for compiler warnings
> and code fixes found by static analyzer scan-build.
>
> Potentional NULL pointer dereferencies are mostly
> in very unusual code execution paths. I've tried to
> order them on their importance - the later ones are not
> event trigerable as surrounding code should not be to
> pass problematic values - but checks seems to be simple
> enough to keep static analysis happy.
>
> Last patches in this set add some instrumentation to
> function calls that might allow better code optimization
> or better static analysis.
>
> Zdenek Kabelac (30):
>   Fix clang warning for ntohl(*((uint32_t *)buf))
>   Use dm_pool_grow_object embeded strlen for 0 delta
>   Check (type) is not NULL before access
>   Fix NULL pointer dereference
>   Print vg_name and do not to access vg->name
>   Ensure we always have origin defined
>   Ensure seg is nonnull
>   Ensure  first is not NULL
>   Reuse result of previous strchr.
>   Fix potential NULL pointer dereference
>   Fix theoretical usage of NULL pointer dereference
>   Check for NULL pointer
>   Eliminate uninitialized_var
>   Fix missing initilisation to 0
>   Function pull_state cannot work with NULL buffer
>   Fix  void* pointer arithmetic
>   Use const pvid for device_from_pvid()
>   Use const pointer for return value of dm_basename
>   Fix constness warning
>   Fix constness warning
>   Fix constness warning
>   Fix constness warning
>   Using const config node
>   bufused is assigned 0 in preceding source line
>   Proper C declaration
>   Use static
>   Instrument compiler about code unreachability
>   Skip uninitialized macro for scan-build
>   Add some __attribute__ instrumentation
>   __attribute__((nonnull(1)))

Nice work!
Getting any project in good enough shape to pass clang's
tests is tedious but well worth it.

[PATCH 01/30] Fix clang warning for ntohl(*((uint32_t *)buf))

[ejt]

At Mon, 25 Oct 2010 10:24:08 +0200,
Zdenek Kabelac wrote:
> 
> We cast (char*) to (uint32_t*) that changes alignment requierements.
> For our case the code has been correct as alloca() returns properly
> aligned buffer, however this patch make it cleaner and more readable
> and avoids warning generation.
> 
> FIXME: code duplication
> 
> Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>

Ack

[PATCH 04/30] Fix NULL pointer dereference

[ejt]

At Mon, 25 Oct 2010 10:24:11 +0200,
Zdenek Kabelac wrote:
> 
> vg pointer is not yet initialized in this place and vgname must be used.
> 
> Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>

ack

[PATCH 02/30] Use dm_pool_grow_object embeded strlen for 0 delta

[ejt]

At Mon, 25 Oct 2010 10:24:09 +0200,
Zdenek Kabelac wrote:
> 
> Use internal strlen for 0
> 
> Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>

Ack, but I don't think this improves the readablility of the code.  The problem is the extra semantics of zero as the third arg to dm_pool_grow_object() (probably my bad).  Maybe introduce a new pool method dm_pool_grow_string(struct dm_pool *p, const char *str) ?

[PATCH 03/30] Check (type) is not NULL before access

[ejt]

At Mon, 25 Oct 2010 10:24:10 +0200,
Zdenek Kabelac wrote:
> 
> clang Logic error	Dereference of null pointer

Should a generic warning be given if |type| is NULL?

[PATCH 05/30] Print vg_name and do not to access vg->name

[ejt]

At Mon, 25 Oct 2010 10:24:12 +0200,
Zdenek Kabelac wrote:
> 
> clang Logic error Dereference of null pointer
> Replacing pointer access with vg_name.
> 
> Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>

Ack

[PATCH 07/30] Ensure seg is nonnull

[ejt]

At Mon, 25 Oct 2010 10:24:14 +0200,
Zdenek Kabelac wrote:
> 
> clang Logic error	Dereference of null pointer
> Make sure we do not try to use NULL seg pointer.
> 
> Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>

Does |seg| being NULL indicate a more serious problem?  Should we fail rather than making the best of it and carrying on silently?

[PATCH 09/30] Reuse result of previous strchr.

[ejt]

At Mon, 25 Oct 2010 10:24:16 +0200,
Zdenek Kabelac wrote:
> 
> clang Argument with 'nonnull' attribute passed null
> Reuse result of the last strchr call - it also makes sure,
> 'st' point is nonnull for next strchr call.
> 
> Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>

Ack

[PATCH 06/30] Ensure we always have origin defined

[ejt]

At Mon, 25 Oct 2010 10:24:13 +0200,
Zdenek Kabelac wrote:
> 
> clang Logic error	Dereference of null pointer
> Ensure the code path could not use origin == NULL because of internal
> error in code.
> 
> Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>

Ack

[PATCH 08/30] Ensure first is not NULL

[ejt]

At Mon, 25 Oct 2010 10:24:15 +0200,
Zdenek Kabelac wrote:
> 
> clang Logic error	Dereference of null pointer
> 
> For empty lists make sure the NULL first pointer is not dereferenced.
> 
> Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>

Again I'm worried you're hiding a problem.  Can |first| be NULL, what does it mean?

[PATCH 10/30] Fix potential NULL pointer dereference

[ejt]

At Mon, 25 Oct 2010 10:24:17 +0200,
Zdenek Kabelac wrote:
> 
> Makes clang happier as it covers all code paths and avoids NULL pointer
> dereference through com pointer (which is NULL by default static
> initialisation).
> 
> Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>

Ack

[PATCH 13/30] Eliminate uninitialized_var

[ejt]

At Mon, 25 Oct 2010 10:24:20 +0200,
Zdenek Kabelac wrote:
> 
> Use some default value instead of a random number for unknown
> minor_version.
> 
> Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>

Nack.  What if your default |sb_offset| is wrong?

[PATCH 14/30] Fix missing initilisation to 0

[ejt]

At Mon, 25 Oct 2010 10:24:21 +0200,
Zdenek Kabelac wrote:
> 
> Missing init value for 'found' which is later tested and may
> containe garbage value.
> 
> Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>

Ack

[PATCH 15/30] Function pull_state cannot work with NULL buffer

[ejt]

At Mon, 25 Oct 2010 10:24:22 +0200,
Zdenek Kabelac wrote:
> 
> In practice this situation should not happen in current lvm code.
> 
> Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>

Ack

[PATCH 16/30] Fix void* pointer arithmetic

[ejt]

At Mon, 25 Oct 2010 10:24:23 +0200,
Zdenek Kabelac wrote:
> 
> 
> Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>

Nack.  No explanation.  eg, Are we deprecating the void pointer arithmetic gcc extension?

[PATCH 11/30] Fix theoretical usage of NULL pointer dereference

[ejt]

At Mon, 25 Oct 2010 10:24:18 +0200,
Zdenek Kabelac wrote:
> 
> clang seems to be happier with this check.
> 
> Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>

Nack, this can't happen.  Or rather if it does something has gone badly wrong.  So if (!*c) { stack; return 0; } would be better.

[PATCH 17/30] Use const pvid for device_from_pvid()

[ejt]

At Mon, 25 Oct 2010 10:24:24 +0200,
Zdenek Kabelac wrote:
> 
> Update interface for device_from_pvid and use const pvid.
> 
> Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>

Ack

[PATCH 20/30] Fix constness warning

[ejt]

At Mon, 25 Oct 2010 10:24:27 +0200,
Zdenek Kabelac wrote:
> 
> 
> Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>

Ack

[PATCH 19/30] Fix constness warning

[ejt]

At Mon, 25 Oct 2010 10:24:26 +0200,
Zdenek Kabelac wrote:
> 
> After update of device_from_pvid API fix few constness warnings
> 
> Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>

Ack.  But all this casting suggests we should change types for some of these functions (eg, info_from_pvid).

[PATCH 21/30] Fix constness warning

[ejt]

At Mon, 25 Oct 2010 10:24:28 +0200,
Zdenek Kabelac wrote:
> 
> 
> Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>

Ack

[PATCH 22/30] Fix constness warning

[ejt]

At Mon, 25 Oct 2010 10:24:29 +0200,
Zdenek Kabelac wrote:
> 
> Fix usage of const 'data' pointer and also assign void* directly without
> uneeded cast for C.
> 
> Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>

Ack

[PATCH 24/30] bufused is assigned 0 in preceding source line

[ejt]

At Mon, 25 Oct 2010 10:24:31 +0200,
Zdenek Kabelac wrote:
> 
> clang Idempotent operation
> 
> Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>

Ack

[PATCH 23/30] Using const config node

[ejt]

At Mon, 25 Oct 2010 10:24:30 +0200,
Zdenek Kabelac wrote:
> 
> 
> Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>

Ack

[PATCH 18/30] Use const pointer for return value of dm_basename

[ejt]

At Mon, 25 Oct 2010 10:24:25 +0200,
Zdenek Kabelac wrote:
> 
> Fix pointer created from passed input const pointer.
> 
> Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>

Ack

[PATCH 26/30] Use static

[ejt]

At Mon, 25 Oct 2010 10:24:33 +0200,
Zdenek Kabelac wrote:
> 
> 
> Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>

Ack.  Add comment

[PATCH 28/30] Skip uninitialized macro for scan-build

[ejt]

At Mon, 25 Oct 2010 10:24:35 +0200,
Zdenek Kabelac wrote:
> 
> scan-build reports assigment of ununitialised values.
> 
> Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>

Ack.  Surprised that works.

[PATCH 27/30] Instrument compiler about code unreachability

[ejt]

At Mon, 25 Oct 2010 10:24:34 +0200,
Zdenek Kabelac wrote:
> 
> Clang needs some instrumentation for code analysis.
> It might probably help also for gcc.
> 
> Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>

Ack.  It helps human readers too.

[PATCH 25/30] Proper C declaration

[ejt]

At Mon, 25 Oct 2010 10:24:32 +0200,
Zdenek Kabelac wrote:
> 
> 
> Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>

Ack.  Fluff, not sure you need to send that to the list.

[PATCH 29/30] Add some __attribute__ instrumentation

[ejt]

At Mon, 25 Oct 2010 10:24:36 +0200,
Zdenek Kabelac wrote:
> 
> Add some malloc intrumentation and warning attributes.
> 
> Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>

Ack.  But I question how far you intend to go with this.  Surely it's more unusual to return a value that can be ignored?

[PATCH 30/30] __attribute__((nonnull(1)))

[ejt]

At Mon, 25 Oct 2010 10:24:37 +0200,
Zdenek Kabelac wrote:
> 
> 
> Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>
> ---
>  lib/device/dev-cache.h |    2 +-
>  libdm/regex/ttree.c    |    3 ++-
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/device/dev-cache.h b/lib/device/dev-cache.h
> index c1c86d6..772d28e 100644
> --- a/lib/device/dev-cache.h
> +++ b/lib/device/dev-cache.h
> @@ -41,7 +41,7 @@ int dev_cache_has_scanned(void);
>  
>  int dev_cache_add_dir(const char *path);
>  int dev_cache_add_loopfile(const char *path);
> -struct device *dev_cache_get(const char *name, struct dev_filter *f);
> +struct device *dev_cache_get(const char *name, struct dev_filter *f) __attribute__((nonnull(1)));
>  
>  void dev_set_preferred_name(struct str_list *sl, struct device *dev);
>  
> diff --git a/libdm/regex/ttree.c b/libdm/regex/ttree.c
> index 0ad40bd..fc24c96 100644
> --- a/libdm/regex/ttree.c
> +++ b/libdm/regex/ttree.c
> @@ -28,7 +28,8 @@ struct ttree {
>  	struct node *root;
>  };
>  
> -static struct node **_lookup_single(struct node **c, unsigned int k)
> +static __attribute__((nonnull (1)))
> +struct node **_lookup_single(struct node **c, unsigned int k)

Ack.  Verbose, isn't it.

[PATCH 02/30] Use dm_pool_grow_object embeded strlen for 0 delta

[Zdenek Kabelac]

Dne 25.10.2010 11:56, ejt at redhat.com napsal(a):
> At Mon, 25 Oct 2010 10:24:09 +0200,
> Zdenek Kabelac wrote:
>>
>> Use internal strlen for 0
>>
>> Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>
> 
> Ack, but I don't think this improves the readablility of the code.  The problem is the extra semantics of zero as the third arg to dm_pool_grow_object() (probably my bad).  Maybe introduce a new pool method dm_pool_grow_string(struct dm_pool *p, const char *str) ?


Well you are not the first one with this idea :)

Me and Milan thinks in the same way - if everyone agree - I'll make another
version with  ...grow_string()

Zdenek

[PATCH 13/30] Eliminate uninitialized_var

[Zdenek Kabelac]

Dne 25.10.2010 12:18, ejt at redhat.com napsal(a):
> At Mon, 25 Oct 2010 10:24:20 +0200,
> Zdenek Kabelac wrote:
>>
>> Use some default value instead of a random number for unknown
>> minor_version.
>>
>> Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>
> 
> Nack.  What if your default |sb_offset| is wrong?

Still better something defined - then completely random value.

But this patch is rather about make compiler analyzer quiet -
maybe  default: assert() for 'generic' version ?

>From the source code - we are actually using only  MD_MINOR_V0,
as is: minor = MD_MINOR_VERSION_MIN (== MD_MINOR_V0)

Zdenek

[PATCH 16/30] Fix void* pointer arithmetic

[Zdenek Kabelac]

Dne 25.10.2010 12:20, ejt at redhat.com napsal(a):
> At Mon, 25 Oct 2010 10:24:23 +0200,
> Zdenek Kabelac wrote:
>>
>>
>> Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>
> 
> Nack.  No explanation.  eg, Are we deprecating the void pointer arithmetic gcc extension?

void* arithmetic is IMHO bad practice and we should avoid it's usage.
char* clearly defines 1byte - I'd prefer to use standard C whenever we can.
(And gcc can show this as warning - so why not fixing it)

Zdenek

[PATCH 13/30] Eliminate uninitialized_var

[ejt]

At Mon, 25 Oct 2010 13:13:13 +0200,
Zdenek Kabelac wrote:
> But this patch is rather about make compiler analyzer quiet -
> maybe  default: assert() for 'generic' version ?

Or just plain abort() so it doesn't get compiled out in the release version?

[PATCH 16/30] Fix void* pointer arithmetic

[ejt]

At Mon, 25 Oct 2010 13:17:05 +0200,
Zdenek Kabelac wrote:
> 
> Dne 25.10.2010 12:20, ejt at redhat.com napsal(a):
> > At Mon, 25 Oct 2010 10:24:23 +0200,
> > Zdenek Kabelac wrote:
> >>
> >>
> >> Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>
> > 
> > Nack.  No explanation.  eg, Are we deprecating the void pointer arithmetic gcc extension?
> 
> void* arithmetic is IMHO bad practice and we should avoid it's usage.
> char* clearly defines 1byte - I'd prefer to use standard C whenever we can.
> (And gcc can show this as warning - so why not fixing it)

Ack

[PATCH 16/30] Fix void* pointer arithmetic

[Milan Broz]

On 10/25/2010 10:24 AM, Zdenek Kabelac wrote:

>  static void _touch_memory(void *mem, size_t size)
>  {
>  	size_t pagesize = lvm_getpagesize();
> -	void *pos = mem;
> -	void *end = mem + size - sizeof(long);
> +	char *pos = mem;
> +	char *end = pos + size - sizeof(long);

And what about trash this "touch memory" code completely?

The whole idea of touching memory before locking is IMHO obsolete.

Locking memory should do this implicitly - can we prove that this
code has some effect on recent systems?

Milan

[PATCH 16/30] Fix void* pointer arithmetic

[Zdenek Kabelac]

Dne 25.10.2010 13:23, Milan Broz napsal(a):
> On 10/25/2010 10:24 AM, Zdenek Kabelac wrote:
> 
>>  static void _touch_memory(void *mem, size_t size)
>>  {
>>  	size_t pagesize = lvm_getpagesize();
>> -	void *pos = mem;
>> -	void *end = mem + size - sizeof(long);
>> +	char *pos = mem;
>> +	char *end = pos + size - sizeof(long);
> 
> And what about trash this "touch memory" code completely?
> 
> The whole idea of touching memory before locking is IMHO obsolete.
> 
> Locking memory should do this implicitly - can we prove that this
> code has some effect on recent systems?
> 

Our memlock code should be physically allocating and paging all the memory to
the process space - so the only difference is IMHO at which point in time
memory failure happens - with memory touching - it might occur sooner in some
cases - but we lose some speed by trashing CPU cache and flagging memory pages.

Zdenek

[PATCH 29/30] Add some __attribute__ instrumentation

[Zdenek Kabelac]

Dne 25.10.2010 12:44, ejt at redhat.com napsal(a):
> At Mon, 25 Oct 2010 10:24:36 +0200,
> Zdenek Kabelac wrote:
>>
>> Add some malloc intrumentation and warning attributes.
>>
>> Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>
> 
> Ack.  But I question how far you intend to go with this.  Surely it's more unusual to return a value that can be ignored?
> 


I think wherever it is an error to ignore returned value on public API
function, we should put some warning.  In this case it must lead to memory
leak - so it's good idea to give the user this knowledge in compile time.

(btw I'm doing nothing more then what original system headers do for you :))

What seems to have some 'hidden' fruits are those 'nonnull' __attributes__.
We have quite a few functions which take a pointer and expect it to be
non-null - so we could mark these functions and get static analysis to be done
on them.

Zdenek

[PATCH 02/30] Use dm_pool_grow_object embeded strlen for 0 delta

[Alasdair G Kergon]

On Mon, Oct 25, 2010 at 12:59:42PM +0200, Zdenek Kabelac wrote:
> version with  ...grow_string()
 
Go for it.

Alasdair