Re: [PATCH 1/8] zone: add config hid functions

[Michael Tremer <michael.tremer@ipfire.org>]

[-- Attachment #1: Type: text/plain, Size: 4045 bytes --]

Hi,

On Tue, 2017-07-25 at 18:07 +0200, Jonatan Schlag wrote:
> The following patch adds the basic functions which are we need to work with
> hids.
> 
> Fixes: #11406 (partially)
> 
> Signed-off-by: Jonatan Schlag <jonatan.schlag(a)ipfire.org>
> ---
>  src/functions/functions.zone | 86
> ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 86 insertions(+)
> 
> diff --git a/src/functions/functions.zone b/src/functions/functions.zone
> index bba1705..28a22fe 100644
> --- a/src/functions/functions.zone
> +++ b/src/functions/functions.zone
> @@ -656,6 +656,75 @@ zone_config_list_ids() {
>  	echo ${ids}
>  }
>  
> +# List all hids of a zone
> +zone_config_list_hids() {
> +	assert [ $# -eq 1 ]
> +	local zone=${1}
> +
> +	local config
> +	local hids
> +
> +	for config in $(zone_configs_list ${zone}); do
> +		# Get hook from config
> +		hook="$(zone_config_get_hook "${zone}" "${config}")"
> +		# Append hids to the list
> +		list_append hids "$(zone_config_get_hid "${zone}"
> "${config}")"
> +	done
> +
> +	echo ${hids}
> +}

In this function, you are using a hook variable that is never initalised nor
used. So I would say that can just be removed.

And unless you insist that the function returns its result in one line, you
could just let "zone_config_get_hid" write to stdout and remove the hids
variable and list_append entirely. That will make this function way faster since
it doesn't need to fork as many sub-shells.

> +
> +# get the hid from a given config
> +zone_config_get_hid() {
> +	assert [ $# -eq 2 ]
> +	local zone=${1}
> +	local config=${2}
> +
> +	local hook
> +
> +	hook="$(zone_config_get_hook "${zone}" "${config}")"
> +	echo "$(hook_exec "config" "${hook}" "hid" "${zone}" "${config}")"
> +}

Same here:

  echo "$(some function)"

could just be:

  some function

Same result. But doesn't call echo or the sub-shell to call the function.

> +
> +# Checks if a hid is valid for a given zone
> +zone_config_hid_is_valid() {
> +	assert [ $# -eq 2]
> +	local zone=${1}
> +	local hid=${2}
> +
> +	local _hid
> +	for _hid in $(zone_config_list_hids "${zone}"); do
> +		if [[ ${_hid} == ${hid} ]]; then
> +			return ${EXIT_TRUE}
> +		fi
> +	done
> +
> +	return ${EXIT_FALSE}
> +
> +}

Empty line before the closing bracket of the function.

And the check for equality in POSIX shell is just "=" and not "==".

> +
> +# This function converts a hid to a id
> +zone_config_convert_hid_to_id() {
> +	assert [ $# -eq 2 ]
> +	local zone=${1}
> +	local hid=${2}
> +
> +	local hook
> +	local config
> +	local id
> +
> +	for config in $(zone_configs_list ${zone}); do
> +		# Get hook from config
> +		hook="$(zone_config_get_hook "${zone}" "${config}")"
> +		if [[ "$(hook_exec "config" "${hook}" "hid" "${zone}"
> "${config}")" == "${hid}" ]]; then
> +			id=$(config_get_id_from_config "${config}")
> +			break
> +		fi
> +	done
> +
> +	echo ${id}
> +}

Same as above. You could just let "config_get_id_from_config" print the ID and
then break or return. The ID variable is not needed.

> +
>  zone_show() {
>  	local zone=${1}
>  
> @@ -1114,6 +1183,23 @@ zone_config_id_is_valid() {
>  	[ -f ${zone_path}/configs/*.${id} ];
>  }
>  
> +# This function checks if a given hid is valid for a zone
> +# Return True when yes and false when no
> +zone_config_hid_is_valid() {
> +	assert [ $# -eq 2 ]
> +	local zone=${1}
> +	local hid=${2}
> +
> +	local _hid
> +	for _hid in $(zone_config_list_hids ${zone}); do
> +		if [[ ${_hid} == ${hid} ]]; then
> +			return ${EXIT_TRUE}
> +		fi
> +	done
> +
> +	return ${EXIT_FALSE}
> +}

This function is a duplicate.

I also think it should be called "zone_config_hid_exists" or
so since we technically don't know what a valid HID looks like. Every hook can
have something different here.

> +
>  zone_config_get_hook_from_id() {
>  	# Returns the hook for a given id
>  	assert [ $# -eq 2 ]

Best,
-Michael

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]