[Buildroot] [PATCH 1/1] package/syrepo: fix SysV init script

[Buildroot] [PATCH 1/1] package/syrepo: fix SysV init script

[Heiko Thiery]

The sysrepo-plugind does not support to be controlled via the PID by the
start-stop-daemon. This is because sysrepo-plugind does a fork.
Thus the daemon is running with another PID known by start-stop-daemon.

Now use the '-x' option of start-stop-daemon to stop.

Signed-off-by: Heiko Thiery <heiko.thiery@gmail.com>
---
 package/sysrepo/S51sysrepo-plugind | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/package/sysrepo/S51sysrepo-plugind b/package/sysrepo/S51sysrepo-plugind
index 74b68396bf..9e15da59f9 100644
--- a/package/sysrepo/S51sysrepo-plugind
+++ b/package/sysrepo/S51sysrepo-plugind
@@ -1,7 +1,6 @@
 #!/bin/sh
 
 DAEMON="sysrepo-plugind"
-PIDFILE="/var/run/$DAEMON.pid"
 
 SYSREPO_PLUGIND_ARGS=""
 
@@ -23,7 +22,7 @@ start() {
 
 stop() {
 	printf 'Stopping %s: ' "$DAEMON"
-	start-stop-daemon -K -q -p $PIDFILE
+	start-stop-daemon -K -q -x "/usr/bin/$DAEMON"
 	status=$?
 	if [ "$status" -eq 0 ]; then
 		echo "OK"
-- 
2.20.1

[Buildroot] [PATCH 1/1] package/syrepo: fix SysV init script

[Thomas Petazzoni]

Hello Heiko,

Minor nit: typo in commit title: syrepo -> sysrepo. More questions below.

On Mon, 20 Apr 2020 11:57:38 +0200
Heiko Thiery <heiko.thiery@gmail.com> wrote:

> The sysrepo-plugind does not support to be controlled via the PID by the
> start-stop-daemon. This is because sysrepo-plugind does a fork.
> Thus the daemon is running with another PID known by start-stop-daemon.

"known" by start-stop-daemon, or "not known" ?

Also, it is not very clear. Indeed, in the "start" step, you're not
asking start-stop-daemon to create a PID file using the -m option. So
you're relying on the daemon to create one. Is this PID file incorrect
? Normally, when it is created by the daemon itself, even if the daemon
forks, it should be fine.

Otherwise, can you ask the daemon not to fork, and let
start-stop-daemon take care of this ?

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH 1/1] package/syrepo: fix SysV init script

[Heiko Thiery]

Hi Thomas and all,

Am Sa., 25. Apr. 2020 um 16:22 Uhr schrieb Thomas Petazzoni
<thomas.petazzoni@bootlin.com>:
>
> Hello Heiko,
>
> Minor nit: typo in commit title: syrepo -> sysrepo. More questions below.
>
> On Mon, 20 Apr 2020 11:57:38 +0200
> Heiko Thiery <heiko.thiery@gmail.com> wrote:
>
> > The sysrepo-plugind does not support to be controlled via the PID by the
> > start-stop-daemon. This is because sysrepo-plugind does a fork.
> > Thus the daemon is running with another PID known by start-stop-daemon.
>
> "known" by start-stop-daemon, or "not known" ?

Ok, I was unclear. The PID used by the daemon is another one that the
start-stop-daemon knows.

>
> Also, it is not very clear. Indeed, in the "start" step, you're not
> asking start-stop-daemon to create a PID file using the -m option. So
> you're relying on the daemon to create one. Is this PID file incorrect
> ? Normally, when it is created by the daemon itself, even if the daemon
> forks, it should be fine.

I recognized that and tried to do it that way (with the -m option).
But in doing so I figured out that the "stop" step does not work. The
daemon is running by a PID that is +1 compared to the one the
start-stop-daemon knows from the PIDFILE.

>
> Otherwise, can you ask the daemon not to fork, and let
> start-stop-daemon take care of this ?

When I ask the daemon not to fork the output does not go any longer to
the syslog. Instead it goes to stderr [1]. I dont think this is the
way it should

[1] https://github.com/sysrepo/sysrepo/blob/master/src/executables/sysrepo-plugind.c#L78


>
> Thomas
> --
> Thomas Petazzoni, CTO, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com

Thank you for the review.

-- 
Heiko

[Buildroot] [PATCH 1/1] package/syrepo: fix SysV init script

[Thomas Petazzoni]

On Sat, 25 Apr 2020 17:26:49 +0200
Heiko Thiery <heiko.thiery@gmail.com> wrote:

> > > The sysrepo-plugind does not support to be controlled via the PID by the
> > > start-stop-daemon. This is because sysrepo-plugind does a fork.
> > > Thus the daemon is running with another PID known by start-stop-daemon.  
> >
> > "known" by start-stop-daemon, or "not known" ?  
> 
> Ok, I was unclear. The PID used by the daemon is another one that the
> start-stop-daemon knows.
> 
> >
> > Also, it is not very clear. Indeed, in the "start" step, you're not
> > asking start-stop-daemon to create a PID file using the -m option. So
> > you're relying on the daemon to create one. Is this PID file incorrect
> > ? Normally, when it is created by the daemon itself, even if the daemon
> > forks, it should be fine.  
> 
> I recognized that and tried to do it that way (with the -m option).
> But in doing so I figured out that the "stop" step does not work. The
> daemon is running by a PID that is +1 compared to the one the
> start-stop-daemon knows from the PIDFILE.

But is sysrepo-plugind creating its own pidfile ?

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH 1/1] package/syrepo: fix SysV init script

