This is an incomplete review of the code adding Mobile ACcess-specific IP address assignment to ISC' DHCP server. I'm especially focusing on the dhcp-server integration here. On 23.03.2011, at 11:14, Rene Hummen wrote: > Author: hummen > Date: Wed Mar 23 11:14:04 2011 > New Revision: 2467 > > Log: > patch for dhcp-3.1.3 enabling static client leases > > NOTE: This patch needs review and testing. > > Added: > trunk/openwrt/patches/120-dhcp-ma-ap.patch > > Added: trunk/openwrt/patches/120-dhcp-ma-ap.patch > ============================================================================== > +diff -ruN dhcp-3.1.3-orig/includes/site.h dhcp-3.1.3/includes/site.h > +--- dhcp-3.1.3-orig/includes/site.h 2009-04-07 22:00:42.000000000 +0200 > ++++ dhcp-3.1.3/includes/site.h 2011-03-21 21:44:00.418827814 +0100 > +@@ -195,3 +195,11 @@ > + traces. */ > + > + #define TRACING > ++ > ++/* Define this if you want to be able to support class A/B networks. */ ISC's DHCP server only supports class C networks out of the box? Are larger subnets relevant for our demonstration scenario? > ++ > ++#define LARGE_SUBNET Has this patch been implemented by Max or is this a known issue that has been patched elsewhere? > ++ > ++/* Define this if you use this dhcpd on a mobileACcess AP */ > ++ > ++#define MOBAC_MODE Note that MOBAC_MODE can be enabled independently from LARGE_SUBNET. This will be important later on. > +diff -ruN dhcp-3.1.3-orig/server/confpars.c dhcp-3.1.3/server/confpars.c > +--- dhcp-3.1.3-orig/server/confpars.c 2009-09-01 22:32:28.000000000 > +0200 > ++++ dhcp-3.1.3/server/confpars.c 2011-03-21 21:44:00.425769109 +0100 > +@@ -3341,8 +3341,64 @@ > + } > + #endif /* FAILOVER_PROTOCOL */ > + > ++#if defined (LARGE_SUBNET) > ++ do { > ++ unsigned min, max; > ++ struct range *range; > ++ > ++ /* Make sure that high and low addresses are in same subnet. */ > ++ net = subnet_number (low, subnet -> netmask); > ++ if (!addr_eq (net, subnet_number (high, subnet -> netmask))) > ++ break; > ++ > ++ /* Make sure that the addresses are on the correct subnet. */ > ++ if (!addr_eq (net, subnet -> net)) > ++ break; > ++ > ++ /* Get the high and low host addresses... */ > ++ max = host_addr (high, subnet -> netmask); > ++ min = host_addr (low, subnet -> netmask); > ++ > ++ /* Allow range to be specified high-to-low as well as > low-to-high. */ > ++ if (min > max) { > ++ max = min; > ++ min = host_addr (high, subnet -> netmask); > ++ } > ++ > ++ /* Allocate if range is small enough */ > ++ if (max - min + 1 <= MAX_ALLOCATE_RANGE) > ++ break; > ++ > ++ /* Record the range for later use */ > ++ range = dmalloc (sizeof(struct range), MDL); > ++ if (!range) > ++ log_fatal ("Can't allocate range"); > ++ low = ip_addr (subnet -> net, subnet -> netmask, min); > ++ high = ip_addr (subnet -> net, subnet -> netmask, max); > ++ range -> low = low; > ++ range -> high = high; > ++ range -> min = min; > ++ range -> max = max; > ++ range -> cnt = max - min + 1; > ++ > ++ range -> next = pool -> range; > ++ pool -> range = range; > ++ > ++ log_debug ("saved range: %u.%u.%u.%u ~ %u.%u.%u.%u, cnt=%u", > ++ low.iabuf[0], low.iabuf[1], low.iabuf[2], > low.iabuf[3], > ++ high.iabuf[0], high.iabuf[1], high.iabuf[2], > high.iabuf[3], range -> cnt); > ++ > ++ pool_dereference (&pool, MDL); > ++ return; > ++ } while (0); do {} while (false)... So we just add just one class C network to our list here? How does this help with larger subnets? > ++ > ++ /* Create the new address range... */ > ++ new_address_range (cfile, low, high, subnet, pool, lpchain, 0); What does the value 0 represent here? > ++#else /*LARGE_SUBNET*/ > + /* Create the new address range... */ > + new_address_range (cfile, low, high, subnet, pool, lpchain); > ++#endif/*LARGE_SUBNET*/ Why do we need to add that parameter to support large subnets? > +diff -ruN dhcp-3.1.3-orig/server/dhcp.c dhcp-3.1.3/server/dhcp.c > +--- dhcp-3.1.3-orig/server/dhcp.c 2009-09-01 22:32:28.000000000 +0200 > ++++ dhcp-3.1.3/server/dhcp.c 2011-03-21 21:44:00.433702018 +0100 > +@@ -39,11 +39,20 @@ > + > + #include "dhcpd.h" > + > ++#if defined (MOBAC_MODE) > ++#include "mac2ip.h" > ++#include <time.h> > ++#endif > ++ > + int outstanding_pings; > + > + static char dhcp_message [256]; > + static int site_code_min; > + > ++#if defined (MOBAC_MODE) > ++const char mobac_interface[] = "br0"; > ++#endif Why do we need to hardcode this value? Can we have that as a configuration option? For the demo-scenario it might even be ok to restrict the dhcp service to an interface and remove this option here. > +@@ -2102,6 +2111,26 @@ > + lease_time = default_lease_time; > + } > + > ++#if defined (MOBAC_MODE) > ++ /* Check the number of seconds till midnight. If this is less > ++ than default_lease_time set it as lease_time */ Why is midnight an important point in time? How does the address-assignment work? > ++ if (strcmp(packet -> interface -> name, mobac_interface) == 0) { > ++ time_t cur_time = time(NULL); > ++ struct tm timestruct; > ++ gmtime_r(&cur_time, ×truct); > ++ > ++ int seconds_to_midnight; > ++ > ++ seconds_to_midnight = (23 - timestruct.tm_hour) * 60 * > 60; > ++ seconds_to_midnight += (59 - timestruct.tm_min) * 60; > ++ seconds_to_midnight += 60 - timestruct.tm_sec; > ++ > ++ if ((int) lease_time > seconds_to_midnight) > ++ lease_time = seconds_to_midnight; > ++ } > ++#endif > ++ > ++ > + #if defined (FAILOVER_PROTOCOL) > + /* Okay, we know the lease duration. Now check the > + failover state, if any. */ > +@@ -3783,6 +3812,91 @@ > + } else > + #endif > + { > ++ > ++#if defined (LARGE_SUBNET) > ++ while (!pool -> free && pool -> range) { > ++ struct range *range = pool -> range; > ++ unsigned cnt = range -> cnt; > ++ struct subnet *subnet = NULL; > ++ > ++ if (cnt < 0) cnt = 0; > ++ if (cnt > MAX_ALLOCATE_RANGE) cnt = > MAX_ALLOCATE_RANGE; > ++ > ++ /* find subnet from shared_network: > next_sibling? */ > ++ if (cnt) { > ++ struct shared_network *share = pool -> > shared_network; > ++ for (subnet = share -> subnets; subnet; > subnet = subnet -> next_sibling) { > ++ struct iaddr net = > subnet_number (range -> low, subnet -> netmask); > ++ if (!addr_eq(net, subnet_number > (range -> high, subnet -> netmask))) > ++ continue; > ++ if (addr_eq(net, subnet -> net)) > ++ break; > ++ } > ++ } > ++ if (!subnet) > ++ break; > ++ > ++ if (cnt) { > ++ > ++#if defined (MOBAC_MODE) The following code will only be executed if LARGE_SUBNET is defined as well. Bug or feature? > ++ struct iaddr new_mobac_ip = {}; > ++ new_mobac_ip.len = 4; > ++ > ++ /* Check if that unknown client is a > mobAC-client. If so, hash its MAC > ++ to an IP without using the normal > IP-pool. */ > ++ if (strcmp(packet -> interface -> name, > mobac_interface) == 0) { > ++ > ++ log_debug("received > DHCPDISCOVER-packet on mobAC-interface!"); > ++ > ++ mac2ip_mac_t clients_mac; > ++ if (packet->haddr->hlen != 7) { Magic number. > ++ log_error("clients > MAC-address length is not 6 byte!"); 6 or 7? > ++ int j = 0; > ++ for (j; j < > packet->haddr->hlen; j++) { > ++ > log_debug("HW[%d]: ", packet->haddr->hbuf[j]); > ++ } > ++ return 1; > ++ } else { > ++ int iter = 1; > ++ for (iter; iter < 7; > iter++) > ++ > clients_mac.x[iter-1] = packet->haddr->hbuf[iter]; > ++ } > ++ > ++ /* Now read the bounds for the > later generated IPs. */ > ++ mac2ip_ipv4_t calculated_ip; > ++ if (mac_to_ip(&clients_mac, > &calculated_ip) != 0) > ++ return 1; So this is where the IP addresses are assigned. However, I don't see where this code overwrites the default assignment done by the original DHCP server code. > +diff -ruN dhcp-3.1.3-orig/server/mdb.c dhcp-3.1.3/server/mdb.c > +--- dhcp-3.1.3-orig/server/mdb.c 2009-09-01 22:32:28.000000000 +0200 > ++++ dhcp-3.1.3/server/mdb.c 2011-03-21 21:44:00.447584608 +0100 > +@@ -525,12 +525,22 @@ > + return 0; > + } > + > ++#if defined (LARGE_SUBNET) > ++int new_address_range (cfile, low, high, subnet, pool, lpchain, instantiate) > ++ struct parse *cfile; > ++ struct iaddr low, high; > ++ struct subnet *subnet; > ++ struct pool *pool; > ++ struct lease **lpchain; > ++ int instantiate; Is this new parameter (instantiate) really necessary? > ++#else /*LARGE_SUBNET*/ > + void new_address_range (cfile, low, high, subnet, pool, lpchain) > + struct parse *cfile; > + struct iaddr low, high; > + struct subnet *subnet; > + struct pool *pool; > + struct lease **lpchain; > ++#endif /*LARGE_SUBNET*/ > + { > + #if defined(COMPACT_LEASES) > + struct lease *address_range; > +@@ -594,6 +604,13 @@ > + #if defined (COMPACT_LEASES) > + address_range = new_leases (max - min + 1, MDL); > + if (!address_range) { > ++ > ++#if defined (LARGE_SUBNET) > ++ if (!cfile && instantiate) { > ++ return 0; > ++ } What kind of problem are we handling here? (more like this below) > ++#endif /*LARGE_SUBNET*/ > ++ > + strcpy (lowbuf, piaddr (low)); > + strcpy (highbuf, piaddr (high)); > + log_fatal ("No memory for address range %s-%s.", > +@@ -611,12 +628,20 @@ > + lease_reference (&lp, &address_range [i], MDL); > + #else > + status = lease_allocate (&lp, MDL); This seems like the right place to hook mac2ip functionality...rather than the place above. René -- Dipl.-Inform. Rene Hummen, Ph.D. Student Chair of Communication and Distributed Systems RWTH Aachen University, Germany tel: +49 241 80 20772 web: http://www.comsys.rwth-aachen.de/team/rene-hummen/