Hello,

for this entire patchset I have a few coding style comments on top of a few questions to clarify a few of the functions for me.

On Fri, 2017-06-09 at 12:17 +0200, Jonatan Schlag wrote:

The configuration of a port was stored in a file called: /etc/network/${port} This is bad because it is very hard to add further information which belong primary not to the configuration to this file.

So we change the settings file to /etc/network/${port}/settings like for the zones.

This make it possible to store other configurations like the color in other files in the directory /etc/network/${port}.

So this is quite a huge change. Ideally we would need to migrate configuration, but given that we are only at alpha stage, it might be okay to break compatibility a bit. I hope I won't regret saying any of this soon.

A workaround to move the config file into the new directory scheme is: port=p1 && mv /etc/network/ports/${port} /etc/network/ports/${port}- save \ && mkdir -p /etc/network/ports/${port} \ && mv /etc/network/ports/${port}-save /etc/network/ports/${port}/settings

where port is the name of the port like p1 or p0.

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

src/functions/functions.ports | 13 +++++++------  1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/src/functions/functions.ports b/src/functions/functions.ports index 40f4eae..b138238 100644 --- a/src/functions/functions.ports +++ b/src/functions/functions.ports @@ -20,12 +20,13 @@  #################################################################### ###########    port_dir() {

• echo "${NETWORK_CONFIG_DIR}/ports"