[Heiko Thiery]

Hi Thomas,

Am Sa., 25. Apr. 2020 um 21:37 Uhr schrieb Thomas Petazzoni
<thomas.petazzoni@bootlin.com>:
>
> On Sat, 25 Apr 2020 17:26:49 +0200
> Heiko Thiery <heiko.thiery@gmail.com> wrote:
>
> > > > The sysrepo-plugind does not support to be controlled via the PID by the
> > > > start-stop-daemon. This is because sysrepo-plugind does a fork.
> > > > Thus the daemon is running with another PID known by start-stop-daemon.
> > >
> > > "known" by start-stop-daemon, or "not known" ?
> >
> > Ok, I was unclear. The PID used by the daemon is another one that the
> > start-stop-daemon knows.
> >
> > >
> > > Also, it is not very clear. Indeed, in the "start" step, you're not
> > > asking start-stop-daemon to create a PID file using the -m option. So
> > > you're relying on the daemon to create one. Is this PID file incorrect
> > > ? Normally, when it is created by the daemon itself, even if the daemon
> > > forks, it should be fine.
> >
> > I recognized that and tried to do it that way (with the -m option).
> > But in doing so I figured out that the "stop" step does not work. The
> > daemon is running by a PID that is +1 compared to the one the
> > start-stop-daemon knows from the PIDFILE.
>
> But is sysrepo-plugind creating its own pidfile ?

No it doesn't. The confusion is due to my bad commit message ;-/ I try
to explain it better.

The current script is not able to stop the daemon.

Options to fix the problem:

A) By adding the "-m -p $PIDFILE" option the pid file will be created
but it will not contain the correct PID used by the daemon. This is
obviously because the daemon forks.
B) By not starting the daemon in background (sysrepo-plugind -d) and
let do it by start-stop-daemon with "-b" option the log messages of
the daemon will not longer ends in the syslog but to stderr.
C) start the daemon without a pidfile and stop the daemon with the "-x" option.

I think the best option is C. This is what the patch does.

-- 
Heiko

[Buildroot] [PATCH 1/1] package/syrepo: fix SysV init script

[Thomas Petazzoni]

On Sat, 25 Apr 2020 21:58:56 +0200
Heiko Thiery <heiko.thiery@gmail.com> wrote:

> No it doesn't. The confusion is due to my bad commit message ;-/ I try
> to explain it better.
> 
> The current script is not able to stop the daemon.
> 
> Options to fix the problem:
> 
> A) By adding the "-m -p $PIDFILE" option the pid file will be created
> but it will not contain the correct PID used by the daemon. This is
> obviously because the daemon forks.
> B) By not starting the daemon in background (sysrepo-plugind -d) and
> let do it by start-stop-daemon with "-b" option the log messages of
> the daemon will not longer ends in the syslog but to stderr.
> C) start the daemon without a pidfile and stop the daemon with the "-x" option.
> 
> I think the best option is C. This is what the patch does.

Absolutely :-) I'll update the commit message with this explanation
when applying.

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH 1/1] package/syrepo: fix SysV init script

[Heiko Thiery]

Am Sa., 25. Apr. 2020 um 22:07 Uhr schrieb Thomas Petazzoni
<thomas.petazzoni@bootlin.com>:
>
> On Sat, 25 Apr 2020 21:58:56 +0200
> Heiko Thiery <heiko.thiery@gmail.com> wrote:
>
> > No it doesn't. The confusion is due to my bad commit message ;-/ I try
> > to explain it better.
> >
> > The current script is not able to stop the daemon.
> >
> > Options to fix the problem:
> >
> > A) By adding the "-m -p $PIDFILE" option the pid file will be created
> > but it will not contain the correct PID used by the daemon. This is
> > obviously because the daemon forks.
> > B) By not starting the daemon in background (sysrepo-plugind -d) and
> > let do it by start-stop-daemon with "-b" option the log messages of
> > the daemon will not longer ends in the syslog but to stderr.
> > C) start the daemon without a pidfile and stop the daemon with the "-x" option.
> >
> > I think the best option is C. This is what the patch does.
>
> Absolutely :-) I'll update the commit message with this explanation
> when applying.
>
> Thanks!

Thanks a lot!