From mboxrd@z Thu Jan  1 00:00:00 1970
From: Michael Tremer <michael.tremer@ipfire.org>
To: network@lists.ipfire.org
Subject: Re: [PATCH 2/6] ports: Change ports settings file to
 /etc/network/${port}/settings
Date: Wed, 14 Jun 2017 21:55:06 +0100
Message-ID: <1497473706.2416.9.camel@ipfire.org>
In-Reply-To: <1497003452-10190-2-git-send-email-jonatan.schlag@ipfire.org>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="===============8528644210555983925=="
List-Id: <network.lists.ipfire.org>

--===============8528644210555983925==
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 8bit

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

--===============8528644210555983925==
Content-Type: application/pgp-signature
Content-Transfer-Encoding: base64
Content-Disposition: attachment; filename="signature.asc"
MIME-Version: 1.0

LS0tLS1CRUdJTiBQR1AgU0lHTkFUVVJFLS0tLS0KVmVyc2lvbjogR251UEcgdjIKCmlRSWNCQUFC
Q2dBR0JRSlpRYUtxQUFvSkVJQjU4UDl2a0FrSE9xY1AvMk1iWnM2Y0swUEk0UkhBM2NGQUNGNG0K
dUhCRnBOVUVSOW81aUM3NmI5dlkvY3I1bGFXVXc2d3lrZHo1VkVDYytNYTIxcHg0ZitTL3lYL0oy
c3FUQWFoUQpiNTBpVWVpWUN0RVhvbWlGOWM4Umw5RnJIVitYWFVVRVdFcEZZd1FOYmd2TGh1R0Rw
QkloTnIzcWMwNzNXekNKCk96Q0l6eVdVaFA0aGRmTWJpRldSOHozVTFPV3plMjVvQ09pQXZUditv
TXhaSVNQYmVCYXFWeVl4YzdEUnNWYjcKNU12KzF0NHR6WTNMTDRFaGhYWXJ5emV4V0JXOG9LKzhr
WmlaWWdKRTVCN1VDREVlVDFHYWE1eFB4OTJkN3dnVgpEZjEwRnpMdnkwbk9wTFEzVUNsc1dBcnln
WXJNSW5IRDlrMjZUdmUyYkM4anhVeU1aTkpMZ0tjcTEvTURsejN1Ck1DYVFsZTVoMTlNa1ErcDcz
NW1MSnZ1TFJXWC8vaTBhbUt0bFdiTWc4c0tUb2lndTh5SHp6VmpqUHh4bWtxTVQKOE8xMG5HMS9o
NW1tRWs5a1ZQc29ScGJUR1VkVmNjM3E5ZXE4cWhYNDBNNmRuVTd3S2dES3NIcS9UMjJZa2xzRQpa
V3VtL3dGWW8vMTFGc0JGREdtMjluR3FEMUJpRmFhTEtPYmpUbVNuWm5PWkhMZ2ZtSGRZRDBaVnpJ
VkJFYStuCnJEUHNKR0ZYcytxVWNLelRaLzAxd0ZkMkpOcUpGUTFJY2NYc01zalU0c1kyQnkvc2d1
ZHRpaDBhVUIwcVE5ZTkKUDhlcWlOWklnbUJvWHB5NTl4cFI5S0tQbUJyN0pzSlRzaU44ZUs5dmFE
WEIrY2VhczNxRWRRbHk3Z2VQYWo1OApsNzJBa1JwbTlualRHd0pwRVlHcwo9TG1hSAotLS0tLUVO
RCBQR1AgU0lHTkFUVVJFLS0tLS0K

--===============8528644210555983925==--