From mboxrd@z Thu Jan  1 00:00:00 1970
From: Michael Tremer <michael.tremer@ipfire.org>
To: network@lists.ipfire.org
Subject: Re: [PATCH] Fix radvd startup
Date: Tue, 06 Feb 2018 00:52:24 +0000
Message-ID: <1517878344.21272.67.camel@ipfire.org>
In-Reply-To: <1517072778-3768-1-git-send-email-jonatan.schlag@ipfire.org>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="===============6193847363211360549=="
List-Id: <network.lists.ipfire.org>

--===============6193847363211360549==
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable

Hi,

On Sat, 2018-01-27 at 17:06 +0000, Jonatan Schlag wrote:
> We now only start radvd when we write a config fr a zone into the config
> file and when no errors happen.
>=20
> Fixes: #11450
>=20
> Signed-off-by: Jonatan Schlag <jonatan.schlag(a)ipfire.org>
> ---
>  src/functions/functions.radvd | 31 +++++++++++++++++++------------
>  1 file changed, 19 insertions(+), 12 deletions(-)
>=20
> diff --git a/src/functions/functions.radvd b/src/functions/functions.radvd
> index 4e41160..bd016a0 100644
> --- a/src/functions/functions.radvd
> +++ b/src/functions/functions.radvd
> @@ -23,16 +23,16 @@ RADVD_CONFIGFILE=3D"/etc/radvd.conf"
> =20
>  radvd_update() {
>  	# (Re-)write the configuration file
> -	radvd_write_config
> -
> -	# Reload the radvd service if it is already running
> -	if service_is_active radvd; then
> -		service_reload radvd
> -		return ${EXIT_OK}
> +	if radvd_write_config; then
> +		# Reload the radvd service if it is already running
> +		if service_is_active radvd; then
> +			service_reload radvd
> +			return ${EXIT_OK}
> +		fi
> +
> +		# Start the radvd service
> +		service_start radvd
>  	fi
> -
> -	# Start the radvd service
> -	service_start radvd
>  }

This part is okay assuming that radvd_write_config is returning an error when
the configuration file is empty.

> =20
>  radvd_clear_config() {
> @@ -48,12 +48,19 @@ radvd_write_config() {
> =20
>  	# Write the configuration for all zones.
>  	local zone
> -	for zone in $(zones_get_local); do
> -		__radvd_config_interface ${zone}
> +	# The return value determine if radvd is started or not
> +	local return_value=3D${EXIT_FALSE}
> =20
> +	for zone in $(zones_get_local); do
> +		if __radvd_config_interface ${zone} && [ ${return_value} =3D
> ${EXIT_TRUE} ]; then
> +			# We return TRUE when __radvd_config_interface
> succeed and no errors happend till now
> +			return_value=3D${EXIT_TRUE}
> +		else
> +			return_value=3D${EXIT_FALSE}
> +		fi
>  	done >> ${RADVD_CONFIGFILE}
> =20
> -	return ${EXIT_OK}
> +	return ${return_value}
>  }

This is a bit spaghetti and can be written better. Like:

  r =3D TRUE

  for zone in ...; do
    if ! __radvd_config_interface "${zone}"; then
       r =3D FALSE
    fi
  done

  return r

Of course that is just pseudo-code, but it spares the else clause and the
comparison which isn't needed.

You will always run radvd even when you have interfaces that don't have IPv6.=
 I
don't see where this is now being skipped.

-Michael

--===============6193847363211360549==
Content-Type: application/pgp-signature
Content-Transfer-Encoding: base64
Content-Disposition: attachment; filename="signature.asc"
MIME-Version: 1.0

LS0tLS1CRUdJTiBQR1AgU0lHTkFUVVJFLS0tLS0KCmlRSXpCQUFCQ2dBZEZpRUU1L3JXNWwzR0dl
Mnlwa3R4Z0hudy8yK1FDUWNGQWxwNC9FZ0FDZ2tRZ0hudy8yK1EKQ1FjTW5SQUFrbDlUcjUrUTJx
TmZTeEc2dGJjaFltSWNBaDlCY3hjaC9rYlR2Z3RNY2k4NXVHY1BQK2xNKzlXegpaSFYvNjk5cVNs
ZTR4ekVQRVlWVkF6bXgweEdHR0ZrajlrQjdKV1JIZ0lOZGQxVXkwbGdFN29qVHRHV3orU3ZxCnZQ
ek9kYXQ5cHNLNjV6cmNaWHBKeUxXNnYyVDdDTzhFOUJkQjJhdytEc0pUMERjN28vTURTMmRydEVC
WS93ODMKelRJT1ZqQkFkWCtoYVRoSkJHRjNYYUFJL0xscElrdm55bWxZZ3ZKWG1BNU5OemZBR0xB
MnVSUjQrL2k2SHk5dwpkSHppWUlaTDEwemhCbkhvV3BDTFhIU1NGTWdIUHhwODZvWURZb2duSjB4
ekJMaTBQNVZkYVpZSkZ1ZWdjcS8xCkZuSU5tcTAxaGhxMkIxM3ZNRlNqUEs4cDV1R1poNytKdHgv
NnlXVWNEY2lNdVZjY2RGT1Q1OVZnWU8zaG5sQnYKMlJ3aFlyYmRTSU1EUEZSeXhubnFtc1FXMUlS
K2ZDNjN2VFJyNTJRelkxemhWa0FYUWQ3RXZiZDgvK2RYYmZ2SApab1hqa2tINGZjTk81N0VDNHc3
VEx4VXF3aE5ab3pnVnNFNXlCdmxkd1l3Z05kQXVQWldDbUZhbzk4Q0xNQ0NHCkdOYlpxY25XYXRo
N29GUTh4TDRtbitEVFJZM1JteURGeTVsSWdzZzJ2SEZEV3FiTmhvK2dsbkhQbWt3SDArWlAKN3Bs
ZDl3TUNXTmN0YmZYQ0t4Q1ZQbUphN1dtcUxxOVE2bEhHeEhBeW0rckNKa2NHRGFud0lHWHF6Rnlv
M0xQeApXNjI3d29JVlhpMTFsZWhwcGYxZlFFMlQyM0k2UmFhTEVpSzdoWkMyVFBFeWxabzk1Sjg9
Cj1kUnZBCi0tLS0tRU5EIFBHUCBTSUdOQVRVUkUtLS0tLQo=

--===============6193847363211360549==--