DHCP server working in production now

Alan DeKok aland at deployingradius.com
Tue Aug 23 15:24:42 CEST 2011

Stephen R. van den Berg wrote:
> Alan DeKok wrote:
>>  Except that the DHCP server already works, and is used in production
>> in multiple environments.
> Is it?  The existing code suggests otherwise.

  Stop being rude.

  Go read the list archives.  MULTIPLE people are using it an a
production environment.  Are they all lying?

>>  What is so wrong with the existing code that it has to be replaced?
> Perhaps the existing code works if the DHCP server is being fed 
> by a DHCP-relay upstream.  Because the current code cannot send
> raw packets, that severely limits its functionality as a native
> DHCP server.

  Maybe that's true.  But it doesn't change the fact that it's being
used in multiple production environments.

> The issues with the existing code were:
> a. No rawpackets and therefore unicast replies are not always possible.

  Sure.  As it turns out, that doesn't cause problems for most clients.

> b. Convoluted and non-transparent broadcast/unicast/IP decision logic.
>    I'm not saying it was/is wrong, but these matters are rather subtle,
>    and it is difficult to audit the existing code.

  Maybe that's true.  But replacing it with incomprehensible code isn't
a step forward.

> c. Too much tapdancing to avoid accessing the struct of the DHCP message,
>    which results in more convoluted code than necessary.

  While that might be true, you *removed* a lot of useful code.  That
code allowed the administrator to over-ride portions of the DHCP message.

  i.e. the main reason to use FreeRADIUS is it's ability to put
*anything* into a DHCP packet, based on policies which create
attributes.  You've removed that capability, which makes it much less
useful as a *better* DHCP server.

  FreeRADIUS can be used as a generic policy/protocol server.  Deleting
that code means it becomes just another DHCP server, which means there
are few reasons to use FreeRADIUS.

> Point a and b were important enough to start replacing parts, point c
> was more or less collateral damage (fixed it as I went along).

  Then *please* submit multiple patches.  Submitting a jumbo patch that
changes everything makes it difficult to separate the good changes from
the bad changes.

>>  Just reading the first few lines of the patch is frustrating.
>> Comments have been removed for no purpose.
> This can be cleaned up, I was merging two DHCP implementations, noone
> said it was easy.

  Deleting existing functionality means your patch *won't* get accepted.

>>  Named constants have been
>> replaced with hard-coded numbers.
> That's because the constants are inconsistently used.  I'll elaborate
> on this later, and am willing to fix this properly.  Let's just say
> that it wasn't good the way it was, and it's not perfect the way it is
> now; it's a sideeffect of the merge, this can be fixed properly.

  I'd prefer simple, clear fixes.

>>  The coding style (indent, etc.) is
>> *very* different from the existing code.
> I warned, that this is preliminary; this is not intended to be merged
> in the existing codebase; I'm giving a preview, that's all.

  Ugly code means I don't review it.  I just delete the message.  I don'
have time to try to figure out what the code is doing.  Well formatted
code means I can review it and provide productive feedback.

> It only breaks DHCP relaying, if that is necessary, I'll add it back.
> I'm still puzzled why freeradius would want to relay itself, and not
> delegate that functionality to a small program that does this quite
> well already.

  *Policy* based relays.

	MAC A -> DHCP server B
	MAC X -> DHCP server Y

  Where the MACs && servers get looked up in an SQL database.

  Or "Windows machines -> server B, printers -> server C"

  Existing DHCP relays can't do this.

  Again, the entire reason to use FreeRADIUS is customizable policies.
Removing that means there's no reason to use FreeRADIUS.

  Alan DeKok.

More information about the Freeradius-Devel mailing list