Skip to contentSkip to conversation

radvd: improve startup script

PATCH New Carlos Santos 4 participants 8 messages Sep 25, 2017 – Apr 26, 2018

lore
Share & tools
lore

Carlos Santos

author
#0 lore raw

PATCH 1/1 radvd: improve startup script

The previous script caused a failure if /etc/radvd.conf did not exist.

This is a simple copy/paste/edit of package/dnsmasq/S80dnsmasq.

Signed-off-by Carlos Santos <[email protected]>
---
 package/radvd/S50radvd | 36 ++++++++++++++++++++++--------------
 1 file changed, 22 insertions(+), 14 deletions(-)
package/radvd/S50radvd +22 -14
index 9f1407c..dcc2af6 100755
@@ -1,18 +1,26 @@
 #!/bin/sh
 
-RADVD=/usr/sbin/radvd
+[ -x /usr/sbin/radvd ] || exit 0
+[ -f /etc/radvd.conf ] || exit 0
 
-echo "1" > /proc/sys/net/ipv6/conf/all/forwarding
+case "$1" in
+	start)
+		printf "Starting radvd: "
+		start-stop-daemon -S -x /usr/sbin/radvd
+		[ $? = 0 ] && echo "OK" || echo "FAIL"
+		;;
+	stop)
+		printf "Stopping radvd: "
+		start-stop-daemon -K -q -x /usr/sbin/radvd
+		[ $? = 0 ] && echo "OK" || echo "FAIL"
+		;;
+	restart|reload)
+		$0 stop
+		$0 start
+		;;
+	*)
+		echo "Usage: $0 {start|stop|restart}"
+		exit 1
+esac
 
-printf "Starting radvd: "
-if [ ! -x "${RADVD}" ]; then
-	echo "missing"
-	exit 1
-fi
-
-if ${RADVD} ; then
-	echo "done"
-else
-	echo "failed"
-	exit 1
-fi
+exit 0
Signature · 1 line
2.7.5
#1 lore raw

PATCH 1/1 radvd: improve startup script

Quoted 1 line · Carlos Santos

The previous script caused a failure if /etc/radvd.conf did not exist.

That's a good thing, no? If you select radvd but forget to install a
configuration file, you'll want to have some kind of warning rather than silently not starting it.

Quoted 2 lines

This is a simple copy/paste/edit of package/dnsmasq/S80dnsmasq.

Perhaps not the best example...

Quoted 11 lines
Signed-off-by Carlos Santos <[email protected]>
---
 package/radvd/S50radvd | 36 ++++++++++++++++++++++--------------
 1 file changed, 22 insertions(+), 14 deletions(-)
package/radvd/S50radvd +0 -0
index 9f1407c..dcc2af6 100755

package/radvd/S50radvd line 3

Quoted 4 lines
package/radvd/S50radvd +1 -1
@@ -1,18 +1,26 @@
 #!/bin/sh
 
-RADVD=/usr/sbin/radvd
+[ -x /usr/sbin/radvd ] || exit 0

This we certainly don't want. If the executable is missing, we want to shout
loudly, not silently skip it.

package/radvd/S50radvd line 5

Quoted 3 lines
package/radvd/S50radvd +1 -1
+[ -f /etc/radvd.conf ] || exit 0
 
-echo "1" > /proc/sys/net/ipv6/conf/all/forwarding

Why remove this? It should of course move to the start stanza.

Quoted 8 lines
package/radvd/S50radvd +8 -0
+case "$1" in
+	start)
+		printf "Starting radvd: "
+		start-stop-daemon -S -x /usr/sbin/radvd
+		[ $? = 0 ] && echo "OK" || echo "FAI> +		;;
+	stop)
+		printf "Stopping radvd: "
+		start-stop-daemon -K -q -x /usr/sbin/radvd

package/radvd/S50radvd line 24

Quoted 10 lines
package/radvd/S50radvd +10 -0
+		[ $? = 0 ] && echo "OK" || echo "FAIL"
+		;;
+	restart|reload)
+		$0 stop
+		$0 start
+		;;
+	*)
+		echo "Usage: $0 {start|stop|restart}"
+		exit 1
+esac

This part looks good.

Regards,
Arnout

Quoted 15 lines
package/radvd/S50radvd +1 -12
-printf "Starting radvd: "
-if [ ! -x "${RADVD}" ]; then
-	echo "missing"
-	exit 1
-fi
-
-if ${RADVD} ; then
-	echo "done"
-else
-	echo "failed"
-	exit 1
-fi
+exit 0
Signature · 6 lines
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF
author
#2 lore raw

PATCH 1/1 radvd: improve startup script

----- Original Message -----

Quoted 11 lines

