[dm-devel] [PATCH v3 00/29] libmultipath: improve cleanup on exit

[dm-devel] [PATCH v3 00/29] libmultipath: improve cleanup on exit

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Hi Christophe, hi Ben, hi lixiaokeng,

this series was inspired by lixiaokeng's recent posting "[QUESTION] memory
leak in main (multipath)". It implements my first idea, registering
cleanup handlers with atexit(). However it turned out to be quite
complex. In particular multipathd has a lot of things to clean up.

This series is based on the previous series "multipath-tools: shutdown, 
libdevmapper races, globals" (v3).

While the bulk of the series is the cleanup handling, it also contains
some bug fixes for issues that I found while working on this.

Changes v2 -> v3:

Removed on_exit() calls, which break build on Alpine Linux (musl libc).

  02/29 "multipathd: Fix liburcu memory leak":
         use atexit() rather than on_exit()
  18/29 "libmultipath: fix log_thread startup and teardown"
         fixed rebase error pointed out by Ben
  19/29 "multipath: use atexit() for cleanup handlers"
         use atexit() rather than on_exit()
  21/29 "multipath: fix leaks in check_path_valid()"
         revert the on_exit() part of the previous version of this patch. As
	 argued in a separate mail, avoiding this corner-case "memory leak"
	 on an irregular exit isn't worth the ugliness of being non-reentrant
	 and using file-scope static variables for things that would naturally
	 be automatic variables.
  24/29 "libmultipath: use libmp_verbosity to track verbosity":
         Removed additional uses of conf->verbosity (Ben)
  25/29 "libmultipath: introduce symbolic values for logsink":
         removed blank line at end of libmultipath/debug.h (Ben)
  28/29 "multipathd: sanitize uxsock_listen()"
         unchanged, but modified commit message such that it doesn't suggest
	 memory was allocated in pages (Ben)
  29/29  "libmultipath: fix race between log_safe and log_thread_stop()":
         Ben had accepted the previous version after some discussion.
	 I moved the call to flush_logqueue() in log_thread_stop()
	 after the pthread_join() call.

Changes v1 -> v2:

Patches 24..29 have been added; they are logging cleanups, 24/29
and 29/29 are fixes to issues Ben had mentioned during his review.

01/29 "multipathd: uxlsnr: avoid deadlock on exit"
      Fix invalid array element reference (Ben)
07/29 "multipathd: move conf destruction into separate function"
      Don't reset logink when the log thread is stopped (Ben)
      Logging via logsink after shutting down the log thread is racy
      (no worse than before); the race will be fixed in 29/29.
14/29 "libmultipath: add libmp_dm_exit()": Fixed the skip_libmp_dm_init()
      case (Ben).
18/29 "libmultipath: fix log_thread startup and teardown"
      No need to wait before joining the log thread in log_thread_stop() (Ben)
      -> "libmultipath: fix race between log_safe and log_thread_stop()"
23/29: "multipath-tools: mpath-tools.supp: file with valgrind suppressions"
       Remove empty line at end of file (Ben)
24/29 "libmultipath: use libmp_verbosity to track verbosity"
       This fixes 13/21 "libmultipath: provide defaults for {get,put}_multipath_config"
       (from the previous patch series "multipath-tools: shutdown, libdevmapper races, globals";
       control of verbosity during config file loading)
29/29 "libmultipath: fix race between log_safe and log_thread_stop()"
      (see 18/29)

Regards
Martin

Martin Wilck (29):
  multipathd: uxlsnr: avoid deadlock on exit
  multipathd: Fix liburcu memory leak
  multipathd: move handling of io_err_stat_attr into libmultipath
  multipathd: move vecs desctruction into cleanup function
  multipathd: make some globals static
  multipathd: move threads destruction into separate function
  multipathd: move conf destruction into separate function
  multipathd: move pid destruction into separate function
  multipathd: close pidfile on exit
  multipathd: add helper for systemd notification at exit
  multipathd: child(): call cleanups in failure case, too
  multipathd: unwatch_all_dmevents: check if waiter is initialized
  multipathd: print error message if config can't be loaded
  libmultipath: add libmp_dm_exit()
  multipathd: fixup libdm deinitialization
  libmultipath: log_thread_stop(): check if logarea is initialized
  multipathd: add cleanup_child() exit handler
  libmultipath: fix log_thread startup and teardown
  multipathd: move cleanup_{prio,checkers,foreign} to libmultipath_exit
  multipath: use atexit() for cleanup handlers
  mpathpersist: use atexit() for cleanup handlers
  multipath: fix leaks in check_path_valid()
  multipath-tools: mpath-tools.supp: file with valgrind suppressions
  libmultipath: use libmp_verbosity to track verbosity
  libmultipath: introduce symbolic values for logsink
  libmultipath: simplify dlog()
  multipathd: common code for "-k" and command args
  multipathd: sanitize uxsock_listen()
  libmultipath: fix race between log_safe and log_thread_stop()

 libmpathpersist/mpath_persist.c       |   7 +-
 libmultipath/config.c                 |  14 +-
 libmultipath/config.h                 |   2 +
 libmultipath/configure.c              |  16 +-
 libmultipath/debug.c                  |  38 +--
 libmultipath/debug.h                  |  27 +-
 libmultipath/devmapper.c              |  30 +-
 libmultipath/devmapper.h              |   1 +
 libmultipath/io_err_stat.c            |   7 +-
 libmultipath/libmultipath.version     |  31 +-
 libmultipath/log_pthread.c            | 111 +++++---
 mpathpersist/main.c                   |   5 +-
 multipath/main.c                      |  91 +++---
 multipathd/dmevents.c                 |   2 +
 multipathd/main.c                     | 394 ++++++++++++++++----------
 multipathd/uxlsnr.c                   |  80 ++++--
 tests/alias.c                         |   1 +
 tests/blacklist.c                     |   2 +
 tests/globals.c                       |   3 +-
 tests/hwtable.c                       |   2 +-
 tests/test-log.c                      |   4 +-
 tests/test-log.h                      |   3 +-
 third-party/valgrind/mpath-tools.supp |  32 +++
 23 files changed, 557 insertions(+), 346 deletions(-)
 create mode 100644 third-party/valgrind/mpath-tools.supp

-- 
2.29.0


--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH v3 01/29] multipathd: uxlsnr: avoid deadlock on exit

[mwilck]

From: Martin Wilck <mwilck@suse.com>

The uxlsnr wouldn't always release the client lock when cancelled,
causing a deadlock in uxsock_cleanup(). While this hasn't been
caused by commit 3d611a2, the deadlock seems to have become much
more likely after that patch. Solving this means that we have to
treat reallocation failure of the pollfd array differently.
We will now just ignore any clients above the last valid pfd index.
That's a minor problem, as we're in an OOM situation anyway.

Moreover, client_lock is not a "struct lock", but a plain
pthread_mutex_t.

Fixes: 3d611a2 ("multipathd: cancel threads early during shutdown")
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipathd/uxlsnr.c | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/multipathd/uxlsnr.c b/multipathd/uxlsnr.c
index 1c5ce9d..ce2b680 100644
--- a/multipathd/uxlsnr.c
+++ b/multipathd/uxlsnr.c
@@ -35,6 +35,7 @@
 #include "config.h"
 #include "mpath_cmd.h"
 #include "time-util.h"
+#include "util.h"
 
 #include "main.h"
 #include "cli.h"
