[PATCH 00/32] Misc Multipath patches

[PATCH 00/32] Misc Multipath patches

[Benjamin Marzinski]

This batch of patches is a resend of my previous 10 patches, as well
as new patches mostly found by running coverity.

patches 0001-0010 are identical to my previous patchset
patches 0012-0015 & 0021-0032 are minor issues found by coverity.
        Not all of them are bugs that could actually occur in practice,
        but they are simple and hopefully non-controversial changes.
patches 0016-0020 are a number of fixes to the tur checker.These are
        the ones that should get the most attention.

Also, I would love to have a 0.7.8 tagged release.

Benjamin Marzinski (32):
  libmultipath: remove last of rbd code
  libmultipath: fix detect alua corner case
  multipath: fix setting conf->version
  mpathpersist: add --param-alltgpt option
  libmutipath: remove unused IDE bus type
  multipathd: add new protocol path wildcard
  libmultipath: add "protocol" blacklist option.
  libmultipath: remove _filter_* blacklist functions
  multipath tests: change to work with old make versions
  multipath tests: add blacklist tests
  mpathpersist: add missing --param-rk usage info
  kpartx: minor fixes to make coverity happy
  kpartx: Fix memory leak of uuid found by coverity
  kpartx: fix bad dm_devn return
  kpartx: remove duplicated gpt validation check
  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

 kpartx/dasd.c                            |   5 +-
 kpartx/del-part-nodes.rules              |   2 +-
 kpartx/devmapper.c                       |  15 +-
 kpartx/gpt.c                             |  10 -
 libmpathpersist/mpath_persist.c          |  10 +-
 libmultipath/blacklist.c                 | 177 +++++------
 libmultipath/blacklist.h                 |   5 +-
 libmultipath/checkers/tur.c              | 165 ++++------
 libmultipath/config.c                    |  15 +
 libmultipath/config.h                    |   2 +
 libmultipath/configure.c                 |   2 +-
 libmultipath/devmapper.c                 |   6 +-
 libmultipath/dict.c                      |  19 +-
 libmultipath/discovery.c                 |  19 +-
 libmultipath/parser.c                    |  12 +-
 libmultipath/print.c                     |  82 ++++-
 libmultipath/print.h                     |   2 +
 libmultipath/prioritizers/path_latency.c |   3 +-
 libmultipath/propsel.c                   |   4 +-
 libmultipath/structs.c                   |   2 +-
 libmultipath/structs.h                   |   2 -
 libmultipath/uevent.c                    |   6 +
 mpathpersist/main.c                      |  12 +-
 mpathpersist/main.h                      |   1 +
 mpathpersist/mpathpersist.8              |   4 +
 multipath/main.c                         |   2 +-
 multipath/multipath.conf.5               |  24 +-
 multipathd/cli_handlers.c                |  11 +-
 multipathd/main.c                        |  26 +-
 tests/Makefile                           |   6 +-
 tests/blacklist.c                        | 512 +++++++++++++++++++++++++++++++
 31 files changed, 889 insertions(+), 274 deletions(-)
 create mode 100644 tests/blacklist.c

-- 
2.7.4

[PATCH 01/32] libmultipath: remove last of rbd code

[Benjamin Marzinski]

My previous patch to remove the rbd code missed a little bit.

Reviewed-by: Martin Wilck <mwilck@suse.com>
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 kpartx/del-part-nodes.rules | 2 +-
 libmultipath/structs.h      | 1 -
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/kpartx/del-part-nodes.rules b/kpartx/del-part-nodes.rules
index 17bc505..0ceecf5 100644
--- a/kpartx/del-part-nodes.rules
+++ b/kpartx/del-part-nodes.rules
@@ -10,7 +10,7 @@
 # or create an udev rule file that sets ENV{DONT_DEL_PART_NODES}="1".
 
 SUBSYSTEM!="block", GOTO="end_del_part_nodes"
-KERNEL!="sd*|dasd*|rbd*", GOTO="end_del_part_nodes"
+KERNEL!="sd*|dasd*", GOTO="end_del_part_nodes"
 ACTION!="add|change", GOTO="end_del_part_nodes"
 ENV{DEVTYPE}=="partition", GOTO="end_del_part_nodes"
 
diff --git a/libmultipath/structs.h b/libmultipath/structs.h
index e5b698b..ca14315 100644
--- a/libmultipath/structs.h
+++ b/libmultipath/structs.h
@@ -61,7 +61,6 @@ enum sysfs_buses {
 	SYSFS_BUS_IDE,
 	SYSFS_BUS_CCW,
 	SYSFS_BUS_CCISS,
-	SYSFS_BUS_RBD,
 	SYSFS_BUS_NVME,
 };
 
-- 
2.7.4

[PATCH 02/32] libmultipath: fix detect alua corner case

[Benjamin Marzinski]

If retain_attach_hw_handler = no, then the paths tpgs state will never
be checked, and the multipath device will always select the alua
handler, if no other handler is selected. the paths tpgs state
should be checked, regardless of the retain_hwhandler value.

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

diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
index af3ed62..fdb5953 100644
--- a/libmultipath/propsel.c
+++ b/libmultipath/propsel.c
@@ -420,9 +420,11 @@ int select_hwhandler(struct config *conf, struct multipath *mp)
 	bool all_tpgs = true;
 
 	dh_state = &handler[2];
