From mboxrd@z Thu Jan 1 00:00:00 1970 From: hofmann@leo-andres.de To: development@lists.ipfire.org Subject: Re: [PATCH] Core 152: the script "network-hotplug-bridges" now reads the variable ${ZONE}_STP from /var/ipfire/ethernet/settings so that STP can be turned on and off for each bridge Date: Fri, 11 Dec 2020 15:33:50 +0000 Message-ID: In-Reply-To: MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1576750567914131164==" List-Id: --===============1576750567914131164== Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Hi Michael, thanks for the explanation! Now we can really close the topic. Have a nice weekend Leo 11. Dezember 2020 09:51, "Michael Tremer" schri= eb: > Hi, >=20 >> On 10 Dec 2020, at 18:59, Leo Hofmann wrote: >>=20 >> Hi, >>=20 >> agreed, I'll do option 1 then and separate the STP options a bit clearer. = We can close this long >> topic now :) >>=20 >> @Michael I'm not really sure how to add new lines (e.g. "STP enable") to t= he language files. But >> I'll look into it and get back to you if I can't figure it out. >=20 > You can simply edit langs/en/cgi-bin/en.pl and add new lines. English is al= ways required because > the web UI falls back to English in case there is no string in the user=E2= =80=99s language. As you are a > native German speaker, you can of course add those strings to the German ve= rsion too. On you test > system, copy the *.pl files to /var/ipfire/lang and run =E2=80=9Cupdate-lan= guage-cache=E2=80=9D and your CGI script > will be able to use the new strings. >=20 > Before committing, please run =E2=80=9C./make.sh lang=E2=80=9D in the build= environment. This will sort the > strings, add a note about missing translations to other languages and so on. >=20 > That is all. >=20 > Best, > -Michael >=20 >> Regards >> Leo >>=20 >> Am 10.12.2020 um 14:34 schrieb Michael Tremer: >>> Hello, >>>=20 >>> I would say we can close this then in favour of option one. >>>=20 >>> Leo, do you need to know anything else or do you have everything to start= hacking? >>>=20 >>> Best, >>> -Michael >>=20 >> On 10 Dec 2020, at 12:13, Daniel Weism=C3=BCller wrote: >>=20 >> H Leo, >>=20 >> I vote for "One big table" but with a few change requests. >>=20 >> - please change the order of the nics to red, green, orange, blue >>=20 >> - please change the way you implement the STP-line. I would discard the ST= P-field and change >> "enable" to "enable STP" and maybe the border between the options and the = assignment a bit bigger. >> At the moment you get the impression that you can switch the zone on and o= ff and not only STP. >>=20 >> - >>=20 >> Daniel >>=20 >> Am 10.12.20 um 09:24 schrieb hofmann(a)leo-andres.de: >> Hi Michael, >>=20 >> I can start working on this next week, so I suggest we keep voting until 1= 3.12.! Votes so far: >> - One big table: II >> - Two tables: II >>=20 >> Regards >> Leo >>=20 >> Am 01.12.2020 um 17:27 schrieb Michael Tremer: >> Hello Leo, Thanks for putting all this leg work in. I would as well vote f= or option two: one big >> table. I do not expect that we will add too much more, and splitting this = into two tables pulls two >> things that belong together apart. Until when do we want to keep the votin= g open? Best, -Michael >> On 28 Nov 2020, at 13:24, Adolf Belka wrote: Hi L= eo, I prefer the one big >> table. There are just a couple of extra rows and boxes in each of the zone= headings, so I don't >> think it is overly busy and easier to scan if you are making changes. At t= he end of the day, if the >> two table option was preferred by more people I could also live with that.= My 2p worth. Regards, >> Adolf. On 28/11/2020 13:06, Leo Hofmann wrote: >> Hi, once again, thanks for your feedback! I spent some time and created tw= o more detailed UI >> drafts. I hope that I have incorporated all your ideas: 1: Two tables, zon= e options on the top, NIC >> assign matrix (without any unrelated options) on the bottom 2: One big tab= le, STP options inside >> NIC selection Your thoughts? Regards Leo Am 27.11.2020 um 11:59 schrieb Mi= chael Tremer: >> Hello, >> On 27 Nov 2020, at 08:34, Jonatan Schlag wro= te: Hi, Sorry for jumping >> late into this conversation, time is rare in these days .... I will try to= bring in my thoughts, >> but maybe they are not well structured. >> Am 26.11.2020 um 15:47 schrieb Daniel Weism=C3=BCller : Hi, there is one >> thing that we didn't talked about... STP and priority must only be activat= able if the zone is in >> bridge mode otherwise it must be grayed out. >> Shouldn=E2=80=99t we just not display these fields, if they do not matter? >> - Daniel >> Am 25.11.20 um 21:57 schrieb Michael Tremer: Hello, >> On 25 Nov 2020, at 17:00, Leo Hofmann wrote: >> Hi Daniel, thank you very much for the draft & the explanation! Do you hap= pen to know if there are >> any other zone-related options that might be added in the future? If this = is a possibility, I think >> we should add a second table. So we don't clutter the NIC assignment with = unrelated options. >> Did I mention that it is very nice, when you wrote what I think so I don= =E2=80=99t have to write it again >> =F0=9F=98=89. +1 For a second structure >> Good question. On one hand it is good to have things that go together in o= ne place. On the other >> hand, this whole page is becoming longer and longer and that simply makes = it complicated. >> Definitely to complicated. Already right now. >> The only thing I can think of is MTU. We currently have no UI to set that,= but it has never been >> asked for. We set it automatically on some of the cloud providers, but tha= t is it. >> I took up your and Michael's suggestions and created a quick HTML demo. >> Looks good :) >> Here I don=E2=80=99t think so, because more zones (4) will make it impossi= ble to display this on very small >> displays. Why not creating a table for every zone and putting them among e= ach other. >> I think this definitely has some upsides, but it also has some downsides: = It would be good to have >> more space for each zone and put all settings for one zone together. The b= iggest problem that I see >> is that it is no longer obvious which ports are now available and configur= ed to other zones and >> that makes this part a little bit more complicated. People would have to s= croll up and down or hit >> Save and see an error message that tells you that you did something wrong.= What do we think about >> this? It is a bit of extra work - and should be considered a step two afte= r STP has been >> implemented - but I do not think that someone will spend a week on impleme= nting this. >> This new table could be placed below the NIC assignment table. What do you= think? >> Call the checkboxes =E2=80=9CEnable=E2=80=9D, because that is what they do= . I would also suggest to have the labels >> (e.g. =E2=80=9CPriority=E2=80=9D) on the left so that it is only wasting s= pace once. With plenty of zones the table >> just becomes unnecessarily wide then. >> @Michael: I would like to base this new feature on my recently patched zon= econf.cgi. Is this >> somehow a bad idea? >> Well, good question. I have no idea why I didn=E2=80=99t merge it yet. I d= idn=E2=80=99t realise it was ready. I >> will check if there is enough testing feedback already. Best, -Michael >> Regards Leo Am 23.11.2020 um 16:13 schrieb Daniel Weism=C3=BCller: >> Hi Leo, that pleases me to hear and I gladly accept your offer. ;-) I quic= kly made a draft and >> attached it. As I said it is only a draft so there is still plenty of room= for improvement. The >> checkbox switches the variable named ${ZONE}_STP to 0 or 1. The input fiel= d fills the variable >> named ${ZONE}_STP_PRIORITY. Here must a number between 1 and 65535 inserte= d. - Daniel Am 21.11.20 >> um 17:39 schrieb Leo Hofmann: >> Hi Daniel, a few days ago I finally submitted my patches for zoneconf.cgi = and I would now have time >> to work on this as well. >> Thank you for submitting these patches. It is enjoyable to read good code. >> (By the way, I almost forgot, thanks @Michael for reviewing my patches!) I= f you want me to take >> this on, it would be very helpful if you could summarize how this should w= ork. For example, which >> config parameters need to be modified. Perhaps you could even paint a simp= le GUI mock-up like you >> did last time? Regards, Leo Am 20.11.2020 um 19:31 schrieb Daniel Weism=C3= =BCller: >> OK. ;-) The first step will be the introduction of the possibility to enab= le STP. The next step >> will be the implementation in the webif. I hope I find someone who can do = that. - Daniel Am >> 20.11.2020 um 16:18 schrieb Kienker, Fred: >> I'm with Michael on this one. If it deserves to be in IPFire, it deserves = to be on the web >> interface. Don't created exceptions which are only available from a comman= d line. Best regards, >> Fred -----Original Message----- From: Michael Tremer Sent: Friday, >> November 20, 2020 5:55 AM To: Daniel Weism=C3=BCller Cc: >> development(a)lists.ipfire.org Subject: Re: [PATCH] Core 152: the script "= network-hotplug-bridges" >> now reads the variable ${ZONE}_STP from /var/ipfire/ethernet/settings so t= hat STP can be turned on >> and off for each bridge Hi, >> On 20 Nov 2020, at 06:58, Daniel Weism=C3=BCller >> wrote: >> Hello, In my opinion it is sufficient to be able to set these parameters v= ia >> command line. Why is that? IPFire is a distribution that is supposed to be= managed entirely by the >> web user interface. >> And here I was wondering a lot. A lot of options are only available via co= mmand line. The setup >> command is entirely based on the command line. So where do we draw a borde= r what should be >> available from the webinterface? These and some other questions I have bel= onging to the >> webinterface, are some how fundamental, so that I like to discuss these in= the telco ... >> The setup is a CLI tool because the web UI is not set up yet, when it is b= eing launched. We must >> have everything that is possible on the web UI and only what is necessary = on the CLI. >> Best Jonatan >> It should only be made sure that the settings are persitend and not >> overwritten by a reboot or the webif. They wont be as they are in /var/ipf= ire/ethernet/settings. >> Best, -Michael >> - Daniel Am 19.11.2020 um 15:56 schrieb Michael Tremer: >> Hello Daniel, This patch looks good to me. I had assumed that we automatic= ally enabled STP on all >> bridges, but >> apparently we do not. >> How do we process with this? I suppose it is not the most user-friendly wa= y to ask the user to >> edit the configuration file. This either must be documented somewhere or t= he zoneconfig.cgi script >> needs to be extended to allow enabling STP. >> Does anyone want to be able to change any STP parameters like >> priority or cost of the ports? >> Best, -Michael >> On 19 Nov 2020, at 13:18, Daniel Weism=C3=BCller >> wrote: >> Signed-off-by: Daniel Weism=C3=BCller --- >> config/udev/network-hotplug-bridges | 4 +++- 1 file changed, 3 insertions(= +), 1 deletion(-) diff >> --git a/config/udev/network-hotplug-bridges >> b/config/udev/network-hotplug-bridges >> index 33d6d65ba..7431377bb 100644 --- a/config/udev/network-hotplug-bridge= s +++ >> b/config/udev/network-hotplug-bridges @@ -81,6 +81,7 @@ MODE=3D"$(get_valu= e "${ZONE}_MODE")" # The >> name of the virtual bridge BRIDGE=3D"$(get_value "${ZONE}_DEV")" +STP=3D"$= (get_value "${ZONE}_STP")" >> case "${MODE}" in bridge) @@ -89,7 +90,8 @@ case "${MODE}" in # We need to= create the bridge if it >> doesn't exist, yet if [ ! -d "/sys/class/net/${BRIDGE}" ]; then - ip link = add "${BRIDGE}" address >> "${ADDRESS}" type >> bridge >> + ip link add "${BRIDGE}" address "${ADDRESS}" type >> bridge \ >> + $([ "${STP}" =3D "on" ] && echo "stp_state 1") #ip link set "${BRIDGE}" = up fi -- 2.28.0 >> --===============1576750567914131164==--