[PATCH 00/11] multipath-tools: add linker version scripts

[PATCH 00/11] multipath-tools: add linker version scripts

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Hi Christophe, hi Ben,

Patch 1-5 are small fixes, the first two resent from an earlier
submission. Patch 6ff. add version scripts for the linker to
libmultipath, libmpathpersist, and libmpathcmd.

Is it useful to do this for libmultipath? We have always said that this is
not a public, stable ABI. However, I still believe it has merits. First of
all, it's a description of the ABI we use. It turns out that it cuts the
size of the exported symbol list of libmultipath roughly in half, which is
better than I'd expected. It leads to ld.so-time failure rather than weird
crashes in the unlikely case that non-matching binaries are used
together. It allows packaging scripts to check compatibility of binaries
and libraries without resorting to version and release. It will help us
stabilize the ABI, albeit only in the long run. Finally, it's a step
towards modernizing our code base in general.

To avoid misunderstanding, my intention is not to provide a stable or even
backward-compatible ABI in libmultipath.so.0. We're still allowed to make
changes to globally visible data structures like "struct config", and to
remove symbols from the ABI, like no serious shared library would do.
We just need to bump the ABI version when we do so.

Regards,
Martin

Martin Wilck (11):
  libmultipath: find_mpe(): don't match with empty WWID
  libmultipath: copy mpp->hwe from pp->hwe
  libmultipath: dm_map_present_by_uuid(): fix dm_task_create() call
  libdmmp tests: fix compilation
  libmultipath: prio: constify some function parameters
  libmultipath: checkers/prio: allow non-lazy .so loading
  multipath-tools Makefiles: separate rules for .so and man pages
  libmultipath: create separate .so for unit tests
  libmultipath: add linker version script
  libmpathpersist: add linker version script
  libmpathcmd: add linker version script

 libdmmp/test/libdmmp_speed_test.c       |   2 +-
 libdmmp/test/libdmmp_test.c             |   2 +-
 libmpathcmd/Makefile                    |  14 +-
 libmpathcmd/libmpathcmd.version         |  13 ++
 libmpathpersist/Makefile                |  16 +-
 libmpathpersist/libmpathpersist.version |  20 +++
 libmultipath/Makefile                   |  22 ++-
 libmultipath/checkers.c                 |  17 ++
 libmultipath/config.c                   |   2 +-
 libmultipath/configure.c                |   7 +
 libmultipath/devmapper.c                |   2 +-
 libmultipath/libmultipath.version       | 215 ++++++++++++++++++++++++
 libmultipath/prio.c                     |  26 ++-
 libmultipath/prio.h                     |   4 +-
 libmultipath/propsel.c                  |   4 +-
 libmultipath/structs.c                  |  15 ++
 libmultipath/structs.h                  |   1 +
 libmultipath/structs_vec.c              |  54 +++---
 multipathd/main.c                       |  10 --
 tests/Makefile                          |  10 +-
 20 files changed, 384 insertions(+), 72 deletions(-)
 create mode 100644 libmpathcmd/libmpathcmd.version
 create mode 100644 libmpathpersist/libmpathpersist.version
 create mode 100644 libmultipath/libmultipath.version

-- 
2.28.0

[PATCH 01/11] libmultipath: find_mpe(): don't match with empty WWID

[mwilck]

From: Martin Wilck <mwilck@suse.com>

---
 libmultipath/config.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libmultipath/config.c b/libmultipath/config.c
index b9bdbdb..df0f8f4 100644
--- a/libmultipath/config.c
+++ b/libmultipath/config.c
@@ -147,7 +147,7 @@ struct mpentry *find_mpe(vector mptable, char *wwid)
 	int i;
 	struct mpentry * mpe;
 
-	if (!wwid)
+	if (!wwid || !*wwid)
 		return NULL;
 
 	vector_foreach_slot (mptable, mpe, i)
-- 
2.28.0

[PATCH 02/11] libmultipath: copy mpp->hwe from pp->hwe

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Since f0462f0 ("libmultipath: use vector for for pp->hwe and mp->hwe"),
we've been trying to fix issues caused by paths getting freed and mpp->hwe
dangling. This approach couldn't work because we need mpp->hwe to persist,
even if all paths are removed from the map. Before f0462f0, a simple
assignment worked, because the lifetime of the hwe wasn't bound to the
path. But now, we need to copy the vector. It turns out that we need to set
mpp->hwe only in two places, add_map_with_path() and setup_map(), and
that the code is simplified overall.

Even now, it can happen that a map is added with add_map_without_paths(),
and has no paths. In that case, calling do_set_from_hwe() with a NULL
pointer is not a bug, so remove the message.

Fixes: f0462f0 ("libmultipath: use vector for for pp->hwe and mp->hwe")
---
 libmultipath/configure.c   |  7 +++++
 libmultipath/propsel.c     |  4 +--
 libmultipath/structs.c     | 15 +++++++++++
 libmultipath/structs.h     |  1 +
 libmultipath/structs_vec.c | 54 ++++++++++++++++----------------------
 multipathd/main.c          | 10 -------
 6 files changed, 46 insertions(+), 45 deletions(-)

diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index 6fb477f..d7afc91 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -311,6 +311,13 @@ int setup_map(struct multipath *mpp, char *params, int params_size,
 	if (mpp->disable_queueing && VECTOR_SIZE(mpp->paths) != 0)
 		mpp->disable_queueing = 0;
 
+	/*
+	 * If this map was created with add_map_without_path(),
+	 * mpp->hwe might not be set yet.
+	 */
+	if (!mpp->hwe)
+		extract_hwe_from_path(mpp);
+
 	/*
 	 * properties selectors
 	 *
diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
index 7e6e0d6..4020134 100644
--- a/libmultipath/propsel.c
+++ b/libmultipath/propsel.c
@@ -65,9 +65,7 @@ do {									\
 	__do_set_from_vec(struct hwentry, var, (src)->hwe, dest)
 
 #define do_set_from_hwe(var, src, dest, msg)				\
-	if (!src->hwe) {						\
-		condlog(0, "BUG: do_set_from_hwe called with hwe == NULL"); \
-	} else if (__do_set_from_hwe(var, src, dest)) {			\
+	if (src->hwe && __do_set_from_hwe(var, src, dest)) {		\
 		origin = msg;						\
 		goto out;						\
 	}
diff --git a/libmultipath/structs.c b/libmultipath/structs.c
index 464596f..2efad6f 100644
--- a/libmultipath/structs.c
+++ b/libmultipath/structs.c
@@ -234,6 +234,17 @@ alloc_multipath (void)
 	return mpp;
 }
 
+void *set_mpp_hwe(struct multipath *mpp, const struct path *pp)
+{
+	if (!mpp || !pp || !pp->hwe)
+		return NULL;
+	if (mpp->hwe)
+		return mpp->hwe;
+	mpp->hwe = vector_convert(NULL, pp->hwe,
+				  struct hwentry, identity);
+	return mpp->hwe;
+}
+
 void free_multipath_attributes(struct multipath *mpp)
 {
 	if (!mpp)
@@ -290,6 +301,10 @@ free_multipath (struct multipath * mpp, enum free_path_mode free_paths)
 
 	free_pathvec(mpp->paths, free_paths);
 	free_pgvec(mpp->pg, free_paths);
+	if (mpp->hwe) {
+		vector_free(mpp->hwe);
+		mpp->hwe = NULL;
+	}
 	FREE_PTR(mpp->mpcontext);
 	FREE(mpp);
 }
diff --git a/libmultipath/structs.h b/libmultipath/structs.h
index 4afd3e8..3bd2bbd 100644
--- a/libmultipath/structs.h
+++ b/libmultipath/structs.h
@@ -421,6 +421,7 @@ struct host_group {
 struct path * alloc_path (void);
 struct pathgroup * alloc_pathgroup (void);
 struct multipath * alloc_multipath (void);
+void *set_mpp_hwe(struct multipath *mpp, const struct path *pp);
 void uninitialize_path(struct path *pp);
 void free_path (struct path *);
 void free_pathvec (vector vec, enum free_path_mode free_paths);
diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
index 7fd860e..b10d347 100644
--- a/libmultipath/structs_vec.c
+++ b/libmultipath/structs_vec.c
@@ -282,11 +282,6 @@ err:
 void orphan_path(struct path *pp, const char *reason)
 {
 	condlog(3, "%s: orphan path, %s", pp->dev, reason);
-	if (pp->mpp && pp->hwe && pp->mpp->hwe == pp->hwe) {
-		condlog(0, "BUG: orphaning path %s that holds hwe of %s",
-			pp->dev, pp->mpp->alias);
-		pp->mpp->hwe = NULL;
-	}
 	pp->mpp = NULL;
 	uninitialize_path(pp);
 }
@@ -296,8 +291,6 @@ void orphan_paths(vector pathvec, struct multipath *mpp, const char *reason)
 	int i;
 	struct path * pp;
 
-	/* Avoid BUG message from orphan_path() */
-	mpp->hwe = NULL;
 	vector_foreach_slot (pathvec, pp, i) {
 		if (pp->mpp == mpp) {
 			if (pp->initialized == INIT_REMOVED) {
@@ -385,24 +378,26 @@ extract_hwe_from_path(struct multipath * mpp)
 	if (mpp->hwe || !mpp->paths)
 		return;
 
-	condlog(3, "%s: searching paths for valid hwe", mpp->alias);
+	condlog(4, "%s: searching paths for valid hwe", mpp->alias);
 	/* doing this in two passes seems like paranoia to me */
 	vector_foreach_slot(mpp->paths, pp, i) {
-		if (pp->state != PATH_UP)
-			continue;
-		if (pp->hwe) {
-			mpp->hwe = pp->hwe;
-			return;
-		}
+		if (pp->state == PATH_UP &&
+		    pp->initialized != INIT_REMOVED && pp->hwe)
+			goto done;
 	}
 	vector_foreach_slot(mpp->paths, pp, i) {
-		if (pp->state == PATH_UP)
-			continue;
-		if (pp->hwe) {
-			mpp->hwe = pp->hwe;
-			return;
-		}
+		if (pp->state != PATH_UP &&
+		    pp->initialized != INIT_REMOVED && pp->hwe)
+			goto done;
 	}
+done:
+	if (i < VECTOR_SIZE(mpp->paths))
+		(void)set_mpp_hwe(mpp, pp);
+
+	if (mpp->hwe)
+		condlog(3, "%s: got hwe from path %s", mpp->alias, pp->dev);
+	else
+		condlog(2, "%s: no hwe found", mpp->alias);
 }
 
 int
@@ -502,8 +497,6 @@ void sync_paths(struct multipath *mpp, vector pathvec)
 		}
 		if (!found) {
 			condlog(3, "%s dropped path %s", mpp->alias, pp->dev);
-			if (mpp->hwe == pp->hwe)
-				mpp->hwe = NULL;
 			vector_del_slot(mpp->paths, i--);
 			orphan_path(pp, "path removed externally");
 		}
@@ -512,8 +505,6 @@ void sync_paths(struct multipath *mpp, vector pathvec)
 	update_mpp_paths(mpp, pathvec);
 	vector_foreach_slot (mpp->paths, pp, i)
 		pp->mpp = mpp;
-	if (mpp->hwe == NULL)
-		extract_hwe_from_path(mpp);
 }
 
 int
@@ -689,9 +680,15 @@ struct multipath *add_map_with_path(struct vectors *vecs, struct path *pp,
 
 	conf = get_multipath_config();
 	mpp->mpe = find_mpe(conf->mptable, pp->wwid);
-	mpp->hwe = pp->hwe;
 	put_multipath_config(conf);
 
+	/*
+	 * We need to call this before select_alias(),
+	 * because that accesses hwe properties.
+	 */
+	if (pp->hwe && !set_mpp_hwe(mpp, pp))
+		goto out;
+
 	strcpy(mpp->wwid, pp->wwid);
 	find_existing_alias(mpp, vecs);
 	if (select_alias(conf, mpp))
@@ -742,12 +739,6 @@ int verify_paths(struct multipath *mpp)
 			vector_del_slot(mpp->paths, i);
 			i--;
 
-			/* Make sure mpp->hwe doesn't point to freed memory.
-			 * We call extract_hwe_from_path() below to restore
-			 * mpp->hwe
-			 */
-			if (mpp->hwe == pp->hwe)
-				mpp->hwe = NULL;
 			/*
 			 * Don't delete path from pathvec yet. We'll do this
 			 * after the path has been removed from the map, in
@@ -759,7 +750,6 @@ int verify_paths(struct multipath *mpp)
 				mpp->alias, pp->dev, pp->dev_t);
 		}
 	}
-	extract_hwe_from_path(mpp);
 	return count;
 }
 
diff --git a/multipathd/main.c b/multipathd/main.c
index a4abbb2..eedc6c1 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -1153,13 +1153,6 @@ ev_remove_path (struct path *pp, struct vectors * vecs, int need_do_map)
 		if (i != -1)
 			vector_del_slot(mpp->paths, i);
 
-		/*
-		 * Make sure mpp->hwe doesn't point to freed memory
-		 * We call extract_hwe_from_path() below to restore mpp->hwe
-		 */
-		if (mpp->hwe == pp->hwe)
-			mpp->hwe = NULL;
-
 		/*
 		 * remove the map IF removing the last path
 		 */
@@ -1191,9 +1184,6 @@ ev_remove_path (struct path *pp, struct vectors * vecs, int need_do_map)
 			 */
 		}
 
-		if (mpp->hwe == NULL)
-			extract_hwe_from_path(mpp);
-
 		if (setup_map(mpp, params, PARAMS_SIZE, vecs)) {
 			condlog(0, "%s: failed to setup map for"
 				" removal of path %s", mpp->alias, pp->dev);
-- 
2.28.0

[PATCH 03/11] libmultipath: dm_map_present_by_uuid(): fix dm_task_create() call

[mwilck]

From: Martin Wilck <mwilck@suse.com>

libmultipath shouldn't call dm_task_create() directly any more.

Fixes: 90535a3 ("multipath: centralize validation code")
---
 libmultipath/devmapper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
index b4d77cb..7394796 100644
--- a/libmultipath/devmapper.c
+++ b/libmultipath/devmapper.c
@@ -834,7 +834,7 @@ dm_map_present_by_uuid(const char *uuid)
 	if (safe_sprintf(prefixed_uuid, UUID_PREFIX "%s", uuid))
 		goto out;
 
-	if (!(dmt = dm_task_create(DM_DEVICE_INFO)))
+	if (!(dmt = libmp_dm_task_create(DM_DEVICE_INFO)))
 		goto out;
 
 	dm_task_no_open_count(dmt);
-- 
2.28.0

[PATCH 04/11] libdmmp tests: fix compilation

[mwilck]

From: Martin Wilck <mwilck@suse.com>

These tests don't compile with -Werror=unused-parameter. Fix it.
---
 libdmmp/test/libdmmp_speed_test.c | 2 +-
 libdmmp/test/libdmmp_test.c       | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/libdmmp/test/libdmmp_speed_test.c b/libdmmp/test/libdmmp_speed_test.c
index 372cd39..d91ba50 100644
--- a/libdmmp/test/libdmmp_speed_test.c
+++ b/libdmmp/test/libdmmp_speed_test.c
@@ -27,7 +27,7 @@
 
 #include <libdmmp/libdmmp.h>
 
-int main(int argc, char *argv[])
+int main(void)
 {
 	struct dmmp_context *ctx = NULL;
 	struct dmmp_mpath **dmmp_mps = NULL;
diff --git a/libdmmp/test/libdmmp_test.c b/libdmmp/test/libdmmp_test.c
index d944e1e..a940b57 100644
--- a/libdmmp/test/libdmmp_test.c
+++ b/libdmmp/test/libdmmp_test.c
@@ -102,7 +102,7 @@ out:
 	return rc;
 }
 
-int main(int argc, char *argv[])
+int main(void)
 {
 	struct dmmp_context *ctx = NULL;
 	struct dmmp_mpath **dmmp_mps = NULL;
-- 
2.28.0

[PATCH 05/11] libmultipath: prio: constify some function parameters

[mwilck]

From: Martin Wilck <mwilck@suse.com>

---
 libmultipath/prio.c | 4 ++--
 libmultipath/prio.h | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/libmultipath/prio.c b/libmultipath/prio.c
index 194563c..3a718ba 100644
--- a/libmultipath/prio.c
+++ b/libmultipath/prio.c
@@ -18,7 +18,7 @@ unsigned int get_prio_timeout(unsigned int timeout_ms,
 	return default_timeout;
 }
 
-int init_prio (char *multipath_dir)
+int init_prio (const char *multipath_dir)
 {
 	if (!add_prio(multipath_dir, DEFAULT_PRIO))
 		return 1;
@@ -87,7 +87,7 @@ int prio_set_args (struct prio * p, const char * args)
 	return snprintf(p->args, PRIO_ARGS_LEN, "%s", args);
 }
 
-struct prio * add_prio (char *multipath_dir, char * name)
+struct prio * add_prio (const char *multipath_dir, const char * name)
 {
 	char libname[LIB_PRIO_NAMELEN];
 	struct stat stbuf;
diff --git a/libmultipath/prio.h b/libmultipath/prio.h
index 599d1d8..26754f7 100644
--- a/libmultipath/prio.h
+++ b/libmultipath/prio.h
@@ -55,9 +55,9 @@ struct prio {
 
 unsigned int get_prio_timeout(unsigned int checker_timeout,
 			      unsigned int default_timeout);
-int init_prio (char *);
+int init_prio (const char *);
 void cleanup_prio (void);
-struct prio * add_prio (char *, char *);
+struct prio * add_prio (const char *, const char *);
 int prio_getprio (struct prio *, struct path *, unsigned int);
 void prio_get (char *, struct prio *, char *, char *);
 void prio_put (struct prio *);
-- 
2.28.0

[PATCH 06/11] libmultipath: checkers/prio: allow non-lazy .so loading

[mwilck]

From: Martin Wilck <mwilck@suse.com>

If compiled with -DLOAD_ALL_SHARED_LIBS, all known prioritizers
and checkers will be loaded immediately on startup. This is useful
for determining symbol usage (start executables with LD_BIND_NOW=1).
---
 libmultipath/checkers.c | 17 +++++++++++++++++
 libmultipath/prio.c     | 22 ++++++++++++++++++++++
 2 files changed, 39 insertions(+)

diff --git a/libmultipath/checkers.c b/libmultipath/checkers.c
index f7ddd53..18b1f5e 100644
--- a/libmultipath/checkers.c
+++ b/libmultipath/checkers.c
@@ -7,6 +7,7 @@
 #include "debug.h"
 #include "checkers.h"
 #include "vector.h"
+#include "util.h"
 
 struct checker_class {
 	struct list_head node;
@@ -375,7 +376,23 @@ void checker_get(const char *multipath_dir, struct checker *dst,
 
 int init_checkers(const char *multipath_dir)
 {
+#ifdef LOAD_ALL_SHARED_LIBS
+	static const char *const all_checkers[] = {
+		DIRECTIO,
+		TUR,
+		HP_SW,
+		RDAC,
+		EMC_CLARIION,
+		READSECTOR0,
+		CCISS_TUR,
+	};
+	unsigned int i;
+
+	for (i = 0; i < ARRAY_SIZE(all_checkers); i++)
+		add_checker_class(multipath_dir, all_checkers[i]);
+#else
 	if (!add_checker_class(multipath_dir, DEFAULT_CHECKER))
 		return 1;
+#endif
 	return 0;
 }
diff --git a/libmultipath/prio.c b/libmultipath/prio.c
index 3a718ba..c92bde7 100644
--- a/libmultipath/prio.c
+++ b/libmultipath/prio.c
@@ -20,8 +20,30 @@ unsigned int get_prio_timeout(unsigned int timeout_ms,
 
 int init_prio (const char *multipath_dir)
 {
+#ifdef LOAD_ALL_SHARED_LIBS
+	static const char *const all_prios[] = {
+		PRIO_ALUA,
+		PRIO_CONST,
+		PRIO_DATACORE,
+		PRIO_EMC,
+		PRIO_HDS,
+		PRIO_HP_SW,
+		PRIO_ONTAP,
+		PRIO_RANDOM,
+		PRIO_RDAC,
+		PRIO_WEIGHTED_PATH,
+		PRIO_SYSFS,
+		PRIO_PATH_LATENCY,
+		PRIO_ANA,
+	};
+	unsigned int i;
+
+	for  (i = 0; i < ARRAY_SIZE(all_prios); i++)
+		add_prio(multipath_dir, all_prios[i]);
+#else
 	if (!add_prio(multipath_dir, DEFAULT_PRIO))
 		return 1;
+#endif
 	return 0;
 }
 
-- 
2.28.0

[PATCH 07/11] multipath-tools Makefiles: separate rules for .so and man pages

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Rely more on "make" functionality than on sequential command execution.
---
 libmpathcmd/Makefile     |  8 +++++---
 libmpathpersist/Makefile | 10 +++++++---
 libmultipath/Makefile    |  8 +++++---
 3 files changed, 17 insertions(+), 9 deletions(-)

diff --git a/libmpathcmd/Makefile b/libmpathcmd/Makefile
index 0f6b816..08ccb81 100644
--- a/libmpathcmd/Makefile
+++ b/libmpathcmd/Makefile
@@ -8,13 +8,15 @@ CFLAGS += $(LIB_CFLAGS)
 
 OBJS = mpath_cmd.o
 
-all: $(LIBS)
+all:	$(DEVLIB)
 
 $(LIBS): $(OBJS)
 	$(CC) $(LDFLAGS) $(SHARED_FLAGS) -Wl,-soname=$@ -o $@ $(OBJS) $(LIBDEPS)
-	$(LN) $@ $(DEVLIB)
 
-install: $(LIBS)
+$(DEVLIB): $(LIBS)
+	$(LN) $(LIBS) $@
+
+install: all
 	$(INSTALL_PROGRAM) -d $(DESTDIR)$(syslibdir)
 	$(INSTALL_PROGRAM) -m 755 $(LIBS) $(DESTDIR)$(syslibdir)/$(LIBS)
 	$(LN) $(LIBS) $(DESTDIR)$(syslibdir)/$(DEVLIB)
diff --git a/libmpathpersist/Makefile b/libmpathpersist/Makefile
index 21fdad8..9e869fd 100644
--- a/libmpathpersist/Makefile
+++ b/libmpathpersist/Makefile
@@ -11,15 +11,19 @@ LIBDEPS += -lpthread -ldevmapper -ldl -L$(multipathdir) -lmultipath \
 
 OBJS = mpath_persist.o mpath_updatepr.o mpath_pr_ioctl.o
 
-all: $(LIBS)
+all: $(DEVLIB) man
 
 $(LIBS): $(OBJS)
 	$(CC) $(LDFLAGS) $(SHARED_FLAGS) $(LIBDEPS) -Wl,-soname=$@ -o $@ $(OBJS)
-	$(LN) $(LIBS) $(DEVLIB)
+
+$(DEVLIB): $(LIBS)
+	$(LN) $(LIBS) $@
+
+man:
 	$(GZIP) mpath_persistent_reserve_in.3 > mpath_persistent_reserve_in.3.gz
 	$(GZIP) mpath_persistent_reserve_out.3 > mpath_persistent_reserve_out.3.gz
 
-install: $(LIBS)
+install: all
 	$(INSTALL_PROGRAM) -d $(DESTDIR)$(syslibdir)
 	$(INSTALL_PROGRAM) -m 755 $(LIBS) $(DESTDIR)$(syslibdir)/$(LIBS)
 	$(INSTALL_PROGRAM) -m 755 -d $(DESTDIR)$(syslibdir)
diff --git a/libmultipath/Makefile b/libmultipath/Makefile
index e5dac5e..af5bb77 100644
--- a/libmultipath/Makefile
+++ b/libmultipath/Makefile
@@ -50,7 +50,7 @@ OBJS = memory.o parser.o vector.o devmapper.o callout.o \
 	io_err_stat.o dm-generic.o generic.o foreign.o nvme-lib.o \
 	libsg.o valid.o
 
-all: $(LIBS)
+all:	$(DEVLIB)
 
 nvme-lib.o: nvme-lib.c nvme-ioctl.c nvme-ioctl.h
 	$(CC) $(CFLAGS) -Wno-unused-function -c -o $@ $<
@@ -70,9 +70,11 @@ nvme-ioctl.h: nvme/nvme-ioctl.h
 
 $(LIBS): $(OBJS)
 	$(CC) $(LDFLAGS) $(SHARED_FLAGS) -Wl,-soname=$@ -o $@ $(OBJS) $(LIBDEPS)
-	$(LN) $@ $(DEVLIB)
 
-install:
+$(DEVLIB): $(LIBS)
+	$(LN) $(LIBS) $@
+
+install: all
 	$(INSTALL_PROGRAM) -d $(DESTDIR)$(syslibdir)
 	$(INSTALL_PROGRAM) -m 755 $(LIBS) $(DESTDIR)$(syslibdir)/$(LIBS)
 	$(INSTALL_PROGRAM) -m 755 -d $(DESTDIR)$(libdir)
-- 
2.28.0

[PATCH 08/11] libmultipath: create separate .so for unit tests

[mwilck]

From: Martin Wilck <mwilck@suse.com>

The unit tests use a lot of internal symbols that we don't want
to add to the ABI if we don't have to. As long as we don't
have to make incompatible changes to functions, we can work around
that by simply using a non-versioned library for the unit tests.
Therefore we add a seperate rule here. Do this before actually
adding a version script, to avoid breakage during bisection.
---
 libmultipath/Makefile |  7 +++++++
 tests/Makefile        | 10 ++++++----
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/libmultipath/Makefile b/libmultipath/Makefile
index af5bb77..cf13561 100644
--- a/libmultipath/Makefile
+++ b/libmultipath/Makefile
@@ -74,6 +74,13 @@ $(LIBS): $(OBJS)
 $(DEVLIB): $(LIBS)
 	$(LN) $(LIBS) $@
 
+../tests/$(LIBS): $(OBJS) $(VERSION_SCRIPT)
+	$(CC) $(LDFLAGS) $(SHARED_FLAGS) -Wl,-soname=`basename $@` \
+		-o $@ $(OBJS) $(LIBDEPS)
+	$(LN) $@ ${@:.so.0=.so}
+
+test-lib:	../tests/$(LIBS)
+
 install: all
 	$(INSTALL_PROGRAM) -d $(DESTDIR)$(syslibdir)
 	$(INSTALL_PROGRAM) -m 755 $(LIBS) $(DESTDIR)$(syslibdir)/$(LIBS)
diff --git a/tests/Makefile b/tests/Makefile
index 502377f..47e6b86 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -10,7 +10,7 @@ W_MISSING_INITIALIZERS := $(call TEST_MISSING_INITIALIZERS)
 
 CFLAGS += $(BIN_CFLAGS) -I$(multipathdir) -I$(mpathcmddir) \
 	-Wno-unused-parameter $(W_MISSING_INITIALIZERS)
-LIBDEPS += -L$(multipathdir) -L$(mpathcmddir) -lmultipath -lmpathcmd -lcmocka
+LIBDEPS += -L. -L$(mpathcmddir) -lmultipath -lmpathcmd -lcmocka
 
 TESTS := uevent parser util dmevents hwtable blacklist unaligned vpd pgpolicy \
 	 alias directio valid devt
@@ -67,7 +67,7 @@ lib/libchecktur.so:
 
 %.out:	%-test lib/libchecktur.so
 	@echo == running $< ==
-	@LD_LIBRARY_PATH=$(multipathdir):$(mpathcmddir) ./$< >$@
+	@LD_LIBRARY_PATH=.:$(mpathcmddir) ./$< >$@
 
 %.vgr:  %-test lib/libchecktur.so
 	@echo == running valgrind for $< ==
@@ -77,7 +77,7 @@ lib/libchecktur.so:
 OBJS = $(TESTS:%=%.o) test-lib.o
 
 test_clean:
-	$(RM) $(TESTS:%=%.out) $(TESTS:%=%.vgr)
+	$(RM) $(TESTS:%=%.out) $(TESTS:%=%.vgr) *.so.*
 
 valgrind_clean:
 	$(RM) $(TESTS:%=%.vgr)
@@ -97,12 +97,14 @@ dep_clean:
 	@sed -n 's/^.*__wrap_\([a-zA-Z0-9_]*\).*$$/-Wl,--wrap=\1/p' $< | \
 		sort -u | tr '\n' ' ' >$@
 
+libmultipath.so.0:
+	$(MAKE) -C $(multipathdir) test-lib
 
 # COLON will get expanded during second expansion below
 COLON:=:
 .SECONDEXPANSION:
 %-test:	%.o %.o.wrap $$($$@_OBJDEPS) $$($$@_TESTDEPS) $$($$@_TESTDEPS$$(COLON).o=.o.wrap) \
-		$(multipathdir)/libmultipath.so Makefile
+		libmultipath.so.0 Makefile
 	$(CC) $(CFLAGS) -o $@ $(LDFLAGS) $< $($@_TESTDEPS) $($@_OBJDEPS) \
 		$(LIBDEPS) $($@_LIBDEPS) \
 		$(shell cat $<.wrap) $(foreach dep,$($@_TESTDEPS),$(shell cat $(dep).wrap))
-- 
2.28.0

[PATCH 09/11] libmultipath: add linker version script

[mwilck]

From: Martin Wilck <mwilck@suse.com>

This version script documents the current ABI of libmultipath,
as used by multipath, multipathd, (lib)mpathpersist, and the
dynamically loaded libraries (prioritizers, checkers, and foreign).

This reduces the amount of exported symbols of libmultipath.so.0
by more than a half (199 vs. 434).
---
 libmultipath/Makefile             |   7 +-
 libmultipath/libmultipath.version | 215 ++++++++++++++++++++++++++++++
 2 files changed, 220 insertions(+), 2 deletions(-)
 create mode 100644 libmultipath/libmultipath.version

diff --git a/libmultipath/Makefile b/libmultipath/Makefile
index cf13561..bfc7263 100644
--- a/libmultipath/Makefile
+++ b/libmultipath/Makefile
@@ -6,6 +6,7 @@ include ../Makefile.inc
 SONAME = 0
 DEVLIB = libmultipath.so
 LIBS = $(DEVLIB).$(SONAME)
+VERSION_SCRIPT := libmultipath.version
 
 CFLAGS += $(LIB_CFLAGS) -I$(mpathcmddir) -I$(mpathpersistdir) -I$(nvmedir)
 
@@ -68,8 +69,10 @@ nvme-ioctl.c: nvme/nvme-ioctl.c
 nvme-ioctl.h: nvme/nvme-ioctl.h
 	$(call make_static,$<,$@)
 
-$(LIBS): $(OBJS)
-	$(CC) $(LDFLAGS) $(SHARED_FLAGS) -Wl,-soname=$@ -o $@ $(OBJS) $(LIBDEPS)
+
+$(LIBS): $(OBJS) $(VERSION_SCRIPT)
+	$(CC) $(LDFLAGS) $(SHARED_FLAGS) -Wl,-soname=$@ \
+		-Wl,--version-script=$(VERSION_SCRIPT) -o $@ $(OBJS) $(LIBDEPS)
 
 $(DEVLIB): $(LIBS)
 	$(LN) $(LIBS) $@
diff --git a/libmultipath/libmultipath.version b/libmultipath/libmultipath.version
new file mode 100644
index 0000000..d6ac0e1
--- /dev/null
+++ b/libmultipath/libmultipath.version
@@ -0,0 +1,215 @@
+LIBMULTIPATH_0.8.4.0 {
+global:
+	/* symbols referenced by multipath and multipathd */
+	add_foreign;
+	add_map_with_path;
+	adopt_paths;
+	alloc_multipath;
+	alloc_path;
+	alloc_path_with_pathinfo;
+	alloc_strvec;
+	change_foreign;
+	check_alias_settings;
+	checker_clear_message;
+	checker_disable;
+	checker_enable;
+	checker_is_sync;
+	checker_message;
+	checker_name;
+	checker_state_name;
+	check_foreign;
+	cleanup_checkers;
+	cleanup_foreign;
+	cleanup_lock;
+	cleanup_prio;
+	close_fd;
+	coalesce_paths;
+	convert_dev;
+	count_active_paths;
+	delete_all_foreign;
+	delete_foreign;
+	disassemble_map;
+	disassemble_status;
+	dlog;
+	dm_cancel_deferred_remove;
+	dm_drv_version;
+	dm_enablegroup;
+	dm_fail_path;
+	_dm_flush_map;
+	dm_flush_map_nopaths;
+	dm_flush_maps;
+	dm_geteventnr;
+	dm_get_info;
+	dm_get_major_minor;
+	dm_get_map;
+	dm_get_maps;
+	dm_get_multipath;
+	dm_get_status;
+	dm_get_uuid;
+	dm_is_mpath;
+	dm_mapname;
+	dm_map_present;
+	dm_queue_if_no_path;
+	dm_reassign;
+	dm_reinstate_path;
+	dm_simplecmd_noflush;
+	dm_switchgroup;
+	dm_tgt_version;
+	domap;
+	ensure_directories_exist;
+	extract_hwe_from_path;
+	filter_devnode;
+	filter_path;
+	filter_wwid;
+	find_mp_by_alias;
+	find_mp_by_minor;
+	find_mp_by_str;
+	find_mp_by_wwid;
+	find_mpe;
+	find_path_by_dev;
+	find_path_by_devt;
+	find_slot;
+	foreign_multipath_layout;
+	foreign_path_layout;
+	free_config;
+	free_multipath;
+	free_multipathvec;
+	free_path;
+	free_pathvec;
+	free_strvec;
+	get_monotonic_time;
+	get_multipath_layout;
+	get_path_layout;
+	get_pgpolicy_id;
+	get_refwwid;
+	get_state;
+	get_udev_device;
+	get_uid;
+	get_used_hwes;
+	group_by_prio;
+	init_checkers;
+	init_foreign;
+	init_prio;
+	io_err_stat_attr;
+	io_err_stat_handle_pathfail;
+	is_path_valid;
+	is_quote;
+	libmp_dm_task_create;
+	libmp_udev_set_sync_support;
+	load_config;
+	log_thread_reset;
+	log_thread_start;
+	log_thread_stop;
+	need_io_err_check;
+	normalize_timespec;
+	orphan_path;
+	orphan_paths;
+	parse_prkey_flags;
+	pathcount;
+	path_discovery;
+	path_get_tpgs;
+	pathinfo;
+	path_offline;
+	print_all_paths;
+	print_foreign_topology;
+	_print_multipath_topology;
+	pthread_cond_init_mono;
+	recv_packet;
+	recv_packet_from_client;
+	reinstate_paths;
+	remember_wwid;
+	remove_map;
+	remove_map_by_alias;
+	remove_maps;
+	remove_wwid;
+	replace_wwids;
+	reset_checker_classes;
+	select_all_tg_pt;
+	select_action;
+	select_find_multipaths_timeout;
+	select_no_path_retry;
+	select_path_group;
+	select_reservation_key;
+	send_packet;
+	set_max_fds;
+	__set_no_path_retry;
+	set_path_removed;
+	set_prkey;
+	setup_map;
+	setup_thread_attr;
+	should_multipath;
+	snprint_blacklist_report;
+	snprint_config;
+	snprint_devices;
+	snprint_foreign_multipaths;
+	snprint_foreign_paths;
+	snprint_foreign_topology;
+	_snprint_multipath;
+	snprint_multipath_header;
+	snprint_multipath_map_json;
+	_snprint_multipath_topology;
+	snprint_multipath_topology_json;
+	_snprint_path;
+	snprint_path_header;
+	snprint_status;
+	snprint_wildcards;
+	stop_io_err_stat_thread;
+	store_path;
+	store_pathinfo;
+	strchop;
+	strlcpy;
+	sync_map_state;
+	sysfs_attr_set_value;
+	sysfs_get_size;
+	sysfs_is_multipathed;
+	timespecsub;
+	trigger_paths_udev_change;
+	uevent_dispatch;
+	uevent_get_dm_str;
+	uevent_get_env_positive_int;
+	uevent_is_mpath;
+	uevent_listen;
+	update_mpp_paths;
+	update_multipath_status;
+	update_multipath_strings;
+	update_multipath_table;
+	update_pathvec_from_dm;
+	update_queue_mode_add_path;
+	update_queue_mode_del_path;
+	ux_socket_listen;
+	valid_alias;
+	vector_alloc;
+	vector_alloc_slot;
+	vector_del_slot;
+	vector_free;
+	vector_reset;
+	vector_set_slot;
+	verify_paths;
+
+	/* checkers */
+	sg_read;
+
+	/* prioritizers */
+	get_asymmetric_access_state;
+	get_prio_timeout;
+	get_target_port_group;
+	get_target_port_group_support;
+	libmp_nvme_ana_log;
+	libmp_nvme_get_nsid;
+	libmp_nvme_identify_ns;
+	log_nvme_errcode;
+	nvme_id_ctrl_ana;
+	snprint_host_wwnn;
+	snprint_host_wwpn;
+	snprint_path_serial;
+	snprint_tgt_wwnn;
+	snprint_tgt_wwpn;
+	sysfs_get_asymmetric_access_state;
+
+	/* foreign */
+	free_scandir_result;
+	sysfs_attr_get_value;
+
+local:
+	*;
+};
-- 
2.28.0

[PATCH 10/11] libmpathpersist: add linker version script

[mwilck]

From: Martin Wilck <mwilck@suse.com>

This defines the ABI of libmpathpersist in the current state.
---
 libmpathpersist/Makefile                |  6 ++++--
 libmpathpersist/libmpathpersist.version | 20 ++++++++++++++++++++
 2 files changed, 24 insertions(+), 2 deletions(-)
 create mode 100644 libmpathpersist/libmpathpersist.version

diff --git a/libmpathpersist/Makefile b/libmpathpersist/Makefile
index 9e869fd..456ce4c 100644
--- a/libmpathpersist/Makefile
+++ b/libmpathpersist/Makefile
@@ -3,6 +3,7 @@ include ../Makefile.inc
 SONAME = 0
 DEVLIB = libmpathpersist.so
 LIBS = $(DEVLIB).$(SONAME)
+VERSION_SCRIPT := libmpathpersist.version
 
 CFLAGS += $(LIB_CFLAGS) -I$(multipathdir) -I$(mpathpersistdir) -I$(mpathcmddir)
 
@@ -13,8 +14,9 @@ OBJS = mpath_persist.o mpath_updatepr.o mpath_pr_ioctl.o
 
 all: $(DEVLIB) man
 
-$(LIBS): $(OBJS)
-	$(CC) $(LDFLAGS) $(SHARED_FLAGS) $(LIBDEPS) -Wl,-soname=$@ -o $@ $(OBJS)
+$(LIBS): $(OBJS) $(VERSION_SCRIPT)
+	$(CC) $(LDFLAGS) $(SHARED_FLAGS) $(LIBDEPS) -Wl,-soname=$@ \
+		-Wl,--version-script=$(VERSION_SCRIPT) -o $@ $(OBJS)
 
 $(DEVLIB): $(LIBS)
 	$(LN) $(LIBS) $@
diff --git a/libmpathpersist/libmpathpersist.version b/libmpathpersist/libmpathpersist.version
new file mode 100644
index 0000000..1bb8212
--- /dev/null
+++ b/libmpathpersist/libmpathpersist.version
@@ -0,0 +1,20 @@
+LIBMPATHPERSIST_0.8.4.0 {
+global:
+
+	__mpath_persistent_reserve_in;
+	__mpath_persistent_reserve_out;
+	dumpHex;
+	mpath_alloc_prin_response;
+	mpath_lib_exit;
+	mpath_lib_init;
+	mpath_mx_alloc_len;
+	mpath_persistent_reserve_in;
+	mpath_persistent_reserve_init_vecs;
+	mpath_persistent_reserve_out;
+	mpath_persistent_reserve_free_vecs;
+	prin_do_scsi_ioctl;
+	prout_do_scsi_ioctl;
+	update_map_pr;
+
+local: *;
+};
-- 
2.28.0

[PATCH 11/11] libmpathcmd: add linker version script

[mwilck]

From: Martin Wilck <mwilck@suse.com>

For completeness, this isn't really necessary.
---
 libmpathcmd/Makefile            |  6 ++++--
 libmpathcmd/libmpathcmd.version | 13 +++++++++++++
 2 files changed, 17 insertions(+), 2 deletions(-)
 create mode 100644 libmpathcmd/libmpathcmd.version

diff --git a/libmpathcmd/Makefile b/libmpathcmd/Makefile
index 08ccb81..2591019 100644
--- a/libmpathcmd/Makefile
+++ b/libmpathcmd/Makefile
@@ -3,6 +3,7 @@ include ../Makefile.inc
 SONAME = 0
 DEVLIB = libmpathcmd.so
 LIBS = $(DEVLIB).$(SONAME)
+VERSION_SCRIPT := libmpathcmd.version
 
 CFLAGS += $(LIB_CFLAGS)
 
@@ -10,8 +11,9 @@ OBJS = mpath_cmd.o
 
 all:	$(DEVLIB)
 
-$(LIBS): $(OBJS)
-	$(CC) $(LDFLAGS) $(SHARED_FLAGS) -Wl,-soname=$@ -o $@ $(OBJS) $(LIBDEPS)
+$(LIBS): $(OBJS) $(VERSION_SCRIPT)
+	$(CC) $(LDFLAGS) $(SHARED_FLAGS) -Wl,-soname=$@ \
+		-Wl,--version-script=$(VERSION_SCRIPT) -o $@ $(OBJS) $(LIBDEPS)
 
 $(DEVLIB): $(LIBS)
 	$(LN) $(LIBS) $@
diff --git a/libmpathcmd/libmpathcmd.version b/libmpathcmd/libmpathcmd.version
new file mode 100644
index 0000000..5382a9a
--- /dev/null
+++ b/libmpathcmd/libmpathcmd.version
@@ -0,0 +1,13 @@
+LIBMPATHCMD_0.8.4.0 {
+global:
+	__mpath_connect;
+	mpath_connect;
+	mpath_disconnect;
+	mpath_process_cmd;
+	mpath_recv_reply;
+	mpath_recv_reply_len;
+	mpath_recv_reply_data;
+	mpath_send_cmd;
+local:
+	*;
+};
-- 
2.28.0

Re: [PATCH 02/11] libmultipath: copy mpp->hwe from pp->hwe

[Benjamin Marzinski]

On Thu, Sep 24, 2020 at 03:36:35PM +0200, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> Since f0462f0 ("libmultipath: use vector for for pp->hwe and mp->hwe"),
> we've been trying to fix issues caused by paths getting freed and mpp->hwe
> dangling. This approach couldn't work because we need mpp->hwe to persist,
> even if all paths are removed from the map. Before f0462f0, a simple
> assignment worked, because the lifetime of the hwe wasn't bound to the
> path. But now, we need to copy the vector. It turns out that we need to set
> mpp->hwe only in two places, add_map_with_path() and setup_map(), and
> that the code is simplified overall.

Unless I'm missing someting, it looks like
__mpath_persistent_reserve_out() will call select_all_tg_pt(), which
uses mpp->hwe, without ever setting it.  Granted, I don't see how this
was supposed to work before your patch either.

-Ben

> 
> Even now, it can happen that a map is added with add_map_without_paths(),
> and has no paths. In that case, calling do_set_from_hwe() with a NULL
> pointer is not a bug, so remove the message.
> 
> Fixes: f0462f0 ("libmultipath: use vector for for pp->hwe and mp->hwe")
> ---
>  libmultipath/configure.c   |  7 +++++
>  libmultipath/propsel.c     |  4 +--
>  libmultipath/structs.c     | 15 +++++++++++
>  libmultipath/structs.h     |  1 +
>  libmultipath/structs_vec.c | 54 ++++++++++++++++----------------------
>  multipathd/main.c          | 10 -------
>  6 files changed, 46 insertions(+), 45 deletions(-)
> 
> diff --git a/libmultipath/configure.c b/libmultipath/configure.c
> index 6fb477f..d7afc91 100644
> --- a/libmultipath/configure.c
> +++ b/libmultipath/configure.c
> @@ -311,6 +311,13 @@ int setup_map(struct multipath *mpp, char *params, int params_size,
>  	if (mpp->disable_queueing && VECTOR_SIZE(mpp->paths) != 0)
>  		mpp->disable_queueing = 0;
>  
> +	/*
> +	 * If this map was created with add_map_without_path(),
> +	 * mpp->hwe might not be set yet.
> +	 */
> +	if (!mpp->hwe)
> +		extract_hwe_from_path(mpp);
> +
>  	/*
>  	 * properties selectors
>  	 *
> diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
> index 7e6e0d6..4020134 100644
> --- a/libmultipath/propsel.c
> +++ b/libmultipath/propsel.c
> @@ -65,9 +65,7 @@ do {									\
>  	__do_set_from_vec(struct hwentry, var, (src)->hwe, dest)
>  
>  #define do_set_from_hwe(var, src, dest, msg)				\
> -	if (!src->hwe) {						\
> -		condlog(0, "BUG: do_set_from_hwe called with hwe == NULL"); \
> -	} else if (__do_set_from_hwe(var, src, dest)) {			\
> +	if (src->hwe && __do_set_from_hwe(var, src, dest)) {		\
>  		origin = msg;						\
>  		goto out;						\
>  	}
> diff --git a/libmultipath/structs.c b/libmultipath/structs.c
> index 464596f..2efad6f 100644
> --- a/libmultipath/structs.c
> +++ b/libmultipath/structs.c
> @@ -234,6 +234,17 @@ alloc_multipath (void)
>  	return mpp;
>  }
>  
> +void *set_mpp_hwe(struct multipath *mpp, const struct path *pp)
> +{
> +	if (!mpp || !pp || !pp->hwe)
> +		return NULL;
> +	if (mpp->hwe)
> +		return mpp->hwe;
> +	mpp->hwe = vector_convert(NULL, pp->hwe,
> +				  struct hwentry, identity);
> +	return mpp->hwe;
> +}
> +
>  void free_multipath_attributes(struct multipath *mpp)
>  {
>  	if (!mpp)
> @@ -290,6 +301,10 @@ free_multipath (struct multipath * mpp, enum free_path_mode free_paths)
>  
>  	free_pathvec(mpp->paths, free_paths);
>  	free_pgvec(mpp->pg, free_paths);
> +	if (mpp->hwe) {
> +		vector_free(mpp->hwe);
> +		mpp->hwe = NULL;
> +	}
>  	FREE_PTR(mpp->mpcontext);
>  	FREE(mpp);
>  }
> diff --git a/libmultipath/structs.h b/libmultipath/structs.h
> index 4afd3e8..3bd2bbd 100644
> --- a/libmultipath/structs.h
> +++ b/libmultipath/structs.h
> @@ -421,6 +421,7 @@ struct host_group {
>  struct path * alloc_path (void);
>  struct pathgroup * alloc_pathgroup (void);
>  struct multipath * alloc_multipath (void);
> +void *set_mpp_hwe(struct multipath *mpp, const struct path *pp);
>  void uninitialize_path(struct path *pp);
>  void free_path (struct path *);
>  void free_pathvec (vector vec, enum free_path_mode free_paths);
> diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
> index 7fd860e..b10d347 100644
> --- a/libmultipath/structs_vec.c
> +++ b/libmultipath/structs_vec.c
> @@ -282,11 +282,6 @@ err:
>  void orphan_path(struct path *pp, const char *reason)
>  {
>  	condlog(3, "%s: orphan path, %s", pp->dev, reason);
> -	if (pp->mpp && pp->hwe && pp->mpp->hwe == pp->hwe) {
> -		condlog(0, "BUG: orphaning path %s that holds hwe of %s",
> -			pp->dev, pp->mpp->alias);
> -		pp->mpp->hwe = NULL;
> -	}
>  	pp->mpp = NULL;
>  	uninitialize_path(pp);
>  }
> @@ -296,8 +291,6 @@ void orphan_paths(vector pathvec, struct multipath *mpp, const char *reason)
>  	int i;
>  	struct path * pp;
>  
> -	/* Avoid BUG message from orphan_path() */
> -	mpp->hwe = NULL;
>  	vector_foreach_slot (pathvec, pp, i) {
>  		if (pp->mpp == mpp) {
>  			if (pp->initialized == INIT_REMOVED) {
> @@ -385,24 +378,26 @@ extract_hwe_from_path(struct multipath * mpp)
>  	if (mpp->hwe || !mpp->paths)
>  		return;
>  
> -	condlog(3, "%s: searching paths for valid hwe", mpp->alias);
> +	condlog(4, "%s: searching paths for valid hwe", mpp->alias);
>  	/* doing this in two passes seems like paranoia to me */
>  	vector_foreach_slot(mpp->paths, pp, i) {
> -		if (pp->state != PATH_UP)
> -			continue;
> -		if (pp->hwe) {
> -			mpp->hwe = pp->hwe;
> -			return;
> -		}
> +		if (pp->state == PATH_UP &&
> +		    pp->initialized != INIT_REMOVED && pp->hwe)
> +			goto done;
>  	}
>  	vector_foreach_slot(mpp->paths, pp, i) {
> -		if (pp->state == PATH_UP)
> -			continue;
> -		if (pp->hwe) {
> -			mpp->hwe = pp->hwe;
> -			return;
> -		}
> +		if (pp->state != PATH_UP &&
> +		    pp->initialized != INIT_REMOVED && pp->hwe)
> +			goto done;
>  	}
> +done:
> +	if (i < VECTOR_SIZE(mpp->paths))
> +		(void)set_mpp_hwe(mpp, pp);
> +
> +	if (mpp->hwe)
> +		condlog(3, "%s: got hwe from path %s", mpp->alias, pp->dev);
> +	else
> +		condlog(2, "%s: no hwe found", mpp->alias);
>  }
>  
>  int
> @@ -502,8 +497,6 @@ void sync_paths(struct multipath *mpp, vector pathvec)
>  		}
>  		if (!found) {
>  			condlog(3, "%s dropped path %s", mpp->alias, pp->dev);
> -			if (mpp->hwe == pp->hwe)
> -				mpp->hwe = NULL;
>  			vector_del_slot(mpp->paths, i--);
>  			orphan_path(pp, "path removed externally");
>  		}
> @@ -512,8 +505,6 @@ void sync_paths(struct multipath *mpp, vector pathvec)
>  	update_mpp_paths(mpp, pathvec);
>  	vector_foreach_slot (mpp->paths, pp, i)
>  		pp->mpp = mpp;
> -	if (mpp->hwe == NULL)
> -		extract_hwe_from_path(mpp);
>  }
>  
>  int
> @@ -689,9 +680,15 @@ struct multipath *add_map_with_path(struct vectors *vecs, struct path *pp,
>  
>  	conf = get_multipath_config();
>  	mpp->mpe = find_mpe(conf->mptable, pp->wwid);
> -	mpp->hwe = pp->hwe;
>  	put_multipath_config(conf);
>  
> +	/*
> +	 * We need to call this before select_alias(),
> +	 * because that accesses hwe properties.
> +	 */
> +	if (pp->hwe && !set_mpp_hwe(mpp, pp))
> +		goto out;
> +
>  	strcpy(mpp->wwid, pp->wwid);
>  	find_existing_alias(mpp, vecs);
>  	if (select_alias(conf, mpp))
> @@ -742,12 +739,6 @@ int verify_paths(struct multipath *mpp)
>  			vector_del_slot(mpp->paths, i);
>  			i--;
>  
> -			/* Make sure mpp->hwe doesn't point to freed memory.
> -			 * We call extract_hwe_from_path() below to restore
> -			 * mpp->hwe
> -			 */
> -			if (mpp->hwe == pp->hwe)
> -				mpp->hwe = NULL;
>  			/*
>  			 * Don't delete path from pathvec yet. We'll do this
>  			 * after the path has been removed from the map, in
> @@ -759,7 +750,6 @@ int verify_paths(struct multipath *mpp)
>  				mpp->alias, pp->dev, pp->dev_t);
>  		}
>  	}
> -	extract_hwe_from_path(mpp);
>  	return count;
>  }
>  
> diff --git a/multipathd/main.c b/multipathd/main.c
> index a4abbb2..eedc6c1 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -1153,13 +1153,6 @@ ev_remove_path (struct path *pp, struct vectors * vecs, int need_do_map)
>  		if (i != -1)
>  			vector_del_slot(mpp->paths, i);
>  
> -		/*
> -		 * Make sure mpp->hwe doesn't point to freed memory
> -		 * We call extract_hwe_from_path() below to restore mpp->hwe
> -		 */
> -		if (mpp->hwe == pp->hwe)
> -			mpp->hwe = NULL;
> -
>  		/*
>  		 * remove the map IF removing the last path
>  		 */
> @@ -1191,9 +1184,6 @@ ev_remove_path (struct path *pp, struct vectors * vecs, int need_do_map)
>  			 */
>  		}
>  
> -		if (mpp->hwe == NULL)
> -			extract_hwe_from_path(mpp);
> -
>  		if (setup_map(mpp, params, PARAMS_SIZE, vecs)) {
>  			condlog(0, "%s: failed to setup map for"
>  				" removal of path %s", mpp->alias, pp->dev);
> -- 
> 2.28.0

Re: [PATCH 06/11] libmultipath: checkers/prio: allow non-lazy .so loading

[Benjamin Marzinski]

On Thu, Sep 24, 2020 at 03:36:39PM +0200, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> If compiled with -DLOAD_ALL_SHARED_LIBS, all known prioritizers
> and checkers will be loaded immediately on startup. This is useful
> for determining symbol usage (start executables with LD_BIND_NOW=1).

It seems like you could avoid having to remember to update these arrays
when new checker or prio DSOs are added by just scanning multipath_dir
for all the existing checkers/prioritizers.  On the flip side, I'm not
sure that's worth the extra work, and this way has the benefit of
warning if any expected chekers/priotitizers are missing, so

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>

> ---
>  libmultipath/checkers.c | 17 +++++++++++++++++
>  libmultipath/prio.c     | 22 ++++++++++++++++++++++
>  2 files changed, 39 insertions(+)
> 
> diff --git a/libmultipath/checkers.c b/libmultipath/checkers.c
> index f7ddd53..18b1f5e 100644
> --- a/libmultipath/checkers.c
> +++ b/libmultipath/checkers.c
> @@ -7,6 +7,7 @@
>  #include "debug.h"
>  #include "checkers.h"
>  #include "vector.h"
> +#include "util.h"
>  
>  struct checker_class {
>  	struct list_head node;
> @@ -375,7 +376,23 @@ void checker_get(const char *multipath_dir, struct checker *dst,
>  
>  int init_checkers(const char *multipath_dir)
>  {
> +#ifdef LOAD_ALL_SHARED_LIBS
> +	static const char *const all_checkers[] = {
> +		DIRECTIO,
> +		TUR,
> +		HP_SW,
> +		RDAC,
> +		EMC_CLARIION,
> +		READSECTOR0,
> +		CCISS_TUR,
> +	};
> +	unsigned int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(all_checkers); i++)
> +		add_checker_class(multipath_dir, all_checkers[i]);
> +#else
>  	if (!add_checker_class(multipath_dir, DEFAULT_CHECKER))
>  		return 1;
> +#endif
>  	return 0;
>  }
> diff --git a/libmultipath/prio.c b/libmultipath/prio.c
> index 3a718ba..c92bde7 100644
> --- a/libmultipath/prio.c
> +++ b/libmultipath/prio.c
> @@ -20,8 +20,30 @@ unsigned int get_prio_timeout(unsigned int timeout_ms,
>  
>  int init_prio (const char *multipath_dir)
>  {
> +#ifdef LOAD_ALL_SHARED_LIBS
> +	static const char *const all_prios[] = {
> +		PRIO_ALUA,
> +		PRIO_CONST,
> +		PRIO_DATACORE,
> +		PRIO_EMC,
> +		PRIO_HDS,
> +		PRIO_HP_SW,
> +		PRIO_ONTAP,
> +		PRIO_RANDOM,
> +		PRIO_RDAC,
> +		PRIO_WEIGHTED_PATH,
> +		PRIO_SYSFS,
> +		PRIO_PATH_LATENCY,
> +		PRIO_ANA,
> +	};
> +	unsigned int i;
> +
> +	for  (i = 0; i < ARRAY_SIZE(all_prios); i++)
> +		add_prio(multipath_dir, all_prios[i]);
> +#else
>  	if (!add_prio(multipath_dir, DEFAULT_PRIO))
>  		return 1;
> +#endif
>  	return 0;
>  }
>  
> -- 
> 2.28.0

Re: [PATCH 08/11] libmultipath: create separate .so for unit tests

[Benjamin Marzinski]

On Thu, Sep 24, 2020 at 03:36:41PM +0200, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> The unit tests use a lot of internal symbols that we don't want
> to add to the ABI if we don't have to. As long as we don't
> have to make incompatible changes to functions, we can work around
> that by simply using a non-versioned library for the unit tests.
> Therefore we add a seperate rule here. Do this before actually
> adding a version script, to avoid breakage during bisection.
> ---
>  libmultipath/Makefile |  7 +++++++
>  tests/Makefile        | 10 ++++++----
>  2 files changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/libmultipath/Makefile b/libmultipath/Makefile
> index af5bb77..cf13561 100644
> --- a/libmultipath/Makefile
> +++ b/libmultipath/Makefile
> @@ -74,6 +74,13 @@ $(LIBS): $(OBJS)
>  $(DEVLIB): $(LIBS)
>  	$(LN) $(LIBS) $@
>  
> +../tests/$(LIBS): $(OBJS) $(VERSION_SCRIPT)
> +	$(CC) $(LDFLAGS) $(SHARED_FLAGS) -Wl,-soname=`basename $@` \
> +		-o $@ $(OBJS) $(LIBDEPS)
> +	$(LN) $@ ${@:.so.0=.so}
> +
> +test-lib:	../tests/$(LIBS)
> +
>  install: all
>  	$(INSTALL_PROGRAM) -d $(DESTDIR)$(syslibdir)
>  	$(INSTALL_PROGRAM) -m 755 $(LIBS) $(DESTDIR)$(syslibdir)/$(LIBS)
> diff --git a/tests/Makefile b/tests/Makefile
> index 502377f..47e6b86 100644
> --- a/tests/Makefile
> +++ b/tests/Makefile
> @@ -10,7 +10,7 @@ W_MISSING_INITIALIZERS := $(call TEST_MISSING_INITIALIZERS)
>  
>  CFLAGS += $(BIN_CFLAGS) -I$(multipathdir) -I$(mpathcmddir) \
>  	-Wno-unused-parameter $(W_MISSING_INITIALIZERS)
> -LIBDEPS += -L$(multipathdir) -L$(mpathcmddir) -lmultipath -lmpathcmd -lcmocka
> +LIBDEPS += -L. -L$(mpathcmddir) -lmultipath -lmpathcmd -lcmocka
>  
>  TESTS := uevent parser util dmevents hwtable blacklist unaligned vpd pgpolicy \
>  	 alias directio valid devt
> @@ -67,7 +67,7 @@ lib/libchecktur.so:
>  
>  %.out:	%-test lib/libchecktur.so
>  	@echo == running $< ==
> -	@LD_LIBRARY_PATH=$(multipathdir):$(mpathcmddir) ./$< >$@
> +	@LD_LIBRARY_PATH=.:$(mpathcmddir) ./$< >$@
>  
>  %.vgr:  %-test lib/libchecktur.so
>  	@echo == running valgrind for $< ==
> @@ -77,7 +77,7 @@ lib/libchecktur.so:
>  OBJS = $(TESTS:%=%.o) test-lib.o
>  
>  test_clean:
> -	$(RM) $(TESTS:%=%.out) $(TESTS:%=%.vgr)
> +	$(RM) $(TESTS:%=%.out) $(TESTS:%=%.vgr) *.so.*

Perhaps:
	$(RM) $(TESTS:%=%.out) $(TESTS:%=%.vgr) *.so*

so you also remove libmultipath.so

-Ben

>  
>  valgrind_clean:
>  	$(RM) $(TESTS:%=%.vgr)
> @@ -97,12 +97,14 @@ dep_clean:
>  	@sed -n 's/^.*__wrap_\([a-zA-Z0-9_]*\).*$$/-Wl,--wrap=\1/p' $< | \
>  		sort -u | tr '\n' ' ' >$@
>  
> +libmultipath.so.0:
> +	$(MAKE) -C $(multipathdir) test-lib
>  
>  # COLON will get expanded during second expansion below
>  COLON:=:
>  .SECONDEXPANSION:
>  %-test:	%.o %.o.wrap $$($$@_OBJDEPS) $$($$@_TESTDEPS) $$($$@_TESTDEPS$$(COLON).o=.o.wrap) \
> -		$(multipathdir)/libmultipath.so Makefile
> +		libmultipath.so.0 Makefile
>  	$(CC) $(CFLAGS) -o $@ $(LDFLAGS) $< $($@_TESTDEPS) $($@_OBJDEPS) \
>  		$(LIBDEPS) $($@_LIBDEPS) \
>  		$(shell cat $<.wrap) $(foreach dep,$($@_TESTDEPS),$(shell cat $(dep).wrap))
> -- 
> 2.28.0

Re: [PATCH 00/11] multipath-tools: add linker version scripts

[Benjamin Marzinski]

On Thu, Sep 24, 2020 at 03:36:33PM +0200, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> Hi Christophe, hi Ben,
> 
> Patch 1-5 are small fixes, the first two resent from an earlier
> submission. Patch 6ff. add version scripts for the linker to
> libmultipath, libmpathpersist, and libmpathcmd.
> 
> Is it useful to do this for libmultipath? We have always said that this is
> not a public, stable ABI. However, I still believe it has merits. First of
> all, it's a description of the ABI we use. It turns out that it cuts the
> size of the exported symbol list of libmultipath roughly in half, which is
> better than I'd expected. It leads to ld.so-time failure rather than weird
> crashes in the unlikely case that non-matching binaries are used
> together. It allows packaging scripts to check compatibility of binaries
> and libraries without resorting to version and release. It will help us
> stabilize the ABI, albeit only in the long run. Finally, it's a step
> towards modernizing our code base in general.
> 
> To avoid misunderstanding, my intention is not to provide a stable or even
> backward-compatible ABI in libmultipath.so.0. We're still allowed to make
> changes to globally visible data structures like "struct config", and to
> remove symbols from the ABI, like no serious shared library would do.
> We just need to bump the ABI version when we do so.
> 

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
For this patchset, except for 0002 and 0008

> Regards,
> Martin
> 
> Martin Wilck (11):
>   libmultipath: find_mpe(): don't match with empty WWID
>   libmultipath: copy mpp->hwe from pp->hwe
>   libmultipath: dm_map_present_by_uuid(): fix dm_task_create() call
>   libdmmp tests: fix compilation
>   libmultipath: prio: constify some function parameters
>   libmultipath: checkers/prio: allow non-lazy .so loading
>   multipath-tools Makefiles: separate rules for .so and man pages
>   libmultipath: create separate .so for unit tests
>   libmultipath: add linker version script
>   libmpathpersist: add linker version script
>   libmpathcmd: add linker version script
> 
>  libdmmp/test/libdmmp_speed_test.c       |   2 +-
>  libdmmp/test/libdmmp_test.c             |   2 +-
>  libmpathcmd/Makefile                    |  14 +-
>  libmpathcmd/libmpathcmd.version         |  13 ++
>  libmpathpersist/Makefile                |  16 +-
>  libmpathpersist/libmpathpersist.version |  20 +++
>  libmultipath/Makefile                   |  22 ++-
>  libmultipath/checkers.c                 |  17 ++
>  libmultipath/config.c                   |   2 +-
>  libmultipath/configure.c                |   7 +
>  libmultipath/devmapper.c                |   2 +-
>  libmultipath/libmultipath.version       | 215 ++++++++++++++++++++++++
>  libmultipath/prio.c                     |  26 ++-
>  libmultipath/prio.h                     |   4 +-
>  libmultipath/propsel.c                  |   4 +-
>  libmultipath/structs.c                  |  15 ++
>  libmultipath/structs.h                  |   1 +
>  libmultipath/structs_vec.c              |  54 +++---
>  multipathd/main.c                       |  10 --
>  tests/Makefile                          |  10 +-
>  20 files changed, 384 insertions(+), 72 deletions(-)
>  create mode 100644 libmpathcmd/libmpathcmd.version
>  create mode 100644 libmpathpersist/libmpathpersist.version
>  create mode 100644 libmultipath/libmultipath.version
> 
> -- 
> 2.28.0

Re: [PATCH 10/11] libmpathpersist: add linker version script

[Benjamin Marzinski]

On Thu, Sep 24, 2020 at 03:36:43PM +0200, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> This defines the ABI of libmpathpersist in the current state.
> ---
>  libmpathpersist/Makefile                |  6 ++++--
>  libmpathpersist/libmpathpersist.version | 20 ++++++++++++++++++++
>  2 files changed, 24 insertions(+), 2 deletions(-)
>  create mode 100644 libmpathpersist/libmpathpersist.version
> 
> diff --git a/libmpathpersist/Makefile b/libmpathpersist/Makefile
> index 9e869fd..456ce4c 100644
> --- a/libmpathpersist/Makefile
> +++ b/libmpathpersist/Makefile
> @@ -3,6 +3,7 @@ include ../Makefile.inc
>  SONAME = 0
>  DEVLIB = libmpathpersist.so
>  LIBS = $(DEVLIB).$(SONAME)
> +VERSION_SCRIPT := libmpathpersist.version
>  
>  CFLAGS += $(LIB_CFLAGS) -I$(multipathdir) -I$(mpathpersistdir) -I$(mpathcmddir)
>  
> @@ -13,8 +14,9 @@ OBJS = mpath_persist.o mpath_updatepr.o mpath_pr_ioctl.o
>  
>  all: $(DEVLIB) man
>  
> -$(LIBS): $(OBJS)
> -	$(CC) $(LDFLAGS) $(SHARED_FLAGS) $(LIBDEPS) -Wl,-soname=$@ -o $@ $(OBJS)
> +$(LIBS): $(OBJS) $(VERSION_SCRIPT)
> +	$(CC) $(LDFLAGS) $(SHARED_FLAGS) $(LIBDEPS) -Wl,-soname=$@ \
> +		-Wl,--version-script=$(VERSION_SCRIPT) -o $@ $(OBJS)
>  
>  $(DEVLIB): $(LIBS)
>  	$(LN) $(LIBS) $@
> diff --git a/libmpathpersist/libmpathpersist.version b/libmpathpersist/libmpathpersist.version
> new file mode 100644
> index 0000000..1bb8212
> --- /dev/null
> +++ b/libmpathpersist/libmpathpersist.version
> @@ -0,0 +1,20 @@
> +LIBMPATHPERSIST_0.8.4.0 {

I have a question about this version. Do you plan on bumping this each
time a new release is tagged? It seems like we only ever want to change
the version if we actually change the ABI. Or is 0.8.4 just because
that's the relesae where we started this?

-Ben

> +global:
> +
> +	__mpath_persistent_reserve_in;
> +	__mpath_persistent_reserve_out;
> +	dumpHex;
> +	mpath_alloc_prin_response;
> +	mpath_lib_exit;
> +	mpath_lib_init;
> +	mpath_mx_alloc_len;
> +	mpath_persistent_reserve_in;
> +	mpath_persistent_reserve_init_vecs;
> +	mpath_persistent_reserve_out;
> +	mpath_persistent_reserve_free_vecs;
> +	prin_do_scsi_ioctl;
> +	prout_do_scsi_ioctl;
> +	update_map_pr;
> +
> +local: *;
> +};
> -- 
> 2.28.0

Re: [PATCH 02/11] libmultipath: copy mpp->hwe from pp->hwe

[Martin Wilck]

On Thu, 2020-09-24 at 15:12 -0500, Benjamin Marzinski wrote:
> On Thu, Sep 24, 2020 at 03:36:35PM +0200, mwilck@suse.com wrote:
> > From: Martin Wilck <mwilck@suse.com>
> > 
> > Since f0462f0 ("libmultipath: use vector for for pp->hwe and mp-
> > >hwe"),
> > we've been trying to fix issues caused by paths getting freed and
> > mpp->hwe
> > dangling. This approach couldn't work because we need mpp->hwe to
> > persist,
> > even if all paths are removed from the map. Before f0462f0, a
> > simple
> > assignment worked, because the lifetime of the hwe wasn't bound to
> > the
> > path. But now, we need to copy the vector. It turns out that we
> > need to set
> > mpp->hwe only in two places, add_map_with_path() and setup_map(),
> > and
> > that the code is simplified overall.
> 
> Unless I'm missing someting, it looks like
> __mpath_persistent_reserve_out() will call select_all_tg_pt(), which
> uses mpp->hwe, without ever setting it.  Granted, I don't see how
> this
> was supposed to work before your patch either.

Right, it doesn't. Thanks for spotting it.
Ok to create a new, separate patch for it?

Regards,
Martin

Re: [PATCH 02/11] libmultipath: copy mpp->hwe from pp->hwe

[Benjamin Marzinski]

On Fri, Sep 25, 2020 at 06:01:26PM +0200, Martin Wilck wrote:
> On Thu, 2020-09-24 at 15:12 -0500, Benjamin Marzinski wrote:
> > On Thu, Sep 24, 2020 at 03:36:35PM +0200, mwilck@suse.com wrote:
> > > From: Martin Wilck <mwilck@suse.com>
> > > 
> > > Since f0462f0 ("libmultipath: use vector for for pp->hwe and mp-
> > > >hwe"),
> > > we've been trying to fix issues caused by paths getting freed and
> > > mpp->hwe
> > > dangling. This approach couldn't work because we need mpp->hwe to
> > > persist,
> > > even if all paths are removed from the map. Before f0462f0, a
> > > simple
> > > assignment worked, because the lifetime of the hwe wasn't bound to
> > > the
> > > path. But now, we need to copy the vector. It turns out that we
> > > need to set
> > > mpp->hwe only in two places, add_map_with_path() and setup_map(),
> > > and
> > > that the code is simplified overall.
> > 
> > Unless I'm missing someting, it looks like
> > __mpath_persistent_reserve_out() will call select_all_tg_pt(), which
> > uses mpp->hwe, without ever setting it.  Granted, I don't see how
> > this
> > was supposed to work before your patch either.
> 
> Right, it doesn't. Thanks for spotting it.
> Ok to create a new, separate patch for it?

Sure.

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>

> 
> Regards,
> Martin
> 

Re: [PATCH 10/11] libmpathpersist: add linker version script

[Martin Wilck]

On Thu, 2020-09-24 at 23:00 -0500, Benjamin Marzinski wrote:
> On Thu, Sep 24, 2020 at 03:36:43PM +0200, mwilck@suse.com wrote:
> > 
> > --- /dev/null
> > +++ b/libmpathpersist/libmpathpersist.version
> > @@ -0,0 +1,20 @@
> > +LIBMPATHPERSIST_0.8.4.0 {
> 
> I have a question about this version. Do you plan on bumping this
> each
> time a new release is tagged? It seems like we only ever want to
> change
> the version if we actually change the ABI. Or is 0.8.4 just because
> that's the relesae where we started this?

That was the idea, yes. And the last digit is because we'll have to
bump it between releases from Christophe. It makes sense for
libmultipath's rapidly changing ABI; much less so for libmpathcmd and
libmpathpersist, which are meant to be stable. I am open for discussing
these numbers; if you prefer, we might as well use LIBMPATHPERSIST_1.0.

I admit I haven't thought about what would happen once Christophe makes
a new release. Probably, nothing - afaics it's impossible to add a new
version without any new symbols, and *renaming* an existing version is
bad; it would introduce artificial incompatibility. 

So libmultipath from multipath-tools 0.8.5 would still have a 0.8.4.x
ABI; only the first change after 0.8.5 would get a 0.8.5.1 number. Hm.

Again, I'm open for discussion here. We might as well choose to not tie
the ABI version to the libmultipath version at all, and simply start at
0.1 or whatevever for libmultipath. After all, looking up the ABI
version in the commit history will be simple enough.

Martin

Re: [PATCH 00/11] multipath-tools: add linker version scripts

[Martin Wilck]

On Thu, 2020-09-24 at 21:09 -0500, Benjamin Marzinski wrote:
> 
> Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> For this patchset, except for 0002 and 0008

Thanks a lot. I've fixes for both, will resubmit as soon as we have
consensus about the ABI version numbering.

Martin

Re: [PATCH 10/11] libmpathpersist: add linker version script

[Benjamin Marzinski]

On Fri, Sep 25, 2020 at 09:52:51PM +0200, Martin Wilck wrote:
> On Thu, 2020-09-24 at 23:00 -0500, Benjamin Marzinski wrote:
> > On Thu, Sep 24, 2020 at 03:36:43PM +0200, mwilck@suse.com wrote:
> > > 
> > > --- /dev/null
> > > +++ b/libmpathpersist/libmpathpersist.version
> > > @@ -0,0 +1,20 @@
> > > +LIBMPATHPERSIST_0.8.4.0 {
> > 
> > I have a question about this version. Do you plan on bumping this
> > each
> > time a new release is tagged? It seems like we only ever want to
> > change
> > the version if we actually change the ABI. Or is 0.8.4 just because
> > that's the relesae where we started this?
> 
> That was the idea, yes. And the last digit is because we'll have to
> bump it between releases from Christophe. It makes sense for
> libmultipath's rapidly changing ABI; much less so for libmpathcmd and
> libmpathpersist, which are meant to be stable. I am open for discussing
> these numbers; if you prefer, we might as well use LIBMPATHPERSIST_1.0.
> 
> I admit I haven't thought about what would happen once Christophe makes
> a new release. Probably, nothing - afaics it's impossible to add a new
> version without any new symbols, and *renaming* an existing version is
> bad; it would introduce artificial incompatibility. 
> 
> So libmultipath from multipath-tools 0.8.5 would still have a 0.8.4.x
> ABI; only the first change after 0.8.5 would get a 0.8.5.1 number. Hm.
> 
> Again, I'm open for discussion here. We might as well choose to not tie
> the ABI version to the libmultipath version at all, and simply start at
> 0.1 or whatevever for libmultipath. After all, looking up the ABI
> version in the commit history will be simple enough.

Since the ABI version isn't always going to match the release version, I
think it makes more sense to decouple them, so it's not sometimes
matching and sometimes not. We could go with a MAJOR.MINOR versioning
scheme, where we bump MINOR whenever we change the interface in a
backwards compatible way (like by adding a new symbol), and bump the
MAJOR and reset the MINOR whenever we change the interface in a
non-backwards compatible way (like by changing the parameters an
interface function takes, or removing a symbol). Although I'm not sure
we need to be so careful for libmultipath, since that library isn't for
external consumption.

-Ben
 
> Martin
> 

Re: [PATCH 10/11] libmpathpersist: add linker version script

[Benjamin Marzinski]

On Sat, Sep 26, 2020 at 01:07:34AM +0200, Martin Wilck wrote:
> On Fri, 2020-09-25 at 17:32 -0500, Benjamin Marzinski wrote:
> > On Fri, Sep 25, 2020 at 05:10:22PM -0500, Benjamin Marzinski wrote:
> > > On Fri, Sep 25, 2020 at 09:52:51PM +0200, Martin Wilck wrote:
> > > > On Thu, 2020-09-24 at 23:00 -0500, Benjamin Marzinski wrote:
> > > > > On Thu, Sep 24, 2020 at 03:36:43PM +0200, mwilck@suse.com
> > > > > wrote:
> > > > > > --- /dev/null
> > > > > > +++ b/libmpathpersist/libmpathpersist.version
> > > > > > @@ -0,0 +1,20 @@
> > > > > > +LIBMPATHPERSIST_0.8.4.0 {
> > > > > 
> > > > > I have a question about this version. Do you plan on bumping
> > > > > this
> > > > > each
> > > > > time a new release is tagged? It seems like we only ever want
> > > > > to
> > > > > change
> > > > > the version if we actually change the ABI. Or is 0.8.4 just
> > > > > because
> > > > > that's the relesae where we started this?
> > > > 
> > > > That was the idea, yes. And the last digit is because we'll have
> > > > to
> > > > bump it between releases from Christophe. It makes sense for
> > > > libmultipath's rapidly changing ABI; much less so for libmpathcmd
> > > > and
> > > > libmpathpersist, which are meant to be stable. I am open for
> > > > discussing
> > > > these numbers; if you prefer, we might as well use
> > > > LIBMPATHPERSIST_1.0.
> > > > 
> > > > I admit I haven't thought about what would happen once Christophe
> > > > makes
> > > > a new release. Probably, nothing - afaics it's impossible to add
> > > > a new
> > > > version without any new symbols, and *renaming* an existing
> > > > version is
> > > > bad; it would introduce artificial incompatibility. 
> > > > 
> > > > So libmultipath from multipath-tools 0.8.5 would still have a
> > > > 0.8.4.x
> > > > ABI; only the first change after 0.8.5 would get a 0.8.5.1
> > > > number. Hm.
> > > > 
> > > > Again, I'm open for discussion here. We might as well choose to
> > > > not tie
> > > > the ABI version to the libmultipath version at all, and simply
> > > > start at
> > > > 0.1 or whatevever for libmultipath. After all, looking up the ABI
> > > > version in the commit history will be simple enough.
> > > 
> > > Since the ABI version isn't always going to match the release
> > > version, I
> > > think it makes more sense to decouple them, so it's not sometimes
> > > matching and sometimes not. We could go with a MAJOR.MINOR
> > > versioning
> > > scheme, where we bump MINOR whenever we change the interface in a
> > > backwards compatible way (like by adding a new symbol), and bump
> > > the
> > > MAJOR and reset the MINOR whenever we change the interface in a
> > > non-backwards compatible way (like by changing the parameters an
> > > interface function takes, or removing a symbol).
> > 
> > Perhaps we might even want MAJOR.MINOR.RELEASE, where for upstream,
> > RELEASE is always 0. Distros can increment it if they need to
> > backport
> > changes, so that they don't conflict with existing upstream versions.
> 
> Sounds good. What MAJOR.MINOR version should we start with? 0.1 ? 1.0?

I realize that multipath-tools doesn't actually have a 1.0 release, but
1.0+ releases tradiontially mean that you believe that it's stable and
ready for use. Setting the libraries to 0.1 says to me that they aren't
meant for serious use, so I would vote for 1.0.

cc-ing dm-devel, because apparently I forgot to on my last email.

-Ben
 
> Martin
>