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

Hi,

On Tue, 2017-07-25 at 18:07 +0200, Jonatan Schlag wrote:

Signed-off-by: Jonatan Schlag jonatan.schlag@ipfire.org

src/functions/functions.zone | 33 ++++++++++++++++++++++++---------  1 file changed, 24 insertions(+), 9 deletions(-)

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() {   zone_config_new "${zone}" "$@"   ;;   destroy)

• 	# id could be also a valid hid
  

local id=${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=$(zone_config_convert_hid_to_id ${zone}
  

${id})   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.

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
  

fi   ;;   list)   zone_config_list "${zone}" "$@"   ;;   *)

• 	# Check is we get a valid id
  

• 	# TODO This could be also a valid hid
  

• 	# id could be also a valid hid
  

local id=${cmd}

• 	if zone_config_id_is_valid ${zone} ${id} && [[ ${1}
  

== "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}
  

== "edit" ]]; then

• 		id=$(zone_config_convert_hid_to_id ${zone}
  

${id})

Can we also remove this ${1} == 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 = $(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.

shift 1   zone_config_edit "${zone}" "${id}" "$@"   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} == "edit" ]]; then

• 			shift 1
  

• 			zone_config_edit "${zone}" "${id}"
  

"$@"

• 		else
  

• 			error "Unrecognized argument: ${cmd}"
  

• 			cli_usage root-zone-config-
  

subcommands

• 			exit ${EXIT_ERROR}
  

• 		fi
  

fi   ;;   esac

This looks good.

It might be better to print just "-" if there is no HID, but that only makes parsing this output easier which nobody should be doing any ways...

-Michael

On Tue, 2017-07-25 at 18:07 +0200, Jonatan Schlag wrote:

Signed-off-by: Jonatan Schlag jonatan.schlag@ipfire.org

src/functions/functions.zone | 9 +++++----  1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/src/functions/functions.zone b/src/functions/functions.zone index 091bcd6..17d0858 100644 --- a/src/functions/functions.zone +++ b/src/functions/functions.zone @@ -634,21 +634,22 @@ zone_config_list() {   assert isset zone     # Print a nice header

• local format="%-3s %-20s"
• print "${format}" "ID" "HOOK"

• local format="%-3s %-20s %-20s"
• print "${format}" "ID" "HOOK" "HID"

local config   local hook   local id

• local hid

# Print for all config:   # id and hook

• # TODO: Add hids here

for config in $(zone_configs_list "${zone}"); do   id=${config##*.}   hook=$(zone_config_get_hook "${zone}" "${config}")

• hid=$(zone_config_get_hid "${zone}" "${config}")
  

assert isset hook

• print "${format}" "${id}" "${hook}"
  

• print "${format}" "${id}" "${hook}" "${hid}"
  

done  }