@@ -116,7 +117,7 @@ static void _dead_client(struct client *c)
 
 static void dead_client(struct client *c)
 {
-	pthread_cleanup_push(cleanup_lock, &client_lock);
+	pthread_cleanup_push(cleanup_mutex, &client_lock);
 	pthread_mutex_lock(&client_lock);
 	_dead_client(c);
 	pthread_cleanup_pop(1);
@@ -302,10 +303,11 @@ void * uxsock_listen(uxsock_trigger_fn uxsock_trigger, long ux_sock,
 	sigdelset(&mask, SIGUSR1);
 	while (1) {
 		struct client *c, *tmp;
-		int i, poll_count, num_clients;
+		int i, n_pfds, poll_count, num_clients;
 
 		/* setup for a poll */
 		pthread_mutex_lock(&client_lock);
+		pthread_cleanup_push(cleanup_mutex, &client_lock);
 		num_clients = 0;
 		list_for_each_entry(c, &clients, node) {
 			num_clients++;
@@ -322,14 +324,13 @@ void * uxsock_listen(uxsock_trigger_fn uxsock_trigger, long ux_sock,
 						sizeof(struct pollfd));
 			}
 			if (!new) {
-				pthread_mutex_unlock(&client_lock);
 				condlog(0, "%s: failed to realloc %d poll fds",
 					"uxsock", 2 + num_clients);
-				sched_yield();
-				continue;
+				num_clients = old_clients;
+			} else {
+				old_clients = num_clients;
+				polls = new;
 			}
-			old_clients = num_clients;
-			polls = new;
 		}
 		polls[0].fd = ux_sock;
 		polls[0].events = POLLIN;
@@ -347,11 +348,14 @@ void * uxsock_listen(uxsock_trigger_fn uxsock_trigger, long ux_sock,
 			polls[i].fd = c->fd;
 			polls[i].events = POLLIN;
 			i++;
+			if (i >= 2 + num_clients)
+				break;
 		}
-		pthread_mutex_unlock(&client_lock);
+		n_pfds = i;
+		pthread_cleanup_pop(1);
 
 		/* most of our life is spent in this call */
-		poll_count = ppoll(polls, i, &sleep_time, &mask);
+		poll_count = ppoll(polls, n_pfds, &sleep_time, &mask);
 
 		handle_signals(false);
 		if (poll_count == -1) {
@@ -384,7 +388,7 @@ void * uxsock_listen(uxsock_trigger_fn uxsock_trigger, long ux_sock,
 		}
 
 		/* see if a client wants to speak to us */
-		for (i = 2; i < num_clients + 2; i++) {
+		for (i = 2; i < n_pfds; i++) {
 			if (polls[i].revents & POLLIN) {
 				struct timespec start_time;
 
-- 
2.29.0


--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH v3 02/29] multipathd: Fix liburcu memory leak

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Fix this leak in multipathd, reported by valgrind, that messes up
multipathd's otherwise clean leak report:

==23823== 336 bytes in 1 blocks are possibly lost in loss record 3 of 3
==23823==    at 0x483AB65: calloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==23823==    by 0x4012F16: _dl_allocate_tls (in /lib64/ld-2.31.so)
==23823==    by 0x493BB8E: pthread_create@@GLIBC_2.2.5 (in /lib64/libpthread-2.31.so)
==23823==    by 0x492A9A9: call_rcu_data_init (urcu-call-rcu-impl.h:437)
==23823==    by 0x492AD2F: UnknownInlinedFun (urcu-call-rcu-impl.h:492)
==23823==    by 0x492AD2F: create_call_rcu_data_memb (urcu-call-rcu-impl.h:504)
==23823==    by 0x1164E3: child.constprop.0.isra.0 (main.c:2915)
==23823==    by 0x10F50C: main (main.c:3335)
==23823==
==23823== LEAK SUMMARY:
==23823==    definitely lost: 0 bytes in 0 blocks
==23823==    indirectly lost: 0 bytes in 0 blocks
==23823==      possibly lost: 336 bytes in 1 blocks

The problem is caused by using liburcu's default RCU call handler,
which liburcu refuses to stop/join. See comments in the code.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipathd/main.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 44 insertions(+), 1 deletion(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index c5c374b..ce14bb6 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2889,6 +2889,48 @@ set_oom_adj (void)
 	condlog(0, "couldn't adjust oom score");
 }
 
+/*
+ * Use a non-default call_rcu_data for child().
+ *
+ * We do this to avoid a memory leak from liburcu.
+ * liburcu never frees the default rcu handler (see comments on
+ * call_rcu_data_free() in urcu-call-rcu-impl.h), its thread
+ * can't be joined with pthread_join(), leaving a memory leak.
+ *
+ * Therefore we create our own, which can be destroyed and joined.
+ */
+static struct call_rcu_data *setup_rcu(void)
+{
+	struct call_rcu_data *crdp;
+
+	rcu_init();
+	rcu_register_thread();
+	crdp = create_call_rcu_data(0UL, -1);
+	if (crdp != NULL)
+		set_thread_call_rcu_data(crdp);
+	return crdp;
+}
+
+static struct call_rcu_data *mp_rcu_data;
+
+static void cleanup_rcu(void)
+{
+	pthread_t rcu_thread;
+
+	/* Wait for any pending RCU calls */
+	rcu_barrier();
+	if (mp_rcu_data != NULL) {
+		rcu_thread = get_call_rcu_thread(mp_rcu_data);
+		/* detach this thread from the RCU thread */
+		set_thread_call_rcu_data(NULL);
+		synchronize_rcu();
+		/* tell RCU thread to exit */
+		call_rcu_data_free(mp_rcu_data);
+		pthread_join(rcu_thread, NULL);
+	}
+	rcu_unregister_thread();
+}
+
 static int
 child (__attribute__((unused)) void *param)
 {
@@ -2906,7 +2948,8 @@ child (__attribute__((unused)) void *param)
 
 	mlockall(MCL_CURRENT | MCL_FUTURE);
 	signal_init();
-	rcu_init();
+	mp_rcu_data = setup_rcu();
+	atexit(cleanup_rcu);
 
 	setup_thread_attr(&misc_attr, 64 * 1024, 0);
 	setup_thread_attr(&uevent_attr, DEFAULT_UEVENT_STACKSIZE * 1024, 0);
-- 
2.29.0


--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH v3 03/29] multipathd: move handling of io_err_stat_attr into libmultipath

[mwilck]

From: Martin Wilck <mwilck@suse.com>

This thread attribute can be dynamically initialized and destroyed.
No need to carry it along in multipathd. Removal of the symbol
requires to bump the ABI version to 3.

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/io_err_stat.c        |  7 +++++--
 libmultipath/libmultipath.version | 23 ++++++++---------------
 multipathd/main.c                 |  2 --
 3 files changed, 13 insertions(+), 19 deletions(-)

diff --git a/libmultipath/io_err_stat.c b/libmultipath/io_err_stat.c
index 58bc1dd..5363049 100644
--- a/libmultipath/io_err_stat.c
+++ b/libmultipath/io_err_stat.c
@@ -34,6 +34,7 @@
 #include "lock.h"
 #include "time-util.h"
 #include "io_err_stat.h"
+#include "util.h"
 
 #define TIMEOUT_NO_IO_NSEC		10000000 /*10ms = 10000000ns*/
 #define FLAKY_PATHFAIL_THRESHOLD	2
@@ -70,8 +71,7 @@ struct io_err_stat_path {
 	int		err_rate_threshold;
 };
 
-pthread_t		io_err_stat_thr;
-pthread_attr_t		io_err_stat_attr;
+static pthread_t	io_err_stat_thr;
 
 static pthread_mutex_t io_err_thread_lock = PTHREAD_MUTEX_INITIALIZER;
 static pthread_cond_t io_err_thread_cond = PTHREAD_COND_INITIALIZER;
@@ -727,6 +727,7 @@ static void *io_err_stat_loop(void *data)
 int start_io_err_stat_thread(void *data)
 {
 	int ret;
+	pthread_attr_t io_err_stat_attr;
 
 	if (uatomic_read(&io_err_thread_running) == 1)
 		return 0;
@@ -739,6 +740,7 @@ int start_io_err_stat_thread(void *data)
 	if (!paths)
 		goto destroy_ctx;
 
+	setup_thread_attr(&io_err_stat_attr, 32 * 1024, 0);
 	pthread_mutex_lock(&io_err_thread_lock);
 	pthread_cleanup_push(cleanup_unlock, &io_err_thread_lock);
 
@@ -750,6 +752,7 @@ int start_io_err_stat_thread(void *data)
 				 &io_err_thread_lock) == 0);
 
 	pthread_cleanup_pop(1);
+	pthread_attr_destroy(&io_err_stat_attr);
 
 	if (ret) {
 		io_err_stat_log(0, "cannot create io_error statistic thread");
diff --git a/libmultipath/libmultipath.version b/libmultipath/libmultipath.version
index 0c300c8..84beb7f 100644
--- a/libmultipath/libmultipath.version
+++ b/libmultipath/libmultipath.version
@@ -31,7 +31,7 @@
  *   The new version inherits the previous ones.
  */
 
-LIBMULTIPATH_2.0.0 {
+LIBMULTIPATH_3.0.0 {
 global:
 	/* symbols referenced by multipath and multipathd */
 	add_foreign;
@@ -121,7 +121,6 @@ global:
 	init_checkers;
 	init_foreign;
 	init_prio;
-	io_err_stat_attr;
 	io_err_stat_handle_pathfail;
 	is_path_valid;
 	is_quote;
@@ -242,30 +241,24 @@ global:
 	free_scandir_result;
 	sysfs_attr_get_value;
 
-local:
-	*;
-};
-
-LIBMULTIPATH_2.1.0 {
-global:
+	/* added in 2.1.0 */
 	libmp_dm_task_run;
 	cleanup_mutex;
-} LIBMULTIPATH_2.0.0;
 
-LIBMULTIPATH_2.2.0 {
-global:
+	/* added in 2.2.0 */
 	libmp_get_multipath_config;
 	get_multipath_config;
 	libmp_put_multipath_config;
 	put_multipath_config;
 	init_config;
 	uninit_config;
-} LIBMULTIPATH_2.1.0;
 
-LIBMULTIPATH_2.3.0 {
-global:
+	/* added in 2.3.0 */
 	udev;
 	logsink;
 	libmultipath_init;
 	libmultipath_exit;
-} LIBMULTIPATH_2.2.0;
+
+local:
+	*;
+};
diff --git a/multipathd/main.c b/multipathd/main.c
index ce14bb6..abc6a9f 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2954,7 +2954,6 @@ child (__attribute__((unused)) void *param)
 	setup_thread_attr(&misc_attr, 64 * 1024, 0);
 	setup_thread_attr(&uevent_attr, DEFAULT_UEVENT_STACKSIZE * 1024, 0);
 	setup_thread_attr(&waiter_attr, 32 * 1024, 1);
-	setup_thread_attr(&io_err_stat_attr, 32 * 1024, 0);
 
 	if (logsink == 1) {
 		setup_thread_attr(&log_attr, 64 * 1024, 0);
@@ -3164,7 +3163,6 @@ child (__attribute__((unused)) void *param)
 	rcu_assign_pointer(multipath_conf, NULL);
 	call_rcu(&conf->rcu, rcu_free_config);
 	pthread_attr_destroy(&waiter_attr);
-	pthread_attr_destroy(&io_err_stat_attr);
 #ifdef _DEBUG_
 	dbg_free_final(NULL);
 #endif
-- 
2.29.0


--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH v3 04/29] multipathd: move vecs desctruction into cleanup function

[mwilck]

From: Martin Wilck <mwilck@suse.com>

This will make it easer to move the stuff around later.
The only functional change is that map destuction now happens after
joining all threads, which should actually improve robustness.

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

diff --git a/multipathd/main.c b/multipathd/main.c
index abc6a9f..3da0d7c 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -148,7 +148,7 @@ int should_exit(void)
 /*
  * global copy of vecs for use in sig handlers
  */
-struct vectors * gvecs;
+static struct vectors * gvecs;
 
 struct config *multipath_conf;
 
@@ -2889,6 +2889,44 @@ set_oom_adj (void)
 	condlog(0, "couldn't adjust oom score");
 }
 
+static void cleanup_maps(struct vectors *vecs)
+{
+	int queue_without_daemon, i;
+	struct multipath *mpp;
+	struct config *conf;
+
+	conf = get_multipath_config();
+	queue_without_daemon = conf->queue_without_daemon;
+	put_multipath_config(conf);
+	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);
+	vecs->mpvec = NULL;
+}
+
+static void cleanup_paths(struct vectors *vecs)
+{
+	free_pathvec(vecs->pathvec, FREE_PATHS);
+	vecs->pathvec = NULL;
+}
+
+static void cleanup_vecs(void)
+{
+	if (!gvecs)
+		return;
+	/*
+	 * We can't take the vecs lock here, because exit() may
+	 * have been called from the child() thread, holding the lock already.
+	 * Anyway, by the time we get here, all threads that might access
+	 * vecs should have been joined already (in cleanup_threads).
+	 */
+	cleanup_maps(gvecs);
+	cleanup_paths(gvecs);
+	pthread_mutex_destroy(&gvecs->lock.mutex);
+	FREE(gvecs);
+}
+
 /*
  * Use a non-default call_rcu_data for child().
  *
@@ -2937,13 +2975,10 @@ child (__attribute__((unused)) void *param)
 	pthread_t check_thr, uevent_thr, uxlsnr_thr, uevq_thr, dmevent_thr;
 	pthread_attr_t log_attr, misc_attr, uevent_attr;
 	struct vectors * vecs;
-	struct multipath * mpp;
-	int i;
 	int rc;
 	int pid_fd = -1;
 	struct config *conf;
 	char *envp;
-	int queue_without_daemon;
 	enum daemon_status state;
 
 	mlockall(MCL_CURRENT | MCL_FUTURE);
@@ -3108,17 +3143,6 @@ child (__attribute__((unused)) void *param)
 	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_join(check_thr, NULL);
 	pthread_join(uevent_thr, NULL);
 	pthread_join(uxlsnr_thr, NULL);
@@ -3128,15 +3152,7 @@ child (__attribute__((unused)) void *param)
 
 	stop_io_err_stat_thread();
 
-	lock(&vecs->lock);
-	free_pathvec(vecs->pathvec, FREE_PATHS);
-	vecs->pathvec = NULL;
-	unlock(&vecs->lock);
-
-	pthread_mutex_destroy(&vecs->lock.mutex);
-	FREE(vecs);
-	vecs = NULL;
-
+	cleanup_vecs();
 	cleanup_foreign();
 	cleanup_checkers();
 	cleanup_prio();
-- 
2.29.0


--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH v3 05/29] multipathd: make some globals static

[mwilck]

From: Martin Wilck <mwilck@suse.com>

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

diff --git a/multipathd/main.c b/multipathd/main.c
index 3da0d7c..eb760a7 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -116,19 +116,19 @@ struct mpath_event_param
 };
 
 int uxsock_timeout;
-int verbosity;
-int bindings_read_only;
+static int verbosity;
+static int bindings_read_only;
 int ignore_new_devs;
 #ifdef NO_DMEVENTS_POLL
-int poll_dmevents = 0;
+static int poll_dmevents = 0;
 #else
-int poll_dmevents = 1;
+static int poll_dmevents = 1;
 #endif
 /* Don't access this variable without holding config_lock */
-volatile enum daemon_status running_state = DAEMON_INIT;
+static 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;
+static pthread_mutex_t config_lock = PTHREAD_MUTEX_INITIALIZER;
+static pthread_cond_t config_cond;
 
 static inline enum daemon_status get_running_state(void)
 {
-- 
2.29.0


--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH v3 06/29] multipathd: move threads destruction into separate function

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Also, introduce booleans that indicate a certain thread has
been started successfully. Using these booleans, we can avoid
crashing by cancelling threads that have never been started.

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

diff --git a/multipathd/main.c b/multipathd/main.c
index eb760a7..9eb658d 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -129,6 +129,9 @@ static volatile enum daemon_status running_state = DAEMON_INIT;
 pid_t daemon_pid;
 static pthread_mutex_t config_lock = PTHREAD_MUTEX_INITIALIZER;
 static pthread_cond_t config_cond;
+static pthread_t check_thr, uevent_thr, uxlsnr_thr, uevq_thr, dmevent_thr;
+static bool check_thr_started, uevent_thr_started, uxlsnr_thr_started,
+	uevq_thr_started, dmevent_thr_started;
 
 static inline enum daemon_status get_running_state(void)
 {
@@ -2927,6 +2930,39 @@ static void cleanup_vecs(void)
 	FREE(gvecs);
 }
 
+static void cleanup_threads(void)
+{
+	stop_io_err_stat_thread();
+
+	if (check_thr_started)
+		pthread_cancel(check_thr);
+	if (uevent_thr_started)
+		pthread_cancel(uevent_thr);
+	if (uxlsnr_thr_started)
+		pthread_cancel(uxlsnr_thr);
+	if (uevq_thr_started)
+		pthread_cancel(uevq_thr);
+	if (dmevent_thr_started)
+		pthread_cancel(dmevent_thr);
+
+	if (check_thr_started)
+		pthread_join(check_thr, NULL);
+	if (uevent_thr_started)
+		pthread_join(uevent_thr, NULL);
+	if (uxlsnr_thr_started)
+		pthread_join(uxlsnr_thr, NULL);
+	if (uevq_thr_started)
+		pthread_join(uevq_thr, NULL);
+	if (dmevent_thr_started)
+		pthread_join(dmevent_thr, NULL);
+
+	/*
+	 * As all threads are joined now, and we're in DAEMON_SHUTDOWN
+	 * state, no new waiter threads will be created any more.
+	 */
+	pthread_attr_destroy(&waiter_attr);
+}
+
 /*
  * Use a non-default call_rcu_data for child().
  *
@@ -2972,7 +3008,6 @@ static void cleanup_rcu(void)
 static int
 child (__attribute__((unused)) void *param)
 {
-	pthread_t check_thr, uevent_thr, uxlsnr_thr, uevq_thr, dmevent_thr;
 	pthread_attr_t log_attr, misc_attr, uevent_attr;
 	struct vectors * vecs;
 	int rc;
@@ -3070,9 +3105,12 @@ child (__attribute__((unused)) void *param)
 		condlog(0, "failed to create cli listener: %d", rc);
 		goto failed;
 	}
-	else if (state != DAEMON_CONFIGURE) {
-		condlog(0, "cli listener failed to start");
-		goto failed;
+	else {
+		uxlsnr_thr_started = true;
+		if (state != DAEMON_CONFIGURE) {
+			condlog(0, "cli listener failed to start");
+			goto failed;
+		}
 	}
 
 	if (poll_dmevents) {
@@ -3085,7 +3123,8 @@ child (__attribute__((unused)) void *param)
 			condlog(0, "failed to create dmevent waiter thread: %d",
 				rc);
 			goto failed;
-		}
+		} else
+			dmevent_thr_started = true;
 	}
 
 	/*
@@ -3094,7 +3133,8 @@ child (__attribute__((unused)) void *param)
 	if ((rc = pthread_create(&uevent_thr, &uevent_attr, ueventloop, udev))) {
 		condlog(0, "failed to create uevent thread: %d", rc);
 		goto failed;
-	}
+	} else
+		uevent_thr_started = true;
 	pthread_attr_destroy(&uevent_attr);
 
 	/*
@@ -3103,11 +3143,13 @@ child (__attribute__((unused)) void *param)
 	if ((rc = pthread_create(&check_thr, &misc_attr, checkerloop, vecs))) {
 		condlog(0,"failed to create checker loop thread: %d", rc);
 		goto failed;
-	}
+	} else
+		check_thr_started = true;
 	if ((rc = pthread_create(&uevq_thr, &misc_attr, uevqloop, vecs))) {
 		condlog(0, "failed to create uevent dispatcher: %d", rc);
 		goto failed;
-	}
+	} else
+		uevq_thr_started = true;
 	pthread_attr_destroy(&misc_attr);
 
 	while (1) {
@@ -3136,22 +3178,7 @@ child (__attribute__((unused)) void *param)
 		}
 	}
 
-	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);
-	pthread_join(uevq_thr, NULL);
-	if (poll_dmevents)
-		pthread_join(dmevent_thr, NULL);
-
-	stop_io_err_stat_thread();
-
+	cleanup_threads();
 	cleanup_vecs();
 	cleanup_foreign();
 	cleanup_checkers();
@@ -3178,7 +3205,6 @@ child (__attribute__((unused)) void *param)
 	conf = rcu_dereference(multipath_conf);
 	rcu_assign_pointer(multipath_conf, NULL);
 	call_rcu(&conf->rcu, rcu_free_config);
-	pthread_attr_destroy(&waiter_attr);
 #ifdef _DEBUG_
 	dbg_free_final(NULL);
 #endif
-- 
2.29.0


--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH v3 07/29] multipathd: move conf destruction into separate function

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Also removing the comment about dlog() and dm_write_log().
dlog() can cope with get_multipath_config() returning NULL,
and dm_write_log() hasn't accessed the configuration for a while.

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

diff --git a/multipathd/main.c b/multipathd/main.c
index 9eb658d..07973e8 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2892,6 +2892,16 @@ set_oom_adj (void)
 	condlog(0, "couldn't adjust oom score");
 }
 
+static void cleanup_conf(void) {
+	struct config *conf;
+
+	conf = rcu_dereference(multipath_conf);
+	if (!conf)
+		return;
+	rcu_assign_pointer(multipath_conf, NULL);
+	call_rcu(&conf->rcu, rcu_free_config);
+}
+
 static void cleanup_maps(struct vectors *vecs)
 {
 	int queue_without_daemon, i;
@@ -3196,15 +3206,7 @@ child (__attribute__((unused)) void *param)
 
 	if (logsink == 1)
 		log_thread_stop();
-
-	/*
-	 * Freeing config must be done after condlog() and dm_lib_exit(),
-	 * because logging functions like dlog() and dm_write_log()
-	 * reference the config.
-	 */
-	conf = rcu_dereference(multipath_conf);
-	rcu_assign_pointer(multipath_conf, NULL);
-	call_rcu(&conf->rcu, rcu_free_config);
+	cleanup_conf();
 #ifdef _DEBUG_
 	dbg_free_final(NULL);
 #endif
-- 
2.29.0


--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH v3 08/29] multipathd: move pid destruction into separate function

[mwilck]

From: Martin Wilck <mwilck@suse.com>

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

diff --git a/multipathd/main.c b/multipathd/main.c
index 07973e8..fc1f8d7 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2892,6 +2892,12 @@ set_oom_adj (void)
 	condlog(0, "couldn't adjust oom score");
 }
 
+static void cleanup_pidfile(void)
+{
+	condlog(3, "unlink pidfile");
+	unlink(DEFAULT_PIDFILE);
+}
+
 static void cleanup_conf(void) {
 	struct config *conf;
 
@@ -3199,9 +3205,7 @@ child (__attribute__((unused)) void *param)
 	dm_lib_exit();
 
 	/* We're done here */
-	condlog(3, "unlink pidfile");
-	unlink(DEFAULT_PIDFILE);
-
+	cleanup_pidfile();
 	condlog(2, "--------shut down-------");
 
 	if (logsink == 1)
-- 
2.29.0


--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH v3 09/29] multipathd: close pidfile on exit

[mwilck]

From: Martin Wilck <mwilck@suse.com>

It seems we've been doing this only in the failure case, for ages.

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 fc1f8d7..f6b8066 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -132,6 +132,7 @@ static pthread_cond_t config_cond;
 static pthread_t check_thr, uevent_thr, uxlsnr_thr, uevq_thr, dmevent_thr;
 static bool check_thr_started, uevent_thr_started, uxlsnr_thr_started,
 	uevq_thr_started, dmevent_thr_started;
+static int pid_fd = -1;
 
 static inline enum daemon_status get_running_state(void)
 {
@@ -2894,6 +2895,8 @@ set_oom_adj (void)
 
 static void cleanup_pidfile(void)
 {
+	if (pid_fd >= 0)
+		close(pid_fd);
 	condlog(3, "unlink pidfile");
 	unlink(DEFAULT_PIDFILE);
 }
@@ -3027,7 +3030,6 @@ child (__attribute__((unused)) void *param)
 	pthread_attr_t log_attr, misc_attr, uevent_attr;
 	struct vectors * vecs;
 	int rc;
-	int pid_fd = -1;
 	struct config *conf;
 	char *envp;
 	enum daemon_status state;
-- 
2.29.0


--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH v3 11/29] multipathd: child(): call cleanups in failure case, too

[mwilck]

From: Martin Wilck <mwilck@suse.com>

So far we haven't called any cleanup code if child() failed.
Fix it.

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

diff --git a/multipathd/main.c b/multipathd/main.c
index 07068e4..6b9e323 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -3044,6 +3044,7 @@ child (__attribute__((unused)) void *param)
 	struct config *conf;
 	char *envp;
 	enum daemon_status state;
+	int exit_code = 1;
 
 	mlockall(MCL_CURRENT | MCL_FUTURE);
 	signal_init();
@@ -3207,6 +3208,8 @@ child (__attribute__((unused)) void *param)
 		}
 	}
 
+	exit_code = 0;
+failed:
 	cleanup_threads();
 	cleanup_vecs();
 	cleanup_foreign();
@@ -3227,12 +3230,7 @@ child (__attribute__((unused)) void *param)
 #ifdef _DEBUG_
 	dbg_free_final(NULL);
 #endif
-	exit(sd_notify_exit(0));
-
-failed:
-	if (pid_fd >= 0)
-		close(pid_fd);
-	exit(sd_notify_exit(1));
+	return sd_notify_exit(exit_code);
 }
 
 static int
-- 
2.29.0


--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH v3 12/29] multipathd: unwatch_all_dmevents: check if waiter is initialized

[mwilck]

From: Martin Wilck <mwilck@suse.com>

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

diff --git a/multipathd/dmevents.c b/multipathd/dmevents.c
index b561cbf..f52f597 100644
--- a/multipathd/dmevents.c
+++ b/multipathd/dmevents.c
@@ -257,6 +257,8 @@ void unwatch_all_dmevents(void)
 	struct dev_event *dev_evt;
 	int i;
 
+	if (!waiter)
+		return;
 	pthread_mutex_lock(&waiter->events_lock);
 	vector_foreach_slot(waiter->events, dev_evt, i)
 		free(dev_evt);
-- 
2.29.0


--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH v3 13/29] multipathd: print error message if config can't be loaded

[mwilck]

From: Martin Wilck <mwilck@suse.com>

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 6b9e323..7ab3eab 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -3074,8 +3074,10 @@ child (__attribute__((unused)) void *param)
 	condlog(2, "read " DEFAULT_CONFIGFILE);
 
 	conf = load_config(DEFAULT_CONFIGFILE);
-	if (!conf)
+	if (!conf) {
+		condlog(0, "failed to load configuration");
 		goto failed;
+	}
 
 	if (verbosity)
 		conf->verbosity = verbosity;
-- 
2.29.0


--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH v3 14/29] libmultipath: add libmp_dm_exit()

[mwilck]

From: Martin Wilck <mwilck@suse.com>

This function prepares for calling dm_lib_exit() on program exit.
It undoes changes to libdm internals done by libmultipath.
It doesn't call dm_lib_exit(), as the caller may want to keep
libdm active.

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

diff --git a/libmultipath/config.c b/libmultipath/config.c
index f74417c..b9cb413 100644
--- a/libmultipath/config.c
+++ b/libmultipath/config.c
@@ -60,6 +60,7 @@ int libmultipath_init(void)
 static void _libmultipath_exit(void)
 {
 	libmultipath_exit_called = true;
+	libmp_dm_exit();
 	udev_unref(udev);
 }
 
diff --git a/libmultipath/config.h b/libmultipath/config.h
index f478df7..5d46035 100644
--- a/libmultipath/config.h
+++ b/libmultipath/config.h
@@ -271,6 +271,8 @@ int libmultipath_init(void);
  *
  * This function un-initializes libmultipath data structures.
  * It is recommended to call this function at program exit.
+ * If the application also calls dm_lib_exit(), it should do so
+ * after libmultipath_exit().
  *
  * Calls to libmultipath_init() after libmultipath_exit() will fail
  * (in other words, libmultipath can't be re-initialized).
diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
index 4eb6f53..e60ab49 100644
--- a/libmultipath/devmapper.c
+++ b/libmultipath/devmapper.c
@@ -335,6 +335,20 @@ void libmp_udev_set_sync_support(int on)
 	libmp_dm_udev_sync = !!on;
 }
 
+static bool libmp_dm_init_called;
+void libmp_dm_exit(void)
+{
+	if (!libmp_dm_init_called)
+		return;
+
+	/* switch back to default libdm logging */
+	dm_log_init(NULL);
+#ifdef LIBDM_API_HOLD_CONTROL
+	/* make sure control fd is closed in dm_lib_release() */
+	dm_hold_control_dev(0);
+#endif
+}
+
 static void libmp_dm_init(void)
 {
 	struct config *conf;
@@ -351,6 +365,7 @@ static void libmp_dm_init(void)
 	dm_hold_control_dev(1);
 #endif
 	dm_udev_set_sync_support(libmp_dm_udev_sync);
+	libmp_dm_init_called = true;
 }
 
 static void _do_skip_libmp_dm_init(void)