+
+	vector_foreach_slot(mp->paths, pp, i)
+		all_tpgs = all_tpgs && (pp->tpgs > 0);
 	if (mp->retain_hwhandler != RETAIN_HWHANDLER_OFF) {
 		vector_foreach_slot(mp->paths, pp, i) {
-			all_tpgs = all_tpgs && (pp->tpgs > 0);
 			if (get_dh_state(pp, dh_state, sizeof(handler) - 2) > 0
 			    && strcmp(dh_state, "detached")) {
 				memcpy(handler, "1 ", 2);
-- 
2.7.4

[PATCH 03/32] multipath: fix setting conf->version

[Benjamin Marzinski]

Commit d3b71498 stopped multipath from setting conf->version. Instead,
it was always being set to 0.0.0. Multipathd was still setting this
correctly.

Fixes: d3b71498 "multipath: fix rcu thread cancellation hang"
Reviewed-by: Martin Wilck <mwilck@suse.com>
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/devmapper.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
index f2befad..8136d15 100644
--- a/libmultipath/devmapper.c
+++ b/libmultipath/devmapper.c
@@ -245,13 +245,13 @@ void libmp_dm_init(void)
 	int verbosity;
 	unsigned int version[3];
 
+	if (dm_prereq(version))
+		exit(1);
 	conf = get_multipath_config();
 	verbosity = conf->verbosity;
-	memcpy(version, conf->version, sizeof(version));
+	memcpy(conf->version, version, sizeof(version));
 	put_multipath_config(conf);
 	dm_init(verbosity);
-	if (dm_prereq(version))
-		exit(1);
 	dm_udev_set_sync_support(libmp_dm_udev_sync);
 }
 
-- 
2.7.4

[PATCH 04/32] mpathpersist: add --param-alltgpt option

[Benjamin Marzinski]

From the limited testing I've been able to do, commit 5b54e772
"mpathpersist: add all_tg_pt option", does appear to enable
--param-alltgpt to work correctly on devices that accept the ALL_TG_PT
flag, so I've added the option to mpathpersist.

Reviewed-by: Martin Wilck <mwilck@suse.com>
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmpathpersist/mpath_persist.c | 10 ++++------
 mpathpersist/main.c             | 11 ++++++++---
 mpathpersist/main.h             |  1 +
 mpathpersist/mpathpersist.8     |  4 ++++
 multipath/multipath.conf.5      |  8 +++++---
 5 files changed, 22 insertions(+), 12 deletions(-)

diff --git a/libmpathpersist/mpath_persist.c b/libmpathpersist/mpath_persist.c
index 6e9e67f..61818e0 100644
--- a/libmpathpersist/mpath_persist.c
+++ b/libmpathpersist/mpath_persist.c
@@ -466,11 +466,14 @@ int mpath_prout_reg(struct multipath *mpp,int rq_servact, int rq_scope,
 	int rc;
 	int count=0;
 	int status = MPATH_PR_SUCCESS;
+	int all_tg_pt;
 	uint64_t sa_key = 0;
 
 	if (!mpp)
 		return MPATH_PR_DMMP_ERROR;
 
+	all_tg_pt = (mpp->all_tg_pt == ALL_TG_PT_ON ||
+		     paramp->sa_flags & MPATH_F_ALL_TG_PT_MASK);
 	active_pathcount = pathcount(mpp, PATH_UP) + pathcount(mpp, PATH_GHOST);
 
 	if (active_pathcount == 0) {
@@ -478,10 +481,6 @@ int mpath_prout_reg(struct multipath *mpp,int rq_servact, int rq_scope,
 		return MPATH_PR_DMMP_ERROR;
 	}
 
-	if ( paramp->sa_flags & MPATH_F_ALL_TG_PT_MASK ) {
-		condlog (1, "Warning: ALL_TG_PT is set. Configuration not supported");
-	}
-
 	struct threadinfo thread[active_pathcount];
 	int hosts[active_pathcount];
 
@@ -518,8 +517,7 @@ int mpath_prout_reg(struct multipath *mpp,int rq_servact, int rq_scope,
 				condlog (1, "%s: %s path not up. Skip.", mpp->wwid, pp->dev);
 				continue;
 			}
-			if (mpp->all_tg_pt == ALL_TG_PT_ON &&
-			    pp->sg_id.host_no != -1) {
+			if (all_tg_pt && pp->sg_id.host_no != -1) {
 				for (k = 0; k < count; k++) {
 					if (pp->sg_id.host_no == hosts[k]) {
 						condlog(3, "%s: %s host %d matches skip.", pp->wwid, pp->dev, pp->sg_id.host_no);
diff --git a/mpathpersist/main.c b/mpathpersist/main.c
index 5b37f3a..99151fe 100644
--- a/mpathpersist/main.c
+++ b/mpathpersist/main.c
@@ -118,7 +118,7 @@ int main (int argc, char * argv[])
 	{
 		int option_index = 0;
 
-		c = getopt_long (argc, argv, "v:Cd:hHioZK:S:PAT:skrGILcRX:l:",
+		c = getopt_long (argc, argv, "v:Cd:hHioYZK:S:PAT:skrGILcRX:l:",
 				long_options, &option_index);
 		if (c == -1)
 			break;
@@ -158,6 +158,10 @@ int main (int argc, char * argv[])
 				prout_flag = 1;
 				break;
 
+			case 'Y':
+				param_alltgpt = 1;
+				++num_prout_param;
+				break;
 			case 'Z':
 				param_aptpl = 1;
 				++num_prout_param;
@@ -443,9 +447,9 @@ int main (int argc, char * argv[])
 		}
 
 		if (param_alltgpt)
-			paramp->sa_flags |= 0x4;
+			paramp->sa_flags |= MPATH_F_ALL_TG_PT_MASK;
 		if (param_aptpl)
-			paramp->sa_flags |= 0x1;
+			paramp->sa_flags |= MPATH_F_APTPL_MASK;
 
 		if (num_transport)
 		{
@@ -698,6 +702,7 @@ static void usage(void)
 			"    --hex|-H                   output response in hex\n"
 			"    --in|-i                    request PR In command \n"
 			"    --out|-o                   request PR Out command\n"
+			"    --param-alltgpt|-Y         PR Out parameter 'ALL_TG_PT\n"
 			"    --param-aptpl|-Z           PR Out parameter 'APTPL'\n"
 			"    --read-keys|-k             PR In: Read Keys\n"
 			"    --param-sark=SARK|-S SARK  PR Out parameter service "
diff --git a/mpathpersist/main.h b/mpathpersist/main.h
index 5c0e089..beb8a21 100644
--- a/mpathpersist/main.h
+++ b/mpathpersist/main.h
@@ -6,6 +6,7 @@ static struct option long_options[] = {
 	{"hex", 0, NULL, 'H'},
 	{"in", 0, NULL, 'i'},
 	{"out", 0, NULL, 'o'},
+	{"param-alltgpt", 0, NULL, 'Y'},
 	{"param-aptpl", 0, NULL, 'Z'},
 	{"param-rk", 1, NULL, 'K'},
 	{"param-sark", 1, NULL, 'S'},
diff --git a/mpathpersist/mpathpersist.8 b/mpathpersist/mpathpersist.8
index a8982e6..885491d 100644
--- a/mpathpersist/mpathpersist.8
+++ b/mpathpersist/mpathpersist.8
@@ -87,6 +87,10 @@ Request PR In command.
 Request PR Out command.
 .
 .TP
+.B \--param-alltgpt|\-Y
+PR Out parameter 'ALL_TG_PT'.
+.
+.TP
 .B \--param-aptpl|\-Z
 PR Out parameter 'APTPL'.
 .
diff --git a/multipath/multipath.conf.5 b/multipath/multipath.conf.5
index e4b25a0..fb863fd 100644
--- a/multipath/multipath.conf.5
+++ b/multipath/multipath.conf.5
@@ -756,9 +756,11 @@ The default is: \fB<unset>\fR
 .
 .TP
 .B all_tg_pt
-This must be set to \fByes\fR to successfully use mpathpersist on arrays that
-automatically set and clear registration keys on all target ports from a
-host, instead of per target port per host.
+Set the 'all targets ports' flag when registering keys with mpathpersist. Some
+arrays automatically set and clear registration keys on all target ports from a
+host, instead of per target port per host. The ALL_TG_PT flag must be set to
+successfully use mpathpersist on these arrays. Setting this option is identical
+to calling mpathpersist with \fI--param-alltgpt\fR
 .RS
 .TP
 The default is: \fBno\fR
-- 
2.7.4

[PATCH 05/32] libmutipath: remove unused IDE bus type

[Benjamin Marzinski]

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

diff --git a/libmultipath/structs.h b/libmultipath/structs.h
index ca14315..0a2623a 100644
--- a/libmultipath/structs.h
+++ b/libmultipath/structs.h
@@ -58,7 +58,6 @@ enum failback_mode {
 enum sysfs_buses {
 	SYSFS_BUS_UNDEF,
 	SYSFS_BUS_SCSI,
-	SYSFS_BUS_IDE,
 	SYSFS_BUS_CCW,
 	SYSFS_BUS_CCISS,
 	SYSFS_BUS_NVME,
-- 
2.7.4

[PATCH 06/32] multipathd: add new protocol path wildcard

[Benjamin Marzinski]

This patch adds a new path wildcard 'P', that will print the path's
protocol.  For scsi devices, it will additionally print the transport
protocol being used.

Reviewed-by: Martin Wilck <mwilck@suse.com>
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/print.c | 43 +++++++++++++++++++++++++++++++++++++++++++
 libmultipath/print.h |  2 ++
 2 files changed, 45 insertions(+)

diff --git a/libmultipath/print.c b/libmultipath/print.c
index 222d270..ecfcb48 100644
--- a/libmultipath/print.c
+++ b/libmultipath/print.c
@@ -638,6 +638,48 @@ snprint_path_failures(char * buff, size_t len, const struct path * pp)
 	return snprint_int(buff, len, pp->failcount);
 }
 
+/* if you add a protocol string bigger than "scsi:unspec" you must
+ * also change PROTOCOL_BUF_SIZE */
+int
+snprint_path_protocol(char * buff, size_t len, const struct path * pp)
+{
+	switch (pp->bus) {
+	case SYSFS_BUS_SCSI:
+		switch (pp->sg_id.proto_id) {
+		case SCSI_PROTOCOL_FCP:
+			return snprintf(buff, len, "scsi:fcp");
+		case SCSI_PROTOCOL_SPI:
+			return snprintf(buff, len, "scsi:spi");
+		case SCSI_PROTOCOL_SSA:
+			return snprintf(buff, len, "scsi:ssa");
+		case SCSI_PROTOCOL_SBP:
+			return snprintf(buff, len, "scsi:sbp");
+		case SCSI_PROTOCOL_SRP:
+			return snprintf(buff, len, "scsi:srp");
+		case SCSI_PROTOCOL_ISCSI:
+			return snprintf(buff, len, "scsi:iscsi");
+		case SCSI_PROTOCOL_SAS:
+			return snprintf(buff, len, "scsi:sas");
+		case SCSI_PROTOCOL_ADT:
+			return snprintf(buff, len, "scsi:adt");
+		case SCSI_PROTOCOL_ATA:
+			return snprintf(buff, len, "scsi:ata");
+		case SCSI_PROTOCOL_UNSPEC:
+		default:
+			return snprintf(buff, len, "scsi:unspec");
+		}
+	case SYSFS_BUS_CCW:
+		return snprintf(buff, len, "ccw");
+	case SYSFS_BUS_CCISS:
+		return snprintf(buff, len, "cciss");
+	case SYSFS_BUS_NVME:
+		return snprintf(buff, len, "nvme");
+	case SYSFS_BUS_UNDEF:
+	default:
+		return snprintf(buff, len, "undef");
+	}
+}
+
 struct multipath_data mpd[] = {
 	{'n', "name",          0, snprint_name},
 	{'w', "uuid",          0, snprint_multipath_uuid},
@@ -687,6 +729,7 @@ struct path_data pd[] = {
 	{'a', "host adapter",  0, snprint_host_adapter},
 	{'G', "foreign",       0, snprint_path_foreign},
 	{'0', "failures",      0, snprint_path_failures},
+	{'P', "protocol",      0, snprint_path_protocol},
 	{0, NULL, 0 , NULL}
 };
 
diff --git a/libmultipath/print.h b/libmultipath/print.h
index 608b7d5..e2fb865 100644
--- a/libmultipath/print.h
+++ b/libmultipath/print.h
@@ -133,6 +133,8 @@ int snprint_host_wwnn (char *, size_t, const struct path *);
 int snprint_host_wwpn (char *, size_t, const struct path *);
 int snprint_tgt_wwnn (char *, size_t, const struct path *);
 int snprint_tgt_wwpn (char *, size_t, const struct path *);
+#define PROTOCOL_BUF_SIZE sizeof("scsi:unspec")
+int snprint_path_protocol(char *, size_t, const struct path *);
 
 void _print_multipath_topology (const struct gen_multipath * gmp,
 				int verbosity);
-- 
2.7.4

[PATCH 07/32] libmultipath: add "protocol" blacklist option.

[Benjamin Marzinski]

Multiple users have requested an easy way to setup blacklists that do
things such as blacklisting all non FC and iSCSI devices. Currently
there is no easy way to do this, without knowing in advance what the
devices are.  Looking into the udev property values, I didn't see a
consistent set of values that would worked for all the different types
of requests like this (which would have allowed us to solve this by
extending the "property" blacklist option to allow comparing values,
instead of just keywords).

Instead I've opted to allow multipath to blacklist/whitelist devices
by the protocol strings printed by "multipathd: add new protocol path
wildcard". This check happens after multipath checks the "device"
keyword, and before it checks wwid. This gives users an easily
understandible method to set up these types of blacklists, without
needing to know the exact arrays being used.

Reviewed-by: Martin Wilck <mwilck@suse.com>
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/blacklist.c   | 51 ++++++++++++++++++++++++++++++++++++++--------
 libmultipath/blacklist.h   |  3 +++
 libmultipath/config.c      | 15 ++++++++++++++
 libmultipath/config.h      |  2 ++
 libmultipath/dict.c        | 14 +++++++++++--
 libmultipath/discovery.c   |  5 +++--
 libmultipath/print.c       | 31 ++++++++++++++++++++++++++++
 multipath/multipath.conf.5 | 16 +++++++++++++--
 8 files changed, 123 insertions(+), 14 deletions(-)

diff --git a/libmultipath/blacklist.c b/libmultipath/blacklist.c
index 361c603..fdd36f7 100644
--- a/libmultipath/blacklist.c
+++ b/libmultipath/blacklist.c
@@ -12,6 +12,8 @@
 #include "structs.h"
 #include "config.h"
 #include "blacklist.h"
+#include "structs_vec.h"
+#include "print.h"
 
 int store_ble(vector blist, char * str, int origin)
 {
@@ -240,12 +242,14 @@ setup_default_blist (struct config * conf)
 		condlog(3, "%s: %s %s %s", dev, (M), wwid, (S));	\
 	else if (env)							\
 		condlog(3, "%s: %s %s %s", dev, (M), env, (S));		\
+	else if (protocol)						\
+		condlog(3, "%s: %s %s %s", dev, (M), protocol, (S));	\
 	else								\
 		condlog(3, "%s: %s %s", dev, (M), (S))
 
 void
 log_filter (const char *dev, char *vendor, char *product, char *wwid,
-	    const char *env, int r)
+	    const char *env, const char *protocol, int r)
 {
 	/*
 	 * Try to sort from most likely to least.
@@ -265,6 +269,9 @@ log_filter (const char *dev, char *vendor, char *product, char *wwid,
 	case MATCH_PROPERTY_BLIST:
 		LOG_BLIST("udev property", "blacklisted");
 		break;
+	case MATCH_PROTOCOL_BLIST:
+		LOG_BLIST("protocol", "blacklisted");
+		break;
 	case MATCH_DEVICE_BLIST_EXCEPT:
 		LOG_BLIST("vendor/product", "whitelisted");
 		break;
@@ -280,6 +287,9 @@ log_filter (const char *dev, char *vendor, char *product, char *wwid,
 	case MATCH_PROPERTY_BLIST_MISSING:
 		LOG_BLIST("blacklisted,", "udev property missing");
 		break;
+	case MATCH_PROTOCOL_BLIST_EXCEPT:
+		LOG_BLIST("protocol", "whitelisted");
+		break;
 	}
 }
 
@@ -299,7 +309,7 @@ int
 filter_device (vector blist, vector elist, char * vendor, char * product)
 {
 	int r = _filter_device(blist, elist, vendor, product);
-	log_filter(NULL, vendor, product, NULL, NULL, r);
+	log_filter(NULL, vendor, product, NULL, NULL, NULL, r);
 	return r;
 }
 
@@ -319,7 +329,7 @@ int
 filter_devnode (vector blist, vector elist, char * dev)
 {
 	int r = _filter_devnode(blist, elist, dev);
-	log_filter(dev, NULL, NULL, NULL, NULL, r);
+	log_filter(dev, NULL, NULL, NULL, NULL, NULL, r);
 	return r;
 }
 
@@ -339,7 +349,29 @@ int
 filter_wwid (vector blist, vector elist, char * wwid, char * dev)
 {
 	int r = _filter_wwid(blist, elist, wwid);
-	log_filter(dev, NULL, NULL, wwid, NULL, r);
+	log_filter(dev, NULL, NULL, wwid, NULL, NULL, r);
+	return r;
+}
+
+static int
+_filter_protocol (vector blist, vector elist, const char * protocol_str)
+{
+	if (_blacklist_exceptions(elist, protocol_str))
+		return MATCH_PROTOCOL_BLIST_EXCEPT;
+	if (_blacklist(blist, protocol_str))
+		return MATCH_PROTOCOL_BLIST;
+	return 0;
+}
+
+int
+filter_protocol(vector blist, vector elist, struct path * pp)
+{
+	char buf[PROTOCOL_BUF_SIZE];
+	int r;
+
+	snprint_path_protocol(buf, sizeof(buf), pp);
+	r = _filter_protocol(blist, elist, buf);
+	log_filter(pp->dev, NULL, NULL, NULL, NULL, buf, r);
 	return r;
 }
 
@@ -351,7 +383,6 @@ _filter_path (struct config * conf, struct path * pp)
 	r = filter_property(conf, pp->udev);
 	if (r > 0)
 		return r;
-
 	r = _filter_devnode(conf->blist_devnode, conf->elist_devnode,pp->dev);
 	if (r > 0)
 		return r;
@@ -359,6 +390,9 @@ _filter_path (struct config * conf, struct path * pp)
 			   pp->vendor_id, pp->product_id);
 	if (r > 0)
 		return r;
+	r = filter_protocol(conf->blist_protocol, conf->elist_protocol, pp);
+	if (r > 0)
+		return r;
 	r = _filter_wwid(conf->blist_wwid, conf->elist_wwid, pp->wwid);
 	return r;
 }
@@ -367,7 +401,8 @@ int
 filter_path (struct config * conf, struct path * pp)
 {
 	int r=_filter_path(conf, pp);
-	log_filter(pp->dev, pp->vendor_id, pp->product_id, pp->wwid, NULL, r);
+	log_filter(pp->dev, pp->vendor_id, pp->product_id, pp->wwid, NULL,
+		   NULL, r);
 	return r;
 }
 
@@ -402,7 +437,7 @@ filter_property(struct config * conf, struct udev_device * udev)
 
 		r = _filter_property(conf, env);
 		if (r) {
-			log_filter(devname, NULL, NULL, NULL, env, r);
+			log_filter(devname, NULL, NULL, NULL, env, NULL, r);
 			return r;
 		}
 	}
@@ -411,7 +446,7 @@ filter_property(struct config * conf, struct udev_device * udev)
 	 * This is the inverse of the 'normal' matching;
 	 * the environment variable _has_ to match.
 	 */
-	log_filter(devname, NULL, NULL, NULL, NULL,
+	log_filter(devname, NULL, NULL, NULL, NULL, NULL,
 		   MATCH_PROPERTY_BLIST_MISSING);
 	return MATCH_PROPERTY_BLIST_MISSING;
 }
diff --git a/libmultipath/blacklist.h b/libmultipath/blacklist.h
index 0b028d4..f7beef2 100644
--- a/libmultipath/blacklist.h
+++ b/libmultipath/blacklist.h
@@ -10,10 +10,12 @@
 #define MATCH_DEVNODE_BLIST  3
 #define MATCH_PROPERTY_BLIST 4
 #define MATCH_PROPERTY_BLIST_MISSING 5
+#define MATCH_PROTOCOL_BLIST 6
 #define MATCH_WWID_BLIST_EXCEPT     -MATCH_WWID_BLIST
 #define MATCH_DEVICE_BLIST_EXCEPT   -MATCH_DEVICE_BLIST
 #define MATCH_DEVNODE_BLIST_EXCEPT  -MATCH_DEVNODE_BLIST
 #define MATCH_PROPERTY_BLIST_EXCEPT -MATCH_PROPERTY_BLIST
+#define MATCH_PROTOCOL_BLIST_EXCEPT -MATCH_PROTOCOL_BLIST
 
 struct blentry {
 	char * str;
@@ -36,6 +38,7 @@ int filter_wwid (vector, vector, char *, char *);
 int filter_device (vector, vector, char *, char *);
 int filter_path (struct config *, struct path *);
 int filter_property(struct config *, struct udev_device *);
+int filter_protocol(vector, vector, struct path *);
 int store_ble (vector, char *, int);
 int set_ble_device (vector, char *, char *, int);
 void free_blacklist (vector);
diff --git a/libmultipath/config.c b/libmultipath/config.c
index afa309d..0aef186 100644
--- a/libmultipath/config.c
+++ b/libmultipath/config.c
@@ -623,11 +623,13 @@ free_config (struct config * conf)
 	free_blacklist(conf->blist_devnode);
 	free_blacklist(conf->blist_wwid);
 	free_blacklist(conf->blist_property);
+	free_blacklist(conf->blist_protocol);
 	free_blacklist_device(conf->blist_device);
 
 	free_blacklist(conf->elist_devnode);
 	free_blacklist(conf->elist_wwid);
 	free_blacklist(conf->elist_property);
+	free_blacklist(conf->elist_protocol);
 	free_blacklist_device(conf->elist_device);
 
 	free_mptable(conf->mptable);
@@ -780,6 +782,12 @@ load_config (char * file)
 		if (!conf->blist_property)
 			goto out;
 	}
+	if (conf->blist_protocol == NULL) {
+		conf->blist_protocol = vector_alloc();
+
+		if (!conf->blist_protocol)
+			goto out;
+	}
 
 	if (conf->elist_devnode == NULL) {
 		conf->elist_devnode = vector_alloc();
@@ -807,6 +815,13 @@ load_config (char * file)
 		if (!conf->elist_property)
 			goto out;
 	}
+	if (conf->elist_protocol == NULL) {
+		conf->elist_protocol = vector_alloc();
+
+		if (!conf->elist_protocol)
+			goto out;
+	}
+
 	if (setup_default_blist(conf))
 		goto out;
 
diff --git a/libmultipath/config.h b/libmultipath/config.h
index 6bd42f0..7d0cd9a 100644
--- a/libmultipath/config.h
+++ b/libmultipath/config.h
@@ -210,10 +210,12 @@ struct config {
 	vector blist_wwid;
 	vector blist_device;
 	vector blist_property;
+	vector blist_protocol;
 	vector elist_devnode;
 	vector elist_wwid;
 	vector elist_device;
 	vector elist_property;
+	vector elist_protocol;
 };
 
 extern struct udev * udev;
diff --git a/libmultipath/dict.c b/libmultipath/dict.c
index 15e7582..32524d5 100644
--- a/libmultipath/dict.c
+++ b/libmultipath/dict.c
@@ -1291,9 +1291,12 @@ blacklist_handler(struct config *conf, vector strvec)
 		conf->blist_device = vector_alloc();
 	if (!conf->blist_property)
 		conf->blist_property = vector_alloc();
+	if (!conf->blist_protocol)
+		conf->blist_protocol = vector_alloc();
 
 	if (!conf->blist_devnode || !conf->blist_wwid ||
-	    !conf->blist_device || !conf->blist_property)
+	    !conf->blist_device || !conf->blist_property ||
+	    !conf->blist_protocol)
 		return 1;
 
 	return 0;
@@ -1310,9 +1313,12 @@ blacklist_exceptions_handler(struct config *conf, vector strvec)
 		conf->elist_device = vector_alloc();
 	if (!conf->elist_property)
 		conf->elist_property = vector_alloc();
+	if (!conf->elist_protocol)
+		conf->elist_protocol = vector_alloc();
 
 	if (!conf->elist_devnode || !conf->elist_wwid ||
-	    !conf->elist_device || !conf->elist_property)
+	    !conf->elist_device || !conf->elist_property ||
+	    !conf->elist_protocol)
 		return 1;
 
 	return 0;
@@ -1356,6 +1362,8 @@ declare_ble_handler(blist_wwid)
 declare_ble_handler(elist_wwid)
 declare_ble_handler(blist_property)
 declare_ble_handler(elist_property)
+declare_ble_handler(blist_protocol)
+declare_ble_handler(elist_protocol)
 
 static int
 snprint_def_uxsock_timeout(struct config *conf, char * buff, int len,
@@ -1627,6 +1635,7 @@ init_keywords(vector keywords)
 	install_keyword_multi("devnode", &ble_blist_devnode_handler, &snprint_ble_simple);
 	install_keyword_multi("wwid", &ble_blist_wwid_handler, &snprint_ble_simple);
 	install_keyword_multi("property", &ble_blist_property_handler, &snprint_ble_simple);
+	install_keyword_multi("protocol", &ble_blist_protocol_handler, &snprint_ble_simple);
 	install_keyword_multi("device", &ble_device_handler, NULL);
 	install_sublevel();
 	install_keyword("vendor", &ble_blist_device_vendor_handler, &snprint_bled_vendor);
@@ -1636,6 +1645,7 @@ init_keywords(vector keywords)
 	install_keyword_multi("devnode", &ble_elist_devnode_handler, &snprint_ble_simple);
 	install_keyword_multi("wwid", &ble_elist_wwid_handler, &snprint_ble_simple);
 	install_keyword_multi("property", &ble_elist_property_handler, &snprint_ble_simple);
+	install_keyword_multi("protocol", &ble_elist_protocol_handler, &snprint_ble_simple);
 	install_keyword_multi("device", &ble_except_device_handler, NULL);
 	install_sublevel();
 	install_keyword("vendor", &ble_elist_device_vendor_handler, &snprint_bled_vendor);
diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index 573d98b..e58a3fa 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -1887,9 +1887,10 @@ int pathinfo(struct path *pp, struct config *conf, int mask)
 
 	if (mask & DI_BLACKLIST && mask & DI_SYSFS) {
 		if (filter_device(conf->blist_device, conf->elist_device,
-				  pp->vendor_id, pp->product_id) > 0) {
+				  pp->vendor_id, pp->product_id) > 0 ||
+		    filter_protocol(conf->blist_protocol, conf->elist_protocol,
+				    pp) > 0)
 			return PATHINFO_SKIPPED;
-		}
 	}
 
 	path_state = path_offline(pp);
diff --git a/libmultipath/print.c b/libmultipath/print.c
index ecfcb48..9da6a77 100644
--- a/libmultipath/print.c
+++ b/libmultipath/print.c
@@ -1688,6 +1688,19 @@ int snprint_blacklist_report(struct config *conf, char *buff, int len)
 
 	if ((len - fwd - threshold) <= 0)
 		return len;
+	fwd += snprintf(buff + fwd, len - fwd, "protocol rules:\n"
+					       "- blacklist:\n");
+	if (!snprint_blacklist_group(buff, len, &fwd, &conf->blist_protocol))
+		return len;
+
+	if ((len - fwd - threshold) <= 0)
+		return len;
+	fwd += snprintf(buff + fwd, len - fwd, "- exceptions:\n");
+	if (snprint_blacklist_group(buff, len, &fwd, &conf->elist_protocol) == 0)
+		return len;
+
+	if ((len - fwd - threshold) <= 0)
+		return len;
 	fwd += snprintf(buff + fwd, len - fwd, "wwid rules:\n"
 					       "- blacklist:\n");
 	if (snprint_blacklist_group(buff, len, &fwd, &conf->blist_wwid) == 0)
@@ -1761,6 +1774,15 @@ static int snprint_blacklist(const struct config *conf, char *buff, int len)
 		if (fwd >= len)
 			return len;
 	}
+	vector_foreach_slot (conf->blist_protocol, ble, i) {
+		kw = find_keyword(conf->keywords, rootkw->sub, "protocol");
+		if (!kw)
+			return 0;
+		fwd += snprint_keyword(buff + fwd, len - fwd, "\t%k %v\n",
+				       kw, ble);
+		if (fwd >= len)
+			return len;
+	}
 	rootkw = find_keyword(conf->keywords, rootkw->sub, "device");
 	if (!rootkw)
 		return 0;
@@ -1838,6 +1860,15 @@ static int snprint_blacklist_except(const struct config *conf,
 		if (fwd >= len)
 			return len;
 	}
+	vector_foreach_slot (conf->elist_protocol, ele, i) {
+		kw = find_keyword(conf->keywords, rootkw->sub, "protocol");
+		if (!kw)
+			return 0;
+		fwd += snprint_keyword(buff + fwd, len - fwd, "\t%k %v\n",
+				       kw, ele);
+		if (fwd >= len)
+			return len;
+	}
 	rootkw = find_keyword(conf->keywords, rootkw->sub, "device");
 	if (!rootkw)
 		return 0;
diff --git a/multipath/multipath.conf.5 b/multipath/multipath.conf.5
index fb863fd..6333366 100644
--- a/multipath/multipath.conf.5
+++ b/multipath/multipath.conf.5
@@ -1205,9 +1205,21 @@ The default \fIblacklist exception\fR is: \fB(SCSI_IDENT_|ID_WWN)\fR, causing
 well-behaved SCSI devices and devices that provide a WWN (World Wide Number)
 to be included, and all others to be excluded.
 .RE
+.TP
+.B protocol
+Regular expression for the protocol of a device to be excluded/included.
+.RS
+.PP
+The protocol strings that multipath recognizes are \fIscsi:fcp\fR,
+\fIscsi:spi\fR, \fIscsi:ssa\fR, \fIscsi:sbp\fR, \fIscsi:srp\fR,
+\fIscsi:iscsi\fR, \fIscsi:sas\fR, \fIscsi:adt\fR, \fIscsi:ata\fR,
+\fIscsi:unspec\fR, \fIccw\fR, \fIcciss\fR, \fInvme\fR, and \fIundef\fR.
+The protocol that a path is using can be viewed by running
+\fBmultipathd show paths format "%d %P"\fR
+.RE
 .LP
-For every device, these 4 blacklist criteria are evaluated in the the order
-"property, dev\%node, device, wwid". If a device turns out to be
+For every device, these 5 blacklist criteria are evaluated in the the order
+"property, dev\%node, device, protocol, wwid". If a device turns out to be
 blacklisted by any criterion, it's excluded from handling by multipathd, and
 the later criteria aren't evaluated any more. For each
 criterion, the whitelist takes precedence over the blacklist if a device
-- 
2.7.4

[PATCH 08/32] libmultipath: remove _filter_* blacklist functions

[Benjamin Marzinski]

The one point of these functions was for _filter_path(), and that wasn't
improved by using them. Since filter_path() only printed one message at
the end, you could have situations where the wwid was blacklisted, but
the blacklist message included the vendor/product instead. Also, the
protocol and property messages were printed twice, and if the device was
on multiple whitelists, only the last one is printed.

Reviewed-by: Martin Wilck <mwilck@suse.com>
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/blacklist.c | 168 +++++++++++++++++++----------------------------
 libmultipath/blacklist.h |   2 +-
 libmultipath/configure.c |   2 +-
 libmultipath/discovery.c |   2 +-
 4 files changed, 71 insertions(+), 103 deletions(-)

diff --git a/libmultipath/blacklist.c b/libmultipath/blacklist.c
index fdd36f7..318ec03 100644
--- a/libmultipath/blacklist.c
+++ b/libmultipath/blacklist.c
@@ -294,161 +294,129 @@ log_filter (const char *dev, char *vendor, char *product, char *wwid,
 }
 
 int
-_filter_device (vector blist, vector elist, char * vendor, char * product)
+filter_device (vector blist, vector elist, char * vendor, char * product,
+	       char * dev)
 {
-	if (!vendor || !product)
-		return 0;
-	if (_blacklist_exceptions_device(elist, vendor, product))
-		return MATCH_DEVICE_BLIST_EXCEPT;
-	if (_blacklist_device(blist, vendor, product))
-		return MATCH_DEVICE_BLIST;
-	return 0;
-}
+	int r = MATCH_NOTHING;
 
-int
-filter_device (vector blist, vector elist, char * vendor, char * product)
-{
-	int r = _filter_device(blist, elist, vendor, product);
-	log_filter(NULL, vendor, product, NULL, NULL, NULL, r);
-	return r;
-}
+	if (vendor && product) {
+		if (_blacklist_exceptions_device(elist, vendor, product))
+			r = MATCH_DEVICE_BLIST_EXCEPT;
+		else if (_blacklist_device(blist, vendor, product))
+			r = MATCH_DEVICE_BLIST;
+	}
 
-int
-_filter_devnode (vector blist, vector elist, char * dev)
-{
-	if (!dev)
-		return 0;
-	if (_blacklist_exceptions(elist, dev))
-		return MATCH_DEVNODE_BLIST_EXCEPT;
-	if (_blacklist(blist, dev))
-		return MATCH_DEVNODE_BLIST;
-	return 0;
+	log_filter(dev, vendor, product, NULL, NULL, NULL, r);
+	return r;
 }
 
 int
 filter_devnode (vector blist, vector elist, char * dev)
 {
-	int r = _filter_devnode(blist, elist, dev);
+	int r = MATCH_NOTHING;
+
+	if (dev) {
+		if (_blacklist_exceptions(elist, dev))
+			r = MATCH_DEVNODE_BLIST_EXCEPT;
+		else if (_blacklist(blist, dev))
+			r = MATCH_DEVNODE_BLIST;
+	}
+
 	log_filter(dev, NULL, NULL, NULL, NULL, NULL, r);
 	return r;
 }
 
 int
-_filter_wwid (vector blist, vector elist, char * wwid)
-{
-	if (!wwid)
-		return 0;
-	if (_blacklist_exceptions(elist, wwid))
-		return MATCH_WWID_BLIST_EXCEPT;
-	if (_blacklist(blist, wwid))
-		return MATCH_WWID_BLIST;
-	return 0;
-}
-
-int
 filter_wwid (vector blist, vector elist, char * wwid, char * dev)
 {
-	int r = _filter_wwid(blist, elist, wwid);
+	int r = MATCH_NOTHING;
+
+	if (wwid) {
+		if (_blacklist_exceptions(elist, wwid))
+			r = MATCH_WWID_BLIST_EXCEPT;
+		else if (_blacklist(blist, wwid))
+			r = MATCH_WWID_BLIST;
+	}
+
 	log_filter(dev, NULL, NULL, wwid, NULL, NULL, r);
 	return r;
 }
 
-static int
-_filter_protocol (vector blist, vector elist, const char * protocol_str)
-{
-	if (_blacklist_exceptions(elist, protocol_str))
-		return MATCH_PROTOCOL_BLIST_EXCEPT;
-	if (_blacklist(blist, protocol_str))
-		return MATCH_PROTOCOL_BLIST;
-	return 0;
-}
-
 int
 filter_protocol(vector blist, vector elist, struct path * pp)
 {
 	char buf[PROTOCOL_BUF_SIZE];
-	int r;
+	int r = MATCH_NOTHING;
+
+	if (pp) {
+		snprint_path_protocol(buf, sizeof(buf), pp);
+
+		if (_blacklist_exceptions(elist, buf))
+			r = MATCH_PROTOCOL_BLIST_EXCEPT;
+		else if (_blacklist(blist, buf))
+			r = MATCH_PROTOCOL_BLIST;
+	}
 
-	snprint_path_protocol(buf, sizeof(buf), pp);
-	r = _filter_protocol(blist, elist, buf);
 	log_filter(pp->dev, NULL, NULL, NULL, NULL, buf, r);
 	return r;
 }
 
 int
-_filter_path (struct config * conf, struct path * pp)
+filter_path (struct config * conf, struct path * pp)
 {
 	int r;
 
 	r = filter_property(conf, pp->udev);
 	if (r > 0)
 		return r;
-	r = _filter_devnode(conf->blist_devnode, conf->elist_devnode,pp->dev);
+	r = filter_devnode(conf->blist_devnode, conf->elist_devnode, pp->dev);
 	if (r > 0)
 		return r;
-	r = _filter_device(conf->blist_device, conf->elist_device,
-			   pp->vendor_id, pp->product_id);
+	r = filter_device(conf->blist_device, conf->elist_device,
+			   pp->vendor_id, pp->product_id, pp->dev);
 	if (r > 0)
 		return r;
 	r = filter_protocol(conf->blist_protocol, conf->elist_protocol, pp);
 	if (r > 0)
 		return r;
-	r = _filter_wwid(conf->blist_wwid, conf->elist_wwid, pp->wwid);
+	r = filter_wwid(conf->blist_wwid, conf->elist_wwid, pp->wwid, pp->dev);
 	return r;
 }
 
 int
-filter_path (struct config * conf, struct path * pp)
-{
-	int r=_filter_path(conf, pp);
-	log_filter(pp->dev, pp->vendor_id, pp->product_id, pp->wwid, NULL,
-		   NULL, r);
-	return r;
-}
-
-int
-_filter_property (struct config *conf, const char *env)
-{
-	if (_blacklist_exceptions(conf->elist_property, env))
-		return MATCH_PROPERTY_BLIST_EXCEPT;
-	if (_blacklist(conf->blist_property, env))
-		return MATCH_PROPERTY_BLIST;
-
-	return 0;
-}
-
-int
 filter_property(struct config * conf, struct udev_device * udev)
 {
 	const char *devname = udev_device_get_sysname(udev);
 	struct udev_list_entry *list_entry;
-	int r;
-
-	if (!udev)
-		return 0;
-
-	udev_list_entry_foreach(list_entry,
+	const char *env = NULL;
+	int r = MATCH_NOTHING;
+
+	if (udev) {
+		/*
+		 * This is the inverse of the 'normal' matching;
+		 * the environment variable _has_ to match.
+		 */
+		r = MATCH_PROPERTY_BLIST_MISSING;
+		udev_list_entry_foreach(list_entry,
 				udev_device_get_properties_list_entry(udev)) {
-		const char *env;
-
-		env = udev_list_entry_get_name(list_entry);
-		if (!env)
-			continue;
 
-		r = _filter_property(conf, env);
-		if (r) {
-			log_filter(devname, NULL, NULL, NULL, env, NULL, r);
-			return r;
+			env = udev_list_entry_get_name(list_entry);
+			if (!env)
+				continue;
+			if (_blacklist_exceptions(conf->elist_property, env)) {
+				r = MATCH_PROPERTY_BLIST_EXCEPT;
+				break;
+			}
+			if (_blacklist(conf->blist_property, env)) {
+				r = MATCH_PROPERTY_BLIST;
+				break;
+			}
+			env = NULL;
 		}
 	}
 
-	/*
-	 * This is the inverse of the 'normal' matching;
-	 * the environment variable _has_ to match.
-	 */
-	log_filter(devname, NULL, NULL, NULL, NULL, NULL,
-		   MATCH_PROPERTY_BLIST_MISSING);
-	return MATCH_PROPERTY_BLIST_MISSING;
+	log_filter(devname, NULL, NULL, NULL, env, NULL, r);
+	return r;
 }
 
 static void free_ble(struct blentry *ble)
diff --git a/libmultipath/blacklist.h b/libmultipath/blacklist.h
index f7beef2..18903b6 100644
--- a/libmultipath/blacklist.h
+++ b/libmultipath/blacklist.h
@@ -35,7 +35,7 @@ int setup_default_blist (struct config *);
 int alloc_ble_device (vector);
 int filter_devnode (vector, vector, char *);
 int filter_wwid (vector, vector, char *, char *);
-int filter_device (vector, vector, char *, char *);
+int filter_device (vector, vector, char *, char *, char *);
 int filter_path (struct config *, struct path *);
 int filter_property(struct config *, struct udev_device *);
 int filter_protocol(vector, vector, struct path *);
diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index 5c54f9b..09c3dcf 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -1030,7 +1030,7 @@ int coalesce_paths (struct vectors * vecs, vector newmp, char * refwwid,
 			invalid = 1;
 		pthread_cleanup_pop(1);
 		if (invalid) {
-			orphan_path(pp1, "wwid blacklisted");
+			orphan_path(pp1, "blacklisted");
 			continue;
 		}
 
diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index e58a3fa..0b1855d 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -1887,7 +1887,7 @@ int pathinfo(struct path *pp, struct config *conf, int mask)
 
 	if (mask & DI_BLACKLIST && mask & DI_SYSFS) {
 		if (filter_device(conf->blist_device, conf->elist_device,
-				  pp->vendor_id, pp->product_id) > 0 ||
+				  pp->vendor_id, pp->product_id, pp->dev) > 0 ||
 		    filter_protocol(conf->blist_protocol, conf->elist_protocol,
 				    pp) > 0)
 			return PATHINFO_SKIPPED;
-- 
2.7.4

[PATCH 09/32] multipath tests: change to work with old make versions

[Benjamin Marzinski]

the $(file <) operation only works with make 4.2 and above. I tried
running the tests on an old machine and it failed. The $shell function
can do the same thing and multipath has been using that in its
Makefiles for a while.

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

diff --git a/tests/Makefile b/tests/Makefile
index 78755ed..d293c87 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -51,4 +51,4 @@ COLON:=:
 		$(multipathdir)/libmultipath.so Makefile
 	$(CC) $(CFLAGS) -o $@ $(LDFLAGS) $< $($@_TESTDEPS) $($@_OBJDEPS) \
 		$(LIBDEPS) $($@_LIBDEPS) \
-		$(file <$<.wrap) $(foreach dep,$($@_TESTDEPS),$(file <$(dep).wrap))
+		$(shell cat $<.wrap) $(foreach dep,$($@_TESTDEPS),$(shell cat $(dep).wrap))
-- 
2.7.4

[PATCH 10/32] multipath tests: add blacklist tests

[Benjamin Marzinski]

These are tests to validate the filter_* blacklist functions. They not
only verify that the device is correctly blacklisted/whitelisted, but
they also verify the log messages that are printed out.

Reviewed-by: Martin Wilck <mwilck@suse.com>
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 tests/Makefile    |   4 +-
 tests/blacklist.c | 512 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 515 insertions(+), 1 deletion(-)
 create mode 100644 tests/blacklist.c

diff --git a/tests/Makefile b/tests/Makefile
index d293c87..98b5c93 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -3,7 +3,7 @@ include ../Makefile.inc
 CFLAGS += $(BIN_CFLAGS) -I$(multipathdir) -I$(mpathcmddir)
 LIBDEPS += -L$(multipathdir) -lmultipath -lcmocka
 
-TESTS := uevent parser util dmevents hwtable
+TESTS := uevent parser util dmevents hwtable blacklist
 
 .SILENT: $(TESTS:%=%.o)
 .PRECIOUS: $(TESTS:%=%-test)
@@ -23,6 +23,8 @@ hwtable-test_TESTDEPS := test-lib.o
 hwtable-test_OBJDEPS := ../libmultipath/discovery.o ../libmultipath/blacklist.o \
 	../libmultipath/prio.o ../libmultipath/callout.o ../libmultipath/structs.o
 hwtable-test_LIBDEPS := -ludev -lpthread -ldl
+blacklist-test_OBJDEPS := ../libmultipath/blacklist.o
+blacklist-test_LIBDEPS := -ludev
 
 %.out:	%-test
 	@echo == running $< ==
diff --git a/tests/blacklist.c b/tests/blacklist.c
new file mode 100644
index 0000000..a55c1c0
--- /dev/null
+++ b/tests/blacklist.c
@@ -0,0 +1,512 @@
+/*
+ * Copyright (c) 2018 Benjamin Marzinski, Redhat
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <https://www.gnu.org/licenses/>.
+ *
+ */
+#include <stdarg.h>
+#include <stddef.h>
+#include <setjmp.h>
+#include <cmocka.h>
+#include "globals.c"
+#include "blacklist.h"
+#include "log.h"
+
+struct udev_device {
+	const char *sysname;
+	char *property_list[];
+};
+
+const char *
+__wrap_udev_device_get_sysname(struct udev_device *udev_device)
+{
+	assert_non_null(udev_device);
+	assert_non_null(udev_device->sysname);
+	return udev_device->sysname;
+}
+
+struct udev_list_entry *
+__wrap_udev_device_get_properties_list_entry(struct udev_device *udev_device)
+{
+	assert_non_null(udev_device);
+	if (!udev_device->property_list)
+		return NULL;
+	if (!*udev_device->property_list)
+		return NULL;
+	return (struct udev_list_entry *)udev_device->property_list;
+}
+
+struct udev_list_entry *
+__wrap_udev_list_entry_get_next(struct udev_list_entry *list_entry)
+{
+	assert_non_null(list_entry);
+	if (!*((char **)list_entry + 1))
+		return NULL;
+	return (struct udev_list_entry *)(((char **)list_entry) + 1);
+}
+
+const char *
+__wrap_udev_list_entry_get_name(struct udev_list_entry *list_entry)
+{
+	return *(const char **)list_entry;
+}
+
+void __wrap_dlog (int sink, int prio, const char * fmt, ...)
+{
+	char buff[MAX_MSG_SIZE];
+	va_list ap;
+
+	assert_int_equal(prio, mock_type(int));
+	va_start(ap, fmt);
+	vsnprintf(buff, MAX_MSG_SIZE, fmt, ap);
+	va_end(ap);
+	assert_string_equal(buff, mock_ptr_type(char *));
+}
+
+void expect_condlog(int prio, char *string)
+{
+	will_return(__wrap_dlog, prio);
+	will_return(__wrap_dlog, string);
+}
+
+vector blist_devnode_sdb;
+vector blist_all;
+vector blist_device_foo_bar;
+vector blist_device_all;
+vector blist_wwid_xyzzy;
+vector blist_protocol_fcp;
+vector blist_property_wwn;
+
+static int setup(void **state)
+{
+	blist_devnode_sdb = vector_alloc();
+	if (!blist_devnode_sdb ||
+	    store_ble(blist_devnode_sdb, strdup("sdb"), ORIGIN_CONFIG))
+		return -1;
+
+	blist_all = vector_alloc();
+	if (!blist_all || store_ble(blist_all, strdup(".*"), ORIGIN_CONFIG))
+		return -1;
+
+	blist_device_foo_bar = vector_alloc();
+	if (!blist_device_foo_bar || alloc_ble_device(blist_device_foo_bar) ||
+	    set_ble_device(blist_device_foo_bar, strdup("foo"), strdup("bar"),
+			   ORIGIN_CONFIG))
+		return -1;
+
+	blist_device_all = vector_alloc();
+	if (!blist_device_all || alloc_ble_device(blist_device_all) ||
+	    set_ble_device(blist_device_all, strdup(".*"), strdup(".*"),
+			   ORIGIN_CONFIG))
+		return -1;
+
+	blist_wwid_xyzzy = vector_alloc();
+	if (!blist_wwid_xyzzy ||
+	    store_ble(blist_wwid_xyzzy, strdup("xyzzy"), ORIGIN_CONFIG))
+		return -1;
+
+	blist_protocol_fcp = vector_alloc();
+	if (!blist_protocol_fcp ||
+	    store_ble(blist_protocol_fcp, strdup("scsi:fcp"), ORIGIN_CONFIG))
+		return -1;
+
+	blist_property_wwn = vector_alloc();
+	if (!blist_property_wwn ||
+	    store_ble(blist_property_wwn, strdup("ID_WWN"), ORIGIN_CONFIG))
+		return -1;
+
+	return 0;
+}
+
+static int teardown(void **state)
+{
+	free_blacklist(blist_devnode_sdb);
+	free_blacklist(blist_all);
+	free_blacklist_device(blist_device_foo_bar);
+	free_blacklist_device(blist_device_all);
+	free_blacklist(blist_wwid_xyzzy);
+	free_blacklist(blist_protocol_fcp);
+	free_blacklist(blist_property_wwn);
+	return 0;
+}
+
+static int reset_blists(void **state)
+{
+	conf.blist_devnode = NULL;
+	conf.blist_wwid = NULL;
+	conf.blist_property = NULL;
+	conf.blist_protocol = NULL;
+	conf.blist_device = NULL;
+	conf.elist_devnode = NULL;
+	conf.elist_wwid = NULL;
+	conf.elist_property = NULL;
+	conf.elist_protocol = NULL;
+	conf.elist_device = NULL;
+	return 0;
+}
+
+static void test_devnode_blacklist(void **state)
+{
+	expect_condlog(3, "sdb: device node name blacklisted\n");
+	assert_int_equal(filter_devnode(blist_devnode_sdb, NULL, "sdb"),
+			 MATCH_DEVNODE_BLIST);
+}
+
+static void test_devnode_whitelist(void **state)
+{
+	expect_condlog(3, "sdb: device node name whitelisted\n");
+	assert_int_equal(filter_devnode(blist_all, blist_devnode_sdb, "sdb"),
+			 MATCH_DEVNODE_BLIST_EXCEPT);
+	expect_condlog(3, "sdc: device node name blacklisted\n");
+	assert_int_equal(filter_devnode(blist_all, blist_devnode_sdb, "sdc"),
+			 MATCH_DEVNODE_BLIST);
+}
+
+static void test_devnode_missing(void **state)
+{
+	assert_int_equal(filter_devnode(blist_devnode_sdb, NULL, "sdc"),
+			 MATCH_NOTHING);
+}
+
+static void test_device_blacklist(void **state)
+{
+	expect_condlog(3, "sdb: (foo:bar) vendor/product blacklisted\n");
+	assert_int_equal(filter_device(blist_device_foo_bar, NULL, "foo",
+				       "bar", "sdb"),
+			 MATCH_DEVICE_BLIST);
+}
+
+static void test_device_whitelist(void **state)
+{
+	expect_condlog(3, "sdb: (foo:bar) vendor/product whitelisted\n");
+	assert_int_equal(filter_device(blist_device_all, blist_device_foo_bar,
+				       "foo", "bar", "sdb"),
+			 MATCH_DEVICE_BLIST_EXCEPT);
+	expect_condlog(3, "sdb: (foo:baz) vendor/product blacklisted\n");
+	assert_int_equal(filter_device(blist_device_all, blist_device_foo_bar,
+				       "foo", "baz", "sdb"),
+			 MATCH_DEVICE_BLIST);
+}
+
+static void test_device_missing(void **state)
+{
+	assert_int_equal(filter_device(blist_device_foo_bar, NULL, "foo",
+				       "baz", "sdb"),
+			 MATCH_NOTHING);
+}
+
+static void test_wwid_blacklist(void **state)
+{
+	expect_condlog(3, "sdb: wwid xyzzy blacklisted\n");
+	assert_int_equal(filter_wwid(blist_wwid_xyzzy, NULL, "xyzzy", "sdb"),
+			 MATCH_WWID_BLIST);
+}
+
+static void test_wwid_whitelist(void **state)
+{
+	expect_condlog(3, "sdb: wwid xyzzy whitelisted\n");
+	assert_int_equal(filter_wwid(blist_all, blist_wwid_xyzzy,
+				     "xyzzy", "sdb"),
+			 MATCH_WWID_BLIST_EXCEPT);
+	expect_condlog(3, "sdb: wwid plugh blacklisted\n");
+	assert_int_equal(filter_wwid(blist_all, blist_wwid_xyzzy,
+				     "plugh", "sdb"),
+			 MATCH_WWID_BLIST);
+}
+
+static void test_wwid_missing(void **state)
+{
+	assert_int_equal(filter_wwid(blist_wwid_xyzzy, NULL, "plugh", "sdb"),
+			 MATCH_NOTHING);
+}
+
+static void test_protocol_blacklist(void **state)
+{
+	struct path pp = { .dev = "sdb", .bus = SYSFS_BUS_SCSI,
+			   .sg_id.proto_id = SCSI_PROTOCOL_FCP };
+	expect_condlog(3, "sdb: protocol scsi:fcp blacklisted\n");
+	assert_int_equal(filter_protocol(blist_protocol_fcp, NULL, &pp),
+			 MATCH_PROTOCOL_BLIST);
+}
+
+static void test_protocol_whitelist(void **state)
+{
+	struct path pp1 = { .dev = "sdb", .bus = SYSFS_BUS_SCSI,
+			   .sg_id.proto_id = SCSI_PROTOCOL_FCP };
+	struct path pp2 = { .dev = "sdb", .bus = SYSFS_BUS_SCSI,
+			   .sg_id.proto_id = SCSI_PROTOCOL_ISCSI };
+	expect_condlog(3, "sdb: protocol scsi:fcp whitelisted\n");
+	assert_int_equal(filter_protocol(blist_all, blist_protocol_fcp, &pp1),
+			 MATCH_PROTOCOL_BLIST_EXCEPT);
+	expect_condlog(3, "sdb: protocol scsi:iscsi blacklisted\n");
+	assert_int_equal(filter_protocol(blist_all, blist_protocol_fcp, &pp2),
+			 MATCH_PROTOCOL_BLIST);
+}
+
+static void test_protocol_missing(void **state)
+{
+	struct path pp = { .dev = "sdb", .bus = SYSFS_BUS_SCSI,
+			   .sg_id.proto_id = SCSI_PROTOCOL_ISCSI };
+	assert_int_equal(filter_protocol(blist_protocol_fcp, NULL, &pp),
+			 MATCH_NOTHING);
+}
+
+static void test_property_blacklist(void **state)
+{
+	static struct udev_device udev = { "sdb", { "ID_FOO", "ID_WWN", "ID_BAR", NULL } };
+	conf.blist_property = blist_property_wwn;
+	expect_condlog(3, "sdb: udev property ID_WWN blacklisted\n");
+	assert_int_equal(filter_property(&conf, &udev), MATCH_PROPERTY_BLIST);
+}
+
+/* the property check works different in that you check all the property
+ * names, so setting a blacklist value will blacklist the device if any
+ * of the property on the blacklist are found before the property names
+ * in the whitelist.  This might be worth changing. although it would
+ * force multipath to go through the properties twice */
+static void test_property_whitelist(void **state)
+{
+	static struct udev_device udev = { "sdb", { "ID_FOO", "ID_WWN", "ID_BAR", NULL } };
+	conf.elist_property = blist_property_wwn;
+	expect_condlog(3, "sdb: udev property ID_WWN whitelisted\n");
+	assert_int_equal(filter_property(&conf, &udev),
+			 MATCH_PROPERTY_BLIST_EXCEPT);
+}
+
+static void test_property_missing(void **state)
+{
+	static struct udev_device udev = { "sdb", { "ID_FOO", "ID_BAZ", "ID_BAR", NULL } };
+	conf.blist_property = blist_property_wwn;
+	expect_condlog(3, "sdb: blacklisted, udev property missing\n");
+	assert_int_equal(filter_property(&conf, &udev),
+			 MATCH_PROPERTY_BLIST_MISSING);
+}
+
+struct udev_device test_udev = { "sdb", { "ID_FOO", "ID_WWN", "ID_BAR", NULL } };
+
+struct path test_pp = { .dev = "sdb", .bus = SYSFS_BUS_SCSI, .udev = &test_udev,
+			.sg_id.proto_id = SCSI_PROTOCOL_FCP, .vendor_id = "foo",
+			.product_id = "bar", .wwid = "xyzzy" };
+
+static void test_filter_path_property(void **state)
+{
+	conf.blist_property = blist_property_wwn;
+	expect_condlog(3, "sdb: udev property ID_WWN blacklisted\n");
+	assert_int_equal(filter_path(&conf, &test_pp), MATCH_PROPERTY_BLIST);
+}
+
+static void test_filter_path_devnode(void **state)
+{
+	/* always must include property elist, to avoid "missing property"
+	 * blacklisting */
+	conf.elist_property = blist_property_wwn;
+	conf.blist_devnode = blist_devnode_sdb;
+	expect_condlog(3, "sdb: udev property ID_WWN whitelisted\n");
+	expect_condlog(3, "sdb: device node name blacklisted\n");
+	assert_int_equal(filter_path(&conf, &test_pp), MATCH_DEVNODE_BLIST);
+}
+
+static void test_filter_path_device(void **state)
+{
+	/* always must include property elist, to avoid "missing property"
+	 * blacklisting */
+	conf.elist_property = blist_property_wwn;
+	conf.blist_device = blist_device_foo_bar;
+	expect_condlog(3, "sdb: udev property ID_WWN whitelisted\n");
+	expect_condlog(3, "sdb: (foo:bar) vendor/product blacklisted\n");
+	assert_int_equal(filter_path(&conf, &test_pp), MATCH_DEVICE_BLIST);
+}
+
+static void test_filter_path_protocol(void **state)
+{
+	conf.elist_property = blist_property_wwn;
+	conf.blist_protocol = blist_protocol_fcp;
+	expect_condlog(3, "sdb: udev property ID_WWN whitelisted\n");
+	expect_condlog(3, "sdb: protocol scsi:fcp blacklisted\n");
+	assert_int_equal(filter_path(&conf, &test_pp), MATCH_PROTOCOL_BLIST);
+}
+
+static void test_filter_path_wwid(void **state)
+{
+	conf.elist_property = blist_property_wwn;
+	conf.blist_wwid = blist_wwid_xyzzy;
+	expect_condlog(3, "sdb: udev property ID_WWN whitelisted\n");
+	expect_condlog(3, "sdb: wwid xyzzy blacklisted\n");
+	assert_int_equal(filter_path(&conf, &test_pp), MATCH_WWID_BLIST);
+}
+
+struct udev_device miss_udev = { "sdb", { "ID_FOO", "ID_BAZ", "ID_BAR", NULL } };
+
+struct path miss1_pp = { .dev = "sdc", .bus = SYSFS_BUS_SCSI,
+			.udev = &miss_udev,
+			.sg_id.proto_id = SCSI_PROTOCOL_ISCSI,
+			.vendor_id = "foo", .product_id = "baz",
+			.wwid = "plugh" };
+
+struct path miss2_pp = { .dev = "sdc", .bus = SYSFS_BUS_SCSI,
+			.udev = &test_udev,
+			.sg_id.proto_id = SCSI_PROTOCOL_ISCSI,
+			.vendor_id = "foo", .product_id = "baz",
+			.wwid = "plugh" };
+
+static void test_filter_path_missing1(void **state)
+{
+	conf.blist_property = blist_property_wwn;
+	conf.blist_devnode = blist_devnode_sdb;
+	conf.blist_device = blist_device_foo_bar;
+	conf.blist_protocol = blist_protocol_fcp;
+	conf.blist_wwid = blist_wwid_xyzzy;
+	expect_condlog(3, "sdb: blacklisted, udev property missing\n");
+	assert_int_equal(filter_path(&conf, &miss1_pp),
+			 MATCH_PROPERTY_BLIST_MISSING);
+}
+
+/* This one matches the property whitelist, to test the other missing
+ * functions */
+static void test_filter_path_missing2(void **state)
+{
+	conf.elist_property = blist_property_wwn;
+	conf.blist_devnode = blist_devnode_sdb;
+	conf.blist_device = blist_device_foo_bar;
+	conf.blist_protocol = blist_protocol_fcp;
+	conf.blist_wwid = blist_wwid_xyzzy;
+	expect_condlog(3, "sdb: udev property ID_WWN whitelisted\n");
+	assert_int_equal(filter_path(&conf, &miss2_pp),
+			 MATCH_NOTHING);
+}
+
+static void test_filter_path_whitelist(void **state)
+{
+	conf.elist_property = blist_property_wwn;
+	conf.elist_devnode = blist_devnode_sdb;
+	conf.elist_device = blist_device_foo_bar;
+	conf.elist_protocol = blist_protocol_fcp;
+	conf.elist_wwid = blist_wwid_xyzzy;
+	expect_condlog(3, "sdb: udev property ID_WWN whitelisted\n");
+	expect_condlog(3, "sdb: device node name whitelisted\n");
+	expect_condlog(3, "sdb: (foo:bar) vendor/product whitelisted\n");
+	expect_condlog(3, "sdb: protocol scsi:fcp whitelisted\n");
+	expect_condlog(3, "sdb: wwid xyzzy whitelisted\n");
+	assert_int_equal(filter_path(&conf, &test_pp),
+			 MATCH_WWID_BLIST_EXCEPT);
+}
+
+static void test_filter_path_whitelist_property(void **state)
+{
+	conf.blist_property = blist_property_wwn;
+	conf.elist_devnode = blist_devnode_sdb;
+	conf.elist_device = blist_device_foo_bar;
+	conf.elist_protocol = blist_protocol_fcp;
+	conf.elist_wwid = blist_wwid_xyzzy;
+	expect_condlog(3, "sdb: udev property ID_WWN blacklisted\n");
+	assert_int_equal(filter_path(&conf, &test_pp), MATCH_PROPERTY_BLIST);
+}
+
+static void test_filter_path_whitelist_devnode(void **state)
+{
+	conf.elist_property = blist_property_wwn;
+	conf.blist_devnode = blist_devnode_sdb;
+	conf.elist_device = blist_device_foo_bar;
+	conf.elist_protocol = blist_protocol_fcp;
+	conf.elist_wwid = blist_wwid_xyzzy;
+	expect_condlog(3, "sdb: udev property ID_WWN whitelisted\n");
+	expect_condlog(3, "sdb: device node name blacklisted\n");
+	assert_int_equal(filter_path(&conf, &test_pp), MATCH_DEVNODE_BLIST);
+}
+
+static void test_filter_path_whitelist_device(void **state)
+{
+	conf.elist_property = blist_property_wwn;
+	conf.elist_devnode = blist_devnode_sdb;
+	conf.blist_device = blist_device_foo_bar;
+	conf.elist_protocol = blist_protocol_fcp;
+	conf.elist_wwid = blist_wwid_xyzzy;
+	expect_condlog(3, "sdb: udev property ID_WWN whitelisted\n");
+	expect_condlog(3, "sdb: device node name whitelisted\n");
+	expect_condlog(3, "sdb: (foo:bar) vendor/product blacklisted\n");
+	assert_int_equal(filter_path(&conf, &test_pp), MATCH_DEVICE_BLIST);
+}
+
+static void test_filter_path_whitelist_protocol(void **state)
+{
+	conf.elist_property = blist_property_wwn;
+	conf.elist_devnode = blist_devnode_sdb;
+	conf.elist_device = blist_device_foo_bar;
+	conf.blist_protocol = blist_protocol_fcp;
+	conf.elist_wwid = blist_wwid_xyzzy;
+	expect_condlog(3, "sdb: udev property ID_WWN whitelisted\n");
+	expect_condlog(3, "sdb: device node name whitelisted\n");
+	expect_condlog(3, "sdb: (foo:bar) vendor/product whitelisted\n");
+	expect_condlog(3, "sdb: protocol scsi:fcp blacklisted\n");
+	assert_int_equal(filter_path(&conf, &test_pp), MATCH_PROTOCOL_BLIST);
+}
+
+static void test_filter_path_whitelist_wwid(void **state)
+{
+	conf.elist_property = blist_property_wwn;
+	conf.elist_devnode = blist_devnode_sdb;
+	conf.elist_device = blist_device_foo_bar;
+	conf.elist_protocol = blist_protocol_fcp;
+	conf.blist_wwid = blist_wwid_xyzzy;
+	expect_condlog(3, "sdb: udev property ID_WWN whitelisted\n");
+	expect_condlog(3, "sdb: device node name whitelisted\n");
+	expect_condlog(3, "sdb: (foo:bar) vendor/product whitelisted\n");
+	expect_condlog(3, "sdb: protocol scsi:fcp whitelisted\n");
+	expect_condlog(3, "sdb: wwid xyzzy blacklisted\n");
+	assert_int_equal(filter_path(&conf, &test_pp), MATCH_WWID_BLIST);
+}
+
+#define test_and_reset(x) cmocka_unit_test_teardown((x), reset_blists)
+
+int test_blacklist(void)
+{
+	const struct CMUnitTest tests[] = {
+		cmocka_unit_test(test_devnode_blacklist),
+		cmocka_unit_test(test_devnode_whitelist),
+		cmocka_unit_test(test_devnode_missing),
+		cmocka_unit_test(test_device_blacklist),
+		cmocka_unit_test(test_device_whitelist),
+		cmocka_unit_test(test_device_missing),
+		cmocka_unit_test(test_wwid_blacklist),
+		cmocka_unit_test(test_wwid_whitelist),
+		cmocka_unit_test(test_wwid_missing),
+		cmocka_unit_test(test_protocol_blacklist),
+		cmocka_unit_test(test_protocol_whitelist),
+		cmocka_unit_test(test_protocol_missing),
+		test_and_reset(test_property_blacklist),
+		test_and_reset(test_property_whitelist),
+		test_and_reset(test_property_missing),
+		test_and_reset(test_filter_path_property),
+		test_and_reset(test_filter_path_devnode),
+		test_and_reset(test_filter_path_device),
+		test_and_reset(test_filter_path_protocol),
+		test_and_reset(test_filter_path_wwid),
+		test_and_reset(test_filter_path_missing1),
+		test_and_reset(test_filter_path_missing2),
+		test_and_reset(test_filter_path_whitelist),
+		test_and_reset(test_filter_path_whitelist_property),
+		test_and_reset(test_filter_path_whitelist_devnode),
+		test_and_reset(test_filter_path_whitelist_device),
+		test_and_reset(test_filter_path_whitelist_protocol),
+		test_and_reset(test_filter_path_whitelist_wwid),
+	};
+	return cmocka_run_group_tests(tests, setup, teardown);
+}
+
+int main(void)
+{
+	int ret = 0;
+	ret += test_blacklist();
+	return ret;
+}
-- 
2.7.4

[PATCH 11/32] mpathpersist: add missing --param-rk usage info

[Benjamin Marzinski]

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 mpathpersist/main.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/mpathpersist/main.c b/mpathpersist/main.c
index 99151fe..0e4d3f2 100644
--- a/mpathpersist/main.c
+++ b/mpathpersist/main.c
@@ -705,6 +705,7 @@ static void usage(void)
 			"    --param-alltgpt|-Y         PR Out parameter 'ALL_TG_PT\n"
 			"    --param-aptpl|-Z           PR Out parameter 'APTPL'\n"
 			"    --read-keys|-k             PR In: Read Keys\n"
+			"    --param-rk=RK|-K RK        PR Out parameter reservation key\n"
 			"    --param-sark=SARK|-S SARK  PR Out parameter service "
 			"action\n"
 			"                               reservation key (SARK is in "
-- 
2.7.4

[PATCH 12/32] kpartx: minor fixes to make coverity happy

[Benjamin Marzinski]

an open() failure for fd_dasd will return -1, not 0. Also, cast blocksize
to a uint64_t to keep coverity from complaining about sign extension issues.

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

diff --git a/kpartx/dasd.c b/kpartx/dasd.c
index e418d5a..94ae81b 100644
--- a/kpartx/dasd.c
+++ b/kpartx/dasd.c
@@ -129,7 +129,7 @@ read_dasd_pt(int fd, struct slice all, struct slice *sp, int ns)
 			 */
 			unlink(pathname);
 		}
-		if (!fd_dasd) {
+		if (fd_dasd < 0) {
 			/* Couldn't open the device */
 			return -1;
 		}
@@ -157,7 +157,8 @@ read_dasd_pt(int fd, struct slice all, struct slice *sp, int ns)
 
 		geo.heads = 15;
 		geo.sectors = recs_per_track(blocksize);
-		cyl = disksize / (blocksize * geo.heads * geo.sectors);
+		cyl = disksize / ((uint64_t)blocksize * geo.heads *
+				  geo.sectors);
 		if (cyl < LV_COMPAT_CYL)
 			geo.cylinders = cyl;
 		else
-- 
2.7.4

[PATCH 13/32] kpartx: Fix memory leak of uuid found by coverity

[Benjamin Marzinski]

After the call to make_prefixed_uuid() allocs uuid, it must be freed if
dm_find_part() fails.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 kpartx/devmapper.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/kpartx/devmapper.c b/kpartx/devmapper.c
index f94d70e..cd33449 100644
--- a/kpartx/devmapper.c
+++ b/kpartx/devmapper.c
@@ -663,7 +663,7 @@ int dm_find_part(const char *parent, const char *delim, int part,
 
 	tmp = dm_find_uuid(uuid);
 	if (tmp == NULL)
-		return r;
+		goto out;
 
 	/* Sanity check on partition, see dm_foreach_partmaps */
 	if (dm_type(tmp, "linear") != 1)
@@ -689,13 +689,14 @@ int dm_find_part(const char *parent, const char *delim, int part,
 		       tmp, uuid, name);
 
 	r = dm_rename(tmp, name);
-	if (r == 0) {
-		free(uuid);
-		if (verbose)
-			fprintf(stderr, "renaming %s->%s failed\n", tmp, name);
-	} else
+	if (r == 1) {
 		*part_uuid = uuid;
+		return 1;
+	}
+	if (verbose)
+		fprintf(stderr, "renaming %s->%s failed\n", tmp, name);
 out:
+	free(uuid);
 	free(tmp);
 	return r;
 }
-- 
2.7.4

[PATCH 14/32] kpartx: fix bad dm_devn return

[Benjamin Marzinski]

dm_devn shouldn't be returning success if you can't create a dm task to
find the device info. Found by coverity.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 kpartx/devmapper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kpartx/devmapper.c b/kpartx/devmapper.c
index cd33449..8db1eb5 100644
--- a/kpartx/devmapper.c
+++ b/kpartx/devmapper.c
@@ -365,7 +365,7 @@ dm_devn (const char * mapname, int *major, int *minor)
 	struct dm_info info;
 
 	if (!(dmt = dm_task_create(DM_DEVICE_INFO)))
-		return 0;
+		return 1;
 
 	if (!dm_task_set_name(dmt, mapname))
 		goto out;
-- 
2.7.4

[PATCH 15/32] kpartx: remove duplicated gpt validation check

[Benjamin Marzinski]

The identical check exists twice in a row.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 kpartx/gpt.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/kpartx/gpt.c b/kpartx/gpt.c
index 6ef20f9..e31611a 100644
--- a/kpartx/gpt.c
+++ b/kpartx/gpt.c
@@ -348,16 +348,6 @@ is_gpt_valid(int fd, uint64_t lba,
 		return 0;
 	}
 
-
-	/* Check that sizeof_partition_entry has the correct value */
-	if (__le32_to_cpu((*gpt)->sizeof_partition_entry) != sizeof(gpt_entry)) {
-		// printf("GUID partition entry size check failed.\n");
-		free(*gpt);
-		*gpt = NULL;
-		return 0;
-	}
-
-
 	if (!(*ptes = alloc_read_gpt_entries(fd, *gpt))) {
 		free(*gpt);
 		*gpt = NULL;
-- 
2.7.4

[PATCH 16/32] 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 kill 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 returning PATH_TIMEOUT whenever the thread hasn't died,
it should only time out if the thread didn't successfully get a value,
which means the previous state was already PATH_TIMEOUT.

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

diff --git a/libmultipath/checkers/tur.c b/libmultipath/checkers/tur.c
index bf8486d..275541f 100644
--- a/libmultipath/checkers/tur.c
+++ b/libmultipath/checkers/tur.c
@@ -355,12 +355,15 @@ 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 */
-			pthread_mutex_unlock(&ct->lock);
-			condlog(3, "%s: tur thread not responding",
-				tur_devt(devt, sizeof(devt), ct));
-			return PATH_TIMEOUT;
+		if (uatomic_read(&ct->holders) > 1) {
+			/* pthread cancel failed. If it didn't get the path
+			   state already, timeout */
+			if (ct->state == PATH_PENDING) {
+				pthread_mutex_unlock(&ct->lock);
+				condlog(3, "%s: tur thread not responding",
+					tur_devt(devt, sizeof(devt), ct));
+				return PATH_TIMEOUT;
+			}
 		}
 		/* Start new TUR checker */
 		ct->state = PATH_UNCHECKED;
-- 
2.7.4

[PATCH 17/32] 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.  It also never uses ct->devt directly. Instead, it
always graps the major and minor, and turns them into a string. This
patch changes ct->devt into that string, and only sets it if it hasn't
been set before, and no thread is holding the context. After that,
this value is only read, and so doesn't need locking.

I realize that this locking was added to make an analyzer happy,
presumably because sometimes ct->devt was being accessed while ct->devt
was held, and sometimes not.  The tur checker locking will be cleaned
up in a later patch to deal with this.

Cc: Bart Van Assche <bart.vanassche@wdc.com>
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/checkers/tur.c | 52 ++++++++++++++-------------------------------
 1 file changed, 16 insertions(+), 36 deletions(-)

diff --git a/libmultipath/checkers/tur.c b/libmultipath/checkers/tur.c
index 275541f..bfb0907 100644
--- a/libmultipath/checkers/tur.c
+++ b/libmultipath/checkers/tur.c
@@ -37,32 +37,19 @@
 #define MSG_TUR_FAILED	"tur checker failed to initialize"
 
 struct tur_checker_context {
-	dev_t devt;
+	char devt[32];
 	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;
@@ -232,14 +219,12 @@ 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, "%s: tur checker starting up", ct->devt);
 
 	/* TUR checker start up */
 	pthread_mutex_lock(&ct->lock);
@@ -256,8 +241,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, "%s: tur checker finished, state %s", ct->devt,
+		checker_state_name(state));
 
 	running = uatomic_xchg(&ct->running, 0);
 	if (!running)
@@ -308,15 +293,14 @@ int libcheck_check(struct checker * c)
 	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 (uatomic_read(&ct->holders) == 1 && ct->devt[0] == 0 &&
+	    fstat(c->fd, &sb) == 0) {
+		snprintf(ct->devt, sizeof(ct->devt), "%d:%d", major(sb.st_rdev),
+			 minor(sb.st_rdev));
 	}
 
 	if (c->sync)
@@ -327,8 +311,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 +321,12 @@ 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, "%s: tur checker timeout", 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, "%s: tur checker not finished", ct->devt);
 			tur_status = PATH_PENDING;
 		} else {
 			/* TUR checker done */
@@ -361,7 +342,7 @@ int libcheck_check(struct checker * c)
 			if (ct->state == PATH_PENDING) {
 				pthread_mutex_unlock(&ct->lock);
 				condlog(3, "%s: tur thread not responding",
-					tur_devt(devt, sizeof(devt), ct));
+					ct->devt);
 				return PATH_TIMEOUT;
 			}
 		}
@@ -381,7 +362,7 @@ int libcheck_check(struct checker * c)
 			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));
+				" sync mode", ct->devt);
 			return tur_check(c->fd, c->timeout,
 					 copy_msg_to_checker, c);
 		}
@@ -392,8 +373,7 @@ 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, "%s: tur checker still running", ct->devt);
 			tur_status = PATH_PENDING;
 		} else {
 			int running = uatomic_xchg(&ct->running, 0);
-- 
2.7.4

[PATCH 18/32] 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.

Cc: Bart Van Assche <bart.vanassche@wdc.com>
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/checkers/tur.c | 46 +++++++++++----------------------------------
 1 file changed, 11 insertions(+), 35 deletions(-)

diff --git a/libmultipath/checkers/tur.c b/libmultipath/checkers/tur.c
index bfb0907..a02c445 100644
--- a/libmultipath/checkers/tur.c
+++ b/libmultipath/checkers/tur.c
@@ -99,17 +99,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 };
@@ -128,7 +119,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) {
@@ -136,7 +127,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) {
@@ -181,14 +172,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;
 }
 
@@ -206,19 +197,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);
@@ -232,12 +215,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);
 
@@ -279,13 +263,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;
@@ -304,7 +281,7 @@ int libcheck_check(struct checker * c)
 	}
 
 	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
