[PATCH 00/12] multipath-tools: Coverity patches

[PATCH 00/12] multipath-tools: Coverity patches

[Martin Wilck]

Hi Christophe,

here are a few simple fixes motivated by a recent
coverity scan of the latest upstream. Most are false positives,
but it can't hurt to fix them anyway, be it only to get a quicker
overview on future scans.

Unfortunately synopsys messed up the scan.coverity.com website
before I could finish this work.

There are two bigger issues with coverity that this set doesn't
address:

1) apparently coverity doesn't understand our frequently
    used paradigm (simplified):

    pthread_cleanup_push(pthread_mutex_unlock, &mutex);
    pthread_mutex_lock(&mutex);
    do_some_work();
    pthread_cleanup_pop(1);

 2) coverity warns that we sleep in multiple places holding the
    vecs lock. That will be a bit harder to fix.

Martin

Martin Wilck (12):
  kpartx(coverity): fix resource leak warning
  libmultipath(coverity): cleanup dup usage in execute_program()
  kpartx(coverity): fix apparent out-of-bounds access
  libmultipath(coverity): make sure readlink result is 0-terminated
  libmultipath(coverity): fix apparent overflow
  libmpathcmd(coverity): limit reply length
  libmultipath(coverity): fix int overflow in sysfs_set_scsi_tmo
  libmultipath(coverity): fix "enum misuse" for find_multipaths
  multipathd(coverity): check retval clock_gettime()
  libmpathpersist(coverity): range checking for PRIN length
  libmultipath/foreign(coverity): retval check in snprint_nvme_path
  libmultipath(coverity): fix possible NULL dereference

 kpartx/dasd.c                    |  7 +++----
 libmpathcmd/mpath_cmd.c          |  4 ++++
 libmpathcmd/mpath_cmd.h          |  6 ++++++
 libmpathpersist/mpath_pr_ioctl.c | 10 +++++++---
 libmultipath/callout.c           | 13 +++++++------
 libmultipath/configure.c         |  2 +-
 libmultipath/dict.c              |  2 +-
 libmultipath/discovery.c         |  4 ++--
 libmultipath/foreign/nvme.c      |  6 ++++--
 libmultipath/util.c              |  1 +
 multipathd/cli.c                 |  2 ++
 multipathd/cli.h                 |  6 ++++++
 multipathd/cli_handlers.c        |  1 +
 multipathd/main.c                |  9 +++++----
 14 files changed, 50 insertions(+), 23 deletions(-)

-- 
2.19.2

[PATCH 01/12] kpartx(coverity): fix resource leak warning

[Martin Wilck]

This was an easy-to-fix false positive.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 kpartx/dasd.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/kpartx/dasd.c b/kpartx/dasd.c
index 94ae81b..fb358ad 100644
--- a/kpartx/dasd.c
+++ b/kpartx/dasd.c
@@ -137,7 +137,7 @@ read_dasd_pt(int fd, struct slice all, struct slice *sp, int ns)
 			/* Not a DASD */
 			return -1;
 	} else {
-		fd_dasd = fd;
+		fd_dasd = dup(fd);
 	}
 
 	if (ioctl(fd_dasd, BIODASDINFO, (unsigned long)&info) != 0) {
@@ -288,7 +288,6 @@ read_dasd_pt(int fd, struct slice all, struct slice *sp, int ns)
 out:
 	if (data != NULL)
 		free(data);
-	if (fd_dasd != -1 && fd_dasd != fd)
-		close(fd_dasd);
+	close(fd_dasd);
 	return retval;
 }
-- 
2.19.2

[PATCH 02/12] libmultipath(coverity): cleanup dup usage in execute_program()

[Martin Wilck]

