[PATCH 0/7] multipathd: make uxlsnr errors really fatal

[PATCH 0/7] multipathd: make uxlsnr errors really fatal

[Martin Wilck]

Hi Christophe,

this series, based on top of the recently submitted
"various multipath-tools patches (v2)" and "checkers overhaul" 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.

Martin

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         | 99 +++++++++++++++++++++++++--------------
 multipathd/uxlsnr.c       | 11 +----
 multipathd/uxlsnr.h       |  2 +-
 8 files changed, 88 insertions(+), 51 deletions(-)

-- 
2.19.1

[PATCH 1/7] libmultipath: set pp->checkint in store_pathinfo()

[Martin Wilck]

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().

Cc: Bart Van Assche <Bart.VanAssche@sandisk.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 467ece7a..4ac3fde1 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 e80ac906..b8423d89 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2322,8 +2322,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

Re: [PATCH 1/7] libmultipath: set pp->checkint in store_pathinfo()

[Benjamin Marzinski]

On Wed, Oct 24, 2018 at 12:05:46AM +0200, Martin Wilck wrote:
> 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 Marzinsk <bmarzins@redhat.com>

> Cc: Bart Van Assche <Bart.VanAssche@sandisk.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 467ece7a..4ac3fde1 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 e80ac906..b8423d89 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -2322,8 +2322,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

[PATCH 2/7] multipathd: remove init_path_check_interval()

[Martin Wilck]

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

Cc: Bart Van Assche <Bart.VanAssche@sandisk.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 b8423d89..3464ebfa 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2131,19 +2131,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)
 {
@@ -2727,7 +2714,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

Re: [PATCH 2/7] multipathd: remove init_path_check_interval()

[Benjamin Marzinski]

On Wed, Oct 24, 2018 at 12:05:47AM +0200, Martin Wilck wrote:
> 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 Marzinsk <bmarzins@redhat.com>

> Cc: Bart Van Assche <Bart.VanAssche@sandisk.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 b8423d89..3464ebfa 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -2131,19 +2131,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)
>  {
> @@ -2727,7 +2714,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

[PATCH 3/7] multipathd: print error message if checkint is not initialized

[Martin Wilck]

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.

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 3464ebfa..083abf28 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -1840,6 +1840,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

Re: [PATCH 3/7] multipathd: print error message if checkint is not initialized

[Benjamin Marzinski]

On Wed, Oct 24, 2018 at 12:05:48AM +0200, Martin Wilck wrote:
> 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 Marzinsk <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 3464ebfa..083abf28 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -1840,6 +1840,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

[PATCH 4/7] multipathd: open client socket early

[Martin Wilck]

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   | 17 ++++++++++++++---
 multipathd/uxlsnr.c | 11 ++---------
 multipathd/uxlsnr.h |  2 +-
 3 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index 083abf28..50f69171 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"
@@ -1494,12 +1495,22 @@ 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();
+		return NULL;
+	}
+
 	if (cli_init()) {
 		condlog(1, "Failed to init uxsock listener");
 		return NULL;
 	}
-	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);
@@ -1554,7 +1565,7 @@ uxlsnrloop (void * ap)
 	set_handler_callback(UNSETPRKEY+MAP, cli_unsetprkey);
 
 	umask(077);
-	uxsock_listen(&uxsock_trigger, ap);
+	uxsock_listen(&uxsock_trigger, ux_sock, ap);
 	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

Re: [PATCH 4/7] multipathd: open client socket early

[Benjamin Marzinski]

On Wed, Oct 24, 2018 at 12:05:49AM +0200, 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   | 17 ++++++++++++++---
>  multipathd/uxlsnr.c | 11 ++---------
>  multipathd/uxlsnr.h |  2 +-
>  3 files changed, 17 insertions(+), 13 deletions(-)
> 
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 083abf28..50f69171 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"
> @@ -1494,12 +1495,22 @@ 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();

I'm pretty sure you want to goto the end of the function, where you
pthread_cleanup_pop(), instead of returning NULL right here.  According
the to the pthread_cleanup_push() man page "Clean-up handlers are not
called if the thread terminates by performing a return from the thread
start function."

> +		return NULL;
> +	}
> +
>  	if (cli_init()) {
>  		condlog(1, "Failed to init uxsock listener");
>  		return NULL;
>  	}
> -	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);
> @@ -1554,7 +1565,7 @@ uxlsnrloop (void * ap)
>  	set_handler_callback(UNSETPRKEY+MAP, cli_unsetprkey);
>  
>  	umask(077);
> -	uxsock_listen(&uxsock_trigger, ap);
> +	uxsock_listen(&uxsock_trigger, ux_sock, ap);
>  	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

[PATCH 5/7] multipathd: set DAEMON_CONFIGURE from uxlsnr thread

[Martin Wilck]

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"
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 50f69171..c71e7d03 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -205,9 +205,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;
 
@@ -217,7 +216,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)
 		condlog(1, "Failed to init uxsock listener");
 		return NULL;
 	}
+
+	/* 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);
@@ -2726,11 +2736,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)) {
@@ -2753,10 +2778,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

Re: [PATCH 5/7] multipathd: set DAEMON_CONFIGURE from uxlsnr thread

[Benjamin Marzinski]

On Wed, Oct 24, 2018 at 12:05:50AM +0200, Martin Wilck wrote:
> 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"
> 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 50f69171..c71e7d03 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -205,9 +205,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;
>  
> @@ -217,7 +216,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)
>  		condlog(1, "Failed to init uxsock listener");
>  		return NULL;
>  	}
> +
> +	/* 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);
> @@ -2726,11 +2736,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);

I don't really understand the need for __post_config_state().  Couldn't
you just call post_config_state(DAEMON_IDLE), and then grab the
config_lock after checking that the thread was created successfully.  If
the scheduler decides to run the uxlsnrloop thread before continuing
with the child thread, it would be better if uxlsnrloop thread was free
to grab the config lock anyway.

At any rate, what you have is perfectly correct, so

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

> +	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)) {
> @@ -2753,10 +2778,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

Re: [PATCH 5/7] multipathd: set DAEMON_CONFIGURE from uxlsnr thread

[Martin Wilck]

On Fri, 2018-10-26 at 15:19 -0500,  Benjamin Marzinski  wrote:
> On Wed, Oct 24, 2018 at 12:05:50AM +0200, Martin Wilck wrote:
> > 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"
> > 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 50f69171..c71e7d03 100644
> > --- a/multipathd/main.c
> > +++ b/multipathd/main.c
> > @@ -2726,11 +2736,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);
> 
> I don't really understand the need for
> __post_config_state().  Couldn't
> you just call post_config_state(DAEMON_IDLE), and then grab the
> config_lock after checking that the thread was created
> successfully.  If
> the scheduler decides to run the uxlsnrloop thread before continuing
> with the child thread, it would be better if uxlsnrloop thread was
> free
> to grab the config lock anyway.

I wanted to enforce ordering here. The uxlsnr won't be able to grab the
lock and change running_state until the child enters
pthread_cond_wait() below, which is intentional. running_state
shouldn't be read or written without holding the lock, so this actually
saves us an unlock/lock operation, as child() would otherwise need to
release the lock before calling pthread_create(), and reacquire it
afterwards. This way the startup sequence is fully deterministic and
doesn't depend on scheduler decisions. I like it that way :-)

> At any rate, what you have is perfectly correct, so
> 
> Reviewed-by: Benjamin Marzinsk <bmarzins@redhat.com>

Thanks a lot.

Martin


> 
> > +	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)) {
> > @@ -2753,10 +2778,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
-- 
Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)


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

[PATCH 6/7] multipathd: make DAEMON_SHUTDOWN a terminal state

[Martin Wilck]

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.

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 c71e7d03..768da8da 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -207,7 +207,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;
@@ -235,7 +235,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);
@@ -2204,7 +2206,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

Re: [PATCH 6/7] multipathd: make DAEMON_SHUTDOWN a terminal state

[Benjamin Marzinski]

On Wed, Oct 24, 2018 at 12:05:51AM +0200, Martin Wilck wrote:
> 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 Marzinsk <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 c71e7d03..768da8da 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -207,7 +207,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;
> @@ -235,7 +235,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);
> @@ -2204,7 +2206,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

[PATCH 7/7] multipathd: only grab conf once for filter_path()

[Martin Wilck]

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

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 768da8da..a7cd3b09 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2332,16 +2332,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

Re: [PATCH 7/7] multipathd: only grab conf once for filter_path()

[Benjamin Marzinski]

On Wed, Oct 24, 2018 at 12:05:52AM +0200, Martin Wilck wrote:
> This saves a possibly large number of cleanup push/pop calls and
> slightly improves readability.
> 

Reviewed-by: Benjamin Marzinsk <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 768da8da..a7cd3b09 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -2332,16 +2332,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