diff --git a/libmultipath/devmapper.h b/libmultipath/devmapper.h
index fa6b3c5..e29b4d4 100644
--- a/libmultipath/devmapper.h
+++ b/libmultipath/devmapper.h
@@ -35,6 +35,7 @@ enum {
 
 int dm_prereq(unsigned int *v);
 void skip_libmp_dm_init(void);
+void libmp_dm_exit(void);
 void libmp_udev_set_sync_support(int on);
 struct dm_task *libmp_dm_task_create(int task);
 int dm_simplecmd_flush (int, const char *, uint16_t);
-- 
2.29.0


--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH v3 15/29] multipathd: fixup libdm deinitialization

[mwilck]

From: Martin Wilck <mwilck@suse.com>

With libmp_dm_exit() in place, we can make sure that the
calls are made in the right order.

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 7ab3eab..4c4e2ea 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -3220,8 +3220,6 @@ failed:
 	if (poll_dmevents)
 		cleanup_dmevent_waiter();
 
-	dm_lib_exit();
-
 	/* We're done here */
 	cleanup_pidfile();
 	condlog(2, "--------shut down-------");
@@ -3318,6 +3316,9 @@ main (int argc, char *argv[])
 
 	pthread_cond_init_mono(&config_cond);
 
+	if (atexit(dm_lib_exit))
+		condlog(3, "failed to register exit handler for libdm");
+
 	libmultipath_init();
 	if (atexit(libmultipath_exit))
 		condlog(3, "failed to register exit handler for libmultipath");
-- 
2.29.0


--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH v3 16/29] libmultipath: log_thread_stop(): check if logarea is initialized

[mwilck]

From: Martin Wilck <mwilck@suse.com>

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

diff --git a/libmultipath/log_pthread.c b/libmultipath/log_pthread.c
index 15baef8..0c327ff 100644
--- a/libmultipath/log_pthread.c
+++ b/libmultipath/log_pthread.c
@@ -112,6 +112,9 @@ void log_thread_reset (void)
 
 void log_thread_stop (void)
 {
+	if (!la)
+		return;
+
 	logdbg(stderr,"enter log_thread_stop\n");
 
 	pthread_mutex_lock(&logev_lock);
-- 
2.29.0


--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH v3 17/29] multipathd: add cleanup_child() exit handler

[mwilck]

From: Martin Wilck <mwilck@suse.com>

cleanup_child() calls all cleanups in the right order, in an
exit handler.

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

diff --git a/multipathd/main.c b/multipathd/main.c
index 4c4e2ea..50cc335 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -3024,6 +3024,27 @@ static void cleanup_rcu(void)
 	rcu_unregister_thread();
 }
 
+static void cleanup_child(void)
+{
+	cleanup_threads();
+	cleanup_vecs();
+	cleanup_foreign();
+	cleanup_checkers();
+	cleanup_prio();
+	if (poll_dmevents)
+		cleanup_dmevent_waiter();
+
+	cleanup_pidfile();
+	if (logsink == 1)
+		log_thread_stop();
+
+	cleanup_conf();
+
+#ifdef _DEBUG_
+	dbg_free_final(NULL);
+#endif
+}
+
 static int sd_notify_exit(int err)
 {
 #ifdef USE_SYSTEMD
@@ -3049,7 +3070,9 @@ child (__attribute__((unused)) void *param)
 	mlockall(MCL_CURRENT | MCL_FUTURE);
 	signal_init();
 	mp_rcu_data = setup_rcu();
-	atexit(cleanup_rcu);
+
+	if (atexit(cleanup_rcu) || atexit(cleanup_child))
+		fprintf(stderr, "failed to register cleanup handlers\n");
 
 	setup_thread_attr(&misc_attr, 64 * 1024, 0);
 	setup_thread_attr(&uevent_attr, DEFAULT_UEVENT_STACKSIZE * 1024, 0);
@@ -3063,8 +3086,6 @@ child (__attribute__((unused)) void *param)
 	pid_fd = pidfile_create(DEFAULT_PIDFILE, daemon_pid);
 	if (pid_fd < 0) {
 		condlog(1, "failed to create pidfile");
-		if (logsink == 1)
-			log_thread_stop();
 		exit(1);
 	}
 
@@ -3212,24 +3233,8 @@ child (__attribute__((unused)) void *param)
 
 	exit_code = 0;
 failed:
-	cleanup_threads();
-	cleanup_vecs();
-	cleanup_foreign();
-	cleanup_checkers();
-	cleanup_prio();
-	if (poll_dmevents)
-		cleanup_dmevent_waiter();
-
-	/* We're done here */
-	cleanup_pidfile();
 	condlog(2, "--------shut down-------");
-
-	if (logsink == 1)
-		log_thread_stop();
-	cleanup_conf();
-#ifdef _DEBUG_
-	dbg_free_final(NULL);
-#endif
+	/* All cleanup is done in the cleanup_child() exit handler */
 	return sd_notify_exit(exit_code);
 }
 
-- 
2.29.0


--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH v3 18/29] libmultipath: fix log_thread startup and teardown

[mwilck]

From: Martin Wilck <mwilck@suse.com>

This fixes several issues with the log_thread. First, the running flag
logq_running should be set by the thread itself, not by
log_thread_start()/_stop(). Second, the thread was both cancelled and
terminated via a flag (again, logq_running). It's sufficient to just cancel
and join it. Third, the locking wasn't cancel-safe in some places. Forth,
log_thread_start() didn't wait for startup properly. Fifth, using (pthread_t)0
is wrong (pthread_t is opaque; there's no guarantee that 0 is not a valid
pthread_t value). Sixth, pthread_cancel() was called under logq_lock, which
doesn't make sense to me.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/log_pthread.c | 62 +++++++++++++++++++++++++++-----------
 1 file changed, 45 insertions(+), 17 deletions(-)

diff --git a/libmultipath/log_pthread.c b/libmultipath/log_pthread.c
index 0c327ff..3a2566a 100644
--- a/libmultipath/log_pthread.c
+++ b/libmultipath/log_pthread.c
@@ -13,6 +13,7 @@
 #include "log_pthread.h"
 #include "log.h"
 #include "lock.h"
+#include "util.h"
 
 static pthread_t log_thr;
 
@@ -56,35 +57,52 @@ static void flush_logqueue (void)
 	} while (empty == 0);
 }
 
+static void cleanup_log_thread(__attribute((unused)) void *arg)
+{
+	logdbg(stderr, "log thread exiting");
+	pthread_mutex_lock(&logev_lock);
+	logq_running = 0;
+	pthread_mutex_unlock(&logev_lock);
+}
+
 static void * log_thread (__attribute__((unused)) void * et)
 {
 	int running;
 
 	pthread_mutex_lock(&logev_lock);
-	logq_running = 1;
+	running = logq_running;
+	if (!running)
+		logq_running = 1;
+	pthread_cond_signal(&logev_cond);
 	pthread_mutex_unlock(&logev_lock);
+	if (running)
+		/* already started */
+		return NULL;
+	pthread_cleanup_push(cleanup_log_thread, NULL);
 
 	mlockall(MCL_CURRENT | MCL_FUTURE);
 	logdbg(stderr,"enter log_thread\n");
 
 	while (1) {
 		pthread_mutex_lock(&logev_lock);
-		if (logq_running && !log_messages_pending)
+		pthread_cleanup_push(cleanup_mutex, &logev_lock);
+		while (!log_messages_pending)
+			/* this is a cancellation point */
 			pthread_cond_wait(&logev_cond, &logev_lock);
 		log_messages_pending = 0;
-		running = logq_running;
-		pthread_mutex_unlock(&logev_lock);
-		if (!running)
-			break;
+		pthread_cleanup_pop(1);
+
 		flush_logqueue();
 	}
+	pthread_cleanup_pop(1);
 	return NULL;
 }
 
 void log_thread_start (pthread_attr_t *attr)
 {
-	logdbg(stderr,"enter log_thread_start\n");
+	int running = 0;
 
+	logdbg(stderr,"enter log_thread_start\n");
 	pthread_mutex_init(&logq_lock, NULL);
 	pthread_mutex_init(&logev_lock, NULL);
 	pthread_cond_init(&logev_cond, NULL);
@@ -93,7 +111,15 @@ void log_thread_start (pthread_attr_t *attr)
 		fprintf(stderr,"can't initialize log buffer\n");
 		exit(1);
 	}
-	if (pthread_create(&log_thr, attr, log_thread, NULL)) {
+
+	pthread_mutex_lock(&logev_lock);
+	pthread_cleanup_push(cleanup_mutex, &logev_lock);
+	if (!pthread_create(&log_thr, attr, log_thread, NULL))
+		while (!(running = logq_running))
+			pthread_cond_wait(&logev_cond, &logev_lock);
+	pthread_cleanup_pop(1);
+
+	if (!running) {
 		fprintf(stderr,"can't start log thread\n");
 		exit(1);
 	}
@@ -112,23 +138,25 @@ void log_thread_reset (void)
 
 void log_thread_stop (void)
 {
+	int running;
+
 	if (!la)
 		return;
 
 	logdbg(stderr,"enter log_thread_stop\n");
 
 	pthread_mutex_lock(&logev_lock);
-	logq_running = 0;
-	pthread_cond_signal(&logev_cond);
-	pthread_mutex_unlock(&logev_lock);
-
-	pthread_mutex_lock(&logq_lock);
-	pthread_cancel(log_thr);
-	pthread_mutex_unlock(&logq_lock);
-	pthread_join(log_thr, NULL);
-	log_thr = (pthread_t)0;
+	pthread_cleanup_push(cleanup_mutex, &logev_lock);
+	running = logq_running;
+	if (running) {
+		pthread_cancel(log_thr);
+		pthread_cond_signal(&logev_cond);
+	}
+	pthread_cleanup_pop(1);
 
 	flush_logqueue();
+	if (running)
+		pthread_join(log_thr, NULL);
 
 	pthread_mutex_destroy(&logq_lock);
 	pthread_mutex_destroy(&logev_lock);
-- 
2.29.0


--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH v3 19/29] multipathd: move cleanup_{prio, checkers, foreign} to libmultipath_exit

[mwilck]

From: Martin Wilck <mwilck@suse.com>

This requires another major ABI bump.

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

diff --git a/libmpathpersist/mpath_persist.c b/libmpathpersist/mpath_persist.c
index e1d1cb7..9ebf91d 100644
--- a/libmpathpersist/mpath_persist.c
+++ b/libmpathpersist/mpath_persist.c
@@ -78,8 +78,6 @@ mpath_lib_init (void)
 
 static void libmpathpersist_cleanup(void)
 {
-	cleanup_prio();
-	cleanup_checkers();
 	libmultipath_exit();
 	dm_lib_exit();
 }
diff --git a/libmultipath/config.c b/libmultipath/config.c
index b9cb413..52b1447 100644
--- a/libmultipath/config.c
+++ b/libmultipath/config.c
@@ -26,6 +26,7 @@
 #include "devmapper.h"
 #include "mpath_cmd.h"
 #include "propsel.h"
+#include "foreign.h"
 
 /*
  * We don't support re-initialization after
@@ -60,6 +61,9 @@ int libmultipath_init(void)
 static void _libmultipath_exit(void)
 {
 	libmultipath_exit_called = true;
+	cleanup_foreign();
+	cleanup_checkers();
+	cleanup_prio();
 	libmp_dm_exit();
 	udev_unref(udev);
 }
diff --git a/libmultipath/libmultipath.version b/libmultipath/libmultipath.version
index 84beb7f..800cff2 100644
--- a/libmultipath/libmultipath.version
+++ b/libmultipath/libmultipath.version
@@ -31,7 +31,7 @@
  *   The new version inherits the previous ones.
  */
 
-LIBMULTIPATH_3.0.0 {
+LIBMULTIPATH_4.0.0 {
 global:
 	/* symbols referenced by multipath and multipathd */
 	add_foreign;
@@ -51,10 +51,7 @@ global:
 	checker_name;
 	checker_state_name;
 	check_foreign;
-	cleanup_checkers;
-	cleanup_foreign;
 	cleanup_lock;
-	cleanup_prio;
 	close_fd;
 	coalesce_paths;
 	convert_dev;
diff --git a/multipathd/main.c b/multipathd/main.c
index 50cc335..4de0978 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -3028,9 +3028,6 @@ static void cleanup_child(void)
 {
 	cleanup_threads();
 	cleanup_vecs();
-	cleanup_foreign();
-	cleanup_checkers();
-	cleanup_prio();
 	if (poll_dmevents)
 		cleanup_dmevent_waiter();
 
-- 
2.29.0


--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH v3 20/29] multipath: use atexit() for cleanup handlers

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipath/main.c | 37 ++++++++++++++++---------------------
 1 file changed, 16 insertions(+), 21 deletions(-)

diff --git a/multipath/main.c b/multipath/main.c
index 9ae46ed..1949a1c 100644
--- a/multipath/main.c
+++ b/multipath/main.c
@@ -452,13 +452,19 @@ static bool released_to_systemd(void)
 	return ret;
 }
 
+static struct vectors vecs;
+static void cleanup_vecs(void)
+{
+	free_multipathvec(vecs.mpvec, KEEP_PATHS);
+	free_pathvec(vecs.pathvec, FREE_PATHS);
+}
+
 static int
 configure (struct config *conf, enum mpath_cmds cmd,
 	   enum devtypes dev_type, char *devpath)
 {
 	vector curmp = NULL;
 	vector pathvec = NULL;
-	struct vectors vecs;
 	int r = RTVL_FAIL, rc;
 	int di_flag = 0;
 	char * refwwid = NULL;
@@ -469,6 +475,7 @@ configure (struct config *conf, enum mpath_cmds cmd,
 	 */
 	curmp = vector_alloc();
 	pathvec = vector_alloc();
+	atexit(cleanup_vecs);
 
 	if (!curmp || !pathvec) {
 		condlog(0, "can not allocate memory");
@@ -580,9 +587,6 @@ out:
 	if (refwwid)
 		FREE(refwwid);
 
-	free_multipathvec(curmp, KEEP_PATHS);
-	free_pathvec(pathvec, FREE_PATHS);
-
 	return r;
 }
 
@@ -808,9 +812,13 @@ main (int argc, char *argv[])
 	bool enable_foreign = false;
 
 	libmultipath_init();
+	if (atexit(dm_lib_exit) || atexit(libmultipath_exit))
+		condlog(1, "failed to register cleanup handler for libmultipath: %m");
 	logsink = 0;
 	if (init_config(DEFAULT_CONFIGFILE))
 		exit(RTVL_FAIL);
+	if (atexit(uninit_config))
+		condlog(1, "failed to register cleanup handler for config: %m");
 	conf = get_multipath_config();
 	conf->retrigger_tries = 0;
 	conf->force_sync = 1;
@@ -887,7 +895,7 @@ main (int argc, char *argv[])
 			break;
 		case 't':
 			r = dump_config(conf, NULL, NULL) ? RTVL_FAIL : RTVL_OK;
-			goto out_free_config;
+			goto out;
 		case 'T':
 			cmd = CMD_DUMP_CONFIG;
 			break;
@@ -1048,26 +1056,13 @@ main (int argc, char *argv[])
 		condlog(3, "restart multipath configuration process");
 
 out:
-	dm_lib_exit();
-
-	cleanup_foreign();
-	cleanup_prio();
-	cleanup_checkers();
+	put_multipath_config(conf);
+	if (dev)
+		FREE(dev);
 
 	if (dev_type == DEV_UEVENT)
 		closelog();
 
-out_free_config:
-	/*
-	 * Freeing config must be done after dm_lib_exit(), because
-	 * the logging function (dm_write_log()), which is called there,
-	 * references the config.
-	 */
-	put_multipath_config(conf);
-	uninit_config();
-	libmultipath_exit();
-	if (dev)
-		FREE(dev);
 #ifdef _DEBUG_
 	dbg_free_final(NULL);
 #endif
-- 
2.29.0


--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH v3 21/29] mpathpersist: use atexit() for cleanup handlers

[mwilck]

From: Martin Wilck <mwilck@suse.com>

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

diff --git a/mpathpersist/main.c b/mpathpersist/main.c
index 3c2e657..14245cc 100644
--- a/mpathpersist/main.c
+++ b/mpathpersist/main.c
@@ -641,11 +641,10 @@ int main(int argc, char *argv[])
 	if (libmpathpersist_init()) {
 		exit(1);
 	}
+	if (atexit((void(*)(void))libmpathpersist_exit))
+		fprintf(stderr, "failed to register cleanup handler for libmpathpersist: %m");
 
 	ret = handle_args(argc, argv, 0);
-
-	libmpathpersist_exit();
-
 	return (ret >= 0) ? ret : MPATH_PR_OTHER;
 }
 
-- 
2.29.0


--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH v3 22/29] multipath: fix leaks in check_path_valid()

[mwilck]

From: Martin Wilck <mwilck@suse.com>

There were two leaks in check_path_valid(): if path status was
successfully determined before calling store_pathvec(), free_path()
wasn't called. Also, if an error exit occured, neither cleanup
function was called.

This patch fixes both, at the cost of using "static" for the pp and
pathvec variables.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipath/main.c | 29 +++++++++++++++++++----------
 1 file changed, 19 insertions(+), 10 deletions(-)

diff --git a/multipath/main.c b/multipath/main.c
index 1949a1c..056e29a 100644
--- a/multipath/main.c
+++ b/multipath/main.c
@@ -93,7 +93,7 @@ void rcu_register_thread_memb(void) {}
 void rcu_unregister_thread_memb(void) {}
 
 static int
-filter_pathvec (vector pathvec, char * refwwid)
+filter_pathvec (vector pathvec, const char *refwwid)
 {
 	int i;
 	struct path * pp;
@@ -594,8 +594,9 @@ static int
 check_path_valid(const char *name, struct config *conf, bool is_uevent)
 {
 	int fd, r = PATH_IS_ERROR;
-	struct path *pp = NULL;
+	struct path *pp;
 	vector pathvec = NULL;
+	const char *wwid;
 
 	pp = alloc_path();
 	if (!pp)
@@ -665,13 +666,17 @@ check_path_valid(const char *name, struct config *conf, bool is_uevent)
 	if (store_path(pathvec, pp) != 0) {
 		free_path(pp);
 		goto fail;
+	} else {
+		/* make sure path isn't freed twice */
+		wwid = pp->wwid;
+		pp = NULL;
 	}
 
 	/* For find_multipaths = SMART, if there is more than one path
 	 * matching the refwwid, then the path is valid */
 	if (path_discovery(pathvec, DI_SYSFS | DI_WWID) < 0)
 		goto fail;
-	filter_pathvec(pathvec, pp->wwid);
+	filter_pathvec(pathvec, wwid);
 	if (VECTOR_SIZE(pathvec) > 1)
 		r = PATH_IS_VALID;
 	else
@@ -679,21 +684,25 @@ check_path_valid(const char *name, struct config *conf, bool is_uevent)
 
 out:
 	r = print_cmd_valid(r, pathvec, conf);
-	free_pathvec(pathvec, FREE_PATHS);
 	/*
 	 * multipath -u must exit with status 0, otherwise udev won't
 	 * import its output.
 	 */
 	if (!is_uevent && r == PATH_IS_NOT_VALID)
-		return RTVL_FAIL;
-	return RTVL_OK;
+		r = RTVL_FAIL;
+	else
+		r = RTVL_OK;
+	goto cleanup;
 
 fail:
-	if (pathvec)
+	r = RTVL_FAIL;
+
+cleanup:
+	if (pp != NULL)
+		free(pp);
+	if (pathvec != NULL)
 		free_pathvec(pathvec, FREE_PATHS);
-	else
-		free_path(pp);
-	return RTVL_FAIL;
+	return r;
 }
 
 static int
-- 
2.29.0


--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH v3 23/29] multipath-tools: mpath-tools.supp: file with valgrind suppressions

[mwilck]

From: Martin Wilck <mwilck@suse.com>

These leaks are caused by other libraries (libsystemd, glibc,
libgcrypt) and should be ignored when debugging with valgrind

Usage example:

valgrind --suppressions=mpath-tools.supp \
    --leak-check=full --show-leak-kinds=all $COMMAND

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 third-party/valgrind/mpath-tools.supp | 32 +++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)
 create mode 100644 third-party/valgrind/mpath-tools.supp

