dm-devel.redhat.com archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 00/21] multipath-tools: shutdown, libdevmapper races, globals
@ 2020-09-24 13:36 mwilck
  2020-09-24 13:36 ` [PATCH v2 01/21] multipathd: allow shutdown during configure() mwilck
                   ` (20 more replies)
  0 siblings, 21 replies; 35+ messages in thread
From: mwilck @ 2020-09-24 13:36 UTC (permalink / raw)
  To: Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel, Martin Wilck

From: Martin Wilck <mwilck@suse.com>

Hi Christophe, hi Ben,

this series contains a number of patches I've wanted to do for some
time. It's based on my "upstream-queue" tree
(https://github.com/openSUSE/multipath-tools/tree/upstream-queue),
plus my previously submitted "multipath-tools:add linker version scripts"
series. The reason is that this series contains a number of ABI
changes, and is thus suitable for demonstrating how to work with
the libmultipath linker version scripts.

Patch 1-8 are related to multipathd shutdown and systemd notifcation.
Patch 1 and 8 have been part of my earlier series "multipath-tools: 
Fix remaining shutdown delay issues" from Jan, 2019. But this is so
long ago that I didn't mark this as a v2. I have tried to address
Ben's issues with #1 (size_mismatch_seen leak, and premature sd_notify)
(https://www.redhat.com/archives/dm-devel/2019-January/msg00097.html).
#8 is just resent, after the recent discussion
(https://www.redhat.com/archives/dm-devel/2020-August/msg00342.html).
Ben's remark about sd_notify() drove me to reexamine that feature,
and actually improve a little by informing systemd of shutdown and
reconfigure operations.

Patch 9-11 are an attempt to fix races in libdevmapper, as discussed
a while ago in the "fix fd leak when iscsi device logs in" thread
(https://www.redhat.com/archives/dm-devel/2020-July/msg00321.html and
references).

Patch 12ff. add definitions of the symbol get_multipath_config(),
put_multipath_config(), udev, and logsink to libmultipath. This way
callers won't have to bother with defining these global symbols any
more in the future (but they still can).

Changes v1 -> v2:

 - rebased on "version script" series. Some patches which modify the
   ABI (10, 11, 13, 14, 17) have the respective hunks added.
 - 10/21: Added fix for the hwtable unit test. added comments.
   avoid logging a version that couldn't be determined.
 - 11/21: renamed libmp_task_run -> libmp_dm_task_run
 - 13/21: get_multipath_config(): return NULL if not initialized
   zero out configuration in _uninit_config() (Ben)
 - 17/21: Changed initialization as discussed with Ben. Added documentation
   in config.h.
 - 19/21: This one is new, as suggested by Ben
 - 20/21: This was 19 before, simplified now as the real changes are
   in libmpathpersist.
 - 21/21: New, also remove the globals in multipathd.

Regards
Martin

Martin Wilck (21):
  multipathd: allow shutdown during configure()
  multipathd: avoid sending "READY=1" to systemd on early shutdown
  multipathd: send "STOPPING=1" to systemd on shutdown
  multipathd: send "RELOADING=1" to systemd on DAEMON_CONFIGURE state
  multipathd: use volatile qualifier for running_state
  multipathd: generalize and fix wait_for_state_change_if()
  multipathd: set_config_state(): avoid code duplication
  multipathd: cancel threads early during shutdown
  multipath-tools: don't call dm_lib_release() any more
  libmultipath: devmapper: refactor libdm version determination
  libmultipath: protect racy libdevmapper calls with a mutex
  libmultipath: constify file argument in config parser
  libmultipath: provide defaults for {get,put}_multipath_config
  libmpathpersist: allow using libmultipath {get,put}_multipath_config
  multipath: use {get_put}_multipath_config from libmultipath
  mpathpersist: use {get,put}_multipath_config() from libmultipath
  libmultipath: add udev and logsink symbols
  multipath: remove logsink and udev
  libmpathpersist: call libmultipath_{init,exit}()
  mpathpersist: remove logsink and udev
  multipathd: remove logsink and udev

 kpartx/kpartx.c                         |   1 -
 libmpathpersist/libmpathpersist.version |   6 +
 libmpathpersist/mpath_persist.c         |  50 ++++-
 libmpathpersist/mpath_persist.h         |  31 ++++
 libmultipath/config.c                   | 124 +++++++++++--
 libmultipath/config.h                   |  70 ++++++-
 libmultipath/configure.c                |   6 +
 libmultipath/debug.c                    |   2 +
 libmultipath/devmapper.c                | 233 ++++++++++++++++--------
 libmultipath/devmapper.h                |  13 +-
 libmultipath/discovery.c                |   3 +
 libmultipath/libmultipath.version       |  29 ++-
 libmultipath/parser.c                   |   9 +-
 libmultipath/parser.h                   |   2 +-
 libmultipath/propsel.c                  |  10 +-
 libmultipath/util.c                     |  10 +
 libmultipath/util.h                     |   2 +
 mpathpersist/main.c                     |  22 +--
 multipath/main.c                        |  28 +--
 multipathd/cli_handlers.c               |   2 -
 multipathd/dmevents.c                   |   4 +-
 multipathd/main.c                       | 126 ++++++-------
 multipathd/waiter.c                     |   2 +-
 tests/Makefile                          |   2 +-
 tests/hwtable.c                         |   3 -
 tests/test-lib.c                        |  13 ++
 26 files changed, 571 insertions(+), 232 deletions(-)

-- 
2.28.0

^ permalink raw reply	[flat|nested] 35+ messages in thread

* [PATCH v2 01/21] multipathd: allow shutdown during configure()
  2020-09-24 13:36 [PATCH v2 00/21] multipath-tools: shutdown, libdevmapper races, globals mwilck
@ 2020-09-24 13:36 ` mwilck
  2020-09-24 13:36 ` [PATCH v2 02/21] multipathd: avoid sending "READY=1" to systemd on early shutdown mwilck
                   ` (19 subsequent siblings)
  20 siblings, 0 replies; 35+ messages in thread
From: mwilck @ 2020-09-24 13:36 UTC (permalink / raw)
  To: Christophe Varoqui, Benjamin Marzinski
  Cc: Chongyun Wu, dm-devel, Martin Wilck

From: Martin Wilck <mwilck@suse.com>

reconfigure() can be a long-running operation; both initial path
discovery and initial map setup can take a long time. Allow
the main program to indicate that the process should be
interrupted if a shutdown signal was received.

We take advantage of the dynamic linker's symbol lookup ordering
here. The default implementation of should_exit never returns
true, but callers (like multipathd) can override it.

Cc: Chongyun Wu <wu.chongyun@h3c.com>
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/configure.c |  6 ++++++
 libmultipath/discovery.c |  3 +++
 libmultipath/util.c      |  5 +++++
 libmultipath/util.h      |  1 +
 multipathd/main.c        | 17 +++++++++++++++++
 5 files changed, 32 insertions(+)

diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index d7afc91..1c8aac0 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -1173,6 +1173,12 @@ int coalesce_paths (struct vectors * vecs, vector newmp, char * refwwid,
 
 	vector_foreach_slot (pathvec, pp1, k) {
 		int invalid;
+
+		if (should_exit()) {
+			ret = CP_FAIL;
+			goto out;
+		}
+
 		/* skip this path for some reason */
 
 		/* 1. if path has no unique id or wwid blacklisted */
diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index 2f301ac..8519ff1 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -200,6 +200,9 @@ path_discovery (vector pathvec, int flag)
 		const char *devtype;
 		const char *devpath;
 
+		if (should_exit())
+			break;
+
 		devpath = udev_list_entry_get_name(entry);
 		condlog(4, "Discover device %s", devpath);
 		udevice = udev_device_new_from_syspath(udev, devpath);
diff --git a/libmultipath/util.c b/libmultipath/util.c
index 1dad90f..66cd721 100644
--- a/libmultipath/util.c
+++ b/libmultipath/util.c
@@ -430,3 +430,8 @@ void _log_bitfield_overflow(const char *f, unsigned int bit, unsigned int len)
 {
 	condlog(0, "%s: bitfield overflow: %u >= %u", f, bit, len);
 }
+
+int should_exit(void)
+{
+	return 0;
+}
diff --git a/libmultipath/util.h b/libmultipath/util.h
index 0f0f6cb..c1ae878 100644
--- a/libmultipath/util.h
+++ b/libmultipath/util.h
@@ -27,6 +27,7 @@ int parse_prkey(const char *ptr, uint64_t *prkey);
 int parse_prkey_flags(const char *ptr, uint64_t *prkey, uint8_t *flags);
 int safe_write(int fd, const void *buf, size_t count);
 void set_max_fds(rlim_t max_fds);
+int should_exit(void);
 
 #define KERNEL_VERSION(maj, min, ptc) ((((maj) * 256) + (min)) * 256 + (ptc))
 #define ARRAY_SIZE(x) (sizeof(x)/sizeof((x)[0]))
diff --git a/multipathd/main.c b/multipathd/main.c
index eedc6c1..fa53e96 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -141,6 +141,11 @@ static inline enum daemon_status get_running_state(void)
 	return st;
 }
 
+int should_exit(void)
+{
+	return get_running_state() == DAEMON_SHUTDOWN;
+}
+
 /*
  * global copy of vecs for use in sig handlers
  */
@@ -2570,6 +2575,9 @@ configure (struct vectors * vecs)
 		goto fail;
 	}
 
+	if (should_exit())
+		goto fail;
+
 	conf = get_multipath_config();
 	pthread_cleanup_push(put_multipath_config, conf);
 	vector_foreach_slot (vecs->pathvec, pp, i){
@@ -2586,6 +2594,9 @@ configure (struct vectors * vecs)
 		goto fail;
 	}
 
+	if (should_exit())
+		goto fail;
+
 	/*
 	 * create new set of maps & push changed ones into dm
 	 * In the first call, use FORCE_RELOAD_WEAK to avoid making
@@ -2600,6 +2611,9 @@ configure (struct vectors * vecs)
 		goto fail;
 	}
 
+	if (should_exit())
+		goto fail;
+
 	/*
 	 * may need to remove some maps which are no longer relevant
 	 * e.g., due to blacklist changes in conf file
@@ -2611,6 +2625,9 @@ configure (struct vectors * vecs)
 
 	dm_lib_release();
 
+	if (should_exit())
+		goto fail;
+
 	sync_maps_state(mpvec);
 	vector_foreach_slot(mpvec, mpp, i){
 		if (remember_wwid(mpp->wwid) == 1)
-- 
2.28.0

^ permalink raw reply related	[flat|nested] 35+ messages in thread

* [PATCH v2 02/21] multipathd: avoid sending "READY=1" to systemd on early shutdown
  2020-09-24 13:36 [PATCH v2 00/21] multipath-tools: shutdown, libdevmapper races, globals mwilck
  2020-09-24 13:36 ` [PATCH v2 01/21] multipathd: allow shutdown during configure() mwilck
@ 2020-09-24 13:36 ` mwilck
  2020-09-24 13:36 ` [PATCH v2 03/21] multipathd: send "STOPPING=1" to systemd on shutdown mwilck
                   ` (18 subsequent siblings)
  20 siblings, 0 replies; 35+ messages in thread
From: mwilck @ 2020-09-24 13:36 UTC (permalink / raw)
  To: Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel, Martin Wilck

From: Martin Wilck <mwilck@suse.com>

If multipathd gets a shutdown request during initial reconfigure(),
it shouldn't send "READY=1" to systemd. Ensure this by sending
"READY=1" via post_config_state().

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipathd/main.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index fa53e96..53a22a4 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -189,6 +189,8 @@ static void do_sd_notify(enum daemon_status old_state,
 {
 	char notify_msg[MSG_SIZE];
 	const char *msg;
+	static bool startup_done = false;
+
 	/*
 	 * Checkerloop switches back and forth between idle and running state.
 	 * No need to tell systemd each time.
@@ -205,6 +207,11 @@ static void do_sd_notify(enum daemon_status old_state,
 
 	if (msg && !safe_sprintf(notify_msg, "STATUS=%s", msg))
 		sd_notify(0, notify_msg);
+
+	if (new_state == DAEMON_IDLE && !startup_done) {
+		sd_notify(0, "READY=1");
+		startup_done = true;
+	}
 }
 #endif
 
@@ -2903,9 +2910,6 @@ child (__attribute__((unused)) void *param)
 	struct vectors * vecs;
 	struct multipath * mpp;
 	int i;
-#ifdef USE_SYSTEMD
-	int startup_done = 0;
-#endif
 	int rc;
 	int pid_fd = -1;
 	struct config *conf;
@@ -3065,12 +3069,6 @@ child (__attribute__((unused)) void *param)
 			}
 			lock_cleanup_pop(vecs->lock);
 			post_config_state(DAEMON_IDLE);
-#ifdef USE_SYSTEMD
-			if (!startup_done) {
-				sd_notify(0, "READY=1");
-				startup_done = 1;
-			}
-#endif
 		}
 	}
 
-- 
2.28.0

^ permalink raw reply related	[flat|nested] 35+ messages in thread

* [PATCH v2 03/21] multipathd: send "STOPPING=1" to systemd on shutdown
  2020-09-24 13:36 [PATCH v2 00/21] multipath-tools: shutdown, libdevmapper races, globals mwilck
  2020-09-24 13:36 ` [PATCH v2 01/21] multipathd: allow shutdown during configure() mwilck
  2020-09-24 13:36 ` [PATCH v2 02/21] multipathd: avoid sending "READY=1" to systemd on early shutdown mwilck
@ 2020-09-24 13:36 ` mwilck
  2020-09-24 13:36 ` [PATCH v2 04/21] multipathd: send "RELOADING=1" to systemd on DAEMON_CONFIGURE state mwilck
                   ` (17 subsequent siblings)
  20 siblings, 0 replies; 35+ messages in thread
From: mwilck @ 2020-09-24 13:36 UTC (permalink / raw)
  To: Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel, Martin Wilck

From: Martin Wilck <mwilck@suse.com>

Inform systemd that the daemon is shutting down. See sd_notify(3).

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipathd/main.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index 53a22a4..c264351 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -208,7 +208,9 @@ static void do_sd_notify(enum daemon_status old_state,
 	if (msg && !safe_sprintf(notify_msg, "STATUS=%s", msg))
 		sd_notify(0, notify_msg);
 
-	if (new_state == DAEMON_IDLE && !startup_done) {
+	if (new_state == DAEMON_SHUTDOWN)
+		sd_notify(0, "STOPPING=1");
+	else if (new_state == DAEMON_IDLE && !startup_done) {
 		sd_notify(0, "READY=1");
 		startup_done = true;
 	}
-- 
2.28.0

^ permalink raw reply related	[flat|nested] 35+ messages in thread

* [PATCH v2 04/21] multipathd: send "RELOADING=1" to systemd on DAEMON_CONFIGURE state
  2020-09-24 13:36 [PATCH v2 00/21] multipath-tools: shutdown, libdevmapper races, globals mwilck
                   ` (2 preceding siblings ...)
  2020-09-24 13:36 ` [PATCH v2 03/21] multipathd: send "STOPPING=1" to systemd on shutdown mwilck
@ 2020-09-24 13:36 ` mwilck
  2020-09-24 13:37 ` [PATCH v2 05/21] multipathd: use volatile qualifier for running_state mwilck
                   ` (16 subsequent siblings)
  20 siblings, 0 replies; 35+ messages in thread
From: mwilck @ 2020-09-24 13:36 UTC (permalink / raw)
  To: Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel, Martin Wilck

From: Martin Wilck <mwilck@suse.com>

The logic is as follows: child() sets DAEMON_IDLE status after
DAEMON_CONFIGURE when reconfigure() has finished. The only other state change
that can race with that is DAEMON_SHUTDOWN. Other state changes will wait for
DAEMON_IDLE first (see set_config_state()). When DAEMON_CONFIGURE is entered,
and we are not just starting up, send a "RELOADING=1" message to
systemd. After that, we must send "READY=1" when we're done reloading. Also
do that on startup, when DAEMON_IDLE is set for the first time.
See sd_notify(3).

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipathd/main.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index c264351..e3f2328 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -210,10 +210,11 @@ static void do_sd_notify(enum daemon_status old_state,
 
 	if (new_state == DAEMON_SHUTDOWN)
 		sd_notify(0, "STOPPING=1");
-	else if (new_state == DAEMON_IDLE && !startup_done) {
+	else if (new_state == DAEMON_IDLE && old_state == DAEMON_CONFIGURE) {
 		sd_notify(0, "READY=1");
 		startup_done = true;
-	}
+	} else if (new_state == DAEMON_CONFIGURE && startup_done)
+		sd_notify(0, "RELOADING=1");
 }
 #endif
 
-- 
2.28.0

^ permalink raw reply related	[flat|nested] 35+ messages in thread

* [PATCH v2 05/21] multipathd: use volatile qualifier for running_state
  2020-09-24 13:36 [PATCH v2 00/21] multipath-tools: shutdown, libdevmapper races, globals mwilck
                   ` (3 preceding siblings ...)
  2020-09-24 13:36 ` [PATCH v2 04/21] multipathd: send "RELOADING=1" to systemd on DAEMON_CONFIGURE state mwilck
@ 2020-09-24 13:37 ` mwilck
  2020-09-24 13:37 ` [PATCH v2 06/21] multipathd: generalize and fix wait_for_state_change_if() mwilck
                   ` (15 subsequent siblings)
  20 siblings, 0 replies; 35+ messages in thread
From: mwilck @ 2020-09-24 13:37 UTC (permalink / raw)
  To: Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel, Martin Wilck

From: Martin Wilck <mwilck@suse.com>

While we access running_state only under the config_lock,
we sometimes do in a loop. Use the volatile qualifier to make
sure compilers can't optimize away the loads.

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipathd/main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index e3f2328..d081b3e 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -126,7 +126,7 @@ int poll_dmevents = 0;
 int poll_dmevents = 1;
 #endif
 /* Don't access this variable without holding config_lock */
-enum daemon_status running_state = DAEMON_INIT;
+volatile enum daemon_status running_state = DAEMON_INIT;
 pid_t daemon_pid;
 pthread_mutex_t config_lock = PTHREAD_MUTEX_INITIALIZER;
 pthread_cond_t config_cond;
-- 
2.28.0

^ permalink raw reply related	[flat|nested] 35+ messages in thread

* [PATCH v2 06/21] multipathd: generalize and fix wait_for_state_change_if()
  2020-09-24 13:36 [PATCH v2 00/21] multipath-tools: shutdown, libdevmapper races, globals mwilck
                   ` (4 preceding siblings ...)
  2020-09-24 13:37 ` [PATCH v2 05/21] multipathd: use volatile qualifier for running_state mwilck
@ 2020-09-24 13:37 ` mwilck
  2020-09-24 13:37 ` [PATCH v2 07/21] multipathd: set_config_state(): avoid code duplication mwilck
                   ` (14 subsequent siblings)
  20 siblings, 0 replies; 35+ messages in thread
From: mwilck @ 2020-09-24 13:37 UTC (permalink / raw)
  To: Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel, Martin Wilck

From: Martin Wilck <mwilck@suse.com>

It's unlikely but not impossible that other threads change the state
while we're waiting, and if we grab the lock again, it's still not
what we wanted. We need to continue waiting until either the condition
is met, or time timeout expired.

Moreover, generalize this code so that it can also be used in
set_config_state().

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipathd/main.c | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index d081b3e..1fb0ee6 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -223,6 +223,23 @@ static void config_cleanup(__attribute__((unused)) void *arg)
 	pthread_mutex_unlock(&config_lock);
 }
 
+#define __wait_for_state_change(condition, ms)				\
+	({								\
+		struct timespec tmo;					\
+		int rc = 0;						\
+									\
+		if (condition) {					\
+			get_monotonic_time(&tmo);			\
+			tmo.tv_nsec += (ms) * 1000 * 1000;		\
+			normalize_timespec(&tmo);			\
+			do						\
+				rc = pthread_cond_timedwait(		\
+					&config_cond, &config_lock, &tmo); \
+			while (rc == 0 && (condition));			\
+		}							\
+		rc;							\
+	})
+
 /*
  * If the current status is @oldstate, wait for at most @ms milliseconds
  * for the state to change, and return the new state, which may still be
@@ -232,20 +249,14 @@ enum daemon_status wait_for_state_change_if(enum daemon_status oldstate,
 					    unsigned long ms)
 {
 	enum daemon_status st;
-	struct timespec tmo;
 
 	if (oldstate == DAEMON_SHUTDOWN)
 		return DAEMON_SHUTDOWN;
 
 	pthread_mutex_lock(&config_lock);
 	pthread_cleanup_push(config_cleanup, NULL);
+	__wait_for_state_change(running_state == oldstate, ms);
 	st = running_state;
-	if (st == oldstate && clock_gettime(CLOCK_MONOTONIC, &tmo) == 0) {
-		tmo.tv_nsec += ms * 1000 * 1000;
-		normalize_timespec(&tmo);
-		(void)pthread_cond_timedwait(&config_cond, &config_lock, &tmo);
-		st = running_state;
-	}
 	pthread_cleanup_pop(1);
 	return st;
 }
-- 
2.28.0

^ permalink raw reply related	[flat|nested] 35+ messages in thread

* [PATCH v2 07/21] multipathd: set_config_state(): avoid code duplication
  2020-09-24 13:36 [PATCH v2 00/21] multipath-tools: shutdown, libdevmapper races, globals mwilck
                   ` (5 preceding siblings ...)
  2020-09-24 13:37 ` [PATCH v2 06/21] multipathd: generalize and fix wait_for_state_change_if() mwilck
@ 2020-09-24 13:37 ` mwilck
  2020-09-24 13:37 ` [PATCH v2 08/21] multipathd: cancel threads early during shutdown mwilck
                   ` (13 subsequent siblings)
  20 siblings, 0 replies; 35+ messages in thread
From: mwilck @ 2020-09-24 13:37 UTC (permalink / raw)
  To: Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel, Martin Wilck

From: Martin Wilck <mwilck@suse.com>

Use __post_config_state() and __wait_for_state_change(). This
way __post_config_state() is the only place where running_state
is ever changed, and we avoid code duplication.

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipathd/main.c | 23 +++++------------------
 1 file changed, 5 insertions(+), 18 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index 1fb0ee6..39aea4a 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -292,27 +292,14 @@ int set_config_state(enum daemon_status state)
 	pthread_cleanup_push(config_cleanup, NULL);
 	pthread_mutex_lock(&config_lock);
 	if (running_state != state) {
-#ifdef USE_SYSTEMD
-		enum daemon_status old_state = running_state;
-#endif
 
 		if (running_state == DAEMON_SHUTDOWN)
 			rc = EINVAL;
-		else if (running_state != DAEMON_IDLE) {
-			struct timespec ts;
-
-			get_monotonic_time(&ts);
-			ts.tv_sec += 1;
-			rc = pthread_cond_timedwait(&config_cond,
-						    &config_lock, &ts);
-		}
-		if (!rc && (running_state != DAEMON_SHUTDOWN)) {
-			running_state = state;
-			pthread_cond_broadcast(&config_cond);
-#ifdef USE_SYSTEMD
-			do_sd_notify(old_state, state);
-#endif
-		}
+		else
+			rc = __wait_for_state_change(
+				running_state != DAEMON_IDLE, 1000);
+		if (!rc)
+			__post_config_state(state);
 	}
 	pthread_cleanup_pop(1);
 	return rc;
-- 
2.28.0

^ permalink raw reply related	[flat|nested] 35+ messages in thread

* [PATCH v2 08/21] multipathd: cancel threads early during shutdown
  2020-09-24 13:36 [PATCH v2 00/21] multipath-tools: shutdown, libdevmapper races, globals mwilck
                   ` (6 preceding siblings ...)
  2020-09-24 13:37 ` [PATCH v2 07/21] multipathd: set_config_state(): avoid code duplication mwilck
@ 2020-09-24 13:37 ` mwilck
  2020-09-24 13:37 ` [PATCH v2 09/21] multipath-tools: don't call dm_lib_release() any more mwilck
                   ` (12 subsequent siblings)
  20 siblings, 0 replies; 35+ messages in thread
From: mwilck @ 2020-09-24 13:37 UTC (permalink / raw)
  To: Christophe Varoqui, Benjamin Marzinski
  Cc: Chongyun Wu, dm-devel, Martin Wilck

From: Martin Wilck <mwilck@suse.com>

Cancel the other threads before taking vecs->lock. This avoids
delays during shutdown caused e.g. by the checker thread holding
the vecs lock.

Note: this makes it possible that cancelled threads leak memory,
because they can now be cancelled before having released the vecs
lock. I believe this is acceptable, as only threads are affected
that are cancelled during multipathd shutdown.

Cc: Chongyun Wu <wu.chongyun@h3c.com>
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipathd/main.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index 39aea4a..d1f8cc1 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -3073,23 +3073,24 @@ child (__attribute__((unused)) void *param)
 		}
 	}
 
-	lock(&vecs->lock);
+	pthread_cancel(check_thr);
+	pthread_cancel(uevent_thr);
+	pthread_cancel(uxlsnr_thr);
+	pthread_cancel(uevq_thr);
+	if (poll_dmevents)
+		pthread_cancel(dmevent_thr);
+
 	conf = get_multipath_config();
 	queue_without_daemon = conf->queue_without_daemon;
 	put_multipath_config(conf);
+
+	lock(&vecs->lock);
 	if (queue_without_daemon == QUE_NO_DAEMON_OFF)
 		vector_foreach_slot(vecs->mpvec, mpp, i)
 			dm_queue_if_no_path(mpp->alias, 0);
 	remove_maps_and_stop_waiters(vecs);
 	unlock(&vecs->lock);
 
-	pthread_cancel(check_thr);
-	pthread_cancel(uevent_thr);
-	pthread_cancel(uxlsnr_thr);
-	pthread_cancel(uevq_thr);
-	if (poll_dmevents)
-		pthread_cancel(dmevent_thr);
-
 	pthread_join(check_thr, NULL);
 	pthread_join(uevent_thr, NULL);
 	pthread_join(uxlsnr_thr, NULL);
-- 
2.28.0

^ permalink raw reply related	[flat|nested] 35+ messages in thread

* [PATCH v2 09/21] multipath-tools: don't call dm_lib_release() any more
  2020-09-24 13:36 [PATCH v2 00/21] multipath-tools: shutdown, libdevmapper races, globals mwilck
                   ` (7 preceding siblings ...)
  2020-09-24 13:37 ` [PATCH v2 08/21] multipathd: cancel threads early during shutdown mwilck
@ 2020-09-24 13:37 ` mwilck
  2020-09-24 13:37 ` [PATCH v2 10/21] libmultipath: devmapper: refactor libdm version determination mwilck
                   ` (11 subsequent siblings)
  20 siblings, 0 replies; 35+ messages in thread
From: mwilck @ 2020-09-24 13:37 UTC (permalink / raw)
  To: Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel, Martin Wilck

From: Martin Wilck <mwilck@suse.com>

The purpose of dm_lib_release() is to release stacked device node
operations in libdevmapper. This is functionality we don't need and
use any more, as we rely on udev to set up device nodes and symlinks.

We always set DM_UDEV_DISABLE_LIBRARY_FALLBACK when we run dm tasks.
In the standard CREATE and REMOVE cases, libdevmapper doesn't
stack any operations if this flag is set. The only exceptions are

 a) RESUME operations with DM_ADD_NODE_ON_RESUME set. This happens
implicity when we create new maps
 b) RENAME operations

In both cases, we call dm_udev_wait() after the libdm operation, which
calls update_devs() and thus has the same effect as dm_lib_release(),
cleaning out stacked operations.

OTOH, dm_lib_releases() accesses static variables in libdevmapper, so
calling it might be racy.

Drop the calls to dm_lib_release().

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 kpartx/kpartx.c                 |  1 -
 libmpathpersist/mpath_persist.c |  1 -
 multipath/main.c                |  1 -
 multipathd/cli_handlers.c       |  2 --
 multipathd/main.c               | 15 ++-------------
 5 files changed, 2 insertions(+), 18 deletions(-)

diff --git a/kpartx/kpartx.c b/kpartx/kpartx.c
index 4a0aae9..6a7933f 100644
--- a/kpartx/kpartx.c
+++ b/kpartx/kpartx.c
@@ -681,7 +681,6 @@ main(int argc, char **argv){
 	}
 
 end:
-	dm_lib_release();
 	dm_lib_exit();
 
 	return r;
diff --git a/libmpathpersist/mpath_persist.c b/libmpathpersist/mpath_persist.c
index 1f9817e..39055ed 100644
--- a/libmpathpersist/mpath_persist.c
+++ b/libmpathpersist/mpath_persist.c
@@ -56,7 +56,6 @@ mpath_lib_init (void)
 int
 mpath_lib_exit (struct config *conf)
 {
-	dm_lib_release();
 	dm_lib_exit();
 	cleanup_prio();
 	cleanup_checkers();
diff --git a/multipath/main.c b/multipath/main.c
index 9e920d8..dc4974b 100644
--- a/multipath/main.c
+++ b/multipath/main.c
@@ -1063,7 +1063,6 @@ main (int argc, char *argv[])
 		condlog(3, "restart multipath configuration process");
 
 out:
-	dm_lib_release();
 	dm_lib_exit();
 
 	cleanup_foreign();
diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
index 235e2a2..5463573 100644
--- a/multipathd/cli_handlers.c
+++ b/multipathd/cli_handlers.c
@@ -860,7 +860,6 @@ cli_add_map (void * v, char ** reply, int * len, void * data)
 				    != CP_OK)
 					condlog(2, "%s: coalesce_paths failed",
 									param);
-				dm_lib_release();
 				FREE(refwwid);
 			}
 		} /*we attempt to create device only once*/
@@ -1032,7 +1031,6 @@ cli_resize(void *v, char **reply, int *len, void *data)
 	if (resize_map(mpp, size, vecs) != 0)
 		return 1;
 
-	dm_lib_release();
 	if (setup_multipath(vecs, mpp) != 0)
 		return 1;
 	sync_map_state(mpp);
diff --git a/multipathd/main.c b/multipathd/main.c
index d1f8cc1..5cc3435 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -510,7 +510,6 @@ retry:
 		sleep(1);
 		goto retry;
 	}
