Linux Device Mapper development
 help / color / mirror / Atom feed
* [PATCH v2 0/7] multipathd: make uxlsnr errors really fatal
@ 2018-10-30 21:22 Martin Wilck
  2018-10-30 21:22 ` [PATCH v2 1/7] libmultipath: set pp->checkint in store_pathinfo() Martin Wilck
                   ` (6 more replies)
  0 siblings, 7 replies; 9+ messages in thread
From: Martin Wilck @ 2018-10-30 21:22 UTC (permalink / raw)
  To: Christophe Varoqui; +Cc: Martin Wilck, dm-devel

Hi Christophe,

this series, based on top of the recently submitted
"various multipath-tools patches (v2)" and "checkers overhaul (v3)"
series, fixes a problem that I recently observed: despite
ee01e841 "multipathd: handle errors in uxlsnr as fatal", multipathd
sometimes doesn't quit when the socket setup fails.

While at that, I stumbled upon init_path_check_interval(), wondered
about its purpose, and came to the conclusion that can be quite
easily obsoleted.

----
Changes v1->v2:
  04/07: Fixed problem observed by Ben.

Martin Wilck (7):
  libmultipath: set pp->checkint in store_pathinfo()
  multipathd: remove init_path_check_interval()
  multipathd: print error message if checkint is not initialized
  multipathd: open client socket early
  multipathd: set DAEMON_CONFIGURE from uxlsnr thread
  multipathd: make DAEMON_SHUTDOWN a terminal state
  multipathd: only grab conf once for filter_path()

 libmultipath/defaults.h   |   1 +
 libmultipath/dict.c       |  14 ++++-
 libmultipath/discovery.c  |   1 +
 libmultipath/structs.c    |   1 +
 multipathd/cli_handlers.c |  10 ++--
 multipathd/main.c         | 104 ++++++++++++++++++++++++--------------
 multipathd/uxlsnr.c       |  11 +---
 multipathd/uxlsnr.h       |   2 +-
 8 files changed, 92 insertions(+), 52 deletions(-)

-- 
2.19.1

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

* [PATCH v2 1/7] libmultipath: set pp->checkint in store_pathinfo()
  2018-10-30 21:22 [PATCH v2 0/7] multipathd: make uxlsnr errors really fatal Martin Wilck
@ 2018-10-30 21:22 ` Martin Wilck
  2018-10-30 21:22 ` [PATCH v2 2/7] multipathd: remove init_path_check_interval() Martin Wilck
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 9+ messages in thread
From: Martin Wilck @ 2018-10-30 21:22 UTC (permalink / raw)
  To: Christophe Varoqui; +Cc: Martin Wilck, dm-devel

store_pathinfo is called with valid conf pointer anyway, so
checkint is available. pp->checkint is now valid for every
path after path_discovery().

This fixes a bad conf access in cli_add_path().

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

diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index 0bcd3c39..def5d6b2 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -103,6 +103,7 @@ store_pathinfo (vector pathvec, struct config *conf,
 	err = store_path(pathvec, pp);
 	if (err)
 		goto out;
+	pp->checkint = conf->checkint;
 
 out:
 	if (err)
diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
index 75000807..4aea4ce7 100644
--- a/multipathd/cli_handlers.c
+++ b/multipathd/cli_handlers.c
@@ -736,7 +736,6 @@ cli_add_path (void * v, char ** reply, int * len, void * data)
 			condlog(0, "%s: failed to store path info", param);
 			return 1;
 		}
-		pp->checkint = conf->checkint;
 	}
 	return ev_add_path(pp, vecs, 1);
 blacklisted:
diff --git a/multipathd/main.c b/multipathd/main.c
index fcf0d2db..c24b2b99 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2321,8 +2321,6 @@ configure (struct vectors * vecs)
 			free_path(pp);
 			i--;
 		}
