[RFC PATCH v2 0/3] multipath: new path validation library

[RFC PATCH v2 0/3] multipath: new path validation library

[Benjamin Marzinski]

This patchset is for a new library that programs can use to determine
if a device belongs to multipath.  The primary user that this is
intended for is SID, the Storage Instantiation Daemon

https://github.com/sid-project

Right now, this doesn't change our existing code to determine path
ownership, and it doesn't do the exact same steps, although it is very
close.  In the future, it would be possible to pull most of this code
entirely into libmultipath, except for some wrappers, and use it for
both methods.  Obviously, this still needs man pages, and there are some
helper functions for things like controlling multipath's logging that
are missing, but I want to see if anyone has strong feelings about what
this looks like.

I also have two more changes that I want to make to the multipath code,
to make path validation do less unnecessary work, which aren't in this
patchset.

1. I want to remove the lock file from the failed wwids code. I don't
see how it actually stops any races, and it means that simply reading
a file, can trigger delays and writes (albeit to a memory base fs).

2. I want to deprecate getuid_callout.  Once this is gone, you will be
able to call pathinfo and get a path's WWID, without ever needing to
open the path.

changes in v2:
0002: make sysfs_is_multipathed only read the sysfs file once, as
suggested by Martin.

0003: dm_is_mpath_uuid() is now dm_map_present_by_uuid(). The library
includes a new function mpath_get_mode(), to get the find_multipaths
mode, and the modes now include MPATH_FIND. mpath_is_path() now accepts
an array of mpath_infos, which the caller can use to pass the previous
path wwids. This allows mpath_is_path() to return MPATH_IS_VALID for
paths if there already is another path with that wwid.

However, mpath_is_path() still treats MPATH_SMART and MPATH_FIND the
same.  I tried to make them work differently, but I realized that I need
a way to signal that the MPATH_FIND path didn't fail because it was
blacklisted, but instead because it needed another paths. Otherwise the
caller won't know that it needs to save the wwid to check when later
paths appear. This is exactly what MPATH_IS_MAYBE_VALID means. In the
multipath -u code, the only difference between the find_multipaths "on"
and "smart" case is what to do when a path that needs another path
appears for the first time.  Dealing with this difference is the
responsiblity of the caller of the mpathvalid library. mpath_get_mode(),
will let it know what the configured find_multipaths mode is.

Benjamin Marzinski (3):
  libmultipath: make libmp_dm_init optional
  libmultipath: make sysfs_is_multipathed able to return wwid
  multipath: add libmpathvalid library

 Makefile                            |   1 +
 Makefile.inc                        |   1 +
 libmpathvalid/Makefile              |  38 ++++++
 libmpathvalid/libmpathvalid.version |   7 +
 libmpathvalid/mpath_valid.c         | 198 ++++++++++++++++++++++++++++
 libmpathvalid/mpath_valid.h         |  56 ++++++++
 libmultipath/Makefile               |   1 +
 libmultipath/devmapper.c            |  66 +++++++++-
 libmultipath/devmapper.h            |   4 +-
 libmultipath/sysfs.c                |  24 +++-
 libmultipath/sysfs.h                |   2 +-
 multipath/main.c                    |   7 +-
 12 files changed, 391 insertions(+), 14 deletions(-)
 create mode 100644 libmpathvalid/Makefile
 create mode 100644 libmpathvalid/libmpathvalid.version
 create mode 100644 libmpathvalid/mpath_valid.c
 create mode 100644 libmpathvalid/mpath_valid.h

-- 
2.17.2

[RFC PATCH v2 1/3] libmultipath: make libmp_dm_init optional

[Benjamin Marzinski]

Move dm_initialized out of libmp_dm_task_create(), and add
a function skip_libmp_dm_init() so that users of libmultipath can skip
initializing device-mapper. This is needed for other programs that
use libmultipath (or a library that depends on it) but want to control
how device-mapper is set up.

Also make dm_prereq a global function.

Reviewed-by: Martin Wilck <mwilck@suse.com>
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/devmapper.c | 17 +++++++++++++----
 libmultipath/devmapper.h |  3 ++-
 2 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
index 13a1cf53..7ed494a1 100644
--- a/libmultipath/devmapper.c
+++ b/libmultipath/devmapper.c
@@ -33,6 +33,8 @@
 #define MAX_WAIT 5
 #define LOOPS_PER_SEC 5
 
+static pthread_once_t dm_initialized = PTHREAD_ONCE_INIT;
+
 static int dm_conf_verbosity;
 
 #ifdef LIBDM_API_DEFERRED
@@ -229,7 +231,7 @@ dm_tgt_prereq (unsigned int *ver)
 	return 1;
 }
 
-static int dm_prereq(unsigned int *v)
+int dm_prereq(unsigned int *v)
 {
 	if (dm_lib_prereq())
 		return 1;
@@ -243,7 +245,7 @@ void libmp_udev_set_sync_support(int on)
 	libmp_dm_udev_sync = !!on;
 }
 
-void libmp_dm_init(void)
+static void libmp_dm_init(void)
 {
 	struct config *conf;
 	int verbosity;
@@ -262,11 +264,18 @@ void libmp_dm_init(void)
 	dm_udev_set_sync_support(libmp_dm_udev_sync);
 }
 
+static void _do_skip_libmp_dm_init(void)
+{
+}
+
+void skip_libmp_dm_init(void)
+{
+	pthread_once(&dm_initialized, _do_skip_libmp_dm_init);
+}
+
 struct dm_task*
 libmp_dm_task_create(int task)
 {
-	static pthread_once_t dm_initialized = PTHREAD_ONCE_INIT;
-
 	pthread_once(&dm_initialized, libmp_dm_init);
 	return dm_task_create(task);
 }
diff --git a/libmultipath/devmapper.h b/libmultipath/devmapper.h
index 7557a86b..17fc9faf 100644
--- a/libmultipath/devmapper.h
+++ b/libmultipath/devmapper.h
@@ -28,7 +28,8 @@
 #define UUID_PREFIX_LEN (sizeof(UUID_PREFIX) - 1)
 
 void dm_init(int verbosity);
-void libmp_dm_init(void);
+int dm_prereq(unsigned int *v);
+void skip_libmp_dm_init(void);
 void libmp_udev_set_sync_support(int on);
 struct dm_task *libmp_dm_task_create(int task);
 int dm_drv_version (unsigned int * version);
-- 
2.17.2

[RFC PATCH v2 2/3] libmultipath: make sysfs_is_multipathed able to return wwid

[Benjamin Marzinski]

sysfs_is_multipathed reads the wwid of the dm device holding a path to
check if its a multipath device.  Add code to optinally set pp->wwid to
that wwid.  This will be used by a future patch.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/sysfs.c | 24 +++++++++++++++++++-----
 libmultipath/sysfs.h |  2 +-
 multipath/main.c     |  7 ++++---
 3 files changed, 24 insertions(+), 9 deletions(-)

diff --git a/libmultipath/sysfs.c b/libmultipath/sysfs.c
index 62ec2ed7..12a82d95 100644
--- a/libmultipath/sysfs.c
+++ b/libmultipath/sysfs.c
@@ -295,7 +295,7 @@ static int select_dm_devs(const struct dirent *di)
 	return fnmatch("dm-*", di->d_name, FNM_FILE_NAME) == 0;
 }
 
