Hi,

and no no no no. We cannot restart the zone just because some configuration is being changed. That would disrupt connections and that might not always be required.

The hook_up() and hook_down() functions are usually designed to take care of this and check if a zone is already up and potentially only change the settings that need to be changed.

That will allow to edit things on a live system without disrupting any users which I think is very important.

-Michael

On Fri, 2017-07-14 at 21:10 +0200, Jonatan Schlag wrote:

Before a config is edit we should bring the config down and after up again. Also we need to have 2 arguments or more.

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

src/header-zone | 9 ++++++++-  1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/src/header-zone b/src/header-zone index 2e3fa09..3b074ee 100644 --- a/src/header-zone +++ b/src/header-zone @@ -225,7 +225,7 @@ hook_config_destroy() {  }    hook_config_edit() {

• assert [ $# -eq 2 ]

• assert [ $# -ge 2 ]

local zone=${1}   # The id must be the id and not the hid.   local id=${2} @@ -240,7 +240,14 @@ hook_config_edit() {   local hook=$(zone_config_get_hook_from_id ${zone} ${id})   assert isset hook

• # Bring the config down
• hook_config_cmd "down" "${zone}" "${hook}" "${hook}.${id}"
• # Edit the hook

hook_config_cmd "edit" "${zone}" "${hook}" "${hook}.${id}" "$@"

• # Bring the config up again
• hook_config_cmd "up" "${zone}" "${hook}" "${hook}.${id}"

}    hook_config_show() {

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.

• 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