Re: [dm-devel] [PATCH] multipathd: use nanosleep for sleeping

[Martin Wilck]

On Mon, 2018-03-05 at 19:20 -0600, Benjamin Marzinski wrote:
> In order to safely use SIGALRM in a multi-threaded program, only one
> thread can schedule and wait on SIGALRM at a time. All other threads
> must have SIGALRM blocked, and be unable to schedule an alarm. The
> strict_timing code in checkerloop was unblocking SIGALRM, and calling
> setitimer(), without any locking.  Instead, it should use nanosleep()
> to sleep for the correct length of time, since that doesn't depend or
> interfere with signals. io_err_stat_loop() was calling usleep()
> without
> any locking. According to the POSIX standards, the result of a
> SIGARLM
> occuring during a usleep call is undefined, even if the calling
> thread
> blocks the signal. While this is unlikely to cause a problem on a
> modern linux system, nanosleep is guaranteed to not interact with
> SIGALRM, as long as the signal is blocked by the calling thread.

The libc manual says "On GNU systems, it is safe to use ‘sleep’ and
‘SIGALRM’ in the same program, because ‘sleep’ does not work by means
of ‘SIGALRM’." Actually, if you watch it under strace, you can see that
the sleep(1) call in checkerloop (without strict timing) translates
into a nanosleep() call. 

I'm generally with you here, it's better to be clearly on the safe
side. Anyway, I'd replaced the "usleep" call by "pselect" already, see
below.

> 
> Signed-off-by: Benjamin Marzinski 
> ---
>  libmultipath/io_err_stat.c |  3 ++-
>  multipathd/main.c  | 27 +--
>  2 files changed, 11 insertions(+), 19 deletions(-)
> 
> diff --git a/libmultipath/io_err_stat.c b/libmultipath/io_err_stat.c
> index 5b10f03..555af5d 100644
> --- a/libmultipath/io_err_stat.c
> +++ b/libmultipath/io_err_stat.c
> @@ -697,8 +697,9 @@ static void *io_err_stat_loop(void *data)
>  
>   mlockall(MCL_CURRENT | MCL_FUTURE);
>   while (1) {
> + struct timespec delay = {0, 1}; /* .1 sec */
>   service_paths();
> - usleep(10);
> + nanosleep(, NULL);
>   }

As you probably noticed, my late patch "libmultipath: fix tur checker
locking" had replaced this usleep call by a pselect() call. pselect()
doesn't use SIGALRM, either.