-bool sysfs_is_multipathed(const struct path *pp)
+bool sysfs_is_multipathed(struct path *pp, bool set_wwid)
 {
 	char pathbuf[PATH_MAX];
 	struct scandir_result sr;
@@ -325,7 +325,7 @@ bool sysfs_is_multipathed(const struct path *pp)
 	for (i = 0; i < r && !found; i++) {
 		long fd;
 		int nr;
-		char uuid[6];
+		char uuid[WWID_SIZE + UUID_PREFIX_LEN];
 
 		if (safe_snprintf(pathbuf + n, sizeof(pathbuf) - n,
 				  "/%s/dm/uuid", di[i]->d_name))
@@ -339,12 +339,26 @@ bool sysfs_is_multipathed(const struct path *pp)
 
 		pthread_cleanup_push(close_fd, (void *)fd);
 		nr = read(fd, uuid, sizeof(uuid));
-		if (nr == sizeof(uuid) && !memcmp(uuid, "mpath-", sizeof(uuid)))
+		if (nr > (int)UUID_PREFIX_LEN &&
+		    !memcmp(uuid, UUID_PREFIX, UUID_PREFIX_LEN))
 			found = true;
 		else if (nr < 0) {
-			condlog(1, "%s: error reading from %s: %s",
-				__func__, pathbuf, strerror(errno));
+			condlog(1, "%s: error reading from %s: %m",
+				__func__, pathbuf);
 		}
+		if (found && set_wwid) {
+			nr -= UUID_PREFIX_LEN;
+			memcpy(pp->wwid, uuid + UUID_PREFIX_LEN, nr);
+			if (nr == WWID_SIZE) {
+				condlog(4, "%s: overflow while reading from %s",
+					__func__, pathbuf);
+				pp->wwid[0] = '\0';
+			} else {
+				pp->wwid[nr] = '\0';
+				strchop(pp->wwid);
+			}
+                }
+
 		pthread_cleanup_pop(1);
 	}
 	pthread_cleanup_pop(1);
diff --git a/libmultipath/sysfs.h b/libmultipath/sysfs.h
index 9ae30b39..72b39ab2 100644
--- a/libmultipath/sysfs.h
+++ b/libmultipath/sysfs.h
@@ -14,5 +14,5 @@ ssize_t sysfs_bin_attr_get_value(struct udev_device *dev, const char *attr_name,
 				 unsigned char * value, size_t value_len);
 int sysfs_get_size (struct path *pp, unsigned long long * size);
 int sysfs_check_holders(char * check_devt, char * new_devt);
-bool sysfs_is_multipathed(const struct path *pp);
+bool sysfs_is_multipathed(struct path *pp, bool set_wwid);
 #endif
diff --git a/multipath/main.c b/multipath/main.c
index cf9d2a28..545ead87 100644
--- a/multipath/main.c
+++ b/multipath/main.c
@@ -638,7 +638,8 @@ configure (struct config *conf, enum mpath_cmds cmd,
 			 * Shortcut for find_multipaths smart:
 			 * Quick check if path is already multipathed.
 			 */
-			if (sysfs_is_multipathed(VECTOR_SLOT(pathvec, 0))) {
+			if (sysfs_is_multipathed(VECTOR_SLOT(pathvec, 0),
+						 false)) {
 				r = RTVL_YES;
 				goto print_valid;
 			}
@@ -747,8 +748,8 @@ configure (struct config *conf, enum mpath_cmds cmd,
 			/*
 			 * Check if we raced with multipathd
 			 */
-			r = sysfs_is_multipathed(VECTOR_SLOT(pathvec, 0)) ?
-				RTVL_YES : RTVL_NO;
+			r = sysfs_is_multipathed(VECTOR_SLOT(pathvec, 0),
+						 false) ? RTVL_YES : RTVL_NO;
 		}
 		goto print_valid;
 	}
-- 
2.17.2

[RFC PATCH v2 3/3] multipath: add libmpathvalid library

[Benjamin Marzinski]

This library allows other programs to check if a path should be claimed
by multipath.  Currently, it only includes two functions.
mpath_get_mode() get the configured find_multipaths mode.
mpath_is_path() returns whether the device is claimed by multipath.
It optionally also returns the wwid. The modes work
mostly like they do for "multipath -c/-u", with a few exceptions.

1. If a device is currently multipathed, it is always VALID. This
perhaps should be true for the existing path valid code.

2. If the mode is MPATH_FIND, and the device would be claimed if there
was another path with the same wwid, but no matching wwid was passed in,
mpath_is_path() will return MPATH_IS_MAYBE_VALID, just like if the
find_multipaths mode was MPATH_SMART. This is so the caller knows to
save this wwid to check against future paths.  It is the callers job to
honor the difference between configurations with MPATH_FIND and
MPATH_SMART.

In order to act more library-like, libmpathvalid doesn't set its own
device-mapper log functions. It leaves this to the caller. It currently
keeps condlog() from printing anything, but should eventually include a
function to allow the caller set the logging. It also uses a statically
compiled version of libmultipath, both to keep that library from
polluting the namespace of the caller, and to avoid the caller needing
to set up the variables and functions (like logsink, and
get_multipath_config) that it expects.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 Makefile                            |   1 +
 Makefile.inc                        |   1 +
 libmpathvalid/Makefile              |  38 ++++++
 libmpathvalid/libmpathvalid.version |   7 +
 libmpathvalid/mpath_valid.c         | 198 ++++++++++++++++++++++++++++
 libmpathvalid/mpath_valid.h         |  56 ++++++++
 libmultipath/Makefile               |   1 +
 libmultipath/devmapper.c            |  49 +++++++
 libmultipath/devmapper.h            |   1 +
 9 files changed, 352 insertions(+)
 create mode 100644 libmpathvalid/Makefile
 create mode 100644 libmpathvalid/libmpathvalid.version
 create mode 100644 libmpathvalid/mpath_valid.c
 create mode 100644 libmpathvalid/mpath_valid.h

diff --git a/Makefile b/Makefile
index 1dee3680..462d6655 100644
--- a/Makefile
+++ b/Makefile
@@ -9,6 +9,7 @@ BUILDDIRS := \
 	libmultipath/checkers \
 	libmultipath/foreign \
 	libmpathpersist \
+	libmpathvalid \
 	multipath \
 	multipathd \
 	mpathpersist \
diff --git a/Makefile.inc b/Makefile.inc
index d4d1e0dd..7e0e8203 100644
--- a/Makefile.inc
+++ b/Makefile.inc
@@ -66,6 +66,7 @@ libdir		= $(prefix)/$(LIB)/multipath
 unitdir		= $(prefix)/$(SYSTEMDPATH)/systemd/system
 mpathpersistdir	= $(TOPDIR)/libmpathpersist
 mpathcmddir	= $(TOPDIR)/libmpathcmd
+mpathvaliddir	= $(TOPDIR)/libmpathvalid
 thirdpartydir	= $(TOPDIR)/third-party
 libdmmpdir	= $(TOPDIR)/libdmmp
 nvmedir		= $(TOPDIR)/libmultipath/nvme
diff --git a/libmpathvalid/Makefile b/libmpathvalid/Makefile
new file mode 100644
index 00000000..5fc97022
--- /dev/null
+++ b/libmpathvalid/Makefile
@@ -0,0 +1,38 @@
+include ../Makefile.inc
+
+SONAME = 0
+DEVLIB = libmpathvalid.so
+LIBS = $(DEVLIB).$(SONAME)
+
+CFLAGS += $(LIB_CFLAGS) -I$(multipathdir) -I$(mpathcmddir)
+
+LIBDEPS += -lpthread -ldevmapper -ldl -L$(multipathdir) \
+	   -lmultipath_nonshared -L$(mpathcmddir) -lmpathcmd -ludev
+
+OBJS = mpath_valid.o
+
+all: $(LIBS)
+
+$(LIBS): $(OBJS)
+	$(CC) $(LDFLAGS) $(SHARED_FLAGS) -Wl,-soname=$@ -o $@ $(OBJS) $(LIBDEPS) -Wl,--version-script=libmpathvalid.version
+	$(LN) $(LIBS) $(DEVLIB)
+
+install: $(LIBS)
+	$(INSTALL_PROGRAM) -m 755 -d $(DESTDIR)$(syslibdir)
+	$(INSTALL_PROGRAM) -m 755 $(LIBS) $(DESTDIR)$(syslibdir)/$(LIBS)
+	$(LN) $(LIBS) $(DESTDIR)$(syslibdir)/$(DEVLIB)
+	$(INSTALL_PROGRAM) -m 755 -d $(DESTDIR)$(includedir)
+	$(INSTALL_PROGRAM) -m 644 mpath_valid.h $(DESTDIR)$(includedir)
+
+uninstall:
+	$(RM) $(DESTDIR)$(syslibdir)/$(LIBS)
+	$(RM) $(DESTDIR)$(syslibdir)/$(DEVLIB)
+	$(RM) $(DESTDIR)$(includedir)/mpath_valid.h
+
+clean: dep_clean
+	$(RM) core *.a *.o *.so *.so.* *.gz
+
+include $(wildcard $(OBJS:.o=.d))
+
+dep_clean:
+	$(RM) $(OBJS:.o=.d)
diff --git a/libmpathvalid/libmpathvalid.version b/libmpathvalid/libmpathvalid.version
new file mode 100644
index 00000000..94a6f86f
--- /dev/null
+++ b/libmpathvalid/libmpathvalid.version
@@ -0,0 +1,7 @@
+MPATH_1.0 {
+	global:
+		mpath_is_path;
+		mpath_get_mode;
+	local:
+		*;
+};
diff --git a/libmpathvalid/mpath_valid.c b/libmpathvalid/mpath_valid.c
new file mode 100644
index 00000000..4a7c19e5
--- /dev/null
+++ b/libmpathvalid/mpath_valid.c
@@ -0,0 +1,198 @@
+#include <stdio.h>
+#include <stdint.h>
+#include <libdevmapper.h>
+#include <libudev.h>
+#include <errno.h>
+
+#include "devmapper.h"
+#include "structs.h"
+#include "util.h"
+#include "config.h"
+#include "discovery.h"
+#include "wwids.h"
+#include "sysfs.h"
+#include "mpath_cmd.h"
+#include "mpath_valid.h"
+
+struct udev *udev;
+int logsink = -1;
+static struct config default_config = { .verbosity = -1 };
+static struct config *multipath_conf;
+
+struct config *get_multipath_config(void)
+{
+	return (multipath_conf)? multipath_conf : &default_config;
+}
+
+void put_multipath_config(__attribute__((unused))void *conf)
+{
+	/* Noop */
+}
+
+static int get_conf_mode(struct config *conf)
+{
+	if (conf->find_multipaths == FIND_MULTIPATHS_ON)
+		return MPATH_FIND;
+	if (conf->find_multipaths == FIND_MULTIPATHS_SMART)
+		return MPATH_SMART;
+	if (conf->find_multipaths == FIND_MULTIPATHS_GREEDY)
+		return MPATH_GREEDY;
+	return MPATH_STRICT;
+}
+
+
+int mpath_get_mode(void)
+{
+	int mode;
+	struct config *conf;
+
+	conf = load_config(DEFAULT_CONFIGFILE);
+	if (!conf)
+		return -1;
+	mode = get_conf_mode(conf);
+	free_config(conf);
+	return mode;
+}
+
+/*
+ * name: name of path to check
+ * mode: mode to use for determination. MPATH_DEFAULT uses configured mode
+ * info: on success, contains the path wwid
+ * paths: array of the returned mpath_info from other claimed paths
+ * nr_paths: the size of the paths array
+ */
+int
+mpath_is_path(const char *name, unsigned int mode, struct mpath_info *info,
+	      struct mpath_info **paths_arg, unsigned int nr_paths)
+{
+	struct config *conf;
+	struct path * pp;
+	int r = MPATH_IS_ERROR;
+	int fd = -1;
+	unsigned int i, version[3];
+	bool already_multipathed = false;
+	/* stupid c implicit conversion rules fail */
+	const struct mpath_info * const *paths = (const struct mpath_info * const *)paths_arg;
+
+	if (info)
+		memset(info, 0, sizeof(struct mpath_info));
+
+	if (!name || mode >= MPATH_MAX_MODE)
+		return r;
+
+	if (nr_paths > 0 && !paths)
+		return r;
+
+	skip_libmp_dm_init();
+	udev = udev_new();
+	if (!udev)
+		goto out;
+	conf = load_config(DEFAULT_CONFIGFILE);
+	if (!conf)
+		goto out_udev;
+	conf->verbosity = -1;
+	if (mode == MPATH_DEFAULT)
+		mode = get_conf_mode(conf);
+
+	if (dm_prereq(version))
+		goto out_config;
+	memcpy(conf->version, version, sizeof(version));
+	multipath_conf = conf;
+
+	pp = alloc_path();
+	if (!pp)
+		goto out_config;
+
+	if (safe_sprintf(pp->dev, "%s", name))
+		goto out_path;
+
+	if (sysfs_is_multipathed(pp, true)) {
+		if (!info || pp->wwid[0] != '\0') {
+			r = MPATH_IS_VALID;
+			goto out_path;
+		}
+		already_multipathed = true;
+	}
+
+	fd = __mpath_connect(1);
+	if (fd < 0) {
+		if (errno != EAGAIN && !systemd_service_enabled(name)) {
+			r = MPATH_IS_NOT_VALID;
+			goto out_path;
+		}
+	} else
+		mpath_disconnect(fd);
+
+	pp->udev = udev_device_new_from_subsystem_sysname(udev, "block", name);
+	if (!pp->udev)
+		goto out_path;
+
+	r = pathinfo(pp, conf, DI_SYSFS | DI_WWID | DI_BLACKLIST);
+	if (r) {
+		if (r == PATHINFO_SKIPPED)
+			r = MPATH_IS_NOT_VALID;
+		else
+			r = MPATH_IS_ERROR;
+		goto out_path;
+	}
+
+	if (pp->wwid[0] == '\0') {
+		r = MPATH_IS_NOT_VALID;
+		goto out_path;
+	}
+
+	if (already_multipathed)
+		goto out_path;
+
+	if (dm_map_present_by_uuid(pp->wwid) == 1) {
+		r = MPATH_IS_VALID;
+		goto out_path;
+	}
+
+	r = is_failed_wwid(pp->wwid);
+	if (r != WWID_IS_NOT_FAILED) {
+		if (r == WWID_IS_FAILED)
+			r = MPATH_IS_NOT_VALID;
+		else
+			r = MPATH_IS_ERROR;
+		goto out_path;
+	}
+
+	if (mode == MPATH_GREEDY) {
+		r = MPATH_IS_VALID;
+		goto out_path;
+	}
+
+	if (check_wwids_file(pp->wwid, 0) == 0) {
+		r = MPATH_IS_VALID;
+		goto out_path;
+	}
+
+	if (mode == MPATH_STRICT) {
+		r = MPATH_IS_NOT_VALID;
+		goto out_path;
+	}
+
+	for (i = 0; i < nr_paths; i++) {
+		if (strncmp(paths[i]->wwid, pp->wwid, 128) == 0) {
+			r = MPATH_IS_VALID;
+			goto out_path;
+		}
+	}
+
+	/* mode == MPATH_SMART || mode == MPATH_FIND */
+	r = MPATH_IS_MAYBE_VALID;
+
+out_path:
+	if (already_multipathed)
+		r = MPATH_IS_VALID;
+	if (info && (r == MPATH_IS_VALID || r == MPATH_IS_MAYBE_VALID))
+		strlcpy(info->wwid, pp->wwid, 128);
+	free_path(pp);
+out_config:
+	free_config(conf);
+out_udev:
+	udev_unref(udev);
+out:
+	return r;
+}
diff --git a/libmpathvalid/mpath_valid.h b/libmpathvalid/mpath_valid.h
new file mode 100644
index 00000000..f832beff
--- /dev/null
+++ b/libmpathvalid/mpath_valid.h
@@ -0,0 +1,56 @@
+/*
+ * Copyright (C) 2015 Red Hat, Inc.
+ *
+ * This file is part of the device-mapper multipath userspace tools.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser 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 Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef LIB_MPATH_VALID_H
+#define LIB_MPATH_VALID_H
+
+#ifdef __cpluscplus
+extern "C" {
+#endif
+
+enum mpath_valid_mode {
+	MPATH_DEFAULT,
+	MPATH_STRICT,
+	MPATH_FIND,
+	MPATH_SMART,
+	MPATH_GREEDY,
+	MPATH_MAX_MODE,  /* used only for bounds checking */
+};
+
+enum mpath_valid_result {
+	MPATH_IS_ERROR = -1,
+	MPATH_IS_NOT_VALID,
+	MPATH_IS_VALID,
+	MPATH_IS_MAYBE_VALID,
+};
+
+struct mpath_info {
+	char wwid[128];
+};
+
+int mpath_get_mode(void);
+int mpath_is_path(const char *name, unsigned int mode, struct mpath_info *info,
+		  struct mpath_info **paths, unsigned int nr_paths);
+
+
+#ifdef __cplusplus
+}
+#endif
+#endif /* LIB_PATH_VALID_H */
+
diff --git a/libmultipath/Makefile b/libmultipath/Makefile
index ad690a49..7e2c00cf 100644
--- a/libmultipath/Makefile
+++ b/libmultipath/Makefile
@@ -69,6 +69,7 @@ nvme-ioctl.h: nvme/nvme-ioctl.h
 
 $(LIBS): $(OBJS)
 	$(CC) $(LDFLAGS) $(SHARED_FLAGS) -Wl,-soname=$@ -o $@ $(OBJS) $(LIBDEPS)
