[Buildroot] [PATCH v2 0/4] Refactor OpenSSH init.d script

[Buildroot] [PATCH v2 0/4] Refactor OpenSSH init.d script

[Fiona Klute via buildroot]

From: "Fiona Klute (WIWA)" <fiona.klute@gmx.de>

This series is the result of running "/etc/init.d/S50sshd restart"
over an SSH connection and finding that not only it destroyed my SSH
session (this is no longer issue with OpenSSH 9.8), the SSH server
also did not come back up. The main issue there was the use of
"killall sshd" instead of stopping only the listening server (patch
1). I assume restarting the SSH server on a Buildroot-created system
is not a common use case, I need a reload to reset its state during
automated tests.

I took the opportunity to make some more improvements: style cleanup
(patch 2), implementing "reload" using SIGHUP (patch 3), and checking
for errors during service start (patch 4).

The added checks for a currently running service before start/stop is
a slight change in behavior, there will be a non-zero exit code when
trying to start when sshd is already running, or stop when it's not. I
consider this useful to find errors in scripts instead of masking
them, but I don't mind removing that if it's considered a problem.

It'd be easy enough to remove the checkpatch error about
DAEMON/PIDFILE variables, too, but I'm not sure if that would make
sense: While the PIDFILE matches the required pattern it's not set by
the init script, it's the sshd default.

Changes v1 -> v2:
* Acknowledge changed session binary with OpenSSH 9.8 in commit
  message (patch 1)
* Check sshd exit code on service start (patch 4)

Fiona Klute (WIWA) (4):
  package/openssh: don't kill open sessions on restart
  package/openssh: fix init script indentation
  package/openssh: implement "reload" using SIGHUP
  package/openssh: check error conditions on "start"

 .checkpackageignore     |  2 +-
 package/openssh/S50sshd | 69 ++++++++++++++++++++++++++++++-----------
 2 files changed, 52 insertions(+), 19 deletions(-)

--
2.45.2

_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

[Buildroot] [PATCH v2 1/4] package/openssh: don't kill open sessions on restart

[Fiona Klute via buildroot]

From: "Fiona Klute (WIWA)" <fiona.klute@gmx.de>

The previously used "killall sshd" stopped all instances of sshd. With
OpenSSH before 9.8 that meant not only the listening server, but also
instances serving currently open sessions, possibly including the one
used to send the restart command, preventing it from completing the
"start" part of "restart" and leaving the system unreachable over SSH.

Instead, use the PID file that sshd creates by default to target only
the listening server. This leaves any open SSH sessions unaffected.

With OpenSSH 9.8 open sessions are handled by the separate
sshd-session binary so killall does not hit them any more, but
a targeted "kill" command is still desirable.

Signed-off-by: Fiona Klute (WIWA) <fiona.klute@gmx.de>
---
Changes v1 -> v2:
* Acknowledge changed session binary with OpenSSH 9.8 in commit
  message

 .checkpackageignore     |  2 +-
 package/openssh/S50sshd | 19 +++++++++++++++----
 2 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/.checkpackageignore b/.checkpackageignore
index 1782161337..9c4a760117 100644
--- a/.checkpackageignore
+++ b/.checkpackageignore
@@ -1150,7 +1150,7 @@ package/openrc/0003-init.d-agetty-replace-sbin-agetty-by-sbin-getty.patch lib_pa
 package/openrc/0004-init.d-agetty-start-agetty-after-all-sevices.patch lib_patch.Upstream
 package/openrc/0005-runlevels-do-not-add-agetty.tty-1-6-if-MKSYSVINIT-ye.patch lib_patch.Upstream
 package/openrc/0006-Also-create-run-lock-subsys-directory.patch lib_patch.Upstream
-package/openssh/S50sshd lib_sysv.EmptyLastLine lib_sysv.Indent lib_sysv.Variables
+package/openssh/S50sshd lib_sysv.Indent lib_sysv.Variables
 package/openswan/0001-lib-libopenswan-constants.c-workaround-missing-ns_t_.patch lib_patch.Upstream
 package/opentyrian/0001-Move-definitions-that-don-t-need-to-be-exposed-from-opl-h-to-opl-c.patch lib_patch.Upstream
 package/openvmtools/0001-no_cflags_werror.patch lib_patch.Upstream
diff --git a/package/openssh/S50sshd b/package/openssh/S50sshd
index 22da41d1ca..f1ff9dfec1 100644
--- a/package/openssh/S50sshd
+++ b/package/openssh/S50sshd
@@ -3,6 +3,8 @@
 # sshd        Starts sshd.
 #

+PIDFILE="/var/run/sshd.pid"
+
 # Make sure the ssh-keygen progam exists
 [ -f /usr/bin/ssh-keygen ] || exit 0

@@ -14,15 +16,25 @@ start() {

 	printf "Starting sshd: "
 	/usr/sbin/sshd
-	touch /var/lock/sshd
 	echo "OK"
 }
+
 stop() {
 	printf "Stopping sshd: "
-	killall sshd
-	rm -f /var/lock/sshd
+	pid="$( cat "${PIDFILE}" 2>/dev/null || true )"
+	if [ -z "${pid}" ]; then
+		echo "ERROR: not running"
+		return 1
+	fi
+	kill "${pid}"
+	# SSHD deletes its PID file when exiting, wait for it to
+	# disappear.
+	while [ -f "${PIDFILE}" ]; do
+		sleep 0.1
+	done
 	echo "OK"
 }
+
 restart() {
 	stop
 	start
@@ -44,4 +56,3 @@ case "$1" in
 esac

 exit $?
-
--
2.45.2

_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH v2 1/4] package/openssh: don't kill open sessions on restart

[Yann E. MORIN]

Fiona, All,

On 2024-07-02 18:57 +0200, Fiona Klute via buildroot spake thusly:
> From: "Fiona Klute (WIWA)" <fiona.klute@gmx.de>
> 
> The previously used "killall sshd" stopped all instances of sshd. With
> OpenSSH before 9.8 that meant not only the listening server, but also
> instances serving currently open sessions, possibly including the one
> used to send the restart command, preventing it from completing the
> "start" part of "restart" and leaving the system unreachable over SSH.
> 
> Instead, use the PID file that sshd creates by default to target only
> the listening server. This leaves any open SSH sessions unaffected.

