Issue 279540043: DHCP Implementation in ns-3.24 (Closed)
|
Created:
9 years, 4 months ago by Mohit Tahiliani Modified:
7 years, 10 months ago CC:
ns-3-reviews_googlegroups.com, deeptir96_gmail.com Visibility:
Public. |
DescriptionThis patch is an extension and a port of Radu Lupu's DHCP Patch (provided for ns-3.12).
The patch provided here works with ns-3.24. Following additions have been done:
1. Ported ns-3.12 DHCP code to work with ns-3.24
2. DHCP ACK has been implemented (it was pending in ns-3.12's patch)
3. DHCP header is now compatible with Wireshark
4. Two test cases have been provided
We have taken permission from Radu Lupu to use his code of ns-3.12 and port it to ns-3.24.
Any suggestions / reviews would be much appreciated.
Thanks and Regards,
Ankit Deepak and Deepti Rajagopal
P.S.:
1. Patch set 6 onwards the code has been ported to work with ns-3.25
2. Patch set 8 onwards the code has been ported to work with ns-3.26
Patch Set 1 #
Total comments: 20
Patch Set 2 : Modified DHCP patch for ns-3.24 #
Total comments: 18
Patch Set 3 : Modified DHCP patch for ns-3.24 #
Total comments: 81
Patch Set 4 : Modified DHCP patch with changes in client #
Total comments: 26
Patch Set 5 : Latest DHCP modifications #
Total comments: 16
Patch Set 6 : DHCP patch shifted to internet-apps #Patch Set 7 : Modified DHCP Patch (Aug 2016) #
Total comments: 6
Patch Set 8 : Revised patch (on ns-3.26) #
Total comments: 2
Patch Set 9 : Revised patch (added Router Option) #
Total comments: 4
Patch Set 10 : Modified patch (bug fix and improved Doxygen comments) #
Total comments: 7
MessagesTotal messages: 38
Hi, I'm reviewing the code, but before we go further it's mandatory to do the documentation. In particular the scope and limitations. The problem is that DHCP is a HUGE protocol, with a lot of options. I guess that you don't want to support them all, neither server or client side. Thus, it is better to state it right away. Moreover, make explicit reference to what RFC are you following (the latest, of course). Some early comments are - mind that I just checked the headers and the example... 1) shouldn't all this go to the internet-apps module ? I mean, radvd is there, dhcp should as well. 2) the header should be divided clearly in the "base" and "optional" parts. About the header, please consider that it's not a true header. You can use a header-like structure because it's easier to use, but DHCP are messages. That's why there's no option header length: the message length implicitly fixes the option length. This is a problem when you deserialize the object, as you must go ahead 'til the message is finished (i.e., the iterator is over) and THEN you know the serialized size. https://codereview.appspot.com/279540043/diff/1/src/internet/model/dhcp-heade... File src/internet/model/dhcp-header.cc (right): https://codereview.appspot.com/279540043/diff/1/src/internet/model/dhcp-heade... src/internet/model/dhcp-header.cc:35: Indentation is wrong. Use check-style.py on maximum level (for the new files only). https://codereview.appspot.com/279540043/diff/1/src/internet/model/dhcp-heade... src/internet/model/dhcp-header.cc:39: // m_ts (Simulator::Now ().GetTimeStep ()) remove commented stuff https://codereview.appspot.com/279540043/diff/1/src/internet/model/dhcp-heade... src/internet/model/dhcp-header.cc:55: m_file[i] = 0; indentation. Moreover... Server name + Boot Filename are 64 + 128 bytes. https://codereview.appspot.com/279540043/diff/1/src/internet/model/dhcp-heade... src/internet/model/dhcp-header.cc:75: } What are these hardcoded values ? And why they are hardcoded ? https://codereview.appspot.com/279540043/diff/1/src/internet/model/dhcp-heade... src/internet/model/dhcp-header.cc:115: .SetParent<Header> () missing .SetGroup https://codereview.appspot.com/279540043/diff/1/src/internet/model/dhcp-header.h File src/internet/model/dhcp-header.h (right): https://codereview.appspot.com/279540043/diff/1/src/internet/model/dhcp-heade... src/internet/model/dhcp-header.h:30: #define DHCP_HEADER_LENGTH 262 Nope, the header length is not fixed. https://codereview.appspot.com/279540043/diff/1/src/internet/model/dhcp-heade... src/internet/model/dhcp-header.h:39: */ Wrong description, update it please. https://codereview.appspot.com/279540043/diff/1/src/internet/model/dhcp-heade... src/internet/model/dhcp-header.h:56: // Time GetTs (void) const; don't leave commented stuff around https://codereview.appspot.com/279540043/diff/1/src/internet/model/dhcp-heade... src/internet/model/dhcp-header.h:79: uint8_t m_file[198]; here is 198 bytes, and in the constructor it is initialized wth a for going up to 202 ? https://codereview.appspot.com/279540043/diff/1/src/internet/model/dhcp-heade... src/internet/model/dhcp-header.h:85: }; and document everything. Most of these are option-specific values.
Sign in to reply to this message.
On 2015/12/28 00:13:41, Tommaso Pecorella wrote: > Hi, > > I'm reviewing the code, but before we go further it's mandatory to do the > documentation. In particular the scope and limitations. > > The problem is that DHCP is a HUGE protocol, with a lot of options. I guess that > you don't want to support them all, neither server or client side. Thus, it is > better to state it right away. > Moreover, make explicit reference to what RFC are you following (the latest, of > course). > > Some early comments are - mind that I just checked the headers and the > example... > > 1) shouldn't all this go to the internet-apps module ? I mean, radvd is there, > dhcp should as well. > 2) the header should be divided clearly in the "base" and "optional" parts. > > About the header, please consider that it's not a true header. You can use a > header-like structure because it's easier to use, but DHCP are messages. That's > why there's no option header length: the message length implicitly fixes the > option length. > This is a problem when you deserialize the object, as you must go ahead 'til the > message is finished (i.e., the iterator is over) and THEN you know the > serialized size. > > https://codereview.appspot.com/279540043/diff/1/src/internet/model/dhcp-heade... > File src/internet/model/dhcp-header.cc (right): > > https://codereview.appspot.com/279540043/diff/1/src/internet/model/dhcp-heade... > src/internet/model/dhcp-header.cc:35: > Indentation is wrong. Use check-style.py on maximum level (for the new files > only). > > https://codereview.appspot.com/279540043/diff/1/src/internet/model/dhcp-heade... > src/internet/model/dhcp-header.cc:39: // m_ts (Simulator::Now ().GetTimeStep > ()) > remove commented stuff > > https://codereview.appspot.com/279540043/diff/1/src/internet/model/dhcp-heade... > src/internet/model/dhcp-header.cc:55: m_file[i] = 0; > indentation. > Moreover... Server name + Boot Filename are 64 + 128 bytes. > > https://codereview.appspot.com/279540043/diff/1/src/internet/model/dhcp-heade... > src/internet/model/dhcp-header.cc:75: } > What are these hardcoded values ? > And why they are hardcoded ? > > https://codereview.appspot.com/279540043/diff/1/src/internet/model/dhcp-heade... > src/internet/model/dhcp-header.cc:115: .SetParent<Header> () > missing .SetGroup > > https://codereview.appspot.com/279540043/diff/1/src/internet/model/dhcp-header.h > File src/internet/model/dhcp-header.h (right): > > https://codereview.appspot.com/279540043/diff/1/src/internet/model/dhcp-heade... > src/internet/model/dhcp-header.h:30: #define DHCP_HEADER_LENGTH 262 > Nope, the header length is not fixed. > > https://codereview.appspot.com/279540043/diff/1/src/internet/model/dhcp-heade... > src/internet/model/dhcp-header.h:39: */ > Wrong description, update it please. > > https://codereview.appspot.com/279540043/diff/1/src/internet/model/dhcp-heade... > src/internet/model/dhcp-header.h:56: // Time GetTs (void) const; > don't leave commented stuff around > > https://codereview.appspot.com/279540043/diff/1/src/internet/model/dhcp-heade... > src/internet/model/dhcp-header.h:79: uint8_t m_file[198]; > here is 198 bytes, and in the constructor it is initialized wth a for going up > to 202 ? > > https://codereview.appspot.com/279540043/diff/1/src/internet/model/dhcp-heade... > src/internet/model/dhcp-header.h:85: }; > and document everything. Most of these are option-specific values. Dear Tommaso Sir, Thank you very much for your feedback on the code. I am working towards incorporating all the changes you suggested and hope to complete the same by this weekend. Regards, Ankit Deepak
Sign in to reply to this message.
Dear Ankit, Thanks for the contribution. I am curious what's your motive for reimplementing DHCP ? I understand people may want to reimplement protocols presenting algoritmics problems or protocols that leave in kernelspace because of their complexity but DHCP lives in userspace: do you know you can run real software in ns3 with its DCE extension ? It may spare you quite some time. Cheers Matt
Sign in to reply to this message.
On 2016/01/05 14:08:51, mattator wrote: > Dear Ankit, > > Thanks for the contribution. > I am curious what's your motive for reimplementing DHCP ? I understand people > may want to reimplement protocols presenting algoritmics problems or protocols > that leave in kernelspace because of their complexity but DHCP lives in > userspace: do you know you can run real software in ns3 with its DCE extension ? > It may spare you quite some time. > > Cheers > Matt Dear Matt, Thanks for your suggestions! This was my networking lab project and the main idea was to get familiar with ns-3 and how to contribute. Since there was a partial implementation of DHCP available for ns-3.12 and I studied the same protocol in this semester, I thought to complete its implementation and try learning the procedure to contribute to ns-3. Regarding DCE - Yes, I am aware of it features but have not tried working with it. Will surely look into it. Regards, Ankit Deepak
Sign in to reply to this message.
Dear Tommaso Sir, I have updated the DHCP patch and uploaded it here. Almost all the suggested corrections have been incorporated. The DHCP module has been shifted to "applications" and later will be shifted to "internet-apps" once ns-3.25 is released. Thank you very much for your guidance. Any suggestions / corrections on the updated patch would be much appreciated. Sincerely, Ankit Deepak https://codereview.appspot.com/279540043/diff/1/src/internet/model/dhcp-heade... File src/internet/model/dhcp-header.cc (right): https://codereview.appspot.com/279540043/diff/1/src/internet/model/dhcp-heade... src/internet/model/dhcp-header.cc:35: On 2015/12/28 00:13:41, Tommaso Pecorella wrote: > Indentation is wrong. Use check-style.py on maximum level (for the new files > only). Done. https://codereview.appspot.com/279540043/diff/1/src/internet/model/dhcp-heade... src/internet/model/dhcp-header.cc:39: // m_ts (Simulator::Now ().GetTimeStep ()) On 2015/12/28 00:13:41, Tommaso Pecorella wrote: > remove commented stuff Done. https://codereview.appspot.com/279540043/diff/1/src/internet/model/dhcp-heade... src/internet/model/dhcp-header.cc:55: m_file[i] = 0; On 2015/12/28 00:13:41, Tommaso Pecorella wrote: > indentation. > Moreover... Server name + Boot Filename are 64 + 128 bytes. Done. https://codereview.appspot.com/279540043/diff/1/src/internet/model/dhcp-heade... src/internet/model/dhcp-header.cc:75: } On 2015/12/28 00:13:41, Tommaso Pecorella wrote: > What are these hardcoded values ? > And why they are hardcoded ? These have been removed from here and added in their respective set functions. https://codereview.appspot.com/279540043/diff/1/src/internet/model/dhcp-heade... src/internet/model/dhcp-header.cc:115: .SetParent<Header> () On 2015/12/28 00:13:41, Tommaso Pecorella wrote: > missing .SetGroup Done. https://codereview.appspot.com/279540043/diff/1/src/internet/model/dhcp-header.h File src/internet/model/dhcp-header.h (right): https://codereview.appspot.com/279540043/diff/1/src/internet/model/dhcp-heade... src/internet/model/dhcp-header.h:30: #define DHCP_HEADER_LENGTH 262 On 2015/12/28 00:13:41, Tommaso Pecorella wrote: > Nope, the header length is not fixed. Done. https://codereview.appspot.com/279540043/diff/1/src/internet/model/dhcp-heade... src/internet/model/dhcp-header.h:39: */ On 2015/12/28 00:13:41, Tommaso Pecorella wrote: > Wrong description, update it please. Done. https://codereview.appspot.com/279540043/diff/1/src/internet/model/dhcp-heade... src/internet/model/dhcp-header.h:56: // Time GetTs (void) const; On 2015/12/28 00:13:41, Tommaso Pecorella wrote: > don't leave commented stuff around Done. https://codereview.appspot.com/279540043/diff/1/src/internet/model/dhcp-heade... src/internet/model/dhcp-header.h:79: uint8_t m_file[198]; On 2015/12/28 00:13:41, Tommaso Pecorella wrote: > here is 198 bytes, and in the constructor it is initialized wth a for going up > to 202 ? Done. https://codereview.appspot.com/279540043/diff/1/src/internet/model/dhcp-heade... src/internet/model/dhcp-header.h:85: }; On 2015/12/28 00:13:41, Tommaso Pecorella wrote: > and document everything. Most of these are option-specific values. Done.
Sign in to reply to this message.
Second round, still only on headers (sorry, but I have little time and I prefer to concentrate on one thing at a time). One further comment: where's the example (there was one) ? The example had some comments I found puzzling and I wanted to discuss, like a strange setup to avoid a problem in broadcast address processing. If you find an unwanted or strange behaviour... we can fix it. https://codereview.appspot.com/279540043/diff/20001/src/applications/model/dh... File src/applications/model/dhcp-header.cc (right): https://codereview.appspot.com/279540043/diff/20001/src/applications/model/dh... src/applications/model/dhcp-header.cc:60: m_len += 3; Adding 3 blindly is not wise. check if the type can be changed first. Use case: if you call this function twice in a row with the same params, the length will be wrong. https://codereview.appspot.com/279540043/diff/20001/src/applications/model/dh... src/applications/model/dhcp-header.cc:204: i.Write (m_pad,202); RFC 2131, here it should be only the sname (64) and file (128), thus 192 bytes. Moreover, I'd split them (for future enhancements and clarity). https://codereview.appspot.com/279540043/diff/20001/src/applications/model/dh... src/applications/model/dhcp-header.cc:206: if (m_opt[53]) Supporting option 51 (IP Address Lease Time) could be a good idea... https://codereview.appspot.com/279540043/diff/20001/src/applications/model/dh... src/applications/model/dhcp-header.cc:282: break; Be prepared to heavy mistakes: check if the iterator is reaching the end of the buffer. https://codereview.appspot.com/279540043/diff/20001/src/applications/model/dh... File src/applications/model/dhcp-header.h (right): https://codereview.appspot.com/279540043/diff/20001/src/applications/model/dh... src/applications/model/dhcp-header.h:30: #define DHCP_HEADER_LENGTH 262 Unused, remove :P https://codereview.appspot.com/279540043/diff/20001/src/applications/model/dh... src/applications/model/dhcp-header.h:35: * \class SeqTsHeader \ingroup is wrong, and \class is superfluous https://codereview.appspot.com/279540043/diff/20001/src/applications/model/dh... src/applications/model/dhcp-header.h:36: * \brief Bootp header with dhcp messages supports option 1, 50, 53, 54 and 255 of bootp Please add a brief description of the header, check for example the sixlowpan header description. A good option is to report the exact header structure from the RFC. https://codereview.appspot.com/279540043/diff/20001/src/applications/model/dh... src/applications/model/dhcp-header.h:36: * \brief Bootp header with dhcp messages supports option 1, 50, 53, 54 and 255 of bootp isn't it better to use an enum for the option numbers ? E.g., what's "53" ? An enum would make it easier to know. https://codereview.appspot.com/279540043/diff/20001/src/applications/model/dh... src/applications/model/dhcp-header.h:117: void Setdhcps (Ipv4Address addr); Beware of CamelCase - this applies to all the functions (there are more occurrences).
Sign in to reply to this message.
Dear Tommaso Sir, Except for the query related to deserialize function, I've incorporated all the suggestions in code. Regarding the example program, it wasn't included in the second patch because I didn't find examples directory in applications module. Should I go ahead and create examples directory inside applications and then add this example program to it? I wasn't sure if that's permitted. Sincerely, Ankit Deepak https://codereview.appspot.com/279540043/diff/20001/src/applications/model/dh... File src/applications/model/dhcp-header.cc (right): https://codereview.appspot.com/279540043/diff/20001/src/applications/model/dh... src/applications/model/dhcp-header.cc:60: m_len += 3; On 2016/02/02 23:11:40, Tommaso Pecorella wrote: > Adding 3 blindly is not wise. check if the type can be changed first. > Use case: if you call this function twice in a row with the same params, the > length will be wrong. Done. https://codereview.appspot.com/279540043/diff/20001/src/applications/model/dh... src/applications/model/dhcp-header.cc:204: i.Write (m_pad,202); On 2016/02/02 23:11:40, Tommaso Pecorella wrote: > RFC 2131, here it should be only the sname (64) and file (128), thus 192 bytes. > Moreover, I'd split them (for future enhancements and clarity). Done. https://codereview.appspot.com/279540043/diff/20001/src/applications/model/dh... src/applications/model/dhcp-header.cc:206: if (m_opt[53]) On 2016/02/02 23:11:40, Tommaso Pecorella wrote: > Supporting option 51 (IP Address Lease Time) could be a good idea... Done. https://codereview.appspot.com/279540043/diff/20001/src/applications/model/dh... src/applications/model/dhcp-header.cc:282: break; On 2016/02/02 23:11:40, Tommaso Pecorella wrote: > Be prepared to heavy mistakes: check if the iterator is reaching the end of the > buffer. Sir, While writing the packet to be sent, in the serialize function (line 230), I'm always including 255 in the header. Wouldn't it ensure that 255 is read at least once by the receiver, making the iterator reach end of the buffer? https://codereview.appspot.com/279540043/diff/20001/src/applications/model/dh... File src/applications/model/dhcp-header.h (right): https://codereview.appspot.com/279540043/diff/20001/src/applications/model/dh... src/applications/model/dhcp-header.h:30: #define DHCP_HEADER_LENGTH 262 On 2016/02/02 23:11:40, Tommaso Pecorella wrote: > Unused, remove :P Done. https://codereview.appspot.com/279540043/diff/20001/src/applications/model/dh... src/applications/model/dhcp-header.h:35: * \class SeqTsHeader On 2016/02/02 23:11:40, Tommaso Pecorella wrote: > \ingroup is wrong, and \class is superfluous Done. https://codereview.appspot.com/279540043/diff/20001/src/applications/model/dh... src/applications/model/dhcp-header.h:36: * \brief Bootp header with dhcp messages supports option 1, 50, 53, 54 and 255 of bootp On 2016/02/02 23:11:40, Tommaso Pecorella wrote: > Please add a brief description of the header, check for example the sixlowpan > header description. > A good option is to report the exact header structure from the RFC. Done. https://codereview.appspot.com/279540043/diff/20001/src/applications/model/dh... src/applications/model/dhcp-header.h:36: * \brief Bootp header with dhcp messages supports option 1, 50, 53, 54 and 255 of bootp On 2016/02/02 23:11:40, Tommaso Pecorella wrote: > isn't it better to use an enum for the option numbers ? E.g., what's "53" ? An > enum would make it easier to know. Done. https://codereview.appspot.com/279540043/diff/20001/src/applications/model/dh... src/applications/model/dhcp-header.h:117: void Setdhcps (Ipv4Address addr); On 2016/02/02 23:11:40, Tommaso Pecorella wrote: > Beware of CamelCase - this applies to all the functions (there are more > occurrences). Done.
Sign in to reply to this message.
Hi Ankit, thanks for your very quick replies. I hope to be able to be as fast as you in reviewing the rest of the code. About the 2 questions you have... Examples directory: of course it is allowed, there's no examples directory at the moment just because there's no example ! Header deserialization: you're right, but you are assuming a well-formed packet. I know that ns-3 will make a well-formed packet, but what about DCE or emulated devices ? Are you sure that a malformed packet can not reach out code ? In case we face a malformed packet you don't have to crash, you must (gracefully) discard the packet. As a consequence, a good practice is to deserialize the header (or any message) taking into consideration the buffer length. If the packet is too short, or something goes wrong, simply use NS_LOG_WARN, stop the deserialization and return zero. Returning zero *should* be the signal that the header wasn't correctly deserialized. The "should" is there because not every piece of code does this check. Cheers, T.
Sign in to reply to this message.
Dear Tommaso Sir, I have uploaded patch 3 with following changes: - created the examples directory in applications and added the example - the updated code now takes care of the malformed packet header and gives warning if something goes wrong. Thank you very much for the valuable suggestions. Sincerely, Ankit Deepak
Sign in to reply to this message.
Hi, here's another round of comments. I still need to check the server tho... expect one more. One general comment about the client. You're making two quite hard assumptions: 1) only one interface under DHCP and 2) only one DHCP server per network. While I can live with the fist assumption, the second one is a different thing. The problem are the users: they have a natural skill for not reading the documentation and be surprised by abnormal (for them) behaviours. Since it is perfectly legal (and even suggested) to have multiple DHCP servers in a network, you should: 1) add a delay between the DHCPOFFER and the DHCPREQUEST, and 2) add a protection / choice when you receive multiple OFFER messages. Finally, I'd really suggest to try to streamline the consistency checks for incoming messages. Doing them all at the beginning will clarify the code greatly. https://codereview.appspot.com/279540043/diff/40001/src/applications/examples... File src/applications/examples/dhcp-example.cc (right): https://codereview.appspot.com/279540043/diff/40001/src/applications/examples... src/applications/examples/dhcp-example.cc:92: // following is a workaround (to support undirected broadcast)!!!!! This workaround is disturbing. A.k.A. Do or do not. There's no workaround. In other terms, if there's a problem, we fix the problem. https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... File src/applications/model/dhcp-client.cc (right): https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... src/applications/model/dhcp-client.cc:56: MakeIpv4AddressAccessor (&DhcpClient::m_peerAddress), Why it's an Attribute ? The peer address (DHCP relay, if present) is discovered, and should not be need to set it manually. Remove the Attribute and the SetRemote. Eventually add a GetRelayAddress function. https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... src/applications/model/dhcp-client.cc:59: UintegerValue (67), You can use a define or a static const for this (it's a standard port). https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... src/applications/model/dhcp-client.cc:62: .AddAttribute ("NetDevice", "Index of netdevice of the node for DHCP", This is a lot more troublesome. Theoretically the DHCP status and application should run on a per-device basis, i.e., it should be possible to run it on different interfaces independently fro each other (think to Wi-Fi and Ethernet). We can *choose* to not do this and limit to one interface only, but it must be documented. https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... src/applications/model/dhcp-client.cc:77: m_ran = CreateObject<UniformRandomVariable> (); Use the new Random Streams syntax. See for example ArpL3Protocol (and its helper). https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... src/applications/model/dhcp-client.cc:142: void DhcpClient::IPCHandler (Ptr<Socket> socket) This seems a dinosaur, i.e., a dead code inherited from old code for unknown purposes. I like my dinosaurs where they belong: in the history museums. Either it does have a purpose, or you should remove it (along with its related things, like the port, define, etc.). https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... src/applications/model/dhcp-client.cc:156: void DhcpClient::NetHandler (Ptr<Socket> socket) Why a function to just call another function ? Use only one ! https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... src/applications/model/dhcp-client.cc:168: RunEfsm (); There's something strange here... Why are you calling RunEfsm ? (more later). https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... src/applications/model/dhcp-client.cc:182: ipv4MN->AddAddress (ifIndex, Ipv4InterfaceAddress (Ipv4Address ("0.0.0.0"), Ipv4Mask ("/0"))); Why did you remove the RemoveAddress ? Indeed, you should add the "0.0.0.0" address, you should remove the previous one. https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... src/applications/model/dhcp-client.cc:190: m_state = IDLEE; Instead of forcing a status, shouldn't you check that you're in the right status ? https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... src/applications/model/dhcp-client.cc:194: void DhcpClient::RunEfsm (void) ... more here. This function does too much. It is both used to send periodic DHCP requests, and to process incoming replies. Split the functionalities in two different functions, and enforce the state machine consistency with proper checks. One way to achieve this is to use a sub-class (e.g., DhcpClient::Status) and to have only setter/getter methods, where the setter can enforce that only "legal" transitions are allowed. The periodic send can just check: if the status is IDLE, then send and reschedule. Otherwise return. https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... src/applications/model/dhcp-client.cc:212: header.SetChaddr (Mac48Address::ConvertFrom (GetNode ()->GetDevice (device)->GetAddress ())); Double conversion, useless. https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... src/applications/model/dhcp-client.cc:229: packet = Create<Packet> (); No need to create a packet if you're receiving it just a second later. https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... src/applications/model/dhcp-client.cc:264: m_state = WAIT_ACK; Nope, you're going into WAIT_ACK even if the packet isn't correct. https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... src/applications/model/dhcp-client.cc:276: packet = m_socket->RecvFrom (from); Is there a specific need to do this ? I mean, why should you do it here and not before ? https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... src/applications/model/dhcp-client.cc:291: ipv4->RemoveAddress (ifIndex, 0); No. Here you're assuming that there is an "old" address or a fake one. Remember that an interface can have multiple (legal9 IP addresses, and you're blindly removing one. Just... no. https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... src/applications/model/dhcp-client.cc:293: ipv4->AddAddress (ifIndex, Ipv4InterfaceAddress ((Ipv4Address)m_myAddress, m_myMask)); HERE you should find out and remember what's the address you added, so to remove it later if needed (not by index, by address). https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... src/applications/model/dhcp-client.cc:306: ipv4->SetUp (0); Why you're setting up the Loopback interface ? It seems unnecessary. https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... File src/applications/model/dhcp-client.h (right): https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... src/applications/model/dhcp-client.h:66: Ipv4Address GetDhcps (void); GetDhcpServer - otherwise it seems a plural form. https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... src/applications/model/dhcp-client.h:78: DHCPNACK = 5 //!< Defining code for nack message These are the same numbers that are defined in the server. It's wise to place them in the header, so they're not duplicated. https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... src/applications/model/dhcp-client.h:83: IDLEE = 0, //!< Defining IDLE state of the client One extra "E" ? https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... src/applications/model/dhcp-client.h:89: static const int RTRS_TIMEOUT = 1; //!< Defining the time for retransmission Better an attribute for this. the user could want to play with it. https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... src/applications/model/dhcp-client.h:91: static const int DHCP_IPC_PORT = 6868; //!< Defining DHCP IPC port I'd move this to the .cc file. https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... src/applications/model/dhcp-client.h:139: uint32_t m_lease; //!< Store the lease time of the address if you need to store a time, use Time, not integers. https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... src/applications/model/dhcp-client.h:140: Ptr<UniformRandomVariable> m_ran; //!< Uniform random variable for transaction ID Use Streams https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... File src/applications/model/dhcp-header.cc (right): https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... src/applications/model/dhcp-header.cc:301: i.Read (m_magic_cookie,4); Check the magic cookie value, it's there for this specific reason :P https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... File src/applications/model/dhcp-header.h (right): https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... src/applications/model/dhcp-header.h:69: You missed the \verbatim commands. Plus, avoid the figure caption (remember that it will end in a Doxygen webpage). https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... src/applications/model/dhcp-header.h:117: void SetMagicCookie (); Since we don't really use this header as a BOOTP header, we can also hardcode the magic cookie in the header constructor. If we don't, there's a fat chance that the user will forget to call this function. In other terms, I'd remove it altogether and I'd put the magic cookie setup in the constructor. https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... src/applications/model/dhcp-header.h:233: uint8_t m_hwpad[10]; //!< Current Implementation for 48 bit Mac Address, 10 bits padded I feel dumb at asking again. These are 10 bytes, not 10 bits. The specification says that we need 16 bytes for the hardware address (chaddr), and we're using a 48 bits address (6), so 10 bytes padding makes sense. What's not "right" is the portability. One should accept any kind of address (also Mac64) and add the appropriate padding on the flight. Hardcoding is not wise. https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... File src/applications/model/dhcp-server.h (right): https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... src/applications/model/dhcp-server.h:55: void RunEfsm (void); Please add all the relevant doxygen. Suggestion: run "doc/doxygen.warnings.report.sh" and look at the warning messages generated by dhcp files. https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... src/applications/model/dhcp-server.h:59: friend inline std::ostream & operator<< (std::ostream& os, Ipv4Address &address) This function shouldn't be necessary, Ipv4Address can be already printed. https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... src/applications/model/dhcp-server.h:78: static const int WORKING = 0; //!< Defining the working state of the server It doesn't seems very useful...
Sign in to reply to this message.
Dear Sir, I have incorporated 14 suggestions out of 32, and working on the rest. I shall upload patch 4 with these 14 modifications. Sincerely, Ankit Deepak https://codereview.appspot.com/279540043/diff/40001/src/applications/examples... File src/applications/examples/dhcp-example.cc (right): https://codereview.appspot.com/279540043/diff/40001/src/applications/examples... src/applications/examples/dhcp-example.cc:92: // following is a workaround (to support undirected broadcast)!!!!! On 2016/02/10 23:40:01, Tommaso Pecorella wrote: > This workaround is disturbing. > A.k.A. > Do or do not. There's no workaround. > > In other terms, if there's a problem, we fix the problem. Acknowledged. https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... File src/applications/model/dhcp-client.cc (right): https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... src/applications/model/dhcp-client.cc:56: MakeIpv4AddressAccessor (&DhcpClient::m_peerAddress), On 2016/02/10 23:40:01, Tommaso Pecorella wrote: > Why it's an Attribute ? > The peer address (DHCP relay, if present) is discovered, and should not be need > to set it manually. > Remove the Attribute and the SetRemote. Eventually add a GetRelayAddress > function. Done. https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... src/applications/model/dhcp-client.cc:59: UintegerValue (67), On 2016/02/10 23:40:01, Tommaso Pecorella wrote: > You can use a define or a static const for this (it's a standard port). Done. https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... src/applications/model/dhcp-client.cc:62: .AddAttribute ("NetDevice", "Index of netdevice of the node for DHCP", On 2016/02/10 23:40:01, Tommaso Pecorella wrote: > This is a lot more troublesome. > Theoretically the DHCP status and application should run on a per-device basis, > i.e., it should be possible to run it on different interfaces independently fro > each other (think to Wi-Fi and Ethernet). > We can *choose* to not do this and limit to one interface only, but it must be > documented. Acknowledged. https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... src/applications/model/dhcp-client.cc:77: m_ran = CreateObject<UniformRandomVariable> (); On 2016/02/10 23:40:02, Tommaso Pecorella wrote: > Use the new Random Streams syntax. See for example ArpL3Protocol (and its > helper). Acknowledged. https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... src/applications/model/dhcp-client.cc:142: void DhcpClient::IPCHandler (Ptr<Socket> socket) On 2016/02/10 23:40:01, Tommaso Pecorella wrote: > This seems a dinosaur, i.e., a dead code inherited from old code for unknown > purposes. > I like my dinosaurs where they belong: in the history museums. > Either it does have a purpose, or you should remove it (along with its related > things, like the port, define, etc.). Done. https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... src/applications/model/dhcp-client.cc:156: void DhcpClient::NetHandler (Ptr<Socket> socket) On 2016/02/10 23:40:01, Tommaso Pecorella wrote: > Why a function to just call another function ? Use only one ! Acknowledged. https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... src/applications/model/dhcp-client.cc:168: RunEfsm (); On 2016/02/10 23:40:01, Tommaso Pecorella wrote: > There's something strange here... Why are you calling RunEfsm ? (more later). Acknowledged. https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... src/applications/model/dhcp-client.cc:182: ipv4MN->AddAddress (ifIndex, Ipv4InterfaceAddress (Ipv4Address ("0.0.0.0"), Ipv4Mask ("/0"))); On 2016/02/10 23:40:01, Tommaso Pecorella wrote: > Why did you remove the RemoveAddress ? > Indeed, you should add the "0.0.0.0" address, you should remove the previous > one. Acknowledged. https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... src/applications/model/dhcp-client.cc:190: m_state = IDLEE; On 2016/02/10 23:40:01, Tommaso Pecorella wrote: > Instead of forcing a status, shouldn't you check that you're in the right status > ? Acknowledged. https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... src/applications/model/dhcp-client.cc:194: void DhcpClient::RunEfsm (void) On 2016/02/10 23:40:02, Tommaso Pecorella wrote: > ... more here. > This function does too much. > It is both used to send periodic DHCP requests, and to process incoming replies. > Split the functionalities in two different functions, and enforce the state > machine consistency with proper checks. > > One way to achieve this is to use a sub-class (e.g., DhcpClient::Status) and to > have only setter/getter methods, where the setter can enforce that only "legal" > transitions are allowed. > > The periodic send can just check: if the status is IDLE, then send and > reschedule. Otherwise return. Acknowledged. https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... src/applications/model/dhcp-client.cc:229: packet = Create<Packet> (); On 2016/02/10 23:40:01, Tommaso Pecorella wrote: > No need to create a packet if you're receiving it just a second later. Done. https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... src/applications/model/dhcp-client.cc:264: m_state = WAIT_ACK; On 2016/02/10 23:40:01, Tommaso Pecorella wrote: > Nope, you're going into WAIT_ACK even if the packet isn't correct. Acknowledged. https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... src/applications/model/dhcp-client.cc:276: packet = m_socket->RecvFrom (from); On 2016/02/10 23:40:01, Tommaso Pecorella wrote: > Is there a specific need to do this ? > I mean, why should you do it here and not before ? Acknowledged. https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... src/applications/model/dhcp-client.cc:291: ipv4->RemoveAddress (ifIndex, 0); On 2016/02/10 23:40:01, Tommaso Pecorella wrote: > No. Here you're assuming that there is an "old" address or a fake one. > Remember that an interface can have multiple (legal9 IP addresses, and you're > blindly removing one. Just... no. Acknowledged. https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... src/applications/model/dhcp-client.cc:293: ipv4->AddAddress (ifIndex, Ipv4InterfaceAddress ((Ipv4Address)m_myAddress, m_myMask)); On 2016/02/10 23:40:02, Tommaso Pecorella wrote: > HERE you should find out and remember what's the address you added, so to remove > it later if needed (not by index, by address). Acknowledged. https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... src/applications/model/dhcp-client.cc:306: ipv4->SetUp (0); On 2016/02/10 23:40:01, Tommaso Pecorella wrote: > Why you're setting up the Loopback interface ? > It seems unnecessary. Done. https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... File src/applications/model/dhcp-client.h (right): https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... src/applications/model/dhcp-client.h:66: Ipv4Address GetDhcps (void); On 2016/02/10 23:40:02, Tommaso Pecorella wrote: > GetDhcpServer - otherwise it seems a plural form. Done. https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... src/applications/model/dhcp-client.h:78: DHCPNACK = 5 //!< Defining code for nack message On 2016/02/10 23:40:02, Tommaso Pecorella wrote: > These are the same numbers that are defined in the server. It's wise to place > them in the header, so they're not duplicated. Done. https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... src/applications/model/dhcp-client.h:83: IDLEE = 0, //!< Defining IDLE state of the client On 2016/02/10 23:40:02, Tommaso Pecorella wrote: > One extra "E" ? Done. https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... src/applications/model/dhcp-client.h:89: static const int RTRS_TIMEOUT = 1; //!< Defining the time for retransmission On 2016/02/10 23:40:02, Tommaso Pecorella wrote: > Better an attribute for this. the user could want to play with it. Acknowledged. https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... src/applications/model/dhcp-client.h:91: static const int DHCP_IPC_PORT = 6868; //!< Defining DHCP IPC port On 2016/02/10 23:40:02, Tommaso Pecorella wrote: > I'd move this to the .cc file. Done. https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... src/applications/model/dhcp-client.h:139: uint32_t m_lease; //!< Store the lease time of the address On 2016/02/10 23:40:02, Tommaso Pecorella wrote: > if you need to store a time, use Time, not integers. Acknowledged. https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... src/applications/model/dhcp-client.h:140: Ptr<UniformRandomVariable> m_ran; //!< Uniform random variable for transaction ID On 2016/02/10 23:40:02, Tommaso Pecorella wrote: > Use Streams Acknowledged. https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... File src/applications/model/dhcp-header.cc (right): https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... src/applications/model/dhcp-header.cc:301: i.Read (m_magic_cookie,4); On 2016/02/10 23:40:02, Tommaso Pecorella wrote: > Check the magic cookie value, it's there for this specific reason :P Done. https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... File src/applications/model/dhcp-header.h (right): https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... src/applications/model/dhcp-header.h:69: On 2016/02/10 23:40:02, Tommaso Pecorella wrote: > You missed the \verbatim commands. Plus, avoid the figure caption (remember that > it will end in a Doxygen webpage). Done. https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... src/applications/model/dhcp-header.h:117: void SetMagicCookie (); On 2016/02/10 23:40:02, Tommaso Pecorella wrote: > Since we don't really use this header as a BOOTP header, we can also hardcode > the magic cookie in the header constructor. > If we don't, there's a fat chance that the user will forget to call this > function. > > In other terms, I'd remove it altogether and I'd put the magic cookie setup in > the constructor. Done. https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... src/applications/model/dhcp-header.h:233: uint8_t m_hwpad[10]; //!< Current Implementation for 48 bit Mac Address, 10 bits padded On 2016/02/10 23:40:02, Tommaso Pecorella wrote: > I feel dumb at asking again. > These are 10 bytes, not 10 bits. > The specification says that we need 16 bytes for the hardware address (chaddr), > and we're using a 48 bits address (6), so 10 bytes padding makes sense. > What's not "right" is the portability. > One should accept any kind of address (also Mac64) and add the appropriate > padding on the flight. > Hardcoding is not wise. Acknowledged. https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... File src/applications/model/dhcp-server.h (right): https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... src/applications/model/dhcp-server.h:55: void RunEfsm (void); On 2016/02/10 23:40:02, Tommaso Pecorella wrote: > Please add all the relevant doxygen. > Suggestion: run "doc/doxygen.warnings.report.sh" and look at the warning > messages generated by dhcp files. Acknowledged. https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... src/applications/model/dhcp-server.h:59: friend inline std::ostream & operator<< (std::ostream& os, Ipv4Address &address) On 2016/02/10 23:40:02, Tommaso Pecorella wrote: > This function shouldn't be necessary, Ipv4Address can be already printed. Done. https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... src/applications/model/dhcp-server.h:78: static const int WORKING = 0; //!< Defining the working state of the server On 2016/02/10 23:40:02, Tommaso Pecorella wrote: > It doesn't seems very useful... Done.
Sign in to reply to this message.
Dear Sir, Patch 4 has been uploaded. Thank you very much for your time and valuable reviews. Sincerely, Ankit Deepak
Sign in to reply to this message.
Hi, I'm sorry if I seems to be extremely boring and utterly precise. The truth is that I am for real like that. Anyway, this is the last round of comments 'til the next global review. Don't rush and take care of all the comments before posting the new version. Cheers, T. PS. Don't forget that a documented bug is called a feature. I.e., you can choose to *not* implement something and just document that it's not implemented. One candidate is the DHCP relay, another is the client identifier, etc. https://codereview.appspot.com/279540043/diff/60001/src/applications/model/dh... File src/applications/model/dhcp-server.cc (right): https://codereview.appspot.com/279540043/diff/60001/src/applications/model/dh... src/applications/model/dhcp-server.cc:51: UintegerValue (67), Like for the Client, this is a standard port, not an Attribute. https://codereview.appspot.com/279540043/diff/60001/src/applications/model/dh... src/applications/model/dhcp-server.cc:55: "Lease for which address will be leased.", Indicate the time unit (seconds) https://codereview.appspot.com/279540043/diff/60001/src/applications/model/dh... src/applications/model/dhcp-server.cc:69: .AddAttribute ("Option_1", Option_1 ? Why this name ? https://codereview.appspot.com/279540043/diff/60001/src/applications/model/dh... src/applications/model/dhcp-server.cc:138: Simulator::Remove (m_expiredEvent); Delete the node's address from the leased list. Otherwise multiple Start / Stop will make the list grow. https://codereview.appspot.com/279540043/diff/60001/src/applications/model/dh... src/applications/model/dhcp-server.cc:142: { As usual, this function is not necessary. Remove it. https://codereview.appspot.com/279540043/diff/60001/src/applications/model/dh... src/applications/model/dhcp-server.cc:179: packet = Create<Packet> (); Don't create a packet if t's not needed. https://codereview.appspot.com/279540043/diff/60001/src/applications/model/dh... src/applications/model/dhcp-server.cc:186: if (header.GetType () == DhcpHeader::DHCPDISCOVER) This whole procedure doesn't seems to be compliant with section 4.3.1 Please triple check this one and the DHCPREQ logic. Moreover, if the two sections become too large, consider splitting them into two different functions. https://codereview.appspot.com/279540043/diff/60001/src/applications/model/dh... src/applications/model/dhcp-server.cc:191: for (;; ) //if no free address found, stay here for ever Err... no ? if no address is found, simply don't reply ! https://codereview.appspot.com/279540043/diff/60001/src/applications/model/dh... src/applications/model/dhcp-server.cc:199: m_IDhost = m_IDhost % (uint32_t)(pow (2, 32 - m_poolMask.GetPrefixLength ()) - 1) + 1; What is this for ? What is the purpose of m_IDhost ? Let me quote the RFC: A DHCP server needs to use some unique identifier to associate a client with its lease. The client MAY choose to explicitly provide the identifier through the 'client identifier' option. If the client supplies a 'client identifier', the client MUST use the same 'client identifier' in all subsequent messages, and the server MUST use that identifier to identify the client. If the client does not provide a 'client identifier' option, the server MUST use the contents of the 'chaddr' field to identify the client. m_IDhost is a wrong identifier. Suggestion: change the list to a map and use the chaddr as the map index. Explicitly say in the documentation that the 'client identifier' option is not used (perhaps suggest this as a future improvement). https://codereview.appspot.com/279540043/diff/60001/src/applications/model/dh... File src/applications/model/dhcp-server.h (right): https://codereview.appspot.com/279540043/diff/60001/src/applications/model/dh... src/applications/model/dhcp-server.h:41: * \defgroup dhcpclientserver DhcpClientServer You can use spaces here https://codereview.appspot.com/279540043/diff/60001/src/applications/model/dh... src/applications/model/dhcp-server.h:47: * \brief A Dhcp server. Add the main limitations (extremely brief statements, you can point to the manual) https://codereview.appspot.com/279540043/diff/60001/src/applications/model/dh... src/applications/model/dhcp-server.h:60: void RunEfsm (void); Why RunEfsm, NetHandler and TimerHandler are public ? The same question can be asked for the client https://codereview.appspot.com/279540043/diff/60001/src/applications/model/dh... src/applications/model/dhcp-server.h:90: Ipv4Address m_poolAddress; //!< The network address available to the server A pool represented by a single address and a mask ? Usually you use a range, see http://linux.die.net/man/5/dhcpd.conf I'm not saying that we should be compatible with the Linux configuration (even though it could be cool), but at least let's use a range.
Sign in to reply to this message.
Dear Sir, Apologies for the delayed response; there were mid term exams. I have uploaded patch 5 and addressed most of the suggestions, except a couple in which I have queries. I'm sorry; forgot to document that DHCP relay is not implemented. Realized this after I uploaded the patch. I've added it in my local repo. Thank you very much for the detailed reviews. I look forward to your suggestions on patch 5. Sincerely, Ankit Deepak https://codereview.appspot.com/279540043/diff/40001/src/applications/examples... File src/applications/examples/dhcp-example.cc (right): https://codereview.appspot.com/279540043/diff/40001/src/applications/examples... src/applications/examples/dhcp-example.cc:92: // following is a workaround (to support undirected broadcast)!!!!! On 2016/02/10 23:40:01, Tommaso Pecorella wrote: > This workaround is disturbing. > A.k.A. > Do or do not. There's no workaround. > > In other terms, if there's a problem, we fix the problem. Yes, there's a problem. I tried but couldn't solve it. The server is unable to receive messages destined to 255.255.255.255. Any suggestions would be much appreciated. https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... File src/applications/model/dhcp-client.cc (right): https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... src/applications/model/dhcp-client.cc:62: .AddAttribute ("NetDevice", "Index of netdevice of the node for DHCP", On 2016/02/10 23:40:01, Tommaso Pecorella wrote: > This is a lot more troublesome. > Theoretically the DHCP status and application should run on a per-device basis, > i.e., it should be possible to run it on different interfaces independently fro > each other (think to Wi-Fi and Ethernet). > We can *choose* to not do this and limit to one interface only, but it must be > documented. For now it has been limited to one interface only and the same has been documented. https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... src/applications/model/dhcp-client.cc:77: m_ran = CreateObject<UniformRandomVariable> (); On 2016/02/12 11:03:07, adadeepak8 wrote: > On 2016/02/10 23:40:02, Tommaso Pecorella wrote: > > Use the new Random Streams syntax. See for example ArpL3Protocol (and its > > helper). > > Acknowledged. Done. https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... src/applications/model/dhcp-client.cc:156: void DhcpClient::NetHandler (Ptr<Socket> socket) On 2016/02/12 11:03:07, adadeepak8 wrote: > On 2016/02/10 23:40:01, Tommaso Pecorella wrote: > > Why a function to just call another function ? Use only one ! > > Acknowledged. Done. https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... src/applications/model/dhcp-client.cc:168: RunEfsm (); On 2016/02/12 11:03:06, adadeepak8 wrote: > On 2016/02/10 23:40:01, Tommaso Pecorella wrote: > > There's something strange here... Why are you calling RunEfsm ? (more later). > > Acknowledged. Done. https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... src/applications/model/dhcp-client.cc:182: ipv4MN->AddAddress (ifIndex, Ipv4InterfaceAddress (Ipv4Address ("0.0.0.0"), Ipv4Mask ("/0"))); On 2016/02/12 11:03:07, adadeepak8 wrote: > On 2016/02/10 23:40:01, Tommaso Pecorella wrote: > > Why did you remove the RemoveAddress ? > > Indeed, you should add the "0.0.0.0" address, you should remove the previous > > one. > > Acknowledged. Done. https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... src/applications/model/dhcp-client.cc:190: m_state = IDLEE; On 2016/02/12 11:03:07, adadeepak8 wrote: > On 2016/02/10 23:40:01, Tommaso Pecorella wrote: > > Instead of forcing a status, shouldn't you check that you're in the right > status > > ? > > Acknowledged. Done. https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... src/applications/model/dhcp-client.cc:194: void DhcpClient::RunEfsm (void) On 2016/02/12 11:03:07, adadeepak8 wrote: > On 2016/02/10 23:40:02, Tommaso Pecorella wrote: > > ... more here. > > This function does too much. > > It is both used to send periodic DHCP requests, and to process incoming > replies. > > Split the functionalities in two different functions, and enforce the state > > machine consistency with proper checks. > > > > One way to achieve this is to use a sub-class (e.g., DhcpClient::Status) and > to > > have only setter/getter methods, where the setter can enforce that only > "legal" > > transitions are allowed. > > > > The periodic send can just check: if the status is IDLE, then send and > > reschedule. Otherwise return. > > Acknowledged. Done. https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... src/applications/model/dhcp-client.cc:212: header.SetChaddr (Mac48Address::ConvertFrom (GetNode ()->GetDevice (device)->GetAddress ())); On 2016/02/10 23:40:01, Tommaso Pecorella wrote: > Double conversion, useless. I didn't get your point, can you please describe it? https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... src/applications/model/dhcp-client.cc:264: m_state = WAIT_ACK; On 2016/02/12 11:03:07, adadeepak8 wrote: > On 2016/02/10 23:40:01, Tommaso Pecorella wrote: > > Nope, you're going into WAIT_ACK even if the packet isn't correct. > > Acknowledged. Done. https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... src/applications/model/dhcp-client.cc:276: packet = m_socket->RecvFrom (from); On 2016/02/10 23:40:01, Tommaso Pecorella wrote: > Is there a specific need to do this ? > I mean, why should you do it here and not before ? Done. https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... src/applications/model/dhcp-client.cc:291: ipv4->RemoveAddress (ifIndex, 0); On 2016/02/12 11:03:07, adadeepak8 wrote: > On 2016/02/10 23:40:01, Tommaso Pecorella wrote: > > No. Here you're assuming that there is an "old" address or a fake one. > > Remember that an interface can have multiple (legal9 IP addresses, and you're > > blindly removing one. Just... no. > > Acknowledged. Done. https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... src/applications/model/dhcp-client.cc:293: ipv4->AddAddress (ifIndex, Ipv4InterfaceAddress ((Ipv4Address)m_myAddress, m_myMask)); On 2016/02/12 11:03:07, adadeepak8 wrote: > On 2016/02/10 23:40:02, Tommaso Pecorella wrote: > > HERE you should find out and remember what's the address you added, so to > remove > > it later if needed (not by index, by address). > > Acknowledged. Done. https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... File src/applications/model/dhcp-client.h (right): https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... src/applications/model/dhcp-client.h:89: static const int RTRS_TIMEOUT = 1; //!< Defining the time for retransmission On 2016/02/12 11:03:07, adadeepak8 wrote: > On 2016/02/10 23:40:02, Tommaso Pecorella wrote: > > Better an attribute for this. the user could want to play with it. > > Acknowledged. Done. https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... src/applications/model/dhcp-client.h:139: uint32_t m_lease; //!< Store the lease time of the address On 2016/02/12 11:03:07, adadeepak8 wrote: > On 2016/02/10 23:40:02, Tommaso Pecorella wrote: > > if you need to store a time, use Time, not integers. > > Acknowledged. Done. https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... src/applications/model/dhcp-client.h:140: Ptr<UniformRandomVariable> m_ran; //!< Uniform random variable for transaction ID On 2016/02/12 11:03:07, adadeepak8 wrote: > On 2016/02/10 23:40:02, Tommaso Pecorella wrote: > > Use Streams > > Acknowledged. Done. https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... File src/applications/model/dhcp-header.h (right): https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... src/applications/model/dhcp-header.h:233: uint8_t m_hwpad[10]; //!< Current Implementation for 48 bit Mac Address, 10 bits padded On 2016/02/10 23:40:02, Tommaso Pecorella wrote: > I feel dumb at asking again. > These are 10 bytes, not 10 bits. > The specification says that we need 16 bytes for the hardware address (chaddr), > and we're using a 48 bits address (6), so 10 bytes padding makes sense. > What's not "right" is the portability. > One should accept any kind of address (also Mac64) and add the appropriate > padding on the flight. > Hardcoding is not wise. Done. https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... File src/applications/model/dhcp-server.h (right): https://codereview.appspot.com/279540043/diff/40001/src/applications/model/dh... src/applications/model/dhcp-server.h:55: void RunEfsm (void); On 2016/02/10 23:40:02, Tommaso Pecorella wrote: > Please add all the relevant doxygen. > Suggestion: run "doc/doxygen.warnings.report.sh" and look at the warning > messages generated by dhcp files. Done. https://codereview.appspot.com/279540043/diff/60001/src/applications/model/dh... File src/applications/model/dhcp-server.cc (right): https://codereview.appspot.com/279540043/diff/60001/src/applications/model/dh... src/applications/model/dhcp-server.cc:51: UintegerValue (67), On 2016/02/12 21:56:08, Tommaso Pecorella wrote: > Like for the Client, this is a standard port, not an Attribute. Done. https://codereview.appspot.com/279540043/diff/60001/src/applications/model/dh... src/applications/model/dhcp-server.cc:55: "Lease for which address will be leased.", On 2016/02/12 21:56:08, Tommaso Pecorella wrote: > Indicate the time unit (seconds) Done. https://codereview.appspot.com/279540043/diff/60001/src/applications/model/dh... src/applications/model/dhcp-server.cc:69: .AddAttribute ("Option_1", On 2016/02/12 21:56:08, Tommaso Pecorella wrote: > Option_1 ? > Why this name ? Done. https://codereview.appspot.com/279540043/diff/60001/src/applications/model/dh... src/applications/model/dhcp-server.cc:138: Simulator::Remove (m_expiredEvent); On 2016/02/12 21:56:08, Tommaso Pecorella wrote: > Delete the node's address from the leased list. Otherwise multiple Start / Stop > will make the list grow. Done. https://codereview.appspot.com/279540043/diff/60001/src/applications/model/dh... src/applications/model/dhcp-server.cc:142: { On 2016/02/12 21:56:08, Tommaso Pecorella wrote: > As usual, this function is not necessary. Remove it. Done. https://codereview.appspot.com/279540043/diff/60001/src/applications/model/dh... src/applications/model/dhcp-server.cc:179: packet = Create<Packet> (); On 2016/02/12 21:56:08, Tommaso Pecorella wrote: > Don't create a packet if t's not needed. Done. https://codereview.appspot.com/279540043/diff/60001/src/applications/model/dh... src/applications/model/dhcp-server.cc:186: if (header.GetType () == DhcpHeader::DHCPDISCOVER) On 2016/02/12 21:56:08, Tommaso Pecorella wrote: > This whole procedure doesn't seems to be compliant with section 4.3.1 > Please triple check this one and the DHCPREQ logic. > > Moreover, if the two sections become too large, consider splitting them into two > different functions. Done. https://codereview.appspot.com/279540043/diff/60001/src/applications/model/dh... src/applications/model/dhcp-server.cc:191: for (;; ) //if no free address found, stay here for ever On 2016/02/12 21:56:08, Tommaso Pecorella wrote: > Err... no ? > if no address is found, simply don't reply ! Done. https://codereview.appspot.com/279540043/diff/60001/src/applications/model/dh... src/applications/model/dhcp-server.cc:199: m_IDhost = m_IDhost % (uint32_t)(pow (2, 32 - m_poolMask.GetPrefixLength ()) - 1) + 1; Sir, The RFC says "The combination of 'client identifier' or 'chaddr' and assigned network address constitute a unique identifier for the client's lease and are used by both the client and server to identify a lease referred to in any DHCP messages." I have modified the code to include chaddr now, but I am keeping m_IDhost (m_nextAddressSeq is the new name) which represents the network address part of the leased address. Is this correct to do? https://codereview.appspot.com/279540043/diff/60001/src/applications/model/dh... File src/applications/model/dhcp-server.h (right): https://codereview.appspot.com/279540043/diff/60001/src/applications/model/dh... src/applications/model/dhcp-server.h:41: * \defgroup dhcpclientserver DhcpClientServer On 2016/02/12 21:56:09, Tommaso Pecorella wrote: > You can use spaces here Done. https://codereview.appspot.com/279540043/diff/60001/src/applications/model/dh... src/applications/model/dhcp-server.h:47: * \brief A Dhcp server. On 2016/02/12 21:56:09, Tommaso Pecorella wrote: > Add the main limitations (extremely brief statements, you can point to the > manual) Done. https://codereview.appspot.com/279540043/diff/60001/src/applications/model/dh... src/applications/model/dhcp-server.h:60: void RunEfsm (void); On 2016/02/12 21:56:09, Tommaso Pecorella wrote: > Why RunEfsm, NetHandler and TimerHandler are public ? > The same question can be asked for the client Done. https://codereview.appspot.com/279540043/diff/60001/src/applications/model/dh... src/applications/model/dhcp-server.h:90: Ipv4Address m_poolAddress; //!< The network address available to the server On 2016/02/12 21:56:09, Tommaso Pecorella wrote: > A pool represented by a single address and a mask ? > Usually you use a range, see http://linux.die.net/man/5/dhcpd.conf > I'm not saying that we should be compatible with the Linux configuration (even > though it could be cool), but at least let's use a range. Done.
Sign in to reply to this message.
Dear Sir, I've shifted the DHCP code to internet-apps in patch set 6, as suggested by you in the review of patch set 1. The doc also has been updated to mention the support of multiple DHCP servers and the lack of support for DHCP Relay. I would be glad to know your views and suggestions on this patch. Sincerely, Ankit Deepak
Sign in to reply to this message.
Hi,
sorry for the delay, my bad. Please find some more comments. The next review
cold be the end one, tho :)
One note that didn't fit elsewhere: you forgot ".SetGroupName ("Internet-Apps")"
in some classes, please check.
As a side note: I commented an "old" revision because I had the side-by-side
diff in this way. The newest is identical to the one I commented with only some
very minor things related to the module move.
https://codereview.appspot.com/279540043/diff/80001/src/applications/model/dh...
File src/applications/model/dhcp-client.cc (right):
https://codereview.appspot.com/279540043/diff/80001/src/applications/model/dh...
src/applications/model/dhcp-client.cc:152: ipv4->AddAddress (ifIndex,
Ipv4InterfaceAddress (Ipv4Address ("0.0.0.0"),Ipv4Mask ("/0")));
No need to add another address. On the other hand, the above code is wrong.
You must da a for on the interface addresses to find out what is the right
address index. An interface can have more than one IP address, and removing the
0-th one is not safe.
https://codereview.appspot.com/279540043/diff/80001/src/applications/model/dh...
src/applications/model/dhcp-client.cc:173: ipv4MN->RemoveAddress (ifIndex, 0);
same comment as before.
https://codereview.appspot.com/279540043/diff/80001/src/applications/model/dh...
File src/applications/model/dhcp-header.cc (right):
https://codereview.appspot.com/279540043/diff/80001/src/applications/model/dh...
src/applications/model/dhcp-header.cc:290: WriteTo (i,m_chAddr64);
Here you'd need 8 bytes more. still, it's not necessary if you do as I said for
chaddr.
https://codereview.appspot.com/279540043/diff/80001/src/applications/model/dh...
File src/applications/model/dhcp-header.h (right):
https://codereview.appspot.com/279540043/diff/80001/src/applications/model/dh...
src/applications/model/dhcp-header.h:156: void SetChaddr48 (Mac48Address addr);
Unnecessary, and wrong. Citing the RFC:
DHCP defines a new 'client identifier' option that is used to pass an
explicit client identifier to a DHCP server. This change eliminates
the overloading of the 'chaddr' field in BOOTP messages, where
'chaddr' is used both as a hardware address for transmission of BOOTP
reply messages and as a client identifier. The 'client identifier'
is an opaque key, not to be interpreted by the server; for example,
the 'client identifier' may contain a hardware address, identical to
the contents of the 'chaddr' field, or it may contain another type of
identifier, such as a DNS name. The 'client identifier' chosen by a
DHCP client MUST be unique to that client within the subnet to which
the client is attached. If the client uses a 'client identifier' in
one message, it MUST use that same identifier in all subsequent
messages, to ensure that all servers correctly identify the client.
Despite the historical name, "chaddr" is not client hardware address, it's just
an opaque identifier, that *can* be tied to the hardware address. As a matter of
fact in DHCPv6 it isn't.
Nevertheless, nothing forbids us to use a "plain" Address converted into a byte
array.
Use Address::CopyAllTo or Address::CopyTo. The first will use 2 more bytes for
the type and length of the address.
It shouldn't be really a problem tho, as chaddr should be used in the server
only as an identifier.
https://codereview.appspot.com/279540043/diff/80001/src/applications/model/dh...
src/applications/model/dhcp-header.h:283: Mac64Address m_chAddr64;
//!< The Mac64Address of the client
uint8_t m_chaddr[16];
https://codereview.appspot.com/279540043/diff/80001/src/applications/model/dh...
src/applications/model/dhcp-header.h:290: uint8_t m_hwpad[10];
//!< When entered address is 48 bit Mac Address, 10 bits padded
padding is unnecessary.
https://codereview.appspot.com/279540043/diff/80001/src/applications/model/dh...
File src/applications/model/dhcp-server.cc (right):
https://codereview.appspot.com/279540043/diff/80001/src/applications/model/dh...
src/applications/model/dhcp-server.cc:135: m_leasedAddresses.insert
(std::make_pair (std::make_pair (Mac48Address::ConvertFrom (GetNode
()->GetDevice (ifIndex)->GetAddress ()), id), 0xffffffff)); // set infinite
GRANTED_LEASED_TIME for my address
Don't assume that the local address is a Mac48.
Always use a IsCompatible.
https://codereview.appspot.com/279540043/diff/80001/src/applications/model/dh...
File src/applications/model/dhcp-server.h (right):
https://codereview.appspot.com/279540043/diff/80001/src/applications/model/dh...
src/applications/model/dhcp-server.h:115: std::map<std::pair<Mac48Address,
uint32_t>, uint32_t> m_leasedAddresses; //!< Leased address and their status
(cache memory)
You lease IP addresses to a specific client.
I'd expect a list of triplets <Ipv4Address, chaddr, status>, where chaddr is
"just" the client identifier.
Sign in to reply to this message.
Dear Sir,
Thank you for your suggestions. I am really sorry for the delay, I will
implement all the changes and submit the next patch soon.
Sincerely,
Ankit Deepak
Sign in to reply to this message.
Dear Sir, I am very sorry for the delay, but the next version incorporating all the changes have been uploaded. Sincerely, Ankit Deepak https://codereview.appspot.com/279540043/diff/80001/src/applications/model/dh... File src/applications/model/dhcp-client.cc (right): https://codereview.appspot.com/279540043/diff/80001/src/applications/model/dh... src/applications/model/dhcp-client.cc:152: ipv4->AddAddress (ifIndex, Ipv4InterfaceAddress (Ipv4Address ("0.0.0.0"),Ipv4Mask ("/0"))); On 2016/05/27 01:16:04, Tommaso Pecorella wrote: > No need to add another address. On the other hand, the above code is wrong. > You must da a for on the interface addresses to find out what is the right > address index. An interface can have more than one IP address, and removing the > 0-th one is not safe. Done. https://codereview.appspot.com/279540043/diff/80001/src/applications/model/dh... src/applications/model/dhcp-client.cc:173: ipv4MN->RemoveAddress (ifIndex, 0); On 2016/05/27 01:16:04, Tommaso Pecorella wrote: > same comment as before. Done. https://codereview.appspot.com/279540043/diff/80001/src/applications/model/dh... File src/applications/model/dhcp-header.cc (right): https://codereview.appspot.com/279540043/diff/80001/src/applications/model/dh... src/applications/model/dhcp-header.cc:290: WriteTo (i,m_chAddr64); On 2016/05/27 01:16:04, Tommaso Pecorella wrote: > Here you'd need 8 bytes more. still, it's not necessary if you do as I said for > chaddr. Done. https://codereview.appspot.com/279540043/diff/80001/src/applications/model/dh... File src/applications/model/dhcp-header.h (right): https://codereview.appspot.com/279540043/diff/80001/src/applications/model/dh... src/applications/model/dhcp-header.h:156: void SetChaddr48 (Mac48Address addr); On 2016/05/27 01:16:04, Tommaso Pecorella wrote: > Unnecessary, and wrong. Citing the RFC: > > DHCP defines a new 'client identifier' option that is used to pass an > explicit client identifier to a DHCP server. This change eliminates > the overloading of the 'chaddr' field in BOOTP messages, where > 'chaddr' is used both as a hardware address for transmission of BOOTP > reply messages and as a client identifier. The 'client identifier' > is an opaque key, not to be interpreted by the server; for example, > the 'client identifier' may contain a hardware address, identical to > the contents of the 'chaddr' field, or it may contain another type of > identifier, such as a DNS name. The 'client identifier' chosen by a > DHCP client MUST be unique to that client within the subnet to which > the client is attached. If the client uses a 'client identifier' in > one message, it MUST use that same identifier in all subsequent > messages, to ensure that all servers correctly identify the client. > > Despite the historical name, "chaddr" is not client hardware address, it's just > an opaque identifier, that *can* be tied to the hardware address. As a matter of > fact in DHCPv6 it isn't. > Nevertheless, nothing forbids us to use a "plain" Address converted into a byte > array. > Use Address::CopyAllTo or Address::CopyTo. The first will use 2 more bytes for > the type and length of the address. > It shouldn't be really a problem tho, as chaddr should be used in the server > only as an identifier. Done. https://codereview.appspot.com/279540043/diff/80001/src/applications/model/dh... src/applications/model/dhcp-header.h:283: Mac64Address m_chAddr64; //!< The Mac64Address of the client On 2016/05/27 01:16:04, Tommaso Pecorella wrote: > uint8_t m_chaddr[16]; Done. https://codereview.appspot.com/279540043/diff/80001/src/applications/model/dh... src/applications/model/dhcp-header.h:290: uint8_t m_hwpad[10]; //!< When entered address is 48 bit Mac Address, 10 bits padded On 2016/05/27 01:16:04, Tommaso Pecorella wrote: > padding is unnecessary. Done. https://codereview.appspot.com/279540043/diff/80001/src/applications/model/dh... File src/applications/model/dhcp-server.cc (right): https://codereview.appspot.com/279540043/diff/80001/src/applications/model/dh... src/applications/model/dhcp-server.cc:135: m_leasedAddresses.insert (std::make_pair (std::make_pair (Mac48Address::ConvertFrom (GetNode ()->GetDevice (ifIndex)->GetAddress ()), id), 0xffffffff)); // set infinite GRANTED_LEASED_TIME for my address On 2016/05/27 01:16:04, Tommaso Pecorella wrote: > Don't assume that the local address is a Mac48. > Always use a IsCompatible. Done. https://codereview.appspot.com/279540043/diff/80001/src/applications/model/dh... File src/applications/model/dhcp-server.h (right): https://codereview.appspot.com/279540043/diff/80001/src/applications/model/dh... src/applications/model/dhcp-server.h:115: std::map<std::pair<Mac48Address, uint32_t>, uint32_t> m_leasedAddresses; //!< Leased address and their status (cache memory) On 2016/05/27 01:16:04, Tommaso Pecorella wrote: > You lease IP addresses to a specific client. > I'd expect a list of triplets <Ipv4Address, chaddr, status>, where chaddr is > "just" the client identifier. Done.
Sign in to reply to this message.
Hi Ankit, I'm deeply sorry that it took this long. There are a couple of last things in a small portion of code. After that I think it's ready for inclusion. Cheers, T. https://codereview.appspot.com/279540043/diff/120001/src/internet-apps/model/... File src/internet-apps/model/dhcp-client.cc (right): https://codereview.appspot.com/279540043/diff/120001/src/internet-apps/model/... src/internet-apps/model/dhcp-client.cc:129: } If I'm right this should add the "0.0.0.0" address to an interface if it doesn't have one yet. I'm not really sure that this is needed / wanted. Point 1: ipv4->GetInterfaceForAddress (m_myAddress) could return true in case of more than one DHCP apps (on different interfaces). Point 2: what this address is used for ? https://codereview.appspot.com/279540043/diff/120001/src/internet-apps/model/... src/internet-apps/model/dhcp-client.cc:165: } You should only remove the addresses you got, not all of them (an interface could have a static AND a dynamic address). https://codereview.appspot.com/279540043/diff/120001/src/internet-apps/model/... src/internet-apps/model/dhcp-client.cc:347: Same as before: you should remove the address you got earlier (perhaps), not all of them.
Sign in to reply to this message.
Dear sir, I have updated the DHCP patch and uploaded it here. I have tried to incorporate all the suggestions. Thank you very much for your guidance. Any suggestions / corrections on the updated patch would be much appreciated. Sincerely, Ankit Deepak https://codereview.appspot.com/279540043/diff/120001/src/internet-apps/model/... File src/internet-apps/model/dhcp-client.cc (right): https://codereview.appspot.com/279540043/diff/120001/src/internet-apps/model/... src/internet-apps/model/dhcp-client.cc:129: } I modified to get interface for the device on which app is installed. This just installs a 0.0.0.0 address on the device if it is not there, as in the beginning of DHCP the client uses the said address https://codereview.appspot.com/279540043/diff/120001/src/internet-apps/model/... src/internet-apps/model/dhcp-client.cc:165: } Modified this too to get interface for device and remove the configured address (m_myAddress from the device) https://codereview.appspot.com/279540043/diff/120001/src/internet-apps/model/... src/internet-apps/model/dhcp-client.cc:347: On 2016/11/12 13:10:02, Tommaso Pecorella wrote: > Same as before: you should remove the address you got earlier (perhaps), not all > of them. Done.
Sign in to reply to this message.
This is the last comment. From my side I have no more things to add (except for a stress test). One user said that in a handover case, where the STA is moving from one AP to another AP (both running DHCP) the routes are not updated in the client. He didn't provide a test script tho. Nevertheless, it's something to check. Perhaps it would also be a good example. https://codereview.appspot.com/279540043/diff/140001/src/internet-apps/model/... File src/internet-apps/model/dhcp-client.cc (right): https://codereview.appspot.com/279540043/diff/140001/src/internet-apps/model/... src/internet-apps/model/dhcp-client.cc:132: break; Set a bool variable here (e.g., "found") and test it later. Your intention will be clearer.
Sign in to reply to this message.
Dear Sir, Sorry for the delay, I had my final examinations for the semester over the past week. I was looking in to https://groups.google.com/forum/?fromgroups#!topic/ns-3-users/Hd0vS7a1AMY thread from user list, which I guess is the one you were referring to. So I think two changes would be required to the current DHCP model. 1) I need to update the routing table at the end of the address allocation process. 2) I need to link a callback to the start application of DHCP to the Link Change Callback of the net device. Please let me know this looks correct. Sincerely, Ankit Deepak
Sign in to reply to this message.
On 2016/11/28 07:08:59, adadeepak8 wrote: > Dear Sir, > > Sorry for the delay, I had my final examinations for the semester over the past > week. > > I was looking in to > https://groups.google.com/forum/?fromgroups#!topic/ns-3-users/Hd0vS7a1AMY thread > from user list, which I guess is the one you were referring to. > > So I think two changes would be required to the current DHCP model. > > 1) I need to update the routing table at the end of the address allocation > process. > 2) I need to link a callback to the start application of DHCP to the Link Change > Callback of the net device. > > Please let me know this looks correct. > > Sincerely, > Ankit Deepak Hi Ankit, yes, you're totally right. However you'll need the Router Option (code 3): 3.5. Router Option The router option specifies a list of IP addresses for routers on the client's subnet. Routers SHOULD be listed in order of preference. The code for the router option is 3. The minimum length for the router option is 4 octets, and the length MUST always be a multiple of 4. Code Len Address 1 Address 2 +-----+-----+-----+-----+-----+-----+-----+-----+-- | 3 | n | a1 | a2 | a3 | a4 | a1 | a2 | ... +-----+-----+-----+-----+-----+-----+-----+-----+-- Moreover, mind that... 3.3. Subnet Mask The subnet mask option specifies the client's subnet mask as per RFC 950 [5]. If both the subnet mask and the router option are specified in a DHCP reply, the subnet mask option MUST be first. The second point is fixed by re-starting the application if the link goes down. Cheers, T.
Sign in to reply to this message.
Sir, While adding the router option, do I make the list of routers that the server sends, a configurable attribute of server? Thanks, Ankit
Sign in to reply to this message.
Sign in to reply to this message.
Hi Ankit, (previous message was empty) I'd say that a single router is more than enough. To have it configurable through an attribute is a must tho. Cheers, T.
Sign in to reply to this message.
Sir, I have completed the setting of default router for client. But, the users thread mentions that the application server did not come to know of the configured address. Even in the example I am currently working on, I use a simple UDP ping application. The client is able to send message to the server, through the gateway, after DHCP configuration, but the server does not respond back because it does not have a route for the newly configured address. Now, if I am not wrong, a proactive/reactive routing protocol will detect this address change and update itself accordingly, but what about static routing? Is it the job of DHCP to update global routing tables (It is fine for a simulator, needs a populate table call but in real internet like scenario)? Thanks, Ankit
Sign in to reply to this message.
On 2017/01/10 16:45:20, adadeepak8 wrote: > Sir, > > I have completed the setting of default router for client. But, the users thread > mentions that the application server did not come to know of the configured > address. Even in the example I am currently working on, I use a simple UDP ping > application. The client is able to send message to the server, through the > gateway, after DHCP configuration, but the server does not respond back because > it does not have a route for the newly configured address. > > Now, if I am not wrong, a proactive/reactive routing protocol will detect this > address change and update itself accordingly, but what about static routing? Is > it the job of DHCP to update global routing tables (It is fine for a simulator, > needs a populate table call but in real internet like scenario)? > > Thanks, > Ankit Hi, In a real scenario the Wi-Fi network will be exposed as a subnet, not as a collection of nodes (this could also be an actual bug in StaticRouting). In ns-3, I'd say that it's subject to discussion. I don't like the idea of calling a global route recall for each new IP address lease, as it could slow down the simulation dramatically. Instead, I'd document the issue, and I'd suggest two alternatives: 1) use a dynamic routing, or 2) hook to the NewLease callback and recalculate global routing. By the way... do we have two callbacks for new leases and expired leases ? (sorry, forgot to ask...) Just a simple signature with the IP address of the new / expired lease, no need for one for lease renewals. Thanks, T.
Sign in to reply to this message.
Sir, I have included the router option and added two trace sources for new lease and expiry. I have also enhanced the example to show the working of router option after lease is given. Please let me know if you have more suggestions. I will be glad to incorporate. Thanks, Ankit Deepak https://codereview.appspot.com/279540043/diff/140001/src/internet-apps/model/... File src/internet-apps/model/dhcp-client.cc (right): https://codereview.appspot.com/279540043/diff/140001/src/internet-apps/model/... src/internet-apps/model/dhcp-client.cc:132: break; On 2016/11/18 22:59:16, Tommaso Pecorella wrote: > Set a bool variable here (e.g., "found") and test it later. Your intention will > be clearer. Done.
Sign in to reply to this message.
The more you look at some code, the more bugs you find. Embarrassing. After the things I pointed out, I really think we're done. https://codereview.appspot.com/279540043/diff/160001/src/internet-apps/model/... File src/internet-apps/model/dhcp-client.cc (right): https://codereview.appspot.com/279540043/diff/160001/src/internet-apps/model/... src/internet-apps/model/dhcp-client.cc:261: void DhcpClient::Boot (void) This is embarrassing... because we never noticed before. This function is (also) used to re-start a DHCP request if the client fails to refresh a lease. In this case the old address (and the default gateway, if it was set by DHCP) have to be removed from the node. Of course also the lease expiration trace must be called. https://codereview.appspot.com/279540043/diff/160001/src/internet-apps/model/... File src/internet-apps/model/dhcp-header.h (right): https://codereview.appspot.com/279540043/diff/160001/src/internet-apps/model/... src/internet-apps/model/dhcp-header.h:223: */ Wrong Doxygen description
Sign in to reply to this message.
On Tuesday, January 10, 2017 at 9:06:00 AM UTC-8, Tommaso Pecorella wrote: > > I don't like the idea > of calling a global route recall for each new IP address lease, as it > could slow down the simulation dramatically. > > https://codereview.appspot.com/279540043/ > A while ago we added lazy route flushing to nix vector (commit 597e36a6eb28), for just that reason: immediate flush was being triggered repeatedly on every node when a set of nodes went up (or down). Even with empty caches, as during initialization, the sheer number of function calls in a large model was killing us. Perhaps similar ideas would help in this situation... Peter
Sign in to reply to this message.
Sir, I have uploaded the updated patch. I scanned the whole code once more and enhanced a few more doxygen comments. Please let me know if any more corrections are needed. Thanks, Ankit https://codereview.appspot.com/279540043/diff/160001/src/internet-apps/model/... File src/internet-apps/model/dhcp-client.cc (right): https://codereview.appspot.com/279540043/diff/160001/src/internet-apps/model/... src/internet-apps/model/dhcp-client.cc:261: void DhcpClient::Boot (void) On 2017/01/13 00:43:32, Tommaso Pecorella wrote: > This is embarrassing... because we never noticed before. > > This function is (also) used to re-start a DHCP request if the client fails to > refresh a lease. > In this case the old address (and the default gateway, if it was set by DHCP) > have to be removed from the node. Of course also the lease expiration trace must > be called. Done. https://codereview.appspot.com/279540043/diff/160001/src/internet-apps/model/... File src/internet-apps/model/dhcp-header.h (right): https://codereview.appspot.com/279540043/diff/160001/src/internet-apps/model/... src/internet-apps/model/dhcp-header.h:223: */ On 2017/01/13 00:43:32, Tommaso Pecorella wrote: > Wrong Doxygen description Done.
Sign in to reply to this message.
I bet you're going to kill me. https://codereview.appspot.com/279540043/diff/180001/src/internet-apps/model/... File src/internet-apps/model/dhcp-client.cc (right): https://codereview.appspot.com/279540043/diff/180001/src/internet-apps/model/... src/internet-apps/model/dhcp-client.cc:187: } Remove only the address (and the gateway, if any) you got from the DHCP server https://codereview.appspot.com/279540043/diff/180001/src/internet-apps/model/... src/internet-apps/model/dhcp-client.cc:232: } Remove only the address (and the gateway, if any) you got from the DHCP server https://codereview.appspot.com/279540043/diff/180001/src/internet-apps/model/... src/internet-apps/model/dhcp-client.cc:311: DhcpHeader header = m_offerList.front (); Missing the else (or a return inside the previous if) https://codereview.appspot.com/279540043/diff/180001/src/internet-apps/model/... src/internet-apps/model/dhcp-client.cc:385: } Why are you removing extra addresses ? They could be perfectly fine. Don't remove what you didn't set. However, if you don't need anymore a "0.0.0.0" address on your interface, remove it. By the way, why it was necessary to add it in the first place ? Could it be simply avoided ? https://codereview.appspot.com/279540043/diff/180001/src/internet-apps/model/... src/internet-apps/model/dhcp-client.cc:397: } I strongly suspect a double brackets horror here https://codereview.appspot.com/279540043/diff/180001/src/internet-apps/model/... src/internet-apps/model/dhcp-client.cc:410: staticRouting->SetDefaultRoute (m_gateway, ifIndex, 0); Only set the gateway if it's not "0.0.0.0". Moreover, we have the gateway option now. Use it ! https://codereview.appspot.com/279540043/diff/180001/src/internet-apps/model/... src/internet-apps/model/dhcp-client.cc:422: void DhcpClient::RemoveAndStart () You must remove ONLY the address you got from DHCP and the gateway you got from DHCP (if any). The interface could have more addresses and even more default outer (with a different metric).
Sign in to reply to this message.
Hi, I don't want to rush things, but... I'd really love to push this in the next release :) Cheers, T.
Sign in to reply to this message.
Sir, Sorry for the delay. I have incorporated most of the changes, but I have two queries with respect to your suggestions: 1. Everytime I remove m_myAddress, which contains the address which I/server set. I cannot think of a case when it will remove an address not set by the DHCP module itself. As for the requirement of 0.0.0.0, if no address is set on the client initially, it gives an underflow at interface. I don't know how to avoid that without setting a 0.0.0.0 address. Can you please suggest a way? 2. While removing the gateway, can I assume that if a route has same values for destination, gateway and metric as the one set during configuration, then it is the route to be removed? Thanks, Ankit
Sign in to reply to this message.
On 2017/02/26 04:30:02, adadeepak8 wrote: > Sir, > > Sorry for the delay. I have incorporated most of the changes, but I have two > queries with respect to your suggestions: > > 1. Everytime I remove m_myAddress, which contains the address which I/server > set. I cannot think of a case when it will remove an address not set by the DHCP > module itself. As for the requirement of 0.0.0.0, if no address is set on the > client initially, it gives an underflow at interface. I don't know how to avoid > that without setting a 0.0.0.0 address. Can you please suggest a way? > > > 2. While removing the gateway, can I assume that if a route has same values for > destination, gateway and metric as the one set during configuration, then it is > the route to be removed? > > Thanks, > Ankit Hi, it seems that I didn't fully check what I was asking... and some comments are redundant. I double checked (again) and, indeed, the addresses are removed correctly almost everywhere. About the gateway, you can check the interface as well, just to be extra sure: "staticRouting->GetRoute (i).GetInterface () == ifIndex" The same goes for the metric and the destination (if we want to be extra, extra sure). About the 0.0.0.0 problem... an underflow? Really? I'll check with a minimal script.
Sign in to reply to this message.
Sir, If you would like to check the code with 0.0.0.0 removed, I have pushed it at https://github.com/adeepkit01/ns-3-DHCP/tree/dhcp-test. If you look into it, please let me know if I am making any mistake after removing the addresses. Thanks, Ankit
Sign in to reply to this message.
On 2017/03/02 10:44:49, adadeepak8 wrote: > Sir, > > If you would like to check the code with 0.0.0.0 removed, I have pushed it at > https://github.com/adeepkit01/ns-3-DHCP/tree/dhcp-test. > > If you look into it, please let me know if I am making any mistake after > removing the addresses. > > Thanks, > Ankit Hi, I checked, and indeed it's a "problem" in the IP layer. Basically you can't send an IP packet if the interface doesn't have any IP address at all. As a consequence, the only possible solutions are: 1) Add a temporary IP address, "0.0.0.0" is fine, or 2) Send the packet directly through the NetDevice. I'd avoid option 2 because it introduces even more issues. However, I'd be a bit more strict on the use of "0.0.0.0", i.e.: a) Don't add it in the example, the application must be able to handle everything, and b) Remove the "0.0.0.0" address as soon as it's not anymore needed, as is after you got your address from the server. I'd suggest this approach: - When you have to send a DHCP request, check if the interface has one address (it could be a static address) - if not add "0.0.0.0" - when you receive the address, remove the "0.0.0.0" (if there's a "0.0.0.0", of course). last but not least, we forgot that you need to add a /** * \ingroup internet-apps * \defgroup dhcp DHCPv4 Client and Server */ somewhere in an header. Cheers, T.
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