+	ar rcs libmultipath_nonshared.a $(OBJS)
 	$(LN) $@ $(DEVLIB)
 
 install:
diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
index 7ed494a1..92f61133 100644
--- a/libmultipath/devmapper.c
+++ b/libmultipath/devmapper.c
@@ -770,6 +770,55 @@ out:
 	return r;
 }
 
+/*
+ * Return
+ *   1 : map with uuid exists
+ *   0 : map with uuid doesn't exist
+ *  -1 : error
+ */
+int
+dm_map_present_by_uuid(const char *uuid)
+{
+	struct dm_task *dmt;
+	struct dm_info info;
+	char prefixed_uuid[WWID_SIZE + UUID_PREFIX_LEN];
+	int r = -1;
+
+	if (!uuid || uuid[0] == '\0')
+		return 0;
+
+	if (safe_sprintf(prefixed_uuid, UUID_PREFIX "%s", uuid))
+		goto out;
+
+	if (!(dmt = dm_task_create(DM_DEVICE_INFO)))
+		goto out;
+
+	dm_task_no_open_count(dmt);
+
+	if (!dm_task_set_uuid(dmt, prefixed_uuid))
+		goto out_task;
+
+	if (!dm_task_run(dmt))
+		goto out_task;
+
+	if (!dm_task_get_info(dmt, &info))
+		goto out_task;
+
+	r = 0;
+
+	if (!info.exists)
+		goto out_task;
+
+	r = 1;
+out_task:
+	dm_task_destroy(dmt);
+out:
+	if (r < 0)
+		condlog(3, "%s: dm command failed in %s: %s", uuid,
+			__FUNCTION__, strerror(errno));
+	return r;
+}
+
 static int
 dm_dev_t (const char * mapname, char * dev_t, int len)
 {
diff --git a/libmultipath/devmapper.h b/libmultipath/devmapper.h
index 17fc9faf..5ed7edc5 100644
--- a/libmultipath/devmapper.h
+++ b/libmultipath/devmapper.h
@@ -39,6 +39,7 @@ int dm_simplecmd_noflush (int, const char *, uint16_t);
 int dm_addmap_create (struct multipath *mpp, char *params);
 int dm_addmap_reload (struct multipath *mpp, char *params, int flush);
 int dm_map_present (const char *);
+int dm_map_present_by_uuid(const char *uuid);
 int dm_get_map(const char *, unsigned long long *, char *);
 int dm_get_status(const char *, char *);
 int dm_type(const char *, char *);
-- 
2.17.2

Re: [RFC PATCH v2 2/3] libmultipath: make sysfs_is_multipathed able to return wwid

[Martin Wilck]

On Fri, 2020-04-03 at 01:50 -0500, Benjamin Marzinski wrote:
> sysfs_is_multipathed reads the wwid of the dm device holding a path
> to
> check if its a multipath device.  Add code to optinally set pp->wwid
> to
> that wwid.  This will be used by a future patch.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  libmultipath/sysfs.c | 24 +++++++++++++++++++-----
>  libmultipath/sysfs.h |  2 +-
>  multipath/main.c     |  7 ++++---
>  3 files changed, 24 insertions(+), 9 deletions(-)
> 
> diff --git a/libmultipath/sysfs.c b/libmultipath/sysfs.c
> index 62ec2ed7..12a82d95 100644
> --- a/libmultipath/sysfs.c
> +++ b/libmultipath/sysfs.c
> @@ -295,7 +295,7 @@ static int select_dm_devs(const struct dirent
> *di)
>  	return fnmatch("dm-*", di->d_name, FNM_FILE_NAME) == 0;
>  }
>  
> -bool sysfs_is_multipathed(const struct path *pp)
> +bool sysfs_is_multipathed(struct path *pp, bool set_wwid)
>  {
>  	char pathbuf[PATH_MAX];
>  	struct scandir_result sr;
> @@ -325,7 +325,7 @@ bool sysfs_is_multipathed(const struct path *pp)
>  	for (i = 0; i < r && !found; i++) {
>  		long fd;
>  		int nr;
> -		char uuid[6];
> +		char uuid[WWID_SIZE + UUID_PREFIX_LEN];
>  
>  		if (safe_snprintf(pathbuf + n, sizeof(pathbuf) - n,
>  				  "/%s/dm/uuid", di[i]->d_name))
> @@ -339,12 +339,26 @@ bool sysfs_is_multipathed(const struct path
> *pp)
>  
>  		pthread_cleanup_push(close_fd, (void *)fd);
>  		nr = read(fd, uuid, sizeof(uuid));
> -		if (nr == sizeof(uuid) && !memcmp(uuid, "mpath-",
> sizeof(uuid)))
> +		if (nr > (int)UUID_PREFIX_LEN &&
> +		    !memcmp(uuid, UUID_PREFIX, UUID_PREFIX_LEN))
>  			found = true;
>  		else if (nr < 0) {
> -			condlog(1, "%s: error reading from %s: %s",
> -				__func__, pathbuf, strerror(errno));
> +			condlog(1, "%s: error reading from %s: %m",
> +				__func__, pathbuf);
>  		}
> +		if (found && set_wwid) {
> +			nr -= UUID_PREFIX_LEN;
> +			memcpy(pp->wwid, uuid + UUID_PREFIX_LEN, nr);
> +			if (nr == WWID_SIZE) {
> +				condlog(4, "%s: overflow while reading
> from %s",
> +					__func__, pathbuf);

I would have checked this condition before calling memcpy(), but that's
just a nit.

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

-- 
Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
SUSE  Software Solutions Germany GmbH
HRB 36809, AG Nürnberg GF: Felix
Imendörffer

Re: [RFC PATCH v2 3/3] multipath: add libmpathvalid library

[Martin Wilck]

On Fri, 2020-04-03 at 01:50 -0500, Benjamin Marzinski wrote:
> This library allows other programs to check if a path should be
> claimed
> by multipath.  Currently, it only includes two functions.
> mpath_get_mode() get the configured find_multipaths mode.
> mpath_is_path() returns whether the device is claimed by multipath.
> It optionally also returns the wwid. The modes work
> mostly like they do for "multipath -c/-u", with a few exceptions.
> 
> 1. If a device is currently multipathed, it is always VALID. This
> perhaps should be true for the existing path valid code.
> 
> 2. If the mode is MPATH_FIND, and the device would be claimed if
> there
> was another path with the same wwid, but no matching wwid was passed
> in,
> mpath_is_path() will return MPATH_IS_MAYBE_VALID, just like if the
> find_multipaths mode was MPATH_SMART. This is so the caller knows to
> save this wwid to check against future paths.  It is the callers job
> to
> honor the difference between configurations with MPATH_FIND and
> MPATH_SMART.
> 
> In order to act more library-like, libmpathvalid doesn't set its own
> device-mapper log functions. It leaves this to the caller. It
> currently
> keeps condlog() from printing anything, but should eventually include
> a
> function to allow the caller set the logging. It also uses a
> statically
> compiled version of libmultipath, both to keep that library from
> polluting the namespace of the caller, and to avoid the caller
> needing
> to set up the variables and functions (like logsink, and
> get_multipath_config) that it expects.

See my response to 0/3 wrt this approach.

> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  Makefile                            |   1 +
>  Makefile.inc                        |   1 +
>  libmpathvalid/Makefile              |  38 ++++++
>  libmpathvalid/libmpathvalid.version |   7 +
>  libmpathvalid/mpath_valid.c         | 198
> ++++++++++++++++++++++++++++
>  libmpathvalid/mpath_valid.h         |  56 ++++++++
>  libmultipath/Makefile               |   1 +
>  libmultipath/devmapper.c            |  49 +++++++
>  libmultipath/devmapper.h            |   1 +
>  9 files changed, 352 insertions(+)
>  create mode 100644 libmpathvalid/Makefile
>  create mode 100644 libmpathvalid/libmpathvalid.version
>  create mode 100644 libmpathvalid/mpath_valid.c
>  create mode 100644 libmpathvalid/mpath_valid.h
> 
> diff --git a/Makefile b/Makefile
> index 1dee3680..462d6655 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -9,6 +9,7 @@ BUILDDIRS := \
>  	libmultipath/checkers \
>  	libmultipath/foreign \
>  	libmpathpersist \
> +	libmpathvalid \
>  	multipath \
>  	multipathd \
>  	mpathpersist \
> diff --git a/Makefile.inc b/Makefile.inc
> index d4d1e0dd..7e0e8203 100644
> --- a/Makefile.inc
> +++ b/Makefile.inc
> @@ -66,6 +66,7 @@ libdir		= $(prefix)/$(LIB)/multipath
>  unitdir		= $(prefix)/$(SYSTEMDPATH)/systemd/system
>  mpathpersistdir	= $(TOPDIR)/libmpathpersist
>  mpathcmddir	= $(TOPDIR)/libmpathcmd
> +mpathvaliddir	= $(TOPDIR)/libmpathvalid
>  thirdpartydir	= $(TOPDIR)/third-party
>  libdmmpdir	= $(TOPDIR)/libdmmp
>  nvmedir		= $(TOPDIR)/libmultipath/nvme
> diff --git a/libmpathvalid/Makefile b/libmpathvalid/Makefile
> new file mode 100644
> index 00000000..5fc97022
> --- /dev/null
> +++ b/libmpathvalid/Makefile
> @@ -0,0 +1,38 @@
> +include ../Makefile.inc
> +
> +SONAME = 0
> +DEVLIB = libmpathvalid.so
> +LIBS = $(DEVLIB).$(SONAME)
> +
> +CFLAGS += $(LIB_CFLAGS) -I$(multipathdir) -I$(mpathcmddir)
> +
> +LIBDEPS += -lpthread -ldevmapper -ldl -L$(multipathdir) \
> +	   -lmultipath_nonshared -L$(mpathcmddir) -lmpathcmd -ludev
> +
> +OBJS = mpath_valid.o
> +
> +all: $(LIBS)
> +
> +$(LIBS): $(OBJS)
> +	$(CC) $(LDFLAGS) $(SHARED_FLAGS) -Wl,-soname=$@ -o $@ $(OBJS)
> $(LIBDEPS) -Wl,--version-script=libmpathvalid.version
> +	$(LN) $(LIBS) $(DEVLIB)
> +
> +install: $(LIBS)
> +	$(INSTALL_PROGRAM) -m 755 -d $(DESTDIR)$(syslibdir)
> +	$(INSTALL_PROGRAM) -m 755 $(LIBS)
> $(DESTDIR)$(syslibdir)/$(LIBS)
> +	$(LN) $(LIBS) $(DESTDIR)$(syslibdir)/$(DEVLIB)
> +	$(INSTALL_PROGRAM) -m 755 -d $(DESTDIR)$(includedir)
> +	$(INSTALL_PROGRAM) -m 644 mpath_valid.h $(DESTDIR)$(includedir)
> +
> +uninstall:
> +	$(RM) $(DESTDIR)$(syslibdir)/$(LIBS)
> +	$(RM) $(DESTDIR)$(syslibdir)/$(DEVLIB)
> +	$(RM) $(DESTDIR)$(includedir)/mpath_valid.h
> +
> +clean: dep_clean
> +	$(RM) core *.a *.o *.so *.so.* *.gz
> +
> +include $(wildcard $(OBJS:.o=.d))
> +
> +dep_clean:
> +	$(RM) $(OBJS:.o=.d)
> diff --git a/libmpathvalid/libmpathvalid.version
> b/libmpathvalid/libmpathvalid.version
> new file mode 100644
> index 00000000..94a6f86f
> --- /dev/null
> +++ b/libmpathvalid/libmpathvalid.version
> @@ -0,0 +1,7 @@
> +MPATH_1.0 {
> +	global:
> +		mpath_is_path;
> +		mpath_get_mode;
> +	local:
> +		*;
> +};
> diff --git a/libmpathvalid/mpath_valid.c
> b/libmpathvalid/mpath_valid.c
> new file mode 100644
> index 00000000..4a7c19e5
> --- /dev/null
> +++ b/libmpathvalid/mpath_valid.c
> @@ -0,0 +1,198 @@
> +#include <stdio.h>
> +#include <stdint.h>
> +#include <libdevmapper.h>
> +#include <libudev.h>
> +#include <errno.h>
> +
> +#include "devmapper.h"
> +#include "structs.h"
> +#include "util.h"
> +#include "config.h"
> +#include "discovery.h"
> +#include "wwids.h"
> +#include "sysfs.h"
> +#include "mpath_cmd.h"
> +#include "mpath_valid.h"
> +
> +struct udev *udev;
> +int logsink = -1;
> +static struct config default_config = { .verbosity = -1 };
> +static struct config *multipath_conf;
> +
> +struct config *get_multipath_config(void)
> +{
> +	return (multipath_conf)? multipath_conf : &default_config;
> +}
> +
> +void put_multipath_config(__attribute__((unused))void *conf)
> +{
> +	/* Noop */
> +}
> +
> +static int get_conf_mode(struct config *conf)
> +{
> +	if (conf->find_multipaths == FIND_MULTIPATHS_ON)
> +		return MPATH_FIND;
> +	if (conf->find_multipaths == FIND_MULTIPATHS_SMART)
> +		return MPATH_SMART;
> +	if (conf->find_multipaths == FIND_MULTIPATHS_GREEDY)
> +		return MPATH_GREEDY;
> +	return MPATH_STRICT;
> +}
> +
> +
> +int mpath_get_mode(void)
> +{
> +	int mode;
> +	struct config *conf;
> +
> +	conf = load_config(DEFAULT_CONFIGFILE);

By using this, you'd pull in a substantial part of libmultipath. 
Why don't you simply rely on the passed-in mode value?

Actually, I wonder if we could split libmultipath into a part "below"
libmpathvalid and a part "above" libmpathvalid. I wouldn't put
"load_config()" in the "below" part. The problematic part is
pathinfo(), which has to be "below" and which would pull in quite a bit
of libmultipath functionality.

> +	if (!conf)
> +		return -1;
> +	mode = get_conf_mode(conf);
> +	free_config(conf);
> +	return mode;
> +}
> +
> +/*
> + * name: name of path to check
> + * mode: mode to use for determination. MPATH_DEFAULT uses
> configured mode
> + * info: on success, contains the path wwid
> + * paths: array of the returned mpath_info from other claimed paths
> + * nr_paths: the size of the paths array
> + */
> +int
> +mpath_is_path(const char *name, unsigned int mode, struct mpath_info
> *info,
> +	      struct mpath_info **paths_arg, unsigned int nr_paths)

I wonder if you couldn't use a vector of "struct path*" instead
of "struct mpath_info*". But well, I the data structures can be 
simply transformed into each other either way, so that's not a big
issue.

> +{
> +	struct config *conf;
> +	struct path * pp;
> +	int r = MPATH_IS_ERROR;
> +	int fd = -1;
> +	unsigned int i, version[3];
> +	bool already_multipathed = false;
> +	/* stupid c implicit conversion rules fail */
> +	const struct mpath_info * const *paths = (const struct
> mpath_info * const *)paths_arg;
> +
> +	if (info)
> +		memset(info, 0, sizeof(struct mpath_info));
> +
> +	if (!name || mode >= MPATH_MAX_MODE)
> +		return r;
> +
> +	if (nr_paths > 0 && !paths)
> +		return r;
> +
> +	skip_libmp_dm_init();
> +	udev = udev_new();
> +	if (!udev)
> +		goto out;
> +	conf = load_config(DEFAULT_CONFIGFILE);
> +	if (!conf)
> +		goto out_udev;
> +	conf->verbosity = -1;

Wouldn't this basically preclude calling the function from "multipath
-u", or any other place in multipath-tools assuming standard library
initialization? I'd like to split this off into some wrapper.

> +	if (mode == MPATH_DEFAULT)
> +		mode = get_conf_mode(conf);
> +
> +	if (dm_prereq(version))
> +		goto out_config;
> +	memcpy(conf->version, version, sizeof(version));
> +	multipath_conf = conf;
> +
> +	pp = alloc_path();
> +	if (!pp)
> +		goto out_config;
> +
> +	if (safe_sprintf(pp->dev, "%s", name))
> +		goto out_path;
> +
> +	if (sysfs_is_multipathed(pp, true)) {
> +		if (!info || pp->wwid[0] != '\0') {
> +			r = MPATH_IS_VALID;
> +			goto out_path;
> +		}
> +		already_multipathed = true;

This is the "WWID overflow" case? I believe multipathd would never
create a map with an WWID longer than WWID_SIZE, and thus this
condition should be treated as an error.

Other than that, you've done a magnificent job in making the logic
of this function easy to understand. I'd love to replace the current
"multipath -u" logic with this.

> +	}
> +
> +	fd = __mpath_connect(1);
> +	if (fd < 0) {
> +		if (errno != EAGAIN && !systemd_service_enabled(name))
> {
> +			r = MPATH_IS_NOT_VALID;
> +			goto out_path;
> +		}
> +	} else
> +		mpath_disconnect(fd);
> +
> +	pp->udev = udev_device_new_from_subsystem_sysname(udev,
> "block", name);
> +	if (!pp->udev)
> +		goto out_path;
> +
> +	r = pathinfo(pp, conf, DI_SYSFS | DI_WWID | DI_BLACKLIST);
> +	if (r) {
> +		if (r == PATHINFO_SKIPPED)
> +			r = MPATH_IS_NOT_VALID;
> +		else
> +			r = MPATH_IS_ERROR;
> +		goto out_path;
> +	}
> +
> +	if (pp->wwid[0] == '\0') {
> +		r = MPATH_IS_NOT_VALID;
> +		goto out_path;
> +	}
> +
> +	if (already_multipathed)
> +		goto out_path;
> +
> +	if (dm_map_present_by_uuid(pp->wwid) == 1) {
> +		r = MPATH_IS_VALID;
> +		goto out_path;
> +	}
> +
> +	r = is_failed_wwid(pp->wwid);
> +	if (r != WWID_IS_NOT_FAILED) {
> +		if (r == WWID_IS_FAILED)
> +			r = MPATH_IS_NOT_VALID;
> +		else
> +			r = MPATH_IS_ERROR;
> +		goto out_path;
> +	}
> +
> +	if (mode == MPATH_GREEDY) {
> +		r = MPATH_IS_VALID;
> +		goto out_path;
> +	}
> +
> +	if (check_wwids_file(pp->wwid, 0) == 0) {
> +		r = MPATH_IS_VALID;
> +		goto out_path;
> +	}
> +
> +	if (mode == MPATH_STRICT) {
> +		r = MPATH_IS_NOT_VALID;
> +		goto out_path;
> +	}

It seems I misunderstood you before. This MPATH_STRICT logic looks
fine.

> +
> +	for (i = 0; i < nr_paths; i++) {
> +		if (strncmp(paths[i]->wwid, pp->wwid, 128) == 0) {
> +			r = MPATH_IS_VALID;
> +			goto out_path;
> +		}
> +	}
> +
> +	/* mode == MPATH_SMART || mode == MPATH_FIND */
> +	r = MPATH_IS_MAYBE_VALID;
> +
> +out_path:
> +	if (already_multipathed)
> +		r = MPATH_IS_VALID;
> +	if (info && (r == MPATH_IS_VALID || r == MPATH_IS_MAYBE_VALID))
> +		strlcpy(info->wwid, pp->wwid, 128);
> +	free_path(pp);
> +out_config:
> +	free_config(conf);
> +out_udev:
> +	udev_unref(udev);
> +out:
> +	return r;
> +}
> diff --git a/libmpathvalid/mpath_valid.h
> b/libmpathvalid/mpath_valid.h
> new file mode 100644
> index 00000000..f832beff
> --- /dev/null
> +++ b/libmpathvalid/mpath_valid.h
> @@ -0,0 +1,56 @@
> +/*
> + * Copyright (C) 2015 Red Hat, Inc.
> + *
> + * This file is part of the device-mapper multipath userspace tools.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser 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 Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> License
> + * along with this program.  If not, see <
> http://www.gnu.org/licenses/>;.
> + */
> +
> +#ifndef LIB_MPATH_VALID_H
> +#define LIB_MPATH_VALID_H
> +
> +#ifdef __cpluscplus
> +extern "C" {
> +#endif
> +
> +enum mpath_valid_mode {
> +	MPATH_DEFAULT,
> +	MPATH_STRICT,
> +	MPATH_FIND,
> +	MPATH_SMART,
> +	MPATH_GREEDY,
> +	MPATH_MAX_MODE,  /* used only for bounds checking */
> +};
> +
> +enum mpath_valid_result {
> +	MPATH_IS_ERROR = -1,
> +	MPATH_IS_NOT_VALID,
> +	MPATH_IS_VALID,
> +	MPATH_IS_MAYBE_VALID,
> +};
> +
> +struct mpath_info {
> +	char wwid[128];
> +};
> +
> +int mpath_get_mode(void);
> +int mpath_is_path(const char *name, unsigned int mode, struct
> mpath_info *info,
> +		  struct mpath_info **paths, unsigned int nr_paths);
> +
> +
> +#ifdef __cplusplus
> +}
> +#endif
> +#endif /* LIB_PATH_VALID_H */
> +
> diff --git a/libmultipath/Makefile b/libmultipath/Makefile
> index ad690a49..7e2c00cf 100644
> --- a/libmultipath/Makefile
> +++ b/libmultipath/Makefile
> @@ -69,6 +69,7 @@ nvme-ioctl.h: nvme/nvme-ioctl.h
>  
>  $(LIBS): $(OBJS)
>  	$(CC) $(LDFLAGS) $(SHARED_FLAGS) -Wl,-soname=$@ -o $@ $(OBJS)
> $(LIBDEPS)
> +	ar rcs libmultipath_nonshared.a $(OBJS)
>  	$(LN) $@ $(DEVLIB)
>  
>  install:
> diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
> index 7ed494a1..92f61133 100644
> --- a/libmultipath/devmapper.c
> +++ b/libmultipath/devmapper.c
> @@ -770,6 +770,55 @@ out:
>  	return r;
>  }
>  
> +/*
> + * Return
> + *   1 : map with uuid exists
> + *   0 : map with uuid doesn't exist
> + *  -1 : error
> + */
> +int
> +dm_map_present_by_uuid(const char *uuid)
> +{
> +	struct dm_task *dmt;
> +	struct dm_info info;
> +	char prefixed_uuid[WWID_SIZE + UUID_PREFIX_LEN];
> +	int r = -1;
> +
> +	if (!uuid || uuid[0] == '\0')
> +		return 0;
> +
> +	if (safe_sprintf(prefixed_uuid, UUID_PREFIX "%s", uuid))
> +		goto out;
> +
> +	if (!(dmt = dm_task_create(DM_DEVICE_INFO)))
> +		goto out;
> +
> +	dm_task_no_open_count(dmt);
> +
> +	if (!dm_task_set_uuid(dmt, prefixed_uuid))
> +		goto out_task;
> +
> +	if (!dm_task_run(dmt))
> +		goto out_task;
> +
> +	if (!dm_task_get_info(dmt, &info))
> +		goto out_task;
> +
> +	r = 0;
> +
> +	if (!info.exists)
> +		goto out_task;
> +
> +	r = 1;
> +out_task:
> +	dm_task_destroy(dmt);
> +out:
> +	if (r < 0)
> +		condlog(3, "%s: dm command failed in %s: %s", uuid,
> +			__FUNCTION__, strerror(errno));
> +	return r;
> +}
> +
>  static int
>  dm_dev_t (const char * mapname, char * dev_t, int len)
>  {
> diff --git a/libmultipath/devmapper.h b/libmultipath/devmapper.h
> index 17fc9faf..5ed7edc5 100644
> --- a/libmultipath/devmapper.h
> +++ b/libmultipath/devmapper.h
> @@ -39,6 +39,7 @@ int dm_simplecmd_noflush (int, const char *,
> uint16_t);
>  int dm_addmap_create (struct multipath *mpp, char *params);
>  int dm_addmap_reload (struct multipath *mpp, char *params, int
> flush);
>  int dm_map_present (const char *);
> +int dm_map_present_by_uuid(const char *uuid);
>  int dm_get_map(const char *, unsigned long long *, char *);
>  int dm_get_status(const char *, char *);
>  int dm_type(const char *, char *);

-- 
Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
SUSE  Software Solutions Germany GmbH
HRB 36809, AG Nürnberg GF: Felix
Imendörffer

Re: [RFC PATCH v2 0/3] multipath: new path validation library

[Martin Wilck]

On Fri, 2020-04-03 at 01:50 -0500, Benjamin Marzinski wrote:
> This patchset is for a new library that programs can use to determine
> if a device belongs to multipath.  The primary user that this is
> intended for is SID, the Storage Instantiation Daemon
> 
> https://github.com/sid-project
> 
> Right now, this doesn't change our existing code to determine path
> ownership, and it doesn't do the exact same steps, although it is
> very
> close.  In the future, it would be possible to pull most of this code
> entirely into libmultipath, except for some wrappers, and use it for
> both methods.

I'd like to see how that's done. To reach that goal, we'd have to
eliminate the differences in the function's logic this way or that way.
Readability-wise, your new code is way better.

>   Obviously, this still needs man pages, and there are some
> helper functions for things like controlling multipath's logging that
> are missing, but I want to see if anyone has strong feelings about
> what
> this looks like.

As you're asking for it, I don't like the static linking linking of
libmultipath, which IMO unnecessarily complicates the multipath-tools
build. If this is what you need, why don't you simply pull multipath-
tools as-is into the SID source tree, e.g. with "git submodule", and
rebuild it there to you suit your needs? It's rather unlikely that
there will be other users of libmpathvalid besides SID any time time
soon. 

To put it more provocatively: I can see the benefit of this patch set
for SID, but what's the benefit for multipath-tools?

OTOH, multipath-tools *would* benefit if we used this as an incentive
to cleanup our libraries, first by cleaning up the "multipath -u"
logic, and later perhaps even so that SID (and other applications)
could simply link with -lmultipath without polluting its namespace in
inacceptible ways.

> I also have two more changes that I want to make to the multipath
> code,
> to make path validation do less unnecessary work, which aren't in
> this
> patchset.
> 
> 1. I want to remove the lock file from the failed wwids code. I don't
> see how it actually stops any races, and it means that simply reading
> a file, can trigger delays and writes (albeit to a memory base fs).

The main idea of the "locking" was that I wanted to create the actual
failed_wwids file atomically, using link(2). open_file() is
unfortunately not atomical at all. If we look into these issues, we
should put open_file() on the table, IMO.

Rather than creating that "lock" file and linking to it, we might 
create "/dev/shm/multipath/failed_wwids/$PID" (just as a 0-byte file,
not with open_file()) and rename it to $WWID atomically.

Moreover, it's possible (though not common) that multipath and
multipathd simultaneously try to set up a certain map. In that case,
one process would likely get an error. But you are right, the actual
race isn't prevented; for that we'd need to handle EEXIST in
dm_addmap_create(). 

> 2. I want to deprecate getuid_callout.  Once this is gone, you will
> be
> able to call pathinfo and get a path's WWID, without ever needing to
> open the path.

It's already been deprecated since 2013. Unfortunately I used it for
the hwtable test because it takes a free-form string argument; so if we
rip it out, we need to rewrite that test.

Best Regards,
Martin


> 
> changes in v2:
> 0002: make sysfs_is_multipathed only read the sysfs file once, as
> suggested by Martin.
> 
> 0003: dm_is_mpath_uuid() is now dm_map_present_by_uuid(). The library
> includes a new function mpath_get_mode(), to get the find_multipaths
> mode, and the modes now include MPATH_FIND. mpath_is_path() now
> accepts
> an array of mpath_infos, which the caller can use to pass the
> previous
> path wwids. This allows mpath_is_path() to return MPATH_IS_VALID for
> paths if there already is another path with that wwid.
> 
> However, mpath_is_path() still treats MPATH_SMART and MPATH_FIND the
> same.  I tried to make them work differently, but I realized that I
> need
> a way to signal that the MPATH_FIND path didn't fail because it was
> blacklisted, but instead because it needed another paths. Otherwise
> the
> caller won't know that it needs to save the wwid to check when later
> paths appear. This is exactly what MPATH_IS_MAYBE_VALID means. In the
> multipath -u code, the only difference between the find_multipaths
> "on"
> and "smart" case is what to do when a path that needs another path
> appears for the first time.  Dealing with this difference is the
> responsiblity of the caller of the mpathvalid library.
> mpath_get_mode(),
> will let it know what the configured find_multipaths mode is.
> 
> Benjamin Marzinski (3):
>   libmultipath: make libmp_dm_init optional
>   libmultipath: make sysfs_is_multipathed able to return wwid
>   multipath: add libmpathvalid library
> 
>  Makefile                            |   1 +
>  Makefile.inc                        |   1 +
>  libmpathvalid/Makefile              |  38 ++++++
>  libmpathvalid/libmpathvalid.version |   7 +
>  libmpathvalid/mpath_valid.c         | 198
> ++++++++++++++++++++++++++++
>  libmpathvalid/mpath_valid.h         |  56 ++++++++
>  libmultipath/Makefile               |   1 +
>  libmultipath/devmapper.c            |  66 +++++++++-
>  libmultipath/devmapper.h            |   4 +-
>  libmultipath/sysfs.c                |  24 +++-
>  libmultipath/sysfs.h                |   2 +-
>  multipath/main.c                    |   7 +-
>  12 files changed, 391 insertions(+), 14 deletions(-)
>  create mode 100644 libmpathvalid/Makefile
>  create mode 100644 libmpathvalid/libmpathvalid.version
>  create mode 100644 libmpathvalid/mpath_valid.c
>  create mode 100644 libmpathvalid/mpath_valid.h
> 

-- 
Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
SUSE  Software Solutions Germany GmbH
HRB 36809, AG Nürnberg GF: Felix
Imendörffer

Re: [RFC PATCH v2 3/3] multipath: add libmpathvalid library

[Benjamin Marzinski]

On Tue, Apr 28, 2020 at 09:42:52PM +0000, Martin Wilck wrote:
> On Fri, 2020-04-03 at 01:50 -0500, Benjamin Marzinski wrote:
> > +int mpath_get_mode(void)
> > +{
> > +	int mode;
> > +	struct config *conf;
> > +
> > +	conf = load_config(DEFAULT_CONFIGFILE);
> 
> By using this, you'd pull in a substantial part of libmultipath. 
> Why don't you simply rely on the passed-in mode value?
> 
> Actually, I wonder if we could split libmultipath into a part "below"
> libmpathvalid and a part "above" libmpathvalid. I wouldn't put
> "load_config()" in the "below" part. The problematic part is
> pathinfo(), which has to be "below" and which would pull in quite a bit
> of libmultipath functionality.

I would love to be able to not load the config, but I don't see how
that's possible.  The config tells us things like what's blacklisted,
what find_multipaths value is, etc.  If the library wants to get the
same result as multipathd, it needs to use the config. Or are you
suggesting something else here?
 
> > +	if (!conf)
> > +		return -1;
> > +	mode = get_conf_mode(conf);
> > +	free_config(conf);
> > +	return mode;
> > +}
> > +
> > +/*
> > + * name: name of path to check
> > + * mode: mode to use for determination. MPATH_DEFAULT uses
> > configured mode
> > + * info: on success, contains the path wwid
> > + * paths: array of the returned mpath_info from other claimed paths
> > + * nr_paths: the size of the paths array
> > + */
> > +int
> > +mpath_is_path(const char *name, unsigned int mode, struct mpath_info
> > *info,
> > +	      struct mpath_info **paths_arg, unsigned int nr_paths)
> 
> I wonder if you couldn't use a vector of "struct path*" instead
> of "struct mpath_info*". But well, I the data structures can be 
> simply transformed into each other either way, so that's not a big
> issue.

I want to use a array of the same things we are passing back to the
caller.  The idea is that the SID collects the mpath infos from earlier
calls, and passes them back here to help mpath_valid pick devices
correctly.  And like you said, it's easy to build that array with the
wwids from existing stuct paths.

> > +{
> > +	struct config *conf;
> > +	struct path * pp;
> > +	int r = MPATH_IS_ERROR;
> > +	int fd = -1;
> > +	unsigned int i, version[3];
> > +	bool already_multipathed = false;
> > +	/* stupid c implicit conversion rules fail */
> > +	const struct mpath_info * const *paths = (const struct
> > mpath_info * const *)paths_arg;
> > +
> > +	if (info)
> > +		memset(info, 0, sizeof(struct mpath_info));
> > +
> > +	if (!name || mode >= MPATH_MAX_MODE)
> > +		return r;
> > +
> > +	if (nr_paths > 0 && !paths)
> > +		return r;
> > +
> > +	skip_libmp_dm_init();
> > +	udev = udev_new();
> > +	if (!udev)
> > +		goto out;
> > +	conf = load_config(DEFAULT_CONFIGFILE);
> > +	if (!conf)
> > +		goto out_udev;
> > +	conf->verbosity = -1;
> 
> Wouldn't this basically preclude calling the function from "multipath
> -u", or any other place in multipath-tools assuming standard library
> initialization? I'd like to split this off into some wrapper.

Yes, to be useful for things other than SID (or even to play nicely with
SIDs verbosity settings), this needs to be configurable.
 
> > +	if (mode == MPATH_DEFAULT)
> > +		mode = get_conf_mode(conf);
> > +
> > +	if (dm_prereq(version))
> > +		goto out_config;
> > +	memcpy(conf->version, version, sizeof(version));
> > +	multipath_conf = conf;
> > +
> > +	pp = alloc_path();
> > +	if (!pp)
> > +		goto out_config;
> > +
> > +	if (safe_sprintf(pp->dev, "%s", name))
> > +		goto out_path;
> > +
> > +	if (sysfs_is_multipathed(pp, true)) {
> > +		if (!info || pp->wwid[0] != '\0') {
> > +			r = MPATH_IS_VALID;
> > +			goto out_path;
> > +		}
> > +		already_multipathed = true;
> 
> This is the "WWID overflow" case? I believe multipathd would never
> create a map with an WWID longer than WWID_SIZE, and thus this
> condition should be treated as an error.

Sure.

-Ben

> Other than that, you've done a magnificent job in making the logic
> of this function easy to understand. I'd love to replace the current
> "multipath -u" logic with this.
> 
> > +	}
> > +
> > +	fd = __mpath_connect(1);
> > +	if (fd < 0) {
> > +		if (errno != EAGAIN && !systemd_service_enabled(name))
> > {
> > +			r = MPATH_IS_NOT_VALID;
> > +			goto out_path;
> > +		}
> > +	} else
> > +		mpath_disconnect(fd);
> > +
> > +	pp->udev = udev_device_new_from_subsystem_sysname(udev,
> > "block", name);
> > +	if (!pp->udev)
> > +		goto out_path;
> > +
> > +	r = pathinfo(pp, conf, DI_SYSFS | DI_WWID | DI_BLACKLIST);
> > +	if (r) {
> > +		if (r == PATHINFO_SKIPPED)
> > +			r = MPATH_IS_NOT_VALID;
> > +		else
> > +			r = MPATH_IS_ERROR;
> > +		goto out_path;
> > +	}
> > +
> > +	if (pp->wwid[0] == '\0') {
> > +		r = MPATH_IS_NOT_VALID;
> > +		goto out_path;
> > +	}
> > +
> > +	if (already_multipathed)
> > +		goto out_path;
> > +
> > +	if (dm_map_present_by_uuid(pp->wwid) == 1) {
> > +		r = MPATH_IS_VALID;
> > +		goto out_path;
> > +	}
> > +
> > +	r = is_failed_wwid(pp->wwid);
> > +	if (r != WWID_IS_NOT_FAILED) {
> > +		if (r == WWID_IS_FAILED)
> > +			r = MPATH_IS_NOT_VALID;
> > +		else
> > +			r = MPATH_IS_ERROR;
> > +		goto out_path;
> > +	}
> > +
> > +	if (mode == MPATH_GREEDY) {
> > +		r = MPATH_IS_VALID;
> > +		goto out_path;
> > +	}
> > +
> > +	if (check_wwids_file(pp->wwid, 0) == 0) {
> > +		r = MPATH_IS_VALID;
> > +		goto out_path;
> > +	}
> > +
> > +	if (mode == MPATH_STRICT) {
> > +		r = MPATH_IS_NOT_VALID;
> > +		goto out_path;
> > +	}
> 
> It seems I misunderstood you before. This MPATH_STRICT logic looks
> fine.
> 
> > +
> > +	for (i = 0; i < nr_paths; i++) {
> > +		if (strncmp(paths[i]->wwid, pp->wwid, 128) == 0) {
> > +			r = MPATH_IS_VALID;
> > +			goto out_path;
> > +		}
> > +	}
> > +
> > +	/* mode == MPATH_SMART || mode == MPATH_FIND */
> > +	r = MPATH_IS_MAYBE_VALID;
> > +
> > +out_path:
> > +	if (already_multipathed)
> > +		r = MPATH_IS_VALID;
> > +	if (info && (r == MPATH_IS_VALID || r == MPATH_IS_MAYBE_VALID))
> > +		strlcpy(info->wwid, pp->wwid, 128);
> > +	free_path(pp);
> > +out_config:
> > +	free_config(conf);
> > +out_udev:
> > +	udev_unref(udev);
> > +out:
> > +	return r;
> > +}
> > diff --git a/libmpathvalid/mpath_valid.h
> > b/libmpathvalid/mpath_valid.h
> > new file mode 100644
> > index 00000000..f832beff
> > --- /dev/null
> > +++ b/libmpathvalid/mpath_valid.h
> > @@ -0,0 +1,56 @@
> > +/*
> > + * Copyright (C) 2015 Red Hat, Inc.
> > + *
> > + * This file is part of the device-mapper multipath userspace tools.
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU Lesser 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 Lesser General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU Lesser General Public
> > License
> > + * along with this program.  If not, see <
> > http://www.gnu.org/licenses/>;.
> > + */
> > +
> > +#ifndef LIB_MPATH_VALID_H
> > +#define LIB_MPATH_VALID_H
> > +
> > +#ifdef __cpluscplus
> > +extern "C" {
> > +#endif
> > +
> > +enum mpath_valid_mode {
> > +	MPATH_DEFAULT,
> > +	MPATH_STRICT,
> > +	MPATH_FIND,
> > +	MPATH_SMART,
> > +	MPATH_GREEDY,
> > +	MPATH_MAX_MODE,  /* used only for bounds checking */
> > +};
> > +
> > +enum mpath_valid_result {
> > +	MPATH_IS_ERROR = -1,
> > +	MPATH_IS_NOT_VALID,
> > +	MPATH_IS_VALID,
> > +	MPATH_IS_MAYBE_VALID,
> > +};
> > +
> > +struct mpath_info {
> > +	char wwid[128];
> > +};
> > +
> > +int mpath_get_mode(void);
> > +int mpath_is_path(const char *name, unsigned int mode, struct
> > mpath_info *info,
> > +		  struct mpath_info **paths, unsigned int nr_paths);
> > +
> > +
> > +#ifdef __cplusplus
> > +}
> > +#endif
> > +#endif /* LIB_PATH_VALID_H */
> > +
> > diff --git a/libmultipath/Makefile b/libmultipath/Makefile
> > index ad690a49..7e2c00cf 100644
> > --- a/libmultipath/Makefile
> > +++ b/libmultipath/Makefile
> > @@ -69,6 +69,7 @@ nvme-ioctl.h: nvme/nvme-ioctl.h
> >  
> >  $(LIBS): $(OBJS)
> >  	$(CC) $(LDFLAGS) $(SHARED_FLAGS) -Wl,-soname=$@ -o $@ $(OBJS)
> > $(LIBDEPS)
> > +	ar rcs libmultipath_nonshared.a $(OBJS)
> >  	$(LN) $@ $(DEVLIB)
> >  
> >  install:
> > diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
> > index 7ed494a1..92f61133 100644
> > --- a/libmultipath/devmapper.c
> > +++ b/libmultipath/devmapper.c
> > @@ -770,6 +770,55 @@ out:
> >  	return r;
> >  }
> >  
> > +/*
> > + * Return
> > + *   1 : map with uuid exists
> > + *   0 : map with uuid doesn't exist
> > + *  -1 : error
> > + */
> > +int
> > +dm_map_present_by_uuid(const char *uuid)
> > +{
> > +	struct dm_task *dmt;
> > +	struct dm_info info;
> > +	char prefixed_uuid[WWID_SIZE + UUID_PREFIX_LEN];
> > +	int r = -1;
> > +
> > +	if (!uuid || uuid[0] == '\0')
> > +		return 0;
> > +
> > +	if (safe_sprintf(prefixed_uuid, UUID_PREFIX "%s", uuid))
> > +		goto out;
> > +
> > +	if (!(dmt = dm_task_create(DM_DEVICE_INFO)))
> > +		goto out;
> > +
> > +	dm_task_no_open_count(dmt);
> > +
> > +	if (!dm_task_set_uuid(dmt, prefixed_uuid))
> > +		goto out_task;
> > +
> > +	if (!dm_task_run(dmt))
> > +		goto out_task;
> > +
> > +	if (!dm_task_get_info(dmt, &info))
> > +		goto out_task;
> > +
> > +	r = 0;
> > +
> > +	if (!info.exists)
> > +		goto out_task;
> > +
> > +	r = 1;
> > +out_task:
> > +	dm_task_destroy(dmt);
> > +out:
> > +	if (r < 0)
> > +		condlog(3, "%s: dm command failed in %s: %s", uuid,
> > +			__FUNCTION__, strerror(errno));
> > +	return r;
> > +}
> > +
> >  static int
> >  dm_dev_t (const char * mapname, char * dev_t, int len)
> >  {
> > diff --git a/libmultipath/devmapper.h b/libmultipath/devmapper.h
> > index 17fc9faf..5ed7edc5 100644
> > --- a/libmultipath/devmapper.h
> > +++ b/libmultipath/devmapper.h
> > @@ -39,6 +39,7 @@ int dm_simplecmd_noflush (int, const char *,
> > uint16_t);
> >  int dm_addmap_create (struct multipath *mpp, char *params);
> >  int dm_addmap_reload (struct multipath *mpp, char *params, int
> > flush);
> >  int dm_map_present (const char *);
> > +int dm_map_present_by_uuid(const char *uuid);
> >  int dm_get_map(const char *, unsigned long long *, char *);
> >  int dm_get_status(const char *, char *);
> >  int dm_type(const char *, char *);
> 
> -- 
> Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
> SUSE  Software Solutions Germany GmbH
> HRB 36809, AG Nürnberg GF: Felix
> Imendörffer
> 

Re: [RFC PATCH v2 0/3] multipath: new path validation library

[Benjamin Marzinski]

On Tue, Apr 28, 2020 at 09:48:03PM +0000, Martin Wilck wrote:
> On Fri, 2020-04-03 at 01:50 -0500, Benjamin Marzinski wrote:
> > This patchset is for a new library that programs can use to determine
> > if a device belongs to multipath.  The primary user that this is
> > intended for is SID, the Storage Instantiation Daemon
> > 
> > https://github.com/sid-project
> > 
> > Right now, this doesn't change our existing code to determine path
> > ownership, and it doesn't do the exact same steps, although it is
> > very
> > close.  In the future, it would be possible to pull most of this code
> > entirely into libmultipath, except for some wrappers, and use it for
> > both methods.
> 
> I'd like to see how that's done. To reach that goal, we'd have to
> eliminate the differences in the function's logic this way or that way.
> Readability-wise, your new code is way better.

I'm going to start working on this.
 
> >   Obviously, this still needs man pages, and there are some
> > helper functions for things like controlling multipath's logging that
> > are missing, but I want to see if anyone has strong feelings about
> > what
> > this looks like.
> 
> As you're asking for it, I don't like the static linking linking of
> libmultipath, which IMO unnecessarily complicates the multipath-tools
> build. If this is what you need, why don't you simply pull multipath-
> tools as-is into the SID source tree, e.g. with "git submodule", and
> rebuild it there to you suit your needs? It's rather unlikely that
> there will be other users of libmpathvalid besides SID any time time
> soon. 
> 
> To put it more provocatively: I can see the benefit of this patch set
> for SID, but what's the benefit for multipath-tools?
> 
> OTOH, multipath-tools *would* benefit if we used this as an incentive
> to cleanup our libraries, first by cleaning up the "multipath -u"
> logic, and later perhaps even so that SID (and other applications)
> could simply link with -lmultipath without polluting its namespace in
> inacceptible ways.

Actually, after reading through the SID code, I've realized that the
multipath code will still be compartmentalized in a DSO. So I'm fine
with using the shared library as is.

> > I also have two more changes that I want to make to the multipath
> > code,
> > to make path validation do less unnecessary work, which aren't in
> > this
> > patchset.
> > 
> > 1. I want to remove the lock file from the failed wwids code. I don't
> > see how it actually stops any races, and it means that simply reading
> > a file, can trigger delays and writes (albeit to a memory base fs).
> 
> The main idea of the "locking" was that I wanted to create the actual
> failed_wwids file atomically, using link(2). open_file() is
> unfortunately not atomical at all. If we look into these issues, we
> should put open_file() on the table, IMO.
> 
> Rather than creating that "lock" file and linking to it, we might 
> create "/dev/shm/multipath/failed_wwids/$PID" (just as a 0-byte file,
> not with open_file()) and rename it to $WWID atomically.
> 
> Moreover, it's possible (though not common) that multipath and
> multipathd simultaneously try to set up a certain map. In that case,
> one process would likely get an error. But you are right, the actual
> race isn't prevented; for that we'd need to handle EEXIST in
> dm_addmap_create(). 

Oops. I forgot to post my patch for that. It's posted now. We can
discuss my idea there.
 
> > 2. I want to deprecate getuid_callout.  Once this is gone, you will
> > be
> > able to call pathinfo and get a path's WWID, without ever needing to
> > open the path.
> 
> It's already been deprecated since 2013. Unfortunately I used it for
> the hwtable test because it takes a free-form string argument; so if we
> rip it out, we need to rewrite that test.

Unfortunately, I might need to have a change of heart about this.  The
issue with SID, now that I've looked at it more, is that it gets called
from udev before all of the storage udev rules that set up the udev
properties that multipath uses to pick the wwid.  There are two ways of
dealing with that. I should note that both of these will require
multipath to detect that it's running with SID and do something
different that normally. udev_device_get_property_value() won't work,
since the property simply won't be in the udev database when SID is
calling the multipath library.

1. pull all the scsi_id and related rules into SID as well, and have
multipath read from the SID database (which is later synced with the
udev database) However, I don't believe that SID would need this
otherwise, and that kind of labeling of devices is something that udev
is good at. Also, not every distrbution uses the exact same udev rules,
so either there would need to be multiple SID modules, or everyone who
used SID would have to do things the same way here. It might also be
harder for people to override these things. I'm still a little fuzzy on
when SID populates the udev database with it's results, but I think it
happens late enough that its values would override those of any
properties that were also set in udev rules.

2. Multipath would have to not rely on udev for the wwid. The
alternative would be sysfs (or ioctls) like multipath currently uses as
a backup.  This would mean that SID wouldn't have to pull in all
scsi_id related code.  However, it would mean that multipathd would also
need to recognize when it was being run with SID, so it could use
sysfs as well, to match libmpathvalid's results. This frees multipath
from needing udev for this, but it makes the wwid criteria much more
rigid.  Currently people can simply add a udev rule to set a property
and configure the multipath device to look at that, if the existing
rules aren't working right for a specific setup.  The getuid_callout
currently also provides that freedom, and it might be necessary if
users can't simply make udev rules for devices that can't work with
getting the info from sysfs.
 
At any rate, I'm still going to push forward with making this library,
with the existing get_uid() code, work with multipath -u.

-Ben

> Best Regards,
> Martin
> 
> 
> > 
> > changes in v2:
> > 0002: make sysfs_is_multipathed only read the sysfs file once, as
> > suggested by Martin.
> > 
> > 0003: dm_is_mpath_uuid() is now dm_map_present_by_uuid(). The library
> > includes a new function mpath_get_mode(), to get the find_multipaths
> > mode, and the modes now include MPATH_FIND. mpath_is_path() now
> > accepts
> > an array of mpath_infos, which the caller can use to pass the
> > previous
> > path wwids. This allows mpath_is_path() to return MPATH_IS_VALID for
> > paths if there already is another path with that wwid.
> > 
> > However, mpath_is_path() still treats MPATH_SMART and MPATH_FIND the
> > same.  I tried to make them work differently, but I realized that I
> > need
> > a way to signal that the MPATH_FIND path didn't fail because it was
> > blacklisted, but instead because it needed another paths. Otherwise
> > the
> > caller won't know that it needs to save the wwid to check when later
> > paths appear. This is exactly what MPATH_IS_MAYBE_VALID means. In the
> > multipath -u code, the only difference between the find_multipaths
> > "on"
> > and "smart" case is what to do when a path that needs another path
> > appears for the first time.  Dealing with this difference is the
> > responsiblity of the caller of the mpathvalid library.
> > mpath_get_mode(),
> > will let it know what the configured find_multipaths mode is.
> > 
> > Benjamin Marzinski (3):
> >   libmultipath: make libmp_dm_init optional
> >   libmultipath: make sysfs_is_multipathed able to return wwid
> >   multipath: add libmpathvalid library
> > 
> >  Makefile                            |   1 +
> >  Makefile.inc                        |   1 +
> >  libmpathvalid/Makefile              |  38 ++++++
> >  libmpathvalid/libmpathvalid.version |   7 +
> >  libmpathvalid/mpath_valid.c         | 198
> > ++++++++++++++++++++++++++++
> >  libmpathvalid/mpath_valid.h         |  56 ++++++++
> >  libmultipath/Makefile               |   1 +
> >  libmultipath/devmapper.c            |  66 +++++++++-
> >  libmultipath/devmapper.h            |   4 +-
> >  libmultipath/sysfs.c                |  24 +++-
> >  libmultipath/sysfs.h                |   2 +-
> >  multipath/main.c                    |   7 +-
> >  12 files changed, 391 insertions(+), 14 deletions(-)
> >  create mode 100644 libmpathvalid/Makefile
> >  create mode 100644 libmpathvalid/libmpathvalid.version
> >  create mode 100644 libmpathvalid/mpath_valid.c
> >  create mode 100644 libmpathvalid/mpath_valid.h
> > 
> 
> -- 
> Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
> SUSE  Software Solutions Germany GmbH
> HRB 36809, AG Nürnberg GF: Felix
> Imendörffer
>