Rather than play with the PID ourselves, can we use start-stop-daemon
instead? Currently, we run sshd and it daemonises all by itself, but if
we pass -D, then start-stop-daemon will be able to do that, and handle
the PID file automatically for us.

Regards,
Yann E. MORIN.

> With OpenSSH 9.8 open sessions are handled by the separate
> sshd-session binary so killall does not hit them any more, but
> a targeted "kill" command is still desirable.
> 
> Signed-off-by: Fiona Klute (WIWA) <fiona.klute@gmx.de>
> ---
[--SNIP--]
> diff --git a/package/openssh/S50sshd b/package/openssh/S50sshd
> index 22da41d1ca..f1ff9dfec1 100644
> --- a/package/openssh/S50sshd
> +++ b/package/openssh/S50sshd
> @@ -3,6 +3,8 @@
>  # sshd        Starts sshd.
>  #
> 
> +PIDFILE="/var/run/sshd.pid"
> +
>  # Make sure the ssh-keygen progam exists
>  [ -f /usr/bin/ssh-keygen ] || exit 0
> 
> @@ -14,15 +16,25 @@ start() {
> 
>  	printf "Starting sshd: "
>  	/usr/sbin/sshd
> -	touch /var/lock/sshd
>  	echo "OK"
>  }
> +
>  stop() {
>  	printf "Stopping sshd: "
> -	killall sshd
> -	rm -f /var/lock/sshd
> +	pid="$( cat "${PIDFILE}" 2>/dev/null || true )"
> +	if [ -z "${pid}" ]; then
> +		echo "ERROR: not running"
> +		return 1
> +	fi
> +	kill "${pid}"
> +	# SSHD deletes its PID file when exiting, wait for it to
> +	# disappear.
> +	while [ -f "${PIDFILE}" ]; do
> +		sleep 0.1
> +	done
>  	echo "OK"
>  }
> +
>  restart() {
>  	stop
>  	start
> @@ -44,4 +56,3 @@ case "$1" in
>  esac
> 
>  exit $?
> -
> --
> 2.45.2
> 
> _______________________________________________
> buildroot mailing list
> buildroot@buildroot.org
> https://lists.buildroot.org/mailman/listinfo/buildroot

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH v2 1/4] package/openssh: don't kill open sessions on restart

[Arnout Vandecappelle via buildroot]

On 02/07/2024 22:15, Yann E. MORIN wrote:
> Fiona, All,
> 
> On 2024-07-02 18:57 +0200, Fiona Klute via buildroot spake thusly:
>> From: "Fiona Klute (WIWA)" <fiona.klute@gmx.de>
>>
>> The previously used "killall sshd" stopped all instances of sshd. With
>> OpenSSH before 9.8 that meant not only the listening server, but also
>> instances serving currently open sessions, possibly including the one
>> used to send the restart command, preventing it from completing the
>> "start" part of "restart" and leaving the system unreachable over SSH.
>>
>> Instead, use the PID file that sshd creates by default to target only
>> the listening server. This leaves any open SSH sessions unaffected.
> 
> Rather than play with the PID ourselves, can we use start-stop-daemon
> instead? Currently, we run sshd and it daemonises all by itself, but if
> we pass -D, then start-stop-daemon will be able to do that, and handle
> the PID file automatically for us.

  Note that there's a little disadvantage of that: when sshd daemonizes by 
itself, it will only return after the real daemon has started listening. If 
start-stop-daemon does the daemonisation, it will return immediately.

  I don't think this is important enough to not daemonise with start-stop-daemon 
- if you need fine-grain control by the init system you should use a proper init 
system. But it's important to be aware of the implied limitations.

  Regards,
  Arnout

> 
> Regards,
> Yann E. MORIN.
> 
>> With OpenSSH 9.8 open sessions are handled by the separate
>> sshd-session binary so killall does not hit them any more, but
>> a targeted "kill" command is still desirable.
>>
>> Signed-off-by: Fiona Klute (WIWA) <fiona.klute@gmx.de>
>> ---
> [--SNIP--]
>> diff --git a/package/openssh/S50sshd b/package/openssh/S50sshd
>> index 22da41d1ca..f1ff9dfec1 100644
>> --- a/package/openssh/S50sshd
>> +++ b/package/openssh/S50sshd
>> @@ -3,6 +3,8 @@
>>   # sshd        Starts sshd.
>>   #
>>
>> +PIDFILE="/var/run/sshd.pid"
>> +
>>   # Make sure the ssh-keygen progam exists
>>   [ -f /usr/bin/ssh-keygen ] || exit 0
>>
>> @@ -14,15 +16,25 @@ start() {
>>
>>   	printf "Starting sshd: "
>>   	/usr/sbin/sshd
>> -	touch /var/lock/sshd
>>   	echo "OK"
>>   }
>> +
>>   stop() {
>>   	printf "Stopping sshd: "
>> -	killall sshd
>> -	rm -f /var/lock/sshd
>> +	pid="$( cat "${PIDFILE}" 2>/dev/null || true )"
>> +	if [ -z "${pid}" ]; then
>> +		echo "ERROR: not running"
>> +		return 1
>> +	fi
>> +	kill "${pid}"
>> +	# SSHD deletes its PID file when exiting, wait for it to
>> +	# disappear.
>> +	while [ -f "${PIDFILE}" ]; do
>> +		sleep 0.1
>> +	done
>>   	echo "OK"
>>   }
>> +
>>   restart() {
>>   	stop
>>   	start
>> @@ -44,4 +56,3 @@ case "$1" in
>>   esac
>>
>>   exit $?
>> -
>> --
>> 2.45.2
>>
>> _______________________________________________
>> buildroot mailing list
>> buildroot@buildroot.org
>> https://lists.buildroot.org/mailman/listinfo/buildroot
> 
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH v2 1/4] package/openssh: don't kill open sessions on restart

[Fiona Klute via buildroot]

