[PATCH 00/21] libmultipath: checkers overhaul

[PATCH 00/21] libmultipath: checkers overhaul

[Martin Wilck]

Hi Christophe,

This series starts with a few minor fixes and then attempts
an overhaul of the checker code.

First, there's a block of patches to get rid of the "message"
char array in struct checker, replacing it with an integer.
This topic had been touched in recent discussion between Ben
and myself.

The next larger block fixes problems with checkers that try
to check unsupported devices. It's an interesting experience
to configure wrong checkers for the existing devices and see
what happens. With these patches, paths won't be falsely
teared down any more in such situations.

The last patch cleans up the checker data structure by splitting
it into a "checker class" and the path "checker instance".

There's more work to do in this area, but this is a start.

Regards,
Martin

Martin Wilck (21):
  libmultipath: fix use of uninitialized memory in write()
  libmultipath: fix memory leaks from scandir() use
  libmultipath/checkers: replace message by msgid
  libmultipath/checkers: cciss_tur: use message id
  libmultipath/checkers: directio: use message id
  libmultipath/checkers: emc_clariion: use message id
  libmultipath/checkers: hp_sw: use message id
  libmultipath/checkers: rdac: use message id
  libmultipath/checkers: readsector0: use message id
  libmultipath/checkers: tur: use message id
  multipathd: improve checker message logging
  libmultipath/checkers: support unsupported paths
  libmultipath: clariion checker: leave unsupported paths alone
  libmultipath: hp_sw checker: leave unsupported paths alone
  libmultipath: rdac checker: leave unsupported paths alone
  libmultipath: tur checker: leave unsupported paths alone
  libmultipath: pathinfo: don't blank wwid if checker fails
  multipathd: check_path: improve logging for "unusable path" case
  libmultipath: coalesce_paths: improve logging of orphaned paths
  libmultipath: sync_map_state: log failing paths
  libmultipath/checkers: cleanup class/instance model

 libmultipath/checkers.c              | 196 +++++++++++++++++----------
 libmultipath/checkers.h              |  65 ++++++---
 libmultipath/checkers/cciss_tur.c    |  13 +-
 libmultipath/checkers/directio.c     |  29 ++--
 libmultipath/checkers/emc_clariion.c | 108 ++++++++++++---
 libmultipath/checkers/hp_sw.c        |  39 ++++--
 libmultipath/checkers/rdac.c         |  92 +++++++++----
 libmultipath/checkers/readsector0.c  |   7 +-
 libmultipath/checkers/tur.c          |  60 ++++----
 libmultipath/config.c                |  10 +-
 libmultipath/configure.c             |  10 +-
 libmultipath/discovery.c             |   7 +-
 libmultipath/foreign.c               |   5 +-
 libmultipath/foreign/nvme.c          |   6 +-
 libmultipath/print.c                 |   2 +-
 libmultipath/propsel.c               |  19 +--
 libmultipath/structs_vec.c           |   5 +-
 libmultipath/sysfs.c                 |   5 +-
 libmultipath/util.c                  |   9 ++
 libmultipath/util.h                  |   9 ++
 multipathd/main.c                    |  37 +++--
 21 files changed, 493 insertions(+), 240 deletions(-)

-- 
2.19.0

[PATCH 01/21] libmultipath: fix use of uninitialized memory in write()

[Martin Wilck]

valgrind complained about this.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/discovery.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index 873035e5..3550c3a7 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -661,7 +661,7 @@ sysfs_set_session_tmo(struct multipath *mpp, struct path *pp)
 		} else {
 			snprintf(value, 11, "%u", mpp->fast_io_fail);
 			if (sysfs_attr_set_value(session_dev, "recovery_tmo",
-						 value, 11) <= 0) {
+						 value, strlen(value)) <= 0) {
 				condlog(3, "%s: Failed to set recovery_tmo, "
 					" error %d", pp->dev, errno);
 			}
@@ -693,7 +693,7 @@ sysfs_set_nexus_loss_tmo(struct multipath *mpp, struct path *pp)
 	if (mpp->dev_loss) {
 		snprintf(value, 11, "%u", mpp->dev_loss);
 		if (sysfs_attr_set_value(sas_dev, "I_T_nexus_loss_timeout",
-					 value, 11) <= 0)
+					 value, strlen(value)) <= 0)
 			condlog(3, "%s: failed to update "
 				"I_T Nexus loss timeout, error %d",
 				pp->dev, errno);
-- 
2.19.0

Re: [PATCH 01/21] libmultipath: fix use of uninitialized memory in write()

[Benjamin Marzinski]

On Fri, Oct 12, 2018 at 12:26:47AM +0200, Martin Wilck wrote:
> valgrind complained about this.
> 

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/discovery.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
> index 873035e5..3550c3a7 100644
> --- a/libmultipath/discovery.c
> +++ b/libmultipath/discovery.c
> @@ -661,7 +661,7 @@ sysfs_set_session_tmo(struct multipath *mpp, struct path *pp)
>  		} else {
>  			snprintf(value, 11, "%u", mpp->fast_io_fail);
>  			if (sysfs_attr_set_value(session_dev, "recovery_tmo",
> -						 value, 11) <= 0) {
> +						 value, strlen(value)) <= 0) {
>  				condlog(3, "%s: Failed to set recovery_tmo, "
>  					" error %d", pp->dev, errno);
>  			}
> @@ -693,7 +693,7 @@ sysfs_set_nexus_loss_tmo(struct multipath *mpp, struct path *pp)
>  	if (mpp->dev_loss) {
>  		snprintf(value, 11, "%u", mpp->dev_loss);
>  		if (sysfs_attr_set_value(sas_dev, "I_T_nexus_loss_timeout",
> -					 value, 11) <= 0)
> +					 value, strlen(value)) <= 0)
>  			condlog(3, "%s: failed to update "
>  				"I_T Nexus loss timeout, error %d",
>  				pp->dev, errno);
> -- 
> 2.19.0

[PATCH 02/21] libmultipath: fix memory leaks from scandir() use

[Martin Wilck]

scandir() users must not only free the resulting dirent* array,
but also every member. Add a cleanup function, and fix the
existing users of scandir() in libmultipath.

Add a small helper macro for casting function pointers to the
type pthread_cleanup_push() expects.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/config.c       | 10 ++++------
 libmultipath/foreign.c      |  5 ++++-
 libmultipath/foreign/nvme.c |  6 +++++-
 libmultipath/sysfs.c        |  5 ++++-
 libmultipath/util.c         |  9 +++++++++
 libmultipath/util.h         |  9 +++++++++
 6 files changed, 35 insertions(+), 9 deletions(-)

diff --git a/libmultipath/config.c b/libmultipath/config.c
index 0aef186a..5af7af58 100644
--- a/libmultipath/config.c
+++ b/libmultipath/config.c
@@ -639,17 +639,13 @@ free_config (struct config * conf)
 	FREE(conf);
 }
 
-static void free_namelist(void *nl)
-{
-	free(nl);
-}
-
 /* if multipath fails to process the config directory, it should continue,
  * with just a warning message */
 static void
 process_config_dir(struct config *conf, vector keywords, char *dir)
 {
 	struct dirent **namelist;
+	struct scandir_result sr;
 	int i, n;
 	char path[LINE_MAX];
 	int old_hwtable_size;
@@ -669,7 +665,9 @@ process_config_dir(struct config *conf, vector keywords, char *dir)
 		return;
 	} else if (n == 0)
 		return;
-	pthread_cleanup_push(free_namelist, namelist);
+	sr.di = namelist;
+	sr.n = n;
+	pthread_cleanup_push_cast(free_scandir_result, &sr);
 	for (i = 0; i < n; i++) {
 		if (!strstr(namelist[i]->d_name, ".conf"))
 			continue;
diff --git a/libmultipath/foreign.c b/libmultipath/foreign.c
index 80b399ba..48e8d247 100644
--- a/libmultipath/foreign.c
+++ b/libmultipath/foreign.c
@@ -115,6 +115,7 @@ static int _init_foreign(const char *multipath_dir)
 {
 	char pathbuf[PATH_MAX];
 	struct dirent **di;
+	struct scandir_result sr;
 	int r, i;
 
 	foreigns = vector_alloc();
@@ -135,7 +136,9 @@ static int _init_foreign(const char *multipath_dir)
 		return -r;
 	}
 
-	pthread_cleanup_push(free, di);
+	sr.di = di;
+	sr.n = r;
+	pthread_cleanup_push_cast(free_scandir_result, &sr);
 	for (i = 0; i < r; i++) {
 		const char *msg, *fn, *c;
 		struct foreign *fgn;
diff --git a/libmultipath/foreign/nvme.c b/libmultipath/foreign/nvme.c
index 8887a755..c753a747 100644
--- a/libmultipath/foreign/nvme.c
+++ b/libmultipath/foreign/nvme.c
@@ -27,6 +27,7 @@
 #include <dirent.h>
 #include <errno.h>
 #include <ctype.h>
+#include "util.h"
 #include "vector.h"
 #include "generic.h"
 #include "foreign.h"
@@ -534,6 +535,7 @@ static void _find_controllers(struct context *ctx, struct nvme_map *map)
 {
 	char pathbuf[PATH_MAX], realbuf[PATH_MAX];
 	struct dirent **di = NULL;
+	struct scandir_result sr;
 	struct udev_device *subsys;
 	struct nvme_path *path;
 	int r, i, n;
@@ -568,7 +570,9 @@ static void _find_controllers(struct context *ctx, struct nvme_map *map)
 		return;
 	}
 
-	pthread_cleanup_push(free, di);
+	sr.di = di;
+	sr.n = r;
+	pthread_cleanup_push_cast(free_scandir_result, &sr);
 	for (i = 0; i < r; i++) {
 		char *fn = di[i]->d_name;
 		struct udev_device *ctrl, *udev;
diff --git a/libmultipath/sysfs.c b/libmultipath/sysfs.c
index b7dacaad..558c8d6a 100644
--- a/libmultipath/sysfs.c
+++ b/libmultipath/sysfs.c
@@ -303,6 +303,7 @@ static void close_fd(void *arg)
 bool sysfs_is_multipathed(const struct path *pp)
 {
 	char pathbuf[PATH_MAX];
+	struct scandir_result sr;
 	struct dirent **di;
 	int n, r, i;
 	bool found = false;
@@ -323,7 +324,9 @@ bool sysfs_is_multipathed(const struct path *pp)
 		return false;
 	}
 
-	pthread_cleanup_push(free, di);
+	sr.di = di;
+	sr.n = r;
+	pthread_cleanup_push_cast(free_scandir_result, &sr);
 	for (i = 0; i < r && !found; i++) {
 		long fd;
 		int nr;
diff --git a/libmultipath/util.c b/libmultipath/util.c
index d08112db..66c47611 100644
--- a/libmultipath/util.c
+++ b/libmultipath/util.c
@@ -496,3 +496,12 @@ void set_max_fds(int max_fds)
 		}
 	}
 }
+
+void free_scandir_result(struct scandir_result *res)
+{
+	int i;
+
+	for (i = 0; i < res->n; i++)
+		FREE(res->di[i]);
+	FREE(res->di);
+}
diff --git a/libmultipath/util.h b/libmultipath/util.h
index c2462950..a818e29a 100644
--- a/libmultipath/util.h
+++ b/libmultipath/util.h
@@ -30,4 +30,13 @@ void set_max_fds(int max_fds);
 #define safe_snprintf(var, size, format, args...)      \
 	snprintf(var, size, format, ##args) >= size
 
+#define pthread_cleanup_push_cast(f, arg)		\
+	pthread_cleanup_push(((void (*)(void *))&f), (arg))
+
+struct scandir_result {
+	struct dirent **di;
+	int n;
+};
+void free_scandir_result(struct scandir_result *);
+
 #endif /* _UTIL_H */
-- 
2.19.0

Re: [PATCH 02/21] libmultipath: fix memory leaks from scandir() use

[Benjamin Marzinski]

On Fri, Oct 12, 2018 at 12:26:48AM +0200, Martin Wilck wrote:
> scandir() users must not only free the resulting dirent* array,
> but also every member. Add a cleanup function, and fix the
> existing users of scandir() in libmultipath.
> 
> Add a small helper macro for casting function pointers to the
> type pthread_cleanup_push() expects.
> 

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/config.c       | 10 ++++------
>  libmultipath/foreign.c      |  5 ++++-
>  libmultipath/foreign/nvme.c |  6 +++++-
>  libmultipath/sysfs.c        |  5 ++++-
>  libmultipath/util.c         |  9 +++++++++
>  libmultipath/util.h         |  9 +++++++++
>  6 files changed, 35 insertions(+), 9 deletions(-)
> 
> diff --git a/libmultipath/config.c b/libmultipath/config.c
> index 0aef186a..5af7af58 100644
> --- a/libmultipath/config.c
> +++ b/libmultipath/config.c
> @@ -639,17 +639,13 @@ free_config (struct config * conf)
>  	FREE(conf);
>  }
>  
> -static void free_namelist(void *nl)
> -{
> -	free(nl);
> -}
> -
>  /* if multipath fails to process the config directory, it should continue,
>   * with just a warning message */
>  static void
>  process_config_dir(struct config *conf, vector keywords, char *dir)
>  {
>  	struct dirent **namelist;
> +	struct scandir_result sr;
>  	int i, n;
>  	char path[LINE_MAX];
>  	int old_hwtable_size;
> @@ -669,7 +665,9 @@ process_config_dir(struct config *conf, vector keywords, char *dir)
>  		return;
>  	} else if (n == 0)
>  		return;
> -	pthread_cleanup_push(free_namelist, namelist);
> +	sr.di = namelist;
> +	sr.n = n;
> +	pthread_cleanup_push_cast(free_scandir_result, &sr);
>  	for (i = 0; i < n; i++) {
>  		if (!strstr(namelist[i]->d_name, ".conf"))
>  			continue;
> diff --git a/libmultipath/foreign.c b/libmultipath/foreign.c
> index 80b399ba..48e8d247 100644
> --- a/libmultipath/foreign.c
> +++ b/libmultipath/foreign.c
> @@ -115,6 +115,7 @@ static int _init_foreign(const char *multipath_dir)
>  {
>  	char pathbuf[PATH_MAX];
>  	struct dirent **di;
> +	struct scandir_result sr;
>  	int r, i;
>  
>  	foreigns = vector_alloc();
> @@ -135,7 +136,9 @@ static int _init_foreign(const char *multipath_dir)
>  		return -r;
>  	}
>  
> -	pthread_cleanup_push(free, di);
> +	sr.di = di;
> +	sr.n = r;
> +	pthread_cleanup_push_cast(free_scandir_result, &sr);
>  	for (i = 0; i < r; i++) {
>  		const char *msg, *fn, *c;
>  		struct foreign *fgn;
> diff --git a/libmultipath/foreign/nvme.c b/libmultipath/foreign/nvme.c
> index 8887a755..c753a747 100644
> --- a/libmultipath/foreign/nvme.c
> +++ b/libmultipath/foreign/nvme.c
> @@ -27,6 +27,7 @@
>  #include <dirent.h>
>  #include <errno.h>
>  #include <ctype.h>
> +#include "util.h"
>  #include "vector.h"
>  #include "generic.h"
>  #include "foreign.h"
> @@ -534,6 +535,7 @@ static void _find_controllers(struct context *ctx, struct nvme_map *map)
>  {
>  	char pathbuf[PATH_MAX], realbuf[PATH_MAX];
>  	struct dirent **di = NULL;
> +	struct scandir_result sr;
>  	struct udev_device *subsys;
>  	struct nvme_path *path;
>  	int r, i, n;
> @@ -568,7 +570,9 @@ static void _find_controllers(struct context *ctx, struct nvme_map *map)
>  		return;
>  	}
>  
> -	pthread_cleanup_push(free, di);
> +	sr.di = di;
> +	sr.n = r;
> +	pthread_cleanup_push_cast(free_scandir_result, &sr);
>  	for (i = 0; i < r; i++) {
>  		char *fn = di[i]->d_name;
>  		struct udev_device *ctrl, *udev;
> diff --git a/libmultipath/sysfs.c b/libmultipath/sysfs.c
> index b7dacaad..558c8d6a 100644
> --- a/libmultipath/sysfs.c
> +++ b/libmultipath/sysfs.c
> @@ -303,6 +303,7 @@ static void close_fd(void *arg)
>  bool sysfs_is_multipathed(const struct path *pp)
>  {
>  	char pathbuf[PATH_MAX];
> +	struct scandir_result sr;
>  	struct dirent **di;
>  	int n, r, i;
>  	bool found = false;
> @@ -323,7 +324,9 @@ bool sysfs_is_multipathed(const struct path *pp)
>  		return false;
>  	}
>  
> -	pthread_cleanup_push(free, di);
> +	sr.di = di;
> +	sr.n = r;
> +	pthread_cleanup_push_cast(free_scandir_result, &sr);
>  	for (i = 0; i < r && !found; i++) {
>  		long fd;
>  		int nr;
> diff --git a/libmultipath/util.c b/libmultipath/util.c
> index d08112db..66c47611 100644
> --- a/libmultipath/util.c
> +++ b/libmultipath/util.c
> @@ -496,3 +496,12 @@ void set_max_fds(int max_fds)
>  		}
>  	}
>  }
> +
> +void free_scandir_result(struct scandir_result *res)
> +{
> +	int i;
> +
> +	for (i = 0; i < res->n; i++)
> +		FREE(res->di[i]);
> +	FREE(res->di);
> +}
> diff --git a/libmultipath/util.h b/libmultipath/util.h
> index c2462950..a818e29a 100644
> --- a/libmultipath/util.h
> +++ b/libmultipath/util.h
> @@ -30,4 +30,13 @@ void set_max_fds(int max_fds);
>  #define safe_snprintf(var, size, format, args...)      \
>  	snprintf(var, size, format, ##args) >= size
>  
> +#define pthread_cleanup_push_cast(f, arg)		\
> +	pthread_cleanup_push(((void (*)(void *))&f), (arg))
> +
> +struct scandir_result {
> +	struct dirent **di;
> +	int n;
> +};
> +void free_scandir_result(struct scandir_result *);
> +
>  #endif /* _UTIL_H */
> -- 
> 2.19.0

[PATCH 03/21] libmultipath/checkers: replace message by msgid

[Martin Wilck]

Replace the character array "message" in struct checker with
a "message ID" field.

The generic checker code defines a couple of standard message IDs
and corresponding messages. Checker-specific message IDs start
at CHECKER_FIRST_MSG. Checkers that implement specific message
IDs must provide a table for converting the IDs into actual log
messages.

This simplifies the checker data structure and the handling of
checker messages in the critical checker code path. It comes at
the cost that only constant message strings are supported. It
turns out that only a single checker log message (in the emc_clariion
checker) was dynamically generated, and the missing information can
be provided with a standard condlog message.

Follow-up patches implement this for the existing checkers.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/checkers.c | 70 +++++++++++++++++++++++++++++++++++------
 libmultipath/checkers.h | 39 +++++++++++++++++++----
 2 files changed, 94 insertions(+), 15 deletions(-)

diff --git a/libmultipath/checkers.c b/libmultipath/checkers.c
index 0bacc864..359791ff 100644
--- a/libmultipath/checkers.c
+++ b/libmultipath/checkers.c
@@ -141,6 +141,22 @@ struct checker * add_checker (char *multipath_dir, char * name)
 	if (!c->free)
 		goto out;
 
+	c->msgtable_size = 0;
+	c->msgtable = dlsym(c->handle, "libcheck_msgtable");
+
+	if (c->msgtable != NULL) {
+		const char **p;
+
+		for (p = c->msgtable;
+		     *p && (p - c->msgtable) < CHECKER_MSGTABLE_SIZE; p++)
+			/* nothing */;
+
+		c->msgtable_size = p - c->msgtable;
+	} else
+		c->msgtable_size = 0;
+	condlog(3, "checker %s: message table size = %d",
+		c->name, c->msgtable_size);
+
 done:
 	c->fd = -1;
 	c->sync = 1;
@@ -222,16 +238,16 @@ int checker_check (struct checker * c, int path_state)
 	if (!c)
 		return PATH_WILD;
 
-	c->message[0] = '\0';
+	c->msgid = CHECKER_MSGID_NONE;
 	if (c->disable) {
-		MSG(c, "checker disabled");
+		c->msgid = CHECKER_MSGID_DISABLED;
 		return PATH_UNCHECKED;
 	}
 	if (!strncmp(c->name, NONE, 4))
 		return path_state;
 
 	if (c->fd < 0) {
-		MSG(c, "no usable fd");
+		c->msgid = CHECKER_MSGID_NO_FD;
 		return PATH_WILD;
 	}
 	r = c->check(c);
@@ -248,25 +264,59 @@ int checker_selected (struct checker * c)
 	return (c->check) ? 1 : 0;
 }
 
-char * checker_name (struct checker * c)
+const char *checker_name(const struct checker *c)
 {
 	if (!c)
 		return NULL;
 	return c->name;
 }
 
-char * checker_message (struct checker * c)
+static const char *generic_msg[CHECKER_LAST_GENERIC_MSGID] = {
+	[CHECKER_MSGID_NONE] = "",
+	[CHECKER_MSGID_DISABLED] = " is disabled",
+	[CHECKER_MSGID_NO_FD] = " has no usable fd",
+	[CHECKER_MSGID_INVALID] = " provided invalid message id",
+	[CHECKER_MSGID_UP] = " reports path is up",
+	[CHECKER_MSGID_DOWN] = " reports path is down",
+	[CHECKER_MSGID_GHOST] = " reports path is ghost",
+};
+
+static const char *_checker_message(const struct checker *c)
 {
-	if (!c)
+	int id;
+
+	if (!c || c->msgid < 0 ||
+	    (c->msgid >= CHECKER_LAST_GENERIC_MSGID &&
+	     c->msgid < CHECKER_FIRST_MSGID))
 		return NULL;
-	return c->message;
+
+	if (c->msgid < CHECKER_LAST_GENERIC_MSGID)
+		return generic_msg[c->msgid];
+
+	id = c->msgid - CHECKER_FIRST_MSGID;
+	if (id < c->msgtable_size)
+		return c->msgtable[id];
+	return NULL;
+}
+
+const char *checker_message(const struct checker *c)
+{
+	static char buf[CHECKER_MSG_LEN];
+	const char *msg = _checker_message(c);
+
+	if (msg == NULL || *msg == '\0')
+		*buf = '\0';
+	else
+		snprintf(buf, sizeof(buf), "%s checker%s",
+			 c->name, msg);
+	return buf;
 }
 
 void checker_clear_message (struct checker *c)
 {
 	if (!c)
 		return;
-	c->message[0] = '\0';
+	c->msgid = CHECKER_MSGID_NONE;
 }
 
 void checker_get (char *multipath_dir, struct checker * dst, char * name)
@@ -288,10 +338,12 @@ void checker_get (char *multipath_dir, struct checker * dst, char * name)
 	dst->fd = src->fd;
 	dst->sync = src->sync;
 	strncpy(dst->name, src->name, CHECKER_NAME_LEN);
-	strncpy(dst->message, src->message, CHECKER_MSG_LEN);
+	dst->msgid = src->msgid;
 	dst->check = src->check;
 	dst->init = src->init;
 	dst->free = src->free;
+	dst->msgtable = src->msgtable;
+	dst->msgtable_size = src->msgtable_size;
 	dst->handle = NULL;
 	src->refcount++;
 }
diff --git a/libmultipath/checkers.h b/libmultipath/checkers.h
index 7b18a1ac..b48f4dc2 100644
--- a/libmultipath/checkers.h
+++ b/libmultipath/checkers.h
@@ -97,6 +97,22 @@ enum path_check_state {
 #define CHECKER_DEV_LEN 256
 #define LIB_CHECKER_NAMELEN 256
 
+/*
+ * Generic message IDs for use in checkers.
+ */
+enum {
+	CHECKER_MSGID_NONE = 0,
+	CHECKER_MSGID_DISABLED,
+	CHECKER_MSGID_NO_FD,
+	CHECKER_MSGID_INVALID,
+	CHECKER_MSGID_UP,
+	CHECKER_MSGID_DOWN,
+	CHECKER_MSGID_GHOST,
+	CHECKER_LAST_GENERIC_MSGID,
+	CHECKER_FIRST_MSGID = 100,	/* lowest msgid for checkers */
+	CHECKER_MSGTABLE_SIZE = 100,	/* max msg table size for checkers */
+};
+
 struct checker {
 	struct list_head node;
 	void *handle;
@@ -106,7 +122,7 @@ struct checker {
 	unsigned int timeout;
 	int disable;
 	char name[CHECKER_NAME_LEN];
-	char message[CHECKER_MSG_LEN];       /* comm with callers */
+	short msgid;		             /* checker-internal extra status */
 	void * context;                      /* store for persistent data */
 	void ** mpcontext;                   /* store for persistent data shared
 						multipath-wide. Use MALLOC if
@@ -114,10 +130,10 @@ struct checker {
 	int (*check)(struct checker *);
 	int (*init)(struct checker *);       /* to allocate the context */
 	void (*free)(struct checker *);      /* to free the context */
+	const char**msgtable;
+	short msgtable_size;
 };
 
-#define MSG(c, fmt, args...) snprintf((c)->message, CHECKER_MSG_LEN, fmt, ##args);
-
 char * checker_state_name (int);
 int init_checkers (char *);
 void cleanup_checkers (void);
@@ -134,14 +150,25 @@ void checker_enable (struct checker *);
 void checker_disable (struct checker *);
 int checker_check (struct checker *, int);
 int checker_selected (struct checker *);
-char * checker_name (struct checker *);
-char * checker_message (struct checker *);
+const char *checker_name (const struct checker *);
+const char *checker_message (const struct checker *);
 void checker_clear_message (struct checker *c);
 void checker_get (char *, struct checker *, char *);
 
-/* Functions exported by path checker dynamic libraries (.so) */
+/* Prototypes for symbols exported by path checker dynamic libraries (.so) */
 int libcheck_check(struct checker *);
 int libcheck_init(struct checker *);
 void libcheck_free(struct checker *);
+/*
+ * msgid => message map.
+ *
+ * It only needs to be provided if the checker defines specific
+ * message IDs.
+ * Message IDs available to checkers start at CHECKER_FIRST_MSG.
+ * The msgtable array is 0-based, i.e. msgtable[0] is the message
+ * for msgid == __CHECKER_FIRST_MSG.
+ * The table ends with a NULL element.
+ */
+extern const char *libcheck_msgtable[];
 
 #endif /* _CHECKERS_H */
-- 
2.19.0

Re: [PATCH 03/21] libmultipath/checkers: replace message by msgid

[Benjamin Marzinski]

On Fri, Oct 12, 2018 at 12:26:49AM +0200, Martin Wilck wrote:
> Replace the character array "message" in struct checker with
> a "message ID" field.
> 
> The generic checker code defines a couple of standard message IDs
> and corresponding messages. Checker-specific message IDs start
> at CHECKER_FIRST_MSG. Checkers that implement specific message
> IDs must provide a table for converting the IDs into actual log
> messages.
> 
> This simplifies the checker data structure and the handling of
> checker messages in the critical checker code path. It comes at
> the cost that only constant message strings are supported. It
> turns out that only a single checker log message (in the emc_clariion
> checker) was dynamically generated, and the missing information can
> be provided with a standard condlog message.
> 
> Follow-up patches implement this for the existing checkers.

I have some cosmetic nits that I'll add inline, but none of them are
real problems so, 

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

> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/checkers.c | 70 +++++++++++++++++++++++++++++++++++------
>  libmultipath/checkers.h | 39 +++++++++++++++++++----
>  2 files changed, 94 insertions(+), 15 deletions(-)
> 
> diff --git a/libmultipath/checkers.c b/libmultipath/checkers.c
> index 0bacc864..359791ff 100644
> --- a/libmultipath/checkers.c
> +++ b/libmultipath/checkers.c
> @@ -141,6 +141,22 @@ struct checker * add_checker (char *multipath_dir, char * name)
>  	if (!c->free)
>  		goto out;
>  
> +	c->msgtable_size = 0;
> +	c->msgtable = dlsym(c->handle, "libcheck_msgtable");
> +
> +	if (c->msgtable != NULL) {
> +		const char **p;
> +
> +		for (p = c->msgtable;
> +		     *p && (p - c->msgtable) < CHECKER_MSGTABLE_SIZE; p++)
> +			/* nothing */;
> +
> +		c->msgtable_size = p - c->msgtable;
> +	} else
> +		c->msgtable_size = 0;
> +	condlog(3, "checker %s: message table size = %d",
> +		c->name, c->msgtable_size);
> +
>  done:
>  	c->fd = -1;
>  	c->sync = 1;
> @@ -222,16 +238,16 @@ int checker_check (struct checker * c, int path_state)
>  	if (!c)
>  		return PATH_WILD;
>  
> -	c->message[0] = '\0';
> +	c->msgid = CHECKER_MSGID_NONE;
>  	if (c->disable) {
> -		MSG(c, "checker disabled");
> +		c->msgid = CHECKER_MSGID_DISABLED;
>  		return PATH_UNCHECKED;
>  	}
>  	if (!strncmp(c->name, NONE, 4))
>  		return path_state;
>  
>  	if (c->fd < 0) {
> -		MSG(c, "no usable fd");
> +		c->msgid = CHECKER_MSGID_NO_FD;
>  		return PATH_WILD;
>  	}
>  	r = c->check(c);
> @@ -248,25 +264,59 @@ int checker_selected (struct checker * c)
>  	return (c->check) ? 1 : 0;
>  }
>  
> -char * checker_name (struct checker * c)
> +const char *checker_name(const struct checker *c)
>  {
>  	if (!c)
>  		return NULL;
>  	return c->name;
>  }
>  
> -char * checker_message (struct checker * c)
> +static const char *generic_msg[CHECKER_LAST_GENERIC_MSGID] = {
> +	[CHECKER_MSGID_NONE] = "",
> +	[CHECKER_MSGID_DISABLED] = " is disabled",
> +	[CHECKER_MSGID_NO_FD] = " has no usable fd",
> +	[CHECKER_MSGID_INVALID] = " provided invalid message id",
> +	[CHECKER_MSGID_UP] = " reports path is up",
> +	[CHECKER_MSGID_DOWN] = " reports path is down",
> +	[CHECKER_MSGID_GHOST] = " reports path is ghost",
> +};
> +
> +static const char *_checker_message(const struct checker *c)
>  {
> -	if (!c)
> +	int id;
> +
> +	if (!c || c->msgid < 0 ||
> +	    (c->msgid >= CHECKER_LAST_GENERIC_MSGID &&
> +	     c->msgid < CHECKER_FIRST_MSGID))
>  		return NULL;
> -	return c->message;
> +
> +	if (c->msgid < CHECKER_LAST_GENERIC_MSGID)
> +		return generic_msg[c->msgid];
> +
> +	id = c->msgid - CHECKER_FIRST_MSGID;
> +	if (id < c->msgtable_size)
> +		return c->msgtable[id];
> +	return NULL;
> +}
> +
> +const char *checker_message(const struct checker *c)
> +{
> +	static char buf[CHECKER_MSG_LEN];
> +	const char *msg = _checker_message(c);
> +
> +	if (msg == NULL || *msg == '\0')
> +		*buf = '\0';
> +	else
> +		snprintf(buf, sizeof(buf), "%s checker%s",
> +			 c->name, msg);
> +	return buf;
>  }

Since we are returning a pointer to a static buffer, it is theoretically
possible for multiple threads to be updating the buffer at the same
time. In reality, we are always calling checker_message with the vecs
lock held, seeing as we need to guarantee that the path we are checking
doesn't get deleted from pathvec while we are using it. However, it is
easy to imagine future code calling checker_message for a path that
hasn't been added to pathvec, and thus couldn't be deleted by another
thread. In this case we wouldn't need the vecs lock, except to guarantee
that we are the only thread using the static buffer. I don't really have
a problem with keeping the static buffer, but we should add a comment
that checker_message needs to be called with the vecs lock held.
  
>  void checker_clear_message (struct checker *c)
>  {
>  	if (!c)
>  		return;
> -	c->message[0] = '\0';
> +	c->msgid = CHECKER_MSGID_NONE;
>  }
>  
>  void checker_get (char *multipath_dir, struct checker * dst, char * name)
> @@ -288,10 +338,12 @@ void checker_get (char *multipath_dir, struct checker * dst, char * name)
>  	dst->fd = src->fd;
>  	dst->sync = src->sync;
>  	strncpy(dst->name, src->name, CHECKER_NAME_LEN);
> -	strncpy(dst->message, src->message, CHECKER_MSG_LEN);
> +	dst->msgid = src->msgid;

It looks odd setting dst->msgid to src->msgid, when we've never set
src->msgid.  I realize that CHECKER_MSGID_NONE = 0, and we zero out the
checker when we alloc is, so src->msgid is set to CHECKER_MSGID_NONE.
But since we are always referring to msgid as a named constant, it would
be clearer if we explicitly set it to CHECKER_MSGID_NONE in add_checker
(even though it is unnecessary). This is purely cosmetic.

>  	dst->check = src->check;
>  	dst->init = src->init;
>  	dst->free = src->free;
> +	dst->msgtable = src->msgtable;
> +	dst->msgtable_size = src->msgtable_size;
>  	dst->handle = NULL;
>  	src->refcount++;
>  }
> diff --git a/libmultipath/checkers.h b/libmultipath/checkers.h
> index 7b18a1ac..b48f4dc2 100644
> --- a/libmultipath/checkers.h
> +++ b/libmultipath/checkers.h
> @@ -97,6 +97,22 @@ enum path_check_state {
>  #define CHECKER_DEV_LEN 256
>  #define LIB_CHECKER_NAMELEN 256
>  
> +/*
> + * Generic message IDs for use in checkers.
> + */
> +enum {
> +	CHECKER_MSGID_NONE = 0,
> +	CHECKER_MSGID_DISABLED,
> +	CHECKER_MSGID_NO_FD,
> +	CHECKER_MSGID_INVALID,
> +	CHECKER_MSGID_UP,
> +	CHECKER_MSGID_DOWN,
> +	CHECKER_MSGID_GHOST,
> +	CHECKER_LAST_GENERIC_MSGID,

I prefer CHECKER_GENERIC_MSGTABLE_SIZE to CHECKER_LAST_GENERIC_MSGID.
"char *generic_msg[CHECKER_LAST_GENERIC_MSGID]" just looks like it's an
off-by-one error when you see it. Again, this is clearly cosmetic. It
just made me worried when I first saw it.

> +	CHECKER_FIRST_MSGID = 100,	/* lowest msgid for checkers */
> +	CHECKER_MSGTABLE_SIZE = 100,	/* max msg table size for checkers */
> +};
> +
>  struct checker {
>  	struct list_head node;
>  	void *handle;
> @@ -106,7 +122,7 @@ struct checker {
>  	unsigned int timeout;
>  	int disable;
>  	char name[CHECKER_NAME_LEN];
> -	char message[CHECKER_MSG_LEN];       /* comm with callers */
> +	short msgid;		             /* checker-internal extra status */
>  	void * context;                      /* store for persistent data */
>  	void ** mpcontext;                   /* store for persistent data shared
>  						multipath-wide. Use MALLOC if
> @@ -114,10 +130,10 @@ struct checker {
>  	int (*check)(struct checker *);
>  	int (*init)(struct checker *);       /* to allocate the context */
>  	void (*free)(struct checker *);      /* to free the context */
> +	const char**msgtable;
> +	short msgtable_size;
>  };
>  
> -#define MSG(c, fmt, args...) snprintf((c)->message, CHECKER_MSG_LEN, fmt, ##args);
> -
>  char * checker_state_name (int);
>  int init_checkers (char *);
>  void cleanup_checkers (void);
> @@ -134,14 +150,25 @@ void checker_enable (struct checker *);
>  void checker_disable (struct checker *);
>  int checker_check (struct checker *, int);
>  int checker_selected (struct checker *);
> -char * checker_name (struct checker *);
> -char * checker_message (struct checker *);
> +const char *checker_name (const struct checker *);
> +const char *checker_message (const struct checker *);
>  void checker_clear_message (struct checker *c);
>  void checker_get (char *, struct checker *, char *);
>  
> -/* Functions exported by path checker dynamic libraries (.so) */
> +/* Prototypes for symbols exported by path checker dynamic libraries (.so) */
>  int libcheck_check(struct checker *);
>  int libcheck_init(struct checker *);
>  void libcheck_free(struct checker *);
> +/*
> + * msgid => message map.
> + *
> + * It only needs to be provided if the checker defines specific
> + * message IDs.
> + * Message IDs available to checkers start at CHECKER_FIRST_MSG.
> + * The msgtable array is 0-based, i.e. msgtable[0] is the message
> + * for msgid == __CHECKER_FIRST_MSG.
> + * The table ends with a NULL element.
> + */
> +extern const char *libcheck_msgtable[];
>  
>  #endif /* _CHECKERS_H */
> -- 
> 2.19.0

Re: [PATCH 03/21] libmultipath/checkers: replace message by msgid

[Martin Wilck]

On Thu, 2018-10-25 at 15:57 -0500,  Benjamin Marzinski  wrote:
> On Fri, Oct 12, 2018 at 12:26:49AM +0200, Martin Wilck wrote:
> > Replace the character array "message" in struct checker with
> > a "message ID" field.
> > 
> > The generic checker code defines a couple of standard message IDs
> > and corresponding messages. Checker-specific message IDs start
> > at CHECKER_FIRST_MSG. Checkers that implement specific message
> > IDs must provide a table for converting the IDs into actual log
> > messages.
> > 
> > This simplifies the checker data structure and the handling of
> > checker messages in the critical checker code path. It comes at
> > the cost that only constant message strings are supported. It
> > turns out that only a single checker log message (in the
> > emc_clariion
> > checker) was dynamically generated, and the missing information can
> > be provided with a standard condlog message.
> > 
> > Follow-up patches implement this for the existing checkers.
> 
> I have some cosmetic nits that I'll add inline, but none of them are
> real problems so, 
> 
> Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> 
> > Signed-off-by: Martin Wilck <mwilck@suse.com>
> > ---
> > 
> [...]
> > +
> > +const char *checker_message(const struct checker *c)
> > +{
> > +	static char buf[CHECKER_MSG_LEN];
> > +	const char *msg = _checker_message(c);
> > +
> > +	if (msg == NULL || *msg == '\0')
> > +		*buf = '\0';
> > +	else
> > +		snprintf(buf, sizeof(buf), "%s checker%s",
> > +			 c->name, msg);
> > +	return buf;
> >  }
> 
> Since we are returning a pointer to a static buffer, it is
> theoretically
> possible for multiple threads to be updating the buffer at the same
> time. In reality, we are always calling checker_message with the vecs
> lock held, seeing as we need to guarantee that the path we are
> checking
> doesn't get deleted from pathvec while we are using it. However, it
> is
> easy to imagine future code calling checker_message for a path that
> hasn't been added to pathvec, and thus couldn't be deleted by another
> thread. In this case we wouldn't need the vecs lock, except to
> guarantee
> that we are the only thread using the static buffer. I don't really
> have
> a problem with keeping the static buffer, but we should add a comment
> that checker_message needs to be called with the vecs lock held.

Where to put that comment? Next to the function itself would be
pointless. Next to the definition of the vecs lock perhaps, where
people who work on the locking are likely to look?

>   
> >  void checker_clear_message (struct checker *c)
> >  {
> >  	if (!c)
> >  		return;
> > -	c->message[0] = '\0';
> > +	c->msgid = CHECKER_MSGID_NONE;
> >  }
> >  
> >  void checker_get (char *multipath_dir, struct checker * dst, char
> > * name)
> > @@ -288,10 +338,12 @@ void checker_get (char *multipath_dir, struct
> > checker * dst, char * name)
> >  	dst->fd = src->fd;
> >  	dst->sync = src->sync;
> >  	strncpy(dst->name, src->name, CHECKER_NAME_LEN);
> > -	strncpy(dst->message, src->message, CHECKER_MSG_LEN);
> > +	dst->msgid = src->msgid;
> 
> It looks odd setting dst->msgid to src->msgid, when we've never set
> src->msgid.  I realize that CHECKER_MSGID_NONE = 0, and we zero out
> the
> checker when we alloc is, so src->msgid is set to CHECKER_MSGID_NONE.
> But since we are always referring to msgid as a named constant, it
> would
> be clearer if we explicitly set it to CHECKER_MSGID_NONE in
> add_checker
> (even though it is unnecessary). This is purely cosmetic.

21/21 cleans this up.

-- 
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 04/21] libmultipath/checkers: cciss_tur: use message id

[Martin Wilck]

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/checkers/cciss_tur.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/libmultipath/checkers/cciss_tur.c b/libmultipath/checkers/cciss_tur.c
index 1cab2015..ea843742 100644
--- a/libmultipath/checkers/cciss_tur.c
+++ b/libmultipath/checkers/cciss_tur.c
@@ -42,9 +42,6 @@
 #define TUR_CMD_LEN 6
 #define HEAVY_CHECK_COUNT       10
 
-#define MSG_CCISS_TUR_UP	"cciss_tur checker reports path is up"
-#define MSG_CCISS_TUR_DOWN	"cciss_tur checker reports path is down"
-
 struct cciss_tur_checker_context {
 	void * dummy;
 };
@@ -69,7 +66,7 @@ int libcheck_check(struct checker * c)
 	IOCTL_Command_struct cic;       // cciss ioctl command
 
 	if ((c->fd) < 0) {
-		MSG(c,"no usable fd");
+		c->msgid = CHECKER_MSGID_NO_FD;
 		ret = -1;
 		goto out;
 	}
@@ -79,7 +76,7 @@ int libcheck_check(struct checker * c)
 		perror("Error: ");
 		fprintf(stderr, "cciss TUR  failed in CCISS_GETLUNINFO: %s\n",
 			strerror(errno));
-		MSG(c,MSG_CCISS_TUR_DOWN);
+		c->msgid = CHECKER_MSGID_DOWN;
 		ret = PATH_DOWN;
 		goto out;
 	} else {
@@ -106,18 +103,18 @@ int libcheck_check(struct checker * c)
 	if (rc < 0) {
 		fprintf(stderr, "cciss TUR  failed: %s\n",
 			strerror(errno));
-		MSG(c,MSG_CCISS_TUR_DOWN);
+		c->msgid = CHECKER_MSGID_DOWN;
 		ret = PATH_DOWN;
 		goto out;
 	}
 
 	if ((cic.error_info.CommandStatus | cic.error_info.ScsiStatus )) {
-		MSG(c,MSG_CCISS_TUR_DOWN);
+		c->msgid = CHECKER_MSGID_DOWN;
 		ret = PATH_DOWN;
 		goto out;
 	}
 
-	MSG(c,MSG_CCISS_TUR_UP);
+	c->msgid = CHECKER_MSGID_UP;
 
 	ret = PATH_UP;
 out:
-- 
2.19.0

Re: [PATCH 04/21] libmultipath/checkers: cciss_tur: use message id

[Benjamin Marzinski]

On Fri, Oct 12, 2018 at 12:26:50AM +0200, Martin Wilck wrote:

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

> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/checkers/cciss_tur.c | 13 +++++--------
>  1 file changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/libmultipath/checkers/cciss_tur.c b/libmultipath/checkers/cciss_tur.c
> index 1cab2015..ea843742 100644
> --- a/libmultipath/checkers/cciss_tur.c
> +++ b/libmultipath/checkers/cciss_tur.c
> @@ -42,9 +42,6 @@
>  #define TUR_CMD_LEN 6
>  #define HEAVY_CHECK_COUNT       10
>  
> -#define MSG_CCISS_TUR_UP	"cciss_tur checker reports path is up"
> -#define MSG_CCISS_TUR_DOWN	"cciss_tur checker reports path is down"
> -
>  struct cciss_tur_checker_context {
>  	void * dummy;
>  };
> @@ -69,7 +66,7 @@ int libcheck_check(struct checker * c)
>  	IOCTL_Command_struct cic;       // cciss ioctl command
>  
>  	if ((c->fd) < 0) {
> -		MSG(c,"no usable fd");
> +		c->msgid = CHECKER_MSGID_NO_FD;
>  		ret = -1;
>  		goto out;
>  	}
> @@ -79,7 +76,7 @@ int libcheck_check(struct checker * c)
>  		perror("Error: ");
>  		fprintf(stderr, "cciss TUR  failed in CCISS_GETLUNINFO: %s\n",
>  			strerror(errno));
> -		MSG(c,MSG_CCISS_TUR_DOWN);
> +		c->msgid = CHECKER_MSGID_DOWN;
>  		ret = PATH_DOWN;
>  		goto out;
>  	} else {
> @@ -106,18 +103,18 @@ int libcheck_check(struct checker * c)
>  	if (rc < 0) {
>  		fprintf(stderr, "cciss TUR  failed: %s\n",
>  			strerror(errno));
> -		MSG(c,MSG_CCISS_TUR_DOWN);
> +		c->msgid = CHECKER_MSGID_DOWN;
>  		ret = PATH_DOWN;
>  		goto out;
>  	}
>  
>  	if ((cic.error_info.CommandStatus | cic.error_info.ScsiStatus )) {
> -		MSG(c,MSG_CCISS_TUR_DOWN);
> +		c->msgid = CHECKER_MSGID_DOWN;
>  		ret = PATH_DOWN;
>  		goto out;
>  	}
>  
> -	MSG(c,MSG_CCISS_TUR_UP);
> +	c->msgid = CHECKER_MSGID_UP;
>  
>  	ret = PATH_UP;
>  out:
> -- 
> 2.19.0

[PATCH 05/21] libmultipath/checkers: directio: use message id

[Martin Wilck]

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/checkers/directio.c | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/libmultipath/checkers/directio.c b/libmultipath/checkers/directio.c
index a80848d4..c4a0712e 100644
--- a/libmultipath/checkers/directio.c
+++ b/libmultipath/checkers/directio.c
@@ -18,10 +18,19 @@
 #include "checkers.h"
 #include "../libmultipath/debug.h"
 
-#define MSG_DIRECTIO_UNKNOWN	"directio checker is not available"
-#define MSG_DIRECTIO_UP		"directio checker reports path is up"
-#define MSG_DIRECTIO_DOWN	"directio checker reports path is down"
-#define MSG_DIRECTIO_PENDING	"directio checker is waiting on aio"
+enum {
+	MSG_DIRECTIO_UNKNOWN = CHECKER_FIRST_MSGID,
+	MSG_DIRECTIO_PENDING,
+	MSG_DIRECTIO_BLOCKSIZE,
+};
+
+#define _IDX(x) (MSG_DIRECTIO_##x - CHECKER_FIRST_MSGID)
+const char *libcheck_msgtable[] = {
+	[_IDX(UNKNOWN)] = " is not available",
+	[_IDX(PENDING)] = " is waiting on aio",
+	[_IDX(BLOCKSIZE)] = " cannot get blocksize, set default",
+	NULL,
+};
 
 #define LOG(prio, fmt, args...) condlog(prio, "directio: " fmt, ##args)
 
@@ -54,7 +63,7 @@ int libcheck_init (struct checker * c)
 	}
 
 	if (ioctl(c->fd, BLKBSZGET, &ct->blksize) < 0) {
-		MSG(c, "cannot get blocksize, set default");
+		c->msgid = MSG_DIRECTIO_BLOCKSIZE;
 		ct->blksize = 512;
 	}
 	if (ct->blksize > 4096) {
@@ -198,16 +207,16 @@ int libcheck_check (struct checker * c)
 	switch (ret)
 	{
 	case PATH_UNCHECKED:
-		MSG(c, MSG_DIRECTIO_UNKNOWN);
+		c->msgid = MSG_DIRECTIO_UNKNOWN;
 		break;
 	case PATH_DOWN:
-		MSG(c, MSG_DIRECTIO_DOWN);
+		c->msgid = CHECKER_MSGID_DOWN;
 		break;
 	case PATH_UP:
-		MSG(c, MSG_DIRECTIO_UP);
+		c->msgid = CHECKER_MSGID_UP;
 		break;
 	case PATH_PENDING:
-		MSG(c, MSG_DIRECTIO_PENDING);
+		c->msgid = MSG_DIRECTIO_PENDING;
 		break;
 	default:
 		break;
-- 
2.19.0

Re: [PATCH 05/21] libmultipath/checkers: directio: use message id

[Benjamin Marzinski]

On Fri, Oct 12, 2018 at 12:26:51AM +0200, Martin Wilck wrote:

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

> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/checkers/directio.c | 27 ++++++++++++++++++---------
>  1 file changed, 18 insertions(+), 9 deletions(-)
> 
> diff --git a/libmultipath/checkers/directio.c b/libmultipath/checkers/directio.c
> index a80848d4..c4a0712e 100644
> --- a/libmultipath/checkers/directio.c
> +++ b/libmultipath/checkers/directio.c
> @@ -18,10 +18,19 @@
>  #include "checkers.h"
>  #include "../libmultipath/debug.h"
>  
> -#define MSG_DIRECTIO_UNKNOWN	"directio checker is not available"
> -#define MSG_DIRECTIO_UP		"directio checker reports path is up"
> -#define MSG_DIRECTIO_DOWN	"directio checker reports path is down"
> -#define MSG_DIRECTIO_PENDING	"directio checker is waiting on aio"
> +enum {
> +	MSG_DIRECTIO_UNKNOWN = CHECKER_FIRST_MSGID,
> +	MSG_DIRECTIO_PENDING,
> +	MSG_DIRECTIO_BLOCKSIZE,
> +};
> +
> +#define _IDX(x) (MSG_DIRECTIO_##x - CHECKER_FIRST_MSGID)
> +const char *libcheck_msgtable[] = {
> +	[_IDX(UNKNOWN)] = " is not available",
> +	[_IDX(PENDING)] = " is waiting on aio",
> +	[_IDX(BLOCKSIZE)] = " cannot get blocksize, set default",
> +	NULL,
> +};
>  
>  #define LOG(prio, fmt, args...) condlog(prio, "directio: " fmt, ##args)
>  
> @@ -54,7 +63,7 @@ int libcheck_init (struct checker * c)
>  	}
>  
>  	if (ioctl(c->fd, BLKBSZGET, &ct->blksize) < 0) {
> -		MSG(c, "cannot get blocksize, set default");
> +		c->msgid = MSG_DIRECTIO_BLOCKSIZE;
>  		ct->blksize = 512;
>  	}
>  	if (ct->blksize > 4096) {
> @@ -198,16 +207,16 @@ int libcheck_check (struct checker * c)
>  	switch (ret)
>  	{
>  	case PATH_UNCHECKED:
> -		MSG(c, MSG_DIRECTIO_UNKNOWN);
> +		c->msgid = MSG_DIRECTIO_UNKNOWN;
>  		break;
>  	case PATH_DOWN:
> -		MSG(c, MSG_DIRECTIO_DOWN);
> +		c->msgid = CHECKER_MSGID_DOWN;
>  		break;
>  	case PATH_UP:
> -		MSG(c, MSG_DIRECTIO_UP);
> +		c->msgid = CHECKER_MSGID_UP;
>  		break;
>  	case PATH_PENDING:
> -		MSG(c, MSG_DIRECTIO_PENDING);
> +		c->msgid = MSG_DIRECTIO_PENDING;
>  		break;
>  	default:
>  		break;
> -- 
> 2.19.0

[PATCH 06/21] libmultipath/checkers: emc_clariion: use message id

[Martin Wilck]

emc_clariion is the only path checker that was using a non-constant
message ("read error" case). This isn't possible with the msgid
approach any more. Use condlog() for the dynamic log message and
simply report "read error" as checker message.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/checkers/emc_clariion.c | 60 +++++++++++++++++++---------
 1 file changed, 42 insertions(+), 18 deletions(-)

diff --git a/libmultipath/checkers/emc_clariion.c b/libmultipath/checkers/emc_clariion.c
index 9115b1b9..f8e55b93 100644
--- a/libmultipath/checkers/emc_clariion.c
+++ b/libmultipath/checkers/emc_clariion.c
@@ -41,6 +41,35 @@
 				((struct emc_clariion_checker_LU_context *)\
 					(*c->mpcontext))->inactive_snap = 0
 
+enum {
+	MSG_CLARIION_QUERY_FAILED = CHECKER_FIRST_MSGID,
+	MSG_CLARIION_QUERY_ERROR,
+	MSG_CLARIION_PATH_CONFIG,
+	MSG_CLARIION_UNIT_REPORT,
+	MSG_CLARIION_PATH_NOT_AVAIL,
+	MSG_CLARIION_LUN_UNBOUND,
+	MSG_CLARIION_WWN_CHANGED,
+	MSG_CLARIION_READ_ERROR,
+	MSG_CLARIION_PASSIVE_GOOD,
+};
+
+#define _IDX(x) (MSG_CLARIION_ ## x - CHECKER_FIRST_MSGID)
+const char *libcheck_msgtable[] = {
+	[_IDX(QUERY_FAILED)] = ": sending query command failed",
+	[_IDX(QUERY_ERROR)] = ": query command indicates error",
+	[_IDX(PATH_CONFIG)] =
+	": Path not correctly configured for failover",
+	[_IDX(UNIT_REPORT)] =
+	": Path unit report page in unknown format",
+	[_IDX(PATH_NOT_AVAIL)] =
+	": Path not available for normal operations",
+	[_IDX(LUN_UNBOUND)] = ": Logical Unit is unbound or LUNZ",
+	[_IDX(WWN_CHANGED)] = ": Logical Unit WWN has changed",
+	[_IDX(READ_ERROR)] = ": Read error",
+	[_IDX(PASSIVE_GOOD)] = ": Active path is healthy",
+	NULL,
+};
+
 struct emc_clariion_checker_path_context {
 	char wwn[16];
 	unsigned wwn_set;
@@ -116,17 +145,16 @@ int libcheck_check (struct checker * c)
 	io_hdr.timeout = c->timeout * 1000;
 	io_hdr.pack_id = 0;
 	if (ioctl(c->fd, SG_IO, &io_hdr) < 0) {
-		MSG(c, "emc_clariion_checker: sending query command failed");
+		c->msgid = MSG_CLARIION_QUERY_FAILED;
 		return PATH_DOWN;
 	}
 	if (io_hdr.info & SG_INFO_OK_MASK) {
-		MSG(c, "emc_clariion_checker: query command indicates error");
+		c->msgid = MSG_CLARIION_QUERY_ERROR;
 		return PATH_DOWN;
 	}
 	if (/* Verify the code page - right page & revision */
 	    sense_buffer[1] != 0xc0 || sense_buffer[9] != 0x00) {
-		MSG(c, "emc_clariion_checker: Path unit report page in "
-		    "unknown format");
+		c->msgid = MSG_CLARIION_UNIT_REPORT;
 		return PATH_DOWN;
 	}
 
@@ -140,22 +168,19 @@ int libcheck_check (struct checker * c)
 		    ((sense_buffer[28] & 0x07) != 0x06))
 		/* Arraycommpath should be set to 1 */
 		|| (sense_buffer[30] & 0x04) != 0x04) {
-		MSG(c, "emc_clariion_checker: Path not correctly configured "
-		    "for failover");
+		c->msgid = MSG_CLARIION_PATH_CONFIG;
 		return PATH_DOWN;
 	}
 
 	if ( /* LUN operations should indicate normal operations */
 		sense_buffer[48] != 0x00) {
-		MSG(c, "emc_clariion_checker: Path not available for normal "
-		    "operations");
+		c->msgid = MSG_CLARIION_PATH_NOT_AVAIL;
 		return PATH_SHAKY;
 	}
 
 	if ( /* LUN should at least be bound somewhere and not be LUNZ */
 		sense_buffer[4] == 0x00) {
-		MSG(c, "emc_clariion_checker: Logical Unit is unbound "
-		    "or LUNZ");
+		c->msgid = MSG_CLARIION_LUN_UNBOUND;
 		return PATH_DOWN;
 	}
 
@@ -166,8 +191,7 @@ int libcheck_check (struct checker * c)
 	 */
 	if (ct->wwn_set) {
 		if (memcmp(ct->wwn, &sense_buffer[10], 16) != 0) {
-			MSG(c, "emc_clariion_checker: Logical Unit WWN "
-			    "has changed!");
+			c->msgid = MSG_CLARIION_WWN_CHANGED;
 			return PATH_DOWN;
 		}
 	} else {
@@ -202,14 +226,15 @@ int libcheck_check (struct checker * c)
 				condlog(3, "emc_clariion_checker: Active "
 					"path to inactive snapshot WWN %s.",
 					wwnstr);
-			} else
-				MSG(c, "emc_clariion_checker: Read "
+			} else {
+				condlog(3, "emc_clariion_checker: Read "
 					"error for WWN %s.  Sense data are "
 					"0x%x/0x%x/0x%x.", wwnstr,
 					sbb[2]&0xf, sbb[12], sbb[13]);
+				c->msgid = MSG_CLARIION_READ_ERROR;
+			}
 		} else {
-			MSG(c, "emc_clariion_checker: Active path is "
-			    "healthy.");
+			c->msgid = MSG_CLARIION_PASSIVE_GOOD;
 			/*
 			 * Remove the path from the set of paths to inactive
 			 * snapshot LUs if it was in this list since the
@@ -225,8 +250,7 @@ int libcheck_check (struct checker * c)
 				wwnstr);
 			ret = PATH_DOWN;
 		} else {
-			MSG(c,
-			    "emc_clariion_checker: Passive path is healthy.");
+			c->msgid = MSG_CLARIION_PASSIVE_GOOD;
 			ret = PATH_UP;	/* not ghost */
 		}
 	}
-- 
2.19.0

Re: [PATCH 06/21] libmultipath/checkers: emc_clariion: use message id

[Benjamin Marzinski]

On Fri, Oct 12, 2018 at 12:26:52AM +0200, Martin Wilck wrote:
> emc_clariion is the only path checker that was using a non-constant
> message ("read error" case). This isn't possible with the msgid
> approach any more. Use condlog() for the dynamic log message and
> simply report "read error" as checker message.
> 

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

> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/checkers/emc_clariion.c | 60 +++++++++++++++++++---------
>  1 file changed, 42 insertions(+), 18 deletions(-)
> 
> diff --git a/libmultipath/checkers/emc_clariion.c b/libmultipath/checkers/emc_clariion.c
> index 9115b1b9..f8e55b93 100644
> --- a/libmultipath/checkers/emc_clariion.c
> +++ b/libmultipath/checkers/emc_clariion.c
> @@ -41,6 +41,35 @@
>  				((struct emc_clariion_checker_LU_context *)\
>  					(*c->mpcontext))->inactive_snap = 0
>  
> +enum {
> +	MSG_CLARIION_QUERY_FAILED = CHECKER_FIRST_MSGID,
> +	MSG_CLARIION_QUERY_ERROR,
> +	MSG_CLARIION_PATH_CONFIG,
> +	MSG_CLARIION_UNIT_REPORT,
> +	MSG_CLARIION_PATH_NOT_AVAIL,
> +	MSG_CLARIION_LUN_UNBOUND,
> +	MSG_CLARIION_WWN_CHANGED,
> +	MSG_CLARIION_READ_ERROR,
> +	MSG_CLARIION_PASSIVE_GOOD,
> +};
> +
> +#define _IDX(x) (MSG_CLARIION_ ## x - CHECKER_FIRST_MSGID)
> +const char *libcheck_msgtable[] = {
> +	[_IDX(QUERY_FAILED)] = ": sending query command failed",
> +	[_IDX(QUERY_ERROR)] = ": query command indicates error",
> +	[_IDX(PATH_CONFIG)] =
> +	": Path not correctly configured for failover",
> +	[_IDX(UNIT_REPORT)] =
> +	": Path unit report page in unknown format",
> +	[_IDX(PATH_NOT_AVAIL)] =
> +	": Path not available for normal operations",
> +	[_IDX(LUN_UNBOUND)] = ": Logical Unit is unbound or LUNZ",
> +	[_IDX(WWN_CHANGED)] = ": Logical Unit WWN has changed",
> +	[_IDX(READ_ERROR)] = ": Read error",
> +	[_IDX(PASSIVE_GOOD)] = ": Active path is healthy",
> +	NULL,
> +};
> +
>  struct emc_clariion_checker_path_context {
>  	char wwn[16];
>  	unsigned wwn_set;
> @@ -116,17 +145,16 @@ int libcheck_check (struct checker * c)
>  	io_hdr.timeout = c->timeout * 1000;
>  	io_hdr.pack_id = 0;
>  	if (ioctl(c->fd, SG_IO, &io_hdr) < 0) {
> -		MSG(c, "emc_clariion_checker: sending query command failed");
> +		c->msgid = MSG_CLARIION_QUERY_FAILED;
>  		return PATH_DOWN;
>  	}
>  	if (io_hdr.info & SG_INFO_OK_MASK) {
> -		MSG(c, "emc_clariion_checker: query command indicates error");
> +		c->msgid = MSG_CLARIION_QUERY_ERROR;
>  		return PATH_DOWN;
>  	}
>  	if (/* Verify the code page - right page & revision */
>  	    sense_buffer[1] != 0xc0 || sense_buffer[9] != 0x00) {
> -		MSG(c, "emc_clariion_checker: Path unit report page in "
> -		    "unknown format");
> +		c->msgid = MSG_CLARIION_UNIT_REPORT;
>  		return PATH_DOWN;
>  	}
>  
> @@ -140,22 +168,19 @@ int libcheck_check (struct checker * c)
>  		    ((sense_buffer[28] & 0x07) != 0x06))
>  		/* Arraycommpath should be set to 1 */
>  		|| (sense_buffer[30] & 0x04) != 0x04) {
> -		MSG(c, "emc_clariion_checker: Path not correctly configured "
> -		    "for failover");
> +		c->msgid = MSG_CLARIION_PATH_CONFIG;
>  		return PATH_DOWN;
>  	}
>  
>  	if ( /* LUN operations should indicate normal operations */
>  		sense_buffer[48] != 0x00) {
> -		MSG(c, "emc_clariion_checker: Path not available for normal "
> -		    "operations");
> +		c->msgid = MSG_CLARIION_PATH_NOT_AVAIL;
>  		return PATH_SHAKY;
>  	}
>  
>  	if ( /* LUN should at least be bound somewhere and not be LUNZ */
>  		sense_buffer[4] == 0x00) {
> -		MSG(c, "emc_clariion_checker: Logical Unit is unbound "
> -		    "or LUNZ");
> +		c->msgid = MSG_CLARIION_LUN_UNBOUND;
>  		return PATH_DOWN;
>  	}
>  
> @@ -166,8 +191,7 @@ int libcheck_check (struct checker * c)
>  	 */
>  	if (ct->wwn_set) {
>  		if (memcmp(ct->wwn, &sense_buffer[10], 16) != 0) {
> -			MSG(c, "emc_clariion_checker: Logical Unit WWN "
> -			    "has changed!");
> +			c->msgid = MSG_CLARIION_WWN_CHANGED;
>  			return PATH_DOWN;
>  		}
>  	} else {
> @@ -202,14 +226,15 @@ int libcheck_check (struct checker * c)
>  				condlog(3, "emc_clariion_checker: Active "
>  					"path to inactive snapshot WWN %s.",
>  					wwnstr);
> -			} else
> -				MSG(c, "emc_clariion_checker: Read "
> +			} else {
> +				condlog(3, "emc_clariion_checker: Read "
>  					"error for WWN %s.  Sense data are "
>  					"0x%x/0x%x/0x%x.", wwnstr,
>  					sbb[2]&0xf, sbb[12], sbb[13]);
> +				c->msgid = MSG_CLARIION_READ_ERROR;
> +			}
>  		} else {
> -			MSG(c, "emc_clariion_checker: Active path is "
> -			    "healthy.");
> +			c->msgid = MSG_CLARIION_PASSIVE_GOOD;
>  			/*
>  			 * Remove the path from the set of paths to inactive
>  			 * snapshot LUs if it was in this list since the
> @@ -225,8 +250,7 @@ int libcheck_check (struct checker * c)
>  				wwnstr);
>  			ret = PATH_DOWN;
>  		} else {
> -			MSG(c,
> -			    "emc_clariion_checker: Passive path is healthy.");
> +			c->msgid = MSG_CLARIION_PASSIVE_GOOD;
>  			ret = PATH_UP;	/* not ghost */
>  		}
>  	}
> -- 
> 2.19.0

[PATCH 07/21] libmultipath/checkers: hp_sw: use message id

[Martin Wilck]

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/checkers/hp_sw.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/libmultipath/checkers/hp_sw.c b/libmultipath/checkers/hp_sw.c
index 0ad34a6b..d7f1018c 100644
--- a/libmultipath/checkers/hp_sw.c
+++ b/libmultipath/checkers/hp_sw.c
@@ -27,10 +27,6 @@
 #define MX_ALLOC_LEN		255
 #define HEAVY_CHECK_COUNT       10
 
-#define MSG_HP_SW_UP	"hp_sw checker reports path is up"
-#define MSG_HP_SW_DOWN	"hp_sw checker reports path is down"
-#define MSG_HP_SW_GHOST	"hp_sw checker reports path is ghost"
-
 struct sw_checker_context {
 	void * dummy;
 };
@@ -128,14 +124,14 @@ int libcheck_check(struct checker * c)
 	char buff[MX_ALLOC_LEN];
 
 	if (0 != do_inq(c->fd, 0, 1, 0x80, buff, MX_ALLOC_LEN, 0, c->timeout)) {
-		MSG(c, MSG_HP_SW_DOWN);
+		c->msgid = CHECKER_MSGID_DOWN;
 		return PATH_DOWN;
-	}
+	};
 
 	if (do_tur(c->fd, c->timeout)) {
-		MSG(c, MSG_HP_SW_GHOST);
+		c->msgid = CHECKER_MSGID_GHOST;
 		return PATH_GHOST;
 	}
-	MSG(c, MSG_HP_SW_UP);
+	c->msgid = CHECKER_MSGID_UP;
 	return PATH_UP;
 }
-- 
2.19.0

Re: [PATCH 07/21] libmultipath/checkers: hp_sw: use message id

[Benjamin Marzinski]

On Fri, Oct 12, 2018 at 12:26:53AM +0200, Martin Wilck wrote:

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

> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/checkers/hp_sw.c | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/libmultipath/checkers/hp_sw.c b/libmultipath/checkers/hp_sw.c
> index 0ad34a6b..d7f1018c 100644
> --- a/libmultipath/checkers/hp_sw.c
> +++ b/libmultipath/checkers/hp_sw.c
> @@ -27,10 +27,6 @@
>  #define MX_ALLOC_LEN		255
>  #define HEAVY_CHECK_COUNT       10
>  
> -#define MSG_HP_SW_UP	"hp_sw checker reports path is up"
> -#define MSG_HP_SW_DOWN	"hp_sw checker reports path is down"
> -#define MSG_HP_SW_GHOST	"hp_sw checker reports path is ghost"
> -
>  struct sw_checker_context {
>  	void * dummy;
>  };
> @@ -128,14 +124,14 @@ int libcheck_check(struct checker * c)
>  	char buff[MX_ALLOC_LEN];
>  
>  	if (0 != do_inq(c->fd, 0, 1, 0x80, buff, MX_ALLOC_LEN, 0, c->timeout)) {
> -		MSG(c, MSG_HP_SW_DOWN);
> +		c->msgid = CHECKER_MSGID_DOWN;
>  		return PATH_DOWN;
> -	}
> +	};
>  
>  	if (do_tur(c->fd, c->timeout)) {
> -		MSG(c, MSG_HP_SW_GHOST);
> +		c->msgid = CHECKER_MSGID_GHOST;
>  		return PATH_GHOST;
>  	}
> -	MSG(c, MSG_HP_SW_UP);
> +	c->msgid = CHECKER_MSGID_UP;
>  	return PATH_UP;
>  }
> -- 
> 2.19.0

[PATCH 08/21] libmultipath/checkers: rdac: use message id

[Martin Wilck]

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

