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: Thu, 10 Dec 2020 14:34:41 +0100 Message-ID: <2F839E98-89AE-4733-9FAE-2A9A7B3EC0CE@ipfire.org> In-Reply-To: MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="===============4632979089180626107==" List-Id: --===============4632979089180626107== Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable 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 hac= king? Best, -Michael > 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 STP= -field and change "enable" to "enable STP" and maybe the border between the o= ptions 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. >=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 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 = for 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 ap= art. 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 b= oxes in each of the zone headings, so I don't think it is overly busy and eas= ier to scan if you are making changes. At the end of the day, if the two tabl= e option was preferred by more people I could also live with that. My 2p wort= h. Regards, Adolf. On 28/11/2020 13:06, Leo Hofmann wrote: >>>>> Hi, once again, thanks for your feedback! I spent some time and created= 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 unrel= ated options) on the bottom 2: One big table, STP options inside NIC selectio= n 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 t= hese days .... I will try to bring in my thoughts, but maybe they are not wel= l 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 ab= out... STP and priority must only be activatable if the zone is in bridge mod= e otherwise it must be grayed out. >>>>>>> Shouldn=E2=80=99t we just not display these fields, if they do not ma= tter? >>>>>>>> - Daniel >>>>>>>>> Am 25.11.20 um 21:57 schrieb Michael Tremer: Hello, >>>>>>>>>>> On 25 Nov 2020, at 17:00, Leo Hofmann w= rote: >>>>>>>>>> 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 secon= d 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 togeth= er in one place. On the other hand, this whole page is becoming longer and lo= nger 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 se= t that, but it has never been asked for. We set it automatically on some of t= he 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 im= possible to display this on very small displays. Why not creating a table for= every zone and putting them among each other. >>>>>> I think this definitely has some upsides, but it also has some downsid= es: It would be good to have more space for each zone and put all settings fo= r one zone together. The biggest problem that I see is that it is no longer o= bvious which ports are now available and configured to other zones and that m= akes 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 som= ething wrong. What do we think about this? It is a bit of extra work - and sh= ould be considered a step two after STP has been implemented - but I do not t= hink that someone will spend a week on implementing this. >>>>>>>>>> This new table could be placed below the NIC assignment table. Wha= t 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 pat= ched zoneconf.cgi. Is this somehow a bad idea? >>>>>>>>> Well, good question. I have no idea why I didn=E2=80=99t merge it y= et. I didn=E2=80=99t realise it was ready. I will check if there is enough te= sting 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 th= ere is still plenty of room for improvement. The checkbox switches the variab= le named ${ZONE}_STP to 0 or 1. The input field fills the variable named ${ZO= NE}_STP_PRIORITY. Here must a number between 1 and 65535 inserted. - Daniel A= m 21.11.20 um 17:39 schrieb Leo Hofmann: >>>>>>>>>>>> Hi Daniel, a few days ago I finally submitted my patches for zon= econf.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 p= atches!) If you want me to take this on, it would be very helpful if you coul= d summarize how this should work. For example, which config parameters need t= o be modified. Perhaps you could even paint a simple GUI mock-up like you did= last time? Regards, Leo Am 20.11.2020 um 19:31 schrieb Daniel Weism=C3=BClle= r: >>>>>>>>>>>>> OK. ;-) The first step will be the introduction of the possibil= ity to enable STP. The next step will be the implementation in the webif. I h= ope I find someone who can do that. - Daniel Am 20.11.2020 um 16:18 schrieb K= ienker, 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 on= ly 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 script= "network-hotplug-bridges" now reads the variable ${ZONE}_STP from /var/ipfir= e/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 these= parameters via >>>>>>>>>>>>>> command line. Why is that? IPFire is a distribution that is su= pposed to be managed entirely by the web user interface. >>>>>>> And here I was wondering a lot. A lot of options are only available v= ia 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? The= se and some other questions I have belonging to the webinterface, are some ho= w 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 and= only what is necessary on the CLI. >>>>>>> Best Jonatan >>>>>>>>>>>>>>> It should only be made sure that the settings are persitend a= nd not >>>>>>>>>>>>>> overwritten by a reboot or the webif. They wont be as they are= 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 tha= t 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 us= er-friendly way to ask the user to >>>>>>>>>>>>>> edit the configuration file. This either must be documented so= mewhere or the zoneconfig.cgi script needs to be extended to allow enabling S= TP. >>>>>>>>>>>>>>>> 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-brid= ges >>>>>>>>>>>>>> b/config/udev/network-hotplug-bridges >>>>>>>>>>>>>>>>> index 33d6d65ba..7431377bb 100644 --- a/config/udev/network= -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 s= et "${BRIDGE}" up fi -- 2.28.0 >>>>>>>>>> >>> =20 --===============4632979089180626107==--