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

[Martin Wilck <mwilck@suse.com>]

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