X-Original-To: alpine-aports@lists.alpinelinux.org Received: from mail-ua0-f177.google.com (mail-ua0-f177.google.com [209.85.217.177]) by lists.alpinelinux.org (Postfix) with ESMTP id 95D535C43CC for ; Mon, 27 Feb 2017 08:35:43 +0000 (GMT) Received: by mail-ua0-f177.google.com with SMTP id e4so14520805uae.0 for ; Mon, 27 Feb 2017 00:35:43 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=6OeTVHC+RGgQu0xrMLWQVpsXeDruJezrxn8L88NeNds=; b=Nl4yHID/nAcrpHnFEtmt67gIdiw4WGqbGog8glEuh4JgUgQBtpgUI9cQhqHjtwNQQO ewc0wrEd4j2jaNPY95QbjTbzKlHoYYdQJaO8RLgsB+CQ2agK7o80eoFxXiPZG4SzOtiC oIwKEzSif00reUGbj0IO5RV8VkRCTcu6NhPFKd2HDVtY+G9Gt+i5Cg7dT/PMPeY+cFc9 NYpEAX8FA7mM8U6j7GlFBzGD8JDxCPrG2p0yrcvdQXTgvIzTd12Dq3kfONehVJ7P/1KZ LYACkWg06jkyklRMtEfLizTJVSZ8zpTxeFiudeMo9QJaDNyQNLkzZXV57euX0HgBe/B/ qKHw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=6OeTVHC+RGgQu0xrMLWQVpsXeDruJezrxn8L88NeNds=; b=HYeXZvmFg3fShhIWtnfpJYZ8po1r4cTr7BcbQVBKlzSH4GhLFUt3xKR6+4XTLE8hlw xK9jDmMTrKfRf60uX74ylq9cN8es+tUDRy7f4fRhw7GahtIPXsbZr3PwlfIQuIdn5hlN 48CIQWeSjofci9QtzEpc+G710WQvLCyLSj1H6ayFPXeY/qXIbRliO/RWMarPTVFJeKm1 Ch58vrk9nF3XK3G758/ZItnkUkYboxqNtBZ2SuXReeZCUzL/CjrHamqso1R/pIJ2fUO2 EMuCwTF5pTBNn5mHWI86qxJKt1SRNOjjjKKmYPzRL1nNG6Qlm7GDrR1Gz29DXcxGRlyI O4zQ== X-Gm-Message-State: AMke39k7vGCeJJBsL9PzGIFaeM8hz510xcX8nTEPEFsaK1QH3tVjK0g6cOTxjnFMCcX5OQL9mlQSqpQHPmpHDQ== X-Received: by 10.176.80.72 with SMTP id z8mr1670040uaz.139.1488184543039; Mon, 27 Feb 2017 00:35:43 -0800 (PST) X-Mailinglist: alpine-aports Precedence: list List-Id: Alpine Development List-Unsubscribe: List-Post: List-Help: List-Subscribe: MIME-Version: 1.0 Received: by 10.31.140.138 with HTTP; Mon, 27 Feb 2017 00:35:42 -0800 (PST) In-Reply-To: <20170225190143.5b599511@ncopa-desktop.copa.dup.pw> References: <20170224143718.22639-1-valery.kartel@gmail.com> <20170224164839.47bce33d@ncopa-desktop.copa.dup.pw> <20170225190143.5b599511@ncopa-desktop.copa.dup.pw> From: Valery Kartel Date: Mon, 27 Feb 2017 10:35:42 +0200 Message-ID: Subject: Re: [alpine-aports] [PATCH] testing/rspamd: upgrade to 1.4.4 To: Natanael Copa Cc: alpine-aports Content-Type: multipart/alternative; boundary=94eb2c1925560c622c05497ef791 --94eb2c1925560c622c05497ef791 Content-Type: text/plain; charset=UTF-8 got it. I'll remade init-script. 2017-02-25 20:01 GMT+02:00 Natanael Copa : > On Fri, 24 Feb 2017 21:56:33 +0200 > Valery Kartel 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 : > > > > > On Fri, 24 Feb 2017 16:37:18 +0200 > > > Valery Kartel 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 > > > > > --94eb2c1925560c622c05497ef791 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable
got it. I'll remade init-script.