@@ -363,8 +340,7 @@ int libcheck_check(struct checker * c)
 			pthread_mutex_unlock(&ct->lock);
 			condlog(3, "%s: failed to start tur thread, using"
 				" sync mode", 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 19/32] 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.

Cc: Bart Van Assche <bart.vanassche@wdc.com>
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/checkers/tur.c | 49 ++++++++++++++++++---------------------------
 1 file changed, 20 insertions(+), 29 deletions(-)

diff --git a/libmultipath/checkers/tur.c b/libmultipath/checkers/tur.c
index a02c445..4a3e39c 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;
 
 	ct = malloc(sizeof(struct tur_checker_context));
 	if (!ct)
@@ -64,10 +63,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);
 	c->context = ct;
 
 	return 0;
@@ -209,12 +205,6 @@ static void *tur_thread(void *ctx)
 
 	condlog(3, "%s: tur checker starting up", 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();
 
@@ -286,13 +276,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);
@@ -308,23 +291,29 @@ 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) {
 			/* pthread cancel failed. If it didn't get the path
 			   state already, timeout */
-			if (ct->state == PATH_PENDING) {
-				pthread_mutex_unlock(&ct->lock);
+			pthread_mutex_lock(&ct->lock);
+			tur_status = ct->state;
+			pthread_mutex_unlock(&ct->lock);
+			if (tur_status == PATH_PENDING) {
 				condlog(3, "%s: tur thread not responding",
 					ct->devt);
 				return PATH_TIMEOUT;
 			}
 		}
 		/* 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);
@@ -337,20 +326,22 @@ 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, "%s: failed to start tur thread, using"
 				" sync mode", 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, "%s: tur checker still running", ct->devt);
-			tur_status = PATH_PENDING;
 		} else {
 			int running = uatomic_xchg(&ct->running, 0);
 			if (running)
-- 
2.7.4

[PATCH 20/32] 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 | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/libmultipath/checkers/tur.c b/libmultipath/checkers/tur.c
index 4a3e39c..41c45e8 100644
--- a/libmultipath/checkers/tur.c
+++ b/libmultipath/checkers/tur.c
@@ -279,12 +279,19 @@ 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, "%s: tur checker timeout", ct->devt);
+				condlog(3, "%s: tur checker timeout", 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, "%s: tur checker not finished", ct->devt);
 			tur_status = PATH_PENDING;
-- 
2.7.4

[PATCH 21/32] 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().

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 22/32] 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.

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 0b1855d..3e0db7f 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 23/32] 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.

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 24/32] 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.

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 25/32] 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.

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 26/32] 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.

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 27/32] 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.

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 28/32] 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.

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 29/32] 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.

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 30/32] 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.

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 cc493c1..125a805 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;
 
@@ -1096,7 +1096,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 31/32] 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.

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 125a805..3c2fe7b 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -1978,14 +1978,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 32/32] multipathd: fix memory leak on error in configure

[Benjamin Marzinski]

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

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 3c2fe7b..61ca455 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2277,7 +2277,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){
@@ -2294,7 +2294,7 @@ configure (struct vectors * vecs)
 	}
 	if (map_discovery(vecs)) {
 		condlog(0, "configure failed at map discovery");
-		return 1;
+		goto fail;
 	}
 
 	/*
@@ -2308,7 +2308,7 @@ configure (struct vectors * vecs)
 		force_reload = FORCE_RELOAD_YES;
 	if (ret) {
 		condlog(0, "configure failed while coalescing paths");
-		return 1;
+		goto fail;
 	}
 
 	/*
@@ -2317,7 +2317,7 @@ configure (struct vectors * vecs)
 	 */
 	if (coalesce_maps(vecs, mpvec)) {
 		condlog(0, "configure failed while coalescing maps");
-		return 1;
+		goto fail;
 	}
 
 	dm_lib_release();
@@ -2353,6 +2353,10 @@ configure (struct vectors * vecs)
 			i--;
 	}
 	return 0;