-		else
-			pp->checkint = conf->checkint;
 		pthread_cleanup_pop(1);
 	}
 	if (map_discovery(vecs)) {
-- 
2.19.1

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

* [PATCH v2 2/7] multipathd: remove init_path_check_interval()
  2018-10-30 21:22 [PATCH v2 0/7] multipathd: make uxlsnr errors really fatal Martin Wilck
  2018-10-30 21:22 ` [PATCH v2 1/7] libmultipath: set pp->checkint in store_pathinfo() Martin Wilck
@ 2018-10-30 21:22 ` Martin Wilck
  2018-10-30 21:22 ` [PATCH v2 3/7] multipathd: print error message if checkint is not initialized Martin Wilck
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 9+ messages in thread
From: Martin Wilck @ 2018-10-30 21:22 UTC (permalink / raw)
  To: Christophe Varoqui; +Cc: Martin Wilck, dm-devel

After "libmultipath: set pp->checkint in store_pathinfo()",
pp-checkint should always be properly initialized, so this code
is not needed any more.

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

diff --git a/multipathd/main.c b/multipathd/main.c
index c24b2b99..e81b9c9e 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2130,19 +2130,6 @@ check_path (struct vectors * vecs, struct path * pp, int ticks)
 	return 1;
 }
 
-static void init_path_check_interval(struct vectors *vecs)
-{
-	struct config *conf;
-	struct path *pp;
-	unsigned int i;
-
-	vector_foreach_slot (vecs->pathvec, pp, i) {
-		conf = get_multipath_config();
-		pp->checkint = conf->checkint;
-		put_multipath_config(conf);
-	}
-}
-
 static void *
 checkerloop (void *ap)
 {
@@ -2726,7 +2713,6 @@ child (void * param)
 	 */
 	post_config_state(DAEMON_CONFIGURE);
 
-	init_path_check_interval(vecs);
 
 	if (poll_dmevents) {
 		if (init_dmevent_waiter(vecs)) {
-- 
2.19.1

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

* [PATCH v2 3/7] multipathd: print error message if checkint is not initialized
  2018-10-30 21:22 [PATCH v2 0/7] multipathd: make uxlsnr errors really fatal Martin Wilck
  2018-10-30 21:22 ` [PATCH v2 1/7] libmultipath: set pp->checkint in store_pathinfo() Martin Wilck
  2018-10-30 21:22 ` [PATCH v2 2/7] multipathd: remove init_path_check_interval() Martin Wilck
@ 2018-10-30 21:22 ` Martin Wilck
  2018-10-30 21:22 ` [PATCH v2 4/7] multipathd: open client socket early Martin Wilck
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 9+ messages in thread
From: Martin Wilck @ 2018-10-30 21:22 UTC (permalink / raw)
  To: Christophe Varoqui; +Cc: Martin Wilck, dm-devel

This is just a safety measure in case I overlooked something wrt
the checkint initialization. It could be reverted once we know the
error message isn't triggered.

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/defaults.h |  1 +
 libmultipath/dict.c     | 14 ++++++++++++--
 libmultipath/structs.c  |  1 +
 multipathd/main.c       |  6 ++++++
 4 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/libmultipath/defaults.h b/libmultipath/defaults.h
index 7f3839fc..65769398 100644
--- a/libmultipath/defaults.h
+++ b/libmultipath/defaults.h
@@ -46,6 +46,7 @@
 #define DEFAULT_UNKNOWN_FIND_MULTIPATHS_TIMEOUT 1
 #define DEFAULT_ALL_TG_PT ALL_TG_PT_OFF
 
+#define CHECKINT_UNDEF		(~0U)
 #define DEFAULT_CHECKINT	5
 #define MAX_CHECKINT(a)		(a << 2)
 
diff --git a/libmultipath/dict.c b/libmultipath/dict.c
index c3f5a6e6..a81c051f 100644
--- a/libmultipath/dict.c
+++ b/libmultipath/dict.c
@@ -264,7 +264,17 @@ snprint_mp_ ## option (struct config *conf, char * buff, int len,	\
 	return function (buff, len, mpe->option);			\
 }
 
-declare_def_handler(checkint, set_int)
+static int checkint_handler(struct config *conf, vector strvec)
+{
+	int rc = set_int(strvec, &conf->checkint);
+
+	if (rc)
+		return rc;
+	if (conf->checkint == CHECKINT_UNDEF)
+		conf->checkint--;
+	return 0;
+}
+
 declare_def_snprint(checkint, print_int)
 
 declare_def_handler(max_checkint, set_int)
@@ -1563,7 +1573,7 @@ init_keywords(vector keywords)
 {
 	install_keyword_root("defaults", NULL);
 	install_keyword("verbosity", &def_verbosity_handler, &snprint_def_verbosity);
-	install_keyword("polling_interval", &def_checkint_handler, &snprint_def_checkint);
+	install_keyword("polling_interval", &checkint_handler, &snprint_def_checkint);
 	install_keyword("max_polling_interval", &def_max_checkint_handler, &snprint_def_max_checkint);
 	install_keyword("reassign_maps", &def_reassign_maps_handler, &snprint_def_reassign_maps);
 	install_keyword("multipath_dir", &def_multipath_dir_handler, &snprint_def_multipath_dir);
diff --git a/libmultipath/structs.c b/libmultipath/structs.c
index caa178a6..fee899bd 100644
--- a/libmultipath/structs.c
+++ b/libmultipath/structs.c
@@ -100,6 +100,7 @@ alloc_path (void)
 		pp->fd = -1;
 		pp->tpgs = TPGS_UNDEF;
 		pp->priority = PRIO_UNDEF;
+		pp->checkint = CHECKINT_UNDEF;
 		checker_clear(&pp->checker);
 		dm_path_to_gen(pp)->ops = &dm_gen_path_ops;
 		pp->hwe = vector_alloc();
diff --git a/multipathd/main.c b/multipathd/main.c
index e81b9c9e..a2b01705 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -1839,6 +1839,12 @@ check_path (struct vectors * vecs, struct path * pp, int ticks)
 	max_checkint = conf->max_checkint;
 	verbosity = conf->verbosity;
 	put_multipath_config(conf);
+
+	if (pp->checkint == CHECKINT_UNDEF) {
+		condlog(0, "%s: BUG: checkint is not set", pp->dev);
+		pp->checkint = checkint;
+	};
+
 	if (!pp->mpp && pp->initialized == INIT_MISSING_UDEV) {
 		if (pp->retriggers < retrigger_tries) {
 			condlog(2, "%s: triggering change event to reinitialize",
-- 
2.19.1

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

* [PATCH v2 4/7] multipathd: open client socket early
  2018-10-30 21:22 [PATCH v2 0/7] multipathd: make uxlsnr errors really fatal Martin Wilck
                   ` (2 preceding siblings ...)
  2018-10-30 21:22 ` [PATCH v2 3/7] multipathd: print error message if checkint is not initialized Martin Wilck
@ 2018-10-30 21:22 ` Martin Wilck
  2018-11-01 21:49   ` Benjamin Marzinski
  2018-10-30 21:22 ` [PATCH v2 5/7] multipathd: set DAEMON_CONFIGURE from uxlsnr thread Martin Wilck
                   ` (2 subsequent siblings)
  6 siblings, 1 reply; 9+ messages in thread
From: Martin Wilck @ 2018-10-30 21:22 UTC (permalink / raw)
  To: Christophe Varoqui; +Cc: Martin Wilck, dm-devel

Open the unix socket in multipathd code and pass the fd to
uxsock_listen(). This will enable us to make the main thread
wait for successful socket initialization in a follow-up patch.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipathd/main.c   | 22 ++++++++++++++++++----
 multipathd/uxlsnr.c | 11 ++---------
 multipathd/uxlsnr.h |  2 +-
 3 files changed, 21 insertions(+), 14 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index a2b01705..e45e45dc 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -66,6 +66,7 @@ static int use_watchdog;
 #include "pgpolicies.h"
 #include "uevent.h"
 #include "log.h"
+#include "uxsock.h"
 
 #include "mpath_cmd.h"
 #include "mpath_persist.h"
@@ -1493,12 +1494,23 @@ uevqloop (void * ap)
 static void *
 uxlsnrloop (void * ap)
 {
+	int ux_sock;
+
+	pthread_cleanup_push(rcu_unregister, NULL);
+	rcu_register_thread();
+
+	ux_sock = ux_socket_listen(DEFAULT_SOCKET);
+	if (ux_sock == -1) {
+		condlog(1, "could not create uxsock: %d", errno);
+		exit_daemon();
+		goto out;
+	}
+
 	if (cli_init()) {
 		condlog(1, "Failed to init uxsock listener");
-		return NULL;
+		exit_daemon();
+		goto out;
 	}
-	pthread_cleanup_push(rcu_unregister, NULL);
-	rcu_register_thread();
 	set_handler_callback(LIST+PATHS, cli_list_paths);
 	set_handler_callback(LIST+PATHS+FMT, cli_list_paths_fmt);
 	set_handler_callback(LIST+PATHS+RAW+FMT, cli_list_paths_raw);
@@ -1553,7 +1565,9 @@ uxlsnrloop (void * ap)
 	set_handler_callback(UNSETPRKEY+MAP, cli_unsetprkey);
 
 	umask(077);
-	uxsock_listen(&uxsock_trigger, ap);
+	uxsock_listen(&uxsock_trigger, ux_sock, ap);
+
+out:
 	pthread_cleanup_pop(1);
 	return NULL;
 }
diff --git a/multipathd/uxlsnr.c b/multipathd/uxlsnr.c
index 6f666663..04967e9a 100644
--- a/multipathd/uxlsnr.c
+++ b/multipathd/uxlsnr.c
@@ -165,22 +165,15 @@ void uxsock_cleanup(void *arg)
 /*
  * entry point
  */
-void * uxsock_listen(uxsock_trigger_fn uxsock_trigger, void * trigger_data)
+void * uxsock_listen(uxsock_trigger_fn uxsock_trigger, long ux_sock,
+		     void * trigger_data)
 {
-	long ux_sock;
 	int rlen;
 	char *inbuf;
 	char *reply;
 	sigset_t mask;
 	int old_clients = MIN_POLLS;
 
-	ux_sock = ux_socket_listen(DEFAULT_SOCKET);
-
-	if (ux_sock == -1) {
-		condlog(1, "could not create uxsock: %d", errno);
-		exit_daemon();
-	}
-
 	pthread_cleanup_push(uxsock_cleanup, (void *)ux_sock);
 
 	condlog(3, "uxsock: startup listener");
diff --git a/multipathd/uxlsnr.h b/multipathd/uxlsnr.h
index d51a8f99..c7f25b2b 100644
--- a/multipathd/uxlsnr.h
+++ b/multipathd/uxlsnr.h
@@ -5,7 +5,7 @@
 
 typedef int (uxsock_trigger_fn)(char *, char **, int *, bool, void *);
 
-void * uxsock_listen(uxsock_trigger_fn uxsock_trigger,
+void * uxsock_listen(uxsock_trigger_fn uxsock_trigger, long ux_sock,
 		     void * trigger_data);
 
 #endif
-- 
2.19.1

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

* [PATCH v2 5/7] multipathd: set DAEMON_CONFIGURE from uxlsnr thread
  2018-10-30 21:22 [PATCH v2 0/7] multipathd: make uxlsnr errors really fatal Martin Wilck
                   ` (3 preceding siblings ...)
  2018-10-30 21:22 ` [PATCH v2 4/7] multipathd: open client socket early Martin Wilck
@ 2018-10-30 21:22 ` Martin Wilck
  2018-10-30 21:22 ` [PATCH v2 6/7] multipathd: make DAEMON_SHUTDOWN a terminal state Martin Wilck
  2018-10-30 21:22 ` [PATCH v2 7/7] multipathd: only grab conf once for filter_path() Martin Wilck
  6 siblings, 0 replies; 9+ messages in thread
From: Martin Wilck @ 2018-10-30 21:22 UTC (permalink / raw)
  To: Christophe Varoqui; +Cc: Martin Wilck, dm-devel

Commit ee01e841 had the intention to make multipathd quit if
the client socket couldn't be set up, because the unix socket
listener is vital for signal handling in multipathd.
But during startup, this condition might be lost if the main
thread doesn't wait for the unix listener to initialize.

Fixes: ee01e841 "multipathd: handle errors in uxlsnr as fatal"

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

diff --git a/multipathd/main.c b/multipathd/main.c
index e45e45dc..1b9b6e18 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -204,9 +204,8 @@ static void config_cleanup(void *arg)
 	pthread_mutex_unlock(&config_lock);
 }
 
-void post_config_state(enum daemon_status state)
+static void __post_config_state(enum daemon_status state)
 {
-	pthread_mutex_lock(&config_lock);
 	if (state != running_state) {
 		enum daemon_status old_state = running_state;
 
@@ -216,7 +215,14 @@ void post_config_state(enum daemon_status state)
 		do_sd_notify(old_state);
 #endif
 	}
-	pthread_mutex_unlock(&config_lock);
+}
+
+void post_config_state(enum daemon_status state)
+{
+	pthread_mutex_lock(&config_lock);
+	pthread_cleanup_push(config_cleanup, NULL);
+	__post_config_state(state);
+	pthread_cleanup_pop(1);
 }
 
 int set_config_state(enum daemon_status state)
@@ -1511,6 +1517,10 @@ uxlsnrloop (void * ap)
 		exit_daemon();
 		goto out;
 	}
+
+	/* Tell main thread that thread has started */
+	post_config_state(DAEMON_CONFIGURE);
+
 	set_handler_callback(LIST+PATHS, cli_list_paths);
 	set_handler_callback(LIST+PATHS+FMT, cli_list_paths_fmt);
 	set_handler_callback(LIST+PATHS+RAW+FMT, cli_list_paths_raw);
@@ -2728,11 +2738,26 @@ child (void * param)
 	 */
 	conf = NULL;
 
