device-mapper-multipath/0023-libmultipath-devmapper-refactor-libdm-version-determ.patch

From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Martin Wilck <mwilck@suse.com>
Date: Tue, 15 Sep 2020 23:12:15 +0200
Subject: [PATCH] libmultipath: devmapper: refactor libdm version determination

As one step towards bundling all possibly racy libdm init calls into a single
function, split the code for determining and checking versions of
libdm and kernel components. Provide a generic helper
libmp_get_version() that makes sure the versions are "lazily" initialized.

External callers may use dm_prereq(), like before.
Minor API change: dm_prereq() does not nullify the argument any more
if an error is encountered.

Remove the conf->version field, which isn't needed any more after this
change. This makes it necessary to fixup the hwtable test. Also, it
represents an incompatible ABI change as offsets in "struct config" are
changed, and two symbols removed. Bump the ABI major version to 2.

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/config.h             |   1 -
 libmultipath/devmapper.c          | 162 ++++++++++++++++++++----------
 libmultipath/devmapper.h          |  10 +-
 libmultipath/libmultipath.version |   5 +-
 libmultipath/propsel.c            |  10 +-
 multipathd/dmevents.c             |   2 +-
 multipathd/main.c                 |   1 -
 tests/Makefile                    |   2 +-
 tests/hwtable.c                   |   3 -
 tests/test-lib.c                  |  13 +++
 10 files changed, 141 insertions(+), 68 deletions(-)

diff --git a/libmultipath/config.h b/libmultipath/config.h
index 290aea58..7af19844 100644
--- a/libmultipath/config.h
+++ b/libmultipath/config.h
@@ -192,7 +192,6 @@ struct config {
 	int find_multipaths_timeout;
 	int marginal_pathgroups;
 	int skip_delegate;
-	unsigned int version[3];
 	unsigned int sequence_nr;
 
 	char * multipath_dir;
diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
index 0bc1d16e..3e36129b 100644
--- a/libmultipath/devmapper.c
+++ b/libmultipath/devmapper.c
@@ -26,6 +26,7 @@
 #include "sysfs.h"
 #include "config.h"
 #include "wwids.h"
+#include "version.h"
 
 #include "log_pthread.h"
 #include <sys/types.h>
@@ -34,7 +35,13 @@
 #define MAX_WAIT 5
 #define LOOPS_PER_SEC 5
 
+#define INVALID_VERSION ~0U
+static unsigned int dm_library_version[3] = { INVALID_VERSION, };
+static unsigned int dm_kernel_version[3] = { INVALID_VERSION, };
+static unsigned int dm_mpath_target_version[3] = { INVALID_VERSION, };
+
 static pthread_once_t dm_initialized = PTHREAD_ONCE_INIT;
+static pthread_once_t versions_initialized = PTHREAD_ONCE_INIT;
 
 static int dm_conf_verbosity;
 
@@ -102,7 +109,7 @@ dm_write_log (int level, const char *file, int line, const char *f, ...)
 	return;
 }
 
