* [PATCH 2/4] libmultipath: add_feature: skip pointless NULL check
2017-08-28 22:05 [PATCH 1/4] multipathd: don't flood system with sd_notify calls Martin Wilck
@ 2017-08-28 22:05 ` Martin Wilck
2017-08-29 6:37 ` Hannes Reinecke
2017-09-07 21:33 ` Benjamin Marzinski
2017-08-28 22:05 ` [PATCH 3/4] libmultipath: add_feature: allow only 1 feature Martin Wilck
` (3 subsequent siblings)
4 siblings, 2 replies; 15+ messages in thread
From: Martin Wilck @ 2017-08-28 22:05 UTC (permalink / raw)
To: Christophe Varoqui, Hannes Reinecke, Benjamin Marzinski; +Cc: dm-devel
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
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH 2/4] libmultipath: add_feature: skip pointless NULL check
2017-08-28 22:05 ` [PATCH 2/4] libmultipath: add_feature: skip pointless NULL check Martin Wilck
@ 2017-08-29 6:37 ` Hannes Reinecke
2017-08-29 7:21 ` Martin Wilck
2017-09-07 21:33 ` Benjamin Marzinski
1 sibling, 1 reply; 15+ messages in thread
From: Hannes Reinecke @ 2017-08-29 6:37 UTC (permalink / raw)
To: Martin Wilck, Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel
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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 2/4] libmultipath: add_feature: skip pointless NULL check
2017-08-29 6:37 ` Hannes Reinecke
@ 2017-08-29 7:21 ` Martin Wilck
0 siblings, 0 replies; 15+ messages in thread
From: Martin Wilck @ 2017-08-29 7:21 UTC (permalink / raw)
To: Hannes Reinecke, Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/4] libmultipath: add_feature: skip pointless NULL check
2017-08-28 22:05 ` [PATCH 2/4] libmultipath: add_feature: skip pointless NULL check Martin Wilck
2017-08-29 6:37 ` Hannes Reinecke
@ 2017-09-07 21:33 ` Benjamin Marzinski
1 sibling, 0 replies; 15+ messages in thread
From: Benjamin Marzinski @ 2017-09-07 21:33 UTC (permalink / raw)
To: Martin Wilck; +Cc: dm-devel
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 3/4] libmultipath: add_feature: allow only 1 feature
2017-08-28 22:05 [PATCH 1/4] multipathd: don't flood system with sd_notify calls Martin Wilck
2017-08-28 22:05 ` [PATCH 2/4] libmultipath: add_feature: skip pointless NULL check Martin Wilck
@ 2017-08-28 22:05 ` Martin Wilck
2017-08-29 6:39 ` Hannes Reinecke
2017-09-07 21:42 ` Benjamin Marzinski
2017-08-28 22:05 ` [PATCH 4/4] multipath: delegate dangerous commands to multipathd Martin Wilck
` (2 subsequent siblings)
4 siblings, 2 replies; 15+ messages in thread
From: Martin Wilck @ 2017-08-28 22:05 UTC (permalink / raw)
To: Christophe Varoqui, Hannes Reinecke, Benjamin Marzinski; +Cc: dm-devel
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
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH 3/4] libmultipath: add_feature: allow only 1 feature
2017-08-28 22:05 ` [PATCH 3/4] libmultipath: add_feature: allow only 1 feature Martin Wilck
@ 2017-08-29 6:39 ` Hannes Reinecke
2017-09-07 21:42 ` Benjamin Marzinski
1 sibling, 0 replies; 15+ messages in thread
From: Hannes Reinecke @ 2017-08-29 6:39 UTC (permalink / raw)
To: Martin Wilck, Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel
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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 3/4] libmultipath: add_feature: allow only 1 feature
2017-08-28 22:05 ` [PATCH 3/4] libmultipath: add_feature: allow only 1 feature Martin Wilck
2017-08-29 6:39 ` Hannes Reinecke
@ 2017-09-07 21:42 ` Benjamin Marzinski
2017-09-07 22:37 ` Benjamin Marzinski
1 sibling, 1 reply; 15+ messages in thread
From: Benjamin Marzinski @ 2017-09-07 21:42 UTC (permalink / raw)
To: Martin Wilck; +Cc: dm-devel
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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 3/4] libmultipath: add_feature: allow only 1 feature
2017-09-07 21:42 ` Benjamin Marzinski
@ 2017-09-07 22:37 ` Benjamin Marzinski
0 siblings, 0 replies; 15+ messages in thread
From: Benjamin Marzinski @ 2017-09-07 22:37 UTC (permalink / raw)
To: Martin Wilck; +Cc: dm-devel
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 4/4] multipath: delegate dangerous commands to multipathd
2017-08-28 22:05 [PATCH 1/4] multipathd: don't flood system with sd_notify calls Martin Wilck
2017-08-28 22:05 ` [PATCH 2/4] libmultipath: add_feature: skip pointless NULL check Martin Wilck
2017-08-28 22:05 ` [PATCH 3/4] libmultipath: add_feature: allow only 1 feature Martin Wilck
@ 2017-08-28 22:05 ` Martin Wilck
2017-08-29 6:40 ` Hannes Reinecke
2017-09-07 21:57 ` Benjamin Marzinski
2017-08-29 6:36 ` [PATCH 1/4] multipathd: don't flood system with sd_notify calls Hannes Reinecke
2017-09-07 21:31 ` Benjamin Marzinski
4 siblings, 2 replies; 15+ messages in thread
From: Martin Wilck @ 2017-08-28 22:05 UTC (permalink / raw)
To: Christophe Varoqui, Hannes Reinecke, Benjamin Marzinski; +Cc: dm-devel
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
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH 4/4] multipath: delegate dangerous commands to multipathd
2017-08-28 22:05 ` [PATCH 4/4] multipath: delegate dangerous commands to multipathd Martin Wilck
@ 2017-08-29 6:40 ` Hannes Reinecke
2017-09-07 21:57 ` Benjamin Marzinski
1 sibling, 0 replies; 15+ messages in thread
From: Hannes Reinecke @ 2017-08-29 6:40 UTC (permalink / raw)
To: Martin Wilck, Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel
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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 4/4] multipath: delegate dangerous commands to multipathd
2017-08-28 22:05 ` [PATCH 4/4] multipath: delegate dangerous commands to multipathd Martin Wilck
2017-08-29 6:40 ` Hannes Reinecke
@ 2017-09-07 21:57 ` Benjamin Marzinski
2017-09-08 8:16 ` Martin Wilck
1 sibling, 1 reply; 15+ messages in thread
From: Benjamin Marzinski @ 2017-09-07 21:57 UTC (permalink / raw)
To: Martin Wilck; +Cc: dm-devel
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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 4/4] multipath: delegate dangerous commands to multipathd
2017-09-07 21:57 ` Benjamin Marzinski
@ 2017-09-08 8:16 ` Martin Wilck
0 siblings, 0 replies; 15+ messages in thread
From: Martin Wilck @ 2017-09-08 8:16 UTC (permalink / raw)
To: Benjamin Marzinski; +Cc: dm-devel
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/4] multipathd: don't flood system with sd_notify calls
2017-08-28 22:05 [PATCH 1/4] multipathd: don't flood system with sd_notify calls Martin Wilck
` (2 preceding siblings ...)
2017-08-28 22:05 ` [PATCH 4/4] multipath: delegate dangerous commands to multipathd Martin Wilck
@ 2017-08-29 6:36 ` Hannes Reinecke
2017-09-07 21:31 ` Benjamin Marzinski
4 siblings, 0 replies; 15+ messages in thread
From: Hannes Reinecke @ 2017-08-29 6:36 UTC (permalink / raw)
To: Martin Wilck, Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel
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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 1/4] multipathd: don't flood system with sd_notify calls
2017-08-28 22:05 [PATCH 1/4] multipathd: don't flood system with sd_notify calls Martin Wilck
` (3 preceding siblings ...)
2017-08-29 6:36 ` [PATCH 1/4] multipathd: don't flood system with sd_notify calls Hannes Reinecke
@ 2017-09-07 21:31 ` Benjamin Marzinski
4 siblings, 0 replies; 15+ messages in thread
From: Benjamin Marzinski @ 2017-09-07 21:31 UTC (permalink / raw)
To: Martin Wilck; +Cc: dm-devel
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
^ permalink raw reply [flat|nested] 15+ messages in thread