* [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
* 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
* [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
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 an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.