DHCP server working in production now

[Alan DeKok]

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.