-	/*
-	 * Signal start of configuration
-	 */
-	post_config_state(DAEMON_CONFIGURE);
+	pthread_cleanup_push(config_cleanup, NULL);
+	pthread_mutex_lock(&config_lock);
 
+	__post_config_state(DAEMON_IDLE);
+	rc = pthread_create(&uxlsnr_thr, &misc_attr, uxlsnrloop, vecs);
+	if (!rc) {
+		/* Wait for uxlsnr startup */
+		while (running_state == DAEMON_IDLE)
+			pthread_cond_wait(&config_cond, &config_lock);
+	}
+	pthread_cleanup_pop(1);
+
+	if (rc) {
+		condlog(0, "failed to create cli listener: %d", rc);
+		goto failed;
+	}
+	else if (running_state != DAEMON_CONFIGURE) {
+		condlog(0, "cli listener failed to start");
+		goto failed;
+	}
 
 	if (poll_dmevents) {
 		if (init_dmevent_waiter(vecs)) {
@@ -2755,10 +2780,6 @@ child (void * param)
 		goto failed;
 	}
 	pthread_attr_destroy(&uevent_attr);
-	if ((rc = pthread_create(&uxlsnr_thr, &misc_attr, uxlsnrloop, vecs))) {
-		condlog(0, "failed to create cli listener: %d", rc);
-		goto failed;
-	}
 
 	/*
 	 * start threads
-- 
2.19.1

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

* [PATCH v2 6/7] multipathd: make DAEMON_SHUTDOWN a terminal state
  2018-10-30 21:22 [PATCH v2 0/7] multipathd: make uxlsnr errors really fatal Martin Wilck
                   ` (4 preceding siblings ...)
  2018-10-30 21:22 ` [PATCH v2 5/7] multipathd: set DAEMON_CONFIGURE from uxlsnr thread Martin Wilck
@ 2018-10-30 21:22 ` Martin Wilck
  2018-10-30 21:22 ` [PATCH v2 7/7] multipathd: only grab conf once for filter_path() Martin Wilck
  6 siblings, 0 replies; 9+ messages in thread
From: Martin Wilck @ 2018-10-30 21:22 UTC (permalink / raw)
  To: Christophe Varoqui; +Cc: Martin Wilck, dm-devel

It can happen that, before the main thread reacts on DAEMON_SHUTDOWN
and starts cancelling threads, another thread resets the state to
something else. Fix that.

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

diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
index 4aea4ce7..a0d57a53 100644
--- a/multipathd/cli_handlers.c
+++ b/multipathd/cli_handlers.c
@@ -1124,12 +1124,17 @@ cli_switch_group(void * v, char ** reply, int * len, void * data)
 int
 cli_reconfigure(void * v, char ** reply, int * len, void * data)
 {
+	int rc;
+
 	condlog(2, "reconfigure (operator)");
 
-	if (set_config_state(DAEMON_CONFIGURE) == ETIMEDOUT) {
+	rc = set_config_state(DAEMON_CONFIGURE); 
+	if (rc == ETIMEDOUT) {
 		condlog(2, "timeout starting reconfiguration");
 		return 1;
-	}
+	} else if (rc == EINVAL)
+		/* daemon shutting down */
+		return 1;
 	return 0;
 }
 
