[RFC PATCH v2 0/3] add library to check if device is a valid path

[RFC PATCH v2 0/3] add library to check if device is a valid path

[Benjamin Marzinski]

The main part of the this patchset is the first patch, which adds a
new library interface to check whether devices are valid paths. This
was designed for use in the Storage Instantiation Daemon (SID).

https://github.com/sid-project

Hopefully, I've removed all the controvertial bits from the last time I
proposed this library.

The second patch adds get_uid fallback code for dasd devices. The third
patch just changes the get_uid log level for devices configured with
uid_attribute "". This is because it is currently necessary to configure
multipath with

overrides {
        uid_attribute ""
}

to claim multipath devices with SID (instead of using multipath.rules),
since SID doesn't currently get the UID information itself, and it is
called by udev before this information is added to the udev database.

I'm resubmitting this as a RFC patchset. Assuming they are ACKed, I will
resend these patches again, along with a man page, and unit tests, on
top of Martin's revised version of his "multipath-tools: shutdown,
libdevmapper races, globals" patchset.

changes from v1
---------------

0001: This patch is now rebased on top of, and makes use of Martin's
patches that provide a default *_multipath_config, udev, and logsink.
Because of this, mpathvalid_init() now has a parameter used to set
logsink. There is also a new API function, mpathvalid_reload_config().

0003: This is completely new, since Martin pointed out that adding a new
config option to always use the fallback getuid code was unnecessary. It
just makes a uid_attribute of "" log at normal levels.

Benjamin Marzinski (3):
  multipath: add libmpathvalid library
  libmultipath: add uid failback for dasd devices
  libmultipath: change log level for null uid_attribute

 Makefile                            |   3 +-
 libmpathvalid/Makefile              |  38 ++++++
 libmpathvalid/libmpathvalid.version |  10 ++
 libmpathvalid/mpath_valid.c         | 199 ++++++++++++++++++++++++++++
 libmpathvalid/mpath_valid.h         |  61 +++++++++
 libmultipath/defaults.h             |   1 +
 libmultipath/discovery.c            |  40 +++++-
 7 files changed, 349 insertions(+), 3 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] multipath: add libmpathvalid library

[Benjamin Marzinski]

This library allows other programs to check if a path should be claimed
by multipath. It exports an init function, that needs to be called
before and after all other library calls, an exit function, that needs
to be called after all library calls, a function to reread the multipath
configuration files, and two more functions.

mpath_get_mode() get the configured find_multipaths mode.
mpath_is_path() returns whether the device is claimed by multipath, and
optionally returns the wwid.  This code works slightly different than
the multipath -c/u code for SMART mode.  Instead of checking all the
existing paths to see if another has the same wwid, it expects the
caller to pass in an array of the already known path wwids, and checks
if the current path matches any of those.

The library also doesn't set up the device-mapper log fuctions. It
leaves this up to the caller.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 Makefile                            |   3 +-
 libmpathvalid/Makefile              |  38 ++++++
 libmpathvalid/libmpathvalid.version |  10 ++
 libmpathvalid/mpath_valid.c         | 199 ++++++++++++++++++++++++++++
 libmpathvalid/mpath_valid.h         |  61 +++++++++
 5 files changed, 310 insertions(+), 1 deletion(-)
 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 4a3491da..f127ff91 100644
--- a/Makefile
+++ b/Makefile
@@ -9,6 +9,7 @@ BUILDDIRS := \
 	libmultipath/checkers \
 	libmultipath/foreign \
 	libmpathpersist \
+	libmpathvalid \
 	multipath \
 	multipathd \
 	mpathpersist \
@@ -29,7 +30,7 @@ $(BUILDDIRS):
 	$(MAKE) -C $@
 
 libmultipath libdmmp: libmpathcmd
-libmpathpersist multipath multipathd: libmultipath
+libmpathpersist libmpathvalid multipath multipathd: libmultipath
 mpathpersist multipathd:  libmpathpersist
 
 libmultipath/checkers.install \