diff --git a/third-party/valgrind/mpath-tools.supp b/third-party/valgrind/mpath-tools.supp
new file mode 100644
index 0000000..0537fd5
--- /dev/null
+++ b/third-party/valgrind/mpath-tools.supp
@@ -0,0 +1,32 @@
+{
+   glibc _dlerror_run leak: https://stackoverflow.com/questions/1542457/memory-leak-reported-by-valgrind-in-dlopen
+   Memcheck:Leak
+   match-leak-kinds: reachable
+   fun:calloc
+   fun:_dlerror_run
+   fun:dlopen*
+}
+
+{
+   systemd mempools are never freed: https://bugzilla.redhat.com/show_bug.cgi?id=1215670
+   Memcheck:Leak
+   match-leak-kinds: reachable
+   fun:malloc
+   fun:mempool_alloc_tile
+   fun:mempool_alloc0_tile
+   fun:hashmap_base_new
+   fun:hashmap_base_ensure_allocated
+}
+
+{
+   libgcrypt library initialization
+   Memcheck:Leak
+   match-leak-kinds: reachable
+   fun:malloc
+   ...
+   fun:_gcry_xmalloc
+   ...
+   fun:global_init.*
+   ...
+   fun:_dl_init
+}
-- 
2.29.0


--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH v3 24/29] libmultipath: use libmp_verbosity to track verbosity

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Introduce a new global variable to set the verbosity of libmultipath.
This avoids accessing the configuration in every dlog() call.
When libmultipath reads its configuration in init_config() or
load_config(), it will use the current value of libmp_verbosity
for logging. Immediately before returning, libmp_verbosity will be
overwritten with the verbosity value from the configuration file,
if it was set there. An application is free to set libmp_verbosity
back to the previous value or not after that, depending on whether
command line options or configuration file settings should take
precedence.

Replace internal access to conf->verbosity with the new variable.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmpathpersist/mpath_persist.c   |  5 +---
 libmultipath/config.c             |  9 +++++--
 libmultipath/configure.c          | 16 +++----------
 libmultipath/debug.c              | 10 ++------
 libmultipath/debug.h              |  1 +
 libmultipath/devmapper.c          |  7 +-----
 libmultipath/libmultipath.version |  5 ++++
 multipath/main.c                  | 21 ++++++----------
 multipathd/main.c                 | 40 ++++++++++++++++++-------------
 tests/alias.c                     |  1 +
 tests/blacklist.c                 |  2 ++
 11 files changed, 53 insertions(+), 64 deletions(-)

diff --git a/libmpathpersist/mpath_persist.c b/libmpathpersist/mpath_persist.c
index 9ebf91d..79322e8 100644
--- a/libmpathpersist/mpath_persist.c
+++ b/libmpathpersist/mpath_persist.c
@@ -170,10 +170,7 @@ void mpath_persistent_reserve_free_vecs(void)
 
 int mpath_persistent_reserve_init_vecs(int verbose)
 {
-	struct config *conf = get_multipath_config();
-
-	conf->verbosity = verbose;
-	put_multipath_config(conf);
+	libmp_verbosity = verbose;
 
 	if (curmp)
 		return MPATH_PR_SUCCESS;
diff --git a/libmultipath/config.c b/libmultipath/config.c
index 52b1447..49e7fb8 100644
--- a/libmultipath/config.c
+++ b/libmultipath/config.c
@@ -828,10 +828,14 @@ int _init_config (const char *file, struct config *conf)
 		conf = &__internal_config;
 
 	/*
-	 * internal defaults
+	 * Processing the config file will overwrite conf->verbosity if set
+	 * When we return, we'll copy the config value back
 	 */
-	conf->verbosity = DEFAULT_VERBOSITY;
+	conf->verbosity = libmp_verbosity;
 
+	/*
+	 * internal defaults
+	 */
 	get_sys_max_fds(&conf->max_fds);
 	conf->bindings_file = set_default(DEFAULT_BINDINGS_FILE);
 	conf->wwids_file = set_default(DEFAULT_WWIDS_FILE);
@@ -997,6 +1001,7 @@ int _init_config (const char *file, struct config *conf)
 	    !conf->wwids_file || !conf->prkeys_file)
 		goto out;
 
+	libmp_verbosity = conf->verbosity;
 	return 0;
 out:
 	_uninit_config(conf);
diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index 1c8aac0..3dbc1f1 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -934,16 +934,12 @@ int domap(struct multipath *mpp, char *params, int is_daemon)
 {
 	int r = DOMAP_FAIL;
 	struct config *conf;
-	int verbosity;
 
 	/*
 	 * last chance to quit before touching the devmaps
 	 */
 	if (mpp->action == ACT_DRY_RUN) {
-		conf = get_multipath_config();
-		verbosity = conf->verbosity;
-		put_multipath_config(conf);
-		print_multipath_topology(mpp, verbosity);
+		print_multipath_topology(mpp, libmp_verbosity);
 		return DOMAP_DRY;
 	}
 
@@ -1311,14 +1307,8 @@ int coalesce_paths (struct vectors * vecs, vector newmp, char * refwwid,
 					       "queue_if_no_path");
 		}
 