-	dm_lib_release();
 
 fail:
 	if (new_map && (retries < 0 || wait_for_events(mpp, vecs))) {
@@ -611,10 +610,8 @@ coalesce_maps(struct vectors *vecs, vector nmpv)
 				vector_del_slot(ompv, i);
 				i--;
 			}
-			else {
-				dm_lib_release();
+			else
 				condlog(2, "%s devmap removed", ompp->alias);
-			}
 		} else if (reassign_maps) {
 			condlog(3, "%s: Reassign existing device-mapper"
 				" devices", ompp->alias);
@@ -660,10 +657,8 @@ flush_map(struct multipath * mpp, struct vectors * vecs, int nopaths)
 		}
 		return r;
 	}
-	else {
-		dm_lib_release();
+	else
 		condlog(2, "%s: map flushed", mpp->alias);
-	}
 
 	orphan_paths(vecs->pathvec, mpp, "map flushed");
 	remove_map_and_stop_waiter(mpp, vecs);
@@ -1080,7 +1075,6 @@ rescan:
 		else
 			goto fail_map;
 	}
-	dm_lib_release();
 
 	if ((mpp->action == ACT_CREATE ||
 	     (mpp->action == ACT_NOTHING && start_waiter && !mpp->waiter)) &&
@@ -1947,8 +1941,6 @@ int reload_and_sync_map(struct multipath *mpp,
 {
 	if (reload_map(vecs, mpp, refresh, 1))
 		return 1;
-
-	dm_lib_release();
 	if (setup_multipath(vecs, mpp) != 0)
 		return 2;
 	sync_map_state(mpp);
@@ -2631,8 +2623,6 @@ configure (struct vectors * vecs)
 		goto fail;
 	}
 
-	dm_lib_release();
-
 	if (should_exit())
 		goto fail;
 
@@ -3115,7 +3105,6 @@ child (__attribute__((unused)) void *param)
 	if (poll_dmevents)
 		cleanup_dmevent_waiter();
 
-	dm_lib_release();
 	dm_lib_exit();
 
 	/* We're done here */
-- 
2.28.0

^ permalink raw reply related	[flat|nested] 35+ messages in thread

* [PATCH v2 10/21] libmultipath: devmapper: refactor libdm version determination
  2020-09-24 13:36 [PATCH v2 00/21] multipath-tools: shutdown, libdevmapper races, globals mwilck
                   ` (8 preceding siblings ...)
  2020-09-24 13:37 ` [PATCH v2 09/21] multipath-tools: don't call dm_lib_release() any more mwilck
@ 2020-09-24 13:37 ` mwilck
  2020-09-25 23:48   ` Benjamin Marzinski
  2020-09-24 13:37 ` [PATCH v2 11/21] libmultipath: protect racy libdevmapper calls with a mutex mwilck
                   ` (10 subsequent siblings)
  20 siblings, 1 reply; 35+ messages in thread
From: mwilck @ 2020-09-24 13:37 UTC (permalink / raw)
  To: Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel, Martin Wilck

From: Martin Wilck <mwilck@suse.com>

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.
Note that retrieving the versions requires libdm calls, thus the
version initialization calls libdm initialization, which might call
version initialization recursively. But that's not an issue, as
the initialization is protected by pthread_once().

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 ABI change as offsets in "struct config" are changed.
Bump the ABI version.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/config.h             |   1 -
 libmultipath/devmapper.c          | 160 ++++++++++++++++++++----------
 libmultipath/devmapper.h          |  11 +-
 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(+), 67 deletions(-)

diff --git a/libmultipath/config.h b/libmultipath/config.h
index 2bb7153..8e13ae9 100644
--- a/libmultipath/config.h
+++ b/libmultipath/config.h
@@ -191,7 +191,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 7394796..08aa09f 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,59 +119,66 @@ 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_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;
@@ -172,10 +186,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;
 
@@ -211,26 +226,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;
 	}
 
@@ -239,13 +253,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)
@@ -263,7 +324,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 f568ab5..c8b37e1 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);
@@ -85,6 +82,14 @@ struct multipath *dm_get_multipath(const char *name);
 	((v[0] == minv[0]) && (v[1] == minv[1]) && (v[2] >= minv[2])) \
 )
 
+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__, \
 		cmd, strerror(dm_task_get_errno(dmt)))	      \
