I, too, have found the operation of this page somewhat unintuitive. Also, while on the topic, I would like to point out that the current page will convert a dynamic lease to a static address without changing the address. This causes the static addresses to overlap with the dynamic pool, which is not supported and can result in duplicate addresses being assigned by DHCP. You can write the config file in such a way to exclude these addresses from the dynamic pool, but that isn’t happening now (and is a bit convoluted). I did open a bug about this a while back, but it’s not a simple fix, I don’t think.

https://bugzilla.ipfire.org/show_bug.cgi?id=10629

Tom

On Apr 17, 2019, at 5:49 PM, Bernhard Bitsch Bernhard.Bitsch@gmx.de wrote:

Hi, some explanations from the author:

...

Gesendet: Mittwoch, 17. April 2019 um 11:31 Uhr Von: "Michael Tremer" michael.tremer@ipfire.org An: "Matthias Fischer" matthias.fischer@ipfire.org Cc: development@lists.ipfire.org, BeBiMa bbitsch@ipfire.org Betreff: Re: [PATCH] Fix for Bug #12050: Adding fixed leases with one 'add' click

Hi,

Thanks Matthias for helping out here. However, I do not get the patch.

There is no explanation about what it is meant to do. The intention already is that the lease is added in the first place.

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.

...

...

On 16 Apr 2019, at 17:41, Matthias Fischer matthias.fischer@ipfire.org wrote:

Signed-off-by: BeBiMa bbitsch@ipfire.org Reviewed-by: Matthias Fischer matthias.fischer@ipfire.org

---

html/cgi-bin/dhcp.cgi | 12 ++++++++++++ 1 file changed, 12 insertions(+)

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') { }

my $key = 0;

