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, 27 Nov 2020 10:59:05 +0000 Message-ID: <3798922E-914C-4F95-B245-5CC8FAE56518@ipfire.org> In-Reply-To: <8AFEC96C-5D6F-498D-A445-193CBDB623E8@ipfire.org> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="===============9208381782961265006==" List-Id: --===============9208381782961265006== Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Hello, > On 27 Nov 2020, at 08:34, Jonatan Schlag wrot= e: >=20 > Hi, >=20 > 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. >=20 >> Am 26.11.2020 um 15:47 schrieb Daniel Weism=C3=BCller : >>=20 >> =EF=BB=BFHi, >>=20 >> there is one thing that we didn't talked about... >>=20 >> STP and priority must only be activatable if the zone is in bridge mode ot= herwise it must be grayed out. >>=20 > Shouldn=E2=80=99t we just not display these fields, if they do not matter? >>=20 >> - >>=20 >> Daniel >>=20 >>=20 >>=20 >>> Am 25.11.20 um 21:57 schrieb Michael Tremer: >>> Hello, >>>=20 >>>>> On 25 Nov 2020, at 17:00, Leo Hofmann wrote: >>>>=20 >>>> Hi Daniel, >>>>=20 >>>> thank you very much for the draft & the explanation! >>>>=20 >>>> Do you happen to know if there are any other zone-related options that m= ight be added in the future? >>>> If this is a possibility, I think we should add a second table. So we do= n'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 a= nd that simply makes it complicated. > Definitely to complicated. Already right now. >>>=20 >>> 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 clo= ud providers, but that is it. >>>=20 >>>> I took up your and Michael's suggestions and created a quick HTML demo. >>> Looks good :) >>>=20 > Here I don=E2=80=99t think so, because more zones (4) will make it impossib= le to display this on very small displays. Why not creating a table for every= zone and putting them among each other.=20 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 on= e zone together. The biggest problem that I see is that it is no longer obvious which ports ar= e now available and configured to other zones and that makes this part a litt= le 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 something 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 impl= ementing this. >>>> This new table could be placed below the NIC assignment table. What do y= ou think? >>> Call the checkboxes =E2=80=9CEnable=E2=80=9D, because that is what they d= o. >>>=20 >>> 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. >>>=20 >>>> @Michael: I would like to base this new feature on my recently patched z= oneconf.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. >>>=20 >>> Best, >>> -Michael >>>=20 >>>> Regards >>>> Leo >>>>=20 >>>> Am 23.11.2020 um 16:13 schrieb Daniel Weism=C3=BCller: >>>>> Hi Leo, >>>>>=20 >>>>> that pleases me to hear and I gladly accept your offer. ;-) >>>>>=20 >>>>> 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. >>>>>=20 >>>>> 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. >>>>>=20 >>>>> - >>>>>=20 >>>>> Daniel >>>>>=20 >>>>> Am 21.11.20 um 17:39 schrieb Leo Hofmann: >>>>>> Hi Daniel, >>>>>>=20 >>>>>> 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. >>>>>>=20 >>>>>> (By the way, I almost forgot, thanks @Michael for reviewing my patches= !) >>>>>>=20 >>>>>> 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? >>>>>>=20 >>>>>> Regards, >>>>>> Leo >>>>>>=20 >>>>>> Am 20.11.2020 um 19:31 schrieb Daniel Weism=C3=BCller: >>>>>>> OK. ;-) >>>>>>>=20 >>>>>>> The first step will be the introduction of the possibility to enable = STP. >>>>>>>=20 >>>>>>> The next step will be the implementation in the webif. >>>>>>>=20 >>>>>>> I hope I find someone who can do that. >>>>>>>=20 >>>>>>>=20 >>>>>>> - >>>>>>> Daniel >>>>>>>=20 >>>>>>> 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. >>>>>>>>=20 >>>>>>>> Best regards, >>>>>>>> Fred >>>>>>>>=20 >>>>>>>> -----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 >>>>>>>>=20 >>>>>>>> Hi, >>>>>>>>=20 >>>>>>>>> On 20 Nov 2020, at 06:58, Daniel Weism=C3=BCller >>>>>>>> wrote: >>>>>>>>> Hello, >>>>>>>>>=20 >>>>>>>>> In my opinion it is sufficient to be able to set these parameters v= ia >>>>>>>> command line. >>>>>>>>=20 >>>>>>>> Why is that? IPFire is a distribution that is supposed to be managed >>>>>>>> entirely by the web user interface. >=20 > And here I was wondering a lot. A lot of options are only available via com= mand 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 an= d some other questions I have belonging to the webinterface, are some how fun= damental, 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 bein= g launched. We must have everything that is possible on the web UI and only what is neces= sary on the CLI. > Best Jonatan >>>>>>>>=20 >>>>>>>>> It should only be made sure that the settings are persitend and not >>>>>>>> overwritten by a reboot or the webif. >>>>>>>>=20 >>>>>>>> They wont be as they are in /var/ipfire/ethernet/settings. >>>>>>>>=20 >>>>>>>> Best, >>>>>>>> -Michael >>>>>>>>=20 >>>>>>>>> - >>>>>>>>> Daniel >>>>>>>>>=20 >>>>>>>>> Am 19.11.2020 um 15:56 schrieb Michael Tremer: >>>>>>>>>> Hello Daniel, >>>>>>>>>>=20 >>>>>>>>>> This patch looks good to me. >>>>>>>>>>=20 >>>>>>>>>> I had assumed that we automatically enabled STP on all bridges, but >>>>>>>> apparently we do not. >>>>>>>>>> How do we process with this? >>>>>>>>>>=20 >>>>>>>>>> I suppose it is not the most user-friendly way to ask the user to >>>>>>>> edit the configuration file. This either must be documented somewher= e or >>>>>>>> the 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 >>>>>>>>>>=20 >>>>>>>>>>> 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(-) >>>>>>>>>>>=20 >>>>>>>>>>> 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")" >>>>>>>>>>>=20 >>>>>>>>>>> # The name of the virtual bridge >>>>>>>>>>> BRIDGE=3D"$(get_value "${ZONE}_DEV")" >>>>>>>>>>> +STP=3D"$(get_value "${ZONE}_STP")" >>>>>>>>>>>=20 >>>>>>>>>>> case "${MODE}" in >>>>>>>>>>> bridge) >>>>>>>>>>> @@ -89,7 +90,8 @@ case "${MODE}" in >>>>>>>>>>>=20 >>>>>>>>>>> # 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 >>>>>>>>>>> --=20 >>>>>>>>>>> 2.28.0 >>>>>>>>>>>=20 >>>>>>>>=20 >>>> --===============9208381782961265006==--