SOCIS2013 bundle protocol (first code review)

SOCIS2013 bundle protocol (first code review)

[Tom Henderson, 2014-01-06 16:23:14]

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

- the use of logging is very light and needs to be improved. In particular,
there is almost no function logging.

- coding style is not followed completely; suggest to run 'check-style.py -l 3'
on all of your source code

- is there only one helper per one BP node (because of the EID uniqueness) or
could one helper install on multiple nodes?

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

- Note that TCP doesn't preserve application level message boundaries, what if
it merges bundles together?

- Is BundleProtocol stop time enforced?  What happens if user tries to write to
the Bundle protocol APIs after it has stopped?

- what happens if two nodes accidentally get the same EID?

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

[dizhi.zhou_gmail.com, 2014-01-07 00:31:05]

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