[PATCH V3 0/5] multipath-tools series: some cleanups and fixes

[PATCH V3 0/5] multipath-tools series: some cleanups and fixes

[Zhiqiang Liu]

When I learn and review the multipath-tools source code, I find
some cleanups and fixes.

V2->V3:
[PATCH V3 1/5] multipathd: adopt static char* arr in daemon_status
  - add default case in sd_notify_status to fix compile error
[PATCH V3 2/5] multipathd: use daemon_status_msg to construct sd notify
  - set MSG_SIZE to 32 and use safe_sprintf as suggested by Martin.

V1->V2:
- rewrite some patches as suggested by Martin.

repo: openSUSE/multipath-tools
repo link: https://github.com/openSUSE/multipath-tools
branch: upstream-queue

ZhiqiangLiu (2):
  multipathd: adopt static char* arr in daemon_status
  multipathd: use daemon_status_msg to construct sd notify msg in
    do_sd_notify func

 multipathd/main.c | 62 +++++++++++++++++++++--------------------------
 multipathd/main.h |  3 ++-
 2 files changed, 29 insertions(+), 36 deletions(-)

-- 
2.24.0.windows.2

[PATCH V3 1/5] multipathd: adopt static char* arr in daemon_status

[Zhiqiang Liu]

We adopt static char* array (demon_status_msg) in daemon_status func,
so it looks more simpler and easier to expand.

V2->V3:
- add default case in sd_notify_status to fix compile error
V1->V2:
- use "int" as the type of "status" (suggested by Martin)

Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
Signed-off-by: lixiaokeng <lixiaokeng@huawei.com>
---
 multipathd/main.c | 32 +++++++++++++++++---------------
 multipathd/main.h |  3 ++-
 2 files changed, 19 insertions(+), 16 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index 9ec65856..27c3a3ae 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -153,24 +153,24 @@ static volatile sig_atomic_t exit_sig;
 static volatile sig_atomic_t reconfig_sig;
 static volatile sig_atomic_t log_reset_sig;

+static const char *daemon_status_msg[DAEMON_STATUS_SIZE] = {
+	[DAEMON_INIT] = "init",
+	[DAEMON_START] = "startup",
+	[DAEMON_CONFIGURE] = "configure",
+	[DAEMON_IDLE] = "idle",
+	[DAEMON_RUNNING] = "running",
+	[DAEMON_SHUTDOWN] = "shutdown",
+};
+
 const char *
 daemon_status(void)
 {
-	switch (get_running_state()) {
-	case DAEMON_INIT:
-		return "init";
-	case DAEMON_START:
-		return "startup";
-	case DAEMON_CONFIGURE:
-		return "configure";
-	case DAEMON_IDLE:
-		return "idle";
-	case DAEMON_RUNNING:
-		return "running";
-	case DAEMON_SHUTDOWN:
-		return "shutdown";
-	}
-	return NULL;
+	int status = get_running_state();
+
+	if (status < DAEMON_INIT || status >= DAEMON_STATUS_SIZE)
+		return NULL;
+
+	return daemon_status_msg[status];
 }

 /*
@@ -192,6 +192,8 @@ sd_notify_status(enum daemon_status state)
 		return "STATUS=up";
 	case DAEMON_SHUTDOWN:
 		return "STATUS=shutdown";
+	default:
+		return NULL;
 	}
 	return NULL;
 }
diff --git a/multipathd/main.h b/multipathd/main.h
index 5dff17e5..6a5592c0 100644
--- a/multipathd/main.h
+++ b/multipathd/main.h
@@ -4,12 +4,13 @@
 #define MAPGCINT 5

 enum daemon_status {
-	DAEMON_INIT,
+	DAEMON_INIT = 0,
 	DAEMON_START,
 	DAEMON_CONFIGURE,
 	DAEMON_IDLE,
 	DAEMON_RUNNING,
 	DAEMON_SHUTDOWN,
+	DAEMON_STATUS_SIZE,
 };

 struct prout_param_descriptor;
-- 
2.24.0.windows.2

Re: [PATCH V3 1/5] multipathd: adopt static char* arr in daemon_status

[Martin Wilck]

On Sat, 2020-08-29 at 11:03 +0800, Zhiqiang Liu wrote:
> We adopt static char* array (demon_status_msg) in daemon_status func,
> so it looks more simpler and easier to expand.
> 
> V2->V3:
> - add default case in sd_notify_status to fix compile error
> V1->V2:
> - use "int" as the type of "status" (suggested by Martin)
> 
> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> Signed-off-by: lixiaokeng <lixiaokeng@huawei.com>
> ---
>  multipathd/main.c | 32 +++++++++++++++++---------------
>  multipathd/main.h |  3 ++-
>  2 files changed, 19 insertions(+), 16 deletions(-)
> 

Reviewed-by: Martin Wilck <mwilck@suse.com>

[PATCH V3 2/5] multipathd: use daemon_status_msg to construct sd notify msg in do_sd_notify func

[Zhiqiang Liu]

sd_notify_status() is very similar with daemon_status(), except
DAEMON_IDLE and DAEMON_RUNNING state. As suggested by Martin,
we can create the sd notification string in a dynamic buffer,
and treat DAEMON_IDLE and DAEMON_RUNNING cases first. Then,
we can use daemon_status_msg[state] for other cases.

V2->V3:
- set MSG_SIZE to 32 and use safe_sprintf as suggested by Martin.

Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
Signed-off-by: lixiaokeng <lixiaokeng@huawei.com>
---
 multipathd/main.c | 34 ++++++++++++----------------------
 1 file changed, 12 insertions(+), 22 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index 27c3a3ae..7fe55b12 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -85,6 +85,7 @@

 #define FILE_NAME_SIZE 256
 #define CMDSIZE 160
+#define MSG_SIZE 32

 #define LOG_MSG(lvl, verb, pp)					\
 do {								\
@@ -177,30 +178,12 @@ daemon_status(void)
  * I love you too, systemd ...
  */
 #ifdef USE_SYSTEMD
-static const char *
-sd_notify_status(enum daemon_status state)
-{
-	switch (state) {
-	case DAEMON_INIT:
-		return "STATUS=init";
-	case DAEMON_START:
-		return "STATUS=startup";
-	case DAEMON_CONFIGURE:
-		return "STATUS=configure";
-	case DAEMON_IDLE:
-	case DAEMON_RUNNING:
-		return "STATUS=up";
-	case DAEMON_SHUTDOWN:
-		return "STATUS=shutdown";
-	default:
-		return NULL;
-	}
-	return NULL;
-}
-
 static void do_sd_notify(enum daemon_status old_state,
 			 enum daemon_status new_state)
 {
+	char notify_msg[MSG_SIZE];
+	const char prefix[] = "STATUS=";
+	const char *msg = NULL;
 	/*
 	 * Checkerloop switches back and forth between idle and running state.
 	 * No need to tell systemd each time.
@@ -209,7 +192,14 @@ static void do_sd_notify(enum daemon_status old_state,
 	if ((new_state == DAEMON_IDLE || new_state == DAEMON_RUNNING) &&
 	    (old_state == DAEMON_IDLE || old_state == DAEMON_RUNNING))
 		return;
-	sd_notify(0, sd_notify_status(new_state));
+
+	if (new_state == DAEMON_IDLE || new_state == DAEMON_RUNNING)
+		msg = "up";
+	else
+		msg = daemon_status_msg[new_state];
+
+	if (!safe_sprintf(notify_msg, "%s%s", prefix, msg))
+		sd_notify(0, notify_msg);
 }
 #endif

-- 
2.24.0.windows.2

Re: [PATCH V3 2/5] multipathd: use daemon_status_msg to construct sd notify msg in do_sd_notify func

[Martin Wilck]

[-- Attachment #1: Type: text/plain, Size: 1029 bytes --]

Hello Zhiqiang,

On Sat, 2020-08-29 at 11:03 +0800, Zhiqiang Liu wrote:
> sd_notify_status() is very similar with daemon_status(), except
> DAEMON_IDLE and DAEMON_RUNNING state. As suggested by Martin,
> we can create the sd notification string in a dynamic buffer,
> and treat DAEMON_IDLE and DAEMON_RUNNING cases first. Then,
> we can use daemon_status_msg[state] for other cases.
> 
> V2->V3:
> - set MSG_SIZE to 32 and use safe_sprintf as suggested by Martin.
> 
> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> Signed-off-by: lixiaokeng <lixiaokeng@huawei.com>
> ---
>  multipathd/main.c | 34 ++++++++++++----------------------
>  1 file changed, 12 insertions(+), 22 deletions(-)

Thanks again. I'd like to modify the patch slightly as attached. 
I'm sorry that I didn't mention these minor issues in my previous
reviews. I'm sending you the fixup patch in order to short-circuit 
the procedure a bit and save both of us some work.

Would this be ok for you?
If yes, please resubmit this as v4.

Regards,
Martin


[-- Attachment #2: 0001-fixup-multipathd-use-daemon_status_msg-to-construct-.patch --]
[-- Type: text/x-patch, Size: 1303 bytes --]

From 73915fb32b2b0d233ee908a4789613c03c27285e Mon Sep 17 00:00:00 2001
From: Martin Wilck <mwilck@suse.com>
Date: Mon, 31 Aug 2020 11:43:56 +0200
Subject: [PATCH] fixup! multipathd: use daemon_status_msg to construct sd
 notify msg in do_sd_notify func

 - no need to initialize msg, it's always set
 - msg could be NULL, at least in theory, so check return value before passing
 it to snprintf()
 - avoid "prefix" variable; rather put the prefix in format string
---
 multipathd/main.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index 5df004a..f7229a7 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -183,8 +183,7 @@ static void do_sd_notify(enum daemon_status old_state,
 			 enum daemon_status new_state)
 {
 	char notify_msg[MSG_SIZE];
-	const char prefix[] = "STATUS=";
-	const char *msg = NULL;
+	const char *msg;
 	/*
 	 * Checkerloop switches back and forth between idle and running state.
 	 * No need to tell systemd each time.
@@ -199,7 +198,7 @@ static void do_sd_notify(enum daemon_status old_state,
 	else
 		msg = daemon_status_msg[new_state];
 
-	if (!safe_sprintf(notify_msg, "%s%s", prefix, msg))
+	if (msg && !safe_sprintf(notify_msg, "STATUS=%s", msg))
 		sd_notify(0, notify_msg);
 }
 #endif
-- 
2.28.0


[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

Re: [PATCH V3 2/5] multipathd: use daemon_status_msg to construct sd notify msg in do_sd_notify func

[Zhiqiang Liu]

On 2020/8/31 17:55, Martin Wilck wrote:
> Hello Zhiqiang,
> 
> On Sat, 2020-08-29 at 11:03 +0800, Zhiqiang Liu wrote:
>> sd_notify_status() is very similar with daemon_status(), except
>> DAEMON_IDLE and DAEMON_RUNNING state. As suggested by Martin,
>> we can create the sd notification string in a dynamic buffer,
>> and treat DAEMON_IDLE and DAEMON_RUNNING cases first. Then,
>> we can use daemon_status_msg[state] for other cases.
>>
>> V2->V3:
>> - set MSG_SIZE to 32 and use safe_sprintf as suggested by Martin.
>>
>> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
>> Signed-off-by: lixiaokeng <lixiaokeng@huawei.com>
>> ---
>>  multipathd/main.c | 34 ++++++++++++----------------------
>>  1 file changed, 12 insertions(+), 22 deletions(-)
> 
> Thanks again. I'd like to modify the patch slightly as attached. 
> I'm sorry that I didn't mention these minor issues in my previous
> reviews. I'm sending you the fixup patch in order to short-circuit 
> the procedure a bit and save both of us some work.
> 
> Would this be ok for you?
> If yes, please resubmit this as v4.
> 
> Regards,
> Martin
> 

Thanks for your suggestion.
I will send the v4 patch as your suggestion.

Regards,
Zhiqiang Liu