diff --git a/libmpathvalid/Makefile b/libmpathvalid/Makefile
new file mode 100644
index 00000000..70b97eca
--- /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 -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..3bd0d3c5
--- /dev/null
+++ b/libmpathvalid/libmpathvalid.version
@@ -0,0 +1,10 @@
+MPATH_1.0 {
+	global:
+		mpathvalid_init;
+		mpathvalid_reload_config;
+		mpathvalid_exit;
+		mpathvalid_is_path;
+		mpathvalid_get_mode;
+	local:
+		*;
+};
diff --git a/libmpathvalid/mpath_valid.c b/libmpathvalid/mpath_valid.c
new file mode 100644
index 00000000..b9ec83a4
--- /dev/null
+++ b/libmpathvalid/mpath_valid.c
@@ -0,0 +1,199 @@
+#include <stdlib.h>
+#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 "valid.h"
+#include "mpath_valid.h"
+#include "debug.h"
+
+static unsigned int get_conf_mode(struct config *conf)
+{
+	if (conf->find_multipaths == FIND_MULTIPATHS_SMART)
+		return MPATH_SMART;
+	if (conf->find_multipaths == FIND_MULTIPATHS_GREEDY)
+		return MPATH_GREEDY;
+	return MPATH_STRICT;
+}
+
+static void set_conf_mode(struct config *conf, unsigned int mode)
+{
+	if (mode == MPATH_SMART)
+		conf->find_multipaths = FIND_MULTIPATHS_SMART;
+	else if (mode == MPATH_GREEDY)
+		conf->find_multipaths = FIND_MULTIPATHS_GREEDY;
+	else
+		conf->find_multipaths = FIND_MULTIPATHS_STRICT;
+}
+
+unsigned int mpathvalid_get_mode(void)
+{
+	int mode;
+	struct config *conf;
+
+	conf = get_multipath_config();
+	if (!conf)
+		return -1;
+	mode = get_conf_mode(conf);
+	put_multipath_config(conf);
+	return mode;
+}
+
+static int convert_result(int result) {
+	switch (result) {
+	case PATH_IS_ERROR:
+		return MPATH_IS_ERROR;
+	case PATH_IS_NOT_VALID:
+		return MPATH_IS_NOT_VALID;
+	case PATH_IS_VALID:
+		return MPATH_IS_VALID;
+	case PATH_IS_VALID_NO_CHECK:
+		return MPATH_IS_VALID_NO_CHECK;
+	case PATH_IS_MAYBE_VALID:
+		return MPATH_IS_MAYBE_VALID;
+	}
+	return MPATH_IS_ERROR;
+}
+
+static int load_default_config(int verbosity)
+{
+	struct config *conf;
+
+	/* need to set verbosity here to control logging during init_config() */
+	conf = get_multipath_config();
+	conf->verbosity = verbosity;
+	put_multipath_config(conf);
+
+	if (init_config(DEFAULT_CONFIGFILE))
+		return -1;
+
+	/* Need to override verbosity from init_config() */
+	conf = get_multipath_config();
+	conf->verbosity = verbosity;
+	put_multipath_config(conf);
+
+	return 0;
+}
+
+int
+mpathvalid_init(int verbosity, int log_style)
+{
+	unsigned int version[3];
+
+	logsink = log_style;
+	if (libmultipath_init())
+		return -1;
+
+	skip_libmp_dm_init();
+	if (load_default_config(verbosity))
+		goto fail;
+
+	if (dm_prereq(version))
+		goto fail_config;
+
+	return 0;
+
+fail_config:
+	uninit_config();
+fail:
+	libmultipath_exit();
+	return -1;
+}
+
+int
+mpathvalid_reload_config(void)
+{
+	int verbosity;
+	struct config *conf;
+
+	conf = get_multipath_config();
+	verbosity = conf->verbosity;
+	put_multipath_config(conf);
+
+	uninit_config();
+
+	return load_default_config(verbosity);
+}
+
+int
+mpathvalid_exit(void)
+{
+	uninit_config();
+	libmultipath_exit();
+	return 0;
+}
+
+/*
+ * 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
+mpathvalid_is_path(const char *name, unsigned int mode, char **wwid,
+	           const char **path_wwids, unsigned int nr_paths)
+{
+	struct config *conf;
+	int r = MPATH_IS_ERROR;
+	unsigned int i;
+	struct path *pp;
+
+	if (!name || mode >= MPATH_MAX_MODE)
+		return r;
+	if (nr_paths > 0 && !path_wwids)
+		return r;
+	if (!udev)
+		return r;
+
+	pp = alloc_path();
+	if (!pp)
+		return r;
+
+	if (wwid) {
+		*wwid = (char *)malloc(WWID_SIZE);
+		if (!*wwid)
+			goto out;
+	}
+
+	conf = get_multipath_config();
+	if (!conf)
+		goto out_wwid;
+	if (mode != MPATH_DEFAULT)
+		set_conf_mode(conf, mode);
+	r = convert_result(is_path_valid(name, conf, pp, true));
+	put_multipath_config(conf);
+
+	if (r == MPATH_IS_MAYBE_VALID) {
+		for (i = 0; i < nr_paths; i++) {
+			if (strncmp(path_wwids[i], pp->wwid, WWID_SIZE) == 0) {
+				r = MPATH_IS_VALID;
+				break;
+			}
+		}
+	}
+
+out_wwid:
+	if (wwid) {
+		if (r == MPATH_IS_VALID || r == MPATH_IS_VALID_NO_CHECK ||
+		    r == MPATH_IS_MAYBE_VALID)
+			strlcpy(*wwid, pp->wwid, WWID_SIZE);
+		else {
+			free(*wwid);
+			*wwid = NULL;
+		}
+	}
+out:
+	free_path(pp);
+	return r;
+}
diff --git a/libmpathvalid/mpath_valid.h b/libmpathvalid/mpath_valid.h
new file mode 100644
index 00000000..c83b8da5
--- /dev/null
+++ b/libmpathvalid/mpath_valid.h
@@ -0,0 +1,61 @@
+/*
+ * 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_SMART,
+	MPATH_GREEDY,
+	MPATH_MAX_MODE,  /* used only for bounds checking */
+};
+
+/* MPATH_IS_VALID_NO_CHECK is used to skip checks to see if the device
+ * has already been unclaimed by multipath in the past */
+enum mpath_valid_result {
+	MPATH_IS_ERROR = -1,
+	MPATH_IS_NOT_VALID,
+	MPATH_IS_VALID,
+	MPATH_IS_VALID_NO_CHECK,
+	MPATH_IS_MAYBE_VALID,
+};
+
+enum mpath_valid_log_style {
+	MPATH_LOG_STDIO = -1,
+	MPATH_LOG_STDIO_TIMESTAMP,
+	MPATH_LOG_SYSLOG,
+};
+
+int mpathvalid_init(int verbosity, int log_style);
+int mpathvalid_reload_config(void);
+int mpathvalid_exit(void);
+unsigned int mpathvalid_get_mode(void);
+int mpathvalid_is_path(const char *name, unsigned int mode, char **wwid,
+		       const char **path_wwids, unsigned int nr_paths);
+
+#ifdef __cplusplus
+}
+#endif
+#endif /* LIB_PATH_VALID_H */
-- 
2.17.2

