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

[Thomas Petazzoni]

Hello,

On Mon, 3 Feb 2020 15:12:53 +0100
Ignacy Gawędzki <ignacy.gawedzki at green-communications.fr> wrote:

> > > 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.

The problem that you're exposing here doesn't seem to be specific to
openssh, so I think we don't want to solve it specifically for openssh.

> > 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.

I meant to make sure the init script looks as much as possible like
S01syslogd, i.e same indentation, same organization, same logic. As I
said, Carlos started an effort to try to make our init scripts more
consistent between each other, so let's try to go in that direction :-)

> > 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.

We had a brief look at it with Peter Korsgaard, we don't see why it
would be needed. Could you include a preparation patch in your series
that drops this touch+rm of /var/lock/sshd ?

Thanks!

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