dhcpd: mac-based dynamic ip address picking. 2nd try

[Vladislav Grishenko]

Redo inversed condition, fixed todo layout

Best Regards, theMIROn
ICQ: 303357
Skype: the.miron


> -----Original Message-----
> From: Denys Vlasenko [mailto:vda.linux at googlemail.com]
> Sent: Sunday, January 30, 2011 7:48 PM
> To: busybox at busybox.net
> Cc: Vladislav Grishenko; Leonid Lisovskiy
> Subject: Re: dhcpd: mac-based dynamic ip address picking
> 
> On Sunday 30 January 2011 12:55, Vladislav Grishenko wrote:
> > Hello,
> >
> > The idea of this patch taken from dnsmasq:
> > * 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.
> 
> 
> > * prevent the same ip address allocation for different interfaces of
> > the same station.
> >
> > 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?
> 
> 
> > 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.
> 
> 
> 
> -//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.
> 
> --
> vda
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-udhcpd-mac-based-ipaddr-dispersion.patch
Type: application/octet-stream
Size: 2969 bytes
Desc: not available
URL: <http://lists.busybox.net/pipermail/busybox/attachments/20110130/5a616b9d/attachment.obj>