Am 03.07.24 um 09:01 schrieb Arnout Vandecappelle:
> On 02/07/2024 22:15, Yann E. MORIN wrote:
>> Fiona, All,
>>
>> On 2024-07-02 18:57 +0200, Fiona Klute via buildroot spake thusly:
>>> From: "Fiona Klute (WIWA)" <fiona.klute@gmx.de>
>>>
>>> The previously used "killall sshd" stopped all instances of sshd. With
>>> OpenSSH before 9.8 that meant not only the listening server, but also
>>> instances serving currently open sessions, possibly including the one
>>> used to send the restart command, preventing it from completing the
>>> "start" part of "restart" and leaving the system unreachable over SSH.
>>>
>>> Instead, use the PID file that sshd creates by default to target only
>>> the listening server. This leaves any open SSH sessions unaffected.
>>
>> Rather than play with the PID ourselves, can we use start-stop-daemon
>> instead? Currently, we run sshd and it daemonises all by itself, but if
>> we pass -D, then start-stop-daemon will be able to do that, and handle
>> the PID file automatically for us.
>
>   Note that there's a little disadvantage of that: when sshd daemonizes
> by itself, it will only return after the real daemon has started
> listening. If start-stop-daemon does the daemonisation, it will return
> immediately.

I looked closer at start-stop-daemon, and there's a solution that avoids
that problem and simplifies the init script, too: don't use "-D" for
sshd. start-stop-daemon expects the daemon to write its own PID file and
put itself into the background, options to handle a service that doesn't
are described as fallbacks in the man page.

I've tested it, and letting start-stop-daemon manage sshd that way works
perfectly fine. I also found it has a built-in option (--retry) to avoid
"has the service really stopped" loops. I'll send the refactored patch
set in a few minutes.

Thanks for the review!

Best regards,
Fiona

>   I don't think this is important enough to not daemonise with
> start-stop-daemon - if you need fine-grain control by the init system
> you should use a proper init system. But it's important to be aware of
> the implied limitations.
>
>   Regards,
>   Arnout
>
>>
>> Regards,
>> Yann E. MORIN.
>>
>>> With OpenSSH 9.8 open sessions are handled by the separate
>>> sshd-session binary so killall does not hit them any more, but
>>> a targeted "kill" command is still desirable.
>>>
>>> Signed-off-by: Fiona Klute (WIWA) <fiona.klute@gmx.de>
>>> ---
>> [--SNIP--]
>>> diff --git a/package/openssh/S50sshd b/package/openssh/S50sshd
>>> index 22da41d1ca..f1ff9dfec1 100644
>>> --- a/package/openssh/S50sshd
>>> +++ b/package/openssh/S50sshd
>>> @@ -3,6 +3,8 @@
>>>   # sshd        Starts sshd.
>>>   #
>>>
>>> +PIDFILE="/var/run/sshd.pid"
>>> +
>>>   # Make sure the ssh-keygen progam exists
>>>   [ -f /usr/bin/ssh-keygen ] || exit 0
>>>
>>> @@ -14,15 +16,25 @@ start() {
>>>
>>>       printf "Starting sshd: "
>>>       /usr/sbin/sshd
>>> -    touch /var/lock/sshd
>>>       echo "OK"
>>>   }
>>> +
>>>   stop() {
>>>       printf "Stopping sshd: "
>>> -    killall sshd
>>> -    rm -f /var/lock/sshd
>>> +    pid="$( cat "${PIDFILE}" 2>/dev/null || true )"
>>> +    if [ -z "${pid}" ]; then
>>> +        echo "ERROR: not running"
>>> +        return 1
>>> +    fi
>>> +    kill "${pid}"
>>> +    # SSHD deletes its PID file when exiting, wait for it to
>>> +    # disappear.
>>> +    while [ -f "${PIDFILE}" ]; do
>>> +        sleep 0.1
>>> +    done
>>>       echo "OK"
>>>   }
>>> +
>>>   restart() {
>>>       stop
>>>       start
>>> @@ -44,4 +56,3 @@ case "$1" in
>>>   esac
>>>
>>>   exit $?
>>> -
>>> --
>>> 2.45.2
>>>
>>> _______________________________________________
>>> buildroot mailing list
>>> buildroot@buildroot.org
>>> https://lists.buildroot.org/mailman/listinfo/buildroot
>>

--
Dipl.-Ing. Fiona Klute
Mollwitzer Str. 2
44141 Dortmund
Germany

USt.-ID/VAT number: DE363488944

_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH v2 1/4] package/openssh: don't kill open sessions on restart

[Arnout Vandecappelle via buildroot]

On 03/07/2024 17:46, Fiona Klute wrote:
> Am 03.07.24 um 09:01 schrieb Arnout Vandecappelle:
>> On 02/07/2024 22:15, Yann E. MORIN wrote:
>>> Fiona, All,
>>>
>>> On 2024-07-02 18:57 +0200, Fiona Klute via buildroot spake thusly:
>>>> From: "Fiona Klute (WIWA)" <fiona.klute@gmx.de>
>>>>
>>>> The previously used "killall sshd" stopped all instances of sshd. With
>>>> OpenSSH before 9.8 that meant not only the listening server, but also
>>>> instances serving currently open sessions, possibly including the one
>>>> used to send the restart command, preventing it from completing the
>>>> "start" part of "restart" and leaving the system unreachable over SSH.
>>>>
>>>> Instead, use the PID file that sshd creates by default to target only
>>>> the listening server. This leaves any open SSH sessions unaffected.
>>>
>>> Rather than play with the PID ourselves, can we use start-stop-daemon
>>> instead? Currently, we run sshd and it daemonises all by itself, but if
>>> we pass -D, then start-stop-daemon will be able to do that, and handle
>>> the PID file automatically for us.
>>
>>   Note that there's a little disadvantage of that: when sshd daemonizes
>> by itself, it will only return after the real daemon has started
>> listening. If start-stop-daemon does the daemonisation, it will return
>> immediately.
> 
> I looked closer at start-stop-daemon, and there's a solution that avoids
> that problem and simplifies the init script, too: don't use "-D" for
> sshd. start-stop-daemon expects the daemon to write its own PID file and
> put itself into the background, options to handle a service that doesn't
> are described as fallbacks in the man page.
> 
> I've tested it, and letting start-stop-daemon manage sshd that way works
> perfectly fine. I also found it has a built-in option (--retry) to avoid

  Does the --retry option exist in busybox's version?

  Regards,
  Arnout