2017-02-25 20:01 GMT+02:00 = Natanael Copa <ncopa@alpinelinux.org>:
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() co= vered
> 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:
>
> =3D=3D=3D
> extra_commands=3D"checkconfig"
> description_checkconfig=3D"Verify configuration file" > ....
> checkconfig() {
>=C2=A0 =C2=A0 =C2=A0ebegin "Checking $RC_SVCNAME config"
>=C2=A0 =C2=A0 =C2=A0startuplog=3D/dev/tty; start_pre
>=C2=A0 =C2=A0 =C2=A0eend $?
> }
> =3D=3D=3D
>
> 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 do= es 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() {
=C2=A0 =C2=A0 =C2=A0 =C2=A0 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<= br> 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 () {
=C2=A0 =C2=A0 =C2=A0 =C2=A0 [ "$RC_CMD" =3D &quo= t;start" ] && checkpath -d -m 750 -o $user:$group ${pidfile%/*= }
=C2=A0 =C2=A0 =C2=A0 =C2=A0 $command $command_args -t >/dev/null 2>&g= t;${startuplog:-/dev/null}
}

The function name indicates that this is something that happens befo= re
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() {
=C2=A0 =C2=A0 =C2=A0 =C2=A0 $command $command_args -t
}

start_pre () {
=C2=A0 =C2=A0 =C2=A0 =C2=A0 checkpath -d -m 750 -o $user:$= group ${pidfile%/*}
=C2=A0 =C2=A0 =C2=A0 =C2=A0 checkconfig >/dev/null 2>>${sta= rtuplog:-/dev/null}
}

It becomes much clearer what is going on.=C2=A0 We could drop checkconfig alltogether and just do "$command -t" all the places we need to c= heck the
config, but "$command -t" is not a good description of what is ac= tually
happening. By wrapping it in a function named "checkconfig" it be= cause
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 f= airly
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" =3D "start" ] condit= ional 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 <vale= ry.kartel@gmail.com> wrote:
> >
> > > fixes in init script
> > > ---
> > >=C2=A0 testing/rspamd/APKBUILD=C2=A0 =C2=A0 =C2=A0| 18 +++---= ------------
> > >=C2=A0 testing/rspamd/rspamd.initd | 10 ++++------
> > >=C2=A0 2 files changed, 7 insertions(+), 21 deletions(-)
> > >
> >
> > ...
> >
> > > diff --git a/testing/rspamd/rspamd.initd b/testing/rspamd/rs= pamd.initd
> > > index 93148ccb4b..b0eb9b057a 100644
> > > --- a/testing/rspamd/rspamd.initd
> > > +++ b/testing/rspamd/rspamd.initd
> > > @@ -18,20 +18,18 @@ depend() {
> > >=C2=A0 }
> > >
> > >=C2=A0 start_pre() {
> > > -=C2=A0 =C2=A0 =C2=A0ebegin
> > > -=C2=A0 =C2=A0 =C2=A0checkpath --directory --mode 750 --owne= r $user:$group ${pidfile%/*}
> > > +=C2=A0 =C2=A0 =C2=A0[ "$RC_CMD" =3D "start&q= uot; ] && checkpath -d -m 750 -o $user:$group
> > ${pidfile%/*}
> > >=C2=A0 =C2=A0 =C2=A0 =C2=A0$command $command_args -t >/dev= /null 2>>${startuplog:-/dev/null}
> > > -=C2=A0 =C2=A0 =C2=A0eend $?
> > >=C2=A0 }
> > >
> > >=C2=A0 reload() {
> > > -=C2=A0 =C2=A0 =C2=A0ebegin "Reloading ${SVCNAME}"=
> > > -=C2=A0 =C2=A0 =C2=A0start-stop-daemon --signal HUP --pidfil= e $pidfile
> > > +=C2=A0 =C2=A0 =C2=A0ebegin "Reloading $RC_SVCNAME conf= ig"
> > > +=C2=A0 =C2=A0 =C2=A0start_pre && start-stop-daemon = --signal HUP --pidfile $pidfile
> > >=C2=A0 =C2=A0 =C2=A0 =C2=A0eend $?
> > >=C2=A0 }
> > >
> > >=C2=A0 reopen() {
> > > -=C2=A0 =C2=A0 =C2=A0ebegin "Reopening ${SVCNAME} log f= iles"
> > > +=C2=A0 =C2=A0 =C2=A0ebegin "Reopening $RC_SVCNAME log = files"
> > >=C2=A0 =C2=A0 =C2=A0 =C2=A0start-stop-daemon --signal USR1 --= pidfile $pidfile
> > >=C2=A0 =C2=A0 =C2=A0 =C2=A0eend $?
> > >=C2=A0 }
> >
> >
> > I would have prefered:
> >
> > checkconfig() {
> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0$command $command_args -t >/d= ev/null 2>>${startuplog:-/dev/null}
> > }
> >
> > start_pre() {
> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0checkpath --directory --mode 750= --owner $user:$group ${pidfile%/*}
> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0checkconfig
> > }
> >
> > reload() {
> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0ebegin "Reloading $RC_SVCNA= ME config"
> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0checkconfig && start-sto= p-daemon --signal HUP --pidfile $pidfile
> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0eend $?
> > }
> >
> > -nc
> >


--94eb2c1925560c622c05497ef791-- --- Unsubscribe: alpine-aports+unsubscribe@lists.alpinelinux.org Help: alpine-aports+help@lists.alpinelinux.org ---