From: "Arnout Vandecappelle" <[email protected]>
To: "Carlos Santos" <[email protected]>, buildroot at buildroot.org
Sent: Tuesday, September 26, 2017 7:26:20 PM
Subject: Re: [Buildroot] [PATCH] radvd: improve startup script

Quoted 1 line · Carlos Santos

The previous script caused a failure if /etc/radvd.conf did not exist.

That's a good thing, no? If you select radvd but forget to install a configuration file, you'll want to have some kind of warning rather than silently not starting it.

Not exactly. There is no default configuration and the daemon may be started later by other services using a configuration created at run-time. In this particular I'm targeting libvirt, which uses both radvd and dnsmasq this way.

Quoted 24 lines
Quoted 2 lines

This is a simple copy/paste/edit of package/dnsmasq/S80dnsmasq.

Perhaps not the best example...

Quoted 11 lines
Signed-off-by Carlos Santos <[email protected]>
---
 package/radvd/S50radvd | 36 ++++++++++++++++++++++--------------
 1 file changed, 22 insertions(+), 14 deletions(-)
package/radvd/S50radvd +0 -0
index 9f1407c..dcc2af6 100755

package/radvd/S50radvd line 3

Quoted 4 lines
package/radvd/S50radvd +1 -1
@@ -1,18 +1,26 @@
 #!/bin/sh
 
-RADVD=/usr/sbin/radvd
+[ -x /usr/sbin/radvd ] || exit 0

This we certainly don't want. If the executable is missing, we want to shout loudly, not silently skip it.

Quoted 5 lines

package/radvd/S50radvd line 5

Quoted 3 lines
package/radvd/S50radvd +1 -1
+[ -f /etc/radvd.conf ] || exit 0
 
-echo "1" > /proc/sys/net/ipv6/conf/all/forwarding

Why remove this? It should of course move to the start stanza.

Correct.

Quoted 23 lines
Quoted 8 lines
package/radvd/S50radvd +8 -0
+case "$1" in
+	start)
+		printf "Starting radvd: "
+		start-stop-daemon -S -x /usr/sbin/radvd
+		[ $? = 0 ] && echo "OK" || echo "FAI> +		;;
+	stop)
+		printf "Stopping radvd: "
+		start-stop-daemon -K -q -x /usr/sbin/radvd

package/radvd/S50radvd line 24

Quoted 10 lines
package/radvd/S50radvd +10 -0
+		[ $? = 0 ] && echo "OK" || echo "FAIL"
+		;;
+	restart|reload)
+		$0 stop
+		$0 start
+		;;
+	*)
+		echo "Usage: $0 {start|stop|restart}"
+		exit 1
+esac

This part looks good.

Regards,
Arnout

Signature · 4 lines
Carlos Santos (Casantos) - DATACOM, P&D
?The greatest triumph that modern PR can offer is the transcendent 
success of having your words and actions judged by your reputation, 
rather than the other way about.? ? Christopher Hitchens
author
#3 lore raw

PATCH v2 1/1 ] radvd: improve startup script

In reply to Arnout Vandecappelle

Print an error message if /usr/sbin/radvd is missing.

Print an error message if the kernel does not support IPv6 forwarding, which is required by radvd.

Ignore any start/stop/restart option if /etc/radvd.conf does not exist. The previous script printed an error message in this case but is valid to install radvd without a configuration file. The daemon may be started later by another service with a configuration created at run-time.

This is a copy/paste/edit/fix of package/dnsmasq/S80dnsmasq.

Signed-off-by Carlos Santos <[email protected]>

---
Changes v1->v2
- Print error message is /usr/sbin/radvd is missing
- Print error message if /proc/sys/net/ipv6/conf/all/forwarding is missing (kernel does not support IPv6 forwarding)
- Echo "1" to /proc/sys/net/ipv6/conf/all/forwarding upon start

---
 package/radvd/S50radvd | 46 +++++++++++++++++++++++++++++++++-------------
 1 file changed, 33 insertions(+), 13 deletions(-)
package/radvd/S50radvd +33 -13
index 9f1407c..cc73f29 100755
@@ -1,18 +1,38 @@
 #!/bin/sh
 
-RADVD=/usr/sbin/radvd
-
-echo "1" > /proc/sys/net/ipv6/conf/all/forwarding
-
-printf "Starting radvd: "
-if [ ! -x "${RADVD}" ]; then
-	echo "missing"
+[ -x /usr/sbin/radvd ] || {
+	echo "Error: /usr/sbin/radvd is missing."
 	exit 1
-fi
+}
 
