[PATCH 0/6] More Misc Multipath patches

[PATCH 0/6] More Misc Multipath patches

[Benjamin Marzinski]

Here are the patches that I've built up while getting my last batch
accepted, as well as a change for the tur checker to hopefully
address Martin's short term objections to commit 455242d
(libmultipath: fix tur checker timeout).

Benjamin Marzinski (6):
  multipath: tweak logging style
  multipathd: check for NULL udevice in cli_add_path
  libmultipath: remove max_fds code duplication
  multipathd: set return code for multipathd commands
  mpathpersist: fix registration rollback issue
  libmultipath: timeout on unresponsive tur thread

 libmpathpersist/mpath_persist.c | 16 +++-------------
 libmultipath/checkers/tur.c     |  6 +++---
 libmultipath/devmapper.c        |  8 ++++----
 libmultipath/util.c             | 31 +++++++++++++++++++++++++++++++
 libmultipath/util.h             |  1 +
 multipath/main.c                | 12 +-----------
 multipathd/cli_handlers.c       |  4 ++++
 multipathd/main.c               | 41 +++++++++--------------------------------
 multipathd/uxclnt.c             | 13 ++++++++-----
 9 files changed, 64 insertions(+), 68 deletions(-)

-- 
2.7.4

[PATCH 1/6] multipath: tweak logging style

[Benjamin Marzinski]

When multipathd commands are run, errors should be printed to stderr,
instead of syslog. Also, when the multipath is run and calls
device-mapper, device-mapper should log to stderr instead of stdout,
just like multipath does now.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/devmapper.c | 8 ++++----
 multipathd/main.c        | 2 ++
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
index 8136d15..0433b49 100644
--- a/libmultipath/devmapper.c
+++ b/libmultipath/devmapper.c
@@ -80,11 +80,11 @@ dm_write_log (int level, const char *file, int line, const char *f, ...)
 			strftime(buff, sizeof(buff), "%b %d %H:%M:%S", tb);
 			buff[sizeof(buff)-1] = '\0';
 
-			fprintf(stdout, "%s | ", buff);
+			fprintf(stderr, "%s | ", buff);
 		}
-		fprintf(stdout, "libdevmapper: %s(%i): ", file, line);
-		vfprintf(stdout, f, ap);
-		fprintf(stdout, "\n");
+		fprintf(stderr, "libdevmapper: %s(%i): ", file, line);
+		vfprintf(stderr, f, ap);
+		fprintf(stderr, "\n");
 	} else {
 		condlog(level, "libdevmapper: %s(%i): ", file, line);
 		log_safe(level + 3, f, ap);
diff --git a/multipathd/main.c b/multipathd/main.c
index af33239..5f0193b 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2984,6 +2984,7 @@ main (int argc, char *argv[])
 			logsink = -1;
 			break;
 		case 'k':
+			logsink = 0;
 			conf = load_config(DEFAULT_CONFIGFILE);
 			if (!conf)
 				exit(1);
@@ -3013,6 +3014,7 @@ main (int argc, char *argv[])
 		char * s = cmd;
 		char * c = s;
 
+		logsink = 0;
 		conf = load_config(DEFAULT_CONFIGFILE);
 		if (!conf)
 			exit(1);
-- 
2.7.4

[PATCH 2/6] multipathd: check for NULL udevice in cli_add_path

[Benjamin Marzinski]

If cli_add_path can't get a udevice for the path, it should fail
immediately, instead of continuing with a NULL udevice, since it will
fail in store_pathinfo() anyway.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 multipathd/cli_handlers.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
index bb16472..7500080 100644
--- a/multipathd/cli_handlers.c
+++ b/multipathd/cli_handlers.c
@@ -720,6 +720,10 @@ cli_add_path (void * v, char ** reply, int * len, void * data)
 		udevice = udev_device_new_from_subsystem_sysname(udev,
 								 "block",
 								 param);
+		if (!udevice) {
+			condlog(0, "%s: can't find path", param);
+			return 1;
+		}
 		conf = get_multipath_config();
 		pthread_cleanup_push(put_multipath_config, conf);
 		r = store_pathinfo(vecs->pathvec, conf,
-- 
2.7.4

[PATCH 3/6] libmultipath: remove max_fds code duplication

[Benjamin Marzinski]

Instead of multipath, multipathd, and mpathpersist all having code to
set the max number of open file descriptors, just use a util function to
do it.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmpathpersist/mpath_persist.c | 11 +----------
 libmultipath/util.c             | 31 +++++++++++++++++++++++++++++++
 libmultipath/util.h             |  1 +
 multipath/main.c                | 12 +-----------
 multipathd/main.c               | 31 +++----------------------------
 5 files changed, 37 insertions(+), 49 deletions(-)

diff --git a/libmpathpersist/mpath_persist.c b/libmpathpersist/mpath_persist.c
index 4229a94..29e7fb4 100644
--- a/libmpathpersist/mpath_persist.c
+++ b/libmpathpersist/mpath_persist.c
@@ -31,7 +31,6 @@
 #include <stdlib.h>
 #include <string.h>
 #include <errno.h>
-#include <sys/resource.h>
 
 #define __STDC_FORMAT_MACROS 1
 
@@ -48,15 +47,7 @@ mpath_lib_init (void)
 		return NULL;
 	}
 
-	if (conf->max_fds) {
-		struct rlimit fd_limit;
-
-		fd_limit.rlim_cur = conf->max_fds;
-		fd_limit.rlim_max = conf->max_fds;
-		if (setrlimit(RLIMIT_NOFILE, &fd_limit) < 0)
-			condlog(0, "can't set open fds limit to %d : %s",
-				   conf->max_fds, strerror(errno));
-	}
+	set_max_fds(conf->max_fds);
 
 	return conf;
 }
diff --git a/libmultipath/util.c b/libmultipath/util.c
index 347af5b..d08112d 100644
--- a/libmultipath/util.c
+++ b/libmultipath/util.c
@@ -7,6 +7,8 @@
 #include <sys/sysmacros.h>
 #include <sys/types.h>
 #include <sys/utsname.h>
+#include <sys/time.h>
+#include <sys/resource.h>
 #include <dirent.h>
 #include <unistd.h>
 #include <errno.h>
@@ -465,3 +467,32 @@ int safe_write(int fd, const void *buf, size_t count)
 	}
 	return 0;
 }
+
+void set_max_fds(int max_fds)
+{
+	struct rlimit fd_limit;
+
+	if (!max_fds)
+		return;
+
+	if (getrlimit(RLIMIT_NOFILE, &fd_limit) < 0) {
+		condlog(0, "can't get open fds limit: %s",
+			strerror(errno));
+		fd_limit.rlim_cur = 0;
+		fd_limit.rlim_max = 0;
+	}
+	if (fd_limit.rlim_cur < max_fds) {
+		fd_limit.rlim_cur = max_fds;
+		if (fd_limit.rlim_max < max_fds)
+			fd_limit.rlim_max = max_fds;
+		if (setrlimit(RLIMIT_NOFILE, &fd_limit) < 0) {
+			condlog(0, "can't set open fds limit to "
+				"%lu/%lu : %s",
+				fd_limit.rlim_cur, fd_limit.rlim_max,
+				strerror(errno));
+		} else {
+			condlog(3, "set open fds limit to %lu/%lu",
+				fd_limit.rlim_cur, fd_limit.rlim_max);
+		}
+	}
+}
diff --git a/libmultipath/util.h b/libmultipath/util.h
index 56cec76..c246295 100644
--- a/libmultipath/util.h
+++ b/libmultipath/util.h
@@ -21,6 +21,7 @@ int get_linux_version_code(void);
 int parse_prkey(char *ptr, uint64_t *prkey);
 int parse_prkey_flags(char *ptr, uint64_t *prkey, uint8_t *flags);
 int safe_write(int fd, const void *buf, size_t count);
+void set_max_fds(int max_fds);
 
 #define KERNEL_VERSION(maj, min, ptc) ((((maj) * 256) + (min)) * 256 + (ptc))
 
diff --git a/multipath/main.c b/multipath/main.c
index d5aad95..05b7bf0 100644
--- a/multipath/main.c
+++ b/multipath/main.c
@@ -56,8 +56,6 @@
 #include "pgpolicies.h"
 #include "version.h"
 #include <errno.h>
-#include <sys/time.h>
-#include <sys/resource.h>
 #include "wwids.h"
 #include "uxsock.h"
 #include "mpath_cmd.h"
@@ -1002,15 +1000,7 @@ main (int argc, char *argv[])
 		logsink = 1;
 	}
 
-	if (conf->max_fds) {
-		struct rlimit fd_limit;
-
-		fd_limit.rlim_cur = conf->max_fds;
-		fd_limit.rlim_max = conf->max_fds;
-		if (setrlimit(RLIMIT_NOFILE, &fd_limit) < 0)
-			condlog(0, "can't set open fds limit to %d : %s",
-				conf->max_fds, strerror(errno));
-	}
+	set_max_fds(conf->max_fds);
 
 	libmp_udev_set_sync_support(1);
 
diff --git a/multipathd/main.c b/multipathd/main.c
index 5f0193b..d3f7719 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -12,8 +12,6 @@
 #include <sys/types.h>
 #include <fcntl.h>
 #include <errno.h>
-#include <sys/time.h>
-#include <sys/resource.h>
 #include <limits.h>
 #include <linux/oom.h>
 #include <libudev.h>
@@ -2663,33 +2661,10 @@ child (void * param)
 
 	envp = getenv("LimitNOFILE");
 
-	if (envp) {
+	if (envp)
 		condlog(2,"Using systemd provided open fds limit of %s", envp);
-	} else if (conf->max_fds) {
-		struct rlimit fd_limit;
-
-		if (getrlimit(RLIMIT_NOFILE, &fd_limit) < 0) {
-			condlog(0, "can't get open fds limit: %s",
-				strerror(errno));
-			fd_limit.rlim_cur = 0;
-			fd_limit.rlim_max = 0;
-		}
-		if (fd_limit.rlim_cur < conf->max_fds) {
-			fd_limit.rlim_cur = conf->max_fds;
-			if (fd_limit.rlim_max < conf->max_fds)
-				fd_limit.rlim_max = conf->max_fds;
-			if (setrlimit(RLIMIT_NOFILE, &fd_limit) < 0) {
-				condlog(0, "can't set open fds limit to "
-					"%lu/%lu : %s",
-					fd_limit.rlim_cur, fd_limit.rlim_max,
-					strerror(errno));
-			} else {
-				condlog(3, "set open fds limit to %lu/%lu",
-					fd_limit.rlim_cur, fd_limit.rlim_max);
-			}
-		}
-
-	}
+	else
+		set_max_fds(conf->max_fds);
 
 	vecs = gvecs = init_vecs();
 	if (!vecs)
-- 
2.7.4

[PATCH 4/6] multipathd: set return code for multipathd commands

[Benjamin Marzinski]

Failed multipathd commands should set the return code to 1.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 multipathd/main.c   |  8 ++++----
 multipathd/uxclnt.c | 13 ++++++++-----
 2 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index d3f7719..2d45d98 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2966,9 +2966,9 @@ main (int argc, char *argv[])
 			if (verbosity)
 				conf->verbosity = verbosity;
 			uxsock_timeout = conf->uxsock_timeout;
-			uxclnt(optarg, uxsock_timeout + 100);
+			err = uxclnt(optarg, uxsock_timeout + 100);
 			free_config(conf);
-			exit(0);
+			return err;
 		case 'B':
 			bindings_read_only = 1;
 			break;
@@ -3005,9 +3005,9 @@ main (int argc, char *argv[])
 			optind++;
 		}
 		c += snprintf(c, s + CMDSIZE - c, "\n");
-		uxclnt(s, uxsock_timeout + 100);
+		err = uxclnt(s, uxsock_timeout + 100);
 		free_config(conf);
-		exit(0);
+		return err;
 	}
 
 	if (foreground) {
diff --git a/multipathd/uxclnt.c b/multipathd/uxclnt.c
index 08db0e8..a76f8e2 100644
--- a/multipathd/uxclnt.c
+++ b/multipathd/uxclnt.c
@@ -103,14 +103,14 @@ static void process(int fd, unsigned int timeout)
 	}
 }
 
-static void process_req(int fd, char * inbuf, unsigned int timeout)
+static int process_req(int fd, char * inbuf, unsigned int timeout)
 {
 	char *reply;
 	int ret;
 
 	if (send_packet(fd, inbuf) != 0) {
 		printf("cannot send packet\n");
-		return;
+		return 1;
 	}
 	ret = recv_packet(fd, &reply, timeout);
 	if (ret < 0) {
@@ -118,9 +118,12 @@ static void process_req(int fd, char * inbuf, unsigned int timeout)
 			printf("timeout receiving packet\n");
 		else
 			printf("error %d receiving packet\n", ret);
+		return 1;
 	} else {
 		printf("%s", reply);
+		ret = (strcmp(reply, "fail\n") == 0);
 		FREE(reply);
+		return ret;
 	}
 }
 
@@ -129,16 +132,16 @@ static void process_req(int fd, char * inbuf, unsigned int timeout)
  */
 int uxclnt(char * inbuf, unsigned int timeout)
 {
-	int fd;
+	int fd, ret = 0;
 
 	fd = mpath_connect();
 	if (fd == -1)
 		exit(1);
 
 	if (inbuf)
-		process_req(fd, inbuf, timeout);
+		ret = process_req(fd, inbuf, timeout);
 	else
 		process(fd, timeout);
 	mpath_disconnect(fd);
-	return 0;
+	return ret;
 }
