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: Tue, 01 Dec 2020 16:27:54 +0000 Message-ID: <277BB02F-8213-487D-B630-EAEAF644BFF7@ipfire.org> In-Reply-To: <94b77ef5-2b24-3042-54de-0a4f579087da@gmail.com> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="===============7545534341190068368==" List-Id: --===============7545534341190068368== Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable 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 t= ables 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: >=20 > Hi Leo, >=20 > 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 easier = to scan if you are making changes. >=20 > At the end of the day, if the two table option was preferred by more people= I could also live with that. >=20 > My 2p worth. >=20 > Regards, >=20 > Adolf. >=20 >=20 > On 28/11/2020 13:06, Leo Hofmann wrote: >> Hi, >> once again, thanks for your feedback! I spent some time and created two mo= re 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 unr= elated options) on the bottom >> 2: One big table, STP options inside NIC selection >> Your thoughts? >> Regards >> Leo >> Am 27.11.2020 um 11:59 schrieb Michael Tremer: >>> Hello, >>>=20 >>>> On 27 Nov 2020, at 08:34, Jonatan Schlag w= rote: >>>>=20 >>>> Hi, >>>>=20 >>>> Sorry for jumping late into this conversation, time is rare in these day= s .... >>>> I will try to bring in my thoughts, but maybe they are not well structur= ed. >>>>=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= otherwise it must be grayed out. >>>>>=20 >>>> Shouldn=E2=80=99t we just not display these fields, if they do not matte= r? >>>>> - >>>>>=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 wrot= e: >>>>>>> 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 tha= t might be added in the future? >>>>>>> If this is a possibility, I think we should add a second 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 together = in one place. On the other hand, this whole page is becoming longer and longe= r 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 t= hat, but it has never been asked for. We set it automatically on some of the = cloud providers, but that is it. >>>>>>=20 >>>>>>> I took up your and Michael's suggestions and created a quick HTML dem= o. >>>>>> Looks good :) >>>>>>=20 >>>> Here I don=E2=80=99t think so, because more zones (4) will make it impos= sible to display this on very small displays. Why not creating a table for ev= ery zone and putting them among each other. >>> 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 fo= r one zone together. >>>=20 >>> The biggest problem that I see is that it is no longer obvious which port= s 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 S= ave and see an error message that tells you that you did something wrong. >>>=20 >>> What do we think about this? >>>=20 >>> 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. >>>=20 >>>>>>> This new table could be placed below the NIC assignment table. What d= o you think? >>>>>> Call the checkboxes =E2=80=9CEnable=E2=80=9D, because that is what the= y do. >>>>>>=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 patche= d 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 testi= ng 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 cod= e. >>>>>>>>> (By the way, I almost forgot, thanks @Michael for reviewing my patc= hes!) >>>>>>>>>=20 >>>>>>>>> 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 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 enab= le 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 whi= ch 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-bridge= s" 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 parameter= s via >>>>>>>>>>> command line. >>>>>>>>>>>=20 >>>>>>>>>>> Why is that? IPFire is a distribution that is supposed to be mana= ged >>>>>>>>>>> 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 whe= re do we draw a border what should be available from the webinterface? These= 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 it is = being launched. >>>=20 >>> We must have everything that is possible on the web UI and only what is n= ecessary 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. >>>>>>>>>>>=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 somew= here 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 >>>>>>> --===============7545534341190068368==--