DHCP server working in production now

[Stephen R. van den Berg]

Alan DeKok wrote:
>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.

Is it?  The existing code suggests otherwise.

>  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.

The issues with the existing code were:
a. No rawpackets and therefore unicast replies are not always possible.
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.
c. Too much tapdancing to avoid accessing the struct of the DHCP message,
   which results in more convoluted code than necessary.

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).

>> 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.

This can be cleaned up, I was merging two DHCP implementations, noone
said it was easy.

>  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.

>  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.

>  Huge amounts of code have been
>deleted for no clear reason.

I have not deleted unless there was a reason.  I'll elaborate on this
later.

>  There are random whitespace changes, with
>no change to the underlying code.

Merging sideeffects, yet to be cleaned up.

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

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.

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

I did that with all the other patches, except for the "last" one (it didn't
come out last in the mail).

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

Explained above.
-- 
Stephen.