What follows is a bunch of patches leading up to a working DHCP server (I use it in several production systems as we speak). 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. -- Stephen.
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.
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.
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.
Alan DeKok wrote:
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.
I was not being rude. I just expressed my genuine startledness.
Go read the list archives. MULTIPLE people are using it an a production environment. Are they all lying?
I admit that I didn't read all the archives. I read the code first and found numerous spots where it was marked "Does not work" and a lot of the code broke due to different macros, and I found places which simply said "Fill in code here". So I assumed that it's not in production. My sincere apologies for that assumption, if I'd known, I'd *not* have deleted the DHCP-relay support.
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.
I am going to make it comprehensible before resubmitting.
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.
It might not seem like that, but I don't think I corrupted this ability. I want/need that flexibility myself, so I was very careful to preserve it.
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.
I normally always do, just not in this preview case.
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.
I understand that. I'm not offended by it. Feedback that it is too timeconsuming to parse for review is good enough too.
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
Interesting. Haven't seen those "in the wild" yet. But yes, I didn't know people did this. -- Stephen. "What do I do when I see someone *extremely gorgeous*? I stare, I smile, and when I get tired... I put the mirror down."
Stephen R. van den Berg wrote:
I was not being rude. I just expressed my genuine startledness.
You were implying that I was lying to you.
I admit that I didn't read all the archives. I read the code first and found numerous spots where it was marked "Does not work" and a lot of the code broke due to different macros, and I found places which simply said "Fill in code here". So I assumed that it's not in production.
And when I told you it was used in production, you implied I was lying to you. That's rude.
I am going to make it comprehensible before resubmitting.
That's good to hear.
It might not seem like that, but I don't think I corrupted this ability. I want/need that flexibility myself, so I was very careful to preserve it.
See src/lib/dhcp.c. You deleted tons of "pairfind VP, use contents in DHCP packet".
MAC A -> DHCP server B MAC X -> DHCP server Y
Interesting. Haven't seen those "in the wild" yet. But yes, I didn't know people did this.
They *want* to do it. They either do it with horrible hacks, or they don't. They usually can't, because of the ridiculous limitations of the existing DHCP relays. Alan DeKok.
Alan DeKok wrote:
Stephen R. van den Berg wrote:
I was not being rude. I just expressed my genuine startledness.
You were implying that I was lying to you.
No, not lying, inaccurate perhaps.
I admit that I didn't read all the archives. I read the code first and found numerous spots where it was marked "Does not work" and a lot of the code broke due to different macros, and I found places which simply said "Fill in code here". So I assumed that it's not in production.
And when I told you it was used in production, you implied I was lying to you. That's rude.
The thing is that "being used in production" is relative. I understand now that older versions of the code are being used in production, but since then this code has had changes which broke functionality (the old vs. new macro patches I submitted). I also understand that the *comments* were lying at times (the comments clearly state that essential parts of the code "do not work"), so forgive me for pointing out that both the *current* code and the comments were in direct conflict with your statement.
It might not seem like that, but I don't think I corrupted this ability. I want/need that flexibility myself, so I was very careful to preserve it.
See src/lib/dhcp.c. You deleted tons of "pairfind VP, use contents in DHCP packet".
I'll take a good look again when I readd the DHCP relay stuff to see if I indeed deleted more than I intended. -- Stephen.
participants (2)
-
Alan DeKok -
Stephen R. van den Berg