-if ${RADVD} ; then
-	echo "done"
-else
-	echo "failed"
+[ -f /proc/sys/net/ipv6/conf/all/forwarding ] || {
+	echo "Error: radvd requires IPv6 forwarding support."
 	exit 1
-fi
+}
+
+[ -f /etc/radvd.conf ] || {
+	exit 0
+}
+
+case "$1" in
+	start)
+		printf "Starting radvd: "
+		echo "1" > /proc/sys/net/ipv6/conf/all/forwarding
+		start-stop-daemon -S -x /usr/sbin/radvd
+		[ $? = 0 ] && echo "OK" || echo "FAIL"
+		;;
+	stop)
+		printf "Stopping radvd: "
+		start-stop-daemon -K -q -x /usr/sbin/radvd
+		[ $? = 0 ] && echo "OK" || echo "FAIL"
+		;;
+	restart|reload)
+		$0 stop
+		$0 start
+		;;
+	*)
+		echo "Usage: $0 {start|stop|restart}"
+		exit 1
+esac
+
+exit 0
Signature · 1 line
2.7.5
#4 lore raw

PATCH v2 1/1 ] radvd: improve startup script

Hello,

Quoted 11 lines · Carlos Santos

Print an error message if /usr/sbin/radvd is missing.

Print an error message if the kernel does not support IPv6 forwarding, which is required by radvd.

Ignore any start/stop/restart option if /etc/radvd.conf does not exist. The previous script printed an error message in this case but is valid to install radvd without a configuration file. The daemon may be started later by another service with a configuration created at run-time.

This is a copy/paste/edit/fix of package/dnsmasq/S80dnsmasq.

Not quite true: since v1, you changed things and you're no longer doing like S80dnsmasq anymore.

package/radvd/S50radvd line 6

Quoted 5 lines

+[ -x /usr/sbin/radvd ] || {

package/radvd/S50radvd +2 -1
+	echo "Error: /usr/sbin/radvd is missing."
 	exit 1
-fi
+}

I think this test is useless. Why not let start-stop-daemon fail is /usr/sbin/radvd is missing? It's unlikely to happen because Buildroot installs both radvd and its init script as part of the same package. But if it ever happens for some reason, the error message from start-stop-daemon should be pretty clear.

Best regards,

Thomas

Signature · 3 lines
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
author
#5 lore raw

PATCH v3 1/1 radvd: improve startup script

In reply to Carlos Santos

- Add start, stop and restart/reload options.

- Do nothing if /etc/radvd.conf does not exist instead of printing an error message. It is valid to install radvd without a configuration file. The daemon may be started later by another service with a configuration created at run-time.

- Print an error message if the kernel does not support IPv6 forwarding, which is required by radvd.

Signed-off-by Carlos Santos <[email protected]>

---
Changes v2->v3
- Don't the test if the binary is executable. It's unlikely to happen because Buildroot installs both radvd and its init script as part of the same package. But if it ever happens for some reason, the error message from start-stop-daemon should be pretty clear (Thomas Petazzoni).
- Move start and stop to functions and rewrite the error handling code to improve its readability.
- Add a one second sleep between stop and start, in restart, as made in several other scripts.

Changes v1->v2
- Print error message is /usr/sbin/radvd is missing
- Print error message if /proc/sys/net/ipv6/conf/all/forwarding is missing (kernel does not support IPv6 forwarding)
- Echo "1" to /proc/sys/net/ipv6/conf/all/forwarding upon start

---
 package/radvd/S50radvd | 54 ++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 41 insertions(+), 13 deletions(-)
package/radvd/S50radvd +41 -13
index 9f1407c95a..c27ac4302e 100755
@@ -1,18 +1,46 @@
 #!/bin/sh
 
-RADVD=/usr/sbin/radvd
+test -f /etc/radvd.conf || exit 0
 
-echo "1" > /proc/sys/net/ipv6/conf/all/forwarding
-
-printf "Starting radvd: "
-if [ ! -x "${RADVD}" ]; then
-	echo "missing"
+test -f /proc/sys/net/ipv6/conf/all/forwarding || {
+	echo "Error: radvd requires IPv6 forwarding support."
 	exit 1
-fi
+}
 
-if ${RADVD} ; then
-	echo "done"
-else
-	echo "failed"
-	exit 1
-fi
+start() {
+	printf "Starting radvd: "
+	echo "1" > /proc/sys/net/ipv6/conf/all/forwarding
+	start-stop-daemon -S -x /usr/sbin/radvd || {
+		echo "FAIL"
+		exit 1
+	}
+	echo "OK"
+}
+
+stop() {
+	printf "Stopping radvd: "
+	start-stop-daemon -K -q -x /usr/sbin/radvd || {
+		echo "FAIL"
+		exit 1
+	}
+	echo "OK"
+}
+
+case "$1" in
+	start)
+		start
+		;;
+	stop)
+		stop
+		;;
+	restart|reload)
+		stop
+		sleep 1
+		start
+		;;
+	*)
+		echo "Usage: $0 {start|stop|restart}"
+		exit 1
+esac
+
+exit 0
Signature · 1 line
2.14.3
#6 lore raw

PATCH v3 1/1 radvd: improve startup script