diff --git a/multipathd/main.c b/multipathd/main.c
index 1b9b6e18..2f6aa3f0 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -206,7 +206,7 @@ static void config_cleanup(void *arg)
 
 static void __post_config_state(enum daemon_status state)
 {
-	if (state != running_state) {
+	if (state != running_state && running_state != DAEMON_SHUTDOWN) {
 		enum daemon_status old_state = running_state;
 
 		running_state = state;
@@ -234,7 +234,9 @@ int set_config_state(enum daemon_status state)
 	if (running_state != state) {
 		enum daemon_status old_state = running_state;
 
-		if (running_state != DAEMON_IDLE) {
+		if (running_state == DAEMON_SHUTDOWN)
+			rc = EINVAL;
+		else if (running_state != DAEMON_IDLE) {
 			struct timespec ts;
 
 			clock_gettime(CLOCK_MONOTONIC, &ts);
@@ -2206,7 +2208,9 @@ checkerloop (void *ap)
 		if (rc == ETIMEDOUT) {
 			condlog(4, "timeout waiting for DAEMON_IDLE");
 			continue;
-		}
+		} else if (rc == EINVAL)
+			/* daemon shutdown */
+			break;
 
 		pthread_cleanup_push(cleanup_lock, &vecs->lock);
 		lock(&vecs->lock);
-- 
2.19.1

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

* [PATCH v2 7/7] multipathd: only grab conf once for filter_path()
  2018-10-30 21:22 [PATCH v2 0/7] multipathd: make uxlsnr errors really fatal Martin Wilck
                   ` (5 preceding siblings ...)
  2018-10-30 21:22 ` [PATCH v2 6/7] multipathd: make DAEMON_SHUTDOWN a terminal state Martin Wilck
@ 2018-10-30 21:22 ` Martin Wilck
  6 siblings, 0 replies; 9+ messages in thread
From: Martin Wilck @ 2018-10-30 21:22 UTC (permalink / raw)
  To: Christophe Varoqui; +Cc: Martin Wilck, dm-devel

This saves a possibly large number of cleanup push/pop calls and
slightly improves readability.

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

diff --git a/multipathd/main.c b/multipathd/main.c
index 2f6aa3f0..6289718d 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2334,16 +2334,17 @@ configure (struct vectors * vecs)
 		goto fail;
 	}
 
+	conf = get_multipath_config();
+	pthread_cleanup_push(put_multipath_config, conf);
 	vector_foreach_slot (vecs->pathvec, pp, i){
-		conf = get_multipath_config();
-		pthread_cleanup_push(put_multipath_config, conf);
 		if (filter_path(conf, pp) > 0){
 			vector_del_slot(vecs->pathvec, i);
 			free_path(pp);
 			i--;
 		}
-		pthread_cleanup_pop(1);
 	}
+	pthread_cleanup_pop(1);
+
 	if (map_discovery(vecs)) {
 		condlog(0, "configure failed at map discovery");
 		goto fail;
-- 
2.19.1

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

* Re: [PATCH v2 4/7] multipathd: open client socket early
  2018-10-30 21:22 ` [PATCH v2 4/7] multipathd: open client socket early Martin Wilck
@ 2018-11-01 21:49   ` Benjamin Marzinski
  0 siblings, 0 replies; 9+ messages in thread
From: Benjamin Marzinski @ 2018-11-01 21:49 UTC (permalink / raw)
  To: Martin Wilck; +Cc: dm-devel

On Tue, Oct 30, 2018 at 10:22:46PM +0100, Martin Wilck wrote:
> Open the unix socket in multipathd code and pass the fd to
> uxsock_listen(). This will enable us to make the main thread
> wait for successful socket initialization in a follow-up patch.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  multipathd/main.c   | 22 ++++++++++++++++++----
>  multipathd/uxlsnr.c | 11 ++---------
>  multipathd/uxlsnr.h |  2 +-
>  3 files changed, 21 insertions(+), 14 deletions(-)
> 
> diff --git a/multipathd/main.c b/multipathd/main.c
> index a2b01705..e45e45dc 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -66,6 +66,7 @@ static int use_watchdog;
>  #include "pgpolicies.h"
>  #include "uevent.h"
>  #include "log.h"
> +#include "uxsock.h"
>  
>  #include "mpath_cmd.h"
>  #include "mpath_persist.h"
> @@ -1493,12 +1494,23 @@ uevqloop (void * ap)
>  static void *
>  uxlsnrloop (void * ap)
>  {
> +	int ux_sock;
> +
> +	pthread_cleanup_push(rcu_unregister, NULL);
> +	rcu_register_thread();
> +
> +	ux_sock = ux_socket_listen(DEFAULT_SOCKET);
> +	if (ux_sock == -1) {
> +		condlog(1, "could not create uxsock: %d", errno);
> +		exit_daemon();
> +		goto out;
> +	}
> +
>  	if (cli_init()) {
>  		condlog(1, "Failed to init uxsock listener");
> -		return NULL;
> +		exit_daemon();
> +		goto out;

I don't know now much this matters, seeing as we're exitting the daemon
anyway, but we don't close ux_sock if we fail here. It would make more
sense to push and pop uxsock_cleanup from this function, instead of
uxsock_listen()

-Ben

>  	}
> -	pthread_cleanup_push(rcu_unregister, NULL);
> -	rcu_register_thread();
>  	set_handler_callback(LIST+PATHS, cli_list_paths);
>  	set_handler_callback(LIST+PATHS+FMT, cli_list_paths_fmt);
>  	set_handler_callback(LIST+PATHS+RAW+FMT, cli_list_paths_raw);
> @@ -1553,7 +1565,9 @@ uxlsnrloop (void * ap)
>  	set_handler_callback(UNSETPRKEY+MAP, cli_unsetprkey);
>  
>  	umask(077);
> -	uxsock_listen(&uxsock_trigger, ap);
> +	uxsock_listen(&uxsock_trigger, ux_sock, ap);
> +
> +out:
>  	pthread_cleanup_pop(1);
>  	return NULL;
>  }
> diff --git a/multipathd/uxlsnr.c b/multipathd/uxlsnr.c
> index 6f666663..04967e9a 100644
> --- a/multipathd/uxlsnr.c
> +++ b/multipathd/uxlsnr.c
> @@ -165,22 +165,15 @@ void uxsock_cleanup(void *arg)
>  /*
>   * entry point
>   */
> -void * uxsock_listen(uxsock_trigger_fn uxsock_trigger, void * trigger_data)
> +void * uxsock_listen(uxsock_trigger_fn uxsock_trigger, long ux_sock,
> +		     void * trigger_data)
>  {
> -	long ux_sock;
>  	int rlen;
>  	char *inbuf;
>  	char *reply;
>  	sigset_t mask;
>  	int old_clients = MIN_POLLS;
>  
> -	ux_sock = ux_socket_listen(DEFAULT_SOCKET);
> -
> -	if (ux_sock == -1) {
> -		condlog(1, "could not create uxsock: %d", errno);
> -		exit_daemon();
> -	}
> -
>  	pthread_cleanup_push(uxsock_cleanup, (void *)ux_sock);
>  
>  	condlog(3, "uxsock: startup listener");
> diff --git a/multipathd/uxlsnr.h b/multipathd/uxlsnr.h
> index d51a8f99..c7f25b2b 100644
> --- a/multipathd/uxlsnr.h
> +++ b/multipathd/uxlsnr.h
> @@ -5,7 +5,7 @@
>  
>  typedef int (uxsock_trigger_fn)(char *, char **, int *, bool, void *);
>  
> -void * uxsock_listen(uxsock_trigger_fn uxsock_trigger,
> +void * uxsock_listen(uxsock_trigger_fn uxsock_trigger, long ux_sock,
>  		     void * trigger_data);
>  
>  #endif
> -- 
> 2.19.1

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

end of thread, other threads:[~2018-11-01 21:49 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-10-30 21:22 [PATCH v2 0/7] multipathd: make uxlsnr errors really fatal Martin Wilck
2018-10-30 21:22 ` [PATCH v2 1/7] libmultipath: set pp->checkint in store_pathinfo() Martin Wilck
2018-10-30 21:22 ` [PATCH v2 2/7] multipathd: remove init_path_check_interval() Martin Wilck
2018-10-30 21:22 ` [PATCH v2 3/7] multipathd: print error message if checkint is not initialized Martin Wilck
2018-10-30 21:22 ` [PATCH v2 4/7] multipathd: open client socket early Martin Wilck
2018-11-01 21:49   ` Benjamin Marzinski
2018-10-30 21:22 ` [PATCH v2 5/7] multipathd: set DAEMON_CONFIGURE from uxlsnr thread Martin Wilck
2018-10-30 21:22 ` [PATCH v2 6/7] multipathd: make DAEMON_SHUTDOWN a terminal state Martin Wilck
2018-10-30 21:22 ` [PATCH v2 7/7] multipathd: only grab conf once for filter_path() Martin Wilck

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox