From mboxrd@z Thu Jan 1 00:00:00 1970 From: Adolf Belka 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, 27 Nov 2020 18:48:17 +0100 Message-ID: <9bf32c2e-f594-63b3-13c5-bcc7c774f050@gmail.com> In-Reply-To: <3798922E-914C-4F95-B245-5CC8FAE56518@ipfire.org> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0378288304400020324==" List-Id: --===============0378288304400020324== Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Hi All, On 27/11/2020 11:59, Michael Tremer wrote: > Hello, >=20 >> 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 : >>> >>> =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 mode o= therwise 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 second table. So we d= on'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= 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 tha= t, but it has never been asked for. We set it automatically on some of the cl= oud 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 impossi= ble to display this on very small displays. Why not creating a table for ever= y zone and putting them among each other. >=20 > I think this definitely has some upsides, but it also has some downsides: >=20 > It would be good to have more space for each zone and put all settings for = one zone together. >=20 > 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 li= ttle bit more complicated. People would have to scroll up and down or hit Sav= e and see an error message that tells you that you did something wrong. >=20 > What do we think about this? As a user just of the native or vlan options in the zone table, I find it ver= y good to be able to see the whole table in one go, with all 4 zones. When I look at the current situation, I have all four zones present on my sys= tem and can see them in one go (desktop system). Above it was written that ha= ving 4 zones would make it impossible to display on very small displays but t= hat would be the current status because the demo page did not have any additi= onal columns compared to the current approach, only more rows. It just only s= howed 3 zones instead of 4. The priority number might be harder to read becau= se the size of the numbers is smaller than the rest but if the priority box h= eight is made similar to the other boxes then the font size should also be si= milar. >=20 > It is a bit of extra work - and should be considered a step two after STP h= as been implemented - but I do not think that someone will spend a week on im= plementing this. >=20 >>>>> 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 space 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 = 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=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 s= o there is still plenty of room for improvement. >>>>>> >>>>>> The checkbox switches the variable 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 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 patche= s!) >>>>>>> >>>>>>> If you want me to take this on, it would be very helpful if you could >>>>>>> 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 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 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, 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 >>>>>>>>> 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 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 border what should be available from the webinterface? These a= nd some other questions I have belonging to the webinterface, are some how fu= ndamental, so that I like to discuss these in the telco ... >=20 > The setup is a CLI tool because the web UI is not set up yet, when it is be= ing launched. >=20 > We must have everything that is possible on the web UI and only what is nec= essary on the CLI. >=20 >> 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/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 that we automatically enabled STP on all bridges, b= ut >>>>>>>>> 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 somewhe= re or >>>>>>>>> the zoneconfig.cgi script needs to be extended to allow enabling ST= P. >>>>>>>>>>> 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-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 "${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 >>>>>>>>>>>> >>>>>>>>>>>> --=20 >>>>>>>>>>>> 2.28.0 >>>>>>>>>>>> >>>>>>>>> >>>>> >=20 --===============0378288304400020324==--