From mboxrd@z Thu Jan 1 00:00:00 1970 From: Michael Tremer 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 09:51:40 +0100 Message-ID: In-Reply-To: <7e4771a3-c1ac-26d9-2a9d-056ae73fb78d@leo-andres.de> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="===============8307970791355704580==" List-Id: --===============8307970791355704580== Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Hi, > 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. W= e can close this long topic now :) >=20 > @Michael I'm not really sure how to add new lines (e.g. "STP enable") to th= e language files. But I'll look into it and get back to you if I can't figure= it out. You can simply edit langs/en/cgi-bin/en.pl and add new lines. English is alwa= ys required because the web UI falls back to English in case there is no stri= ng in the user=E2=80=99s language. As you are a native German speaker, you ca= n of course add those strings to the German version too. On you test system, = copy the *.pl files to /var/ipfire/lang and run =E2=80=9Cupdate-language-cach= e=E2=80=9D and your CGI script will be able to use the new strings. Before committing, please run =E2=80=9C./make.sh lang=E2=80=9D in the build e= nvironment. This will sort the strings, add a note about missing translations= to other languages and so on. That is all. 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 S= TP-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 impressio= n that you can switch the zone on and off and not only STP. >>>=20 >>> - >>>=20 >>> Daniel >>>=20 >>>=20 >>>=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= 13.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 vot= e for option two: one big table. I do not expect that we will add too much mo= re, and splitting this into two tables pulls two things that belong together = apart. Until when do we want to keep the voting open? Best, -Michael >>>>>> On 28 Nov 2020, at 13:24, Adolf Belka wrote: = Hi Leo, 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 e= asier to scan if you are making changes. At the end of the day, if the two ta= ble option was preferred by more people I could also live with that. My 2p wo= rth. Regards, Adolf. On 28/11/2020 13:06, Leo Hofmann wrote: >>>>>>> Hi, once again, thanks for your feedback! I spent some time and creat= ed two more detailed UI drafts. I hope that I have incorporated all your idea= s: 1: Two tables, zone options on the top, NIC assign matrix (without any unr= elated options) on the bottom 2: One big table, STP options inside NIC select= ion Your thoughts? Regards Leo Am 27.11.2020 um 11:59 schrieb Michael Tremer: >>>>>>>> Hello, >>>>>>>>> On 27 Nov 2020, at 08:34, Jonatan Schlag wrote: 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 w= ell structured. >>>>>>>>>> Am 26.11.2020 um 15:47 schrieb Daniel Weism=C3=BCller : =EF=BB=BFHi, there is one thing that we didn't talked = about... STP and priority must only be activatable if the zone is in bridge m= ode 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 happen 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 sec= ond 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 toge= ther in one 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 that is it. >>>>>>>>>>>> I took up your and Michael's suggestions and created a quick HTM= L demo. >>>>>>>>>>> Looks good :) >>>>>>>>> Here I don=E2=80=99t think so, because more zones (4) will make it = impossible to display this on very small displays. Why not creating a table f= or every zone and putting them among each other. >>>>>>>> I think this definitely has some upsides, but it also has some downs= ides: It would be good to have more space for each zone and put all settings = for one zone together. The biggest problem that I see is that it is no longer= obvious which ports are now available and configured to other zones and that= makes this part a little bit more complicated. People would have to scroll u= p and down or hit Save and see an error message that tells you that you did s= omething wrong. What do we think about this? It is a bit of extra work - and = should be considered a step two after STP has been implemented - but I do not= think that someone will spend a week on implementing this. >>>>>>>>>>>> This new table could be placed below the NIC assignment table. W= hat do you think? >>>>>>>>>>> Call the checkboxes =E2=80=9CEnable=E2=80=9D, because that is wha= t 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 space once. With plenty of zon= es the table just becomes unnecessarily wide then. >>>>>>>>>>>> @Michael: I would like to base this new feature on my recently p= atched zoneconf.cgi. Is this somehow a bad idea? >>>>>>>>>>> Well, good question. I have no idea why I didn=E2=80=99t merge it= yet. I didn=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=BClle= r: >>>>>>>>>>>>> Hi Leo, that pleases me to hear and I gladly accept your offer.= ;-) I quickly 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 vari= able named ${ZONE}_STP to 0 or 1. The input field fills the variable named ${= ZONE}_STP_PRIORITY. Here must a number between 1 and 65535 inserted. - Daniel= Am 21.11.20 um 17:39 schrieb Leo Hofmann: >>>>>>>>>>>>>> Hi Daniel, a few days ago I finally submitted my patches for z= oneconf.cgi and I would now have time to work on this as well. >>>>>>>>> Thank you for submitting these patches. It is enjoyable to read goo= d code. >>>>>>>>>>>>>> (By the way, I almost forgot, thanks @Michael for reviewing my= patches!) If you want me to take this on, it would be very helpful if you co= uld summarize how this should work. For example, which config parameters need= to be modified. Perhaps you could even paint a simple GUI mock-up like you d= id last time? Regards, Leo Am 20.11.2020 um 19:31 schrieb Daniel Weism=C3=BCl= ler: >>>>>>>>>>>>>>> OK. ;-) The first step will be the introduction of the possib= ility to enable 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 command line. Best regards, Fred -----Original Message-= ---- From: Michael Tremer Sent: Friday, Novembe= r 20, 2020 5:55 AM To: Daniel Weism=C3=BCller Cc: development(a)lists.ipfire.org Subject: Re: [PATCH] Core 152: the scri= pt "network-hotplug-bridges" now reads the variable ${ZONE}_STP from /var/ipf= ire/ethernet/settings so that 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 the= se parameters via >>>>>>>>>>>>>>>> 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 command line. The setup command is entirely based on the command line. S= o where do we draw a border what should be available from the webinterface? T= hese and some other questions I have belonging 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 i= t is being launched. We must have everything that is possible on the web UI a= nd 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 a= re in /var/ipfire/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 t= hat we automatically 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 way to ask the user to >>>>>>>>>>>>>>>> edit the configuration file. This either must be documented = somewhere or the zoneconfig.cgi script needs to be extended to allow enabling= STP. >>>>>>>>>>>>>>>>>> Does anyone want to be able to change any STP parameters l= ike >>>>>>>>>>>>>>>> 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 change= d, 3 insertions(+), 1 deletion(-) diff --git a/config/udev/network-hotplug-br= idges >>>>>>>>>>>>>>>> b/config/udev/network-hotplug-bridges >>>>>>>>>>>>>>>>>>> index 33d6d65ba..7431377bb 100644 --- a/config/udev/netwo= rk-hotplug-bridges +++ b/config/udev/network-hotplug-bridges @@ -81,6 +81,7 @= @ MODE=3D"$(get_value "${ZONE}_MODE")" # The name of the virtual bridge BRIDG= E=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 >>>>>>>>>>>> >>>>> =20 --===============8307970791355704580==--