coverity complained about resource leakage here.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/callout.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/libmultipath/callout.c b/libmultipath/callout.c
index d5ca27b..dac088c 100644
--- a/libmultipath/callout.c
+++ b/libmultipath/callout.c
@@ -68,19 +68,20 @@ int execute_program(char *path, char *value, int len)
 	switch(pid) {
 	case 0:
 		/* child */
-		close(STDOUT_FILENO);
 
 		/* dup write side of pipe to STDOUT */
-		if (dup(fds[1]) < 0)
+		if (dup2(fds[1], STDOUT_FILENO) < 0) {
+			condlog(1, "failed to dup2 stdout: %m");
 			return -1;
+		}
+		close(fds[0]);
+		close(fds[1]);
 
 		/* Ignore writes to stderr */
 		null_fd = open("/dev/null", O_WRONLY);
 		if (null_fd > 0) {
-			int err_fd __attribute__ ((unused));
-
-			close(STDERR_FILENO);
-			err_fd = dup(null_fd);
+			if (dup2(null_fd, STDERR_FILENO) < 0)
+				condlog(1, "failed to dup2 stderr: %m");
 			close(null_fd);
 		}
 
-- 
2.19.2

[PATCH 03/12] kpartx(coverity): fix apparent out-of-bounds access

[Martin Wilck]

This was a false positive.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 kpartx/dasd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kpartx/dasd.c b/kpartx/dasd.c
index fb358ad..61b609a 100644
--- a/kpartx/dasd.c
+++ b/kpartx/dasd.c
@@ -190,7 +190,7 @@ read_dasd_pt(int fd, struct slice all, struct slice *sp, int ns)
 		memcpy (&vlabel, data, sizeof(vlabel));
 	else {
 		bzero(&vlabel,4);
-		memcpy (&vlabel.vollbl, data, sizeof(vlabel) - 4);
+		memcpy ((char *)&vlabel + 4, data, sizeof(vlabel) - 4);
 	}
 	vtoc_ebcdic_dec(vlabel.vollbl, type, 4);
 
-- 
2.19.2

[PATCH 04/12] libmultipath(coverity): make sure readlink result is 0-terminated

[Martin Wilck]

Coverity warned that readlink() results aren't necessarily 0-terminated.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/util.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/libmultipath/util.c b/libmultipath/util.c
index 944c632..5b838d5 100644
--- a/libmultipath/util.c
+++ b/libmultipath/util.c
@@ -176,6 +176,7 @@ int devt2devname(char *devname, int devname_len, char *devt)
 	if (stat("/sys/dev/block", &statbuf) == 0) {
 		/* Newer kernels have /sys/dev/block */
 		sprintf(block_path,"/sys/dev/block/%u:%u", major, minor);
+		dev[FILE_NAME_SIZE - 1] = '\0';
 		if (lstat(block_path, &statbuf) == 0) {
 			if (S_ISLNK(statbuf.st_mode) &&
 			    readlink(block_path, dev, FILE_NAME_SIZE-1) > 0) {
-- 
2.19.2

[PATCH 05/12] libmultipath(coverity): fix apparent overflow

[Martin Wilck]

"preferred_path" contains always "0" or "1".

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/discovery.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index 7f983a6..3fd79a3 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -520,7 +520,7 @@ sysfs_get_asymmetric_access_state(struct path *pp, char *buff, int buflen)
 		/* Parse error, ignore */
 		return 0;
 	}
-	return  preferred;
+	return !!preferred;
 }
 
 static void
-- 
2.19.2

[PATCH 06/12] libmpathcmd(coverity): limit reply length

[Martin Wilck]

coverity warned about tainted input data.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmpathcmd/mpath_cmd.c   | 4 ++++
 libmpathcmd/mpath_cmd.h   | 6 ++++++
 multipathd/cli.c          | 2 ++
 multipathd/cli.h          | 6 ++++++
 multipathd/cli_handlers.c | 1 +
 5 files changed, 19 insertions(+)

diff --git a/libmpathcmd/mpath_cmd.c b/libmpathcmd/mpath_cmd.c
index 61e6a98..df4ca54 100644
--- a/libmpathcmd/mpath_cmd.c
+++ b/libmpathcmd/mpath_cmd.c
@@ -133,6 +133,10 @@ ssize_t mpath_recv_reply_len(int fd, unsigned int timeout)
 		errno = EIO;
 		return -1;
 	}
+	if (len <= 0 || len >= MAX_REPLY_LEN) {
+		errno = ERANGE;
+		return -1;
+	}
 	return len;
 }
 