[RFC PATCH v2 2/3] libmultipath: add uid failback for dasd devices

[Benjamin Marzinski]

Add failback code to get the uid for dasd devices from sysfs. Copied
from dasdinfo

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

diff --git a/libmultipath/defaults.h b/libmultipath/defaults.h
index 39a5e415..947ba467 100644
--- a/libmultipath/defaults.h
+++ b/libmultipath/defaults.h
@@ -8,6 +8,7 @@
  */
 #define DEFAULT_UID_ATTRIBUTE	"ID_SERIAL"
 #define DEFAULT_NVME_UID_ATTRIBUTE	"ID_WWN"
+#define DEFAULT_DASD_UID_ATTRIBUTE	"ID_UID"
 #define DEFAULT_UDEVDIR		"/dev"
 #define DEFAULT_MULTIPATHDIR	"/" LIB_STRING "/multipath"
 #define DEFAULT_SELECTOR	"service-time 0"
diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index 000f4c96..f650534f 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -1953,12 +1953,44 @@ get_vpd_uid(struct path * pp)
 	return get_vpd_sysfs(parent, 0x83, pp->wwid, WWID_SIZE);
 }
 
+/* based on code from s390-tools/dasdinfo/dasdinfo.c */
+static ssize_t dasd_get_uid(struct path *pp)
+{
+	struct udev_device *parent;
+	char value[80];
+	char *p;
+	int i;
+
+	parent = udev_device_get_parent_with_subsystem_devtype(pp->udev, "ccw",
+							       NULL);
+	if (!parent)
+		return -1;
+
+	if (sysfs_attr_get_value(parent, "uid", value, 80) < 0)
+		return -1;
+
+	p = value - 1;
+	/* look for the 4th '.' and cut there */
+	for (i = 0; i < 4; i++) {
+		p = index(p + 1, '.');
+		if (!p)
+			break;
+	}
+	if (p)
+		*p = '\0';
+
+	return strlcpy(pp->wwid, value, WWID_SIZE);
+}
+
 static ssize_t uid_fallback(struct path *pp, int path_state,
 			    const char **origin)
 {
 	ssize_t len = -1;
 
-	if (pp->bus == SYSFS_BUS_SCSI) {
+	if (pp->bus == SYSFS_BUS_CCW) {
+		len = dasd_get_uid(pp);
+		*origin = "sysfs";
+	} else if (pp->bus == SYSFS_BUS_SCSI) {
 		len = get_vpd_uid(pp);
 		*origin = "sysfs";
 		if (len < 0 && path_state == PATH_UP) {
@@ -2006,6 +2038,9 @@ static bool has_uid_fallback(struct path *pp)
 		  !strcmp(pp->uid_attribute, ""))) ||
 		(pp->bus == SYSFS_BUS_NVME &&
 		 (!strcmp(pp->uid_attribute, DEFAULT_NVME_UID_ATTRIBUTE) ||
+		  !strcmp(pp->uid_attribute, ""))) ||
+		(pp->bus == SYSFS_BUS_CCW &&
+		 (!strcmp(pp->uid_attribute, DEFAULT_DASD_UID_ATTRIBUTE) ||
 		  !strcmp(pp->uid_attribute, ""))));
 }
 
