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 3/3] dhcp: add hook_edit() function
Date: Fri, 14 Jul 2017 20:53:09 -0400
Message-ID: <1500079989.2325.5.camel@ipfire.org>
In-Reply-To: <1500059452-3421-3-git-send-email-jonatan.schlag@ipfire.org>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="===============5908486827999689701=="
List-Id: <network.lists.ipfire.org>

--===============5908486827999689701==
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 8bit

Hello,

On Fri, 2017-07-14 at 21:10 +0200, Jonatan Schlag wrote:
> Basically we just read in the config,
> parsing the passed command line options (so we set the new value for
> he passed options)
> and writing the config back.
> 
> Signed-off-by: Jonatan Schlag <jonatan.schlag(a)ipfire.org>
> ---
>  src/hooks/configs/dhcp | 40 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
> 
> diff --git a/src/hooks/configs/dhcp b/src/hooks/configs/dhcp
> index 8bb6aa9..98ef5e3 100644
> --- a/src/hooks/configs/dhcp
> +++ b/src/hooks/configs/dhcp
> @@ -83,6 +83,46 @@ hook_new() {
>  	exit ${EXIT_OK}
>  }
>  
> +hook_edit() {
> +	local zone=${1}
> +	local config=${2}
> +	shift 2
> +
> +	if ! device_exists ${zone}; then
> +		error "Zone '${zone}' doesn't exist."
> +		exit ${EXIT_ERROR}
> +	fi

I think you want to call zone_exists here. device_exists only returns
true when an interface exists with that name which would result into
the hook not being editable when the zone is down. That's bad.

I am quite sure that this is checked before so that the hook can rely
on the calling zone existing. No need to do this here again.

> +
> +	local ${HOOK_CONFIG_SETTINGS}
> +
> +	# If reading the config fails we cannot go on
> +	if ! zone_config_settings_read "${zone}" "${config}"; then
> +		log ERROR "Could read the config ${config} for zone
> ${upl0}"
> +		return ${EXIT_ERROR}
> +	fi
> +
> +	if ! hook_parse_cmdline $@; then
> +		# Return an error if the parsing of the cmd line
> fails
> +		return ${EXIT_ERROR}
> +	fi
> +
> +	# Check if the user disabled ipv4 and ipv6
> +	if ! enabled ENABLE_IPV6 && ! enabled ENABLE_IPV4; then
> +		log ERROR "You disabled IPv6 and IPv4. At least one
> must be enabled"
> +		return ${EXIT_ERROR}
> +	fi

Should this check not be in the hook_parse_cmdline function since it is
needed when the config is created, too. We don't want duplication and
it would be nice if we could have a generic hook_edit() function when
hook_parse_cmdline exists.

> +
> +	local hook="${config//.[[:digit:]]/}"
> +	local id="${config//"${hook}."/}"

Can we have functions for these two? This looks not very intuitive and
we will need this in other hooks, too.

> +
> +	if ! zone_config_settings_write "${zone}" "${hook}" ${id};
> then
> +		log ERROR "Could not write config settings file
> ${config} for ${zone}"
> +		return ${EXIT_ERROR}
> +	fi
> +
> +	exit ${EXIT_OK}
> +}
> +
>  hook_up() {
>  	local zone=${1}
>  	local config=${2}

-Michael

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

LS0tLS1CRUdJTiBQR1AgU0lHTkFUVVJFLS0tLS0KVmVyc2lvbjogR251UEcgdjIKCmlRSWNCQUFC
Q2dBR0JRSlphV2QxQUFvSkVJQjU4UDl2a0FrSFFiSVFBTFZodXl1QXdGeHk5OXBQMkxyamVLS00K
NkdUVXo5MmtvbXVGM3Y4RTZPZUN4M255d0JwNHcxK3N6WkRTSk5GNW1XRzJ5Mkxvc2dQZFRKLzRq
ck1YZ1l0QQpXUTZxQmNYaHZqRlg0aWcwVFFkOE9Ka0VsV1MyK1FoSGZ6ZHF2MUs3dlZ3R09YTVdy
QVpUcDFNbDIrUzJINFRuCjdJYm5ueFFQcXNndzI4VCtqbzd2NXVocEpQc0ttd2c2Z0J5Ry9NVElF
Ylg1eklHREZ4K3pSVSttTWlDMURnSEEKd09EaVE5RW84bUJSL3NlZ1hzK25DcGhiSjVGTGl2am5x
eG1aQTdJeGpIVEdtdDNCZ1VweG8yTFFnbWRPNHhoQgpoejU5MHMzVW9ldDM0bUcvL3V4Z1JhaHhO
ODVpdGdVSUxERTJ0ODNkSWlkM1RBVGg2NlFCOFpsNS95QVNiamlzCnN0OWlOenpuNkQyRVVpWDNB
WUVUU3RROVJIbWllZWYzM2NscXRLbnNKcGQrZ1R1OFI4Z1hFbG5nWWxLL3o2ak4KaC93RVBZUUY2
VXlGL1ZCZU95UCt6NzJuZ1o3Q2hlY3MrN3hMbFZ5SGJ5Q2VCREJ1Q0RaVDZTTXpJOXpGbDNEbQpG
QSsvZEJ5YW5jSlM1dkhwalZNdXIvSzdTa2U0QnJudlhpbjBvaUZES3NHbHBmVlhIQW11OHg2cnV6
L0c3RGd6CnEwUEhSdHJISG9KTXBBUkZaTWhaZnBuWGZkL2VoWk5hb1A0bXAvVzRIb1RHK3ZHTTlz
WVlaZ2xvdW1mek93UWcKMGVsTUYxV0NqMzU2SnAxOERpcVcyQjJ2WW5Jc0tpemxJT1FlWmRQNU1v
Ny92VFg0ZTdGWC9SRi9nSFhpR2FZRApzR0l0WG9NL1R5MWNMREdvMzVlRgo9SCtXcwotLS0tLUVO
RCBQR1AgU0lHTkFUVVJFLS0tLS0K

--===============5908486827999689701==--