> "has the service really stopped" loops. I'll send the refactored patch
> set in a few minutes.
> 
> Thanks for the review!
> 
> Best regards,
> Fiona
> 
>>   I don't think this is important enough to not daemonise with
>> start-stop-daemon - if you need fine-grain control by the init system
>> you should use a proper init system. But it's important to be aware of
>> the implied limitations.
>>
>>   Regards,
>>   Arnout
>>
>>>
>>> Regards,
>>> Yann E. MORIN.
>>>
>>>> With OpenSSH 9.8 open sessions are handled by the separate
>>>> sshd-session binary so killall does not hit them any more, but
>>>> a targeted "kill" command is still desirable.
>>>>
>>>> Signed-off-by: Fiona Klute (WIWA) <fiona.klute@gmx.de>
>>>> ---
>>> [--SNIP--]
>>>> diff --git a/package/openssh/S50sshd b/package/openssh/S50sshd
>>>> index 22da41d1ca..f1ff9dfec1 100644
>>>> --- a/package/openssh/S50sshd
>>>> +++ b/package/openssh/S50sshd
>>>> @@ -3,6 +3,8 @@
>>>>   # sshd        Starts sshd.
>>>>   #
>>>>
>>>> +PIDFILE="/var/run/sshd.pid"
>>>> +
>>>>   # Make sure the ssh-keygen progam exists
>>>>   [ -f /usr/bin/ssh-keygen ] || exit 0
>>>>
>>>> @@ -14,15 +16,25 @@ start() {
>>>>
>>>>       printf "Starting sshd: "
>>>>       /usr/sbin/sshd
>>>> -    touch /var/lock/sshd
>>>>       echo "OK"
>>>>   }
>>>> +
>>>>   stop() {
>>>>       printf "Stopping sshd: "
>>>> -    killall sshd
>>>> -    rm -f /var/lock/sshd
>>>> +    pid="$( cat "${PIDFILE}" 2>/dev/null || true )"
>>>> +    if [ -z "${pid}" ]; then
>>>> +        echo "ERROR: not running"
>>>> +        return 1
>>>> +    fi
>>>> +    kill "${pid}"
>>>> +    # SSHD deletes its PID file when exiting, wait for it to
>>>> +    # disappear.
>>>> +    while [ -f "${PIDFILE}" ]; do
>>>> +        sleep 0.1
>>>> +    done
>>>>       echo "OK"
>>>>   }
>>>> +
>>>>   restart() {
>>>>       stop
>>>>       start
>>>> @@ -44,4 +56,3 @@ case "$1" in
>>>>   esac
>>>>
>>>>   exit $?
>>>> -
>>>> -- 
>>>> 2.45.2
>>>>
>>>> _______________________________________________
>>>> buildroot mailing list
>>>> buildroot@buildroot.org
>>>> https://lists.buildroot.org/mailman/listinfo/buildroot
>>>
> 
> -- 
> Dipl.-Ing. Fiona Klute
> Mollwitzer Str. 2
> 44141 Dortmund
> Germany
> 
> USt.-ID/VAT number: DE363488944
> 
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH v2 1/4] package/openssh: don't kill open sessions on restart

[Fiona Klute via buildroot]

Am 04.07.24 um 10:00 schrieb Arnout Vandecappelle:
> On 03/07/2024 17:46, Fiona Klute wrote:
>> Am 03.07.24 um 09:01 schrieb Arnout Vandecappelle:
>>> On 02/07/2024 22:15, Yann E. MORIN wrote:
>>>> Fiona, All,
>>>>
>>>> On 2024-07-02 18:57 +0200, Fiona Klute via buildroot spake thusly:
>>>>> From: "Fiona Klute (WIWA)" <fiona.klute@gmx.de>
>>>>>
>>>>> The previously used "killall sshd" stopped all instances of sshd. With
>>>>> OpenSSH before 9.8 that meant not only the listening server, but also
>>>>> instances serving currently open sessions, possibly including the one
>>>>> used to send the restart command, preventing it from completing the
>>>>> "start" part of "restart" and leaving the system unreachable over SSH.
>>>>>
>>>>> Instead, use the PID file that sshd creates by default to target only
>>>>> the listening server. This leaves any open SSH sessions unaffected.
>>>>
>>>> Rather than play with the PID ourselves, can we use start-stop-daemon
>>>> instead? Currently, we run sshd and it daemonises all by itself, but if
>>>> we pass -D, then start-stop-daemon will be able to do that, and handle
>>>> the PID file automatically for us.
>>>
>>>   Note that there's a little disadvantage of that: when sshd daemonizes
>>> by itself, it will only return after the real daemon has started
>>> listening. If start-stop-daemon does the daemonisation, it will return
>>> immediately.
>>
>> I looked closer at start-stop-daemon, and there's a solution that avoids
>> that problem and simplifies the init script, too: don't use "-D" for
>> sshd. start-stop-daemon expects the daemon to write its own PID file and
>> put itself into the background, options to handle a service that doesn't
>> are described as fallbacks in the man page.
>>
>> I've tested it, and letting start-stop-daemon manage sshd that way works
>> perfectly fine. I also found it has a built-in option (--retry) to avoid
>
>   Does the --retry option exist in busybox's version?

Good question. It worked in my tests, but after checking the Busybox
code the answer is "not really". With
CONFIG_FEATURE_START_STOP_DAEMON_FANCY=y (the default) the code accepts
the option without error, but doesn't actually use it (seems like bad
design to me). So I guess I got lucky in the sense that sshd stops fast
enough that there was no conflict with starting it right after when I
tested the restart command. So I guess I'll need a wait loop after all. :-(

Another related question: I used the long options because in my opinion
it makes the script easier to read. However, I noticed that this would
break if someone were to set
CONFIG_FEATURE_START_STOP_DAEMON_LONG_OPTIONS=n (default is y). I guess
that means I should stick to the short options?

Best regards,
Fiona

