Re: [PATCH 3/3] dhcp: add hook_edit() function

Hi,

Am Sa, 15. Jul, 2017 um 2:53 schrieb Michael Tremer michael.tremer@ipfire.org:

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

I would not be so sure if device_exist is called before but i will have a look at it.

...

• 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.

Ok

...

• 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.

Ok

...

• 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

Jonatan