[dm-devel] [PATCH V2 2/5] multipathd: use daemon_status_msg to construct sd notify msg in do_sd_notify func

[Martin Wilck]

On Fri, 2020-08-21 at 18:59 +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.
> 
> Signed-off-by: Zhiqiang Liu <liuzhiqiang26 at huawei.com>
> Signed-off-by: lixiaokeng <lixiaokeng at huawei.com>
> ---
>  multipathd/main.c | 36 ++++++++++++++++--------------------
>  1 file changed, 16 insertions(+), 20 deletions(-)
> 
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 62cf4ff4..4ba015bb 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -85,6 +85,7 @@
> 
>  #define FILE_NAME_SIZE 256
>  #define CMDSIZE 160
> +#define MSG_SIZE 64
> 
>  #define LOG_MSG(lvl, verb, pp)					
> \
>  do {								\
> @@ -177,28 +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";
> -	}
> -	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.
> @@ -207,7 +192,18 @@ 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 (snprintf(notify_msg, MSG_SIZE, "%s%s", prefix, msg) >=
> MSG_SIZE) {
> +		condlog(2, "len of msg is should be shorter than %d",
> MSG_SIZE);

You can *prove* that this condition can never occur (actually, you'll
never need more than 17 bytes - it's ok to have some head room). The
compiler may force you to check the return value, but it's safe to
write simply

if (!safe_sprintf(notify_msg, "%s%s", prefix, msg))
       sd_notify(0, notify_msg);

No error message is necessary here.

Martin

> +		return;
> +	}
> +
> +	sd_notify(0, notify_msg);
>  }
>  #endif
>