>   Regards,
>   Arnout
>
>> "has the service really stopped" loops. I'll send the refactored patch
>> set in a few minutes.
>>
>> Thanks for the review!
>>
>> Best regards,
>> Fiona
>>
>>>   I don't think this is important enough to not daemonise with
>>> start-stop-daemon - if you need fine-grain control by the init system
>>> you should use a proper init system. But it's important to be aware of
>>> the implied limitations.
>>>
>>>   Regards,
>>>   Arnout
>>>
>>>>
>>>> Regards,
>>>> Yann E. MORIN.
>>>>
>>>>> With OpenSSH 9.8 open sessions are handled by the separate
>>>>> sshd-session binary so killall does not hit them any more, but
>>>>> a targeted "kill" command is still desirable.
>>>>>
>>>>> Signed-off-by: Fiona Klute (WIWA) <fiona.klute@gmx.de>
>>>>> ---
>>>> [--SNIP--]
>>>>> diff --git a/package/openssh/S50sshd b/package/openssh/S50sshd
>>>>> index 22da41d1ca..f1ff9dfec1 100644
>>>>> --- a/package/openssh/S50sshd
>>>>> +++ b/package/openssh/S50sshd
>>>>> @@ -3,6 +3,8 @@
>>>>>   # sshd        Starts sshd.
>>>>>   #
>>>>>
>>>>> +PIDFILE="/var/run/sshd.pid"
>>>>> +
>>>>>   # Make sure the ssh-keygen progam exists
>>>>>   [ -f /usr/bin/ssh-keygen ] || exit 0
>>>>>
>>>>> @@ -14,15 +16,25 @@ start() {
>>>>>
>>>>>       printf "Starting sshd: "
>>>>>       /usr/sbin/sshd
>>>>> -    touch /var/lock/sshd
>>>>>       echo "OK"
>>>>>   }
>>>>> +
>>>>>   stop() {
>>>>>       printf "Stopping sshd: "
>>>>> -    killall sshd
>>>>> -    rm -f /var/lock/sshd
>>>>> +    pid="$( cat "${PIDFILE}" 2>/dev/null || true )"
>>>>> +    if [ -z "${pid}" ]; then
>>>>> +        echo "ERROR: not running"
>>>>> +        return 1
>>>>> +    fi
>>>>> +    kill "${pid}"
>>>>> +    # SSHD deletes its PID file when exiting, wait for it to
>>>>> +    # disappear.
>>>>> +    while [ -f "${PIDFILE}" ]; do
>>>>> +        sleep 0.1
>>>>> +    done
>>>>>       echo "OK"
>>>>>   }
>>>>> +
>>>>>   restart() {
>>>>>       stop
>>>>>       start
>>>>> @@ -44,4 +56,3 @@ case "$1" in
>>>>>   esac
>>>>>
>>>>>   exit $?
>>>>> -
>>>>> --
>>>>> 2.45.2
>>>>>
>>>>> _______________________________________________
>>>>> buildroot mailing list
>>>>> buildroot@buildroot.org
>>>>> https://lists.buildroot.org/mailman/listinfo/buildroot

_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH v2 1/4] package/openssh: don't kill open sessions on restart

[Arnout Vandecappelle via buildroot]

On 04/07/2024 10:21, Fiona Klute wrote:
> Am 04.07.24 um 10:00 schrieb Arnout Vandecappelle:
>> On 03/07/2024 17:46, Fiona Klute wrote:
>>> Am 03.07.24 um 09:01 schrieb Arnout Vandecappelle:
>>>> On 02/07/2024 22:15, Yann E. MORIN wrote:
>>>>> Fiona, All,
>>>>>
>>>>> On 2024-07-02 18:57 +0200, Fiona Klute via buildroot spake thusly:
>>>>>> From: "Fiona Klute (WIWA)" <fiona.klute@gmx.de>
>>>>>>
>>>>>> The previously used "killall sshd" stopped all instances of sshd. With
>>>>>> OpenSSH before 9.8 that meant not only the listening server, but also
>>>>>> instances serving currently open sessions, possibly including the one
>>>>>> used to send the restart command, preventing it from completing the
>>>>>> "start" part of "restart" and leaving the system unreachable over SSH.
>>>>>>
>>>>>> Instead, use the PID file that sshd creates by default to target only
>>>>>> the listening server. This leaves any open SSH sessions unaffected.
>>>>>
>>>>> Rather than play with the PID ourselves, can we use start-stop-daemon
>>>>> instead? Currently, we run sshd and it daemonises all by itself, but if
>>>>> we pass -D, then start-stop-daemon will be able to do that, and handle
>>>>> the PID file automatically for us.
>>>>
>>>>   Note that there's a little disadvantage of that: when sshd daemonizes
>>>> by itself, it will only return after the real daemon has started
>>>> listening. If start-stop-daemon does the daemonisation, it will return
>>>> immediately.
>>>
>>> I looked closer at start-stop-daemon, and there's a solution that avoids
>>> that problem and simplifies the init script, too: don't use "-D" for
>>> sshd. start-stop-daemon expects the daemon to write its own PID file and
>>> put itself into the background, options to handle a service that doesn't
>>> are described as fallbacks in the man page.
>>>
>>> I've tested it, and letting start-stop-daemon manage sshd that way works
>>> perfectly fine. I also found it has a built-in option (--retry) to avoid
>>
>>   Does the --retry option exist in busybox's version?
> 
> Good question. It worked in my tests, but after checking the Busybox
> code the answer is "not really". With
> CONFIG_FEATURE_START_STOP_DAEMON_FANCY=y (the default) the code accepts
> the option without error, but doesn't actually use it (seems like bad
> design to me). So I guess I got lucky in the sense that sshd stops fast
> enough that there was no conflict with starting it right after when I
> tested the restart command. So I guess I'll need a wait loop after all. :-(
> 
> Another related question: I used the long options because in my opinion
> it makes the script easier to read. However, I noticed that this would
> break if someone were to set
> CONFIG_FEATURE_START_STOP_DAEMON_LONG_OPTIONS=n (default is y). I guess
> that means I should stick to the short options?

  No, for all of the Buildroot-supplied scripts, we can assume that the 
Buildroot-default busybox (and uClibc and ...) options are turned on.

  If there is some option that we need turned on for some specific thing, we 
typically update our default busybox config to have it enabled.

  People who use a custom busybox config are responsible for making sure that 
all the scripts still work, and if not, adapt the scripts accordingly (in an 
overlay).

  Of course, all of the above is entirely undocumented. Feel free to update the 
documentation to explain this!

  Regards,
  Arnout

