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

["Kienker, Fred" <fkienker@at4b.com>]

[-- Attachment #1: Type: text/plain, Size: 10834 bytes --]

I would vote for option 1 as well - two separate tables. Option 2 just 
increases visual clutter and increases the time required to find the 
settings. Jonatan is correct, there will be more options added here as 
time goes on. Why start with so much crammed onto the table when it will 
likely have to be split out later anyway?

Best regards, 
Fred

Please note: Although we may sometimes respond to email, text and phone 
calls instantly at all hours of the day, our regular business hours are 
9:00 AM - 6:00 PM ET, Monday thru Friday.

-----Original Message-----
From: Leo Hofmann <hofmann(a)leo-andres.de> 
Sent: Saturday, November 28, 2020 7:06 AM
To: Michael Tremer <michael.tremer(a)ipfire.org>; Jonatan Schlag 
<jonatan.schlag(a)ipfire.org>
Cc: development <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,

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 
unrelated 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,
>
>> On 27 Nov 2020, at 08:34, Jonatan Schlag <jonatan.schlag(a)ipfire.org> 
wrote:
>>
>> 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üller 
<daniel.weismueller(a)ipfire.org>:
>>>
>>> Hi,
>>>
>>> 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’t 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 <hofmann(a)leo-andres.de> 
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’t have to write it again 😉. +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 that, but it has never been asked for. We set it automatically on 
some of the 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’t think so, because more zones (4) will make it 
impossible 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 
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 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 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 implementing this.
>
>>>>> This new table could be placed below the NIC assignment table. 
What do you think?
>>>> Call the checkboxes “Enable”, because that is what they do.
>>>>
>>>> I would also suggest to have the labels (e.g. “Priority”) 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’t merge it yet. I 
didn’t 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üller:
>>>>>> 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 
>>>>>>> patches!)
>>>>>>>
>>>>>>> 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üller:
>>>>>>>> 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 <michael.tremer(a)ipfire.org>
>>>>>>>>> Sent: Friday, November 20, 2020 5:55 AM
>>>>>>>>> To: Daniel Weismüller <daniel.weismueller(a)ipfire.org>
>>>>>>>>> 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üller
>>>>>>>>> <daniel.weismueller(a)ipfire.org> 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 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?  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.
>
> 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 
>>>>>>>>>> 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, 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 
>>>>>>>>> somewhere 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
>>>>>>>>>>>
>>>>>>>>>>>> On 19 Nov 2020, at 13:18, Daniel Weismüller
>>>>>>>>> <daniel.weismueller(a)ipfire.org> wrote:
>>>>>>>>>>>> Signed-off-by: Daniel Weismüller 
>>>>>>>>>>>> <daniel.weismueller(a)ipfire.org>
>>>>>>>>>>>> ---
>>>>>>>>>>>> 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="$(get_value "${ZONE}_MODE")"
>>>>>>>>>>>>
>>>>>>>>>>>> # The name of the virtual bridge BRIDGE="$(get_value 
>>>>>>>>>>>> "${ZONE}_DEV")"
>>>>>>>>>>>> +STP="$(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}" = "on" ] && echo "stp_state 
>>>>>>>>>>>> + 1")
>>>>>>>>>>>>             #ip link set "${BRIDGE}" up
>>>>>>>>>>>>         fi
>>>>>>>>>>>>
>>>>>>>>>>>> --
>>>>>>>>>>>> 2.28.0
>>>>>>>>>>>>
>>>>> <zoneconf-stp.png>