From mboxrd@z Thu Jan 1 00:00:00 1970
From: Bernhard Bitsch <Bernhard.Bitsch@gmx.de>
To: development@lists.ipfire.org
Subject:
Aw: Re: [PATCH] Fix for Bug #12050: Adding fixed leases with one 'add' click
Date: Thu, 18 Apr 2019 13:23:10 +0200
Message-ID:
<trinity-39d29b8c-1733-4de5-b5e9-b5cbeac10a31-1555586590763@3c-app-gmx-bs26>
In-Reply-To: <960C9950-EE01-4B2F-9AA4-4E888AD90B1B@ipfire.org>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="===============4266628783015904583=="
List-Id: <development.lists.ipfire.org>
--===============4266628783015904583==
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
Hi,
> Gesendet: Donnerstag, 18. April 2019 um 11:50 Uhr
> Von: "Michael Tremer" <michael.tremer(a)ipfire.org>
> An: "Bernhard Bitsch" <Bernhard.Bitsch(a)gmx.de>
> Cc: BeBiMa <bbitsch(a)ipfire.org>, "IPFire: Development-List" <development(=
a)lists.ipfire.org>
> Betreff: Re: [PATCH] Fix for Bug #12050: Adding fixed leases with one 'add'=
click
>
> Hi,
>=20
> > On 17 Apr 2019, at 22:49, Bernhard Bitsch <Bernhard.Bitsch(a)gmx.de> wrot=
e:
> >=20
> > Hi,
> > some explanations from the author:
> >=20
> >> Gesendet: Mittwoch, 17. April 2019 um 11:31 Uhr
> >> Von: "Michael Tremer" <michael.tremer(a)ipfire.org>
> >> An: "Matthias Fischer" <matthias.fischer(a)ipfire.org>
> >> Cc: development(a)lists.ipfire.org, BeBiMa <bbitsch(a)ipfire.org>
> >> Betreff: Re: [PATCH] Fix for Bug #12050: Adding fixed leases with one 'a=
dd' click
> >>=20
> >> Hi,
> >>=20
> >> Thanks Matthias for helping out here. However, I do not get the patch.
> >>=20
> >> There is no explanation about what it is meant to do. The intention alre=
ady is that the lease is added in the first place.
> >>=20
> >=20
> > The intention for the patch is to include new leases at the end with all =
fields defined by the admin.
> > Up to now a new lease was added after an additional edit.
>=20
> Those comments *must* be in the code. Nobody goes through thousands of emai=
ls on a mailing list to find out what is actually intended in the code.
>=20
You're right. But knowing, there should be some more work on this page, this =
wasn't my main effort for this quick and short solution. For this special cas=
e I regarded the forum post and the bugzilla entry to be sufficient documenta=
tion, for the moment.
> >=20
> >>> On 16 Apr 2019, at 17:41, Matthias Fischer <matthias.fischer(a)ipfire.o=
rg> wrote:
> >>>=20
> >>> Signed-off-by: BeBiMa <bbitsch(a)ipfire.org>
> >>> Reviewed-by: Matthias Fischer <matthias.fischer(a)ipfire.org>
> >>> ---
> >>> html/cgi-bin/dhcp.cgi | 12 ++++++++++++
> >>> 1 file changed, 12 insertions(+)
> >>>=20
> >>> diff --git a/html/cgi-bin/dhcp.cgi b/html/cgi-bin/dhcp.cgi
> >>> index 675d80012..ba5b54f84 100644
> >>> --- a/html/cgi-bin/dhcp.cgi
> >>> +++ b/html/cgi-bin/dhcp.cgi
> >>> @@ -412,12 +412,16 @@ if ($dhcpsettings{'ACTION'} eq $Lang::tr{'add'}.'=
2') {
> >>> }
> >>>=20
> >>> my $key =3D 0;
> >>> + my $szc =3D scalar(@current2);
> >>> CHECK:foreach my $line (@current2) {
> >>> my @temp =3D split(/\,/,$line);
> >>> if($dhcpsettings{'KEY2'} ne $key) {
> >>> # same MAC is OK on different subnets. This test is not complete b=
ecause
> >>> # if ip are not inside a known subnet, I don't warn.
> >>> # Also it may be needed to put duplicate fixed lease in their righ=
t subnet definition..
> >>> + if ((lc($dhcpsettings{'FIX_MAC'}) eq lc($temp[0])) &&(lc($dhcpset=
tings{'FIX_ADDR'}) eq lc($temp[1]))) {
> >>> + last CHECK;
> >>> + }
> >>=20
> >> Why is this needed?
> >=20
> > Check for existing lease. If <MAC,IP> is defined already we don't need to=
loop further.
> >=20
> >>=20
> >>> foreach my $itf (@ITFs) {
> >>> my $scoped =3D &General::IpInSubnet($dhcpsettings{'FIX_ADDR'},
> >>> $netsettings{"${itf}_NETADDRESS"},
> >>> @@ -442,11 +446,19 @@ if ($dhcpsettings{'ACTION'} eq $Lang::tr{'add'}.'=
2') {
> >>> $dhcpsettings{'FIX_FILENAME'} =3D &Header::cleanhtml($dhcpsettings{'FI=
X_FILENAME'});
> >>> $dhcpsettings{'FIX_ROOTPATH'} =3D &Header::cleanhtml($dhcpsettings{'FI=
X_ROOTPATH'});
> >>> if ($dhcpsettings{'KEY2'} eq '') { #add or edit ?
> >>=20
> >> This block here is not indented correctly.
> >>=20
> >> I understand that the code is already very messy, but we should not make=
it worse either.
> >>=20
> > How should indention be done? By spaces, tabs? How many colums define an =
indent?
>=20
> In the patch it looks like you wrapped the block into another if condition.=
So it has to be indented to make that obvious.
>=20
Maybe this generated by different editors, I used. Indention of the patch isn=
't worse than the existing indentation. Not being a final version ( see above=
) it is sufficient, in my opinion.
> > The original file (and many ohters!) have an mixture of tab/space. Should=
we patch that step by step ( tab=3D4 ) to increase readability.
>=20
> This is not about tabs or spaces.
>=20
Sure! This mixture doesnt't matter only in case of "tab=3D4" definition, whic=
h I didn't find in the docs (yet).
I'll use this definition for further development ( and formated dhcpi.cgi in =
my work copy in the neighbourhood of the change, yet ).
This means that further patches may contain such "cosmetics", if they are nec=
essary to understand the code working on.
> > No problem. But this will produce a great number of patches. I made these=
modifications local, but generated the diff with "different spacing isn't a =
difference" option.
>=20
> Please just use =E2=80=9Cgit diff=E2=80=9D. It has a variety of settings th=
at are just right.
>=20
I'll use this in future. Because I didn't do the modification in a git repo, =
but in the working system, I didn't realize the possibility of diffing two ar=
bitrary files with "git diff". Sorry.
> > This generated a short diff without real corrections only.
> >=20
> >>> + if($key =3D=3D $szc) { #add
> >>> + @current2[$key] =3D "$dhcpsettings{'FIX_MAC'},$dhcpsettings{'=
FIX_ADDR'},$dhcpsettings{'FIX_ENABLED'},$dhcpsettings{'FIX_NEXTADDR'},$dhcpse=
ttings{'FIX_FILENAME'},$dhcpsettings{'FIX_ROOTPATH'},$dhcpsettings{'FIX_REMAR=
K'}\n";
> >>> + # sort newly added/modified entry
> >>> + &sortcurrent2;
> >>=20
> >> Are you sure we can sort here?
> >>=20
> >> See: https://git.ipfire.org/?p=3Dipfire-2.x.git;a=3Dcommitdiff;h=3D31672=
dc8bdb223ebf425ff96be64318f2d68e0d7
> >>=20
> >=20
> > Yes! Why not?
>=20
> Because of the commit I referred to.
Didn't understand this commit, because of lack of commentation. ;)
>=20
> >=20
> >>> + &General::log($Lang::tr{'fixed ip lease added'});
> >>> + $dhcpsettings{'KEY2'} =3D '';
> >>> + } else { #edit
> >>> unshift (@current2, "$dhcpsettings{'FIX_MAC'},$dhcpsettings{'FIX_A=
DDR'},$dhcpsettings{'FIX_ENABLED'},$dhcpsettings{'FIX_NEXTADDR'},$dhcpsetting=
s{'FIX_FILENAME'},$dhcpsettings{'FIX_ROOTPATH'},$dhcpsettings{'FIX_REMARK'}\n=
");
> >>> &General::log($Lang::tr{'fixed ip lease added'});
> >>>=20
> >>> # Enter edit mode
> >>> $dhcpsettings{'KEY2'} =3D 0;
> >>> + }
> >>> } else {
> >>> @current2[$dhcpsettings{'KEY2'}] =3D "$dhcpsettings{'FIX_MAC'},$dh=
cpsettings{'FIX_ADDR'},$dhcpsettings{'FIX_ENABLED'},$dhcpsettings{'FIX_NEXTAD=
DR'},$dhcpsettings{'FIX_FILENAME'},$dhcpsettings{'FIX_ROOTPATH'},$dhcpsetting=
s{'FIX_REMARK'}\n";
> >>> $dhcpsettings{'KEY2'} =3D ''; # End edit mode
> >>> --
> >>> 2.18.0
> >>>=20
> >>=20
> >> -Michael
> >>=20
> >>=20
> >=20
> > Again, my intention was a quick resolution for the behaviour mentioned in=
the forum post, which initiated my code review of dhcpi.cgi
>=20
> Okay, I will try to explain this one last time - not just only for you, but=
generally:
>=20
> I am not interested in quick and dirty solutions. That is how you break thi=
ngs. I am interested in well-documented, peer-reviewed and tested code. We ar=
e creating some piece of high-quality software - or aim to do so at least - a=
nd there is no space for quick and dirty.
This special modication wasn't intended as quick and dirty, but as quick and =
stable and intuitive. Sorry, the first version was really a 'dirty shot'. But=
I wanted to present this as soon as possible for review and test, which was =
done by Matthias Fischer.=20
I agree fully with your aim, but this means readability also! This means some=
( many? ) 'cosmetic' changes in the future, IMHO. Is this accepted?
>=20
> Please figure out how to set up a local Git repository, how to set up a bra=
nch, how to commit things and what rules there are to follow. Then find out o=
n how to submit a patchset to the mailing list - after it has been tested. It=
is all in the link that I sent you.
>=20
> This is not a competition about who can submit patches the fastest.
>=20
As stated before, the fact I didn't use a git repo has reasons in my personal=
installation at the moment. Therefore your reminder about that isn't relevan=
t for this single case. I also stated, that I'll surely use git for greater m=
odifications I do on this topic ( and others ).
I didn't want to start a competition. It was just one more post in the forum =
about not intuitive behaviour of the DHCP WUI page, resulting in a bugzilla t=
opic by Matthias.=20
> > I found a couple of behaviours of this page, which are not obvious or str=
aight forward ( adding dynamic leases to static leases, for example, maybe t=
his is invoked now by my modification).
>=20
> What are those? Why are we not talking about those first and then come up w=
ith fixes? The whole DHCP page is a mess. I am not sure if it can even be fix=
ed or of things won=E2=80=99t just become worse.
>=20
Being an experienced software developer, I think this is possible and I am ju=
st working on this. I'll discuss these topics in the list before sending patc=
hes, being aware this is a community project this many opinions.
> > I am reviewing this piece of code at the moment and plan to suggest some=
modifications/corrections with formally legal patches. I hope this is ok for=
the core devs.
>=20
> As I said, please follow the process. It is there for a reason. This is not=
to make things difficult. In fact it makes things a lot easier in the long t=
erm.
>=20
Agreed, too. But again, this special problem appears from time to time in the=
forums. It is no problem, to describe the behaviour each time ( define fixed=
lease -> press 'add' -> press 'update' --> definition is stored ), but what =
about a easy solution, which deletes this discussion? Exactly this was the re=
ason for the patch. The amount of modified code isn't so big to demand the ca=
nonical development process, IMHO. The patch can be applied by any core devel=
opper reading DevList/bugzilla/forums regulary.
-Bernhard
> -Michael
>=20
> >=20
> > Regards,
> > Bernhard
>=20
>
--===============4266628783015904583==--