>  
>   pthread_cleanup_pop(1);
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 889670c..55b92be 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -1834,7 +1834,6 @@ checkerloop (void *ap)
>   struct path *pp;
>   int count = 0;
>   unsigned int i;
> - struct itimerval timer_tick_it;
>   struct timespec last_time;
>   struct config *conf;
>  
> @@ -1852,8 +1851,7 @@ checkerloop (void *ap)
>  
>   while (1) {
>   struct timespec diff_time, start_time, end_time;
> - int num_paths = 0, ticks = 0, signo, strict_timing,
> rc = 0;
> - sigset_t mask;
> + int num_paths = 0, ticks = 0, strict_timing, rc = 0;
>  
>   if (clock_gettime(CLOCK_MONOTONIC, _time) !=
> 0)
>   start_time.tv_sec = 0;
> @@ -1941,25 +1939,18 @@ checkerloop (void *ap)
>   if (!strict_timing)
>   sleep(1);

Hm, if the usleep() in the io_error thread was a problem, why wouldn't
the sleep() here be one, too? We're not holding the vecs lock here.
Btw there's a couple more sleep() and usleep() calls scattered around
the code. Anyway, it seems that they all resolve to nanosleep().

>   else {
> - timer_tick_it.it_interval.tv_sec = 0;
> - timer_tick_it.it_interval.tv_usec = 0;
>   if (diff_time.tv_nsec) {
> - timer_tick_it.it_value.tv_sec = 0;
> - timer_tick_it.it_value.tv_usec =
> + diff_time.tv_sec = 0;
> + diff_time.tv_nsec =
>1000UL * 1000 * 1000 -
> diff_time.tv_nsec;
> - } else {
> - timer_tick_it.it_value.tv_sec = 1;
> - timer_tick_it.it_value.tv_usec = 0;
> - }
> - setitimer(ITIMER_REAL, _tick_it,
> NULL);
> + } else
> + diff_time.tv_sec = 1;
>  
> - sigemptyset();
> - sigaddset(, SIGALRM);
>   condlog(3, "waiting for %lu.%06lu secs",
> - timer_tick_it.it_value.tv_sec,
> - timer_tick_it.it_value.tv_usec);
> - if (sigwait(, ) != 0) {
> - condlog(3, "sigwait failed with
> error %d",
> + diff_time.tv_sec,
> + diff_time.tv_nsec / 1000);
> + if (nanosleep(_time, NULL) != 0) {
> + condlog(3, "nanosleep failed with
> error %d",
>   

[dm-devel] [PATCH v2 2/2] multipathd: start marginal path checker thread lazily

[Martin Wilck]

I noticed that the io_error checker thread accounts for most of the
activity of multipathd even if the marginal path checking paramters
are not set (which is still the default in most installations I assume).

Therefore, start the io_error checker thread only if there's at least
one map with marginal error path checking configured. Also, make sure
the thread is really up when start_io_err_stat_thread() returns.

This requires adding a "vecs" argument to setup_map, because vecs
needs to be passed to the io_error checking code.

Signed-off-by: Martin Wilck 
---
 libmultipath/configure.c   | 14 +---
 libmultipath/configure.h   |  3 ++-
 libmultipath/io_err_stat.c | 55 ++
 libmultipath/structs_vec.c |  3 ++-
 multipathd/cli_handlers.c  |  2 +-
 multipathd/main.c  |  8 ++-
 6 files changed, 69 insertions(+), 16 deletions(-)

diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index 42b7c896ee65..fa6e21cb31af 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -41,6 +41,7 @@
 #include "uxsock.h"
 #include "wwids.h"
 #include "sysfs.h"
+#include "io_err_stat.h"
 
 /* group paths in pg by host adapter
  */
@@ -255,7 +256,8 @@ int rr_optimize_path_order(struct pathgroup *pgp)
return 0;
 }
 
-int setup_map(struct multipath *mpp, char *params, int params_size)
+int setup_map(struct multipath *mpp, char *params, int params_size,
+ struct vectors *vecs)
 {
struct pathgroup * pgp;
struct config *conf;
@@ -315,6 +317,12 @@ int setup_map(struct multipath *mpp, char *params, int 
params_size)
 
sysfs_set_scsi_tmo(mpp, conf->checkint);
put_multipath_config(conf);
+
+   if (mpp->marginal_path_double_failed_time > 0 &&
+   mpp->marginal_path_err_sample_time > 0 &&
+   mpp->marginal_path_err_recheck_gap_time > 0 &&
+   mpp->marginal_path_err_rate_threshold >= 0)
+   start_io_err_stat_thread(vecs);
/*
 * assign paths to path groups -- start with no groups and all paths
 * in mpp->paths
@@ -1019,7 +1027,7 @@ int coalesce_paths (struct vectors * vecs, vector newmp, 
char * refwwid,
verify_paths(mpp, vecs);
 
params[0] = '\0';
-   if (setup_map(mpp, params, PARAMS_SIZE)) {
+   if (setup_map(mpp, params, PARAMS_SIZE, vecs)) {
remove_map(mpp, vecs, 0);
continue;
}
@@ -1348,7 +1356,7 @@ int reload_map(struct vectors *vecs, struct multipath 
*mpp, int refresh,
}
}
}
-   if (setup_map(mpp, params, PARAMS_SIZE)) {
+   if (setup_map(mpp, params, PARAMS_SIZE, vecs)) {
condlog(0, "%s: failed to setup map", mpp->alias);
return 1;
}
diff --git a/libmultipath/configure.h b/libmultipath/configure.h
index 0f5d30a540ca..27a7e6f60a63 100644
--- a/libmultipath/configure.h
+++ b/libmultipath/configure.h
@@ -28,7 +28,8 @@ enum actions {
 
 struct vectors;
 
-int setup_map (struct multipath * mpp, char * params, int params_size );
+int setup_map (struct multipath * mpp, char * params, int params_size,
+  struct vectors *vecs );
 int domap (struct multipath * mpp, char * params, int is_daemon);
 int reinstate_paths (struct multipath *mpp);
 int coalesce_paths (struct vectors *vecs, vector curmp, char * refwwid, int 
force_reload, enum mpath_cmds cmd);
diff --git a/libmultipath/io_err_stat.c b/libmultipath/io_err_stat.c
index 536ba87968fd..ac81b4b9390d 100644
--- a/libmultipath/io_err_stat.c
+++ b/libmultipath/io_err_stat.c
@@ -74,6 +74,10 @@ struct io_err_stat_path {
 pthread_t  io_err_stat_thr;
 pthread_attr_t io_err_stat_attr;
 
+static pthread_mutex_t io_err_thread_lock = PTHREAD_MUTEX_INITIALIZER;
+static pthread_cond_t io_err_thread_cond = PTHREAD_COND_INITIALIZER;
+static int io_err_thread_running = 0;
+
 static struct io_err_stat_pathvec *paths;
 struct vectors *vecs;
 io_context_t   ioctx;
@@ -316,6 +320,9 @@ int io_err_stat_handle_pathfail(struct path *path)
struct timespec curr_time;
int res;
 
+   if (uatomic_read(_err_thread_running) == 0)
+   return 1;
+
if (path->io_err_disable_reinstate) {
io_err_stat_log(3, "%s: reinstate is already disabled",
path->dev);
@@ -380,6 +387,8 @@ int hit_io_err_recheck_time(struct path *pp)
struct timespec curr_time;
int r;
 
+   if (uatomic_read(_err_thread_running) == 0)
+   return 0;
if (pp->mpp->nr_active <= 0) {
io_err_stat_log(2, "%s: recover path early", pp->dev);
goto recover;
@@ -690,6 +699,16 @@ static void service_paths(void)
pthread_mutex_unlock(>mutex);
 }
 
+static void cleanup_unlock(void *arg)
+{
+   pthread_mutex_unlock((pthread_mutex_t*) arg);
+}
+

[dm-devel] [PATCH] multipathd: fix -Wpointer-to-int-cast warning in uxlsnr

[Martin Wilck]

Fixes: "multipathd: release uxsocket and resource when cancel thread"
Signed-off-by: Martin Wilck 
---
 multipathd/uxlsnr.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/multipathd/uxlsnr.c b/multipathd/uxlsnr.c
index 0531061912b3..cdafd82943e7 100644
--- a/multipathd/uxlsnr.c
+++ b/multipathd/uxlsnr.c
@@ -148,7 +148,7 @@ void uxsock_cleanup(void *arg)
 {
struct client *client_loop;
struct client *client_tmp;
-   int ux_sock = (int)arg;
+   long ux_sock = (long)arg;
 
close(ux_sock);
 
@@ -167,7 +167,7 @@ void uxsock_cleanup(void *arg)
  */
 void * uxsock_listen(uxsock_trigger_fn uxsock_trigger, void * trigger_data)
 {
-   int ux_sock;
+   long ux_sock;
int rlen;
char *inbuf;
char *reply;
-- 
2.16.1

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

Re: [dm-devel] [PATCH 0/6] Fixes for config file parsing

[Martin Wilck]

Just in case this is getting confusing, I've collected the latest
versions of my submissions, and what I gathered from others since
0.7.4, here (currently 83 commits ahead of last upstream commit):

https://github.com/openSUSE/multipath-tools/tree/upstream-queue

Please notify me if I missed something or made other mistakes.

Regards,
Martin

-- 
Dr. Martin Wilck , 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

Re: [dm-devel] dm-bufio: avoid false-positive Wmaybe-uninitialized warning

[Mike Snitzer]

On Tue, Mar 06 2018 at  4:33pm -0500,
Arnd Bergmann  wrote:

> On Thu, Feb 22, 2018 at 5:04 PM, Mike Snitzer  wrote:
> > On Thu, Feb 22 2018 at 10:56am -0500,
> > Arnd Bergmann  wrote:
> 
> >
> > Mikulas already sent a fix for this:
> > https://patchwork.kernel.org/patch/10211631/
> >
> > But I like yours a bit better, though I'll likely move the declaration
> > of 'noio_flag' temporary inside the conditional.
> >
> > Anyway, I'll get this fixed up shortly, thanks.
> 
> I see the fix made it into linux-next on the same day, but the build bots 
> still
> report the warning for mainline kernels and now also for stable kernels
> that got a backport of the patch that introduced it on arm64.
> 
> I assume you had not planned to send it for mainline, any chance you
> could change that and send it as a bugfix with a 'Cc:
> sta...@vger.kernel.org' tag to restore a clean build?

I did/do plan to send to Linus this week.

But I've updated the commit header to include the CC: stable like you
asked.

Thanks,
Mike

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

Re: [dm-devel] dm-bufio: avoid false-positive Wmaybe-uninitialized warning

[Arnd Bergmann]

On Thu, Feb 22, 2018 at 5:04 PM, Mike Snitzer  wrote:
> On Thu, Feb 22 2018 at 10:56am -0500,
> Arnd Bergmann  wrote:

>
> Mikulas already sent a fix for this:
> https://patchwork.kernel.org/patch/10211631/
>
> But I like yours a bit better, though I'll likely move the declaration
> of 'noio_flag' temporary inside the conditional.
>
> Anyway, I'll get this fixed up shortly, thanks.

I see the fix made it into linux-next on the same day, but the build bots still
report the warning for mainline kernels and now also for stable kernels
that got a backport of the patch that introduced it on arm64.

I assume you had not planned to send it for mainline, any chance you
could change that and send it as a bugfix with a 'Cc:
sta...@vger.kernel.org' tag to restore a clean build?

   Arnd

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

[dm-devel] [PATCH 1/6] tests/uevent: fix -Wcast-qual errors

[Martin Wilck]

I overlooked these in my previous series of fixes.

Fixes: "libmultipath: fix compiler warnings for -Wcast-qual"
Signed-off-by: Martin Wilck 
---
 tests/uevent.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/tests/uevent.c b/tests/uevent.c
index b7d6458710f4..0836b08d5b50 100644
--- a/tests/uevent.c
+++ b/tests/uevent.c
@@ -172,7 +172,7 @@ static void test_dm_name_good(void **state)
const char *name = uevent_get_dm_name(uev);
 
assert_string_equal(name, DM_NAME);
-   free((void*)name);
+   FREE_CONST(name);
 }
 
 static void test_dm_name_bad_0(void **state)
@@ -183,7 +183,7 @@ static void test_dm_name_bad_0(void **state)
uev->envp[3] = "DM_NAME" DM_NAME;
name = uevent_get_dm_name(uev);
assert_ptr_equal(name, NULL);
-   free((void*)name);
+   FREE_CONST(name);
 }
 
 static void test_dm_name_bad_1(void **state)
@@ -194,7 +194,7 @@ static void test_dm_name_bad_1(void **state)
uev->envp[3] = "DM_NAMES=" DM_NAME;
name = uevent_get_dm_name(uev);
assert_ptr_equal(name, NULL);
-   free((void*)name);
+   FREE_CONST(name);
 }
 
 static void test_dm_name_good_1(void **state)
@@ -206,7 +206,7 @@ static void test_dm_name_good_1(void **state)
uev->envp[2] = "DM_NAME=" DM_NAME;
name = uevent_get_dm_name(uev);
assert_string_equal(name, DM_NAME);
-   free((void*)name);
+   FREE_CONST(name);
 }
 
 static void test_dm_uuid_false_0(void **state)
-- 
2.16.1

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

[dm-devel] [PATCH 0/6] Fixes for config file parsing

[Martin Wilck]

This series was motivated by the real-world problem that a user couldn't
figure out how to write a blacklist entry for a device called '1.8" SSD'.
Fixing this for good turned out to be a little tricky, therefore I also
added a test suite.

Martin Wilck (6):
  tests/uevent: fix -Wcast-qual errors
  tests: add unit tests for config file parser
  libmultipath: config parser: don't strip whitepace between quotes
  libmultipath: config parser: Allow '"' in strings
  libmultipath: config parser: fix corner case for double quotes
  multipath.conf(5): improve syntax documentation

 libmultipath/parser.c  |  65 --
 multipath/multipath.conf.5 |  17 ++
 tests/Makefile |   2 +-
 tests/globals.c|   1 +
 tests/parser.c | 497 +
 tests/uevent.c |   8 +-
 6 files changed, 566 insertions(+), 24 deletions(-)
 create mode 100644 tests/parser.c

-- 
2.16.1

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

[dm-devel] [PATCH 3/6] libmultipath: config parser: don't strip whitepace between quotes

[Martin Wilck]

Between double quotes, the parser currently strips leading (but not
trailing) whitespace. That's inconsistent and unexpected. Fix it.

Signed-off-by: Martin Wilck 
---
 libmultipath/parser.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/libmultipath/parser.c b/libmultipath/parser.c
index 5caa2019a1a4..3d9656f47945 100644
--- a/libmultipath/parser.c
+++ b/libmultipath/parser.c
@@ -262,7 +262,8 @@ alloc_strvec(char *string)
}
vector_set_slot(strvec, token);
 