> 
> Best regards,
> Fiona
> 
>>   Regards,
>>   Arnout
>>
>>> "has the service really stopped" loops. I'll send the refactored patch
>>> set in a few minutes.
>>>
>>> Thanks for the review!
>>>
>>> Best regards,
>>> Fiona
>>>
>>>>   I don't think this is important enough to not daemonise with
>>>> start-stop-daemon - if you need fine-grain control by the init system
>>>> you should use a proper init system. But it's important to be aware of
>>>> the implied limitations.
>>>>
>>>>   Regards,
>>>>   Arnout
>>>>
>>>>>
>>>>> Regards,
>>>>> Yann E. MORIN.
>>>>>
>>>>>> With OpenSSH 9.8 open sessions are handled by the separate
>>>>>> sshd-session binary so killall does not hit them any more, but
>>>>>> a targeted "kill" command is still desirable.
>>>>>>
>>>>>> Signed-off-by: Fiona Klute (WIWA) <fiona.klute@gmx.de>
>>>>>> ---
>>>>> [--SNIP--]
>>>>>> diff --git a/package/openssh/S50sshd b/package/openssh/S50sshd
>>>>>> index 22da41d1ca..f1ff9dfec1 100644
>>>>>> --- a/package/openssh/S50sshd
>>>>>> +++ b/package/openssh/S50sshd
>>>>>> @@ -3,6 +3,8 @@
>>>>>>   # sshd        Starts sshd.
>>>>>>   #
>>>>>>
>>>>>> +PIDFILE="/var/run/sshd.pid"
>>>>>> +
>>>>>>   # Make sure the ssh-keygen progam exists
>>>>>>   [ -f /usr/bin/ssh-keygen ] || exit 0
>>>>>>
>>>>>> @@ -14,15 +16,25 @@ start() {
>>>>>>
>>>>>>       printf "Starting sshd: "
>>>>>>       /usr/sbin/sshd
>>>>>> -    touch /var/lock/sshd
>>>>>>       echo "OK"
>>>>>>   }
>>>>>> +
>>>>>>   stop() {
>>>>>>       printf "Stopping sshd: "
>>>>>> -    killall sshd
>>>>>> -    rm -f /var/lock/sshd
>>>>>> +    pid="$( cat "${PIDFILE}" 2>/dev/null || true )"
>>>>>> +    if [ -z "${pid}" ]; then
>>>>>> +        echo "ERROR: not running"
>>>>>> +        return 1
>>>>>> +    fi
>>>>>> +    kill "${pid}"
>>>>>> +    # SSHD deletes its PID file when exiting, wait for it to
>>>>>> +    # disappear.
>>>>>> +    while [ -f "${PIDFILE}" ]; do
>>>>>> +        sleep 0.1
>>>>>> +    done
>>>>>>       echo "OK"
>>>>>>   }
>>>>>> +
>>>>>>   restart() {
>>>>>>       stop
>>>>>>       start
>>>>>> @@ -44,4 +56,3 @@ case "$1" in
>>>>>>   esac
>>>>>>
>>>>>>   exit $?
>>>>>> -
>>>>>> -- 
>>>>>> 2.45.2
>>>>>>
>>>>>> _______________________________________________
>>>>>> buildroot mailing list
>>>>>> buildroot@buildroot.org
>>>>>> https://lists.buildroot.org/mailman/listinfo/buildroot
> 
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH v2 1/4] package/openssh: don't kill open sessions on restart

[Fiona Klute via buildroot]