diff --git a/libmpathcmd/mpath_cmd.h b/libmpathcmd/mpath_cmd.h
index df9d938..15aeb06 100644
--- a/libmpathcmd/mpath_cmd.h
+++ b/libmpathcmd/mpath_cmd.h
@@ -20,6 +20,12 @@
 #ifndef LIB_MPATH_CMD_H
 #define LIB_MPATH_CMD_H
 
+/*
+ * This should be sufficient for json output for >10000 maps,
+ * and >60000 paths.
+ */
+#define MAX_REPLY_LEN (32 * 1024 * 1024)
+
 #ifdef __cplusplus
 extern "C" {
 #endif
diff --git a/multipathd/cli.c b/multipathd/cli.c
index a75afe3..ca176a9 100644
--- a/multipathd/cli.c
+++ b/multipathd/cli.c
@@ -13,7 +13,9 @@
 #include "version.h"
 #include <readline/readline.h>
 
+#include "mpath_cmd.h"
 #include "cli.h"
+#include "debug.h"
 
 static vector keys;
 static vector handlers;
diff --git a/multipathd/cli.h b/multipathd/cli.h
index 7cc7e4b..f3fa077 100644
--- a/multipathd/cli.h
+++ b/multipathd/cli.h
@@ -96,6 +96,12 @@ enum {
 	do {							\
 		if ((a)) {					\
 			char *tmp = (r);			\
+								\
+			if (m >= MAX_REPLY_LEN) {		\
+				condlog(1, "Warning: max reply length exceeded"); \
+				free(tmp);			\
+				r = NULL;			\
+			}					\
 			(r) = REALLOC((r), (m) * 2);		\
 			if ((r)) {				\
 				memset((r) + (m), 0, (m));	\
diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
index 6304ed3..f979a18 100644
--- a/multipathd/cli_handlers.c
+++ b/multipathd/cli_handlers.c
@@ -26,6 +26,7 @@
 #include "prkey.h"
 #include "propsel.h"
 #include "main.h"
+#include "mpath_cmd.h"
 #include "cli.h"
 #include "uevent.h"
 #include "foreign.h"
-- 
2.19.2

[PATCH 07/12] libmultipath(coverity): fix int overflow in sysfs_set_scsi_tmo

[Martin Wilck]

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/discovery.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index 3fd79a3..1748eeb 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -711,7 +711,7 @@ sysfs_set_scsi_tmo (struct multipath *mpp, int checkint)
 	int dev_loss_tmo = mpp->dev_loss;
 
 	if (mpp->no_path_retry > 0) {
-		uint64_t no_path_retry_tmo = mpp->no_path_retry * checkint;
+		uint64_t no_path_retry_tmo = (uint64_t)mpp->no_path_retry * checkint;
 
 		if (no_path_retry_tmo > MAX_DEV_LOSS_TMO)
 			no_path_retry_tmo = MAX_DEV_LOSS_TMO;
-- 
2.19.2

[PATCH 08/12] libmultipath(coverity): fix "enum misuse" for find_multipaths

[Martin Wilck]

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/dict.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libmultipath/dict.c b/libmultipath/dict.c
index fd29abc..eaad4f1 100644
--- a/libmultipath/dict.c
+++ b/libmultipath/dict.c
@@ -327,7 +327,7 @@ def_find_multipaths_handler(struct config *conf, vector strvec)
 	int i;
 
 	if (set_yes_no_undef(strvec, &conf->find_multipaths) == 0 &&
-	    conf->find_multipaths != YNU_UNDEF)
+	    conf->find_multipaths != FIND_MULTIPATHS_UNDEF)
 		return 0;
 
 	buff = set_value(strvec);
-- 
2.19.2

[PATCH 09/12] multipathd(coverity): check retval clock_gettime()

[Martin Wilck]

Checking this is pointless, as we'd bail out early in
pthread_cond_init_mono if CLOCK_MONOTONIC was unsupported, and this
is the only error condition of clock_gettime worth checking.
Do it anyway to make coverity feel better.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipathd/main.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index 01cf70f..0270e18 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -242,10 +242,11 @@ int set_config_state(enum daemon_status state)
 		else if (running_state != DAEMON_IDLE) {
 			struct timespec ts;
 
-			clock_gettime(CLOCK_MONOTONIC, &ts);
-			ts.tv_sec += 1;
-			rc = pthread_cond_timedwait(&config_cond,
-						    &config_lock, &ts);
+			if (clock_gettime(CLOCK_MONOTONIC, &ts) == 0) {
+				ts.tv_sec += 1;
+				rc = pthread_cond_timedwait(&config_cond,
+							    &config_lock, &ts);
+			}
 		}
 		if (!rc && (running_state != DAEMON_SHUTDOWN)) {
 			running_state = state;
-- 
2.19.2

[PATCH 10/12] libmpathpersist(coverity): range checking for PRIN length

[Martin Wilck]

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmpathpersist/mpath_pr_ioctl.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/libmpathpersist/mpath_pr_ioctl.c b/libmpathpersist/mpath_pr_ioctl.c
index c4f4ccd..cf528fe 100644
--- a/libmpathpersist/mpath_pr_ioctl.c
+++ b/libmpathpersist/mpath_pr_ioctl.c
@@ -211,7 +211,8 @@ void mpath_format_readfullstatus(struct prin_resp *pr_buff, int len, int noisy)
 	unsigned char *p;
 	char  *ppbuff;
 	uint32_t additional_length;
-
+	char tempbuff[MPATH_MAX_PARAM_LEN];
+	struct prin_fulldescr fdesc;
 
 	convert_be32_to_cpu(&pr_buff->prin_descriptor.prin_readfd.prgeneration);
 	convert_be32_to_cpu(&pr_buff->prin_descriptor.prin_readfd.number_of_descriptor);
@@ -223,9 +224,12 @@ void mpath_format_readfullstatus(struct prin_resp *pr_buff, int len, int noisy)
 	}
 
 	additional_length = pr_buff->prin_descriptor.prin_readfd.number_of_descriptor;
+	if (additional_length > MPATH_MAX_PARAM_LEN) {
+		condlog(3, "PRIN length %u exceeds max length %d", additional_length,
+			MPATH_MAX_PARAM_LEN);
+		return;
+	}
 
-	char tempbuff[MPATH_MAX_PARAM_LEN];
-	struct prin_fulldescr fdesc;
 	memset(&fdesc, 0, sizeof(struct prin_fulldescr));
 
 	memcpy( tempbuff, pr_buff->prin_descriptor.prin_readfd.private_buffer,MPATH_MAX_PARAM_LEN );
-- 
2.19.2

[PATCH 11/12] libmultipath/foreign(coverity): retval check in snprint_nvme_path

[Martin Wilck]

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/foreign/nvme.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/libmultipath/foreign/nvme.c b/libmultipath/foreign/nvme.c
index f0e8293..7e654ec 100644
--- a/libmultipath/foreign/nvme.c
+++ b/libmultipath/foreign/nvme.c
@@ -249,8 +249,10 @@ static int snprint_nvme_path(const struct gen_path *gp,
 		devt = udev_device_get_devnum(np->udev);
 		return snprintf(buff, len, "%u:%u", major(devt), minor(devt));
 	case 'o':
-		sysfs_attr_get_value(np->ctl, "state", fld, sizeof(fld));
-		return snprintf(buff, len, "%s", fld);
+		if (sysfs_attr_get_value(np->ctl, "state",
+					 fld, sizeof(fld)) > 0)
+			return snprintf(buff, len, "%s", fld);
+		break;
 	case 'T':
 		if (sysfs_attr_get_value(np->udev, "ana_state", fld,
 					 sizeof(fld)) > 0)
-- 
2.19.2

[PATCH 12/12] libmultipath(coverity): fix possible NULL dereference

[Martin Wilck]

coverity warns that recv_packet may set reply to NULL.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/configure.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index 64a1f7b..970b913 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -1042,7 +1042,7 @@ int check_daemon(void)
 	if (recv_packet(fd, &reply, timeout) != 0)
 		goto out;
 
-	if (strstr(reply, "shutdown"))
+	if (reply && strstr(reply, "shutdown"))
 		goto out_free;
 
 	ret = 1;
-- 
2.19.2

Re: [PATCH 08/12] libmultipath(coverity): fix "enum misuse" for find_multipaths

[Benjamin Marzinski]

On Tue, Jan 08, 2019 at 11:54:05PM +0100, Martin Wilck wrote:
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/dict.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libmultipath/dict.c b/libmultipath/dict.c
> index fd29abc..eaad4f1 100644
> --- a/libmultipath/dict.c
> +++ b/libmultipath/dict.c
> @@ -327,7 +327,7 @@ def_find_multipaths_handler(struct config *conf, vector strvec)
>  	int i;
>  
>  	if (set_yes_no_undef(strvec, &conf->find_multipaths) == 0 &&
> -	    conf->find_multipaths != YNU_UNDEF)
> +	    conf->find_multipaths != FIND_MULTIPATHS_UNDEF)
>  		return 0;

Is there some reason that I don't understand why YNU_UNDEF is
problematic here, but not later in the function at the:

        if (conf->find_multipaths == YNU_UNDEF) {
                condlog(0, "illegal value for find_multipaths: %s", buff);
                conf->find_multipaths = DEFAULT_FIND_MULTIPATHS;
        }

check?

-Ben

>  
>  	buff = set_value(strvec);
> -- 
> 2.19.2

Re: [PATCH 00/12] multipath-tools: Coverity patches

[Benjamin Marzinski]

On Tue, Jan 08, 2019 at 11:53:57PM +0100, Martin Wilck wrote:

Aside from my question on 08/12, ACK for the set

-Ben

> Hi Christophe,
> 
> here are a few simple fixes motivated by a recent
> coverity scan of the latest upstream. Most are false positives,
> but it can't hurt to fix them anyway, be it only to get a quicker
> overview on future scans.
> 
> Unfortunately synopsys messed up the scan.coverity.com website
> before I could finish this work.
> 
> There are two bigger issues with coverity that this set doesn't
> address:
> 
> 1) apparently coverity doesn't understand our frequently
>     used paradigm (simplified):
> 
>     pthread_cleanup_push(pthread_mutex_unlock, &mutex);
>     pthread_mutex_lock(&mutex);
>     do_some_work();
>     pthread_cleanup_pop(1);
> 
>  2) coverity warns that we sleep in multiple places holding the
>     vecs lock. That will be a bit harder to fix.
> 
> Martin
> 
> Martin Wilck (12):
>   kpartx(coverity): fix resource leak warning
>   libmultipath(coverity): cleanup dup usage in execute_program()
>   kpartx(coverity): fix apparent out-of-bounds access
>   libmultipath(coverity): make sure readlink result is 0-terminated
>   libmultipath(coverity): fix apparent overflow
>   libmpathcmd(coverity): limit reply length
>   libmultipath(coverity): fix int overflow in sysfs_set_scsi_tmo
>   libmultipath(coverity): fix "enum misuse" for find_multipaths
>   multipathd(coverity): check retval clock_gettime()
>   libmpathpersist(coverity): range checking for PRIN length
>   libmultipath/foreign(coverity): retval check in snprint_nvme_path
>   libmultipath(coverity): fix possible NULL dereference
> 
>  kpartx/dasd.c                    |  7 +++----
>  libmpathcmd/mpath_cmd.c          |  4 ++++
>  libmpathcmd/mpath_cmd.h          |  6 ++++++
>  libmpathpersist/mpath_pr_ioctl.c | 10 +++++++---
>  libmultipath/callout.c           | 13 +++++++------
>  libmultipath/configure.c         |  2 +-
>  libmultipath/dict.c              |  2 +-
>  libmultipath/discovery.c         |  4 ++--
>  libmultipath/foreign/nvme.c      |  6 ++++--
>  libmultipath/util.c              |  1 +
>  multipathd/cli.c                 |  2 ++
>  multipathd/cli.h                 |  6 ++++++
>  multipathd/cli_handlers.c        |  1 +
>  multipathd/main.c                |  9 +++++----
>  14 files changed, 50 insertions(+), 23 deletions(-)
> 
> -- 
> 2.19.2

Re: [PATCH 08/12] libmultipath(coverity): fix "enum misuse" for find_multipaths

[Martin Wilck]

On Thu, 2019-01-17 at 13:55 -0600, Benjamin Marzinski wrote:
> On Tue, Jan 08, 2019 at 11:54:05PM +0100, Martin Wilck wrote:
> > Signed-off-by: Martin Wilck <mwilck@suse.com>
> > ---
> >  libmultipath/dict.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/libmultipath/dict.c b/libmultipath/dict.c
> > index fd29abc..eaad4f1 100644
> > --- a/libmultipath/dict.c
> > +++ b/libmultipath/dict.c
> > @@ -327,7 +327,7 @@ def_find_multipaths_handler(struct config
> > *conf, vector strvec)
> >  	int i;
> >  
> >  	if (set_yes_no_undef(strvec, &conf->find_multipaths) == 0 &&
> > -	    conf->find_multipaths != YNU_UNDEF)
> > +	    conf->find_multipaths != FIND_MULTIPATHS_UNDEF)
> >  		return 0;
> 
> Is there some reason that I don't understand why YNU_UNDEF is
> problematic here, but not later in the function at the:
> 
>         if (conf->find_multipaths == YNU_UNDEF) {
>                 condlog(0, "illegal value for find_multipaths: %s",
> buff);
>                 conf->find_multipaths = DEFAULT_FIND_MULTIPATHS;
>         }
> 
> check?

Good catch. Maybe coverity would have found that in a second pass? Or I
was just too blind... As I said, this round of work on coverity was
disrupted by coverity's server downtime, which is still not fixed.

Anyway, this isn't wrong, only incomplete. I'd rather not resend the
whole series because of this. When coverity is up again, I'll do
another round of fixes.

Martin

-- 
Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)


--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [PATCH 08/12] libmultipath(coverity): fix "enum misuse" for find_multipaths

[Benjamin Marzinski]

On Thu, Jan 17, 2019 at 10:25:03PM +0100, Martin Wilck wrote:
> On Thu, 2019-01-17 at 13:55 -0600, Benjamin Marzinski wrote:
> > On Tue, Jan 08, 2019 at 11:54:05PM +0100, Martin Wilck wrote:
> > > Signed-off-by: Martin Wilck <mwilck@suse.com>
> > > ---
> > >  libmultipath/dict.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/libmultipath/dict.c b/libmultipath/dict.c
> > > index fd29abc..eaad4f1 100644
> > > --- a/libmultipath/dict.c
> > > +++ b/libmultipath/dict.c
> > > @@ -327,7 +327,7 @@ def_find_multipaths_handler(struct config
> > > *conf, vector strvec)
> > >  	int i;
> > >  
> > >  	if (set_yes_no_undef(strvec, &conf->find_multipaths) == 0 &&
> > > -	    conf->find_multipaths != YNU_UNDEF)
> > > +	    conf->find_multipaths != FIND_MULTIPATHS_UNDEF)
> > >  		return 0;
> > 
> > Is there some reason that I don't understand why YNU_UNDEF is
> > problematic here, but not later in the function at the:
> > 
> >         if (conf->find_multipaths == YNU_UNDEF) {
> >                 condlog(0, "illegal value for find_multipaths: %s",
> > buff);
> >                 conf->find_multipaths = DEFAULT_FIND_MULTIPATHS;
> >         }
> > 
> > check?
> 
> Good catch. Maybe coverity would have found that in a second pass? Or I
> was just too blind... As I said, this round of work on coverity was
> disrupted by coverity's server downtime, which is still not fixed.
> 
> Anyway, this isn't wrong, only incomplete. I'd rather not resend the
> whole series because of this. When coverity is up again, I'll do
> another round of fixes.
> 

Sure. ACK.

-Ben

> Martin
> 
> -- 
> Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
> SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
> HRB 21284 (AG Nürnberg)
>