Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(2623)

Issue 279540043: DHCP Implementation in ns-3.24 (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
8 years, 3 months ago by Mohit Tahiliani
Modified:
6 years, 8 months ago
Reviewers:
adadeepak8, mattator, barnes26, Tommaso Pecorella
CC:
ns-3-reviews_googlegroups.com, deeptir96_gmail.com
Visibility:
Public.

Description

This 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
Unified diffs Side-by-side diffs Delta from patch set Stats (+2586 lines, -0 lines) Patch
A src/internet-apps/doc/dhcp.rst View 1 2 3 4 5 1 chunk +66 lines, -0 lines 0 comments Download
A src/internet-apps/examples/dhcp-example.cc View 1 2 3 4 5 6 7 8 1 chunk +165 lines, -0 lines 0 comments Download
A src/internet-apps/examples/wscript View 1 2 3 4 5 1 chunk +8 lines, -0 lines 0 comments Download
A src/internet-apps/helper/dhcp-helper.h View 1 2 3 4 5 6 7 8 9 1 chunk +121 lines, -0 lines 0 comments Download
A src/internet-apps/helper/dhcp-helper.cc View 1 2 3 4 5 6 7 8 1 chunk +90 lines, -0 lines 0 comments Download
A src/internet-apps/model/dhcp-client.h View 1 2 3 4 5 6 7 8 9 1 chunk +183 lines, -0 lines 0 comments Download
A src/internet-apps/model/dhcp-client.cc View 1 2 3 4 5 6 7 8 9 1 chunk +456 lines, -0 lines 7 comments Download
A src/internet-apps/model/dhcp-header.h View 1 2 3 4 5 6 7 8 9 1 chunk +303 lines, -0 lines 0 comments Download
A src/internet-apps/model/dhcp-header.cc View 1 2 3 4 5 6 7 8 1 chunk +508 lines, -0 lines 0 comments Download
A src/internet-apps/model/dhcp-server.h View 1 2 3 4 5 6 7 8 1 chunk +127 lines, -0 lines 0 comments Download
A src/internet-apps/model/dhcp-server.cc View 1 2 3 4 5 6 7 8 9 1 chunk +381 lines, -0 lines 0 comments Download
A src/internet-apps/test/dhcp-test.cc View 1 2 3 4 5 6 7 8 1 chunk +163 lines, -0 lines 0 comments Download
M src/internet-apps/wscript View 1 2 3 4 5 2 chunks +15 lines, -0 lines 0 comments Download

Messages

Total messages: 38
Tommaso Pecorella
Hi, I'm reviewing the code, but before we go further it's mandatory to do the ...
8 years, 3 months ago (2015-12-28 00:13:41 UTC) #1
adadeepak8
On 2015/12/28 00:13:41, Tommaso Pecorella wrote: > Hi, > > I'm reviewing the code, but ...
8 years, 2 months ago (2016-01-05 09:47:11 UTC) #2
mattator
Dear Ankit, Thanks for the contribution. I am curious what's your motive for reimplementing DHCP ...
8 years, 2 months ago (2016-01-05 14:08:51 UTC) #3
adadeepak8
On 2016/01/05 14:08:51, mattator wrote: > Dear Ankit, > > Thanks for the contribution. > ...
8 years, 2 months ago (2016-01-06 08:50:09 UTC) #4
adadeepak8
Dear Tommaso Sir, I have updated the DHCP patch and uploaded it here. Almost all ...
8 years, 1 month ago (2016-02-01 11:01:18 UTC) #5
Tommaso Pecorella
Second round, still only on headers (sorry, but I have little time and I prefer ...
8 years, 1 month ago (2016-02-02 23:11:40 UTC) #6
adadeepak8
Dear Tommaso Sir, Except for the query related to deserialize function, I've incorporated all the ...
8 years, 1 month ago (2016-02-04 10:33:41 UTC) #7
Tommaso Pecorella
Hi Ankit, thanks for your very quick replies. I hope to be able to be ...
8 years, 1 month ago (2016-02-04 11:20:58 UTC) #8
adadeepak8
Dear Tommaso Sir, I have uploaded patch 3 with following changes: - created the examples ...
8 years, 1 month ago (2016-02-08 11:32:57 UTC) #9
Tommaso Pecorella
Hi, here's another round of comments. I still need to check the server tho... expect ...
8 years, 1 month ago (2016-02-10 23:40:02 UTC) #10
adadeepak8
Dear Sir, I have incorporated 14 suggestions out of 32, and working on the rest. ...
8 years, 1 month ago (2016-02-12 11:03:08 UTC) #11
adadeepak8
Dear Sir, Patch 4 has been uploaded. Thank you very much for your time and ...
8 years, 1 month ago (2016-02-12 11:39:11 UTC) #12
Tommaso Pecorella
Hi, I'm sorry if I seems to be extremely boring and utterly precise. The truth ...
8 years, 1 month ago (2016-02-12 21:56:09 UTC) #13
adadeepak8
Dear Sir, Apologies for the delayed response; there were mid term exams. I have uploaded ...
8 years ago (2016-03-01 14:10:02 UTC) #14
adadeepak8
Dear Sir, I've shifted the DHCP code to internet-apps in patch set 6, as suggested ...
7 years, 10 months ago (2016-05-06 06:47:55 UTC) #15
Tommaso Pecorella
Hi, sorry for the delay, my bad. Please find some more comments. The next review ...
7 years, 10 months ago (2016-05-27 01:16:04 UTC) #16
adadeepak8
Dear Sir, Thank you for your suggestions. I am really sorry for the delay, I ...
7 years, 9 months ago (2016-06-16 20:36:27 UTC) #17
adadeepak8
Dear Sir, I am very sorry for the delay, but the next version incorporating all ...
7 years, 7 months ago (2016-08-29 04:09:42 UTC) #18
Tommaso Pecorella
Hi Ankit, I'm deeply sorry that it took this long. There are a couple of ...
7 years, 4 months ago (2016-11-12 13:10:02 UTC) #19
adadeepak8
Dear sir, I have updated the DHCP patch and uploaded it here. I have tried ...
7 years, 4 months ago (2016-11-15 16:38:14 UTC) #20
Tommaso Pecorella
This is the last comment. From my side I have no more things to add ...
7 years, 4 months ago (2016-11-18 22:59:16 UTC) #21
adadeepak8
Dear Sir, Sorry for the delay, I had my final examinations for the semester over ...
7 years, 4 months ago (2016-11-28 07:08:59 UTC) #22
Tommaso Pecorella
On 2016/11/28 07:08:59, adadeepak8 wrote: > Dear Sir, > > Sorry for the delay, I ...
7 years, 4 months ago (2016-11-29 15:02:04 UTC) #23
adadeepak8
Sir, While adding the router option, do I make the list of routers that the ...
7 years, 3 months ago (2016-12-12 13:05:00 UTC) #24
Tommaso Pecorella
7 years, 2 months ago (2017-01-09 11:24:06 UTC) #25
Tommaso Pecorella
Hi Ankit, (previous message was empty) I'd say that a single router is more than ...
7 years, 2 months ago (2017-01-09 11:25:56 UTC) #26
adadeepak8
Sir, I have completed the setting of default router for client. But, the users thread ...
7 years, 2 months ago (2017-01-10 16:45:20 UTC) #27
Tommaso Pecorella
On 2017/01/10 16:45:20, adadeepak8 wrote: > Sir, > > I have completed the setting of ...
7 years, 2 months ago (2017-01-10 17:05:59 UTC) #28
adadeepak8
Sir, I have included the router option and added two trace sources for new lease ...
7 years, 2 months ago (2017-01-11 05:12:44 UTC) #29
Tommaso Pecorella
The more you look at some code, the more bugs you find. Embarrassing. After the ...
7 years, 2 months ago (2017-01-13 00:43:32 UTC) #30
barnes26_llnl.gov
On Tuesday, January 10, 2017 at 9:06:00 AM UTC-8, Tommaso Pecorella wrote: > > I ...
7 years, 2 months ago (2017-01-13 18:47:44 UTC) #31
adadeepak8
Sir, I have uploaded the updated patch. I scanned the whole code once more and ...
7 years, 2 months ago (2017-01-16 07:47:59 UTC) #32
Tommaso Pecorella
I bet you're going to kill me. https://codereview.appspot.com/279540043/diff/180001/src/internet-apps/model/dhcp-client.cc File src/internet-apps/model/dhcp-client.cc (right): https://codereview.appspot.com/279540043/diff/180001/src/internet-apps/model/dhcp-client.cc#newcode187 src/internet-apps/model/dhcp-client.cc:187: } Remove ...
7 years, 2 months ago (2017-01-16 20:01:56 UTC) #33
Tommaso Pecorella
Hi, I don't want to rush things, but... I'd really love to push this in ...
7 years, 1 month ago (2017-02-16 23:41:34 UTC) #34
adadeepak8
Sir, Sorry for the delay. I have incorporated most of the changes, but I have ...
7 years, 1 month ago (2017-02-26 04:30:02 UTC) #35
Tommaso Pecorella
On 2017/02/26 04:30:02, adadeepak8 wrote: > Sir, > > Sorry for the delay. I have ...
7 years ago (2017-03-01 20:59:03 UTC) #36
adadeepak8
Sir, If you would like to check the code with 0.0.0.0 removed, I have pushed ...
7 years ago (2017-03-02 10:44:49 UTC) #37
Tommaso Pecorella
7 years ago (2017-03-03 01:14:21 UTC) #38
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.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b