* [PATCH V3 0/5] multipath-tools series: some cleanups and fixes
@ 2020-08-29 3:02 Zhiqiang Liu
2020-08-29 3:03 ` [PATCH V3 1/5] multipathd: adopt static char* arr in daemon_status Zhiqiang Liu
2020-08-29 3:03 ` [PATCH V3 2/5] multipathd: use daemon_status_msg to construct sd notify msg in do_sd_notify func Zhiqiang Liu
0 siblings, 2 replies; 6+ messages in thread
From: Zhiqiang Liu @ 2020-08-29 3:02 UTC (permalink / raw)
To: Martin Wilck
Cc: lixiaokeng, yanxiaodan, linfeilong, dm-devel, Zdenek Kabelac,
liuzhiqiang26
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH V3 1/5] multipathd: adopt static char* arr in daemon_status
2020-08-29 3:02 [PATCH V3 0/5] multipath-tools series: some cleanups and fixes Zhiqiang Liu
@ 2020-08-29 3:03 ` Zhiqiang Liu
2020-08-31 14:37 ` Martin Wilck
2020-08-29 3:03 ` [PATCH V3 2/5] multipathd: use daemon_status_msg to construct sd notify msg in do_sd_notify func Zhiqiang Liu
1 sibling, 1 reply; 6+ messages in thread
From: Zhiqiang Liu @ 2020-08-29 3:03 UTC (permalink / raw)
To: Martin Wilck
Cc: lixiaokeng, yanxiaodan, linfeilong, dm-devel, Zdenek Kabelac,
liuzhiqiang26
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH V3 2/5] multipathd: use daemon_status_msg to construct sd notify msg in do_sd_notify func
2020-08-29 3:02 [PATCH V3 0/5] multipath-tools series: some cleanups and fixes Zhiqiang Liu
2020-08-29 3:03 ` [PATCH V3 1/5] multipathd: adopt static char* arr in daemon_status Zhiqiang Liu
@ 2020-08-29 3:03 ` Zhiqiang Liu
2020-08-31 9:55 ` Martin Wilck
1 sibling, 1 reply; 6+ messages in thread
From: Zhiqiang Liu @ 2020-08-29 3:03 UTC (permalink / raw)
To: Martin Wilck
Cc: lixiaokeng, yanxiaodan, linfeilong, dm-devel, Zdenek Kabelac,
liuzhiqiang26
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH V3 2/5] multipathd: use daemon_status_msg to construct sd notify msg in do_sd_notify func
2020-08-29 3:03 ` [PATCH V3 2/5] multipathd: use daemon_status_msg to construct sd notify msg in do_sd_notify func Zhiqiang Liu
@ 2020-08-31 9:55 ` Martin Wilck
2020-08-31 11:03 ` Zhiqiang Liu
0 siblings, 1 reply; 6+ messages in thread
From: Martin Wilck @ 2020-08-31 9:55 UTC (permalink / raw)
To: Zhiqiang Liu; +Cc: lixiaokeng, yanxiaodan, linfeilong, dm-devel, Zdenek Kabelac
[-- 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 --]
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH V3 2/5] multipathd: use daemon_status_msg to construct sd notify msg in do_sd_notify func
2020-08-31 9:55 ` Martin Wilck
@ 2020-08-31 11:03 ` Zhiqiang Liu
0 siblings, 0 replies; 6+ messages in thread
From: Zhiqiang Liu @ 2020-08-31 11:03 UTC (permalink / raw)
To: Martin Wilck; +Cc: lixiaokeng, yanxiaodan, linfeilong, dm-devel, Zdenek Kabelac
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH V3 1/5] multipathd: adopt static char* arr in daemon_status
2020-08-29 3:03 ` [PATCH V3 1/5] multipathd: adopt static char* arr in daemon_status Zhiqiang Liu
@ 2020-08-31 14:37 ` Martin Wilck
0 siblings, 0 replies; 6+ messages in thread
From: Martin Wilck @ 2020-08-31 14:37 UTC (permalink / raw)
To: Zhiqiang Liu; +Cc: lixiaokeng, yanxiaodan, linfeilong, dm-devel, Zdenek Kabelac
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>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-08-31 14:37 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-29 3:02 [PATCH V3 0/5] multipath-tools series: some cleanups and fixes Zhiqiang Liu
2020-08-29 3:03 ` [PATCH V3 1/5] multipathd: adopt static char* arr in daemon_status Zhiqiang Liu
2020-08-31 14:37 ` Martin Wilck
2020-08-29 3:03 ` [PATCH V3 2/5] multipathd: use daemon_status_msg to construct sd notify msg in do_sd_notify func Zhiqiang Liu
2020-08-31 9:55 ` Martin Wilck
2020-08-31 11:03 ` Zhiqiang Liu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).