• my $szc = scalar(@current2); CHECK:foreach my $line (@current2) { my @temp = split(/,/,$line); if($dhcpsettings{'KEY2'} ne $key) { # same MAC is OK on different subnets. This test is not complete because # if ip are not inside a known subnet, I don't warn. # Also it may be needed to put duplicate fixed lease in their right subnet definition..

•    if ((lc($dhcpsettings{'FIX_MAC'}) eq lc($temp[0])) &&(lc($dhcpsettings{'FIX_ADDR'}) eq lc($temp[1]))) {
  

•        last CHECK;
  

•   }
  

Why is this needed?

Check for existing lease. If <MAC,IP> is defined already we don't need to loop further.

...

...

   foreach my $itf (@ITFs) {
   my $scoped = &General::IpInSubnet($dhcpsettings{'FIX_ADDR'},
                     $netsettings{"${itf}_NETADDRESS"},

@@ -442,11 +446,19 @@ if ($dhcpsettings{'ACTION'} eq $Lang::tr{'add'}.'2') { $dhcpsettings{'FIX_FILENAME'} = &Header::cleanhtml($dhcpsettings{'FIX_FILENAME'}); $dhcpsettings{'FIX_ROOTPATH'} = &Header::cleanhtml($dhcpsettings{'FIX_ROOTPATH'}); if ($dhcpsettings{'KEY2'} eq '') { #add or edit ?

This block here is not indented correctly.

I understand that the code is already very messy, but we should not make it worse either.

How should indention be done? By spaces, tabs? How many colums define an indent? The original file (and many ohters!) have an mixture of tab/space. Should we patch that step by step ( tab=4 ) to increase readability. 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. This generated a short diff without real corrections only.

...

...

•    if($key == $szc) { #add
  

•        @current2[$key] = "$dhcpsettings{'FIX_MAC'},$dhcpsettings{'FIX_ADDR'},$dhcpsettings{'FIX_ENABLED'},$dhcpsettings{'FIX_NEXTADDR'},$dhcpsettings{'FIX_FILENAME'},$dhcpsettings{'FIX_ROOTPATH'},$dhcpsettings{'FIX_REMARK'}\n";
  

•        # sort newly added/modified entry
  

•        &sortcurrent2;
  

Are you sure we can sort here?

See: https://git.ipfire.org/?p=ipfire-2.x.git;a=commitdiff;h=31672dc8bdb223ebf425...

Yes! Why not?

...

...

•        &General::log($Lang::tr{'fixed ip lease added'});
  

•        $dhcpsettings{'KEY2'} = '';
  

•    } else { #edit
   unshift (@current2, "$dhcpsettings{'FIX_MAC'},$dhcpsettings{'FIX_ADDR'},$dhcpsettings{'FIX_ENABLED'},$dhcpsettings{'FIX_NEXTADDR'},$dhcpsettings{'FIX_FILENAME'},$dhcpsettings{'FIX_ROOTPATH'},$dhcpsettings{'FIX_REMARK'}\n");
   &General::log($Lang::tr{'fixed ip lease added'});
  
   # Enter edit mode
   $dhcpsettings{'KEY2'} = 0;
  

•    }
  

  } else { @current2[$dhcpsettings{'KEY2'}] = "$dhcpsettings{'FIX_MAC'},$dhcpsettings{'FIX_ADDR'},$dhcpsettings{'FIX_ENABLED'},$dhcpsettings{'FIX_NEXTADDR'},$dhcpsettings{'FIX_FILENAME'},$dhcpsettings{'FIX_ROOTPATH'},$dhcpsettings{'FIX_REMARK'}\n"; $dhcpsettings{'KEY2'} = ''; # End edit mode

-- 2.18.0

-Michael

Again, my intention was a quick resolution for the behaviour mentioned in the forum post, which initiated my code review of dhcpi.cgi I found a couple of behaviours of this page, which are not obvious or straight forward ( adding dynamic leases to static leases, for example, maybe this is invoked now by my modification). 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.

Regards, Bernhard

The bug only describes the problem, I have not tried to fix it. Unfortunately, I lack the skills to implement the solution, and it is also a fairly minor problem. An easy workaround is to ensure that any static leases are not overlapping with the dynamic range. Also, this only manifests, IIRC, if the static device is not present on the network (away or powered off) when another device requests an address, and even then perhaps only when there are no other dynamic addresses available?

Tom

On Apr 17, 2019, at 6:15 PM, Bernhard Bitsch Bernhard.Bitsch@gmx.de wrote:

Thanks for mentioning this. I saw this happening some days before, when I redefined some my fixed leases lost during test of my first version of this fix. I'll look at your bug. Did you use the original dhcpi.cgi or the patched one?

Bernhard

Gesendet: Donnerstag, 18. April 2019 um 00:01 Uhr Von: "Tom Rymes" trymes@rymes.com An: "Bernhard Bitsch" Bernhard.Bitsch@gmx.de Cc: BeBiMa bbitsch@ipfire.org, development@lists.ipfire.org Betreff: Re: Aw: Re: [PATCH] Fix for Bug #12050: Adding fixed leases with one 'add' click

I, too, have found the operation of this page somewhat unintuitive. Also, while on the topic, I would like to point out that the current page will convert a dynamic lease to a static address without changing the address. This causes the static addresses to overlap with the dynamic pool, which is not supported and can result in duplicate addresses being assigned by DHCP. You can write the config file in such a way to exclude these addresses from the dynamic pool, but that isn’t happening now (and is a bit convoluted). I did open a bug about this a while back, but it’s not a simple fix, I don’t think.

https://bugzilla.ipfire.org/show_bug.cgi?id=10629

Tom

On Apr 17, 2019, at 5:49 PM, Bernhard Bitsch Bernhard.Bitsch@gmx.de wrote:

Hi, some explanations from the author:

Gesendet: Mittwoch, 17. April 2019 um 11:31 Uhr Von: "Michael Tremer" michael.tremer@ipfire.org An: "Matthias Fischer" matthias.fischer@ipfire.org Cc: development@lists.ipfire.org, BeBiMa bbitsch@ipfire.org Betreff: Re: [PATCH] Fix for Bug #12050: Adding fixed leases with one 'add' click

Hi,

Thanks Matthias for helping out here. However, I do not get the patch.

There is no explanation about what it is meant to do. The intention already is that the lease is added in the first place.

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.

On 16 Apr 2019, at 17:41, Matthias Fischer matthias.fischer@ipfire.org wrote:

Signed-off-by: BeBiMa bbitsch@ipfire.org Reviewed-by: Matthias Fischer matthias.fischer@ipfire.org

---

html/cgi-bin/dhcp.cgi | 12 ++++++++++++ 1 file changed, 12 insertions(+)

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') { }

my $key = 0;

• my $szc = scalar(@current2); CHECK:foreach my $line (@current2) { my @temp = split(/,/,$line); if($dhcpsettings{'KEY2'} ne $key) { # same MAC is OK on different subnets. This test is not complete because # if ip are not inside a known subnet, I don't warn. # Also it may be needed to put duplicate fixed lease in their right subnet definition..

•    if ((lc($dhcpsettings{'FIX_MAC'}) eq lc($temp[0])) &&(lc($dhcpsettings{'FIX_ADDR'}) eq lc($temp[1]))) {
  

•        last CHECK;
  

•   }
  
  

Why is this needed?

Check for existing lease. If <MAC,IP> is defined already we don't need to loop further.

   foreach my $itf (@ITFs) {
   my $scoped = &General::IpInSubnet($dhcpsettings{'FIX_ADDR'},
                     $netsettings{"${itf}_NETADDRESS"},

@@ -442,11 +446,19 @@ if ($dhcpsettings{'ACTION'} eq $Lang::tr{'add'}.'2') { $dhcpsettings{'FIX_FILENAME'} = &Header::cleanhtml($dhcpsettings{'FIX_FILENAME'}); $dhcpsettings{'FIX_ROOTPATH'} = &Header::cleanhtml($dhcpsettings{'FIX_ROOTPATH'}); if ($dhcpsettings{'KEY2'} eq '') { #add or edit ?

This block here is not indented correctly.

I understand that the code is already very messy, but we should not make it worse either.

How should indention be done? By spaces, tabs? How many colums define an indent? The original file (and many ohters!) have an mixture of tab/space. Should we patch that step by step ( tab=4 ) to increase readability. 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. This generated a short diff without real corrections only.

•    if($key == $szc) { #add
  

•        @current2[$key] = "$dhcpsettings{'FIX_MAC'},$dhcpsettings{'FIX_ADDR'},$dhcpsettings{'FIX_ENABLED'},$dhcpsettings{'FIX_NEXTADDR'},$dhcpsettings{'FIX_FILENAME'},$dhcpsettings{'FIX_ROOTPATH'},$dhcpsettings{'FIX_REMARK'}\n";
  

•        # sort newly added/modified entry
  

•        &sortcurrent2;
  
  

Are you sure we can sort here?

See: https://git.ipfire.org/?p=ipfire-2.x.git;a=commitdiff;h=31672dc8bdb223ebf425...

Yes! Why not?

•        &General::log($Lang::tr{'fixed ip lease added'});
  

•        $dhcpsettings{'KEY2'} = '';
  

•    } else { #edit
   unshift (@current2, "$dhcpsettings{'FIX_MAC'},$dhcpsettings{'FIX_ADDR'},$dhcpsettings{'FIX_ENABLED'},$dhcpsettings{'FIX_NEXTADDR'},$dhcpsettings{'FIX_FILENAME'},$dhcpsettings{'FIX_ROOTPATH'},$dhcpsettings{'FIX_REMARK'}\n");
   &General::log($Lang::tr{'fixed ip lease added'});
  
   # Enter edit mode
   $dhcpsettings{'KEY2'} = 0;
  

•    }
  

  } else { @current2[$dhcpsettings{'KEY2'}] = "$dhcpsettings{'FIX_MAC'},$dhcpsettings{'FIX_ADDR'},$dhcpsettings{'FIX_ENABLED'},$dhcpsettings{'FIX_NEXTADDR'},$dhcpsettings{'FIX_FILENAME'},$dhcpsettings{'FIX_ROOTPATH'},$dhcpsettings{'FIX_REMARK'}\n"; $dhcpsettings{'KEY2'} = ''; # End edit mode

-- 2.18.0

-Michael

Again, my intention was a quick resolution for the behaviour mentioned in the forum post, which initiated my code review of dhcpi.cgi I found a couple of behaviours of this page, which are not obvious or straight forward ( adding dynamic leases to static leases, for example, maybe this is invoked now by my modification). 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.

Regards, Bernhard

Bernhard,

While it would be possible to force the system to assign a new address to a device when the user converts a dynamic lease to a static one, that would be quite unusual, and contrary to what most users expect. Without a doubt, it would be a pain, as the user would then have to point all traffic to that device at a different IP address from that point forward.

IMHO, either the file needs to be written in such a way that the dynamic range excludes the statically defined addresses (see the bug description for how this is done), or the user needs to be warned that the configuration is unsupported and should be modified.

In the end, this is not an urgent matter, as it is a bit of a corner case. Let's take the time and get it right the first time.

Tom

On 04/18/2019 4:07 AM, Bernhard Bitsch wrote:

You are right. I tried to describe the problem shortly in a comment to bug #10629. Our WUI just takes the MAC ( and IP ) of the dynamic lease and generates a fixed lease, which should be supplied with an IP from "fixed pool". Thus the next  DHCP request from the client receives this fixed IP. The obsolete dynamic IP is reused for another MAC not before all other dynamic IPs are used. My personal setup is as follows: ( I think this not too unusual )

• all leases are fixed; allows clear rules using the IP of the device
• for easy addition of new devices / "guest access" on blue there is

little dynamic range; it is possible to do the definition of fixed lease "a bit" after first access of the device. I didn't look at the lease process very deeply yet. Therefore I don't know whether it is possible to "undefine" the old <MAC,IP> pair in the dynamic pool. This would be the straight forward way to avoid these double definitions ( only one is active anyway! ). The file constituting the WUI page has "grown" over time and is somehow messy. Therefore the review will take some time for a adequate development. Bernhard *Gesendet:* Donnerstag, 18. April 2019 um 00:56 Uhr *Von:* "Tom Rymes" trymes@rymes.com *An:* "Bernhard Bitsch" Bernhard.Bitsch@gmx.de *Cc:* development@lists.ipfire.org *Betreff:* Re: Aw: Re: Re: [PATCH] Fix for Bug #12050: Adding fixed leases with one 'add' click The bug only describes the problem, I have not tried to fix it. Unfortunately, I lack the skills to implement the solution, and it is also a fairly minor problem. An easy workaround is to ensure that any static leases are not overlapping with the dynamic range. Also, this only manifests, IIRC, if the static device is not present on the network (away or powered off) when another device requests an address, and even then perhaps only when there are no other dynamic addresses available? Tom

On Apr 17, 2019, at 6:15 PM, Bernhard Bitsch <Bernhard.Bitsch@gmx.de mailto:Bernhard.Bitsch@gmx.de> wrote:

Thanks for mentioning this.
I saw this happening some days before, when I redefined some my
fixed leases lost during test of my first version of this fix.
I'll look at your bug. Did you use the original dhcpi.cgi or the
patched one?
Bernhard
*Gesendet:* Donnerstag, 18. April 2019 um 00:01 Uhr
*Von:* "Tom Rymes" <trymes@rymes.com <mailto:trymes@rymes.com>>
*An:* "Bernhard Bitsch" <Bernhard.Bitsch@gmx.de
<mailto:Bernhard.Bitsch@gmx.de>>
*Cc:* BeBiMa <bbitsch@ipfire.org <mailto:bbitsch@ipfire.org>>,
development@lists.ipfire.org <mailto:development@lists.ipfire.org>
*Betreff:* Re: Aw: Re: [PATCH] Fix for Bug #12050: Adding fixed
leases with one 'add' click
I, too, have found the operation of this page somewhat unintuitive.
Also, while on the topic, I would like to point out that the current
page will convert a dynamic lease to a static address without
changing the address. This causes the static addresses to overlap
with the dynamic pool, which is not supported and can result in
duplicate addresses being assigned by DHCP. You can write the config
file in such a way to exclude these addresses from the dynamic pool,
but that isn’t happening now (and is a bit convoluted). I did open a
bug about this a while back, but it’s not a simple fix, I don’t think.
https://bugzilla.ipfire.org/show_bug.cgi?id=10629
Tom

On Apr 17, 2019, at 5:49 PM, Bernhard Bitsch <Bernhard.Bitsch@gmx.de
<mailto:Bernhard.Bitsch@gmx.de>> wrote:

    Hi,
    some explanations from the author:

        Gesendet: Mittwoch, 17. April 2019 um 11:31 Uhr

        Von: "Michael Tremer" <michael.tremer@ipfire.org
        <mailto:michael.tremer@ipfire.org>>

        An: "Matthias Fischer" <matthias.fischer@ipfire.org
        <mailto:matthias.fischer@ipfire.org>>

        Cc: development@lists.ipfire.org
        <mailto:development@lists.ipfire.org>, BeBiMa
        <bbitsch@ipfire.org <mailto:bbitsch@ipfire.org>>

        Betreff: Re: [PATCH] Fix for Bug #12050: Adding fixed leases
        with one 'add' click

        Hi,

        Thanks Matthias for helping out here. However, I do not get
        the patch.

        There is no explanation about what it is meant to do. The
        intention already is that the lease is added in the first place.


    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.

            On 16 Apr 2019, at 17:41, Matthias Fischer
            <matthias.fischer@ipfire.org
            <mailto:matthias.fischer@ipfire.org>> wrote:

            Signed-off-by: BeBiMa <bbitsch@ipfire.org
            <mailto:bbitsch@ipfire.org>>

            Reviewed-by: Matthias Fischer
            <matthias.fischer@ipfire.org
            <mailto:matthias.fischer@ipfire.org>>

            ---

            html/cgi-bin/dhcp.cgi | 12 ++++++++++++

            1 file changed, 12 insertions(+)

            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') {

                }

                my $key = 0;

            +    my $szc =  scalar(@current2);

                CHECK:foreach my $line (@current2) {

                    my @temp = split(/\,/,$line);

                    if($dhcpsettings{'KEY2'} ne $key) {

                    # same MAC is OK on different subnets. This test
            is not complete because

                    # if ip are not inside a known subnet, I don't warn.

                    # Also it may be needed to put duplicate fixed
            lease in their right subnet definition..

            +        if ((lc($dhcpsettings{'FIX_MAC'}) eq
            lc($temp[0])) &&(lc($dhcpsettings{'FIX_ADDR'}) eq
            lc($temp[1]))) {

            +            last CHECK;

            +       }

        Why is this needed?


    Check for existing lease. If <MAC,IP> is defined already we
    don't need to loop further.

                    foreach my $itf (@ITFs) {

                    my $scoped =
            &General::IpInSubnet($dhcpsettings{'FIX_ADDR'},

                                      $netsettings{"${itf}_NETADDRESS"},

            @@ -442,11 +446,19 @@ if ($dhcpsettings{'ACTION'} eq
            $Lang::tr{'add'}.'2') {

                $dhcpsettings{'FIX_FILENAME'} =
            &Header::cleanhtml($dhcpsettings{'FIX_FILENAME'});

                $dhcpsettings{'FIX_ROOTPATH'} =
            &Header::cleanhtml($dhcpsettings{'FIX_ROOTPATH'});

                if ($dhcpsettings{'KEY2'} eq '') { #add or edit ?

        This block here is not indented correctly.

        I understand that the code is already very messy, but we
        should not make it worse either.

    How should indention be done? By spaces, tabs? How many colums
    define an indent?
    The original file (and many ohters!) have an mixture of
    tab/space. Should we patch that step by step ( tab=4 ) to
    increase readability.
    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. This generated a
    short diff without real corrections only.

            +        if($key == $szc) { #add

            +            @current2[$key] =
            "$dhcpsettings{'FIX_MAC'},$dhcpsettings{'FIX_ADDR'},$dhcpsettings{'FIX_ENABLED'},$dhcpsettings{'FIX_NEXTADDR'},$dhcpsettings{'FIX_FILENAME'},$dhcpsettings{'FIX_ROOTPATH'},$dhcpsettings{'FIX_REMARK'}\n";

            +            # sort newly added/modified entry

            +            &sortcurrent2;

        Are you sure we can sort here?

        See:
        https://git.ipfire.org/?p=ipfire-2.x.git;a=commitdiff;h=31672dc8bdb223ebf425ff96be64318f2d68e0d7


    Yes! Why not?

            +            &General::log($Lang::tr{'fixed ip lease
            added'});

            +            $dhcpsettings{'KEY2'} = '';

            +        } else { #edit

                    unshift (@current2,
            "$dhcpsettings{'FIX_MAC'},$dhcpsettings{'FIX_ADDR'},$dhcpsettings{'FIX_ENABLED'},$dhcpsettings{'FIX_NEXTADDR'},$dhcpsettings{'FIX_FILENAME'},$dhcpsettings{'FIX_ROOTPATH'},$dhcpsettings{'FIX_REMARK'}\n");

                    &General::log($Lang::tr{'fixed ip lease added'});

                    # Enter edit mode

                    $dhcpsettings{'KEY2'} = 0;

            +        }

                } else {

                    @current2[$dhcpsettings{'KEY2'}] =
            "$dhcpsettings{'FIX_MAC'},$dhcpsettings{'FIX_ADDR'},$dhcpsettings{'FIX_ENABLED'},$dhcpsettings{'FIX_NEXTADDR'},$dhcpsettings{'FIX_FILENAME'},$dhcpsettings{'FIX_ROOTPATH'},$dhcpsettings{'FIX_REMARK'}\n";

                    $dhcpsettings{'KEY2'} = '';       # End edit mode

            --

            2.18.0

        -Michael


    Again, my intention was a quick resolution for the behaviour
    mentioned in the forum post, which initiated my code review of
    dhcpi.cgi
    I found a couple of behaviours of this page, which are not
    obvious or straight forward ( adding dynamic leases to  static
    leases, for example, maybe this is invoked now by my modification).
    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.

    Regards,
    Bernhard

Hi,

Gesendet: Donnerstag, 18. April 2019 um 11:50 Uhr Von: "Michael Tremer" michael.tremer@ipfire.org An: "Bernhard Bitsch" Bernhard.Bitsch@gmx.de Cc: BeBiMa bbitsch@ipfire.org, "IPFire: Development-List" development@lists.ipfire.org Betreff: Re: [PATCH] Fix for Bug #12050: Adding fixed leases with one 'add' click

Hi,

...

On 17 Apr 2019, at 22:49, Bernhard Bitsch Bernhard.Bitsch@gmx.de wrote:

Hi, some explanations from the author:

...

Gesendet: Mittwoch, 17. April 2019 um 11:31 Uhr Von: "Michael Tremer" michael.tremer@ipfire.org An: "Matthias Fischer" matthias.fischer@ipfire.org Cc: development@lists.ipfire.org, BeBiMa bbitsch@ipfire.org Betreff: Re: [PATCH] Fix for Bug #12050: Adding fixed leases with one 'add' click

Hi,

Thanks Matthias for helping out here. However, I do not get the patch.

There is no explanation about what it is meant to do. The intention already is that the lease is added in the first place.

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.

Those comments *must* be in the code. Nobody goes through thousands of emails on a mailing list to find out what is actually intended in the code.

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 case I regarded the forum post and the bugzilla entry to be sufficient documentation, for the moment.

...

...

...

On 16 Apr 2019, at 17:41, Matthias Fischer matthias.fischer@ipfire.org wrote:

Signed-off-by: BeBiMa bbitsch@ipfire.org Reviewed-by: Matthias Fischer matthias.fischer@ipfire.org

---

html/cgi-bin/dhcp.cgi | 12 ++++++++++++ 1 file changed, 12 insertions(+)

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') { }

my $key = 0;

• my $szc = scalar(@current2); CHECK:foreach my $line (@current2) { my @temp = split(/,/,$line); if($dhcpsettings{'KEY2'} ne $key) { # same MAC is OK on different subnets. This test is not complete because # if ip are not inside a known subnet, I don't warn. # Also it may be needed to put duplicate fixed lease in their right subnet definition..

•    if ((lc($dhcpsettings{'FIX_MAC'}) eq lc($temp[0])) &&(lc($dhcpsettings{'FIX_ADDR'}) eq lc($temp[1]))) {
  

•        last CHECK;
  

•   }
  

Why is this needed?

Check for existing lease. If <MAC,IP> is defined already we don't need to loop further.

...

...

   foreach my $itf (@ITFs) {
my $scoped = &General::IpInSubnet($dhcpsettings{'FIX_ADDR'},
				  $netsettings{"${itf}_NETADDRESS"},

@@ -442,11 +446,19 @@ if ($dhcpsettings{'ACTION'} eq $Lang::tr{'add'}.'2') { $dhcpsettings{'FIX_FILENAME'} = &Header::cleanhtml($dhcpsettings{'FIX_FILENAME'}); $dhcpsettings{'FIX_ROOTPATH'} = &Header::cleanhtml($dhcpsettings{'FIX_ROOTPATH'}); if ($dhcpsettings{'KEY2'} eq '') { #add or edit ?

This block here is not indented correctly.

I understand that the code is already very messy, but we should not make it worse either.

How should indention be done? By spaces, tabs? How many colums define an indent?

In the patch it looks like you wrapped the block into another if condition. So it has to be indented to make that obvious.

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=4 ) to increase readability.

This is not about tabs or spaces.

Sure! This mixture doesnt't matter only in case of "tab=4" definition, which 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 necessary 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.

Please just use “git diff”. It has a variety of settings that are just right.

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 arbitrary files with "git diff". Sorry.

...

This generated a short diff without real corrections only.

...

...

•    if($key == $szc) { #add
  

•        @current2[$key] = "$dhcpsettings{'FIX_MAC'},$dhcpsettings{'FIX_ADDR'},$dhcpsettings{'FIX_ENABLED'},$dhcpsettings{'FIX_NEXTADDR'},$dhcpsettings{'FIX_FILENAME'},$dhcpsettings{'FIX_ROOTPATH'},$dhcpsettings{'FIX_REMARK'}\n";
  

•        # sort newly added/modified entry
  

•        &sortcurrent2;
  

Are you sure we can sort here?

See: https://git.ipfire.org/?p=ipfire-2.x.git;a=commitdiff;h=31672dc8bdb223ebf425...

Yes! Why not?

Because of the commit I referred to.

Didn't understand this commit, because of lack of commentation. ;)

...

...

...

•        &General::log($Lang::tr{'fixed ip lease added'});
  

•        $dhcpsettings{'KEY2'} = '';
  

•    } else { #edit
   unshift (@current2, "$dhcpsettings{'FIX_MAC'},$dhcpsettings{'FIX_ADDR'},$dhcpsettings{'FIX_ENABLED'},$dhcpsettings{'FIX_NEXTADDR'},$dhcpsettings{'FIX_FILENAME'},$dhcpsettings{'FIX_ROOTPATH'},$dhcpsettings{'FIX_REMARK'}\n");
   &General::log($Lang::tr{'fixed ip lease added'});
  
   # Enter edit mode
   $dhcpsettings{'KEY2'} = 0;
  

•    }
  

  } else { @current2[$dhcpsettings{'KEY2'}] = "$dhcpsettings{'FIX_MAC'},$dhcpsettings{'FIX_ADDR'},$dhcpsettings{'FIX_ENABLED'},$dhcpsettings{'FIX_NEXTADDR'},$dhcpsettings{'FIX_FILENAME'},$dhcpsettings{'FIX_ROOTPATH'},$dhcpsettings{'FIX_REMARK'}\n"; $dhcpsettings{'KEY2'} = ''; # End edit mode

-- 2.18.0

-Michael

Again, my intention was a quick resolution for the behaviour mentioned in the forum post, which initiated my code review of dhcpi.cgi

Okay, I will try to explain this one last time - not just only for you, but generally:

I am not interested in quick and dirty solutions. That is how you break things. I am interested in well-documented, peer-reviewed and tested code. We are creating some piece of high-quality software - or aim to do so at least - and 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. I agree fully with your aim, but this means readability also! This means some ( many? ) 'cosmetic' changes in the future, IMHO. Is this accepted?

Please figure out how to set up a local Git repository, how to set up a branch, how to commit things and what rules there are to follow. Then find out on how to submit a patchset to the mailing list - after it has been tested. It is all in the link that I sent you.

This is not a competition about who can submit patches the fastest.

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 relevant for this single case. I also stated, that I'll surely use git for greater modifications 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 topic by Matthias.

...

I found a couple of behaviours of this page, which are not obvious or straight forward ( adding dynamic leases to static leases, for example, maybe this is invoked now by my modification).

What are those? Why are we not talking about those first and then come up with fixes? The whole DHCP page is a mess. I am not sure if it can even be fixed or of things won’t just become worse.

Being an experienced software developer, I think this is possible and I am just working on this. I'll discuss these topics in the list before sending patches, 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.

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 term.

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 reason for the patch. The amount of modified code isn't so big to demand the canonical development process, IMHO. The patch can be applied by any core developper reading DevList/bugzilla/forums regulary.

-Bernhard

-Michael

...

Regards, Bernhard

Hello,

I do not want to start any discussions about the way the project does its work. Therefore some (hopefully) short annotations below.

Gesendet: Donnerstag, 18. April 2019 um 13:42 Uhr Von: "Michael Tremer" michael.tremer@ipfire.org An: "Bernhard Bitsch" Bernhard.Bitsch@gmx.de Cc: "IPFire: Development-List" development@lists.ipfire.org Betreff: Re: [PATCH] Fix for Bug #12050: Adding fixed leases with one 'add' click

Hello,

...

On 18 Apr 2019, at 12:23, Bernhard Bitsch Bernhard.Bitsch@gmx.de wrote:

Hi,

...

Gesendet: Donnerstag, 18. April 2019 um 11:50 Uhr Von: "Michael Tremer" michael.tremer@ipfire.org An: "Bernhard Bitsch" Bernhard.Bitsch@gmx.de Cc: BeBiMa bbitsch@ipfire.org, "IPFire: Development-List" development@lists.ipfire.org Betreff: Re: [PATCH] Fix for Bug #12050: Adding fixed leases with one 'add' click

Hi,

...

On 17 Apr 2019, at 22:49, Bernhard Bitsch Bernhard.Bitsch@gmx.de wrote:

Hi, some explanations from the author:

...

Gesendet: Mittwoch, 17. April 2019 um 11:31 Uhr Von: "Michael Tremer" michael.tremer@ipfire.org An: "Matthias Fischer" matthias.fischer@ipfire.org Cc: development@lists.ipfire.org, BeBiMa bbitsch@ipfire.org Betreff: Re: [PATCH] Fix for Bug #12050: Adding fixed leases with one 'add' click

Hi,

Thanks Matthias for helping out here. However, I do not get the patch.

There is no explanation about what it is meant to do. The intention already is that the lease is added in the first place.

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.

Those comments *must* be in the code. Nobody goes through thousands of emails on a mailing list to find out what is actually intended in the code.

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 case I regarded the forum post and the bugzilla entry to be sufficient documentation, for the moment.

No.

Every commit must contain itself.

...

...

...

...

...

On 16 Apr 2019, at 17:41, Matthias Fischer matthias.fischer@ipfire.org wrote:

Signed-off-by: BeBiMa bbitsch@ipfire.org Reviewed-by: Matthias Fischer matthias.fischer@ipfire.org

---

html/cgi-bin/dhcp.cgi | 12 ++++++++++++ 1 file changed, 12 insertions(+)

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') { }

my $key = 0;

• my $szc = scalar(@current2); CHECK:foreach my $line (@current2) { my @temp = split(/,/,$line); if($dhcpsettings{'KEY2'} ne $key) { # same MAC is OK on different subnets. This test is not complete because # if ip are not inside a known subnet, I don't warn. # Also it may be needed to put duplicate fixed lease in their right subnet definition..

•    if ((lc($dhcpsettings{'FIX_MAC'}) eq lc($temp[0])) &&(lc($dhcpsettings{'FIX_ADDR'}) eq lc($temp[1]))) {
  

•        last CHECK;
  

•   }
  

Why is this needed?

Check for existing lease. If <MAC,IP> is defined already we don't need to loop further.

...

...

   foreach my $itf (@ITFs) {
my $scoped = &General::IpInSubnet($dhcpsettings{'FIX_ADDR'},
				  $netsettings{"${itf}_NETADDRESS"},

@@ -442,11 +446,19 @@ if ($dhcpsettings{'ACTION'} eq $Lang::tr{'add'}.'2') { $dhcpsettings{'FIX_FILENAME'} = &Header::cleanhtml($dhcpsettings{'FIX_FILENAME'}); $dhcpsettings{'FIX_ROOTPATH'} = &Header::cleanhtml($dhcpsettings{'FIX_ROOTPATH'}); if ($dhcpsettings{'KEY2'} eq '') { #add or edit ?

This block here is not indented correctly.

I understand that the code is already very messy, but we should not make it worse either.

How should indention be done? By spaces, tabs? How many colums define an indent?

In the patch it looks like you wrapped the block into another if condition. So it has to be indented to make that obvious.

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.

Do *not* submit non-final versions. There is no point in it. A patch is meant to go into a final release. We do not want beta stuff in there.

Okay. Got it. I should have sent my modified dhcp.cgi because of lack of a working git repo on my site.

...

...

...

The original file (and many ohters!) have an mixture of tab/space. Should we patch that step by step ( tab=4 ) to increase readability.

This is not about tabs or spaces.

Sure! This mixture doesnt't matter only in case of "tab=4" definition, which 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 necessary to understand the code working on.

If you insist to take this conversation down this route, then feel free to do so. We do not have a policy that commands spaces.

I liked you the coding style.

Especially in this file I found a messy mixture of tabs and spaces. Therefore my statement. If we commit us to tab=4 these spaces can be eliminated.

...

...

...

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.

Please just use “git diff”. It has a variety of settings that are just right.

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 arbitrary files with "git diff". Sorry.

Of course you would have the files in your working system. How else would you test?

But for development purposes git is being used. It is the standard. Otherwise patches won’t apply. There is no point in sending patches that other developers cannot apply. Use Git.

Ok. If I don't have a git repo myself, I send the modified source. Each developer with an actual git repo can apply it by commit. Right?

...

...

...

This generated a short diff without real corrections only.

...

...

•    if($key == $szc) { #add
  

•        @current2[$key] = "$dhcpsettings{'FIX_MAC'},$dhcpsettings{'FIX_ADDR'},$dhcpsettings{'FIX_ENABLED'},$dhcpsettings{'FIX_NEXTADDR'},$dhcpsettings{'FIX_FILENAME'},$dhcpsettings{'FIX_ROOTPATH'},$dhcpsettings{'FIX_REMARK'}\n";
  

•        # sort newly added/modified entry
  

•        &sortcurrent2;
  

Are you sure we can sort here?

See: https://git.ipfire.org/?p=ipfire-2.x.git;a=commitdiff;h=31672dc8bdb223ebf425...

Yes! Why not?

Because of the commit I referred to.

Didn't understand this commit, because of lack of commentation. ;)

And you didn’t ask any questions then?

Ok. I ask you now. Why doesn't it function?

...

...

...

...

...

•        &General::log($Lang::tr{'fixed ip lease added'});
  

•        $dhcpsettings{'KEY2'} = '';
  

•    } else { #edit
   unshift (@current2, "$dhcpsettings{'FIX_MAC'},$dhcpsettings{'FIX_ADDR'},$dhcpsettings{'FIX_ENABLED'},$dhcpsettings{'FIX_NEXTADDR'},$dhcpsettings{'FIX_FILENAME'},$dhcpsettings{'FIX_ROOTPATH'},$dhcpsettings{'FIX_REMARK'}\n");
   &General::log($Lang::tr{'fixed ip lease added'});
  
   # Enter edit mode
   $dhcpsettings{'KEY2'} = 0;
  

•    }
  

  } else { @current2[$dhcpsettings{'KEY2'}] = "$dhcpsettings{'FIX_MAC'},$dhcpsettings{'FIX_ADDR'},$dhcpsettings{'FIX_ENABLED'},$dhcpsettings{'FIX_NEXTADDR'},$dhcpsettings{'FIX_FILENAME'},$dhcpsettings{'FIX_ROOTPATH'},$dhcpsettings{'FIX_REMARK'}\n"; $dhcpsettings{'KEY2'} = ''; # End edit mode

-- 2.18.0

-Michael

Again, my intention was a quick resolution for the behaviour mentioned in the forum post, which initiated my code review of dhcpi.cgi

Okay, I will try to explain this one last time - not just only for you, but generally:

I am not interested in quick and dirty solutions. That is how you break things. I am interested in well-documented, peer-reviewed and tested code. We are creating some piece of high-quality software - or aim to do so at least - and 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. I agree fully with your aim, but this means readability also! This means some ( many? ) 'cosmetic' changes in the future, IMHO. Is this accepted?

You cannot submit a patch that does not fulfil the guidelines and then promise to fix it later. There are obvious problems with the patch and there are open questions. Why would we merge something that clearly adds more problems than it solves?

So no, it is not accepted.

Where are the problems? Be more specific please.

...

...

Please figure out how to set up a local Git repository, how to set up a branch, how to commit things and what rules there are to follow. Then find out on how to submit a patchset to the mailing list - after it has been tested. It is all in the link that I sent you.

This is not a competition about who can submit patches the fastest.

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 relevant for this single case. I also stated, that I'll surely use git for greater modifications 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 topic by Matthias.

...

...

I found a couple of behaviours of this page, which are not obvious or straight forward ( adding dynamic leases to static leases, for example, maybe this is invoked now by my modification).

What are those? Why are we not talking about those first and then come up with fixes? The whole DHCP page is a mess. I am not sure if it can even be fixed or of things won’t just become worse.

Being an experienced software developer, I think this is possible and I am just working on this. I'll discuss these topics in the list before sending patches, being aware this is a community project this many opinions.

This is not necessarily about opinions. It is about what we can support in the end and where we all want to invest our time.

Helping users with known problems, which could be resolved, is wating time also. In the moment I do this mainly.

Think about that we are all in the same boat and we want to improve IPFire wherever we can. But we need to talk about things because one set of eyes often is not enough. Something that works for one person does not work for another. We can never break backwards-compatibility.

I personally want to see this static lease bug fixed. I am trying to help you to develop a good solution that we do not have to worry about in the future. I am also enforcing the rules that we all have come up with some long time ago and that work for this project. That way we will hopefully all benefit from this.

Why didn't you just say this. A request for the whole file would have been enough. BTW the file can be found in the forum post.

But I do not want to have endless discussions on this list about why the rules are as they are. If there is a constructive proposal to make things easier for all then we are all of course open for this. On the other hand debating what is the standard now is just a waste of time. I am happy to explain this, but I am not willing to debate them or compromise on this. Again, this is a general statement and not limited to this conversation.

You started this discussion ( once more ).

...

...

...

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.

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 term.

Agreed, too. But again, this special problem appears from time to time in the forums.

Why do we not have a ticket on BZ then?

The ticket is there ( see subject of this discussion ) and the solution exists.

-Bernhard

...

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 reason for the patch. The amount of modified code isn't so big to demand the canonical development process, IMHO. The patch can be applied by any core developper reading DevList/bugzilla/forums regulary.

It is entirely up to you how you develop your code. How it is submitted to the list is clear.

-Michael

...

-Bernhard

...

-Michael

...

Regards, Bernhard

Hi,

On 18 Apr 2019, at 13:54, Bernhard Bitsch Bernhard.Bitsch@gmx.de wrote:

Hello,

I do not want to start any discussions about the way the project does its work. Therefore some (hopefully) short annotations below.

...

Gesendet: Donnerstag, 18. April 2019 um 13:42 Uhr Von: "Michael Tremer" michael.tremer@ipfire.org An: "Bernhard Bitsch" Bernhard.Bitsch@gmx.de Cc: "IPFire: Development-List" development@lists.ipfire.org Betreff: Re: [PATCH] Fix for Bug #12050: Adding fixed leases with one 'add' click

Hello,

...

On 18 Apr 2019, at 12:23, Bernhard Bitsch Bernhard.Bitsch@gmx.de wrote:

Hi,

...

Gesendet: Donnerstag, 18. April 2019 um 11:50 Uhr Von: "Michael Tremer" michael.tremer@ipfire.org An: "Bernhard Bitsch" Bernhard.Bitsch@gmx.de Cc: BeBiMa bbitsch@ipfire.org, "IPFire: Development-List" development@lists.ipfire.org Betreff: Re: [PATCH] Fix for Bug #12050: Adding fixed leases with one 'add' click

Hi,

...

On 17 Apr 2019, at 22:49, Bernhard Bitsch Bernhard.Bitsch@gmx.de wrote:

Hi, some explanations from the author:

...

Gesendet: Mittwoch, 17. April 2019 um 11:31 Uhr Von: "Michael Tremer" michael.tremer@ipfire.org An: "Matthias Fischer" matthias.fischer@ipfire.org Cc: development@lists.ipfire.org, BeBiMa bbitsch@ipfire.org Betreff: Re: [PATCH] Fix for Bug #12050: Adding fixed leases with one 'add' click

Hi,

Thanks Matthias for helping out here. However, I do not get the patch.

There is no explanation about what it is meant to do. The intention already is that the lease is added in the first place.

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.

Those comments *must* be in the code. Nobody goes through thousands of emails on a mailing list to find out what is actually intended in the code.

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 case I regarded the forum post and the bugzilla entry to be sufficient documentation, for the moment.

No.

Every commit must contain itself.

...

...

...

...

> On 16 Apr 2019, at 17:41, Matthias Fischer matthias.fischer@ipfire.org wrote: > > Signed-off-by: BeBiMa bbitsch@ipfire.org > Reviewed-by: Matthias Fischer matthias.fischer@ipfire.org > --- > html/cgi-bin/dhcp.cgi | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > 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') { > } > > my $key = 0; > + my $szc = scalar(@current2); > CHECK:foreach my $line (@current2) { > my @temp = split(/,/,$line); > if($dhcpsettings{'KEY2'} ne $key) { > # same MAC is OK on different subnets. This test is not complete because > # if ip are not inside a known subnet, I don't warn. > # Also it may be needed to put duplicate fixed lease in their right subnet definition.. > + if ((lc($dhcpsettings{'FIX_MAC'}) eq lc($temp[0])) &&(lc($dhcpsettings{'FIX_ADDR'}) eq lc($temp[1]))) { > + last CHECK; > + }

Why is this needed?

Check for existing lease. If <MAC,IP> is defined already we don't need to loop further.

...

> foreach my $itf (@ITFs) { > my $scoped = &General::IpInSubnet($dhcpsettings{'FIX_ADDR'}, > $netsettings{"${itf}_NETADDRESS"}, > @@ -442,11 +446,19 @@ if ($dhcpsettings{'ACTION'} eq $Lang::tr{'add'}.'2') { > $dhcpsettings{'FIX_FILENAME'} = &Header::cleanhtml($dhcpsettings{'FIX_FILENAME'}); > $dhcpsettings{'FIX_ROOTPATH'} = &Header::cleanhtml($dhcpsettings{'FIX_ROOTPATH'}); > if ($dhcpsettings{'KEY2'} eq '') { #add or edit ?

This block here is not indented correctly.

I understand that the code is already very messy, but we should not make it worse either.

How should indention be done? By spaces, tabs? How many colums define an indent?

In the patch it looks like you wrapped the block into another if condition. So it has to be indented to make that obvious.

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.

Do *not* submit non-final versions. There is no point in it. A patch is meant to go into a final release. We do not want beta stuff in there.

Okay. Got it. I should have sent my modified dhcp.cgi because of lack of a working git repo on my site.

Please learn how to use git and use “git send-email”. There is an introduction on the wiki on how to set it up and there is tons of resources on the Internet that explain Git to you in the form of howtos and videos.

...

...

...

...

The original file (and many ohters!) have an mixture of tab/space. Should we patch that step by step ( tab=4 ) to increase readability.

This is not about tabs or spaces.

Sure! This mixture doesnt't matter only in case of "tab=4" definition, which 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 necessary to understand the code working on.

If you insist to take this conversation down this route, then feel free to do so. We do not have a policy that commands spaces.

I liked you the coding style.

Especially in this file I found a messy mixture of tabs and spaces. Therefore my statement. If we commit us to tab=4 these spaces can be eliminated.

We inherited some code from IPCop that we did not clean up.

So the guideline is to use the coding style used in the particular file. Do not re-indent the whole file.

...

...

...

...

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.

Please just use “git diff”. It has a variety of settings that are just right.

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 arbitrary files with "git diff". Sorry.

Of course you would have the files in your working system. How else would you test?

But for development purposes git is being used. It is the standard. Otherwise patches won’t apply. There is no point in sending patches that other developers cannot apply. Use Git.

Ok. If I don't have a git repo myself, I send the modified source. Each developer with an actual git repo can apply it by commit. Right?

Please learn how to use git.

Of course you could send modified files around, but that does not work. Of course you could send instructions to other people on how to change files, but that doesn’t work either.

I personally won’t do any work of trying to apply any patches that are send in other forms. There is also reasons that we send patches inline: We can comment on them.

Please learn how to use git.

...

...

...

...

This generated a short diff without real corrections only.

...

> + if($key == $szc) { #add > + @current2[$key] = "$dhcpsettings{'FIX_MAC'},$dhcpsettings{'FIX_ADDR'},$dhcpsettings{'FIX_ENABLED'},$dhcpsettings{'FIX_NEXTADDR'},$dhcpsettings{'FIX_FILENAME'},$dhcpsettings{'FIX_ROOTPATH'},$dhcpsettings{'FIX_REMARK'}\n"; > + # sort newly added/modified entry > + &sortcurrent2;

Are you sure we can sort here?

See: https://git.ipfire.org/?p=ipfire-2.x.git;a=commitdiff;h=31672dc8bdb223ebf425...

Yes! Why not?

Because of the commit I referred to.

Didn't understand this commit, because of lack of commentation. ;)

And you didn’t ask any questions then?

Ok. I ask you now. Why doesn't it function?

Well, as the patch there states the key is saved and used later. When the leases file is sorted, the key changes but is not updated. That lead to the case that you edited a different lease than you wanted.

I did not test this, but I could imagine that this could happen here, too.

...

...

...

...

...

> + &General::log($Lang::tr{'fixed ip lease added'}); > + $dhcpsettings{'KEY2'} = ''; > + } else { #edit > unshift (@current2, "$dhcpsettings{'FIX_MAC'},$dhcpsettings{'FIX_ADDR'},$dhcpsettings{'FIX_ENABLED'},$dhcpsettings{'FIX_NEXTADDR'},$dhcpsettings{'FIX_FILENAME'},$dhcpsettings{'FIX_ROOTPATH'},$dhcpsettings{'FIX_REMARK'}\n"); > &General::log($Lang::tr{'fixed ip lease added'}); > > # Enter edit mode > $dhcpsettings{'KEY2'} = 0; > + } > } else { > @current2[$dhcpsettings{'KEY2'}] = "$dhcpsettings{'FIX_MAC'},$dhcpsettings{'FIX_ADDR'},$dhcpsettings{'FIX_ENABLED'},$dhcpsettings{'FIX_NEXTADDR'},$dhcpsettings{'FIX_FILENAME'},$dhcpsettings{'FIX_ROOTPATH'},$dhcpsettings{'FIX_REMARK'}\n"; > $dhcpsettings{'KEY2'} = ''; # End edit mode > -- > 2.18.0 >

-Michael

Again, my intention was a quick resolution for the behaviour mentioned in the forum post, which initiated my code review of dhcpi.cgi

Okay, I will try to explain this one last time - not just only for you, but generally:

I am not interested in quick and dirty solutions. That is how you break things. I am interested in well-documented, peer-reviewed and tested code. We are creating some piece of high-quality software - or aim to do so at least - and 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. I agree fully with your aim, but this means readability also! This means some ( many? ) 'cosmetic' changes in the future, IMHO. Is this accepted?

You cannot submit a patch that does not fulfil the guidelines and then promise to fix it later. There are obvious problems with the patch and there are open questions. Why would we merge something that clearly adds more problems than it solves?

So no, it is not accepted.

Where are the problems? Be more specific please.

Read my first email on the patch. That is as specific as it gets.

...

...

...

Please figure out how to set up a local Git repository, how to set up a branch, how to commit things and what rules there are to follow. Then find out on how to submit a patchset to the mailing list - after it has been tested. It is all in the link that I sent you.

This is not a competition about who can submit patches the fastest.

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 relevant for this single case. I also stated, that I'll surely use git for greater modifications 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 topic by Matthias.

...

...

I found a couple of behaviours of this page, which are not obvious or straight forward ( adding dynamic leases to static leases, for example, maybe this is invoked now by my modification).

What are those? Why are we not talking about those first and then come up with fixes? The whole DHCP page is a mess. I am not sure if it can even be fixed or of things won’t just become worse.

Being an experienced software developer, I think this is possible and I am just working on this. I'll discuss these topics in the list before sending patches, being aware this is a community project this many opinions.

This is not necessarily about opinions. It is about what we can support in the end and where we all want to invest our time.

Helping users with known problems, which could be resolved, is wating time also. In the moment I do this mainly.

...

Think about that we are all in the same boat and we want to improve IPFire wherever we can. But we need to talk about things because one set of eyes often is not enough. Something that works for one person does not work for another. We can never break backwards-compatibility.

I personally want to see this static lease bug fixed. I am trying to help you to develop a good solution that we do not have to worry about in the future. I am also enforcing the rules that we all have come up with some long time ago and that work for this project. That way we will hopefully all benefit from this.

Why didn't you just say this. A request for the whole file would have been enough. BTW the file can be found in the forum post.

There are problems *in* the file. I raised questions.

...

But I do not want to have endless discussions on this list about why the rules are as they are. If there is a constructive proposal to make things easier for all then we are all of course open for this. On the other hand debating what is the standard now is just a waste of time. I am happy to explain this, but I am not willing to debate them or compromise on this. Again, this is a general statement and not limited to this conversation.

You started this discussion ( once more ).

Okay, I tried to be helpful here.

If you prefer to insist that this patch is not being amended and if you also prefer to point fingers, keep doing it.

I will remember this and keep my comments to myself in the future then.

...

...

...

...

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.

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 term.

Agreed, too. But again, this special problem appears from time to time in the forums.

Why do we not have a ticket on BZ then?

The ticket is there ( see subject of this discussion ) and the solution exists.

I disagree with this “solution”.

-Bernhard

...

...

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 reason for the patch. The amount of modified code isn't so big to demand the canonical development process, IMHO. The patch can be applied by any core developper reading DevList/bugzilla/forums regulary.

It is entirely up to you how you develop your code. How it is submitted to the list is clear.

-Michael

...

-Bernhard

...

-Michael

...

Regards, Bernhard

Hello,

I just wanted to share with the list that Bernhard has emailed me in private, telling me that he no longer wants to pursue getting this patch merged.

I find this whole situation very frustrating, but of course I accept his decision.

I guess we just have to agree that we disagree here.

Best, -Michael

On 18 Apr 2019, at 21:37, Bernhard Bitsch Bernhard.Bitsch@gmx.de wrote:

...

Gesendet: Donnerstag, 18. April 2019 um 16:47 Uhr Von: "Michael Tremer" michael.tremer@ipfire.org An: "Bernhard Bitsch" Bernhard.Bitsch@gmx.de Cc: "IPFire: Development-List" development@lists.ipfire.org Betreff: Re: [PATCH] Fix for Bug #12050: Adding fixed leases with one 'add' click

Hi,

...

On 18 Apr 2019, at 13:54, Bernhard Bitsch Bernhard.Bitsch@gmx.de wrote:

Hello,

I do not want to start any discussions about the way the project does its work. Therefore some (hopefully) short annotations below.

...

Gesendet: Donnerstag, 18. April 2019 um 13:42 Uhr Von: "Michael Tremer" michael.tremer@ipfire.org An: "Bernhard Bitsch" Bernhard.Bitsch@gmx.de Cc: "IPFire: Development-List" development@lists.ipfire.org Betreff: Re: [PATCH] Fix for Bug #12050: Adding fixed leases with one 'add' click

Hello,

...

On 18 Apr 2019, at 12:23, Bernhard Bitsch Bernhard.Bitsch@gmx.de wrote:

Hi,

...

Gesendet: Donnerstag, 18. April 2019 um 11:50 Uhr Von: "Michael Tremer" michael.tremer@ipfire.org An: "Bernhard Bitsch" Bernhard.Bitsch@gmx.de Cc: BeBiMa bbitsch@ipfire.org, "IPFire: Development-List" development@lists.ipfire.org Betreff: Re: [PATCH] Fix for Bug #12050: Adding fixed leases with one 'add' click

Hi,

> On 17 Apr 2019, at 22:49, Bernhard Bitsch Bernhard.Bitsch@gmx.de wrote: > > Hi, > some explanations from the author: > >> Gesendet: Mittwoch, 17. April 2019 um 11:31 Uhr >> Von: "Michael Tremer" michael.tremer@ipfire.org >> An: "Matthias Fischer" matthias.fischer@ipfire.org >> Cc: development@lists.ipfire.org, BeBiMa bbitsch@ipfire.org >> Betreff: Re: [PATCH] Fix for Bug #12050: Adding fixed leases with one 'add' click >> >> Hi, >> >> Thanks Matthias for helping out here. However, I do not get the patch. >> >> There is no explanation about what it is meant to do. The intention already is that the lease is added in the first place. >> > > 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.

Those comments *must* be in the code. Nobody goes through thousands of emails on a mailing list to find out what is actually intended in the code.

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 case I regarded the forum post and the bugzilla entry to be sufficient documentation, for the moment.

No.

Every commit must contain itself.

...

...

> >>> On 16 Apr 2019, at 17:41, Matthias Fischer matthias.fischer@ipfire.org wrote: >>> >>> Signed-off-by: BeBiMa bbitsch@ipfire.org >>> Reviewed-by: Matthias Fischer matthias.fischer@ipfire.org >>> --- >>> html/cgi-bin/dhcp.cgi | 12 ++++++++++++ >>> 1 file changed, 12 insertions(+) >>> >>> 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') { >>> } >>> >>> my $key = 0; >>> + my $szc = scalar(@current2); >>> CHECK:foreach my $line (@current2) { >>> my @temp = split(/,/,$line); >>> if($dhcpsettings{'KEY2'} ne $key) { >>> # same MAC is OK on different subnets. This test is not complete because >>> # if ip are not inside a known subnet, I don't warn. >>> # Also it may be needed to put duplicate fixed lease in their right subnet definition.. >>> + if ((lc($dhcpsettings{'FIX_MAC'}) eq lc($temp[0])) &&(lc($dhcpsettings{'FIX_ADDR'}) eq lc($temp[1]))) { >>> + last CHECK; >>> + } >> >> Why is this needed? > > Check for existing lease. If <MAC,IP> is defined already we don't need to loop further. > >> >>> foreach my $itf (@ITFs) { >>> my $scoped = &General::IpInSubnet($dhcpsettings{'FIX_ADDR'}, >>> $netsettings{"${itf}_NETADDRESS"}, >>> @@ -442,11 +446,19 @@ if ($dhcpsettings{'ACTION'} eq $Lang::tr{'add'}.'2') { >>> $dhcpsettings{'FIX_FILENAME'} = &Header::cleanhtml($dhcpsettings{'FIX_FILENAME'}); >>> $dhcpsettings{'FIX_ROOTPATH'} = &Header::cleanhtml($dhcpsettings{'FIX_ROOTPATH'}); >>> if ($dhcpsettings{'KEY2'} eq '') { #add or edit ? >> >> This block here is not indented correctly. >> >> I understand that the code is already very messy, but we should not make it worse either. >> > How should indention be done? By spaces, tabs? How many colums define an indent?

In the patch it looks like you wrapped the block into another if condition. So it has to be indented to make that obvious.

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.

Do *not* submit non-final versions. There is no point in it. A patch is meant to go into a final release. We do not want beta stuff in there.

Okay. Got it. I should have sent my modified dhcp.cgi because of lack of a working git repo on my site.

Please learn how to use git and use “git send-email”. There is an introduction on the wiki on how to set it up and there is tons of resources on the Internet that explain Git to you in the form of howtos and videos.

Sorry, I know about git. It is not the lack of knowledge, but my limited equipment. You invite to donate for the project, which is very urgent, I think. On the other hand I've tried to help with limited sources and without monetary assistance, till now. If this isn't possible, it is okay for me. I'll retire to the status "consuming user".

...

...

...

...

...

> The original file (and many ohters!) have an mixture of tab/space. Should we patch that step by step ( tab=4 ) to increase readability.

This is not about tabs or spaces.

Sure! This mixture doesnt't matter only in case of "tab=4" definition, which 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 necessary to understand the code working on.

If you insist to take this conversation down this route, then feel free to do so. We do not have a policy that commands spaces.

I liked you the coding style.

Especially in this file I found a messy mixture of tabs and spaces. Therefore my statement. If we commit us to tab=4 these spaces can be eliminated.

We inherited some code from IPCop that we did not clean up.

So the guideline is to use the coding style used in the particular file. Do not re-indent the whole file.

If that's the opinion of the majority of the developers, live with it. One reason of the ending of IPCop development was this rigid interpretation of "don't touch a running system", IMHO.

...

...

...

...

...

> 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.

Please just use “git diff”. It has a variety of settings that are just right.

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 arbitrary files with "git diff". Sorry.

Of course you would have the files in your working system. How else would you test?

But for development purposes git is being used. It is the standard. Otherwise patches won’t apply. There is no point in sending patches that other developers cannot apply. Use Git.

Ok. If I don't have a git repo myself, I send the modified source. Each developer with an actual git repo can apply it by commit. Right?

Please learn how to use git.

Of course you could send modified files around, but that does not work. Of course you could send instructions to other people on how to change files, but that doesn’t work either.

I personally won’t do any work of trying to apply any patches that are send in other forms. There is also reasons that we send patches inline: We can comment on them.

Please learn how to use git.

All said about this above.

...

...

...

...

...

> This generated a short diff without real corrections only. > >>> + if($key == $szc) { #add >>> + @current2[$key] = "$dhcpsettings{'FIX_MAC'},$dhcpsettings{'FIX_ADDR'},$dhcpsettings{'FIX_ENABLED'},$dhcpsettings{'FIX_NEXTADDR'},$dhcpsettings{'FIX_FILENAME'},$dhcpsettings{'FIX_ROOTPATH'},$dhcpsettings{'FIX_REMARK'}\n"; >>> + # sort newly added/modified entry >>> + &sortcurrent2; >> >> Are you sure we can sort here? >> >> See: https://git.ipfire.org/?p=ipfire-2.x.git;a=commitdiff;h=31672dc8bdb223ebf425... >> > > Yes! Why not?

Because of the commit I referred to.

Didn't understand this commit, because of lack of commentation. ;)

And you didn’t ask any questions then?

Ok. I ask you now. Why doesn't it function?

Well, as the patch there states the key is saved and used later. When the leases file is sorted, the key changes but is not updated. That lead to the case that you edited a different lease than you wanted.

I did not test this, but I could imagine that this could happen here, too.

I know from reading and understandig the code, that this works. The approval by Matthias Fischer shows, that I'm right. See the related forum thread. Software development and code review is no field of imagination.

...

...

...

...

...

> >>> + &General::log($Lang::tr{'fixed ip lease added'}); >>> + $dhcpsettings{'KEY2'} = ''; >>> + } else { #edit >>> unshift (@current2, "$dhcpsettings{'FIX_MAC'},$dhcpsettings{'FIX_ADDR'},$dhcpsettings{'FIX_ENABLED'},$dhcpsettings{'FIX_NEXTADDR'},$dhcpsettings{'FIX_FILENAME'},$dhcpsettings{'FIX_ROOTPATH'},$dhcpsettings{'FIX_REMARK'}\n"); >>> &General::log($Lang::tr{'fixed ip lease added'}); >>> >>> # Enter edit mode >>> $dhcpsettings{'KEY2'} = 0; >>> + } >>> } else { >>> @current2[$dhcpsettings{'KEY2'}] = "$dhcpsettings{'FIX_MAC'},$dhcpsettings{'FIX_ADDR'},$dhcpsettings{'FIX_ENABLED'},$dhcpsettings{'FIX_NEXTADDR'},$dhcpsettings{'FIX_FILENAME'},$dhcpsettings{'FIX_ROOTPATH'},$dhcpsettings{'FIX_REMARK'}\n"; >>> $dhcpsettings{'KEY2'} = ''; # End edit mode >>> -- >>> 2.18.0 >>> >> >> -Michael >> >> > > Again, my intention was a quick resolution for the behaviour mentioned in the forum post, which initiated my code review of dhcpi.cgi

Okay, I will try to explain this one last time - not just only for you, but generally:

I am not interested in quick and dirty solutions. That is how you break things. I am interested in well-documented, peer-reviewed and tested code. We are creating some piece of high-quality software - or aim to do so at least - and 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. I agree fully with your aim, but this means readability also! This means some ( many? ) 'cosmetic' changes in the future, IMHO. Is this accepted?

You cannot submit a patch that does not fulfil the guidelines and then promise to fix it later. There are obvious problems with the patch and there are open questions. Why would we merge something that clearly adds more problems than it solves?

So no, it is not accepted.

Where are the problems? Be more specific please.

Read my first email on the patch. That is as specific as it gets.

I'll do this. But I don't know whether this changes the my opinion about the fixed problem.

...

...

...

...

...

Please figure out how to set up a local Git repository, how to set up a branch, how to commit things and what rules there are to follow. Then find out on how to submit a patchset to the mailing list - after it has been tested. It is all in the link that I sent you.

This is not a competition about who can submit patches the fastest.

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 relevant for this single case. I also stated, that I'll surely use git for greater modifications 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 topic by Matthias.

...

> I found a couple of behaviours of this page, which are not obvious or straight forward ( adding dynamic leases to static leases, for example, maybe this is invoked now by my modification).

What are those? Why are we not talking about those first and then come up with fixes? The whole DHCP page is a mess. I am not sure if it can even be fixed or of things won’t just become worse.

Being an experienced software developer, I think this is possible and I am just working on this. I'll discuss these topics in the list before sending patches, being aware this is a community project this many opinions.

This is not necessarily about opinions. It is about what we can support in the end and where we all want to invest our time.

Helping users with known problems, which could be resolved, is wating time also. In the moment I do this mainly.

...

Think about that we are all in the same boat and we want to improve IPFire wherever we can. But we need to talk about things because one set of eyes often is not enough. Something that works for one person does not work for another. We can never break backwards-compatibility.

I personally want to see this static lease bug fixed. I am trying to help you to develop a good solution that we do not have to worry about in the future. I am also enforcing the rules that we all have come up with some long time ago and that work for this project. That way we will hopefully all benefit from this.

Why didn't you just say this. A request for the whole file would have been enough. BTW the file can be found in the forum post.

There are problems *in* the file. I raised questions.

see above.

...

...

...

But I do not want to have endless discussions on this list about why the rules are as they are. If there is a constructive proposal to make things easier for all then we are all of course open for this. On the other hand debating what is the standard now is just a waste of time. I am happy to explain this, but I am not willing to debate them or compromise on this. Again, this is a general statement and not limited to this conversation.

You started this discussion ( once more ).

Okay, I tried to be helpful here.

If you prefer to insist that this patch is not being amended and if you also prefer to point fingers, keep doing it.

I will remember this and keep my comments to myself in the future then.

There is no need to behave like this. I'll be quiet myself upon source quality and proposed fixes for bugs.

...

...

...

...

...

> 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.

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 term.

Agreed, too. But again, this special problem appears from time to time in the forums.

Why do we not have a ticket on BZ then?

The ticket is there ( see subject of this discussion ) and the solution exists.

I disagree with this “solution”.

Why?

-Bernhard

...

...

-Bernhard

...

...

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 reason for the patch. The amount of modified code isn't so big to demand the canonical development process, IMHO. The patch can be applied by any core developper reading DevList/bugzilla/forums regulary.

It is entirely up to you how you develop your code. How it is submitted to the list is clear.

-Michael

...

-Bernhard

...

-Michael

> > Regards, > Bernhard

Hi,

This bug is currently assigned to Florian, because I have asked him to have a look at it.

I do not really care who is working on this, but I would like everyone to work together on it.

I would also like to stress that we have urgent fixes for loads of Intel processors in Core Update 132 and I think that this problem should also be fixed in this update. So, please work on this with a matter of urgency.

Best, -Michael

On 17 May 2019, at 11:58, Bernhard Bitsch Bernhard.Bitsch@gmx.de wrote:

Hello,

just some news about this topic.

• Michael was right to refuse my quick and dirty patch. It did not really solve the problem, maybe it introduced some new problems. Sorry for my quick and dirty reaction.

• I wasn't satisfied with the situation being. The problem exists furthermore and isn't easy solved. Therefore I started a review and commenting for my own. With some effort I think I've located the main error ( see my short post in bugzilla ).

Thus, you will read again from me about solutions for Bug #12050. My current state is as follows: I've added a bunch of comments for understanding the program. The sort algorithm for fixed leases ( maybe dynamic leases also ) should and can be optimised. The operations "add new fixed lease", "edit existing fixed lease", "add dynamic lease to fixed leases" must be verified and corrected.

Proposal for behaviour: "add new fixed lease" : add a new entry with parameters to fixed leases list, sort according the existing order. It should not be necessary to edit it first. "edit existing fixed lease": move parameters of selected entry to edit box. Highlight edited entry at his place ( if entry #12 should be edited, row #12 is highlighted ). Change entry with new paramters, sort. "add dynamic lease to fixed lease": move values of dynamic lease to edit box. Editing is mandatory! We should not merge sets of fixed and dynamic leases. Add new entry as new fixed lease with changed parameters, sort.

When I've succeeded in implementing these topics, I'll post the result. Because of the number of changes in source ( comments, enhancements for readability, .... ) I would suggest a commit of approved file as a whole, not just as single little patches. How can this be accomplished? I think little patches here and there cannot solve the problem of low maintainability of this file.

Regards, Bernhard

...

Gesendet: Samstag, 20. April 2019 um 18:35 Uhr Von: "Michael Tremer" michael.tremer@ipfire.org An: "Bernhard Bitsch" Bernhard.Bitsch@gmx.de Cc: "IPFire: Development-List" development@lists.ipfire.org Betreff: Re: [PATCH] Fix for Bug #12050: Adding fixed leases with one 'add' click

Hello,

I just wanted to share with the list that Bernhard has emailed me in private, telling me that he no longer wants to pursue getting this patch merged.

I find this whole situation very frustrating, but of course I accept his decision.

I guess we just have to agree that we disagree here.

Best, -Michael

...

On 18 Apr 2019, at 21:37, Bernhard Bitsch Bernhard.Bitsch@gmx.de wrote:

...

Gesendet: Donnerstag, 18. April 2019 um 16:47 Uhr Von: "Michael Tremer" michael.tremer@ipfire.org An: "Bernhard Bitsch" Bernhard.Bitsch@gmx.de Cc: "IPFire: Development-List" development@lists.ipfire.org Betreff: Re: [PATCH] Fix for Bug #12050: Adding fixed leases with one 'add' click

Hi,

...

On 18 Apr 2019, at 13:54, Bernhard Bitsch Bernhard.Bitsch@gmx.de wrote:

Hello,

I do not want to start any discussions about the way the project does its work. Therefore some (hopefully) short annotations below.

...

Gesendet: Donnerstag, 18. April 2019 um 13:42 Uhr Von: "Michael Tremer" michael.tremer@ipfire.org An: "Bernhard Bitsch" Bernhard.Bitsch@gmx.de Cc: "IPFire: Development-List" development@lists.ipfire.org Betreff: Re: [PATCH] Fix for Bug #12050: Adding fixed leases with one 'add' click

Hello,

> On 18 Apr 2019, at 12:23, Bernhard Bitsch Bernhard.Bitsch@gmx.de wrote: > > Hi, > >> Gesendet: Donnerstag, 18. April 2019 um 11:50 Uhr >> Von: "Michael Tremer" michael.tremer@ipfire.org >> An: "Bernhard Bitsch" Bernhard.Bitsch@gmx.de >> Cc: BeBiMa bbitsch@ipfire.org, "IPFire: Development-List" development@lists.ipfire.org >> Betreff: Re: [PATCH] Fix for Bug #12050: Adding fixed leases with one 'add' click >> >> Hi, >> >>> On 17 Apr 2019, at 22:49, Bernhard Bitsch Bernhard.Bitsch@gmx.de wrote: >>> >>> Hi, >>> some explanations from the author: >>> >>>> Gesendet: Mittwoch, 17. April 2019 um 11:31 Uhr >>>> Von: "Michael Tremer" michael.tremer@ipfire.org >>>> An: "Matthias Fischer" matthias.fischer@ipfire.org >>>> Cc: development@lists.ipfire.org, BeBiMa bbitsch@ipfire.org >>>> Betreff: Re: [PATCH] Fix for Bug #12050: Adding fixed leases with one 'add' click >>>> >>>> Hi, >>>> >>>> Thanks Matthias for helping out here. However, I do not get the patch. >>>> >>>> There is no explanation about what it is meant to do. The intention already is that the lease is added in the first place. >>>> >>> >>> 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. >> >> Those comments *must* be in the code. Nobody goes through thousands of emails on a mailing list to find out what is actually intended in the code. >> > > 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 case I regarded the forum post and the bugzilla entry to be sufficient documentation, for the moment.

No.

Every commit must contain itself.

>>> >>>>> On 16 Apr 2019, at 17:41, Matthias Fischer matthias.fischer@ipfire.org wrote: >>>>> >>>>> Signed-off-by: BeBiMa bbitsch@ipfire.org >>>>> Reviewed-by: Matthias Fischer matthias.fischer@ipfire.org >>>>> --- >>>>> html/cgi-bin/dhcp.cgi | 12 ++++++++++++ >>>>> 1 file changed, 12 insertions(+) >>>>> >>>>> 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') { >>>>> } >>>>> >>>>> my $key = 0; >>>>> + my $szc = scalar(@current2); >>>>> CHECK:foreach my $line (@current2) { >>>>> my @temp = split(/,/,$line); >>>>> if($dhcpsettings{'KEY2'} ne $key) { >>>>> # same MAC is OK on different subnets. This test is not complete because >>>>> # if ip are not inside a known subnet, I don't warn. >>>>> # Also it may be needed to put duplicate fixed lease in their right subnet definition.. >>>>> + if ((lc($dhcpsettings{'FIX_MAC'}) eq lc($temp[0])) &&(lc($dhcpsettings{'FIX_ADDR'}) eq lc($temp[1]))) { >>>>> + last CHECK; >>>>> + } >>>> >>>> Why is this needed? >>> >>> Check for existing lease. If <MAC,IP> is defined already we don't need to loop further. >>> >>>> >>>>> foreach my $itf (@ITFs) { >>>>> my $scoped = &General::IpInSubnet($dhcpsettings{'FIX_ADDR'}, >>>>> $netsettings{"${itf}_NETADDRESS"}, >>>>> @@ -442,11 +446,19 @@ if ($dhcpsettings{'ACTION'} eq $Lang::tr{'add'}.'2') { >>>>> $dhcpsettings{'FIX_FILENAME'} = &Header::cleanhtml($dhcpsettings{'FIX_FILENAME'}); >>>>> $dhcpsettings{'FIX_ROOTPATH'} = &Header::cleanhtml($dhcpsettings{'FIX_ROOTPATH'}); >>>>> if ($dhcpsettings{'KEY2'} eq '') { #add or edit ? >>>> >>>> This block here is not indented correctly. >>>> >>>> I understand that the code is already very messy, but we should not make it worse either. >>>> >>> How should indention be done? By spaces, tabs? How many colums define an indent? >> >> In the patch it looks like you wrapped the block into another if condition. So it has to be indented to make that obvious. >> > > 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.

Do *not* submit non-final versions. There is no point in it. A patch is meant to go into a final release. We do not want beta stuff in there.

Okay. Got it. I should have sent my modified dhcp.cgi because of lack of a working git repo on my site.

Please learn how to use git and use “git send-email”. There is an introduction on the wiki on how to set it up and there is tons of resources on the Internet that explain Git to you in the form of howtos and videos.

Sorry, I know about git. It is not the lack of knowledge, but my limited equipment. You invite to donate for the project, which is very urgent, I think. On the other hand I've tried to help with limited sources and without monetary assistance, till now. If this isn't possible, it is okay for me. I'll retire to the status "consuming user".

...

...

...

> >>> The original file (and many ohters!) have an mixture of tab/space. Should we patch that step by step ( tab=4 ) to increase readability. >> >> This is not about tabs or spaces. >> > > Sure! This mixture doesnt't matter only in case of "tab=4" definition, which 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 necessary to understand the code working on.

If you insist to take this conversation down this route, then feel free to do so. We do not have a policy that commands spaces.

I liked you the coding style.

Especially in this file I found a messy mixture of tabs and spaces. Therefore my statement. If we commit us to tab=4 these spaces can be eliminated.

We inherited some code from IPCop that we did not clean up.

So the guideline is to use the coding style used in the particular file. Do not re-indent the whole file.

If that's the opinion of the majority of the developers, live with it. One reason of the ending of IPCop development was this rigid interpretation of "don't touch a running system", IMHO.

...

...

...

> >>> 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. >> >> Please just use “git diff”. It has a variety of settings that are just right. >> > > 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 arbitrary files with "git diff". Sorry.

Of course you would have the files in your working system. How else would you test?

But for development purposes git is being used. It is the standard. Otherwise patches won’t apply. There is no point in sending patches that other developers cannot apply. Use Git.

Ok. If I don't have a git repo myself, I send the modified source. Each developer with an actual git repo can apply it by commit. Right?

Please learn how to use git.

Of course you could send modified files around, but that does not work. Of course you could send instructions to other people on how to change files, but that doesn’t work either.

I personally won’t do any work of trying to apply any patches that are send in other forms. There is also reasons that we send patches inline: We can comment on them.

Please learn how to use git.

All said about this above.

...

...

...

> >>> This generated a short diff without real corrections only. >>> >>>>> + if($key == $szc) { #add >>>>> + @current2[$key] = "$dhcpsettings{'FIX_MAC'},$dhcpsettings{'FIX_ADDR'},$dhcpsettings{'FIX_ENABLED'},$dhcpsettings{'FIX_NEXTADDR'},$dhcpsettings{'FIX_FILENAME'},$dhcpsettings{'FIX_ROOTPATH'},$dhcpsettings{'FIX_REMARK'}\n"; >>>>> + # sort newly added/modified entry >>>>> + &sortcurrent2; >>>> >>>> Are you sure we can sort here? >>>> >>>> See: https://git.ipfire.org/?p=ipfire-2.x.git;a=commitdiff;h=31672dc8bdb223ebf425... >>>> >>> >>> Yes! Why not? >> >> Because of the commit I referred to. > > Didn't understand this commit, because of lack of commentation. ;)

And you didn’t ask any questions then?

Ok. I ask you now. Why doesn't it function?

Well, as the patch there states the key is saved and used later. When the leases file is sorted, the key changes but is not updated. That lead to the case that you edited a different lease than you wanted.

I did not test this, but I could imagine that this could happen here, too.

I know from reading and understandig the code, that this works. The approval by Matthias Fischer shows, that I'm right. See the related forum thread. Software development and code review is no field of imagination.

...

...

...

>> >>> >>>>> + &General::log($Lang::tr{'fixed ip lease added'}); >>>>> + $dhcpsettings{'KEY2'} = ''; >>>>> + } else { #edit >>>>> unshift (@current2, "$dhcpsettings{'FIX_MAC'},$dhcpsettings{'FIX_ADDR'},$dhcpsettings{'FIX_ENABLED'},$dhcpsettings{'FIX_NEXTADDR'},$dhcpsettings{'FIX_FILENAME'},$dhcpsettings{'FIX_ROOTPATH'},$dhcpsettings{'FIX_REMARK'}\n"); >>>>> &General::log($Lang::tr{'fixed ip lease added'}); >>>>> >>>>> # Enter edit mode >>>>> $dhcpsettings{'KEY2'} = 0; >>>>> + } >>>>> } else { >>>>> @current2[$dhcpsettings{'KEY2'}] = "$dhcpsettings{'FIX_MAC'},$dhcpsettings{'FIX_ADDR'},$dhcpsettings{'FIX_ENABLED'},$dhcpsettings{'FIX_NEXTADDR'},$dhcpsettings{'FIX_FILENAME'},$dhcpsettings{'FIX_ROOTPATH'},$dhcpsettings{'FIX_REMARK'}\n"; >>>>> $dhcpsettings{'KEY2'} = ''; # End edit mode >>>>> -- >>>>> 2.18.0 >>>>> >>>> >>>> -Michael >>>> >>>> >>> >>> Again, my intention was a quick resolution for the behaviour mentioned in the forum post, which initiated my code review of dhcpi.cgi >> >> Okay, I will try to explain this one last time - not just only for you, but generally: >> >> I am not interested in quick and dirty solutions. That is how you break things. I am interested in well-documented, peer-reviewed and tested code. We are creating some piece of high-quality software - or aim to do so at least - and 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. > I agree fully with your aim, but this means readability also! This means some ( many? ) 'cosmetic' changes in the future, IMHO. Is this accepted?

You cannot submit a patch that does not fulfil the guidelines and then promise to fix it later. There are obvious problems with the patch and there are open questions. Why would we merge something that clearly adds more problems than it solves?

So no, it is not accepted.

Where are the problems? Be more specific please.

Read my first email on the patch. That is as specific as it gets.

I'll do this. But I don't know whether this changes the my opinion about the fixed problem.

...

...

...

>> Please figure out how to set up a local Git repository, how to set up a branch, how to commit things and what rules there are to follow. Then find out on how to submit a patchset to the mailing list - after it has been tested. It is all in the link that I sent you. >> >> This is not a competition about who can submit patches the fastest. >> > > 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 relevant for this single case. I also stated, that I'll surely use git for greater modifications 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 topic by Matthias. > >>> I found a couple of behaviours of this page, which are not obvious or straight forward ( adding dynamic leases to static leases, for example, maybe this is invoked now by my modification). >> >> What are those? Why are we not talking about those first and then come up with fixes? The whole DHCP page is a mess. I am not sure if it can even be fixed or of things won’t just become worse. >> > > Being an experienced software developer, I think this is possible and I am just working on this. I'll discuss these topics in the list before sending patches, being aware this is a community project this many opinions.

This is not necessarily about opinions. It is about what we can support in the end and where we all want to invest our time.

Helping users with known problems, which could be resolved, is wating time also. In the moment I do this mainly.

...

Think about that we are all in the same boat and we want to improve IPFire wherever we can. But we need to talk about things because one set of eyes often is not enough. Something that works for one person does not work for another. We can never break backwards-compatibility.

I personally want to see this static lease bug fixed. I am trying to help you to develop a good solution that we do not have to worry about in the future. I am also enforcing the rules that we all have come up with some long time ago and that work for this project. That way we will hopefully all benefit from this.

Why didn't you just say this. A request for the whole file would have been enough. BTW the file can be found in the forum post.

There are problems *in* the file. I raised questions.

see above.

...

...

...

But I do not want to have endless discussions on this list about why the rules are as they are. If there is a constructive proposal to make things easier for all then we are all of course open for this. On the other hand debating what is the standard now is just a waste of time. I am happy to explain this, but I am not willing to debate them or compromise on this. Again, this is a general statement and not limited to this conversation.

You started this discussion ( once more ).

Okay, I tried to be helpful here.

If you prefer to insist that this patch is not being amended and if you also prefer to point fingers, keep doing it.

I will remember this and keep my comments to myself in the future then.

There is no need to behave like this. I'll be quiet myself upon source quality and proposed fixes for bugs.

...

...

...

>>> 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. >> >> 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 term. >> > > Agreed, too. But again, this special problem appears from time to time in the forums.

Why do we not have a ticket on BZ then?

The ticket is there ( see subject of this discussion ) and the solution exists.

I disagree with this “solution”.

Why?

-Bernhard

...

...

-Bernhard

...

> 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 reason for the patch. The amount of modified code isn't so big to demand the canonical development process, IMHO. The patch can be applied by any core developper reading DevList/bugzilla/forums regulary.

It is entirely up to you how you develop your code. How it is submitted to the list is clear.

-Michael

> > -Bernhard > >> -Michael >> >>> >>> Regards, >>> Bernhard

Hi,

On 17 May 2019, at 23:21, Bernhard Bitsch Bernhard.Bitsch@gmx.de wrote:

Hi,

sorry if I were not exact enough. I am working with a matter of urgency on the DHCP config problem and surely will contact Florian, if I've found the error and a real fix for it.

No need to contact him directly. That is what this list is for.

My post was just a heads up about this work. The rewriting will be the second step, after bug elimination. The purpose should be to faciliate future bug fixes.

I do not think that it makes any sense to rewrite this CGI file. We are going to get rid of the whole web UI in IPFire 3 and there are no new features to expect in the DHCP area that it makes sense to tidy up to much. I am not advocating to have messy code here, but it works, and it easily breaks if you touch something.

My idea for quick inclusion of fix: Florian is working on it, therefore I'll discuss possible solutions with him. He will commit these to the git repo, based on the actual dhcp.cgi file.

Why don’t you say it here what you have found?

When I am finished with the rewrite (including the error fix) I'll publish it either here in the devel list or in git or both. Is this ok?

That really depends on what you want to achieve here. If you are sending in patches that clean up code but do not change any behaviour you should collect them in a git branch and send that branch as a patchset. Bug fixes should be sent separately if it makes sense.

If you plan to change any behaviour of the CGI file, that is a matter open for discussion first and then work should start.

Hope this makes sense.

-Michael

Best, Bernhard

...

Gesendet: Freitag, 17. Mai 2019 um 21:18 Uhr Von: "Michael Tremer" michael.tremer@ipfire.org An: "Bernhard Bitsch" Bernhard.Bitsch@gmx.de Cc: "IPFire: Development-List" development@lists.ipfire.org, florian.buehrle@ipfire.org Betreff: Re: [PATCH] Fix for Bug #12050: Adding fixed leases with one 'add' click

Hi,

This bug is currently assigned to Florian, because I have asked him to have a look at it.

I do not really care who is working on this, but I would like everyone to work together on it.

I would also like to stress that we have urgent fixes for loads of Intel processors in Core Update 132 and I think that this problem should also be fixed in this update. So, please work on this with a matter of urgency.

Best, -Michael

...

On 17 May 2019, at 11:58, Bernhard Bitsch Bernhard.Bitsch@gmx.de wrote:

Hello,

just some news about this topic.

• Michael was right to refuse my quick and dirty patch. It did not really solve the problem, maybe it introduced some new problems. Sorry for my quick and dirty reaction.

• I wasn't satisfied with the situation being. The problem exists furthermore and isn't easy solved. Therefore I started a review and commenting for my own. With some effort I think I've located the main error ( see my short post in bugzilla ).

Thus, you will read again from me about solutions for Bug #12050. My current state is as follows: I've added a bunch of comments for understanding the program. The sort algorithm for fixed leases ( maybe dynamic leases also ) should and can be optimised. The operations "add new fixed lease", "edit existing fixed lease", "add dynamic lease to fixed leases" must be verified and corrected.

Proposal for behaviour: "add new fixed lease" : add a new entry with parameters to fixed leases list, sort according the existing order. It should not be necessary to edit it first. "edit existing fixed lease": move parameters of selected entry to edit box. Highlight edited entry at his place ( if entry #12 should be edited, row #12 is highlighted ). Change entry with new paramters, sort. "add dynamic lease to fixed lease": move values of dynamic lease to edit box. Editing is mandatory! We should not merge sets of fixed and dynamic leases. Add new entry as new fixed lease with changed parameters, sort.

When I've succeeded in implementing these topics, I'll post the result. Because of the number of changes in source ( comments, enhancements for readability, .... ) I would suggest a commit of approved file as a whole, not just as single little patches. How can this be accomplished? I think little patches here and there cannot solve the problem of low maintainability of this file.

Regards, Bernhard

...

Gesendet: Samstag, 20. April 2019 um 18:35 Uhr Von: "Michael Tremer" michael.tremer@ipfire.org An: "Bernhard Bitsch" Bernhard.Bitsch@gmx.de Cc: "IPFire: Development-List" development@lists.ipfire.org Betreff: Re: [PATCH] Fix for Bug #12050: Adding fixed leases with one 'add' click

Hello,

I just wanted to share with the list that Bernhard has emailed me in private, telling me that he no longer wants to pursue getting this patch merged.

I find this whole situation very frustrating, but of course I accept his decision.

I guess we just have to agree that we disagree here.

Best, -Michael

...

On 18 Apr 2019, at 21:37, Bernhard Bitsch Bernhard.Bitsch@gmx.de wrote:

...

Gesendet: Donnerstag, 18. April 2019 um 16:47 Uhr Von: "Michael Tremer" michael.tremer@ipfire.org An: "Bernhard Bitsch" Bernhard.Bitsch@gmx.de Cc: "IPFire: Development-List" development@lists.ipfire.org Betreff: Re: [PATCH] Fix for Bug #12050: Adding fixed leases with one 'add' click

Hi,

> On 18 Apr 2019, at 13:54, Bernhard Bitsch Bernhard.Bitsch@gmx.de wrote: > > Hello, > > I do not want to start any discussions about the way the project does its work. > Therefore some (hopefully) short annotations below. > >> Gesendet: Donnerstag, 18. April 2019 um 13:42 Uhr >> Von: "Michael Tremer" michael.tremer@ipfire.org >> An: "Bernhard Bitsch" Bernhard.Bitsch@gmx.de >> Cc: "IPFire: Development-List" development@lists.ipfire.org >> Betreff: Re: [PATCH] Fix for Bug #12050: Adding fixed leases with one 'add' click >> >> Hello, >> >>> On 18 Apr 2019, at 12:23, Bernhard Bitsch Bernhard.Bitsch@gmx.de wrote: >>> >>> Hi, >>> >>>> Gesendet: Donnerstag, 18. April 2019 um 11:50 Uhr >>>> Von: "Michael Tremer" michael.tremer@ipfire.org >>>> An: "Bernhard Bitsch" Bernhard.Bitsch@gmx.de >>>> Cc: BeBiMa bbitsch@ipfire.org, "IPFire: Development-List" development@lists.ipfire.org >>>> Betreff: Re: [PATCH] Fix for Bug #12050: Adding fixed leases with one 'add' click >>>> >>>> Hi, >>>> >>>>> On 17 Apr 2019, at 22:49, Bernhard Bitsch Bernhard.Bitsch@gmx.de wrote: >>>>> >>>>> Hi, >>>>> some explanations from the author: >>>>> >>>>>> Gesendet: Mittwoch, 17. April 2019 um 11:31 Uhr >>>>>> Von: "Michael Tremer" michael.tremer@ipfire.org >>>>>> An: "Matthias Fischer" matthias.fischer@ipfire.org >>>>>> Cc: development@lists.ipfire.org, BeBiMa bbitsch@ipfire.org >>>>>> Betreff: Re: [PATCH] Fix for Bug #12050: Adding fixed leases with one 'add' click >>>>>> >>>>>> Hi, >>>>>> >>>>>> Thanks Matthias for helping out here. However, I do not get the patch. >>>>>> >>>>>> There is no explanation about what it is meant to do. The intention already is that the lease is added in the first place. >>>>>> >>>>> >>>>> 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. >>>> >>>> Those comments *must* be in the code. Nobody goes through thousands of emails on a mailing list to find out what is actually intended in the code. >>>> >>> >>> 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 case I regarded the forum post and the bugzilla entry to be sufficient documentation, for the moment. >> >> No. >> >> Every commit must contain itself. >> >>>>> >>>>>>> On 16 Apr 2019, at 17:41, Matthias Fischer matthias.fischer@ipfire.org wrote: >>>>>>> >>>>>>> Signed-off-by: BeBiMa bbitsch@ipfire.org >>>>>>> Reviewed-by: Matthias Fischer matthias.fischer@ipfire.org >>>>>>> --- >>>>>>> html/cgi-bin/dhcp.cgi | 12 ++++++++++++ >>>>>>> 1 file changed, 12 insertions(+) >>>>>>> >>>>>>> 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') { >>>>>>> } >>>>>>> >>>>>>> my $key = 0; >>>>>>> + my $szc = scalar(@current2); >>>>>>> CHECK:foreach my $line (@current2) { >>>>>>> my @temp = split(/,/,$line); >>>>>>> if($dhcpsettings{'KEY2'} ne $key) { >>>>>>> # same MAC is OK on different subnets. This test is not complete because >>>>>>> # if ip are not inside a known subnet, I don't warn. >>>>>>> # Also it may be needed to put duplicate fixed lease in their right subnet definition.. >>>>>>> + if ((lc($dhcpsettings{'FIX_MAC'}) eq lc($temp[0])) &&(lc($dhcpsettings{'FIX_ADDR'}) eq lc($temp[1]))) { >>>>>>> + last CHECK; >>>>>>> + } >>>>>> >>>>>> Why is this needed? >>>>> >>>>> Check for existing lease. If <MAC,IP> is defined already we don't need to loop further. >>>>> >>>>>> >>>>>>> foreach my $itf (@ITFs) { >>>>>>> my $scoped = &General::IpInSubnet($dhcpsettings{'FIX_ADDR'}, >>>>>>> $netsettings{"${itf}_NETADDRESS"}, >>>>>>> @@ -442,11 +446,19 @@ if ($dhcpsettings{'ACTION'} eq $Lang::tr{'add'}.'2') { >>>>>>> $dhcpsettings{'FIX_FILENAME'} = &Header::cleanhtml($dhcpsettings{'FIX_FILENAME'}); >>>>>>> $dhcpsettings{'FIX_ROOTPATH'} = &Header::cleanhtml($dhcpsettings{'FIX_ROOTPATH'}); >>>>>>> if ($dhcpsettings{'KEY2'} eq '') { #add or edit ? >>>>>> >>>>>> This block here is not indented correctly. >>>>>> >>>>>> I understand that the code is already very messy, but we should not make it worse either. >>>>>> >>>>> How should indention be done? By spaces, tabs? How many colums define an indent? >>>> >>>> In the patch it looks like you wrapped the block into another if condition. So it has to be indented to make that obvious. >>>> >>> >>> 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. >> >> Do *not* submit non-final versions. There is no point in it. A patch is meant to go into a final release. We do not want beta stuff in there. >> > > Okay. Got it. I should have sent my modified dhcp.cgi because of lack of a working git repo on my site.

Please learn how to use git and use “git send-email”. There is an introduction on the wiki on how to set it up and there is tons of resources on the Internet that explain Git to you in the form of howtos and videos.

Sorry, I know about git. It is not the lack of knowledge, but my limited equipment. You invite to donate for the project, which is very urgent, I think. On the other hand I've tried to help with limited sources and without monetary assistance, till now. If this isn't possible, it is okay for me. I'll retire to the status "consuming user".

...

>>> >>>>> The original file (and many ohters!) have an mixture of tab/space. Should we patch that step by step ( tab=4 ) to increase readability. >>>> >>>> This is not about tabs or spaces. >>>> >>> >>> Sure! This mixture doesnt't matter only in case of "tab=4" definition, which 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 necessary to understand the code working on. >> >> If you insist to take this conversation down this route, then feel free to do so. We do not have a policy that commands spaces. >> >> I liked you the coding style. >> > > Especially in this file I found a messy mixture of tabs and spaces. Therefore my statement. If we commit us to tab=4 these spaces can be eliminated.

We inherited some code from IPCop that we did not clean up.

So the guideline is to use the coding style used in the particular file. Do not re-indent the whole file.

If that's the opinion of the majority of the developers, live with it. One reason of the ending of IPCop development was this rigid interpretation of "don't touch a running system", IMHO.

...

>>> >>>>> 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. >>>> >>>> Please just use “git diff”. It has a variety of settings that are just right. >>>> >>> >>> 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 arbitrary files with "git diff". Sorry. >> >> Of course you would have the files in your working system. How else would you test? >> >> But for development purposes git is being used. It is the standard. Otherwise patches won’t apply. There is no point in sending patches that other developers cannot apply. Use Git. >> > > Ok. If I don't have a git repo myself, I send the modified source. Each developer with an actual git repo can apply it by commit. Right?

Please learn how to use git.

Of course you could send modified files around, but that does not work. Of course you could send instructions to other people on how to change files, but that doesn’t work either.

I personally won’t do any work of trying to apply any patches that are send in other forms. There is also reasons that we send patches inline: We can comment on them.

Please learn how to use git.

All said about this above.

...

>>> >>>>> This generated a short diff without real corrections only. >>>>> >>>>>>> + if($key == $szc) { #add >>>>>>> + @current2[$key] = "$dhcpsettings{'FIX_MAC'},$dhcpsettings{'FIX_ADDR'},$dhcpsettings{'FIX_ENABLED'},$dhcpsettings{'FIX_NEXTADDR'},$dhcpsettings{'FIX_FILENAME'},$dhcpsettings{'FIX_ROOTPATH'},$dhcpsettings{'FIX_REMARK'}\n"; >>>>>>> + # sort newly added/modified entry >>>>>>> + &sortcurrent2; >>>>>> >>>>>> Are you sure we can sort here? >>>>>> >>>>>> See: https://git.ipfire.org/?p=ipfire-2.x.git;a=commitdiff;h=31672dc8bdb223ebf425... >>>>>> >>>>> >>>>> Yes! Why not? >>>> >>>> Because of the commit I referred to. >>> >>> Didn't understand this commit, because of lack of commentation. ;) >> >> And you didn’t ask any questions then? >> > > Ok. I ask you now. Why doesn't it function?

Well, as the patch there states the key is saved and used later. When the leases file is sorted, the key changes but is not updated. That lead to the case that you edited a different lease than you wanted.

I did not test this, but I could imagine that this could happen here, too.

I know from reading and understandig the code, that this works. The approval by Matthias Fischer shows, that I'm right. See the related forum thread. Software development and code review is no field of imagination.

...

>>>> >>>>> >>>>>>> + &General::log($Lang::tr{'fixed ip lease added'}); >>>>>>> + $dhcpsettings{'KEY2'} = ''; >>>>>>> + } else { #edit >>>>>>> unshift (@current2, "$dhcpsettings{'FIX_MAC'},$dhcpsettings{'FIX_ADDR'},$dhcpsettings{'FIX_ENABLED'},$dhcpsettings{'FIX_NEXTADDR'},$dhcpsettings{'FIX_FILENAME'},$dhcpsettings{'FIX_ROOTPATH'},$dhcpsettings{'FIX_REMARK'}\n"); >>>>>>> &General::log($Lang::tr{'fixed ip lease added'}); >>>>>>> >>>>>>> # Enter edit mode >>>>>>> $dhcpsettings{'KEY2'} = 0; >>>>>>> + } >>>>>>> } else { >>>>>>> @current2[$dhcpsettings{'KEY2'}] = "$dhcpsettings{'FIX_MAC'},$dhcpsettings{'FIX_ADDR'},$dhcpsettings{'FIX_ENABLED'},$dhcpsettings{'FIX_NEXTADDR'},$dhcpsettings{'FIX_FILENAME'},$dhcpsettings{'FIX_ROOTPATH'},$dhcpsettings{'FIX_REMARK'}\n"; >>>>>>> $dhcpsettings{'KEY2'} = ''; # End edit mode >>>>>>> -- >>>>>>> 2.18.0 >>>>>>> >>>>>> >>>>>> -Michael >>>>>> >>>>>> >>>>> >>>>> Again, my intention was a quick resolution for the behaviour mentioned in the forum post, which initiated my code review of dhcpi.cgi >>>> >>>> Okay, I will try to explain this one last time - not just only for you, but generally: >>>> >>>> I am not interested in quick and dirty solutions. That is how you break things. I am interested in well-documented, peer-reviewed and tested code. We are creating some piece of high-quality software - or aim to do so at least - and 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. >>> I agree fully with your aim, but this means readability also! This means some ( many? ) 'cosmetic' changes in the future, IMHO. Is this accepted? >> >> You cannot submit a patch that does not fulfil the guidelines and then promise to fix it later. There are obvious problems with the patch and there are open questions. Why would we merge something that clearly adds more problems than it solves? >> >> So no, it is not accepted. >> > > Where are the problems? > Be more specific please.

Read my first email on the patch. That is as specific as it gets.

I'll do this. But I don't know whether this changes the my opinion about the fixed problem.

...

>>>> Please figure out how to set up a local Git repository, how to set up a branch, how to commit things and what rules there are to follow. Then find out on how to submit a patchset to the mailing list - after it has been tested. It is all in the link that I sent you. >>>> >>>> This is not a competition about who can submit patches the fastest. >>>> >>> >>> 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 relevant for this single case. I also stated, that I'll surely use git for greater modifications 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 topic by Matthias. >>> >>>>> I found a couple of behaviours of this page, which are not obvious or straight forward ( adding dynamic leases to static leases, for example, maybe this is invoked now by my modification). >>>> >>>> What are those? Why are we not talking about those first and then come up with fixes? The whole DHCP page is a mess. I am not sure if it can even be fixed or of things won’t just become worse. >>>> >>> >>> Being an experienced software developer, I think this is possible and I am just working on this. I'll discuss these topics in the list before sending patches, being aware this is a community project this many opinions. >> >> This is not necessarily about opinions. It is about what we can support in the end and where we all want to invest our time. >> > > Helping users with known problems, which could be resolved, is wating time also. In the moment I do this mainly. > >> Think about that we are all in the same boat and we want to improve IPFire wherever we can. But we need to talk about things because one set of eyes often is not enough. Something that works for one person does not work for another. We can never break backwards-compatibility. >> >> I personally want to see this static lease bug fixed. I am trying to help you to develop a good solution that we do not have to worry about in the future. I am also enforcing the rules that we all have come up with some long time ago and that work for this project. That way we will hopefully all benefit from this. >> > > Why didn't you just say this. A request for the whole file would have been enough. BTW the file can be found in the forum post.

There are problems *in* the file. I raised questions.

see above.

...

>> But I do not want to have endless discussions on this list about why the rules are as they are. If there is a constructive proposal to make things easier for all then we are all of course open for this. On the other hand debating what is the standard now is just a waste of time. I am happy to explain this, but I am not willing to debate them or compromise on this. Again, this is a general statement and not limited to this conversation. >> > > You started this discussion ( once more ).

Okay, I tried to be helpful here.

If you prefer to insist that this patch is not being amended and if you also prefer to point fingers, keep doing it.

I will remember this and keep my comments to myself in the future then.

There is no need to behave like this. I'll be quiet myself upon source quality and proposed fixes for bugs.

...

>>>>> 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. >>>> >>>> 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 term. >>>> >>> >>> Agreed, too. But again, this special problem appears from time to time in the forums. >> >> Why do we not have a ticket on BZ then? >> > > The ticket is there ( see subject of this discussion ) and the solution exists.

I disagree with this “solution”.

Why?

-Bernhard

...

> > -Bernhard > >>> 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 reason for the patch. The amount of modified code isn't so big to demand the canonical development process, IMHO. The patch can be applied by any core developper reading DevList/bugzilla/forums regulary. >> >> It is entirely up to you how you develop your code. How it is submitted to the list is clear. >> >> -Michael >> >>> >>> -Bernhard >>> >>>> -Michael >>>> >>>>> >>>>> Regards, >>>>> Bernhard

Hi,

On 18 May 2019, at 19:28, Bernhard Bitsch Bernhard.Bitsch@gmx.de wrote:

Hi,

...

Gesendet: Samstag, 18. Mai 2019 um 00:27 Uhr Von: "Michael Tremer" michael.tremer@ipfire.org An: "Bernhard Bitsch" Bernhard.Bitsch@gmx.de Cc: "IPFire: Development-List" development@lists.ipfire.org, florian.buehrle@ipfire.org Betreff: Re: [PATCH] Fix for Bug #12050: Adding fixed leases with one 'add' click

Hi,

...

On 17 May 2019, at 23:21, Bernhard Bitsch Bernhard.Bitsch@gmx.de wrote:

Hi,

sorry if I were not exact enough. I am working with a matter of urgency on the DHCP config problem and surely will contact Florian, if I've found the error and a real fix for it.

No need to contact him directly. That is what this list is for.

That is not he point. Maybe it is better to work directly together.

I doubt that. This is an Open Source project, we do things “in the open”.

https://blog.ipfire.org/post/default-to-open-please

...

...

My post was just a heads up about this work. The rewriting will be the second step, after bug elimination. The purpose should be to faciliate future bug fixes.

I do not think that it makes any sense to rewrite this CGI file. We are going to get rid of the whole web UI in IPFire 3 and there are no new features to expect in the DHCP area that it makes sense to tidy up to much. I am not advocating to have messy code here, but it works, and it easily breaks if you touch something.

When will IPFire 3 come?

That is unknown. But every minute spend on IPFire 2 will delay it by another minute.

How should we find and correct errors in the meantime? About breaking by touching this messy code, I agree with you. But if the mess remains, the probability of importing new errors by "bug fixes" increases. Just my opinion.

...

...

My idea for quick inclusion of fix: Florian is working on it, therefore I'll discuss possible solutions with him. He will commit these to the git repo, based on the actual dhcp.cgi file.

Why don’t you say it here what you have found?

Because I haven't found the error, yet.

...

...

When I am finished with the rewrite (including the error fix) I'll publish it either here in the devel list or in git or both. Is this ok?

That really depends on what you want to achieve here. If you are sending in patches that clean up code but do not change any behaviour you should collect them in a git branch and send that branch as a patchset. Bug fixes should be sent separately if it makes sense.

Okay, that is the way I thought of. At the moment Florian fixes the actual error in the existing code. Independend of that I represent the rewrite in git and/or devel list.

...

If you plan to change any behaviour of the CGI file, that is a matter open for discussion first and then work should start.

When is this discussed? I made a suggestion of changes of behaviour; yet:

• Adding a new fixed lease adds this directly, without having to click a second time.

That is already the case.

• Adding a dynamic lease to the fixed leases should work in two steps: first the data from dynamic leases is copied to the edit fields, user can change and complete the definition and adds this by clicking "add". A check for disjunction of sets of fixed and dynamic leases would be possible.

If you add it from the DHCP leases list, the static lease is meant to be added right away, but you can still edit it to give it a better name or so. This what currently seems to be broken.

So you are not talking about changing any behaviour. This *is* the bug we are talking about all along.

-Michael

• Bernhard

...

Hope this makes sense.

-Michael

...

Best, Bernhard

...

Gesendet: Freitag, 17. Mai 2019 um 21:18 Uhr Von: "Michael Tremer" michael.tremer@ipfire.org An: "Bernhard Bitsch" Bernhard.Bitsch@gmx.de Cc: "IPFire: Development-List" development@lists.ipfire.org, florian.buehrle@ipfire.org Betreff: Re: [PATCH] Fix for Bug #12050: Adding fixed leases with one 'add' click

Hi,

This bug is currently assigned to Florian, because I have asked him to have a look at it.

I do not really care who is working on this, but I would like everyone to work together on it.

I would also like to stress that we have urgent fixes for loads of Intel processors in Core Update 132 and I think that this problem should also be fixed in this update. So, please work on this with a matter of urgency.

Best, -Michael

...

On 17 May 2019, at 11:58, Bernhard Bitsch Bernhard.Bitsch@gmx.de wrote:

Hello,

just some news about this topic.

• Michael was right to refuse my quick and dirty patch. It did not really solve the problem, maybe it introduced some new problems. Sorry for my quick and dirty reaction.

• I wasn't satisfied with the situation being. The problem exists furthermore and isn't easy solved. Therefore I started a review and commenting for my own. With some effort I think I've located the main error ( see my short post in bugzilla ).

Thus, you will read again from me about solutions for Bug #12050. My current state is as follows: I've added a bunch of comments for understanding the program. The sort algorithm for fixed leases ( maybe dynamic leases also ) should and can be optimised. The operations "add new fixed lease", "edit existing fixed lease", "add dynamic lease to fixed leases" must be verified and corrected.

Proposal for behaviour: "add new fixed lease" : add a new entry with parameters to fixed leases list, sort according the existing order. It should not be necessary to edit it first. "edit existing fixed lease": move parameters of selected entry to edit box. Highlight edited entry at his place ( if entry #12 should be edited, row #12 is highlighted ). Change entry with new paramters, sort. "add dynamic lease to fixed lease": move values of dynamic lease to edit box. Editing is mandatory! We should not merge sets of fixed and dynamic leases. Add new entry as new fixed lease with changed parameters, sort.

When I've succeeded in implementing these topics, I'll post the result. Because of the number of changes in source ( comments, enhancements for readability, .... ) I would suggest a commit of approved file as a whole, not just as single little patches. How can this be accomplished? I think little patches here and there cannot solve the problem of low maintainability of this file.

Regards, Bernhard

...

Gesendet: Samstag, 20. April 2019 um 18:35 Uhr Von: "Michael Tremer" michael.tremer@ipfire.org An: "Bernhard Bitsch" Bernhard.Bitsch@gmx.de Cc: "IPFire: Development-List" development@lists.ipfire.org Betreff: Re: [PATCH] Fix for Bug #12050: Adding fixed leases with one 'add' click

Hello,

I just wanted to share with the list that Bernhard has emailed me in private, telling me that he no longer wants to pursue getting this patch merged.

I find this whole situation very frustrating, but of course I accept his decision.

I guess we just have to agree that we disagree here.

Best, -Michael

> On 18 Apr 2019, at 21:37, Bernhard Bitsch Bernhard.Bitsch@gmx.de wrote: > > > >> Gesendet: Donnerstag, 18. April 2019 um 16:47 Uhr >> Von: "Michael Tremer" michael.tremer@ipfire.org >> An: "Bernhard Bitsch" Bernhard.Bitsch@gmx.de >> Cc: "IPFire: Development-List" development@lists.ipfire.org >> Betreff: Re: [PATCH] Fix for Bug #12050: Adding fixed leases with one 'add' click >> >> Hi, >> >>> On 18 Apr 2019, at 13:54, Bernhard Bitsch Bernhard.Bitsch@gmx.de wrote: >>> >>> Hello, >>> >>> I do not want to start any discussions about the way the project does its work. >>> Therefore some (hopefully) short annotations below. >>> >>>> Gesendet: Donnerstag, 18. April 2019 um 13:42 Uhr >>>> Von: "Michael Tremer" michael.tremer@ipfire.org >>>> An: "Bernhard Bitsch" Bernhard.Bitsch@gmx.de >>>> Cc: "IPFire: Development-List" development@lists.ipfire.org >>>> Betreff: Re: [PATCH] Fix for Bug #12050: Adding fixed leases with one 'add' click >>>> >>>> Hello, >>>> >>>>> On 18 Apr 2019, at 12:23, Bernhard Bitsch Bernhard.Bitsch@gmx.de wrote: >>>>> >>>>> Hi, >>>>> >>>>>> Gesendet: Donnerstag, 18. April 2019 um 11:50 Uhr >>>>>> Von: "Michael Tremer" michael.tremer@ipfire.org >>>>>> An: "Bernhard Bitsch" Bernhard.Bitsch@gmx.de >>>>>> Cc: BeBiMa bbitsch@ipfire.org, "IPFire: Development-List" development@lists.ipfire.org >>>>>> Betreff: Re: [PATCH] Fix for Bug #12050: Adding fixed leases with one 'add' click >>>>>> >>>>>> Hi, >>>>>> >>>>>>> On 17 Apr 2019, at 22:49, Bernhard Bitsch Bernhard.Bitsch@gmx.de wrote: >>>>>>> >>>>>>> Hi, >>>>>>> some explanations from the author: >>>>>>> >>>>>>>> Gesendet: Mittwoch, 17. April 2019 um 11:31 Uhr >>>>>>>> Von: "Michael Tremer" michael.tremer@ipfire.org >>>>>>>> An: "Matthias Fischer" matthias.fischer@ipfire.org >>>>>>>> Cc: development@lists.ipfire.org, BeBiMa bbitsch@ipfire.org >>>>>>>> Betreff: Re: [PATCH] Fix for Bug #12050: Adding fixed leases with one 'add' click >>>>>>>> >>>>>>>> Hi, >>>>>>>> >>>>>>>> Thanks Matthias for helping out here. However, I do not get the patch. >>>>>>>> >>>>>>>> There is no explanation about what it is meant to do. The intention already is that the lease is added in the first place. >>>>>>>> >>>>>>> >>>>>>> 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. >>>>>> >>>>>> Those comments *must* be in the code. Nobody goes through thousands of emails on a mailing list to find out what is actually intended in the code. >>>>>> >>>>> >>>>> 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 case I regarded the forum post and the bugzilla entry to be sufficient documentation, for the moment. >>>> >>>> No. >>>> >>>> Every commit must contain itself. >>>> >>>>>>> >>>>>>>>> On 16 Apr 2019, at 17:41, Matthias Fischer matthias.fischer@ipfire.org wrote: >>>>>>>>> >>>>>>>>> Signed-off-by: BeBiMa bbitsch@ipfire.org >>>>>>>>> Reviewed-by: Matthias Fischer matthias.fischer@ipfire.org >>>>>>>>> --- >>>>>>>>> html/cgi-bin/dhcp.cgi | 12 ++++++++++++ >>>>>>>>> 1 file changed, 12 insertions(+) >>>>>>>>> >>>>>>>>> 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') { >>>>>>>>> } >>>>>>>>> >>>>>>>>> my $key = 0; >>>>>>>>> + my $szc = scalar(@current2); >>>>>>>>> CHECK:foreach my $line (@current2) { >>>>>>>>> my @temp = split(/,/,$line); >>>>>>>>> if($dhcpsettings{'KEY2'} ne $key) { >>>>>>>>> # same MAC is OK on different subnets. This test is not complete because >>>>>>>>> # if ip are not inside a known subnet, I don't warn. >>>>>>>>> # Also it may be needed to put duplicate fixed lease in their right subnet definition.. >>>>>>>>> + if ((lc($dhcpsettings{'FIX_MAC'}) eq lc($temp[0])) &&(lc($dhcpsettings{'FIX_ADDR'}) eq lc($temp[1]))) { >>>>>>>>> + last CHECK; >>>>>>>>> + } >>>>>>>> >>>>>>>> Why is this needed? >>>>>>> >>>>>>> Check for existing lease. If <MAC,IP> is defined already we don't need to loop further. >>>>>>> >>>>>>>> >>>>>>>>> foreach my $itf (@ITFs) { >>>>>>>>> my $scoped = &General::IpInSubnet($dhcpsettings{'FIX_ADDR'}, >>>>>>>>> $netsettings{"${itf}_NETADDRESS"}, >>>>>>>>> @@ -442,11 +446,19 @@ if ($dhcpsettings{'ACTION'} eq $Lang::tr{'add'}.'2') { >>>>>>>>> $dhcpsettings{'FIX_FILENAME'} = &Header::cleanhtml($dhcpsettings{'FIX_FILENAME'}); >>>>>>>>> $dhcpsettings{'FIX_ROOTPATH'} = &Header::cleanhtml($dhcpsettings{'FIX_ROOTPATH'}); >>>>>>>>> if ($dhcpsettings{'KEY2'} eq '') { #add or edit ? >>>>>>>> >>>>>>>> This block here is not indented correctly. >>>>>>>> >>>>>>>> I understand that the code is already very messy, but we should not make it worse either. >>>>>>>> >>>>>>> How should indention be done? By spaces, tabs? How many colums define an indent? >>>>>> >>>>>> In the patch it looks like you wrapped the block into another if condition. So it has to be indented to make that obvious. >>>>>> >>>>> >>>>> 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. >>>> >>>> Do *not* submit non-final versions. There is no point in it. A patch is meant to go into a final release. We do not want beta stuff in there. >>>> >>> >>> Okay. Got it. I should have sent my modified dhcp.cgi because of lack of a working git repo on my site. >> >> Please learn how to use git and use “git send-email”. There is an introduction on the wiki on how to set it up and there is tons of resources on the Internet that explain Git to you in the form of howtos and videos. >> > > Sorry, I know about git. It is not the lack of knowledge, but my limited equipment. You invite to donate for the project, which is very urgent, I think. On the other hand I've tried to help with limited sources and without monetary assistance, till now. If this isn't possible, it is okay for me. I'll retire to the status "consuming user". > >>>>> >>>>>>> The original file (and many ohters!) have an mixture of tab/space. Should we patch that step by step ( tab=4 ) to increase readability. >>>>>> >>>>>> This is not about tabs or spaces. >>>>>> >>>>> >>>>> Sure! This mixture doesnt't matter only in case of "tab=4" definition, which 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 necessary to understand the code working on. >>>> >>>> If you insist to take this conversation down this route, then feel free to do so. We do not have a policy that commands spaces. >>>> >>>> I liked you the coding style. >>>> >>> >>> Especially in this file I found a messy mixture of tabs and spaces. Therefore my statement. If we commit us to tab=4 these spaces can be eliminated. >> >> We inherited some code from IPCop that we did not clean up. >> >> So the guideline is to use the coding style used in the particular file. Do not re-indent the whole file. >> > > If that's the opinion of the majority of the developers, live with it. One reason of the ending of IPCop development was this rigid interpretation of "don't touch a running system", IMHO. > > >>>>> >>>>>>> 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. >>>>>> >>>>>> Please just use “git diff”. It has a variety of settings that are just right. >>>>>> >>>>> >>>>> 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 arbitrary files with "git diff". Sorry. >>>> >>>> Of course you would have the files in your working system. How else would you test? >>>> >>>> But for development purposes git is being used. It is the standard. Otherwise patches won’t apply. There is no point in sending patches that other developers cannot apply. Use Git. >>>> >>> >>> Ok. If I don't have a git repo myself, I send the modified source. Each developer with an actual git repo can apply it by commit. Right? >> >> Please learn how to use git. >> >> Of course you could send modified files around, but that does not work. Of course you could send instructions to other people on how to change files, but that doesn’t work either. >> >> I personally won’t do any work of trying to apply any patches that are send in other forms. There is also reasons that we send patches inline: We can comment on them. >> >> Please learn how to use git. >> > > All said about this above. > >>>>> >>>>>>> This generated a short diff without real corrections only. >>>>>>> >>>>>>>>> + if($key == $szc) { #add >>>>>>>>> + @current2[$key] = "$dhcpsettings{'FIX_MAC'},$dhcpsettings{'FIX_ADDR'},$dhcpsettings{'FIX_ENABLED'},$dhcpsettings{'FIX_NEXTADDR'},$dhcpsettings{'FIX_FILENAME'},$dhcpsettings{'FIX_ROOTPATH'},$dhcpsettings{'FIX_REMARK'}\n"; >>>>>>>>> + # sort newly added/modified entry >>>>>>>>> + &sortcurrent2; >>>>>>>> >>>>>>>> Are you sure we can sort here? >>>>>>>> >>>>>>>> See: https://git.ipfire.org/?p=ipfire-2.x.git;a=commitdiff;h=31672dc8bdb223ebf425... >>>>>>>> >>>>>>> >>>>>>> Yes! Why not? >>>>>> >>>>>> Because of the commit I referred to. >>>>> >>>>> Didn't understand this commit, because of lack of commentation. ;) >>>> >>>> And you didn’t ask any questions then? >>>> >>> >>> Ok. I ask you now. Why doesn't it function? >> >> Well, as the patch there states the key is saved and used later. When the leases file is sorted, the key changes but is not updated. That lead to the case that you edited a different lease than you wanted. >> >> I did not test this, but I could imagine that this could happen here, too. >> > > I know from reading and understandig the code, that this works. The approval by Matthias Fischer shows, that I'm right. See the related forum thread. > Software development and code review is no field of imagination. > >>>>>> >>>>>>> >>>>>>>>> + &General::log($Lang::tr{'fixed ip lease added'}); >>>>>>>>> + $dhcpsettings{'KEY2'} = ''; >>>>>>>>> + } else { #edit >>>>>>>>> unshift (@current2, "$dhcpsettings{'FIX_MAC'},$dhcpsettings{'FIX_ADDR'},$dhcpsettings{'FIX_ENABLED'},$dhcpsettings{'FIX_NEXTADDR'},$dhcpsettings{'FIX_FILENAME'},$dhcpsettings{'FIX_ROOTPATH'},$dhcpsettings{'FIX_REMARK'}\n"); >>>>>>>>> &General::log($Lang::tr{'fixed ip lease added'}); >>>>>>>>> >>>>>>>>> # Enter edit mode >>>>>>>>> $dhcpsettings{'KEY2'} = 0; >>>>>>>>> + } >>>>>>>>> } else { >>>>>>>>> @current2[$dhcpsettings{'KEY2'}] = "$dhcpsettings{'FIX_MAC'},$dhcpsettings{'FIX_ADDR'},$dhcpsettings{'FIX_ENABLED'},$dhcpsettings{'FIX_NEXTADDR'},$dhcpsettings{'FIX_FILENAME'},$dhcpsettings{'FIX_ROOTPATH'},$dhcpsettings{'FIX_REMARK'}\n"; >>>>>>>>> $dhcpsettings{'KEY2'} = ''; # End edit mode >>>>>>>>> -- >>>>>>>>> 2.18.0 >>>>>>>>> >>>>>>>> >>>>>>>> -Michael >>>>>>>> >>>>>>>> >>>>>>> >>>>>>> Again, my intention was a quick resolution for the behaviour mentioned in the forum post, which initiated my code review of dhcpi.cgi >>>>>> >>>>>> Okay, I will try to explain this one last time - not just only for you, but generally: >>>>>> >>>>>> I am not interested in quick and dirty solutions. That is how you break things. I am interested in well-documented, peer-reviewed and tested code. We are creating some piece of high-quality software - or aim to do so at least - and 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. >>>>> I agree fully with your aim, but this means readability also! This means some ( many? ) 'cosmetic' changes in the future, IMHO. Is this accepted? >>>> >>>> You cannot submit a patch that does not fulfil the guidelines and then promise to fix it later. There are obvious problems with the patch and there are open questions. Why would we merge something that clearly adds more problems than it solves? >>>> >>>> So no, it is not accepted. >>>> >>> >>> Where are the problems? >>> Be more specific please. >> >> Read my first email on the patch. That is as specific as it gets. >> > > I'll do this. But I don't know whether this changes the my opinion about the fixed problem. > >>>>>> Please figure out how to set up a local Git repository, how to set up a branch, how to commit things and what rules there are to follow. Then find out on how to submit a patchset to the mailing list - after it has been tested. It is all in the link that I sent you. >>>>>> >>>>>> This is not a competition about who can submit patches the fastest. >>>>>> >>>>> >>>>> 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 relevant for this single case. I also stated, that I'll surely use git for greater modifications 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 topic by Matthias. >>>>> >>>>>>> I found a couple of behaviours of this page, which are not obvious or straight forward ( adding dynamic leases to static leases, for example, maybe this is invoked now by my modification). >>>>>> >>>>>> What are those? Why are we not talking about those first and then come up with fixes? The whole DHCP page is a mess. I am not sure if it can even be fixed or of things won’t just become worse. >>>>>> >>>>> >>>>> Being an experienced software developer, I think this is possible and I am just working on this. I'll discuss these topics in the list before sending patches, being aware this is a community project this many opinions. >>>> >>>> This is not necessarily about opinions. It is about what we can support in the end and where we all want to invest our time. >>>> >>> >>> Helping users with known problems, which could be resolved, is wating time also. In the moment I do this mainly. >>> >>>> Think about that we are all in the same boat and we want to improve IPFire wherever we can. But we need to talk about things because one set of eyes often is not enough. Something that works for one person does not work for another. We can never break backwards-compatibility. >>>> >>>> I personally want to see this static lease bug fixed. I am trying to help you to develop a good solution that we do not have to worry about in the future. I am also enforcing the rules that we all have come up with some long time ago and that work for this project. That way we will hopefully all benefit from this. >>>> >>> >>> Why didn't you just say this. A request for the whole file would have been enough. BTW the file can be found in the forum post. >> >> There are problems *in* the file. I raised questions. >> > > see above. > >>>> But I do not want to have endless discussions on this list about why the rules are as they are. If there is a constructive proposal to make things easier for all then we are all of course open for this. On the other hand debating what is the standard now is just a waste of time. I am happy to explain this, but I am not willing to debate them or compromise on this. Again, this is a general statement and not limited to this conversation. >>>> >>> >>> You started this discussion ( once more ). >> >> Okay, I tried to be helpful here. >> >> If you prefer to insist that this patch is not being amended and if you also prefer to point fingers, keep doing it. >> >> I will remember this and keep my comments to myself in the future then. >> > > There is no need to behave like this. I'll be quiet myself upon source quality and proposed fixes for bugs. > >>>>>>> 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. >>>>>> >>>>>> 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 term. >>>>>> >>>>> >>>>> Agreed, too. But again, this special problem appears from time to time in the forums. >>>> >>>> Why do we not have a ticket on BZ then? >>>> >>> >>> The ticket is there ( see subject of this discussion ) and the solution exists. >> >> I disagree with this “solution”. >> > > Why? > > -Bernhard > >>> >>> -Bernhard >>> >>>>> 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 reason for the patch. The amount of modified code isn't so big to demand the canonical development process, IMHO. The patch can be applied by any core developper reading DevList/bugzilla/forums regulary. >>>> >>>> It is entirely up to you how you develop your code. How it is submitted to the list is clear. >>>> >>>> -Michael >>>> >>>>> >>>>> -Bernhard >>>>> >>>>>> -Michael >>>>>> >>>>>>> >>>>>>> Regards, >>>>>>> Bernhard

Hi,

to come to a short end, I've posted a solution in Bugzilla ( for #12050 only). Hope this helps.

Bernhard

Gesendet: Montag, 20. Mai 2019 um 16:21 Uhr Von: "Bernhard Bitsch" Bernhard.Bitsch@gmx.de An: "Michael Tremer" michael.tremer@ipfire.org Cc: "IPFire: Development-List" development@lists.ipfire.org, florian.buehrle@ipfire.org Betreff: Aw: Re: [PATCH] Fix for Bug #12050: Adding fixed leases with one 'add' click

Hi,

...

Gesendet: Montag, 20. Mai 2019 um 11:37 Uhr Von: "Michael Tremer" michael.tremer@ipfire.org An: "Bernhard Bitsch" Bernhard.Bitsch@gmx.de Cc: "IPFire: Development-List" development@lists.ipfire.org, florian.buehrle@ipfire.org Betreff: Re: [PATCH] Fix for Bug #12050: Adding fixed leases with one 'add' click

Hi,

...

On 18 May 2019, at 19:28, Bernhard Bitsch Bernhard.Bitsch@gmx.de wrote:

Hi,

...

Gesendet: Samstag, 18. Mai 2019 um 00:27 Uhr Von: "Michael Tremer" michael.tremer@ipfire.org An: "Bernhard Bitsch" Bernhard.Bitsch@gmx.de Cc: "IPFire: Development-List" development@lists.ipfire.org, florian.buehrle@ipfire.org Betreff: Re: [PATCH] Fix for Bug #12050: Adding fixed leases with one 'add' click

Hi,

...

On 17 May 2019, at 23:21, Bernhard Bitsch Bernhard.Bitsch@gmx.de wrote:

Hi,

sorry if I were not exact enough. I am working with a matter of urgency on the DHCP config problem and surely will contact Florian, if I've found the error and a real fix for it.

No need to contact him directly. That is what this list is for.

That is not he point. Maybe it is better to work directly together.

I doubt that. This is an Open Source project, we do things “in the open”.

https://blog.ipfire.org/post/default-to-open-please

Agreed! But I would prefere to discuss in the list mainly. Some things can be cleared in IIRC quicker, okay. But then the solution should appear in the list. This means, I do not have to check generally two communication channels to be uptodate.

...

...

...

...

My post was just a heads up about this work. The rewriting will be the second step, after bug elimination. The purpose should be to faciliate future bug fixes.

I do not think that it makes any sense to rewrite this CGI file. We are going to get rid of the whole web UI in IPFire 3 and there are no new features to expect in the DHCP area that it makes sense to tidy up to much. I am not advocating to have messy code here, but it works, and it easily breaks if you touch something.

When will IPFire 3 come?

That is unknown. But every minute spend on IPFire 2 will delay it by another minute.

...

How should we find and correct errors in the meantime? About breaking by touching this messy code, I agree with you. But if the mess remains, the probability of importing new errors by "bug fixes" increases. Just my opinion.

...

...

My idea for quick inclusion of fix: Florian is working on it, therefore I'll discuss possible solutions with him. He will commit these to the git repo, based on the actual dhcp.cgi file.

Why don’t you say it here what you have found?

Because I haven't found the error, yet.

...

...

When I am finished with the rewrite (including the error fix) I'll publish it either here in the devel list or in git or both. Is this ok?

That really depends on what you want to achieve here. If you are sending in patches that clean up code but do not change any behaviour you should collect them in a git branch and send that branch as a patchset. Bug fixes should be sent separately if it makes sense.

Okay, that is the way I thought of. At the moment Florian fixes the actual error in the existing code. Independend of that I represent the rewrite in git and/or devel list.

...

If you plan to change any behaviour of the CGI file, that is a matter open for discussion first and then work should start.

When is this discussed? I made a suggestion of changes of behaviour; yet:

• Adding a new fixed lease adds this directly, without having to click a second time.

That is already the case.

That's not true! Adding a new entry must retain the existing entries, what isn't the case ( see postings in forum ).

...

...

• Adding a dynamic lease to the fixed leases should work in two steps: first the data from dynamic leases is copied to the edit fields, user can change and complete the definition and adds this by clicking "add". A check for disjunction of sets of fixed and dynamic leases would be possible.

If you add it from the DHCP leases list, the static lease is meant to be added right away, but you can still edit it to give it a better name or so. This what currently seems to be broken.

Maybe this the intention of the actual implementation. The misfunction lays in the bug, indeed. But my suggestion goes one step further. It isn't desirable to mix up static and dynamic leases. This is based on the mechanics, how dynamic leases are found by dhcpd ( see man page ). IMHO, the problem with this is not shining up immediately, but some times later ( with no modifications meantime ). A two step process with check if the two sets are disjoint avoids this problem.

-Bernhard

...

So you are not talking about changing any behaviour. This *is* the bug we are talking about all along.

-Michael

...

• Bernhard

...

Hope this makes sense.

-Michael

...

Best, Bernhard

...

Gesendet: Freitag, 17. Mai 2019 um 21:18 Uhr Von: "Michael Tremer" michael.tremer@ipfire.org An: "Bernhard Bitsch" Bernhard.Bitsch@gmx.de Cc: "IPFire: Development-List" development@lists.ipfire.org, florian.buehrle@ipfire.org Betreff: Re: [PATCH] Fix for Bug #12050: Adding fixed leases with one 'add' click

Hi,

This bug is currently assigned to Florian, because I have asked him to have a look at it.

I do not really care who is working on this, but I would like everyone to work together on it.

I would also like to stress that we have urgent fixes for loads of Intel processors in Core Update 132 and I think that this problem should also be fixed in this update. So, please work on this with a matter of urgency.

Best, -Michael

> On 17 May 2019, at 11:58, Bernhard Bitsch Bernhard.Bitsch@gmx.de wrote: > > Hello, > > just some news about this topic. > > - Michael was right to refuse my quick and dirty patch. It did not really solve the problem, maybe it introduced some new problems. Sorry for my quick and dirty reaction. > > - I wasn't satisfied with the situation being. The problem exists furthermore and isn't easy solved. Therefore I started a review and commenting for my own. With some effort I think I've located the main error ( see my short post in bugzilla ). > > Thus, you will read again from me about solutions for Bug #12050. > My current state is as follows: > I've added a bunch of comments for understanding the program. > The sort algorithm for fixed leases ( maybe dynamic leases also ) should and can be optimised. > The operations "add new fixed lease", "edit existing fixed lease", "add dynamic lease to fixed leases" must be verified and corrected. > > Proposal for behaviour: > "add new fixed lease" : add a new entry with parameters to fixed leases list, sort according the existing order. It should not be necessary to edit it first. > "edit existing fixed lease": move parameters of selected entry to edit box. Highlight edited entry at his place ( if entry #12 should be edited, row #12 is highlighted ). Change entry with new paramters, sort. > "add dynamic lease to fixed lease": move values of dynamic lease to edit box. Editing is mandatory! We should not merge sets of fixed and dynamic leases. Add new entry as new fixed lease with changed parameters, sort. > > When I've succeeded in implementing these topics, I'll post the result. > Because of the number of changes in source ( comments, enhancements for readability, .... ) I would suggest a commit of approved file as a whole, not just as single little patches. How can this be accomplished? > I think little patches here and there cannot solve the problem of low maintainability of this file. > > Regards, > Bernhard > >> Gesendet: Samstag, 20. April 2019 um 18:35 Uhr >> Von: "Michael Tremer" michael.tremer@ipfire.org >> An: "Bernhard Bitsch" Bernhard.Bitsch@gmx.de >> Cc: "IPFire: Development-List" development@lists.ipfire.org >> Betreff: Re: [PATCH] Fix for Bug #12050: Adding fixed leases with one 'add' click >> >> Hello, >> >> I just wanted to share with the list that Bernhard has emailed me in private, telling me that he no longer wants to pursue getting this patch merged. >> >> I find this whole situation very frustrating, but of course I accept his decision. >> >> I guess we just have to agree that we disagree here. >> >> Best, >> -Michael >> >>> On 18 Apr 2019, at 21:37, Bernhard Bitsch Bernhard.Bitsch@gmx.de wrote: >>> >>> >>> >>>> Gesendet: Donnerstag, 18. April 2019 um 16:47 Uhr >>>> Von: "Michael Tremer" michael.tremer@ipfire.org >>>> An: "Bernhard Bitsch" Bernhard.Bitsch@gmx.de >>>> Cc: "IPFire: Development-List" development@lists.ipfire.org >>>> Betreff: Re: [PATCH] Fix for Bug #12050: Adding fixed leases with one 'add' click >>>> >>>> Hi, >>>> >>>>> On 18 Apr 2019, at 13:54, Bernhard Bitsch Bernhard.Bitsch@gmx.de wrote: >>>>> >>>>> Hello, >>>>> >>>>> I do not want to start any discussions about the way the project does its work. >>>>> Therefore some (hopefully) short annotations below. >>>>> >>>>>> Gesendet: Donnerstag, 18. April 2019 um 13:42 Uhr >>>>>> Von: "Michael Tremer" michael.tremer@ipfire.org >>>>>> An: "Bernhard Bitsch" Bernhard.Bitsch@gmx.de >>>>>> Cc: "IPFire: Development-List" development@lists.ipfire.org >>>>>> Betreff: Re: [PATCH] Fix for Bug #12050: Adding fixed leases with one 'add' click >>>>>> >>>>>> Hello, >>>>>> >>>>>>> On 18 Apr 2019, at 12:23, Bernhard Bitsch Bernhard.Bitsch@gmx.de wrote: >>>>>>> >>>>>>> Hi, >>>>>>> >>>>>>>> Gesendet: Donnerstag, 18. April 2019 um 11:50 Uhr >>>>>>>> Von: "Michael Tremer" michael.tremer@ipfire.org >>>>>>>> An: "Bernhard Bitsch" Bernhard.Bitsch@gmx.de >>>>>>>> Cc: BeBiMa bbitsch@ipfire.org, "IPFire: Development-List" development@lists.ipfire.org >>>>>>>> Betreff: Re: [PATCH] Fix for Bug #12050: Adding fixed leases with one 'add' click >>>>>>>> >>>>>>>> Hi, >>>>>>>> >>>>>>>>> On 17 Apr 2019, at 22:49, Bernhard Bitsch Bernhard.Bitsch@gmx.de wrote: >>>>>>>>> >>>>>>>>> Hi, >>>>>>>>> some explanations from the author: >>>>>>>>> >>>>>>>>>> Gesendet: Mittwoch, 17. April 2019 um 11:31 Uhr >>>>>>>>>> Von: "Michael Tremer" michael.tremer@ipfire.org >>>>>>>>>> An: "Matthias Fischer" matthias.fischer@ipfire.org >>>>>>>>>> Cc: development@lists.ipfire.org, BeBiMa bbitsch@ipfire.org >>>>>>>>>> Betreff: Re: [PATCH] Fix for Bug #12050: Adding fixed leases with one 'add' click >>>>>>>>>> >>>>>>>>>> Hi, >>>>>>>>>> >>>>>>>>>> Thanks Matthias for helping out here. However, I do not get the patch. >>>>>>>>>> >>>>>>>>>> There is no explanation about what it is meant to do. The intention already is that the lease is added in the first place. >>>>>>>>>> >>>>>>>>> >>>>>>>>> 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. >>>>>>>> >>>>>>>> Those comments *must* be in the code. Nobody goes through thousands of emails on a mailing list to find out what is actually intended in the code. >>>>>>>> >>>>>>> >>>>>>> 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 case I regarded the forum post and the bugzilla entry to be sufficient documentation, for the moment. >>>>>> >>>>>> No. >>>>>> >>>>>> Every commit must contain itself. >>>>>> >>>>>>>>> >>>>>>>>>>> On 16 Apr 2019, at 17:41, Matthias Fischer matthias.fischer@ipfire.org wrote: >>>>>>>>>>> >>>>>>>>>>> Signed-off-by: BeBiMa bbitsch@ipfire.org >>>>>>>>>>> Reviewed-by: Matthias Fischer matthias.fischer@ipfire.org >>>>>>>>>>> --- >>>>>>>>>>> html/cgi-bin/dhcp.cgi | 12 ++++++++++++ >>>>>>>>>>> 1 file changed, 12 insertions(+) >>>>>>>>>>> >>>>>>>>>>> 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') { >>>>>>>>>>> } >>>>>>>>>>> >>>>>>>>>>> my $key = 0; >>>>>>>>>>> + my $szc = scalar(@current2); >>>>>>>>>>> CHECK:foreach my $line (@current2) { >>>>>>>>>>> my @temp = split(/,/,$line); >>>>>>>>>>> if($dhcpsettings{'KEY2'} ne $key) { >>>>>>>>>>> # same MAC is OK on different subnets. This test is not complete because >>>>>>>>>>> # if ip are not inside a known subnet, I don't warn. >>>>>>>>>>> # Also it may be needed to put duplicate fixed lease in their right subnet definition.. >>>>>>>>>>> + if ((lc($dhcpsettings{'FIX_MAC'}) eq lc($temp[0])) &&(lc($dhcpsettings{'FIX_ADDR'}) eq lc($temp[1]))) { >>>>>>>>>>> + last CHECK; >>>>>>>>>>> + } >>>>>>>>>> >>>>>>>>>> Why is this needed? >>>>>>>>> >>>>>>>>> Check for existing lease. If <MAC,IP> is defined already we don't need to loop further. >>>>>>>>> >>>>>>>>>> >>>>>>>>>>> foreach my $itf (@ITFs) { >>>>>>>>>>> my $scoped = &General::IpInSubnet($dhcpsettings{'FIX_ADDR'}, >>>>>>>>>>> $netsettings{"${itf}_NETADDRESS"}, >>>>>>>>>>> @@ -442,11 +446,19 @@ if ($dhcpsettings{'ACTION'} eq $Lang::tr{'add'}.'2') { >>>>>>>>>>> $dhcpsettings{'FIX_FILENAME'} = &Header::cleanhtml($dhcpsettings{'FIX_FILENAME'}); >>>>>>>>>>> $dhcpsettings{'FIX_ROOTPATH'} = &Header::cleanhtml($dhcpsettings{'FIX_ROOTPATH'}); >>>>>>>>>>> if ($dhcpsettings{'KEY2'} eq '') { #add or edit ? >>>>>>>>>> >>>>>>>>>> This block here is not indented correctly. >>>>>>>>>> >>>>>>>>>> I understand that the code is already very messy, but we should not make it worse either. >>>>>>>>>> >>>>>>>>> How should indention be done? By spaces, tabs? How many colums define an indent? >>>>>>>> >>>>>>>> In the patch it looks like you wrapped the block into another if condition. So it has to be indented to make that obvious. >>>>>>>> >>>>>>> >>>>>>> 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. >>>>>> >>>>>> Do *not* submit non-final versions. There is no point in it. A patch is meant to go into a final release. We do not want beta stuff in there. >>>>>> >>>>> >>>>> Okay. Got it. I should have sent my modified dhcp.cgi because of lack of a working git repo on my site. >>>> >>>> Please learn how to use git and use “git send-email”. There is an introduction on the wiki on how to set it up and there is tons of resources on the Internet that explain Git to you in the form of howtos and videos. >>>> >>> >>> Sorry, I know about git. It is not the lack of knowledge, but my limited equipment. You invite to donate for the project, which is very urgent, I think. On the other hand I've tried to help with limited sources and without monetary assistance, till now. If this isn't possible, it is okay for me. I'll retire to the status "consuming user". >>> >>>>>>> >>>>>>>>> The original file (and many ohters!) have an mixture of tab/space. Should we patch that step by step ( tab=4 ) to increase readability. >>>>>>>> >>>>>>>> This is not about tabs or spaces. >>>>>>>> >>>>>>> >>>>>>> Sure! This mixture doesnt't matter only in case of "tab=4" definition, which 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 necessary to understand the code working on. >>>>>> >>>>>> If you insist to take this conversation down this route, then feel free to do so. We do not have a policy that commands spaces. >>>>>> >>>>>> I liked you the coding style. >>>>>> >>>>> >>>>> Especially in this file I found a messy mixture of tabs and spaces. Therefore my statement. If we commit us to tab=4 these spaces can be eliminated. >>>> >>>> We inherited some code from IPCop that we did not clean up. >>>> >>>> So the guideline is to use the coding style used in the particular file. Do not re-indent the whole file. >>>> >>> >>> If that's the opinion of the majority of the developers, live with it. One reason of the ending of IPCop development was this rigid interpretation of "don't touch a running system", IMHO. >>> >>> >>>>>>> >>>>>>>>> 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. >>>>>>>> >>>>>>>> Please just use “git diff”. It has a variety of settings that are just right. >>>>>>>> >>>>>>> >>>>>>> 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 arbitrary files with "git diff". Sorry. >>>>>> >>>>>> Of course you would have the files in your working system. How else would you test? >>>>>> >>>>>> But for development purposes git is being used. It is the standard. Otherwise patches won’t apply. There is no point in sending patches that other developers cannot apply. Use Git. >>>>>> >>>>> >>>>> Ok. If I don't have a git repo myself, I send the modified source. Each developer with an actual git repo can apply it by commit. Right? >>>> >>>> Please learn how to use git. >>>> >>>> Of course you could send modified files around, but that does not work. Of course you could send instructions to other people on how to change files, but that doesn’t work either. >>>> >>>> I personally won’t do any work of trying to apply any patches that are send in other forms. There is also reasons that we send patches inline: We can comment on them. >>>> >>>> Please learn how to use git. >>>> >>> >>> All said about this above. >>> >>>>>>> >>>>>>>>> This generated a short diff without real corrections only. >>>>>>>>> >>>>>>>>>>> + if($key == $szc) { #add >>>>>>>>>>> + @current2[$key] = "$dhcpsettings{'FIX_MAC'},$dhcpsettings{'FIX_ADDR'},$dhcpsettings{'FIX_ENABLED'},$dhcpsettings{'FIX_NEXTADDR'},$dhcpsettings{'FIX_FILENAME'},$dhcpsettings{'FIX_ROOTPATH'},$dhcpsettings{'FIX_REMARK'}\n"; >>>>>>>>>>> + # sort newly added/modified entry >>>>>>>>>>> + &sortcurrent2; >>>>>>>>>> >>>>>>>>>> Are you sure we can sort here? >>>>>>>>>> >>>>>>>>>> See: https://git.ipfire.org/?p=ipfire-2.x.git;a=commitdiff;h=31672dc8bdb223ebf425... >>>>>>>>>> >>>>>>>>> >>>>>>>>> Yes! Why not? >>>>>>>> >>>>>>>> Because of the commit I referred to. >>>>>>> >>>>>>> Didn't understand this commit, because of lack of commentation. ;) >>>>>> >>>>>> And you didn’t ask any questions then? >>>>>> >>>>> >>>>> Ok. I ask you now. Why doesn't it function? >>>> >>>> Well, as the patch there states the key is saved and used later. When the leases file is sorted, the key changes but is not updated. That lead to the case that you edited a different lease than you wanted. >>>> >>>> I did not test this, but I could imagine that this could happen here, too. >>>> >>> >>> I know from reading and understandig the code, that this works. The approval by Matthias Fischer shows, that I'm right. See the related forum thread. >>> Software development and code review is no field of imagination. >>> >>>>>>>> >>>>>>>>> >>>>>>>>>>> + &General::log($Lang::tr{'fixed ip lease added'}); >>>>>>>>>>> + $dhcpsettings{'KEY2'} = ''; >>>>>>>>>>> + } else { #edit >>>>>>>>>>> unshift (@current2, "$dhcpsettings{'FIX_MAC'},$dhcpsettings{'FIX_ADDR'},$dhcpsettings{'FIX_ENABLED'},$dhcpsettings{'FIX_NEXTADDR'},$dhcpsettings{'FIX_FILENAME'},$dhcpsettings{'FIX_ROOTPATH'},$dhcpsettings{'FIX_REMARK'}\n"); >>>>>>>>>>> &General::log($Lang::tr{'fixed ip lease added'}); >>>>>>>>>>> >>>>>>>>>>> # Enter edit mode >>>>>>>>>>> $dhcpsettings{'KEY2'} = 0; >>>>>>>>>>> + } >>>>>>>>>>> } else { >>>>>>>>>>> @current2[$dhcpsettings{'KEY2'}] = "$dhcpsettings{'FIX_MAC'},$dhcpsettings{'FIX_ADDR'},$dhcpsettings{'FIX_ENABLED'},$dhcpsettings{'FIX_NEXTADDR'},$dhcpsettings{'FIX_FILENAME'},$dhcpsettings{'FIX_ROOTPATH'},$dhcpsettings{'FIX_REMARK'}\n"; >>>>>>>>>>> $dhcpsettings{'KEY2'} = ''; # End edit mode >>>>>>>>>>> -- >>>>>>>>>>> 2.18.0 >>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> -Michael >>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>>> Again, my intention was a quick resolution for the behaviour mentioned in the forum post, which initiated my code review of dhcpi.cgi >>>>>>>> >>>>>>>> Okay, I will try to explain this one last time - not just only for you, but generally: >>>>>>>> >>>>>>>> I am not interested in quick and dirty solutions. That is how you break things. I am interested in well-documented, peer-reviewed and tested code. We are creating some piece of high-quality software - or aim to do so at least - and 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. >>>>>>> I agree fully with your aim, but this means readability also! This means some ( many? ) 'cosmetic' changes in the future, IMHO. Is this accepted? >>>>>> >>>>>> You cannot submit a patch that does not fulfil the guidelines and then promise to fix it later. There are obvious problems with the patch and there are open questions. Why would we merge something that clearly adds more problems than it solves? >>>>>> >>>>>> So no, it is not accepted. >>>>>> >>>>> >>>>> Where are the problems? >>>>> Be more specific please. >>>> >>>> Read my first email on the patch. That is as specific as it gets. >>>> >>> >>> I'll do this. But I don't know whether this changes the my opinion about the fixed problem. >>> >>>>>>>> Please figure out how to set up a local Git repository, how to set up a branch, how to commit things and what rules there are to follow. Then find out on how to submit a patchset to the mailing list - after it has been tested. It is all in the link that I sent you. >>>>>>>> >>>>>>>> This is not a competition about who can submit patches the fastest. >>>>>>>> >>>>>>> >>>>>>> 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 relevant for this single case. I also stated, that I'll surely use git for greater modifications 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 topic by Matthias. >>>>>>> >>>>>>>>> I found a couple of behaviours of this page, which are not obvious or straight forward ( adding dynamic leases to static leases, for example, maybe this is invoked now by my modification). >>>>>>>> >>>>>>>> What are those? Why are we not talking about those first and then come up with fixes? The whole DHCP page is a mess. I am not sure if it can even be fixed or of things won’t just become worse. >>>>>>>> >>>>>>> >>>>>>> Being an experienced software developer, I think this is possible and I am just working on this. I'll discuss these topics in the list before sending patches, being aware this is a community project this many opinions. >>>>>> >>>>>> This is not necessarily about opinions. It is about what we can support in the end and where we all want to invest our time. >>>>>> >>>>> >>>>> Helping users with known problems, which could be resolved, is wating time also. In the moment I do this mainly. >>>>> >>>>>> Think about that we are all in the same boat and we want to improve IPFire wherever we can. But we need to talk about things because one set of eyes often is not enough. Something that works for one person does not work for another. We can never break backwards-compatibility. >>>>>> >>>>>> I personally want to see this static lease bug fixed. I am trying to help you to develop a good solution that we do not have to worry about in the future. I am also enforcing the rules that we all have come up with some long time ago and that work for this project. That way we will hopefully all benefit from this. >>>>>> >>>>> >>>>> Why didn't you just say this. A request for the whole file would have been enough. BTW the file can be found in the forum post. >>>> >>>> There are problems *in* the file. I raised questions. >>>> >>> >>> see above. >>> >>>>>> But I do not want to have endless discussions on this list about why the rules are as they are. If there is a constructive proposal to make things easier for all then we are all of course open for this. On the other hand debating what is the standard now is just a waste of time. I am happy to explain this, but I am not willing to debate them or compromise on this. Again, this is a general statement and not limited to this conversation. >>>>>> >>>>> >>>>> You started this discussion ( once more ). >>>> >>>> Okay, I tried to be helpful here. >>>> >>>> If you prefer to insist that this patch is not being amended and if you also prefer to point fingers, keep doing it. >>>> >>>> I will remember this and keep my comments to myself in the future then. >>>> >>> >>> There is no need to behave like this. I'll be quiet myself upon source quality and proposed fixes for bugs. >>> >>>>>>>>> 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. >>>>>>>> >>>>>>>> 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 term. >>>>>>>> >>>>>>> >>>>>>> Agreed, too. But again, this special problem appears from time to time in the forums. >>>>>> >>>>>> Why do we not have a ticket on BZ then? >>>>>> >>>>> >>>>> The ticket is there ( see subject of this discussion ) and the solution exists. >>>> >>>> I disagree with this “solution”. >>>> >>> >>> Why? >>> >>> -Bernhard >>> >>>>> >>>>> -Bernhard >>>>> >>>>>>> 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 reason for the patch. The amount of modified code isn't so big to demand the canonical development process, IMHO. The patch can be applied by any core developper reading DevList/bugzilla/forums regulary. >>>>>> >>>>>> It is entirely up to you how you develop your code. How it is submitted to the list is clear. >>>>>> >>>>>> -Michael >>>>>> >>>>>>> >>>>>>> -Bernhard >>>>>>> >>>>>>>> -Michael >>>>>>>> >>>>>>>>> >>>>>>>>> Regards, >>>>>>>>> Bernhard