-- 
2.17.2

[RFC PATCH v2 3/3] libmultipath: change log level for null uid_attribute

[Benjamin Marzinski]

If uid_attribute is explicitly set to an empty string, multipath should
log the uid at the default log level, since using the fallback code is
the expected behavior.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/discovery.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index f650534f..435f2639 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -2091,7 +2091,8 @@ get_uid (struct path * pp, int path_state, struct udev_device *udev,
 		}
 		if ((!udev_available || (len <= 0 && allow_fallback))
 		    && has_uid_fallback(pp)) {
-			used_fallback = 1;
+			if (udev_available || !(udev && pp->uid_attribute))
+				used_fallback = 1;
 			len = uid_fallback(pp, path_state, &origin);
 		}
 	}
-- 
2.17.2

Re: [RFC PATCH v2 3/3] libmultipath: change log level for null uid_attribute

[Martin Wilck]

On Wed, 2020-09-23 at 23:59 -0500, Benjamin Marzinski wrote:
> If uid_attribute is explicitly set to an empty string, multipath
> should
> log the uid at the default log level, since using the fallback code
> is
> the expected behavior.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  libmultipath/discovery.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
> index f650534f..435f2639 100644
> --- a/libmultipath/discovery.c
> +++ b/libmultipath/discovery.c
> @@ -2091,7 +2091,8 @@ get_uid (struct path * pp, int path_state,
> struct udev_device *udev,
>  		}
>  		if ((!udev_available || (len <= 0 && allow_fallback))
>  		    && has_uid_fallback(pp)) {
> -			used_fallback = 1;
> +			if (udev_available || !(udev && pp-
> >uid_attribute))
> +				used_fallback = 1;
>  			len = uid_fallback(pp, path_state, &origin);
>  		}
>  	}

Uff, this logic was convoluted anyway, now it's even harder to grasp.
IIUC, if !udev, used_fallback will be set to 1, even if 
pp->uid_attribute is the empty string. Is that intended?
To make this easier to read, I'd suggest something like this:

		bool valid_uid_attr = pp->uid_attribute
			&& *pp->uid_attribute;
		bool empty_uid_attr = pp->uid_attribute
			&& !*pp->uid_attribute;
		bool udev_available = udev && valid_uid_attr;
      ...
		if ((!udev_available || (len <= 0 && allow_fallback))
		    && has_uid_fallback(pp)) {
			if (!empty_uid_attr)
				used_fallback = 1;
			len = uid_fallback(pp, path_state, &origin);

Regards,
Martin

-- 
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] add library to check if device is a valid path

[Martin Wilck]

On Wed, 2020-09-23 at 23:59 -0500, Benjamin Marzinski wrote:
> The main part of the this patchset is the first patch, which adds a
> new library interface to check whether devices are valid paths. This
> was designed for use in the Storage Instantiation Daemon (SID).
> 
> https://github.com/sid-project
> 
> Hopefully, I've removed all the controvertial bits from the last time
> I
> proposed this library.
> 
> The second patch adds get_uid fallback code for dasd devices. The
> third
> patch just changes the get_uid log level for devices configured with
> uid_attribute "". This is because it is currently necessary to
> configure
> multipath with
> 
> overrides {
>         uid_attribute ""
> }
> 
> to claim multipath devices with SID (instead of using
> multipath.rules),
> since SID doesn't currently get the UID information itself, and it is
> called by udev before this information is added to the udev database.
> 

This makes me wonder how SID and multipathd are supposed to coexist.
We wouldn't want this overrides directive for multipathd itself, would
we?

Actually that "overrides" begs for allowing a custom configuration file
for libmultipath to be used with SID. OTOH, that would allow the
configurations to diverge, which might cause issues, too (in particular
if blacklisting options were different).