Am 04.07.24 um 10:58 schrieb Arnout Vandecappelle:
> On 04/07/2024 10:21, Fiona Klute wrote:
>> Am 04.07.24 um 10:00 schrieb Arnout Vandecappelle:
>>> On 03/07/2024 17:46, Fiona Klute wrote:
>>>> Am 03.07.24 um 09:01 schrieb Arnout Vandecappelle:
>>>>> On 02/07/2024 22:15, Yann E. MORIN wrote:
>>>>>> Fiona, All,
>>>>>>
>>>>>> On 2024-07-02 18:57 +0200, Fiona Klute via buildroot spake thusly:
>>>>>>> From: "Fiona Klute (WIWA)" <fiona.klute@gmx.de>
>>>>>>>
>>>>>>> The previously used "killall sshd" stopped all instances of sshd.
>>>>>>> With
>>>>>>> OpenSSH before 9.8 that meant not only the listening server, but
>>>>>>> also
>>>>>>> instances serving currently open sessions, possibly including the
>>>>>>> one
>>>>>>> used to send the restart command, preventing it from completing the
>>>>>>> "start" part of "restart" and leaving the system unreachable over
>>>>>>> SSH.
>>>>>>>
>>>>>>> Instead, use the PID file that sshd creates by default to target
>>>>>>> only
>>>>>>> the listening server. This leaves any open SSH sessions unaffected.
>>>>>>
>>>>>> Rather than play with the PID ourselves, can we use start-stop-daemon
>>>>>> instead? Currently, we run sshd and it daemonises all by itself,
>>>>>> but if
>>>>>> we pass -D, then start-stop-daemon will be able to do that, and
>>>>>> handle
>>>>>> the PID file automatically for us.
>>>>>
>>>>>   Note that there's a little disadvantage of that: when sshd
>>>>> daemonizes
>>>>> by itself, it will only return after the real daemon has started
>>>>> listening. If start-stop-daemon does the daemonisation, it will return
>>>>> immediately.
>>>>
>>>> I looked closer at start-stop-daemon, and there's a solution that
>>>> avoids
>>>> that problem and simplifies the init script, too: don't use "-D" for
>>>> sshd. start-stop-daemon expects the daemon to write its own PID file
>>>> and
>>>> put itself into the background, options to handle a service that
>>>> doesn't
>>>> are described as fallbacks in the man page.
>>>>
>>>> I've tested it, and letting start-stop-daemon manage sshd that way
>>>> works
>>>> perfectly fine. I also found it has a built-in option (--retry) to
>>>> avoid
>>>
>>>   Does the --retry option exist in busybox's version?
>>
>> Good question. It worked in my tests, but after checking the Busybox
>> code the answer is "not really". With
>> CONFIG_FEATURE_START_STOP_DAEMON_FANCY=y (the default) the code accepts
>> the option without error, but doesn't actually use it (seems like bad
>> design to me). So I guess I got lucky in the sense that sshd stops fast
>> enough that there was no conflict with starting it right after when I
>> tested the restart command. So I guess I'll need a wait loop after
>> all. :-(
>>
>> Another related question: I used the long options because in my opinion
>> it makes the script easier to read. However, I noticed that this would
>> break if someone were to set
>> CONFIG_FEATURE_START_STOP_DAEMON_LONG_OPTIONS=n (default is y). I guess
>> that means I should stick to the short options?
>
>   No, for all of the Buildroot-supplied scripts, we can assume that the
> Buildroot-default busybox (and uClibc and ...) options are turned on.
>
>   If there is some option that we need turned on for some specific
> thing, we typically update our default busybox config to have it enabled.
>
>   People who use a custom busybox config are responsible for making sure
> that all the scripts still work, and if not, adapt the scripts
> accordingly (in an overlay).
>
>   Of course, all of the above is entirely undocumented. Feel free to
> update the documentation to explain this!

Thanks for the explanation! Where would be the right place to add that?
The Busybox section in "Chapter 7. Configuration of other components",
"18.5. The SNNfoo start script", or somewhere else?

I see that package/busybox/busybox.config and
package/busybox/busybox-minimal.config both set
CONFIG_FEATURE_START_STOP_DAEMON_LONG_OPTIONS=y, so I've send a v4 that
avoids --retry but otherwise keeps the long options.

Best regards,
Fiona

>   Regards,
>   Arnout
>
>>
>> Best regards,
>> Fiona
>>
>>>   Regards,
>>>   Arnout
>>>
>>>> "has the service really stopped" loops. I'll send the refactored patch
>>>> set in a few minutes.
>>>>
>>>> Thanks for the review!
>>>>
>>>> Best regards,
>>>> Fiona
>>>>
>>>>>   I don't think this is important enough to not daemonise with
>>>>> start-stop-daemon - if you need fine-grain control by the init system
>>>>> you should use a proper init system. But it's important to be aware of
>>>>> the implied limitations.
>>>>>
>>>>>   Regards,
>>>>>   Arnout
>>>>>
>>>>>>
>>>>>> Regards,
>>>>>> Yann E. MORIN.
>>>>>>
>>>>>>> With OpenSSH 9.8 open sessions are handled by the separate
>>>>>>> sshd-session binary so killall does not hit them any more, but
>>>>>>> a targeted "kill" command is still desirable.
>>>>>>>
>>>>>>> Signed-off-by: Fiona Klute (WIWA) <fiona.klute@gmx.de>
>>>>>>> ---
>>>>>> [--SNIP--]
>>>>>>> diff --git a/package/openssh/S50sshd b/package/openssh/S50sshd
>>>>>>> index 22da41d1ca..f1ff9dfec1 100644
>>>>>>> --- a/package/openssh/S50sshd
>>>>>>> +++ b/package/openssh/S50sshd
>>>>>>> @@ -3,6 +3,8 @@
>>>>>>>   # sshd        Starts sshd.
>>>>>>>   #
>>>>>>>
>>>>>>> +PIDFILE="/var/run/sshd.pid"
>>>>>>> +
>>>>>>>   # Make sure the ssh-keygen progam exists
>>>>>>>   [ -f /usr/bin/ssh-keygen ] || exit 0
>>>>>>>
>>>>>>> @@ -14,15 +16,25 @@ start() {
>>>>>>>
>>>>>>>       printf "Starting sshd: "
>>>>>>>       /usr/sbin/sshd
>>>>>>> -    touch /var/lock/sshd
>>>>>>>       echo "OK"
>>>>>>>   }
>>>>>>> +
>>>>>>>   stop() {
>>>>>>>       printf "Stopping sshd: "
>>>>>>> -    killall sshd
>>>>>>> -    rm -f /var/lock/sshd
>>>>>>> +    pid="$( cat "${PIDFILE}" 2>/dev/null || true )"
>>>>>>> +    if [ -z "${pid}" ]; then
>>>>>>> +        echo "ERROR: not running"
>>>>>>> +        return 1
>>>>>>> +    fi
>>>>>>> +    kill "${pid}"
>>>>>>> +    # SSHD deletes its PID file when exiting, wait for it to
>>>>>>> +    # disappear.
>>>>>>> +    while [ -f "${PIDFILE}" ]; do
>>>>>>> +        sleep 0.1
>>>>>>> +    done
>>>>>>>       echo "OK"
>>>>>>>   }
>>>>>>> +
>>>>>>>   restart() {
>>>>>>>       stop
>>>>>>>       start
>>>>>>> @@ -44,4 +56,3 @@ case "$1" in
>>>>>>>   esac
>>>>>>>
>>>>>>>   exit $?
>>>>>>> -
>>>>>>> --
>>>>>>> 2.45.2
>>>>>>>
>>>>>>> _______________________________________________
>>>>>>> buildroot mailing list
>>>>>>> buildroot@buildroot.org
>>>>>>> https://lists.buildroot.org/mailman/listinfo/buildroot

_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH v2 1/4] package/openssh: don't kill open sessions on restart

[Arnout Vandecappelle via buildroot]

On 04/07/2024 12:25, Fiona Klute wrote:
> Am 04.07.24 um 10:58 schrieb Arnout Vandecappelle:
>> On 04/07/2024 10:21, Fiona Klute wrote:
>>> Am 04.07.24 um 10:00 schrieb Arnout Vandecappelle:
>>>> On 03/07/2024 17:46, Fiona Klute wrote:
[snip]
>>> Another related question: I used the long options because in my opinion
>>> it makes the script easier to read. However, I noticed that this would
>>> break if someone were to set
>>> CONFIG_FEATURE_START_STOP_DAEMON_LONG_OPTIONS=n (default is y). I guess
>>> that means I should stick to the short options?
>>
>>   No, for all of the Buildroot-supplied scripts, we can assume that the
>> Buildroot-default busybox (and uClibc and ...) options are turned on.
>>
>>   If there is some option that we need turned on for some specific
>> thing, we typically update our default busybox config to have it enabled.
>>
>>   People who use a custom busybox config are responsible for making sure
>> that all the scripts still work, and if not, adapt the scripts
>> accordingly (in an overlay).
>>
>>   Of course, all of the above is entirely undocumented. Feel free to
>> update the documentation to explain this!
> 
> Thanks for the explanation! Where would be the right place to add that?
> The Busybox section in "Chapter 7. Configuration of other components",
> "18.5. The SNNfoo start script", or somewhere else?

  Hm, no, I don't think so. I think it would be better to add a new section 
"Integration principles" under "10. Integration topics".

> I see that package/busybox/busybox.config and
> package/busybox/busybox-minimal.config both set
> CONFIG_FEATURE_START_STOP_DAEMON_LONG_OPTIONS=y, so I've send a v4 that
> avoids --retry but otherwise keeps the long options.

  Oh, right, I forgot to answer: I'm also a fan of using long options as much as 
possible in scripts, except for some very well-known ones (like rm -f). It's not 
how things currently are in Buildroot, but I feel that that is a "rule" we can 
break.

  Regards,
  Arnout

[snip]

_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

[Buildroot] [PATCH v2 2/4] package/openssh: fix init script indentation

[Fiona Klute via buildroot]

From: "Fiona Klute (WIWA)" <fiona.klute@gmx.de>

This brings the script in line with .editorconfig settings and other
newer init scripts.

Signed-off-by: Fiona Klute (WIWA) <fiona.klute@gmx.de>
---
 .checkpackageignore     |  2 +-
 package/openssh/S50sshd | 24 ++++++++++++------------
 2 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/.checkpackageignore b/.checkpackageignore
index 9c4a760117..f8c00e3f9e 100644
--- a/.checkpackageignore
+++ b/.checkpackageignore
@@ -1150,7 +1150,7 @@ package/openrc/0003-init.d-agetty-replace-sbin-agetty-by-sbin-getty.patch lib_pa
 package/openrc/0004-init.d-agetty-start-agetty-after-all-sevices.patch lib_patch.Upstream
 package/openrc/0005-runlevels-do-not-add-agetty.tty-1-6-if-MKSYSVINIT-ye.patch lib_patch.Upstream
 package/openrc/0006-Also-create-run-lock-subsys-directory.patch lib_patch.Upstream
-package/openssh/S50sshd lib_sysv.Indent lib_sysv.Variables
+package/openssh/S50sshd lib_sysv.Variables
 package/openswan/0001-lib-libopenswan-constants.c-workaround-missing-ns_t_.patch lib_patch.Upstream
 package/opentyrian/0001-Move-definitions-that-don-t-need-to-be-exposed-from-opl-h-to-opl-c.patch lib_patch.Upstream
 package/openvmtools/0001-no_cflags_werror.patch lib_patch.Upstream
diff --git a/package/openssh/S50sshd b/package/openssh/S50sshd
index f1ff9dfec1..6ee0fc6b86 100644
--- a/package/openssh/S50sshd
+++ b/package/openssh/S50sshd
@@ -41,18 +41,18 @@ restart() {
 }

 case "$1" in
-  start)
-	start
-	;;
-  stop)
-	stop
-	;;
-  restart|reload)
-	restart
-	;;
-  *)
-	echo "Usage: $0 {start|stop|restart}"
-	exit 1
+	start)
+		start
+		;;
+	stop)
+		stop
+		;;
+	restart|reload)
+		restart
+		;;
+	*)
+		echo "Usage: $0 {start|stop|restart}"
+		exit 1
 esac

 exit $?
