Re: [PATCH 2/6] ports: Change ports settings file to /etc/network/${port}/settings

[Michael Tremer <michael.tremer@ipfire.org>]

[-- Attachment #1: Type: text/plain, Size: 2918 bytes --]

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(a)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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]