• local port="${1}"
• echo "${NETWORK_CONFIG_DIR}/ports/${port}"

}    port_list() {   local port

• for port in $(port_dir)/*; do

• for port in $(port_dir)*; do

port="$(basename "${port}")"   if port_exists "${port}"; then   print "${port}"

Not really sure if this works as it should. I think the original version was okay. It would probably be better to not call the port_dir function here at all.

@@ -113,13 +114,13 @@ port_file() {   local port="${1}"   assert isset port

• echo "$(port_dir)/${port}"

• echo "$(port_dir ${port})/settings"

}    port_exists() {   local port=${1}

• [ -f "${NETWORK_CONFIG_DIR}/ports/${port}" ]

• [ -d "${NETWORK_CONFIG_DIR}/ports/${port}" ]

}    port_get_hook() { @@ -207,7 +208,7 @@ port_destroy() {     port_remove "${port}"

• rm -f $(port_file ${port})

• rm -rf $(port_dir ${port})

}    port_create() { @@ -263,7 +264,7 @@ port_cmd() {    ports_get() {   local port

• for port in $(port_dir)/*; do

• for port in $(port_dir)*; do

port=$(basename ${port})   if port_exists ${port}; then   echo "${port}"

Same as above.

-Michael

Hi,

On Fri, 2017-06-09 at 12:17 +0200, Jonatan Schlag wrote:

The following patch adds color support for zone and ports

These comments about what the functions do should actually be in the code.

color_cli() Is the cli function to parse the options submitted by a user.

color_set() Write a given color into the color config file of a zone or port.

color_read() Read a color out of color config file of a zone or port. If this is unsuccessful we use white.

color_format_filename() Formats the color config file name.

color_hex_is_valid() Check if a color hex is valid.

color_hex2rgb() Converts a color hex into rgb values.

_find_nearest_rgb_value() Find the nearest value to an rgb value out of: 0; 95; 135; 175; 215; 255;

color_rgb2shell() Converts a rgb value triple into an xterm color code.

_set_color() Set the shell color which unfourtunately does not work for putty.

shell_set_color() Function to set the back and foreground color at once.

shell_reset_color() Reset the shell color.

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

src/functions/functions.colors | 196 +++++++++++++++++++++++++++++++++++++++++  1 file changed, 196 insertions(+)

diff --git a/src/functions/functions.colors b/src/functions/functions.colors index 8d7193c..9a3a529 100644 --- a/src/functions/functions.colors +++ b/src/functions/functions.colors @@ -73,3 +73,199 @@ MSG_STP_DISCARDING="${CLR_RED_BG}${CLR_WHITE_B} DISCARDING ${CLR_RESET}"  MSG_STP_LEARNING="${CLR_YELLOW_BG}${CLR_WHITE_B}  LEARNING   ${CLR_R ESET}"  MSG_STP_LISTENING="${CLR_YELLOW_BG}${CLR_WHITE_B}  LISTENING  ${CLR_ RESET}"  MSG_STP_BLOCKING="${CLR_RED_BG}${CLR_WHITE_B}  BLOCKING   ${CLR_RESE T}"

+color_cli() {

• local type=${1}
• local name=${2}
• local action=${3}
• shift 3
• case ${action} in

• set)
  

• 	color_set ${type} ${name} ${@}
  

• 	;;
  

• reset)
  

• 	# We set the color to white.
  

• 	color_set ${type} ${name} "ffffff"
  

• 	;;
  

• *)
  

• 	error "Invalid argument: ${action}"
  

• 	;;
  

• esac

+}

+color_set() {

... There is a little bit too much whitespace before color_set() starts.

• local type=${1}
• local name=${2}
• local COLOR=${3}
• # Check if we get to many arguments
• shift 3
• if [ $# -gt 0 ]; then

• error "Too many arguments: $@"
  

• return ${EXIT_ERROR}
  

• fi

This will just print the message "Too many arguments" without telling what function is actually complaining about anything.

• # Check if the color code is valid
• if ! color_hex_is_valid ${COLOR}; then

• error "Hexadecimal color code '${COLOR}' is not
  

valid"

• return ${EXIT_ERROR}
  

• fi
• local file=$(color_format_filename ${type} ${name})
• settings_write ${file} COLOR

+}

+color_read() {

• local type=${1}
• local name=${2}
• local file=$(color_format_filename ${type} ${name})
• local COLOR
• if ! settings_read ${file} COLOR; then

• COLOR="ffffff"
  

• fi
• print "${COLOR}"

+}

+color_format_filename() {

• local type=${1}
• local name=${2}
• case ${type} in

• zone)
  

• 	echo "$(zone_dir ${name})/color"
  

• 	;;
  

• port)
  

• 	echo "$(port_dir ${name})/color"
  

• 	;;
  

• esac

+}

+color_hex_is_valid() {

• [[ ${1} =~ ^[0-9a-fA-F]{6}$ ]]

+}

+color_hex2rgb() {

• local hex=${1}
• assert [ ${#hex} -eq 6 ]
• for (( i = 0; i < 6; i += 2 )); do

• hex2dec ${hex:${i}:2}
  

• done | tr '\n' ' '
• print # newline

+}

+_find_nearest_rgb_value() {

• # For the calculation of the xterm value the rgb values must

be:

• # 0; 95; 135; 175; 215; 255;
• # this function find the closest value of these 6 numbers

for a give rgb number

• local rgb=${1}
• local best_value
• local best_value_index
• local values=( 0 95 135 175 215 255 )
• local result
• local i=0
• local value
• for value in ${values[@]}; do

• result=$(( ${value} - ${rgb} ))
  

• result=$(abs ${result})
  

• if [ -z ${best_value} ]; then
  

• 	best_value=${result}
  

• 	best_value_index=${i}
  

• # In the first iteration best_value is empty and so
  

set to ${result}

• # two lines above. So if statement must use -le
  

because in the first iteration

• # is the best_value eqal to result
  

• elif [ ${result} -le ${best_value} ]; then
  

• 	best_value=${result}
  

• 	best_value_index=${i}
  

• fi
  

• (( i++ ))
  

• done
• echo "${best_value_index}"

+}

+color_rgb2shell() {

• assert [ $# -eq 3 ]
• local red=${1}
• local green=${2}
• local blue=${3}
• local color
• for color in red green blue; do

• printf -v "${color}" $(_find_nearest_rgb_value
  

${!color})

• done
• print $(( 16 + 36 * ${red} + 6 * ${green} + ${blue} ))

+}

+_set_color() {

• local where=${1}
• local color=${2}
• local prefix
• case "${where}" in

• fg)
  

• 	prefix="\e[38"
  

• 	;;
  

• bg)
  

• 	prefix="\e[48"
  

• 	;;
  

• esac
• # Convert color from hex to RGB
• local red green blue
• read red green blue <<< $(color_hex2rgb ${color})
• # Set standard shell color
• local shell_color=$(color_rgb2shell ${red} ${green} ${blue})
• printf "${prefix};5;${shell_color}m"
• # For shells that support it, we will try to set the RGB

color code

• case "${TERM}" in

• putty*)
  

• 	# PuTTY us a piece of garbage and does not
  

know

• 	# how to handle colors at all although it
  

has nice

• 	# checkboxes to enable them, but they
  

actually make

• 	# things even worse. So no colors for you
  

Windows

• 	# users.
  

• 	;;
  

• *)
  

• 	printf
  

"${prefix};2;${red};${green};${blue}m"

• 	;;
  

• esac

+}

The shell functions should probably be moved to an own functions file (ending with .shell of course), or should be renamed to cli_*, because I think that is where they actually fit in best.

+shell_set_color() {

• local fg=${1}
• local bg=${2}
• local i
• for i in fg bg; do

• # Skip if color is empty
  

• [ -n "${!i}" ] || continue
  

• # Skip for dash
  

• [ "${!i}" = "-" ] && continue
  

• _set_color ${i} ${!i}
  

• done

+}

+shell_reset_color() {

• printf "\e[0m"

+}

Hi,

On Fri, 2017-06-09 at 12:17 +0200, Jonatan Schlag wrote:

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

src/functions/functions.cli | 18 ++++++++++++++++++  1 file changed, 18 insertions(+)

diff --git a/src/functions/functions.cli b/src/functions/functions.cli index a4690b2..cd4fd06 100644 --- a/src/functions/functions.cli +++ b/src/functions/functions.cli @@ -112,6 +112,12 @@ cli_device_headline() {   ;;   esac   cli_print_fmt1 1 "Status" "${status}"

• # Print the color of the device.
• if [[ "${type}" == "zone" ]] || [[ "${type}" == "port" ]];

then

• cli_print_fmt1 1 "Color " "$(cli_color ${type}
  

${device})"

• fi

To compare two string values in shell you actually only use = instead of ==.

I would probably have written this as:

case "${type}" in port|zone) ... ;; esac

I always considered this performing faster.

if enabled long; then   cli_print_fmt1 1 "Address" "$(device_get_address ${device})"   fi @@ -423,3 +429,15 @@ cli_show_man() {     man ${manpage}  }

+cli_color() {

Whitespace again :)

• assert [ $# -eq 2 ]
• local type=${1}
• local name=${2}
• local color=$(color_read ${type} ${name})
• echo "$(shell_set_color - ${color})            ${CLR_RESET}"

+}

Hi,

On Fri, 2017-06-09 at 12:17 +0200, Jonatan Schlag wrote:

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

src/network | 6 ++++++  1 file changed, 6 insertions(+)

diff --git a/src/network b/src/network index e0d57a0..1bccdea 100644 --- a/src/network +++ b/src/network @@ -518,6 +518,9 @@ cli_port() {   edit|create|remove|up|down|status|identify)   port_${action} "${port}" $@   ;;

• 	color)
  

• 		color_cli "port" "${port}" $@
  

• 	;;
  

*)   error "Unrecognized argument: ${action}"   exit ${EXIT_ERROR}

Indentation of ;; is incorrect.

@@ -576,6 +579,9 @@ cli_zone() {   config|disable|down|edit|enable|identify|sta tus|up)   zone_${action} ${zone} $@   ;;

• 	color)
  

• 		color_cli "zone" "${zone}" $@
  

• 		;;
  

*)   error "Unrecognized argument: ${action}"   cli_show_man network-zone

Apart from that this is really elegantly solved ;)

-Michael

Hi,

On Fri, 2017-06-09 at 12:17 +0200, Jonatan Schlag wrote:

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

src/bash-completion/network | 30 ++++++++++++++++++++++++++----  1 file changed, 26 insertions(+), 4 deletions(-)

diff --git a/src/bash-completion/network b/src/bash- completion/network index 4b5e34d..321d0ff 100644 --- a/src/bash-completion/network +++ b/src/bash-completion/network @@ -48,6 +48,17 @@ _network_complete_zones() {   COMPREPLY=( $(compgen -W "$(network raw list-zones)" -- "${cur}") )  }

The function below should be prefixed with _network, because _color is quite a common name and could be used by something else as well.

+_color() {

• local words=( $@ )
• local commands="set reset"
• local cmd="$(_network_find_on_cmdline "${commands}")"
• if [[ -z "${cmd}" ]]; then

• COMPREPLY=( $(compgen -W "${commands}" -- "${cur}")
  

)

• return 0
  

• fi

+}

_network_device() {   local words=( $@ )   @@ -78,7 +89,7 @@ _network_device_subcommand() {   COMPREPLY=( $(compgen -W "${commands}" -- "${cur}") )   return 0   fi

case "${cmd}" in   ussd)   # TODO @@ -247,12 +258,20 @@ _network_port() {  _network_port_subcommand() {   local words=( $@ )

• local commands="create down edit identify remove status up"

• local commands="create down edit identify remove status up

color"

Could you order the commands alphabetically?

local cmd="$(_network_find_on_cmdline "${commands}")"   if [[ -z "${cmd}" ]]; then   COMPREPLY=( $(compgen -W "${commands}" -- "${cur}") )   return 0   fi

• local args="${words[@]:1}"
• case "${cmd}" in

• color)
  

• 	_color ${args}
  

• 	;;
  

• esac

}    _network_route() { @@ -359,13 +378,13 @@ _network_zone_subcommand() {     local words=( $@ )

• local commands="config disable down edit enable identify

port rename status up"

• local commands="config disable down edit enable identify

port rename status up color"

Same as above.

local cmd="$(_network_find_on_cmdline "${commands}")"   if [[ -z "${cmd}" ]]; then   COMPREPLY=( $(compgen -W "${commands}" -- "${cur}") )   return 0   fi

local args="${words[@]:1}"   case "${cmd}" in   config) @@ -374,6 +393,9 @@ _network_zone_subcommand() {   port)   _network_zone_subcommand_port "${zone}" ${args}   ;;

• color)
  

• 	_color ${args}
  

• 	;;
  

esac  }

-Michael