-- 
2.7.4

[PATCH 5/6] mpathpersist: fix registration rollback issue

[Benjamin Marzinski]

When mpathpersist tries to rollback the registration, it copies
the SARK to the RK, and clears the SARK. However, it repeated this step
for each thread. This means that the first rollback thread correctly
had the RK set to the SARK used during registration. However, if more
than one registration needed to be rolled back, later threads would have
both the RK and SARK cleared. This commit fixes that by only copying and
clearing the SARK once.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmpathpersist/mpath_persist.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/libmpathpersist/mpath_persist.c b/libmpathpersist/mpath_persist.c
index 29e7fb4..2ffe56e 100644
--- a/libmpathpersist/mpath_persist.c
+++ b/libmpathpersist/mpath_persist.c
@@ -559,11 +559,10 @@ int mpath_prout_reg(struct multipath *mpp,int rq_servact, int rq_scope,
 	}
 	if (rollback && ((rq_servact == MPATH_PROUT_REG_SA) && sa_key != 0 )){
 		condlog (3, "%s: ERROR: initiating pr out rollback", mpp->wwid);
+		memcpy(&paramp->key, &paramp->sa_key, 8);
+		memset(&paramp->sa_key, 0, 8);
 		for( i=0 ; i < count ; i++){
 			if(thread[i].param.status == MPATH_PR_SUCCESS) {
-				memcpy(&thread[i].param.paramp->key, &thread[i].param.paramp->sa_key, 8);
-				memset(&thread[i].param.paramp->sa_key, 0, 8);
-				thread[i].param.status = MPATH_PR_SUCCESS;
 				rc = pthread_create(&thread[i].id, &attr, mpath_prout_pthread_fn,
 						(void *)(&thread[i].param));
 				if (rc){
-- 
2.7.4

[PATCH 6/6] libmultipath: timeout on unresponsive tur thread

[Benjamin Marzinski]

If the tur checker thread has been cancelled but isn't responding,
timeout instead of doing a sync check.  This will keep one bad
device from impacting all of multipathd.

Fixes: 455242d ("libmultipath: fix tur checker timeout")
Cc: Martin Wilck <mwilck@suse.com>
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/checkers/tur.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/libmultipath/checkers/tur.c b/libmultipath/checkers/tur.c
index 86c0cdc..b2a2170 100644
--- a/libmultipath/checkers/tur.c
+++ b/libmultipath/checkers/tur.c
@@ -305,10 +305,10 @@ int libcheck_check(struct checker * c)
 	} else {
 		if (uatomic_read(&ct->holders) > 1) {
 			/* The thread has been cancelled but hasn't
-			 * quilt. Fail back to synchronous mode */
-			condlog(3, "%d:%d : tur checker failing back to sync",
+			 * quit. exit with timeout. */
+			condlog(3, "%d:%d : tur thread not responding",
 				major(ct->devt), minor(ct->devt));
-			return tur_check(c->fd, c->timeout, c->message);
+			return PATH_TIMEOUT;
 		}
 		/* Start new TUR checker */
 		pthread_mutex_lock(&ct->lock);
-- 
2.7.4

Re: [PATCH 5/6] mpathpersist: fix registration rollback issue

[Martin Wilck]

On Wed, 2018-10-10 at 13:01 -0500, Benjamin Marzinski wrote:
> When mpathpersist tries to rollback the registration, it copies
> the SARK to the RK, and clears the SARK. However, it repeated this
> step
> for each thread. This means that the first rollback thread correctly
> had the RK set to the SARK used during registration. However, if more
> than one registration needed to be rolled back, later threads would
> have
> both the RK and SARK cleared. This commit fixes that by only copying
> and
> clearing the SARK once.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  libmpathpersist/mpath_persist.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/libmpathpersist/mpath_persist.c
> b/libmpathpersist/mpath_persist.c
> index 29e7fb4..2ffe56e 100644
> --- a/libmpathpersist/mpath_persist.c
> +++ b/libmpathpersist/mpath_persist.c
> @@ -559,11 +559,10 @@ int mpath_prout_reg(struct multipath *mpp,int
> rq_servact, int rq_scope,
>  	}
>  	if (rollback && ((rq_servact == MPATH_PROUT_REG_SA) && sa_key
> != 0 )){
>  		condlog (3, "%s: ERROR: initiating pr out rollback",
> mpp->wwid);
> +		memcpy(&paramp->key, &paramp->sa_key, 8);
> +		memset(&paramp->sa_key, 0, 8);
>  		for( i=0 ; i < count ; i++){
>  			if(thread[i].param.status == MPATH_PR_SUCCESS)
> {
> -				memcpy(&thread[i].param.paramp->key,
> &thread[i].param.paramp->sa_key, 8);
> -				memset(&thread[i].param.paramp->sa_key, 
> 0, 8);
> -				thread[i].param.status =
> MPATH_PR_SUCCESS;

Why did you delete this last statement, too?

Martin

>  				rc = pthread_create(&thread[i].id,
> &attr, mpath_prout_pthread_fn,
>  						(void
> *)(&thread[i].param));
>  				if (rc){

-- 
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 1/6] multipath: tweak logging style

[Martin Wilck]

On Wed, 2018-10-10 at 13:01 -0500, Benjamin Marzinski wrote:
> When multipathd commands are run, errors should be printed to stderr,
> instead of syslog. Also, when the multipath is run and calls
> device-mapper, device-mapper should log to stderr instead of stdout,
> just like multipath does now.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>

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

-- 
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 2/6] multipathd: check for NULL udevice in cli_add_path

[Martin Wilck]

On Wed, 2018-10-10 at 13:01 -0500, Benjamin Marzinski wrote:
> If cli_add_path can't get a udevice for the path, it should fail
> immediately, instead of continuing with a NULL udevice, since it will
> fail in store_pathinfo() anyway.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>


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

-- 
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 3/6] libmultipath: remove max_fds code duplication

[Martin Wilck]

On Wed, 2018-10-10 at 13:01 -0500, Benjamin Marzinski wrote:
> Instead of multipath, multipathd, and mpathpersist all having code to
> set the max number of open file descriptors, just use a util function
> to
> do it.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>


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

-- 
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 4/6] multipathd: set return code for multipathd commands

[Martin Wilck]

On Wed, 2018-10-10 at 13:01 -0500, Benjamin Marzinski wrote:
> Failed multipathd commands should set the return code to 1.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---

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

-- 
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 6/6] libmultipath: timeout on unresponsive tur thread

[Martin Wilck]

On Wed, 2018-10-10 at 13:01 -0500, Benjamin Marzinski wrote:
> If the tur checker thread has been cancelled but isn't responding,
> timeout instead of doing a sync check.  This will keep one bad
> device from impacting all of multipathd.
> 
> Fixes: 455242d ("libmultipath: fix tur checker timeout")
> Cc: Martin Wilck <mwilck@suse.com>
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---

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

-- 
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 5/6] mpathpersist: fix registration rollback issue

[Martin Wilck]

On Wed, 2018-10-10 at 13:01 -0500, Benjamin Marzinski wrote:
> When mpathpersist tries to rollback the registration, it copies
> the SARK to the RK, and clears the SARK. However, it repeated this
> step
> for each thread. This means that the first rollback thread correctly
> had the RK set to the SARK used during registration. However, if more
> than one registration needed to be rolled back, later threads would
> have
> both the RK and SARK cleared. This commit fixes that by only copying
> and
> clearing the SARK once.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>

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

-- 
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 5/6] mpathpersist: fix registration rollback issue

[Martin Wilck]

On Wed, 2018-10-10 at 20:12 +0200, Martin Wilck wrote:

> > diff --git a/libmpathpersist/mpath_persist.c
> > b/libmpathpersist/mpath_persist.c
> > index 29e7fb4..2ffe56e 100644
> > --- a/libmpathpersist/mpath_persist.c
> > +++ b/libmpathpersist/mpath_persist.c
> > @@ -559,11 +559,10 @@ int mpath_prout_reg(struct multipath *mpp,int
> > rq_servact, int rq_scope,
> >  	}
> >  	if (rollback && ((rq_servact == MPATH_PROUT_REG_SA) && sa_key
> > != 0 )){
> >  		condlog (3, "%s: ERROR: initiating pr out rollback",
> > mpp->wwid);
> > +		memcpy(&paramp->key, &paramp->sa_key, 8);
> > +		memset(&paramp->sa_key, 0, 8);
> >  		for( i=0 ; i < count ; i++){
> >  			if(thread[i].param.status == MPATH_PR_SUCCESS)
> > {
> > -				memcpy(&thread[i].param.paramp->key,
> > &thread[i].param.paramp->sa_key, 8);
> > -				memset(&thread[i].param.paramp-
> > >sa_key, 
> > 0, 8);
> > -				thread[i].param.status =
> > MPATH_PR_SUCCESS;
> 
> Why did you delete this last statement, too?

For obvious reasons ... Sorry for the noise. 
I added a Reviewed-by: tag.

Best,
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