--
2.45.2

_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

[Buildroot] [PATCH v2 3/4] package/openssh: implement "reload" using SIGHUP

[Fiona Klute via buildroot]

From: "Fiona Klute (WIWA)" <fiona.klute@gmx.de>

This is the documented "reload configuration" method for OpenSSH.

Signed-off-by: Fiona Klute (WIWA) <fiona.klute@gmx.de>
---
 package/openssh/S50sshd | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/package/openssh/S50sshd b/package/openssh/S50sshd
index 6ee0fc6b86..87430bbec8 100644
--- a/package/openssh/S50sshd
+++ b/package/openssh/S50sshd
@@ -40,6 +40,17 @@ restart() {
 	start
 }

+reload() {
+	printf "Reloading sshd config: "
+	pid="$( cat "${PIDFILE}" 2>/dev/null || true )"
+	if [ -z "${pid}" ]; then
+		echo "ERROR: not running"
+		return 1
+	fi
+	kill -HUP "${pid}"
+	echo "OK"
+}
+
 case "$1" in
 	start)
 		start
@@ -47,11 +58,14 @@ case "$1" in
 	stop)
 		stop
 		;;
-	restart|reload)
+	restart)
 		restart
 		;;
+	reload)
+		reload
+		;;
 	*)
-		echo "Usage: $0 {start|stop|restart}"
+		echo "Usage: $0 {start|stop|restart|reload}"
 		exit 1
 esac

--
2.45.2

_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

[Buildroot] [PATCH v2 4/4] package/openssh: check error conditions on "start"

[Fiona Klute via buildroot]

From: "Fiona Klute (WIWA)" <fiona.klute@gmx.de>

If there already is a running instance a new sshd fails to start,
leaving the old one in place. The init script silently ignored
that. Show an error message and return a non-zero exit code instead.

Also check the return code from sshd, without the check "OK" still
appeared if sshd failed to start. Now the init script returns a
non-zero exit code if start fails.

Signed-off-by: Fiona Klute (WIWA) <fiona.klute@gmx.de>
---
Changes v1 -> v2:
* Check sshd exit code on service start

 package/openssh/S50sshd | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/package/openssh/S50sshd b/package/openssh/S50sshd
index 87430bbec8..a677327378 100644
--- a/package/openssh/S50sshd
+++ b/package/openssh/S50sshd
@@ -15,8 +15,16 @@ start() {
 	/usr/bin/ssh-keygen -A

 	printf "Starting sshd: "
-	/usr/sbin/sshd
-	echo "OK"
+	if [ -f "${PIDFILE}" ]; then
+		echo "ERROR: already running"
+		return 1
+	fi
+	if /usr/sbin/sshd; then
+		echo "OK"
+	else
+		echo "ERROR"
+		return 1
+	fi
 }

 stop() {
--
2.45.2

_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot