dhcpd: mac-based dynamic ip address picking

[Vladislav Grishenko]

> From: Denys Vlasenko
> Subject: Re: dhcpd: mac-based dynamic ip address picking

> > * the same ip address offering for clients with expired lease (almost
> > always) without any additional state saving
> 
> Every time you take lease_epoch++ branch, all future IPs will shift one
> address up, and you lose this property.

Not exactly, static IPs in DHCP range isn't the best practice,
Most of online clients gets his addrs back via requested IP, and epoch shifts to make sure this host will get the same addr, and all subsequent clients too.
For years of dnsmasq usage it's really ok, no complains.

> > dumb Windows dhcp-client (win7) ACKs same-subnet IP address, no
> matter
> > if it was previously assigned to the other interfaces, and can't
> > assign it to the current one, even if that different interface is down.
> 
> (1) Isn't it a bug in Win7?
> (2) What will happen if hash of both interfaces'MACs results in the same IP
> being picked?
1. Maybe bug, maybe not, it's reality, same as .0 and .255 avoidance. Google says it's rather common issue with notebooks.
2. Same as before changes. But SDBM hash from unique (almost always) 6-byte array collision probability on the same station is much more less than 100%, as without changes.

> > mac-based allocation makes this "endless discover-offer-request-ack loop"
> > issue almost impossible.
> 
> 
> -               /* ie, 192.168.55.0 */
> -               if ((addr & 0xff) == 0)
> -                       continue;
> -               /* ie, 192.168.55.255 */
> -               if ((addr & 0xff) == 0xff)
> -                       continue;
> -               nip = htonl(addr);
> -               /* is this a static lease addr? */
> -               if (is_nip_reserved(server_config.static_leases, nip))
> -                       continue;
> -
> +               if (
> +                   /* ie, not 192.168.55.0 */
> +                   ((addr & 0xff) != 0) &&
> +                   /* ie, not 192.168.55.255 */
> +                   ((addr & 0xff) != 0xff) &&
> +                   /* is this not a static lease addr? */
> +                   !(is_nip_reserved(server_config.static_leases, nip = htonl(addr)))
> +               ) {
> 
> This looks like a anti-improvement wrt code readability.
> Why do you do this change at all? The change isn't needed for your patch.

Address shifting is done at the end of do { } while loop, so no continues to the start from here.
 +               if (!(
 +                   /* ie, 192.168.55.0 */
 +                   ((addr & 0xff) == 0) ||
 +                   /* ie, 192.168.55.255 */
 +                   ((addr & 0xff) == 0xff) ||
 +                   /* is this a static lease addr? */
 +                   (is_nip_reserved(server_config.static_leases, nip = htonl(addr)))
 +               )) {
Would it be better?

> -//TODO: DHCP servers do not always sit on the same subnet as clients:
> should *ping*, not arp-ping!
> +                               /* TODO: DHCP servers do not always sit on the same
> +                                * subnet as clients: should *ping*, not arp-ping!
> +                                */
> 
> Please don't so this. The comment meant to stand out, because it indicates a
> bug we need to fix.

Oh, ok. There's one more TODO about using actual hlen instead of 6, but same hardcoded length is used 

Best Regards, theMIROn