[Buildroot] [PATCH] radvd: improve startup script

[Buildroot] [PATCH] radvd: improve startup script

[Carlos Santos]

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 <casantos@datacom.ind.br>
---
 package/radvd/S50radvd | 36 ++++++++++++++++++++++--------------
 1 file changed, 22 insertions(+), 14 deletions(-)

diff --git a/package/radvd/S50radvd b/package/radvd/S50radvd
index 9f1407c..dcc2af6 100755
--- a/package/radvd/S50radvd
+++ b/package/radvd/S50radvd
@@ -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
-- 
2.7.5

[Buildroot] [PATCH] radvd: improve startup script

[Arnout Vandecappelle]

On 25-09-17 11:57, Carlos Santos wrote:
> 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.

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

 Perhaps not the best example...

> 
> Signed-off-by: Carlos Santos <casantos@datacom.ind.br>
> ---
>  package/radvd/S50radvd | 36 ++++++++++++++++++++++--------------
>  1 file changed, 22 insertions(+), 14 deletions(-)
> 
> diff --git a/package/radvd/S50radvd b/package/radvd/S50radvd
> index 9f1407c..dcc2af6 100755
> --- a/package/radvd/S50radvd
> +++ b/package/radvd/S50radvd
> @@ -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.

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

> +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
> +		[ $? = 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

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

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

[Buildroot] [PATCH] radvd: improve startup script

[Carlos Santos]

----- Original Message -----
> From: "Arnout Vandecappelle" <arnout@mind.be>
> To: "Carlos Santos" <casantos@datacom.ind.br>, buildroot at buildroot.org
> Sent: Tuesday, September 26, 2017 7:26:20 PM
> Subject: Re: [Buildroot] [PATCH] radvd: improve startup script

> On 25-09-17 11:57, Carlos Santos wrote:
>> 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.

>> 
>> This is a simple copy/paste/edit of package/dnsmasq/S80dnsmasq.
> 
> Perhaps not the best example...
> 
>> 
>> Signed-off-by: Carlos Santos <casantos@datacom.ind.br>
>> ---
>>  package/radvd/S50radvd | 36 ++++++++++++++++++++++--------------
>>  1 file changed, 22 insertions(+), 14 deletions(-)
>> 
>> diff --git a/package/radvd/S50radvd b/package/radvd/S50radvd
>> index 9f1407c..dcc2af6 100755
>> --- a/package/radvd/S50radvd
>> +++ b/package/radvd/S50radvd
>> @@ -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.
> 
>> +[ -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.

>> +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
>> +		[ $? = 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

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

[Buildroot] [PATCH v2]] radvd: improve startup script

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

Signed-off-by: Carlos Santos <casantos@datacom.ind.br>
---
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(-)

diff --git a/package/radvd/S50radvd b/package/radvd/S50radvd
index 9f1407c..cc73f29 100755
--- a/package/radvd/S50radvd
+++ b/package/radvd/S50radvd
@@ -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
-- 
2.7.5

[Buildroot] [PATCH v2]] radvd: improve startup script

[Thomas Petazzoni]

Hello,

On Tue, 26 Sep 2017 23:16:09 -0300, Carlos Santos wrote:
> 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.

> +[ -x /usr/sbin/radvd ] || {
> +	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
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[Buildroot] [PATCH v3] radvd: improve startup script

[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 <casantos@datacom.ind.br>
---
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(-)

diff --git a/package/radvd/S50radvd b/package/radvd/S50radvd
index 9f1407c95a..c27ac4302e 100755
--- a/package/radvd/S50radvd
+++ b/package/radvd/S50radvd
@@ -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
-- 
2.14.3

[Buildroot] [PATCH v3] radvd: improve startup script

[Thomas Petazzoni]

Hello,

On Sun, 15 Apr 2018 23:10:37 -0300, Carlos Santos wrote:

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

> +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 ?

> +	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
-- 
Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH v3] radvd: improve startup script

[Carlos Santos]

> From: "Thomas Petazzoni" <thomas.petazzoni@bootlin.com>
> To: "Carlos Santos" <casantos@datacom.ind.br>
> Cc: "buildroot" <buildroot@buildroot.org>
> Sent: Wednesday, April 25, 2018 6:25:12 PM
> Subject: Re: [Buildroot] [PATCH v3] radvd: improve startup script

> Hello,
> 
> On Sun, 15 Apr 2018 23:10:37 -0300, Carlos Santos wrote:
> 
>> -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

>> +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 ?

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

> 
>> +	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) ?

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

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