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 2/8] zone: accept also hids in zone_config()
Date: Tue, 25 Jul 2017 23:27:22 +0100
Message-ID: <1501021642.20312.33.camel@ipfire.org>
In-Reply-To: <1500998833-10543-3-git-send-email-jonatan.schlag@ipfire.org>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="===============8825013840632592275=="
List-Id: <network.lists.ipfire.org>

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

Hi,

On Tue, 2017-07-25 at 18:07 +0200, Jonatan Schlag wrote:
> Signed-off-by: Jonatan Schlag <jonatan.schlag(a)ipfire.org>
> ---
> =C2=A0src/functions/functions.zone | 33 ++++++++++++++++++++++++---------
> =C2=A01 file changed, 24 insertions(+), 9 deletions(-)
>=20
> diff --git a/src/functions/functions.zone b/src/functions/functions.zone
> index 28a22fe..091bcd6 100644
> --- a/src/functions/functions.zone
> +++ b/src/functions/functions.zone
> @@ -550,28 +550,43 @@ zone_config() {
> =C2=A0			zone_config_new "${zone}" "$@"
> =C2=A0			;;
> =C2=A0		destroy)
> +			# id could be also a valid hid
> =C2=A0			local id=3D${1}
> -			if zone_config_id_is_valid ${zone} ${id}; then
> +			# usually we will get a hid so we check first if we
> get one
> +			if zone_config_hid_is_valid ${zone} ${id}; then
> +				# Convert the hid into an id
> +				id=3D$(zone_config_convert_hid_to_id ${zone}
> ${id})
> =C2=A0				zone_config_destroy "${zone}" "$@"

Could we not just call "zone_config_convert_hid_to_id" (maybe the name would =
be
better without "convert") and if it does not return an ID, we assume that the
HID does not exist?

That would save us running through all the hooks twice which is quite an
expensive thing to do.

> =C2=A0			else
> -				log ERROR "${id} is not a valid id"
> +				# We get no valid hid so we check if we get a
> valid id
> +				if zone_config_id_is_valid ${zone} ${id};
> then
> +					zone_config_destroy "${zone}" "$@"
> +				else
> +					log ERROR "${id} is not a valid id or
> hid"
> +				fi
> =C2=A0			fi
> =C2=A0			;;
> =C2=A0		list)
> =C2=A0			zone_config_list "${zone}" "$@"
> =C2=A0			;;
> =C2=A0		*)
> -			# Check is we get a valid id
> -			# TODO This could be also a valid hid
> +			# id could be also a valid hid
> =C2=A0			local id=3D${cmd}
> -
> -			if zone_config_id_is_valid ${zone} ${id} && [[ ${1}
> =3D=3D "edit" ]]; then
> +			# usually we will get a hid so we check first if we
> get one
> +			if zone_config_hid_is_valid ${zone} ${id} && [[ ${1}
> =3D=3D "edit" ]]; then
> +				id=3D$(zone_config_convert_hid_to_id ${zone}
> ${id})

Can we also remove this ${1} =3D=3D edit thing. That has confused me quite a =
few
times and doesn't make the code very obvious to read.

But same thing here as above. Could be something like:

  local id =3D $(hid to id ${hid}
  if ! isset id; then
    ... check if HID is an ID
  fi

  ... assume that we have a valid ID from here on ...

This would have a little bit of a performance penalty if the user has given an
ID or an invalid HID. But I think it is rather likely that someone will give a
HID than an ID. Mainly because auto-completion will give a HID instead of an =
ID.

> =C2=A0				shift 1
> =C2=A0				zone_config_edit "${zone}" "${id}" "$@"
> =C2=A0			else
> -				error "Unrecognized argument: ${cmd}"
> -				cli_usage root-zone-config-subcommands
> -				exit ${EXIT_ERROR}
> +				# We get no valid hid so we check if we get a
> valid id
> +				if zone_config_id_is_valid ${zone} ${id} &&
> [[ ${1} =3D=3D "edit" ]]; then
> +					shift 1
> +					zone_config_edit "${zone}" "${id}"
> "$@"
> +				else
> +					error "Unrecognized argument: ${cmd}"
> +					cli_usage root-zone-config-
> subcommands
> +					exit ${EXIT_ERROR}
> +				fi
> =C2=A0			fi
> =C2=A0			;;
> =C2=A0	esac

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

LS0tLS1CRUdJTiBQR1AgU0lHTkFUVVJFLS0tLS0KVmVyc2lvbjogR251UEcgdjIKCmlRSWNCQUFC
Q2dBR0JRSlpkOFhLQUFvSkVJQjU4UDl2a0FrSDVCSVAvUlZETDR2SkZOT0ozRDZuOFZtajBQSlgK
aVB4SWtFQVZQOTBiNkRMRStrelowYXE5bExCMWQ4M1FxQWtCZm5SVGRBODNFMktXc0w4Z2p5WFkw
MGgyTHkrMQo0V29oUFBTYmVJY3J1ak1mN2xKVTlFKy9NTzBrUnd3dVlUVkJzQ2JoVFBlZnJuUmVs
ek5GTlNnVjNsN0haUEJaCk9sb1NPbVRJbW50Rk1rc3NnSjBqUzJFcGhCK20zcUdZc0xnYlYrSU1y
TldDK25jZXJjKzV0SWRjNlpWVlVMTHQKTXpYVHIzN1hveXpQODlmZ2Vub05OQW1DUS8rUXFjSmZz
NHJ3aHh6ZDVxWFZ0ZllJRFl1UC9CY3NPUEM5OWoxZAowL2NkQmt1T3FnOW1BSkVYdndndVNJMUtj
SkhFOWhUdDIxS2d2b01rak14cCtqYzI2K1dnNHhIWVBBRnlSV1g0CldmTC9ONU1KdmhidE1ZajRT
UWNCazZFZVovdVlFSGZvZjJSTm1aQW81S0RkVHZqUk1GR1lMWTkrNWpSd0R2S3MKZjhvc0RlRkFC
MzFtc3dNc3FvYXFEc1dOQlZ4YVN3M2xOZUpsamhtZ0lubVE4dnRzdTcwOGNYZkk4VU1HenV1YQpp
WDdlSitINVRhUTdqOEhIK3k2bDNUckhRRm8rTmhFNGhxaXg3M3FyajA3cHppSDAvNFZCcWFmZFdO
VUJoK0pvCmR0NUhGcUIyeEhnd2JMbGhFNEkwWHNsVFpKR1pCTTBmakREalJIMDFvNnYyQWhQZUdI
dzFUbHFBQUxUNWtpMlEKSTFiNTNURzZVWWVWeXFvQXVrYU9BVlEyMlE3c25jeFptUHBpMWp5YTJx
bzBTTkxSRndYSmtqaVk5enhzaGhWeAptRllGTkowOXVHcHRiMlF6QmhBNwo9ZTVqTAotLS0tLUVO
RCBQR1AgU0lHTkFUVVJFLS0tLS0K

--===============8825013840632592275==--