-void dm_init(int v)
+static void dm_init(int v)
 {
 	/*
 	 * This maps libdm's standard loglevel _LOG_WARN (= 4), which is rather
@@ -112,61 +119,68 @@ void dm_init(int v)
 	dm_log_init(&dm_write_log);
 }
 
+static void init_dm_library_version(void)
+{
+	char version[64];
+	unsigned int v[3];
+
+	dm_get_library_version(version, sizeof(version));
+	if (sscanf(version, "%u.%u.%u ", &v[0], &v[1], &v[2]) != 3) {
+		condlog(0, "invalid libdevmapper version %s", version);
+		return;
+	}
+	memcpy(dm_library_version, v, sizeof(dm_library_version));
+	condlog(3, "libdevmapper version %u.%.2u.%.2u",
+		dm_library_version[0], dm_library_version[1],
+		dm_library_version[2]);
+}
+
 static int
 dm_lib_prereq (void)
 {
-	char version[64];
-	int v[3];
+
 #if defined(LIBDM_API_HOLD_CONTROL)
-	int minv[3] = {1, 2, 111};
+	unsigned int minv[3] = {1, 2, 111};
 #elif defined(LIBDM_API_GET_ERRNO)
-	int minv[3] = {1, 2, 99};
+	unsigned int minv[3] = {1, 2, 99};
 #elif defined(LIBDM_API_DEFERRED)
-	int minv[3] = {1, 2, 89};
+	unsigned int minv[3] = {1, 2, 89};
 #elif defined(DM_SUBSYSTEM_UDEV_FLAG0)
-	int minv[3] = {1, 2, 82};
+	unsigned int minv[3] = {1, 2, 82};
 #elif defined(LIBDM_API_COOKIE)
-	int minv[3] = {1, 2, 38};
+	unsigned int minv[3] = {1, 2, 38};
 #else
-	int minv[3] = {1, 2, 8};
+	unsigned int minv[3] = {1, 2, 8};
 #endif
 
-	dm_get_library_version(version, sizeof(version));
-	condlog(3, "libdevmapper version %s", version);
-	if (sscanf(version, "%d.%d.%d ", &v[0], &v[1], &v[2]) != 3) {
-		condlog(0, "invalid libdevmapper version %s", version);
-		return 1;
-	}
-
-	if VERSION_GE(v, minv)
+	if (VERSION_GE(dm_library_version, minv))
 		return 0;
-	condlog(0, "libdevmapper version must be >= %d.%.2d.%.2d",
+	condlog(0, "libdevmapper version must be >= %u.%.2u.%.2u",
 		minv[0], minv[1], minv[2]);
 	return 1;
 }
 
-int
-dm_drv_version(unsigned int *v)
+static void init_dm_drv_version(void)
 {
 	char buff[64];
-
-	v[0] = 0;
-	v[1] = 0;
-	v[2] = 0;
+	unsigned int v[3];
 
 	if (!dm_driver_version(buff, sizeof(buff))) {
 		condlog(0, "cannot get kernel dm version");
-		return 1;
+		return;
 	}
 	if (sscanf(buff, "%u.%u.%u ", &v[0], &v[1], &v[2]) != 3) {
 		condlog(0, "invalid kernel dm version '%s'", buff);
-		return 1;
+		return;
 	}
-	return 0;
+	memcpy(dm_kernel_version, v, sizeof(dm_library_version));
+	condlog(3, "kernel device mapper v%u.%u.%u",
+		dm_kernel_version[0],
+		dm_kernel_version[1],
+		dm_kernel_version[2]);
 }
 
-int
-dm_tgt_version (unsigned int * version, char * str)
+static int dm_tgt_version (unsigned int *version, char *str)
 {
 	int r = 2;
 	struct dm_task *dmt;
@@ -174,10 +188,11 @@ dm_tgt_version (unsigned int * version, char * str)
 	struct dm_versions *last_target;
 	unsigned int *v;
 
-	version[0] = 0;
-	version[1] = 0;
-	version[2] = 0;
-
+	/*
+	 * We have to call dm_task_create() and not libmp_dm_task_create()
+	 * here to avoid a recursive invocation of
+	 * pthread_once(&dm_initialized), which would cause a deadlock.
+	 */
 	if (!(dmt = dm_task_create(DM_DEVICE_LIST_VERSIONS)))
 		return 1;
 
@@ -213,26 +228,25 @@ out:
 	return r;
 }
 
-static int
-dm_tgt_prereq (unsigned int *ver)
+static void init_dm_mpath_version(void)
 {
-	unsigned int minv[3] = {1, 0, 3};
-	unsigned int version[3] = {0, 0, 0};
-	unsigned int * v = version;
-
-	if (dm_tgt_version(v, TGT_MPATH)) {
-		/* in doubt return not capable */
-		return 1;
-	}
+	if (!dm_tgt_version(dm_mpath_target_version, TGT_MPATH))
+		condlog(3, "DM multipath kernel driver v%u.%u.%u",
+			dm_mpath_target_version[0],
+			dm_mpath_target_version[1],
+			dm_mpath_target_version[2]);
+}
 
