Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add static DHCPv4 leases settings #376

Merged
merged 13 commits into from
Jan 31, 2017
Merged

Add static DHCPv4 leases settings #376

merged 13 commits into from
Jan 31, 2017

Conversation

DL6ER
Copy link
Member

@DL6ER DL6ER commented Jan 25, 2017

By submitting this pull request, I confirm the following (please check boxes, eg [X] - no spaces) Failure to fill the template will close your PR:

Please submit all pull requests against the development branch. Failure to do so will delay or deny your request

  • I have read and understood the contributors guide.
  • I have checked that another pull request for this purpose does not exist.
  • I have considered, and confirmed that this submission will be valuable to others.
  • I accept that this submission may not be used, and the pull request closed at the will of the maintainer.
  • I give this submission freely, and claim no ownership to its content.

How familiar are you with the codebase?: 10


Add static DHCPv4 leases settings to Settings page. Batch-processing is not supported (add one entry at a time).

Backend changes: pi-hole/pi-hole#1170

screenshot at 2017-01-25 11-59-39

This template was created based on the work of udemy-dl.

@darkrain-nl
Copy link

Can you please explain why the MAC address is mandatory?

DNSMasq also works nice if you specify a hostname and the IP it should get, very handy if you don't know the MAC but do know the hostname...

@DL6ER
Copy link
Member Author

DL6ER commented Jan 25, 2017

From dnsmasq man-page

dhcp-host=lap,192.168.0.199 tells dnsmasq to always allocate the machine lap the IP address 192.168.0.199.

We cannot detect if multiple hosts are asking for the same address (because they have the same host name, like debian) and that could lead to issues.

@darkrain-nl
Copy link

That's true, but that's up to the administrator to make sure hostnames are unique.
In fact this is the first thing I do when setting up a new device...

@DL6ER
Copy link
Member Author

DL6ER commented Jan 25, 2017

Well, still people might bring laptops and/or smartphones to your WiFi and you might give them the password without thinking that their devices could have a bad host name.

Imagine you have a Win phone whose host name is just winphone and you cannot change it (not sure if they do it). Any second winphone coming to your network would cause trouble, as dnsmasq will do nothing to prevent IP collisions from happening.

@darkrain-nl
Copy link

I agree, in that case leave it like it is.
I only have one or two devices configured with only hostname, and that's still possible by editing the files manually...

@DL6ER
Copy link
Member Author

DL6ER commented Jan 25, 2017

that's still possible by editing the files manually...

I think we should really leave that for professionals. However, I will have to adapt the translation routines so your results will still be display correctly.

@darkrain-nl
Copy link

I think we should really leave that for professionals.

I am ;-)

However, I will have to adapt the translation routines so your results will still be display correctly.

That would be nice!

@DL6ER
Copy link
Member Author

DL6ER commented Jan 25, 2017

screenshot at 2017-01-25 13-07-57

@darkrain-nl
Copy link

Looks good!

@DL6ER DL6ER added this to the v2.5 milestone Jan 29, 2017
Copy link
Contributor

@AzureMarker AzureMarker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Debug output is still enabled

if(!strlen($error))
{
exec("sudo pihole -a removestaticdhcp ".$mac);
$success .= "The static address with MAC address ".htmlentities($mac)." has been added";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added -> removed

@AzureMarker
Copy link
Contributor

AzureMarker commented Jan 30, 2017

Approved.

Approved with PullApprove

@DL6ER DL6ER merged commit 4c952da into devel Jan 31, 2017
@DL6ER DL6ER deleted the new/DHCPstaticleases branch January 31, 2017 20:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants