[PATCH v4 00/19] Misc Multipath patches

[PATCH v4 00/19] Misc Multipath patches

[Benjamin Marzinski]

This batch is a resend of v4, rebased on the latest upstream code with
changes base on Martin's review

Changes in v4
	0001-libmultipath-fix-tur-checker-timeout.patch now fails back
	to synchronous mode if the thread was cancelled but continues to
	run.  Since this is an unlikely event, I decided that it made
	more sense to not complicate the existing thread synchronization
	code to allow multiple threads. However, if someone else want's
	to make those changes, I'm open to the idea.

	0002-libmultipath-fix-tur-checker-double-locking.patch now saves
	dev_t instead of a string in the checker context, as suggested
	by Martin

	The only differences in patches 0003-0005 are that they are
	rebased on top of the new patches 0001-0002, but I took off
	Martin's Reviewed-by (just it case I messed something up when
	rebasing them).

Changes in v3
        added patches 0018-0019

Changes in v2
        0002-libmultipath-fix-tur-checker-double-locking.patch now sets
        ct->devt when initially creating the tur_checker_context, while
        that structure is still only referenced by a local variable.
        After that, ct->devt is only ever read. This should remove any
        issues with it needing locking.

Benjamin Marzinski (19):
  libmultipath: fix tur checker timeout
  libmultipath: fix tur checker double locking
  libmultipath: fix tur memory misuse
  libmultipath: cleanup tur locking
  libmultipath: fix tur checker timeout issue
  libmultipath: fix set_int error path
  libmultipath: fix length issues in get_vpd_sgio
  libmultipath: _install_keyword cleanup
  libmultipath: remove unused code
  libmultipath: fix memory issue in path_latency prio
  libmultipath: fix null dereference int alloc_path_group
  libmutipath: don't use malformed uevents
  multipath: fix max array size in print_cmd_valid
  multipathd: function return value tweaks
  multipathd: minor fixes
  multipathd: remove useless check and fix format
  multipathd: fix memory leak on error in configure
  libmultipath: Don't blank intialized paths
  libmultipath: Fixup updating paths

 libmultipath/checkers/tur.c              | 168 +++++++++++--------------------
 libmultipath/dict.c                      |   5 +-
 libmultipath/discovery.c                 |  18 ++--
 libmultipath/parser.c                    |  12 ++-
 libmultipath/print.c                     |   8 --
 libmultipath/prioritizers/path_latency.c |   3 +-
 libmultipath/structs.c                   |   2 +-
 libmultipath/uevent.c                    |   6 ++
 multipath/main.c                         |   2 +-
 multipathd/cli_handlers.c                |  11 +-
 multipathd/main.c                        |  50 +++++----
 11 files changed, 130 insertions(+), 155 deletions(-)

-- 
2.7.4

[PATCH v4 01/19] libmultipath: fix tur checker timeout

[Benjamin Marzinski]

The code previously was timing out mode if ct->thread was 0 but
ct->running wasn't. This combination never happens.  The idea was to
timeout if for some reason the path checker tried to cancel the thread,
but it didn't die.  The correct thing to check for this is ct->holders.
ct->holders will always be at least one when libcheck_check() is called,
since libcheck_free() won't get called until the device is no longer
being checked. So, if ct->holders is 2, that means that the tur thread
is has not shut down yet.

