[PATCH 00/23] Static analyzis set

[PATCH 00/23] Static analyzis set

[Zdenek Kabelac]

This is mostly the last bigger patch sets for covering
issues reported by Coverity tool.

Few patches needs some closer looking,
Most of them are trivial, but as they may present
some forgotten tests or missing code I'm posting
them for review.


Zdenek Kabelac (23):
  Remove dead assignment to thisfd
  Remove dead assignment
  Remove dead assigment of mirr_state
  Remove dead assignment of lock_flags
  Remove dead assignment in vgconvert_single
  Log of pthread_join operation
  Move  lastfd handling into for()
  Fix memory leak in error path of restart_clvmd
  Add test for errors in _setup_task
  Fix memory leak of dev_filter on error path
  Add stack traces for archive
  Add tests for dm_task_set operations
  Make it obvious this code is not used
  Add stack trace for backup and backup_remove
  Add test check for find_config_str
  Remove check for vg != NULL
  Add check for dm_snprintf result
  Return PERCENT_INVALID for error case
  Warning - dead code problem elimination
  exit if lvmchache_init fails
  Add default error path for get_property
  Remove dead assignment in dm_event_get_registered_device
  Remove dead assignment in vg_split_mdas

 daemons/clvmd/clvmd-command.c         |    3 +-
 daemons/clvmd/clvmd.c                 |   25 ++++++++++---------
 daemons/dmeventd/dmeventd.c           |    3 +-
 daemons/dmeventd/libdevmapper-event.c |   41 +++++++++++++++++---------------
 lib/activate/dev_manager.c            |   21 ++++++++++------
 lib/commands/toolcontext.c            |    2 +
 lib/filters/filter-persistent.c       |   22 ++++++++++++-----
 lib/format_text/archiver.c            |    9 +++++--
 lib/format_text/import_vsn1.c         |   18 ++++++--------
 lib/locking/cluster_locking.c         |   14 ++++++-----
 lib/locking/file_locking.c            |   16 +++++++++---
 lib/metadata/metadata.c               |    8 +++---
 lib/mirror/mirrored.c                 |    3 --
 lib/report/properties.c               |   15 +++++++-----
 liblvm/lvm_misc.c                     |    4 +++
 tools/lvscan.c                        |    6 -----
 tools/polldaemon.c                    |    3 +-
 tools/vgconvert.c                     |    4 +--
 tools/vgrename.c                      |    6 +++-
 19 files changed, 125 insertions(+), 98 deletions(-)

--
1.7.3.4

[PATCH 01/23] Remove dead assignment to thisfd

[Zdenek Kabelac]

Value 'thisfd' is not read again after this assigment.

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

diff --git a/daemons/clvmd/clvmd.c b/daemons/clvmd/clvmd.c
index 1a35c49..a6839f4 100644
--- a/daemons/clvmd/clvmd.c
+++ b/daemons/clvmd/clvmd.c
@@ -827,7 +827,6 @@ static void main_loop(int local_sock, int cmd_timeout)
 					struct local_client *free_fd;
 					lastfd->next = thisfd->next;
 					free_fd = thisfd;
-					thisfd = lastfd;
 
 					DEBUGLOG("removeme set for fd %d\n", free_fd->fd);
 
@@ -863,7 +862,6 @@ static void main_loop(int local_sock, int cmd_timeout)
 							 ret, errno);
 						lastfd->next = thisfd->next;
 						free_fd = thisfd;
-						thisfd = lastfd;
 						safe_close(&(free_fd->fd));
 
 						/* Queue cleanup, this also frees the client struct */
-- 
1.7.3.4

[PATCH 01/23] Remove dead assignment to thisfd

[Alasdair G Kergon]

On Tue, Dec 21, 2010 at 04:41:33PM +0100, Zdenek Kabelac wrote:
> Value 'thisfd' is not read again after this assigment.
 
Ack.

Alasdair

[PATCH 02/23] Remove dead assignment

[Zdenek Kabelac]

Variables 'lv_total' and 'lv_capasity_total' are unused.

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

diff --git a/tools/lvscan.c b/tools/lvscan.c
index 638d2f5..1f91d39 100644
--- a/tools/lvscan.c
+++ b/tools/lvscan.c
@@ -19,8 +19,6 @@ static int lvscan_single(struct cmd_context *cmd, struct logical_volume *lv,
 			 void *handle __attribute__((unused)))
 {
 	struct lvinfo info;
-	int lv_total = 0;
-	uint64_t lv_capacity_total = 0;
 	int inkernel, snap_active = 1;
 	struct lv_segment *snap_seg = NULL;
 	percent_t snap_percent;     /* fused, fsize; */
@@ -66,10 +64,6 @@ static int lvscan_single(struct cmd_context *cmd, struct logical_volume *lv,
 		  display_size(cmd, lv->size),
 		  get_alloc_string(lv->alloc));
 
-	lv_total++;
-
-	lv_capacity_total += lv->size;
-
 	return ECMD_PROCESSED;
 }
 
-- 
1.7.3.4

[PATCH 02/23] Remove dead assignment

[Alasdair G Kergon]

On Tue, Dec 21, 2010 at 04:41:34PM +0100, Zdenek Kabelac wrote:
> Variables 'lv_total' and 'lv_capasity_total' are unused.

Ack.

Probably LVM1 used to display these, but we never bothered to
reimplement lvscan fully as there are better commands to use
instead.

Alasdair

[PATCH 03/23] Remove dead assigment of mirr_state

[Zdenek Kabelac]

Variable 'mirr_state' is not used.

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

