From mboxrd@z Thu Jan  1 00:00:00 1970
From: Michael Tremer <michael.tremer@ipfire.org>
To: development@lists.ipfire.org
Subject: Re: [PATCH] convert-dns-settings: Fix check to prevent doubble-adding
 the same server
Date: Tue, 28 Jan 2020 15:32:08 +0000
Message-ID: <F7C6E232-950A-4EEE-8398-4A17A9C8A7B7@ipfire.org>
In-Reply-To: <20200128151435.8671-1-stefan.schantl@ipfire.org>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="===============1060951760510866072=="
List-Id: <development.lists.ipfire.org>

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

Hi,

I am not sure if I agree with this patch, yet...

> On 28 Jan 2020, at 15:14, Stefan Schantl <stefan.schantl(a)ipfire.org> wrot=
e:
>=20
> Signed-off-by: Stefan Schantl <stefan.schantl(a)ipfire.org>
> ---
> src/scripts/convert-dns-settings | 23 +++++++++++++++--------
> 1 file changed, 15 insertions(+), 8 deletions(-)
>=20
> diff --git a/src/scripts/convert-dns-settings b/src/scripts/convert-dns-set=
tings
> index e9d4de86b..073356f50 100755
> --- a/src/scripts/convert-dns-settings
> +++ b/src/scripts/convert-dns-settings
> @@ -43,17 +43,24 @@ main() {
> 				for var in DNS1 DNS2; do
> 					local server=3D"${!var}"
>=20
> -					# Check if the current server is allready part
> -					# of the array.
> -					for element in "${SERVERS[@]}"; do
> -						[[ $element =3D=3D $server ]] && continue
> -					done
> -
> -					SERVERS+=3D($server)
> +					# Check if the servers array is empty.
> +					if [ ${#SERVERS[@]} -eq 0 ]; then
> +						# Allways add the first found nameserver to the array.
> +						SERVERS+=3D($server)

Please quote things in shell and use curly brackets for variables. It should =
read: SERVERS+=3D( =E2=80=9C${server}=E2=80=9D )

> +					else
> +						# Check if the current server is allready part ot the array.
> +						if [[ ! "${SERVERS[@]}" =3D~ "${server}" ]]; then

Why would this not work when the array is empty? i.e. why do we need to check=
 above?

> +							# Add the server to the array.
> +							SERVERS+=3D($server)

Same.

> +						fi
> +					fi
> 				done
>=20
> 				# Remove DNS1 and DNS2 settings from profile file.
> -				sed -i "/^DNS[12]?=3D/d" $file
> +				sed -i "/^DNS[12]=3D/d" $file

Again missing quotes, but why is the DNS=3DAutomatic or DNS=3DManual settings=
 not being removed any more?

The question mark was intentional.

> +
> +				# Unset the local variables for the next round.
> +				unset DNS1 DNS2
> 			done
>=20
> 		elif [ -s "/var/ipfire/dns/settings" ]; then
> =E2=80=94
> 2.25.0
>=20


-Michael


--===============1060951760510866072==--