fff-vpn-select: improve logic for fastd start/stop #13

adschm · 2020-12-14T13:35:05+01:00

adschm commented

2020-12-14 13:35:05 +01:00

The start/stop part of fff-vpn-select currently uses a subshell
for what is meant to be logic grouping. Address that by swapping
condition levels and add comment to explain what is actually done.

The start/stop part of fff-vpn-select currently uses a subshell for what is meant to be logic grouping. Address that by swapping condition levels and add comment to explain what is actually done.

adschm referenced this pull request

2020-12-14 13:35:44 +01:00

Make vpn-select modular #8

adschm added the

packages/fff

node

labels 2020-12-14 13:37:20 +01:00

adschm added the

bug

label 2020-12-14 14:16:41 +01:00

jkimmel reviewed 2020-12-15 12:59:29 +01:00

src/packages/fff/fff-vpn-select/files/usr/sbin/vpn-select Outdated

						
				@ -54,3 +54,2 @@

						pidfile="/tmp/run/fastd.fff.pid"

						if [ "$(ls /etc/fastd/fff/peers/* 2>/dev/null)" ]; then

							([ -s "$pidfile" ] && [ -d "/proc/$(cat "$pidfile")" ]) || /etc/init.d/fastd start

						if [ -s "$pidfile" ] && [ -d "/proc/$(cat "$pidfile")" ]; then

jkimmel commented

2020-12-15 12:59:29 +01:00

[ -s "$pidfile" ] && [ -d "/proc/$(cat "$pidfile")" ] sollte aequivalent zu [ -s "$pidfile" -a -d "/proc/$(cat "$pidfile")" ] sein und spart einen aufruf an test

`[ -s "$pidfile" ] && [ -d "/proc/$(cat "$pidfile")" ]` sollte aequivalent zu `[ -s "$pidfile" -a -d "/proc/$(cat "$pidfile")" ]` sein und spart einen aufruf an test

adschm commented

2020-12-15 13:04:05 +01:00

shellcheck sagt "Prefer [ p ] && [ q ] as [ p -a q ] is not well defined."

jkimmel marked this conversation as resolved

jkimmel reviewed 2020-12-15 13:04:36 +01:00

src/packages/fff/fff-vpn-select/files/usr/sbin/vpn-select Outdated

						
				@ -56,1 +55,3 @@

							([ -s "$pidfile" ] && [ -d "/proc/$(cat "$pidfile")" ]) || /etc/init.d/fastd start

						if [ -s "$pidfile" ] && [ -d "/proc/$(cat "$pidfile")" ]; then

							# Stop if service is running but no peers present

							[ "$(ls /etc/fastd/fff/peers/* 2>/dev/null)" ] || /etc/init.d/fastd stop

jkimmel commented

2020-12-15 13:04:36 +01:00

Gibt es einen Grund

[ "$(ls /etc/fastd/fff/peers/* 2>/dev/null)" ] || ...

statt einfach direkt

ls /etc/fastd/fff/peers/* 2>/dev/null || ...

zu nutzen?

Gibt es einen Grund `[ "$(ls /etc/fastd/fff/peers/* 2>/dev/null)" ] || ...` statt einfach direkt `ls /etc/fastd/fff/peers/* 2>/dev/null || ...` zu nutzen?

adschm commented

2020-12-15 13:06:56 +01:00

Ich fand das auch ekelig, habe dann gegoogled und tatsächlich wird genau das bestehende als Optimum empfohlen. Bei ls ohne [] muss man ja noch den Output unterdrücken ... Außerdem bin ich mir nicht sicher, wie ls mit return values umgeht, wenn man wildcards verwendet ...

adschm commented

2020-12-15 13:08:52 +01:00

ls /etc/fastd/fff/peers/* &>/dev/null || ...

scheint zu funktionieren, aber ob das jetzt besser ist?

`ls /etc/fastd/fff/peers/* &>/dev/null || ...` scheint zu funktionieren, aber ob das jetzt besser ist?

jkimmel commented

2020-12-15 13:10:32 +01:00

also $(...) wuerde meines wissens eine neue subshell aufmachen. Die wuerden wir uns mit dem Ansatz sparen.

also `$(...)` wuerde meines wissens eine neue subshell aufmachen. Die wuerden wir uns mit dem Ansatz sparen.

rohammer commented

2020-12-19 14:00:10 +01:00

Die Sach ist ganz simpel. ls wirft bei einem leeren Verzeichnis keinen Fehler. Mit * hinten dran bekommen wir den Fehler weil das globbing nicht funzt. D.h. nicht unbedingt, dass das Verzeichnis leer ist. Darum der Test auf "leere" Ausgabe von ls.
In unserem Fall koennte es sogar besser sein, den * wegzulassen.

Die Sach ist ganz simpel. ls wirft bei einem leeren Verzeichnis keinen Fehler. Mit * hinten dran bekommen wir den Fehler weil das globbing nicht funzt. D.h. nicht unbedingt, dass das Verzeichnis leer ist. Darum der Test auf "leere" Ausgabe von ls. In unserem Fall koennte es sogar besser sein, den * wegzulassen.