DHCP server working in production now

Alan DeKok aland at deployingradius.com
Tue Aug 23 14:12:55 CEST 2011


Stephen R. van den Berg wrote:
> What follows is a bunch of patches leading up to a working DHCP server
> (I use it in several production systems as we speak).

  Except that the DHCP server already works, and is used in production
in multiple environments.

  What is so wrong with the existing code that it has to be replaced?

> The earlier patches are generic ones, please review and commit if appropriatec.
> The last is a patch that replaces parts of the old DHCP core with parts
> from my own custom cucidhcp server.  That patch has not been fully reformatted
> yet.  Please provide feedback on the implementation.  If it looks right,
> I'll clean up that patch too and resubmit it for inclusion.

  Just reading the first few lines of the patch is frustrating.
Comments have been removed for no purpose.  Named constants have been
replaced with hard-coded numbers.  The coding style (indent, etc.) is
*very* different from the existing code.  Huge amounts of code have been
deleted for no clear reason.  There are random whitespace changes, with
no change to the underlying code.

  While I understand your intention to contribute, this patch *breaks*
existing functionality.  It's not clear why.

  Please submit patches as a series of small changes, that each does
*one* thing, and changes as little as possible.

  And formatting *matters*.  For example, the following code is pretty
much incomprehensible:

+        { struct ifreq ifr;
+          memset(&ifr,0,sizeof ifr);
+          ifr.ifr_addr.sa_family=AF_INET;
+          strncpy(ifr.ifr_ifrn.ifrn_name,sock->interface,IFNAMSIZ);
+          if(!ioctl(s,SIOCGIFADDR,&ifr))
+           { sock->ipaddr.ipaddr.ip4addr.s_addr
+	      =((struct sockaddr_in*)&ifr.ifr_addr)->sin_addr.s_addr;
+             /* do not bind to the interface address, or broadcasts
will not
+              * be received
+              */
+              if(!ioctl(s,SIOCGIFNETMASK,&ifr))
+               { sock->netmask.ipaddr.ip4addr.s_addr=
+                  ((struct sockaddr_in*)&ifr.ifr_netmask)->sin_addr.s_addr;
+                 if(!ioctl(s,SIOCGIFHWADDR,&ifr))
+                  { memcpy(sock->hwaddr,ifr.ifr_hwaddr.sa_data,6);
+                    sock->hwfamily=ifr.ifr_hwaddr.sa_family;
+                    if(!ioctl(s,SIOCGIFINDEX,&ifr))
+                     { sock->ifindex=ifr.ifr_ifindex;
+                       if((sock->rsocket=s=
+                        socket(PF_PACKET,SOCK_DGRAM,htons(ETH_P_IP)))>=0)
+                        { struct sockaddr_ll bif;int n=1;
+                          bif.sll_family=AF_PACKET;
+                          bif.sll_protocol=htons(ETH_P_IP);
+                          bif.sll_ifindex=sock->ifindex;
+
if(setsockopt(s,SOL_SOCKET,SO_BROADCAST,(char*)&n,
+                            sizeof n)
+                           ||bind(s,(struct sockaddr*)&bif,sizeof bif))
+                             close(s),sock->rsocket=-1;

  Alan DeKok.



More information about the Freeradius-Devel mailing list