Hello,

package/radvd/S50radvd line 3

Quoted 2 lines · Carlos Santos
package/radvd/S50radvd +1 -1
-RADVD=/usr/sbin/radvd
+test -f /etc/radvd.conf || exit 0

I'm still not impressed by silent exit cases. Shouldn't we let radvd fail to start and complain about the lack of radvd.conf ?

package/radvd/S50radvd line 16

Quoted 7 lines
package/radvd/S50radvd +7 -0
+start() {
+	printf "Starting radvd: "
+	echo "1" > /proc/sys/net/ipv6/conf/all/forwarding
+	start-stop-daemon -S -x /usr/sbin/radvd || {
+		echo "FAIL"
+		exit 1
+	}

Can we use the

	[ $? = 0 ] && echo "OK" || echo "FAIL"

syntax that we use in almost all other init scripts ?

package/radvd/S50radvd line 25

Quoted 9 lines
package/radvd/S50radvd +9 -0
+	echo "OK"
+}
+
+stop() {
+	printf "Stopping radvd: "
+	start-stop-daemon -K -q -x /usr/sbin/radvd || {
+		echo "FAIL"
+		exit 1
+	}

Ditto here.

Also, can we use a pid file managed by start-stop-daemon, like S50dropbear is doing (and many other init scripts) ?

Thanks!

Thomas

Signature · 3 lines
Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
author
#7 lore raw

PATCH v3 1/1 radvd: improve startup script

Quoted 8 lines

From: "Thomas Petazzoni" <[email protected]>
To: "Carlos Santos" <[email protected]>
Cc: "buildroot" <[email protected]>
Sent: Wednesday, April 25, 2018 6:25:12 PM
Subject: Re: [Buildroot] [PATCH v3] radvd: improve startup script

Hello,

Quoted 7 lines

package/radvd/S50radvd line 3

Quoted 2 lines · Carlos Santos
package/radvd/S50radvd +1 -1
-RADVD=/usr/sbin/radvd
+test -f /etc/radvd.conf || exit 0

I'm still not impressed by silent exit cases. Shouldn't we let radvd fail to start and complain about the lack of radvd.conf ?

Hum, yes, but as already discussed in a different thread there is no standard regarding these cases.

So far I'm just following the example existing in other start-up scripts:

    $ grep 'test -f .* || exit' package/*/S[0-9]*
    package/dhcp/S80dhcp-relay:test -f /usr/sbin/dhcrelay || exit 0
    package/dhcp/S80dhcp-server:test -f /usr/sbin/dhcpd || exit 0
    package/dhcp/S80dhcp-server:test -f /etc/dhcp/dhcpd.conf || exit 0
    package/mpd/S95mpd:test -f /etc/mpd.conf || exit 0
Quoted 10 lines

package/radvd/S50radvd line 16

Quoted 7 lines
package/radvd/S50radvd +7 -0
+start() {
+	printf "Starting radvd: "
+	echo "1" > /proc/sys/net/ipv6/conf/all/forwarding
+	start-stop-daemon -S -x /usr/sbin/radvd || {
+		echo "FAIL"
+		exit 1
+	}

Can we use the

Quoted 3 lines
	[ $? = 0 ] && echo "OK" || echo "FAIL"

syntax that we use in almost all other init scripts ?

No, because the echo "FAIL" command succeeds, masking the error result:

    $ (false; [ $? = 0 ] && echo "OK" || echo "FAIL";); echo $?
    FAIL
    0

instead of

    $ (false || { echo "FAIL"; exit 1; }; echo "OK";); echo $?
    FAIL
    1

Moreover shellcheck complains that it is bad syntax:

    $ shellcheck package/radvd/S50radvd

In package/radvd/S50radvd line 14:
[ $? = 0 ] && echo "OK" || echo "FAIL"
^-- SC2181: Check exit code directly with e.g. 'if mycmd;', not indirectly with $?.

In fact many Buildroot start-up scripts make shellcheck have heart attacks. :-)

Quoted 13 lines

package/radvd/S50radvd line 25

Quoted 10 lines
package/radvd/S50radvd +9 -0
+	echo "OK"
+}
+
+stop() {
+	printf "Stopping radvd: "
+	start-stop-daemon -K -q -x /usr/sbin/radvd || {
+		echo "FAIL"
+		exit 1
+	}

Ditto here.

Quoted 2 lines

Also, can we use a pid file managed by start-stop-daemon, like S50dropbear is doing (and many other init scripts) ?

Yes, but I'd prefer make this change in a different patch if you don't mind, since it requires additional testing.

Signature · 4 lines
Carlos Santos (Casantos) - DATACOM, P&D
?The greatest triumph that modern PR can offer is the transcendent 
success of having your words and actions judged by your reputation, 
rather than the other way about.? ? Christopher Hitchens