From mboxrd@z Thu Jan 1 00:00:00 1970
From: "Adolf Belka (ipfire)" <adolf.belka@ipfire.org>
To: development@lists.ipfire.org
Subject: Re: [PATCH v2 6/6] zoneconf.cgi: Improve VLAN & STP inputs
Date: Mon, 22 Feb 2021 23:27:03 +0100
Message-ID: <ed253ad7-5f91-109b-eb28-01654c118556@ipfire.org>
In-Reply-To: <8A7EEE6D-A2B8-4703-B072-1DB10C19FAB9@ipfire.org>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="===============5067493173791817681=="
List-Id: <development.lists.ipfire.org>
--===============5067493173791817681==
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
Hi all,
On 22/02/2021 20:02, Michael Tremer wrote:
> Hi,
>
>> On 21 Feb 2021, at 10:38, Leo Hofmann <hofmann(a)leo-andres.de> wrote:
>>
>> 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> w=
rote:
>>>>
>>>> Add default values and mark fields as required.
>>>>
>>>> Signed-off-by: Leo-Andres Hofmann <hofmann(a)leo-andres.de>
>>>> ---
>>>> 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 co=
nnect the wrong VLAN with another one?
>>>
>>> Usability issues like that cannot be prevented entirely, but I was wonder=
ing if this didn=E2=80=99t make it easier to run into that error.
>> Agreed, this could happen if you are careless!
>> However, I hope you only enable VLAN if you are sure which ID you are supp=
osed to use. In that case, having a default value appear makes it obvious whe=
re you need to put your desired ID.
>> But now I'm not sure about this one. I can revert this, of course.
> Well, we do not have many votes, but let=E2=80=99s maybe leave it empty by =
default.
Sorry for late reply. For me, I had no problem figuring out where I should pu=
t the VLAN ID. I am happy with the system as it was but no hard feelings agai=
nst having a default value either.
A decision to stay with empty by default is fine with me.
Regards,
Adolf
> I will merge the patch and edit this change out.
>
>>>> print <<END
>>>> <td class=3D"$highlight">
>>>> @@ -486,7 +489,7 @@ END
>>>> <option value=3D"NATIVE" $access_selected{"NATIVE"}>$Lang::tr{"zonec=
onf access native"}</option>
>>>> <option value=3D"VLAN" $access_selected{"VLAN"} $vlan_disabled>$Lang=
::tr{"zoneconf access vlan"}</option>
>>>> </select>
>>>> - <input type=3D"number" class=3D"vlanid" id=3D"TAG-$uc-$mac" name=3D"=
TAG $uc $mac" min=3D"1" max=3D"4095" value=3D"$zone_vlan_id" $field_disabled>
>>>> + <input type=3D"number" class=3D"vlanid" id=3D"TAG-$uc-$mac" name=3D"=
TAG $uc $mac" min=3D"1" max=3D"4095" value=3D"$zone_vlan_id" required $field_=
disabled>
>>>> </td>
>>>> 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 be=
longs.
>>>
>>> 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)!
> No need for such a small thing.
>
> But thanks anyways :)
>
> -Michael
>
>>> -Michael
>>>
>>>> # form element modifiers
>>>> my $checked =3D "";
>>>> @@ -532,7 +538,7 @@ END
>>>> # priority input box HTML
>>>> my $row_2 =3D <<END
>>>> <td>
>>>> - <input type=3D"number" class=3D"stp-priority" id=3D"STP-PRIORITY-$uc=
" name=3D"STP-PRIORITY-$uc" min=3D"1" max=3D"65535" value=3D"$stp_priority" $=
disabled>
>>>> + <input type=3D"number" class=3D"stp-priority" id=3D"STP-PRIORITY-$uc=
" name=3D"STP-PRIORITY-$uc" min=3D"1" max=3D"65535" value=3D"$stp_priority" r=
equired $disabled>
>>>> </td>
>>>> END
>>>> ;
>>>> --=20
>>>> 2.27.0.windows.1
>>>>
>> Regards, Leo
--===============5067493173791817681==--