* [Buildroot] [PATCH v2 0/4] Refactor OpenSSH init.d script
@ 2024-07-02 16:57 Fiona Klute via buildroot
2024-07-02 16:57 ` [Buildroot] [PATCH v2 1/4] package/openssh: don't kill open sessions on restart Fiona Klute via buildroot
` (3 more replies)
0 siblings, 4 replies; 13+ messages in thread
From: Fiona Klute via buildroot @ 2024-07-02 16:57 UTC (permalink / raw)
To: buildroot; +Cc: Fiona Klute (WIWA)
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* [Buildroot] [PATCH v2 1/4] package/openssh: don't kill open sessions on restart
2024-07-02 16:57 [Buildroot] [PATCH v2 0/4] Refactor OpenSSH init.d script Fiona Klute via buildroot
@ 2024-07-02 16:57 ` Fiona Klute via buildroot
2024-07-02 20:15 ` Yann E. MORIN
2024-07-02 16:57 ` [Buildroot] [PATCH v2 2/4] package/openssh: fix init script indentation Fiona Klute via buildroot
` (2 subsequent siblings)
3 siblings, 1 reply; 13+ messages in thread
From: Fiona Klute via buildroot @ 2024-07-02 16:57 UTC (permalink / raw)
To: buildroot; +Cc: Fiona Klute (WIWA)
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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [Buildroot] [PATCH v2 2/4] package/openssh: fix init script indentation
2024-07-02 16:57 [Buildroot] [PATCH v2 0/4] Refactor OpenSSH init.d script Fiona Klute via buildroot
2024-07-02 16:57 ` [Buildroot] [PATCH v2 1/4] package/openssh: don't kill open sessions on restart Fiona Klute via buildroot
@ 2024-07-02 16:57 ` Fiona Klute via buildroot
2024-07-02 16:57 ` [Buildroot] [PATCH v2 3/4] package/openssh: implement "reload" using SIGHUP Fiona Klute via buildroot
2024-07-02 16:57 ` [Buildroot] [PATCH v2 4/4] package/openssh: check error conditions on "start" Fiona Klute via buildroot
3 siblings, 0 replies; 13+ messages in thread
From: Fiona Klute via buildroot @ 2024-07-02 16:57 UTC (permalink / raw)
To: buildroot; +Cc: Fiona Klute (WIWA)
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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [Buildroot] [PATCH v2 3/4] package/openssh: implement "reload" using SIGHUP
2024-07-02 16:57 [Buildroot] [PATCH v2 0/4] Refactor OpenSSH init.d script Fiona Klute via buildroot
2024-07-02 16:57 ` [Buildroot] [PATCH v2 1/4] package/openssh: don't kill open sessions on restart Fiona Klute via buildroot
2024-07-02 16:57 ` [Buildroot] [PATCH v2 2/4] package/openssh: fix init script indentation Fiona Klute via buildroot
@ 2024-07-02 16:57 ` Fiona Klute via buildroot
2024-07-02 16:57 ` [Buildroot] [PATCH v2 4/4] package/openssh: check error conditions on "start" Fiona Klute via buildroot
3 siblings, 0 replies; 13+ messages in thread
From: Fiona Klute via buildroot @ 2024-07-02 16:57 UTC (permalink / raw)
To: buildroot; +Cc: Fiona Klute (WIWA)
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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [Buildroot] [PATCH v2 4/4] package/openssh: check error conditions on "start"
2024-07-02 16:57 [Buildroot] [PATCH v2 0/4] Refactor OpenSSH init.d script Fiona Klute via buildroot
` (2 preceding siblings ...)
2024-07-02 16:57 ` [Buildroot] [PATCH v2 3/4] package/openssh: implement "reload" using SIGHUP Fiona Klute via buildroot
@ 2024-07-02 16:57 ` Fiona Klute via buildroot
3 siblings, 0 replies; 13+ messages in thread
From: Fiona Klute via buildroot @ 2024-07-02 16:57 UTC (permalink / raw)
To: buildroot; +Cc: Fiona Klute (WIWA)
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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [Buildroot] [PATCH v2 1/4] package/openssh: don't kill open sessions on restart
2024-07-02 16:57 ` [Buildroot] [PATCH v2 1/4] package/openssh: don't kill open sessions on restart Fiona Klute via buildroot
@ 2024-07-02 20:15 ` Yann E. MORIN
2024-07-03 7:01 ` Arnout Vandecappelle via buildroot
0 siblings, 1 reply; 13+ messages in thread
From: Yann E. MORIN @ 2024-07-02 20:15 UTC (permalink / raw)
To: Fiona Klute; +Cc: buildroot
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Buildroot] [PATCH v2 1/4] package/openssh: don't kill open sessions on restart
2024-07-02 20:15 ` Yann E. MORIN
@ 2024-07-03 7:01 ` Arnout Vandecappelle via buildroot
2024-07-03 15:46 ` Fiona Klute via buildroot
0 siblings, 1 reply; 13+ messages in thread
From: Arnout Vandecappelle via buildroot @ 2024-07-03 7:01 UTC (permalink / raw)
To: Yann E. MORIN, Fiona Klute; +Cc: 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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Buildroot] [PATCH v2 1/4] package/openssh: don't kill open sessions on restart
2024-07-03 7:01 ` Arnout Vandecappelle via buildroot
@ 2024-07-03 15:46 ` Fiona Klute via buildroot
2024-07-04 8:00 ` Arnout Vandecappelle via buildroot
0 siblings, 1 reply; 13+ messages in thread
From: Fiona Klute via buildroot @ 2024-07-03 15:46 UTC (permalink / raw)
To: Arnout Vandecappelle, Yann E. MORIN; +Cc: 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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Buildroot] [PATCH v2 1/4] package/openssh: don't kill open sessions on restart
2024-07-03 15:46 ` Fiona Klute via buildroot
@ 2024-07-04 8:00 ` Arnout Vandecappelle via buildroot
2024-07-04 8:21 ` Fiona Klute via buildroot
0 siblings, 1 reply; 13+ messages in thread
From: Arnout Vandecappelle via buildroot @ 2024-07-04 8:00 UTC (permalink / raw)
To: Fiona Klute, Yann E. MORIN; +Cc: 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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Buildroot] [PATCH v2 1/4] package/openssh: don't kill open sessions on restart
2024-07-04 8:00 ` Arnout Vandecappelle via buildroot
@ 2024-07-04 8:21 ` Fiona Klute via buildroot
2024-07-04 8:58 ` Arnout Vandecappelle via buildroot
0 siblings, 1 reply; 13+ messages in thread
From: Fiona Klute via buildroot @ 2024-07-04 8:21 UTC (permalink / raw)
To: Arnout Vandecappelle, Yann E. MORIN; +Cc: 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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Buildroot] [PATCH v2 1/4] package/openssh: don't kill open sessions on restart
2024-07-04 8:21 ` Fiona Klute via buildroot
@ 2024-07-04 8:58 ` Arnout Vandecappelle via buildroot
2024-07-04 10:25 ` Fiona Klute via buildroot
0 siblings, 1 reply; 13+ messages in thread
From: Arnout Vandecappelle via buildroot @ 2024-07-04 8:58 UTC (permalink / raw)
To: Fiona Klute, Yann E. MORIN; +Cc: 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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Buildroot] [PATCH v2 1/4] package/openssh: don't kill open sessions on restart
2024-07-04 8:58 ` Arnout Vandecappelle via buildroot
@ 2024-07-04 10:25 ` Fiona Klute via buildroot
2024-07-05 7:39 ` Arnout Vandecappelle via buildroot
0 siblings, 1 reply; 13+ messages in thread
From: Fiona Klute via buildroot @ 2024-07-04 10:25 UTC (permalink / raw)
To: Arnout Vandecappelle, Yann E. MORIN; +Cc: 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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Buildroot] [PATCH v2 1/4] package/openssh: don't kill open sessions on restart
2024-07-04 10:25 ` Fiona Klute via buildroot
@ 2024-07-05 7:39 ` Arnout Vandecappelle via buildroot
0 siblings, 0 replies; 13+ messages in thread
From: Arnout Vandecappelle via buildroot @ 2024-07-05 7:39 UTC (permalink / raw)
To: Fiona Klute, Yann E. MORIN; +Cc: 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
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2024-07-05 7:39 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-07-02 16:57 [Buildroot] [PATCH v2 0/4] Refactor OpenSSH init.d script Fiona Klute via buildroot
2024-07-02 16:57 ` [Buildroot] [PATCH v2 1/4] package/openssh: don't kill open sessions on restart Fiona Klute via buildroot
2024-07-02 20:15 ` Yann E. MORIN
2024-07-03 7:01 ` Arnout Vandecappelle via buildroot
2024-07-03 15:46 ` Fiona Klute via buildroot
2024-07-04 8:00 ` Arnout Vandecappelle via buildroot
2024-07-04 8:21 ` Fiona Klute via buildroot
2024-07-04 8:58 ` Arnout Vandecappelle via buildroot
2024-07-04 10:25 ` Fiona Klute via buildroot
2024-07-05 7:39 ` Arnout Vandecappelle via buildroot
2024-07-02 16:57 ` [Buildroot] [PATCH v2 2/4] package/openssh: fix init script indentation Fiona Klute via buildroot
2024-07-02 16:57 ` [Buildroot] [PATCH v2 3/4] package/openssh: implement "reload" using SIGHUP Fiona Klute via buildroot
2024-07-02 16:57 ` [Buildroot] [PATCH v2 4/4] package/openssh: check error conditions on "start" Fiona Klute via buildroot
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox