[PATCH 1/4] multipathd: don't flood system with sd_notify calls

[PATCH 1/4] multipathd: don't flood system with sd_notify calls

[Martin Wilck]

DAEMON_RUNNING is only used by checkerloop to indicate that
path checkers are running. checkerloop switches back and forth
between DAEMON_IDLE and DAEMON_RUNNING in every iteration, i.e.
it performs two status changes per second on an idle system.
The repeated sd_notify() calls cause a lot of traffic on dbus as
systemd forwards these messages on the system bus. This can be
seen with systemd.log_level=debug. Better skip these notifications
that don't provide useful information to the user, who is very
unlikely to catch the daemon in "running" state anyway.

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

diff --git a/multipathd/main.c b/multipathd/main.c
index 4be2c579..b5e2b00d 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -161,15 +161,27 @@ sd_notify_status(void)
 	case DAEMON_CONFIGURE:
 		return "STATUS=configure";
 	case DAEMON_IDLE:
-		return "STATUS=idle";
 	case DAEMON_RUNNING:
-		return "STATUS=running";
+		return "STATUS=up";
 	case DAEMON_SHUTDOWN:
 		return "STATUS=shutdown";
 	}
 	return NULL;
 }
 
+static void do_sd_notify(enum daemon_status old_state)
+{
+	/*
+	 * Checkerloop switches back and forth between idle and running state.
+	 * No need to tell systemd each time.
+	 * These notifications cause a lot of overhead on dbus.
+	 */
+	if ((running_state == DAEMON_IDLE || running_state == DAEMON_RUNNING) &&
+	    (old_state == DAEMON_IDLE || old_state == DAEMON_RUNNING))
+		return;
+	sd_notify(0, sd_notify_status());
+}
+
 static void config_cleanup(void *arg)
 {
 	pthread_mutex_unlock(&config_lock);
@@ -179,10 +191,12 @@ void post_config_state(enum daemon_status state)
 {
 	pthread_mutex_lock(&config_lock);
 	if (state != running_state) {
+		enum daemon_status old_state = running_state;
+
 		running_state = state;
 		pthread_cond_broadcast(&config_cond);
 #ifdef USE_SYSTEMD
-		sd_notify(0, sd_notify_status());
+		do_sd_notify(old_state);
 #endif
 	}
 	pthread_mutex_unlock(&config_lock);
@@ -195,6 +209,8 @@ int set_config_state(enum daemon_status state)
 	pthread_cleanup_push(config_cleanup, NULL);
 	pthread_mutex_lock(&config_lock);
 	if (running_state != state) {
+		enum daemon_status old_state = running_state;
+
 		if (running_state != DAEMON_IDLE) {
 			struct timespec ts;
 
@@ -207,7 +223,7 @@ int set_config_state(enum daemon_status state)
 			running_state = state;
 			pthread_cond_broadcast(&config_cond);
 #ifdef USE_SYSTEMD
-			sd_notify(0, sd_notify_status());
+			do_sd_notify(old_state);
 #endif
 		}
 	}
-- 
2.14.0

[PATCH 2/4] libmultipath: add_feature: skip pointless NULL check

[Martin Wilck]

The case f == NULL is already handled by the code from
b1ecdd46b6ec "segment faulty occured in add_feature()" above.

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

diff --git a/libmultipath/structs.c b/libmultipath/structs.c
index 28704676..11e33676 100644
--- a/libmultipath/structs.c
+++ b/libmultipath/structs.c
@@ -515,7 +515,7 @@ void setup_feature(struct multipath *mpp, char *feature)
 
 int add_feature(char **f, const char *n)
 {
-	int c = 0, d, l = 0;
+	int c = 0, d, l;
 	char *e, *p, *t;
 	const char *q;
 
@@ -538,19 +538,18 @@ int add_feature(char **f, const char *n)
 	}
 
 	/* Check if feature is already present */
-	if (*f) {
-		if (strstr(*f, n))
-			return 0;
+	if (strstr(*f, n))
+		return 0;
 
-		/* Get feature count */
-		c = strtoul(*f, &e, 10);
-		if (*f == e)
-			/* parse error */
-			return 1;
+	/* Get feature count */
+	c = strtoul(*f, &e, 10);
+	if (*f == e)
+		/* parse error */
+		return 1;
+
+	/* Check if we need to increase feature count space */
+	l = strlen(*f) + strlen(n) + 1;
 
-		/* Check if we need to increase feature count space */
-		l = strlen(*f) + strlen(n) + 1;
-	}
 	/* Count new features */
 	if ((c % 10) == 9)
 		l++;
@@ -582,9 +581,7 @@ int add_feature(char **f, const char *n)
 	snprintf(p, l + 2, "%0d ", c);
 
 	/* Copy the feature string */
-	p = NULL;
-	if (*f)
-		p = strchr(*f, ' ');
+	p = strchr(*f, ' ');
 
 	if (p) {
 		while (*p == ' ')
-- 
2.14.0

[PATCH 3/4] libmultipath: add_feature: allow only 1 feature

[Martin Wilck]

The existing test "if feature is already present" doesn't work for
multiple features, and we are only using add_feature() for single
feature additions anyway. Simplify the code by not allowing spaces
in the feature string to be added. This way we can drop the
complex "count new features" code. Moreover, print an error message if
the existing features string is malformed.

The old code calculated the width of the feature count twice, and the
second time messed up the buffer length field passed to snprintf(); fix
that, too.  Finally, replace several strcat() invocations by one
strncpy() call to make the code easier to review.

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

diff --git a/libmultipath/structs.c b/libmultipath/structs.c
index 11e33676..828e7907 100644
--- a/libmultipath/structs.c
+++ b/libmultipath/structs.c
@@ -516,8 +516,7 @@ void setup_feature(struct multipath *mpp, char *feature)
 int add_feature(char **f, const char *n)
 {
 	int c = 0, d, l;
-	char *e, *p, *t;
-	const char *q;
+	char *e, *t;
 
 	if (!f)
 		return 1;
@@ -526,6 +525,11 @@ int add_feature(char **f, const char *n)
 	if (!n || *n == '0')
 		return 0;
 
+	if (strchr(n, ' ') != NULL) {
+		condlog(0, "internal error: feature \"%s\" contains spaces", n);
+		return 1;
+	}
+
 	/* default feature is null */
 	if(!*f)
 	{
@@ -543,55 +547,29 @@ int add_feature(char **f, const char *n)
 
 	/* Get feature count */
 	c = strtoul(*f, &e, 10);
-	if (*f == e)
-		/* parse error */
+	if (*f == e || (*e != ' ' && *e != '\0')) {
+		condlog(0, "parse error in feature string \"%s\"", *f);
 		return 1;
-
-	/* Check if we need to increase feature count space */
-	l = strlen(*f) + strlen(n) + 1;
-
-	/* Count new features */
-	if ((c % 10) == 9)
-		l++;
-	c++;
-	q = n;
-	while (*q != '\0') {
-		if (*q == ' ' && q[1] != '\0' && q[1] != ' ') {
-			if ((c % 10) == 9)
-				l++;
-			c++;
-		}
-		q++;
 	}
 
+	/* Add 1 digit and 1 space */
+	l = strlen(e) + strlen(n) + 2;
+
+	c++;
+	/* Check if we need more digits for feature count */
+	for (d = c; d >= 10; d /= 10)
+		l++;
+
 	t = MALLOC(l + 1);
 	if (!t)
 		return 1;
 
-	memset(t, 0, l + 1);
+	/* e: old feature string with leading space, or "" */
+	if (*e == ' ')
+		while (*(e + 1) == ' ')
+			e++;
 
-	/* Update feature count */
-	d = c;
-	l = 1;
-	while (d > 9) {
-		d /= 10;
-		l++;
-	}
-	p = t;
-	snprintf(p, l + 2, "%0d ", c);
-
-	/* Copy the feature string */
-	p = strchr(*f, ' ');
-
-	if (p) {
-		while (*p == ' ')
-			p++;
-		strcat(t, p);
-		strcat(t, " ");
-	} else {
-		p = t + strlen(t);
-	}
-	strcat(t, n);
+	snprintf(t, l + 1, "%0d%s %s", c, e, n);
 
 	FREE(*f);
 	*f = t;
-- 
2.14.0

[PATCH 4/4] multipath: delegate dangerous commands to multipathd

[Martin Wilck]

Some multipath commands are dangerous to run while multipathd is running.
For example, "multipath -r" may apply a modified configuration to the kernel,
while multipathd is still using the old configuration, leading to
inconsistent state between multipathd and the kernel.

It is safer to use equivalent multipathd client commands instead.
For now, do this only for "multipath -r", but other invocations
may be added in the future. Perhaps some day, all "multipath"
commands will be mapped to multipathd actions.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 Makefile.inc     |  2 +-
 multipath/main.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 74 insertions(+), 1 deletion(-)

diff --git a/Makefile.inc b/Makefile.inc
index 29c290a2..d012b3d7 100644
--- a/Makefile.inc
+++ b/Makefile.inc
@@ -90,7 +90,7 @@ OPTFLAGS	= -O2 -g -pipe -Wall -Wextra -Wformat=2 -Werror=implicit-int \
 		  -Wp,-D_FORTIFY_SOURCE=2 $(STACKPROT) \
 		  --param=ssp-buffer-size=4
 
-CFLAGS		= $(OPTFLAGS) -DLIB_STRING=\"${LIB}\" -DRUN_DIR=\"${RUN}\"
+CFLAGS		= $(OPTFLAGS) -D BIN_DIR=\"$(bindir)\" -DLIB_STRING=\"${LIB}\" -DRUN_DIR=\"${RUN}\"
 BIN_CFLAGS	= -fPIE -DPIE
 LIB_CFLAGS	= -fPIC
 SHARED_FLAGS	= -shared
diff --git a/multipath/main.c b/multipath/main.c
index dede017e..b54aa9bb 100644
--- a/multipath/main.c
+++ b/multipath/main.c
@@ -502,6 +502,74 @@ get_dev_type(char *dev) {
 	return DEV_NONE;
 }
 
+/*
+ * Some multipath commands are dangerous to run while multipathd is running.
+ * For example, "multipath -r" may apply a modified configuration to the kernel,
+ * while multipathd is still using the old configuration, leading to
+ * inconsistent state.
+ *
+ * It is safer to use equivalent multipathd client commands instead.
+ */
+int delegate_to_multipathd(enum mpath_cmds cmd, const struct config *conf)
+{
+#define DELEGATE_MAX_ARGS 5
+	int fd = mpath_connect();
+	char mpd_path[PATH_MAX];
+	char *argv[DELEGATE_MAX_ARGS];
+	char log[4096], *p;
+	char verbosity[2] = { '0', '\0' };
+	int idx = 1, cmd_idx, sz, n, i;
+
+	if (fd == -1)
+		return 0;
+	close(fd);
+
+	if (conf->verbosity > 0 && conf->verbosity < 8)
+		verbosity[0] = '0' + conf->verbosity;
+	argv[idx++] = "-v";
+	argv[idx++] = verbosity;
+
+	cmd_idx = idx;
+	if (cmd == CMD_CREATE && conf->force_reload == FORCE_RELOAD_YES) {
+		argv[idx++] = "reconfigure";
+	} /* Add other translations here */
+
+	if (idx == cmd_idx)
+		/* No command found, no need to delegate */
+		return 0;
+	else if (idx >= DELEGATE_MAX_ARGS) {
+		condlog(0, "internal error - argv overflow");
+		exit(1);
+	}
+
+	argv[idx] = NULL;
+	snprintf(mpd_path, sizeof(mpd_path), "%s/%s", BIN_DIR, "multipathd");
+	argv[0] = mpd_path;
+
+	condlog(1, "delegating command to multipathd");
+
+	sz = sizeof(log);
+	p = log;
+	i = 0;
+	n = snprintf(p, sz, "command: %s", argv[i++]);
+	while (i < idx && n < sz) {
+		sz -= n;
+		p += n;
+		n = snprintf(p, sz, " %s", argv[i++]);
+	}
+	condlog(2, "%s", log);
+	if (n >= sz)
+		condlog(2, "(command on previous line was truncated)");
+
+	if (execv(mpd_path, argv) == -1) {
+		condlog(0, "failed to execute multipathd: %s", strerror(errno));
+		exit(1);
+	}
+
+	/* not reached */
+	return 1;
+}
+
 int
 main (int argc, char *argv[])
 {
@@ -695,10 +763,15 @@ main (int argc, char *argv[])
 		} else
 			mpath_disconnect(fd);
 	}
+
 	if (cmd == CMD_REMOVE_WWID && !dev) {
 		condlog(0, "the -w option requires a device");
 		goto out;
 	}
+
+	if (delegate_to_multipathd(cmd, conf))
+		exit(0); /* not reached */
+
 	if (cmd == CMD_RESET_WWIDS) {
 		struct multipath * mpp;
 		int i;
-- 
2.14.0

Re: [PATCH 1/4] multipathd: don't flood system with sd_notify calls

[Hannes Reinecke]

On 08/29/2017 12:05 AM, Martin Wilck wrote:
> DAEMON_RUNNING is only used by checkerloop to indicate that
> path checkers are running. checkerloop switches back and forth
> between DAEMON_IDLE and DAEMON_RUNNING in every iteration, i.e.
> it performs two status changes per second on an idle system.
> The repeated sd_notify() calls cause a lot of traffic on dbus as
> systemd forwards these messages on the system bus. This can be
> seen with systemd.log_level=debug. Better skip these notifications
> that don't provide useful information to the user, who is very
> unlikely to catch the daemon in "running" state anyway.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  multipathd/main.c | 24 ++++++++++++++++++++----
>  1 file changed, 20 insertions(+), 4 deletions(-)
> 
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 4be2c579..b5e2b00d 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -161,15 +161,27 @@ sd_notify_status(void)
>  	case DAEMON_CONFIGURE:
>  		return "STATUS=configure";
>  	case DAEMON_IDLE:
> -		return "STATUS=idle";
>  	case DAEMON_RUNNING:
> -		return "STATUS=running";
> +		return "STATUS=up";
>  	case DAEMON_SHUTDOWN:
>  		return "STATUS=shutdown";
>  	}
>  	return NULL;
>  }
>  
> +static void do_sd_notify(enum daemon_status old_state)
> +{
> +	/*
> +	 * Checkerloop switches back and forth between idle and running state.
> +	 * No need to tell systemd each time.
> +	 * These notifications cause a lot of overhead on dbus.
> +	 */
> +	if ((running_state == DAEMON_IDLE || running_state == DAEMON_RUNNING) &&
> +	    (old_state == DAEMON_IDLE || old_state == DAEMON_RUNNING))
> +		return;
> +	sd_notify(0, sd_notify_status());
> +}
> +
>  static void config_cleanup(void *arg)
>  {
>  	pthread_mutex_unlock(&config_lock);
> @@ -179,10 +191,12 @@ void post_config_state(enum daemon_status state)
>  {
>  	pthread_mutex_lock(&config_lock);
>  	if (state != running_state) {
> +		enum daemon_status old_state = running_state;
> +
>  		running_state = state;
>  		pthread_cond_broadcast(&config_cond);
>  #ifdef USE_SYSTEMD
> -		sd_notify(0, sd_notify_status());
> +		do_sd_notify(old_state);
>  #endif
>  	}
>  	pthread_mutex_unlock(&config_lock);
> @@ -195,6 +209,8 @@ int set_config_state(enum daemon_status state)
>  	pthread_cleanup_push(config_cleanup, NULL);
>  	pthread_mutex_lock(&config_lock);
>  	if (running_state != state) {
> +		enum daemon_status old_state = running_state;
> +
>  		if (running_state != DAEMON_IDLE) {
>  			struct timespec ts;
>  
> @@ -207,7 +223,7 @@ int set_config_state(enum daemon_status state)
>  			running_state = state;
>  			pthread_cond_broadcast(&config_cond);
>  #ifdef USE_SYSTEMD
> -			sd_notify(0, sd_notify_status());
> +			do_sd_notify(old_state);
>  #endif
>  		}
>  	}
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

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

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

Re: [PATCH 2/4] libmultipath: add_feature: skip pointless NULL check

[Hannes Reinecke]

On 08/29/2017 12:05 AM, Martin Wilck wrote:
> The case f == NULL is already handled by the code from
> b1ecdd46b6ec "segment faulty occured in add_feature()" above.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/structs.c | 27 ++++++++++++---------------
>  1 file changed, 12 insertions(+), 15 deletions(-)
> 
> diff --git a/libmultipath/structs.c b/libmultipath/structs.c
> index 28704676..11e33676 100644
> --- a/libmultipath/structs.c
> +++ b/libmultipath/structs.c
> @@ -515,7 +515,7 @@ void setup_feature(struct multipath *mpp, char *feature)
>  
>  int add_feature(char **f, const char *n)
>  {
> -	int c = 0, d, l = 0;
> +	int c = 0, d, l;
>  	char *e, *p, *t;
>  	const char *q;
>  
> @@ -538,19 +538,18 @@ int add_feature(char **f, const char *n)
>  	}
>  
>  	/* Check if feature is already present */
> -	if (*f) {
> -		if (strstr(*f, n))
> -			return 0;
> +	if (strstr(*f, n))
> +		return 0;
>  
> -		/* Get feature count */
> -		c = strtoul(*f, &e, 10);
> -		if (*f == e)
> -			/* parse error */
> -			return 1;
> +	/* Get feature count */
> +	c = strtoul(*f, &e, 10);
> +	if (*f == e)
> +		/* parse error */
> +		return 1;
> +
> +	/* Check if we need to increase feature count space */
> +	l = strlen(*f) + strlen(n) + 1;
>  
> -		/* Check if we need to increase feature count space */
> -		l = strlen(*f) + strlen(n) + 1;
> -	}
>  	/* Count new features */
>  	if ((c % 10) == 9)
>  		l++;
> @@ -582,9 +581,7 @@ int add_feature(char **f, const char *n)
>  	snprintf(p, l + 2, "%0d ", c);
>  
>  	/* Copy the feature string */
> -	p = NULL;
> -	if (*f)
> -		p = strchr(*f, ' ');
> +	p = strchr(*f, ' ');
>  
>  	if (p) {
>  		while (*p == ' ')
> 
At one point we should have a test case for adding/removing features...
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

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

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

Re: [PATCH 3/4] libmultipath: add_feature: allow only 1 feature

[Hannes Reinecke]

On 08/29/2017 12:05 AM, Martin Wilck wrote:
> The existing test "if feature is already present" doesn't work for
> multiple features, and we are only using add_feature() for single
> feature additions anyway. Simplify the code by not allowing spaces
> in the feature string to be added. This way we can drop the
> complex "count new features" code. Moreover, print an error message if
> the existing features string is malformed.
> 
> The old code calculated the width of the feature count twice, and the
> second time messed up the buffer length field passed to snprintf(); fix
> that, too.  Finally, replace several strcat() invocations by one
> strncpy() call to make the code easier to review.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/structs.c | 64 +++++++++++++++++---------------------------------
>  1 file changed, 21 insertions(+), 43 deletions(-)
> 
> diff --git a/libmultipath/structs.c b/libmultipath/structs.c
> index 11e33676..828e7907 100644
> --- a/libmultipath/structs.c
> +++ b/libmultipath/structs.c
> @@ -516,8 +516,7 @@ void setup_feature(struct multipath *mpp, char *feature)
>  int add_feature(char **f, const char *n)
>  {
>  	int c = 0, d, l;
> -	char *e, *p, *t;
> -	const char *q;
> +	char *e, *t;
>  
>  	if (!f)
>  		return 1;
> @@ -526,6 +525,11 @@ int add_feature(char **f, const char *n)
>  	if (!n || *n == '0')
>  		return 0;
>  
> +	if (strchr(n, ' ') != NULL) {
> +		condlog(0, "internal error: feature \"%s\" contains spaces", n);
> +		return 1;
> +	}
> +
>  	/* default feature is null */
>  	if(!*f)
>  	{
> @@ -543,55 +547,29 @@ int add_feature(char **f, const char *n)
>  
>  	/* Get feature count */
>  	c = strtoul(*f, &e, 10);
> -	if (*f == e)
> -		/* parse error */
> +	if (*f == e || (*e != ' ' && *e != '\0')) {
> +		condlog(0, "parse error in feature string \"%s\"", *f);
>  		return 1;
> -
> -	/* Check if we need to increase feature count space */
> -	l = strlen(*f) + strlen(n) + 1;
> -
> -	/* Count new features */
> -	if ((c % 10) == 9)
> -		l++;
> -	c++;
> -	q = n;
> -	while (*q != '\0') {
> -		if (*q == ' ' && q[1] != '\0' && q[1] != ' ') {
> -			if ((c % 10) == 9)
> -				l++;
> -			c++;
> -		}
> -		q++;
>  	}
>  
> +	/* Add 1 digit and 1 space */
> +	l = strlen(e) + strlen(n) + 2;
> +
> +	c++;
> +	/* Check if we need more digits for feature count */
> +	for (d = c; d >= 10; d /= 10)
> +		l++;
> +
>  	t = MALLOC(l + 1);
>  	if (!t)
>  		return 1;
>  
> -	memset(t, 0, l + 1);
> +	/* e: old feature string with leading space, or "" */
> +	if (*e == ' ')
> +		while (*(e + 1) == ' ')
> +			e++;
>  
> -	/* Update feature count */
> -	d = c;
> -	l = 1;
> -	while (d > 9) {
> -		d /= 10;
> -		l++;
> -	}
> -	p = t;
> -	snprintf(p, l + 2, "%0d ", c);
> -
> -	/* Copy the feature string */
> -	p = strchr(*f, ' ');
> -
> -	if (p) {
> -		while (*p == ' ')
> -			p++;
> -		strcat(t, p);
> -		strcat(t, " ");
> -	} else {
> -		p = t + strlen(t);
> -	}
> -	strcat(t, n);
> +	snprintf(t, l + 1, "%0d%s %s", c, e, n);
>  
>  	FREE(*f);
>  	*f = t;
> 
I did say we should have a testcase, right?

Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

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

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

Re: [PATCH 4/4] multipath: delegate dangerous commands to multipathd

[Hannes Reinecke]

On 08/29/2017 12:05 AM, Martin Wilck wrote:
> Some multipath commands are dangerous to run while multipathd is running.
> For example, "multipath -r" may apply a modified configuration to the kernel,
> while multipathd is still using the old configuration, leading to
> inconsistent state between multipathd and the kernel.
> 
> It is safer to use equivalent multipathd client commands instead.
> For now, do this only for "multipath -r", but other invocations
> may be added in the future. Perhaps some day, all "multipath"
> commands will be mapped to multipathd actions.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  Makefile.inc     |  2 +-
>  multipath/main.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 74 insertions(+), 1 deletion(-)
> 
> diff --git a/Makefile.inc b/Makefile.inc
> index 29c290a2..d012b3d7 100644
> --- a/Makefile.inc
> +++ b/Makefile.inc
> @@ -90,7 +90,7 @@ OPTFLAGS	= -O2 -g -pipe -Wall -Wextra -Wformat=2 -Werror=implicit-int \
>  		  -Wp,-D_FORTIFY_SOURCE=2 $(STACKPROT) \
>  		  --param=ssp-buffer-size=4
>  
> -CFLAGS		= $(OPTFLAGS) -DLIB_STRING=\"${LIB}\" -DRUN_DIR=\"${RUN}\"
> +CFLAGS		= $(OPTFLAGS) -D BIN_DIR=\"$(bindir)\" -DLIB_STRING=\"${LIB}\" -DRUN_DIR=\"${RUN}\"
>  BIN_CFLAGS	= -fPIE -DPIE
>  LIB_CFLAGS	= -fPIC
>  SHARED_FLAGS	= -shared
> diff --git a/multipath/main.c b/multipath/main.c
> index dede017e..b54aa9bb 100644
> --- a/multipath/main.c
> +++ b/multipath/main.c
> @@ -502,6 +502,74 @@ get_dev_type(char *dev) {
>  	return DEV_NONE;
>  }
>  
> +/*
> + * Some multipath commands are dangerous to run while multipathd is running.
> + * For example, "multipath -r" may apply a modified configuration to the kernel,
> + * while multipathd is still using the old configuration, leading to
> + * inconsistent state.
> + *
> + * It is safer to use equivalent multipathd client commands instead.
> + */
> +int delegate_to_multipathd(enum mpath_cmds cmd, const struct config *conf)
> +{
> +#define DELEGATE_MAX_ARGS 5
> +	int fd = mpath_connect();
> +	char mpd_path[PATH_MAX];
> +	char *argv[DELEGATE_MAX_ARGS];
> +	char log[4096], *p;
> +	char verbosity[2] = { '0', '\0' };
> +	int idx = 1, cmd_idx, sz, n, i;
> +
> +	if (fd == -1)
> +		return 0;
> +	close(fd);
> +
> +	if (conf->verbosity > 0 && conf->verbosity < 8)
> +		verbosity[0] = '0' + conf->verbosity;
> +	argv[idx++] = "-v";
> +	argv[idx++] = verbosity;
> +
> +	cmd_idx = idx;
> +	if (cmd == CMD_CREATE && conf->force_reload == FORCE_RELOAD_YES) {
> +		argv[idx++] = "reconfigure";
> +	} /* Add other translations here */
> +
> +	if (idx == cmd_idx)
> +		/* No command found, no need to delegate */
> +		return 0;
> +	else if (idx >= DELEGATE_MAX_ARGS) {
> +		condlog(0, "internal error - argv overflow");
> +		exit(1);
> +	}
> +
> +	argv[idx] = NULL;
> +	snprintf(mpd_path, sizeof(mpd_path), "%s/%s", BIN_DIR, "multipathd");
> +	argv[0] = mpd_path;
> +
> +	condlog(1, "delegating command to multipathd");
> +
> +	sz = sizeof(log);
> +	p = log;
> +	i = 0;
> +	n = snprintf(p, sz, "command: %s", argv[i++]);
> +	while (i < idx && n < sz) {
> +		sz -= n;
> +		p += n;
> +		n = snprintf(p, sz, " %s", argv[i++]);
> +	}
> +	condlog(2, "%s", log);
> +	if (n >= sz)
> +		condlog(2, "(command on previous line was truncated)");
> +
> +	if (execv(mpd_path, argv) == -1) {
> +		condlog(0, "failed to execute multipathd: %s", strerror(errno));
> +		exit(1);
> +	}
> +
> +	/* not reached */
> +	return 1;
> +}
> +
>  int
>  main (int argc, char *argv[])
>  {
> @@ -695,10 +763,15 @@ main (int argc, char *argv[])
>  		} else
>  			mpath_disconnect(fd);
>  	}
> +
>  	if (cmd == CMD_REMOVE_WWID && !dev) {
>  		condlog(0, "the -w option requires a device");
>  		goto out;
>  	}
> +
> +	if (delegate_to_multipathd(cmd, conf))
> +		exit(0); /* not reached */
> +
>  	if (cmd == CMD_RESET_WWIDS) {
>  		struct multipath * mpp;
>  		int i;
> 
Good idea.
We should be adding '-t' to it, too; after all, where's the point in
having multipath presenting you with a different table as the daemon is
using?

Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

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

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

Re: [PATCH 2/4] libmultipath: add_feature: skip pointless NULL check

[Martin Wilck]

On Tue, 2017-08-29 at 08:37 +0200, Hannes Reinecke wrote:
> On 08/29/2017 12:05 AM, Martin Wilck wrote:
> > The case f == NULL is already handled by the code from
> > b1ecdd46b6ec "segment faulty occured in add_feature()" above.
> > 
> > 
> At one point we should have a test case for adding/removing
> features...

I'm actually working on it.

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 1/4] multipathd: don't flood system with sd_notify calls

[Benjamin Marzinski]

On Tue, Aug 29, 2017 at 12:05:33AM +0200, Martin Wilck wrote:
> DAEMON_RUNNING is only used by checkerloop to indicate that
> path checkers are running. checkerloop switches back and forth
> between DAEMON_IDLE and DAEMON_RUNNING in every iteration, i.e.
> it performs two status changes per second on an idle system.
> The repeated sd_notify() calls cause a lot of traffic on dbus as
> systemd forwards these messages on the system bus. This can be
> seen with systemd.log_level=debug. Better skip these notifications
> that don't provide useful information to the user, who is very
> unlikely to catch the daemon in "running" state anyway.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  multipathd/main.c | 24 ++++++++++++++++++++----
>  1 file changed, 20 insertions(+), 4 deletions(-)
> 
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 4be2c579..b5e2b00d 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -161,15 +161,27 @@ sd_notify_status(void)
>  	case DAEMON_CONFIGURE:
>  		return "STATUS=configure";
>  	case DAEMON_IDLE:
> -		return "STATUS=idle";
>  	case DAEMON_RUNNING:
> -		return "STATUS=running";
> +		return "STATUS=up";
>  	case DAEMON_SHUTDOWN:
>  		return "STATUS=shutdown";
>  	}
>  	return NULL;
>  }
>  
> +static void do_sd_notify(enum daemon_status old_state)
> +{
> +	/*
> +	 * Checkerloop switches back and forth between idle and running state.
> +	 * No need to tell systemd each time.
> +	 * These notifications cause a lot of overhead on dbus.
> +	 */
> +	if ((running_state == DAEMON_IDLE || running_state == DAEMON_RUNNING) &&
> +	    (old_state == DAEMON_IDLE || old_state == DAEMON_RUNNING))
> +		return;
> +	sd_notify(0, sd_notify_status());
> +}
> +
>  static void config_cleanup(void *arg)
>  {
>  	pthread_mutex_unlock(&config_lock);
> @@ -179,10 +191,12 @@ void post_config_state(enum daemon_status state)
>  {
>  	pthread_mutex_lock(&config_lock);
>  	if (state != running_state) {
> +		enum daemon_status old_state = running_state;
> +
>  		running_state = state;
>  		pthread_cond_broadcast(&config_cond);
>  #ifdef USE_SYSTEMD
> -		sd_notify(0, sd_notify_status());
> +		do_sd_notify(old_state);
>  #endif
>  	}
>  	pthread_mutex_unlock(&config_lock);
> @@ -195,6 +209,8 @@ int set_config_state(enum daemon_status state)
>  	pthread_cleanup_push(config_cleanup, NULL);
>  	pthread_mutex_lock(&config_lock);
>  	if (running_state != state) {
> +		enum daemon_status old_state = running_state;
> +
>  		if (running_state != DAEMON_IDLE) {
>  			struct timespec ts;
>  
> @@ -207,7 +223,7 @@ int set_config_state(enum daemon_status state)
>  			running_state = state;
>  			pthread_cond_broadcast(&config_cond);
>  #ifdef USE_SYSTEMD
> -			sd_notify(0, sd_notify_status());
> +			do_sd_notify(old_state);
>  #endif
>  		}
>  	}
> -- 
> 2.14.0
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>

-Ben

Re: [PATCH 2/4] libmultipath: add_feature: skip pointless NULL check

[Benjamin Marzinski]

On Tue, Aug 29, 2017 at 12:05:34AM +0200, Martin Wilck wrote:
> The case f == NULL is already handled by the code from
> b1ecdd46b6ec "segment faulty occured in add_feature()" above.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/structs.c | 27 ++++++++++++---------------
>  1 file changed, 12 insertions(+), 15 deletions(-)
> 
> diff --git a/libmultipath/structs.c b/libmultipath/structs.c
> index 28704676..11e33676 100644
> --- a/libmultipath/structs.c
> +++ b/libmultipath/structs.c
> @@ -515,7 +515,7 @@ void setup_feature(struct multipath *mpp, char *feature)
>  
>  int add_feature(char **f, const char *n)
>  {
> -	int c = 0, d, l = 0;
> +	int c = 0, d, l;
>  	char *e, *p, *t;
>  	const char *q;
>  
> @@ -538,19 +538,18 @@ int add_feature(char **f, const char *n)
>  	}
>  
>  	/* Check if feature is already present */
> -	if (*f) {
> -		if (strstr(*f, n))
> -			return 0;
> +	if (strstr(*f, n))
> +		return 0;
>  
> -		/* Get feature count */
> -		c = strtoul(*f, &e, 10);
> -		if (*f == e)
> -			/* parse error */
> -			return 1;
> +	/* Get feature count */
> +	c = strtoul(*f, &e, 10);
> +	if (*f == e)
> +		/* parse error */
> +		return 1;
> +
> +	/* Check if we need to increase feature count space */
> +	l = strlen(*f) + strlen(n) + 1;
>  
> -		/* Check if we need to increase feature count space */
> -		l = strlen(*f) + strlen(n) + 1;
> -	}
>  	/* Count new features */
>  	if ((c % 10) == 9)
>  		l++;
> @@ -582,9 +581,7 @@ int add_feature(char **f, const char *n)
>  	snprintf(p, l + 2, "%0d ", c);
>  
>  	/* Copy the feature string */
> -	p = NULL;
> -	if (*f)
> -		p = strchr(*f, ' ');
> +	p = strchr(*f, ' ');
>  
>  	if (p) {
>  		while (*p == ' ')
> -- 
> 2.14.0
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>

-Ben

Re: [PATCH 3/4] libmultipath: add_feature: allow only 1 feature

[Benjamin Marzinski]

On Tue, Aug 29, 2017 at 12:05:35AM +0200, Martin Wilck wrote:
> The existing test "if feature is already present" doesn't work for
> multiple features, and we are only using add_feature() for single
> feature additions anyway. Simplify the code by not allowing spaces
> in the feature string to be added. This way we can drop the
> complex "count new features" code. Moreover, print an error message if
> the existing features string is malformed.
> 
> The old code calculated the width of the feature count twice, and the
> second time messed up the buffer length field passed to snprintf(); fix
> that, too.  Finally, replace several strcat() invocations by one
> strncpy() call to make the code easier to review.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/structs.c | 64 +++++++++++++++++---------------------------------
>  1 file changed, 21 insertions(+), 43 deletions(-)
> 
> diff --git a/libmultipath/structs.c b/libmultipath/structs.c
> index 11e33676..828e7907 100644
> --- a/libmultipath/structs.c
> +++ b/libmultipath/structs.c
> @@ -516,8 +516,7 @@ void setup_feature(struct multipath *mpp, char *feature)
>  int add_feature(char **f, const char *n)
>  {
>  	int c = 0, d, l;
> -	char *e, *p, *t;
> -	const char *q;
> +	char *e, *t;
>  
>  	if (!f)
>  		return 1;
> @@ -526,6 +525,11 @@ int add_feature(char **f, const char *n)
>  	if (!n || *n == '0')
>  		return 0;
>  
> +	if (strchr(n, ' ') != NULL) {
> +		condlog(0, "internal error: feature \"%s\" contains spaces", n);
> +		return 1;
> +	}
> +
>  	/* default feature is null */
>  	if(!*f)
>  	{
> @@ -543,55 +547,29 @@ int add_feature(char **f, const char *n)
>  
>  	/* Get feature count */
>  	c = strtoul(*f, &e, 10);
> -	if (*f == e)
> -		/* parse error */
> +	if (*f == e || (*e != ' ' && *e != '\0')) {
> +		condlog(0, "parse error in feature string \"%s\"", *f);
>  		return 1;
> -
> -	/* Check if we need to increase feature count space */
> -	l = strlen(*f) + strlen(n) + 1;
> -
> -	/* Count new features */
> -	if ((c % 10) == 9)
> -		l++;
> -	c++;
> -	q = n;
> -	while (*q != '\0') {
> -		if (*q == ' ' && q[1] != '\0' && q[1] != ' ') {
> -			if ((c % 10) == 9)
> -				l++;
> -			c++;
> -		}
> -		q++;
>  	}
>  
> +	/* Add 1 digit and 1 space */
> +	l = strlen(e) + strlen(n) + 2;
> +
> +	c++;
> +	/* Check if we need more digits for feature count */
> +	for (d = c; d >= 10; d /= 10)
> +		l++;
> +
>  	t = MALLOC(l + 1);
>  	if (!t)
>  		return 1;
>  
> -	memset(t, 0, l + 1);
> +	/* e: old feature string with leading space, or "" */
> +	if (*e == ' ')
> +		while (*(e + 1) == ' ')
> +			e++;
>  
> -	/* Update feature count */
> -	d = c;
> -	l = 1;
> -	while (d > 9) {
> -		d /= 10;
> -		l++;
> -	}
> -	p = t;
> -	snprintf(p, l + 2, "%0d ", c);
> -
> -	/* Copy the feature string */
> -	p = strchr(*f, ' ');
> -
> -	if (p) {
> -		while (*p == ' ')
> -			p++;
> -		strcat(t, p);
> -		strcat(t, " ");
> -	} else {
> -		p = t + strlen(t);
> -	}
> -	strcat(t, n);
> +	snprintf(t, l + 1, "%0d%s %s", c, e, n);

Just one nit, that I know I'm guilty of too (so Coverity says).  If
you're sure that you have enough size for the string, you don't need
snprintf. If you want to make sure that the string isn't too big, you
should probably use "l" instead of "l + 1", so that you know that the
string will get null termintated (from your earlier memset), and you
won't read off the end of the array later. Otherwise, ACK. 

>  
>  	FREE(*f);
>  	*f = t;
> -- 
> 2.14.0

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

-Ben

Re: [PATCH 4/4] multipath: delegate dangerous commands to multipathd

[Benjamin Marzinski]

On Tue, Aug 29, 2017 at 12:05:36AM +0200, Martin Wilck wrote:
> Some multipath commands are dangerous to run while multipathd is running.
> For example, "multipath -r" may apply a modified configuration to the kernel,
> while multipathd is still using the old configuration, leading to
> inconsistent state between multipathd and the kernel.
> 
> It is safer to use equivalent multipathd client commands instead.
> For now, do this only for "multipath -r", but other invocations
> may be added in the future. Perhaps some day, all "multipath"
> commands will be mapped to multipathd actions.

Thanks. I've been meaning to do something like this forever.

> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  Makefile.inc     |  2 +-
>  multipath/main.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 74 insertions(+), 1 deletion(-)
> 
> diff --git a/Makefile.inc b/Makefile.inc
> index 29c290a2..d012b3d7 100644
> --- a/Makefile.inc
> +++ b/Makefile.inc
> @@ -90,7 +90,7 @@ OPTFLAGS	= -O2 -g -pipe -Wall -Wextra -Wformat=2 -Werror=implicit-int \
>  		  -Wp,-D_FORTIFY_SOURCE=2 $(STACKPROT) \
>  		  --param=ssp-buffer-size=4
>  
> -CFLAGS		= $(OPTFLAGS) -DLIB_STRING=\"${LIB}\" -DRUN_DIR=\"${RUN}\"
> +CFLAGS		= $(OPTFLAGS) -D BIN_DIR=\"$(bindir)\" -DLIB_STRING=\"${LIB}\" -DRUN_DIR=\"${RUN}\"
>  BIN_CFLAGS	= -fPIE -DPIE
>  LIB_CFLAGS	= -fPIC
>  SHARED_FLAGS	= -shared
> diff --git a/multipath/main.c b/multipath/main.c
> index dede017e..b54aa9bb 100644
> --- a/multipath/main.c
> +++ b/multipath/main.c
> @@ -502,6 +502,74 @@ get_dev_type(char *dev) {
>  	return DEV_NONE;
>  }
>  
> +/*
> + * Some multipath commands are dangerous to run while multipathd is running.
> + * For example, "multipath -r" may apply a modified configuration to the kernel,
> + * while multipathd is still using the old configuration, leading to
> + * inconsistent state.
> + *
> + * It is safer to use equivalent multipathd client commands instead.
> + */
> +int delegate_to_multipathd(enum mpath_cmds cmd, const struct config *conf)
> +{
> +#define DELEGATE_MAX_ARGS 5
> +	int fd = mpath_connect();
> +	char mpd_path[PATH_MAX];
> +	char *argv[DELEGATE_MAX_ARGS];
> +	char log[4096], *p;
> +	char verbosity[2] = { '0', '\0' };
> +	int idx = 1, cmd_idx, sz, n, i;
> +
> +	if (fd == -1)
> +		return 0;
> +	close(fd);
> +
> +	if (conf->verbosity > 0 && conf->verbosity < 8)
> +		verbosity[0] = '0' + conf->verbosity;
> +	argv[idx++] = "-v";
> +	argv[idx++] = verbosity;
> +
> +	cmd_idx = idx;
> +	if (cmd == CMD_CREATE && conf->force_reload == FORCE_RELOAD_YES) {
> +		argv[idx++] = "reconfigure";
> +	} /* Add other translations here */
> +
> +	if (idx == cmd_idx)
> +		/* No command found, no need to delegate */
> +		return 0;
> +	else if (idx >= DELEGATE_MAX_ARGS) {
> +		condlog(0, "internal error - argv overflow");
> +		exit(1);
> +	}

Why not just return 0 here, so that we can try the failing back to the
multipath code. In the longer term, it's definitely worth asking if that
code needs to exist at all, of if a large chunk of multipath code can
just turn into wrapped calls to multipathd.

> +
> +	argv[idx] = NULL;
> +	snprintf(mpd_path, sizeof(mpd_path), "%s/%s", BIN_DIR, "multipathd");
> +	argv[0] = mpd_path;
> +
> +	condlog(1, "delegating command to multipathd");
> +
> +	sz = sizeof(log);
> +	p = log;
> +	i = 0;
> +	n = snprintf(p, sz, "command: %s", argv[i++]);
> +	while (i < idx && n < sz) {
> +		sz -= n;
> +		p += n;
> +		n = snprintf(p, sz, " %s", argv[i++]);
> +	}
> +	condlog(2, "%s", log);
> +	if (n >= sz)
> +		condlog(2, "(command on previous line was truncated)");
> +
> +	if (execv(mpd_path, argv) == -1) {
> +		condlog(0, "failed to execute multipathd: %s", strerror(errno));
> +		exit(1);
> +	}

Why exec multipathd, instead of using the libmpathcmd api, like
mpathpersist, libdmmp, and the multipathd client do?

-Ben

> +
> +	/* not reached */
> +	return 1;
> +}
> +
>  int
>  main (int argc, char *argv[])
>  {
> @@ -695,10 +763,15 @@ main (int argc, char *argv[])
>  		} else
>  			mpath_disconnect(fd);
>  	}
> +
>  	if (cmd == CMD_REMOVE_WWID && !dev) {
>  		condlog(0, "the -w option requires a device");
>  		goto out;
>  	}
> +
> +	if (delegate_to_multipathd(cmd, conf))
> +		exit(0); /* not reached */
> +
>  	if (cmd == CMD_RESET_WWIDS) {
>  		struct multipath * mpp;
>  		int i;
> -- 
> 2.14.0

Re: [PATCH 3/4] libmultipath: add_feature: allow only 1 feature

[Benjamin Marzinski]

On Thu, Sep 07, 2017 at 04:42:09PM -0500, Benjamin Marzinski wrote:
> On Tue, Aug 29, 2017 at 12:05:35AM +0200, Martin Wilck wrote:
> > The existing test "if feature is already present" doesn't work for
> > multiple features, and we are only using add_feature() for single
> > feature additions anyway. Simplify the code by not allowing spaces
> > in the feature string to be added. This way we can drop the
> > complex "count new features" code. Moreover, print an error message if
> > the existing features string is malformed.
> > 
> > The old code calculated the width of the feature count twice, and the
> > second time messed up the buffer length field passed to snprintf(); fix
> > that, too.  Finally, replace several strcat() invocations by one
> > strncpy() call to make the code easier to review.
> > 
> > Signed-off-by: Martin Wilck <mwilck@suse.com>
> > ---
> >  libmultipath/structs.c | 64 +++++++++++++++++---------------------------------
> >  1 file changed, 21 insertions(+), 43 deletions(-)
> > 
> > diff --git a/libmultipath/structs.c b/libmultipath/structs.c
> > index 11e33676..828e7907 100644
> > --- a/libmultipath/structs.c
> > +++ b/libmultipath/structs.c
> > @@ -516,8 +516,7 @@ void setup_feature(struct multipath *mpp, char *feature)
> >  int add_feature(char **f, const char *n)
> >  {
> >  	int c = 0, d, l;
> > -	char *e, *p, *t;
> > -	const char *q;
> > +	char *e, *t;
> >  
> >  	if (!f)
> >  		return 1;
> > @@ -526,6 +525,11 @@ int add_feature(char **f, const char *n)
> >  	if (!n || *n == '0')
> >  		return 0;
> >  
> > +	if (strchr(n, ' ') != NULL) {
> > +		condlog(0, "internal error: feature \"%s\" contains spaces", n);
> > +		return 1;
> > +	}
> > +
> >  	/* default feature is null */
> >  	if(!*f)
> >  	{
> > @@ -543,55 +547,29 @@ int add_feature(char **f, const char *n)
> >  
> >  	/* Get feature count */
> >  	c = strtoul(*f, &e, 10);
> > -	if (*f == e)
> > -		/* parse error */
> > +	if (*f == e || (*e != ' ' && *e != '\0')) {
> > +		condlog(0, "parse error in feature string \"%s\"", *f);
> >  		return 1;
> > -
> > -	/* Check if we need to increase feature count space */
> > -	l = strlen(*f) + strlen(n) + 1;
> > -
> > -	/* Count new features */
> > -	if ((c % 10) == 9)
> > -		l++;
> > -	c++;
> > -	q = n;
> > -	while (*q != '\0') {
> > -		if (*q == ' ' && q[1] != '\0' && q[1] != ' ') {
> > -			if ((c % 10) == 9)
> > -				l++;
> > -			c++;
> > -		}
> > -		q++;
> >  	}
> >  
> > +	/* Add 1 digit and 1 space */
> > +	l = strlen(e) + strlen(n) + 2;
> > +
> > +	c++;
> > +	/* Check if we need more digits for feature count */
> > +	for (d = c; d >= 10; d /= 10)
> > +		l++;
> > +
> >  	t = MALLOC(l + 1);
> >  	if (!t)
> >  		return 1;
> >  
> > -	memset(t, 0, l + 1);
> > +	/* e: old feature string with leading space, or "" */
> > +	if (*e == ' ')
> > +		while (*(e + 1) == ' ')
> > +			e++;
> >  
> > -	/* Update feature count */
> > -	d = c;
> > -	l = 1;
> > -	while (d > 9) {
> > -		d /= 10;
> > -		l++;
> > -	}
> > -	p = t;
> > -	snprintf(p, l + 2, "%0d ", c);
> > -
> > -	/* Copy the feature string */
> > -	p = strchr(*f, ' ');
> > -
> > -	if (p) {
> > -		while (*p == ' ')
> > -			p++;
> > -		strcat(t, p);
> > -		strcat(t, " ");
> > -	} else {
> > -		p = t + strlen(t);
> > -	}
> > -	strcat(t, n);
> > +	snprintf(t, l + 1, "%0d%s %s", c, e, n);
> 
> Just one nit, that I know I'm guilty of too (so Coverity says).  If
> you're sure that you have enough size for the string, you don't need
> snprintf. If you want to make sure that the string isn't too big, you
> should probably use "l" instead of "l + 1", so that you know that the
> string will get null termintated (from your earlier memset), and you
> won't read off the end of the array later. Otherwise, ACK. 

Nevermind, I was having a brain fart and thinking of strncpy.

-Ben

> 
> >  
> >  	FREE(*f);
> >  	*f = t;
> > -- 
> > 2.14.0
> 
> Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> 
> -Ben
> 
> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel

Re: [PATCH 4/4] multipath: delegate dangerous commands to multipathd

[Martin Wilck]

On Thu, 2017-09-07 at 16:57 -0500, Benjamin Marzinski wrote:
> On Tue, Aug 29, 2017 at 12:05:36AM +0200, Martin Wilck wrote:
> > Some multipath commands are dangerous to run while multipathd is
> > running.
> > For example, "multipath -r" may apply a modified configuration to
> > the kernel,
> > while multipathd is still using the old configuration, leading to
> > inconsistent state between multipathd and the kernel.
> > 
> > It is safer to use equivalent multipathd client commands instead.
> > For now, do this only for "multipath -r", but other invocations
> > may be added in the future. Perhaps some day, all "multipath"
> > commands will be mapped to multipathd actions.
> 
> Thanks. I've been meaning to do something like this forever.

[...]

> +	if (execv(mpd_path, argv) == -1) {
> > +		condlog(0, "failed to execute multipathd: %s",
> > strerror(errno));
> > +		exit(1);
> > +	}
> 
> Why exec multipathd, instead of using the libmpathcmd api, like
> mpathpersist, libdmmp, and the multipathd client do?

Thanks for the review. I'll look into that, and your other suggestions.

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