[dm-devel] [PATCH 3/6] multipathd: adopt static char* arr in sd_notify_status func

[Zhiqiang Liu]

On 2020/8/17 23:44, Martin Wilck wrote:
> On Mon, 2020-08-17 at 23:33 +0800, Zhiqiang Liu wrote:
>>
>> On 2020/8/17 16:35, Martin Wilck wrote:
>>> On Sun, 2020-08-16 at 09:44 +0800, Zhiqiang Liu wrote:
>>>> We adopt static char* array (sd_notify_status_msg) in
>>>> sd_notify_status func, so it looks more simpler and easier
>>>> to expand.
>>>>
>>>> Signed-off-by: Zhiqiang Liu <liuzhiqiang26 at huawei.com>
>>>> Signed-off-by: lixiaokeng <lixiaokeng at huawei.com>
>>>> ---
>>>>  multipathd/main.c | 26 ++++++++++++--------------
>>>>  1 file changed, 12 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/multipathd/main.c b/multipathd/main.c
>>>> index cab1d0d..a09ccd1 100644
>>>> --- a/multipathd/main.c
>>>> +++ b/multipathd/main.c
>>>> @@ -177,23 +177,21 @@ daemon_status(void)
>>>>   * I love you too, systemd ...
>>>>   */
>>>>  #ifdef USE_SYSTEMD
>>>> +static const char *sd_notify_status_msg[DAEMON_STATUS_SIZE] = {
>>>> +	[DAEMON_INIT] = "STATUS=init",
>>>> +	[DAEMON_START] = "STATUS=startup",
>>>> +	[DAEMON_CONFIGURE] = "STATUS=configure",
>>>> +	[DAEMON_IDLE] = "STATUS=up",
>>>> +	[DAEMON_RUNNING] = "STATUS=up",
>>>> +	[DAEMON_SHUTDOWN] = "STATUS=shutdown",
>>>> +};
>>>> +
>>>
>>> This repetition of "STATUS=" looks clumsy. It's not your fault,
>>> because
>>> the current code does the same thing. But if you want to clean this
>>> up,
>>> please create the notification string in a dynamic buffer, and use
>>> daemon_status() for those cases where it applies.
>>>
>>> Regards
>>> Martin
>>>
>> Thanks for your reply.
>> Besides the prefixes "STATUS=", there are also some differences
>> between
>> sd_notify_status_msg[DAEMON_IDLE|DAEMON_RUNNING] and
>> demon_status_msg[DAEMON_IDLE|DAEMON_RUNNING].
>>
>> For example,
>> sd_notify_status_msg[DAEMON_RUNNING] = "STATUS=up",
>> demon_status_msg[DAEMON_RUNNING] = "running"
>>
>> So if we create the notification string in a dynamic buffer with
>> using daemon_status, we have to make some special judgement
>> on DAEMON_RUNNING and DAEMON_IDLE status. This may be why
>> the sd_notify_status func was created.
>>
>> We can implement both solutions. Martin, which one do you prefer?
> 
> That's what I meant with "where it applies". You treat the IDLE and
> RUNNING cases first (probably in a switch statement), and use
> daemon_status() for the "default" case.
> 
> The  reason we don't differentiate between "running" and "idle" in the
> sd_notify() code path is that the daemon switches between these states
> very often (at least once per tick) and these notifications causes lots
> of dbus traffic without much value.
> 
> Regards
> Martin
> 

I will do that as your suggestion in v2 patch.
Thanks again.

Regards
Zhiqiang Liu