I think what we should do is allow applications to set overrides like
this at runtime, modifying the configuration. Perhaps we could support
an application-specific, additional config_dir, from which items like
the above could be read in addition to the regular configuration files.
This additional configuration would not be used by multipathd and
multipath. Does that make sense?

Regards
Martin

-- 
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] libmultipath: change log level for null uid_attribute

[Benjamin Marzinski]

On Thu, Sep 24, 2020 at 08:06:41AM +0000, Martin Wilck wrote:
> On Wed, 2020-09-23 at 23:59 -0500, Benjamin Marzinski wrote:
> > If uid_attribute is explicitly set to an empty string, multipath
> > should
> > log the uid at the default log level, since using the fallback code
> > is
> > the expected behavior.
> > 
> > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> > ---
> >  libmultipath/discovery.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
> > index f650534f..435f2639 100644
> > --- a/libmultipath/discovery.c
> > +++ b/libmultipath/discovery.c
> > @@ -2091,7 +2091,8 @@ get_uid (struct path * pp, int path_state,
> > struct udev_device *udev,
> >  		}
> >  		if ((!udev_available || (len <= 0 && allow_fallback))
> >  		    && has_uid_fallback(pp)) {
> > -			used_fallback = 1;
> > +			if (udev_available || !(udev && pp-
> > >uid_attribute))
> > +				used_fallback = 1;
> >  			len = uid_fallback(pp, path_state, &origin);
> >  		}
> >  	}
> 
> Uff, this logic was convoluted anyway, now it's even harder to grasp.
> IIUC, if !udev, used_fallback will be set to 1, even if 
> pp->uid_attribute is the empty string. Is that intended?
> To make this easier to read, I'd suggest something like this:

My though was that not having a udev device is an unexpected situation,
and so we should continue to log the message at an increased logging
level.  If you're against that, it's not that important. I can certainly
go with your code logic, or "if (!udev || !empty_uid_attr)" if you think
it's reasonable to log at an increased level whenever the udev_device is
NULL, even if the device was configured to ignore the udev database.

-Ben

> 
> 		bool valid_uid_attr = pp->uid_attribute
> 			&& *pp->uid_attribute;
> 		bool empty_uid_attr = pp->uid_attribute
> 			&& !*pp->uid_attribute;
> 		bool udev_available = udev && valid_uid_attr;
>       ...
> 		if ((!udev_available || (len <= 0 && allow_fallback))
> 		    && has_uid_fallback(pp)) {
> 			if (!empty_uid_attr)
> 				used_fallback = 1;
> 			len = uid_fallback(pp, path_state, &origin);
> 
> Regards,
> Martin
> 
> -- 
> 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] add library to check if device is a valid path

[Benjamin Marzinski]

On Thu, Sep 24, 2020 at 08:18:02AM +0000, Martin Wilck wrote:
> On Wed, 2020-09-23 at 23:59 -0500, Benjamin Marzinski wrote:
> > The main part of the this patchset is the first patch, which adds a
> > new library interface to check whether devices are valid paths. This
> > was designed for use in the Storage Instantiation Daemon (SID).
> > 
> > https://github.com/sid-project
> > 
> > Hopefully, I've removed all the controvertial bits from the last time
> > I
> > proposed this library.
> > 
> > The second patch adds get_uid fallback code for dasd devices. The
> > third
> > patch just changes the get_uid log level for devices configured with
> > uid_attribute "". This is because it is currently necessary to
> > configure
> > multipath with
> > 
> > overrides {
> >         uid_attribute ""
> > }
> > 
> > to claim multipath devices with SID (instead of using
> > multipath.rules),
> > since SID doesn't currently get the UID information itself, and it is
> > called by udev before this information is added to the udev database.
> > 
> 
> This makes me wonder how SID and multipathd are supposed to coexist.
> We wouldn't want this overrides directive for multipathd itself, would
> we?

Don't we need it to be used by everything?  We certainly don't want
multipathd to get a different value for the wwid than SID has. It seems
like all programs that access the multipath devices should use the same
method to get the WWIDs.

The long term solution is that SID will call out to the devices, and
grab these uid attributes, just like udev currently does. It already
does this for the blkid values. When SID is running, these udev rules
will be disabled, and SID will provide udev with this data. So
eventually, multipath won't need any configuration changes to work with
SID. setting this overrides line is just a stop-gap, so that people can
test SID and multipath now. 

-Ben
 
> Actually that "overrides" begs for allowing a custom configuration file
> for libmultipath to be used with SID. OTOH, that would allow the
> configurations to diverge, which might cause issues, too (in particular
> if blacklisting options were different).
> 
> I think what we should do is allow applications to set overrides like
> this at runtime, modifying the configuration. Perhaps we could support
> an application-specific, additional config_dir, from which items like
> the above could be read in addition to the regular configuration files.
> This additional configuration would not be used by multipathd and
> multipath. Does that make sense?
> 
> Regards
> Martin
> 
> -- 
> 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] libmultipath: change log level for null uid_attribute

[Martin Wilck]

On Thu, 2020-09-24 at 11:20 -0500, Benjamin Marzinski wrote:
> On Thu, Sep 24, 2020 at 08:06:41AM +0000, Martin Wilck wrote:
> > On Wed, 2020-09-23 at 23:59 -0500, Benjamin Marzinski wrote:
> > > 
> > > diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
> > > index f650534f..435f2639 100644
> > > --- a/libmultipath/discovery.c
> > > +++ b/libmultipath/discovery.c
> > > @@ -2091,7 +2091,8 @@ get_uid (struct path * pp, int path_state,
> > > struct udev_device *udev,
> > >  		}
> > >  		if ((!udev_available || (len <= 0 && allow_fallback))
> > >  		    && has_uid_fallback(pp)) {
> > > -			used_fallback = 1;
> > > +			if (udev_available || !(udev && pp-
> > > > uid_attribute))
> > > +				used_fallback = 1;
> > >  			len = uid_fallback(pp, path_state, &origin);
> > >  		}
> > >  	}
> > 
> > Uff, this logic was convoluted anyway, now it's even harder to
> > grasp.
> > IIUC, if !udev, used_fallback will be set to 1, even if 
> > pp->uid_attribute is the empty string. Is that intended?
> > To make this easier to read, I'd suggest something like this:
> 
> My though was that not having a udev device is an unexpected
> situation,
> and so we should continue to log the message at an increased logging
> level.  If you're against that, it's not that important. I can
> certainly
> go with your code logic, or "if (!udev || !empty_uid_attr)" if you
> think
> it's reasonable to log at an increased level whenever the udev_device
> is
> NULL, even if the device was configured to ignore the udev database

My main concern was not the !udev case, nor the logging. It was just
that my brain choked on the complex boolean expression. My suggestion
was intended to make it somewhat easier to grok, that's all.

Cheers,
Martin

Re: [RFC PATCH v2 0/3] add library to check if device is a valid path

[Martin Wilck]

On Thu, 2020-09-24 at 11:30 -0500, Benjamin Marzinski wrote:
> On Thu, Sep 24, 2020 at 08:18:02AM +0000, Martin Wilck wrote:
> > On Wed, 2020-09-23 at 23:59 -0500, Benjamin Marzinski wrote:
> > > 
> > > The second patch adds get_uid fallback code for dasd devices. The
> > > third
> > > patch just changes the get_uid log level for devices configured
> > > with
> > > uid_attribute "". This is because it is currently necessary to
> > > configure
> > > multipath with
> > > 
> > > overrides {
> > >         uid_attribute ""
> > > }
> > > 
> > > to claim multipath devices with SID (instead of using
> > > multipath.rules),
> > > since SID doesn't currently get the UID information itself, and
> > > it is
> > > called by udev before this information is added to the udev
> > > database.
> > > 
> > 
> > This makes me wonder how SID and multipathd are supposed to
> > coexist.
> > We wouldn't want this overrides directive for multipathd itself,
> > would
> > we?
> 
> Don't we need it to be used by everything?  We certainly don't want
> multipathd to get a different value for the wwid than SID has. It
> seems
> like all programs that access the multipath devices should use the
> same
> method to get the WWIDs.

Well, as soon as SID is authoritative for the question whether or not a
device should be multipathed, this is certainly true.

That would mean, then, that multipath-tools would abandon the current
philosophy of relying on udev, and attempt to derive device properties
directly from sysfs instead. That makes a certain amount of sense to
me, even though it's contrary to what we've been doing the last years.
Relying on udev is not without issues, as we both know. uevents being
delayed or never delivered is one problem, the other is the fact that
udev rules can be customized without limits, leading to a
profliferation of variables and configuration options. All this has
historical reasons, today we could very well obtain almost all device
attributes we need directly from sysfs.

... but I gather that this is a temporary scenario ... 

> The long term solution is that SID will call out to the devices, and
> grab these uid attributes, just like udev currently does. It already
> does this for the blkid values. When SID is running, these udev rules
> will be disabled, and SID will provide udev with this data. So
> eventually, multipath won't need any configuration changes to work
> with
> SID. setting this overrides line is just a stop-gap, so that people
> can
> test SID and multipath now.

So, SID will call into libmultipath via libmpathvalid, udev will obtain
the properties from SID, and multipathd will fetch them from udev in
turn? Or will multipathd talk directly to SID? I seem to be missing the
overall picture.

Anyway, if you can live with simply adding an "overrides" statement to
multipath.conf for SID at the current stage, fine with me.

Forgot to say: ACK for this series from me, with the exception of the
minor nit I had on #3.

Regards,
Martin

-- 
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] add library to check if device is a valid path

[Benjamin Marzinski]

On Thu, Sep 24, 2020 at 07:22:21PM +0000, Martin Wilck wrote:
> On Thu, 2020-09-24 at 11:30 -0500, Benjamin Marzinski wrote:
> > On Thu, Sep 24, 2020 at 08:18:02AM +0000, Martin Wilck wrote:
> > > On Wed, 2020-09-23 at 23:59 -0500, Benjamin Marzinski wrote:
> > > > 
> > > > The second patch adds get_uid fallback code for dasd devices. The
> > > > third
> > > > patch just changes the get_uid log level for devices configured
> > > > with
> > > > uid_attribute "". This is because it is currently necessary to
> > > > configure
> > > > multipath with
> > > > 
> > > > overrides {
> > > >         uid_attribute ""
> > > > }
> > > > 
> > > > to claim multipath devices with SID (instead of using
> > > > multipath.rules),
> > > > since SID doesn't currently get the UID information itself, and
> > > > it is
> > > > called by udev before this information is added to the udev
> > > > database.
> > > > 
> > > 
> > > This makes me wonder how SID and multipathd are supposed to
> > > coexist.
> > > We wouldn't want this overrides directive for multipathd itself,
> > > would
> > > we?
> > 
> > Don't we need it to be used by everything?  We certainly don't want
> > multipathd to get a different value for the wwid than SID has. It
> > seems
> > like all programs that access the multipath devices should use the
> > same
> > method to get the WWIDs.
> 
> Well, as soon as SID is authoritative for the question whether or not a
> device should be multipathed, this is certainly true.
> 
> That would mean, then, that multipath-tools would abandon the current
> philosophy of relying on udev, and attempt to derive device properties
> directly from sysfs instead. That makes a certain amount of sense to
> me, even though it's contrary to what we've been doing the last years.
> Relying on udev is not without issues, as we both know. uevents being
> delayed or never delivered is one problem, the other is the fact that
> udev rules can be customized without limits, leading to a
> profliferation of variables and configuration options. All this has
> historical reasons, today we could very well obtain almost all device
> attributes we need directly from sysfs.
> 
> ... but I gather that this is a temporary scenario ... 
> 
> > The long term solution is that SID will call out to the devices, and
> > grab these uid attributes, just like udev currently does. It already
> > does this for the blkid values. When SID is running, these udev rules
> > will be disabled, and SID will provide udev with this data. So
> > eventually, multipath won't need any configuration changes to work
> > with
> > SID. setting this overrides line is just a stop-gap, so that people
> > can
> > test SID and multipath now.
> 
> So, SID will call into libmultipath via libmpathvalid, udev will obtain
> the properties from SID, and multipathd will fetch them from udev in
> turn? Or will multipathd talk directly to SID? I seem to be missing the
> overall picture.

Yeah. SID will populate the udev database with the necessary udev
properties, and multipathd will get those udev properties just like it
always does. There is no change necessary to multipathd. Right now, the
only thing that SID's multipath module replaces is multipath.rules, and
it doesn't currently handle smart mode, since the part of SID that's
necssary to trigger and respond to a timeout doesn't exist yet.

SID sets the udev properties just like anything else called by udev.
There's a SID udev rule like this:

# cat /lib/udev/rules.d/00-sid.rules 
SUBSYSTEM=="block", ACTION=="add|change|remove", IMPORT{program}="usid scan"

> Anyway, if you can live with simply adding an "overrides" statement to
> multipath.conf for SID at the current stage, fine with me.
> 
> Forgot to say: ACK for this series from me, with the exception of the
> minor nit I had on #3.
> 
> Regards,
> Martin
> 
> -- 
> 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] add library to check if device is a valid path

[Martin Wilck]