diff --git a/libmultipath/libmultipath.version b/libmultipath/libmultipath.version
index d6ac0e1..5699a0b 100644
--- a/libmultipath/libmultipath.version
+++ b/libmultipath/libmultipath.version
@@ -1,4 +1,4 @@
-LIBMULTIPATH_0.8.4.0 {
+LIBMULTIPATH_0.8.4.1 {
 global:
 	/* symbols referenced by multipath and multipathd */
 	add_foreign;
@@ -32,7 +32,6 @@ global:
 	disassemble_status;
 	dlog;
 	dm_cancel_deferred_remove;
-	dm_drv_version;
 	dm_enablegroup;
 	dm_fail_path;
 	_dm_flush_map;
@@ -54,7 +53,6 @@ global:
 	dm_reinstate_path;
 	dm_simplecmd_noflush;
 	dm_switchgroup;
-	dm_tgt_version;
 	domap;
 	ensure_directories_exist;
 	extract_hwe_from_path;
@@ -95,6 +93,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 4020134..3f2c2cf 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 5f2d210..fc97c8a 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 5cc3435..00b66ba 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 47e6b86..a681c11 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -42,7 +42,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 12660da..57f832b 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 b7c09cc..e7663f9 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.28.0

^ permalink raw reply related	[flat|nested] 35+ messages in thread

* [PATCH v2 11/21] libmultipath: protect racy libdevmapper calls with a mutex
  2020-09-24 13:36 [PATCH v2 00/21] multipath-tools: shutdown, libdevmapper races, globals mwilck
                   ` (9 preceding siblings ...)
  2020-09-24 13:37 ` [PATCH v2 10/21] libmultipath: devmapper: refactor libdm version determination mwilck
@ 2020-09-24 13:37 ` mwilck
  2020-09-29  5:57   ` Benjamin Marzinski
  2020-09-24 13:37 ` [PATCH v2 12/21] libmultipath: constify file argument in config parser mwilck
                   ` (9 subsequent siblings)
  20 siblings, 1 reply; 35+ messages in thread
From: mwilck @ 2020-09-24 13:37 UTC (permalink / raw)
  To: Christophe Varoqui, Benjamin Marzinski; +Cc: lixiaokeng, dm-devel, Martin Wilck

From: Martin Wilck <mwilck@suse.com>

dm_udev_wait() and dm_task_run() may access global / static state
in libdevmapper. They need to be protected by a lock in in our
multithreaded library.

Cc: lixiaokeng@huawei.com
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/devmapper.c          | 73 +++++++++++++++++++------------
 libmultipath/devmapper.h          |  2 +
 libmultipath/libmultipath.version |  6 +++
 libmultipath/util.c               |  5 +++
 libmultipath/util.h               |  1 +
 multipathd/dmevents.c             |  2 +-
 multipathd/waiter.c               |  2 +-
 7 files changed, 62 insertions(+), 29 deletions(-)

diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
index 08aa09f..2e3ae8a 100644
--- a/libmultipath/devmapper.c
+++ b/libmultipath/devmapper.c
@@ -42,6 +42,7 @@ 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 pthread_mutex_t libmp_dm_lock = PTHREAD_MUTEX_INITIALIZER;
 
 static int dm_conf_verbosity;
 
@@ -59,16 +60,34 @@ static inline int dm_task_set_cookie(struct dm_task *dmt, uint32_t *c, int a)
 	return 1;
 }
 
-void dm_udev_wait(unsigned int c)
+static void libmp_udev_wait(unsigned int c)
 {
 }
 
-void dm_udev_set_sync_support(int c)
+static void dm_udev_set_sync_support(int c)
 {
 }
-
+#else
+static void libmp_udev_wait(unsigned int c)
+{
+	pthread_mutex_lock(&libmp_dm_lock);
+	pthread_cleanup_push(cleanup_mutex, &libmp_dm_lock);
+	dm_udev_wait(c);
+	pthread_cleanup_pop(1);
+}
 #endif
 
+int libmp_dm_task_run(struct dm_task *dmt)
+{
+	int r;
+
+	pthread_mutex_lock(&libmp_dm_lock);
+	pthread_cleanup_push(cleanup_mutex, &libmp_dm_lock);
+	r = dm_task_run(dmt);
+	pthread_cleanup_pop(1);
+	return r;
+}
+
 __attribute__((format(printf, 4, 5))) static void
 dm_write_log (int level, const char *file, int line, const char *f, ...)
 {
@@ -196,7 +215,7 @@ static int dm_tgt_version (unsigned int *version, char *str)
 
 	dm_task_no_open_count(dmt);
 
-	if (!dm_task_run(dmt)) {
+	if (!libmp_dm_task_run(dmt)) {
 		dm_log_error(2, DM_DEVICE_LIST_VERSIONS, dmt);
 		condlog(0, "Can not communicate with kernel DM");
 		goto out;
@@ -380,12 +399,12 @@ dm_simplecmd (int task, const char *name, int no_flush, int need_sync, uint16_t
 				DM_UDEV_DISABLE_LIBRARY_FALLBACK | udev_flags))
 		goto out;
 
-	r = dm_task_run (dmt);
+	r = libmp_dm_task_run (dmt);
 	if (!r)
 		dm_log_error(2, task, dmt);
 
 	if (udev_wait_flag)
-			dm_udev_wait(cookie);
+			libmp_udev_wait(cookie);
 out:
 	dm_task_destroy (dmt);
 	return r;
@@ -472,12 +491,12 @@ dm_addmap (int task, const char *target, struct multipath *mpp,
 	    !dm_task_set_cookie(dmt, &cookie, udev_flags))
 		goto freeout;
 
-	r = dm_task_run (dmt);
+	r = libmp_dm_task_run (dmt);
 	if (!r)
 		dm_log_error(2, task, dmt);
 
 	if (task == DM_DEVICE_CREATE)
-			dm_udev_wait(cookie);
+			libmp_udev_wait(cookie);
 freeout:
 	if (prefixed_uuid)
 		FREE(prefixed_uuid);
@@ -587,7 +606,7 @@ do_get_info(const char *name, struct dm_info *info)
 
 	dm_task_no_open_count(dmt);
 
-	if (!dm_task_run(dmt)) {
+	if (!libmp_dm_task_run(dmt)) {
 		dm_log_error(3, DM_DEVICE_INFO, dmt);
 		goto out;
 	}
@@ -628,7 +647,7 @@ int dm_get_map(const char *name, unsigned long long *size, char *outparams)
 	dm_task_no_open_count(dmt);
 
 	errno = 0;
-	if (!dm_task_run(dmt)) {
+	if (!libmp_dm_task_run(dmt)) {
 		dm_log_error(3, DM_DEVICE_TABLE, dmt);
 		if (dm_task_get_errno(dmt) == ENXIO)
 			r = DMP_NOT_FOUND;
@@ -670,7 +689,7 @@ dm_get_prefixed_uuid(const char *name, char *uuid, int uuid_len)
 	if (!dm_task_set_name (dmt, name))
 		goto uuidout;
 
-	if (!dm_task_run(dmt)) {
+	if (!libmp_dm_task_run(dmt)) {
 		dm_log_error(3, DM_DEVICE_INFO, dmt);
 		goto uuidout;
 	}
@@ -741,7 +760,7 @@ int dm_get_status(const char *name, char *outstatus)
 	dm_task_no_open_count(dmt);
 
 	errno = 0;
-	if (!dm_task_run(dmt)) {
+	if (!libmp_dm_task_run(dmt)) {
 		dm_log_error(3, DM_DEVICE_STATUS, dmt);
 		if (dm_task_get_errno(dmt) == ENXIO)
 			r = DMP_NOT_FOUND;
@@ -794,7 +813,7 @@ int dm_type(const char *name, char *type)
 
 	dm_task_no_open_count(dmt);
 
-	if (!dm_task_run(dmt)) {
+	if (!libmp_dm_task_run(dmt)) {
 		dm_log_error(3, DM_DEVICE_TABLE, dmt);
 		goto out;
 	}
@@ -838,7 +857,7 @@ int dm_is_mpath(const char *name)
 
 	dm_task_no_open_count(dmt);
 
-	if (!dm_task_run(dmt)) {
+	if (!libmp_dm_task_run(dmt)) {
 		dm_log_error(3, DM_DEVICE_TABLE, dmt);
 		goto out_task;
 	}
@@ -902,7 +921,7 @@ dm_map_present_by_uuid(const char *uuid)
 	if (!dm_task_set_uuid(dmt, prefixed_uuid))
 		goto out_task;
 
-	if (!dm_task_run(dmt)) {
+	if (!libmp_dm_task_run(dmt)) {
 		dm_log_error(3, DM_DEVICE_INFO, dmt);
 		goto out_task;
 	}
@@ -948,7 +967,7 @@ dm_get_opencount (const char * mapname)
 	if (!dm_task_set_name(dmt, mapname))
 		goto out;
 
-	if (!dm_task_run(dmt)) {
+	if (!libmp_dm_task_run(dmt)) {
 		dm_log_error(3, DM_DEVICE_INFO, dmt);
 		goto out;
 	}
@@ -1108,7 +1127,7 @@ int dm_flush_maps (int need_suspend, int retries)
 
 	dm_task_no_open_count(dmt);
 
-	if (!dm_task_run (dmt)) {
+	if (!libmp_dm_task_run (dmt)) {
 		dm_log_error(3, DM_DEVICE_LIST, dmt);
 		goto out;
 	}
@@ -1154,7 +1173,7 @@ dm_message(const char * mapname, char * message)
 
 	dm_task_no_open_count(dmt);
 
-	if (!dm_task_run(dmt)) {
+	if (!libmp_dm_task_run(dmt)) {
 		dm_log_error(2, DM_DEVICE_TARGET_MSG, dmt);
 		goto out;
 	}
@@ -1274,7 +1293,7 @@ dm_get_maps (vector mp)
 
 	dm_task_no_open_count(dmt);
 
-	if (!dm_task_run(dmt)) {
+	if (!libmp_dm_task_run(dmt)) {
 		dm_log_error(3, DM_DEVICE_LIST, dmt);
 		goto out;
 	}
@@ -1359,7 +1378,7 @@ dm_mapname(int major, int minor)
 	 * daemon uev_trigger -> uev_add_map
 	 */
 	while (--loop) {
-		r = dm_task_run(dmt);
+		r = libmp_dm_task_run(dmt);
 
 		if (r)
 			break;
@@ -1404,7 +1423,7 @@ do_foreach_partmaps (const char * mapname,
 
 	dm_task_no_open_count(dmt);
 
-	if (!dm_task_run(dmt)) {
+	if (!libmp_dm_task_run(dmt)) {
 		dm_log_error(3, DM_DEVICE_LIST, dmt);
 		goto out;
 	}
@@ -1639,11 +1658,11 @@ dm_rename (const char * old, char * new, char *delim, int skip_kpartx)
 
 	if (!dm_task_set_cookie(dmt, &cookie, udev_flags))
 		goto out;
-	r = dm_task_run(dmt);
+	r = libmp_dm_task_run(dmt);
 	if (!r)
 		dm_log_error(2, DM_DEVICE_RENAME, dmt);
 
-	dm_udev_wait(cookie);
+	libmp_udev_wait(cookie);
 
 out:
 	dm_task_destroy(dmt);
@@ -1685,7 +1704,7 @@ int dm_reassign_table(const char *name, char *old, char *new)
 
 	dm_task_no_open_count(dmt);
 
-	if (!dm_task_run(dmt)) {
+	if (!libmp_dm_task_run(dmt)) {
 		dm_log_error(3, DM_DEVICE_TABLE, dmt);
 		goto out;
 	}
@@ -1718,7 +1737,7 @@ int dm_reassign_table(const char *name, char *old, char *new)
 	if (modified) {
 		dm_task_no_open_count(reload_dmt);
 
-		if (!dm_task_run(reload_dmt)) {
+		if (!libmp_dm_task_run(reload_dmt)) {
 			dm_log_error(3, DM_DEVICE_RELOAD, reload_dmt);
 			condlog(3, "%s: failed to reassign targets", name);
 			goto out_reload;
@@ -1765,7 +1784,7 @@ int dm_reassign(const char *mapname)
 
 	dm_task_no_open_count(dmt);
 
-	if (!dm_task_run(dmt)) {
+	if (!libmp_dm_task_run(dmt)) {
 		dm_log_error(3, DM_DEVICE_DEPS, dmt);
 		goto out;
 	}
@@ -1833,7 +1852,7 @@ int dm_setgeometry(struct multipath *mpp)
 		goto out;
 	}
 
-	r = dm_task_run(dmt);
+	r = libmp_dm_task_run(dmt);
 	if (!r)
 		dm_log_error(3, DM_DEVICE_SET_GEOMETRY, dmt);
 out:
diff --git a/libmultipath/devmapper.h b/libmultipath/devmapper.h
index c8b37e1..158057e 100644
--- a/libmultipath/devmapper.h
+++ b/libmultipath/devmapper.h
@@ -89,6 +89,8 @@ enum {
 	MULTIPATH_VERSION
 };
 int libmp_get_version(int which, unsigned int version[3]);
+struct dm_task;
+int libmp_dm_task_run(struct dm_task *dmt);
 
 #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 5699a0b..0a96010 100644
--- a/libmultipath/libmultipath.version
+++ b/libmultipath/libmultipath.version
@@ -212,3 +212,9 @@ global:
 local:
 	*;
 };
+
+LIBMULTIPATH_0.8.4.2 {
+global:
+	libmp_dm_task_run;
+	cleanup_mutex;
+} LIBMULTIPATH_0.8.4.1;
diff --git a/libmultipath/util.c b/libmultipath/util.c
index 66cd721..41da6ce 100644
--- a/libmultipath/util.c
+++ b/libmultipath/util.c
@@ -409,6 +409,11 @@ void cleanup_free_ptr(void *arg)
 		free(*p);
 }
 
+void cleanup_mutex(void *arg)
+{
+	pthread_mutex_unlock(arg);
+}
+
 struct bitfield *alloc_bitfield(unsigned int maxbit)
 {
 	unsigned int n;
diff --git a/libmultipath/util.h b/libmultipath/util.h
index c1ae878..b8481af 100644
--- a/libmultipath/util.h
+++ b/libmultipath/util.h
@@ -49,6 +49,7 @@ int should_exit(void);
 
 void close_fd(void *arg);
 void cleanup_free_ptr(void *arg);
+void cleanup_mutex(void *arg);
 
 struct scandir_result {
 	struct dirent **di;
diff --git a/multipathd/dmevents.c b/multipathd/dmevents.c
index fc97c8a..b561cbf 100644
--- a/multipathd/dmevents.c
+++ b/multipathd/dmevents.c
@@ -156,7 +156,7 @@ static int dm_get_events(void)
 
 	dm_task_no_open_count(dmt);
 
-	if (!dm_task_run(dmt)) {
+	if (!libmp_dm_task_run(dmt)) {
 		dm_log_error(3, DM_DEVICE_LIST, dmt);
 		goto fail;
 	}
diff --git a/multipathd/waiter.c b/multipathd/waiter.c
index 16fbdc8..620bfa7 100644
--- a/multipathd/waiter.c
+++ b/multipathd/waiter.c
@@ -118,7 +118,7 @@ static int waiteventloop (struct event_thread *waiter)
 	pthread_sigmask(SIG_UNBLOCK, &set, &oldset);
 
 	pthread_testcancel();
-	r = dm_task_run(waiter->dmt);
+	r = libmp_dm_task_run(waiter->dmt);
 	if (!r)
 		dm_log_error(2, DM_DEVICE_WAITEVENT, waiter->dmt);
 	pthread_testcancel();
-- 
2.28.0

^ permalink raw reply related	[flat|nested] 35+ messages in thread

* [PATCH v2 12/21] libmultipath: constify file argument in config parser
  2020-09-24 13:36 [PATCH v2 00/21] multipath-tools: shutdown, libdevmapper races, globals mwilck
                   ` (10 preceding siblings ...)
  2020-09-24 13:37 ` [PATCH v2 11/21] libmultipath: protect racy libdevmapper calls with a mutex mwilck
@ 2020-09-24 13:37 ` mwilck
  2020-09-24 13:37 ` [PATCH v2 13/21] libmultipath: provide defaults for {get, put}_multipath_config mwilck
                   ` (8 subsequent siblings)
  20 siblings, 0 replies; 35+ messages in thread
From: mwilck @ 2020-09-24 13:37 UTC (permalink / raw)
  To: Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel, Martin Wilck

From: Martin Wilck <mwilck@suse.com>

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/config.c | 3 +--
 libmultipath/config.h | 2 +-
 libmultipath/parser.c | 9 +++++----
 libmultipath/parser.h | 2 +-
 4 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/libmultipath/config.c b/libmultipath/config.c
index df0f8f4..5c91a09 100644
--- a/libmultipath/config.c
+++ b/libmultipath/config.c
@@ -719,8 +719,7 @@ static void set_max_checkint_from_watchdog(struct config *conf)
 }
 #endif
 
-struct config *
-load_config (char * file)
+struct config *load_config(const char *file)
 {
 	struct config *conf = alloc_config();
 
diff --git a/libmultipath/config.h b/libmultipath/config.h
index 8e13ae9..116fe37 100644
--- a/libmultipath/config.h
+++ b/libmultipath/config.h
@@ -250,7 +250,7 @@ void free_mptable (vector mptable);
 
 int store_hwe (vector hwtable, struct hwentry *);
 
-struct config *load_config (char * file);
+struct config *load_config (const char *file);
 struct config * alloc_config (void);
 void free_config (struct config * conf);
 extern struct config *get_multipath_config(void);
diff --git a/libmultipath/parser.c b/libmultipath/parser.c
index ed6d5d6..163ffbc 100644
--- a/libmultipath/parser.c
+++ b/libmultipath/parser.c
@@ -390,7 +390,7 @@ oom:
 /* non-recursive configuration stream handler */
 static int kw_level = 0;
 
-int warn_on_duplicates(vector uniques, char *str, char *file)
+int warn_on_duplicates(vector uniques, char *str, const char *file)
 {
 	char *tmp;
 	int i;
@@ -434,7 +434,7 @@ is_sublevel_keyword(char *str)
 }
 
 int
-validate_config_strvec(vector strvec, char *file)
+validate_config_strvec(vector strvec, const char *file)
 {
 	char *str = NULL;
 	int i;
@@ -499,7 +499,8 @@ validate_config_strvec(vector strvec, char *file)
 }
 
 static int
-process_stream(struct config *conf, FILE *stream, vector keywords, char *file)
+process_stream(struct config *conf, FILE *stream, vector keywords,
+	       const char *file)
 {
 	int i;
 	int r = 0, t;
@@ -584,7 +585,7 @@ out:
 
 /* Data initialization */
 int
-process_file(struct config *conf, char *file)
+process_file(struct config *conf, const char *file)
 {
 	int r;
 	FILE *stream;
diff --git a/libmultipath/parser.h b/libmultipath/parser.h
index 62906e9..06666cc 100644
--- a/libmultipath/parser.h
+++ b/libmultipath/parser.h
@@ -77,7 +77,7 @@ extern void dump_keywords(vector keydump, int level);
 extern void free_keywords(vector keywords);
 extern vector alloc_strvec(char *string);
 extern void *set_value(vector strvec);
-extern int process_file(struct config *conf, char *conf_file);
+extern int process_file(struct config *conf, const char *conf_file);
 extern struct keyword * find_keyword(vector keywords, vector v, char * name);
 int snprint_keyword(char *buff, int len, char *fmt, struct keyword *kw,
 		    const void *data);
-- 
2.28.0

^ permalink raw reply related	[flat|nested] 35+ messages in thread

* [PATCH v2 13/21] libmultipath: provide defaults for {get, put}_multipath_config
  2020-09-24 13:36 [PATCH v2 00/21] multipath-tools: shutdown, libdevmapper races, globals mwilck
                   ` (11 preceding siblings ...)
  2020-09-24 13:37 ` [PATCH v2 12/21] libmultipath: constify file argument in config parser mwilck
@ 2020-09-24 13:37 ` mwilck
  2020-09-25  4:34   ` Benjamin Marzinski
  2020-09-24 13:37 ` [PATCH v2 14/21] libmpathpersist: allow using libmultipath " mwilck
                   ` (7 subsequent siblings)
  20 siblings, 1 reply; 35+ messages in thread
From: mwilck @ 2020-09-24 13:37 UTC (permalink / raw)
  To: Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel, Martin Wilck

From: Martin Wilck <mwilck@suse.com>

Add an implementation of get_multipath_config() and put_multipath_config()
to libmultipath. The linker's symbol lookup rules will make sure that
applications can override these functions if they need to. Defining
these functions in libmultipath avoids the need to provide stubs
for these functions in every appliation linking to libmultipath.

libmultipath's internal functions simply refer to a static "struct config".
It must be initialized with init_config() rather than load_config(),
which always allocates a new struct for backward compatibility, and must
be teared down using uninit_config().

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/config.c             | 75 ++++++++++++++++++++++++++-----
 libmultipath/config.h             | 21 +++++++--
 libmultipath/libmultipath.version | 10 +++++
 3 files changed, 93 insertions(+), 13 deletions(-)

diff --git a/libmultipath/config.c b/libmultipath/config.c
index 5c91a09..01b77df 100644
--- a/libmultipath/config.c
+++ b/libmultipath/config.c
@@ -27,6 +27,26 @@
 #include "mpath_cmd.h"
 #include "propsel.h"
 
+static struct config __internal_config;
+struct config *libmp_get_multipath_config(void)
+{
+	if (!__internal_config.hwtable)
+		/* not initialized */
+		return NULL;
+	return &__internal_config;
+}
+
+struct config *get_multipath_config(void)
+	__attribute__((weak, alias("libmp_get_multipath_config")));
+
+void libmp_put_multipath_config(void *conf __attribute__((unused)))
+{
+	/* empty */
+}
+
+void put_multipath_config(void *conf)
+	__attribute__((weak, alias("libmp_put_multipath_config")));
+
 static int
 hwe_strmatch (const struct hwentry *hwe1, const struct hwentry *hwe2)
 {
@@ -574,17 +594,15 @@ restart:
 	return;
 }
 
-struct config *
-alloc_config (void)
+static struct config *alloc_config (void)
 {
 	return (struct config *)MALLOC(sizeof(struct config));
 }
 
-void
-free_config (struct config * conf)
+static void _uninit_config(struct config *conf)
 {
 	if (!conf)
-		return;
+		conf = &__internal_config;
 
 	if (conf->multipath_dir)
 		FREE(conf->multipath_dir);
@@ -650,7 +668,27 @@ free_config (struct config * conf)
 	free_hwtable(conf->hwtable);
 	free_hwe(conf->overrides);
 	free_keywords(conf->keywords);
-	FREE(conf);
+
+	memset(conf, 0, sizeof(*conf));
+}
+
+void uninit_config(void)
+{
+	_uninit_config(&__internal_config);
+}
+
+void free_config(struct config *conf)
+{
+	if (!conf)
+		return;
+	else if (conf == &__internal_config) {
+		condlog(0, "ERROR: %s called for internal config. Use uninit_config() instead",
+			__func__);
+		return;
+	}
+
+	_uninit_config(conf);
+	free(conf);
 }
 
 /* if multipath fails to process the config directory, it should continue,
@@ -719,12 +757,29 @@ static void set_max_checkint_from_watchdog(struct config *conf)
 }
 #endif
 
+static int _init_config (const char *file, struct config *conf);
+
+int init_config(const char *file)
+{
+	return _init_config(file, &__internal_config);
+}
+
 struct config *load_config(const char *file)
 {
 	struct config *conf = alloc_config();
 
+	if (conf && !_init_config(file, conf))
+		return conf;
+
+	free(conf);
+	return NULL;
+}
+
+int _init_config (const char *file, struct config *conf)
+{
+
 	if (!conf)
-		return NULL;
+		conf = &__internal_config;
 
 	/*
 	 * internal defaults
@@ -896,10 +951,10 @@ struct config *load_config(const char *file)
 	    !conf->wwids_file || !conf->prkeys_file)
 		goto out;
 
-	return conf;
+	return 0;
 out:
-	free_config(conf);
-	return NULL;
+	_uninit_config(conf);
+	return 1;
 }
 
 char *get_uid_attribute_by_attrs(struct config *conf,
diff --git a/libmultipath/config.h b/libmultipath/config.h
index 116fe37..5997b71 100644
--- a/libmultipath/config.h
+++ b/libmultipath/config.h
@@ -251,10 +251,25 @@ void free_mptable (vector mptable);
 int store_hwe (vector hwtable, struct hwentry *);
 
 struct config *load_config (const char *file);
-struct config * alloc_config (void);
 void free_config (struct config * conf);
-extern struct config *get_multipath_config(void);
-extern void put_multipath_config(void *);
+int init_config(const char *file);
+void uninit_config(void);
+
+/*
+ * libmultipath provides default implementations of
+ * get_multipath_config() and put_multipath_config().
+ * Applications using these should use init_config(file, NULL)
+ * to load the configuration, rather than load_config(file).
+ * Likewise, uninit_config() should be used for teardown, but
+ * using free_config() for that is supported, too.
+ * Applications can define their own {get,put}_multipath_config()
+ * functions, which override the library-internal ones, but
+ * could still call libmp_{get,put}_multipath_config().
+ */
+struct config *libmp_get_multipath_config(void);
+struct config *get_multipath_config(void);
+void libmp_put_multipath_config(void *);
+void put_multipath_config(void *);
 
 int parse_uid_attrs(char *uid_attrs, struct config *conf);
 char *get_uid_attribute_by_attrs(struct config *conf,
diff --git a/libmultipath/libmultipath.version b/libmultipath/libmultipath.version
index 0a96010..81bcc9d 100644
--- a/libmultipath/libmultipath.version
+++ b/libmultipath/libmultipath.version
@@ -218,3 +218,13 @@ global:
 	libmp_dm_task_run;
 	cleanup_mutex;
 } LIBMULTIPATH_0.8.4.1;
+
+LIBMULTIPATH_0.8.4.3 {
+global:
+	libmp_get_multipath_config;
+	get_multipath_config;
+	libmp_put_multipath_config;
+	put_multipath_config;
+	init_config;
+	uninit_config;
+} LIBMULTIPATH_0.8.4.2;
-- 
2.28.0

^ permalink raw reply related	[flat|nested] 35+ messages in thread

* [PATCH v2 14/21] libmpathpersist: allow using libmultipath {get, put}_multipath_config
  2020-09-24 13:36 [PATCH v2 00/21] multipath-tools: shutdown, libdevmapper races, globals mwilck
                   ` (12 preceding siblings ...)
  2020-09-24 13:37 ` [PATCH v2 13/21] libmultipath: provide defaults for {get, put}_multipath_config mwilck
@ 2020-09-24 13:37 ` mwilck
  2020-09-24 13:37 ` [PATCH v2 15/21] multipath: use {get_put}_multipath_config from libmultipath mwilck
                   ` (6 subsequent siblings)
  20 siblings, 0 replies; 35+ messages in thread
From: mwilck @ 2020-09-24 13:37 UTC (permalink / raw)
  To: Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel, Martin Wilck

From: Martin Wilck <mwilck@suse.com>

Provide an alternative init function libmpathpersist_init() which
avoids allocating a new struct config, simply using libmultipath's
internal implementation.

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmpathpersist/libmpathpersist.version |  6 ++++
 libmpathpersist/mpath_persist.c         | 42 +++++++++++++++++++++----
 libmpathpersist/mpath_persist.h         | 28 +++++++++++++++++
 3 files changed, 70 insertions(+), 6 deletions(-)

diff --git a/libmpathpersist/libmpathpersist.version b/libmpathpersist/libmpathpersist.version
index 1bb8212..e81e625 100644
--- a/libmpathpersist/libmpathpersist.version
+++ b/libmpathpersist/libmpathpersist.version
@@ -18,3 +18,9 @@ global:
 
 local: *;
 };
+
+LIBMPATHPERSIST_0.8.4.1 {
+global:
+	libmpathpersist_init;
+	libmpathpersist_exit;
+} LIBMPATHPERSIST_0.8.4.0;
diff --git a/libmpathpersist/mpath_persist.c b/libmpathpersist/mpath_persist.c
index 39055ed..a529a38 100644
--- a/libmpathpersist/mpath_persist.c
+++ b/libmpathpersist/mpath_persist.c
@@ -37,6 +37,27 @@
 
 extern struct udev *udev;
 
+static void adapt_config(struct config *conf)
+{
+	conf->force_sync = 1;
+	set_max_fds(conf->max_fds);
+}
+
+int libmpathpersist_init(void)
+{
+	struct config *conf;
+	int rc = 0;
+
+	if (init_config(DEFAULT_CONFIGFILE)) {
+		condlog(0, "Failed to initialize multipath config.");
+		return 1;
+	}
+	conf = libmp_get_multipath_config();
+	adapt_config(conf);
+	libmp_put_multipath_config(conf);
+	return rc;
+}
+
 struct config *
 mpath_lib_init (void)
 {
@@ -47,20 +68,29 @@ mpath_lib_init (void)
 		condlog(0, "Failed to initialize multipath config.");
 		return NULL;
 	}
-	conf->force_sync = 1;
-	set_max_fds(conf->max_fds);
-
+	adapt_config(conf);
 	return conf;
 }
 
-int
-mpath_lib_exit (struct config *conf)
+static void libmpathpersist_cleanup(void)
 {
 	dm_lib_exit();
 	cleanup_prio();
 	cleanup_checkers();
+}
+
+int
+mpath_lib_exit (struct config *conf)
+{
+	libmpathpersist_cleanup();
 	free_config(conf);
-	conf = NULL;
+	return 0;
+}
+
+int libmpathpersist_exit(void)
+{
+	libmpathpersist_cleanup();
+	uninit_config();
 	return 0;
 }
 
diff --git a/libmpathpersist/mpath_persist.h b/libmpathpersist/mpath_persist.h
index 7cf4faf..91606ef 100644
--- a/libmpathpersist/mpath_persist.h
+++ b/libmpathpersist/mpath_persist.h
@@ -175,6 +175,22 @@ struct prout_param_descriptor {		/* PROUT parameter descriptor */
  * DESCRIPTION :
  *	Initialize device mapper multipath configuration. This function must be invoked first
  *	before performing reservation management functions.
+ *	Either this function or mpath_lib_init() may be used.
+ *	Use this function to work with libmultipath's internal "struct config".
+ *	Call libmpathpersist_exit() for cleanup.
+ * RESTRICTIONS:
+ *
+ * RETURNS: 0->Success, 1->Failed.
+ */
+extern int libmpathpersist_init (void);
+
+/*
+ * DESCRIPTION :
+ *	Initialize device mapper multipath configuration. This function must be invoked first
+ *	before performing reservation management functions.
+ *	Either this function or libmpathpersist_init() may be used.
+ *	Use this function to work with an application-specific "struct config".
+ *	Call mpath_lib_exit() for cleanup.
  * RESTRICTIONS:
  *
  * RETURNS: struct config ->Success, NULL->Failed.
@@ -186,12 +202,24 @@ extern struct config * mpath_lib_init (void);
  * DESCRIPTION :
  *	Release device mapper multipath configuration. This function must be invoked after
  *	performing reservation management functions.
+ *	Use this after initialization with mpath_lib_init().
  * RESTRICTIONS:
  *
  * RETURNS: 0->Success, 1->Failed.
  */
 extern int mpath_lib_exit (struct config *conf);
 
+/*
+ * DESCRIPTION :
+ *	Release device mapper multipath configuration. This function must be invoked after
+ *	performing reservation management functions.
+ *	Use this after initialization with libmpathpersist_init().
+ * RESTRICTIONS:
+ *
+ * RETURNS: 0->Success, 1->Failed.
+ */
+extern int libmpathpersist_exit (void);
+
 
 /*
  * DESCRIPTION :
-- 
2.28.0

^ permalink raw reply related	[flat|nested] 35+ messages in thread

* [PATCH v2 15/21] multipath: use {get_put}_multipath_config from libmultipath
  2020-09-24 13:36 [PATCH v2 00/21] multipath-tools: shutdown, libdevmapper races, globals mwilck
                   ` (13 preceding siblings ...)
  2020-09-24 13:37 ` [PATCH v2 14/21] libmpathpersist: allow using libmultipath " mwilck
@ 2020-09-24 13:37 ` mwilck
  2020-09-24 13:37 ` [PATCH v2 16/21] mpathpersist: use {get, put}_multipath_config() " mwilck
                   ` (5 subsequent siblings)
  20 siblings, 0 replies; 35+ messages in thread
From: mwilck @ 2020-09-24 13:37 UTC (permalink / raw)
  To: Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel, Martin Wilck

From: Martin Wilck <mwilck@suse.com>

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipath/main.c | 20 ++++----------------
 1 file changed, 4 insertions(+), 16 deletions(-)

diff --git a/multipath/main.c b/multipath/main.c
index dc4974b..4bbfce9 100644
--- a/multipath/main.c
+++ b/multipath/main.c
@@ -68,7 +68,6 @@
 
 int logsink;
 struct udev *udev;
-struct config *multipath_conf;
 
 /*
  * Return values of configure(), check_path_valid(), and main().
@@ -79,16 +78,6 @@ enum {
 	RTVL_RETRY, /* returned by configure(), not by main() */
 };
 
-struct config *get_multipath_config(void)
-{
-	return multipath_conf;
-}
-
-void put_multipath_config(__attribute__((unused)) void *arg)
-{
-	/* Noop for now */
-}
-
 static int
 dump_config (struct config *conf, vector hwes, vector mpvec)
 {
@@ -823,10 +812,9 @@ main (int argc, char *argv[])
 
 	udev = udev_new();
 	logsink = 0;
-	conf = load_config(DEFAULT_CONFIGFILE);
-	if (!conf)
+	if (init_config(DEFAULT_CONFIGFILE))
 		exit(RTVL_FAIL);
-	multipath_conf = conf;
+	conf = get_multipath_config();
 	conf->retrigger_tries = 0;
 	conf->force_sync = 1;
 	while ((arg = getopt(argc, argv, ":adDcChl::eFfM:v:p:b:BrR:itTquUwW")) != EOF ) {
@@ -1078,8 +1066,8 @@ out_free_config:
 	 * the logging function (dm_write_log()), which is called there,
 	 * references the config.
 	 */
-	free_config(conf);
-	conf = NULL;
+	put_multipath_config(conf);
+	uninit_config();
 	udev_unref(udev);
 	if (dev)
 		FREE(dev);
-- 
2.28.0

^ permalink raw reply related	[flat|nested] 35+ messages in thread

* [PATCH v2 16/21] mpathpersist: use {get, put}_multipath_config() from libmultipath
  2020-09-24 13:36 [PATCH v2 00/21] multipath-tools: shutdown, libdevmapper races, globals mwilck
                   ` (14 preceding siblings ...)
  2020-09-24 13:37 ` [PATCH v2 15/21] multipath: use {get_put}_multipath_config from libmultipath mwilck
@ 2020-09-24 13:37 ` mwilck
  2020-09-24 13:37 ` [PATCH v2 17/21] libmultipath: add udev and logsink symbols mwilck
                   ` (4 subsequent siblings)
  20 siblings, 0 replies; 35+ messages in thread
From: mwilck @ 2020-09-24 13:37 UTC (permalink / raw)
  To: Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel, Martin Wilck

From: Martin Wilck <mwilck@suse.com>

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 mpathpersist/main.c | 16 ++--------------
 1 file changed, 2 insertions(+), 14 deletions(-)

diff --git a/mpathpersist/main.c b/mpathpersist/main.c
index a6a3bcf..278e48f 100644
--- a/mpathpersist/main.c
+++ b/mpathpersist/main.c
@@ -43,17 +43,6 @@ void mpath_print_transport_id(struct prin_fulldescr *fdesc);
 int construct_transportid(const char * inp, struct transportid transid[], int num_transportids);
 
 int logsink;
-struct config *multipath_conf;
-
-struct config *get_multipath_config(void)
-{
-	return multipath_conf;
-}
-
-void put_multipath_config(__attribute__((unused)) void * arg)
-{
-	/* Noop for now */
-}
 
 void rcu_register_thread_memb(void) {}
 
@@ -653,15 +642,14 @@ int main(int argc, char *argv[])
 	}
 
 	udev = udev_new();
-	multipath_conf = mpath_lib_init();
-	if(!multipath_conf) {
+	if (libmpathpersist_init()) {
 		udev_unref(udev);
 		exit(1);
 	}
 
 	ret = handle_args(argc, argv, 0);
 
-	mpath_lib_exit(multipath_conf);
+	libmpathpersist_exit();
 	udev_unref(udev);
 
 	return (ret >= 0) ? ret : MPATH_PR_OTHER;
-- 
2.28.0

^ permalink raw reply related	[flat|nested] 35+ messages in thread

* [PATCH v2 17/21] libmultipath: add udev and logsink symbols
  2020-09-24 13:36 [PATCH v2 00/21] multipath-tools: shutdown, libdevmapper races, globals mwilck
                   ` (15 preceding siblings ...)
  2020-09-24 13:37 ` [PATCH v2 16/21] mpathpersist: use {get, put}_multipath_config() " mwilck
@ 2020-09-24 13:37 ` mwilck
  2020-09-25 23:03   ` Benjamin Marzinski
  2020-09-24 13:37 ` [PATCH v2 18/21] multipath: remove logsink and udev mwilck
                   ` (3 subsequent siblings)
  20 siblings, 1 reply; 35+ messages in thread
From: mwilck @ 2020-09-24 13:37 UTC (permalink / raw)
  To: Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel, Martin Wilck

From: Martin Wilck <mwilck@suse.com>

With these symbols added, applications using libmultipath don't
need to define global variables "udev" and "logsink" any more.
This comes at the cost of having to call an init function.
Currently, libmultipath_init() does nothing but initialize
"udev".

The linker's symbol lookup order still allows applications to use
their own "logsink" and "udev" variables, which will take precendence
over libmultipath's internal ones. In this case, calling
libmultipath_init() can be skipped, but like before,
udev should be initialized (using udev_new()) before making any
libmultipath calls.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/config.c             | 46 +++++++++++++++++++++++++++++++
 libmultipath/config.h             | 46 ++++++++++++++++++++++++++++++-
 libmultipath/debug.c              |  2 ++
 libmultipath/libmultipath.version |  8 ++++++
 4 files changed, 101 insertions(+), 1 deletion(-)

diff --git a/libmultipath/config.c b/libmultipath/config.c
index 01b77df..fbb66b3 100644
--- a/libmultipath/config.c
+++ b/libmultipath/config.c
@@ -27,6 +27,52 @@
 #include "mpath_cmd.h"
 #include "propsel.h"
 
+/*
+ * We don't support re-initialization after
+ * libmultipath_exit().
+ */
+static bool libmultipath_exit_called;
+static pthread_once_t _init_once = PTHREAD_ONCE_INIT;
+static pthread_once_t _exit_once = PTHREAD_ONCE_INIT;
+struct udev *udev;
+
+static void _udev_init(void)
+{
+	if (udev)
+		udev_ref(udev);
+	else
+		udev = udev_new();
+	if (!udev)
+		condlog(0, "%s: failed to initialize udev", __func__);
+}
+
+static void _libmultipath_init(void)
+{
+	_udev_init();
+}
+
+static bool _is_libmultipath_initialized(void)
+{
+	return !libmultipath_exit_called && !!udev;
+}
+
+int libmultipath_init(void)
+{
+	pthread_once(&_init_once, _libmultipath_init);
+	return !_is_libmultipath_initialized();
+}
+
+static void _libmultipath_exit(void)
+{
+	libmultipath_exit_called = true;
+	udev_unref(udev);
+}
+
+void libmultipath_exit(void)
+{
+	pthread_once(&_exit_once, _libmultipath_exit);
+}
+
 static struct config __internal_config;
 struct config *libmp_get_multipath_config(void)
 {
diff --git a/libmultipath/config.h b/libmultipath/config.h
index 5997b71..dac4e8f 100644
--- a/libmultipath/config.h
+++ b/libmultipath/config.h
@@ -232,7 +232,51 @@ struct config {
 	char *enable_foreign;
 };
 
-extern struct udev * udev;
+/**
+ * extern variable: udev
+ *
+ * A &struct udev instance used by libmultipath. libmultipath expects
+ * a valid, initialized &struct udev in this variable.
+ * An application can define this variable itself, in which case
+ * the applications's instance will take precedence.
+ * The application can initialize and destroy this variable by
+ * calling libmultipath_init() and libmultipath_exit(), respectively,
+ * whether or not it defines the variable itself.
+ * An application can initialize udev with udev_new() before calling
+ * libmultipath_init(), e.g. if it has to make libudev calls before
+ * libmultipath calls. If an application wants to keep using the
+ * udev variable after calling libmultipath_exit(), it should have taken
+ * an additional reference on it beforehand. This is the case e.g.
+ * after initiazing udev with udev_new().
+ */
+extern struct udev *udev;
+
+/**
+ * libmultipath_init() - library initialization
+ *
+ * This function initializes libmultipath data structures.
+ * It is light-weight; some other initializations, like device-mapper
+ * initialization, are done lazily when the respective functionality
+ * is required.
+ *
+ * Clean up by libmultipath_exit() when the program terminates.
+ * It is an error to call libmultipath_init() after libmultipath_exit().
+ * Return: 0 on success, 1 on failure.
+ */
+int libmultipath_init(void);
+
+/**
+ * libmultipath_exit() - library un-initialization
+ *
+ * This function un-initializes libmultipath data structures.
+ * It is recommended to call this function at program exit.
+ *
+ * Calls to libmultipath_init() after libmultipath_exit() will fail
+ * (in other words, libmultipath can't be re-initialized).
+ * Any other libmultipath calls after libmultipath_exit() may cause
+ * undefined behavior.
+ */
+void libmultipath_exit(void);
 
 int find_hwe (const struct _vector *hwtable,
 	      const char * vendor, const char * product, const char *revision,
diff --git a/libmultipath/debug.c b/libmultipath/debug.c
index 4128cb9..b3a1de9 100644
--- a/libmultipath/debug.c
+++ b/libmultipath/debug.c
@@ -15,6 +15,8 @@
 #include "defaults.h"
 #include "debug.h"
 
+int logsink;
+
 void dlog (int sink, int prio, const char * fmt, ...)
 {
 	va_list ap;
diff --git a/libmultipath/libmultipath.version b/libmultipath/libmultipath.version
index 81bcc9d..2e531ef 100644
--- a/libmultipath/libmultipath.version
+++ b/libmultipath/libmultipath.version
@@ -228,3 +228,11 @@ global:
 	init_config;
 	uninit_config;
 } LIBMULTIPATH_0.8.4.2;
+
+LIBMULTIPATH_0.8.4.4 {
+global:
+	udev;
+	logsink;
+	libmultipath_init;
+	libmultipath_exit;
+} LIBMULTIPATH_0.8.4.3;
-- 
2.28.0

^ permalink raw reply related	[flat|nested] 35+ messages in thread

* [PATCH v2 18/21] multipath: remove logsink and udev
  2020-09-24 13:36 [PATCH v2 00/21] multipath-tools: shutdown, libdevmapper races, globals mwilck
                   ` (16 preceding siblings ...)
  2020-09-24 13:37 ` [PATCH v2 17/21] libmultipath: add udev and logsink symbols mwilck
@ 2020-09-24 13:37 ` mwilck
  2020-09-24 13:37 ` [PATCH v2 19/21] libmpathpersist: call libmultipath_{init, exit}() mwilck
                   ` (2 subsequent siblings)
  20 siblings, 0 replies; 35+ messages in thread
From: mwilck @ 2020-09-24 13:37 UTC (permalink / raw)
  To: Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel, Martin Wilck

From: Martin Wilck <mwilck@suse.com>

We can use libmultipath's symbols now.

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipath/main.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/multipath/main.c b/multipath/main.c
index 4bbfce9..9ae46ed 100644
--- a/multipath/main.c
+++ b/multipath/main.c
@@ -66,9 +66,6 @@
 #include "valid.h"
 #include "alias.h"
 
-int logsink;
-struct udev *udev;
-
 /*
  * Return values of configure(), check_path_valid(), and main().
  */
@@ -810,7 +807,7 @@ main (int argc, char *argv[])
 	int retries = -1;
 	bool enable_foreign = false;
 
-	udev = udev_new();
+	libmultipath_init();
 	logsink = 0;
 	if (init_config(DEFAULT_CONFIGFILE))
 		exit(RTVL_FAIL);
@@ -1068,7 +1065,7 @@ out_free_config:
 	 */
 	put_multipath_config(conf);
 	uninit_config();
-	udev_unref(udev);
+	libmultipath_exit();
 	if (dev)
 		FREE(dev);
 #ifdef _DEBUG_
-- 
2.28.0

^ permalink raw reply related	[flat|nested] 35+ messages in thread

* [PATCH v2 19/21] libmpathpersist: call libmultipath_{init, exit}()
  2020-09-24 13:36 [PATCH v2 00/21] multipath-tools: shutdown, libdevmapper races, globals mwilck
                   ` (17 preceding siblings ...)
  2020-09-24 13:37 ` [PATCH v2 18/21] multipath: remove logsink and udev mwilck
@ 2020-09-24 13:37 ` mwilck
  2020-09-25 23:55   ` Benjamin Marzinski
  2020-09-24 13:37 ` [PATCH v2 20/21] mpathpersist: remove logsink and udev mwilck
  2020-09-24 13:37 ` [PATCH v2 21/21] multipathd: " mwilck
  20 siblings, 1 reply; 35+ messages in thread
From: mwilck @ 2020-09-24 13:37 UTC (permalink / raw)
  To: Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel, Martin Wilck

From: Martin Wilck <mwilck@suse.com>

Have libmpathpersist_{init,exit} do the udev initialization, too.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmpathpersist/mpath_persist.c | 11 ++++++++---
 libmpathpersist/mpath_persist.h |  9 ++++++---
 2 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/libmpathpersist/mpath_persist.c b/libmpathpersist/mpath_persist.c
index a529a38..873b419 100644
--- a/libmpathpersist/mpath_persist.c
+++ b/libmpathpersist/mpath_persist.c
@@ -48,6 +48,10 @@ int libmpathpersist_init(void)
 	struct config *conf;
 	int rc = 0;
 
+	if (libmultipath_init()) {
+		condlog(0, "Failed to initialize libmultipath.");
+		return 1;
+	}
 	if (init_config(DEFAULT_CONFIGFILE)) {
 		condlog(0, "Failed to initialize multipath config.");
 		return 1;
@@ -74,23 +78,24 @@ mpath_lib_init (void)
 
 static void libmpathpersist_cleanup(void)
 {
-	dm_lib_exit();
 	cleanup_prio();
 	cleanup_checkers();
+	libmultipath_exit();
+	dm_lib_exit();
 }
 
 int
 mpath_lib_exit (struct config *conf)
 {
-	libmpathpersist_cleanup();
 	free_config(conf);
+	libmpathpersist_cleanup();
 	return 0;
 }
 
 int libmpathpersist_exit(void)
 {
-	libmpathpersist_cleanup();
 	uninit_config();
+	libmpathpersist_cleanup();
 	return 0;
 }
 
diff --git a/libmpathpersist/mpath_persist.h b/libmpathpersist/mpath_persist.h
index 91606ef..5435eae 100644
--- a/libmpathpersist/mpath_persist.h
+++ b/libmpathpersist/mpath_persist.h
@@ -176,7 +176,8 @@ struct prout_param_descriptor {		/* PROUT parameter descriptor */
  *	Initialize device mapper multipath configuration. This function must be invoked first
  *	before performing reservation management functions.
  *	Either this function or mpath_lib_init() may be used.
- *	Use this function to work with libmultipath's internal "struct config".
+ *	Use this function to work with libmultipath's internal "struct config"
+ *	and "struct udev". The latter will be initialized automatically.
  *	Call libmpathpersist_exit() for cleanup.
  * RESTRICTIONS:
  *
@@ -189,7 +190,8 @@ extern int libmpathpersist_init (void);
  *	Initialize device mapper multipath configuration. This function must be invoked first
  *	before performing reservation management functions.
  *	Either this function or libmpathpersist_init() may be used.
- *	Use this function to work with an application-specific "struct config".
+ *	Use this function to work with an application-specific "struct config"
+ *	and "struct udev". The latter must be initialized by the application.
  *	Call mpath_lib_exit() for cleanup.
  * RESTRICTIONS:
  *
@@ -211,9 +213,10 @@ extern int mpath_lib_exit (struct config *conf);
 
 /*
  * DESCRIPTION :
- *	Release device mapper multipath configuration. This function must be invoked after
+ *	Release device mapper multipath configuration a. This function must be invoked after
  *	performing reservation management functions.
  *	Use this after initialization with libmpathpersist_init().
+ *	Calling libmpathpersist_init() after libmpathpersist_exit() will fail.
  * RESTRICTIONS:
  *
  * RETURNS: 0->Success, 1->Failed.
-- 
2.28.0

^ permalink raw reply related	[flat|nested] 35+ messages in thread

* [PATCH v2 20/21] mpathpersist: remove logsink and udev
  2020-09-24 13:36 [PATCH v2 00/21] multipath-tools: shutdown, libdevmapper races, globals mwilck
                   ` (18 preceding siblings ...)
  2020-09-24 13:37 ` [PATCH v2 19/21] libmpathpersist: call libmultipath_{init, exit}() mwilck
@ 2020-09-24 13:37 ` mwilck
  2020-09-25 23:56   ` Benjamin Marzinski
  2020-09-24 13:37 ` [PATCH v2 21/21] multipathd: " mwilck
  20 siblings, 1 reply; 35+ messages in thread
From: mwilck @ 2020-09-24 13:37 UTC (permalink / raw)
  To: Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel, Martin Wilck

From: Martin Wilck <mwilck@suse.com>

We can use libmultipath's internal symbols now. The libmultipath
initialization is taken care of by libmpathpersist_init().

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 mpathpersist/main.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/mpathpersist/main.c b/mpathpersist/main.c
index 278e48f..3c2e657 100644
--- a/mpathpersist/main.c
+++ b/mpathpersist/main.c
@@ -42,13 +42,10 @@ void * mpath_alloc_prin_response(int prin_sa);
 void mpath_print_transport_id(struct prin_fulldescr *fdesc);
 int construct_transportid(const char * inp, struct transportid transid[], int num_transportids);
 
-int logsink;
-
 void rcu_register_thread_memb(void) {}
 
 void rcu_unregister_thread_memb(void) {}
 
-struct udev *udev;
 
 static int verbose, loglevel, noisy;
 
@@ -641,16 +638,13 @@ int main(int argc, char *argv[])
 		exit (1);
 	}
 
-	udev = udev_new();
 	if (libmpathpersist_init()) {
-		udev_unref(udev);
 		exit(1);
 	}
 
 	ret = handle_args(argc, argv, 0);
 
 	libmpathpersist_exit();
-	udev_unref(udev);
 
 	return (ret >= 0) ? ret : MPATH_PR_OTHER;
 }
-- 
2.28.0

^ permalink raw reply related	[flat|nested] 35+ messages in thread

* [PATCH v2 21/21] multipathd: remove logsink and udev
  2020-09-24 13:36 [PATCH v2 00/21] multipath-tools: shutdown, libdevmapper races, globals mwilck
                   ` (19 preceding siblings ...)
  2020-09-24 13:37 ` [PATCH v2 20/21] mpathpersist: remove logsink and udev mwilck
@ 2020-09-24 13:37 ` mwilck
  2020-09-26  0:19   ` Benjamin Marzinski
  20 siblings, 1 reply; 35+ messages in thread
From: mwilck @ 2020-09-24 13:37 UTC (permalink / raw)
  To: Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel, Martin Wilck

From: Martin Wilck <mwilck@suse.com>

We can use the symbols from libmultipath now.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipathd/main.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index 00b66ba..c5c374b 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -115,7 +115,6 @@ struct mpath_event_param
 	struct multipath *mpp;
 };
 
-int logsink;
 int uxsock_timeout;
 int verbosity;
 int bindings_read_only;
@@ -151,8 +150,6 @@ int should_exit(void)
  */
 struct vectors * gvecs;
 
-struct udev * udev;
-
 struct config *multipath_conf;
 
 /* Local variables */
@@ -3123,8 +3120,6 @@ child (__attribute__((unused)) void *param)
 	conf = rcu_dereference(multipath_conf);
 	rcu_assign_pointer(multipath_conf, NULL);
 	call_rcu(&conf->rcu, rcu_free_config);
-	udev_unref(udev);
-	udev = NULL;
 	pthread_attr_destroy(&waiter_attr);
 	pthread_attr_destroy(&io_err_stat_attr);
 #ifdef _DEBUG_
@@ -3228,7 +3223,9 @@ main (int argc, char *argv[])
 
 	pthread_cond_init_mono(&config_cond);
 
-	udev = udev_new();
+	libmultipath_init();
+	if (atexit(libmultipath_exit))
+		condlog(3, "failed to register exit handler for libmultipath");
 	libmp_udev_set_sync_support(0);
 
 	while ((arg = getopt(argc, argv, ":dsv:k::Bniw")) != EOF ) {
-- 
2.28.0

^ permalink raw reply related	[flat|nested] 35+ messages in thread

* Re: [PATCH v2 13/21] libmultipath: provide defaults for {get, put}_multipath_config
  2020-09-24 13:37 ` [PATCH v2 13/21] libmultipath: provide defaults for {get, put}_multipath_config mwilck
@ 2020-09-25  4:34   ` Benjamin Marzinski
  2020-09-25 20:00     ` Martin Wilck
  0 siblings, 1 reply; 35+ messages in thread
From: Benjamin Marzinski @ 2020-09-25  4:34 UTC (permalink / raw)
  To: mwilck; +Cc: dm-devel

On Thu, Sep 24, 2020 at 03:37:08PM +0200, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> Add an implementation of get_multipath_config() and put_multipath_config()
> to libmultipath. The linker's symbol lookup rules will make sure that
> applications can override these functions if they need to. Defining
> these functions in libmultipath avoids the need to provide stubs
> for these functions in every appliation linking to libmultipath.
> 
> libmultipath's internal functions simply refer to a static "struct config".
> It must be initialized with init_config() rather than load_config(),
> which always allocates a new struct for backward compatibility, and must
> be teared down using uninit_config().
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/config.c             | 75 ++++++++++++++++++++++++++-----
>  libmultipath/config.h             | 21 +++++++--
>  libmultipath/libmultipath.version | 10 +++++
>  3 files changed, 93 insertions(+), 13 deletions(-)
> 
> diff --git a/libmultipath/config.c b/libmultipath/config.c
> index 5c91a09..01b77df 100644
> --- a/libmultipath/config.c
> +++ b/libmultipath/config.c
> @@ -27,6 +27,26 @@
>  #include "mpath_cmd.h"
>  #include "propsel.h"
>  
> +static struct config __internal_config;
> +struct config *libmp_get_multipath_config(void)

This causes problems with the libmpathvalid library code I wrote.  The
issue is that right now, when you run _init_config() if
get_multipath_config() returns NULL, you will use the default loglevel.
I would like the library user to have control of the log level, even
during the calls to _init_config().

One possiblity would be to make init_config() take verbosity as an
argument.  There would also need to be some other config variable that
gets set at the start of init_config(), which is used by
libmp_get_multipath_config() to check if it is initialized.

-Ben

> +{
> +	if (!__internal_config.hwtable)
> +		/* not initialized */
> +		return NULL;
> +	return &__internal_config;
> +}
> +
> +struct config *get_multipath_config(void)
> +	__attribute__((weak, alias("libmp_get_multipath_config")));
> +
> +void libmp_put_multipath_config(void *conf __attribute__((unused)))
> +{
> +	/* empty */
> +}
> +
> +void put_multipath_config(void *conf)
> +	__attribute__((weak, alias("libmp_put_multipath_config")));
> +
>  static int
>  hwe_strmatch (const struct hwentry *hwe1, const struct hwentry *hwe2)
>  {
> @@ -574,17 +594,15 @@ restart:
>  	return;
>  }
>  
> -struct config *
> -alloc_config (void)
> +static struct config *alloc_config (void)
>  {
>  	return (struct config *)MALLOC(sizeof(struct config));
>  }
>  
> -void
> -free_config (struct config * conf)
> +static void _uninit_config(struct config *conf)
>  {
>  	if (!conf)
> -		return;
> +		conf = &__internal_config;
>  
>  	if (conf->multipath_dir)
>  		FREE(conf->multipath_dir);
> @@ -650,7 +668,27 @@ free_config (struct config * conf)
>  	free_hwtable(conf->hwtable);
>  	free_hwe(conf->overrides);
>  	free_keywords(conf->keywords);
> -	FREE(conf);
> +
> +	memset(conf, 0, sizeof(*conf));
> +}
> +
> +void uninit_config(void)

I didn't realize that this was o.k. I thought you had to specify static
in the function definition, even if it was specified in the protype.

> +{
> +	_uninit_config(&__internal_config);
> +}
> +
> +void free_config(struct config *conf)
> +{
> +	if (!conf)
> +		return;
> +	else if (conf == &__internal_config) {
> +		condlog(0, "ERROR: %s called for internal config. Use uninit_config() instead",
> +			__func__);
> +		return;
> +	}
> +
> +	_uninit_config(conf);
> +	free(conf);
>  }
>  
>  /* if multipath fails to process the config directory, it should continue,
> @@ -719,12 +757,29 @@ static void set_max_checkint_from_watchdog(struct config *conf)
>  }
>  #endif
>  
> +static int _init_config (const char *file, struct config *conf);
> +
> +int init_config(const char *file)
> +{
> +	return _init_config(file, &__internal_config);
> +}
> +
>  struct config *load_config(const char *file)
>  {
>  	struct config *conf = alloc_config();
>  
> +	if (conf && !_init_config(file, conf))
> +		return conf;
> +
> +	free(conf);
> +	return NULL;
> +}
> +
> +int _init_config (const char *file, struct config *conf)
> +{
> +
>  	if (!conf)
> -		return NULL;
> +		conf = &__internal_config;
>  
>  	/*
>  	 * internal defaults
> @@ -896,10 +951,10 @@ struct config *load_config(const char *file)
>  	    !conf->wwids_file || !conf->prkeys_file)
>  		goto out;
>  
> -	return conf;
> +	return 0;
>  out:
> -	free_config(conf);
> -	return NULL;
> +	_uninit_config(conf);
> +	return 1;
>  }
>  
>  char *get_uid_attribute_by_attrs(struct config *conf,
> diff --git a/libmultipath/config.h b/libmultipath/config.h
> index 116fe37..5997b71 100644
> --- a/libmultipath/config.h
> +++ b/libmultipath/config.h
> @@ -251,10 +251,25 @@ void free_mptable (vector mptable);
>  int store_hwe (vector hwtable, struct hwentry *);
>  
>  struct config *load_config (const char *file);
> -struct config * alloc_config (void);
>  void free_config (struct config * conf);
> -extern struct config *get_multipath_config(void);
> -extern void put_multipath_config(void *);
> +int init_config(const char *file);
> +void uninit_config(void);
> +
> +/*
> + * libmultipath provides default implementations of
> + * get_multipath_config() and put_multipath_config().
> + * Applications using these should use init_config(file, NULL)
> + * to load the configuration, rather than load_config(file).
> + * Likewise, uninit_config() should be used for teardown, but
> + * using free_config() for that is supported, too.
> + * Applications can define their own {get,put}_multipath_config()
> + * functions, which override the library-internal ones, but
> + * could still call libmp_{get,put}_multipath_config().
> + */
> +struct config *libmp_get_multipath_config(void);
> +struct config *get_multipath_config(void);
> +void libmp_put_multipath_config(void *);
> +void put_multipath_config(void *);
>  
>  int parse_uid_attrs(char *uid_attrs, struct config *conf);
>  char *get_uid_attribute_by_attrs(struct config *conf,
> diff --git a/libmultipath/libmultipath.version b/libmultipath/libmultipath.version
> index 0a96010..81bcc9d 100644
> --- a/libmultipath/libmultipath.version
> +++ b/libmultipath/libmultipath.version
> @@ -218,3 +218,13 @@ global:
>  	libmp_dm_task_run;
>  	cleanup_mutex;
>  } LIBMULTIPATH_0.8.4.1;
> +
> +LIBMULTIPATH_0.8.4.3 {
> +global:
> +	libmp_get_multipath_config;
> +	get_multipath_config;
> +	libmp_put_multipath_config;
> +	put_multipath_config;
> +	init_config;
> +	uninit_config;
> +} LIBMULTIPATH_0.8.4.2;
> -- 
> 2.28.0

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v2 13/21] libmultipath: provide defaults for {get, put}_multipath_config
  2020-09-25  4:34   ` Benjamin Marzinski
@ 2020-09-25 20:00     ` Martin Wilck
  2020-09-25 21:57       ` Benjamin Marzinski
  0 siblings, 1 reply; 35+ messages in thread
From: Martin Wilck @ 2020-09-25 20:00 UTC (permalink / raw)
  To: Benjamin Marzinski; +Cc: dm-devel

On Thu, 2020-09-24 at 23:34 -0500, Benjamin Marzinski wrote:
> 
> This causes problems with the libmpathvalid library code I
> wrote.  The
> issue is that right now, when you run _init_config() if
> get_multipath_config() returns NULL, you will use the default
> loglevel.
> I would like the library user to have control of the log level, even
> during the calls to _init_config().

I see. So using init_config() actually had a benefit for you already
over load_config() :-) Such control over the verbosity would actually
be good for multipath-tools, too.

> One possiblity would be to make init_config() take verbosity as an
> argument.  There would also need to be some other config variable
> that
> gets set at the start of init_config(), which is used by
> libmp_get_multipath_config() to check if it is initialized.

I suggest to track the verbosity independently in a different variable,
and just set it from init_config() if it was actually set in the
config file. Most of the time, we set it from the command line.
This would have the additional benefit not to have to call
get_multipath_config() in dlog().

Martin

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v2 13/21] libmultipath: provide defaults for {get, put}_multipath_config
  2020-09-25 20:00     ` Martin Wilck
@ 2020-09-25 21:57       ` Benjamin Marzinski
  2020-09-26  9:43         ` Martin Wilck
  0 siblings, 1 reply; 35+ messages in thread
From: Benjamin Marzinski @ 2020-09-25 21:57 UTC (permalink / raw)
  To: Martin Wilck; +Cc: dm-devel

On Fri, Sep 25, 2020 at 10:00:10PM +0200, Martin Wilck wrote:
> On Thu, 2020-09-24 at 23:34 -0500, Benjamin Marzinski wrote:
> > 
> > This causes problems with the libmpathvalid library code I
> > wrote.  The
> > issue is that right now, when you run _init_config() if
> > get_multipath_config() returns NULL, you will use the default
> > loglevel.
> > I would like the library user to have control of the log level, even
> > during the calls to _init_config().
> 
> I see. So using init_config() actually had a benefit for you already
> over load_config() :-) Such control over the verbosity would actually
> be good for multipath-tools, too.
> 
> > One possiblity would be to make init_config() take verbosity as an
> > argument.  There would also need to be some other config variable
> > that
> > gets set at the start of init_config(), which is used by
> > libmp_get_multipath_config() to check if it is initialized.
> 
> I suggest to track the verbosity independently in a different variable,
> and just set it from init_config() if it was actually set in the
> config file. Most of the time, we set it from the command line.
> This would have the additional benefit not to have to call
> get_multipath_config() in dlog().

Sure. That sounds like a much simpler solution than what I was
proposing.

-Ben

> 
> Martin
> 

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v2 17/21] libmultipath: add udev and logsink symbols
  2020-09-24 13:37 ` [PATCH v2 17/21] libmultipath: add udev and logsink symbols mwilck
@ 2020-09-25 23:03   ` Benjamin Marzinski
  2020-09-25 23:05     ` Martin Wilck
  0 siblings, 1 reply; 35+ messages in thread
From: Benjamin Marzinski @ 2020-09-25 23:03 UTC (permalink / raw)
  To: mwilck; +Cc: dm-devel

On Thu, Sep 24, 2020 at 03:37:12PM +0200, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> With these symbols added, applications using libmultipath don't
> need to define global variables "udev" and "logsink" any more.
> This comes at the cost of having to call an init function.
> Currently, libmultipath_init() does nothing but initialize
> "udev".
> 
> The linker's symbol lookup order still allows applications to use
> their own "logsink" and "udev" variables, which will take precendence
> over libmultipath's internal ones. In this case, calling
> libmultipath_init() can be skipped, but like before,
> udev should be initialized (using udev_new()) before making any
> libmultipath calls.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/config.c             | 46 +++++++++++++++++++++++++++++++
>  libmultipath/config.h             | 46 ++++++++++++++++++++++++++++++-
>  libmultipath/debug.c              |  2 ++
>  libmultipath/libmultipath.version |  8 ++++++
>  4 files changed, 101 insertions(+), 1 deletion(-)
> 
> diff --git a/libmultipath/config.c b/libmultipath/config.c
> index 01b77df..fbb66b3 100644
> --- a/libmultipath/config.c
> +++ b/libmultipath/config.c
> @@ -27,6 +27,52 @@
>  #include "mpath_cmd.h"
>  #include "propsel.h"
>  
> +/*
> + * We don't support re-initialization after
> + * libmultipath_exit().
> + */
> +static bool libmultipath_exit_called;
> +static pthread_once_t _init_once = PTHREAD_ONCE_INIT;
> +static pthread_once_t _exit_once = PTHREAD_ONCE_INIT;
> +struct udev *udev;
> +
> +static void _udev_init(void)
> +{
> +	if (udev)
> +		udev_ref(udev);
> +	else
> +		udev = udev_new();
> +	if (!udev)
> +		condlog(0, "%s: failed to initialize udev", __func__);
> +}
> +
> +static void _libmultipath_init(void)
> +{
> +	_udev_init();
> +}

I don't understand why we need both _udev_init() and
_libmultipath_init().

-Ben

> +
> +static bool _is_libmultipath_initialized(void)
> +{
> +	return !libmultipath_exit_called && !!udev;
> +}
> +
> +int libmultipath_init(void)
> +{
> +	pthread_once(&_init_once, _libmultipath_init);
> +	return !_is_libmultipath_initialized();
> +}
> +
> +static void _libmultipath_exit(void)
> +{
> +	libmultipath_exit_called = true;
> +	udev_unref(udev);
> +}
> +
> +void libmultipath_exit(void)
> +{
> +	pthread_once(&_exit_once, _libmultipath_exit);
> +}
> +
>  static struct config __internal_config;
>  struct config *libmp_get_multipath_config(void)
>  {
> diff --git a/libmultipath/config.h b/libmultipath/config.h
> index 5997b71..dac4e8f 100644
> --- a/libmultipath/config.h
> +++ b/libmultipath/config.h
> @@ -232,7 +232,51 @@ struct config {
>  	char *enable_foreign;
>  };
>  
> -extern struct udev * udev;
> +/**
> + * extern variable: udev
> + *
> + * A &struct udev instance used by libmultipath. libmultipath expects
> + * a valid, initialized &struct udev in this variable.
> + * An application can define this variable itself, in which case
> + * the applications's instance will take precedence.
> + * The application can initialize and destroy this variable by
> + * calling libmultipath_init() and libmultipath_exit(), respectively,
> + * whether or not it defines the variable itself.
> + * An application can initialize udev with udev_new() before calling
> + * libmultipath_init(), e.g. if it has to make libudev calls before
> + * libmultipath calls. If an application wants to keep using the
> + * udev variable after calling libmultipath_exit(), it should have taken
> + * an additional reference on it beforehand. This is the case e.g.
> + * after initiazing udev with udev_new().
> + */
> +extern struct udev *udev;
> +
> +/**
> + * libmultipath_init() - library initialization
> + *
> + * This function initializes libmultipath data structures.
> + * It is light-weight; some other initializations, like device-mapper
> + * initialization, are done lazily when the respective functionality
> + * is required.
> + *
> + * Clean up by libmultipath_exit() when the program terminates.
> + * It is an error to call libmultipath_init() after libmultipath_exit().
> + * Return: 0 on success, 1 on failure.
> + */
> +int libmultipath_init(void);
> +
> +/**
> + * libmultipath_exit() - library un-initialization
> + *
> + * This function un-initializes libmultipath data structures.
> + * It is recommended to call this function at program exit.
> + *
> + * Calls to libmultipath_init() after libmultipath_exit() will fail
> + * (in other words, libmultipath can't be re-initialized).
> + * Any other libmultipath calls after libmultipath_exit() may cause
> + * undefined behavior.
> + */
> +void libmultipath_exit(void);
>  
>  int find_hwe (const struct _vector *hwtable,
>  	      const char * vendor, const char * product, const char *revision,
> diff --git a/libmultipath/debug.c b/libmultipath/debug.c
> index 4128cb9..b3a1de9 100644
> --- a/libmultipath/debug.c
> +++ b/libmultipath/debug.c
> @@ -15,6 +15,8 @@
>  #include "defaults.h"
>  #include "debug.h"
>  
> +int logsink;
> +
>  void dlog (int sink, int prio, const char * fmt, ...)
>  {
>  	va_list ap;
> diff --git a/libmultipath/libmultipath.version b/libmultipath/libmultipath.version
> index 81bcc9d..2e531ef 100644
> --- a/libmultipath/libmultipath.version
> +++ b/libmultipath/libmultipath.version
> @@ -228,3 +228,11 @@ global:
>  	init_config;
>  	uninit_config;
>  } LIBMULTIPATH_0.8.4.2;
> +
> +LIBMULTIPATH_0.8.4.4 {
> +global:
> +	udev;
> +	logsink;
> +	libmultipath_init;
> +	libmultipath_exit;
> +} LIBMULTIPATH_0.8.4.3;
> -- 
> 2.28.0

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v2 17/21] libmultipath: add udev and logsink symbols
  2020-09-25 23:03   ` Benjamin Marzinski
@ 2020-09-25 23:05     ` Martin Wilck
  0 siblings, 0 replies; 35+ messages in thread
From: Martin Wilck @ 2020-09-25 23:05 UTC (permalink / raw)
  To: Benjamin Marzinski; +Cc: dm-devel

On Fri, 2020-09-25 at 18:03 -0500, Benjamin Marzinski wrote:
> 
> I don't understand why we need both _udev_init() and
> _libmultipath_init().

Just in case we need more initialization in the future, then we could
add _foo_init(). Not important of course, I can change  it back.

Cheers,
Martin

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v2 10/21] libmultipath: devmapper: refactor libdm version determination
  2020-09-24 13:37 ` [PATCH v2 10/21] libmultipath: devmapper: refactor libdm version determination mwilck
@ 2020-09-25 23:48   ` Benjamin Marzinski
  0 siblings, 0 replies; 35+ messages in thread
From: Benjamin Marzinski @ 2020-09-25 23:48 UTC (permalink / raw)
  To: mwilck; +Cc: dm-devel

On Thu, Sep 24, 2020 at 03:37:05PM +0200, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> 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.
> Note that retrieving the versions requires libdm calls, thus the
> version initialization calls libdm initialization, which might call
> version initialization recursively. But that's not an issue, as
> the initialization is protected by pthread_once().

It's not a big deal, but weren't you going to change the commit message
to not talk about a recursive version initialization, since we avoid
that.

-Ben
 
> 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 ABI change as offsets in "struct config" are changed.
> Bump the ABI version.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/config.h             |   1 -
>  libmultipath/devmapper.c          | 160 ++++++++++++++++++++----------
>  libmultipath/devmapper.h          |  11 +-
>  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(+), 67 deletions(-)
> 
> diff --git a/libmultipath/config.h b/libmultipath/config.h
> index 2bb7153..8e13ae9 100644
> --- a/libmultipath/config.h
> +++ b/libmultipath/config.h
> @@ -191,7 +191,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 7394796..08aa09f 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,59 +119,66 @@ 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_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;
> @@ -172,10 +186,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;
>  
> @@ -211,26 +226,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;
>  	}
>  
> @@ -239,13 +253,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)
> @@ -263,7 +324,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 f568ab5..c8b37e1 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);
> @@ -85,6 +82,14 @@ struct multipath *dm_get_multipath(const char *name);
>  	((v[0] == minv[0]) && (v[1] == minv[1]) && (v[2] >= minv[2])) \
>  )
>  
> +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__, \
>  		cmd, strerror(dm_task_get_errno(dmt)))	      \
> diff --git a/libmultipath/libmultipath.version b/libmultipath/libmultipath.version
> index d6ac0e1..5699a0b 100644
> --- a/libmultipath/libmultipath.version
> +++ b/libmultipath/libmultipath.version
> @@ -1,4 +1,4 @@
> -LIBMULTIPATH_0.8.4.0 {
> +LIBMULTIPATH_0.8.4.1 {
>  global:
>  	/* symbols referenced by multipath and multipathd */
>  	add_foreign;
> @@ -32,7 +32,6 @@ global:
>  	disassemble_status;
>  	dlog;
>  	dm_cancel_deferred_remove;
> -	dm_drv_version;
>  	dm_enablegroup;
>  	dm_fail_path;
>  	_dm_flush_map;
> @@ -54,7 +53,6 @@ global:
>  	dm_reinstate_path;
>  	dm_simplecmd_noflush;
>  	dm_switchgroup;
> -	dm_tgt_version;
>  	domap;
>  	ensure_directories_exist;
>  	extract_hwe_from_path;
> @@ -95,6 +93,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 4020134..3f2c2cf 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 5f2d210..fc97c8a 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 5cc3435..00b66ba 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 47e6b86..a681c11 100644
> --- a/tests/Makefile
> +++ b/tests/Makefile
> @@ -42,7 +42,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 12660da..57f832b 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 b7c09cc..e7663f9 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.28.0

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v2 19/21] libmpathpersist: call libmultipath_{init, exit}()
  2020-09-24 13:37 ` [PATCH v2 19/21] libmpathpersist: call libmultipath_{init, exit}() mwilck
@ 2020-09-25 23:55   ` Benjamin Marzinski
  0 siblings, 0 replies; 35+ messages in thread
From: Benjamin Marzinski @ 2020-09-25 23:55 UTC (permalink / raw)
  To: mwilck; +Cc: dm-devel

On Thu, Sep 24, 2020 at 03:37:14PM +0200, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> Have libmpathpersist_{init,exit} do the udev initialization, too.
> 
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmpathpersist/mpath_persist.c | 11 ++++++++---
>  libmpathpersist/mpath_persist.h |  9 ++++++---
>  2 files changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/libmpathpersist/mpath_persist.c b/libmpathpersist/mpath_persist.c
> index a529a38..873b419 100644
> --- a/libmpathpersist/mpath_persist.c
> +++ b/libmpathpersist/mpath_persist.c
> @@ -48,6 +48,10 @@ int libmpathpersist_init(void)
>  	struct config *conf;
>  	int rc = 0;
>  
> +	if (libmultipath_init()) {
> +		condlog(0, "Failed to initialize libmultipath.");
> +		return 1;
> +	}
>  	if (init_config(DEFAULT_CONFIGFILE)) {
>  		condlog(0, "Failed to initialize multipath config.");
>  		return 1;
> @@ -74,23 +78,24 @@ mpath_lib_init (void)
>  
>  static void libmpathpersist_cleanup(void)
>  {
> -	dm_lib_exit();
>  	cleanup_prio();
>  	cleanup_checkers();
> +	libmultipath_exit();
> +	dm_lib_exit();
>  }
>  
>  int
>  mpath_lib_exit (struct config *conf)
>  {
> -	libmpathpersist_cleanup();
>  	free_config(conf);
> +	libmpathpersist_cleanup();
>  	return 0;
>  }
>  
>  int libmpathpersist_exit(void)
>  {
> -	libmpathpersist_cleanup();
>  	uninit_config();
> +	libmpathpersist_cleanup();
>  	return 0;
>  }
>  
> diff --git a/libmpathpersist/mpath_persist.h b/libmpathpersist/mpath_persist.h
> index 91606ef..5435eae 100644
> --- a/libmpathpersist/mpath_persist.h
> +++ b/libmpathpersist/mpath_persist.h
> @@ -176,7 +176,8 @@ struct prout_param_descriptor {		/* PROUT parameter descriptor */
>   *	Initialize device mapper multipath configuration. This function must be invoked first
>   *	before performing reservation management functions.
>   *	Either this function or mpath_lib_init() may be used.
> - *	Use this function to work with libmultipath's internal "struct config".
> + *	Use this function to work with libmultipath's internal "struct config"
> + *	and "struct udev". The latter will be initialized automatically.
>   *	Call libmpathpersist_exit() for cleanup.
>   * RESTRICTIONS:
>   *
> @@ -189,7 +190,8 @@ extern int libmpathpersist_init (void);
>   *	Initialize device mapper multipath configuration. This function must be invoked first
>   *	before performing reservation management functions.
>   *	Either this function or libmpathpersist_init() may be used.
> - *	Use this function to work with an application-specific "struct config".
> + *	Use this function to work with an application-specific "struct config"
> + *	and "struct udev". The latter must be initialized by the application.
>   *	Call mpath_lib_exit() for cleanup.
>   * RESTRICTIONS:
>   *
> @@ -211,9 +213,10 @@ extern int mpath_lib_exit (struct config *conf);
>  
>  /*
>   * DESCRIPTION :
> - *	Release device mapper multipath configuration. This function must be invoked after
> + *	Release device mapper multipath configuration a. This function must be invoked after
>   *	performing reservation management functions.
>   *	Use this after initialization with libmpathpersist_init().
> + *	Calling libmpathpersist_init() after libmpathpersist_exit() will fail.
>   * RESTRICTIONS:
>   *
>   * RETURNS: 0->Success, 1->Failed.
> -- 
> 2.28.0

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v2 20/21] mpathpersist: remove logsink and udev
  2020-09-24 13:37 ` [PATCH v2 20/21] mpathpersist: remove logsink and udev mwilck
@ 2020-09-25 23:56   ` Benjamin Marzinski
  0 siblings, 0 replies; 35+ messages in thread
From: Benjamin Marzinski @ 2020-09-25 23:56 UTC (permalink / raw)
  To: mwilck; +Cc: dm-devel

On Thu, Sep 24, 2020 at 03:37:15PM +0200, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> We can use libmultipath's internal symbols now. The libmultipath
> initialization is taken care of by libmpathpersist_init().
> 
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  mpathpersist/main.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/mpathpersist/main.c b/mpathpersist/main.c
> index 278e48f..3c2e657 100644
> --- a/mpathpersist/main.c
> +++ b/mpathpersist/main.c
> @@ -42,13 +42,10 @@ void * mpath_alloc_prin_response(int prin_sa);
>  void mpath_print_transport_id(struct prin_fulldescr *fdesc);
>  int construct_transportid(const char * inp, struct transportid transid[], int num_transportids);
>  
> -int logsink;
> -
>  void rcu_register_thread_memb(void) {}
>  
>  void rcu_unregister_thread_memb(void) {}
>  
> -struct udev *udev;
>  
>  static int verbose, loglevel, noisy;
>  
> @@ -641,16 +638,13 @@ int main(int argc, char *argv[])
>  		exit (1);
>  	}
>  
> -	udev = udev_new();
>  	if (libmpathpersist_init()) {
> -		udev_unref(udev);
>  		exit(1);
>  	}
>  
>  	ret = handle_args(argc, argv, 0);
>  
>  	libmpathpersist_exit();
> -	udev_unref(udev);
>  
>  	return (ret >= 0) ? ret : MPATH_PR_OTHER;
>  }
> -- 
> 2.28.0

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v2 21/21] multipathd: remove logsink and udev
  2020-09-24 13:37 ` [PATCH v2 21/21] multipathd: " mwilck
@ 2020-09-26  0:19   ` Benjamin Marzinski
  0 siblings, 0 replies; 35+ messages in thread
From: Benjamin Marzinski @ 2020-09-26  0:19 UTC (permalink / raw)
  To: mwilck; +Cc: dm-devel

On Thu, Sep 24, 2020 at 03:37:16PM +0200, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> We can use the symbols from libmultipath now.
> 
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  multipathd/main.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 00b66ba..c5c374b 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -115,7 +115,6 @@ struct mpath_event_param
>  	struct multipath *mpp;
>  };
>  
> -int logsink;
>  int uxsock_timeout;
>  int verbosity;
>  int bindings_read_only;
> @@ -151,8 +150,6 @@ int should_exit(void)
>   */
>  struct vectors * gvecs;
>  
> -struct udev * udev;
> -
>  struct config *multipath_conf;
>  
>  /* Local variables */
> @@ -3123,8 +3120,6 @@ child (__attribute__((unused)) void *param)
>  	conf = rcu_dereference(multipath_conf);
>  	rcu_assign_pointer(multipath_conf, NULL);
>  	call_rcu(&conf->rcu, rcu_free_config);
> -	udev_unref(udev);
> -	udev = NULL;
>  	pthread_attr_destroy(&waiter_attr);
>  	pthread_attr_destroy(&io_err_stat_attr);
>  #ifdef _DEBUG_
> @@ -3228,7 +3223,9 @@ main (int argc, char *argv[])
>  
>  	pthread_cond_init_mono(&config_cond);
>  
> -	udev = udev_new();
> +	libmultipath_init();
> +	if (atexit(libmultipath_exit))
> +		condlog(3, "failed to register exit handler for libmultipath");
>  	libmp_udev_set_sync_support(0);
>  
>  	while ((arg = getopt(argc, argv, ":dsv:k::Bniw")) != EOF ) {
> -- 
> 2.28.0

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v2 13/21] libmultipath: provide defaults for {get, put}_multipath_config
  2020-09-25 21:57       ` Benjamin Marzinski
@ 2020-09-26  9:43         ` Martin Wilck
  2020-09-27  1:03           ` Benjamin Marzinski
  0 siblings, 1 reply; 35+ messages in thread
From: Martin Wilck @ 2020-09-26  9:43 UTC (permalink / raw)
  To: Benjamin Marzinski; +Cc: dm-devel

On Fri, 2020-09-25 at 16:57 -0500, Benjamin Marzinski wrote:
> On Fri, Sep 25, 2020 at 10:00:10PM +0200, Martin Wilck wrote:
> > 
> > I suggest to track the verbosity independently in a different
> > variable,
> > and just set it from init_config() if it was actually set in the
> > config file. Most of the time, we set it from the command line.
> > This would have the additional benefit not to have to call
> > get_multipath_config() in dlog().
> 
> Sure. That sounds like a much simpler solution than what I was
> proposing.

I will do that in a separate patch. It will actually be a short
series with logging fixes. Would you ack this patch then?
In general it makes sense for get_multipath_config() to return NULL if
the struct config is not initialized.

Thanks,
Martin

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v2 13/21] libmultipath: provide defaults for {get, put}_multipath_config
  2020-09-26  9:43         ` Martin Wilck
@ 2020-09-27  1:03           ` Benjamin Marzinski
  0 siblings, 0 replies; 35+ messages in thread
From: Benjamin Marzinski @ 2020-09-27  1:03 UTC (permalink / raw)
  To: Martin Wilck; +Cc: dm-devel

On Sat, Sep 26, 2020 at 11:43:46AM +0200, Martin Wilck wrote:
> On Fri, 2020-09-25 at 16:57 -0500, Benjamin Marzinski wrote:
> > On Fri, Sep 25, 2020 at 10:00:10PM +0200, Martin Wilck wrote:
> > > 
> > > I suggest to track the verbosity independently in a different
> > > variable,
> > > and just set it from init_config() if it was actually set in the
> > > config file. Most of the time, we set it from the command line.
> > > This would have the additional benefit not to have to call
> > > get_multipath_config() in dlog().
> > 
> > Sure. That sounds like a much simpler solution than what I was
> > proposing.
> 
> I will do that in a separate patch. It will actually be a short
> series with logging fixes. Would you ack this patch then?
> In general it makes sense for get_multipath_config() to return NULL if
> the struct config is not initialized.
> 

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


> Thanks,
> Martin
> 

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v2 11/21] libmultipath: protect racy libdevmapper calls with a mutex
  2020-09-24 13:37 ` [PATCH v2 11/21] libmultipath: protect racy libdevmapper calls with a mutex mwilck
@ 2020-09-29  5:57   ` Benjamin Marzinski
  2020-09-29  9:09     ` Martin Wilck
  0 siblings, 1 reply; 35+ messages in thread
From: Benjamin Marzinski @ 2020-09-29  5:57 UTC (permalink / raw)
  To: mwilck; +Cc: lixiaokeng, dm-devel

On Thu, Sep 24, 2020 at 03:37:06PM +0200, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> dm_udev_wait() and dm_task_run() may access global / static state
> in libdevmapper. They need to be protected by a lock in in our
> multithreaded library.
> 

This breaks the dmevents unit tests. dm_task_run is no longer called in
dmevents.c. Intead, its only called in devmapper.c, so this needs to be
in dmevents-test_OBJDEPS

> Cc: lixiaokeng@huawei.com
> Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/devmapper.c          | 73 +++++++++++++++++++------------
>  libmultipath/devmapper.h          |  2 +
>  libmultipath/libmultipath.version |  6 +++
>  libmultipath/util.c               |  5 +++
>  libmultipath/util.h               |  1 +
>  multipathd/dmevents.c             |  2 +-
>  multipathd/waiter.c               |  2 +-
>  7 files changed, 62 insertions(+), 29 deletions(-)
> 
> diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
> index 08aa09f..2e3ae8a 100644
> --- a/libmultipath/devmapper.c
> +++ b/libmultipath/devmapper.c
> @@ -42,6 +42,7 @@ 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 pthread_mutex_t libmp_dm_lock = PTHREAD_MUTEX_INITIALIZER;
>  
>  static int dm_conf_verbosity;
>  
> @@ -59,16 +60,34 @@ static inline int dm_task_set_cookie(struct dm_task *dmt, uint32_t *c, int a)
>  	return 1;
>  }
>  
> -void dm_udev_wait(unsigned int c)
> +static void libmp_udev_wait(unsigned int c)
>  {
>  }
>  
> -void dm_udev_set_sync_support(int c)
> +static void dm_udev_set_sync_support(int c)
>  {
>  }
> -
> +#else
> +static void libmp_udev_wait(unsigned int c)
> +{
> +	pthread_mutex_lock(&libmp_dm_lock);
> +	pthread_cleanup_push(cleanup_mutex, &libmp_dm_lock);
> +	dm_udev_wait(c);
> +	pthread_cleanup_pop(1);
> +}
>  #endif
>  
> +int libmp_dm_task_run(struct dm_task *dmt)
> +{
> +	int r;
> +
> +	pthread_mutex_lock(&libmp_dm_lock);
> +	pthread_cleanup_push(cleanup_mutex, &libmp_dm_lock);
> +	r = dm_task_run(dmt);
> +	pthread_cleanup_pop(1);
> +	return r;
> +}
> +
>  __attribute__((format(printf, 4, 5))) static void
>  dm_write_log (int level, const char *file, int line, const char *f, ...)
>  {
> @@ -196,7 +215,7 @@ static int dm_tgt_version (unsigned int *version, char *str)
>  
>  	dm_task_no_open_count(dmt);
>  
> -	if (!dm_task_run(dmt)) {
> +	if (!libmp_dm_task_run(dmt)) {
>  		dm_log_error(2, DM_DEVICE_LIST_VERSIONS, dmt);
>  		condlog(0, "Can not communicate with kernel DM");
>  		goto out;
> @@ -380,12 +399,12 @@ dm_simplecmd (int task, const char *name, int no_flush, int need_sync, uint16_t
>  				DM_UDEV_DISABLE_LIBRARY_FALLBACK | udev_flags))
>  		goto out;
>  
> -	r = dm_task_run (dmt);
> +	r = libmp_dm_task_run (dmt);
>  	if (!r)
>  		dm_log_error(2, task, dmt);
>  
>  	if (udev_wait_flag)
> -			dm_udev_wait(cookie);
> +			libmp_udev_wait(cookie);
>  out:
>  	dm_task_destroy (dmt);
>  	return r;
> @@ -472,12 +491,12 @@ dm_addmap (int task, const char *target, struct multipath *mpp,
>  	    !dm_task_set_cookie(dmt, &cookie, udev_flags))
>  		goto freeout;
>  
> -	r = dm_task_run (dmt);
> +	r = libmp_dm_task_run (dmt);
>  	if (!r)
>  		dm_log_error(2, task, dmt);
>  
>  	if (task == DM_DEVICE_CREATE)
> -			dm_udev_wait(cookie);
> +			libmp_udev_wait(cookie);
>  freeout:
>  	if (prefixed_uuid)
>  		FREE(prefixed_uuid);
> @@ -587,7 +606,7 @@ do_get_info(const char *name, struct dm_info *info)
>  
>  	dm_task_no_open_count(dmt);
>  
> -	if (!dm_task_run(dmt)) {
> +	if (!libmp_dm_task_run(dmt)) {
>  		dm_log_error(3, DM_DEVICE_INFO, dmt);
>  		goto out;
>  	}
> @@ -628,7 +647,7 @@ int dm_get_map(const char *name, unsigned long long *size, char *outparams)
>  	dm_task_no_open_count(dmt);
>  
>  	errno = 0;
> -	if (!dm_task_run(dmt)) {
> +	if (!libmp_dm_task_run(dmt)) {
>  		dm_log_error(3, DM_DEVICE_TABLE, dmt);
>  		if (dm_task_get_errno(dmt) == ENXIO)
>  			r = DMP_NOT_FOUND;
> @@ -670,7 +689,7 @@ dm_get_prefixed_uuid(const char *name, char *uuid, int uuid_len)
>  	if (!dm_task_set_name (dmt, name))
>  		goto uuidout;
>  
> -	if (!dm_task_run(dmt)) {
> +	if (!libmp_dm_task_run(dmt)) {
>  		dm_log_error(3, DM_DEVICE_INFO, dmt);
>  		goto uuidout;
>  	}
> @@ -741,7 +760,7 @@ int dm_get_status(const char *name, char *outstatus)
>  	dm_task_no_open_count(dmt);
>  
>  	errno = 0;
> -	if (!dm_task_run(dmt)) {
> +	if (!libmp_dm_task_run(dmt)) {
>  		dm_log_error(3, DM_DEVICE_STATUS, dmt);
>  		if (dm_task_get_errno(dmt) == ENXIO)
>  			r = DMP_NOT_FOUND;
> @@ -794,7 +813,7 @@ int dm_type(const char *name, char *type)
>  
>  	dm_task_no_open_count(dmt);
>  
> -	if (!dm_task_run(dmt)) {
> +	if (!libmp_dm_task_run(dmt)) {
>  		dm_log_error(3, DM_DEVICE_TABLE, dmt);
>  		goto out;
>  	}
> @@ -838,7 +857,7 @@ int dm_is_mpath(const char *name)
>  
>  	dm_task_no_open_count(dmt);
>  
> -	if (!dm_task_run(dmt)) {
> +	if (!libmp_dm_task_run(dmt)) {
>  		dm_log_error(3, DM_DEVICE_TABLE, dmt);
>  		goto out_task;
>  	}
> @@ -902,7 +921,7 @@ dm_map_present_by_uuid(const char *uuid)
>  	if (!dm_task_set_uuid(dmt, prefixed_uuid))
>  		goto out_task;
>  
> -	if (!dm_task_run(dmt)) {
> +	if (!libmp_dm_task_run(dmt)) {
>  		dm_log_error(3, DM_DEVICE_INFO, dmt);
>  		goto out_task;
>  	}
> @@ -948,7 +967,7 @@ dm_get_opencount (const char * mapname)
>  	if (!dm_task_set_name(dmt, mapname))
>  		goto out;
>  
> -	if (!dm_task_run(dmt)) {
> +	if (!libmp_dm_task_run(dmt)) {
>  		dm_log_error(3, DM_DEVICE_INFO, dmt);
>  		goto out;
>  	}
> @@ -1108,7 +1127,7 @@ int dm_flush_maps (int need_suspend, int retries)
>  
>  	dm_task_no_open_count(dmt);
>  
> -	if (!dm_task_run (dmt)) {
> +	if (!libmp_dm_task_run (dmt)) {
>  		dm_log_error(3, DM_DEVICE_LIST, dmt);
>  		goto out;
>  	}
> @@ -1154,7 +1173,7 @@ dm_message(const char * mapname, char * message)
>  
>  	dm_task_no_open_count(dmt);
>  
> -	if (!dm_task_run(dmt)) {
> +	if (!libmp_dm_task_run(dmt)) {
>  		dm_log_error(2, DM_DEVICE_TARGET_MSG, dmt);
>  		goto out;
>  	}
> @@ -1274,7 +1293,7 @@ dm_get_maps (vector mp)
>  
>  	dm_task_no_open_count(dmt);
>  
> -	if (!dm_task_run(dmt)) {
> +	if (!libmp_dm_task_run(dmt)) {
>  		dm_log_error(3, DM_DEVICE_LIST, dmt);
>  		goto out;
>  	}
> @@ -1359,7 +1378,7 @@ dm_mapname(int major, int minor)
>  	 * daemon uev_trigger -> uev_add_map
>  	 */
>  	while (--loop) {
> -		r = dm_task_run(dmt);
> +		r = libmp_dm_task_run(dmt);
>  
>  		if (r)
>  			break;
> @@ -1404,7 +1423,7 @@ do_foreach_partmaps (const char * mapname,
>  
>  	dm_task_no_open_count(dmt);
>  
> -	if (!dm_task_run(dmt)) {
> +	if (!libmp_dm_task_run(dmt)) {
>  		dm_log_error(3, DM_DEVICE_LIST, dmt);
>  		goto out;
>  	}
> @@ -1639,11 +1658,11 @@ dm_rename (const char * old, char * new, char *delim, int skip_kpartx)
>  
>  	if (!dm_task_set_cookie(dmt, &cookie, udev_flags))
>  		goto out;
> -	r = dm_task_run(dmt);
> +	r = libmp_dm_task_run(dmt);
>  	if (!r)
>  		dm_log_error(2, DM_DEVICE_RENAME, dmt);
>  
> -	dm_udev_wait(cookie);
> +	libmp_udev_wait(cookie);
>  
>  out:
>  	dm_task_destroy(dmt);
> @@ -1685,7 +1704,7 @@ int dm_reassign_table(const char *name, char *old, char *new)
>  
>  	dm_task_no_open_count(dmt);
>  
> -	if (!dm_task_run(dmt)) {
> +	if (!libmp_dm_task_run(dmt)) {
>  		dm_log_error(3, DM_DEVICE_TABLE, dmt);
>  		goto out;
>  	}
> @@ -1718,7 +1737,7 @@ int dm_reassign_table(const char *name, char *old, char *new)
>  	if (modified) {
>  		dm_task_no_open_count(reload_dmt);
>  
> -		if (!dm_task_run(reload_dmt)) {
> +		if (!libmp_dm_task_run(reload_dmt)) {
>  			dm_log_error(3, DM_DEVICE_RELOAD, reload_dmt);
>  			condlog(3, "%s: failed to reassign targets", name);
>  			goto out_reload;
> @@ -1765,7 +1784,7 @@ int dm_reassign(const char *mapname)
>  
>  	dm_task_no_open_count(dmt);
>  
> -	if (!dm_task_run(dmt)) {
> +	if (!libmp_dm_task_run(dmt)) {
>  		dm_log_error(3, DM_DEVICE_DEPS, dmt);
>  		goto out;
>  	}
> @@ -1833,7 +1852,7 @@ int dm_setgeometry(struct multipath *mpp)
>  		goto out;
>  	}
>  
> -	r = dm_task_run(dmt);
> +	r = libmp_dm_task_run(dmt);
>  	if (!r)
>  		dm_log_error(3, DM_DEVICE_SET_GEOMETRY, dmt);
>  out:
> diff --git a/libmultipath/devmapper.h b/libmultipath/devmapper.h
> index c8b37e1..158057e 100644
> --- a/libmultipath/devmapper.h
> +++ b/libmultipath/devmapper.h
> @@ -89,6 +89,8 @@ enum {
>  	MULTIPATH_VERSION
>  };
>  int libmp_get_version(int which, unsigned int version[3]);
> +struct dm_task;
> +int libmp_dm_task_run(struct dm_task *dmt);
>  
>  #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 5699a0b..0a96010 100644
> --- a/libmultipath/libmultipath.version
> +++ b/libmultipath/libmultipath.version
> @@ -212,3 +212,9 @@ global:
>  local:
>  	*;
>  };
> +
> +LIBMULTIPATH_0.8.4.2 {
> +global:
> +	libmp_dm_task_run;
> +	cleanup_mutex;
> +} LIBMULTIPATH_0.8.4.1;
> diff --git a/libmultipath/util.c b/libmultipath/util.c
> index 66cd721..41da6ce 100644
> --- a/libmultipath/util.c
> +++ b/libmultipath/util.c
> @@ -409,6 +409,11 @@ void cleanup_free_ptr(void *arg)
>  		free(*p);
>  }
>  
> +void cleanup_mutex(void *arg)
> +{
> +	pthread_mutex_unlock(arg);
> +}
> +
>  struct bitfield *alloc_bitfield(unsigned int maxbit)
>  {
>  	unsigned int n;
> diff --git a/libmultipath/util.h b/libmultipath/util.h
> index c1ae878..b8481af 100644
> --- a/libmultipath/util.h
> +++ b/libmultipath/util.h
> @@ -49,6 +49,7 @@ int should_exit(void);
>  
>  void close_fd(void *arg);
>  void cleanup_free_ptr(void *arg);
> +void cleanup_mutex(void *arg);
>  
>  struct scandir_result {
>  	struct dirent **di;
> diff --git a/multipathd/dmevents.c b/multipathd/dmevents.c
> index fc97c8a..b561cbf 100644
> --- a/multipathd/dmevents.c
> +++ b/multipathd/dmevents.c
> @@ -156,7 +156,7 @@ static int dm_get_events(void)
>  
>  	dm_task_no_open_count(dmt);
>  
> -	if (!dm_task_run(dmt)) {
> +	if (!libmp_dm_task_run(dmt)) {
>  		dm_log_error(3, DM_DEVICE_LIST, dmt);
>  		goto fail;
>  	}
> diff --git a/multipathd/waiter.c b/multipathd/waiter.c
> index 16fbdc8..620bfa7 100644
> --- a/multipathd/waiter.c
> +++ b/multipathd/waiter.c
> @@ -118,7 +118,7 @@ static int waiteventloop (struct event_thread *waiter)
>  	pthread_sigmask(SIG_UNBLOCK, &set, &oldset);
>  
>  	pthread_testcancel();
> -	r = dm_task_run(waiter->dmt);
> +	r = libmp_dm_task_run(waiter->dmt);
>  	if (!r)
>  		dm_log_error(2, DM_DEVICE_WAITEVENT, waiter->dmt);
>  	pthread_testcancel();
> -- 
> 2.28.0

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v2 11/21] libmultipath: protect racy libdevmapper calls with a mutex
  2020-09-29  5:57   ` Benjamin Marzinski
@ 2020-09-29  9:09     ` Martin Wilck
  0 siblings, 0 replies; 35+ messages in thread
From: Martin Wilck @ 2020-09-29  9:09 UTC (permalink / raw)
  To: Benjamin Marzinski; +Cc: lixiaokeng, dm-devel

On Tue, 2020-09-29 at 00:57 -0500, Benjamin Marzinski wrote:
> On Thu, Sep 24, 2020 at 03:37:06PM +0200, mwilck@suse.com wrote:
> > From: Martin Wilck <mwilck@suse.com>
> > 
> > dm_udev_wait() and dm_task_run() may access global / static state
> > in libdevmapper. They need to be protected by a lock in in our
> > multithreaded library.
> > 
> 
> This breaks the dmevents unit tests. dm_task_run is no longer called
> in
> dmevents.c. Intead, its only called in devmapper.c, so this needs to
> be
> in dmevents-test_OBJDEPS

Argh, sorry. We need to figure out a way to run this test routinely.
It's the only one I don't run after every patch, simply because it
needs root. Anyway, no excuse. Sorry. I'll fix it up.

Martin

^ permalink raw reply	[flat|nested] 35+ messages in thread

end of thread, other threads:[~2020-09-29  9:09 UTC | newest]

Thread overview: 35+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-24 13:36 [PATCH v2 00/21] multipath-tools: shutdown, libdevmapper races, globals mwilck
2020-09-24 13:36 ` [PATCH v2 01/21] multipathd: allow shutdown during configure() mwilck
2020-09-24 13:36 ` [PATCH v2 02/21] multipathd: avoid sending "READY=1" to systemd on early shutdown mwilck
2020-09-24 13:36 ` [PATCH v2 03/21] multipathd: send "STOPPING=1" to systemd on shutdown mwilck
2020-09-24 13:36 ` [PATCH v2 04/21] multipathd: send "RELOADING=1" to systemd on DAEMON_CONFIGURE state mwilck
2020-09-24 13:37 ` [PATCH v2 05/21] multipathd: use volatile qualifier for running_state mwilck
2020-09-24 13:37 ` [PATCH v2 06/21] multipathd: generalize and fix wait_for_state_change_if() mwilck
2020-09-24 13:37 ` [PATCH v2 07/21] multipathd: set_config_state(): avoid code duplication mwilck
2020-09-24 13:37 ` [PATCH v2 08/21] multipathd: cancel threads early during shutdown mwilck
2020-09-24 13:37 ` [PATCH v2 09/21] multipath-tools: don't call dm_lib_release() any more mwilck
2020-09-24 13:37 ` [PATCH v2 10/21] libmultipath: devmapper: refactor libdm version determination mwilck
2020-09-25 23:48   ` Benjamin Marzinski
2020-09-24 13:37 ` [PATCH v2 11/21] libmultipath: protect racy libdevmapper calls with a mutex mwilck
2020-09-29  5:57   ` Benjamin Marzinski
2020-09-29  9:09     ` Martin Wilck
2020-09-24 13:37 ` [PATCH v2 12/21] libmultipath: constify file argument in config parser mwilck
2020-09-24 13:37 ` [PATCH v2 13/21] libmultipath: provide defaults for {get, put}_multipath_config mwilck
2020-09-25  4:34   ` Benjamin Marzinski
2020-09-25 20:00     ` Martin Wilck
2020-09-25 21:57       ` Benjamin Marzinski
2020-09-26  9:43         ` Martin Wilck
2020-09-27  1:03           ` Benjamin Marzinski
2020-09-24 13:37 ` [PATCH v2 14/21] libmpathpersist: allow using libmultipath " mwilck
2020-09-24 13:37 ` [PATCH v2 15/21] multipath: use {get_put}_multipath_config from libmultipath mwilck
2020-09-24 13:37 ` [PATCH v2 16/21] mpathpersist: use {get, put}_multipath_config() " mwilck
2020-09-24 13:37 ` [PATCH v2 17/21] libmultipath: add udev and logsink symbols mwilck
2020-09-25 23:03   ` Benjamin Marzinski
2020-09-25 23:05     ` Martin Wilck
2020-09-24 13:37 ` [PATCH v2 18/21] multipath: remove logsink and udev mwilck
2020-09-24 13:37 ` [PATCH v2 19/21] libmpathpersist: call libmultipath_{init, exit}() mwilck
2020-09-25 23:55   ` Benjamin Marzinski
2020-09-24 13:37 ` [PATCH v2 20/21] mpathpersist: remove logsink and udev mwilck
2020-09-25 23:56   ` Benjamin Marzinski
2020-09-24 13:37 ` [PATCH v2 21/21] multipathd: " mwilck
2020-09-26  0:19   ` Benjamin Marzinski

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).