Also, instead of timing out, the tur checker will switch to synchronous
mode.  The chance of this code path happening is very low.  I simply
exists because the old thread must not interfere with a new thread
starting up. But if something does go very wrong, and a thread does get
stuck, this solution will keep the checker from just ignoring the device
forever.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/checkers/tur.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/libmultipath/checkers/tur.c b/libmultipath/checkers/tur.c
index bf8486d..3c5e236 100644
--- a/libmultipath/checkers/tur.c
+++ b/libmultipath/checkers/tur.c
@@ -355,12 +355,13 @@ int libcheck_check(struct checker * c)
 		}
 		pthread_mutex_unlock(&ct->lock);
 	} else {
-		if (uatomic_read(&ct->running) != 0) {
-			/* pthread cancel failed. continue in sync mode */
+		if (uatomic_read(&ct->holders) > 1) {
+			/* The thread has been cancelled but hasn't
+			 * quilt. Fail back to synchronous mode */
 			pthread_mutex_unlock(&ct->lock);
-			condlog(3, "%s: tur thread not responding",
+			condlog(3, "%s: tur checker failing back to sync",
 				tur_devt(devt, sizeof(devt), ct));
-			return PATH_TIMEOUT;
+			return tur_check(c->fd, c->timeout, copy_msg_to_checker, c);
 		}
 		/* Start new TUR checker */
 		ct->state = PATH_UNCHECKED;
-- 
2.7.4

[PATCH v4 02/19] libmultipath: fix tur checker double locking

[Benjamin Marzinski]

tur_devt() locks ct->lock. However, it is ocassionally called while
ct->lock is already locked. In reality, there is no reason why we need
to lock all the accesses to ct->devt. The tur checker only needs to
write to this variable one time, when it first gets the file descripter
that it is checking. This patch sets ct->devt in libcheck_init() when it
is first initializing the checker context. After that, ct->devt is only
ever read.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/checkers/tur.c | 60 +++++++++++++++------------------------------
 1 file changed, 20 insertions(+), 40 deletions(-)

diff --git a/libmultipath/checkers/tur.c b/libmultipath/checkers/tur.c
index 3c5e236..5844639 100644
--- a/libmultipath/checkers/tur.c
+++ b/libmultipath/checkers/tur.c
@@ -39,34 +39,22 @@
 struct tur_checker_context {
 	dev_t devt;
 	int state;
-	int running;
+	int running; /* uatomic access only */
 	int fd;
 	unsigned int timeout;
 	time_t time;
 	pthread_t thread;
 	pthread_mutex_t lock;
 	pthread_cond_t active;
-	int holders;
+	int holders; /* uatomic access only */
 	char message[CHECKER_MSG_LEN];
 };
 
-static const char *tur_devt(char *devt_buf, int size,
-			    struct tur_checker_context *ct)
-{
-	dev_t devt;
-
-	pthread_mutex_lock(&ct->lock);
-	devt = ct->devt;
-	pthread_mutex_unlock(&ct->lock);
-
-	snprintf(devt_buf, size, "%d:%d", major(devt), minor(devt));
-	return devt_buf;
-}
-
 int libcheck_init (struct checker * c)
 {
 	struct tur_checker_context *ct;
 	pthread_mutexattr_t attr;
+	struct stat sb;
 
 	ct = malloc(sizeof(struct tur_checker_context));
 	if (!ct)
@@ -81,6 +69,8 @@ int libcheck_init (struct checker * c)
 	pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_RECURSIVE);
 	pthread_mutex_init(&ct->lock, &attr);
 	pthread_mutexattr_destroy(&attr);
+	if (fstat(c->fd, &sb) == 0)
+		ct->devt = sb.st_rdev;
 	c->context = ct;
 
 	return 0;
@@ -232,14 +222,13 @@ static void *tur_thread(void *ctx)
 {
 	struct tur_checker_context *ct = ctx;
 	int state, running;
-	char devt[32];
 
 	/* This thread can be canceled, so setup clean up */
 	tur_thread_cleanup_push(ct);
 	rcu_register_thread();
 
-	condlog(3, "%s: tur checker starting up",
-		tur_devt(devt, sizeof(devt), ct));
+	condlog(3, "%d:%d : tur checker starting up", major(ct->devt),
+		minor(ct->devt));
 
 	/* TUR checker start up */
 	pthread_mutex_lock(&ct->lock);
@@ -256,8 +245,8 @@ static void *tur_thread(void *ctx)
 	pthread_cond_signal(&ct->active);
 	pthread_mutex_unlock(&ct->lock);
 
-	condlog(3, "%s: tur checker finished, state %s",
-		tur_devt(devt, sizeof(devt), ct), checker_state_name(state));
+	condlog(3, "%d:%d : tur checker finished, state %s", major(ct->devt),
+		minor(ct->devt), checker_state_name(state));
 
 	running = uatomic_xchg(&ct->running, 0);
 	if (!running)
@@ -305,20 +294,12 @@ int libcheck_check(struct checker * c)
 {
 	struct tur_checker_context *ct = c->context;
 	struct timespec tsp;
-	struct stat sb;
 	pthread_attr_t attr;
 	int tur_status, r;
-	char devt[32];
 
 	if (!ct)
 		return PATH_UNCHECKED;
 
-	if (fstat(c->fd, &sb) == 0) {
-		pthread_mutex_lock(&ct->lock);
-		ct->devt = sb.st_rdev;
-		pthread_mutex_unlock(&ct->lock);
-	}
-
 	if (c->sync)
 		return tur_check(c->fd, c->timeout, copy_msg_to_checker, c);
 
@@ -327,8 +308,7 @@ int libcheck_check(struct checker * c)
 	 */
 	r = pthread_mutex_lock(&ct->lock);
 	if (r != 0) {
-		condlog(2, "%s: tur mutex lock failed with %d",
-			tur_devt(devt, sizeof(devt), ct), r);
+		condlog(2, "%s: tur mutex lock failed with %d", ct->devt, r);
 		MSG(c, MSG_TUR_FAILED);
 		return PATH_WILD;
 	}
@@ -338,14 +318,14 @@ int libcheck_check(struct checker * c)
 			int running = uatomic_xchg(&ct->running, 0);
 			if (running)
 				pthread_cancel(ct->thread);
-			condlog(3, "%s: tur checker timeout",
-				tur_devt(devt, sizeof(devt), ct));
+			condlog(3, "%d:%d : tur checker timeout",
+				major(ct->devt), minor(ct->devt));
 			ct->thread = 0;
 			MSG(c, MSG_TUR_TIMEOUT);
 			tur_status = PATH_TIMEOUT;
 		} else if (uatomic_read(&ct->running) != 0) {
-			condlog(3, "%s: tur checker not finished",
-					tur_devt(devt, sizeof(devt), ct));
+			condlog(3, "%d:%d : tur checker not finished",
+				major(ct->devt), minor(ct->devt));
 			tur_status = PATH_PENDING;
 		} else {
 			/* TUR checker done */
@@ -359,8 +339,8 @@ int libcheck_check(struct checker * c)
 			/* The thread has been cancelled but hasn't
 			 * quilt. Fail back to synchronous mode */
 			pthread_mutex_unlock(&ct->lock);
-			condlog(3, "%s: tur checker failing back to sync",
-				tur_devt(devt, sizeof(devt), ct));
+			condlog(3, "%d:%d : tur checker failing back to sync",
+				major(ct->devt), minor(ct->devt));
 			return tur_check(c->fd, c->timeout, copy_msg_to_checker, c);
 		}
 		/* Start new TUR checker */
@@ -378,8 +358,8 @@ int libcheck_check(struct checker * c)
 			uatomic_set(&ct->running, 0);
 			ct->thread = 0;
 			pthread_mutex_unlock(&ct->lock);
-			condlog(3, "%s: failed to start tur thread, using"
-				" sync mode", tur_devt(devt, sizeof(devt), ct));
+			condlog(3, "%d:%d : failed to start tur thread, using"
+				" sync mode", major(ct->devt), minor(ct->devt));
 			return tur_check(c->fd, c->timeout,
 					 copy_msg_to_checker, c);
 		}
@@ -390,8 +370,8 @@ int libcheck_check(struct checker * c)
 		pthread_mutex_unlock(&ct->lock);
 		if (uatomic_read(&ct->running) != 0 &&
 		    (tur_status == PATH_PENDING || tur_status == PATH_UNCHECKED)) {
-			condlog(3, "%s: tur checker still running",
-				tur_devt(devt, sizeof(devt), ct));
+			condlog(3, "%d:%d : tur checker still running",
+				major(ct->devt), minor(ct->devt));
 			tur_status = PATH_PENDING;
 		} else {
 			int running = uatomic_xchg(&ct->running, 0);
-- 
2.7.4

[PATCH v4 03/19] libmultipath: fix tur memory misuse

[Benjamin Marzinski]

when tur_thread() was calling tur_check(), it was passing ct->message as
the copy argument, but copy_msg_to_tcc() was assuming that it was
getting a tur_checker_context pointer. This means it was treating
ct->message as ct. This is why the tur checker never printed checker
messages. Intead of simply changing the copy argument passed in, I just
removed all the copying code, since it is completely unnecessary. The
callers of tur_check() can just pass in a buffer that it is safe to
write to, and copy it later, if necessary.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/checkers/tur.c | 48 ++++++++++++---------------------------------
 1 file changed, 12 insertions(+), 36 deletions(-)

diff --git a/libmultipath/checkers/tur.c b/libmultipath/checkers/tur.c
index 5844639..0c7b5ca 100644
--- a/libmultipath/checkers/tur.c
+++ b/libmultipath/checkers/tur.c
@@ -102,17 +102,8 @@ void libcheck_free (struct checker * c)
 	return;
 }
 
-#define TUR_MSG(fmt, args...)					\
-	do {							\
-		char msg[CHECKER_MSG_LEN];			\
-								\
-		snprintf(msg, sizeof(msg), fmt, ##args);	\
-		copy_message(cb_arg, msg);			\
-	} while (0)
-
 static int
-tur_check(int fd, unsigned int timeout,
-	  void (*copy_message)(void *, const char *), void *cb_arg)
+tur_check(int fd, unsigned int timeout, char *msg)
 {
 	struct sg_io_hdr io_hdr;
 	unsigned char turCmdBlk[TUR_CMD_LEN] = { 0x00, 0, 0, 0, 0, 0 };
@@ -131,7 +122,7 @@ retry:
 	io_hdr.timeout = timeout * 1000;
 	io_hdr.pack_id = 0;
 	if (ioctl(fd, SG_IO, &io_hdr) < 0) {
-		TUR_MSG(MSG_TUR_DOWN);
+		snprintf(msg, CHECKER_MSG_LEN, MSG_TUR_DOWN);
 		return PATH_DOWN;
 	}
 	if ((io_hdr.status & 0x7e) == 0x18) {
@@ -139,7 +130,7 @@ retry:
 		 * SCSI-3 arrays might return
 		 * reservation conflict on TUR
 		 */
-		TUR_MSG(MSG_TUR_UP);
+		snprintf(msg, CHECKER_MSG_LEN, MSG_TUR_UP);
 		return PATH_UP;
 	}
 	if (io_hdr.info & SG_INFO_OK_MASK) {
@@ -184,14 +175,14 @@ retry:
 				 * LOGICAL UNIT NOT ACCESSIBLE,
 				 * TARGET PORT IN STANDBY STATE
 				 */
-				TUR_MSG(MSG_TUR_GHOST);
+				snprintf(msg, CHECKER_MSG_LEN, MSG_TUR_GHOST);
 				return PATH_GHOST;
 			}
 		}
-		TUR_MSG(MSG_TUR_DOWN);
+		snprintf(msg, CHECKER_MSG_LEN, MSG_TUR_DOWN);
 		return PATH_DOWN;
 	}
-	TUR_MSG(MSG_TUR_UP);
+	snprintf(msg, CHECKER_MSG_LEN, MSG_TUR_UP);
 	return PATH_UP;
 }
 
@@ -209,19 +200,11 @@ static void cleanup_func(void *data)
 	rcu_unregister_thread();
 }
 
-static void copy_msg_to_tcc(void *ct_p, const char *msg)
-{
-	struct tur_checker_context *ct = ct_p;
-
-	pthread_mutex_lock(&ct->lock);
-	strlcpy(ct->message, msg, sizeof(ct->message));
-	pthread_mutex_unlock(&ct->lock);
-}
-
 static void *tur_thread(void *ctx)
 {
 	struct tur_checker_context *ct = ctx;
 	int state, running;
+	char msg[CHECKER_MSG_LEN];
 
 	/* This thread can be canceled, so setup clean up */
 	tur_thread_cleanup_push(ct);
@@ -236,12 +219,13 @@ static void *tur_thread(void *ctx)
 	ct->message[0] = '\0';
 	pthread_mutex_unlock(&ct->lock);
 
-	state = tur_check(ct->fd, ct->timeout, copy_msg_to_tcc, ct->message);
+	state = tur_check(ct->fd, ct->timeout, msg);
 	pthread_testcancel();
 
 	/* TUR checker done */
 	pthread_mutex_lock(&ct->lock);
 	ct->state = state;
+	strlcpy(ct->message, msg, sizeof(ct->message));
 	pthread_cond_signal(&ct->active);
 	pthread_mutex_unlock(&ct->lock);
 
@@ -283,13 +267,6 @@ static int tur_check_async_timeout(struct checker *c)
 	return (now.tv_sec > ct->time);
 }
 
-static void copy_msg_to_checker(void *c_p, const char *msg)
-{
-	struct checker *c = c_p;
-
-	strlcpy(c->message, msg, sizeof(c->message));
-}
-
 int libcheck_check(struct checker * c)
 {
 	struct tur_checker_context *ct = c->context;
@@ -301,7 +278,7 @@ int libcheck_check(struct checker * c)
 		return PATH_UNCHECKED;
 
 	if (c->sync)
-		return tur_check(c->fd, c->timeout, copy_msg_to_checker, c);
+		return tur_check(c->fd, c->timeout, c->message);
 
 	/*
 	 * Async mode
@@ -341,7 +318,7 @@ int libcheck_check(struct checker * c)
 			pthread_mutex_unlock(&ct->lock);
 			condlog(3, "%d:%d : tur checker failing back to sync",
 				major(ct->devt), minor(ct->devt));
-			return tur_check(c->fd, c->timeout, copy_msg_to_checker, c);
+			return tur_check(c->fd, c->timeout, c->message);
 		}
 		/* Start new TUR checker */
 		ct->state = PATH_UNCHECKED;
@@ -360,8 +337,7 @@ int libcheck_check(struct checker * c)
 			pthread_mutex_unlock(&ct->lock);
 			condlog(3, "%d:%d : failed to start tur thread, using"
 				" sync mode", major(ct->devt), minor(ct->devt));
-			return tur_check(c->fd, c->timeout,
-					 copy_msg_to_checker, c);
+			return tur_check(c->fd, c->timeout, c->message);
 		}
 		tur_timeout(&tsp);
 		r = pthread_cond_timedwait(&ct->active, &ct->lock, &tsp);
-- 
2.7.4

[PATCH v4 04/19] libmultipath: cleanup tur locking

[Benjamin Marzinski]

There are only three variables whose access needs to be synchronized
between the tur thread and the path checker itself: state, message, and
active.  The rest of the variables are either only written when the tur
thread isn't running, or they aren't accessed by the tur thread, or they
are atomics that are themselves used to synchronize things.

This patch limits the amount of code that is covered by ct->lock to
only what needs to be locked. It also makes ct->lock no longer a
recursive lock. To make this simpler, tur_thread now only sets the
state and message one time, instead of twice, since PATH_UNCHECKED
was never able to be returned anyway.

One benefit of this is that the tur checker thread gets more time to
call tur_check() and return before libcheck_check() gives up and
return PATH_PENDING.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/checkers/tur.c | 44 ++++++++++++++++----------------------------
 1 file changed, 16 insertions(+), 28 deletions(-)

diff --git a/libmultipath/checkers/tur.c b/libmultipath/checkers/tur.c
index 0c7b5ca..983ba4c 100644
--- a/libmultipath/checkers/tur.c
+++ b/libmultipath/checkers/tur.c
@@ -53,7 +53,6 @@ struct tur_checker_context {
 int libcheck_init (struct checker * c)
 {
 	struct tur_checker_context *ct;
-	pthread_mutexattr_t attr;
 	struct stat sb;
 
 	ct = malloc(sizeof(struct tur_checker_context));
@@ -65,10 +64,7 @@ int libcheck_init (struct checker * c)
 	ct->fd = -1;
 	uatomic_set(&ct->holders, 1);
 	pthread_cond_init_mono(&ct->active);
-	pthread_mutexattr_init(&attr);
-	pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_RECURSIVE);
-	pthread_mutex_init(&ct->lock, &attr);
-	pthread_mutexattr_destroy(&attr);
+	pthread_mutex_init(&ct->lock, NULL);
 	if (fstat(c->fd, &sb) == 0)
 		ct->devt = sb.st_rdev;
 	c->context = ct;
@@ -213,12 +209,6 @@ static void *tur_thread(void *ctx)
 	condlog(3, "%d:%d : tur checker starting up", major(ct->devt),
 		minor(ct->devt));
 
-	/* TUR checker start up */
-	pthread_mutex_lock(&ct->lock);
-	ct->state = PATH_PENDING;
-	ct->message[0] = '\0';
-	pthread_mutex_unlock(&ct->lock);
-
 	state = tur_check(ct->fd, ct->timeout, msg);
 	pthread_testcancel();
 
@@ -283,13 +273,6 @@ int libcheck_check(struct checker * c)
 	/*
 	 * Async mode
 	 */
-	r = pthread_mutex_lock(&ct->lock);
-	if (r != 0) {
-		condlog(2, "%s: tur mutex lock failed with %d", ct->devt, r);
-		MSG(c, MSG_TUR_FAILED);
-		return PATH_WILD;
-	}
-
 	if (ct->thread) {
 		if (tur_check_async_timeout(c)) {
 			int running = uatomic_xchg(&ct->running, 0);
@@ -307,21 +290,24 @@ int libcheck_check(struct checker * c)
 		} else {
 			/* TUR checker done */
 			ct->thread = 0;
+			pthread_mutex_lock(&ct->lock);
 			tur_status = ct->state;
 			strlcpy(c->message, ct->message, sizeof(c->message));
+			pthread_mutex_unlock(&ct->lock);
 		}
-		pthread_mutex_unlock(&ct->lock);
 	} else {
 		if (uatomic_read(&ct->holders) > 1) {
 			/* The thread has been cancelled but hasn't
 			 * quilt. Fail back to synchronous mode */
-			pthread_mutex_unlock(&ct->lock);
 			condlog(3, "%d:%d : tur checker failing back to sync",
 				major(ct->devt), minor(ct->devt));
 			return tur_check(c->fd, c->timeout, c->message);
 		}
 		/* Start new TUR checker */
-		ct->state = PATH_UNCHECKED;
+		pthread_mutex_lock(&ct->lock);
+		tur_status = ct->state = PATH_PENDING;
+		ct->message[0] = '\0';
+		pthread_mutex_unlock(&ct->lock);
 		ct->fd = c->fd;
 		ct->timeout = c->timeout;
 		uatomic_add(&ct->holders, 1);
@@ -334,21 +320,23 @@ int libcheck_check(struct checker * c)
 			uatomic_sub(&ct->holders, 1);
 			uatomic_set(&ct->running, 0);
 			ct->thread = 0;
-			pthread_mutex_unlock(&ct->lock);
 			condlog(3, "%d:%d : failed to start tur thread, using"
 				" sync mode", major(ct->devt), minor(ct->devt));
 			return tur_check(c->fd, c->timeout, c->message);
 		}
 		tur_timeout(&tsp);
-		r = pthread_cond_timedwait(&ct->active, &ct->lock, &tsp);
-		tur_status = ct->state;
-		strlcpy(c->message, ct->message, sizeof(c->message));
+		pthread_mutex_lock(&ct->lock);
+		if (ct->state == PATH_PENDING)
+			r = pthread_cond_timedwait(&ct->active, &ct->lock, 
+						   &tsp);
+		if (!r) {
+			tur_status = ct->state;
+			strlcpy(c->message, ct->message, sizeof(c->message));
+		}
 		pthread_mutex_unlock(&ct->lock);
-		if (uatomic_read(&ct->running) != 0 &&
-		    (tur_status == PATH_PENDING || tur_status == PATH_UNCHECKED)) {
+		if (tur_status == PATH_PENDING) {
 			condlog(3, "%d:%d : tur checker still running",
 				major(ct->devt), minor(ct->devt));
-			tur_status = PATH_PENDING;
 		} else {
 			int running = uatomic_xchg(&ct->running, 0);
 			if (running)
-- 
2.7.4

[PATCH v4 05/19] libmultipath: fix tur checker timeout issue

[Benjamin Marzinski]

If the tur checker is run, and the tur_thread has timed out,
libcheck_check() doesn't actually check if the thread is still running.
This means that the thread could have already completed successfully,
but the tur checker would still return PATH_TIMEOUT, instead of the
value returned by the thread. This patch makes libcheck_check() actually
check if the thread completed, and if so, it returns the proper value.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/checkers/tur.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/libmultipath/checkers/tur.c b/libmultipath/checkers/tur.c
index 983ba4c..86c0cdc 100644
--- a/libmultipath/checkers/tur.c
+++ b/libmultipath/checkers/tur.c
@@ -276,13 +276,20 @@ int libcheck_check(struct checker * c)
 	if (ct->thread) {
 		if (tur_check_async_timeout(c)) {
 			int running = uatomic_xchg(&ct->running, 0);
-			if (running)
+			if (running) {
 				pthread_cancel(ct->thread);
-			condlog(3, "%d:%d : tur checker timeout",
-				major(ct->devt), minor(ct->devt));
+				condlog(3, "%d:%d : tur checker timeout",
+					major(ct->devt), minor(ct->devt));
+				MSG(c, MSG_TUR_TIMEOUT);
+				tur_status = PATH_TIMEOUT;
+			} else {
+				pthread_mutex_lock(&ct->lock);
+				tur_status = ct->state;
+				strlcpy(c->message, ct->message,
+					sizeof(c->message));
+				pthread_mutex_unlock(&ct->lock);
+			}
 			ct->thread = 0;
-			MSG(c, MSG_TUR_TIMEOUT);
-			tur_status = PATH_TIMEOUT;
 		} else if (uatomic_read(&ct->running) != 0) {
 			condlog(3, "%d:%d : tur checker not finished",
 				major(ct->devt), minor(ct->devt));
-- 
2.7.4

[PATCH v4 06/19] libmultipath: fix set_int error path

[Benjamin Marzinski]

set_int() wasn't checking if the line actually had a value before
converting it to an integer.  Found by coverity. Also, it should
be using set_value().

Reviewed-by: Martin Wilck <mwilck@suse.com>
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/dict.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/libmultipath/dict.c b/libmultipath/dict.c
index 32524d5..bf4701e 100644
--- a/libmultipath/dict.c
+++ b/libmultipath/dict.c
@@ -33,7 +33,10 @@ set_int(vector strvec, void *ptr)
 	int *int_ptr = (int *)ptr;
 	char * buff;
 
-	buff = VECTOR_SLOT(strvec, 1);
+	buff = set_value(strvec);
+	if (!buff)
+		return 1;
+
 	*int_ptr = atoi(buff);
 
 	return 0;
-- 
2.7.4

[PATCH v4 07/19] libmultipath: fix length issues in get_vpd_sgio

[Benjamin Marzinski]

When get_vpd_sgio() finds out that the vpd info needed to be truncated
to fit in the buffer, it doesn't trucate the size as well,  which allows
it to overwrite the buffer. Also, in once len is set to -ENODATA,
get_vpd_sgio() should exit, instead of using the negative len in
memcpy(). Found by coverity.

Reviewed-by: Martin Wilck <mwilck@suse.com>
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/discovery.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index f973d4b..301093f 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -1116,17 +1116,21 @@ get_vpd_sgio (int fd, int pg, char * str, int maxlen)
 		return -ENODATA;
 	}
 	buff_len = get_unaligned_be16(&buff[2]) + 4;
-	if (buff_len > 4096)
+	if (buff_len > 4096) {
 		condlog(3, "vpd pg%02x page truncated", pg);
-
+		buff_len = 4096;
+	}
 	if (pg == 0x80)
 		len = parse_vpd_pg80(buff, str, maxlen);
 	else if (pg == 0x83)
 		len = parse_vpd_pg83(buff, buff_len, str, maxlen);
 	else if (pg == 0xc9 && maxlen >= 8) {
-		len = buff_len < 8 ? -ENODATA :
-			(buff_len <= maxlen ? buff_len : maxlen);
-		memcpy (str, buff, len);
+		if (buff_len < 8)
+			len = -ENODATA;
+		else {
+			len = (buff_len <= maxlen)? buff_len : maxlen;
+			memcpy (str, buff, len);
+		}
 	} else
 		len = -ENOSYS;
 
-- 
2.7.4

[PATCH v4 08/19] libmultipath: _install_keyword cleanup

[Benjamin Marzinski]

_install_keyword should use VECTOR_LAST_SLOT(), which has better error
checking. It should also fail if it gets a NULL pointer, instead of
dereferencing it. Found by coverity.

Reviewed-by: Martin Wilck <mwilck@suse.com>
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/parser.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/libmultipath/parser.c b/libmultipath/parser.c
index b8b7e0d..92ef7cf 100644
--- a/libmultipath/parser.c
+++ b/libmultipath/parser.c
@@ -79,12 +79,16 @@ _install_keyword(vector keywords, char *string,
 	struct keyword *keyword;
 
 	/* fetch last keyword */
-	keyword = VECTOR_SLOT(keywords, VECTOR_SIZE(keywords) - 1);
+	keyword = VECTOR_LAST_SLOT(keywords);
+	if (!keyword)
+		return 1;
 
 	/* position to last sub level */
-	for (i = 0; i < sublevel; i++)
-		keyword =
-		    VECTOR_SLOT(keyword->sub, VECTOR_SIZE(keyword->sub) - 1);
+	for (i = 0; i < sublevel; i++) {
+		keyword = VECTOR_LAST_SLOT(keyword->sub);
+		if (!keyword)
+			return 1;
+	}
 
 	/* First sub level allocation */
 	if (!keyword->sub)
-- 
2.7.4

[PATCH v4 09/19] libmultipath: remove unused code

[Benjamin Marzinski]

since vector_foreach_slot() already checks if the entry is NULL, there's
no point in checking it in the loop, since it can't be NULL there. Found
by coverity.

Reviewed-by: Martin Wilck <mwilck@suse.com>
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/print.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/libmultipath/print.c b/libmultipath/print.c
index 9da6a77..7b610b9 100644
--- a/libmultipath/print.c
+++ b/libmultipath/print.c
@@ -275,8 +275,6 @@ snprint_multipath_vpr (char * buff, size_t len, const struct multipath * mpp)
 	int i, j;
 
 	vector_foreach_slot(mpp->pg, pgp, i) {
-		if (!pgp)
-			continue;
 		vector_foreach_slot(pgp->paths, pp, j) {
 			if (strlen(pp->vendor_id) && strlen(pp->product_id))
 				return snprintf(buff, len, "%s,%s",
@@ -295,8 +293,6 @@ snprint_multipath_vend (char * buff, size_t len, const struct multipath * mpp)
 	int i, j;
 
 	vector_foreach_slot(mpp->pg, pgp, i) {
-		if (!pgp)
-			continue;
 		vector_foreach_slot(pgp->paths, pp, j) {
 			if (strlen(pp->vendor_id))
 				return snprintf(buff, len, "%s", pp->vendor_id);
@@ -313,8 +309,6 @@ snprint_multipath_prod (char * buff, size_t len, const struct multipath * mpp)
 	int i, j;
 
 	vector_foreach_slot(mpp->pg, pgp, i) {
-		if (!pgp)
-			continue;
 		vector_foreach_slot(pgp->paths, pp, j) {
 			if (strlen(pp->product_id))
 				return snprintf(buff, len, "%s", pp->product_id);
@@ -331,8 +325,6 @@ snprint_multipath_rev (char * buff, size_t len, const struct multipath * mpp)
 	int i, j;
 
 	vector_foreach_slot(mpp->pg, pgp, i) {
-		if (!pgp)
-			continue;
 		vector_foreach_slot(pgp->paths, pp, j) {
 			if (strlen(pp->rev))
 				return snprintf(buff, len, "%s", pp->rev);
-- 
2.7.4

[PATCH v4 10/19] libmultipath: fix memory issue in path_latency prio

[Benjamin Marzinski]

The path_latency prioriziter was assuming that prepare_directio_read()
always succeeds. However, it doesn't, and when it fails, the prioritizer
used buf without it pointing to alloced memory. Found by coverity.

Reviewed-by: Martin Wilck <mwilck@suse.com>
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/prioritizers/path_latency.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/libmultipath/prioritizers/path_latency.c b/libmultipath/prioritizers/path_latency.c
index 765265c..eeee01e 100644
--- a/libmultipath/prioritizers/path_latency.c
+++ b/libmultipath/prioritizers/path_latency.c
@@ -237,7 +237,8 @@ int getprio(struct path *pp, char *args, unsigned int timeout)
 	lg_maxavglatency = log(MAX_AVG_LATENCY) / lg_base;
 	lg_minavglatency = log(MIN_AVG_LATENCY) / lg_base;
 
-	prepare_directio_read(pp->fd, &blksize, &buf, &restore_flags);
+	if (prepare_directio_read(pp->fd, &blksize, &buf, &restore_flags) < 0)
+		return PRIO_UNDEF;
 
 	temp = io_num;
 	while (temp-- > 0) {
-- 
2.7.4

[PATCH v4 11/19] libmultipath: fix null dereference int alloc_path_group

[Benjamin Marzinski]

If all_pathgroup failed to allocate a vector for pgp->paths, instead of
failing after it freed pgp, it would set pgp to NULL and then
dereference it. This patch fixes that. Found by coverity.

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

diff --git a/libmultipath/structs.c b/libmultipath/structs.c
index ae847d6..caa178a 100644
--- a/libmultipath/structs.c
+++ b/libmultipath/structs.c
@@ -165,7 +165,7 @@ alloc_pathgroup (void)
 
 	if (!pgp->paths) {
 		FREE(pgp);
-		pgp = NULL;
+		return NULL;
 	}
 
 	dm_pathgroup_to_gen(pgp)->ops = &dm_gen_pathgroup_ops;
-- 
2.7.4

[PATCH v4 12/19] libmutipath: don't use malformed uevents

[Benjamin Marzinski]

A uevent that doesn't include the ACTION and DEVPATH fields is
malformed. It should be ignored, instead of used with those fields being
NULL.

Reviewed-by: Martin Wilck <mwilck@suse.com>
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/uevent.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/libmultipath/uevent.c b/libmultipath/uevent.c
index fd8ca35..5f910e6 100644
--- a/libmultipath/uevent.c
+++ b/libmultipath/uevent.c
@@ -729,6 +729,12 @@ struct uevent *uevent_from_udev_device(struct udev_device *dev)
 		if (i == HOTPLUG_NUM_ENVP - 1)
 			break;
 	}
+	if (!uev->devpath || ! uev->action) {
+		udev_device_unref(dev);
+		condlog(1, "uevent missing necessary fields");
+		FREE(uev);
+		return NULL;
+	}
 	uev->udev = dev;
 	uev->envp[i] = NULL;
 
-- 
2.7.4

[PATCH v4 13/19] multipath: fix max array size in print_cmd_valid

[Benjamin Marzinski]

The code is attempting to verify that 0 <= k < 3
However, sizeof(val) is 12, assuming 4 byte integers. The check needs to
take integer size into account. Found by coverity.

Reviewed-by: Martin Wilck <mwilck@suse.com>
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 multipath/main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/multipath/main.c b/multipath/main.c
index fc5bf16..d5aad95 100644
--- a/multipath/main.c
+++ b/multipath/main.c
@@ -482,7 +482,7 @@ static int print_cmd_valid(int k, const vector pathvec,
 	struct timespec until;
 	struct path *pp;
 
-	if (k < 0 || k >= sizeof(vals))
+	if (k < 0 || k >= (sizeof(vals) / sizeof(int)))
 		return 1;
 
 	if (k == 2) {
-- 
2.7.4

[PATCH v4 14/19] multipathd: function return value tweaks

[Benjamin Marzinski]

In cli_add_map() the return value of get_refwwid is never used, and
refwwid is checked to see if the function returned successfully, so the
return value doesn't need to be saved.

In resize_map, if setup_map fails, multipathd shouldn't attempt to
create the device with resulting params string. It should just fail
instead. Found by coverity.

Reviewed-by: Martin Wilck <mwilck@suse.com>
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 multipathd/cli_handlers.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
index 5682b5c..bb16472 100644
--- a/multipathd/cli_handlers.c
+++ b/multipathd/cli_handlers.c
@@ -796,8 +796,8 @@ cli_add_map (void * v, char ** reply, int * len, void * data)
 		if (!alias && !count) {
 			condlog(2, "%s: mapname not found for %d:%d",
 				param, major, minor);
-			rc = get_refwwid(CMD_NONE, param, DEV_DEVMAP,
-					 vecs->pathvec, &refwwid);
+			get_refwwid(CMD_NONE, param, DEV_DEVMAP,
+				    vecs->pathvec, &refwwid);
 			if (refwwid) {
 				if (coalesce_paths(vecs, NULL, refwwid,
 						   FORCE_RELOAD_NONE, CMD_NONE))
@@ -881,7 +881,12 @@ int resize_map(struct multipath *mpp, unsigned long long size,
 
 	mpp->size = size;
 	update_mpp_paths(mpp, vecs->pathvec);
-	setup_map(mpp, params, PARAMS_SIZE, vecs);
+	if (setup_map(mpp, params, PARAMS_SIZE, vecs) != 0) {
+		condlog(0, "%s: failed to setup map for resize : %s",
+			mpp->alias, strerror(errno));
+		mpp->size = orig_size;
+		return 1;
+	}
 	mpp->action = ACT_RESIZE;
 	mpp->force_udev_reload = 1;
 	if (domap(mpp, params, 1) <= 0) {
-- 
2.7.4

[PATCH v4 15/19] multipathd: minor fixes

[Benjamin Marzinski]

In update_multipath(), conf is set again in a couple of lines, and
nothing uses it before then, so there's no point in setting it twice.
Also, in ev_remove_path(), strncpy() could end up unterminated, so
use strlcpy() instead.

Reviewed-by: Martin Wilck <mwilck@suse.com>
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 multipathd/main.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index ba796ab..cd96304 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -429,7 +429,7 @@ int update_multipath (struct vectors *vecs, char *mapname, int reset)
 				continue;
 
 			if (pp->state != PATH_DOWN) {
-				struct config *conf = get_multipath_config();
+				struct config *conf;
 				int oldstate = pp->state;
 				int checkint;
 
@@ -1097,7 +1097,7 @@ ev_remove_path (struct path *pp, struct vectors * vecs, int need_do_map)
 			/*
 			 * flush_map will fail if the device is open
 			 */
-			strncpy(alias, mpp->alias, WWID_SIZE);
+			strlcpy(alias, mpp->alias, WWID_SIZE);
 			if (mpp->flush_on_last_del == FLUSH_ENABLED) {
 				condlog(2, "%s Last path deleted, disabling queueing", mpp->alias);
 				mpp->retry_tick = 0;
-- 
2.7.4

[PATCH v4 16/19] multipathd: remove useless check and fix format

[Benjamin Marzinski]

The only thing this patch changes is to remove the check for pp->mpp
before the check for pp->mpp->prflags, since check_path() already
verified that pp->mpp != NULL. This fixes a number of coverity warnings.
Also, I normalized the spacing and indenting of the nearby code.

Reviewed-by: Martin Wilck <mwilck@suse.com>
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 multipathd/main.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index cd96304..463b1b8 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -1979,14 +1979,14 @@ check_path (struct vectors * vecs, struct path * pp, int ticks)
 			return 1;
 		}
 
-		if(newstate == PATH_UP || newstate == PATH_GHOST){
-			if ( pp->mpp && pp->mpp->prflag ){
+		if (newstate == PATH_UP || newstate == PATH_GHOST) {
+			if (pp->mpp->prflag) {
 				/*
 				 * Check Persistent Reservation.
 				 */
-			condlog(2, "%s: checking persistent reservation "
-				"registration", pp->dev);
-			mpath_pr_event_handle(pp);
+				condlog(2, "%s: checking persistent "
+					"reservation registration", pp->dev);
+				mpath_pr_event_handle(pp);
 			}
 		}
 
-- 
2.7.4

[PATCH v4 17/19] multipathd: fix memory leak on error in configure

[Benjamin Marzinski]

If configure fails after allocing mpvec, it must free it. Found by
coverity.

Reviewed-by: Martin Wilck <mwilck@suse.com>
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 multipathd/main.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index 463b1b8..04dce04 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2278,7 +2278,7 @@ configure (struct vectors * vecs)
 	ret = path_discovery(vecs->pathvec, DI_ALL);
 	if (ret < 0) {
 		condlog(0, "configure failed at path discovery");
-		return 1;
+		goto fail;
 	}
 
 	vector_foreach_slot (vecs->pathvec, pp, i){
@@ -2295,7 +2295,7 @@ configure (struct vectors * vecs)
 	}
 	if (map_discovery(vecs)) {
 		condlog(0, "configure failed at map discovery");
-		return 1;
+		goto fail;
 	}
 
 	/*
@@ -2309,7 +2309,7 @@ configure (struct vectors * vecs)
 		force_reload = FORCE_RELOAD_YES;
 	if (ret) {
 		condlog(0, "configure failed while coalescing paths");
-		return 1;
+		goto fail;
 	}
 
 	/*
@@ -2318,7 +2318,7 @@ configure (struct vectors * vecs)
 	 */
 	if (coalesce_maps(vecs, mpvec)) {
 		condlog(0, "configure failed while coalescing maps");
-		return 1;
+		goto fail;
 	}
 
 	dm_lib_release();
@@ -2354,6 +2354,10 @@ configure (struct vectors * vecs)
 			i--;
 	}
 	return 0;
+
+fail:
+	vector_free(mpvec);
+	return 1;
 }
 
 int
-- 
2.7.4

[PATCH v4 18/19] libmultipath: Don't blank intialized paths

[Benjamin Marzinski]

When pathinfo fails for some likely transient reason, it clears the path
wwid, but otherwise returns successfully, to keep the path around but
not usable until it gets fully initialized. However, if the path has
already been initialized, and pathinfo hits a transient error, it
shouldn't clear the wwid.

Reviewed-by: Martin Wilck <mwilck@suse.com>
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/discovery.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index 301093f..b267f07 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -2004,9 +2004,9 @@ blank:
 	/*
 	 * Recoverable error, for example faulty or offline path
 	 */
-	memset(pp->wwid, 0, WWID_SIZE);
 	pp->chkrstate = pp->state = PATH_DOWN;
-	pp->initialized = INIT_FAILED;
+	if (pp->initialized == INIT_FAILED)
+		memset(pp->wwid, 0, WWID_SIZE);
 
 	return PATHINFO_OK;
 }
-- 
2.7.4

[PATCH v4 19/19] libmultipath: Fixup updating paths

[Benjamin Marzinski]

Commit 582c56cc broke some code paths in uev_update_path. First, it
changed the handling of paths that were not fully initialized.
uev_update_path was simply setting the wwids for all of these paths.
Instead, it should ignore the ones that had not requested a new uevent.
These paths are likely down, and are already getting handled by
check_path, after it verifies that they have become active. Also,
setting the wwid doesn't update all of the other information that
may have been missed when the path was initially added.

Also, it wasn't possible for pp->wwid_changed to transition back to
zero, unless the path's wwid was empty, in which case there was no
reason to worry about the wwid change in the first place, since the path
hadn't been fully initialized yet. So, even if a path's wwid changed and
then changed back to the original value, the path still could not be
used.

This patch fixes these issues, and also moves the check for paths that
have requested a new uevent up in the functions. These paths will get
fully reinitialized anyway, so there is no reason to do all the other
work first.

Fixes: 582c56cc ("libmultipath: uev_update_path: always warn if WWID
changed")

Reviewed-by: Martin Wilck <mwilck@suse.com>
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 multipathd/main.c | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index 04dce04..af33239 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -1208,6 +1208,15 @@ uev_update_path (struct uevent *uev, struct vectors * vecs)
 		struct multipath *mpp = pp->mpp;
 		char wwid[WWID_SIZE];
 
+		if (pp->initialized == INIT_REQUESTED_UDEV) {
+			needs_reinit = 1;
+			goto out;
+		}
+		/* Don't deal with other types of failed initialization
+		 * now. check_path will handle it */
+		if (!strlen(pp->wwid))
+			goto out;
+
 		strcpy(wwid, pp->wwid);
 		get_uid(pp, pp->state, uev->udev);
 
@@ -1216,9 +1225,8 @@ uev_update_path (struct uevent *uev, struct vectors * vecs)
 				uev->kernel, wwid, pp->wwid,
 				(disable_changed_wwids ? "disallowing" :
 				 "continuing"));
-			if (disable_changed_wwids &&
-			    (strlen(wwid) || pp->wwid_changed)) {
-				strcpy(pp->wwid, wwid);
+			strcpy(pp->wwid, wwid);
+			if (disable_changed_wwids) {
 				if (!pp->wwid_changed) {
 					pp->wwid_changed = 1;
 					pp->tick = 1;
@@ -1226,11 +1234,9 @@ uev_update_path (struct uevent *uev, struct vectors * vecs)
 						dm_fail_path(pp->mpp->alias, pp->dev_t);
 				}
 				goto out;
-			} else if (!disable_changed_wwids)
-				strcpy(pp->wwid, wwid);
-			else
-				pp->wwid_changed = 0;
+			}
 		} else {
+			pp->wwid_changed = 0;
 			udev_device_unref(pp->udev);
 			pp->udev = udev_device_ref(uev->udev);
 			conf = get_multipath_config();
@@ -1241,9 +1247,7 @@ uev_update_path (struct uevent *uev, struct vectors * vecs)
 			pthread_cleanup_pop(1);
 		}
 
-		if (pp->initialized == INIT_REQUESTED_UDEV)
-			needs_reinit = 1;
-		else if (mpp && ro >= 0) {
+		if (mpp && ro >= 0) {
 			condlog(2, "%s: update path write_protect to '%d' (uevent)", uev->kernel, ro);
 
 			if (mpp->wait_for_udev)
-- 
2.7.4

Re: [PATCH v4 01/19] libmultipath: fix tur checker timeout

[Martin Wilck]

On Tue, 2018-10-09 at 18:02 -0500, Benjamin Marzinski wrote:
> The code previously was timing out mode if ct->thread was 0 but
> ct->running wasn't. This combination never happens.  The idea was to
> timeout if for some reason the path checker tried to cancel the
> thread,
> but it didn't die.  The correct thing to check for this is ct-
> >holders.
> ct->holders will always be at least one when libcheck_check() is
> called,
> since libcheck_free() won't get called until the device is no longer
> being checked. So, if ct->holders is 2, that means that the tur
> thread
> is has not shut down yet.
> 
> Also, instead of timing out, the tur checker will switch to
> synchronous
> mode.  The chance of this code path happening is very low.  I simply
> exists because the old thread must not interfere with a new thread
> starting up. But if something does go very wrong, and a thread does
> get
> stuck, this solution will keep the checker from just ignoring the
> device
> forever.

Well, the previous tur thread hanging means that future attempts might
hang as well, in which case the synchronous approach would block _all_
path checkers. Wouldn't the following reasoning apply here?

commit 05cbea354172be5507ac83c98bbac8e02aa8cf3c
Author: Hannes Reinecke <hare@suse.de>
Date:   Fri Dec 13 13:12:42 2013 +0100

    multipath: do not call tur in sync mode if pthread_cancel fails
    
    When pthread_cancel fails the thread is stuck, most likely
    during I/O submission. So it would be pointless to call the
    tur checker in sync mode here, as this would be stuck, too.

I argued before that the current PATH_TIMEOUT return code is wrong, but
I think it's better than falling back to synchronous mode.

I'm fine with this patch if the return PATH_TIMEOUT remains for now,
and we vow to fix this for good soon.

> s
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  libmultipath/checkers/tur.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/libmultipath/checkers/tur.c
> b/libmultipath/checkers/tur.c
> index bf8486d..3c5e236 100644
> --- a/libmultipath/checkers/tur.c
> +++ b/libmultipath/checkers/tur.c
> @@ -355,12 +355,13 @@ int libcheck_check(struct checker * c)
>  		}
>  		pthread_mutex_unlock(&ct->lock);
>  	} else {
> -		if (uatomic_read(&ct->running) != 0) {
> -			/* pthread cancel failed. continue in sync mode
> */
> +		if (uatomic_read(&ct->holders) > 1) {
> +			/* The thread has been cancelled but hasn't
> +			 * quilt. Fail back to synchronous mode */

Typo.

>  			pthread_mutex_unlock(&ct->lock);
> -			condlog(3, "%s: tur thread not responding",
> +			condlog(3, "%s: tur checker failing back to
> sync",
>  				tur_devt(devt, sizeof(devt), ct));
> -			return PATH_TIMEOUT;
> +			return tur_check(c->fd, c->timeout,
> copy_msg_to_checker, c);
>  		}
>  		/* Start new TUR checker */
>  		ct->state = PATH_UNCHECKED;

Regards,
Martin

Re: [PATCH v4 02/19] libmultipath: fix tur checker double locking

[Martin Wilck]

On Tue, 2018-10-09 at 18:02 -0500, Benjamin Marzinski wrote:
> tur_devt() locks ct->lock. However, it is ocassionally called while
> ct->lock is already locked. In reality, there is no reason why we
> need
> to lock all the accesses to ct->devt. The tur checker only needs to
> write to this variable one time, when it first gets the file
> descripter
> that it is checking. This patch sets ct->devt in libcheck_init() when
> it
> is first initializing the checker context. After that, ct->devt is
> only
> ever read.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>

Reviewed-by: Martin Wilck <mwilck@suse.com>

Re: [PATCH v4 03/19] libmultipath: fix tur memory misuse

[Martin Wilck]

On Tue, 2018-10-09 at 18:03 -0500, Benjamin Marzinski wrote:
> when tur_thread() was calling tur_check(), it was passing ct->message 
> as
> the copy argument, but copy_msg_to_tcc() was assuming that it was
> getting a tur_checker_context pointer. This means it was treating
> ct->message as ct. This is why the tur checker never printed checker
> messages. Intead of simply changing the copy argument passed in, I
> just
> removed all the copying code, since it is completely unnecessary. The
> callers of tur_check() can just pass in a buffer that it is safe to
> write to, and copy it later, if necessary.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>

Reviewed-by: Martin Wilck <mwilck@suse.com>

Re: [PATCH v4 00/19] Misc Multipath patches

[Martin Wilck]

On Tue, 2018-10-09 at 18:02 -0500, Benjamin Marzinski wrote:
> This batch is a resend of v4, rebased on the latest upstream code
> with
> changes base on Martin's review
> 
> Changes in v4
> 	0001-libmultipath-fix-tur-checker-timeout.patch now fails back
> 	to synchronous mode if the thread was cancelled but continues
> to
> 	run.  Since this is an unlikely event, I decided that it made
> 	more sense to not complicate the existing thread
> synchronization
> 	code to allow multiple threads. However, if someone else want's
> 	to make those changes, I'm open to the idea.
> 
> 	0002-libmultipath-fix-tur-checker-double-locking.patch now
> saves
> 	dev_t instead of a string in the checker context, as suggested
> 	by Martin
> 
> 	The only differences in patches 0003-0005 are that they are
> 	rebased on top of the new patches 0001-0002, but I took off
> 	Martin's Reviewed-by (just it case I messed something up when
> 	rebasing them).

For the whole series except 01/19:

Reviewed-by: Martin Wilck <mwilck@suse.com>

(I believe minor rebasing would be necessary if my suggested change for
01/19 is applied).

Martin

Re: [PATCH v4 00/19] Misc Multipath patches

[Martin Wilck]

On Wed, 2018-10-10 at 09:19 +0200, Martin Wilck wrote:
> On Tue, 2018-10-09 at 18:02 -0500, Benjamin Marzinski wrote:
> > This batch is a resend of v4, rebased on the latest upstream code
> > with
> > changes base on Martin's review
> > 
> > Changes in v4
> > 	0001-libmultipath-fix-tur-checker-timeout.patch now fails back
> > 	to synchronous mode if the thread was cancelled but continues
> > to
> > 	run.  Since this is an unlikely event, I decided that it made
> > 	more sense to not complicate the existing thread
> > synchronization
> > 	code to allow multiple threads. However, if someone else want's
> > 	to make those changes, I'm open to the idea.
> > 
> > 	0002-libmultipath-fix-tur-checker-double-locking.patch now
> > saves
> > 	dev_t instead of a string in the checker context, as suggested
> > 	by Martin
> > 
> > 	The only differences in patches 0003-0005 are that they are
> > 	rebased on top of the new patches 0001-0002, but I took off
> > 	Martin's Reviewed-by (just it case I messed something up when
> > 	rebasing them).
> 
> For the whole series except 01/19:
> 
> Reviewed-by: Martin Wilck <mwilck@suse.com>
> 
> (I believe minor rebasing would be necessary if my suggested change
> for
> 01/19 is applied).

I just realized that Christophe already applied the whole series. So
I'll send my own suggestions as patches on top of this.

Martin