-   while ((isspace((int) *cp) || !isascii((int) *cp))
+   while ((!in_string &&
+   (isspace((int) *cp) || !isascii((int) *cp)))
   && *cp != '\0')
cp++;
if (*cp == '\0' || *cp == '!' || *cp == '#')
-- 
2.16.1

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

[dm-devel] [PATCH 2/6] tests: add unit tests for config file parser

[Martin Wilck]

Add test cases for parsing the config file.

Some of these tests currently fail unless the CPP macros at the top
are set to 1. The patches that follow fix them.

Signed-off-by: Martin Wilck 
---
 tests/Makefile  |   2 +-
 tests/globals.c |   1 +
 tests/parser.c  | 497 
 3 files changed, 499 insertions(+), 1 deletion(-)
 create mode 100644 tests/parser.c

diff --git a/tests/Makefile b/tests/Makefile
index f6b55836a434..231f73bc7332 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -3,7 +3,7 @@ include ../Makefile.inc
 CFLAGS += $(BIN_CFLAGS) -I$(multipathdir) -I$(mpathcmddir)
 LIBDEPS += -L$(multipathdir) -lmultipath -lcmocka
 
-TESTS := uevent
+TESTS := uevent parser
 
 .SILENT: $(TESTS:%=%.o)
 .PRECIOUS: $(TESTS:%=%-test)
diff --git a/tests/globals.c b/tests/globals.c
index 96a56515fd09..80f57bd3639a 100644
--- a/tests/globals.c
+++ b/tests/globals.c
@@ -6,6 +6,7 @@ struct udev *udev;
 int logsink = 0;
 struct config conf = {
.uid_attrs = "sd:ID_BOGUS",
+   .verbosity = 4,
 };
 
 struct config *get_multipath_config(void)
diff --git a/tests/parser.c b/tests/parser.c
new file mode 100644
index ..e77d7ef56caf
--- /dev/null
+++ b/tests/parser.c
@@ -0,0 +1,497 @@
+/*
+ * Copyright (c) 2018 SUSE Linux GmbH
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ *
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+// #include "list.h"
+#include "parser.h"
+#include "vector.h"
+
+#include "globals.c"
+
+/* Set these to 1 to get success for current broken behavior */
+/* Strip leading whitespace between quotes */
+#define LSTRIP_QUOTED_WSP 0
+/* Stop parsing at 2nd quote */
+#define TWO_QUOTES_ONLY 0
+
+#if TWO_QUOTES_ONLY
+static const char quote_marker[] = { '"', '\0' };
+#else
+static const char quote_marker[] = { '\0', '"', '\0' };
+#endif
+static bool is_quote(const char* token)
+{
+   return !memcmp(token, quote_marker, sizeof(quote_marker));
+}
+
+static char *test_file = "test.conf";
+
+/* Missing declaration */
+int validate_config_strvec(vector strvec, char *file);
+
+/* Stringify helpers */
+#define _str_(x) #x
+#define str(x) _str_(x)
+
+static int setup(void **state)
+{
+   return 0;
+}
+
+static int teardown(void **state)
+{
+   return 0;
+}
+
+static void test01(void **state)
+{
+   vector v = alloc_strvec("keyword value");
+   char *val;
+
+   assert_int_equal(validate_config_strvec(v, test_file), 0);
+   assert_int_equal(VECTOR_SIZE(v), 2);
+   assert_string_equal(VECTOR_SLOT(v, 0), "keyword");
+   assert_string_equal(VECTOR_SLOT(v, 1), "value");
+
+   val = set_value(v);
+   assert_string_equal(val, "value");
+
+   free(val);
+   free_strvec(v);
+}
+
+static void test02(void **state)
+{
+   vector v = alloc_strvec("keyword \"value\"");
+   char *val;
+
+   assert_int_equal(validate_config_strvec(v, test_file), 0);
+   assert_int_equal(VECTOR_SIZE(v), 4);
+   assert_string_equal(VECTOR_SLOT(v, 0), "keyword");
+   assert_true(is_quote(VECTOR_SLOT(v, 1)));;
+   assert_string_equal(VECTOR_SLOT(v, 2), "value");
+   assert_true(is_quote(VECTOR_SLOT(v, 3)));;
+
+   val = set_value(v);
+   assert_string_equal(val, "value");
+
+   free(val);
+   free_strvec(v);
+}
+
+static void test03(void **state)
+{
+   vector v = alloc_strvec("keyword value\n");
+   char *val;
+
+   assert_int_equal(validate_config_strvec(v, test_file), 0);
+   assert_int_equal(VECTOR_SIZE(v), 2);
+   assert_string_equal(VECTOR_SLOT(v, 0), "keyword");
+   assert_string_equal(VECTOR_SLOT(v, 1), "value");
+
+   val = set_value(v);
+   assert_string_equal(val, "value");
+
+   free(val);
+   free_strvec(v);
+}
+
+static void test04(void **state)
+{
+   vector v = alloc_strvec("keyword \t   value   \t \n   ");
+   char *val;
+
+   assert_int_equal(validate_config_strvec(v, test_file), 0);
+   assert_int_equal(VECTOR_SIZE(v), 2);
+   assert_string_equal(VECTOR_SLOT(v, 0), "keyword");
+   assert_string_equal(VECTOR_SLOT(v, 1), "value");
+
+   val = set_value(v);
+   assert_string_equal(val, "value");
+
+   free(val);
+   free_strvec(v);
+}
+
+static void test05(void **state)
+{
+   vector v = alloc_strvec("keyword \t   value   \t ! comment  ");
+   

[dm-devel] [PATCH 6/6] multipath.conf(5): improve syntax documentation

[Martin Wilck]

Describe the syntax of attribute / value pairs, comments, and quoted
strings, as well as the peculiarities of section beginnings and ends.
Also describe the newly added '""' feature.

Signed-off-by: Martin Wilck 
---
 multipath/multipath.conf.5 | 17 +
 1 file changed, 17 insertions(+)

diff --git a/multipath/multipath.conf.5 b/multipath/multipath.conf.5
index ab151e720d75..c4d0789475a3 100644
--- a/multipath/multipath.conf.5
+++ b/multipath/multipath.conf.5
@@ -67,6 +67,23 @@ recognized keywords for attributes or subsections depend on 
the
 section in which they occur.
 .LP
 .
+\fB\fR and \fB\fR must be on a single line.
+\fB\fR is one of the keywords listed in this man page.
+\fB\fR is either a simple word (containing no whitespace and none of the
+characters '\(dq', '#', and '!') or \fIone\fR string enclosed in double
+quotes ("..."). Outside a quoted string, text starting with '#', and '!' is
+regarded as a comment and ignored until the end of the line. Inside a quoted
+string, '#' and '!' are normal characters, and whitespace is preserved.
+To represent a double quote character inside a double quoted string, use two
+consecutive double quotes ('""'). Thus '2.5\(dq SSD' can be written as "2.5"" 
SSD".
+.LP
+.
+Opening braces ('{') must follow the (sub)section name on the same line. 
Closing
+braces ('}') that mark the end of a (sub)section must be the only 
non-whitespace
+character on the line. Whitespace is ignored except inside double quotes, thus
+the indentation shown in the above example is helpful for human readers but
+not mandatory.
+.LP
 .
 The following \fIsection\fP keywords are recognized:
 .TP 17
-- 
2.16.1

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

[dm-devel] [PATCH 3/4] libmultipath: uev_update_path: always warn if WWID changed

[Martin Wilck]

Print the warning about changed WWID not only if disable_changed_wwids
is set, but always. It's actually more dangerous if that option is
not set.

Signed-off-by: Martin Wilck 
---
 multipathd/main.c | 21 +
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index 6d502aceb252..f7b9676ddb28 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -940,15 +940,18 @@ uev_update_path (struct uevent *uev, struct vectors * 
vecs)
pp = find_path_by_dev(vecs->pathvec, uev->kernel);
if (pp) {
struct multipath *mpp = pp->mpp;
+   char wwid[WWID_SIZE];
 
-   if (disable_changed_wwids &&
-   (strlen(pp->wwid) || pp->wwid_changed)) {
-   char wwid[WWID_SIZE];
+   strcpy(wwid, pp->wwid);
+   get_uid(pp, pp->state, uev->udev);
 
-   strcpy(wwid, pp->wwid);
-   get_uid(pp, pp->state, uev->udev);
-   if (strcmp(wwid, pp->wwid) != 0) {
-   condlog(0, "%s: path wwid changed from '%s' to 
'%s'. disallowing", uev->kernel, wwid, pp->wwid);
+   if (strncmp(wwid, pp->wwid, WWID_SIZE) != 0) {
+   condlog(0, "%s: path wwid changed from '%s' to '%s'. 
%s",
+   uev->kernel, wwid, pp->wwid,
+   (disable_changed_wwids ? "disallowing" :
+"continuing"));
+   if (disable_changed_wwids &&
+   (strlen(wwid) || pp->wwid_changed)) {
strcpy(pp->wwid, wwid);
if (!pp->wwid_changed) {
pp->wwid_changed = 1;
@@ -957,7 +960,9 @@ uev_update_path (struct uevent *uev, struct vectors * vecs)
dm_fail_path(pp->mpp->alias, 
pp->dev_t);
}
goto out;
-   } else
+   } else if (!disable_changed_wwids)
+   strcpy(pp->wwid, wwid);
+   else
pp->wwid_changed = 0;
}
 
-- 
2.16.1

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

[dm-devel] [PATCH 5/6] libmultipath: config parser: fix corner case for double quotes

[Martin Wilck]

A corner case of the previous patch are strings starting with a double quote,
such as '"prepended to itself is false" prepended to itself is false' or
'"" is the empty string', and in particular, the string '"' ("\"" in C
notation), which is indistinguishable from the "QUOTE" token in the parsed 
strvec.

This patch fixes that by introducing a special token that can't occur as part
of a normal string to indicate the beginning and end of a quoted string.

'"' is admittedly not a very likely keyword value for multipath.conf, but
a) this is a matter of correctness, b) we didn't think of '2.5"' before, 
either, and
c) the (*str != '"') expressions would need to be patched anyway to fix the
'string starting with "' case.

Signed-off-by: Martin Wilck 
---
 libmultipath/parser.c | 36 +++-
 1 file changed, 19 insertions(+), 17 deletions(-)

diff --git a/libmultipath/parser.c b/libmultipath/parser.c
index 592269a9b5b1..da8de305f680 100644
--- a/libmultipath/parser.c
+++ b/libmultipath/parser.c
@@ -186,6 +186,12 @@ snprint_keyword(char *buff, int len, char *fmt, struct 
keyword *kw,
return fwd;
 }
 
+static const char quote_marker[] = { '\0', '"', '\0' };
+static bool is_quote(const char* token)
+{
+   return !memcmp(token, quote_marker, sizeof(quote_marker));
+}
+
 vector
 alloc_strvec(char *string)
 {
@@ -225,17 +231,13 @@ alloc_strvec(char *string)
start = cp;
if (!in_string && *cp == '"') {
cp++;
-   token = MALLOC(2);
+   token = MALLOC(sizeof(quote_marker));
 
if (!token)
goto out;
 
-   *(token) = '"';
-   *(token + 1) = '\0';
-   if (in_string)
-   in_string = 0;
-   else
-   in_string = 1;
+   memcpy(token, quote_marker, sizeof(quote_marker));
+   in_string = 1;
} else if (!in_string && (*cp == '{' || *cp == '}')) {
token = MALLOC(2);
 
@@ -324,13 +326,13 @@ set_value(vector strvec)
(char *)VECTOR_SLOT(strvec, 0));
return NULL;
}
-   size = strlen(str);
-   if (size == 0) {
-   condlog(0, "option '%s' has empty value",
-   (char *)VECTOR_SLOT(strvec, 0));
-   return NULL;
-   }
-   if (*str != '"') {
+   if (!is_quote(str)) {
+   size = strlen(str);
+   if (size == 0) {
+   condlog(0, "option '%s' has empty value",
+   (char *)VECTOR_SLOT(strvec, 0));
+   return NULL;
+   }
alloc = MALLOC(sizeof (char) * (size + 1));
if (alloc)
memcpy(alloc, str, size);
@@ -354,7 +356,7 @@ set_value(vector strvec)
(char *)VECTOR_SLOT(strvec, 0));
return NULL;
}
-   if (*str == '"')
+   if (is_quote(str))
break;
tmp = alloc;
/* The first +1 is for the NULL byte. The rest are for the
@@ -460,7 +462,7 @@ validate_config_strvec(vector strvec, char *file)
(char *)VECTOR_SLOT(strvec, 0), line_nr, file);
return -1;
}
-   if (*str != '"') {
+   if (!is_quote(str)) {
if (VECTOR_SIZE(strvec) > 2)
condlog(0, "ignoring extra data starting with '%s' on 
line %d of %s", (char *)VECTOR_SLOT(strvec, 2), line_nr, file);
return 0;
@@ -472,7 +474,7 @@ validate_config_strvec(vector strvec, char *file)
line_nr, file);
return -1;
}
-   if (*str == '"') {
+   if (is_quote(str)) {
if (VECTOR_SIZE(strvec) > i + 1)
condlog(0, "ignoring extra data starting with 
'%s' on line %d of %s", (char *)VECTOR_SLOT(strvec, (i + 1)), line_nr, file);
return 0;
-- 
2.16.1

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

[dm-devel] [PATCH 4/6] libmultipath: config parser: Allow '"' in strings

[Martin Wilck]

We have seen model strings lile '2.5" SSD' which can't be parsed
by the current config parser. This patch fixes this by allowing
'""' to represent a double quote character inside a a string.
The above model string could now be entered in the config file like this:

blacklist {
  vendor SomeCorp
  product "2.5"" SSD"
}

Signed-off-by: Martin Wilck 
---
 libmultipath/parser.c | 26 +-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/libmultipath/parser.c b/libmultipath/parser.c
index 3d9656f47945..592269a9b5b1 100644
--- a/libmultipath/parser.c
+++ b/libmultipath/parser.c
@@ -223,7 +223,7 @@ alloc_strvec(char *string)
goto out;
 
start = cp;
-   if (*cp == '"') {
+   if (!in_string && *cp == '"') {
cp++;
token = MALLOC(2);
 
@@ -246,11 +246,25 @@ alloc_strvec(char *string)
*(token + 1) = '\0';
cp++;
} else {
+   int two_quotes = 0;
+
+   move_on:
while ((in_string ||
(!isspace((int) *cp) && isascii((int) *cp) &&
 *cp != '!' && *cp != '#' && *cp != '{' &&
 *cp != '}')) && *cp != '\0' && *cp != '"')
cp++;
+
+   /* Two consecutive double quotes - don't end string */
+   if (in_string && *cp == '"') {
+   if (*(cp + 1) == '"') {
+   two_quotes = 1;
+   cp += 2;
+   goto move_on;
+   } else
+   in_string = 0;
+   }
+
strlen = cp - start;
token = MALLOC(strlen + 1);
 
@@ -259,6 +273,16 @@ alloc_strvec(char *string)
 
memcpy(token, start, strlen);
*(token + strlen) = '\0';
+
+   /* Replace "" by " */
+   if (two_quotes) {
+   char *qq = strstr(token, "\"\"");
+   while (qq != NULL) {
+   memmove(qq + 1, qq + 2,
+   strlen + 1 - (qq + 2 - token));
+   qq = strstr(qq + 1, "\"\"");
+   }
+   }
}
vector_set_slot(strvec, token);
 
-- 
2.16.1

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

[dm-devel] [PATCH 0/4] fixes for path wwid detection and path change uevents

[Martin Wilck]

Hi Christophe,

this small series fixes some minor glitches I found in the current path
discovery code, and attempts to implement the safe part of the functionality
discussed in the thread "multipathd: update path's udev in uev_update_path"
in January (based on an idea from Wu Chongyun).

Martin Wilck (4):
  libmultipath: get_uid: check VPD pages for SCSI only
  libmultipath: get_uid: don't quit prematurely without udev
  libmultipath: uev_update_path: always warn if WWID changed
  libmultipath: uev_update_path: update path properties

 libmultipath/discovery.c | 58 
 multipathd/main.c| 29 +---
 2 files changed, 55 insertions(+), 32 deletions(-)

-- 
2.16.1

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

[dm-devel] [PATCH 1/4] libmultipath: get_uid: check VPD pages for SCSI only

[Martin Wilck]

The VPD code won't work for non-SCSI devices, anyway. For indentation
reasons, I moved the "retrigger_tries" case to a separate function,
which is also called only for SCSI devices.

Signed-off-by: Martin Wilck 
---
 libmultipath/discovery.c | 50 +++-
 1 file changed, 32 insertions(+), 18 deletions(-)

diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index 3d38a2550980..53182a85fa10 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -1807,9 +1807,38 @@ get_vpd_uid(struct path * pp)
parent = udev_device_get_parent(parent);
}
 
+   if (!parent)
+   return -EINVAL;
+
return get_vpd_sysfs(parent, 0x83, pp->wwid, WWID_SIZE);
 }
 
+static ssize_t scsi_uid_fallback(struct path *pp, int path_state,
+const char **origin)
+{
+   ssize_t len = 0;
+   int retrigger;
+   struct config *conf;
+
+   conf = get_multipath_config();
+   retrigger = conf->retrigger_tries;
+   put_multipath_config(conf);
+   if (pp->retriggers >= retrigger &&
+   !strcmp(pp->uid_attribute, DEFAULT_UID_ATTRIBUTE)) {
+   len = get_vpd_uid(pp);
+   *origin = "sysfs";
+   pp->uid_attribute = NULL;
+   if (len < 0 && path_state == PATH_UP) {
+   condlog(1, "%s: failed to get sysfs uid: %s",
+   pp->dev, strerror(-len));
+   len = get_vpd_sgio(pp->fd, 0x83, pp->wwid,
+  WWID_SIZE);
+   *origin = "sgio";
+   }
+   }
+   return len;
+}
+
 int
 get_uid (struct path * pp, int path_state, struct udev_device *udev)
 {
@@ -1851,7 +1880,6 @@ get_uid (struct path * pp, int path_state, struct 
udev_device *udev)
len = get_rbd_uid(pp);
origin = "sysfs";
} else {
-   int retrigger;
 
if (pp->uid_attribute) {
len = get_udev_uid(pp, pp->uid_attribute, udev);
@@ -1861,26 +1889,12 @@ get_uid (struct path * pp, int path_state, struct 
udev_device *udev)
"%s: failed to get udev uid: %s",
pp->dev, strerror(-len));
 
-   } else {
+   } else if (pp->bus == SYSFS_BUS_SCSI) {
len = get_vpd_uid(pp);
origin = "sysfs";
}
-   conf = get_multipath_config();
-   retrigger = conf->retrigger_tries;
-   put_multipath_config(conf);
-   if (len <= 0 && pp->retriggers >= retrigger &&
-   !strcmp(pp->uid_attribute, DEFAULT_UID_ATTRIBUTE)) {
-   len = get_vpd_uid(pp);
-   origin = "sysfs";
-   pp->uid_attribute = NULL;
-   if (len < 0 && path_state == PATH_UP) {
-   condlog(1, "%s: failed to get sysfs uid: %s",
-   pp->dev, strerror(-len));
-   len = get_vpd_sgio(pp->fd, 0x83, pp->wwid,
-  WWID_SIZE);
-   origin = "sgio";
-   }
-   }
+   if (len <= 0 && pp->bus == SYSFS_BUS_SCSI)
+   len = scsi_uid_fallback(pp, path_state, );
}
if ( len < 0 ) {
condlog(1, "%s: failed to get %s uid: %s",
-- 
2.16.1

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

[dm-devel] [PATCH 2/4] libmultipath: get_uid: don't quit prematurely without udev

[Martin Wilck]

Not all the implemented methods to derive the UID rely on udev
information being present. For example getuid callout, rbd,
and the SCSI vpd code work fine without it. It's unlikely that
we don't get udev data, but we want to be as good as possible
at deriving the uid.

Signed-off-by: Martin Wilck 
---
 libmultipath/discovery.c | 8 ++--
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index 53182a85fa10..9f2a9c907914 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -1853,11 +1853,6 @@ get_uid (struct path * pp, int path_state, struct 
udev_device *udev)
put_multipath_config(conf);
}
 
-   if (!udev) {
-   condlog(1, "%s: no udev information", pp->dev);
-   return 1;
-   }
-
memset(pp->wwid, 0, WWID_SIZE);
if (pp->getuid) {
char buff[CALLOUT_MAX_SIZE];
@@ -1881,7 +1876,7 @@ get_uid (struct path * pp, int path_state, struct 
udev_device *udev)
origin = "sysfs";
} else {
 
-   if (pp->uid_attribute) {
+   if (udev && pp->uid_attribute) {
len = get_udev_uid(pp, pp->uid_attribute, udev);
origin = "udev";
if (len <= 0)
@@ -1900,6 +1895,7 @@ get_uid (struct path * pp, int path_state, struct 
udev_device *udev)
condlog(1, "%s: failed to get %s uid: %s",
pp->dev, origin, strerror(-len));
memset(pp->wwid, 0x0, WWID_SIZE);
+   return 1;
} else {
/* Strip any trailing blanks */
c = strchr(pp->wwid, '\0');
-- 
2.16.1

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

[dm-devel] [PATCH 4/4] libmultipath: uev_update_path: update path properties

[Martin Wilck]

Update pp->udev and those path attributes that can be cheaply
updated from sysfs, i.e. without IO to the disk.

Signed-off-by: Martin Wilck 
---
 multipathd/main.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/multipathd/main.c b/multipathd/main.c
index f7b9676ddb28..90f0b29ade70 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -964,6 +964,14 @@ uev_update_path (struct uevent *uev, struct vectors * vecs)
strcpy(pp->wwid, wwid);
else
pp->wwid_changed = 0;
+   } else {
+   udev_device_unref(pp->udev);
+   pp->udev = udev_device_ref(uev->udev);
+   conf = get_multipath_config();
+   if (pathinfo(pp, conf, DI_SYSFS|DI_NOIO) != PATHINFO_OK)
+   condlog(1, "%s: pathinfo failed after change 
uevent",
+   uev->kernel);
+   put_multipath_config(conf);
}
 
if (pp->initialized == INIT_REQUESTED_UDEV)
-- 
2.16.1

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

Re: [dm-devel] [PATCH v2 1/2] libmultipath: fix race in stop_io_err_stat_thread

[Hannes Reinecke]

On 03/06/2018 10:18 PM, Martin Wilck wrote:
> It's wrong, and unnecessary, to call pthread_kill after
> pthread_cancel. I have observed cases where the io_err checker
> thread hung in libpthread after receiving the USR2 signal, in particular
> when multipathd is run under strace. (If multipathd is killed with
> SIGINT under strace, and the io_error thread is running, it happens
> almost every time). If this happens, the io_err thread
> tries to obtain a mutex in the urcu code (presumably rcu_unregister_thread())
> and the main thread hangs in pthread_join().
> 
> With the change from this patch, the thread is shut down cleanly. I haven't
> observed the hang under strace with the patch.
> 
> Signed-off-by: Martin Wilck 
> ---
>  libmultipath/io_err_stat.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/libmultipath/io_err_stat.c b/libmultipath/io_err_stat.c
> index 00bac9e0e755..536ba87968fd 100644
> --- a/libmultipath/io_err_stat.c
> +++ b/libmultipath/io_err_stat.c
> @@ -749,7 +749,6 @@ destroy_ctx:
>  void stop_io_err_stat_thread(void)
>  {
>   pthread_cancel(io_err_stat_thr);
> - pthread_kill(io_err_stat_thr, SIGUSR2);
>   pthread_join(io_err_stat_thr, NULL);
>   free_io_err_pathvec(paths);
>   io_destroy(ioctx);
> 
Good point.

Reviewed-by: Hannes Reinecke 

Cheers,

Hannes
-- 
Dr. Hannes ReineckeTeamlead Storage & Networking
h...@suse.de   +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

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

Re: [dm-devel] [PATCH 0/6] Various multipath-tools patches

[Benjamin Marzinski]

On Thu, Mar 01, 2018 at 11:29:29AM -0800, Bart Van Assche wrote:
> Hello Christophe,
> 
> This series contains the following patches:
> - Four patches address Coverity complaints.
> - One patch makes a function argument const.
> - One patch addresses a bug that I discovered while inspecting the multipath
>   command line tool output.
> 
> Please consider these patches for the multipath-tools project.

Reviewed-by: Benjamin Marzinski 

For the set. Buy I agree with Martin that we should fold some of these
into his already posted patches.

-Ben

> 
> Thanks,
> 
> Bart.
> 
> Bart Van Assche (6):
>   multipathd/main.c: Fix indentation
>   libmultipath, alloc_path_with_pathinfo(): Ensure that pp->wwid is
> '\0'-terminated
>   libmultipath, alloc_path_with_pathinfo(): Declare third argument const
>   kpartx: Improve reliability of find_loop_by_file()
>   libmultipath: Fix sgio_get_vpd()
>   Introduce the ibmultipath/unaligned.h header file
> 
>  kpartx/lopart.c   |  7 ---
>  libmpathpersist/mpath_pr_ioctl.c  |  8 
>  libmultipath/checkers/hp_sw.c |  4 ++--
>  libmultipath/discovery.c  | 22 +++-
>  libmultipath/discovery.h  |  2 +-
>  libmultipath/prioritizers/alua_rtpg.c | 13 ++--
>  libmultipath/prioritizers/alua_spc3.h | 35 ++--
>  libmultipath/prioritizers/ontap.c |  4 ++--
>  libmultipath/unaligned.h  | 38 
> +++
>  multipathd/main.c |  4 ++--
>  10 files changed, 74 insertions(+), 63 deletions(-)
>  create mode 100644 libmultipath/unaligned.h
> 
> -- 
> 2.16.2
> 
> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel

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

Re: [dm-devel] [PATCH] multipathd: fix inverted signal blocking logic

[Martin Wilck]

On Mon, 2018-03-05 at 18:46 -0600, Benjamin Marzinski wrote:
> 
> This is prety close to what I had in mind. The only things missing,
> that
> I noticed when I was looking at multipathd's signal handling, is
> dealing
> with SIGUSR2 in io_err_stat.c, which it sounds like you have already
> spotted, and changing the calls to setitimer and usleep outside of
> the
> vecs lock to use nanosleep, so they don't interact with SIGALRM.
> 
> I'll send a patch based on yours, to fix these.

I'm curious to see what you come up with. I've something on my
workbench which seems to work well but needs some cleanup. The idea is
to have a dedicated "alarm dispatcher" thread that deals with kernel
timers exclusively, and forwards expiry events to the threads that have
asked for it. The dispatcher maintains a list of active alarms and sets
a kernel timer to the expiry time of the first alarm in the list.

Cheers,
Martin

-- 
Dr. Martin Wilck , 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

[dm-devel] confusion about multipath_prepare_ioctl

[Wang Sheng-Hui]

Dear,

Sorry to trouble you.

I noticed some code in dm-*.c like:
"
static int multipath_prepare_ioctl(struct dm_target *ti,
struct block_device **bdev, fmode_t *mode)
{
...
/*
 * Only pass ioctls through if the device sizes match exactly.
 */
if (!r && ti->len != i_size_read((*bdev)->bd_inode) >> SECTOR_SHIFT)
return 1;
...
}
"
Here, return value 1 means 
"ioctl is being issued against a subset of the parent bdev; require extra 
privileges."
(comment in dm_blk_ioctl)

I'm confused by the comment and '!=' test for multipath. 
In which cases, the size of low level single device is not equal to the parent 
size of multipath device?


Regards,
shenghui

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

Re: [dm-devel] [PATCH v2 23/23] multipathd: fix signal blocking logic

[Martin Wilck]

On Tue, 2018-03-06 at 08:16 +0100, Hannes Reinecke wrote:
> On 03/06/2018 12:15 AM, Martin Wilck wrote:
> > multipathd is supposed to block all signals in all threads, except
> > the uxlsnr thread which handles termination and reconfiguration
> > signals (SIGUSR1) in its ppoll() call, SIGUSR2 in the waiter thread
> > and the marginal path checker thread, and occasional SIGALRM. The
> > current
> > logic does exactly the oppsite, it blocks termination signals in
> > SIGPOLL and
> > allows multipathd to be killed e.g. by SIGALRM.
> > 
> > Fix that by inverting the logic. The argument to pthread_sigmask
> > and
> > ppoll is the set of *blocked* signals, not vice versa.
> > 
> > The marginal paths code needs to unblock SIGUSR2 now explicity, as
> > the dm-event waiter code already does. Doing this with pselect()
> > avoids asynchronous cancellation.
> > 
> > Fixes: 810082e "libmultipath, multipathd: Rework SIGPIPE handling"
> > Fixes: 534ec4c "multipathd: Ensure that SIGINT, SIGTERM, SIGHUP and
> > SIGUSR1
> > are delivered to the uxsock thread"
> > 
> > Signed-off-by: Martin Wilck 
> > ---
> >  libmultipath/io_err_stat.c | 17 -
> >  multipathd/main.c  |  7 +--
> >  multipathd/uxlsnr.c| 10 +-
> >  3 files changed, 26 insertions(+), 8 deletions(-)
> > 
> 
> Sigh.
> Will we ever get signal handling correct?

I'm quite confident that we're close now.
But time will tell.

Martin

-- 
Dr. Martin Wilck , 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