diff --git a/libmultipath/checkers/rdac.c b/libmultipath/checkers/rdac.c
index 5104e4e5..266f8e10 100644
--- a/libmultipath/checkers/rdac.c
+++ b/libmultipath/checkers/rdac.c
@@ -31,9 +31,7 @@
 #define CURRENT_PAGE_CODE_VALUES	0
 #define CHANGEABLE_PAGE_CODE_VALUES	1
 
-#define MSG_RDAC_UP    "rdac checker reports path is up"
-#define MSG_RDAC_DOWN  "rdac checker reports path is down"
-#define MSG_RDAC_GHOST "rdac checker reports path is ghost"
+#define MSG_RDAC_DOWN  " reports path is down"
 #define MSG_RDAC_DOWN_TYPE(STR) MSG_RDAC_DOWN": "STR
 
 #define RTPG_UNAVAILABLE	0x3
@@ -219,41 +217,69 @@ struct volume_access_inq
 	char dontcare1[34];
 };
 
-const char
-*checker_msg_string(struct volume_access_inq *inq)
+enum {
+	RDAC_MSGID_NOT_CONN = CHECKER_FIRST_MSGID,
+	RDAC_MSGID_IN_STARTUP,
+	RDAC_MSGID_NON_RESPONSIVE,
+	RDAC_MSGID_IN_RESET,
+	RDAC_MSGID_FW_DOWNLOADING,
+	RDAC_MSGID_QUIESCED,
+	RDAC_MSGID_SERVICE_MODE,
+	RDAC_MSGID_UNAVAILABLE,
+	RDAC_MSGID_INQUIRY_FAILED,
+};
+
+#define _IDX(x) (RDAC_MSGID_##x - CHECKER_FIRST_MSGID)
+const char *libcheck_msgtable[] = {
+	[_IDX(NOT_CONN)] = MSG_RDAC_DOWN_TYPE("lun not connected"),
+	[_IDX(IN_STARTUP)] = MSG_RDAC_DOWN_TYPE("ctlr is in startup sequence"),
+	[_IDX(NON_RESPONSIVE)] =
+	MSG_RDAC_DOWN_TYPE("non-responsive to queries"),
+	[_IDX(IN_RESET)] = MSG_RDAC_DOWN_TYPE("ctlr held in reset"),
+	[_IDX(FW_DOWNLOADING)] =
+	MSG_RDAC_DOWN_TYPE("ctlr firmware downloading"),
+	[_IDX(QUIESCED)] = MSG_RDAC_DOWN_TYPE("ctlr quiesced by admin request"),
+	[_IDX(SERVICE_MODE)] = MSG_RDAC_DOWN_TYPE("ctlr is in service mode"),
+	[_IDX(UNAVAILABLE)] = MSG_RDAC_DOWN_TYPE("ctlr is unavailable"),
+	[_IDX(INQUIRY_FAILED)] = MSG_RDAC_DOWN_TYPE("inquiry failed"),
+	NULL,
+};
+
+static int
+checker_msg_string(const struct volume_access_inq *inq)
 {
 	/* lun not connected */
 	if (((inq->PQ_PDT & 0xE0) == 0x20) || (inq->PQ_PDT & 0x7f))
-		return MSG_RDAC_DOWN_TYPE("lun not connected");
+		return RDAC_MSGID_NOT_CONN;
 
 	/* if no tpg data is available, give the generic path down message */
 	if (!(inq->avtcvp & 0x10))
-		return MSG_RDAC_DOWN;
+		return CHECKER_MSGID_DOWN;
 
 	/* controller is booting up */
 	if (((inq->aas_cur & 0x0F) == RTPG_TRANSITIONING) &&
 		(inq->aas_alt & 0x0F) != RTPG_TRANSITIONING)
-		return MSG_RDAC_DOWN_TYPE("ctlr is in startup sequence");
+		return RDAC_MSGID_IN_STARTUP;
 
 	/* if not unavailable, give generic message */
 	if ((inq->aas_cur & 0x0F) != RTPG_UNAVAILABLE)
-		return MSG_RDAC_DOWN;
+		return CHECKER_MSGID_DOWN;
 
 	/* target port group unavailable */
 	switch (inq->vendor_specific_cur) {
 	case RTPG_UNAVAIL_NON_RESPONSIVE:
-		return MSG_RDAC_DOWN_TYPE("non-responsive to queries");
+		return RDAC_MSGID_NON_RESPONSIVE;
 	case RTPG_UNAVAIL_IN_RESET:
-		return MSG_RDAC_DOWN_TYPE("ctlr held in reset");
+		return RDAC_MSGID_IN_RESET;
 	case RTPG_UNAVAIL_CFW_DL1:
 	case RTPG_UNAVAIL_CFW_DL2:
-		return MSG_RDAC_DOWN_TYPE("ctlr firmware downloading");
+		return RDAC_MSGID_FW_DOWNLOADING;
 	case RTPG_UNAVAIL_QUIESCED:
-		return MSG_RDAC_DOWN_TYPE("ctlr quiesced by admin request");
+		return RDAC_MSGID_QUIESCED;
 	case RTPG_UNAVAIL_SERVICE_MODE:
-		return MSG_RDAC_DOWN_TYPE("ctlr is in service mode");
+		return RDAC_MSGID_SERVICE_MODE;
 	default:
-		return MSG_RDAC_DOWN_TYPE("ctlr is unavailable");
+		return RDAC_MSGID_UNAVAILABLE;
 	}
 }
 
@@ -307,14 +333,14 @@ int libcheck_check(struct checker * c)
 done:
 	switch (ret) {
 	case PATH_DOWN:
-		MSG(c, "%s", (inqfail) ? MSG_RDAC_DOWN_TYPE("inquiry failed") :
-			checker_msg_string(&inq));
+		c->msgid = (inqfail ? RDAC_MSGID_INQUIRY_FAILED :
+			    checker_msg_string(&inq));
 		break;
 	case PATH_UP:
-		MSG(c, MSG_RDAC_UP);
+		c->msgid = CHECKER_MSGID_UP;
 		break;
 	case PATH_GHOST:
-		MSG(c, MSG_RDAC_GHOST);
+		c->msgid = CHECKER_MSGID_GHOST;
 		break;
 	}
 
-- 
2.19.0

Re: [PATCH 08/21] libmultipath/checkers: rdac: use message id

[Benjamin Marzinski]

On Fri, Oct 12, 2018 at 12:26:54AM +0200, Martin Wilck wrote:

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

> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/checkers/rdac.c | 64 +++++++++++++++++++++++++-----------
>  1 file changed, 45 insertions(+), 19 deletions(-)
> 
> diff --git a/libmultipath/checkers/rdac.c b/libmultipath/checkers/rdac.c
> index 5104e4e5..266f8e10 100644
> --- a/libmultipath/checkers/rdac.c
> +++ b/libmultipath/checkers/rdac.c
> @@ -31,9 +31,7 @@
>  #define CURRENT_PAGE_CODE_VALUES	0
>  #define CHANGEABLE_PAGE_CODE_VALUES	1
>  
> -#define MSG_RDAC_UP    "rdac checker reports path is up"
> -#define MSG_RDAC_DOWN  "rdac checker reports path is down"
> -#define MSG_RDAC_GHOST "rdac checker reports path is ghost"
> +#define MSG_RDAC_DOWN  " reports path is down"
>  #define MSG_RDAC_DOWN_TYPE(STR) MSG_RDAC_DOWN": "STR
>  
>  #define RTPG_UNAVAILABLE	0x3
> @@ -219,41 +217,69 @@ struct volume_access_inq
>  	char dontcare1[34];
>  };
>  
> -const char
> -*checker_msg_string(struct volume_access_inq *inq)
> +enum {
> +	RDAC_MSGID_NOT_CONN = CHECKER_FIRST_MSGID,
> +	RDAC_MSGID_IN_STARTUP,
> +	RDAC_MSGID_NON_RESPONSIVE,
> +	RDAC_MSGID_IN_RESET,
> +	RDAC_MSGID_FW_DOWNLOADING,
> +	RDAC_MSGID_QUIESCED,
> +	RDAC_MSGID_SERVICE_MODE,
> +	RDAC_MSGID_UNAVAILABLE,
> +	RDAC_MSGID_INQUIRY_FAILED,
> +};
> +
> +#define _IDX(x) (RDAC_MSGID_##x - CHECKER_FIRST_MSGID)
> +const char *libcheck_msgtable[] = {
> +	[_IDX(NOT_CONN)] = MSG_RDAC_DOWN_TYPE("lun not connected"),
> +	[_IDX(IN_STARTUP)] = MSG_RDAC_DOWN_TYPE("ctlr is in startup sequence"),
> +	[_IDX(NON_RESPONSIVE)] =
> +	MSG_RDAC_DOWN_TYPE("non-responsive to queries"),
> +	[_IDX(IN_RESET)] = MSG_RDAC_DOWN_TYPE("ctlr held in reset"),
> +	[_IDX(FW_DOWNLOADING)] =
> +	MSG_RDAC_DOWN_TYPE("ctlr firmware downloading"),
> +	[_IDX(QUIESCED)] = MSG_RDAC_DOWN_TYPE("ctlr quiesced by admin request"),
> +	[_IDX(SERVICE_MODE)] = MSG_RDAC_DOWN_TYPE("ctlr is in service mode"),
> +	[_IDX(UNAVAILABLE)] = MSG_RDAC_DOWN_TYPE("ctlr is unavailable"),
> +	[_IDX(INQUIRY_FAILED)] = MSG_RDAC_DOWN_TYPE("inquiry failed"),
> +	NULL,
> +};
> +
> +static int
> +checker_msg_string(const struct volume_access_inq *inq)
>  {
>  	/* lun not connected */
>  	if (((inq->PQ_PDT & 0xE0) == 0x20) || (inq->PQ_PDT & 0x7f))
> -		return MSG_RDAC_DOWN_TYPE("lun not connected");
> +		return RDAC_MSGID_NOT_CONN;
>  
>  	/* if no tpg data is available, give the generic path down message */
>  	if (!(inq->avtcvp & 0x10))
> -		return MSG_RDAC_DOWN;
> +		return CHECKER_MSGID_DOWN;
>  
>  	/* controller is booting up */
>  	if (((inq->aas_cur & 0x0F) == RTPG_TRANSITIONING) &&
>  		(inq->aas_alt & 0x0F) != RTPG_TRANSITIONING)
> -		return MSG_RDAC_DOWN_TYPE("ctlr is in startup sequence");
> +		return RDAC_MSGID_IN_STARTUP;
>  
>  	/* if not unavailable, give generic message */
>  	if ((inq->aas_cur & 0x0F) != RTPG_UNAVAILABLE)
> -		return MSG_RDAC_DOWN;
> +		return CHECKER_MSGID_DOWN;
>  
>  	/* target port group unavailable */
>  	switch (inq->vendor_specific_cur) {
>  	case RTPG_UNAVAIL_NON_RESPONSIVE:
> -		return MSG_RDAC_DOWN_TYPE("non-responsive to queries");
> +		return RDAC_MSGID_NON_RESPONSIVE;
>  	case RTPG_UNAVAIL_IN_RESET:
> -		return MSG_RDAC_DOWN_TYPE("ctlr held in reset");
> +		return RDAC_MSGID_IN_RESET;
>  	case RTPG_UNAVAIL_CFW_DL1:
>  	case RTPG_UNAVAIL_CFW_DL2:
> -		return MSG_RDAC_DOWN_TYPE("ctlr firmware downloading");
> +		return RDAC_MSGID_FW_DOWNLOADING;
>  	case RTPG_UNAVAIL_QUIESCED:
> -		return MSG_RDAC_DOWN_TYPE("ctlr quiesced by admin request");
> +		return RDAC_MSGID_QUIESCED;
>  	case RTPG_UNAVAIL_SERVICE_MODE:
> -		return MSG_RDAC_DOWN_TYPE("ctlr is in service mode");
> +		return RDAC_MSGID_SERVICE_MODE;
>  	default:
> -		return MSG_RDAC_DOWN_TYPE("ctlr is unavailable");
> +		return RDAC_MSGID_UNAVAILABLE;
>  	}
>  }
>  
> @@ -307,14 +333,14 @@ int libcheck_check(struct checker * c)
>  done:
>  	switch (ret) {
>  	case PATH_DOWN:
> -		MSG(c, "%s", (inqfail) ? MSG_RDAC_DOWN_TYPE("inquiry failed") :
> -			checker_msg_string(&inq));
> +		c->msgid = (inqfail ? RDAC_MSGID_INQUIRY_FAILED :
> +			    checker_msg_string(&inq));
>  		break;
>  	case PATH_UP:
> -		MSG(c, MSG_RDAC_UP);
> +		c->msgid = CHECKER_MSGID_UP;
>  		break;
>  	case PATH_GHOST:
> -		MSG(c, MSG_RDAC_GHOST);
> +		c->msgid = CHECKER_MSGID_GHOST;
>  		break;
>  	}
>  
> -- 
> 2.19.0

[PATCH 09/21] libmultipath/checkers: readsector0: use message id

[Martin Wilck]

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/checkers/readsector0.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/libmultipath/checkers/readsector0.c b/libmultipath/checkers/readsector0.c
index 1c2a868e..cf79e067 100644
--- a/libmultipath/checkers/readsector0.c
+++ b/libmultipath/checkers/readsector0.c
@@ -6,9 +6,6 @@
 #include "checkers.h"
 #include "libsg.h"
 
-#define MSG_READSECTOR0_UP	"readsector0 checker reports path is up"
-#define MSG_READSECTOR0_DOWN	"readsector0 checker reports path is down"
-
 struct readsector0_checker_context {
 	void * dummy;
 };
@@ -35,10 +32,10 @@ int libcheck_check (struct checker * c)
 	switch (ret)
 	{
 	case PATH_DOWN:
-		MSG(c, MSG_READSECTOR0_DOWN);
+		c->msgid = CHECKER_MSGID_DOWN;
 		break;
 	case PATH_UP:
-		MSG(c, MSG_READSECTOR0_UP);
+		c->msgid = CHECKER_MSGID_UP;
 		break;
 	default:
 		break;
-- 
2.19.0

Re: [PATCH 09/21] libmultipath/checkers: readsector0: use message id

[Benjamin Marzinski]

On Fri, Oct 12, 2018 at 12:26:55AM +0200, Martin Wilck wrote:

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

> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/checkers/readsector0.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/libmultipath/checkers/readsector0.c b/libmultipath/checkers/readsector0.c
> index 1c2a868e..cf79e067 100644
> --- a/libmultipath/checkers/readsector0.c
> +++ b/libmultipath/checkers/readsector0.c
> @@ -6,9 +6,6 @@
>  #include "checkers.h"
>  #include "libsg.h"
>  
> -#define MSG_READSECTOR0_UP	"readsector0 checker reports path is up"
> -#define MSG_READSECTOR0_DOWN	"readsector0 checker reports path is down"
> -
>  struct readsector0_checker_context {
>  	void * dummy;
>  };
> @@ -35,10 +32,10 @@ int libcheck_check (struct checker * c)
>  	switch (ret)
>  	{
>  	case PATH_DOWN:
> -		MSG(c, MSG_READSECTOR0_DOWN);
> +		c->msgid = CHECKER_MSGID_DOWN;
>  		break;
>  	case PATH_UP:
> -		MSG(c, MSG_READSECTOR0_UP);
> +		c->msgid = CHECKER_MSGID_UP;
>  		break;
>  	default:
>  		break;
> -- 
> 2.19.0

[PATCH 10/21] libmultipath/checkers: tur: use message id

[Martin Wilck]

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/checkers/tur.c | 54 ++++++++++++++++++++-----------------
 1 file changed, 30 insertions(+), 24 deletions(-)

diff --git a/libmultipath/checkers/tur.c b/libmultipath/checkers/tur.c
index 9ecca5bd..448c396e 100644
--- a/libmultipath/checkers/tur.c
+++ b/libmultipath/checkers/tur.c
@@ -29,12 +29,19 @@
 #define TUR_CMD_LEN 6
 #define HEAVY_CHECK_COUNT       10
 
-#define MSG_TUR_UP	"tur checker reports path is up"
-#define MSG_TUR_DOWN	"tur checker reports path is down"
-#define MSG_TUR_GHOST	"tur checker reports path is in standby state"
-#define MSG_TUR_RUNNING	"tur checker still running"
-#define MSG_TUR_TIMEOUT	"tur checker timed out"
-#define MSG_TUR_FAILED	"tur checker failed to initialize"
+enum {
+	MSG_TUR_RUNNING = CHECKER_FIRST_MSGID,
+	MSG_TUR_TIMEOUT,
+	MSG_TUR_FAILED,
+};
+
+#define _IDX(x) (MSG_ ## x - CHECKER_FIRST_MSGID)
+const char *libcheck_msgtable[] = {
+	[_IDX(TUR_RUNNING)] = " still running",
+	[_IDX(TUR_TIMEOUT)] = " timed out",
+	[_IDX(TUR_FAILED)] = " failed to initialize",
+	NULL,
+};
 
 struct tur_checker_context {
 	dev_t devt;
@@ -47,7 +54,7 @@ struct tur_checker_context {
 	pthread_mutex_t lock;
 	pthread_cond_t active;
 	int holders; /* uatomic access only */
-	char message[CHECKER_MSG_LEN];
+	int msgid;
 };
 
 int libcheck_init (struct checker * c)
@@ -99,7 +106,7 @@ void libcheck_free (struct checker * c)
 }
 
 static int
-tur_check(int fd, unsigned int timeout, char *msg)
+tur_check(int fd, unsigned int timeout, short *msgid)
 {
 	struct sg_io_hdr io_hdr;
 	unsigned char turCmdBlk[TUR_CMD_LEN] = { 0x00, 0, 0, 0, 0, 0 };
@@ -118,7 +125,7 @@ retry:
 	io_hdr.timeout = timeout * 1000;
 	io_hdr.pack_id = 0;
 	if (ioctl(fd, SG_IO, &io_hdr) < 0) {
-		snprintf(msg, CHECKER_MSG_LEN, MSG_TUR_DOWN);
+		*msgid = CHECKER_MSGID_DOWN;
 		return PATH_DOWN;
 	}
 	if ((io_hdr.status & 0x7e) == 0x18) {
@@ -126,7 +133,7 @@ retry:
 		 * SCSI-3 arrays might return
 		 * reservation conflict on TUR
 		 */
-		snprintf(msg, CHECKER_MSG_LEN, MSG_TUR_UP);
+		*msgid = CHECKER_MSGID_UP;
 		return PATH_UP;
 	}
 	if (io_hdr.info & SG_INFO_OK_MASK) {
@@ -171,14 +178,14 @@ retry:
 				 * LOGICAL UNIT NOT ACCESSIBLE,
 				 * TARGET PORT IN STANDBY STATE
 				 */
-				snprintf(msg, CHECKER_MSG_LEN, MSG_TUR_GHOST);
+				*msgid = CHECKER_MSGID_GHOST;
 				return PATH_GHOST;
 			}
 		}
-		snprintf(msg, CHECKER_MSG_LEN, MSG_TUR_DOWN);
+		*msgid = CHECKER_MSGID_DOWN;
 		return PATH_DOWN;
 	}
-	snprintf(msg, CHECKER_MSG_LEN, MSG_TUR_UP);
+	*msgid = CHECKER_MSGID_UP;
 	return PATH_UP;
 }
 
@@ -244,7 +251,7 @@ static void *tur_thread(void *ctx)
 {
 	struct tur_checker_context *ct = ctx;
 	int state, running;
-	char msg[CHECKER_MSG_LEN];
+	short msgid;
 
 	/* This thread can be canceled, so setup clean up */
 	tur_thread_cleanup_push(ct);
@@ -254,13 +261,13 @@ static void *tur_thread(void *ctx)
 		minor(ct->devt));
 
 	tur_deep_sleep(ct);
-	state = tur_check(ct->fd, ct->timeout, msg);
+	state = tur_check(ct->fd, ct->timeout, &msgid);
 	pthread_testcancel();
 
 	/* TUR checker done */
 	pthread_mutex_lock(&ct->lock);
 	ct->state = state;
-	strlcpy(ct->message, msg, sizeof(ct->message));
+	ct->msgid = msgid;
 	pthread_cond_signal(&ct->active);
 	pthread_mutex_unlock(&ct->lock);
 
@@ -313,7 +320,7 @@ int libcheck_check(struct checker * c)
 		return PATH_UNCHECKED;
 
 	if (c->sync)
-		return tur_check(c->fd, c->timeout, c->message);
+		return tur_check(c->fd, c->timeout, &c->msgid);
 
 	/*
 	 * Async mode
@@ -325,13 +332,12 @@ int libcheck_check(struct checker * c)
 				pthread_cancel(ct->thread);
 				condlog(3, "%d:%d : tur checker timeout",
 					major(ct->devt), minor(ct->devt));
-				MSG(c, MSG_TUR_TIMEOUT);
+				c->msgid = MSG_TUR_TIMEOUT;
 				tur_status = PATH_TIMEOUT;
 			} else {
 				pthread_mutex_lock(&ct->lock);
 				tur_status = ct->state;
-				strlcpy(c->message, ct->message,
-					sizeof(c->message));
+				c->msgid = ct->msgid;
 				pthread_mutex_unlock(&ct->lock);
 			}
 			ct->thread = 0;
@@ -344,7 +350,7 @@ int libcheck_check(struct checker * c)
 			ct->thread = 0;
 			pthread_mutex_lock(&ct->lock);
 			tur_status = ct->state;
-			strlcpy(c->message, ct->message, sizeof(c->message));
+			c->msgid = ct->msgid;
 			pthread_mutex_unlock(&ct->lock);
 		}
 	} else {
@@ -376,7 +382,7 @@ int libcheck_check(struct checker * c)
 		/* Start new TUR checker */
 		pthread_mutex_lock(&ct->lock);
 		tur_status = ct->state = PATH_PENDING;
-		ct->message[0] = '\0';
+		ct->msgid = CHECKER_MSGID_NONE;
 		pthread_mutex_unlock(&ct->lock);
 		ct->fd = c->fd;
 		ct->timeout = c->timeout;
@@ -392,7 +398,7 @@ int libcheck_check(struct checker * c)
 			ct->thread = 0;
 			condlog(3, "%d:%d : failed to start tur thread, using"
 				" sync mode", major(ct->devt), minor(ct->devt));
-			return tur_check(c->fd, c->timeout, c->message);
+			return tur_check(c->fd, c->timeout, &c->msgid);
 		}
 		tur_timeout(&tsp);
 		pthread_mutex_lock(&ct->lock);