+
+fail:
+	vector_free(mpvec);
+	return 1;
 }
 
 int
-- 
2.7.4

Re: [PATCH 17/32] libmultipath: fix tur checker double locking

[Bart Van Assche]

[-- Attachment #1: Type: text/plain, Size: 918 bytes --]

On Wed, 2018-08-01 at 15:57 -0500, Benjamin Marzinski wrote:
> I realize that this locking was added to make an analyzer happy,
> presumably because sometimes ct->devt was being accessed while ct->devt
> was held, and sometimes not.  The tur checker locking will be cleaned
> up in a later patch to deal with this.

Are you perhaps referring to commit 873be9fef222 ("libmultipath/checkers/tur:
Serialize tur_checker_context.devt accesses")? Your assumption about how DRD
works is wrong. DRD doesn't care about which mutex or other synchronization
primitives are used to serialize accesses to data that is shared between
threads. All it cares about is that concurrent accesses to shared data are
serialized. I'm afraid that your patch reintroduces the race conditions that
were fixed by commit 873be9fef222. Have you considered to fix this without
removing the locking?

Bart.



[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 18/32] libmultipath: fix tur memory misuse

[Bart Van Assche]

[-- Attachment #1: Type: text/plain, Size: 1738 bytes --]

On Wed, 2018-08-01 at 15:57 -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.
> [ ... ]
> -#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 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);
> -}

The above code was introduced because multiple threads can access ct->message
concurrently. Does your patch reintroduce the race conditions on the ct->message
buffer that were fixed by commit f63f4d115813 ("libmultipath/checkers/tur: Protect
tur_checker_context.message changes")?

Bart.




[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 17/32] libmultipath: fix tur checker double locking

[Benjamin Marzinski]

On Sun, Aug 05, 2018 at 03:40:30PM +0000, Bart Van Assche wrote:
> On Wed, 2018-08-01 at 15:57 -0500, Benjamin Marzinski wrote:
> > I realize that this locking was added to make an analyzer happy,
> > presumably because sometimes ct->devt was being accessed while ct->devt
> > was held, and sometimes not.  The tur checker locking will be cleaned
> > up in a later patch to deal with this.
> 
> Are you perhaps referring to commit 873be9fef222 ("libmultipath/checkers/tur:
> Serialize tur_checker_context.devt accesses")? Your assumption about how DRD
> works is wrong. DRD doesn't care about which mutex or other synchronization
> primitives are used to serialize accesses to data that is shared between
> threads. All it cares about is that concurrent accesses to shared data are
> serialized. I'm afraid that your patch reintroduces the race conditions that
> were fixed by commit 873be9fef222. Have you considered to fix this without
> removing the locking?

I'm confused here. If the data is only read by multiple threads, I don't
see why we need to synchronize it. Unless I'm missing something, my
patch does make sure that the only time ct->devt is written is when
there is only one thread running that has access to ct.

-       if (fstat(c->fd, &sb) == 0) {
...
+       if (uatomic_read(&ct->holders) == 1 && ct->devt[0] == 0 &&
+           fstat(c->fd, &sb) == 0) {

holders will always be 2 when a tur checker thread is running (well, the
thread will decrement it when it finishes, but it will not touch ct
after that action). Since only the thread calling libcheck_init can
start the tur checker thread using this context, if there is no thread
running at the start of libcheck_check(), then no thread can be started
until libcheck_check() starts it.

Ideally, ct->devt would be set in libcheck_init(), but we don't have the
information there, so setting it once, on the first time we call
libcheck_check() seems like a reasonable, and safe, answer.  I certainly
prefer it to adding recusive locking where it isn't needed.

-Ben

> 
> Bart.

Re: [PATCH 18/32] libmultipath: fix tur memory misuse

[Benjamin Marzinski]

On Sun, Aug 05, 2018 at 03:43:18PM +0000, Bart Van Assche wrote:
> On Wed, 2018-08-01 at 15:57 -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.
> > [ ... ]
> > -#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 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);
> > -}
> 
> The above code was introduced because multiple threads can access ct->message
> concurrently. Does your patch reintroduce the race conditions on the ct->message
> buffer that were fixed by commit f63f4d115813 ("libmultipath/checkers/tur: Protect
> tur_checker_context.message changes")?

It shouldn't. Since the tur checker thread only has access to the
tur_checker_context, the only race we have to worry about here is on
access to ct->message. The main checker code is free to pass c->message
to tur_check(), since the tur checker thread doesn't have access to
that.  With this patch, tur_thread() passes tur_check() a local variable
and copies that variable to ct->message while holding the mutex.  the
main checker code copies ct->message to c->message while holding the
same mutex. The main checker code even clears ct->message before
starting the thread while holding the mutex.

If you see something I've missed, please let me know.

-Ben

> 
> Bart.
> 

Re: [PATCH 00/32] Misc Multipath patches

[Christophe Varoqui]

[-- Attachment #1.1: Type: text/plain, Size: 4260 bytes --]

Hi,

The backlog is merged, mostly.

Ben, your latest aggregated patchset is merged up to 15/32 included,
pending consensus about tur locking with Bart.
Would you rather have a release tag at this point or can it wait the merge
of your pending patches ?

Thanks,
Christophe.


On Wed, Aug 1, 2018 at 10:57 PM Benjamin Marzinski <bmarzins@redhat.com>
wrote:

> This batch of patches is a resend of my previous 10 patches, as well
> as new patches mostly found by running coverity.
>
> patches 0001-0010 are identical to my previous patchset
> patches 0012-0015 & 0021-0032 are minor issues found by coverity.
>         Not all of them are bugs that could actually occur in practice,
>         but they are simple and hopefully non-controversial changes.
> patches 0016-0020 are a number of fixes to the tur checker.These are
>         the ones that should get the most attention.
>
> Also, I would love to have a 0.7.8 tagged release.
>
> Benjamin Marzinski (32):
>   libmultipath: remove last of rbd code
>   libmultipath: fix detect alua corner case
>   multipath: fix setting conf->version
>   mpathpersist: add --param-alltgpt option
>   libmutipath: remove unused IDE bus type
>   multipathd: add new protocol path wildcard
>   libmultipath: add "protocol" blacklist option.
>   libmultipath: remove _filter_* blacklist functions
>   multipath tests: change to work with old make versions
>   multipath tests: add blacklist tests
>   mpathpersist: add missing --param-rk usage info
>   kpartx: minor fixes to make coverity happy
>   kpartx: Fix memory leak of uuid found by coverity
>   kpartx: fix bad dm_devn return
>   kpartx: remove duplicated gpt validation check
>   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
>
>  kpartx/dasd.c                            |   5 +-
>  kpartx/del-part-nodes.rules              |   2 +-
>  kpartx/devmapper.c                       |  15 +-
>  kpartx/gpt.c                             |  10 -
>  libmpathpersist/mpath_persist.c          |  10 +-
>  libmultipath/blacklist.c                 | 177 +++++------
>  libmultipath/blacklist.h                 |   5 +-
>  libmultipath/checkers/tur.c              | 165 ++++------
>  libmultipath/config.c                    |  15 +
>  libmultipath/config.h                    |   2 +
>  libmultipath/configure.c                 |   2 +-
>  libmultipath/devmapper.c                 |   6 +-
>  libmultipath/dict.c                      |  19 +-
>  libmultipath/discovery.c                 |  19 +-
>  libmultipath/parser.c                    |  12 +-
>  libmultipath/print.c                     |  82 ++++-
>  libmultipath/print.h                     |   2 +
>  libmultipath/prioritizers/path_latency.c |   3 +-
>  libmultipath/propsel.c                   |   4 +-
>  libmultipath/structs.c                   |   2 +-
>  libmultipath/structs.h                   |   2 -
>  libmultipath/uevent.c                    |   6 +
>  mpathpersist/main.c                      |  12 +-
>  mpathpersist/main.h                      |   1 +
>  mpathpersist/mpathpersist.8              |   4 +
>  multipath/main.c                         |   2 +-
>  multipath/multipath.conf.5               |  24 +-
>  multipathd/cli_handlers.c                |  11 +-
>  multipathd/main.c                        |  26 +-
>  tests/Makefile                           |   6 +-
>  tests/blacklist.c                        | 512
> +++++++++++++++++++++++++++++++
>  31 files changed, 889 insertions(+), 274 deletions(-)
>  create mode 100644 tests/blacklist.c
>
> --
> 2.7.4
>
>

[-- Attachment #1.2: Type: text/html, Size: 5309 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 00/32] Misc Multipath patches

[Benjamin Marzinski]

On Tue, Aug 07, 2018 at 03:39:55PM +0200, Christophe Varoqui wrote:
>    Hi,
>    The backlog is merged, mostly.
>    Ben, your latest aggregated patchset is merged up to 15/32 included,
>    pending consensus about tur locking with Bart.
>    Would you rather have a release tag at this point or can it wait the merge
>    of your pending patches ?

I'd prefer to wait for a resolution on all of my patches.
Thanks.

-Ben

>    Thanks,
>    Christophe.
>    On Wed, Aug 1, 2018 at 10:57 PM Benjamin Marzinski
>    <[1]bmarzins@redhat.com> wrote:
> 
>      This batch of patches is a resend of my previous 10 patches, as well
>      as new patches mostly found by running coverity.
> 
>      patches 0001-0010 are identical to my previous patchset
>      patches 0012-0015 & 0021-0032 are minor issues found by coverity.
>              Not all of them are bugs that could actually occur in practice,
>              but they are simple and hopefully non-controversial changes.
>      patches 0016-0020 are a number of fixes to the tur checker.These are
>              the ones that should get the most attention.
> 
>      Also, I would love to have a 0.7.8 tagged release.
> 
>      Benjamin Marzinski (32):
>        libmultipath: remove last of rbd code
>        libmultipath: fix detect alua corner case
>        multipath: fix setting conf->version
>        mpathpersist: add --param-alltgpt option
>        libmutipath: remove unused IDE bus type
>        multipathd: add new protocol path wildcard
>        libmultipath: add "protocol" blacklist option.
>        libmultipath: remove _filter_* blacklist functions
>        multipath tests: change to work with old make versions
>        multipath tests: add blacklist tests
>        mpathpersist: add missing --param-rk usage info
>        kpartx: minor fixes to make coverity happy
>        kpartx: Fix memory leak of uuid found by coverity
>        kpartx: fix bad dm_devn return
>        kpartx: remove duplicated gpt validation check
>        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
> 
>       kpartx/dasd.c                            |   5 +-
>       kpartx/del-part-nodes.rules              |   2 +-
>       kpartx/devmapper.c                       |  15 +-
>       kpartx/gpt.c                             |  10 -
>       libmpathpersist/mpath_persist.c          |  10 +-
>       libmultipath/blacklist.c                 | 177 +++++------
>       libmultipath/blacklist.h                 |   5 +-
>       libmultipath/checkers/tur.c              | 165 ++++------
>       libmultipath/config.c                    |  15 +
>       libmultipath/config.h                    |   2 +
>       libmultipath/configure.c                 |   2 +-
>       libmultipath/devmapper.c                 |   6 +-
>       libmultipath/dict.c                      |  19 +-
>       libmultipath/discovery.c                 |  19 +-
>       libmultipath/parser.c                    |  12 +-
>       libmultipath/print.c                     |  82 ++++-
>       libmultipath/print.h                     |   2 +
>       libmultipath/prioritizers/path_latency.c |   3 +-
>       libmultipath/propsel.c                   |   4 +-
>       libmultipath/structs.c                   |   2 +-
>       libmultipath/structs.h                   |   2 -
>       libmultipath/uevent.c                    |   6 +
>       mpathpersist/main.c                      |  12 +-
>       mpathpersist/main.h                      |   1 +
>       mpathpersist/mpathpersist.8              |   4 +
>       multipath/main.c                         |   2 +-
>       multipath/multipath.conf.5               |  24 +-
>       multipathd/cli_handlers.c                |  11 +-
>       multipathd/main.c                        |  26 +-
>       tests/Makefile                           |   6 +-
>       tests/blacklist.c                        | 512
>      +++++++++++++++++++++++++++++++
>       31 files changed, 889 insertions(+), 274 deletions(-)
>       create mode 100644 tests/blacklist.c
> 
>      --
>      2.7.4
> 
> References
> 
>    Visible links
>    1. mailto:bmarzins@redhat.com

Re: [PATCH 18/32] libmultipath: fix tur memory misuse

[Bart Van Assche]

[-- Attachment #1: Type: text/plain, Size: 3168 bytes --]

On Mon, 2018-08-06 at 18:12 -0500, Benjamin Marzinski wrote:
> On Sun, Aug 05, 2018 at 03:43:18PM +0000, Bart Van Assche wrote:
> > On Wed, 2018-08-01 at 15:57 -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.
> > > [ ... ]
> > > -#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 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);
> > > -}
> > 
> > The above code was introduced because multiple threads can access ct->message
> > concurrently. Does your patch reintroduce the race conditions on the ct->message
> > buffer that were fixed by commit f63f4d115813 ("libmultipath/checkers/tur: Protect
> > tur_checker_context.message changes")?
> 
> It shouldn't. Since the tur checker thread only has access to the
> tur_checker_context, the only race we have to worry about here is on
> access to ct->message. The main checker code is free to pass c->message
> to tur_check(), since the tur checker thread doesn't have access to
> that.  With this patch, tur_thread() passes tur_check() a local variable
> and copies that variable to ct->message while holding the mutex.  the
> main checker code copies ct->message to c->message while holding the
> same mutex. The main checker code even clears ct->message before
> starting the thread while holding the mutex.
> 
> If you see something I've missed, please let me know.

Hello Ben,

Thanks for the additional clarification. I think this patch is a good improvement
for the TUR checker since it simplifies the code.

Bart.



[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 17/32] libmultipath: fix tur checker double locking

[Bart Van Assche]

On Mon, 2018-08-06 at 17:55 -0500, Benjamin Marzinski wrote:
> I'm confused here. If the data is only read by multiple threads, I don't
> see why we need to synchronize it. Unless I'm missing something, my
> patch does make sure that the only time ct->devt is written is when
> there is only one thread running that has access to ct.
> 
> -       if (fstat(c->fd, &sb) == 0) {
> ...
> +       if (uatomic_read(&ct->holders) == 1 && ct->devt[0] == 0 &&
> +           fstat(c->fd, &sb) == 0) {
> 
> holders will always be 2 when a tur checker thread is running (well, the
> thread will decrement it when it finishes, but it will not touch ct
> after that action). Since only the thread calling libcheck_init can
> start the tur checker thread using this context, if there is no thread
> running at the start of libcheck_check(), then no thread can be started
> until libcheck_check() starts it.
> 
> Ideally, ct->devt would be set in libcheck_init(), but we don't have the
> information there, so setting it once, on the first time we call
> libcheck_check() seems like a reasonable, and safe, answer.  I certainly
> prefer it to adding recusive locking where it isn't needed.

Replacing a locking scheme into an approach based on atomic variables is
something I don't like because it makes the code harder to analyze for
correctness. Have you considered to keep the locking and fix the bug that
you described in the commit message?

Thanks,

Bart.