Precedence: list
MIME-Version: 1.0
References: <F61YXLPGSM.1YB7GLGGYUHJH@firemail.cc>
In-Reply-To: <F61YXLPGSM.1YB7GLGGYUHJH@firemail.cc>
From: Leonardo Arena <rnalrd@gmail.com>
Date: Fri, 26 Oct 2018 15:43:43 +0200
Message-ID: <CAGG_d8Ddf5k1rZsxAT1UZ20WLVTKShU=VuZb7ZCVHQ94gpOWMg@mail.gmail.com>
Subject: Re: [alpine-aports] [PATCH] main/darkhttpd: Fix multiple instances support
To: milliardo@firemail.cc
Cc: alpine-aports <alpine-aports@lists.alpinelinux.org>
Content-Type: multipart/alternative; boundary="00000000000015b3c9057921e9e7"

--00000000000015b3c9057921e9e7
Content-Type: text/plain; charset="UTF-8"

Hi,

thank you for your patch. Please look at some comments below.

On Thu, Oct 25, 2018 at 5:15 PM Milliardo Peacecraft <milliardo@firemail.cc>
wrote:

> 5fd553eceec8325f829089d13e5828e002393438 was supposed to allow
> multiple instances, but given that it defines `procname`, SIGTERM
> gets delivered to all running instances. Instead, don't daemonize
> darkhttpd and let start-stop-daemon(8) handle process forking and
> pidfile creation. This was chosen over using `--pidfile` flag of
> darkhttpd because pidfile is created inside the chroot in such
> case.
>
> Signed-off-by: Milliardo Peacecraft <milliardo@firemail.cc>
> ---
>  main/darkhttpd/darkhttpd.initd | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/main/darkhttpd/darkhttpd.initd
> b/main/darkhttpd/darkhttpd.initd
> index b16244b25c..38d33f3a69 100644
> --- a/main/darkhttpd/darkhttpd.initd
> +++ b/main/darkhttpd/darkhttpd.initd
> @@ -2,10 +2,9 @@
>
>  description="darkhttpd web server"
>  command="/usr/bin/darkhttpd"
> -command_args="${document_root:-/var/www/localhost/htdocs} --chroot
> --daemon --uid darkhttpd --gid www-data $darkhttpd_args"
> -procname="darkhttpd"
> -pidfile=""
> -stopsig="SIGTERM"
> +command_args="${document_root:-/var/www/localhost/htdocs} --chroot --uid
> darkhttpd --gid www-data $darkhttpd_args"
> +command_background="true"
> +pidfile="/run/$SVCNAME.pid"
>

Looking at sources and man page I think $SVCNAME is deprecated, $RC_SVCNAME
should be used instead.
Also, since it does not run as root it would be preferrable to place the
pidfile under "/run/darkhttpd" and use "checkpath" from init in order to
create $rundir with the right uid/gid. Since the pidfile is created by
openrc, the $pidfile creation is successful anyway in this case.

Also please remember to bump $pkgrel in APKBUILD for any change.

Thanks!

/eo

--00000000000015b3c9057921e9e7
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

<div dir=3D"ltr"><div>Hi,</div><div><br></div><div>thank you for your patch=
. Please look at some comments below.<br></div><br><div class=3D"gmail_quot=
e"><div dir=3D"ltr">On Thu, Oct 25, 2018 at 5:15 PM Milliardo Peacecraft &l=
t;milliardo@firemail.cc&gt; wrote:<br></div><blockquote class=3D"gmail_quot=
e" style=3D"margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">=
5fd553eceec8325f829089d13e5828e002393438 was supposed to allow<br>
multiple instances, but given that it defines `procname`, SIGTERM<br>
gets delivered to all running instances. Instead, don&#39;t daemonize<br>
darkhttpd and let start-stop-daemon(8) handle process forking and<br>
pidfile creation. This was chosen over using `--pidfile` flag of<br>
darkhttpd because pidfile is created inside the chroot in such<br>
case.<br>
<br></blockquote><blockquote class=3D"gmail_quote" style=3D"margin:0 0 0 .8=
ex;border-left:1px #ccc solid;padding-left:1ex">
Signed-off-by: Milliardo Peacecraft &lt;milliardo@firemail.cc&gt;<br>
---<br>
=C2=A0main/darkhttpd/darkhttpd.initd | 7 +++----<br>
=C2=A01 file changed, 3 insertions(+), 4 deletions(-)<br>
<br>
diff --git a/main/darkhttpd/darkhttpd.initd b/main/darkhttpd/darkhttpd.init=
d<br>
index b16244b25c..38d33f3a69 100644<br>
--- a/main/darkhttpd/darkhttpd.initd<br>
+++ b/main/darkhttpd/darkhttpd.initd<br>
@@ -2,10 +2,9 @@<br>
<br>
=C2=A0description=3D&quot;darkhttpd web server&quot;<br>
=C2=A0command=3D&quot;/usr/bin/darkhttpd&quot;<br>
-command_args=3D&quot;${document_root:-/var/www/localhost/htdocs} --chroot =
--daemon --uid darkhttpd --gid www-data $darkhttpd_args&quot;<br>
-procname=3D&quot;darkhttpd&quot;<br>
-pidfile=3D&quot;&quot;<br>
-stopsig=3D&quot;SIGTERM&quot;<br>
+command_args=3D&quot;${document_root:-/var/www/localhost/htdocs} --chroot =
--uid darkhttpd --gid www-data $darkhttpd_args&quot;<br>
+command_background=3D&quot;true&quot;<br>
+pidfile=3D&quot;/run/$SVCNAME.pid&quot;<br></blockquote><div><br></div><di=
v>Looking at sources and man page I think $SVCNAME is deprecated, $RC_SVCNA=
ME should be used instead.</div><div>Also, since it does not run as root it=
 would be preferrable to place the pidfile under &quot;/run/darkhttpd&quot;=
 and use &quot;checkpath&quot; from init in order to create $rundir with th=
e right uid/gid. Since the pidfile is created by openrc, the $pidfile creat=
ion is successful anyway in this case.</div><div><br></div><div>Also please=
 remember to bump $pkgrel in APKBUILD for any change.<br></div></div><div c=
lass=3D"gmail_quote"><br></div><div class=3D"gmail_quote">Thanks!</div><div=
 class=3D"gmail_quote"><br></div><div class=3D"gmail_quote">/eo<br></div></=
div>

--00000000000015b3c9057921e9e7--


---
Unsubscribe:  alpine-aports+unsubscribe@lists.alpinelinux.org
Help:         alpine-aports+help@lists.alpinelinux.org
---