Mail archive
alpine-aports

Re: [alpine-aports] [PATCH] testing/rspamd: upgrade to 1.4.4

From: Valery Kartel <valery.kartel_at_gmail.com>
Date: Mon, 27 Feb 2017 10:35:42 +0200

got it. I'll remade init-script.

2017-02-25 20:01 GMT+02:00 Natanael Copa <ncopa_at_alpinelinux.org>:

> On Fri, 24 Feb 2017 21:56:33 +0200
> Valery Kartel <valery.kartel_at_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_at_alpinelinux.org>:
> >
> > > On Fri, 24 Feb 2017 16:37:18 +0200
> > > Valery Kartel <valery.kartel_at_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
> > > > _at__at_ -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_at_lists.alpinelinux.org
Help:         alpine-aports+help_at_lists.alpinelinux.org
---
Received on Mon Feb 27 2017 - 10:35:42 GMT