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: Sat, 28 Nov 2020 14:24:20 +0100 Message-ID: <94b77ef5-2b24-3042-54de-0a4f579087da@gmail.com> In-Reply-To: MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="===============4340547401109847917==" List-Id: --===============4340547401109847917== Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Hi Leo, I prefer the one big table. There are just a couple of extra rows and boxes i= n each of the zone headings, so I don't think it is overly busy and easier to= scan if you are making changes. At the end of the day, if the two table option was preferred by more people I= could also live with that. My 2p worth. Regards, Adolf. On 28/11/2020 13:06, Leo Hofmann wrote: > Hi, >=20 > once again, thanks for your feedback! I spent some time and created two mor= e detailed UI drafts. I hope that I have incorporated all your ideas: >=20 > 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 selection >=20 > Your thoughts? >=20 > Regards > Leo >=20 > Am 27.11.2020 um 11:59 schrieb Michael Tremer: >> Hello, >> >>> On 27 Nov 2020, at 08:34, Jonatan Schlag wr= ote: >>> >>> 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 structure= d. >>> >>>> 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 = otherwise 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 = 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 i= n 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 th= at, but it has never been asked for. We set it automatically on some of the c= loud 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 imposs= ible to display this on very small displays. Why not creating a table for eve= ry zone and putting them among each other. >> 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= 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 l= ittle bit more complicated. People would have to scroll up and down or hit Sa= ve 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 i= mplementing this. >> >>>>>> 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 testin= g 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 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 patch= es!) >>>>>>>> >>>>>>>> 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 enabl= e 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 whic= h 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 manag= ed >>>>>>>>>> entirely by the web user interface. >>> And here I was wondering a lot. A lot of options are only available via c= ommand line. The setup command is entirely based on the command line. So wher= e do we draw a border what should be available from the webinterface?=C2=A0 T= hese 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 b= eing launched. >> >> We must have everything that is possible on the web UI and only what is ne= cessary on the CLI. >> >>> Best Jonatan >>>>>>>>>>> It should only be made sure that the settings are persitend and n= ot >>>>>>>>>> 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, = but >>>>>>>>>> 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 somewh= ere 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-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 >>>>>>>>>>>>> =C2=A0=C2=A0=C2=A0 bridge) >>>>>>>>>>>>> @@ -89,7 +90,8 @@ case "${MODE}" in >>>>>>>>>>>>> >>>>>>>>>>>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 # We need to create = the bridge if it doesn't exist, yet >>>>>>>>>>>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if [ ! -d "/sys/clas= s/net/${BRIDGE}" ]; then >>>>>>>>>>>>> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0 ip link add "${BRIDGE}" address "${ADDRESS}" type >>>>>>>>>> bridge >>>>>>>>>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0 ip link add "${BRIDGE}" address "${ADDRESS}" type >>>>>>>>>> bridge \ >>>>>>>>>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 $([ "${STP}" =3D "on" ] && echo "stp_state 1") >>>>>>>>>>>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0 #ip link set "${BRIDGE}" up >>>>>>>>>>>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 fi >>>>>>>>>>>>> >>>>>>>>>>>>> --=20 >>>>>>>>>>>>> 2.28.0 >>>>>>>>>>>>> >>>>>> --===============4340547401109847917==--