[Buildroot] [PATCH 1/1] package/openssh: improve init script

Ignacy Gawędzki ignacy.gawedzki at green-communications.fr
Mon Feb 3 14:12:53 UTC 2020


On Mon, Feb 03, 2020 at 02:27:21PM +0100, thus spake Thomas Petazzoni:
> Hello Ignacy,

Hi,

> Thanks a lot for working on the openssh init script. I'm adding in Cc:
> Carlos Santos, who worked on a template for our init scripts, and is
> generally trying to make our init script more consistent.
> 
> See some comments below.

See my replies below them.

> On Thu, 23 Jan 2020 13:17:55 +0100
> Ignacy Gawędzki <ignacy.gawedzki at green-communications.fr> wrote:
> 
> > The current init script for sshd is too simplistic and its use of
> > killall to terminate the daemon has the annoying downside of killing
> > every instance of sshd, possibly including the one spawned for the
> > interactive session in which the script itself is started.  If the
> > intention was to simply restart the daemon, killing the current
> > session ultimately kills the script and the daemon is not properly
> > started again.
> > 
> > Improve the init script in the following ways:
> > 
> >   Use start-stop-daemon to avoid running the daemon more than once and
> >   to safely send termination signals to the right process.
> > 
> >   Add a proper reload action, by sending the daemon the SIGHUP signal.
> 
> This should ideally be done in a separate patch.
> 
> >   During restart, check that the daemon has properly terminated and if
> >   not, wait one second before sending it the SIGKILL signal.
> 
> We never do that in any of our other init scripts, I'm not sure we want
> to do that here. Is there any reason to do that?

Of course, if start-stop-daemon -S finds a matching running process,
it returns an error.  Sending a SIGTERM to the running process during
the stop phase doesn't guarantee that the process will have already terminated
during the start phase, even after sleeping.  If I could use
the --retry option of standalone start-stop-daemon, I would.  But with
the assumption of Busybox, I need to make sure that the daemon has
terminated by the time I attempt to start it again.

> > Signed-off-by: Ignacy Gawędzki <ignacy.gawedzki at green-communications.fr>
> > ---
> >  package/openssh/S50sshd | 29 ++++++++++++++++++++---------
> >  1 file changed, 20 insertions(+), 9 deletions(-)
> 
> Overall, could you look at package/busybox/S01syslogd, and use it as a
> template for S50openssh ?

If you mean that I should be testing for start-stop-daemon's return
code explicitly by setting status=$? and using if-then-else-fi
constructs, then sure, yes.

> 
> > diff --git a/package/openssh/S50sshd b/package/openssh/S50sshd
> > index 22da41d1ca..66cdd5e291 100644
> > --- a/package/openssh/S50sshd
> > +++ b/package/openssh/S50sshd
> > @@ -13,20 +13,29 @@ start() {
> >  	/usr/bin/ssh-keygen -A
> >  
> >  	printf "Starting sshd: "
> > -	/usr/sbin/sshd
> > -	touch /var/lock/sshd
> > -	echo "OK"
> > +	start-stop-daemon -S -q -x /usr/sbin/sshd -p /run/sshd.pid &&
> > +	{ touch /var/lock/sshd; echo "OK"; } || echo "FAIL"
> 
> I am a bit confused by this touch /var/lock/sshd. Is it really
> necessary? The git history was not very helpful on this, as it has been
> in Buildroot since the init script was introduced years ago. The fact
> that it is created *after* the sshd daemon is started makes it really
> weird.
> 
> Do you have any comment/insight on this ?

Quite frankly, I have no idea why it's there and by keeping it, I just
wanted stay compatible with whatever is possibly using it.

Cheers,

Ignacy

-- 
Ignacy Gawędzki
R&D Engineer
Green Communications


More information about the buildroot mailing list