-	/* test request based multipath capability */
-	condlog(3, "DM multipath kernel driver v%u.%u.%u",
-		v[0], v[1], v[2]);
+static int dm_tgt_prereq (unsigned int *ver)
+{
+	unsigned int minv[3] = {1, 0, 3};
 
-	if (VERSION_GE(v, minv)) {
-		ver[0] = v[0];
-		ver[1] = v[1];
-		ver[2] = v[2];
+	if (VERSION_GE(dm_mpath_target_version, minv)) {
+		if (ver) {
+			ver[0] = dm_mpath_target_version[0];
+			ver[1] = dm_mpath_target_version[1];
+			ver[2] = dm_mpath_target_version[2];
+		}
 		return 0;
 	}
 
@@ -241,13 +255,60 @@ dm_tgt_prereq (unsigned int *ver)
 	return 1;
 }
 
+static void _init_versions(void)
+{
+	dlog(logsink, 3, VERSION_STRING);
+	init_dm_library_version();
+	init_dm_drv_version();
+	init_dm_mpath_version();
+}
+
+static int init_versions(void) {
+	pthread_once(&versions_initialized, _init_versions);
+	return (dm_library_version[0] == INVALID_VERSION ||
+		dm_kernel_version[0] == INVALID_VERSION ||
+		dm_mpath_target_version[0] == INVALID_VERSION);
+}
+
 int dm_prereq(unsigned int *v)
 {
+	if (init_versions())
+		return 1;
 	if (dm_lib_prereq())
 		return 1;
 	return dm_tgt_prereq(v);
 }
 
+int libmp_get_version(int which, unsigned int version[3])
+{
+	unsigned int *src_version;
+
+	init_versions();
+	switch (which) {
+	case DM_LIBRARY_VERSION:
+		src_version = dm_library_version;
+		break;
+	case DM_KERNEL_VERSION:
+		src_version = dm_kernel_version;
+		break;
+	case DM_MPATH_TARGET_VERSION:
+		src_version = dm_mpath_target_version;
+		break;
+	case MULTIPATH_VERSION:
+		version[0] = (VERSION_CODE >> 16) & 0xff;
+		version[1] = (VERSION_CODE >> 8) & 0xff;
+		version[2] = VERSION_CODE & 0xff;
+		return 0;
+	default:
+		condlog(0, "%s: invalid value for 'which'", __func__);
+		return 1;
+	}
+	if (src_version[0] == INVALID_VERSION)
+		return 1;
+	memcpy(version, src_version, 3 * sizeof(*version));
+	return 0;
+}
+
 static int libmp_dm_udev_sync = 0;
 
 void libmp_udev_set_sync_support(int on)
@@ -265,7 +326,6 @@ static void libmp_dm_init(void)
 		exit(1);
 	conf = get_multipath_config();
 	verbosity = conf->verbosity;
-	memcpy(conf->version, version, sizeof(version));
 	put_multipath_config(conf);
 	dm_init(verbosity);
 #ifdef LIBDM_API_HOLD_CONTROL
diff --git a/libmultipath/devmapper.h b/libmultipath/devmapper.h
index f469c98a..a0bcd137 100644
--- a/libmultipath/devmapper.h
+++ b/libmultipath/devmapper.h
@@ -33,13 +33,10 @@ enum {
 	DMP_NOT_FOUND,
 };
 
-void dm_init(int verbosity);
 int dm_prereq(unsigned int *v);
 void skip_libmp_dm_init(void);
 void libmp_udev_set_sync_support(int on);
 struct dm_task *libmp_dm_task_create(int task);
-int dm_drv_version (unsigned int * version);
-int dm_tgt_version (unsigned int * version, char * str);
 int dm_simplecmd_flush (int, const char *, uint16_t);
 int dm_simplecmd_noflush (int, const char *, uint16_t);
 int dm_addmap_create (struct multipath *mpp, char *params);
@@ -89,6 +86,13 @@ struct multipath *dm_get_multipath(const char *name);
 #include <errno.h>
 #define dm_task_get_errno(x) errno
 #endif
+enum {
+	DM_LIBRARY_VERSION,
+	DM_KERNEL_VERSION,
+	DM_MPATH_TARGET_VERSION,
+	MULTIPATH_VERSION
+};
+int libmp_get_version(int which, unsigned int version[3]);
 
 #define dm_log_error(lvl, cmd, dmt)			      \
 	condlog(lvl, "%s: libdm task=%d error: %s", __func__, \
diff --git a/libmultipath/libmultipath.version b/libmultipath/libmultipath.version
index a6bf8218..ab5bb0ad 100644
--- a/libmultipath/libmultipath.version
+++ b/libmultipath/libmultipath.version
@@ -31,7 +31,7 @@
  *   The new version inherits the previous ones.
  */
 
-LIBMULTIPATH_1.0.0 {
+LIBMULTIPATH_2.0.0 {
 global:
 	/* symbols referenced by multipath and multipathd */
 	add_foreign;
@@ -65,7 +65,6 @@ global:
 	disassemble_status;
 	dlog;
 	dm_cancel_deferred_remove;
-	dm_drv_version;
 	dm_enablegroup;
 	dm_fail_path;
 	_dm_flush_map;
@@ -87,7 +86,6 @@ global:
 	dm_reinstate_path;
 	dm_simplecmd_noflush;
 	dm_switchgroup;
-	dm_tgt_version;
 	domap;
 	ensure_directories_exist;
 	extract_hwe_from_path;
@@ -128,6 +126,7 @@ global:
 	is_path_valid;
 	is_quote;
 	libmp_dm_task_create;
+	libmp_get_version;
 	libmp_udev_set_sync_support;
 	load_config;
 	log_thread_reset;
diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
index 40201344..3f2c2cfa 100644
--- a/libmultipath/propsel.c
+++ b/libmultipath/propsel.c
@@ -735,9 +735,10 @@ out:
 
 int select_minio(struct config *conf, struct multipath *mp)
 {
-	unsigned int minv_dmrq[3] = {1, 1, 0};
+	unsigned int minv_dmrq[3] = {1, 1, 0}, version[3];
 
-	if (VERSION_GE(conf->version, minv_dmrq))
+	if (!libmp_get_version(DM_MPATH_TARGET_VERSION, version)
+	    && VERSION_GE(version, minv_dmrq))
 		return select_minio_rq(conf, mp);
 	else
 		return select_minio_bio(conf, mp);
@@ -820,9 +821,10 @@ out:
 int select_retain_hwhandler(struct config *conf, struct multipath *mp)
 {
 	const char *origin;
-	unsigned int minv_dm_retain[3] = {1, 5, 0};
+	unsigned int minv_dm_retain[3] = {1, 5, 0}, version[3];
 
-	if (!VERSION_GE(conf->version, minv_dm_retain)) {
+	if (!libmp_get_version(DM_MPATH_TARGET_VERSION, version) &&
+	    !VERSION_GE(version, minv_dm_retain)) {
 		mp->retain_hwhandler = RETAIN_HWHANDLER_OFF;
 		origin = "(setting: WARNING, requires kernel dm-mpath version >= 1.5.0)";
 		goto out;
diff --git a/multipathd/dmevents.c b/multipathd/dmevents.c
index 5f2d210d..fc97c8a2 100644
--- a/multipathd/dmevents.c
+++ b/multipathd/dmevents.c
@@ -60,7 +60,7 @@ int dmevent_poll_supported(void)
 {
 	unsigned int v[3];
 
-	if (dm_drv_version(v))
+	if (libmp_get_version(DM_KERNEL_VERSION, v))
 		return 0;
 
 	if (VERSION_GE(v, DM_VERSION_FOR_ARM_POLL))
diff --git a/multipathd/main.c b/multipathd/main.c
index 5cc34357..00b66ba4 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2709,7 +2709,6 @@ reconfigure (struct vectors * vecs)
 	/* Re-read any timezone changes */
 	tzset();
 
-	dm_tgt_version(conf->version, TGT_MPATH);
 	if (verbosity)
 		conf->verbosity = verbosity;
 	if (bindings_read_only)
diff --git a/tests/Makefile b/tests/Makefile
index 9658c9fd..78777bec 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -43,7 +43,7 @@ endif
 dmevents-test_LIBDEPS = -lpthread -ldevmapper -lurcu
 hwtable-test_TESTDEPS := test-lib.o
 hwtable-test_OBJDEPS := ../libmultipath/discovery.o ../libmultipath/blacklist.o \
-	../libmultipath/structs.o
+	../libmultipath/structs.o ../libmultipath/propsel.o
 hwtable-test_LIBDEPS := -ludev -lpthread -ldl
 blacklist-test_TESTDEPS := test-log.o
 blacklist-test_OBJDEPS := ../libmultipath/blacklist.o
diff --git a/tests/hwtable.c b/tests/hwtable.c
index 12660da2..57f832b7 100644
--- a/tests/hwtable.c
+++ b/tests/hwtable.c
@@ -30,8 +30,6 @@
 #define N_CONF_FILES 2
 
 static const char tmplate[] = "/tmp/hwtable-XXXXXX";
-/* pretend new dm, use minio_rq */
-static const unsigned int dm_tgt_version[3] = { 1, 1, 1 };
 
 struct key_value {
 	const char *key;
@@ -360,7 +358,6 @@ static void write_device(FILE *ff, int nkv, const struct key_value *kv)
 	assert_ptr_not_equal(__cf, NULL);				\
 	assert_ptr_not_equal(__cf->hwtable, NULL);			\
 	__cf->verbosity = VERBOSITY;					\
-	memcpy(&__cf->version, dm_tgt_version, sizeof(__cf->version));	\
 	__cf; })
 
 #define FREE_CONFIG(conf) do {			\
diff --git a/tests/test-lib.c b/tests/test-lib.c
index b7c09cc2..e7663f9a 100644
--- a/tests/test-lib.c
+++ b/tests/test-lib.c
@@ -56,6 +56,15 @@ int __wrap_execute_program(char *path, char *value, int len)
 	return 0;
 }
 
+int __wrap_libmp_get_version(int which, unsigned int version[3])
+{
+	unsigned int *vers = mock_ptr_type(unsigned int *);
+
+	condlog(4, "%s: %d", __func__, which);
+	memcpy(version, vers, 3 * sizeof(unsigned int));
+	return 0;
+}
+
 struct udev_list_entry
 *__wrap_udev_device_get_properties_list_entry(struct udev_device *ud)
 {
@@ -339,6 +348,8 @@ struct multipath *__mock_multipath(struct vectors *vecs, struct path *pp)
 	struct multipath *mp;
 	struct config *conf;
 	struct mocked_path mop;
+	/* pretend new dm, use minio_rq,  */
+	static const unsigned int fake_dm_tgt_version[3] = { 1, 1, 1 };
 
 	mocked_path_from_path(&mop, pp);
 	/* pathinfo() call in adopt_paths */
@@ -351,7 +362,9 @@ struct multipath *__mock_multipath(struct vectors *vecs, struct path *pp)
 	conf = get_multipath_config();
 	select_pgpolicy(conf, mp);
 	select_no_path_retry(conf, mp);
+	will_return(__wrap_libmp_get_version, fake_dm_tgt_version);
 	select_retain_hwhandler(conf, mp);
+	will_return(__wrap_libmp_get_version, fake_dm_tgt_version);
 	select_minio(conf, mp);
 	put_multipath_config(conf);
 
-- 
2.17.2