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

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@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