On Thu, 2020-09-24 at 20:08 -0500, Benjamin Marzinski wrote:
> On Thu, Sep 24, 2020 at 07:22:21PM +0000, Martin Wilck wrote:
> > 
> > So, SID will call into libmultipath via libmpathvalid, udev will
> > obtain
> > the properties from SID, and multipathd will fetch them from udev
> > in
> > turn? Or will multipathd talk directly to SID? I seem to be missing
> > the
> > overall picture.
> 
> Yeah. SID will populate the udev database with the necessary udev
> properties, and multipathd will get those udev properties just like
> it
> always does. 

But then I'm not getting how you'll get along with a SID-specific
configuration for libmultipath's behavior. You want get_uid() to use
direct sysfs access for SID, and use udev for multipath(d). How else
would you achieve that?

More generally, I'm not quite convinced of the the design yet. The
information flow kernel -> (sysfs or ioctl) -> libmultipath(sid mode) 
-> libmpathvalid -> SID -> udev -> udev db -> libudev -> libmultipath
(udev mode) -> multipathd is more complex than it needs to be. It might
actually increase the lags experienced by multipathd, which will still
have to wait for uevent workers to finish until it can be certain about
device properties. Not to mention that SID must be rock stable and
always available during boot, initrd processing, etc.

Why don't we rather write a common library for determining WWIDs and
the "should be multipathed" predicate, to be used by udev (with a
plugin), multipath-tools, SID, and possibly other tools like systemd
and LVM, with common, simple configuration, guaranteed to always
provide the same results? I mean, libmultipath already has all the
"intelligence" built-in to do this. We'd "just" need to cut down
configuration options drastically to get more reprocucible results, and
refactor things to obtain a minimalistic API. Unlike the current
libmpathvalid design, this wouldn't be built on top of current
libmultipath, rather vice-versa. multipath-tools would also benefit a
lot from such work.

Regards
Martin

-- 
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] add library to check if device is a valid path

[Benjamin Marzinski]

On Fri, Sep 25, 2020 at 10:01:01AM +0000, Martin Wilck wrote:
> On Thu, 2020-09-24 at 20:08 -0500, Benjamin Marzinski wrote:
> > On Thu, Sep 24, 2020 at 07:22:21PM +0000, Martin Wilck wrote:
> > > 
> > > So, SID will call into libmultipath via libmpathvalid, udev will
> > > obtain
> > > the properties from SID, and multipathd will fetch them from udev
> > > in
> > > turn? Or will multipathd talk directly to SID? I seem to be missing
> > > the
> > > overall picture.
> > 
> > Yeah. SID will populate the udev database with the necessary udev
> > properties, and multipathd will get those udev properties just like
> > it
> > always does. 
> 
> But then I'm not getting how you'll get along with a SID-specific
> configuration for libmultipath's behavior. You want get_uid() to use
> direct sysfs access for SID, and use udev for multipath(d). How else
> would you achieve that?

In the future, libmapathvalid will have access to the udev properties
that SID will be passing back to udev, so it can use the same data.
Those values just aren't there yet. I admit, how exactly this will work
is not completely nailed down.
 
> More generally, I'm not quite convinced of the the design yet. The
> information flow kernel -> (sysfs or ioctl) -> libmultipath(sid mode) 
> -> libmpathvalid -> SID -> udev -> udev db -> libudev -> libmultipath
> (udev mode) -> multipathd is more complex than it needs to be. It might
> actually increase the lags experienced by multipathd, which will still
> have to wait for uevent workers to finish until it can be certain about
> device properties. Not to mention that SID must be rock stable and
> always available during boot, initrd processing, etc.

Yes, clearly SID will need to be just as robust as udev.

> Why don't we rather write a common library for determining WWIDs and
> the "should be multipathed" predicate, to be used by udev (with a
> plugin), multipath-tools, SID, and possibly other tools like systemd
> and LVM, with common, simple configuration, guaranteed to always
> provide the same results? I mean, libmultipath already has all the
> "intelligence" built-in to do this. We'd "just" need to cut down
> configuration options drastically to get more reprocucible results, and
> refactor things to obtain a minimalistic API. Unlike the current
> libmpathvalid design, this wouldn't be built on top of current
> libmultipath, rather vice-versa. multipath-tools would also benefit a
> lot from such work.

Right now a lot of this infomation is being gathered by libraries. For
udev's builtin commands, those libraries are already being called
directly. SID will just be calling all those libraries directly, instead
of having to exec a program that bascially just calles the library.
Obviously not everything is in a library, however. But the idea of WWID
library sounds great. libmultipath probably doesn't have all the
intelligence built-in, because I assume people would want this library
would handle more device types than multipath does. Although you are
correct that just with what libmultipath does now, it would still have
a use.

-Ben

> Regards
> Martin
> 
> -- 
> 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
>