From mboxrd@z Thu Jan 1 00:00:00 1970 From: Leo Hofmann 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: Thu, 10 Dec 2020 18:59:32 +0100 Message-ID: <7e4771a3-c1ac-26d9-2a9d-056ae73fb78d@leo-andres.de> In-Reply-To: <2F839E98-89AE-4733-9FAE-2A9A7B3EC0CE@ipfire.org> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="===============6567611153388760937==" List-Id: --===============6567611153388760937== Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Hi, agreed, I'll do option 1 then and separate the STP options a bit clearer. We = can close this long topic now :) @Michael I'm not really sure how to add new lines (e.g. "STP enable") to the = language files. But I'll look into it and get back to you if I can't figure i= t out. Regards Leo Am 10.12.2020 um 14:34 schrieb Michael Tremer: > Hello, > > I would say we can close this then in favour of option one. > > Leo, do you need to know anything else or do you have everything to start h= acking? > > Best, > -Michael > >> On 10 Dec 2020, at 12:13, Daniel Weism=C3=BCller wrote: >> >> H Leo, >> >> I vote for "One big table" but with a few change requests. >> >> - please change the order of the nics to red, green, orange, blue >> >> - 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 off and not only STP. >> >> - >> >> Daniel >> >> >> >> Am 10.12.20 um 09:24 schrieb hofmann(a)leo-andres.de: >>> Hi Michael, >>> >>> 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 >>> >>> Regards >>> Leo >>> >>> 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= for option two: one big table. I do not expect that we will add too much mor= e, and splitting this into two tables pulls two things that belong together a= part. Until when do we want to keep the voting open? Best, -Michael >>>>> On 28 Nov 2020, at 13:24, Adolf Belka wrote: H= i 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 ea= sier to scan if you are making changes. At the end of the day, if the two tab= le option was preferred by more people I could also live with that. My 2p wor= th. Regards, Adolf. On 28/11/2020 13:06, Leo Hofmann wrote: >>>>>> Hi, once again, thanks for your feedback! I spent some time and create= d two more detailed UI drafts. I hope that I have incorporated all your ideas= : 1: Two tables, zone options on the top, NIC assign matrix (without any unre= lated options) on the bottom 2: One big table, STP options inside NIC selecti= on 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 we= ll 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 a= bout... STP and priority must only be activatable if the zone is in bridge mo= de otherwise it must be grayed out. >>>>>>>> Shouldn=E2=80=99t we just not display these fields, if they do not m= atter? >>>>>>>>> - 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! D= o you happen to know if there are any other zone-related options that might b= e added in the future? If this is a possibility, I think we should add a seco= nd 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 toget= her in one place. On the other hand, this whole page is becoming longer and l= onger 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 s= et 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 HTML= demo. >>>>>>>>>> Looks good :) >>>>>>>> Here I don=E2=80=99t think so, because more zones (4) will make it i= mpossible to display this on very small displays. Why not creating a table fo= r every zone and putting them among each other. >>>>>>> I think this definitely has some upsides, but it also has some downsi= des: It would be good to have more space for each zone and put all settings f= or 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 up= and down or hit Save and see an error message that tells you that you did so= mething wrong. What do we think about this? It is a bit of extra work - and s= hould 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. Wh= at 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 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 pa= tched 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 t= esting 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 quickly made a draft and attached it. As I said it is only a draft so t= here is still plenty of room for improvement. The checkbox switches the varia= ble named ${ZONE}_STP to 0 or 1. The input field fills the variable named ${Z= ONE}_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 zo= neconf.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!) If you want me to take this on, it would be very helpful if you cou= ld 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 di= d last time? Regards, Leo Am 20.11.2020 um 19:31 schrieb Daniel Weism=C3=BCll= er: >>>>>>>>>>>>>> OK. ;-) The first step will be the introduction of the possibi= lity 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 o= nly available from a command 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 scrip= t "network-hotplug-bridges" now reads the variable ${ZONE}_STP from /var/ipfi= re/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 thes= e parameters via >>>>>>>>>>>>>>> command line. Why is that? IPFire is a distribution that is s= upposed 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. So= where do we draw a border what should be available from the webinterface? Th= ese and some other questions I have belonging to the webinterface, are some h= ow 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 being launched. We must have everything that is possible on the web UI an= d 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 ar= e 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 th= at 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 u= ser-friendly way to ask the user to >>>>>>>>>>>>>>> edit the configuration file. This either must be documented s= omewhere or the zoneconfig.cgi script needs to be extended to allow enabling = STP. >>>>>>>>>>>>>>>>> Does anyone want to be able to change any STP parameters li= ke >>>>>>>>>>>>>>> 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-bri= dges >>>>>>>>>>>>>>> b/config/udev/network-hotplug-bridges >>>>>>>>>>>>>>>>>> index 33d6d65ba..7431377bb 100644 --- a/config/udev/networ= k-hotplug-bridges +++ b/config/udev/network-hotplug-bridges @@ -81,6 +81,7 @@= MODE=3D"$(get_value "${ZONE}_MODE")" # The name of the virtual bridge BRIDGE= =3D"$(get_value "${ZONE}_DEV")" +STP=3D"$(get_value "${ZONE}_STP")" case "${M= ODE}" 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 --===============6567611153388760937==--