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

[Martin Wilck]

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