| Message ID | 20190815115203.10340-1-fabian@blaese.de |
|---|---|
| State | Superseded |
| Headers | show |
diff --git a/src/packages/fff/fff-gateway/files/etc/gateway.d/01-version b/src/packages/fff/fff-gateway/files/etc/gateway.d/01-version new file mode 100644 index 0000000..1f0b1b1 --- /dev/null +++ b/src/packages/fff/fff-gateway/files/etc/gateway.d/01-version @@ -0,0 +1,19 @@ +configure() { + local expected_version=1 + local config_version=$(uci -q get gateway.version.config_version) + + # check if gateway config exists + if ! uci -q get gateway; then + echo "ERROR: Gateway config does not exists." + + exit 1 + fi + + # check version of configuration + if [ "$config_version" != "$expected_version" ]; then + echo "ERROR: Invalid config version. Expected '$expected_version', got '$config_version'." + echo "Please check what has been changed and adjust your config appropriately." + + exit 1 + fi +}
Hi Fabian, Reviewed-by: Robert Langhammer <rlanghammer@web.de> Am 15.08.19 um 13:52 schrieb Fabian Bläse: > This adds two checks: > - Does gateway config exist? > - Does gateway config version match? > > Signed-off-by: Fabian Bläse <fabian@blaese.de> > --- > Changes in v2: > - Add exit to gateway config check > - Fix incorrectly escaped apostrophe > --- > .../files/etc/gateway.d/01-version | 19 +++++++++++++++++++ > 1 file changed, 19 insertions(+) > create mode 100644 src/packages/fff/fff-gateway/files/etc/gateway.d/01-version > > diff --git a/src/packages/fff/fff-gateway/files/etc/gateway.d/01-version b/src/packages/fff/fff-gateway/files/etc/gateway.d/01-version > new file mode 100644 > index 0000000..1f0b1b1 > --- /dev/null > +++ b/src/packages/fff/fff-gateway/files/etc/gateway.d/01-version > @@ -0,0 +1,19 @@ > +configure() { > + local expected_version=1 > + local config_version=$(uci -q get gateway.version.config_version) > + > + # check if gateway config exists > + if ! uci -q get gateway; then > + echo "ERROR: Gateway config does not exists." > + > + exit 1 > + fi > + > + # check version of configuration > + if [ "$config_version" != "$expected_version" ]; then > + echo "ERROR: Invalid config version. Expected '$expected_version', got '$config_version'." > + echo "Please check what has been changed and adjust your config appropriately." > + > + exit 1 > + fi > +}
Dieser Patch ist echt verteufelt. On 15.08.19 13:52, Fabian Bläse wrote: > This adds two checks: > - Does gateway config exist? > - Does gateway config version match? > > Signed-off-by: Fabian Bläse <fabian@blaese.de> > --- > Changes in v2: > - Add exit to gateway config check > - Fix incorrectly escaped apostrophe > --- > .../files/etc/gateway.d/01-version | 19 +++++++++++++++++++ > 1 file changed, 19 insertions(+) > create mode 100644 src/packages/fff/fff-gateway/files/etc/gateway.d/01-version > > diff --git a/src/packages/fff/fff-gateway/files/etc/gateway.d/01-version b/src/packages/fff/fff-gateway/files/etc/gateway.d/01-version > new file mode 100644 > index 0000000..1f0b1b1 > --- /dev/null > +++ b/src/packages/fff/fff-gateway/files/etc/gateway.d/01-version > @@ -0,0 +1,19 @@ > +configure() { > + local expected_version=1 > + local config_version=$(uci -q get gateway.version.config_version) Hier muss es gateway.@version[0].config_version heißen. Damit kann dann auch ein Tested-by: Fabian Bläse <fabian@blaese.de> dran. Gruß Fabian
hi Fabian, Am 17.08.19 um 22:36 schrieb Fabian Bläse: > Dieser Patch ist echt verteufelt. > > > +configure() { > + local expected_version=1 > + local config_version=$(uci -q get gateway.version.config_version) > Hier muss es gateway.@version[0].config_version heißen. muss es gar nicht. Kommt darauf an, was in /etc/config/gateway steht. Und da kann man das so reinschreiben wie es hier gebraucht wird. Die Doku muss nur passen. Gruss Robert > > Damit kann dann auch ein > Tested-by: Fabian Bläse <fabian@blaese.de> > dran. > > Gruß > Fabian >
Hallo nochmal, ich finde es besser gateway.version als named section zu definieren und das so zu lassen. Die section sollte es ja auch nur einmal geben. Robert Am 18. August 2019 09:02:42 MESZ schrieb robert <rlanghammer@web.de>: >hi Fabian, > >Am 17.08.19 um 22:36 schrieb Fabian Bläse: >> Dieser Patch ist echt verteufelt. >> >> >> +configure() { >> + local expected_version=1 >> + local config_version=$(uci -q get gateway.version.config_version) >> Hier muss es gateway.@version[0].config_version heißen. > >muss es gar nicht. Kommt darauf an, was in /etc/config/gateway steht. >Und da kann man das so reinschreiben wie es hier gebraucht wird. Die >Doku muss nur passen. > >Gruss Robert > >> >> Damit kann dann auch ein >> Tested-by: Fabian Bläse <fabian@blaese.de> >> dran. >> >> Gruß >> Fabian >>
Hallo, nennt die uci section "meta" und die option "version" oder "config_version". Dann ist das in Zukunft noch erweiterbar, und ein doppeltes version bringt recht wenig. Grüße Adrian > -----Original Message----- > From: franken-dev [mailto:franken-dev-bounces@freifunk.net] On Behalf > Of Robert Langhammer > Sent: Sonntag, 18. August 2019 10:44 > To: franken-dev@freifunk.net > Subject: Re: [PATCH v2] fff-gateway: Add sanity checks > > Hallo nochmal, > ich finde es besser gateway.version als named section zu definieren und das > so zu lassen. Die section sollte es ja auch nur einmal geben. > Robert > > Am 18. August 2019 09:02:42 MESZ schrieb robert <rlanghammer@web.de>: > >hi Fabian, > > > >Am 17.08.19 um 22:36 schrieb Fabian Bläse: > >> Dieser Patch ist echt verteufelt. > >> > >> > >> +configure() { > >> + local expected_version=1 > >> + local config_version=$(uci -q get gateway.version.config_version) > >> Hier muss es gateway.@version[0].config_version heißen. > > > >muss es gar nicht. Kommt darauf an, was in /etc/config/gateway steht. > >Und da kann man das so reinschreiben wie es hier gebraucht wird. Die > >Doku muss nur passen. > > > >Gruss Robert > > > >> > >> Damit kann dann auch ein > >> Tested-by: Fabian Bläse <fabian@blaese.de> dran. > >> > >> Gruß > >> Fabian > >>
Hey Robert, dann müsste die Sektion aber "config version 'version'" heißen.. On 18.08.19 10:44, Robert Langhammer wrote: > Hallo nochmal, > ich finde es besser gateway.version als named section zu definieren und das so zu lassen. Die section sollte es ja auch nur einmal geben. > Robert > > Am 18. August 2019 09:02:42 MESZ schrieb robert <rlanghammer@web.de>: >> hi Fabian, >> >> Am 17.08.19 um 22:36 schrieb Fabian Bläse: >>> Dieser Patch ist echt verteufelt. >>> >>> >>> +configure() { >>> + local expected_version=1 >>> + local config_version=$(uci -q get gateway.version.config_version) >>> Hier muss es gateway.@version[0].config_version heißen. >> >> muss es gar nicht. Kommt darauf an, was in /etc/config/gateway steht. >> Und da kann man das so reinschreiben wie es hier gebraucht wird. Die >> Doku muss nur passen. >> >> Gruss Robert >> >>> >>> Damit kann dann auch ein >>> Tested-by: Fabian Bläse <fabian@blaese.de> >>> dran. >>> >>> Gruß >>> Fabian >>>
Hey Adrian, On 18.08.19 10:47, mail@adrianschmutzler.de wrote: > Hallo, > > nennt die uci section "meta" und die option "version" oder "config_version". > Dann ist das in Zukunft noch erweiterbar, und ein doppeltes version bringt recht wenig. Das gefällt mir aktuell eigentlich am besten. Wir sollten die Option dann auf jeden Fall config_version nennen. Gruß Fabian
Hallo, um das nochmal etwas zu spezifizieren, ich würde folgendes machen: config gateway 'meta' option config_version '1' Ansonsten noch ein Kommentar/Frage: > > + if ! uci -q get gateway; then Mir war gar nicht bewusst, dass das geht (aber ja, habs getestet). Ich hätte da jetzt ! [ -s "/etc/config/gateway" ] gemacht. Da die von mir genannte Variante an diversen Stellen (für andere uci config Files) verwendet wird, würde mich interessieren, was ihr für besser haltet (ist ja auch ein bisschen Speicher vs. Dateisystem). Zudem frage ich mich, ob die Syntax von Fabian absichtlich oder aus Versehen funktioniert, da sie nichts ins std-out schreibt. Wenn Fabian eine v3 schickt, gibt’s ein Review. Grüße Adrian > > + echo "ERROR: Gateway config does not exists." > > + > > + exit 1 > > + fi > > + > > + # check version of configuration > > + if [ "$config_version" != "$expected_version" ]; then > > + echo "ERROR: Invalid config version. Expected '$expected_version', got '$config_version'." > > + echo "Please check what has been changed and adjust your config appropriately." > > + > > + exit 1 > > + fi > > +}
Hey Adrian, On 19.08.19 15:17, Adrian Schmutzler wrote: > Hallo, > > um das nochmal etwas zu spezifizieren, ich würde folgendes machen: > > config gateway 'meta' > option config_version '1' Klingt gut. Ich baue aus diesem Vorschlag demnächst eine v3. > Ansonsten noch ein Kommentar/Frage: > >>> + if ! uci -q get gateway; then > > Mir war gar nicht bewusst, dass das geht (aber ja, habs getestet). Ich hätte da jetzt ! [ -s "/etc/config/gateway" ] gemacht. > > Da die von mir genannte Variante an diversen Stellen (für andere uci config Files) verwendet wird, würde mich interessieren, was ihr für besser haltet (ist ja auch ein bisschen Speicher vs. Dateisystem). Vermutlich ist das "uci get" sinnvoller, weil das direkt überprüft, ob es mit uci funktioniert. > Zudem frage ich mich, ob die Syntax von Fabian absichtlich oder aus Versehen funktioniert, da sie nichts ins std-out schreibt. Ein shell-if wertet nicht den Output, sondern den Exitstatus bzw Rückgabewert eines Befehls aus. In einer Shell kannst du diesen mit "echo $?" abfragen. (Vorsicht: Geht nur einmal. Bei nächsten mal kriegst du dann natürlich den Exitstatus des zuvor ausgeführten echos) Ein einfaches Beispiel: > ~$ true > ~$ echo $? > 0 > ~$ false > ~$ echo $? > 1 > ~$ echo $? > 0 Gruß Fabian
Hallo Fabian, das mit dem if ist mir bewusst, ich habe einfach auf meinem dezentralen Gateway uci get gateway eingegeben, und dann kommt … nichts. Das hat mich gewundert. Exit Status passt: uci get gateway && echo „a“ || echo „b“ gibt „a“ uci get gateway2 && echo „a“ || echo „b“ gibt „b“ Grüße Adrian From: Fabian Bläse [mailto:fabian@blaese.de] Sent: Montag, 19. August 2019 20:20 To: Adrian Schmutzler <mail@adrianschmutzler.de>; franken-dev@freifunk.net Subject: Re: [PATCH v2] fff-gateway: Add sanity checks Hey Adrian, On 19.08.19 15:17, Adrian Schmutzler wrote: > Hallo, > > um das nochmal etwas zu spezifizieren, ich würde folgendes machen: > > config gateway 'meta' > option config_version '1' Klingt gut. Ich baue aus diesem Vorschlag demnächst eine v3. > Ansonsten noch ein Kommentar/Frage: > >>> + if ! uci -q get gateway; then > > Mir war gar nicht bewusst, dass das geht (aber ja, habs getestet). Ich hätte da jetzt ! [ -s "/etc/config/gateway" ] gemacht. > > Da die von mir genannte Variante an diversen Stellen (für andere uci config Files) verwendet wird, würde mich interessieren, was ihr für besser haltet (ist ja auch ein bisschen Speicher vs. Dateisystem). Vermutlich ist das "uci get" sinnvoller, weil das direkt überprüft, ob es mit uci funktioniert. > Zudem frage ich mich, ob die Syntax von Fabian absichtlich oder aus Versehen funktioniert, da sie nichts ins std-out schreibt. Ein shell-if wertet nicht den Output, sondern den Exitstatus bzw Rückgabewert eines Befehls aus. In einer Shell kannst du diesen mit "echo $?" abfragen. (Vorsicht: Geht nur einmal. Bei nächsten mal kriegst du dann natürlich den Exitstatus des zuvor ausgeführten echos) Ein einfaches Beispiel: > ~$ true > ~$ echo $? > 0 > ~$ false > ~$ echo $? > 1 > ~$ echo $? > 0 Gruß Fabian
Hey Adrian, dann hab ich die Frage falsch verstanden oder unaufmerksam gelesen, sorry. Du hast recht, die uci help sagt, dass eine "section" mit angegeben werden soll. In so fern ist es wohl fraglich, ob wir uns auf das Verhalten verlassen können. Man könnte auch "uci show gateway >/dev/null" verwenden.. Gruß Fabian
Hi, ein uci get config prüft nur die Existenz der Datei. Test: root@c2600:/etc/config# > robert root@c2600:/etc/config# uci get robert root@c2600:/etc/config# echo $? 0 root@c2600:/etc/config# rm robert root@c2600:/etc/config# uci get robert uci: Entry not found root@c2600:/etc/config# echo $? 1 Also nicht mal den Inhalt. Man könnte also auch ein [ -f Datei ] nehmen. Da es keinen Unterschied macht, würde ich es so lassen. Grüße Robert Am 20.08.19 um 17:06 schrieb Fabian Bläse: > Hey Adrian, > > dann hab ich die Frage falsch verstanden oder unaufmerksam gelesen, sorry. > > Du hast recht, die uci help sagt, dass eine "section" mit angegeben werden soll. > In so fern ist es wohl fraglich, ob wir uns auf das Verhalten verlassen können. > > Man könnte auch "uci show gateway >/dev/null" verwenden.. > > Gruß > Fabian >
Also ich bin eher dagegen, da das effektiv ein undokumentiertes Feature ist. Ich würde entweder Fabians Lösung mit show > dev/null oder den alten [ -s ] nehmen. Grüße Adrian From: franken-dev [mailto:franken-dev-bounces@freifunk.net] On Behalf Of robert Sent: Dienstag, 20. August 2019 22:59 To: franken-dev@freifunk.net Subject: Re: [PATCH v2] fff-gateway: Add sanity checks Hi, ein uci get config prüft nur die Existenz der Datei. Test: root@c2600:/etc/config# > robert root@c2600:/etc/config# uci get robert root@c2600:/etc/config# echo $? 0 root@c2600:/etc/config# rm robert root@c2600:/etc/config# uci get robert uci: Entry not found root@c2600:/etc/config# echo $? 1 Also nicht mal den Inhalt. Man könnte also auch ein [ -f Datei ] nehmen. Da es keinen Unterschied macht, würde ich es so lassen. Grüße Robert Am 20.08.19 um 17:06 schrieb Fabian Bläse: > Hey Adrian, > > dann hab ich die Frage falsch verstanden oder unaufmerksam gelesen, sorry. > > Du hast recht, die uci help sagt, dass eine "section" mit angegeben werden soll. > In so fern ist es wohl fraglich, ob wir uns auf das Verhalten verlassen können. > > Man könnte auch "uci show gateway >/dev/null" verwenden.. > > Gruß > Fabian >
Mir ist grade folgendes untergekommen, warum die uci get oder uci show Lösung vielleicht dennoch eine gute Idee ist: Sie kann auch Syntaxfehler sinnvoll erkennen. > root@doofi-router:~# configuregateway -c > This script might remove existing vlans, interfaces, addresses, etc. > Do you really want to continue? (y/n) y > > uci: Parse error (EOF with unterminated ') at line 32, byte 620 > ERROR: Gateway config does not exists. > > Error when executing configure from 01-version Die Fehlermeldung sollte man dann entsprechend anpassen: "Gateway config could not be parsed or does not exist." Zudem ist es dann sinnvoll, das -q im passenden uci-call zu entfernen. Beides zusammen kann man eigentlich auch in einem Test abbilden, da der Versionscheck gleichzeitig auch prüft, ob die Datei existiert: - Zuerst uci get auf die Version machen. uci return Wert != 0 -> does not exist or could not be parsed - Dann config version auswerten. Gruß Fabian On 20.08.19 23:04, mail@adrianschmutzler.de wrote: > Also ich bin eher dagegen, da das effektiv ein undokumentiertes Feature ist. > > > > Ich würde entweder Fabians Lösung mit show > dev/null oder den alten [ -s ] nehmen. > > > > Grüße > > > > Adrian > > > > *From:*franken-dev [mailto:franken-dev-bounces@freifunk.net] *On Behalf Of *robert > *Sent:* Dienstag, 20. August 2019 22:59 > *To:* franken-dev@freifunk.net > *Subject:* Re: [PATCH v2] fff-gateway: Add sanity checks > > > > Hi, > > ein uci get config prüft nur die Existenz der Datei. Test: > > root@c2600:/etc/config# > robert > root@c2600:/etc/config# uci get robert > root@c2600:/etc/config# echo $? > 0 > root@c2600:/etc/config# rm robert > root@c2600:/etc/config# uci get robert > uci: Entry not found > root@c2600:/etc/config# echo $? > 1 > > Also nicht mal den Inhalt. Man könnte also auch ein [ -f Datei ] nehmen. > > Da es keinen Unterschied macht, würde ich es so lassen. > > Grüße > Robert > > Am 20.08.19 um 17:06 schrieb Fabian Bläse: >> Hey Adrian, >> >> dann hab ich die Frage falsch verstanden oder unaufmerksam gelesen, sorry. >> >> Du hast recht, die uci help sagt, dass eine "section" mit angegeben werden soll. >> In so fern ist es wohl fraglich, ob wir uns auf das Verhalten verlassen können. >> >> Man könnte auch "uci show gateway >/dev/null" verwenden.. >> >> Gruß >> Fabian >> > > >
Ja, finde ich gut. Aber dann bevorzuge ich „uci show gateway > /dev/null“ Grüße Adrian From: Fabian Bläse [mailto:fabian@blaese.de] Sent: Sonntag, 8. September 2019 12:39 To: mail@adrianschmutzler.de; franken-dev@freifunk.net; 'robert' <rlanghammer@web.de> Subject: Re: [PATCH v2] fff-gateway: Add sanity checks Mir ist grade folgendes untergekommen, warum die uci get oder uci show Lösung vielleicht dennoch eine gute Idee ist: Sie kann auch Syntaxfehler sinnvoll erkennen. > root@doofi-router:~# configuregateway -c > This script might remove existing vlans, interfaces, addresses, etc. > Do you really want to continue? (y/n) y > > uci: Parse error (EOF with unterminated ') at line 32, byte 620 > ERROR: Gateway config does not exists. > > Error when executing configure from 01-version Die Fehlermeldung sollte man dann entsprechend anpassen: "Gateway config could not be parsed or does not exist." Zudem ist es dann sinnvoll, das -q im passenden uci-call zu entfernen. Beides zusammen kann man eigentlich auch in einem Test abbilden, da der Versionscheck gleichzeitig auch prüft, ob die Datei existiert: - Zuerst uci get auf die Version machen. uci return Wert != 0 -> does not exist or could not be parsed - Dann config version auswerten. Gruß Fabian On 20.08.19 23:04, mail@adrianschmutzler.de <mailto:mail@adrianschmutzler.de> wrote: > Also ich bin eher dagegen, da das effektiv ein undokumentiertes Feature ist. > > > > Ich würde entweder Fabians Lösung mit show > dev/null oder den alten [ -s ] nehmen. > > > > Grüße > > > > Adrian > > > > *From:*franken-dev [mailto:franken-dev-bounces@freifunk.net] *On Behalf Of *robert > *Sent:* Dienstag, 20. August 2019 22:59 > *To:* franken-dev@freifunk.net <mailto:franken-dev@freifunk.net> > *Subject:* Re: [PATCH v2] fff-gateway: Add sanity checks > > > > Hi, > > ein uci get config prüft nur die Existenz der Datei. Test: > > root@c2600:/etc/config# > robert > root@c2600:/etc/config# uci get robert > root@c2600:/etc/config# echo $? > 0 > root@c2600:/etc/config# rm robert > root@c2600:/etc/config# uci get robert > uci: Entry not found > root@c2600:/etc/config# echo $? > 1 > > Also nicht mal den Inhalt. Man könnte also auch ein [ -f Datei ] nehmen. > > Da es keinen Unterschied macht, würde ich es so lassen. > > Grüße > Robert > > Am 20.08.19 um 17:06 schrieb Fabian Bläse: >> Hey Adrian, >> >> dann hab ich die Frage falsch verstanden oder unaufmerksam gelesen, sorry. >> >> Du hast recht, die uci help sagt, dass eine "section" mit angegeben werden soll. >> In so fern ist es wohl fraglich, ob wir uns auf das Verhalten verlassen können. >> >> Man könnte auch "uci show gateway >/dev/null" verwenden.. >> >> Gruß >> Fabian >> > > >
This adds two checks: - Does gateway config exist? - Does gateway config version match? Signed-off-by: Fabian Bläse <fabian@blaese.de> --- Changes in v2: - Add exit to gateway config check - Fix incorrectly escaped apostrophe --- .../files/etc/gateway.d/01-version | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) create mode 100644 src/packages/fff/fff-gateway/files/etc/gateway.d/01-version