@@ -401,7 +407,7 @@ int libcheck_check(struct checker * c)
 						   &tsp);
 		if (!r) {
 			tur_status = ct->state;
-			strlcpy(c->message, ct->message, sizeof(c->message));
+			c->msgid = ct->msgid;
 		}
 		pthread_mutex_unlock(&ct->lock);
 		if (tur_status == PATH_PENDING) {
-- 
2.19.0

Re: [PATCH 10/21] libmultipath/checkers: tur: use message id

[Benjamin Marzinski]

On Fri, Oct 12, 2018 at 12:26:56AM +0200, Martin Wilck wrote:

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

> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/checkers/tur.c | 54 ++++++++++++++++++++-----------------
>  1 file changed, 30 insertions(+), 24 deletions(-)
> 
> diff --git a/libmultipath/checkers/tur.c b/libmultipath/checkers/tur.c
> index 9ecca5bd..448c396e 100644
> --- a/libmultipath/checkers/tur.c
> +++ b/libmultipath/checkers/tur.c
> @@ -29,12 +29,19 @@
>  #define TUR_CMD_LEN 6
>  #define HEAVY_CHECK_COUNT       10
>  
> -#define MSG_TUR_UP	"tur checker reports path is up"
> -#define MSG_TUR_DOWN	"tur checker reports path is down"
> -#define MSG_TUR_GHOST	"tur checker reports path is in standby state"
> -#define MSG_TUR_RUNNING	"tur checker still running"
> -#define MSG_TUR_TIMEOUT	"tur checker timed out"
> -#define MSG_TUR_FAILED	"tur checker failed to initialize"
> +enum {
> +	MSG_TUR_RUNNING = CHECKER_FIRST_MSGID,
> +	MSG_TUR_TIMEOUT,
> +	MSG_TUR_FAILED,
> +};
> +
> +#define _IDX(x) (MSG_ ## x - CHECKER_FIRST_MSGID)
> +const char *libcheck_msgtable[] = {
> +	[_IDX(TUR_RUNNING)] = " still running",
> +	[_IDX(TUR_TIMEOUT)] = " timed out",
> +	[_IDX(TUR_FAILED)] = " failed to initialize",
> +	NULL,
> +};
>  
>  struct tur_checker_context {
>  	dev_t devt;
> @@ -47,7 +54,7 @@ struct tur_checker_context {
>  	pthread_mutex_t lock;
>  	pthread_cond_t active;
>  	int holders; /* uatomic access only */
> -	char message[CHECKER_MSG_LEN];
> +	int msgid;
>  };
>  
>  int libcheck_init (struct checker * c)
> @@ -99,7 +106,7 @@ void libcheck_free (struct checker * c)
>  }
>  
>  static int
> -tur_check(int fd, unsigned int timeout, char *msg)
> +tur_check(int fd, unsigned int timeout, short *msgid)
>  {
>  	struct sg_io_hdr io_hdr;
>  	unsigned char turCmdBlk[TUR_CMD_LEN] = { 0x00, 0, 0, 0, 0, 0 };
> @@ -118,7 +125,7 @@ retry:
>  	io_hdr.timeout = timeout * 1000;
>  	io_hdr.pack_id = 0;
>  	if (ioctl(fd, SG_IO, &io_hdr) < 0) {
> -		snprintf(msg, CHECKER_MSG_LEN, MSG_TUR_DOWN);
> +		*msgid = CHECKER_MSGID_DOWN;
>  		return PATH_DOWN;
>  	}
>  	if ((io_hdr.status & 0x7e) == 0x18) {
> @@ -126,7 +133,7 @@ retry:
>  		 * SCSI-3 arrays might return
>  		 * reservation conflict on TUR
>  		 */
> -		snprintf(msg, CHECKER_MSG_LEN, MSG_TUR_UP);
> +		*msgid = CHECKER_MSGID_UP;
>  		return PATH_UP;
>  	}
>  	if (io_hdr.info & SG_INFO_OK_MASK) {
> @@ -171,14 +178,14 @@ retry:
>  				 * LOGICAL UNIT NOT ACCESSIBLE,
>  				 * TARGET PORT IN STANDBY STATE
>  				 */
> -				snprintf(msg, CHECKER_MSG_LEN, MSG_TUR_GHOST);
> +				*msgid = CHECKER_MSGID_GHOST;
>  				return PATH_GHOST;
>  			}
>  		}
> -		snprintf(msg, CHECKER_MSG_LEN, MSG_TUR_DOWN);
> +		*msgid = CHECKER_MSGID_DOWN;
>  		return PATH_DOWN;
>  	}
> -	snprintf(msg, CHECKER_MSG_LEN, MSG_TUR_UP);
> +	*msgid = CHECKER_MSGID_UP;
>  	return PATH_UP;
>  }
>  
> @@ -244,7 +251,7 @@ static void *tur_thread(void *ctx)
>  {
>  	struct tur_checker_context *ct = ctx;
>  	int state, running;
> -	char msg[CHECKER_MSG_LEN];
> +	short msgid;
>  
>  	/* This thread can be canceled, so setup clean up */
>  	tur_thread_cleanup_push(ct);
> @@ -254,13 +261,13 @@ static void *tur_thread(void *ctx)
>  		minor(ct->devt));
>  
>  	tur_deep_sleep(ct);
> -	state = tur_check(ct->fd, ct->timeout, msg);
> +	state = tur_check(ct->fd, ct->timeout, &msgid);
>  	pthread_testcancel();
>  
>  	/* TUR checker done */
>  	pthread_mutex_lock(&ct->lock);
>  	ct->state = state;
> -	strlcpy(ct->message, msg, sizeof(ct->message));
> +	ct->msgid = msgid;
>  	pthread_cond_signal(&ct->active);
>  	pthread_mutex_unlock(&ct->lock);
>  
> @@ -313,7 +320,7 @@ int libcheck_check(struct checker * c)
>  		return PATH_UNCHECKED;
>  
>  	if (c->sync)
> -		return tur_check(c->fd, c->timeout, c->message);
> +		return tur_check(c->fd, c->timeout, &c->msgid);
>  
>  	/*
>  	 * Async mode
> @@ -325,13 +332,12 @@ int libcheck_check(struct checker * c)
>  				pthread_cancel(ct->thread);
>  				condlog(3, "%d:%d : tur checker timeout",
>  					major(ct->devt), minor(ct->devt));
> -				MSG(c, MSG_TUR_TIMEOUT);
> +				c->msgid = MSG_TUR_TIMEOUT;
>  				tur_status = PATH_TIMEOUT;
>  			} else {
>  				pthread_mutex_lock(&ct->lock);
>  				tur_status = ct->state;
> -				strlcpy(c->message, ct->message,
> -					sizeof(c->message));
> +				c->msgid = ct->msgid;
>  				pthread_mutex_unlock(&ct->lock);
>  			}
>  			ct->thread = 0;
> @@ -344,7 +350,7 @@ int libcheck_check(struct checker * c)
>  			ct->thread = 0;
>  			pthread_mutex_lock(&ct->lock);
>  			tur_status = ct->state;
> -			strlcpy(c->message, ct->message, sizeof(c->message));
> +			c->msgid = ct->msgid;
>  			pthread_mutex_unlock(&ct->lock);
>  		}
>  	} else {
> @@ -376,7 +382,7 @@ int libcheck_check(struct checker * c)
>  		/* Start new TUR checker */
>  		pthread_mutex_lock(&ct->lock);
>  		tur_status = ct->state = PATH_PENDING;
> -		ct->message[0] = '\0';
> +		ct->msgid = CHECKER_MSGID_NONE;
>  		pthread_mutex_unlock(&ct->lock);
>  		ct->fd = c->fd;
>  		ct->timeout = c->timeout;
> @@ -392,7 +398,7 @@ int libcheck_check(struct checker * c)
>  			ct->thread = 0;
>  			condlog(3, "%d:%d : failed to start tur thread, using"
>  				" sync mode", major(ct->devt), minor(ct->devt));
> -			return tur_check(c->fd, c->timeout, c->message);
> +			return tur_check(c->fd, c->timeout, &c->msgid);
>  		}
>  		tur_timeout(&tsp);
>  		pthread_mutex_lock(&ct->lock);
> @@ -401,7 +407,7 @@ int libcheck_check(struct checker * c)
>  						   &tsp);
>  		if (!r) {
>  			tur_status = ct->state;
> -			strlcpy(c->message, ct->message, sizeof(c->message));
> +			c->msgid = ct->msgid;
>  		}
>  		pthread_mutex_unlock(&ct->lock);
>  		if (tur_status == PATH_PENDING) {
> -- 
> 2.19.0

[PATCH 11/21] multipathd: improve checker message logging

[Martin Wilck]

Don't rely on any variables being defined in LOG_MSG.
If message log level is low, don't bother to fetch the message.

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

diff --git a/multipathd/main.c b/multipathd/main.c
index a9e1a4bd..1bd1518a 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -89,12 +89,22 @@ static int use_watchdog;
 #define FILE_NAME_SIZE 256
 #define CMDSIZE 160
 
-#define LOG_MSG(a, b) \
-do { \
-	if (pp->offline) \
-		condlog(a, "%s: %s - path offline", pp->mpp->alias, pp->dev); \
-	else if (strlen(b)) \
-		condlog(a, "%s: %s - %s", pp->mpp->alias, pp->dev, b); \
+#define LOG_MSG(lvl, verb, pp)					\
+do {								\
+	if (lvl <= verb) {					\
+		if (pp->offline)				\
+			condlog(lvl, "%s: %s - path offline",	\
+				pp->mpp->alias, pp->dev);	\
+		else {						\
+			const char *__m =			\
+				checker_message(&pp->checker);	\
+								\
+			if (strlen(__m))			\
+				condlog(lvl, "%s: %s - %s",	\
+					pp->mpp->alias,		\
+					pp->dev, __m);		\
+		}						\
+	}							\
 } while(0)
 
 struct mpath_event_param
@@ -1811,7 +1821,7 @@ check_path (struct vectors * vecs, struct path * pp, int ticks)
 	int add_active;
 	int disable_reinstate = 0;
 	int oldchkrstate = pp->chkrstate;
-	int retrigger_tries, checkint;
+	int retrigger_tries, checkint, verbosity;
 	struct config *conf;
 	int ret;
 
@@ -1827,6 +1837,7 @@ check_path (struct vectors * vecs, struct path * pp, int ticks)
 	conf = get_multipath_config();
 	retrigger_tries = conf->retrigger_tries;
 	checkint = conf->checkint;
+	verbosity = conf->verbosity;
 	put_multipath_config(conf);
 	if (!pp->mpp && pp->initialized == INIT_MISSING_UDEV) {
 		if (pp->retriggers < retrigger_tries) {
@@ -1961,7 +1972,7 @@ check_path (struct vectors * vecs, struct path * pp, int ticks)
 		int oldstate = pp->state;
 		pp->state = newstate;
 
-		LOG_MSG(1, checker_message(&pp->checker));
+		LOG_MSG(1, verbosity, pp);
 
 		/*
 		 * upon state change, reset the checkint
@@ -2050,7 +2061,8 @@ check_path (struct vectors * vecs, struct path * pp, int ticks)
 			}
 		} else {
 			unsigned int max_checkint;
-			LOG_MSG(4, checker_message(&pp->checker));
+
+			LOG_MSG(4, verbosity, pp);
 			conf = get_multipath_config();
 			max_checkint = conf->max_checkint;
 			put_multipath_config(conf);
@@ -2083,9 +2095,9 @@ check_path (struct vectors * vecs, struct path * pp, int ticks)
 			log_checker_err = conf->log_checker_err;
 			put_multipath_config(conf);
 			if (log_checker_err == LOG_CHKR_ERR_ONCE)
-				LOG_MSG(3, checker_message(&pp->checker));
+				LOG_MSG(3, verbosity, pp);
 			else
-				LOG_MSG(2, checker_message(&pp->checker));
+				LOG_MSG(2, verbosity, pp);
 		}
 	}
 
-- 
2.19.0

Re: [PATCH 11/21] multipathd: improve checker message logging

[Benjamin Marzinski]

On Fri, Oct 12, 2018 at 12:26:57AM +0200, Martin Wilck wrote:
> Don't rely on any variables being defined in LOG_MSG.
> If message log level is low, don't bother to fetch the message.
> 

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

> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  multipathd/main.c | 34 +++++++++++++++++++++++-----------
>  1 file changed, 23 insertions(+), 11 deletions(-)
> 
> diff --git a/multipathd/main.c b/multipathd/main.c
> index a9e1a4bd..1bd1518a 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -89,12 +89,22 @@ static int use_watchdog;
>  #define FILE_NAME_SIZE 256
>  #define CMDSIZE 160
>  
> -#define LOG_MSG(a, b) \
> -do { \
> -	if (pp->offline) \
> -		condlog(a, "%s: %s - path offline", pp->mpp->alias, pp->dev); \
> -	else if (strlen(b)) \
> -		condlog(a, "%s: %s - %s", pp->mpp->alias, pp->dev, b); \
> +#define LOG_MSG(lvl, verb, pp)					\
> +do {								\
> +	if (lvl <= verb) {					\
> +		if (pp->offline)				\
> +			condlog(lvl, "%s: %s - path offline",	\
> +				pp->mpp->alias, pp->dev);	\
> +		else {						\
> +			const char *__m =			\
> +				checker_message(&pp->checker);	\
> +								\
> +			if (strlen(__m))			\
> +				condlog(lvl, "%s: %s - %s",	\
> +					pp->mpp->alias,		\
> +					pp->dev, __m);		\
> +		}						\
> +	}							\
>  } while(0)
>  
>  struct mpath_event_param
> @@ -1811,7 +1821,7 @@ check_path (struct vectors * vecs, struct path * pp, int ticks)
>  	int add_active;
>  	int disable_reinstate = 0;
>  	int oldchkrstate = pp->chkrstate;
> -	int retrigger_tries, checkint;
> +	int retrigger_tries, checkint, verbosity;
>  	struct config *conf;
>  	int ret;
>  
> @@ -1827,6 +1837,7 @@ check_path (struct vectors * vecs, struct path * pp, int ticks)
>  	conf = get_multipath_config();
>  	retrigger_tries = conf->retrigger_tries;
>  	checkint = conf->checkint;
> +	verbosity = conf->verbosity;
>  	put_multipath_config(conf);
>  	if (!pp->mpp && pp->initialized == INIT_MISSING_UDEV) {
>  		if (pp->retriggers < retrigger_tries) {
> @@ -1961,7 +1972,7 @@ check_path (struct vectors * vecs, struct path * pp, int ticks)
>  		int oldstate = pp->state;
>  		pp->state = newstate;
>  
> -		LOG_MSG(1, checker_message(&pp->checker));
> +		LOG_MSG(1, verbosity, pp);
>  
>  		/*
>  		 * upon state change, reset the checkint
> @@ -2050,7 +2061,8 @@ check_path (struct vectors * vecs, struct path * pp, int ticks)
>  			}
>  		} else {
>  			unsigned int max_checkint;
> -			LOG_MSG(4, checker_message(&pp->checker));
> +
> +			LOG_MSG(4, verbosity, pp);
>  			conf = get_multipath_config();
>  			max_checkint = conf->max_checkint;
>  			put_multipath_config(conf);
> @@ -2083,9 +2095,9 @@ check_path (struct vectors * vecs, struct path * pp, int ticks)
>  			log_checker_err = conf->log_checker_err;
>  			put_multipath_config(conf);
>  			if (log_checker_err == LOG_CHKR_ERR_ONCE)
> -				LOG_MSG(3, checker_message(&pp->checker));
> +				LOG_MSG(3, verbosity, pp);
>  			else
> -				LOG_MSG(2, checker_message(&pp->checker));
> +				LOG_MSG(2, verbosity, pp);
>  		}
>  	}
>  
> -- 
> 2.19.0

[PATCH 12/21] libmultipath/checkers: support unsupported paths

[Martin Wilck]

We should be able to distinguish the case where a checker
determines a path to be positively down from the case where
the checker fails to obtain necessary information, e.g.
because of a configuration problem (wrong checker).
Use PATH_WILD for the latter case, as it's hardly used now.

Provide a generic message for the situation that a path
checker can't handle a certain path.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/checkers.c | 1 +
 libmultipath/checkers.h | 7 +++++--
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/libmultipath/checkers.c b/libmultipath/checkers.c
index 359791ff..19c8ad58 100644
--- a/libmultipath/checkers.c
+++ b/libmultipath/checkers.c
@@ -279,6 +279,7 @@ static const char *generic_msg[CHECKER_LAST_GENERIC_MSGID] = {
 	[CHECKER_MSGID_UP] = " reports path is up",
 	[CHECKER_MSGID_DOWN] = " reports path is down",
 	[CHECKER_MSGID_GHOST] = " reports path is ghost",
+	[CHECKER_MSGID_UNSUPPORTED] = " doesn't support this device",
 };
 
 static const char *_checker_message(const struct checker *c)
diff --git a/libmultipath/checkers.h b/libmultipath/checkers.h
index b48f4dc2..98fec2c4 100644
--- a/libmultipath/checkers.h
+++ b/libmultipath/checkers.h
@@ -10,8 +10,10 @@
  * Userspace (multipath/multipathd) path states
  *
  * PATH_WILD:
- * - Use: None of the checkers (returned if we don't have an fd)
- * - Description: Corner case where "fd < 0" for path fd (see checker_check())
+ * - Use: Any checker
+ * - Description: Corner case where "fd < 0" for path fd (see checker_check()),
+ *   or where a checker detects an unsupported device
+ *   (e.g. wrong checker configured for a given device).
  *
  * PATH_UNCHECKED:
  * - Use: Only in directio checker
@@ -108,6 +110,7 @@ enum {
 	CHECKER_MSGID_UP,
 	CHECKER_MSGID_DOWN,
 	CHECKER_MSGID_GHOST,
+	CHECKER_MSGID_UNSUPPORTED,
 	CHECKER_LAST_GENERIC_MSGID,
 	CHECKER_FIRST_MSGID = 100,	/* lowest msgid for checkers */
 	CHECKER_MSGTABLE_SIZE = 100,	/* max msg table size for checkers */
-- 
2.19.0

Re: [PATCH 12/21] libmultipath/checkers: support unsupported paths

[Benjamin Marzinski]

On Fri, Oct 12, 2018 at 12:26:58AM +0200, Martin Wilck wrote:
> We should be able to distinguish the case where a checker
> determines a path to be positively down from the case where
> the checker fails to obtain necessary information, e.g.
> because of a configuration problem (wrong checker).
> Use PATH_WILD for the latter case, as it's hardly used now.
> 
> Provide a generic message for the situation that a path
> checker can't handle a certain path.
> 

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

> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/checkers.c | 1 +
>  libmultipath/checkers.h | 7 +++++--
>  2 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/libmultipath/checkers.c b/libmultipath/checkers.c
> index 359791ff..19c8ad58 100644
> --- a/libmultipath/checkers.c
> +++ b/libmultipath/checkers.c
> @@ -279,6 +279,7 @@ static const char *generic_msg[CHECKER_LAST_GENERIC_MSGID] = {
>  	[CHECKER_MSGID_UP] = " reports path is up",
>  	[CHECKER_MSGID_DOWN] = " reports path is down",
>  	[CHECKER_MSGID_GHOST] = " reports path is ghost",
> +	[CHECKER_MSGID_UNSUPPORTED] = " doesn't support this device",
>  };
>  
>  static const char *_checker_message(const struct checker *c)
> diff --git a/libmultipath/checkers.h b/libmultipath/checkers.h
> index b48f4dc2..98fec2c4 100644
> --- a/libmultipath/checkers.h
> +++ b/libmultipath/checkers.h
> @@ -10,8 +10,10 @@
>   * Userspace (multipath/multipathd) path states
>   *
>   * PATH_WILD:
> - * - Use: None of the checkers (returned if we don't have an fd)
> - * - Description: Corner case where "fd < 0" for path fd (see checker_check())
> + * - Use: Any checker
> + * - Description: Corner case where "fd < 0" for path fd (see checker_check()),
> + *   or where a checker detects an unsupported device
> + *   (e.g. wrong checker configured for a given device).
>   *
>   * PATH_UNCHECKED:
>   * - Use: Only in directio checker
> @@ -108,6 +110,7 @@ enum {
>  	CHECKER_MSGID_UP,
>  	CHECKER_MSGID_DOWN,
>  	CHECKER_MSGID_GHOST,
> +	CHECKER_MSGID_UNSUPPORTED,
>  	CHECKER_LAST_GENERIC_MSGID,
>  	CHECKER_FIRST_MSGID = 100,	/* lowest msgid for checkers */
>  	CHECKER_MSGTABLE_SIZE = 100,	/* max msg table size for checkers */
> -- 
> 2.19.0

[PATCH 13/21] libmultipath: clariion checker: leave unsupported paths alone

[Martin Wilck]

A checker shouldn't set the path state to PATH_DOWN if it fails
to obtain information about the path in the first place. Add logic
to the checker to distinguish a failed path from an unsupported path.

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

diff --git a/libmultipath/checkers/emc_clariion.c b/libmultipath/checkers/emc_clariion.c
index f8e55b93..a277a004 100644
--- a/libmultipath/checkers/emc_clariion.c
+++ b/libmultipath/checkers/emc_clariion.c
@@ -20,6 +20,11 @@
 #define INQUIRY_CMD     0x12
 #define INQUIRY_CMDLEN  6
 #define HEAVY_CHECK_COUNT       10
+#define SCSI_COMMAND_TERMINATED	0x22
+#define SCSI_CHECK_CONDITION	0x2
+#define RECOVERED_ERROR		0x01
+#define ILLEGAL_REQUEST		0x05
+#define SG_ERR_DRIVER_SENSE	0x08
 
 /*
  * Mechanism to track CLARiiON inactive snapshot LUs.
@@ -130,7 +135,9 @@ int libcheck_check (struct checker * c)
 		(struct emc_clariion_checker_path_context *)c->context;
 	char wwnstr[33];
 	int ret;
+	int retry_emc = 5;
 
+retry:
 	memset(&io_hdr, 0, sizeof (struct sg_io_hdr));
 	memset(sense_buffer, 0, 128);
 	memset(sb, 0, SENSE_BUFF_LEN);
@@ -145,12 +152,51 @@ int libcheck_check (struct checker * c)
 	io_hdr.timeout = c->timeout * 1000;
 	io_hdr.pack_id = 0;
 	if (ioctl(c->fd, SG_IO, &io_hdr) < 0) {
+		if (errno == ENOTTY) {
+			c->msgid = CHECKER_MSGID_UNSUPPORTED;
+			return PATH_WILD;
+		}
 		c->msgid = MSG_CLARIION_QUERY_FAILED;
 		return PATH_DOWN;
 	}
+
 	if (io_hdr.info & SG_INFO_OK_MASK) {
-		c->msgid = MSG_CLARIION_QUERY_ERROR;
-		return PATH_DOWN;
+		switch (io_hdr.host_status) {
+		case DID_BUS_BUSY:
+		case DID_ERROR:
+		case DID_SOFT_ERROR:
+		case DID_TRANSPORT_DISRUPTED:
+			/* Transport error, retry */
+			if (--retry_emc)
+				goto retry;
+			break;
+		default:
+			break;
+		}
+	}
+
+	if (SCSI_CHECK_CONDITION == io_hdr.status ||
+	    SCSI_COMMAND_TERMINATED == io_hdr.status ||
+	    SG_ERR_DRIVER_SENSE == (0xf & io_hdr.driver_status)) {
+		if (io_hdr.sbp && (io_hdr.sb_len_wr > 2)) {
+			unsigned char *sbp = io_hdr.sbp;
+			int sense_key;
+
+			if (sbp[0] & 0x2)
+				sense_key = sbp[1] & 0xf;
+			else
+				sense_key = sbp[2] & 0xf;
+
+			if (sense_key == ILLEGAL_REQUEST) {
+				c->msgid = CHECKER_MSGID_UNSUPPORTED;
+				return PATH_WILD;
+			} else if (sense_key != RECOVERED_ERROR) {
+				condlog(1, "emc_clariion_checker: INQUIRY failed with sense key %02x",
+					sense_key);
+				c->msgid = MSG_CLARIION_QUERY_ERROR;
+				return PATH_DOWN;
+			}
+		}
 	}
 	if (/* Verify the code page - right page & revision */
 	    sense_buffer[1] != 0xc0 || sense_buffer[9] != 0x00) {
-- 
2.19.0

Re: [PATCH 13/21] libmultipath: clariion checker: leave unsupported paths alone

[Benjamin Marzinski]

On Fri, Oct 12, 2018 at 12:26:59AM +0200, Martin Wilck wrote:
> A checker shouldn't set the path state to PATH_DOWN if it fails
> to obtain information about the path in the first place. Add logic
> to the checker to distinguish a failed path from an unsupported path.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/checkers/emc_clariion.c | 50 ++++++++++++++++++++++++++--
>  1 file changed, 48 insertions(+), 2 deletions(-)
> 
> diff --git a/libmultipath/checkers/emc_clariion.c b/libmultipath/checkers/emc_clariion.c
> index f8e55b93..a277a004 100644
> --- a/libmultipath/checkers/emc_clariion.c
> +++ b/libmultipath/checkers/emc_clariion.c
> @@ -20,6 +20,11 @@
>  #define INQUIRY_CMD     0x12
>  #define INQUIRY_CMDLEN  6
>  #define HEAVY_CHECK_COUNT       10
> +#define SCSI_COMMAND_TERMINATED	0x22
> +#define SCSI_CHECK_CONDITION	0x2
> +#define RECOVERED_ERROR		0x01
> +#define ILLEGAL_REQUEST		0x05
> +#define SG_ERR_DRIVER_SENSE	0x08
>  
>  /*
>   * Mechanism to track CLARiiON inactive snapshot LUs.
> @@ -130,7 +135,9 @@ int libcheck_check (struct checker * c)
>  		(struct emc_clariion_checker_path_context *)c->context;
>  	char wwnstr[33];
>  	int ret;
> +	int retry_emc = 5;
>  
> +retry:
>  	memset(&io_hdr, 0, sizeof (struct sg_io_hdr));
>  	memset(sense_buffer, 0, 128);
>  	memset(sb, 0, SENSE_BUFF_LEN);
> @@ -145,12 +152,51 @@ int libcheck_check (struct checker * c)
>  	io_hdr.timeout = c->timeout * 1000;
>  	io_hdr.pack_id = 0;
>  	if (ioctl(c->fd, SG_IO, &io_hdr) < 0) {
> +		if (errno == ENOTTY) {
> +			c->msgid = CHECKER_MSGID_UNSUPPORTED;
> +			return PATH_WILD;
> +		}
>  		c->msgid = MSG_CLARIION_QUERY_FAILED;
>  		return PATH_DOWN;
>  	}
> +
>  	if (io_hdr.info & SG_INFO_OK_MASK) {
> -		c->msgid = MSG_CLARIION_QUERY_ERROR;
> -		return PATH_DOWN;

Previously, if (io_hdr.info & SG_INFO_OK_MASK) the checker would always
return PATH_DOWN.  Both the tur and rdac checker always return either
PATH_DOWN or PATH_WILD if (io_hdr.info & SG_INFO_OK_MASK && sense_key
!= RECOVERED_ERROR). With the clariion checker, it now appears possible
to have (io_hdr.info & SG_INFO_OK_MASK && sense_key != RECOVERED_ERROR)
and still return PATH_UP. Is this intentional? Am I missing something
here?

> +		switch (io_hdr.host_status) {
> +		case DID_BUS_BUSY:
> +		case DID_ERROR:
> +		case DID_SOFT_ERROR:
> +		case DID_TRANSPORT_DISRUPTED:
> +			/* Transport error, retry */
> +			if (--retry_emc)
> +				goto retry;
> +			break;
> +		default:
> +			break;
> +		}
> +	}
> +
> +	if (SCSI_CHECK_CONDITION == io_hdr.status ||
> +	    SCSI_COMMAND_TERMINATED == io_hdr.status ||
> +	    SG_ERR_DRIVER_SENSE == (0xf & io_hdr.driver_status)) {
> +		if (io_hdr.sbp && (io_hdr.sb_len_wr > 2)) {
> +			unsigned char *sbp = io_hdr.sbp;
> +			int sense_key;
> +
> +			if (sbp[0] & 0x2)
> +				sense_key = sbp[1] & 0xf;
> +			else
> +				sense_key = sbp[2] & 0xf;
> +
> +			if (sense_key == ILLEGAL_REQUEST) {
> +				c->msgid = CHECKER_MSGID_UNSUPPORTED;
> +				return PATH_WILD;
> +			} else if (sense_key != RECOVERED_ERROR) {
> +				condlog(1, "emc_clariion_checker: INQUIRY failed with sense key %02x",
> +					sense_key);
> +				c->msgid = MSG_CLARIION_QUERY_ERROR;
> +				return PATH_DOWN;
> +			}
> +		}
>  	}
>  	if (/* Verify the code page - right page & revision */
>  	    sense_buffer[1] != 0xc0 || sense_buffer[9] != 0x00) {
> -- 
> 2.19.0

Re: [PATCH 13/21] libmultipath: clariion checker: leave unsupported paths alone

[Martin Wilck]

On Thu, 2018-10-25 at 16:41 -0500,  Benjamin Marzinski  wrote:
> On Fri, Oct 12, 2018 at 12:26:59AM +0200, Martin Wilck wrote:
> > A checker shouldn't set the path state to PATH_DOWN if it fails
> > to obtain information about the path in the first place. Add logic
> > to the checker to distinguish a failed path from an unsupported
> > path.
> > 
> > Signed-off-by: Martin Wilck <mwilck@suse.com>
> > ---
> >  libmultipath/checkers/emc_clariion.c | 50
> > ++++++++++++++++++++++++++--
> >  1 file changed, 48 insertions(+), 2 deletions(-)
> > 
> > diff --git a/libmultipath/checkers/emc_clariion.c
> > b/libmultipath/checkers/emc_clariion.c
> > index f8e55b93..a277a004 100644
> > --- a/libmultipath/checkers/emc_clariion.c
> > +++ b/libmultipath/checkers/emc_clariion.c
> > @@ -20,6 +20,11 @@
> >  #define INQUIRY_CMD     0x12
> >  #define INQUIRY_CMDLEN  6
> >  #define HEAVY_CHECK_COUNT       10
> > +#define SCSI_COMMAND_TERMINATED	0x22
> > +#define SCSI_CHECK_CONDITION	0x2
> > +#define RECOVERED_ERROR		0x01
> > +#define ILLEGAL_REQUEST		0x05
> > +#define SG_ERR_DRIVER_SENSE	0x08
> >  
> >  /*
> >   * Mechanism to track CLARiiON inactive snapshot LUs.
> > @@ -130,7 +135,9 @@ int libcheck_check (struct checker * c)
> >  		(struct emc_clariion_checker_path_context *)c->context;
> >  	char wwnstr[33];
> >  	int ret;
> > +	int retry_emc = 5;
> >  
> > +retry:
> >  	memset(&io_hdr, 0, sizeof (struct sg_io_hdr));
> >  	memset(sense_buffer, 0, 128);
> >  	memset(sb, 0, SENSE_BUFF_LEN);
> > @@ -145,12 +152,51 @@ int libcheck_check (struct checker * c)
> >  	io_hdr.timeout = c->timeout * 1000;
> >  	io_hdr.pack_id = 0;
> >  	if (ioctl(c->fd, SG_IO, &io_hdr) < 0) {
> > +		if (errno == ENOTTY) {
> > +			c->msgid = CHECKER_MSGID_UNSUPPORTED;
> > +			return PATH_WILD;
> > +		}
> >  		c->msgid = MSG_CLARIION_QUERY_FAILED;
> >  		return PATH_DOWN;
> >  	}
> > +
> >  	if (io_hdr.info & SG_INFO_OK_MASK) {
> > -		c->msgid = MSG_CLARIION_QUERY_ERROR;
> > -		return PATH_DOWN;
> 
> Previously, if (io_hdr.info & SG_INFO_OK_MASK) the checker would
> always
> return PATH_DOWN.  Both the tur and rdac checker always return either
> PATH_DOWN or PATH_WILD if (io_hdr.info & SG_INFO_OK_MASK && sense_key
> != RECOVERED_ERROR). With the clariion checker, it now appears
> possible
> to have (io_hdr.info & SG_INFO_OK_MASK && sense_key !=
> RECOVERED_ERROR)
> and still return PATH_UP. Is this intentional? Am I missing something
> here?

I copied this code from the rdac checker. But the default return value
in emc_clariion is different, thanks for spotting that. I will post an
update.

Handling of SCSI status and sense codes is done differently in
different parts of multipath-tools. We really need common SCSI code :-/

Martin

-- 
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 14/21] libmultipath: hp_sw checker: leave unsupported paths alone

[Martin Wilck]

A checker shouldn't set the path state to PATH_DOWN if it fails
to obtain information about the path in the first place. Add logic
to the checker to distinguish a failed path from an unsupported path.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/checkers/hp_sw.c | 29 +++++++++++++++++++++--------
 1 file changed, 21 insertions(+), 8 deletions(-)

diff --git a/libmultipath/checkers/hp_sw.c b/libmultipath/checkers/hp_sw.c
index d7f1018c..1a820223 100644
--- a/libmultipath/checkers/hp_sw.c
+++ b/libmultipath/checkers/hp_sw.c
@@ -24,6 +24,7 @@
 #define SCSI_COMMAND_TERMINATED	0x22
 #define SG_ERR_DRIVER_SENSE	0x08
 #define RECOVERED_ERROR		0x01
+#define ILLEGAL_REQUEST		0x05
 #define MX_ALLOC_LEN		255
 #define HEAVY_CHECK_COUNT       10
 
@@ -68,14 +69,19 @@ do_inq(int sg_fd, int cmddt, int evpd, unsigned int pg_op,
 	io_hdr.sbp = sense_b;
 	io_hdr.timeout = timeout * 1000;
 
-	if (ioctl(sg_fd, SG_IO, &io_hdr) < 0)
-		return 1;
+	if (ioctl(sg_fd, SG_IO, &io_hdr) < 0) {
+		if (errno == ENOTTY)
+			return PATH_WILD;
+		else
+			return PATH_DOWN;
+	}
 
 	/* treat SG_ERR here to get rid of sg_err.[ch] */
 	io_hdr.status &= 0x7e;
 	if ((0 == io_hdr.status) && (0 == io_hdr.host_status) &&
 	    (0 == io_hdr.driver_status))
-		return 0;
+		return PATH_UP;
+
 	if ((SCSI_CHECK_CONDITION == io_hdr.status) ||
 	    (SCSI_COMMAND_TERMINATED == io_hdr.status) ||
 	    (SG_ERR_DRIVER_SENSE == (0xf & io_hdr.driver_status))) {
@@ -86,11 +92,13 @@ do_inq(int sg_fd, int cmddt, int evpd, unsigned int pg_op,
 				sense_key = sense_buffer[1] & 0xf;
 			else
 				sense_key = sense_buffer[2] & 0xf;
-			if(RECOVERED_ERROR == sense_key)
-				return 0;
+			if (RECOVERED_ERROR == sense_key)
+				return PATH_UP;
+			else if (ILLEGAL_REQUEST == sense_key)
+				return PATH_WILD;
 		}
 	}
-	return 1;
+	return PATH_DOWN;
 }
 
 static int
@@ -122,10 +130,15 @@ do_tur (int fd, unsigned int timeout)
 int libcheck_check(struct checker * c)
 {
 	char buff[MX_ALLOC_LEN];
+	int ret = do_inq(c->fd, 0, 1, 0x80, buff, MX_ALLOC_LEN, 0, c->timeout);
 
-	if (0 != do_inq(c->fd, 0, 1, 0x80, buff, MX_ALLOC_LEN, 0, c->timeout)) {
+	if (ret == PATH_WILD) {
+		c->msgid = CHECKER_MSGID_UNSUPPORTED;
+		return ret;
+	}
+	if (ret != PATH_UP) {
 		c->msgid = CHECKER_MSGID_DOWN;
-		return PATH_DOWN;
+		return ret;
 	};
 
 	if (do_tur(c->fd, c->timeout)) {
-- 
2.19.0

Re: [PATCH 14/21] libmultipath: hp_sw checker: leave unsupported paths alone

[Benjamin Marzinski]

On Fri, Oct 12, 2018 at 12:27:00AM +0200, Martin Wilck wrote:
> A checker shouldn't set the path state to PATH_DOWN if it fails
> to obtain information about the path in the first place. Add logic
> to the checker to distinguish a failed path from an unsupported path.
> 

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

> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/checkers/hp_sw.c | 29 +++++++++++++++++++++--------
>  1 file changed, 21 insertions(+), 8 deletions(-)
> 
> diff --git a/libmultipath/checkers/hp_sw.c b/libmultipath/checkers/hp_sw.c
> index d7f1018c..1a820223 100644
> --- a/libmultipath/checkers/hp_sw.c
> +++ b/libmultipath/checkers/hp_sw.c
> @@ -24,6 +24,7 @@
>  #define SCSI_COMMAND_TERMINATED	0x22
>  #define SG_ERR_DRIVER_SENSE	0x08
>  #define RECOVERED_ERROR		0x01
> +#define ILLEGAL_REQUEST		0x05
>  #define MX_ALLOC_LEN		255
>  #define HEAVY_CHECK_COUNT       10
>  
> @@ -68,14 +69,19 @@ do_inq(int sg_fd, int cmddt, int evpd, unsigned int pg_op,
>  	io_hdr.sbp = sense_b;
>  	io_hdr.timeout = timeout * 1000;
>  
> -	if (ioctl(sg_fd, SG_IO, &io_hdr) < 0)
> -		return 1;
> +	if (ioctl(sg_fd, SG_IO, &io_hdr) < 0) {
> +		if (errno == ENOTTY)
> +			return PATH_WILD;
> +		else
> +			return PATH_DOWN;
> +	}
>  
>  	/* treat SG_ERR here to get rid of sg_err.[ch] */
>  	io_hdr.status &= 0x7e;
>  	if ((0 == io_hdr.status) && (0 == io_hdr.host_status) &&
>  	    (0 == io_hdr.driver_status))
> -		return 0;
> +		return PATH_UP;
> +
>  	if ((SCSI_CHECK_CONDITION == io_hdr.status) ||
>  	    (SCSI_COMMAND_TERMINATED == io_hdr.status) ||
>  	    (SG_ERR_DRIVER_SENSE == (0xf & io_hdr.driver_status))) {
> @@ -86,11 +92,13 @@ do_inq(int sg_fd, int cmddt, int evpd, unsigned int pg_op,
>  				sense_key = sense_buffer[1] & 0xf;
>  			else
>  				sense_key = sense_buffer[2] & 0xf;
> -			if(RECOVERED_ERROR == sense_key)
> -				return 0;
> +			if (RECOVERED_ERROR == sense_key)
> +				return PATH_UP;
> +			else if (ILLEGAL_REQUEST == sense_key)
> +				return PATH_WILD;
>  		}
>  	}
> -	return 1;
> +	return PATH_DOWN;
>  }
>  
>  static int
> @@ -122,10 +130,15 @@ do_tur (int fd, unsigned int timeout)
>  int libcheck_check(struct checker * c)
>  {
>  	char buff[MX_ALLOC_LEN];
> +	int ret = do_inq(c->fd, 0, 1, 0x80, buff, MX_ALLOC_LEN, 0, c->timeout);
>  
> -	if (0 != do_inq(c->fd, 0, 1, 0x80, buff, MX_ALLOC_LEN, 0, c->timeout)) {
> +	if (ret == PATH_WILD) {
> +		c->msgid = CHECKER_MSGID_UNSUPPORTED;
> +		return ret;
> +	}
> +	if (ret != PATH_UP) {
>  		c->msgid = CHECKER_MSGID_DOWN;
> -		return PATH_DOWN;
> +		return ret;
>  	};
>  
>  	if (do_tur(c->fd, c->timeout)) {
> -- 
> 2.19.0

[PATCH 15/21] libmultipath: rdac checker: leave unsupported paths alone

[Martin Wilck]

A checker shouldn't set the path state to PATH_DOWN if it fails
to obtain information about the path in the first place. Add logic
to the checker to distinguish a failed path from an unsupported path.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/checkers/rdac.c | 28 +++++++++++++++++++---------
 1 file changed, 19 insertions(+), 9 deletions(-)

diff --git a/libmultipath/checkers/rdac.c b/libmultipath/checkers/rdac.c
index 266f8e10..8a3b73ec 100644
--- a/libmultipath/checkers/rdac.c
+++ b/libmultipath/checkers/rdac.c
@@ -26,6 +26,7 @@
 #define SCSI_COMMAND_TERMINATED	0x22
 #define SG_ERR_DRIVER_SENSE	0x08
 #define RECOVERED_ERROR		0x01
+#define ILLEGAL_REQUEST		0x05
 
 
 #define CURRENT_PAGE_CODE_VALUES	0
@@ -162,14 +163,14 @@ retry:
 	io_hdr.sbp = sense_b;
 	io_hdr.timeout = timeout * 1000;
 
-	if (ioctl(sg_fd, SG_IO, &io_hdr) < 0)
-		return 1;
+	if (ioctl(sg_fd, SG_IO, &io_hdr) < 0 && errno == ENOTTY)
+		return PATH_WILD;
 
 	/* treat SG_ERR here to get rid of sg_err.[ch] */
 	io_hdr.status &= 0x7e;
 	if ((0 == io_hdr.status) && (0 == io_hdr.host_status) &&
 	    (0 == io_hdr.driver_status))
-		return 0;
+		return PATH_UP;
 
 	/* check if we need to retry this error */
 	if (io_hdr.info & SG_INFO_OK_MASK) {
@@ -198,10 +199,14 @@ retry:
 			else
 				sense_key = sense_buffer[2] & 0xf;
 			if (RECOVERED_ERROR == sense_key)
-				return 0;
+				return PATH_UP;
+			else if (ILLEGAL_REQUEST == sense_key)
+				return PATH_WILD;
+			condlog(1, "rdac checker: INQUIRY failed with sense key %02x",
+				sense_key);
 		}
 	}
-	return 1;
+	return PATH_DOWN;
 }
 
 struct volume_access_inq
@@ -290,12 +295,14 @@ int libcheck_check(struct checker * c)
 
 	inqfail = 0;
 	memset(&inq, 0, sizeof(struct volume_access_inq));
-	if (0 != do_inq(c->fd, 0xC9, &inq, sizeof(struct volume_access_inq),
-			c->timeout)) {
-		ret = PATH_DOWN;
+	ret = do_inq(c->fd, 0xC9, &inq, sizeof(struct volume_access_inq),
+		     c->timeout);
+	if (ret != PATH_UP) {
 		inqfail = 1;
 		goto done;
-	} else if (((inq.PQ_PDT & 0xE0) == 0x20) || (inq.PQ_PDT & 0x7f)) {
+	}
+
+	if (((inq.PQ_PDT & 0xE0) == 0x20) || (inq.PQ_PDT & 0x7f)) {
 		/* LUN not connected*/
 		ret = PATH_DOWN;
 		goto done;
@@ -332,6 +339,9 @@ int libcheck_check(struct checker * c)
 
 done:
 	switch (ret) {
+	case PATH_WILD:
+		c->msgid = CHECKER_MSGID_UNSUPPORTED;
+		break;
 	case PATH_DOWN:
 		c->msgid = (inqfail ? RDAC_MSGID_INQUIRY_FAILED :
 			    checker_msg_string(&inq));
-- 
2.19.0

Re: [PATCH 15/21] libmultipath: rdac checker: leave unsupported paths alone

[Benjamin Marzinski]

On Fri, Oct 12, 2018 at 12:27:01AM +0200, Martin Wilck wrote:
> A checker shouldn't set the path state to PATH_DOWN if it fails
> to obtain information about the path in the first place. Add logic
> to the checker to distinguish a failed path from an unsupported path.
> 

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

> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/checkers/rdac.c | 28 +++++++++++++++++++---------
>  1 file changed, 19 insertions(+), 9 deletions(-)
> 
> diff --git a/libmultipath/checkers/rdac.c b/libmultipath/checkers/rdac.c
> index 266f8e10..8a3b73ec 100644
> --- a/libmultipath/checkers/rdac.c
> +++ b/libmultipath/checkers/rdac.c
> @@ -26,6 +26,7 @@
>  #define SCSI_COMMAND_TERMINATED	0x22
>  #define SG_ERR_DRIVER_SENSE	0x08
>  #define RECOVERED_ERROR		0x01
> +#define ILLEGAL_REQUEST		0x05
>  
>  
>  #define CURRENT_PAGE_CODE_VALUES	0
> @@ -162,14 +163,14 @@ retry:
>  	io_hdr.sbp = sense_b;
>  	io_hdr.timeout = timeout * 1000;
>  
> -	if (ioctl(sg_fd, SG_IO, &io_hdr) < 0)
> -		return 1;
> +	if (ioctl(sg_fd, SG_IO, &io_hdr) < 0 && errno == ENOTTY)
> +		return PATH_WILD;
>  
>  	/* treat SG_ERR here to get rid of sg_err.[ch] */
>  	io_hdr.status &= 0x7e;
>  	if ((0 == io_hdr.status) && (0 == io_hdr.host_status) &&
>  	    (0 == io_hdr.driver_status))
> -		return 0;
> +		return PATH_UP;
>  
>  	/* check if we need to retry this error */
>  	if (io_hdr.info & SG_INFO_OK_MASK) {
> @@ -198,10 +199,14 @@ retry:
>  			else
>  				sense_key = sense_buffer[2] & 0xf;
>  			if (RECOVERED_ERROR == sense_key)
> -				return 0;
> +				return PATH_UP;
> +			else if (ILLEGAL_REQUEST == sense_key)
> +				return PATH_WILD;
> +			condlog(1, "rdac checker: INQUIRY failed with sense key %02x",
> +				sense_key);
>  		}
>  	}
> -	return 1;
> +	return PATH_DOWN;
>  }
>  
>  struct volume_access_inq
> @@ -290,12 +295,14 @@ int libcheck_check(struct checker * c)
>  
>  	inqfail = 0;
>  	memset(&inq, 0, sizeof(struct volume_access_inq));
> -	if (0 != do_inq(c->fd, 0xC9, &inq, sizeof(struct volume_access_inq),
> -			c->timeout)) {
> -		ret = PATH_DOWN;
> +	ret = do_inq(c->fd, 0xC9, &inq, sizeof(struct volume_access_inq),
> +		     c->timeout);
> +	if (ret != PATH_UP) {
>  		inqfail = 1;
>  		goto done;
> -	} else if (((inq.PQ_PDT & 0xE0) == 0x20) || (inq.PQ_PDT & 0x7f)) {
> +	}
> +
> +	if (((inq.PQ_PDT & 0xE0) == 0x20) || (inq.PQ_PDT & 0x7f)) {
>  		/* LUN not connected*/
>  		ret = PATH_DOWN;
>  		goto done;
> @@ -332,6 +339,9 @@ int libcheck_check(struct checker * c)
>  
>  done:
>  	switch (ret) {
> +	case PATH_WILD:
> +		c->msgid = CHECKER_MSGID_UNSUPPORTED;
> +		break;
>  	case PATH_DOWN:
>  		c->msgid = (inqfail ? RDAC_MSGID_INQUIRY_FAILED :
>  			    checker_msg_string(&inq));
> -- 
> 2.19.0

[PATCH 16/21] libmultipath: tur checker: leave unsupported paths alone

[Martin Wilck]

A checker shouldn't set the path state to PATH_DOWN if it fails
to obtain information about the path in the first place. Add logic
to the checker to distinguish a failed path from an unsupported path.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/checkers/tur.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/libmultipath/checkers/tur.c b/libmultipath/checkers/tur.c
index 448c396e..8f4bdc8b 100644
--- a/libmultipath/checkers/tur.c
+++ b/libmultipath/checkers/tur.c
@@ -125,6 +125,10 @@ retry:
 	io_hdr.timeout = timeout * 1000;
 	io_hdr.pack_id = 0;
 	if (ioctl(fd, SG_IO, &io_hdr) < 0) {
+		if (errno == ENOTTY) {
+			*msgid = CHECKER_MSGID_UNSUPPORTED;
+			return PATH_WILD;
+		}
 		*msgid = CHECKER_MSGID_DOWN;
 		return PATH_DOWN;
 	}
-- 
2.19.0

Re: [PATCH 16/21] libmultipath: tur checker: leave unsupported paths alone

[Benjamin Marzinski]

On Fri, Oct 12, 2018 at 12:27:02AM +0200, Martin Wilck wrote:
> A checker shouldn't set the path state to PATH_DOWN if it fails
> to obtain information about the path in the first place. Add logic
> to the checker to distinguish a failed path from an unsupported path.
> 

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

> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/checkers/tur.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/libmultipath/checkers/tur.c b/libmultipath/checkers/tur.c
> index 448c396e..8f4bdc8b 100644
> --- a/libmultipath/checkers/tur.c
> +++ b/libmultipath/checkers/tur.c
> @@ -125,6 +125,10 @@ retry:
>  	io_hdr.timeout = timeout * 1000;
>  	io_hdr.pack_id = 0;
>  	if (ioctl(fd, SG_IO, &io_hdr) < 0) {
> +		if (errno == ENOTTY) {
> +			*msgid = CHECKER_MSGID_UNSUPPORTED;
> +			return PATH_WILD;
> +		}
>  		*msgid = CHECKER_MSGID_DOWN;
>  		return PATH_DOWN;
>  	}
> -- 
> 2.19.0

[PATCH 17/21] libmultipath: pathinfo: don't blank wwid if checker fails

[Martin Wilck]

Blanking a WWID is a dangerous operation. E.g. configure() would
consider the path in question as invalid and orphan it if the
WWID is blank. Don't do this for possibly transient checker failures.
Moreover, we try to determine WWID even if path_offline returns
PATH_DOWN in the first place, so why should we not if the checker
has a problem?

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

diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index 3550c3a7..467ece7a 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -1944,9 +1944,6 @@ int pathinfo(struct path *pp, struct config *conf, int mask)
 		if (path_state == PATH_UP) {
 			pp->chkrstate = pp->state = get_state(pp, conf, 0,
 							      path_state);
-			if (pp->state == PATH_UNCHECKED ||
-			    pp->state == PATH_WILD)
-				goto blank;
 			if (pp->state == PATH_TIMEOUT)
 				pp->state = PATH_DOWN;
 			if (pp->state == PATH_UP && !pp->size) {
-- 
2.19.0

Re: [PATCH 17/21] libmultipath: pathinfo: don't blank wwid if checker fails

[Benjamin Marzinski]

On Fri, Oct 12, 2018 at 12:27:03AM +0200, Martin Wilck wrote:
> Blanking a WWID is a dangerous operation. E.g. configure() would
> consider the path in question as invalid and orphan it if the
> WWID is blank. Don't do this for possibly transient checker failures.
> Moreover, we try to determine WWID even if path_offline returns
> PATH_DOWN in the first place, so why should we not if the checker
> has a problem?
> 

Is PATH_WILD a transient error?  Clearly we have cases where
PATH_UNCHECKED happens because of a transient error. What's the case
that you are worried about where we temporarily get a PATH_WILD, but we
think we should continue will setting up the path?

> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/discovery.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
> index 3550c3a7..467ece7a 100644
> --- a/libmultipath/discovery.c
> +++ b/libmultipath/discovery.c
> @@ -1944,9 +1944,6 @@ int pathinfo(struct path *pp, struct config *conf, int mask)
>  		if (path_state == PATH_UP) {
>  			pp->chkrstate = pp->state = get_state(pp, conf, 0,
>  							      path_state);
> -			if (pp->state == PATH_UNCHECKED ||
> -			    pp->state == PATH_WILD)
> -				goto blank;
>  			if (pp->state == PATH_TIMEOUT)
>  				pp->state = PATH_DOWN;
>  			if (pp->state == PATH_UP && !pp->size) {
> -- 
> 2.19.0

Re: [PATCH 17/21] libmultipath: pathinfo: don't blank wwid if checker fails

[Martin Wilck]

On Thu, 2018-10-25 at 16:50 -0500,  Benjamin Marzinski  wrote:
> On Fri, Oct 12, 2018 at 12:27:03AM +0200, Martin Wilck wrote:
> > Blanking a WWID is a dangerous operation. E.g. configure() would
> > consider the path in question as invalid and orphan it if the
> > WWID is blank. Don't do this for possibly transient checker
> > failures.
> > Moreover, we try to determine WWID even if path_offline returns
> > PATH_DOWN in the first place, so why should we not if the checker
> > has a problem?
> > 
> 
> Is PATH_WILD a transient error?  Clearly we have cases where
> PATH_UNCHECKED happens because of a transient error. What's the case
> that you are worried about where we temporarily get a PATH_WILD, but
> we
> think we should continue will setting up the path?

My mentioning of a "transient" error condition is misleading. 

But do I think that the patch is correct. In particular with the
previous changes in the series, PATH_WILD would mean that the checker
couldn't do it's job and simply has no information whether or not the
path is good. IMO it is wrong not to try to obtain a WWID, or even
blank an already retrieved WWID, in this case.

Do you disagree?

Regards,
Martin

-- 
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

Re: [PATCH 17/21] libmultipath: pathinfo: don't blank wwid if checker fails

[Benjamin Marzinski]

On Fri, Oct 26, 2018 at 11:16:23AM +0200, Martin Wilck wrote:
> On Thu, 2018-10-25 at 16:50 -0500,  Benjamin Marzinski  wrote:
> > On Fri, Oct 12, 2018 at 12:27:03AM +0200, Martin Wilck wrote:
> > > Blanking a WWID is a dangerous operation. E.g. configure() would
> > > consider the path in question as invalid and orphan it if the
> > > WWID is blank. Don't do this for possibly transient checker
> > > failures.
> > > Moreover, we try to determine WWID even if path_offline returns
> > > PATH_DOWN in the first place, so why should we not if the checker
> > > has a problem?
> > > 
> > 
> > Is PATH_WILD a transient error?  Clearly we have cases where
> > PATH_UNCHECKED happens because of a transient error. What's the case
> > that you are worried about where we temporarily get a PATH_WILD, but
> > we
> > think we should continue will setting up the path?
> 
> My mentioning of a "transient" error condition is misleading. 
> 
> But do I think that the patch is correct. In particular with the
> previous changes in the series, PATH_WILD would mean that the checker
> couldn't do it's job and simply has no information whether or not the
> path is good. IMO it is wrong not to try to obtain a WWID, or even
> blank an already retrieved WWID, in this case.
> 
> Do you disagree?

I guess the question is "Should we create a multipath device if we think
that it's likely misconfigured?" If there really are multiple paths,
then it makes sense that we should create the multipath device.  On the
other hand, if the checker is misconfigured and a path fails, multipathd
may not be able to restore it, and users might not find that out until
they actually have the path fail, if they aren't paying attention.

I guess I don't really have any strong feelings against your approach.

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

> 
> Regards,
> Martin
> 
> -- 
> 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)
> 

[PATCH 18/21] multipathd: check_path: improve logging for "unusable path" case

[Martin Wilck]

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

diff --git a/multipathd/main.c b/multipathd/main.c
index 1bd1518a..8ce72fce 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -1894,7 +1894,8 @@ check_path (struct vectors * vecs, struct path * pp, int ticks)
 	}
 
 	if (newstate == PATH_WILD || newstate == PATH_UNCHECKED) {
-		condlog(2, "%s: unusable path", pp->dev);
+		condlog(2, "%s: unusable path - checker failed", pp->dev);
+		LOG_MSG(2, verbosity, pp);
 		conf = get_multipath_config();
 		pthread_cleanup_push(put_multipath_config, conf);
 		pathinfo(pp, conf, 0);
-- 
2.19.0

Re: [PATCH 18/21] multipathd: check_path: improve logging for "unusable path" case

[Benjamin Marzinski]

On Fri, Oct 12, 2018 at 12:27:04AM +0200, Martin Wilck wrote:

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

> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  multipathd/main.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 1bd1518a..8ce72fce 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -1894,7 +1894,8 @@ check_path (struct vectors * vecs, struct path * pp, int ticks)
>  	}
>  
>  	if (newstate == PATH_WILD || newstate == PATH_UNCHECKED) {
> -		condlog(2, "%s: unusable path", pp->dev);
> +		condlog(2, "%s: unusable path - checker failed", pp->dev);
> +		LOG_MSG(2, verbosity, pp);
>  		conf = get_multipath_config();
>  		pthread_cleanup_push(put_multipath_config, conf);
>  		pathinfo(pp, conf, 0);
> -- 
> 2.19.0

[PATCH 19/21] libmultipath: coalesce_paths: improve logging of orphaned paths

[Martin Wilck]

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/configure.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index 09c3dcf2..ed3e30f5 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -1020,14 +1020,18 @@ int coalesce_paths (struct vectors * vecs, vector newmp, char * refwwid,
 		}
 	}
 	vector_foreach_slot (pathvec, pp1, k) {
-		int invalid = 0;
+		int invalid;
 		/* skip this path for some reason */
 
 		/* 1. if path has no unique id or wwid blacklisted */
+		if (strlen(pp1->wwid) == 0) {
+			orphan_path(pp1, "no WWID");
+			continue;
+		}
+
 		conf = get_multipath_config();
 		pthread_cleanup_push(put_multipath_config, conf);
-		if (strlen(pp1->wwid) == 0 || filter_path(conf, pp1) > 0)
-			invalid = 1;
+		invalid = (filter_path(conf, pp1) > 0);
 		pthread_cleanup_pop(1);
 		if (invalid) {
 			orphan_path(pp1, "blacklisted");
-- 
2.19.0

Re: [PATCH 19/21] libmultipath: coalesce_paths: improve logging of orphaned paths

[Benjamin Marzinski]

On Fri, Oct 12, 2018 at 12:27:05AM +0200, Martin Wilck wrote:

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

> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/configure.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/libmultipath/configure.c b/libmultipath/configure.c
> index 09c3dcf2..ed3e30f5 100644
> --- a/libmultipath/configure.c
> +++ b/libmultipath/configure.c
> @@ -1020,14 +1020,18 @@ int coalesce_paths (struct vectors * vecs, vector newmp, char * refwwid,
>  		}
>  	}
>  	vector_foreach_slot (pathvec, pp1, k) {
> -		int invalid = 0;
> +		int invalid;
>  		/* skip this path for some reason */
>  
>  		/* 1. if path has no unique id or wwid blacklisted */
> +		if (strlen(pp1->wwid) == 0) {
> +			orphan_path(pp1, "no WWID");
> +			continue;
> +		}
> +
>  		conf = get_multipath_config();
>  		pthread_cleanup_push(put_multipath_config, conf);
> -		if (strlen(pp1->wwid) == 0 || filter_path(conf, pp1) > 0)
> -			invalid = 1;
> +		invalid = (filter_path(conf, pp1) > 0);
>  		pthread_cleanup_pop(1);
>  		if (invalid) {
>  			orphan_path(pp1, "blacklisted");
> -- 
> 2.19.0

[PATCH 20/21] libmultipath: sync_map_state: log failing paths

[Martin Wilck]

Emit a log message when force-failing exisiting paths.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/structs_vec.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
index f87d69d4..c85823a0 100644
--- a/libmultipath/structs_vec.c
+++ b/libmultipath/structs_vec.c
@@ -318,8 +318,11 @@ sync_map_state(struct multipath *mpp)
 			else if ((pp->dmstate == PSTATE_ACTIVE ||
 				  pp->dmstate == PSTATE_UNDEF) &&
 				 (pp->state == PATH_DOWN ||
-				  pp->state == PATH_SHAKY))
+				  pp->state == PATH_SHAKY)) {
+				condlog(2, "sync_map_state: failing %s state %d dmstate %d",
+					pp->dev, pp->state, pp->dmstate);
 				dm_fail_path(mpp->alias, pp->dev_t);
+			}
 		}
 	}
 }
-- 
2.19.0

Re: [PATCH 20/21] libmultipath: sync_map_state: log failing paths

[Benjamin Marzinski]

On Fri, Oct 12, 2018 at 12:27:06AM +0200, Martin Wilck wrote:
> Emit a log message when force-failing exisiting paths.
> 

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

> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/structs_vec.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
> index f87d69d4..c85823a0 100644
> --- a/libmultipath/structs_vec.c
> +++ b/libmultipath/structs_vec.c
> @@ -318,8 +318,11 @@ sync_map_state(struct multipath *mpp)
>  			else if ((pp->dmstate == PSTATE_ACTIVE ||
>  				  pp->dmstate == PSTATE_UNDEF) &&
>  				 (pp->state == PATH_DOWN ||
> -				  pp->state == PATH_SHAKY))
> +				  pp->state == PATH_SHAKY)) {
> +				condlog(2, "sync_map_state: failing %s state %d dmstate %d",
> +					pp->dev, pp->state, pp->dmstate);
>  				dm_fail_path(mpp->alias, pp->dev_t);
> +			}
>  		}
>  	}
>  }
> -- 
> 2.19.0

[PATCH 21/21] libmultipath/checkers: cleanup class/instance model

[Martin Wilck]

The checkers code implicitly uses a sort-of OOP class/instance model,
but very clumsily. Separate the checker "class" and "instance" cleanly,
and do a few further cleanups (constifications etc) on the way.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/checkers.c          | 137 ++++++++++++++++---------------
 libmultipath/checkers.h          |  23 ++----
 libmultipath/checkers/directio.c |   2 +-
 libmultipath/checkers/tur.c      |   2 +-
 libmultipath/print.c             |   2 +-
 libmultipath/propsel.c           |  19 +++--
 6 files changed, 92 insertions(+), 93 deletions(-)

diff --git a/libmultipath/checkers.c b/libmultipath/checkers.c
index 19c8ad58..7733471c 100644
--- a/libmultipath/checkers.c
+++ b/libmultipath/checkers.c
@@ -8,6 +8,19 @@
 #include "checkers.h"
 #include "vector.h"
 
+struct checker_class {
+	struct list_head node;
+	void *handle;
+	int refcount;
+	int sync;
+	char name[CHECKER_NAME_LEN];
+	int (*check)(struct checker *);
+	int (*init)(struct checker *);       /* to allocate the context */
+	void (*free)(struct checker *);      /* to free the context */
+	const char **msgtable;
+	short msgtable_size;
+};
+
 char *checker_state_names[] = {
 	"wild",
 	"unchecked",
@@ -23,38 +36,30 @@ char *checker_state_names[] = {
 
 static LIST_HEAD(checkers);
 
-char * checker_state_name (int i)
+const char *checker_state_name(int i)
 {
 	return checker_state_names[i];
 }
 
-int init_checkers (char *multipath_dir)
-{
-	if (!add_checker(multipath_dir, DEFAULT_CHECKER))
-		return 1;
-	return 0;
-}
-
-struct checker * alloc_checker (void)
+static struct checker_class *alloc_checker_class(void)
 {
-	struct checker *c;
+	struct checker_class *c;
 
-	c = MALLOC(sizeof(struct checker));
+	c = MALLOC(sizeof(struct checker_class));
 	if (c) {
 		INIT_LIST_HEAD(&c->node);
 		c->refcount = 1;
-		c->fd = -1;
 	}
 	return c;
 }
 
-void free_checker (struct checker * c)
+void free_checker_class(struct checker_class *c)
 {
 	if (!c)
 		return;
 	c->refcount--;
 	if (c->refcount) {
-		condlog(3, "%s checker refcount %d",
+		condlog(4, "%s checker refcount %d",
 			c->name, c->refcount);
 		return;
 	}
@@ -71,17 +76,17 @@ void free_checker (struct checker * c)
 
 void cleanup_checkers (void)
 {
-	struct checker * checker_loop;
-	struct checker * checker_temp;
+	struct checker_class *checker_loop;
+	struct checker_class *checker_temp;
 
 	list_for_each_entry_safe(checker_loop, checker_temp, &checkers, node) {
-		free_checker(checker_loop);
+		free_checker_class(checker_loop);
 	}
 }
 
-struct checker * checker_lookup (char * name)
+static struct checker_class *checker_class_lookup(const char *name)
 {
-	struct checker * c;
+	struct checker_class *c;
 
 	if (!name || !strlen(name))
 		return NULL;
@@ -92,14 +97,15 @@ struct checker * checker_lookup (char * name)
 	return NULL;
 }
 
-struct checker * add_checker (char *multipath_dir, char * name)
+static struct checker_class *add_checker_class(const char *multipath_dir,
+					       const char *name)
 {
 	char libname[LIB_CHECKER_NAMELEN];
 	struct stat stbuf;
-	struct checker * c;
+	struct checker_class *c;
 	char *errstr;
 
-	c = alloc_checker();
+	c = alloc_checker_class();
 	if (!c)
 		return NULL;
 	snprintf(c->name, CHECKER_NAME_LEN, "%s", name);
@@ -158,12 +164,11 @@ struct checker * add_checker (char *multipath_dir, char * name)
 		c->name, c->msgtable_size);
 
 done:
-	c->fd = -1;
 	c->sync = 1;
 	list_add(&c->node, &checkers);
 	return c;
 out:
-	free_checker(c);
+	free_checker_class(c);
 	return NULL;
 }
 
@@ -176,16 +181,16 @@ void checker_set_fd (struct checker * c, int fd)
 
 void checker_set_sync (struct checker * c)
 {
-	if (!c)
+	if (!c || !c->cls)
 		return;
-	c->sync = 1;
+	c->cls->sync = 1;
 }
 
 void checker_set_async (struct checker * c)
 {
-	if (!c)
+	if (!c || !c->cls)
 		return;
-	c->sync = 0;
+	c->cls->sync = 0;
 }
 
 void checker_enable (struct checker * c)
@@ -204,11 +209,11 @@ void checker_disable (struct checker * c)
 
 int checker_init (struct checker * c, void ** mpctxt_addr)
 {
-	if (!c)
+	if (!c || !c->cls)
 		return 1;
 	c->mpcontext = mpctxt_addr;
-	if (c->init)
-		return c->init(c);
+	if (c->cls->init)
+		return c->cls->init(c);
 	return 0;
 }
 
@@ -220,15 +225,16 @@ void checker_clear (struct checker *c)
 
 void checker_put (struct checker * dst)
 {
-	struct checker * src;
+	struct checker_class *src;
 
-	if (!dst || !strlen(dst->name))
+	if (!dst)
 		return;
-	src = checker_lookup(dst->name);
-	if (dst->free)
-		dst->free(dst);
+	src = dst->cls;
+
+	if (src && src->free)
+		src->free(dst);
 	checker_clear(dst);
-	free_checker(src);
+	free_checker_class(src);
 }
 
 int checker_check (struct checker * c, int path_state)
@@ -243,32 +249,35 @@ int checker_check (struct checker * c, int path_state)
 		c->msgid = CHECKER_MSGID_DISABLED;
 		return PATH_UNCHECKED;
 	}
-	if (!strncmp(c->name, NONE, 4))
+	if (!strncmp(c->cls->name, NONE, 4))
 		return path_state;
 
 	if (c->fd < 0) {
 		c->msgid = CHECKER_MSGID_NO_FD;
 		return PATH_WILD;
 	}
-	r = c->check(c);
+	r = c->cls->check(c);
 
 	return r;
 }
 
-int checker_selected (struct checker * c)
+int checker_selected(const struct checker *c)
 {
 	if (!c)
 		return 0;
-	if (!strncmp(c->name, NONE, 4))
-		return 1;
-	return (c->check) ? 1 : 0;
+	return c->cls != NULL;
 }
 
 const char *checker_name(const struct checker *c)
 {
-	if (!c)
+	if (!c || !c->cls)
 		return NULL;
-	return c->name;
+	return c->cls->name;
+}
+
+int checker_is_sync(const struct checker *c)
+{
+	return c && c->cls && c->cls->sync;
 }
 
 static const char *generic_msg[CHECKER_LAST_GENERIC_MSGID] = {
@@ -295,8 +304,8 @@ static const char *_checker_message(const struct checker *c)
 		return generic_msg[c->msgid];
 
 	id = c->msgid - CHECKER_FIRST_MSGID;
-	if (id < c->msgtable_size)
-		return c->msgtable[id];
+	if (id < c->cls->msgtable_size)
+		return c->cls->msgtable[id];
 	return NULL;
 }
 
@@ -309,7 +318,7 @@ const char *checker_message(const struct checker *c)
 		*buf = '\0';
 	else
 		snprintf(buf, sizeof(buf), "%s checker%s",
-			 c->name, msg);
+			 c->cls->name, msg);
 	return buf;
 }
 
@@ -320,31 +329,29 @@ void checker_clear_message (struct checker *c)
 	c->msgid = CHECKER_MSGID_NONE;
 }
 
-void checker_get (char *multipath_dir, struct checker * dst, char * name)
+void checker_get(const char *multipath_dir, struct checker *dst,
+		 const char *name)
 {
-	struct checker * src = NULL;
+	struct checker_class *src = NULL;
 
 	if (!dst)
 		return;
 
 	if (name && strlen(name)) {
-		src = checker_lookup(name);
+		src = checker_class_lookup(name);
 		if (!src)
-			src = add_checker(multipath_dir, name);
+			src = add_checker_class(multipath_dir, name);
 	}
-	if (!src) {
-		dst->check = NULL;
+	dst->cls = src;
+	if (!src)
 		return;
-	}
-	dst->fd = src->fd;
-	dst->sync = src->sync;
-	strncpy(dst->name, src->name, CHECKER_NAME_LEN);
-	dst->msgid = src->msgid;
-	dst->check = src->check;
-	dst->init = src->init;
-	dst->free = src->free;
-	dst->msgtable = src->msgtable;
-	dst->msgtable_size = src->msgtable_size;
-	dst->handle = NULL;
+
 	src->refcount++;
 }
+
+int init_checkers(const char *multipath_dir)
+{
+	if (!add_checker_class(multipath_dir, DEFAULT_CHECKER))
+		return 1;
+	return 0;
+}
diff --git a/libmultipath/checkers.h b/libmultipath/checkers.h
index 98fec2c4..c46f7ffa 100644
--- a/libmultipath/checkers.h
+++ b/libmultipath/checkers.h
@@ -116,32 +116,22 @@ enum {
 	CHECKER_MSGTABLE_SIZE = 100,	/* max msg table size for checkers */
 };
 
+struct checker_class;
 struct checker {
-	struct list_head node;
-	void *handle;
-	int refcount;
+	struct checker_class *cls;
 	int fd;
-	int sync;
 	unsigned int timeout;
 	int disable;
-	char name[CHECKER_NAME_LEN];
 	short msgid;		             /* checker-internal extra status */
 	void * context;                      /* store for persistent data */
 	void ** mpcontext;                   /* store for persistent data shared
 						multipath-wide. Use MALLOC if
 						you want to stuff data in. */
-	int (*check)(struct checker *);
-	int (*init)(struct checker *);       /* to allocate the context */
-	void (*free)(struct checker *);      /* to free the context */
-	const char**msgtable;
-	short msgtable_size;
 };
 
-char * checker_state_name (int);
-int init_checkers (char *);
+const char *checker_state_name(int);
+int init_checkers(const char *);
 void cleanup_checkers (void);
-struct checker * add_checker (char *, char *);
-struct checker * checker_lookup (char *);
 int checker_init (struct checker *, void **);
 void checker_clear (struct checker *);
 void checker_put (struct checker *);
@@ -152,11 +142,12 @@ void checker_set_fd (struct checker *, int);
 void checker_enable (struct checker *);
 void checker_disable (struct checker *);
 int checker_check (struct checker *, int);
-int checker_selected (struct checker *);
+int checker_selected(const struct checker *);
+int checker_is_sync(const struct checker *);
 const char *checker_name (const struct checker *);
 const char *checker_message (const struct checker *);
 void checker_clear_message (struct checker *c);
-void checker_get (char *, struct checker *, char *);
+void checker_get(const char *, struct checker *, const char *);
 
 /* Prototypes for symbols exported by path checker dynamic libraries (.so) */
 int libcheck_check(struct checker *);
diff --git a/libmultipath/checkers/directio.c b/libmultipath/checkers/directio.c
index c4a0712e..1b00b775 100644
--- a/libmultipath/checkers/directio.c
+++ b/libmultipath/checkers/directio.c
@@ -202,7 +202,7 @@ int libcheck_check (struct checker * c)
 	if (!ct)
 		return PATH_UNCHECKED;
 
-	ret = check_state(c->fd, ct, c->sync, c->timeout);
+	ret = check_state(c->fd, ct, checker_is_sync(c), c->timeout);
 
 	switch (ret)
 	{
diff --git a/libmultipath/checkers/tur.c b/libmultipath/checkers/tur.c
index 8f4bdc8b..05e7bed6 100644
--- a/libmultipath/checkers/tur.c
+++ b/libmultipath/checkers/tur.c
@@ -323,7 +323,7 @@ int libcheck_check(struct checker * c)
 	if (!ct)
 		return PATH_UNCHECKED;
 
-	if (c->sync)
+	if (checker_is_sync(c))
 		return tur_check(c->fd, c->timeout, &c->msgid);
 
 	/*
diff --git a/libmultipath/print.c b/libmultipath/print.c
index 7b610b94..fc40b0f0 100644
--- a/libmultipath/print.c
+++ b/libmultipath/print.c
@@ -615,7 +615,7 @@ static int
 snprint_path_checker (char * buff, size_t len, const struct path * pp)
 {
 	const struct checker * c = &pp->checker;
-	return snprint_str(buff, len, c->name);
+	return snprint_str(buff, len, checker_name(c));
 }
 
 static int
diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
index fdb5953a..970a3b5c 100644
--- a/libmultipath/propsel.c
+++ b/libmultipath/propsel.c
@@ -479,26 +479,27 @@ check_rdac(struct path * pp)
 int select_checker(struct config *conf, struct path *pp)
 {
 	const char *origin;
-	char *checker_name;
+	char *ckr_name;
 	struct checker * c = &pp->checker;
 
 	if (pp->detect_checker == DETECT_CHECKER_ON) {
 		origin = autodetect_origin;
 		if (check_rdac(pp)) {
-			checker_name = RDAC;
+			ckr_name = RDAC;
 			goto out;
 		} else if (pp->tpgs > 0) {
-			checker_name = TUR;
+			ckr_name = TUR;
 			goto out;
 		}
 	}
-	do_set(checker_name, conf->overrides, checker_name, overrides_origin);
-	do_set_from_hwe(checker_name, pp, checker_name, hwe_origin);
-	do_set(checker_name, conf, checker_name, conf_origin);
-	do_default(checker_name, DEFAULT_CHECKER);
+	do_set(checker_name, conf->overrides, ckr_name, overrides_origin);
+	do_set_from_hwe(checker_name, pp, ckr_name, hwe_origin);
+	do_set(checker_name, conf, ckr_name, conf_origin);
+	do_default(ckr_name, DEFAULT_CHECKER);
 out:
-	checker_get(conf->multipath_dir, c, checker_name);
-	condlog(3, "%s: path_checker = %s %s", pp->dev, c->name, origin);
+	checker_get(conf->multipath_dir, c, ckr_name);
+	condlog(3, "%s: path_checker = %s %s", pp->dev,
+		checker_name(c), origin);
 	if (conf->checker_timeout) {
 		c->timeout = conf->checker_timeout;
 		condlog(3, "%s: checker timeout = %u s %s",
-- 
2.19.0

Re: [PATCH 21/21] libmultipath/checkers: cleanup class/instance model

[Benjamin Marzinski]

On Fri, Oct 12, 2018 at 12:27:07AM +0200, Martin Wilck wrote:
> The checkers code implicitly uses a sort-of OOP class/instance model,
> but very clumsily. Separate the checker "class" and "instance" cleanly,
> and do a few further cleanups (constifications etc) on the way.
> 

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

> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/checkers.c          | 137 ++++++++++++++++---------------
>  libmultipath/checkers.h          |  23 ++----
>  libmultipath/checkers/directio.c |   2 +-
>  libmultipath/checkers/tur.c      |   2 +-
>  libmultipath/print.c             |   2 +-
>  libmultipath/propsel.c           |  19 +++--
>  6 files changed, 92 insertions(+), 93 deletions(-)
> 
> diff --git a/libmultipath/checkers.c b/libmultipath/checkers.c
> index 19c8ad58..7733471c 100644
> --- a/libmultipath/checkers.c
> +++ b/libmultipath/checkers.c
> @@ -8,6 +8,19 @@
>  #include "checkers.h"
>  #include "vector.h"
>  
> +struct checker_class {
> +	struct list_head node;
> +	void *handle;
> +	int refcount;
> +	int sync;
> +	char name[CHECKER_NAME_LEN];
> +	int (*check)(struct checker *);
> +	int (*init)(struct checker *);       /* to allocate the context */
> +	void (*free)(struct checker *);      /* to free the context */
> +	const char **msgtable;
> +	short msgtable_size;
> +};
> +
>  char *checker_state_names[] = {
>  	"wild",
>  	"unchecked",
> @@ -23,38 +36,30 @@ char *checker_state_names[] = {
>  
>  static LIST_HEAD(checkers);
>  
> -char * checker_state_name (int i)
> +const char *checker_state_name(int i)
>  {
>  	return checker_state_names[i];
>  }
>  
> -int init_checkers (char *multipath_dir)
> -{
> -	if (!add_checker(multipath_dir, DEFAULT_CHECKER))
> -		return 1;
> -	return 0;
> -}
> -
> -struct checker * alloc_checker (void)
> +static struct checker_class *alloc_checker_class(void)
>  {
> -	struct checker *c;
> +	struct checker_class *c;
>  
> -	c = MALLOC(sizeof(struct checker));
> +	c = MALLOC(sizeof(struct checker_class));
>  	if (c) {
>  		INIT_LIST_HEAD(&c->node);
>  		c->refcount = 1;
> -		c->fd = -1;
>  	}
>  	return c;
>  }
>  
> -void free_checker (struct checker * c)
> +void free_checker_class(struct checker_class *c)
>  {
>  	if (!c)
>  		return;
>  	c->refcount--;
>  	if (c->refcount) {
> -		condlog(3, "%s checker refcount %d",
> +		condlog(4, "%s checker refcount %d",
>  			c->name, c->refcount);
>  		return;
>  	}
> @@ -71,17 +76,17 @@ void free_checker (struct checker * c)
>  
>  void cleanup_checkers (void)
>  {
> -	struct checker * checker_loop;
> -	struct checker * checker_temp;
> +	struct checker_class *checker_loop;
> +	struct checker_class *checker_temp;
>  
>  	list_for_each_entry_safe(checker_loop, checker_temp, &checkers, node) {
> -		free_checker(checker_loop);
> +		free_checker_class(checker_loop);
>  	}
>  }
>  
> -struct checker * checker_lookup (char * name)
> +static struct checker_class *checker_class_lookup(const char *name)
>  {
> -	struct checker * c;
> +	struct checker_class *c;
>  
>  	if (!name || !strlen(name))
>  		return NULL;
> @@ -92,14 +97,15 @@ struct checker * checker_lookup (char * name)
>  	return NULL;
>  }
>  
> -struct checker * add_checker (char *multipath_dir, char * name)
> +static struct checker_class *add_checker_class(const char *multipath_dir,
> +					       const char *name)
>  {
>  	char libname[LIB_CHECKER_NAMELEN];
>  	struct stat stbuf;
> -	struct checker * c;
> +	struct checker_class *c;
>  	char *errstr;
>  
> -	c = alloc_checker();
> +	c = alloc_checker_class();
>  	if (!c)
>  		return NULL;
>  	snprintf(c->name, CHECKER_NAME_LEN, "%s", name);
> @@ -158,12 +164,11 @@ struct checker * add_checker (char *multipath_dir, char * name)
>  		c->name, c->msgtable_size);
>  
>  done:
> -	c->fd = -1;
>  	c->sync = 1;
>  	list_add(&c->node, &checkers);
>  	return c;
>  out:
> -	free_checker(c);
> +	free_checker_class(c);
>  	return NULL;
>  }
>  
> @@ -176,16 +181,16 @@ void checker_set_fd (struct checker * c, int fd)
>  
>  void checker_set_sync (struct checker * c)
>  {
> -	if (!c)
> +	if (!c || !c->cls)
>  		return;
> -	c->sync = 1;
> +	c->cls->sync = 1;
>  }
>  
>  void checker_set_async (struct checker * c)
>  {
> -	if (!c)
> +	if (!c || !c->cls)
>  		return;
> -	c->sync = 0;
> +	c->cls->sync = 0;
>  }
>  
>  void checker_enable (struct checker * c)
> @@ -204,11 +209,11 @@ void checker_disable (struct checker * c)
>  
>  int checker_init (struct checker * c, void ** mpctxt_addr)
>  {
> -	if (!c)
> +	if (!c || !c->cls)
>  		return 1;
>  	c->mpcontext = mpctxt_addr;
> -	if (c->init)
> -		return c->init(c);
> +	if (c->cls->init)
> +		return c->cls->init(c);
>  	return 0;
>  }
>  
> @@ -220,15 +225,16 @@ void checker_clear (struct checker *c)
>  
>  void checker_put (struct checker * dst)
>  {
> -	struct checker * src;
> +	struct checker_class *src;
>  
> -	if (!dst || !strlen(dst->name))
> +	if (!dst)
>  		return;
> -	src = checker_lookup(dst->name);
> -	if (dst->free)
> -		dst->free(dst);
> +	src = dst->cls;
> +
> +	if (src && src->free)
> +		src->free(dst);
>  	checker_clear(dst);
> -	free_checker(src);
> +	free_checker_class(src);
>  }
>  
>  int checker_check (struct checker * c, int path_state)
> @@ -243,32 +249,35 @@ int checker_check (struct checker * c, int path_state)
>  		c->msgid = CHECKER_MSGID_DISABLED;
>  		return PATH_UNCHECKED;
>  	}
> -	if (!strncmp(c->name, NONE, 4))
> +	if (!strncmp(c->cls->name, NONE, 4))
>  		return path_state;
>  
>  	if (c->fd < 0) {
>  		c->msgid = CHECKER_MSGID_NO_FD;
>  		return PATH_WILD;
>  	}
> -	r = c->check(c);
> +	r = c->cls->check(c);
>  
>  	return r;
>  }
>  
> -int checker_selected (struct checker * c)
> +int checker_selected(const struct checker *c)
>  {
>  	if (!c)
>  		return 0;
> -	if (!strncmp(c->name, NONE, 4))
> -		return 1;
> -	return (c->check) ? 1 : 0;
> +	return c->cls != NULL;
>  }
>  
>  const char *checker_name(const struct checker *c)
>  {
> -	if (!c)
> +	if (!c || !c->cls)
>  		return NULL;
> -	return c->name;
> +	return c->cls->name;
> +}
> +
> +int checker_is_sync(const struct checker *c)
> +{
> +	return c && c->cls && c->cls->sync;
>  }
>  
>  static const char *generic_msg[CHECKER_LAST_GENERIC_MSGID] = {
> @@ -295,8 +304,8 @@ static const char *_checker_message(const struct checker *c)
>  		return generic_msg[c->msgid];
>  
>  	id = c->msgid - CHECKER_FIRST_MSGID;
> -	if (id < c->msgtable_size)
> -		return c->msgtable[id];
> +	if (id < c->cls->msgtable_size)
> +		return c->cls->msgtable[id];
>  	return NULL;
>  }
>  
> @@ -309,7 +318,7 @@ const char *checker_message(const struct checker *c)
>  		*buf = '\0';
>  	else
>  		snprintf(buf, sizeof(buf), "%s checker%s",
> -			 c->name, msg);
> +			 c->cls->name, msg);
>  	return buf;
>  }
>  
> @@ -320,31 +329,29 @@ void checker_clear_message (struct checker *c)
>  	c->msgid = CHECKER_MSGID_NONE;
>  }
>  
> -void checker_get (char *multipath_dir, struct checker * dst, char * name)
> +void checker_get(const char *multipath_dir, struct checker *dst,
> +		 const char *name)
>  {
> -	struct checker * src = NULL;
> +	struct checker_class *src = NULL;
>  
>  	if (!dst)
>  		return;
>  
>  	if (name && strlen(name)) {
> -		src = checker_lookup(name);
> +		src = checker_class_lookup(name);
>  		if (!src)
> -			src = add_checker(multipath_dir, name);
> +			src = add_checker_class(multipath_dir, name);
>  	}
> -	if (!src) {
> -		dst->check = NULL;
> +	dst->cls = src;
> +	if (!src)
>  		return;
> -	}
> -	dst->fd = src->fd;
> -	dst->sync = src->sync;
> -	strncpy(dst->name, src->name, CHECKER_NAME_LEN);
> -	dst->msgid = src->msgid;
> -	dst->check = src->check;
> -	dst->init = src->init;
> -	dst->free = src->free;
> -	dst->msgtable = src->msgtable;
> -	dst->msgtable_size = src->msgtable_size;
> -	dst->handle = NULL;
> +
>  	src->refcount++;
>  }
> +
> +int init_checkers(const char *multipath_dir)
> +{
> +	if (!add_checker_class(multipath_dir, DEFAULT_CHECKER))
> +		return 1;
> +	return 0;
> +}
> diff --git a/libmultipath/checkers.h b/libmultipath/checkers.h
> index 98fec2c4..c46f7ffa 100644
> --- a/libmultipath/checkers.h
> +++ b/libmultipath/checkers.h
> @@ -116,32 +116,22 @@ enum {
>  	CHECKER_MSGTABLE_SIZE = 100,	/* max msg table size for checkers */
>  };
>  
> +struct checker_class;
>  struct checker {
> -	struct list_head node;
> -	void *handle;
> -	int refcount;
> +	struct checker_class *cls;
>  	int fd;
> -	int sync;
>  	unsigned int timeout;
>  	int disable;
> -	char name[CHECKER_NAME_LEN];
>  	short msgid;		             /* checker-internal extra status */
>  	void * context;                      /* store for persistent data */
>  	void ** mpcontext;                   /* store for persistent data shared
>  						multipath-wide. Use MALLOC if
>  						you want to stuff data in. */
> -	int (*check)(struct checker *);
> -	int (*init)(struct checker *);       /* to allocate the context */
> -	void (*free)(struct checker *);      /* to free the context */
> -	const char**msgtable;
> -	short msgtable_size;
>  };
>  
> -char * checker_state_name (int);
> -int init_checkers (char *);
> +const char *checker_state_name(int);
> +int init_checkers(const char *);
>  void cleanup_checkers (void);
> -struct checker * add_checker (char *, char *);
> -struct checker * checker_lookup (char *);
>  int checker_init (struct checker *, void **);
>  void checker_clear (struct checker *);
>  void checker_put (struct checker *);
> @@ -152,11 +142,12 @@ void checker_set_fd (struct checker *, int);
>  void checker_enable (struct checker *);
>  void checker_disable (struct checker *);
>  int checker_check (struct checker *, int);
> -int checker_selected (struct checker *);
> +int checker_selected(const struct checker *);
> +int checker_is_sync(const struct checker *);
>  const char *checker_name (const struct checker *);
>  const char *checker_message (const struct checker *);
>  void checker_clear_message (struct checker *c);
> -void checker_get (char *, struct checker *, char *);
> +void checker_get(const char *, struct checker *, const char *);
>  
>  /* Prototypes for symbols exported by path checker dynamic libraries (.so) */
>  int libcheck_check(struct checker *);
> diff --git a/libmultipath/checkers/directio.c b/libmultipath/checkers/directio.c
> index c4a0712e..1b00b775 100644
> --- a/libmultipath/checkers/directio.c
> +++ b/libmultipath/checkers/directio.c
> @@ -202,7 +202,7 @@ int libcheck_check (struct checker * c)
>  	if (!ct)
>  		return PATH_UNCHECKED;
>  
> -	ret = check_state(c->fd, ct, c->sync, c->timeout);
> +	ret = check_state(c->fd, ct, checker_is_sync(c), c->timeout);
>  
>  	switch (ret)
>  	{
> diff --git a/libmultipath/checkers/tur.c b/libmultipath/checkers/tur.c
> index 8f4bdc8b..05e7bed6 100644
> --- a/libmultipath/checkers/tur.c
> +++ b/libmultipath/checkers/tur.c
> @@ -323,7 +323,7 @@ int libcheck_check(struct checker * c)
>  	if (!ct)
>  		return PATH_UNCHECKED;
>  
> -	if (c->sync)
> +	if (checker_is_sync(c))
>  		return tur_check(c->fd, c->timeout, &c->msgid);
>  
>  	/*
> diff --git a/libmultipath/print.c b/libmultipath/print.c
> index 7b610b94..fc40b0f0 100644
> --- a/libmultipath/print.c
> +++ b/libmultipath/print.c
> @@ -615,7 +615,7 @@ static int
>  snprint_path_checker (char * buff, size_t len, const struct path * pp)
>  {
>  	const struct checker * c = &pp->checker;
> -	return snprint_str(buff, len, c->name);
> +	return snprint_str(buff, len, checker_name(c));
>  }
>  
>  static int
> diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
> index fdb5953a..970a3b5c 100644
> --- a/libmultipath/propsel.c
> +++ b/libmultipath/propsel.c
> @@ -479,26 +479,27 @@ check_rdac(struct path * pp)
>  int select_checker(struct config *conf, struct path *pp)
>  {
>  	const char *origin;
> -	char *checker_name;
> +	char *ckr_name;
>  	struct checker * c = &pp->checker;
>  
>  	if (pp->detect_checker == DETECT_CHECKER_ON) {
>  		origin = autodetect_origin;
>  		if (check_rdac(pp)) {
> -			checker_name = RDAC;
> +			ckr_name = RDAC;
>  			goto out;
>  		} else if (pp->tpgs > 0) {
> -			checker_name = TUR;
> +			ckr_name = TUR;
>  			goto out;
>  		}
>  	}
> -	do_set(checker_name, conf->overrides, checker_name, overrides_origin);
> -	do_set_from_hwe(checker_name, pp, checker_name, hwe_origin);
> -	do_set(checker_name, conf, checker_name, conf_origin);
> -	do_default(checker_name, DEFAULT_CHECKER);
> +	do_set(checker_name, conf->overrides, ckr_name, overrides_origin);
> +	do_set_from_hwe(checker_name, pp, ckr_name, hwe_origin);
> +	do_set(checker_name, conf, ckr_name, conf_origin);
> +	do_default(ckr_name, DEFAULT_CHECKER);
>  out:
> -	checker_get(conf->multipath_dir, c, checker_name);
> -	condlog(3, "%s: path_checker = %s %s", pp->dev, c->name, origin);
> +	checker_get(conf->multipath_dir, c, ckr_name);
> +	condlog(3, "%s: path_checker = %s %s", pp->dev,
> +		checker_name(c), origin);
>  	if (conf->checker_timeout) {
>  		c->timeout = conf->checker_timeout;
>  		condlog(3, "%s: checker timeout = %u s %s",
> -- 
> 2.19.0