diff --git a/lib/mirror/mirrored.c b/lib/mirror/mirrored.c
index 3d25d70..0bf3b3b 100644
--- a/lib/mirror/mirrored.c
+++ b/lib/mirror/mirrored.c
@@ -184,7 +184,6 @@ static int _mirrored_target_percent(void **target_state,
 				    uint64_t *total_numerator,
 				    uint64_t *total_denominator)
 {
-	struct mirror_state *mirr_state;
 	uint64_t numerator, denominator;
 	unsigned mirror_count, m;
 	int used;
@@ -193,8 +192,6 @@ static int _mirrored_target_percent(void **target_state,
 	if (!*target_state)
 		*target_state = _mirrored_init_target(mem, cmd);
 
-	mirr_state = *target_state;
-
 	/* Status line: <#mirrors> (maj:min)+ <synced>/<total_regions> */
 	log_debug("Mirror status: %s", params);
 
-- 
1.7.3.4

[PATCH 03/23] Remove dead assigment of mirr_state

[Alasdair G Kergon]

On Tue, Dec 21, 2010 at 04:41:35PM +0100, Zdenek Kabelac wrote:
> Variable 'mirr_state' is not used.
 
Ack.

Alasdair

[PATCH 04/23] Remove dead assignment of lock_flags

[Zdenek Kabelac]

Variable 'lock_flags' is not used.

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

diff --git a/daemons/clvmd/clvmd-command.c b/daemons/clvmd/clvmd-command.c
index 49f1197..95d924b 100644
--- a/daemons/clvmd/clvmd-command.c
+++ b/daemons/clvmd/clvmd-command.c
@@ -182,7 +182,6 @@ static int lock_vg(struct local_client *client)
     struct clvm_header *header =
 	(struct clvm_header *) client->bits.localsock.cmd;
     unsigned char lock_cmd;
-    unsigned char lock_flags;
     int lock_mode;
     char *args = header->node + strlen(header->node) + 1;
     int lkid;
@@ -204,7 +203,6 @@ static int lock_vg(struct local_client *client)
 
     lock_cmd = args[0] & (LCK_NONBLOCK | LCK_HOLD | LCK_SCOPE_MASK | LCK_TYPE_MASK);
     lock_mode = ((int)lock_cmd & LCK_TYPE_MASK);
-    lock_flags = args[1];
     lockname = &args[2];
     DEBUGLOG("doing PRE command LOCK_VG '%s' at %x (client=%p)\n", lockname, lock_cmd, client);
 
-- 
1.7.3.4

[PATCH 04/23] Remove dead assignment of lock_flags

[Milan Broz]

On 12/21/2010 04:41 PM, Zdenek Kabelac wrote:
> @@ -204,7 +203,6 @@ static int lock_vg(struct local_client *client)
>  
>      lock_cmd = args[0] & (LCK_NONBLOCK | LCK_HOLD | LCK_SCOPE_MASK | LCK_TYPE_MASK);
>      lock_mode = ((int)lock_cmd & LCK_TYPE_MASK);
> -    lock_flags = args[1];
>      lockname = &args[2];

I intentionally kept unused args[1] here because it is clear how the request look like
without searching code, IOW

- 0 byte is command
- 1 byte flags
- 2 lockname

So any possible changes is easy on place.
Yes, it should be struct and should use some friendly names. But now there is byte array...

Complier is clever enough to remove that code.

I do not care what some stupid static analysis says, the code is more readable with
"dead" variable for me.

Milan

[PATCH 04/23] Remove dead assignment of lock_flags

[Zdenek Kabelac]

Dne 21.12.2010 17:20, Milan Broz napsal(a):
> On 12/21/2010 04:41 PM, Zdenek Kabelac wrote:
>> @@ -204,7 +203,6 @@ static int lock_vg(struct local_client *client)
>>  
>>      lock_cmd = args[0] & (LCK_NONBLOCK | LCK_HOLD | LCK_SCOPE_MASK | LCK_TYPE_MASK);
>>      lock_mode = ((int)lock_cmd & LCK_TYPE_MASK);
>> -    lock_flags = args[1];
>>      lockname = &args[2];
> 
> I intentionally kept unused args[1] here because it is clear how the request look like
> without searching code, IOW
> 
> - 0 byte is command
> - 1 byte flags
> - 2 lockname
> 
> So any possible changes is easy on place.
> Yes, it should be struct and should use some friendly names. But now there is byte array...
> 
> Complier is clever enough to remove that code.
> 
> I do not care what some stupid static analysis says, the code is more readable with
> "dead" variable for me.

I always thought that's why /* comments */  were invented ;) for things like
this, but anyway in this case if you prefer to keep the unused value present -
I'll hide it only for my local scan-builds :)

Zdenek

[PATCH 04/23] Remove dead assignment of lock_flags

[Alasdair G Kergon]

On Wed, Dec 22, 2010 at 11:11:36AM +0100, Zdenek Kabelac wrote:
> I always thought that's why /* comments */  were invented ;) for things like
> this, but anyway in this case if you prefer to keep the unused value present -
> I'll hide it only for my local scan-builds :)
 
So do a patch that moves it into a comment?

Alasdair

[PATCH 04/23] Remove dead assignment of lock_flags

[Milan Broz]

On 01/05/2011 01:30 AM, Alasdair G Kergon wrote:
> On Wed, Dec 22, 2010 at 11:11:36AM +0100, Zdenek Kabelac wrote:
>> I always thought that's why /* comments */  were invented ;) for things like
>> this, but anyway in this case if you prefer to keep the unused value present -
>> I'll hide it only for my local scan-builds :)
>  
> So do a patch that moves it into a comment?

That code is messy, and it should be cleaned - the lock_vg() name is misleading
(it should be called pre_lock_vg(), it should take pre-parsed flags and not parse
it itself from client struct - the same like lock_lv etc)

So if you think cleaning one variable help anything, better do it than spent another
week discussing it :-)

Milan

[PATCH 05/23] Remove dead assignment in vgconvert_single

[Zdenek Kabelac]

'pe_end' is not used - remove it.

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

diff --git a/tools/vgconvert.c b/tools/vgconvert.c
index acae0fc..7d1ee19 100644
--- a/tools/vgconvert.c
+++ b/tools/vgconvert.c
@@ -26,7 +26,7 @@ static int vgconvert_single(struct cmd_context *cmd, const char *vg_name,
 	struct dm_list mdas;
 	int pvmetadatacopies = 0;
 	uint64_t pvmetadatasize = 0;
-	uint64_t pe_end = 0, pe_start = 0;
+	uint64_t pe_start = 0;
 	struct pv_list *pvl;
 	int change_made = 0;
 	struct lvinfo info;
@@ -119,8 +119,6 @@ static int vgconvert_single(struct cmd_context *cmd, const char *vg_name,
 		existing_pv = pvl->pv;
 
 		pe_start = pv_pe_start(existing_pv);
-		pe_end = pv_pe_count(existing_pv) * pv_pe_size(existing_pv)
-		    + pe_start - 1;
 
 		dm_list_init(&mdas);
 		if (!(pv = pv_create(cmd, pv_dev(existing_pv),
-- 
1.7.3.4

[PATCH 05/23] Remove dead assignment in vgconvert_single

[Alasdair G Kergon]

On Tue, Dec 21, 2010 at 04:41:37PM +0100, Zdenek Kabelac wrote:
> 'pe_end' is not used - remove it.
 
>  		pe_start = pv_pe_start(existing_pv);
> -		pe_end = pv_pe_count(existing_pv) * pv_pe_size(existing_pv)
> -		    + pe_start - 1;

Please retain that formula in a comment instead.

Alasdair

[PATCH 06/23] Log of pthread_join operation

[Zdenek Kabelac]

Value jstat is unused - so replace it with logging via log_sys_error().

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

diff --git a/daemons/clvmd/clvmd.c b/daemons/clvmd/clvmd.c
index a6839f4..7bb28a3 100644
--- a/daemons/clvmd/clvmd.c
+++ b/daemons/clvmd/clvmd.c
@@ -666,7 +666,6 @@ static int local_pipe_callback(struct local_client *thisfd, char *buf,
 
 	/* EOF on pipe or an error, close it */
 	if (len <= 0) {
-		int jstat;
 		void *ret = &status;
 		close(thisfd->fd);
 
@@ -677,7 +676,9 @@ static int local_pipe_callback(struct local_client *thisfd, char *buf,
 
 		/* Reap child thread */
 		if (thisfd->bits.pipe.threadid) {
-			jstat = pthread_join(thisfd->bits.pipe.threadid, &ret);
+			if (pthread_join(thisfd->bits.pipe.threadid, &ret))
+				log_sys_error("pthread_join", "");
+
 			thisfd->bits.pipe.threadid = 0;
 			if (thisfd->bits.pipe.client != NULL)
 				thisfd->bits.pipe.client->bits.localsock.
@@ -1039,7 +1040,6 @@ static int read_from_local_sock(struct local_client *thisfd)
 	/* EOF or error on socket */
 	if (len <= 0) {
 		int *status;
-		int jstat;
 
 		DEBUGLOG("EOF on local socket: inprogress=%d\n",
 			 thisfd->bits.localsock.in_progress);
@@ -1066,9 +1066,10 @@ static int read_from_local_sock(struct local_client *thisfd)
 			pthread_cond_signal(&thisfd->bits.localsock.cond);
 			pthread_mutex_unlock(&thisfd->bits.localsock.mutex);
 
-			jstat =
-			    pthread_join(thisfd->bits.localsock.threadid,
-					 (void **) &status);
+			if (pthread_join(thisfd->bits.localsock.threadid,
+					 (void **) &status))
+				log_sys_error("pthread_join", "");
+
 			DEBUGLOG("Joined child thread\n");
 
 			thisfd->bits.localsock.threadid = 0;
-- 
1.7.3.4

[PATCH 06/23] Log of pthread_join operation

[Alasdair G Kergon]

On Tue, Dec 21, 2010 at 04:41:38PM +0100, Zdenek Kabelac wrote:
> Value jstat is unused - so replace it with logging via log_sys_error().
 
Ack.

Alasdair

[PATCH 07/23] Move lastfd handling into for()

[Zdenek Kabelac]

WARNING - unclear code.

The loop inside for() works with assumption lastfd would be defined
when internal 'if' branches are taken.

Patch adds check for the case lastfd was not yet defined - i.e. removeme
of the first element in the list (code itself should be probably switched
to standard dm_list?)

>From the code reading it's not quite clear how the code should work and
if the case could actually ever happen.

As I'm not familiar with CLVMD logic here - it's probably for someone with
better knowledge to decide what is the right step here.

Other solution could be - to check 'lastfd' before if() and continue for
the first loop - i.e. expecting at least 2 members in list ?

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

diff --git a/daemons/clvmd/clvmd.c b/daemons/clvmd/clvmd.c
index 7bb28a3..fa5a726 100644
--- a/daemons/clvmd/clvmd.c
+++ b/daemons/clvmd/clvmd.c
@@ -826,7 +826,8 @@ static void main_loop(int local_sock, int cmd_timeout)
 
 				if (thisfd->removeme) {
 					struct local_client *free_fd;
-					lastfd->next = thisfd->next;
+					if (lastfd)
+						lastfd->next = thisfd->next;
 					free_fd = thisfd;
 
 					DEBUGLOG("removeme set for fd %d\n", free_fd->fd);
@@ -861,7 +862,8 @@ static void main_loop(int local_sock, int cmd_timeout)
 
 						DEBUGLOG("ret == %d, errno = %d. removing client\n",
 							 ret, errno);
-						lastfd->next = thisfd->next;
+						if (lastfd)
+							lastfd->next = thisfd->next;
 						free_fd = thisfd;
 						safe_close(&(free_fd->fd));
 
@@ -1092,8 +1094,8 @@ static int read_from_local_sock(struct local_client *thisfd)
 					    pipe_client == newfd) {
 						thisfd->bits.localsock.
 						    pipe_client = NULL;
-
-						lastfd->next = newfd->next;
+						if (lastfd)
+							lastfd->next = newfd->next;
 						free_fd = newfd;
 						newfd->next = lastfd;
 						free(free_fd);
-- 
1.7.3.4

[PATCH 07/23] Move lastfd handling into for()

[Milan Broz]

On 12/21/2010 04:41 PM, Zdenek Kabelac wrote:

> @@ -826,7 +826,8 @@ static void main_loop(int local_sock, int cmd_timeout)
>  
>  				if (thisfd->removeme) {
>  					struct local_client *free_fd;
> -					lastfd->next = thisfd->next;
> +					if (lastfd)
> +						lastfd->next = thisfd->next;

Isn't that replacing one bug with another?

If lastfd is NULL, it will crash.
After your change it will quietly do something, apparently with wrong pointers.

If lastfd cannot be NULL, the check is not needed and is redundant.

I *think* that the first fd in select is local one, removing itself is nonsense,
so lastfd should be always set. (lastfd is set in the for cycle)

Anyway I prefer to keep the code intact instead of hiding possible bug.

(Or rewrite it to something more readable. e.g. removeme flag
seems to be used only in gulm code, which is dead and unsupported etc)

Milan

[PATCH 07/23] Move lastfd handling into for()

[Zdenek Kabelac]

Dne 21.12.2010 17:50, Milan Broz napsal(a):
> On 12/21/2010 04:41 PM, Zdenek Kabelac wrote:
> 
>> @@ -826,7 +826,8 @@ static void main_loop(int local_sock, int cmd_timeout)
>>  
>>  				if (thisfd->removeme) {
>>  					struct local_client *free_fd;
>> -					lastfd->next = thisfd->next;
>> +					if (lastfd)
>> +						lastfd->next = thisfd->next;
> 
> Isn't that replacing one bug with another?
> 
> If lastfd is NULL, it will crash.
> After your change it will quietly do something, apparently with wrong pointers.
> 
> If lastfd cannot be NULL, the check is not needed and is redundant.
> 
> I *think* that the first fd in select is local one, removing itself is nonsense,
> so lastfd should be always set. (lastfd is set in the for cycle)
> 
> Anyway I prefer to keep the code intact instead of hiding possible bug.
> 
> (Or rewrite it to something more readable. e.g. removeme flag
> seems to be used only in gulm code, which is dead and unsupported etc)
> 


The code is really a bit complicated to read in this case - so that's why I've
commented the patch that clvmd expert is needed.

Other way around in the patch could be - to simply skip first loop case
and assign the first value directly to  'lastfd' and start for() directly with
the next element. That would make code more obvious that the list always needs
to have at least 2 members.

Other question is - if we do not support gulm anymore - why do we keep
unmaintained and untested code ? :)

Zdenek

[PATCH 07/23] Move lastfd handling into for()

[Alasdair G Kergon]

On Wed, Dec 22, 2010 at 11:16:40AM +0100, Zdenek Kabelac wrote:
> Other question is - if we do not support gulm anymore - why do we keep
> unmaintained and untested code ? :)
 
If this is gulm-only code, just leave it exactly as it is.  No need to
try to clean it up or to fix theoretical problems in it unless
responding to patches from people using it and trying to fix it.

Alasdair

[PATCH 07/23] Move lastfd handling into for()

[Alasdair G Kergon]

On Tue, Dec 21, 2010 at 05:50:09PM +0100, Milan Broz wrote:
> Anyway I prefer to keep the code intact instead of hiding possible bug.
 
Ack, if this is only a gulm thing.
Just add a comment to that effect.

Alasdair

[PATCH 08/23] Fix memory leak in error path of restart_clvmd

[Zdenek Kabelac]

Minor memory leak in debug mode.

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

diff --git a/daemons/clvmd/clvmd-command.c b/daemons/clvmd/clvmd-command.c
index 95d924b..b1ea9fa 100644
--- a/daemons/clvmd/clvmd-command.c
+++ b/daemons/clvmd/clvmd-command.c
@@ -406,6 +406,7 @@ out:
 	for (i = 0; i < argc && argv[i]; i++)
 		free(argv[i]);
 	free(argv);
+	free(debug_arg);
 
 	return 0;
 }
-- 
1.7.3.4

[PATCH 09/23] Add test for errors in _setup_task

[Zdenek Kabelac]

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

diff --git a/lib/activate/dev_manager.c b/lib/activate/dev_manager.c
index a11d918..2c2fc19 100644
--- a/lib/activate/dev_manager.c
+++ b/lib/activate/dev_manager.c
@@ -75,19 +75,22 @@ static struct dm_task *_setup_task(const char *name, const char *uuid,
 	if (!(dmt = dm_task_create(task)))
 		return_NULL;
 
-	if (name)
-		dm_task_set_name(dmt, name);
+	if (name && !dm_task_set_name(dmt, name))
+		goto_out;
 
-	if (uuid && *uuid)
-		dm_task_set_uuid(dmt, uuid);
+	if (uuid && *uuid && !dm_task_set_uuid(dmt, uuid))
+		goto_out;
 
-	if (event_nr)
-		dm_task_set_event_nr(dmt, *event_nr);
+	if (event_nr && !dm_task_set_event_nr(dmt, *event_nr))
+		goto_out;
 
-	if (major)
-		dm_task_set_major_minor(dmt, major, minor, 1);
+	if (major && !dm_task_set_major_minor(dmt, major, minor, 1))
+		goto_out;
 
 	return dmt;
+      out:
+	dm_task_destroy(dmt);
+	return NULL;
 }
 
 static int _info_run(const char *name, const char *dlid, struct dm_info *info,
-- 
1.7.3.4

[PATCH 09/23] Add test for errors in _setup_task

[Milan Broz]

On 12/21/2010 04:41 PM, Zdenek Kabelac wrote:
> -	if (uuid && *uuid)
> -		dm_task_set_uuid(dmt, uuid);
> +	if (uuid && *uuid && !dm_task_set_uuid(dmt, uuid))
> +		goto_out;

Shouldn't it fail if uuid is requested but it is empty string?
(but that's another problem:-)

Milan

[PATCH 09/23] Add test for errors in _setup_task

[Zdenek Kabelac]

Dne 21.12.2010 18:12, Milan Broz napsal(a):
> On 12/21/2010 04:41 PM, Zdenek Kabelac wrote:
>> -	if (uuid && *uuid)
>> -		dm_task_set_uuid(dmt, uuid);
>> +	if (uuid && *uuid && !dm_task_set_uuid(dmt, uuid))
>> +		goto_out;
> 
> Shouldn't it fail if uuid is requested but it is empty string?
> (but that's another problem:-)
> 

If this is invalid combination:

(uuid && (!*uuid || !dm_task_set_uuid())

Zdenek

[PATCH 09/23] Add test for errors in _setup_task

[Alasdair G Kergon]

On Wed, Dec 22, 2010 at 11:23:15AM +0100, Zdenek Kabelac wrote:
> Dne 21.12.2010 18:12, Milan Broz napsal(a):
> > On 12/21/2010 04:41 PM, Zdenek Kabelac wrote:
> >> -	if (uuid && *uuid)
> >> -		dm_task_set_uuid(dmt, uuid);
> >> +	if (uuid && *uuid && !dm_task_set_uuid(dmt, uuid))
> >> +		goto_out;
> > 
> > Shouldn't it fail if uuid is requested but it is empty string?
> > (but that's another problem:-)

Is there a case for moving the "Is uuid empty?" test inside dm_task_set_uuid().
IOW if dm_task_set_uuid() is passed an empty string, I think it should erase
the existing uuid but not bother to create a new one.

Perhaps?

int dm_task_set_uuid(struct dm_task *dmt, const char *uuid)
{
        dm_free(dmt->uuid);

	if (!uuid || !*uuid)
		return 1;

        if (!(dmt->uuid = dm_strdup(uuid))) {
                log_error("dm_task_set_uuid: strdup(%s) failed", uuid);
                return 0;
        }

        return 1;
}

Alasdair

[PATCH 09/23] Add test for errors in _setup_task

[Alasdair G Kergon]

Ack.

Alasdair

[PATCH 10/23] Fix memory leak of dev_filter on error path

[Zdenek Kabelac]

If the allocation of peristent filter fails - its memory reference
is lost.

Also fix log_error message for failing allocation and add proper
destroy call on error path.

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

diff --git a/lib/commands/toolcontext.c b/lib/commands/toolcontext.c
index c4da38a..0f24ef9 100644
--- a/lib/commands/toolcontext.c
+++ b/lib/commands/toolcontext.c
@@ -707,6 +707,7 @@ static int _init_filters(struct cmd_context *cmd, unsigned load_persistent_cache
 		    cache_dir ? "" : "/",
 		    cache_dir ? : DEFAULT_CACHE_SUBDIR,
 		    cache_file_prefix ? : DEFAULT_CACHE_FILE_PREFIX) < 0) {
+			f3->destroy(f3);
 			log_error("Persistent cache filename too long.");
 			return 0;
 		}
@@ -715,6 +716,7 @@ static int _init_filters(struct cmd_context *cmd, unsigned load_persistent_cache
 				"%s/%s/%s.cache",
 				cmd->system_dir, DEFAULT_CACHE_SUBDIR,
 				DEFAULT_CACHE_FILE_PREFIX) < 0)) {
+		f3->destroy(f3);
 		log_error("Persistent cache filename too long.");
 		return 0;
 	}
diff --git a/lib/filters/filter-persistent.c b/lib/filters/filter-persistent.c
index f3b1e05..b8b0488 100644
--- a/lib/filters/filter-persistent.c
+++ b/lib/filters/filter-persistent.c
@@ -318,13 +318,16 @@ struct dev_filter *persistent_filter_create(struct dev_filter *real,
 	struct dev_filter *f = NULL;
 	struct stat info;
 
-	if (!(pf = dm_zalloc(sizeof(*pf))))
-		return_NULL;
+	if (!(pf = dm_zalloc(sizeof(*pf)))) {
+		log_error("Allocation of persistent filter failed.");
+		goto fail;
+	}
 
-	if (!(pf->file = dm_malloc(strlen(file) + 1)))
-		goto_bad;
+	if (!(pf->file = dm_strdup(file))) {
+		log_error("Filename duplication for persistent filter failed.");
+		goto bad;
+	}
 
-	strcpy(pf->file, file);
 	pf->real = real;
 
 	if (!(_init_hash(pf))) {
@@ -332,8 +335,10 @@ struct dev_filter *persistent_filter_create(struct dev_filter *real,
 		goto bad;
 	}
 
-	if (!(f = dm_malloc(sizeof(*f))))
-		goto_bad;
+	if (!(f = dm_malloc(sizeof(*f)))) {
+		log_error("Allocation of device filter for persistent filter failed.");
+		goto bad;
+	}
 
 	/* Only merge cache file before dumping it if it changed externally. */
 	if (!stat(pf->file, &info))
@@ -352,5 +357,8 @@ struct dev_filter *persistent_filter_create(struct dev_filter *real,
 		dm_hash_destroy(pf->devices);
 	dm_free(pf);
 	dm_free(f);
+
+     fail:
+	real->destroy(real);
 	return NULL;
 }
-- 
1.7.3.4

[PATCH 10/23] Fix memory leak of dev_filter on error path

[Milan Broz]

On 12/21/2010 04:41 PM, Zdenek Kabelac wrote:

> @@ -352,5 +357,8 @@ struct dev_filter *persistent_filter_create(struct dev_filter *real,
>  		dm_hash_destroy(pf->devices);
>  	dm_free(pf);
>  	dm_free(f);
> +
> +     fail:
> +	real->destroy(real);
>  	return NULL;
>  }

Why not move it to the caller instead - the same like previous two?

	if (!(f4 = persistent_filter_create(f3, dev_cache))) {
		log_error("Failed to create persistent device filter");
+		f3->destroy(f3); 
		return 0;
	}

Milan

[PATCH 10/23] Fix memory leak of dev_filter on error path

[Zdenek Kabelac]

Dne 21.12.2010 18:21, Milan Broz napsal(a):
> On 12/21/2010 04:41 PM, Zdenek Kabelac wrote:
> 
>> @@ -352,5 +357,8 @@ struct dev_filter *persistent_filter_create(struct dev_filter *real,
>>  		dm_hash_destroy(pf->devices);
>>  	dm_free(pf);
>>  	dm_free(f);
>> +
>> +     fail:
>> +	real->destroy(real);
>>  	return NULL;
>>  }
> 
> Why not move it to the caller instead - the same like previous two?
> 
> 	if (!(f4 = persistent_filter_create(f3, dev_cache))) {
> 		log_error("Failed to create persistent device filter");
> +		f3->destroy(f3); 
> 		return 0;
> 	}
> 


I'm not sure about the best logic - so used this as a plain proposal - of
course both variants could be seen as valid. And as I've been already fixing
error messages in the function it self, I've put the call of destructor there.

My idea was - one the persistent_filter_create() is called - it takes
ownership of the passed struct dev_filter.

Your proposal means - that in error case the function doesn't take
responsibility for this value and caller is supposed to cleanup.

Do we agree on this semantic ?

Zdenek

[PATCH 10/23] Fix memory leak of dev_filter on error path

[Alasdair G Kergon]

On Wed, Dec 22, 2010 at 11:05:14AM +0100, Zdenek Kabelac wrote:
> I'm not sure about the best logic - so used this as a plain proposal - of
> course both variants could be seen as valid. And as I've been already fixing
> error messages in the function it self, I've put the call of destructor there.
> 
> My idea was - one the persistent_filter_create() is called - it takes
> ownership of the passed struct dev_filter.
 
I don't think it matters which you pick, but add comments to make it clear
who's responsible for cleanup.

Alasdair

[PATCH 10/23] Fix memory leak of dev_filter on error path

[Alasdair G Kergon]

On Tue, Dec 21, 2010 at 06:21:47PM +0100, Milan Broz wrote:
> Why not move it to the caller instead - the same like previous two?

Oh, consistency, yes please!

Alasdair

[PATCH 11/23] Add stack traces for archive

[Zdenek Kabelac]

If archive of back_locally fails - add stack trace.

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

diff --git a/lib/format_text/archiver.c b/lib/format_text/archiver.c
index ef85c6c..3d291c8 100644
--- a/lib/format_text/archiver.c
+++ b/lib/format_text/archiver.c
@@ -452,9 +452,12 @@ void check_current_backup(struct volume_group *vg)
 	log_suppress(old_suppress);
 
 	if (vg_backup) {
-		archive(vg_backup);
+		if (!archive(vg_backup))
+			stack;
 		free_vg(vg_backup);
 	}
-	archive(vg);
-	backup_locally(vg);
+	if (!archive(vg))
+		stack;
+	if (!backup_locally(vg))
+		stack;
 }
-- 
1.7.3.4

[PATCH 12/23] Add tests for dm_task_set operations

[Zdenek Kabelac]

Check for errors in dm_task_set calls and use  goto_bad macro with stack
Replace  label  failed: with bad:

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

diff --git a/daemons/dmeventd/libdevmapper-event.c b/daemons/dmeventd/libdevmapper-event.c
index bc8ad99..05ebf0a 100644
--- a/daemons/dmeventd/libdevmapper-event.c
+++ b/daemons/dmeventd/libdevmapper-event.c
@@ -538,34 +538,37 @@ static struct dm_task *_get_device_info(const struct dm_event_handler *dmevh)
 		return NULL;
 	}
 
-	if (dmevh->uuid)
-		dm_task_set_uuid(dmt, dmevh->uuid);
-	else if (dmevh->dev_name)
-		dm_task_set_name(dmt, dmevh->dev_name);
-	else if (dmevh->major && dmevh->minor) {
-		dm_task_set_major(dmt, dmevh->major);
-		dm_task_set_minor(dmt, dmevh->minor);
-        }
+	if (dmevh->uuid) {
+		if (!dm_task_set_uuid(dmt, dmevh->uuid))
+			goto_bad;
+	} else if (dmevh->dev_name) {
+		if (!dm_task_set_name(dmt, dmevh->dev_name))
+			goto_bad;
+	} else if (dmevh->major && dmevh->minor) {
+		if (!dm_task_set_major(dmt, dmevh->major) ||
+		    !dm_task_set_minor(dmt, dmevh->minor))
+			goto_bad;
+	}
 
 	/* FIXME Add name or uuid or devno to messages */
 	if (!dm_task_run(dmt)) {
 		log_error("_get_device_info: dm_task_run() failed");
-		goto failed;
+		goto bad;
 	}
 
 	if (!dm_task_get_info(dmt, &info)) {
 		log_error("_get_device_info: failed to get info for device");
-		goto failed;
+		goto bad;
 	}
 
 	if (!info.exists) {
 		log_error("_get_device_info: device not found");
-		goto failed;
+		goto bad;
 	}
 
 	return dmt;
 
-failed:
+      bad:
 	dm_task_destroy(dmt);
 	return NULL;
 }
-- 
1.7.3.4

[PATCH 13/23] Make it obvious this code is not used

[Zdenek Kabelac]

Static analyzis and lcov likes to know this code is unused.

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

diff --git a/lib/activate/dev_manager.c b/lib/activate/dev_manager.c
index 2c2fc19..6d7d905 100644
--- a/lib/activate/dev_manager.c
+++ b/lib/activate/dev_manager.c
@@ -309,6 +309,7 @@ static const struct dm_info *_cached_info(struct dm_pool *mem,
 	return dinfo;
 }
 
+#if 0
 /* FIXME Interface must cope with multiple targets */
 static int _status_run(const char *name, const char *uuid,
 		       unsigned long long *s, unsigned long long *l,
@@ -386,6 +387,7 @@ static int _status(const char *name, const char *uuid,
 
 	return 0;
 }
+#endif
 
 int lv_has_target_type(struct dm_pool *mem, struct logical_volume *lv,
 		       const char *layer, const char *target_type)
-- 
1.7.3.4

[PATCH 14/23] Add stack trace for backup and backup_remove

[Zdenek Kabelac]

Add missing 'stack' traces for errors.

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

diff --git a/lib/metadata/metadata.c b/lib/metadata/metadata.c
index 5f75a66..27abaea 100644
--- a/lib/metadata/metadata.c
+++ b/lib/metadata/metadata.c
@@ -611,7 +611,8 @@ int vg_remove(struct volume_group *vg)
 		}
 	}
 
-	backup_remove(vg->cmd, vg->name);
+	if (!backup_remove(vg->cmd, vg->name))
+		stack;
 
 	if (ret)
 		log_print("Volume group \"%s\" successfully removed", vg->name);
diff --git a/tools/vgrename.c b/tools/vgrename.c
index 98be9a9..6739ebb 100644
--- a/tools/vgrename.c
+++ b/tools/vgrename.c
@@ -164,8 +164,10 @@ static int vg_rename_path(struct cmd_context *cmd, const char *old_vg_path,
 		}
 	}
 
-	backup(vg);
-	backup_remove(cmd, vg_name_old);
+	if (!backup(vg))
+                stack;
+	if (!backup_remove(cmd, vg_name_old))
+                stack;
 
 	unlock_vg(cmd, vg_name_new);
 	unlock_and_free_vg(cmd, vg, vg_name_old);
-- 
1.7.3.4

[PATCH 15/23] Add test check for find_config_str

[Zdenek Kabelac]

It looks like there should be some error code returned from _read_desc
when something fails.

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

diff --git a/lib/format_text/import_vsn1.c b/lib/format_text/import_vsn1.c
index e5e83d4..5a2d664 100644
--- a/lib/format_text/import_vsn1.c
+++ b/lib/format_text/import_vsn1.c
@@ -842,9 +842,9 @@ static void _read_desc(struct dm_pool *mem,
 	old_suppress = log_suppress(1);
 	d = find_config_str(cft->root, "description", "");
 	log_suppress(old_suppress);
-	*desc = dm_pool_strdup(mem, d);
+	*desc = (d) ? dm_pool_strdup(mem, d) : NULL;
 
-	get_config_uint32(cft->root, "creation_time", &u);
+	(void) get_config_uint32(cft->root, "creation_time", &u);
 	*when = u;
 }
 
-- 
1.7.3.4

[PATCH 15/23] Add test check for find_config_str

[Milan Broz]

On 12/21/2010 04:41 PM, Zdenek Kabelac wrote:

> -	*desc = dm_pool_strdup(mem, d);
> +	*desc = (d) ? dm_pool_strdup(mem, d) : NULL;

dm_pool_strdup(NULL) tries to alloc 1 byte and returning pointer, 
and not setting to \0 and calling strlen(NULL).

dunno what was the intention here.

> -	get_config_uint32(cft->root, "creation_time", &u);
> +	(void) get_config_uint32(cft->root, "creation_time", &u);

What's wrong here? I thought that this is needed only
if is defined with warn_if_unused_result?

Milan

[PATCH 15/23] Add test check for find_config_str

[Zdenek Kabelac]

Dne 21.12.2010 18:32, Milan Broz napsal(a):
> On 12/21/2010 04:41 PM, Zdenek Kabelac wrote:
> 
>> -	*desc = dm_pool_strdup(mem, d);
>> +	*desc = (d) ? dm_pool_strdup(mem, d) : NULL;
> 
> dm_pool_strdup(NULL) tries to alloc 1 byte and returning pointer, 
> and not setting to \0 and calling strlen(NULL).

hmm libdm/mm/pool.c - line 30
It looks like fault case.


> 
> dunno what was the intention here.
> 
>> -	get_config_uint32(cft->root, "creation_time", &u);
>> +	(void) get_config_uint32(cft->root, "creation_time", &u);
> 
> What's wrong here? I thought that this is needed only
> if is defined with warn_if_unused_result?
> 

When we ignore return value - we should do that explicitly
when we test all other cases - it's being reported as suspicious.

IMHO more important question here is - if it's correct to have
this function returning void.

Zdenek

[PATCH 15/23] Add test check for find_config_str

[Alasdair G Kergon]

On Wed, Dec 22, 2010 at 11:00:41AM +0100, Zdenek Kabelac wrote:
> >> -	get_config_uint32(cft->root, "creation_time", &u);
> >> +	(void) get_config_uint32(cft->root, "creation_time", &u);
> > 
> > What's wrong here? I thought that this is needed only
> > if is defined with warn_if_unused_result?
> 
> When we ignore return value - we should do that explicitly
> when we test all other cases - it's being reported as suspicious.
> 
> IMHO more important question here is - if it's correct to have
> this function returning void.
 
Exactly - the real problem is: what if the field is not present?
Well the code seems to permit that:
It uses a description of "" if there's no description, and a time
of 0 if there's no creation_time.

So I think (void) was indeed the intention.

Alasdair

[PATCH 15/23] Add test check for find_config_str

[Alasdair G Kergon]

On Tue, Dec 21, 2010 at 04:41:47PM +0100, Zdenek Kabelac wrote:
> It looks like there should be some error code returned from _read_desc
> when something fails.
 
No need - the code is just using default values of "" and 0.

> -	*desc = dm_pool_strdup(mem, d);
> +	*desc = (d) ? dm_pool_strdup(mem, d) : NULL;
  
Superfluous - this follows

        d = find_config_str(cft->root, "description", "");

which returns "" on failure so d is never NULL.

Alasdair

[PATCH 16/23] Remove check for vg != NULL

[Zdenek Kabelac]

Checking for vg being != NULL in this place is not needed.
We are derefercing vg already in this function above this code line.
Also the function _read_pv is always called with 'vg' assigned some
valid pointer.

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

diff --git a/lib/format_text/import_vsn1.c b/lib/format_text/import_vsn1.c
index 5a2d664..083c8ee 100644
--- a/lib/format_text/import_vsn1.c
+++ b/lib/format_text/import_vsn1.c
@@ -257,14 +257,12 @@ static int _read_pv(struct format_instance *fid, struct dm_pool *mem,
 		log_verbose("Fixing up missing size (%s) "
 			    "for PV %s", display_size(fid->fmt->cmd, pv->size),
 			    pv_dev_name(pv));
-		if (vg) {
-			size = pv->pe_count * (uint64_t) vg->extent_size +
-			       pv->pe_start;
-			if (size > pv->size)
-				log_warn("WARNING: Physical Volume %s is too "
-					 "large for underlying device",
-					 pv_dev_name(pv));
-		}
+		size = pv->pe_count * (uint64_t) vg->extent_size +
+			pv->pe_start;
+		if (size > pv->size)
+			log_warn("WARNING: Physical Volume %s is too "
+				 "large for underlying device",
+				 pv_dev_name(pv));
 	}
 
 	if (!alloc_pv_segment_whole_pv(mem, pv))
-- 
1.7.3.4

[PATCH 17/23] Add check for dm_snprintf result

[Zdenek Kabelac]

Check whether dm_snprintf() was successfull.
For cluster lock use one dm_snprintf() instead of two calls with
just one different letter. (For file lock we print different
resouce string for P_ - skiping '#' sign - is that needed ?

Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>
---
 lib/locking/cluster_locking.c |   14 ++++++++------
 lib/locking/file_locking.c    |   16 ++++++++++++----
 2 files changed, 20 insertions(+), 10 deletions(-)

diff --git a/lib/locking/cluster_locking.c b/lib/locking/cluster_locking.c
index eea9974..8b7fe99 100644
--- a/lib/locking/cluster_locking.c
+++ b/lib/locking/cluster_locking.c
@@ -409,12 +409,14 @@ int lock_resource(struct cmd_context *cmd, const char *resource, uint32_t flags)
 		}
 
 		/* If the VG name is empty then lock the unused PVs */
-		if (is_orphan_vg(resource) || is_global_vg(resource) || (flags & LCK_CACHE))
-			dm_snprintf(lockname, sizeof(lockname), "P_%s",
-				    resource);
-		else
-			dm_snprintf(lockname, sizeof(lockname), "V_%s",
-				    resource);
+		if (dm_snprintf(lockname, sizeof(lockname), "%c_%s",
+				(is_orphan_vg(resource) ||
+				 is_global_vg(resource) ||
+				 (flags & LCK_CACHE)) ?  'P' : 'V',
+				resource)  < 0) {
+			log_error("Locking resource %s too long.", resource);
+			return 0;
+		}
 
 		lock_scope = "VG";
 		clvmd_cmd = CLVMD_CMD_LOCK_VG;
diff --git a/lib/locking/file_locking.c b/lib/locking/file_locking.c
index 9137a30..bfee314 100644
--- a/lib/locking/file_locking.c
+++ b/lib/locking/file_locking.c
@@ -269,11 +269,19 @@ static int _file_lock_resource(struct cmd_context *cmd, const char *resource,
 			break;
 
 		if (is_orphan_vg(resource) || is_global_vg(resource))
-			dm_snprintf(lockfile, sizeof(lockfile),
-				     "%s/P_%s", _lock_dir, resource + 1);
+			if (dm_snprintf(lockfile, sizeof(lockfile),
+					"%s/P_%s", _lock_dir, resource + 1) < 0) {
+				log_error("Too long locking filename %s/P_%s.",
+					  _lock_dir, resource + 1);
+				return 0;
+			}
 		else
-			dm_snprintf(lockfile, sizeof(lockfile),
-				     "%s/V_%s", _lock_dir, resource);
+			if (dm_snprintf(lockfile, sizeof(lockfile),
+					"%s/V_%s", _lock_dir, resource) < 0) {
+				log_error("Too long locking filename %s/V_%s.",
+					  _lock_dir, resource);
+				return 0;
+			}
 
 		if (!_lock_file(lockfile, flags))
 			return_0;
-- 
1.7.3.4

[PATCH 17/23] Add check for dm_snprintf result

[Milan Broz]

>  		/* If the VG name is empty then lock the unused PVs */
> -		if (is_orphan_vg(resource) || is_global_vg(resource) || (flags & LCK_CACHE))
> -			dm_snprintf(lockname, sizeof(lockname), "P_%s",
> -				    resource);
> -		else
> -			dm_snprintf(lockname, sizeof(lockname), "V_%s",
> -				    resource);
> +		if (dm_snprintf(lockname, sizeof(lockname), "%c_%s",
> +				(is_orphan_vg(resource) ||
> +				 is_global_vg(resource) ||
> +				 (flags & LCK_CACHE)) ?  'P' : 'V',
> +				resource)  < 0) {
> +			log_error("Locking resource %s too long.", resource);
> +			return 0;
> +		}

I cannot imagine how this can happen. First parm is fixed char, second is resource string
with exact length. And char lockname[PATH_MAX];

Should we check for glibc and clompiler bugs also? :)

Well, isn't return_0 here enough?

Milan

[PATCH 17/23] Add check for dm_snprintf result

[Zdenek Kabelac]

Dne 21.12.2010 18:38, Milan Broz napsal(a):
>>  		/* If the VG name is empty then lock the unused PVs */
>> -		if (is_orphan_vg(resource) || is_global_vg(resource) || (flags & LCK_CACHE))
>> -			dm_snprintf(lockname, sizeof(lockname), "P_%s",
>> -				    resource);
>> -		else
>> -			dm_snprintf(lockname, sizeof(lockname), "V_%s",
>> -				    resource);
>> +		if (dm_snprintf(lockname, sizeof(lockname), "%c_%s",
>> +				(is_orphan_vg(resource) ||
>> +				 is_global_vg(resource) ||
>> +				 (flags & LCK_CACHE)) ?  'P' : 'V',
>> +				resource)  < 0) {
>> +			log_error("Locking resource %s too long.", resource);
>> +			return 0;
>> +		}
> 
> I cannot imagine how this can happen. First parm is fixed char, second is resource string
> with exact length. And char lockname[PATH_MAX];

One shiny afternoon day someone could think about using 256byte long VG and LV
uuids and it will nicely fail ;)

So take this just like an assert.

> 
> Should we check for glibc and clompiler bugs also? :)
> 
> Well, isn't return_0 here enough?
> 

First trace is supposed to be always some 'log_error'
If you print only 'stack' without log_error - internal error is generated.
(Thought we have it wrong on many places in the code...)

Zdenek

[PATCH 17/23] Add check for dm_snprintf result

[Alasdair G Kergon]

On Wed, Dec 22, 2010 at 11:34:47AM +0100, Zdenek Kabelac wrote:
> >> +		if (dm_snprintf(lockname, sizeof(lockname), "%c_%s",
> >> +				(is_orphan_vg(resource) ||
> >> +				 is_global_vg(resource) ||
> >> +				 (flags & LCK_CACHE)) ?  'P' : 'V',
> >> +				resource)  < 0) {
> >> +			log_error("Locking resource %s too long.", resource);
> >> +			return 0;
> >> +		}
> > I cannot imagine how this can happen. First parm is fixed char, second is resource string
> > with exact length. And char lockname[PATH_MAX];
> One shiny afternoon day someone could think about using 256byte long VG and LV
> uuids and it will nicely fail ;)
 
I've no objection to adding tests like that, though adding '(void)' in places like
this that are safe is also fine.

Alasdair

[PATCH 18/23] Return PERCENT_INVALID for error case

[Zdenek Kabelac]

If the percent value could not be determined - return PERCENT_INVALD.
Indent function with tabs.

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

diff --git a/lib/report/properties.c b/lib/report/properties.c
index 0b4929a..06e11ba 100644
--- a/lib/report/properties.c
+++ b/lib/report/properties.c
@@ -87,15 +87,18 @@ static int _not_implemented_set(void *obj, struct lvm_property_type *prop)
 }
 
 static percent_t _copy_percent(const struct logical_volume *lv) {
-    percent_t perc;
-    lv_mirror_percent(lv->vg->cmd, (struct logical_volume *) lv, 0, &perc, NULL);
-    return perc;
+	percent_t perc;
+	if (!lv_mirror_percent(lv->vg->cmd, (struct logical_volume *) lv,
+			   0, &perc, NULL))
+		perc = PERCENT_INVALID;
+	return perc;
 }
 
 static percent_t _snap_percent(const struct logical_volume *lv) {
-    percent_t perc;
-    lv_snapshot_percent(lv, &perc);
-    return perc;
+	percent_t perc;
+	if (!lv_snapshot_percent(lv, &perc))
+		perc = PERCENT_INVALID;
+	return perc;
 }
 
 /* PV */
-- 
1.7.3.4

[PATCH 18/23] Return PERCENT_INVALID for error case

[Milan Broz]

On 12/21/2010 04:41 PM, Zdenek Kabelac wrote:
> +	if (!lv_mirror_percent(lv->vg->cmd, (struct logical_volume *) lv,
> +			   0, &perc, NULL))
> +		perc = PERCENT_INVALID;
> +	return perc;

dunno, these functions returns 0 if (!activation()) and if lv is not activated
 is the PERCENT_INVALID (-1) correct value here?

But it is apparent bug, it returns undefined value in API now.

Milan

[PATCH 18/23] Return PERCENT_INVALID for error case

[Alasdair G Kergon]

On Tue, Dec 21, 2010 at 06:47:30PM +0100, Milan Broz wrote:
> On 12/21/2010 04:41 PM, Zdenek Kabelac wrote:
> > +	if (!lv_mirror_percent(lv->vg->cmd, (struct logical_volume *) lv,
> > +			   0, &perc, NULL))
> > +		perc = PERCENT_INVALID;
> > +	return perc;
> 
> dunno, these functions returns 0 if (!activation()) and if lv is not activated
>  is the PERCENT_INVALID (-1) correct value here?

Seems a reasonable use of this.

Ack (current code is definitely wrong) pending a review of whether all
the code is making best use of the values of the new percent type.
 
Alasdair

[PATCH 19/23] Warning - dead code problem elimination

[Zdenek Kabelac]

Original code assign  ret = DM_WAIT_INTR and immediately
reassing this value with result code from dm_task_get_info.

It's not quite clear how such result code is supposed to
fit DM_WAIT defines used in this function (maybe wrong merge?)

Maybe it's actually wrong to assing ret with dm_task_get_info.
So reather then removing line:  ret = DM_WAIT_INTR
patch removes later assignment.

CHECKME!!!

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

diff --git a/daemons/dmeventd/dmeventd.c b/daemons/dmeventd/dmeventd.c
index 2b454f9..e5adc4d 100644
--- a/daemons/dmeventd/dmeventd.c
+++ b/daemons/dmeventd/dmeventd.c
@@ -653,8 +653,7 @@ static int _event_wait(struct thread_status *thread, struct dm_task **task)
 	if (dm_task_run(dmt)) {
 		thread->current_events |= DM_EVENT_DEVICE_ERROR;
 		ret = DM_WAIT_INTR;
-
-		if ((ret = dm_task_get_info(dmt, &info)))
+		if (dm_task_get_info(dmt, &info))
 			thread->event_nr = info.event_nr;
 	} else if (thread->events & DM_EVENT_TIMEOUT && errno == EINTR) {
 		thread->current_events |= DM_EVENT_TIMEOUT;
-- 
1.7.3.4

[PATCH 19/23] Warning - dead code problem elimination

[Milan Broz]

On 12/21/2010 04:41 PM, Zdenek Kabelac wrote:
> @@ -653,8 +653,7 @@ static int _event_wait(struct thread_status *thread, struct dm_task **task)
>  	if (dm_task_run(dmt)) {
>  		thread->current_events |= DM_EVENT_DEVICE_ERROR;
>  		ret = DM_WAIT_INTR;
> -
> -		if ((ret = dm_task_get_info(dmt, &info)))
> +		if (dm_task_get_info(dmt, &info))
>  			thread->event_nr = info.event_nr;
>  	} else if (thread->events & DM_EVENT_TIMEOUT && errno == EINTR) {
>  		thread->current_events |= DM_EVENT_TIMEOUT;

#define DM_WAIT_RETRY 0
#define DM_WAIT_INTR 1

dm_task_get_info returns 0/1 - it is probably intended such way?

I think you should return DM_WAIT_RETRY if info fails then (so the code _was_ correct)?

mornfall? :)

Milan

[PATCH 19/23] Warning - dead code problem elimination

[Petr Rockai]

Milan Broz <mbroz@redhat.com> writes:

> On 12/21/2010 04:41 PM, Zdenek Kabelac wrote:
>> @@ -653,8 +653,7 @@ static int _event_wait(struct thread_status *thread, struct dm_task **task)
>>  	if (dm_task_run(dmt)) {
>>  		thread->current_events |= DM_EVENT_DEVICE_ERROR;
>>  		ret = DM_WAIT_INTR;
>> -
>> -		if ((ret = dm_task_get_info(dmt, &info)))
>> +		if (dm_task_get_info(dmt, &info))
>>  			thread->event_nr = info.event_nr;
>>  	} else if (thread->events & DM_EVENT_TIMEOUT && errno == EINTR) {
>>  		thread->current_events |= DM_EVENT_TIMEOUT;
>
> #define DM_WAIT_RETRY 0
> #define DM_WAIT_INTR 1
>
> dm_task_get_info returns 0/1 - it is probably intended such way?
>
> I think you should return DM_WAIT_RETRY if info fails then (so the code _was_ correct)?
>
> mornfall? :)

Jeez, I wish I knew! I don't think I wrote this code. Anyway, it would
make sense to add ret = DM_WAIT_RETRY in the else branch (i.e. get_info
fails). Not that this is likely to ever trigger...

Petr

[PATCH 19/23] Warning - dead code problem elimination

[Alasdair G Kergon]

> > I think you should return DM_WAIT_RETRY if info fails then (so the code _was_ correct)?

> Jeez, I wish I knew! I don't think I wrote this code. Anyway, it would
> make sense to add ret = DM_WAIT_RETRY in the else branch (i.e. get_info
> fails). Not that this is likely to ever trigger...

Do that then.

Alasdair

[PATCH 20/23] exit if lvmchache_init fails

[Zdenek Kabelac]

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

diff --git a/tools/polldaemon.c b/tools/polldaemon.c
index 50579ba..bcc649f 100644
--- a/tools/polldaemon.c
+++ b/tools/polldaemon.c
@@ -64,7 +64,8 @@ static int _become_daemon(struct cmd_context *cmd)
 	strncpy(*cmd->argv, "(lvm2)", strlen(*cmd->argv));
 
 	reset_locking();
-	lvmcache_init();
+	if (!lvmcache_init())
+		_exit(ECMD_FAILED);
 	dev_close_all();
 
 	return 1;
-- 
1.7.3.4

[PATCH 20/23] exit if lvmchache_init fails

[Alasdair G Kergon]

On Tue, Dec 21, 2010 at 04:41:52PM +0100, Zdenek Kabelac wrote:
>  	reset_locking();
> -	lvmcache_init();
> +	if (!lvmcache_init())
> +		_exit(ECMD_FAILED);
>  	dev_close_all();
  
Ack.

Not ideal, but we actually already have some existing tricky cleanup problems
here that I never fixed, so just add a FIXME "Clean up properly here!" to that
_exit().

Alasdair

[PATCH 21/23] Add default error path for get_property

[Zdenek Kabelac]

Set invalid property value for error path when none of handlers is set.
Fixes use of uninitialized prop variable.

Maybe add log_error() for this programming error?

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

diff --git a/liblvm/lvm_misc.c b/liblvm/lvm_misc.c
index 62fef61..e2937c9 100644
--- a/liblvm/lvm_misc.c
+++ b/liblvm/lvm_misc.c
@@ -78,7 +78,11 @@ struct lvm_property_value get_property(const pv_t pv, const vg_t vg,
 			v.is_valid = 0;
 			return v;
 		}
+	} else {
+		v.is_valid = 0;
+		return v;
 	}
+
 	v.is_settable = prop.is_settable;
 	v.is_string = prop.is_string;
 	v.is_integer = prop.is_integer;
-- 
1.7.3.4

[PATCH 21/23] Add default error path for get_property

[Alasdair G Kergon]

On Tue, Dec 21, 2010 at 04:41:53PM +0100, Zdenek Kabelac wrote:
> Set invalid property value for error path when none of handlers is set.
> Fixes use of uninitialized prop variable.
 
Ack.

> Maybe add log_error() for this programming error?
 
You could probably add a log_errno() for errors with use of the library
interface, yes.

Alasdair

[PATCH 22/23] Remove dead assignment in dm_event_get_registered_device

[Zdenek Kabelac]

ret assigned from _do_event was actually not used and replaced with next
assignment without any read of returned value.

Code is reformated - so error path is put in the if branch and normal
code is put after the 'if' together with FIXME comment.

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

diff --git a/daemons/dmeventd/libdevmapper-event.c b/daemons/dmeventd/libdevmapper-event.c
index 05ebf0a..4859189 100644
--- a/daemons/dmeventd/libdevmapper-event.c
+++ b/daemons/dmeventd/libdevmapper-event.c
@@ -718,17 +718,17 @@ int dm_event_get_registered_device(struct dm_event_handler *dmevh, int next)
 
 	uuid = dm_task_get_uuid(dmt);
 
-	if (!(ret = _do_event(next ? DM_EVENT_CMD_GET_NEXT_REGISTERED_DEVICE :
-			     DM_EVENT_CMD_GET_REGISTERED_DEVICE, dmevh->dmeventd_path,
-			      &msg, dmevh->dso, uuid, dmevh->mask, 0))) {
-		/* FIXME this will probably horribly break if we get
-		   ill-formatted reply */
-		ret = _parse_message(&msg, &reply_dso, &reply_uuid, &reply_mask);
-	} else {
+	if (_do_event(next ? DM_EVENT_CMD_GET_NEXT_REGISTERED_DEVICE :
+		      DM_EVENT_CMD_GET_REGISTERED_DEVICE, dmevh->dmeventd_path,
+		      &msg, dmevh->dso, uuid, dmevh->mask, 0)) {
 		ret = -ENOENT;
 		goto fail;
 	}
 
+	/* FIXME this will probably horribly break if we get
+	   ill-formatted reply */
+	ret = _parse_message(&msg, &reply_dso, &reply_uuid, &reply_mask);
+
 	dm_task_destroy(dmt);
 	dmt = NULL;
 
-- 
1.7.3.4

[PATCH 22/23] Remove dead assignment in dm_event_get_registered_device

[Alasdair G Kergon]

On Tue, Dec 21, 2010 at 04:41:54PM +0100, Zdenek Kabelac wrote:
> ret assigned from _do_event was actually not used and replaced with next
> assignment without any read of returned value.
 
But does the actual error already get logged somewhere?
(I don't see where.)
Some _do_event() failures are reported, others aren't.  I don't see
why there's a distinction.

A DM_EVENT_CMD_REGISTER_FOR_EVENT failure is logged.
A DM_EVENT_CMD_GET_TIMEOUT failure is not.

Surely these failures should never occur on a properly-running system, so why
not log them all in a similar way?

Alasdair

[PATCH 23/23] Remove dead assignment in vg_split_mdas

[Zdenek Kabelac]

'common_mda' is assigned again with next code line.

Why is the result not important from the first call?
CHECKME!

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

diff --git a/lib/metadata/metadata.c b/lib/metadata/metadata.c
index 27abaea..ebd19fa 100644
--- a/lib/metadata/metadata.c
+++ b/lib/metadata/metadata.c
@@ -1263,15 +1263,14 @@ int vg_split_mdas(struct cmd_context *cmd __attribute__((unused)),
 {
 	struct dm_list *mdas_from_in_use, *mdas_to_in_use;
 	struct dm_list *mdas_from_ignored, *mdas_to_ignored;
-	int common_mda = 0;
+	int common_mda;
 
 	mdas_from_in_use = &vg_from->fid->metadata_areas_in_use;
 	mdas_from_ignored = &vg_from->fid->metadata_areas_ignored;
 	mdas_to_in_use = &vg_to->fid->metadata_areas_in_use;
 	mdas_to_ignored = &vg_to->fid->metadata_areas_ignored;
 
-	common_mda = _move_mdas(vg_from, vg_to,
-				mdas_from_in_use, mdas_to_in_use);
+	_move_mdas(vg_from, vg_to, mdas_from_in_use, mdas_to_in_use);
 	common_mda = _move_mdas(vg_from, vg_to,
 				mdas_from_ignored, mdas_to_ignored);
 
-- 
1.7.3.4

[PATCH 23/23] Remove dead assignment in vg_split_mdas

[Alasdair G Kergon]

On Tue, Dec 21, 2010 at 04:41:55PM +0100, Zdenek Kabelac wrote:
> 'common_mda' is assigned again with next code line.
 
>  	mdas_from_in_use = &vg_from->fid->metadata_areas_in_use;
>  	mdas_from_ignored = &vg_from->fid->metadata_areas_ignored;
>  	mdas_to_in_use = &vg_to->fid->metadata_areas_in_use;
>  	mdas_to_ignored = &vg_to->fid->metadata_areas_ignored;
>  
> -	common_mda = _move_mdas(vg_from, vg_to,
> -				mdas_from_in_use, mdas_to_in_use);
> +	_move_mdas(vg_from, vg_to, mdas_from_in_use, mdas_to_in_use);
>  	common_mda = _move_mdas(vg_from, vg_to,
>  				mdas_from_ignored, mdas_to_ignored);

Not immediately obvious how this code is correct.

The function returns 0 (giving user error message and aborting the split
operation) if common_mda is 0 i.e. if after the split there'll be no mdas
in the 'from' VG and there was no 'common' mda on the 'mdas_from_ignored'
list.

I'd guess the two lines should be in the opposite order, ignoring
the result from the 'ignored' move, but I don't understand why
the 'common_mdas' are not also taken into account (ignored) in
_vg_adjust_ignored_mdas.

_move_mdas() uses '!mda->ops->mda_in_vg' to detect a common_mda -
why are the functions that manipulate which mdas are ignored
not also doing this?  (This seems to be being used as a flag
to indicate 'This mda must always be used and cannot be ignored'.)

Even if the code is correct, it needs to be made clearer what
it is doing.

Alasdair

[PATCH 00/23] Static analyzis set

[Milan Broz]

On 12/21/2010 04:41 PM, Zdenek Kabelac wrote:
> This is mostly the last bigger patch sets for covering
> issues reported by Coverity tool.

If it help to stop spamming list, my ACK for:
2,3,6,8,9,11,12,13,14,16,20,21,22,23

(No, these are not numbers from National Lottery and
you did not won $1,000,000:-)

others I do not like but but do not care (no comment)
or ... see comment:)

Milan