| Message ID | 20200420095738.16104-1-heiko.thiery@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/1] package/syrepo: fix SysV init script | expand |
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
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.
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
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.
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
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!
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"
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(-)