Suggestions for DHCP leases status page

kjep · May 2, 2009, 7:40 PM

Hi everyone,

I have a few suggestions for improvements to the DHCP leases status page. First of all, for a static lease the start and end times are now displayed as current time and current time plus five minutes. To me that is just ugly. So I suggest something cleaner, like "n/a".

Second, when more than one lease is recorded in /var/dhcpd/var/db/dhcpd.leases for an address, the first lease is displayed on the web page. This is wrong, it is the last lease in the file that is the valid one.

Attached is a diff for /usr/local/www/diag_dhcp_leases.php.

Thank you for a superb firewall!

/Johan
dhcp_diff.txt

jimp · May 2, 2009, 10:39 PM

I'll have a look at this. It does seem to make sense.

Not sure if it would be something that would make it into 1.2.3, but it shouldn't have any problem finding its way into 2.0 (I can commit it there).

You may want to open two separate tickets - one for each issue, and put the patches there as well. Let me know what the resulting ticket numbers are.

Showing the wrong lease would definitely be a bug, but the other is debatable. I'm not sure why the now/now+5 was put there.

kjep · May 3, 2009, 9:04 AM

Certainly, I have now created two tickets. Number 1921 for the start and end times, and number 1922 for the wrong lease displayed.

jimp · May 3, 2009, 2:52 PM

Thanks, I grabbed the tickets, and I'll see what can be done.

GruensFroeschli · May 3, 2009, 3:42 PM

@jimp:

Showing the wrong lease would definitely be a bug, but the other is debatable. I'm not sure why the now/now+5 was put there.

Isnt the online/offline status dependable on if the MAC to this lease is in the arp-cache or not?
And dont entries in the cache expire after 5 minutes?

Then it would make sense to force the static leases to communicate with the pfSense periodically, thus keeping the online in the lease table.

jimp · May 3, 2009, 4:00 PM

@GruensFroeschli:

Isnt the online/offline status dependable on if the MAC to this lease is in the arp-cache or not?
And dont entries in the cache expire after 5 minutes?

Then it would make sense to force the static leases to communicate with the pfSense periodically, thus keeping the online in the lease table.

Except in the lease display this is purely cosmetic and has no real value. For static leases, in the code, it really just says "now" and "now + 5 minutes" for the time. As though the original author just wanted that field to have a time in it so it didn't look out of place.

That display doesn't have any bearing on when any kind of communication happens, and isn't reflective of how long ago it was 'seen' either… That's what is confusing me about it. Plus, it's a static lease, so the lease itself cannot expire. If it was using some method to poll when the entry would expire from the arp cache that might have made sense, but it's really just making it up as it goes along.

I have seen some DHCP implementations that faked a static lease by extending the expiry time to years instead of hours, but ISC DHCPD doesn't operate that way...

Seems "n/a" or just leaving it blank is probably the best course of action.

jimp · May 3, 2009, 6:08 PM

I commited the fixes to HEAD1,2. Trying to see if they can work their way into 1.2.3 or not. I wouldn't really count on it, but since they are simple, it might happen.

It may be a little too late in the release process (It is already at RC1) for more than critical bug fixes.

1: https://rcs.pfsense.org/projects/pfsense/repos/mainline/commits/aedd7929dc40d49bd631cd0dc8c88d6ee96e911e
2: https://rcs.pfsense.org/projects/pfsense/repos/mainline/commits/29e9dc64cfa2807d4c21b64c7a27802820f29dd4

jimp · May 3, 2009, 6:29 PM

The first fix was just committed to 1.2.3 by cmb, the other will have to wait for 2.0.

It should show up in the next snapshots.

jimp · May 3, 2009, 6:41 PM

I spoke too soon, it got in too.

Should be fixed all around.

kjep · May 4, 2009, 10:50 AM

Impressive! Thanks a lot!

/Johan