-		if (!is_daemon && mpp->action != ACT_NOTHING) {
-			int verbosity;
-
-			conf = get_multipath_config();
-			verbosity = conf->verbosity;
-			put_multipath_config(conf);
-			print_multipath_topology(mpp, verbosity);
-		}
+		if (!is_daemon && mpp->action != ACT_NOTHING)
+			print_multipath_topology(mpp, libmp_verbosity);
 
 		if (newmp) {
 			if (mpp->action != ACT_REJECT) {
diff --git a/libmultipath/debug.c b/libmultipath/debug.c
index b3a1de9..a1713b9 100644
--- a/libmultipath/debug.c
+++ b/libmultipath/debug.c
@@ -16,21 +16,15 @@
 #include "debug.h"
 
 int logsink;
+int libmp_verbosity = DEFAULT_VERBOSITY;
 
 void dlog (int sink, int prio, const char * fmt, ...)
 {
 	va_list ap;
-	int thres;
-	struct config *conf;
 
 	va_start(ap, fmt);
-	conf = get_multipath_config();
-	ANNOTATE_IGNORE_READS_BEGIN();
-	thres = (conf) ? conf->verbosity : DEFAULT_VERBOSITY;
-	ANNOTATE_IGNORE_READS_END();
-	put_multipath_config(conf);
 
-	if (prio <= thres) {
+	if (prio <= libmp_verbosity) {
 		if (sink < 1) {
 			if (sink == 0) {
 				time_t t = time(NULL);
diff --git a/libmultipath/debug.h b/libmultipath/debug.h
index c6120c1..1f3bc8b 100644
--- a/libmultipath/debug.h
+++ b/libmultipath/debug.h
@@ -8,6 +8,7 @@ void dlog (int sink, int prio, const char * fmt, ...)
 #include "log_pthread.h"
 
 extern int logsink;
+extern int libmp_verbosity;
 
 #define condlog(prio, fmt, args...) \
 	dlog(logsink, prio, fmt "\n", ##args)
diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
index e60ab49..dfe95d2 100644
--- a/libmultipath/devmapper.c
+++ b/libmultipath/devmapper.c
@@ -351,16 +351,11 @@ void libmp_dm_exit(void)
 
 static void libmp_dm_init(void)
 {
-	struct config *conf;
-	int verbosity;
 	unsigned int version[3];
 
 	if (dm_prereq(version))
 		exit(1);
-	conf = get_multipath_config();
-	verbosity = conf->verbosity;
-	put_multipath_config(conf);
-	dm_init(verbosity);
+	dm_init(libmp_verbosity);
 #ifdef LIBDM_API_HOLD_CONTROL
 	dm_hold_control_dev(1);
 #endif
diff --git a/libmultipath/libmultipath.version b/libmultipath/libmultipath.version
index 800cff2..67a7379 100644
--- a/libmultipath/libmultipath.version
+++ b/libmultipath/libmultipath.version
@@ -259,3 +259,8 @@ global:
 local:
 	*;
 };
+
+LIBMULTIPATH_4.1.0 {
+global:
+	libmp_verbosity;
+} LIBMULTIPATH_4.0.0;
diff --git a/multipath/main.c b/multipath/main.c
index 056e29a..371be2a 100644
--- a/multipath/main.c
+++ b/multipath/main.c
@@ -208,22 +208,15 @@ get_dm_mpvec (enum mpath_cmds cmd, vector curmp, vector pathvec, char * refwwid)
 			mpp->bestpg = select_path_group(mpp);
 
 		if (cmd == CMD_LIST_SHORT ||
-		    cmd == CMD_LIST_LONG) {
-			struct config *conf = get_multipath_config();
-			print_multipath_topology(mpp, conf->verbosity);
-			put_multipath_config(conf);
-		}
+		    cmd == CMD_LIST_LONG)
+			print_multipath_topology(mpp, libmp_verbosity);
 
 		if (cmd == CMD_CREATE)
 			reinstate_paths(mpp);
 	}
 
-	if (cmd == CMD_LIST_SHORT || cmd == CMD_LIST_LONG) {
-		struct config *conf = get_multipath_config();
-
-		print_foreign_topology(conf->verbosity);
-		put_multipath_config(conf);
-	}
+	if (cmd == CMD_LIST_SHORT || cmd == CMD_LIST_LONG)
+		print_foreign_topology(libmp_verbosity);
 
 	return 0;
 }
@@ -552,7 +545,7 @@ configure (struct config *conf, enum mpath_cmds cmd,
 	if (path_discovery(pathvec, di_flag) < 0)
 		goto out;
 
-	if (conf->verbosity > 2)
+	if (libmp_verbosity > 2)
 		print_all_paths(pathvec, 1);
 
 	get_path_layout(pathvec, 0);
@@ -842,7 +835,7 @@ main (int argc, char *argv[])
 				exit(RTVL_FAIL);
 			}
 
-			conf->verbosity = atoi(optarg);
+			libmp_verbosity = atoi(optarg);
 			break;
 		case 'b':
 			conf->bindings_file = strdup(optarg);
@@ -973,7 +966,7 @@ main (int argc, char *argv[])
 	}
 	if (dev_type == DEV_UEVENT) {
 		openlog("multipath", 0, LOG_DAEMON);
-		setlogmask(LOG_UPTO(conf->verbosity + 3));
+		setlogmask(LOG_UPTO(libmp_verbosity + 3));
 		logsink = 1;
 	}
 
diff --git a/multipathd/main.c b/multipathd/main.c
index 4de0978..ba25751 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -88,10 +88,10 @@
 #define CMDSIZE 160
 #define MSG_SIZE 32
 
-#define LOG_MSG(lvl, verb, pp)					\
+#define LOG_MSG(lvl, pp)					\
 do {								\
 	if (pp->mpp && checker_selected(&pp->checker) &&	\
-	    lvl <= verb) {					\
+	    lvl <= libmp_verbosity) {					\
 		if (pp->offline)				\
 			condlog(lvl, "%s: %s - path offline",	\
 				pp->mpp->alias, pp->dev);	\
@@ -2070,7 +2070,7 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks)
 	int chkr_new_path_up = 0;
 	int disable_reinstate = 0;
 	int oldchkrstate = pp->chkrstate;
-	int retrigger_tries, verbosity;
+	int retrigger_tries;
 	unsigned int checkint, max_checkint;
 	struct config *conf;
 	int marginal_pathgroups, marginal_changed = 0;
@@ -2090,7 +2090,6 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks)
 	retrigger_tries = conf->retrigger_tries;
 	checkint = conf->checkint;
 	max_checkint = conf->max_checkint;
-	verbosity = conf->verbosity;
 	marginal_pathgroups = conf->marginal_pathgroups;
 	put_multipath_config(conf);
 
@@ -2152,7 +2151,7 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks)
 	if (newstate == PATH_WILD || newstate == PATH_UNCHECKED) {
 		condlog(2, "%s: unusable path (%s) - checker failed",
 			pp->dev, checker_state_name(newstate));
-		LOG_MSG(2, verbosity, pp);
+		LOG_MSG(2, pp);
 		conf = get_multipath_config();
 		pthread_cleanup_push(put_multipath_config, conf);
 		pathinfo(pp, conf, 0);
@@ -2257,7 +2256,7 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks)
 		int oldstate = pp->state;
 		pp->state = newstate;
 
-		LOG_MSG(1, verbosity, pp);
+		LOG_MSG(1, pp);
 
 		/*
 		 * upon state change, reset the checkint
@@ -2321,7 +2320,7 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks)
 			/* Clear IO errors */
 			reinstate_path(pp);
 		else {
-			LOG_MSG(4, verbosity, pp);
+			LOG_MSG(4, pp);
 			if (pp->checkint != max_checkint) {
 				/*
 				 * double the next check delay.
@@ -2349,9 +2348,9 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks)
 			log_checker_err = conf->log_checker_err;
 			put_multipath_config(conf);
 			if (log_checker_err == LOG_CHKR_ERR_ONCE)
-				LOG_MSG(3, verbosity, pp);
+				LOG_MSG(3, pp);
 			else
-				LOG_MSG(2, verbosity, pp);
+				LOG_MSG(2, pp);
 		}
 	}
 
@@ -2696,6 +2695,10 @@ reconfigure (struct vectors * vecs)
 	if (!conf)
 		return 1;
 
+	if (verbosity)
+		libmp_verbosity = verbosity;
+	setlogmask(LOG_UPTO(libmp_verbosity + 3));
+
 	/*
 	 * free old map and path vectors ... they use old conf state
 	 */
@@ -2710,8 +2713,6 @@ reconfigure (struct vectors * vecs)
 	/* Re-read any timezone changes */
 	tzset();
 
-	if (verbosity)
-		conf->verbosity = verbosity;
 	if (bindings_read_only)
 		conf->bindings_read_only = bindings_read_only;
 	check_alias_settings(conf);
@@ -3091,14 +3092,18 @@ child (__attribute__((unused)) void *param)
 	condlog(2, "--------start up--------");
 	condlog(2, "read " DEFAULT_CONFIGFILE);
 
+	if (verbosity)
+		libmp_verbosity = verbosity;
 	conf = load_config(DEFAULT_CONFIGFILE);
+	if (verbosity)
+		libmp_verbosity = verbosity;
+	setlogmask(LOG_UPTO(libmp_verbosity + 3));
+
 	if (!conf) {
 		condlog(0, "failed to load configuration");
 		goto failed;
 	}
 
-	if (verbosity)
-		conf->verbosity = verbosity;
 	if (bindings_read_only)
 		conf->bindings_read_only = bindings_read_only;
 	uxsock_timeout = conf->uxsock_timeout;
@@ -3117,7 +3122,6 @@ child (__attribute__((unused)) void *param)
 
 	if (poll_dmevents)
 		poll_dmevents = dmevent_poll_supported();
-	setlogmask(LOG_UPTO(conf->verbosity + 3));
 
 	envp = getenv("LimitNOFILE");
 
@@ -3339,7 +3343,7 @@ main (int argc, char *argv[])
 			    !isdigit(optarg[0]))
 				exit(1);
 
-			verbosity = atoi(optarg);
+			libmp_verbosity = verbosity = atoi(optarg);
 			break;
 		case 's':
 			logsink = -1;
@@ -3350,7 +3354,7 @@ main (int argc, char *argv[])
 			if (!conf)
 				exit(1);
 			if (verbosity)
-				conf->verbosity = verbosity;
+				libmp_verbosity = verbosity;
 			uxsock_timeout = conf->uxsock_timeout;
 			err = uxclnt(optarg, uxsock_timeout + 100);
 			free_config(conf);
@@ -3376,11 +3380,13 @@ main (int argc, char *argv[])
 		char * c = s;
 
 		logsink = 0;
+		if (verbosity)
+			libmp_verbosity = verbosity;
 		conf = load_config(DEFAULT_CONFIGFILE);
 		if (!conf)
 			exit(1);
 		if (verbosity)
-			conf->verbosity = verbosity;
+			libmp_verbosity = verbosity;
 		uxsock_timeout = conf->uxsock_timeout;
 		memset(cmd, 0x0, CMDSIZE);
 		while (optind < argc) {
diff --git a/tests/alias.c b/tests/alias.c
index 7fda679..0311faa 100644
--- a/tests/alias.c
+++ b/tests/alias.c
@@ -735,6 +735,7 @@ static int test_allocate_binding(void)
 int main(void)
 {
 	int ret = 0;
+	libmp_verbosity = conf.verbosity;
 
 	ret += test_format_devname();
 	ret += test_scan_devname();
diff --git a/tests/blacklist.c b/tests/blacklist.c
index 84a3ba2..0b42e25 100644
--- a/tests/blacklist.c
+++ b/tests/blacklist.c
@@ -22,6 +22,7 @@
 #include "globals.c"
 #include "blacklist.h"
 #include "test-log.h"
+#include "debug.h"
 
 struct udev_device {
 	const char *sysname;
@@ -152,6 +153,7 @@ static int setup(void **state)
 	    store_ble(blist_property_wwn_inv, "!ID_WWN", ORIGIN_CONFIG))
 		return -1;
 
+	libmp_verbosity = conf.verbosity = 4;
 	return 0;
 }
 
-- 
2.29.0


--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH v3 25/29] libmultipath: introduce symbolic values for logsink

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/debug.c     |  4 ++--
 libmultipath/debug.h     |  6 ++++++
 libmultipath/devmapper.c |  4 ++--
 multipath/main.c         |  4 ++--
 multipathd/main.c        | 17 ++++++++---------
 tests/globals.c          |  3 ++-
 tests/hwtable.c          |  2 +-
 7 files changed, 23 insertions(+), 17 deletions(-)

diff --git a/libmultipath/debug.c b/libmultipath/debug.c
index a1713b9..f9b7755 100644
--- a/libmultipath/debug.c
+++ b/libmultipath/debug.c
@@ -25,8 +25,8 @@ void dlog (int sink, int prio, const char * fmt, ...)
 	va_start(ap, fmt);
 
 	if (prio <= libmp_verbosity) {
-		if (sink < 1) {
-			if (sink == 0) {
+		if (sink != LOGSINK_SYSLOG) {
+			if (sink == LOGSINK_STDERR_WITH_TIME) {
 				time_t t = time(NULL);
 				struct tm *tb = localtime(&t);
 				char buff[16];
diff --git a/libmultipath/debug.h b/libmultipath/debug.h
index 1f3bc8b..b6ce70a 100644
--- a/libmultipath/debug.h
+++ b/libmultipath/debug.h
@@ -12,3 +12,9 @@ extern int libmp_verbosity;
 
 #define condlog(prio, fmt, args...) \
 	dlog(logsink, prio, fmt "\n", ##args)
+
+enum {
+	LOGSINK_STDERR_WITH_TIME = 0,
+	LOGSINK_STDERR_WITHOUT_TIME = -1,
+	LOGSINK_SYSLOG = 1,
+};
diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
index dfe95d2..f8b180e 100644
--- a/libmultipath/devmapper.c
+++ b/libmultipath/devmapper.c
@@ -104,8 +104,8 @@ dm_write_log (int level, const char *file, int line, const char *f, ...)
 		return;
 
 	va_start(ap, f);
-	if (logsink < 1) {
-		if (logsink == 0) {
+	if (logsink != LOGSINK_SYSLOG) {
+		if (logsink == LOGSINK_STDERR_WITH_TIME) {
 			time_t t = time(NULL);
 			struct tm *tb = localtime(&t);
 			char buff[16];
diff --git a/multipath/main.c b/multipath/main.c
index 371be2a..c94bb26 100644
--- a/multipath/main.c
+++ b/multipath/main.c
@@ -816,7 +816,7 @@ main (int argc, char *argv[])
 	libmultipath_init();
 	if (atexit(dm_lib_exit) || atexit(libmultipath_exit))
 		condlog(1, "failed to register cleanup handler for libmultipath: %m");
-	logsink = 0;
+	logsink = LOGSINK_STDERR_WITH_TIME;
 	if (init_config(DEFAULT_CONFIGFILE))
 		exit(RTVL_FAIL);
 	if (atexit(uninit_config))
@@ -967,7 +967,7 @@ main (int argc, char *argv[])
 	if (dev_type == DEV_UEVENT) {
 		openlog("multipath", 0, LOG_DAEMON);
 		setlogmask(LOG_UPTO(libmp_verbosity + 3));
-		logsink = 1;
+		logsink = LOGSINK_SYSLOG;
 	}
 
 	set_max_fds(conf->max_fds);
diff --git a/multipathd/main.c b/multipathd/main.c
index ba25751..867f0f8 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2780,7 +2780,7 @@ handle_signals(bool nonfatal)
 	}
 	if (log_reset_sig) {
 		condlog(2, "reset log (signal)");
-		if (logsink == 1)
+		if (logsink == LOGSINK_SYSLOG)
 			log_thread_reset();
 	}
 	reconfig_sig = 0;
@@ -3033,7 +3033,7 @@ static void cleanup_child(void)
 		cleanup_dmevent_waiter();
 
 	cleanup_pidfile();
-	if (logsink == 1)
+	if (logsink == LOGSINK_SYSLOG)
 		log_thread_stop();
 
 	cleanup_conf();
@@ -3076,7 +3076,7 @@ child (__attribute__((unused)) void *param)
 	setup_thread_attr(&uevent_attr, DEFAULT_UEVENT_STACKSIZE * 1024, 0);
 	setup_thread_attr(&waiter_attr, 32 * 1024, 1);
 
-	if (logsink == 1) {
+	if (logsink == LOGSINK_SYSLOG) {
 		setup_thread_attr(&log_attr, 64 * 1024, 0);
 		log_thread_start(&log_attr);
 		pthread_attr_destroy(&log_attr);
@@ -3307,7 +3307,7 @@ main (int argc, char *argv[])
 	ANNOTATE_BENIGN_RACE_SIZED(&uxsock_timeout, sizeof(uxsock_timeout),
 		"Suppress complaints about this scalar variable");
 
-	logsink = 1;
+	logsink = LOGSINK_SYSLOG;
 
 	if (getuid() != 0) {
 		fprintf(stderr, "need to be root\n");
@@ -3334,9 +3334,8 @@ main (int argc, char *argv[])
 		switch(arg) {
 		case 'd':
 			foreground = 1;
-			if (logsink > 0)
-				logsink = 0;
-			//debug=1; /* ### comment me out ### */
+			if (logsink == LOGSINK_SYSLOG)
+				logsink = LOGSINK_STDERR_WITH_TIME;
 			break;
 		case 'v':
 			if (sizeof(optarg) > sizeof(char *) ||
@@ -3346,7 +3345,7 @@ main (int argc, char *argv[])
 			libmp_verbosity = verbosity = atoi(optarg);
 			break;
 		case 's':
-			logsink = -1;
+			logsink = LOGSINK_STDERR_WITHOUT_TIME;
 			break;
 		case 'k':
 			logsink = 0;
@@ -3379,7 +3378,7 @@ main (int argc, char *argv[])
 		char * s = cmd;
 		char * c = s;
 
-		logsink = 0;
+		logsink = LOGSINK_STDERR_WITH_TIME;
 		if (verbosity)
 			libmp_verbosity = verbosity;
 		conf = load_config(DEFAULT_CONFIGFILE);
diff --git a/tests/globals.c b/tests/globals.c
index 8add5eb..fc0c07a 100644
--- a/tests/globals.c
+++ b/tests/globals.c
@@ -1,9 +1,10 @@
 #include "structs.h"
 #include "config.h"
+#include "debug.h"
 
 /* Required globals */
 struct udev *udev;
-int logsink = -1;
+int logsink = LOGSINK_STDERR_WITHOUT_TIME;
 struct config conf = {
 	.verbosity = 4,
 };
diff --git a/tests/hwtable.c b/tests/hwtable.c
index 57f832b..4dd0873 100644
--- a/tests/hwtable.c
+++ b/tests/hwtable.c
@@ -53,7 +53,7 @@ struct hwt_state {
 
 static struct config *_conf;
 struct udev *udev;
-int logsink = -1;
+int logsink = LOGSINK_STDERR_WITHOUT_TIME;
 
 struct config *get_multipath_config(void)
 {
-- 
2.29.0


--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH v3 26/29] libmultipath: simplify dlog()

[mwilck]

From: Martin Wilck <mwilck@suse.com>

By checking the log level in condlog() directly, we can simplify
dlog(). Also, it's now possible to limit the log level at compile
time by setting MAX_VERBOSITY, enabling the compiler to optimize
away log messages with higher loglevel.

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/debug.c     | 30 +++++++++++++-----------------
 libmultipath/debug.h     | 20 ++++++++++++++++----
 libmultipath/devmapper.c |  4 +++-
 tests/test-log.c         |  4 ++--
 tests/test-log.h         |  3 ++-
 5 files changed, 36 insertions(+), 25 deletions(-)

diff --git a/libmultipath/debug.c b/libmultipath/debug.c
index f9b7755..429f269 100644
--- a/libmultipath/debug.c
+++ b/libmultipath/debug.c
@@ -18,29 +18,25 @@
 int logsink;
 int libmp_verbosity = DEFAULT_VERBOSITY;
 
-void dlog (int sink, int prio, const char * fmt, ...)
+void dlog(int prio, const char * fmt, ...)
 {
 	va_list ap;
 
 	va_start(ap, fmt);
+	if (logsink != LOGSINK_SYSLOG) {
+		if (logsink == LOGSINK_STDERR_WITH_TIME) {
+			time_t t = time(NULL);
+			struct tm *tb = localtime(&t);
+			char buff[16];
 
-	if (prio <= libmp_verbosity) {
-		if (sink != LOGSINK_SYSLOG) {
-			if (sink == LOGSINK_STDERR_WITH_TIME) {
-				time_t t = time(NULL);
-				struct tm *tb = localtime(&t);
-				char buff[16];
-
-				strftime(buff, sizeof(buff),
-					 "%b %d %H:%M:%S", tb);
-				buff[sizeof(buff)-1] = '\0';
-
-				fprintf(stderr, "%s | ", buff);
-			}
-			vfprintf(stderr, fmt, ap);
+			strftime(buff, sizeof(buff),
+				 "%b %d %H:%M:%S", tb);
+			buff[sizeof(buff)-1] = '\0';
+			fprintf(stderr, "%s | ", buff);
 		}
-		else
-			log_safe(prio + 3, fmt, ap);
+		vfprintf(stderr, fmt, ap);
 	}
+	else
+		log_safe(prio + 3, fmt, ap);
 	va_end(ap);
 }
diff --git a/libmultipath/debug.h b/libmultipath/debug.h
index b6ce70a..705a5d7 100644
--- a/libmultipath/debug.h
+++ b/libmultipath/debug.h
@@ -1,5 +1,7 @@
-void dlog (int sink, int prio, const char * fmt, ...)
-	__attribute__((format(printf, 3, 4)));
+#ifndef _DEBUG_H
+#define _DEBUG_H
+void dlog (int prio, const char *fmt, ...)
+	__attribute__((format(printf, 2, 3)));
 
 
 #include <pthread.h>
@@ -10,11 +12,21 @@ void dlog (int sink, int prio, const char * fmt, ...)
 extern int logsink;
 extern int libmp_verbosity;
 
-#define condlog(prio, fmt, args...) \
-	dlog(logsink, prio, fmt "\n", ##args)
+#ifndef MAX_VERBOSITY
+#define MAX_VERBOSITY 4
+#endif
 
 enum {
 	LOGSINK_STDERR_WITH_TIME = 0,
 	LOGSINK_STDERR_WITHOUT_TIME = -1,
 	LOGSINK_SYSLOG = 1,
 };
+
+#define condlog(prio, fmt, args...)					\
+	do {								\
+		int __p = (prio);					\
+									\
+		if (__p <= MAX_VERBOSITY && __p <= libmp_verbosity)	\
+			dlog(__p, fmt "\n", ##args);			\
+	} while (0)
+#endif /* _DEBUG_H */
diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
index f8b180e..4977b31 100644
--- a/libmultipath/devmapper.c
+++ b/libmultipath/devmapper.c
@@ -276,7 +276,9 @@ static int dm_tgt_prereq (unsigned int *ver)
 
 static void _init_versions(void)
 {
-	dlog(logsink, 3, VERSION_STRING);
+	/* Can't use condlog here because of how VERSION_STRING is defined */
+	if (3 <= libmp_verbosity)
+		dlog(3, VERSION_STRING);
 	init_dm_library_version();
 	init_dm_drv_version();
 	init_dm_mpath_version();
diff --git a/tests/test-log.c b/tests/test-log.c
index 1c901cb..14f25b9 100644
--- a/tests/test-log.c
+++ b/tests/test-log.c
@@ -7,8 +7,8 @@
 #include "log.h"
 #include "test-log.h"
 
-__attribute__((format(printf, 3, 0)))
-void __wrap_dlog (int sink, int prio, const char * fmt, ...)
+__attribute__((format(printf, 2, 0)))
+void __wrap_dlog (int prio, const char * fmt, ...)
 {
 	char buff[MAX_MSG_SIZE];
 	va_list ap;
diff --git a/tests/test-log.h b/tests/test-log.h
index 2c878c6..6d22cd2 100644
--- a/tests/test-log.h
+++ b/tests/test-log.h
@@ -1,7 +1,8 @@
 #ifndef _TEST_LOG_H
 #define _TEST_LOG_H
 
-void __wrap_dlog (int sink, int prio, const char * fmt, ...);
+__attribute__((format(printf, 2, 0)))
+void __wrap_dlog (int prio, const char * fmt, ...);
 void expect_condlog(int prio, char *string);
 
 #endif
-- 
2.29.0


--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH v3 27/29] multipathd: common code for "-k" and command args

[mwilck]

From: Martin Wilck <mwilck@suse.com>

'multipathd -k"cmd"' and 'multipath cmd' are the same thing.
Treat it with common code.

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

diff --git a/multipathd/main.c b/multipathd/main.c
index 867f0f8..b6a5f5b 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -3301,6 +3301,8 @@ main (int argc, char *argv[])
 	int err;
 	int foreground = 0;
 	struct config *conf;
+	char *opt_k_arg = NULL;
+	bool opt_k = false;
 
 	ANNOTATE_BENIGN_RACE_SIZED(&multipath_conf, sizeof(multipath_conf),
 				   "Manipulated through RCU");
@@ -3348,16 +3350,9 @@ main (int argc, char *argv[])
 			logsink = LOGSINK_STDERR_WITHOUT_TIME;
 			break;
 		case 'k':
-			logsink = 0;
-			conf = load_config(DEFAULT_CONFIGFILE);
-			if (!conf)
-				exit(1);
-			if (verbosity)
-				libmp_verbosity = verbosity;
-			uxsock_timeout = conf->uxsock_timeout;
-			err = uxclnt(optarg, uxsock_timeout + 100);
-			free_config(conf);
-			return err;
+			opt_k = true;
+			opt_k_arg = optarg;
+			break;
 		case 'B':
 			bindings_read_only = 1;
 			break;
@@ -3373,7 +3368,7 @@ main (int argc, char *argv[])
 			exit(1);
 		}
 	}
-	if (optind < argc) {
+	if (opt_k || optind < argc) {
 		char cmd[CMDSIZE];
 		char * s = cmd;
 		char * c = s;
@@ -3388,14 +3383,20 @@ main (int argc, char *argv[])
 			libmp_verbosity = verbosity;
 		uxsock_timeout = conf->uxsock_timeout;
 		memset(cmd, 0x0, CMDSIZE);
-		while (optind < argc) {
-			if (strchr(argv[optind], ' '))
-				c += snprintf(c, s + CMDSIZE - c, "\"%s\" ", argv[optind]);
-			else
-				c += snprintf(c, s + CMDSIZE - c, "%s ", argv[optind]);
-			optind++;
+		if (opt_k)
+			s = opt_k_arg;
+		else {
+			while (optind < argc) {
+				if (strchr(argv[optind], ' '))
+					c += snprintf(c, s + CMDSIZE - c,
+						      "\"%s\" ", argv[optind]);
+				else
+					c += snprintf(c, s + CMDSIZE - c,
+						      "%s ", argv[optind]);
+				optind++;
+			}
+			c += snprintf(c, s + CMDSIZE - c, "\n");
 		}
-		c += snprintf(c, s + CMDSIZE - c, "\n");
 		err = uxclnt(s, uxsock_timeout + 100);
 		free_config(conf);
 		return err;
-- 
2.29.0


--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH v3 28/29] multipathd: sanitize uxsock_listen()

[mwilck]

From: Martin Wilck <mwilck@suse.com>

We were allocating 1025 poll fds, which is weird. Change it to a power of two,
and make this more easily customizable in general. Use POLLFDS_BASE rather
than the hard-coded "2" for the number of fds we poll besides client
connections.  Introduce a maximum number of clients that can connect. When
this number is reached, we simply stop polling the accept socket, so that new
connections aren't accepted any more.  Don't attempt to realloc() the pollfd
array if the number of clients decreases. It's unlikely to ever be more than
one or two pages. Finally, there's no need to wake up every 5s. Our signal
handling is robust. Just sleep forever in ppoll() if nothing happens.

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

diff --git a/multipathd/uxlsnr.c b/multipathd/uxlsnr.c
index ce2b680..cd462b6 100644
--- a/multipathd/uxlsnr.c
+++ b/multipathd/uxlsnr.c
@@ -41,14 +41,25 @@
 #include "cli.h"
 #include "uxlsnr.h"
 
-static struct timespec sleep_time = {5, 0};
-
 struct client {
 	struct list_head node;
 	int fd;
 };
 
-#define MIN_POLLS 1023
+/* The number of fds we poll on, other than individual client connections */
+#define POLLFDS_BASE 2
+#define POLLFD_CHUNK (4096 / sizeof(struct pollfd))
+/* Minimum mumber of pollfds to reserve for clients */
+#define MIN_POLLS (POLLFD_CHUNK - POLLFDS_BASE)
+/*
+ * Max number of client connections allowed
+ * During coldplug, there may be a large number of "multipath -u"
+ * processes connecting.
+ */
+#define MAX_CLIENTS (16384 - POLLFDS_BASE)
+
+/* Compile-time error if POLLFD_CHUNK is too small */
+static __attribute__((unused)) char ___a[-(MIN_POLLS <= 0)];
 
 static LIST_HEAD(clients);
 static pthread_mutex_t client_lock = PTHREAD_MUTEX_INITIALIZER;
@@ -282,13 +293,13 @@ void * uxsock_listen(uxsock_trigger_fn uxsock_trigger, long ux_sock,
 	char *inbuf;
 	char *reply;
 	sigset_t mask;
-	int old_clients = MIN_POLLS;
+	int max_pfds = MIN_POLLS + POLLFDS_BASE;
 	/* conf->sequence_nr will be 1 when uxsock_listen is first called */
 	unsigned int sequence_nr = 0;
 	struct watch_descriptors wds = { .conf_wd = -1, .dir_wd = -1 };
 
 	condlog(3, "uxsock: startup listener");
-	polls = (struct pollfd *)MALLOC((MIN_POLLS + 2) * sizeof(struct pollfd));
+	polls = MALLOC(max_pfds * sizeof(*polls));
 	if (!polls) {
 		condlog(0, "uxsock: failed to allocate poll fds");
 		exit_daemon();
@@ -312,28 +323,33 @@ void * uxsock_listen(uxsock_trigger_fn uxsock_trigger, long ux_sock,
 		list_for_each_entry(c, &clients, node) {
 			num_clients++;
 		}
-		if (num_clients != old_clients) {
+		if (num_clients + POLLFDS_BASE > max_pfds) {
 			struct pollfd *new;
-			if (num_clients <= MIN_POLLS && old_clients > MIN_POLLS) {
-				new = REALLOC(polls, (2 + MIN_POLLS) *
-						sizeof(struct pollfd));
-			} else if (num_clients <= MIN_POLLS && old_clients <= MIN_POLLS) {
-				new = polls;
-			} else {
-				new = REALLOC(polls, (2 + num_clients) *
-						sizeof(struct pollfd));
-			}
-			if (!new) {
-				condlog(0, "%s: failed to realloc %d poll fds",
-					"uxsock", 2 + num_clients);
-				num_clients = old_clients;
-			} else {
-				old_clients = num_clients;
+			int n_new = max_pfds + POLLFD_CHUNK;
+
+			new = REALLOC(polls, n_new * sizeof(*polls));
+			if (new) {
+				max_pfds = n_new;
 				polls = new;
+			} else {
+				condlog(1, "%s: realloc failure, %d clients not served",
+					__func__,
+					num_clients + POLLFDS_BASE - max_pfds);
+				num_clients = max_pfds - POLLFDS_BASE;
 			}
 		}
-		polls[0].fd = ux_sock;
-		polls[0].events = POLLIN;
+		if (num_clients < MAX_CLIENTS) {
+			polls[0].fd = ux_sock;
+			polls[0].events = POLLIN;
+		} else {
+			/*
+			 * New clients can't connect, num_clients won't grow
+			 * to MAX_CLIENTS or higher
+			 */
+			condlog(1, "%s: max client connections reached, pausing polling",
+				__func__);
+			polls[0].fd = -1;
+		}
 
 		reset_watch(notify_fd, &wds, &sequence_nr);
 		if (notify_fd == -1 || (wds.conf_wd == -1 && wds.dir_wd == -1))
@@ -343,19 +359,19 @@ void * uxsock_listen(uxsock_trigger_fn uxsock_trigger, long ux_sock,
 		polls[1].events = POLLIN;
 
 		/* setup the clients */
-		i = 2;
+		i = POLLFDS_BASE;
 		list_for_each_entry(c, &clients, node) {
 			polls[i].fd = c->fd;
 			polls[i].events = POLLIN;
 			i++;
-			if (i >= 2 + num_clients)
+			if (i >= max_pfds)
 				break;
 		}
 		n_pfds = i;
 		pthread_cleanup_pop(1);
 
 		/* most of our life is spent in this call */
-		poll_count = ppoll(polls, n_pfds, &sleep_time, &mask);
+		poll_count = ppoll(polls, n_pfds, NULL, &mask);
 
 		handle_signals(false);
 		if (poll_count == -1) {
@@ -388,7 +404,7 @@ void * uxsock_listen(uxsock_trigger_fn uxsock_trigger, long ux_sock,
 		}
 
 		/* see if a client wants to speak to us */
-		for (i = 2; i < n_pfds; i++) {
+		for (i = POLLFDS_BASE; i < n_pfds; i++) {
 			if (polls[i].revents & POLLIN) {
 				struct timespec start_time;
 
-- 
2.29.0


--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH v3 29/29] libmultipath: fix race between log_safe and log_thread_stop()

[mwilck]

From: Martin Wilck <mwilck@suse.com>

log_safe() could race with log_thread_stop(); simply
checking the value of log_thr has never been safe. By converting the
mutexes to static initializers, we avoid having to destroy them, and thus
possibly accessing a destroyed mutex in log_safe(). Furthermore, taking
both the logev_lock and the logq_lock makes sure the logarea isn't freed
while we are writing to it.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/log_pthread.c | 48 +++++++++++++++++++++-----------------
 1 file changed, 26 insertions(+), 22 deletions(-)

diff --git a/libmultipath/log_pthread.c b/libmultipath/log_pthread.c
index 3a2566a..0d48c52 100644
--- a/libmultipath/log_pthread.c
+++ b/libmultipath/log_pthread.c
@@ -17,31 +17,42 @@
 
 static pthread_t log_thr;
 
-static pthread_mutex_t logq_lock;
-static pthread_mutex_t logev_lock;
-static pthread_cond_t logev_cond;
+/* logev_lock must not be taken with logq_lock held */
+static pthread_mutex_t logq_lock = PTHREAD_MUTEX_INITIALIZER;
+static pthread_mutex_t logev_lock = PTHREAD_MUTEX_INITIALIZER;
+static pthread_cond_t logev_cond = PTHREAD_COND_INITIALIZER;
 
 static int logq_running;
 static int log_messages_pending;
 
 void log_safe (int prio, const char * fmt, va_list ap)
 {
+	bool running;
+
 	if (prio > LOG_DEBUG)
 		prio = LOG_DEBUG;
 
-	if (log_thr == (pthread_t)0) {
-		vsyslog(prio, fmt, ap);
-		return;
-	}
+	/*
+	 * logev_lock protects logq_running. By holding it, we avoid a race
+	 * with log_thread_stop() -> log_close(), which would free the logarea.
+	 */
+	pthread_mutex_lock(&logev_lock);
+	pthread_cleanup_push(cleanup_mutex, &logev_lock);
+	running = logq_running;
 
-	pthread_mutex_lock(&logq_lock);
-	log_enqueue(prio, fmt, ap);
-	pthread_mutex_unlock(&logq_lock);
+	if (running) {
+		pthread_mutex_lock(&logq_lock);
+		pthread_cleanup_push(cleanup_mutex, &logq_lock);
+		log_enqueue(prio, fmt, ap);
+		pthread_cleanup_pop(1);
 
-	pthread_mutex_lock(&logev_lock);
-	log_messages_pending = 1;
-	pthread_cond_signal(&logev_cond);
-	pthread_mutex_unlock(&logev_lock);
+		log_messages_pending = 1;
+		pthread_cond_signal(&logev_cond);
+	}
+	pthread_cleanup_pop(1);
+
+	if (!running)
+		vsyslog(prio, fmt, ap);
 }
 
 static void flush_logqueue (void)
@@ -103,9 +114,6 @@ void log_thread_start (pthread_attr_t *attr)
 	int running = 0;
 
 	logdbg(stderr,"enter log_thread_start\n");
-	pthread_mutex_init(&logq_lock, NULL);
-	pthread_mutex_init(&logev_lock, NULL);
-	pthread_cond_init(&logev_cond, NULL);
 
 	if (log_init("multipathd", 0)) {
 		fprintf(stderr,"can't initialize log buffer\n");
@@ -154,13 +162,9 @@ void log_thread_stop (void)
 	}
 	pthread_cleanup_pop(1);
 
-	flush_logqueue();
 	if (running)
 		pthread_join(log_thr, NULL);
 
-	pthread_mutex_destroy(&logq_lock);
-	pthread_mutex_destroy(&logev_lock);
-	pthread_cond_destroy(&logev_cond);
-
+	flush_logqueue();
 	log_close();
 }
-- 
2.29.0


--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH v3 00/29] libmultipath: improve cleanup on exit

[Martin Wilck]

On Wed, 2020-12-16 at 19:16 +0100, mwilck@suse.com wrote:
> 
> This series is based on the previous series "multipath-tools:
> shutdown, 
> libdevmapper races, globals" (v3).
> 

I forgot to mention: 

The previous 2 series

   "multipath-tools: shutdown,  libdevmapper races, globals" (v3)
   "multipath-tools:add linker version scripts" (v2)

got fully acked by Ben and have been pushed to my "upstream-queue"
branch: https://github.com/openSUSE/multipath-tools/tree/upstream-queue

The current series is based on these two.

Regards,
Martin


--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH v3 02/29] multipathd: Fix liburcu memory leak

[Benjamin Marzinski]

On Wed, Dec 16, 2020 at 07:16:41PM +0100, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> Fix this leak in multipathd, reported by valgrind, that messes up
> multipathd's otherwise clean leak report:
> 
> ==23823== 336 bytes in 1 blocks are possibly lost in loss record 3 of 3
> ==23823==    at 0x483AB65: calloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==23823==    by 0x4012F16: _dl_allocate_tls (in /lib64/ld-2.31.so)
> ==23823==    by 0x493BB8E: pthread_create@@GLIBC_2.2.5 (in /lib64/libpthread-2.31.so)
> ==23823==    by 0x492A9A9: call_rcu_data_init (urcu-call-rcu-impl.h:437)
> ==23823==    by 0x492AD2F: UnknownInlinedFun (urcu-call-rcu-impl.h:492)
> ==23823==    by 0x492AD2F: create_call_rcu_data_memb (urcu-call-rcu-impl.h:504)
> ==23823==    by 0x1164E3: child.constprop.0.isra.0 (main.c:2915)
> ==23823==    by 0x10F50C: main (main.c:3335)
> ==23823==
> ==23823== LEAK SUMMARY:
> ==23823==    definitely lost: 0 bytes in 0 blocks
> ==23823==    indirectly lost: 0 bytes in 0 blocks
> ==23823==      possibly lost: 336 bytes in 1 blocks
> 
> The problem is caused by using liburcu's default RCU call handler,
> which liburcu refuses to stop/join. See comments in the code.
> 
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  multipathd/main.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 44 insertions(+), 1 deletion(-)
> 
> diff --git a/multipathd/main.c b/multipathd/main.c
> index c5c374b..ce14bb6 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -2889,6 +2889,48 @@ set_oom_adj (void)
>  	condlog(0, "couldn't adjust oom score");
>  }
>  
> +/*
> + * Use a non-default call_rcu_data for child().
> + *
> + * We do this to avoid a memory leak from liburcu.
> + * liburcu never frees the default rcu handler (see comments on
> + * call_rcu_data_free() in urcu-call-rcu-impl.h), its thread
> + * can't be joined with pthread_join(), leaving a memory leak.
> + *
> + * Therefore we create our own, which can be destroyed and joined.
> + */
> +static struct call_rcu_data *setup_rcu(void)
> +{
> +	struct call_rcu_data *crdp;
> +
> +	rcu_init();
> +	rcu_register_thread();
> +	crdp = create_call_rcu_data(0UL, -1);
> +	if (crdp != NULL)
> +		set_thread_call_rcu_data(crdp);
> +	return crdp;
> +}
> +
> +static struct call_rcu_data *mp_rcu_data;
> +
> +static void cleanup_rcu(void)
> +{
> +	pthread_t rcu_thread;
> +
> +	/* Wait for any pending RCU calls */
> +	rcu_barrier();
> +	if (mp_rcu_data != NULL) {
> +		rcu_thread = get_call_rcu_thread(mp_rcu_data);
> +		/* detach this thread from the RCU thread */
> +		set_thread_call_rcu_data(NULL);
> +		synchronize_rcu();
> +		/* tell RCU thread to exit */
> +		call_rcu_data_free(mp_rcu_data);
> +		pthread_join(rcu_thread, NULL);
> +	}
> +	rcu_unregister_thread();
> +}
> +
>  static int
>  child (__attribute__((unused)) void *param)
>  {
> @@ -2906,7 +2948,8 @@ child (__attribute__((unused)) void *param)
>  
>  	mlockall(MCL_CURRENT | MCL_FUTURE);
>  	signal_init();
> -	rcu_init();
> +	mp_rcu_data = setup_rcu();
> +	atexit(cleanup_rcu);
>  
>  	setup_thread_attr(&misc_attr, 64 * 1024, 0);
>  	setup_thread_attr(&uevent_attr, DEFAULT_UEVENT_STACKSIZE * 1024, 0);
> -- 
> 2.29.0

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH v3 18/29] libmultipath: fix log_thread startup and teardown

[Benjamin Marzinski]

On Wed, Dec 16, 2020 at 07:16:57PM +0100, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> This fixes several issues with the log_thread. First, the running flag
> logq_running should be set by the thread itself, not by
> log_thread_start()/_stop(). Second, the thread was both cancelled and
> terminated via a flag (again, logq_running). It's sufficient to just cancel
> and join it. Third, the locking wasn't cancel-safe in some places. Forth,
> log_thread_start() didn't wait for startup properly. Fifth, using (pthread_t)0
> is wrong (pthread_t is opaque; there's no guarantee that 0 is not a valid
> pthread_t value). Sixth, pthread_cancel() was called under logq_lock, which
> doesn't make sense to me.
> 
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/log_pthread.c | 62 +++++++++++++++++++++++++++-----------
>  1 file changed, 45 insertions(+), 17 deletions(-)
> 
> diff --git a/libmultipath/log_pthread.c b/libmultipath/log_pthread.c
> index 0c327ff..3a2566a 100644
> --- a/libmultipath/log_pthread.c
> +++ b/libmultipath/log_pthread.c
> @@ -13,6 +13,7 @@
>  #include "log_pthread.h"
>  #include "log.h"
>  #include "lock.h"
> +#include "util.h"
>  
>  static pthread_t log_thr;
>  
> @@ -56,35 +57,52 @@ static void flush_logqueue (void)
>  	} while (empty == 0);
>  }
>  
> +static void cleanup_log_thread(__attribute((unused)) void *arg)
> +{
> +	logdbg(stderr, "log thread exiting");
> +	pthread_mutex_lock(&logev_lock);
> +	logq_running = 0;
> +	pthread_mutex_unlock(&logev_lock);
> +}
> +
>  static void * log_thread (__attribute__((unused)) void * et)
>  {
>  	int running;
>  
>  	pthread_mutex_lock(&logev_lock);
> -	logq_running = 1;
> +	running = logq_running;
> +	if (!running)
> +		logq_running = 1;
> +	pthread_cond_signal(&logev_cond);
>  	pthread_mutex_unlock(&logev_lock);
> +	if (running)
> +		/* already started */
> +		return NULL;
> +	pthread_cleanup_push(cleanup_log_thread, NULL);
>  
>  	mlockall(MCL_CURRENT | MCL_FUTURE);
>  	logdbg(stderr,"enter log_thread\n");
>  
>  	while (1) {
>  		pthread_mutex_lock(&logev_lock);
> -		if (logq_running && !log_messages_pending)
> +		pthread_cleanup_push(cleanup_mutex, &logev_lock);
> +		while (!log_messages_pending)
> +			/* this is a cancellation point */
>  			pthread_cond_wait(&logev_cond, &logev_lock);
>  		log_messages_pending = 0;
> -		running = logq_running;
> -		pthread_mutex_unlock(&logev_lock);
> -		if (!running)
> -			break;
> +		pthread_cleanup_pop(1);
> +
>  		flush_logqueue();
>  	}
> +	pthread_cleanup_pop(1);
>  	return NULL;
>  }
>  
>  void log_thread_start (pthread_attr_t *attr)
>  {
> -	logdbg(stderr,"enter log_thread_start\n");
> +	int running = 0;
>  
> +	logdbg(stderr,"enter log_thread_start\n");
>  	pthread_mutex_init(&logq_lock, NULL);
>  	pthread_mutex_init(&logev_lock, NULL);
>  	pthread_cond_init(&logev_cond, NULL);
> @@ -93,7 +111,15 @@ void log_thread_start (pthread_attr_t *attr)
>  		fprintf(stderr,"can't initialize log buffer\n");
>  		exit(1);
>  	}
> -	if (pthread_create(&log_thr, attr, log_thread, NULL)) {
> +
> +	pthread_mutex_lock(&logev_lock);
> +	pthread_cleanup_push(cleanup_mutex, &logev_lock);
> +	if (!pthread_create(&log_thr, attr, log_thread, NULL))
> +		while (!(running = logq_running))
> +			pthread_cond_wait(&logev_cond, &logev_lock);
> +	pthread_cleanup_pop(1);
> +
> +	if (!running) {
>  		fprintf(stderr,"can't start log thread\n");
>  		exit(1);
>  	}
> @@ -112,23 +138,25 @@ void log_thread_reset (void)
>  
>  void log_thread_stop (void)
>  {
> +	int running;
> +
>  	if (!la)
>  		return;
>  
>  	logdbg(stderr,"enter log_thread_stop\n");
>  
>  	pthread_mutex_lock(&logev_lock);
> -	logq_running = 0;
> -	pthread_cond_signal(&logev_cond);
> -	pthread_mutex_unlock(&logev_lock);
> -
> -	pthread_mutex_lock(&logq_lock);
> -	pthread_cancel(log_thr);
> -	pthread_mutex_unlock(&logq_lock);
> -	pthread_join(log_thr, NULL);
> -	log_thr = (pthread_t)0;
> +	pthread_cleanup_push(cleanup_mutex, &logev_lock);
> +	running = logq_running;
> +	if (running) {
> +		pthread_cancel(log_thr);
> +		pthread_cond_signal(&logev_cond);
> +	}
> +	pthread_cleanup_pop(1);
>  
>  	flush_logqueue();
> +	if (running)
> +		pthread_join(log_thr, NULL);
>  
>  	pthread_mutex_destroy(&logq_lock);
>  	pthread_mutex_destroy(&logev_lock);
> -- 
> 2.29.0

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH v3 20/29] multipath: use atexit() for cleanup handlers

[Benjamin Marzinski]

On Wed, Dec 16, 2020 at 07:16:59PM +0100, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  multipath/main.c | 37 ++++++++++++++++---------------------
>  1 file changed, 16 insertions(+), 21 deletions(-)
> 
> diff --git a/multipath/main.c b/multipath/main.c
> index 9ae46ed..1949a1c 100644
> --- a/multipath/main.c
> +++ b/multipath/main.c
> @@ -452,13 +452,19 @@ static bool released_to_systemd(void)
>  	return ret;
>  }
>  
> +static struct vectors vecs;
> +static void cleanup_vecs(void)
> +{
> +	free_multipathvec(vecs.mpvec, KEEP_PATHS);
> +	free_pathvec(vecs.pathvec, FREE_PATHS);
> +}
> +
>  static int
>  configure (struct config *conf, enum mpath_cmds cmd,
>  	   enum devtypes dev_type, char *devpath)
>  {
>  	vector curmp = NULL;
>  	vector pathvec = NULL;
> -	struct vectors vecs;
>  	int r = RTVL_FAIL, rc;
>  	int di_flag = 0;
>  	char * refwwid = NULL;
> @@ -469,6 +475,7 @@ configure (struct config *conf, enum mpath_cmds cmd,
>  	 */
>  	curmp = vector_alloc();
>  	pathvec = vector_alloc();
> +	atexit(cleanup_vecs);
>  
>  	if (!curmp || !pathvec) {
>  		condlog(0, "can not allocate memory");
> @@ -580,9 +587,6 @@ out:
>  	if (refwwid)
>  		FREE(refwwid);
>  
> -	free_multipathvec(curmp, KEEP_PATHS);
> -	free_pathvec(pathvec, FREE_PATHS);
> -
>  	return r;
>  }
>  
> @@ -808,9 +812,13 @@ main (int argc, char *argv[])
>  	bool enable_foreign = false;
>  
>  	libmultipath_init();
> +	if (atexit(dm_lib_exit) || atexit(libmultipath_exit))
> +		condlog(1, "failed to register cleanup handler for libmultipath: %m");
>  	logsink = 0;
>  	if (init_config(DEFAULT_CONFIGFILE))
>  		exit(RTVL_FAIL);
> +	if (atexit(uninit_config))
> +		condlog(1, "failed to register cleanup handler for config: %m");
>  	conf = get_multipath_config();
>  	conf->retrigger_tries = 0;
>  	conf->force_sync = 1;
> @@ -887,7 +895,7 @@ main (int argc, char *argv[])
>  			break;
>  		case 't':
>  			r = dump_config(conf, NULL, NULL) ? RTVL_FAIL : RTVL_OK;
> -			goto out_free_config;
> +			goto out;
>  		case 'T':
>  			cmd = CMD_DUMP_CONFIG;
>  			break;
> @@ -1048,26 +1056,13 @@ main (int argc, char *argv[])
>  		condlog(3, "restart multipath configuration process");
>  
>  out:
> -	dm_lib_exit();
> -
> -	cleanup_foreign();
> -	cleanup_prio();
> -	cleanup_checkers();
> +	put_multipath_config(conf);
> +	if (dev)
> +		FREE(dev);
>  
>  	if (dev_type == DEV_UEVENT)
>  		closelog();
>  
> -out_free_config:
> -	/*
> -	 * Freeing config must be done after dm_lib_exit(), because
> -	 * the logging function (dm_write_log()), which is called there,
> -	 * references the config.
> -	 */
> -	put_multipath_config(conf);
> -	uninit_config();
> -	libmultipath_exit();
> -	if (dev)
> -		FREE(dev);
>  #ifdef _DEBUG_
>  	dbg_free_final(NULL);
>  #endif
> -- 
> 2.29.0

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH v3 22/29] multipath: fix leaks in check_path_valid()

[Benjamin Marzinski]

On Wed, Dec 16, 2020 at 07:17:01PM +0100, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> There were two leaks in check_path_valid(): if path status was
> successfully determined before calling store_pathvec(), free_path()
> wasn't called. Also, if an error exit occured, neither cleanup
> function was called.
> 
> This patch fixes both, at the cost of using "static" for the pp and
> pathvec variables.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  multipath/main.c | 29 +++++++++++++++++++----------
>  1 file changed, 19 insertions(+), 10 deletions(-)
> 
> diff --git a/multipath/main.c b/multipath/main.c
> index 1949a1c..056e29a 100644
> --- a/multipath/main.c
> +++ b/multipath/main.c
> @@ -93,7 +93,7 @@ void rcu_register_thread_memb(void) {}
>  void rcu_unregister_thread_memb(void) {}
>  
>  static int
> -filter_pathvec (vector pathvec, char * refwwid)
> +filter_pathvec (vector pathvec, const char *refwwid)
>  {
>  	int i;
>  	struct path * pp;
> @@ -594,8 +594,9 @@ static int
>  check_path_valid(const char *name, struct config *conf, bool is_uevent)
>  {
>  	int fd, r = PATH_IS_ERROR;
> -	struct path *pp = NULL;
> +	struct path *pp;
>  	vector pathvec = NULL;
> +	const char *wwid;
>  
>  	pp = alloc_path();
>  	if (!pp)
> @@ -665,13 +666,17 @@ check_path_valid(const char *name, struct config *conf, bool is_uevent)
>  	if (store_path(pathvec, pp) != 0) {

This will double-free the path, once here and again in cleanup.
>  		free_path(pp);
>  		goto fail;
> +	} else {
> +		/* make sure path isn't freed twice */
> +		wwid = pp->wwid;
> +		pp = NULL;
>  	}
>  
>  	/* For find_multipaths = SMART, if there is more than one path
>  	 * matching the refwwid, then the path is valid */
>  	if (path_discovery(pathvec, DI_SYSFS | DI_WWID) < 0)
>  		goto fail;
> -	filter_pathvec(pathvec, pp->wwid);
> +	filter_pathvec(pathvec, wwid);
>  	if (VECTOR_SIZE(pathvec) > 1)
>  		r = PATH_IS_VALID;
>  	else
> @@ -679,21 +684,25 @@ check_path_valid(const char *name, struct config *conf, bool is_uevent)
>  
>  out:
>  	r = print_cmd_valid(r, pathvec, conf);
> -	free_pathvec(pathvec, FREE_PATHS);
>  	/*
>  	 * multipath -u must exit with status 0, otherwise udev won't
>  	 * import its output.
>  	 */
>  	if (!is_uevent && r == PATH_IS_NOT_VALID)
> -		return RTVL_FAIL;
> -	return RTVL_OK;
> +		r = RTVL_FAIL;
> +	else
> +		r = RTVL_OK;
> +	goto cleanup;
>  
>  fail:
> -	if (pathvec)
> +	r = RTVL_FAIL;
> +
> +cleanup:
> +	if (pp != NULL)

shouldn't this be free_path(pp)

-Ben

> +		free(pp);
> +	if (pathvec != NULL)
>  		free_pathvec(pathvec, FREE_PATHS);
> -	else
> -		free_path(pp);
> -	return RTVL_FAIL;
> +	return r;
>  }
>  
>  static int
> -- 
> 2.29.0

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH v3 24/29] libmultipath: use libmp_verbosity to track verbosity

[Benjamin Marzinski]

On Wed, Dec 16, 2020 at 07:17:03PM +0100, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> Introduce a new global variable to set the verbosity of libmultipath.
> This avoids accessing the configuration in every dlog() call.
> When libmultipath reads its configuration in init_config() or
> load_config(), it will use the current value of libmp_verbosity
> for logging. Immediately before returning, libmp_verbosity will be
> overwritten with the verbosity value from the configuration file,
> if it was set there. An application is free to set libmp_verbosity
> back to the previous value or not after that, depending on whether
> command line options or configuration file settings should take
> precedence.
> 
> Replace internal access to conf->verbosity with the new variable.
> 
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmpathpersist/mpath_persist.c   |  5 +---
>  libmultipath/config.c             |  9 +++++--
>  libmultipath/configure.c          | 16 +++----------
>  libmultipath/debug.c              | 10 ++------
>  libmultipath/debug.h              |  1 +
>  libmultipath/devmapper.c          |  7 +-----
>  libmultipath/libmultipath.version |  5 ++++
>  multipath/main.c                  | 21 ++++++----------
>  multipathd/main.c                 | 40 ++++++++++++++++++-------------
>  tests/alias.c                     |  1 +
>  tests/blacklist.c                 |  2 ++
>  11 files changed, 53 insertions(+), 64 deletions(-)
> 
> diff --git a/libmpathpersist/mpath_persist.c b/libmpathpersist/mpath_persist.c
> index 9ebf91d..79322e8 100644
> --- a/libmpathpersist/mpath_persist.c
> +++ b/libmpathpersist/mpath_persist.c
> @@ -170,10 +170,7 @@ void mpath_persistent_reserve_free_vecs(void)
>  
>  int mpath_persistent_reserve_init_vecs(int verbose)
>  {
> -	struct config *conf = get_multipath_config();
> -
> -	conf->verbosity = verbose;
> -	put_multipath_config(conf);
> +	libmp_verbosity = verbose;
>  
>  	if (curmp)
>  		return MPATH_PR_SUCCESS;
> diff --git a/libmultipath/config.c b/libmultipath/config.c
> index 52b1447..49e7fb8 100644
> --- a/libmultipath/config.c
> +++ b/libmultipath/config.c
> @@ -828,10 +828,14 @@ int _init_config (const char *file, struct config *conf)
>  		conf = &__internal_config;
>  
>  	/*
> -	 * internal defaults
> +	 * Processing the config file will overwrite conf->verbosity if set
> +	 * When we return, we'll copy the config value back
>  	 */
> -	conf->verbosity = DEFAULT_VERBOSITY;
> +	conf->verbosity = libmp_verbosity;
>  
> +	/*
> +	 * internal defaults
> +	 */
>  	get_sys_max_fds(&conf->max_fds);
>  	conf->bindings_file = set_default(DEFAULT_BINDINGS_FILE);
>  	conf->wwids_file = set_default(DEFAULT_WWIDS_FILE);
> @@ -997,6 +1001,7 @@ int _init_config (const char *file, struct config *conf)
>  	    !conf->wwids_file || !conf->prkeys_file)
>  		goto out;
>  
> +	libmp_verbosity = conf->verbosity;
>  	return 0;
>  out:
>  	_uninit_config(conf);
> diff --git a/libmultipath/configure.c b/libmultipath/configure.c
> index 1c8aac0..3dbc1f1 100644
> --- a/libmultipath/configure.c
> +++ b/libmultipath/configure.c
> @@ -934,16 +934,12 @@ int domap(struct multipath *mpp, char *params, int is_daemon)
>  {
>  	int r = DOMAP_FAIL;
>  	struct config *conf;
> -	int verbosity;
>  
>  	/*
>  	 * last chance to quit before touching the devmaps
>  	 */
>  	if (mpp->action == ACT_DRY_RUN) {
> -		conf = get_multipath_config();
> -		verbosity = conf->verbosity;
> -		put_multipath_config(conf);
> -		print_multipath_topology(mpp, verbosity);
> +		print_multipath_topology(mpp, libmp_verbosity);
>  		return DOMAP_DRY;
>  	}
>  
> @@ -1311,14 +1307,8 @@ int coalesce_paths (struct vectors * vecs, vector newmp, char * refwwid,
>  					       "queue_if_no_path");
>  		}
>  
> -		if (!is_daemon && mpp->action != ACT_NOTHING) {
> -			int verbosity;
> -
> -			conf = get_multipath_config();
> -			verbosity = conf->verbosity;
> -			put_multipath_config(conf);
> -			print_multipath_topology(mpp, verbosity);
> -		}
> +		if (!is_daemon && mpp->action != ACT_NOTHING)
> +			print_multipath_topology(mpp, libmp_verbosity);
>  
>  		if (newmp) {
>  			if (mpp->action != ACT_REJECT) {
> diff --git a/libmultipath/debug.c b/libmultipath/debug.c
> index b3a1de9..a1713b9 100644
> --- a/libmultipath/debug.c
> +++ b/libmultipath/debug.c
> @@ -16,21 +16,15 @@
>  #include "debug.h"
>  
>  int logsink;
> +int libmp_verbosity = DEFAULT_VERBOSITY;
>  
>  void dlog (int sink, int prio, const char * fmt, ...)
>  {
>  	va_list ap;
> -	int thres;
> -	struct config *conf;
>  
>  	va_start(ap, fmt);
> -	conf = get_multipath_config();
> -	ANNOTATE_IGNORE_READS_BEGIN();
> -	thres = (conf) ? conf->verbosity : DEFAULT_VERBOSITY;
> -	ANNOTATE_IGNORE_READS_END();
> -	put_multipath_config(conf);
>  
> -	if (prio <= thres) {
> +	if (prio <= libmp_verbosity) {
>  		if (sink < 1) {
>  			if (sink == 0) {
>  				time_t t = time(NULL);
> diff --git a/libmultipath/debug.h b/libmultipath/debug.h
> index c6120c1..1f3bc8b 100644
> --- a/libmultipath/debug.h
> +++ b/libmultipath/debug.h
> @@ -8,6 +8,7 @@ void dlog (int sink, int prio, const char * fmt, ...)
>  #include "log_pthread.h"
>  
>  extern int logsink;
> +extern int libmp_verbosity;
>  
>  #define condlog(prio, fmt, args...) \
>  	dlog(logsink, prio, fmt "\n", ##args)
> diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
> index e60ab49..dfe95d2 100644
> --- a/libmultipath/devmapper.c
> +++ b/libmultipath/devmapper.c
> @@ -351,16 +351,11 @@ void libmp_dm_exit(void)
>  
>  static void libmp_dm_init(void)
>  {
> -	struct config *conf;
> -	int verbosity;
>  	unsigned int version[3];
>  
>  	if (dm_prereq(version))
>  		exit(1);
> -	conf = get_multipath_config();
> -	verbosity = conf->verbosity;
> -	put_multipath_config(conf);
> -	dm_init(verbosity);
> +	dm_init(libmp_verbosity);
>  #ifdef LIBDM_API_HOLD_CONTROL
>  	dm_hold_control_dev(1);
>  #endif
> diff --git a/libmultipath/libmultipath.version b/libmultipath/libmultipath.version
> index 800cff2..67a7379 100644
> --- a/libmultipath/libmultipath.version
> +++ b/libmultipath/libmultipath.version
> @@ -259,3 +259,8 @@ global:
>  local:
>  	*;
>  };
> +
> +LIBMULTIPATH_4.1.0 {
> +global:
> +	libmp_verbosity;
> +} LIBMULTIPATH_4.0.0;
> diff --git a/multipath/main.c b/multipath/main.c
> index 056e29a..371be2a 100644
> --- a/multipath/main.c
> +++ b/multipath/main.c
> @@ -208,22 +208,15 @@ get_dm_mpvec (enum mpath_cmds cmd, vector curmp, vector pathvec, char * refwwid)
>  			mpp->bestpg = select_path_group(mpp);
>  
>  		if (cmd == CMD_LIST_SHORT ||
> -		    cmd == CMD_LIST_LONG) {
> -			struct config *conf = get_multipath_config();
> -			print_multipath_topology(mpp, conf->verbosity);
> -			put_multipath_config(conf);
> -		}
> +		    cmd == CMD_LIST_LONG)
> +			print_multipath_topology(mpp, libmp_verbosity);
>  
>  		if (cmd == CMD_CREATE)
>  			reinstate_paths(mpp);
>  	}
>  
> -	if (cmd == CMD_LIST_SHORT || cmd == CMD_LIST_LONG) {
> -		struct config *conf = get_multipath_config();
> -
> -		print_foreign_topology(conf->verbosity);
> -		put_multipath_config(conf);
> -	}
> +	if (cmd == CMD_LIST_SHORT || cmd == CMD_LIST_LONG)
> +		print_foreign_topology(libmp_verbosity);
>  
>  	return 0;
>  }
> @@ -552,7 +545,7 @@ configure (struct config *conf, enum mpath_cmds cmd,
>  	if (path_discovery(pathvec, di_flag) < 0)
>  		goto out;
>  
> -	if (conf->verbosity > 2)
> +	if (libmp_verbosity > 2)
>  		print_all_paths(pathvec, 1);
>  
>  	get_path_layout(pathvec, 0);
> @@ -842,7 +835,7 @@ main (int argc, char *argv[])
>  				exit(RTVL_FAIL);
>  			}
>  
> -			conf->verbosity = atoi(optarg);
> +			libmp_verbosity = atoi(optarg);
>  			break;
>  		case 'b':
>  			conf->bindings_file = strdup(optarg);
> @@ -973,7 +966,7 @@ main (int argc, char *argv[])
>  	}
>  	if (dev_type == DEV_UEVENT) {
>  		openlog("multipath", 0, LOG_DAEMON);
> -		setlogmask(LOG_UPTO(conf->verbosity + 3));
> +		setlogmask(LOG_UPTO(libmp_verbosity + 3));
>  		logsink = 1;
>  	}
>  
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 4de0978..ba25751 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -88,10 +88,10 @@
>  #define CMDSIZE 160
>  #define MSG_SIZE 32
>  
> -#define LOG_MSG(lvl, verb, pp)					\
> +#define LOG_MSG(lvl, pp)					\
>  do {								\
>  	if (pp->mpp && checker_selected(&pp->checker) &&	\
> -	    lvl <= verb) {					\
> +	    lvl <= libmp_verbosity) {					\
>  		if (pp->offline)				\
>  			condlog(lvl, "%s: %s - path offline",	\
>  				pp->mpp->alias, pp->dev);	\
> @@ -2070,7 +2070,7 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks)
>  	int chkr_new_path_up = 0;
>  	int disable_reinstate = 0;
>  	int oldchkrstate = pp->chkrstate;
> -	int retrigger_tries, verbosity;
> +	int retrigger_tries;
>  	unsigned int checkint, max_checkint;
>  	struct config *conf;
>  	int marginal_pathgroups, marginal_changed = 0;
> @@ -2090,7 +2090,6 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks)
>  	retrigger_tries = conf->retrigger_tries;
>  	checkint = conf->checkint;
>  	max_checkint = conf->max_checkint;
> -	verbosity = conf->verbosity;
>  	marginal_pathgroups = conf->marginal_pathgroups;
>  	put_multipath_config(conf);
>  
> @@ -2152,7 +2151,7 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks)
>  	if (newstate == PATH_WILD || newstate == PATH_UNCHECKED) {
>  		condlog(2, "%s: unusable path (%s) - checker failed",
>  			pp->dev, checker_state_name(newstate));
> -		LOG_MSG(2, verbosity, pp);
> +		LOG_MSG(2, pp);
>  		conf = get_multipath_config();
>  		pthread_cleanup_push(put_multipath_config, conf);
>  		pathinfo(pp, conf, 0);
> @@ -2257,7 +2256,7 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks)
>  		int oldstate = pp->state;
>  		pp->state = newstate;
>  
> -		LOG_MSG(1, verbosity, pp);
> +		LOG_MSG(1, pp);
>  
>  		/*
>  		 * upon state change, reset the checkint
> @@ -2321,7 +2320,7 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks)
>  			/* Clear IO errors */
>  			reinstate_path(pp);
>  		else {
> -			LOG_MSG(4, verbosity, pp);
> +			LOG_MSG(4, pp);
>  			if (pp->checkint != max_checkint) {
>  				/*
>  				 * double the next check delay.
> @@ -2349,9 +2348,9 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks)
>  			log_checker_err = conf->log_checker_err;
>  			put_multipath_config(conf);
>  			if (log_checker_err == LOG_CHKR_ERR_ONCE)
> -				LOG_MSG(3, verbosity, pp);
> +				LOG_MSG(3, pp);
>  			else
> -				LOG_MSG(2, verbosity, pp);
> +				LOG_MSG(2, pp);
>  		}
>  	}
>  
> @@ -2696,6 +2695,10 @@ reconfigure (struct vectors * vecs)
>  	if (!conf)
>  		return 1;
>  
> +	if (verbosity)
> +		libmp_verbosity = verbosity;
> +	setlogmask(LOG_UPTO(libmp_verbosity + 3));
> +
>  	/*
>  	 * free old map and path vectors ... they use old conf state
>  	 */
> @@ -2710,8 +2713,6 @@ reconfigure (struct vectors * vecs)
>  	/* Re-read any timezone changes */
>  	tzset();
>  
> -	if (verbosity)
> -		conf->verbosity = verbosity;
>  	if (bindings_read_only)
>  		conf->bindings_read_only = bindings_read_only;
>  	check_alias_settings(conf);
> @@ -3091,14 +3092,18 @@ child (__attribute__((unused)) void *param)
>  	condlog(2, "--------start up--------");
>  	condlog(2, "read " DEFAULT_CONFIGFILE);
>  
> +	if (verbosity)
> +		libmp_verbosity = verbosity;
>  	conf = load_config(DEFAULT_CONFIGFILE);
> +	if (verbosity)
> +		libmp_verbosity = verbosity;
> +	setlogmask(LOG_UPTO(libmp_verbosity + 3));
> +
>  	if (!conf) {
>  		condlog(0, "failed to load configuration");
>  		goto failed;
>  	}
>  
> -	if (verbosity)
> -		conf->verbosity = verbosity;
>  	if (bindings_read_only)
>  		conf->bindings_read_only = bindings_read_only;
>  	uxsock_timeout = conf->uxsock_timeout;
> @@ -3117,7 +3122,6 @@ child (__attribute__((unused)) void *param)
>  
>  	if (poll_dmevents)
>  		poll_dmevents = dmevent_poll_supported();
> -	setlogmask(LOG_UPTO(conf->verbosity + 3));
>  
>  	envp = getenv("LimitNOFILE");
>  
> @@ -3339,7 +3343,7 @@ main (int argc, char *argv[])
>  			    !isdigit(optarg[0]))
>  				exit(1);
>  
> -			verbosity = atoi(optarg);
> +			libmp_verbosity = verbosity = atoi(optarg);
>  			break;
>  		case 's':
>  			logsink = -1;
> @@ -3350,7 +3354,7 @@ main (int argc, char *argv[])
>  			if (!conf)
>  				exit(1);
>  			if (verbosity)
> -				conf->verbosity = verbosity;
> +				libmp_verbosity = verbosity;
>  			uxsock_timeout = conf->uxsock_timeout;
>  			err = uxclnt(optarg, uxsock_timeout + 100);
>  			free_config(conf);
> @@ -3376,11 +3380,13 @@ main (int argc, char *argv[])
>  		char * c = s;
>  
>  		logsink = 0;
> +		if (verbosity)
> +			libmp_verbosity = verbosity;
>  		conf = load_config(DEFAULT_CONFIGFILE);
>  		if (!conf)
>  			exit(1);
>  		if (verbosity)
> -			conf->verbosity = verbosity;
> +			libmp_verbosity = verbosity;
>  		uxsock_timeout = conf->uxsock_timeout;
>  		memset(cmd, 0x0, CMDSIZE);
>  		while (optind < argc) {
> diff --git a/tests/alias.c b/tests/alias.c
> index 7fda679..0311faa 100644
> --- a/tests/alias.c
> +++ b/tests/alias.c
> @@ -735,6 +735,7 @@ static int test_allocate_binding(void)
>  int main(void)
>  {
>  	int ret = 0;
> +	libmp_verbosity = conf.verbosity;
>  
>  	ret += test_format_devname();
>  	ret += test_scan_devname();
> diff --git a/tests/blacklist.c b/tests/blacklist.c
> index 84a3ba2..0b42e25 100644
> --- a/tests/blacklist.c
> +++ b/tests/blacklist.c
> @@ -22,6 +22,7 @@
>  #include "globals.c"
>  #include "blacklist.h"
>  #include "test-log.h"
> +#include "debug.h"
>  
>  struct udev_device {
>  	const char *sysname;
> @@ -152,6 +153,7 @@ static int setup(void **state)
>  	    store_ble(blist_property_wwn_inv, "!ID_WWN", ORIGIN_CONFIG))
>  		return -1;
>  
> +	libmp_verbosity = conf.verbosity = 4;
>  	return 0;
>  }
>  
> -- 
> 2.29.0

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH v3 25/29] libmultipath: introduce symbolic values for logsink

[Benjamin Marzinski]

On Wed, Dec 16, 2020 at 07:17:04PM +0100, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/debug.c     |  4 ++--
>  libmultipath/debug.h     |  6 ++++++
>  libmultipath/devmapper.c |  4 ++--
>  multipath/main.c         |  4 ++--
>  multipathd/main.c        | 17 ++++++++---------
>  tests/globals.c          |  3 ++-
>  tests/hwtable.c          |  2 +-
>  7 files changed, 23 insertions(+), 17 deletions(-)
> 
> diff --git a/libmultipath/debug.c b/libmultipath/debug.c
> index a1713b9..f9b7755 100644
> --- a/libmultipath/debug.c
> +++ b/libmultipath/debug.c
> @@ -25,8 +25,8 @@ void dlog (int sink, int prio, const char * fmt, ...)
>  	va_start(ap, fmt);
>  
>  	if (prio <= libmp_verbosity) {
> -		if (sink < 1) {
> -			if (sink == 0) {
> +		if (sink != LOGSINK_SYSLOG) {
> +			if (sink == LOGSINK_STDERR_WITH_TIME) {
>  				time_t t = time(NULL);
>  				struct tm *tb = localtime(&t);
>  				char buff[16];
> diff --git a/libmultipath/debug.h b/libmultipath/debug.h
> index 1f3bc8b..b6ce70a 100644
> --- a/libmultipath/debug.h
> +++ b/libmultipath/debug.h
> @@ -12,3 +12,9 @@ extern int libmp_verbosity;
>  
>  #define condlog(prio, fmt, args...) \
>  	dlog(logsink, prio, fmt "\n", ##args)
> +
> +enum {
> +	LOGSINK_STDERR_WITH_TIME = 0,
> +	LOGSINK_STDERR_WITHOUT_TIME = -1,
> +	LOGSINK_SYSLOG = 1,
> +};
> diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
> index dfe95d2..f8b180e 100644
> --- a/libmultipath/devmapper.c
> +++ b/libmultipath/devmapper.c
> @@ -104,8 +104,8 @@ dm_write_log (int level, const char *file, int line, const char *f, ...)
>  		return;
>  
>  	va_start(ap, f);
> -	if (logsink < 1) {
> -		if (logsink == 0) {
> +	if (logsink != LOGSINK_SYSLOG) {
> +		if (logsink == LOGSINK_STDERR_WITH_TIME) {
>  			time_t t = time(NULL);
>  			struct tm *tb = localtime(&t);
>  			char buff[16];
> diff --git a/multipath/main.c b/multipath/main.c
> index 371be2a..c94bb26 100644
> --- a/multipath/main.c
> +++ b/multipath/main.c
> @@ -816,7 +816,7 @@ main (int argc, char *argv[])
>  	libmultipath_init();
>  	if (atexit(dm_lib_exit) || atexit(libmultipath_exit))
>  		condlog(1, "failed to register cleanup handler for libmultipath: %m");
> -	logsink = 0;
> +	logsink = LOGSINK_STDERR_WITH_TIME;
>  	if (init_config(DEFAULT_CONFIGFILE))
>  		exit(RTVL_FAIL);
>  	if (atexit(uninit_config))
> @@ -967,7 +967,7 @@ main (int argc, char *argv[])
>  	if (dev_type == DEV_UEVENT) {
>  		openlog("multipath", 0, LOG_DAEMON);
>  		setlogmask(LOG_UPTO(libmp_verbosity + 3));
> -		logsink = 1;
> +		logsink = LOGSINK_SYSLOG;
>  	}
>  
>  	set_max_fds(conf->max_fds);
> diff --git a/multipathd/main.c b/multipathd/main.c
> index ba25751..867f0f8 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -2780,7 +2780,7 @@ handle_signals(bool nonfatal)
>  	}
>  	if (log_reset_sig) {
>  		condlog(2, "reset log (signal)");
> -		if (logsink == 1)
> +		if (logsink == LOGSINK_SYSLOG)
>  			log_thread_reset();
>  	}
>  	reconfig_sig = 0;
> @@ -3033,7 +3033,7 @@ static void cleanup_child(void)
>  		cleanup_dmevent_waiter();
>  
>  	cleanup_pidfile();
> -	if (logsink == 1)
> +	if (logsink == LOGSINK_SYSLOG)
>  		log_thread_stop();
>  
>  	cleanup_conf();
> @@ -3076,7 +3076,7 @@ child (__attribute__((unused)) void *param)
>  	setup_thread_attr(&uevent_attr, DEFAULT_UEVENT_STACKSIZE * 1024, 0);
>  	setup_thread_attr(&waiter_attr, 32 * 1024, 1);
>  
> -	if (logsink == 1) {
> +	if (logsink == LOGSINK_SYSLOG) {
>  		setup_thread_attr(&log_attr, 64 * 1024, 0);
>  		log_thread_start(&log_attr);
>  		pthread_attr_destroy(&log_attr);
> @@ -3307,7 +3307,7 @@ main (int argc, char *argv[])
>  	ANNOTATE_BENIGN_RACE_SIZED(&uxsock_timeout, sizeof(uxsock_timeout),
>  		"Suppress complaints about this scalar variable");
>  
> -	logsink = 1;
> +	logsink = LOGSINK_SYSLOG;
>  
>  	if (getuid() != 0) {
>  		fprintf(stderr, "need to be root\n");
> @@ -3334,9 +3334,8 @@ main (int argc, char *argv[])
>  		switch(arg) {
>  		case 'd':
>  			foreground = 1;
> -			if (logsink > 0)
> -				logsink = 0;
> -			//debug=1; /* ### comment me out ### */
> +			if (logsink == LOGSINK_SYSLOG)
> +				logsink = LOGSINK_STDERR_WITH_TIME;
>  			break;
>  		case 'v':
>  			if (sizeof(optarg) > sizeof(char *) ||
> @@ -3346,7 +3345,7 @@ main (int argc, char *argv[])
>  			libmp_verbosity = verbosity = atoi(optarg);
>  			break;
>  		case 's':
> -			logsink = -1;
> +			logsink = LOGSINK_STDERR_WITHOUT_TIME;
>  			break;
>  		case 'k':
>  			logsink = 0;
> @@ -3379,7 +3378,7 @@ main (int argc, char *argv[])
>  		char * s = cmd;
>  		char * c = s;
>  
> -		logsink = 0;
> +		logsink = LOGSINK_STDERR_WITH_TIME;
>  		if (verbosity)
>  			libmp_verbosity = verbosity;
>  		conf = load_config(DEFAULT_CONFIGFILE);
> diff --git a/tests/globals.c b/tests/globals.c
> index 8add5eb..fc0c07a 100644
> --- a/tests/globals.c
> +++ b/tests/globals.c
> @@ -1,9 +1,10 @@
>  #include "structs.h"
>  #include "config.h"
> +#include "debug.h"
>  
>  /* Required globals */
>  struct udev *udev;
> -int logsink = -1;
> +int logsink = LOGSINK_STDERR_WITHOUT_TIME;
>  struct config conf = {
>  	.verbosity = 4,
>  };
> diff --git a/tests/hwtable.c b/tests/hwtable.c
> index 57f832b..4dd0873 100644
> --- a/tests/hwtable.c
> +++ b/tests/hwtable.c
> @@ -53,7 +53,7 @@ struct hwt_state {
>  
>  static struct config *_conf;
>  struct udev *udev;
> -int logsink = -1;
> +int logsink = LOGSINK_STDERR_WITHOUT_TIME;
>  
>  struct config *get_multipath_config(void)
>  {
> -- 
> 2.29.0

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH v3 29/29] libmultipath: fix race between log_safe and log_thread_stop()

[Benjamin Marzinski]

On Wed, Dec 16, 2020 at 07:17:08PM +0100, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> log_safe() could race with log_thread_stop(); simply
> checking the value of log_thr has never been safe. By converting the
> mutexes to static initializers, we avoid having to destroy them, and thus
> possibly accessing a destroyed mutex in log_safe(). Furthermore, taking
> both the logev_lock and the logq_lock makes sure the logarea isn't freed
> while we are writing to it.
> 
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/log_pthread.c | 48 +++++++++++++++++++++-----------------
>  1 file changed, 26 insertions(+), 22 deletions(-)
> 
> diff --git a/libmultipath/log_pthread.c b/libmultipath/log_pthread.c
> index 3a2566a..0d48c52 100644
> --- a/libmultipath/log_pthread.c
> +++ b/libmultipath/log_pthread.c
> @@ -17,31 +17,42 @@
>  
>  static pthread_t log_thr;
>  
> -static pthread_mutex_t logq_lock;
> -static pthread_mutex_t logev_lock;
> -static pthread_cond_t logev_cond;
> +/* logev_lock must not be taken with logq_lock held */
> +static pthread_mutex_t logq_lock = PTHREAD_MUTEX_INITIALIZER;
> +static pthread_mutex_t logev_lock = PTHREAD_MUTEX_INITIALIZER;
> +static pthread_cond_t logev_cond = PTHREAD_COND_INITIALIZER;
>  
>  static int logq_running;
>  static int log_messages_pending;
>  
>  void log_safe (int prio, const char * fmt, va_list ap)
>  {
> +	bool running;
> +
>  	if (prio > LOG_DEBUG)
>  		prio = LOG_DEBUG;
>  
> -	if (log_thr == (pthread_t)0) {
> -		vsyslog(prio, fmt, ap);
> -		return;
> -	}
> +	/*
> +	 * logev_lock protects logq_running. By holding it, we avoid a race
> +	 * with log_thread_stop() -> log_close(), which would free the logarea.
> +	 */
> +	pthread_mutex_lock(&logev_lock);
> +	pthread_cleanup_push(cleanup_mutex, &logev_lock);
> +	running = logq_running;
>  
> -	pthread_mutex_lock(&logq_lock);
> -	log_enqueue(prio, fmt, ap);
> -	pthread_mutex_unlock(&logq_lock);
> +	if (running) {
> +		pthread_mutex_lock(&logq_lock);
> +		pthread_cleanup_push(cleanup_mutex, &logq_lock);
> +		log_enqueue(prio, fmt, ap);
> +		pthread_cleanup_pop(1);
>  
> -	pthread_mutex_lock(&logev_lock);
> -	log_messages_pending = 1;
> -	pthread_cond_signal(&logev_cond);
> -	pthread_mutex_unlock(&logev_lock);
> +		log_messages_pending = 1;
> +		pthread_cond_signal(&logev_cond);
> +	}
> +	pthread_cleanup_pop(1);
> +
> +	if (!running)
> +		vsyslog(prio, fmt, ap);
>  }
>  
>  static void flush_logqueue (void)
> @@ -103,9 +114,6 @@ void log_thread_start (pthread_attr_t *attr)
>  	int running = 0;
>  
>  	logdbg(stderr,"enter log_thread_start\n");
> -	pthread_mutex_init(&logq_lock, NULL);
> -	pthread_mutex_init(&logev_lock, NULL);
> -	pthread_cond_init(&logev_cond, NULL);
>  
>  	if (log_init("multipathd", 0)) {
>  		fprintf(stderr,"can't initialize log buffer\n");
> @@ -154,13 +162,9 @@ void log_thread_stop (void)
>  	}
>  	pthread_cleanup_pop(1);
>  
> -	flush_logqueue();
>  	if (running)
>  		pthread_join(log_thr, NULL);
>  
> -	pthread_mutex_destroy(&logq_lock);
> -	pthread_mutex_destroy(&logev_lock);
> -	pthread_cond_destroy(&logev_cond);
> -
> +	flush_logqueue();
>  	log_close();
>  }
> -- 
> 2.29.0

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH v3 22/29] multipath: fix leaks in check_path_valid()

[Martin Wilck]

On Wed, 2020-12-16 at 21:34 -0600, Benjamin Marzinski wrote:
> On Wed, Dec 16, 2020 at 07:17:01PM +0100, mwilck@suse.com wrote:
> > 
> >  
> >  	pp = alloc_path();
> >  	if (!pp)
> > @@ -665,13 +666,17 @@ check_path_valid(const char *name, struct
> > config *conf, bool is_uevent)
> >  	if (store_path(pathvec, pp) != 0) {
> 
> This will double-free the path, once here and again in cleanup.

argh, thanks - especially dumb with comment below :-/

> >  		free_path(pp);
> >  		goto fail;
> > +	} else {
> > +		/* make sure path isn't freed twice */
> > +		wwid = pp->wwid;
> > +		pp = NULL;
> >  	}
> > 
> ...
> > +
> > +cleanup:
> > +	if (pp != NULL)
> 
> shouldn't this be free_path(pp)

Absolutely. I'll resend this one. As everything else is reviewed now,
I'll just resubmit this one.

Martin


--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel