[prev in list] [next in list] [prev in thread] [next in thread]
List: busybox
Subject: RE: dhcpd: mac-based dynamic ip address picking. try3
From: "Vladislav Grishenko" <themiron () mail ! ru>
Date: 2011-01-31 9:11:40
Message-ID: 001001cbc126$df6f9160$9e4eb420$ () mail ! ru
[Download RAW message or body]
This is a multipart message in MIME format.
Hello,
> 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.
Kept as is, only jump to the address post-increment was added.
> -//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.
Roger that
Best Regards, theMIROn
ICQ: 303357
Skype: the.miron
> -----Original Message-----
> From: Denys Vlasenko [mailto:vda.linux@googlemail.com]
> Sent: Sunday, January 30, 2011 7:48 PM
> To: busybox@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
["0003.patch" (application/octet-stream)]
diff --git a/networking/udhcp/leases.c b/networking/udhcp/leases.c
index 7aeb37b..d7b49b9 100644
--- a/networking/udhcp/leases.c
+++ b/networking/udhcp/leases.c
@@ -7,6 +7,8 @@
#include "common.h"
#include "dhcpd.h"
+static unsigned lease_epoch = 0;
+
/* Find the oldest expired lease, NULL if there are no expired leases */
static struct dyn_lease *oldest_expired_lease(void)
{
@@ -134,35 +136,51 @@ static int nobody_responds_to_arp(uint32_t nip, const uint8_t *safe_mac)
/* Find a new usable (we think) address */
uint32_t FAST_FUNC find_free_or_expired_nip(const uint8_t *safe_mac)
{
- uint32_t addr;
+ uint32_t start, addr;
struct dyn_lease *oldest_lease = NULL;
-
- addr = server_config.start_ip; /* addr is in host order here */
- for (; addr <= server_config.end_ip; addr++) {
+ unsigned i, j;
+
+ /* hash hwaddr: use the SDBM hashing algorithm. Seems to give good
+ * dispersal even with similarly-valued "strings".
+ */
+//TODO: dhcp_packet.hlen should be used instead of hardcoded mac length
+#define hlen 6
+ for (j = 0, i = 0; i < hlen; i++)
+ j += safe_mac[i] + (j << 6) + (j << 16) - j;
+
+ /* pick a seed based on hwaddr then iterate until we find a free address. */
+ start = addr = server_config.start_ip +
+ ((j + lease_epoch) % (1 + server_config.end_ip - server_config.start_ip));
+ do {
uint32_t nip;
struct dyn_lease *lease;
/* ie, 192.168.55.0 */
if ((addr & 0xff) == 0)
- continue;
+ goto next_addr;
/* ie, 192.168.55.255 */
if ((addr & 0xff) == 0xff)
- continue;
+ goto next_addr;
nip = htonl(addr);
/* is this a static lease addr? */
if (is_nip_reserved(server_config.static_leases, nip))
- continue;
+ goto next_addr;
lease = find_lease_by_nip(nip);
if (!lease) {
//TODO: DHCP servers do not always sit on the same subnet as clients: should *ping*, not arp-ping!
if (nobody_responds_to_arp(nip, safe_mac))
return nip;
+ lease_epoch++;
} else {
if (!oldest_lease || lease->expires < oldest_lease->expires)
oldest_lease = lease;
}
- }
+ next_addr:
+ addr++;
+ if (addr > server_config.end_ip)
+ addr = server_config.start_ip;
+ } while (addr != start);
if (oldest_lease
&& is_expired_lease(oldest_lease)
_______________________________________________
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic