[PATCH 00/54] multipath-tools series part IV: identify paths by dev_t

[PATCH 00/54] multipath-tools series part IV: identify paths by dev_t

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Hi Christophe, hi Ben,

This is part IV of a larger patch series for multipath-tools I've been preparing.
It's based on the previously submitted part III.

The full series will also be available here:
https://github.com/mwilck/multipath-tools/tree/ups/submit-2007

There are tags in that repo for each part of the series.
This part is tagged "submit-200709-4".

The dominant idea of the patches in this subseries is to move away from
the device node name as the main path identifier towards the dev_t aka
major/minor number. This is motivated by the fact that dev_t is used by the
kernel device mapper for referring to devices, both maps and paths.
If searching a device by dev_t fails, it's basically certain not to exist.
DM calls with major/minor number may even succeed if the respective devnode
has not been created yet. dev_t also works better for devices that aren't
fully initialized yet. Of course we still track the device name, but mostly
for log messages and user-directed output.

In short, using dev_t as primary identifier makes our code more robust.

The subseries also contains some unrelated fixes for functions I worked on
while making these changes.

Regards,
Martin

Martin Wilck (12):
  libmultipath: adopt_paths(): use find_path_by_devt()
  libmultipath: adopt_paths(): don't bail out on single path failure
  libmultipath: path_discover(): use find_path_by_devt()
  libmultipath: path_discover(): always set DI_BLACKLIST
  libmultipath: get_refwwid(): use find_path_by_devt()
  libmultipath: get_refwwid(): call get_multipath_config() only once
  libmultipath: get_refwwid(): get rid of "check" label
  libmultipath: get_refwwid(): use symbolic return values
  libmultipath: get_refwwid(): use switch statement
  libmultipath: constify get_mpe_wwid()
  multipath: call strchop() on command line argument
  libmultipath: get_refwwid(): skip strchop(), and constify dev
    parameter

 libmultipath/config.c      |   2 +-
 libmultipath/config.h      |   2 +-
 libmultipath/configure.c   | 200 +++++++++++++------------------------
 libmultipath/configure.h   |   2 +-
 libmultipath/discovery.c   |  32 +++---
 libmultipath/structs_vec.c |  14 ++-
 multipath/main.c           |   2 +
 7 files changed, 94 insertions(+), 160 deletions(-)

-- 
2.26.2

[PATCH 43/54] libmultipath: adopt_paths(): use find_path_by_devt()

[mwilck]

From: Martin Wilck <mwilck@suse.com>

pp->dev_t is the primary identifying property for both dm and udev.
Use it here, too.

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

diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
index c7dffb7..55fca9b 100644
--- a/libmultipath/structs_vec.c
+++ b/libmultipath/structs_vec.c
@@ -75,7 +75,7 @@ int adopt_paths(vector pathvec, struct multipath *mpp)
 			if (!mpp->paths && !(mpp->paths = vector_alloc()))
 				return 1;
 
-			if (!find_path_by_dev(mpp->paths, pp->dev) &&
+			if (!find_path_by_devt(mpp->paths, pp->dev_t) &&
 			    store_path(mpp->paths, pp))
 					return 1;
 			conf = get_multipath_config();
-- 
2.26.2

[PATCH 44/54] libmultipath: adopt_paths(): don't bail out on single path failure

[mwilck]

From: Martin Wilck <mwilck@suse.com>

If pathinfo fails for one path to be adopted, we currently
fail the entire function. This may cause ev_add_path() for a valid
path to fail because some other path is broken. Fix it by just
skipping paths that don't look healthy.

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

diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
index 55fca9b..bc47d1e 100644
--- a/libmultipath/structs_vec.c
+++ b/libmultipath/structs_vec.c
@@ -75,16 +75,20 @@ int adopt_paths(vector pathvec, struct multipath *mpp)
 			if (!mpp->paths && !(mpp->paths = vector_alloc()))
 				return 1;
 
-			if (!find_path_by_devt(mpp->paths, pp->dev_t) &&
-			    store_path(mpp->paths, pp))
-					return 1;
 			conf = get_multipath_config();
 			pthread_cleanup_push(put_multipath_config, conf);
 			ret = pathinfo(pp, conf,
 				       DI_PRIO | DI_CHECKER);
 			pthread_cleanup_pop(1);
-			if (ret)
-				return 1;
+			if (ret) {
+				condlog(3, "%s: pathinfo failed for %s",
+					__func__, pp->dev);
+				continue;
+			}
+
+			if (!find_path_by_devt(mpp->paths, pp->dev_t) &&
+			    store_path(mpp->paths, pp))
+					return 1;
 		}
 	}
 	return 0;
-- 
2.26.2

[PATCH 45/54] libmultipath: path_discover(): use find_path_by_devt()

[mwilck]

From: Martin Wilck <mwilck@suse.com>

In path_discover(), it's actually expected that a the path to be discovered is
not already in pathvec. So, do search by devt in the first place rather than
searching twice.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/discovery.c | 32 ++++++++++++--------------------
 1 file changed, 12 insertions(+), 20 deletions(-)

diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index e26aae2..5f4ebf0 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -125,27 +125,19 @@ static int
 path_discover (vector pathvec, struct config * conf,
 	       struct udev_device *udevice, int flag)
 {
-	struct path * pp;
-	const char * devname;
-
-	devname = udev_device_get_sysname(udevice);
-	if (!devname)
-		return PATHINFO_FAILED;
-
-	pp = find_path_by_dev(pathvec, devname);
-	if (!pp) {
-		char devt[BLK_DEV_SIZE];
-		dev_t devnum = udev_device_get_devnum(udevice);
+	struct path *pp;
+	char devt[BLK_DEV_SIZE];
+	dev_t devnum = udev_device_get_devnum(udevice);
 
-		snprintf(devt, BLK_DEV_SIZE, "%d:%d",
-			 major(devnum), minor(devnum));
-		pp = find_path_by_devt(pathvec, devt);
-		if (!pp)
-			return store_pathinfo(pathvec, conf,
-					      udevice, flag | DI_BLACKLIST,
-					      NULL);
-	}
-	return pathinfo(pp, conf, flag);
+	snprintf(devt, BLK_DEV_SIZE, "%d:%d",
+		 major(devnum), minor(devnum));
+	pp = find_path_by_devt(pathvec, devt);
+	if (!pp)
+		return store_pathinfo(pathvec, conf,
+				      udevice, flag | DI_BLACKLIST,
+				      NULL);
+	else
+		return pathinfo(pp, conf, flag);
 }
 
 static void cleanup_udev_enumerate_ptr(void *arg)
-- 
2.26.2

[PATCH 46/54] libmultipath: path_discover(): always set DI_BLACKLIST

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Since 65e1845 ("multipath: call store_pathinfo with DI_BLACKLIST"), we
use DI_BLACKLIST for new paths. There's no reason why we shouldn't do the
same with paths which are (unexpectedly) already in pathvec. As argued
for 65e1845, this might save some unnecessary work for paths which are
blacklisted anyway.

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

diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index 5f4ebf0..caabfef 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -137,7 +137,7 @@ path_discover (vector pathvec, struct config * conf,
 				      udevice, flag | DI_BLACKLIST,
 				      NULL);
 	else
-		return pathinfo(pp, conf, flag);
+		return pathinfo(pp, conf, flag | DI_BLACKLIST);
 }
 
 static void cleanup_udev_enumerate_ptr(void *arg)
-- 
2.26.2

[PATCH 47/54] libmultipath: get_refwwid(): use find_path_by_devt()

[mwilck]

From: Martin Wilck <mwilck@suse.com>

If get_refwwid is called with a dev_t argument, there's no point in converting
it into a devname first. If the device doesn't exist, get_udev_device() will fail.

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

diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index 75e11fd..e8d6db8 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -1414,17 +1414,15 @@ int get_refwwid(enum mpath_cmds cmd, char *dev, enum devtypes dev_type,
 
 	if (dev_type == DEV_DEVT) {
 		strchop(dev);
-		if (devt2devname(buff, FILE_NAME_SIZE, dev)) {
-			condlog(0, "%s: cannot find block device\n", dev);
-			return 1;
-		}
-		pp = find_path_by_dev(pathvec, buff);
+		pp = find_path_by_devt(pathvec, dev);
 		if (!pp) {
 			struct udev_device *udevice =
 				get_udev_device(dev, dev_type);
 
-			if (!udevice)
+			if (!udevice) {
+				condlog(0, "%s: cannot find block device", dev);
 				return 1;
+			}
 
 			conf = get_multipath_config();
 			pthread_cleanup_push(put_multipath_config, conf);
-- 
2.26.2

[PATCH 48/54] libmultipath: get_refwwid(): call get_multipath_config() only once

[mwilck]

From: Martin Wilck <mwilck@suse.com>

rather than 7 times in a single function. In get_refwwid(), the code that
is not run under the RCU read lock is negligible, so we might as well
keep the lock.

The "invalid" variable becomes obsolete by this change.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/configure.c | 60 ++++++++++++++--------------------------
 1 file changed, 21 insertions(+), 39 deletions(-)

diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index e8d6db8..c4712d7 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -1344,22 +1344,14 @@ struct udev_device *get_udev_device(const char *dev, enum devtypes dev_type)
 	return ud;
 }
 
-/*
- * returns:
- * 0 - success
- * 1 - failure
- * 2 - blacklist
- */
-int get_refwwid(enum mpath_cmds cmd, char *dev, enum devtypes dev_type,
-		vector pathvec, char **wwid)
+static int _get_refwwid(enum mpath_cmds cmd, char *dev, enum devtypes dev_type,
+			vector pathvec, struct config *conf, char **wwid)
 {
 	int ret = 1;
 	struct path * pp;
 	char buff[FILE_NAME_SIZE];
 	char * refwwid = NULL, tmpwwid[WWID_SIZE];
 	int flags = DI_SYSFS | DI_WWID;
-	struct config *conf;
-	int invalid = 0;
 
 	if (!wwid)
 		return 1;
@@ -1386,11 +1378,8 @@ int get_refwwid(enum mpath_cmds cmd, char *dev, enum devtypes dev_type,
 			if (!udevice)
 				return 1;
 
-			conf = get_multipath_config();
-			pthread_cleanup_push(put_multipath_config, conf);
 			ret = store_pathinfo(pathvec, conf, udevice,
 					     flags, &pp);
-			pthread_cleanup_pop(1);
 			udev_device_unref(udevice);
 			if (!pp) {
 				if (ret == 1)
@@ -1399,13 +1388,8 @@ int get_refwwid(enum mpath_cmds cmd, char *dev, enum devtypes dev_type,
 				return ret;
 			}
 		}
-		conf = get_multipath_config();
-		pthread_cleanup_push(put_multipath_config, conf);
 		if (pp->udev && pp->uid_attribute &&
 		    filter_property(conf, pp->udev, 3, pp->uid_attribute) > 0)
-			invalid = 1;
-		pthread_cleanup_pop(1);
-		if (invalid)
 			return 2;
 
 		refwwid = pp->wwid;
@@ -1424,11 +1408,8 @@ int get_refwwid(enum mpath_cmds cmd, char *dev, enum devtypes dev_type,
 				return 1;
 			}
 
-			conf = get_multipath_config();
-			pthread_cleanup_push(put_multipath_config, conf);
 			ret = store_pathinfo(pathvec, conf, udevice,
 					     flags, &pp);
-			pthread_cleanup_pop(1);
 			udev_device_unref(udevice);
 			if (!pp) {
 				if (ret == 1)
@@ -1437,13 +1418,8 @@ int get_refwwid(enum mpath_cmds cmd, char *dev, enum devtypes dev_type,
 				return ret;
 			}
 		}
-		conf = get_multipath_config();
-		pthread_cleanup_push(put_multipath_config, conf);
 		if (pp->udev && pp->uid_attribute &&
 		    filter_property(conf, pp->udev, 3, pp->uid_attribute) > 0)
-			invalid = 1;
-		pthread_cleanup_pop(1);
-		if (invalid)
 			return 2;
 		refwwid = pp->wwid;
 		goto out;
@@ -1455,24 +1431,16 @@ int get_refwwid(enum mpath_cmds cmd, char *dev, enum devtypes dev_type,
 		if (!udevice)
 			return 1;
 
-		conf = get_multipath_config();
-		pthread_cleanup_push(put_multipath_config, conf);
 		ret = store_pathinfo(pathvec, conf, udevice,
 				     flags, &pp);
-		pthread_cleanup_pop(1);
 		udev_device_unref(udevice);
 		if (!pp) {
 			if (ret == 1)
 				condlog(0, "%s: can't store path info", dev);
 			return ret;
 		}
-		conf = get_multipath_config();
-		pthread_cleanup_push(put_multipath_config, conf);
 		if (pp->udev && pp->uid_attribute &&
 		    filter_property(conf, pp->udev, 3, pp->uid_attribute) > 0)
-			invalid = 1;
-		pthread_cleanup_pop(1);
-		if (invalid)
 			return 2;
 		refwwid = pp->wwid;
 		goto out;
@@ -1480,8 +1448,6 @@ int get_refwwid(enum mpath_cmds cmd, char *dev, enum devtypes dev_type,
 
 	if (dev_type == DEV_DEVMAP) {
 
-		conf = get_multipath_config();
-		pthread_cleanup_push(put_multipath_config, conf);
 		if (((dm_get_uuid(dev, tmpwwid, WWID_SIZE)) == 0)
 		    && (strlen(tmpwwid))) {
 			refwwid = tmpwwid;
@@ -1512,9 +1478,6 @@ check:
 		if (refwwid && strlen(refwwid) &&
 		    filter_wwid(conf->blist_wwid, conf->elist_wwid, refwwid,
 				NULL) > 0)
-			invalid = 1;
-		pthread_cleanup_pop(1);
-		if (invalid)
 			return 2;
 	}
 out:
@@ -1526,6 +1489,25 @@ out:
 	return 1;
 }
 
+/*
+ * returns:
+ * 0 - success
+ * 1 - failure
+ * 2 - blacklist
+ */
+int get_refwwid(enum mpath_cmds cmd, char *dev, enum devtypes dev_type,
+		vector pathvec, char **wwid)
+
+{
+	int ret;
+	struct config *conf = get_multipath_config();
+
+	pthread_cleanup_push(put_multipath_config, conf);
+	ret = _get_refwwid(cmd, dev, dev_type, pathvec, conf, wwid);
+	pthread_cleanup_pop(1);
+	return ret;
+}
+
 int reload_map(struct vectors *vecs, struct multipath *mpp, int refresh,
 	       int is_daemon)
 {
-- 
2.26.2

[PATCH 49/54] libmultipath: get_refwwid(): get rid of "check" label

[mwilck]

From: Martin Wilck <mwilck@suse.com>

This was necessary with with the interspersed pthread_cleanup_push()/pop()
statements, now we can write the code more cleanly.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/configure.c | 31 +++++++++++--------------------
 1 file changed, 11 insertions(+), 20 deletions(-)

diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index c4712d7..defc54b 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -1449,32 +1449,23 @@ static int _get_refwwid(enum mpath_cmds cmd, char *dev, enum devtypes dev_type,
 	if (dev_type == DEV_DEVMAP) {
 
 		if (((dm_get_uuid(dev, tmpwwid, WWID_SIZE)) == 0)
-		    && (strlen(tmpwwid))) {
+		    && (strlen(tmpwwid)))
 			refwwid = tmpwwid;
-			goto check;
-		}
 
-		/*
-		 * may be a binding
-		 */
-		if (get_user_friendly_wwid(dev, tmpwwid,
-					   conf->bindings_file) == 0) {
+		/* or may be a binding */
+		else if (get_user_friendly_wwid(dev, tmpwwid,
+						conf->bindings_file) == 0)
 			refwwid = tmpwwid;
-			goto check;
-		}
 
-		/*
-		 * or may be an alias
-		 */
-		refwwid = get_mpe_wwid(conf->mptable, dev);
+		/* or may be an alias */
+		else {
+			refwwid = get_mpe_wwid(conf->mptable, dev);
 
-		/*
-		 * or directly a wwid
-		 */
-		if (!refwwid)
-			refwwid = dev;
+			/* or directly a wwid */
+			if (!refwwid)
+				refwwid = dev;
+		}
 
-check:
 		if (refwwid && strlen(refwwid) &&
 		    filter_wwid(conf->blist_wwid, conf->elist_wwid, refwwid,
 				NULL) > 0)
-- 
2.26.2

[PATCH 50/54] libmultipath: get_refwwid(): use symbolic return values

[mwilck]

From: Martin Wilck <mwilck@suse.com>

The return values of get_refwwid() are the same as those of pathinfo().
So use them.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/configure.c | 35 ++++++++++++++++-------------------
 1 file changed, 16 insertions(+), 19 deletions(-)

diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index defc54b..e68494b 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -1354,11 +1354,11 @@ static int _get_refwwid(enum mpath_cmds cmd, char *dev, enum devtypes dev_type,
 	int flags = DI_SYSFS | DI_WWID;
 
 	if (!wwid)
-		return 1;
+		return PATHINFO_FAILED;
 	*wwid = NULL;
 
 	if (dev_type == DEV_NONE)
-		return 1;
+		return PATHINFO_FAILED;
 
 	if (cmd != CMD_REMOVE_WWID)
 		flags |= DI_BLACKLIST;
@@ -1367,7 +1367,7 @@ static int _get_refwwid(enum mpath_cmds cmd, char *dev, enum devtypes dev_type,
 		if (basenamecpy(dev, buff, FILE_NAME_SIZE) == 0) {
 			condlog(1, "basename failed for '%s' (%s)",
 				dev, buff);
-			return 1;
+			return PATHINFO_FAILED;
 		}
 
 		pp = find_path_by_dev(pathvec, buff);
@@ -1376,13 +1376,13 @@ static int _get_refwwid(enum mpath_cmds cmd, char *dev, enum devtypes dev_type,
 				get_udev_device(buff, dev_type);
 
 			if (!udevice)
-				return 1;
+				return PATHINFO_FAILED;
 
 			ret = store_pathinfo(pathvec, conf, udevice,
 					     flags, &pp);
 			udev_device_unref(udevice);
 			if (!pp) {
-				if (ret == 1)
+				if (ret == PATHINFO_FAILED)
 					condlog(0, "%s: can't store path info",
 						dev);
 				return ret;
@@ -1390,7 +1390,7 @@ static int _get_refwwid(enum mpath_cmds cmd, char *dev, enum devtypes dev_type,
 		}
 		if (pp->udev && pp->uid_attribute &&
 		    filter_property(conf, pp->udev, 3, pp->uid_attribute) > 0)
-			return 2;
+			return PATHINFO_SKIPPED;
 
 		refwwid = pp->wwid;
 		goto out;
@@ -1405,14 +1405,14 @@ static int _get_refwwid(enum mpath_cmds cmd, char *dev, enum devtypes dev_type,
 
 			if (!udevice) {
 				condlog(0, "%s: cannot find block device", dev);
-				return 1;
+				return PATHINFO_FAILED;
 			}
 
 			ret = store_pathinfo(pathvec, conf, udevice,
 					     flags, &pp);
 			udev_device_unref(udevice);
 			if (!pp) {
-				if (ret == 1)
+				if (ret == PATHINFO_FAILED)
 					condlog(0, "%s can't store path info",
 						buff);
 				return ret;
@@ -1420,7 +1420,7 @@ static int _get_refwwid(enum mpath_cmds cmd, char *dev, enum devtypes dev_type,
 		}
 		if (pp->udev && pp->uid_attribute &&
 		    filter_property(conf, pp->udev, 3, pp->uid_attribute) > 0)
-			return 2;
+			return PATHINFO_SKIPPED;
 		refwwid = pp->wwid;
 		goto out;
 	}
@@ -1429,19 +1429,19 @@ static int _get_refwwid(enum mpath_cmds cmd, char *dev, enum devtypes dev_type,
 		struct udev_device *udevice = get_udev_device(dev, dev_type);
 
 		if (!udevice)
-			return 1;
+			return PATHINFO_FAILED;
 
 		ret = store_pathinfo(pathvec, conf, udevice,
 				     flags, &pp);
 		udev_device_unref(udevice);
 		if (!pp) {
-			if (ret == 1)
+			if (ret == PATHINFO_FAILED)
 				condlog(0, "%s: can't store path info", dev);
 			return ret;
 		}
 		if (pp->udev && pp->uid_attribute &&
 		    filter_property(conf, pp->udev, 3, pp->uid_attribute) > 0)
-			return 2;
+			return PATHINFO_SKIPPED;
 		refwwid = pp->wwid;
 		goto out;
 	}
@@ -1469,22 +1469,19 @@ static int _get_refwwid(enum mpath_cmds cmd, char *dev, enum devtypes dev_type,
 		if (refwwid && strlen(refwwid) &&
 		    filter_wwid(conf->blist_wwid, conf->elist_wwid, refwwid,
 				NULL) > 0)
-			return 2;
+			return PATHINFO_SKIPPED;
 	}
 out:
 	if (refwwid && strlen(refwwid)) {
 		*wwid = STRDUP(refwwid);
-		return 0;
+		return PATHINFO_OK;
 	}
 
-	return 1;
+	return PATHINFO_FAILED;
 }
 
 /*
- * returns:
- * 0 - success
- * 1 - failure
- * 2 - blacklist
+ * Returns: PATHINFO_OK, PATHINFO_FAILED, or PATHINFO_SKIPPED (see pathinfo())
  */
 int get_refwwid(enum mpath_cmds cmd, char *dev, enum devtypes dev_type,
 		vector pathvec, char **wwid)
-- 
2.26.2

[PATCH 51/54] libmultipath: get_refwwid(): use switch statement

[mwilck]

From: Martin Wilck <mwilck@suse.com>

This code calls for a switch. Some additional compaction is possible by
observing that the code for DEV_DEVNODE, DEV_DEVT, and DEV_UEVENT is almost
the same, and factoring it out into a "common" section. Doing this with
a goto inside the switch statement is a bit unusual, but shows the intention
of the code more clearly than other variants I tried.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/configure.c | 81 ++++++++++++----------------------------
 1 file changed, 24 insertions(+), 57 deletions(-)

diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index e68494b..db9a255 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -1351,6 +1351,7 @@ static int _get_refwwid(enum mpath_cmds cmd, char *dev, enum devtypes dev_type,
 	struct path * pp;
 	char buff[FILE_NAME_SIZE];
 	char * refwwid = NULL, tmpwwid[WWID_SIZE];
+	struct udev_device *udevice;
 	int flags = DI_SYSFS | DI_WWID;
 
 	if (!wwid)
@@ -1363,45 +1364,31 @@ static int _get_refwwid(enum mpath_cmds cmd, char *dev, enum devtypes dev_type,
 	if (cmd != CMD_REMOVE_WWID)
 		flags |= DI_BLACKLIST;
 
-	if (dev_type == DEV_DEVNODE) {
+	switch (dev_type) {
+	case DEV_DEVNODE:
 		if (basenamecpy(dev, buff, FILE_NAME_SIZE) == 0) {
 			condlog(1, "basename failed for '%s' (%s)",
 				dev, buff);
 			return PATHINFO_FAILED;
 		}
 
-		pp = find_path_by_dev(pathvec, buff);
-		if (!pp) {
-			struct udev_device *udevice =
-				get_udev_device(buff, dev_type);
+		/* dev is used in common code below */
+		dev = buff;
+		pp = find_path_by_dev(pathvec, dev);
+		goto common;
 
-			if (!udevice)
-				return PATHINFO_FAILED;
-
-			ret = store_pathinfo(pathvec, conf, udevice,
-					     flags, &pp);
-			udev_device_unref(udevice);
-			if (!pp) {
-				if (ret == PATHINFO_FAILED)
-					condlog(0, "%s: can't store path info",
-						dev);
-				return ret;
-			}
-		}
-		if (pp->udev && pp->uid_attribute &&
-		    filter_property(conf, pp->udev, 3, pp->uid_attribute) > 0)
-			return PATHINFO_SKIPPED;
-
-		refwwid = pp->wwid;
-		goto out;
-	}
-
-	if (dev_type == DEV_DEVT) {
+	case DEV_DEVT:
 		strchop(dev);
 		pp = find_path_by_devt(pathvec, dev);
+		goto common;
+
+	case DEV_UEVENT:
+		pp = NULL;
+		/* For condlog below, dev is unused in get_udev_device() */
+		dev = "environment";
+	common:
 		if (!pp) {
-			struct udev_device *udevice =
-				get_udev_device(dev, dev_type);
+			udevice = get_udev_device(dev, dev_type);
 
 			if (!udevice) {
 				condlog(0, "%s: cannot find block device", dev);
@@ -1413,8 +1400,8 @@ static int _get_refwwid(enum mpath_cmds cmd, char *dev, enum devtypes dev_type,
 			udev_device_unref(udevice);
 			if (!pp) {
 				if (ret == PATHINFO_FAILED)
-					condlog(0, "%s can't store path info",
-						buff);
+					condlog(0, "%s: can't store path info",
+						dev);
 				return ret;
 			}
 		}
@@ -1422,32 +1409,9 @@ static int _get_refwwid(enum mpath_cmds cmd, char *dev, enum devtypes dev_type,
 		    filter_property(conf, pp->udev, 3, pp->uid_attribute) > 0)
 			return PATHINFO_SKIPPED;
 		refwwid = pp->wwid;
-		goto out;
-	}
-
-	if (dev_type == DEV_UEVENT) {
-		struct udev_device *udevice = get_udev_device(dev, dev_type);
-
-		if (!udevice)
-			return PATHINFO_FAILED;
-
-		ret = store_pathinfo(pathvec, conf, udevice,
-				     flags, &pp);
-		udev_device_unref(udevice);
-		if (!pp) {
-			if (ret == PATHINFO_FAILED)
-				condlog(0, "%s: can't store path info", dev);
-			return ret;
-		}
-		if (pp->udev && pp->uid_attribute &&
-		    filter_property(conf, pp->udev, 3, pp->uid_attribute) > 0)
-			return PATHINFO_SKIPPED;
-		refwwid = pp->wwid;
-		goto out;
-	}
-
-	if (dev_type == DEV_DEVMAP) {
+		break;
 
+	case DEV_DEVMAP:
 		if (((dm_get_uuid(dev, tmpwwid, WWID_SIZE)) == 0)
 		    && (strlen(tmpwwid)))
 			refwwid = tmpwwid;
@@ -1470,8 +1434,11 @@ static int _get_refwwid(enum mpath_cmds cmd, char *dev, enum devtypes dev_type,
 		    filter_wwid(conf->blist_wwid, conf->elist_wwid, refwwid,
 				NULL) > 0)
 			return PATHINFO_SKIPPED;
+		break;
+	default:
+		break;
 	}
-out:
+
 	if (refwwid && strlen(refwwid)) {
 		*wwid = STRDUP(refwwid);
 		return PATHINFO_OK;
-- 
2.26.2

[PATCH 52/54] libmultipath: constify get_mpe_wwid()

[mwilck]

From: Martin Wilck <mwilck@suse.com>

As this returns a pointer to a struct member, the return value
should also be a const char*.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/config.c    | 2 +-
 libmultipath/config.h    | 2 +-
 libmultipath/configure.c | 3 ++-
 3 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/libmultipath/config.c b/libmultipath/config.c
index 658bec8..69a2723 100644
--- a/libmultipath/config.c
+++ b/libmultipath/config.c
@@ -157,7 +157,7 @@ struct mpentry *find_mpe(vector mptable, char *wwid)
 	return NULL;
 }
 
-char *get_mpe_wwid(vector mptable, char *alias)
+const char *get_mpe_wwid(const struct _vector *mptable, const char *alias)
 {
 	int i;
 	struct mpentry * mpe;
diff --git a/libmultipath/config.h b/libmultipath/config.h
index 92c61a0..2bb7153 100644
--- a/libmultipath/config.h
+++ b/libmultipath/config.h
@@ -239,7 +239,7 @@ int find_hwe (const struct _vector *hwtable,
 	      const char * vendor, const char * product, const char *revision,
 	      vector result);
 struct mpentry * find_mpe (vector mptable, char * wwid);
-char * get_mpe_wwid (vector mptable, char * alias);
+const char *get_mpe_wwid (const struct _vector *mptable, const char *alias);
 
 struct hwentry * alloc_hwe (void);
 struct mpentry * alloc_mpe (void);
diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index db9a255..7461e99 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -1350,7 +1350,8 @@ static int _get_refwwid(enum mpath_cmds cmd, char *dev, enum devtypes dev_type,
 	int ret = 1;
 	struct path * pp;
 	char buff[FILE_NAME_SIZE];
-	char * refwwid = NULL, tmpwwid[WWID_SIZE];
+	const char *refwwid = NULL;
+	char tmpwwid[WWID_SIZE];
 	struct udev_device *udevice;
 	int flags = DI_SYSFS | DI_WWID;
 
-- 
2.26.2

[PATCH 53/54] multipath: call strchop() on command line argument

[mwilck]

From: Martin Wilck <mwilck@suse.com>

It's useful to sanitize these right away. We can't do this for DEV_DEVMAP,
as aliases with trailing whitespace aren't strictly forbidden, but for
the other types we can.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipath/main.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/multipath/main.c b/multipath/main.c
index 4c43314..cfb85dc 100644
--- a/multipath/main.c
+++ b/multipath/main.c
@@ -1028,6 +1028,8 @@ main (int argc, char *argv[])
 			condlog(0, "'%s' is not a valid argument\n", dev);
 			goto out;
 		}
+		if (dev_type == DEV_DEVNODE || dev_type == DEV_DEVT)
+			strchop(dev);
 	}
 	if (dev_type == DEV_UEVENT) {
 		openlog("multipath", 0, LOG_DAEMON);
-- 
2.26.2

[PATCH 54/54] libmultipath: get_refwwid(): skip strchop(), and constify dev parameter

[mwilck]

From: Martin Wilck <mwilck@suse.com>

With the previous change, we can safely assume that strchop() has been
called already where appropriate (the only caller is multipath's
configure()). We can now use const char* for the "dev" parameter.

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

diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index 7461e99..48426cd 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -1344,7 +1344,8 @@ struct udev_device *get_udev_device(const char *dev, enum devtypes dev_type)
 	return ud;
 }
 
-static int _get_refwwid(enum mpath_cmds cmd, char *dev, enum devtypes dev_type,
+static int _get_refwwid(enum mpath_cmds cmd, const char *dev,
+			enum devtypes dev_type,
 			vector pathvec, struct config *conf, char **wwid)
 {
 	int ret = 1;
@@ -1379,7 +1380,6 @@ static int _get_refwwid(enum mpath_cmds cmd, char *dev, enum devtypes dev_type,
 		goto common;
 
 	case DEV_DEVT:
-		strchop(dev);
 		pp = find_path_by_devt(pathvec, dev);
 		goto common;
 
@@ -1451,7 +1451,7 @@ static int _get_refwwid(enum mpath_cmds cmd, char *dev, enum devtypes dev_type,
 /*
  * Returns: PATHINFO_OK, PATHINFO_FAILED, or PATHINFO_SKIPPED (see pathinfo())
  */
-int get_refwwid(enum mpath_cmds cmd, char *dev, enum devtypes dev_type,
+int get_refwwid(enum mpath_cmds cmd, const char *dev, enum devtypes dev_type,
 		vector pathvec, char **wwid)
 
 {
diff --git a/libmultipath/configure.h b/libmultipath/configure.h
index 0e33bf4..d9a7de6 100644
--- a/libmultipath/configure.h
+++ b/libmultipath/configure.h
@@ -52,7 +52,7 @@ int setup_map (struct multipath * mpp, char * params, int params_size,
 int domap (struct multipath * mpp, char * params, int is_daemon);
 int reinstate_paths (struct multipath *mpp);
 int coalesce_paths (struct vectors *vecs, vector curmp, char * refwwid, int force_reload, enum mpath_cmds cmd);
-int get_refwwid (enum mpath_cmds cmd, char * dev, enum devtypes dev_type,
+int get_refwwid (enum mpath_cmds cmd, const char *dev, enum devtypes dev_type,
 		 vector pathvec, char **wwid);
 int reload_map(struct vectors *vecs, struct multipath *mpp, int refresh, int is_daemon);
 struct udev_device *get_udev_device(const char *dev, enum devtypes dev_type);
-- 
2.26.2

Re: [PATCH 44/54] libmultipath: adopt_paths(): don't bail out on single path failure

[Benjamin Marzinski]

On Thu, Jul 09, 2020 at 12:36:13PM +0200, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> If pathinfo fails for one path to be adopted, we currently
> fail the entire function. This may cause ev_add_path() for a valid
> path to fail because some other path is broken. Fix it by just
> skipping paths that don't look healthy.

This looks problematic to me.  While I agree that we shouldn't make
ev_add_path fail because some other path failed, but what about if the
path we are trying to add fails in pathinfo().  In this case multipathd
won't orphan the path and it will report "path added to devmap", even
though it wasn't. Also what is the correct response for when you try
to create a multipath device but none of the paths can be added.
Should multipathd create a device with no paths? 
 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/structs_vec.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
> index 55fca9b..bc47d1e 100644
> --- a/libmultipath/structs_vec.c
> +++ b/libmultipath/structs_vec.c
> @@ -75,16 +75,20 @@ int adopt_paths(vector pathvec, struct multipath *mpp)
>  			if (!mpp->paths && !(mpp->paths = vector_alloc()))
>  				return 1;
>  
> -			if (!find_path_by_devt(mpp->paths, pp->dev_t) &&
> -			    store_path(mpp->paths, pp))
> -					return 1;
>  			conf = get_multipath_config();
>  			pthread_cleanup_push(put_multipath_config, conf);
>  			ret = pathinfo(pp, conf,
>  				       DI_PRIO | DI_CHECKER);
>  			pthread_cleanup_pop(1);
> -			if (ret)
> -				return 1;
> +			if (ret) {
> +				condlog(3, "%s: pathinfo failed for %s",
> +					__func__, pp->dev);
> +				continue;
> +			}
> +
> +			if (!find_path_by_devt(mpp->paths, pp->dev_t) &&
> +			    store_path(mpp->paths, pp))
> +					return 1;
>  		}
>  	}
>  	return 0;
> -- 
> 2.26.2

Re: [PATCH 46/54] libmultipath: path_discover(): always set DI_BLACKLIST

[Benjamin Marzinski]

On Thu, Jul 09, 2020 at 12:36:15PM +0200, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> Since 65e1845 ("multipath: call store_pathinfo with DI_BLACKLIST"), we
> use DI_BLACKLIST for new paths. There's no reason why we shouldn't do the
> same with paths which are (unexpectedly) already in pathvec. As argued
> for 65e1845, this might save some unnecessary work for paths which are
> blacklisted anyway.

It seems to me like either we should assume that if we added the path to
pathvec, it's valid, or we should check, and if it's blacklisted, remove
it. Just leaving it on pathvec without all of the pathinfo work done
doesn't make much sense to me. Am I missing something here?

-Ben
 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/discovery.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
> index 5f4ebf0..caabfef 100644
> --- a/libmultipath/discovery.c
> +++ b/libmultipath/discovery.c
> @@ -137,7 +137,7 @@ path_discover (vector pathvec, struct config * conf,
>  				      udevice, flag | DI_BLACKLIST,
>  				      NULL);
>  	else
> -		return pathinfo(pp, conf, flag);
> +		return pathinfo(pp, conf, flag | DI_BLACKLIST);
>  }
>  
>  static void cleanup_udev_enumerate_ptr(void *arg)
> -- 
> 2.26.2

Re: [PATCH 00/54] multipath-tools series part IV: identify paths by dev_t

[Benjamin Marzinski]

On Thu, Jul 09, 2020 at 12:36:11PM +0200, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> Hi Christophe, hi Ben,
> 
> This is part IV of a larger patch series for multipath-tools I've been preparing.
> It's based on the previously submitted part III.
> 
> The full series will also be available here:
> https://github.com/mwilck/multipath-tools/tree/ups/submit-2007
> 
> There are tags in that repo for each part of the series.
> This part is tagged "submit-200709-4".

For the part, with the exception of patches 44 & 46
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
 
> The dominant idea of the patches in this subseries is to move away from
> the device node name as the main path identifier towards the dev_t aka
> major/minor number. This is motivated by the fact that dev_t is used by the
> kernel device mapper for referring to devices, both maps and paths.
> If searching a device by dev_t fails, it's basically certain not to exist.
> DM calls with major/minor number may even succeed if the respective devnode
> has not been created yet. dev_t also works better for devices that aren't
> fully initialized yet. Of course we still track the device name, but mostly
> for log messages and user-directed output.
> 
> In short, using dev_t as primary identifier makes our code more robust.
> 
> The subseries also contains some unrelated fixes for functions I worked on
> while making these changes.
> 
> Regards,
> Martin
> 
> Martin Wilck (12):
>   libmultipath: adopt_paths(): use find_path_by_devt()
>   libmultipath: adopt_paths(): don't bail out on single path failure
>   libmultipath: path_discover(): use find_path_by_devt()
>   libmultipath: path_discover(): always set DI_BLACKLIST
>   libmultipath: get_refwwid(): use find_path_by_devt()
>   libmultipath: get_refwwid(): call get_multipath_config() only once
>   libmultipath: get_refwwid(): get rid of "check" label
>   libmultipath: get_refwwid(): use symbolic return values
>   libmultipath: get_refwwid(): use switch statement
>   libmultipath: constify get_mpe_wwid()
>   multipath: call strchop() on command line argument
>   libmultipath: get_refwwid(): skip strchop(), and constify dev
>     parameter
> 
>  libmultipath/config.c      |   2 +-
>  libmultipath/config.h      |   2 +-
>  libmultipath/configure.c   | 200 +++++++++++++------------------------
>  libmultipath/configure.h   |   2 +-
>  libmultipath/discovery.c   |  32 +++---
>  libmultipath/structs_vec.c |  14 ++-
>  multipath/main.c           |   2 +
>  7 files changed, 94 insertions(+), 160 deletions(-)
> 
> -- 
> 2.26.2

Re: [PATCH 44/54] libmultipath: adopt_paths(): don't bail out on single path failure

[Martin Wilck]

On Fri, 2020-07-17 at 16:25 -0500, Benjamin Marzinski wrote:
> On Thu, Jul 09, 2020 at 12:36:13PM +0200, mwilck@suse.com wrote:
> > From: Martin Wilck <mwilck@suse.com>
> > 
> > If pathinfo fails for one path to be adopted, we currently
> > fail the entire function. This may cause ev_add_path() for a valid
> > path to fail because some other path is broken. Fix it by just
> > skipping paths that don't look healthy.
> 
> This looks problematic to me.  While I agree that we shouldn't make
> ev_add_path fail because some other path failed, but what about if
> the
> path we are trying to add fails in pathinfo().  In this case
> multipathd
> won't orphan the path and it will report "path added to devmap", even
> though it wasn't. Also what is the correct response for when you try
> to create a multipath device but none of the paths can be added.
> Should multipathd create a device with no paths? 

There are 3 callers of adopt_path():

 1 add_map_with_path()
 2 ev_add_path()
 3 update_map()

adopt_paths() only ever adds new paths to a map, so in case 3), the map
would not come out without paths unless it had been empty before
already. For this caller, my patch is definitely an improvement.

For 1) and 2), we are adding a specific path, so we should test in the
caller whether that specific path was successfully added, and fail
otherwise. Would that address your concern?

Regards,
Martin

Re: [PATCH 46/54] libmultipath: path_discover(): always set DI_BLACKLIST

[Martin Wilck]

On Fri, 2020-07-17 at 17:21 -0500, Benjamin Marzinski wrote:
> On Thu, Jul 09, 2020 at 12:36:15PM +0200, mwilck@suse.com wrote:
> > From: Martin Wilck <mwilck@suse.com>
> > 
> > Since 65e1845 ("multipath: call store_pathinfo with DI_BLACKLIST"),
> > we
> > use DI_BLACKLIST for new paths. There's no reason why we shouldn't
> > do the
> > same with paths which are (unexpectedly) already in pathvec. As
> > argued
> > for 65e1845, this might save some unnecessary work for paths which
> > are
> > blacklisted anyway.
> 
> It seems to me like either we should assume that if we added the path
> to
> pathvec, it's valid, or we should check, and if it's blacklisted,
> remove
> it. Just leaving it on pathvec without all of the pathinfo work done
> doesn't make much sense to me. Am I missing something here?

TL;DR: let's skip this patch for now.

path_discover() is only called from path_discovery(). path_discovery()
is called from 

 1 configure() (multipathd), with an empty pathvec
 2 configure() (multipath), with a pathvec possibly pre-populated by
get_refwwid()
 3 check_valid_path(), with pathvec pre-populated with the path to be
checked

For 1) my patch obviously makes no difference. In case 2) get_refwwid()
sets DI_BLACKLIST anyway (except for CMD_REMOVE_WWID, in which case
path_discovery() is not called). In case 3), DI_BLACKLIST
has already been used by is_path_valid() (the only exception being the
"is already multipathed" case, in which path_discovery() is not
called).

So, this patch makes no difference in practice - all callers make sure
that the pathvec is pre-populated only with non-blacklisted paths.

The reason I wrote this patch was because I wanted to use consistent
flags in path_discover() - it was non-obvious to me why we treated
existing and new paths differently. The side effect was a very small
speed-up. 

But your argument above is valid. 

To me it would come as a surprise if path_discovery() removed paths;
that's the kind of side effect I'd like to minimize in our code. Thus,
path_discover() should rely on the caller wrt the pre-populated paths,
and not use DI_BLACKLIST on them.

For now, I'll replace this patch with a comment explaining why we use
different flags in the two pathinfo() calls. 

In the future, we may want to change path_discovery() such that it only
works on an empty pathvec. IMO that would be cleaner, and require only
minimal changes to callers.

Thanks,
Martin


> -Ben
>  
> > Signed-off-by: Martin Wilck <mwilck@suse.com>
> > ---
> >  libmultipath/discovery.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
> > index 5f4ebf0..caabfef 100644
> > --- a/libmultipath/discovery.c
> > +++ b/libmultipath/discovery.c
> > @@ -137,7 +137,7 @@ path_discover (vector pathvec, struct config *
> > conf,
> >  				      udevice, flag | DI_BLACKLIST,
> >  				      NULL);
> >  	else
> > -		return pathinfo(pp, conf, flag);
> > +		return pathinfo(pp, conf, flag | DI_BLACKLIST);
> >  }
> >  
> >  static void cleanup_udev_enumerate_ptr(void *arg)
> > -- 
> > 2.26.2

Re: [PATCH 44/54] libmultipath: adopt_paths(): don't bail out on single path failure

[Benjamin Marzinski]

On Wed, Aug 05, 2020 at 02:05:00PM +0200, Martin Wilck wrote:
> On Fri, 2020-07-17 at 16:25 -0500, Benjamin Marzinski wrote:
> > On Thu, Jul 09, 2020 at 12:36:13PM +0200, mwilck@suse.com wrote:
> > > From: Martin Wilck <mwilck@suse.com>
> > > 
> > > If pathinfo fails for one path to be adopted, we currently
> > > fail the entire function. This may cause ev_add_path() for a valid
> > > path to fail because some other path is broken. Fix it by just
> > > skipping paths that don't look healthy.
> > 
> > This looks problematic to me.  While I agree that we shouldn't make
> > ev_add_path fail because some other path failed, but what about if
> > the
> > path we are trying to add fails in pathinfo().  In this case
> > multipathd
> > won't orphan the path and it will report "path added to devmap", even
> > though it wasn't. Also what is the correct response for when you try
> > to create a multipath device but none of the paths can be added.
> > Should multipathd create a device with no paths? 
> 
> There are 3 callers of adopt_path():
> 
>  1 add_map_with_path()
>  2 ev_add_path()
>  3 update_map()
> 
> adopt_paths() only ever adds new paths to a map, so in case 3), the map
> would not come out without paths unless it had been empty before
> already. For this caller, my patch is definitely an improvement.
> 
> For 1) and 2), we are adding a specific path, so we should test in the
> caller whether that specific path was successfully added, and fail
> otherwise. Would that address your concern?

That sounds reasonable

-Ben

> 
> Regards,
> Martin
> 
>