From mboxrd@z Thu Jan 1 00:00:00 1970 From: Leo Hofmann To: development@lists.ipfire.org Subject: Re: [PATCH v2 6/6] zoneconf.cgi: Improve VLAN & STP inputs Date: Tue, 23 Feb 2021 00:46:39 +0100 Message-ID: <63a905f1-ed4e-2668-3514-f6692395d9fb@leo-andres.de> In-Reply-To: MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="===============3897243515427607016==" List-Id: --===============3897243515427607016== Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Hi Jonatan, thank you for looking into this, I'll answer below! Am 21.02.2021 um 20:06 schrieb Jonatan Schlag: > Hi, > > first thanks for the patchset. Overall it looks quite good > > Am Sonntag, den 21.02.2021, 11:38 +0100 schrieb Leo Hofmann: >> Hello Michael, >> >> thank you for looking into these patches. I'll answer below! >> >> Am 19.02.2021 um 20:22 schrieb Michael Tremer: >>> Hello, >>> >>>> On 18 Feb 2021, at 14:30, Leo-Andres Hofmann < >>>> hofmann(a)leo-andres.de> wrote: >>>> >>>> Add default values and mark fields as required. >>>> >>>> Signed-off-by: Leo-Andres Hofmann >>>> --- >>>> html/cgi-bin/zoneconf.cgi | 10 ++++++++-- >>>> 1 file changed, 8 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/html/cgi-bin/zoneconf.cgi b/html/cgi- >>>> bin/zoneconf.cgi >>>> index 9d01d06ce..bbd042ffc 100644 >>>> --- a/html/cgi-bin/zoneconf.cgi >>>> +++ b/html/cgi-bin/zoneconf.cgi >>>> @@ -478,6 +478,9 @@ END >>>> if ($access_selected{"NONE"} eq "") { >>>> $highlight =3D $_; >>>> } >>>> + =09 >>>> + # default VLAN tag if not configured >>>> + $zone_vlan_id =3D 1 unless >>>> looks_like_number($zone_vlan_id); >>> I am not sure if it is a good idea to add a default here. >>> >>> Isn=E2=80=99t there a danger that people will hit save prematurely and >>> connect the wrong VLAN with another one? > This was also my first thought. >>> Usability issues like that cannot be prevented entirely, but I was >>> wondering if this didn=E2=80=99t make it easier to run into that error. > Thats definitly the case. >> Agreed, this could happen if you are careless! >> However, I hope you only enable VLAN if you are sure which ID you are >> supposed to use. In that case, having a default value appear makes it >> obvious where you need to put your desired ID. >> But now I'm not sure about this one. I can revert this, of course. > I would vote for without default value. There is just absolutey no save > default value here and people could assume that this is something like, > "I am clicking save and I am done". And thats not the case people have > to think about vlan ids. I unterstand that field might be hard to spot, > but that should not be solved by a default value. > > Another question > which came to my mind while reviewing this: Is there any good error > when the vlan id is wrong? Shure it is checked in the html, but i have > not found any backend check ... (Not a problem with your patch, but i > want to mention it.) I just tried this on my test system. There is a range check but no error mess= age. Instead, it reverts the entire assignment, removes the NIC from the zone and = saves the configuration like this. Well, now what... I don't think this is very critical right now because the HTML range check wi= ll usually prevent this. But I think we should have a proper check in the bac= kend at some point. I'll put it on my list. > > Apart from this linter is happy too. > > Nice work. > > Jonat > an > > PS.: Apart from this reading the file took 10 mins, till I found out > that $uc has something todo with zones ... . At least a good learning > how to not name variables :-) $uc is the upper case zone name (RED, ..). This is already used everywhere, s= o I decided to keep it. I can put this on my list too. Best regards Leo > > >>>> print <>>> >>>> @@ -486,7 +489,7 @@ END >>>> >>>> >>>> >>>> - >>> $uc-$mac" name=3D"TAG $uc $mac" min=3D"1" max=3D"4095" >>>> value=3D"$zone_vlan_id" $field_disabled> >>>> + >>> $uc-$mac" name=3D"TAG $uc $mac" min=3D"1" max=3D"4095" >>>> value=3D"$zone_vlan_id" required $field_disabled> >>>> >>>> END >>>> ; >>>> @@ -513,6 +516,9 @@ foreach (@zones) { # load settings and >>>> prepare form elements for each zone >>>> my $stp_available =3D $ethsettings{"${uc}_MODE"} eq "bridge"; # >>>> STP is only available in bridge mode >>>> my $stp_enabled =3D $ethsettings{"${uc}_STP"} eq "on"; >>>> my $stp_priority =3D $ethsettings{"${uc}_STP_PRIORITY"}; >>>> +=09 >>>> + # set priority to default value if no numerical value is >>>> configured >>>> + $stp_priority =3D 32768 unless looks_like_number($stp_priority); >>> This is very good. >> Thanks :) >>> Since this is in this patchset and comes with the dependency to the >>> other code above, I cannot pull this in with the STP patchset where >>> it actually belongs. >>> >>> But I guess we should merge this all together anyways :) >> I still have these commits in my local git. I could just submit these >> last two changes if that makes your job easier (and you don't mind me >> submitting everything multiple times)! >>> -Michael >>> >>>> # form element modifiers >>>> my $checked =3D ""; >>>> @@ -532,7 +538,7 @@ END >>>> # priority input box HTML >>>> my $row_2 =3D <>>> >>>> - >>> id=3D"STP-PRIORITY-$uc" name=3D"STP-PRIORITY-$uc" min=3D"1" max=3D"65535" >>>> value=3D"$stp_priority" $disabled> >>>> + >>> id=3D"STP-PRIORITY-$uc" name=3D"STP-PRIORITY-$uc" min=3D"1" max=3D"65535" >>>> value=3D"$stp_priority" required $disabled> >>>> >>>> END >>>> ; >>>> --=20 >>>> 2.27.0.windows.1 >>>> >> Regards, Leo --===============3897243515427607016==--