Date: Sat, 25 Feb 2017 19:01:43 +0100
From: Natanael Copa <ncopa@alpinelinux.org>
To: Valery Kartel <valery.kartel@gmail.com>
Cc: alpine-aports <alpine-aports@lists.alpinelinux.org>
Subject: Re: [alpine-aports] [PATCH] testing/rspamd: upgrade to 1.4.4
Message-ID: <20170225190143.5b599511@ncopa-desktop.copa.dup.pw>
In-Reply-To: <CAKTwcDNt305ELVuptOT+w1DXjKwVQ=hYB-iiLcn17tQvRYj6NA@mail.gmail.com>
References: <20170224143718.22639-1-valery.kartel@gmail.com>
	<20170224164839.47bce33d@ncopa-desktop.copa.dup.pw>
	<CAKTwcDNt305ELVuptOT+w1DXjKwVQ=hYB-iiLcn17tQvRYj6NA@mail.gmail.com>
Precedence: list
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

On Fri, 24 Feb 2017 21:56:33 +0200
Valery Kartel <valery.kartel@gmail.com> wrote:

> Yes, it's a good idea, and sometimes useful.
> 
> But I think checkconfig() need to be just a wrapper for start_pre() covered
> with user-friendly ebegin...eend.

I think it needs to be the other way around.

> Because start_pre() is an internal function with silent code, but
> checkconfig is a user-called command with some smarter output.
> 
> so start_pre() would stay as-is and checkconfig would be added to
> extra_commands list ... like this:
> 
> ===
> extra_commands="checkconfig"
> description_checkconfig="Verify configuration file"
> ....
> checkconfig() {
>     ebegin "Checking $RC_SVCNAME config"
>     startuplog=/dev/tty; start_pre
>     eend $?
> }
> ===
> 
> What do you think?

I still don't like it. The goal was not to add support for a
"checkconfig" command, the goal was to improve readability. Code that
is easy to read and clearly describes what it does.

The name "start_pre" describes *when* something happens but it does not
describe anything about *what* it does. The name "checkconfig"
describes *what* but does not really say anything about *when*.

If I would have seen a function without the context like this:

checkconfig() {
	start_pre
}

I would have thought something like: "to check the config it is needed
to start something called pre?". Which is not the case. It would work
and do what it should but it would be misleading or confusing.

The same thing happens with the start_pre function. If I see a function
called "start_pre" that looks like:

start_pre () {
	[ "$RC_CMD" = "start" ] && checkpath -d -m 750 -o $user:$group ${pidfile%/*}  
	$command $command_args -t >/dev/null 2>>${startuplog:-/dev/null}
}

The function name indicates that this is something that happens before
start, but in the code, there is a condition for the "checkpath". To
understand when checkpath actually happen you need to understand when
RC_CMD is set to "start" and you start wonder, how and what sets RC_CMD?

You can also read that it runs a command with arg -t but you have no
idea what this does. To figure out what it does you need to first
figure out what command it actually runs, then you need to check the
manpage what -t does.

Lets say that you at some point someone else would need to make
something else happen before start. He or she would find the
"start_pre" function and add the needed thing, but if this person does
not pay close attention to what actually happens (which is not very
clear) this thing would also happen whenever someone wanted to only
check the config with "checkconfig". A bug could easily creep in here.

If we do it the other way around though:

checkconfig() {
	$command $command_args -t
}

start_pre () {
	checkpath -d -m 750 -o $user:$group ${pidfile%/*}
	checkconfig >/dev/null 2>>${startuplog:-/dev/null}
}

It becomes much clearer what is going on.  We could drop checkconfig
alltogether and just do "$command -t" all the places we need to check the
config, but "$command -t" is not a good description of what is actually
happening. By wrapping it in a function named "checkconfig" it because
easier to understand what it does. (btw we should probably avoid using
$command_args there)

The function "start_pre" also becomes more readable. It becomes fairly
clear that before start we need to check some path and then check the
config. If someone else needs to change the code in the future, he or
she can easily just add it to the "start_pre" without unexpected things
happens when someone wants to check the config.

We also get rid of the [ "$RC_CMD" = "start" ] conditional which also
makes code clearer.

In this case, the file is small and there are few lines of code so you
will quickly figure out whats going on, but if this would have been a
huge file or those functions would have been spread out over multiple
files, then it would been a real problem.

So yes, your patch works, but code needs to be easier to read. As a
side-effect of making the code readable you also get the extra command
"checkconfig", but thats just a side-effect. The goal is to make code
more readable and prevent future bugs.

-nc

> 
> 2017-02-24 17:48 GMT+02:00 Natanael Copa <ncopa@alpinelinux.org>:
> 
> > On Fri, 24 Feb 2017 16:37:18 +0200
> > Valery Kartel <valery.kartel@gmail.com> wrote:
> >  
> > > fixes in init script
> > > ---
> > >  testing/rspamd/APKBUILD     | 18 +++---------------
> > >  testing/rspamd/rspamd.initd | 10 ++++------
> > >  2 files changed, 7 insertions(+), 21 deletions(-)
> > >  
> >
> > ...
> >  
> > > diff --git a/testing/rspamd/rspamd.initd b/testing/rspamd/rspamd.initd
> > > index 93148ccb4b..b0eb9b057a 100644
> > > --- a/testing/rspamd/rspamd.initd
> > > +++ b/testing/rspamd/rspamd.initd
> > > @@ -18,20 +18,18 @@ depend() {
> > >  }
> > >
> > >  start_pre() {
> > > -     ebegin
> > > -     checkpath --directory --mode 750 --owner $user:$group ${pidfile%/*}
> > > +     [ "$RC_CMD" = "start" ] && checkpath -d -m 750 -o $user:$group  
> > ${pidfile%/*}  
> > >       $command $command_args -t >/dev/null 2>>${startuplog:-/dev/null}
> > > -     eend $?
> > >  }
> > >
> > >  reload() {
> > > -     ebegin "Reloading ${SVCNAME}"
> > > -     start-stop-daemon --signal HUP --pidfile $pidfile
> > > +     ebegin "Reloading $RC_SVCNAME config"
> > > +     start_pre && start-stop-daemon --signal HUP --pidfile $pidfile
> > >       eend $?
> > >  }
> > >
> > >  reopen() {
> > > -     ebegin "Reopening ${SVCNAME} log files"
> > > +     ebegin "Reopening $RC_SVCNAME log files"
> > >       start-stop-daemon --signal USR1 --pidfile $pidfile
> > >       eend $?
> > >  }  
> >
> >
> > I would have prefered:
> >
> > checkconfig() {
> >         $command $command_args -t >/dev/null 2>>${startuplog:-/dev/null}
> > }
> >
> > start_pre() {
> >         checkpath --directory --mode 750 --owner $user:$group ${pidfile%/*}
> >         checkconfig
> > }
> >
> > reload() {
> >         ebegin "Reloading $RC_SVCNAME config"
> >         checkconfig && start-stop-daemon --signal HUP --pidfile $pidfile
> >         eend $?
> > }
> >
> > -nc
> >  



---
Unsubscribe:  alpine-aports+unsubscribe@lists.alpinelinux.org
Help:         alpine-aports+help@lists.alpinelinux.org
---