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

Issue 38130049: SOCIS2013 bundle protocol (first code review)

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 years, 9 months ago by Dizhi.Zhou
Modified:
5 years, 8 months ago
Reviewers:
Tom Henderson
CC:
ns-3-reviews_googlegroups.com
Visibility:
Public.

Description

SOCIS2013 bundle protocol (first code review)

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+21840 lines, -0 lines) Patch
A src/bundle-protocol/doc/bundle-protocol.rst View 1 chunk +181 lines, -0 lines 0 comments Download
A src/bundle-protocol/doc/figures/bundle-protocol-structure.eps View 1 chunk +17334 lines, -0 lines 0 comments Download
A src/bundle-protocol/examples/bundle-protocol-simple.cc View 1 chunk +172 lines, -0 lines 0 comments Download
A src/bundle-protocol/examples/wscript View 1 chunk +6 lines, -0 lines 0 comments Download
A src/bundle-protocol/helper/bundle-protocol-container.h View 1 chunk +205 lines, -0 lines 0 comments Download
A src/bundle-protocol/helper/bundle-protocol-container.cc View 1 chunk +107 lines, -0 lines 0 comments Download
A src/bundle-protocol/helper/bundle-protocol-helper.h View 1 chunk +110 lines, -0 lines 0 comments Download
A src/bundle-protocol/helper/bundle-protocol-helper.cc View 1 chunk +89 lines, -0 lines 0 comments Download
A src/bundle-protocol/model/bp-cla-protocol.h View 1 chunk +118 lines, -0 lines 0 comments Download
A src/bundle-protocol/model/bp-cla-protocol.cc View 1 chunk +47 lines, -0 lines 0 comments Download
A src/bundle-protocol/model/bp-endpoint-id.h View 1 chunk +163 lines, -0 lines 0 comments Download
A src/bundle-protocol/model/bp-endpoint-id.cc View 1 chunk +127 lines, -0 lines 0 comments Download
A src/bundle-protocol/model/bp-header.h View 1 chunk +379 lines, -0 lines 0 comments Download
A src/bundle-protocol/model/bp-header.cc View 1 chunk +594 lines, -0 lines 0 comments Download
A src/bundle-protocol/model/bp-payload-header.h View 1 chunk +161 lines, -0 lines 0 comments Download
A src/bundle-protocol/model/bp-payload-header.cc View 1 chunk +238 lines, -0 lines 0 comments Download
A src/bundle-protocol/model/bp-routing-protocol.h View 1 chunk +59 lines, -0 lines 0 comments Download
A src/bundle-protocol/model/bp-routing-protocol.cc View 1 chunk +42 lines, -0 lines 0 comments Download
A src/bundle-protocol/model/bp-static-routing-protocol.h View 1 chunk +74 lines, -0 lines 0 comments Download
A src/bundle-protocol/model/bp-static-routing-protocol.cc View 1 chunk +87 lines, -0 lines 0 comments Download
A src/bundle-protocol/model/bp-tcp-cla-protocol.h View 1 chunk +198 lines, -0 lines 0 comments Download
A src/bundle-protocol/model/bp-tcp-cla-protocol.cc View 1 chunk +317 lines, -0 lines 0 comments Download
A src/bundle-protocol/model/bundle-protocol.h View 1 chunk +334 lines, -0 lines 0 comments Download
A src/bundle-protocol/model/bundle-protocol.cc View 1 chunk +582 lines, -0 lines 0 comments Download
A src/bundle-protocol/test/bundle-protocol-test-suite.cc View 1 chunk +68 lines, -0 lines 0 comments Download
A src/bundle-protocol/wscript View 1 chunk +48 lines, -0 lines 0 comments Download

Messages

Total messages: 2
Tom Henderson
I will send some suggested edits to the example and .rst file by unicast email. ...
5 years, 8 months ago (2014-01-06 16:23:14 UTC) #1
dizhi.zhou_gmail.com
5 years, 8 months ago (2014-01-07 00:31:05 UTC) #2
Hi Tom,

Thanks for your comments. Please find my responses below.

Regards
Dizhi


On Mon, Jan 6, 2014 at 12:23 PM, <tomh.org@gmail.com> wrote:

> I will send some suggested edits to the example and .rst file by unicast
> email.
>
> Here are a few general review comments (not inserted inline with the
> code because I reviewed this offline).
>
> - the code lacks any test coverage, which must be addressed before we
> can consider to merge
>
Ok. I will add it soon.


>
> - the use of logging is very light and needs to be improved. In
> particular, there is almost no function logging.
>
Do you mean the NS_LOG_FUNCTION macro for each method?


>
> - coding style is not followed completely; suggest to run
> 'check-style.py -l 3' on all of your source code
>
Ok. I will check it tomorrow.


>
> - is there only one helper per one BP node (because of the EID
> uniqueness) or could one helper install on multiple nodes?
>
A helper can install multiple BP protocols into multiple nodes. Each node
is installed one BP protocol.


>
> - why is there a comment in the example to limit the bundle size to 300
> and the TCP segment size to 512, then later in the example program,
> bundles of size 1000 are sent?
>
It should be deleted since it is for the previous version. I will update it
in the new version.


>
> - Note that TCP doesn't preserve application level message boundaries,
> what if it merges bundles together?
>
This is the mission of bundle aggregation. The BP protocol reads data from
TCP receive buffer and stores it in a BP buffer
(BundleProtocol::ReceivePacket () method).

Then the BP protocol will detect whether or not the BP buffer contains a
whole bundle. If it is, the BP protocol will
read a bundle from BP buffer and stores the bundle in the local BP
persistent storage (BundleProtocol::ProcessBundle () method ).


>
> - Is BundleProtocol stop time enforced?  What happens if user tries to
> write to the Bundle protocol APIs after it has stopped?
>
The stop time is not enforced. When the stop time is customized by user, BP
will call Close () method to close the
BP connection. If users wants to send bundles after it is stopped, users
need to create a BP connection again (call Register () method)


>
> - what happens if two nodes accidentally get the same EID?
>
This is related to the BP routing protocol. The BP model now only supports
the static routing. If two nodes with the same EID add a route
in the static routing protocol, the second adding operation will return -1
to indicate that the adding route operation is fail.


>
> - What happens if the TCP connection fails (e.g. the nodes are
> disconnected)? Will the protocol retry?  What about a two-node wireless
> example where the node is first out-of-range and then moves into range
> and the TCP connection finally succeeds and the bundle is sent-- would
> that type of example be supported?
>
The existing BP model does not support this.This is the mission for the TCP
CLA layer (show below). There is an IETF draft defines the operations when
TCP is used for BP at the transport layer. But it is not implemented in the
BP model yet.
http://tools.ietf.org/html/draft-irtf-dtnrg-tcp-clayer-07



>
>
> https://codereview.appspot.com/38130049/
>



-- 
Dizhi Zhou
Ph.D. Candidate
Faculty of Computer Science
University of New Brunswick
540 Windsor Street
Fredericton,New Brunswick,Canada
E3B 5A3

E. q5frc@unb.ca
Homepage: www.cs.unb.ca/~q5frc/ <http://www.cs.unb.ca/%7Eq5frc/>
LinkedIn: http://www.linkedin.com/in/dizhizhou
Sign in to reply to this message.

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