Chord/DHash support in ns-3

[Mathieu Lacage, 2010-01-13 12:51:12]

hi,


A couple of high-level comments:
  - coding style: gustavo pointed it out already: you could use our new style
script to make a pass over your code
  - constness: getters should be const and methods which take arguments which
they do not modify should mark them const
  - ChordVNode looks a bit nasty, especially the handling of
predecessor/successor. i.e., it looks like you can change the
predecessor/successor by calling many different methods (Shift* but also
SetPredecessor and SetSuccessor) and it's unclear to me how the class is
expected to behave. 

Other than that, detailed comments below.

http://codereview.appspot.com/180107/diff/1/8
File src/applications/chord-ipv4/chord-identifier.h (right):

http://codereview.appspot.com/180107/diff/1/8#newcode34
src/applications/chord-ipv4/chord-identifier.h:34: class ChordIdentifier :
public Object
that class probably should not derive from Object. It should derive from
SimpleRefCount<ChordIdentifier> so that you get refcounting for free but nothing
more.

http://codereview.appspot.com/180107/diff/1/8#newcode47
src/applications/chord-ipv4/chord-identifier.h:47: ChordIdentifier (uint8_t*
key, uint8_t numBytes);
This should be:
ChordIdentifier (const uint8_t* key, uint8_t numBytes); 

to make it clear to the caller that the callee does not take ownership of the
key buffer. i.e., it makes a copy.

http://codereview.appspot.com/180107/diff/1/8#newcode59
src/applications/chord-ipv4/chord-identifier.h:59: virtual void DoDispose
(void);
DoDispose should go away if you stop deriving from Object.

http://codereview.appspot.com/180107/diff/1/8#newcode66
src/applications/chord-ipv4/chord-identifier.h:66: bool IsEqual
(Ptr<ChordIdentifier> identifier);
this should be:
bool IsEqual (Ptr<const ChordIdentifier> id) const;

to make it clear to the caller that you are not going to modify the content of
the identifier.

http://codereview.appspot.com/180107/diff/1/8#newcode72
src/applications/chord-ipv4/chord-identifier.h:72: bool IsLess
(Ptr<ChordIdentifier> identifier);
same as above.

http://codereview.appspot.com/180107/diff/1/8#newcode78
src/applications/chord-ipv4/chord-identifier.h:78: bool IsGreater
(Ptr<ChordIdentifier> identifier);
same as above

http://codereview.appspot.com/180107/diff/1/8#newcode85
src/applications/chord-ipv4/chord-identifier.h:85: bool IsInBetween
(Ptr<ChordIdentifier> identifierLow, Ptr<ChordIdentifier> identifierHigh);
this should be:
bool IsInBetween (Ptr<const ChordIdentifier> a, Ptr<const ChordIdentifier> b)
const;

http://codereview.appspot.com/180107/diff/1/8#newcode94
src/applications/chord-ipv4/chord-identifier.h:94: uint8_t* GetKey (void);
This method could go away if you changed the signature of all the notification
callbacks you use to take instead a Ptr<ChordIdentifier> directly.

http://codereview.appspot.com/180107/diff/1/8#newcode106
src/applications/chord-ipv4/chord-identifier.h:106: void SetKey (uint8_t* key,
uint8_t numBytes);
this should be:
void SetKey (const uint8_t *key, uint8_t size);

to make it clear to the caller that the callee will not take ownership of the
input key buffer.

http://codereview.appspot.com/180107/diff/1/9
File src/applications/chord-ipv4/chord-ipv4.cc (right):

http://codereview.appspot.com/180107/diff/1/9#newcode57
src/applications/chord-ipv4/chord-ipv4.cc:57: UintegerValue (0),
can't you have a decent default so that users do not have to come up with
something ?

http://codereview.appspot.com/180107/diff/1/9#newcode61
src/applications/chord-ipv4/chord-ipv4.cc:61: "Local IP address (mandatory)",
Can't you have a decent default here ? i.e., could you just bind your listening
socket on all ips by default and restrict it to a specific ip if specified ?

http://codereview.appspot.com/180107/diff/1/9#newcode729
src/applications/chord-ipv4/chord-ipv4.cc:729: if (ret == true)
ret is of type boolean so, the proper style is:
if (ret)

http://codereview.appspot.com/180107/diff/1/9#newcode733
src/applications/chord-ipv4/chord-ipv4.cc:733: if
(virtualNode->FindTransaction(chordMessage.GetTransactionId(), chordTransaction)
== false)
same here: x == false should be instead !x

http://codereview.appspot.com/180107/diff/1/10
File src/applications/chord-ipv4/chord-ipv4.h (right):

http://codereview.appspot.com/180107/diff/1/10#newcode93
src/applications/chord-ipv4/chord-ipv4.h:93: * On invocation, Join request is
sent to Boot Strap node (default). If VirtualNode(ChordVNode)(s) already exist
on local physical node, then Join Request is sent via node closest (in terms of
identifier distance) to the VirtualNode(ChordVNode) identifier.
you must have a real fancy screen to produce such long lines :)

The current coding style requests 80 columns per line and the newer coding style
under discussion requests 120 so, it probably would make sense to wrap these
lines to 120.

http://codereview.appspot.com/180107/diff/1/10#newcode295
src/applications/chord-ipv4/chord-ipv4.h:295: bool isBootStrapNode;
m_

http://codereview.appspot.com/180107/diff/1/12
File src/applications/chord-ipv4/chord-message.h (right):

http://codereview.appspot.com/180107/diff/1/12#newcode63
src/applications/chord-ipv4/chord-message.h:63: void SetMessageType (MessageType
messageType)
no code in headers unless for performance reasons

http://codereview.appspot.com/180107/diff/1/12#newcode457
src/applications/chord-ipv4/chord-message.h:457: JoinReq& GetJoinReq ()
same comments as in DHashMessage

http://codereview.appspot.com/180107/diff/1/13
File src/applications/chord-ipv4/chord-node-table.cc (right):

http://codereview.appspot.com/180107/diff/1/13#newcode55
src/applications/chord-ipv4/chord-node-table.cc:55:
chordNode->SetTimestamp(Simulator::Now());
should be moved out of the if/else statement and the else statement can
disappear.

http://codereview.appspot.com/180107/diff/1/13#newcode64
src/applications/chord-ipv4/chord-node-table.cc:64: if (chordNode->GetName() ==
"")
missing braces around single-line statements

http://codereview.appspot.com/180107/diff/1/14
File src/applications/chord-ipv4/chord-node-table.h (right):

http://codereview.appspot.com/180107/diff/1/14#newcode29
src/applications/chord-ipv4/chord-node-table.h:29: typedef
std::map<ChordIdentifier, Ptr<ChordNode> > ChordNodeMap;
once you have change GetMap as suggested below, you can move these to private
typedefs of the class

http://codereview.appspot.com/180107/diff/1/14#newcode60
src/applications/chord-ipv4/chord-node-table.h:60: bool FindNode
(Ptr<ChordIdentifier> &chordIdentifier, Ptr<ChordNode> &ChordNode);
A much simpler API which is much harder to abuse:
Ptr<ChordNode> FindNode (Ptr<ChordIdentifier> identifier);

http://codereview.appspot.com/180107/diff/1/14#newcode67
src/applications/chord-ipv4/chord-node-table.h:67: bool FindNode (std::string
&name, Ptr<ChordNode> &chordNode);
why do you pass the name as a reference ?
Again, the following is much simpler conceptually:
Ptr<ChordNode> FindNode (std::string name);

we can optimize it later if needed.

http://codereview.appspot.com/180107/diff/1/14#newcode72
src/applications/chord-ipv4/chord-node-table.h:72: void RemoveNode
(Ptr<ChordIdentifier> &chordIdentifier);
gaaaah. please, don't pass all these arguments as references: it makes no sense
since you do not intend to modify their content.

This should be:
void RemoveNode (Ptr<ChordIdentifier> chordIdentifier);

http://codereview.appspot.com/180107/diff/1/14#newcode77
src/applications/chord-ipv4/chord-node-table.h:77: void RemoveNode (std::string
&name);
again, this should be:

void RemoveNode (std::string name);

http://codereview.appspot.com/180107/diff/1/14#newcode84
src/applications/chord-ipv4/chord-node-table.h:84: bool FindNearestNode
(Ptr<ChordIdentifier> &targetIdentifier, Ptr<ChordNode> &chordNode);
and that should be:
Ptr<ChordNode> FindNearestNode (Ptr<ChordIdentifier> id);

http://codereview.appspot.com/180107/diff/1/14#newcode101
src/applications/chord-ipv4/chord-node-table.h:101: ChordNodeMap& GetMap();
Every user of this method uses it to iterate over the list of ChordNode so, I
would rewrite this to the much less efficient but much safer:

std::vector<Ptr<ChordNode> > GetNodes (void);

which returns a copy so the caller cannot modify the internal data structure of
your call and which allows you to simplify your caller because every caller had
to access the internal second field of the map.

http://codereview.appspot.com/180107/diff/1/16
File src/applications/chord-ipv4/chord-node.h (right):

http://codereview.appspot.com/180107/diff/1/16#newcode86
src/applications/chord-ipv4/chord-node.h:86: Ipv4Address GetIpAddress ();
Ipv4Address GetIpAddress () const;

http://codereview.appspot.com/180107/diff/1/16#newcode90
src/applications/chord-ipv4/chord-node.h:90: uint16_t GetPort ();
same as above. Same for all getters.

http://codereview.appspot.com/180107/diff/1/16#newcode114
src/applications/chord-ipv4/chord-node.h:114: bool GetRoutable ();
Usually, I prefer to use Is prefixes for booleans:
bool IsRoutable () const;

http://codereview.appspot.com/180107/diff/1/18
File src/applications/chord-ipv4/chord-transaction.h (right):

http://codereview.appspot.com/180107/diff/1/18#newcode37
src/applications/chord-ipv4/chord-transaction.h:37: class ChordTransaction :
public Object
again, probably no reason to derive from Object. Probably you want to derive
from SimpleRefCount<ChordTransaction>

http://codereview.appspot.com/180107/diff/1/20
File src/applications/chord-ipv4/chord-vnode.h (right):

http://codereview.appspot.com/180107/diff/1/20#newcode61
src/applications/chord-ipv4/chord-vnode.h:61: std::vector<Ptr<ChordNode> >& 
GetSuccessorList(void);
returns a reference to an internal private field: evil because it allows the
caller to wreak havoc in the internal state of this class without the class
knowing about it. This method is unused (as well as GetPredecessorList) so, kill
them both, please.

http://codereview.appspot.com/180107/diff/1/20#newcode218
src/applications/chord-ipv4/chord-vnode.h:218: bool FindTransaction (uint32_t
transactionId, Ptr<ChordTransaction> &chordTransaction);
A simpler API would be:
Ptr<ChrodTransaction> FindTransaction (uint32_t transactionId);

and the caller would check if the returned value is == 0 or != 0

http://codereview.appspot.com/180107/diff/1/23
File src/applications/chord-ipv4/dhash-ipv4.cc (right):

http://codereview.appspot.com/180107/diff/1/23#newcode682
src/applications/chord-ipv4/dhash-ipv4.cc:682: Ptr<DHashConnection>
dHashConnection = Create<DHashConnection> (ipAddress, port, socket);
bwaaaaaaah. Create<DHashConnection> (...) might work but you are really really
lucky.

DHashConnection derives from Object. As such, you must create instances of
DHashConnection with CreateObject, not Create.

Now, it's unclear to me why DHashConnection derives from Object so, another
option would be to keep the call to Create and make DHashConnection derive from
SimpleRefCount<DhashConnection> instead of Object.

http://codereview.appspot.com/180107/diff/1/23#newcode683
src/applications/chord-ipv4/dhash-ipv4.cc:683: socket->SetRecvCallback
(MakeCallback(&DHashConnection::ReadTCPBuffer, dHashConnection));
I see that the DHashConnection class already has a pointer to the socket so, if
I were you, I would move this call to Socket::SetRecvCallback (same for
Socket::SetCloseCallbacks) inside the DHashConnection constructor and, if you
need this DHashIpv4 class to be notified that the socket/connection has been
closed, you can introduce a DHashConnection::SetCloseCallback method, just like
you did introduce DHashConnection::SetRecvCallback.

http://codereview.appspot.com/180107/diff/1/23#newcode694
src/applications/chord-ipv4/dhash-ipv4.cc:694: DHashIpv4::FindConnection
(Ptr<Socket> socket, Ptr<DHashConnection>& dHashConnection)
I did not find any caller of this method so, if there is none, it makes little
sense to use a std::map to store the connections: you could store them in a
std::list (or std::vector)

http://codereview.appspot.com/180107/diff/1/26
File src/applications/chord-ipv4/dhash-message.h (right):

http://codereview.appspot.com/180107/diff/1/26#newcode45
src/applications/chord-ipv4/dhash-message.h:45: void SetLength (uint32_t length)
no code in headers unless you have clear performance reasons (clear = hard
performance measurements to assess the impact of that specific change on a
realistic benchmark): move to .cc file.

http://codereview.appspot.com/180107/diff/1/26#newcode59
src/applications/chord-ipv4/dhash-message.h:59: TypeId GetInstanceTypeId (void)
const;
we usually repeat the virtual keyword when we define a virtual method defined in
a base class. This helps someone who is reading this header for the first time
to figure out the semantics of the code

http://codereview.appspot.com/180107/diff/1/26#newcode284
src/applications/chord-ipv4/dhash-message.h:284: StoreReq& GetStoreReq ()
don't ever return a reference or pointer to a private field. Return a copy to
make sure that the user can't abuse it. 

In this case, I see that callers of this method use it to actually modify the
content of this private field. In this case, you need to add API like this:

void SetStoreReq (StoreReq req);

And if you do this, you can remove SetMessageType from your API because
SetStoreReq can explicitely also set the type of the message.

http://codereview.appspot.com/180107/diff/1/27
File src/applications/chord-ipv4/dhash-object.cc (right):

http://codereview.appspot.com/180107/diff/1/27#newcode127
src/applications/chord-ipv4/dhash-object.cc:127: for (uint32_t j=0 ;
j<m_sizeOfObject; j++)
a more efficient version would be start.Write (m_object, m_sizeOfObject);

http://codereview.appspot.com/180107/diff/1/27#newcode153
src/applications/chord-ipv4/dhash-object.cc:153: for (j=0; j<
m_sizeOfObject;j++)
same here: start.Read (m_object, m_sizeOfObject);

http://codereview.appspot.com/180107/diff/1/28
File src/applications/chord-ipv4/dhash-object.h (right):

http://codereview.appspot.com/180107/diff/1/28#newcode33
src/applications/chord-ipv4/dhash-object.h:33: class DHashObject : public Object
same question here: why do you derive from Object ?

http://codereview.appspot.com/180107/diff/1/30
File src/applications/chord-ipv4/dhash-transaction.h (right):

http://codereview.appspot.com/180107/diff/1/30#newcode35
src/applications/chord-ipv4/dhash-transaction.h:35: class DHashTransaction :
public Object
why exactly are you deriving from Object here ? If all you need is reference
counting (i.e., the Ptr<T> stuff), you can do this instead:

class DHashTransaction : public SimpleRefCount<DHashTransaction>

[harjotgill, 2010-01-16 17:01:08]

Thanks for reviewing our code. I am working on your suggestions and will
post a new version soon.

Thanks,
Harjot

On Wed, Jan 13, 2010 at 7:51 AM, <mathieu.lacage@gmail.com> wrote:

> hi,
>
>
> A couple of high-level comments:
>  - coding style: gustavo pointed it out already: you could use our new
> style script to make a pass over your code
>  - constness: getters should be const and methods which take arguments
> which they do not modify should mark them const
>  - ChordVNode looks a bit nasty, especially the handling of
> predecessor/successor. i.e., it looks like you can change the
> predecessor/successor by calling many different methods (Shift* but also
> SetPredecessor and SetSuccessor) and it's unclear to me how the class is
> expected to behave.
>
> Other than that, detailed comments below.
>
>
> http://codereview.appspot.com/180107/diff/1/8
> File src/applications/chord-ipv4/chord-identifier.h (right):
>
> http://codereview.appspot.com/180107/diff/1/8#newcode34
> src/applications/chord-ipv4/chord-identifier.h:34: class ChordIdentifier
> : public Object
> that class probably should not derive from Object. It should derive from
> SimpleRefCount<ChordIdentifier> so that you get refcounting for free but
> nothing more.
>
> http://codereview.appspot.com/180107/diff/1/8#newcode47
> src/applications/chord-ipv4/chord-identifier.h:47: ChordIdentifier
> (uint8_t* key, uint8_t numBytes);
> This should be:
> ChordIdentifier (const uint8_t* key, uint8_t numBytes);
>
> to make it clear to the caller that the callee does not take ownership
> of the key buffer. i.e., it makes a copy.
>
> http://codereview.appspot.com/180107/diff/1/8#newcode59
> src/applications/chord-ipv4/chord-identifier.h:59: virtual void
> DoDispose (void);
> DoDispose should go away if you stop deriving from Object.
>
> http://codereview.appspot.com/180107/diff/1/8#newcode66
> src/applications/chord-ipv4/chord-identifier.h:66: bool IsEqual
> (Ptr<ChordIdentifier> identifier);
> this should be:
> bool IsEqual (Ptr<const ChordIdentifier> id) const;
>
> to make it clear to the caller that you are not going to modify the
> content of the identifier.
>
> http://codereview.appspot.com/180107/diff/1/8#newcode72
> src/applications/chord-ipv4/chord-identifier.h:72: bool IsLess
> (Ptr<ChordIdentifier> identifier);
> same as above.
>
> http://codereview.appspot.com/180107/diff/1/8#newcode78
> src/applications/chord-ipv4/chord-identifier.h:78: bool IsGreater
> (Ptr<ChordIdentifier> identifier);
> same as above
>
> http://codereview.appspot.com/180107/diff/1/8#newcode85
> src/applications/chord-ipv4/chord-identifier.h:85: bool IsInBetween
> (Ptr<ChordIdentifier> identifierLow, Ptr<ChordIdentifier>
> identifierHigh);
> this should be:
> bool IsInBetween (Ptr<const ChordIdentifier> a, Ptr<const
> ChordIdentifier> b) const;
>
> http://codereview.appspot.com/180107/diff/1/8#newcode94
> src/applications/chord-ipv4/chord-identifier.h:94: uint8_t* GetKey
> (void);
> This method could go away if you changed the signature of all the
> notification callbacks you use to take instead a Ptr<ChordIdentifier>
> directly.
>
> http://codereview.appspot.com/180107/diff/1/8#newcode106
> src/applications/chord-ipv4/chord-identifier.h:106: void SetKey
> (uint8_t* key, uint8_t numBytes);
> this should be:
> void SetKey (const uint8_t *key, uint8_t size);
>
> to make it clear to the caller that the callee will not take ownership
> of the input key buffer.
>
> http://codereview.appspot.com/180107/diff/1/9
> File src/applications/chord-ipv4/chord-ipv4.cc (right):
>
> http://codereview.appspot.com/180107/diff/1/9#newcode57
> src/applications/chord-ipv4/chord-ipv4.cc:57: UintegerValue (0),
> can't you have a decent default so that users do not have to come up
> with something ?
>
> http://codereview.appspot.com/180107/diff/1/9#newcode61
> src/applications/chord-ipv4/chord-ipv4.cc:61: "Local IP address
> (mandatory)",
> Can't you have a decent default here ? i.e., could you just bind your
> listening socket on all ips by default and restrict it to a specific ip
> if specified ?
>
> http://codereview.appspot.com/180107/diff/1/9#newcode729
> src/applications/chord-ipv4/chord-ipv4.cc:729: if (ret == true)
> ret is of type boolean so, the proper style is:
> if (ret)
>
> http://codereview.appspot.com/180107/diff/1/9#newcode733
> src/applications/chord-ipv4/chord-ipv4.cc:733: if
> (virtualNode->FindTransaction(chordMessage.GetTransactionId(),
> chordTransaction) == false)
> same here: x == false should be instead !x
>
> http://codereview.appspot.com/180107/diff/1/10
> File src/applications/chord-ipv4/chord-ipv4.h (right):
>
> http://codereview.appspot.com/180107/diff/1/10#newcode93
> src/applications/chord-ipv4/chord-ipv4.h:93: * On invocation, Join
> request is sent to Boot Strap node (default). If
> VirtualNode(ChordVNode)(s) already exist on local physical node, then
> Join Request is sent via node closest (in terms of identifier distance)
> to the VirtualNode(ChordVNode) identifier.
> you must have a real fancy screen to produce such long lines :)
>
> The current coding style requests 80 columns per line and the newer
> coding style under discussion requests 120 so, it probably would make
> sense to wrap these lines to 120.
>
> http://codereview.appspot.com/180107/diff/1/10#newcode295
> src/applications/chord-ipv4/chord-ipv4.h:295: bool isBootStrapNode;
> m_
>
> http://codereview.appspot.com/180107/diff/1/12
> File src/applications/chord-ipv4/chord-message.h (right):
>
> http://codereview.appspot.com/180107/diff/1/12#newcode63
> src/applications/chord-ipv4/chord-message.h:63: void SetMessageType
> (MessageType messageType)
> no code in headers unless for performance reasons
>
> http://codereview.appspot.com/180107/diff/1/12#newcode457
> src/applications/chord-ipv4/chord-message.h:457: JoinReq& GetJoinReq ()
> same comments as in DHashMessage
>
> http://codereview.appspot.com/180107/diff/1/13
> File src/applications/chord-ipv4/chord-node-table.cc (right):
>
> http://codereview.appspot.com/180107/diff/1/13#newcode55
> src/applications/chord-ipv4/chord-node-table.cc:55:
> chordNode->SetTimestamp(Simulator::Now());
> should be moved out of the if/else statement and the else statement can
> disappear.
>
> http://codereview.appspot.com/180107/diff/1/13#newcode64
> src/applications/chord-ipv4/chord-node-table.cc:64: if
> (chordNode->GetName() == "")
> missing braces around single-line statements
>
> http://codereview.appspot.com/180107/diff/1/14
> File src/applications/chord-ipv4/chord-node-table.h (right):
>
> http://codereview.appspot.com/180107/diff/1/14#newcode29
> src/applications/chord-ipv4/chord-node-table.h:29: typedef
> std::map<ChordIdentifier, Ptr<ChordNode> > ChordNodeMap;
> once you have change GetMap as suggested below, you can move these to
> private typedefs of the class
>
> http://codereview.appspot.com/180107/diff/1/14#newcode60
> src/applications/chord-ipv4/chord-node-table.h:60: bool FindNode
> (Ptr<ChordIdentifier> &chordIdentifier, Ptr<ChordNode> &ChordNode);
> A much simpler API which is much harder to abuse:
> Ptr<ChordNode> FindNode (Ptr<ChordIdentifier> identifier);
>
> http://codereview.appspot.com/180107/diff/1/14#newcode67
> src/applications/chord-ipv4/chord-node-table.h:67: bool FindNode
> (std::string &name, Ptr<ChordNode> &chordNode);
> why do you pass the name as a reference ?
> Again, the following is much simpler conceptually:
> Ptr<ChordNode> FindNode (std::string name);
>
> we can optimize it later if needed.
>
> http://codereview.appspot.com/180107/diff/1/14#newcode72
> src/applications/chord-ipv4/chord-node-table.h:72: void RemoveNode
> (Ptr<ChordIdentifier> &chordIdentifier);
> gaaaah. please, don't pass all these arguments as references: it makes
> no sense since you do not intend to modify their content.
>
> This should be:
> void RemoveNode (Ptr<ChordIdentifier> chordIdentifier);
>
> http://codereview.appspot.com/180107/diff/1/14#newcode77
> src/applications/chord-ipv4/chord-node-table.h:77: void RemoveNode
> (std::string &name);
> again, this should be:
>
> void RemoveNode (std::string name);
>
> http://codereview.appspot.com/180107/diff/1/14#newcode84
> src/applications/chord-ipv4/chord-node-table.h:84: bool FindNearestNode
> (Ptr<ChordIdentifier> &targetIdentifier, Ptr<ChordNode> &chordNode);
> and that should be:
> Ptr<ChordNode> FindNearestNode (Ptr<ChordIdentifier> id);
>
> http://codereview.appspot.com/180107/diff/1/14#newcode101
> src/applications/chord-ipv4/chord-node-table.h:101: ChordNodeMap&
> GetMap();
> Every user of this method uses it to iterate over the list of ChordNode
> so, I would rewrite this to the much less efficient but much safer:
>
> std::vector<Ptr<ChordNode> > GetNodes (void);
>
> which returns a copy so the caller cannot modify the internal data
> structure of your call and which allows you to simplify your caller
> because every caller had to access the internal second field of the map.
>
> http://codereview.appspot.com/180107/diff/1/16
> File src/applications/chord-ipv4/chord-node.h (right):
>
> http://codereview.appspot.com/180107/diff/1/16#newcode86
> src/applications/chord-ipv4/chord-node.h:86: Ipv4Address GetIpAddress
> ();
> Ipv4Address GetIpAddress () const;
>
> http://codereview.appspot.com/180107/diff/1/16#newcode90
> src/applications/chord-ipv4/chord-node.h:90: uint16_t GetPort ();
> same as above. Same for all getters.
>
> http://codereview.appspot.com/180107/diff/1/16#newcode114
> src/applications/chord-ipv4/chord-node.h:114: bool GetRoutable ();
> Usually, I prefer to use Is prefixes for booleans:
> bool IsRoutable () const;
>
> http://codereview.appspot.com/180107/diff/1/18
> File src/applications/chord-ipv4/chord-transaction.h (right):
>
> http://codereview.appspot.com/180107/diff/1/18#newcode37
> src/applications/chord-ipv4/chord-transaction.h:37: class
> ChordTransaction : public Object
> again, probably no reason to derive from Object. Probably you want to
> derive from SimpleRefCount<ChordTransaction>
>
> http://codereview.appspot.com/180107/diff/1/20
> File src/applications/chord-ipv4/chord-vnode.h (right):
>
> http://codereview.appspot.com/180107/diff/1/20#newcode61
> src/applications/chord-ipv4/chord-vnode.h:61: std::vector<Ptr<ChordNode>
>
>> &  GetSuccessorList(void);
>>
> returns a reference to an internal private field: evil because it allows
> the caller to wreak havoc in the internal state of this class without
> the class knowing about it. This method is unused (as well as
> GetPredecessorList) so, kill them both, please.
>
> http://codereview.appspot.com/180107/diff/1/20#newcode218
> src/applications/chord-ipv4/chord-vnode.h:218: bool FindTransaction
> (uint32_t transactionId, Ptr<ChordTransaction> &chordTransaction);
> A simpler API would be:
> Ptr<ChrodTransaction> FindTransaction (uint32_t transactionId);
>
> and the caller would check if the returned value is == 0 or != 0
>
> http://codereview.appspot.com/180107/diff/1/23
> File src/applications/chord-ipv4/dhash-ipv4.cc (right):
>
> http://codereview.appspot.com/180107/diff/1/23#newcode682
> src/applications/chord-ipv4/dhash-ipv4.cc:682: Ptr<DHashConnection>
> dHashConnection = Create<DHashConnection> (ipAddress, port, socket);
> bwaaaaaaah. Create<DHashConnection> (...) might work but you are really
> really lucky.
>
> DHashConnection derives from Object. As such, you must create instances
> of DHashConnection with CreateObject, not Create.
>
> Now, it's unclear to me why DHashConnection derives from Object so,
> another option would be to keep the call to Create and make
> DHashConnection derive from SimpleRefCount<DhashConnection> instead of
> Object.
>
> http://codereview.appspot.com/180107/diff/1/23#newcode683
> src/applications/chord-ipv4/dhash-ipv4.cc:683: socket->SetRecvCallback
> (MakeCallback(&DHashConnection::ReadTCPBuffer, dHashConnection));
> I see that the DHashConnection class already has a pointer to the socket
> so, if I were you, I would move this call to Socket::SetRecvCallback
> (same for Socket::SetCloseCallbacks) inside the DHashConnection
> constructor and, if you need this DHashIpv4 class to be notified that
> the socket/connection has been closed, you can introduce a
> DHashConnection::SetCloseCallback method, just like you did introduce
> DHashConnection::SetRecvCallback.
>
> http://codereview.appspot.com/180107/diff/1/23#newcode694
> src/applications/chord-ipv4/dhash-ipv4.cc:694: DHashIpv4::FindConnection
> (Ptr<Socket> socket, Ptr<DHashConnection>& dHashConnection)
> I did not find any caller of this method so, if there is none, it makes
> little sense to use a std::map to store the connections: you could store
> them in a std::list (or std::vector)
>
> http://codereview.appspot.com/180107/diff/1/26
> File src/applications/chord-ipv4/dhash-message.h (right):
>
> http://codereview.appspot.com/180107/diff/1/26#newcode45
> src/applications/chord-ipv4/dhash-message.h:45: void SetLength (uint32_t
> length)
> no code in headers unless you have clear performance reasons (clear =
> hard performance measurements to assess the impact of that specific
> change on a realistic benchmark): move to .cc file.
>
> http://codereview.appspot.com/180107/diff/1/26#newcode59
> src/applications/chord-ipv4/dhash-message.h:59: TypeId GetInstanceTypeId
> (void) const;
> we usually repeat the virtual keyword when we define a virtual method
> defined in a base class. This helps someone who is reading this header
> for the first time to figure out the semantics of the code
>
> http://codereview.appspot.com/180107/diff/1/26#newcode284
> src/applications/chord-ipv4/dhash-message.h:284: StoreReq& GetStoreReq
> ()
> don't ever return a reference or pointer to a private field. Return a
> copy to make sure that the user can't abuse it.
>
> In this case, I see that callers of this method use it to actually
> modify the content of this private field. In this case, you need to add
> API like this:
>
> void SetStoreReq (StoreReq req);
>
> And if you do this, you can remove SetMessageType from your API because
> SetStoreReq can explicitely also set the type of the message.
>
> http://codereview.appspot.com/180107/diff/1/27
> File src/applications/chord-ipv4/dhash-object.cc (right):
>
> http://codereview.appspot.com/180107/diff/1/27#newcode127
> src/applications/chord-ipv4/dhash-object.cc:127: for (uint32_t j=0 ;
> j<m_sizeOfObject; j++)
> a more efficient version would be start.Write (m_object,
> m_sizeOfObject);
>
> http://codereview.appspot.com/180107/diff/1/27#newcode153
> src/applications/chord-ipv4/dhash-object.cc:153: for (j=0; j<
> m_sizeOfObject;j++)
> same here: start.Read (m_object, m_sizeOfObject);
>
> http://codereview.appspot.com/180107/diff/1/28
> File src/applications/chord-ipv4/dhash-object.h (right):
>
> http://codereview.appspot.com/180107/diff/1/28#newcode33
> src/applications/chord-ipv4/dhash-object.h:33: class DHashObject :
> public Object
> same question here: why do you derive from Object ?
>
> http://codereview.appspot.com/180107/diff/1/30
> File src/applications/chord-ipv4/dhash-transaction.h (right):
>
> http://codereview.appspot.com/180107/diff/1/30#newcode35
> src/applications/chord-ipv4/dhash-transaction.h:35: class
> DHashTransaction : public Object
> why exactly are you deriving from Object here ? If all you need is
> reference counting (i.e., the Ptr<T> stuff), you can do this instead:
>
> class DHashTransaction : public SimpleRefCount<DHashTransaction>
>
> http://codereview.appspot.com/180107
>

[Tom Henderson, 2010-01-18 07:32:18]

Sorry for the delay in reviewing; thanks for submitting to ns-3.  I would like
to recommend that more documentation be added (consider a standalone chord.h
file in that directory that summarizes what is modeled and what is not, and how
people may typically use the model; see src/routing/aodv/aodv.h for an example).
 Also, is there a published paper or tech report that can be used as a
reference?

Can some test code (a TestSuite for src/test/) be added?  I think that
examples/chord-run/chord-test-script could be the start of a test program.  It
would be nice to regression test somehow that the DHT gets set up properly and
responds properly to queries, given some input configuration.

I noticed that there were no trace sources in the model.  Are there no events
worth tracing, or would you like help with enabling some trace sources?

http://codereview.appspot.com/180107/diff/1/2
File examples/chord-run/chord-run.cc (right):

http://codereview.appspot.com/180107/diff/1/2#newcode23
examples/chord-run/chord-run.cc:23: //
Can you add some more comments about what this example is supposed to be showing
the user, and how it should be run?  It is not really clear at first; upon
running, the user gets a "Command >" prompt.

http://codereview.appspot.com/180107/diff/1/6
File src/applications/chord-ipv4/AUTHORS (right):

http://codereview.appspot.com/180107/diff/1/6#newcode9
src/applications/chord-ipv4/AUTHORS:9: Vishal Patil (patilv@seas.upenn.edu)
Please do not add a separate AUTHORS file for the subdirectory; I would suggest
to just slip the credit statement into one of the headers such as chord-ipv4.h,
and extend the top-level AUTHORS file.

http://codereview.appspot.com/180107/diff/1/9
File src/applications/chord-ipv4/chord-ipv4.cc (right):

http://codereview.appspot.com/180107/diff/1/9#newcode468
src/applications/chord-ipv4/chord-ipv4.cc:468: NS_LOG_FUNCTION_NOARGS ();
In general, we prefer to only use NS_LOG_FUNCTION_NOARGS () for static
functions.  In this case (and normally for class methods), a better choice is
something like:

NS_LOG_FUNCTION (this << vNodeName << key << keyBytes);

http://codereview.appspot.com/180107/diff/1/10
File src/applications/chord-ipv4/chord-ipv4.h (right):

http://codereview.appspot.com/180107/diff/1/10#newcode36
src/applications/chord-ipv4/chord-ipv4.h:36: // m = 160 bits i.e. 20 bytes
I do not see the point of these defines.  They are already commented in the
attributes.

http://codereview.appspot.com/180107/diff/1/20
File src/applications/chord-ipv4/chord-vnode.h (right):

http://codereview.appspot.com/180107/diff/1/20#newcode263
src/applications/chord-ipv4/chord-vnode.h:263: VNodeStats& GetStats();
Do not return reference to a private data member (how about returning by value
here)?

http://codereview.appspot.com/180107/diff/1/24
File src/applications/chord-ipv4/dhash-ipv4.h (right):

http://codereview.appspot.com/180107/diff/1/24#newcode53
src/applications/chord-ipv4/dhash-ipv4.h:53: *  \brief DHash over Chord
Please doxygenate more description of what this class is.  Relationship between
DHash and Chord is not apparent at first glance.

[taher007saeed, 2010-01-24 20:47:01]

Dear all,

Thanks Mathieu/Gustavo/Tom for reviewing our code. We have incorporated several
suggestions, please visit http://codereview.appspot.com/180107/show for our
comments. We still have to incorporate review comments sent by Tom. We will do
that over the next weekend.

Thanks and Regards,
Taher

http://codereview.appspot.com/180107/diff/1/8
File src/applications/chord-ipv4/chord-identifier.h (right):

http://codereview.appspot.com/180107/diff/1/8#newcode34
src/applications/chord-ipv4/chord-identifier.h:34: class ChordIdentifier :
public Object
On 2010/01/13 12:51:12, Mathieu Lacage wrote:
> that class probably should not derive from Object. It should derive from
> SimpleRefCount<ChordIdentifier> so that you get refcounting for free but
nothing
> more.

Done.

http://codereview.appspot.com/180107/diff/1/8#newcode47
src/applications/chord-ipv4/chord-identifier.h:47: ChordIdentifier (uint8_t*
key, uint8_t numBytes);
On 2010/01/13 12:51:12, Mathieu Lacage wrote:
> This should be:
> ChordIdentifier (const uint8_t* key, uint8_t numBytes); 
> 
> to make it clear to the caller that the callee does not take ownership of the
> key buffer. i.e., it makes a copy.

Done.

http://codereview.appspot.com/180107/diff/1/8#newcode59
src/applications/chord-ipv4/chord-identifier.h:59: virtual void DoDispose
(void);
On 2010/01/13 12:51:12, Mathieu Lacage wrote:
> DoDispose should go away if you stop deriving from Object.

Done.

http://codereview.appspot.com/180107/diff/1/8#newcode66
src/applications/chord-ipv4/chord-identifier.h:66: bool IsEqual
(Ptr<ChordIdentifier> identifier);
On 2010/01/13 12:51:12, Mathieu Lacage wrote:
> this should be:
> bool IsEqual (Ptr<const ChordIdentifier> id) const;
> 
> to make it clear to the caller that you are not going to modify the content of
> the identifier.

Done.

http://codereview.appspot.com/180107/diff/1/8#newcode72
src/applications/chord-ipv4/chord-identifier.h:72: bool IsLess
(Ptr<ChordIdentifier> identifier);
On 2010/01/13 12:51:12, Mathieu Lacage wrote:
> same as above.

Done.

http://codereview.appspot.com/180107/diff/1/8#newcode78
src/applications/chord-ipv4/chord-identifier.h:78: bool IsGreater
(Ptr<ChordIdentifier> identifier);
On 2010/01/13 12:51:12, Mathieu Lacage wrote:
> same as above

Done.

http://codereview.appspot.com/180107/diff/1/8#newcode85
src/applications/chord-ipv4/chord-identifier.h:85: bool IsInBetween
(Ptr<ChordIdentifier> identifierLow, Ptr<ChordIdentifier> identifierHigh);
On 2010/01/13 12:51:12, Mathieu Lacage wrote:
> this should be:
> bool IsInBetween (Ptr<const ChordIdentifier> a, Ptr<const ChordIdentifier> b)
> const;

Done.

http://codereview.appspot.com/180107/diff/1/8#newcode94
src/applications/chord-ipv4/chord-identifier.h:94: uint8_t* GetKey (void);
We knowingly designed this API to be consistent with ns3 philosophy of providing
real world APIs. Using ChordIdentifier to communicate with service user will
"force" people to stick with our structures. So, on every function
call/notification, we accept pointer to key array and number of bytes in key.
Such an API will allow people to port Application code easily.

On 2010/01/13 12:51:12, Mathieu Lacage wrote:
> This method could go away if you changed the signature of all the notification
> callbacks you use to take instead a Ptr<ChordIdentifier> directly.

http://codereview.appspot.com/180107/diff/1/8#newcode106
src/applications/chord-ipv4/chord-identifier.h:106: void SetKey (uint8_t* key,
uint8_t numBytes);
On 2010/01/13 12:51:12, Mathieu Lacage wrote:
> this should be:
> void SetKey (const uint8_t *key, uint8_t size);
> 
> to make it clear to the caller that the callee will not take ownership of the
> input key buffer.

Done.

http://codereview.appspot.com/180107/diff/1/9
File src/applications/chord-ipv4/chord-ipv4.cc (right):

http://codereview.appspot.com/180107/diff/1/9#newcode57
src/applications/chord-ipv4/chord-ipv4.cc:57: UintegerValue (0),
On 2010/01/13 12:51:12, Mathieu Lacage wrote:
> can't you have a decent default so that users do not have to come up with
> something ?

Done.

http://codereview.appspot.com/180107/diff/1/9#newcode61
src/applications/chord-ipv4/chord-ipv4.cc:61: "Local IP address (mandatory)",
Please elaborate more this point.

On 2010/01/13 12:51:12, Mathieu Lacage wrote:
> Can't you have a decent default here ? i.e., could you just bind your
listening
> socket on all ips by default and restrict it to a specific ip if specified ?

http://codereview.appspot.com/180107/diff/1/9#newcode468
src/applications/chord-ipv4/chord-ipv4.cc:468: NS_LOG_FUNCTION_NOARGS ();
On 2010/01/18 07:32:18, Tom Henderson wrote:
> In general, we prefer to only use NS_LOG_FUNCTION_NOARGS () for static
> functions.  In this case (and normally for class methods), a better choice is
> something like:
> 
> NS_LOG_FUNCTION (this << vNodeName << key << keyBytes);

Done.

http://codereview.appspot.com/180107/diff/1/9#newcode729
src/applications/chord-ipv4/chord-ipv4.cc:729: if (ret == true)
On 2010/01/13 12:51:12, Mathieu Lacage wrote:
> ret is of type boolean so, the proper style is:
> if (ret)

Done.

http://codereview.appspot.com/180107/diff/1/9#newcode733
src/applications/chord-ipv4/chord-ipv4.cc:733: if
(virtualNode->FindTransaction(chordMessage.GetTransactionId(), chordTransaction)
== false)
On 2010/01/13 12:51:12, Mathieu Lacage wrote:
> same here: x == false should be instead !x

Done.

http://codereview.appspot.com/180107/diff/1/12
File src/applications/chord-ipv4/chord-message.h (right):

http://codereview.appspot.com/180107/diff/1/12#newcode63
src/applications/chord-ipv4/chord-message.h:63: void SetMessageType (MessageType
messageType)
On 2010/01/13 12:51:12, Mathieu Lacage wrote:
> no code in headers unless for performance reasons

Done.

http://codereview.appspot.com/180107/diff/1/12#newcode457
src/applications/chord-ipv4/chord-message.h:457: JoinReq& GetJoinReq ()
On 2010/01/13 12:51:12, Mathieu Lacage wrote:
> same comments as in DHashMessage

Done.

http://codereview.appspot.com/180107/diff/1/13
File src/applications/chord-ipv4/chord-node-table.cc (right):

http://codereview.appspot.com/180107/diff/1/13#newcode55
src/applications/chord-ipv4/chord-node-table.cc:55:
chordNode->SetTimestamp(Simulator::Now());
On 2010/01/13 12:51:12, Mathieu Lacage wrote:
> should be moved out of the if/else statement and the else statement can
> disappear.

Done.

http://codereview.appspot.com/180107/diff/1/13#newcode64
src/applications/chord-ipv4/chord-node-table.cc:64: if (chordNode->GetName() ==
"")
On 2010/01/13 12:51:12, Mathieu Lacage wrote:
> missing braces around single-line statements

Done.

http://codereview.appspot.com/180107/diff/1/14
File src/applications/chord-ipv4/chord-node-table.h (right):

http://codereview.appspot.com/180107/diff/1/14#newcode29
src/applications/chord-ipv4/chord-node-table.h:29: typedef
std::map<ChordIdentifier, Ptr<ChordNode> > ChordNodeMap;
On 2010/01/13 12:51:12, Mathieu Lacage wrote:
> once you have change GetMap as suggested below, you can move these to private
> typedefs of the class

Done.

http://codereview.appspot.com/180107/diff/1/14#newcode60
src/applications/chord-ipv4/chord-node-table.h:60: bool FindNode
(Ptr<ChordIdentifier> &chordIdentifier, Ptr<ChordNode> &ChordNode);
On 2010/01/13 12:51:12, Mathieu Lacage wrote:
> A much simpler API which is much harder to abuse:
> Ptr<ChordNode> FindNode (Ptr<ChordIdentifier> identifier);

Done.

http://codereview.appspot.com/180107/diff/1/14#newcode67
src/applications/chord-ipv4/chord-node-table.h:67: bool FindNode (std::string
&name, Ptr<ChordNode> &chordNode);
On 2010/01/13 12:51:12, Mathieu Lacage wrote:
> why do you pass the name as a reference ?
> Again, the following is much simpler conceptually:
> Ptr<ChordNode> FindNode (std::string name);
> 
> we can optimize it later if needed.

Done.

http://codereview.appspot.com/180107/diff/1/14#newcode72
src/applications/chord-ipv4/chord-node-table.h:72: void RemoveNode
(Ptr<ChordIdentifier> &chordIdentifier);
On 2010/01/13 12:51:12, Mathieu Lacage wrote:
> gaaaah. please, don't pass all these arguments as references: it makes no
sense
> since you do not intend to modify their content.
> 
> This should be:
> void RemoveNode (Ptr<ChordIdentifier> chordIdentifier); 

Done.

http://codereview.appspot.com/180107/diff/1/14#newcode77
src/applications/chord-ipv4/chord-node-table.h:77: void RemoveNode (std::string
&name);
On 2010/01/13 12:51:12, Mathieu Lacage wrote:
> again, this should be:
> 
> void RemoveNode (std::string name);

Done.

http://codereview.appspot.com/180107/diff/1/14#newcode84
src/applications/chord-ipv4/chord-node-table.h:84: bool FindNearestNode
(Ptr<ChordIdentifier> &targetIdentifier, Ptr<ChordNode> &chordNode);
On 2010/01/13 12:51:12, Mathieu Lacage wrote:
> and that should be:
> Ptr<ChordNode> FindNearestNode (Ptr<ChordIdentifier> id);

Done.

http://codereview.appspot.com/180107/diff/1/14#newcode101
src/applications/chord-ipv4/chord-node-table.h:101: ChordNodeMap& GetMap();
On 2010/01/13 12:51:12, Mathieu Lacage wrote:
> Every user of this method uses it to iterate over the list of ChordNode so, I
> would rewrite this to the much less efficient but much safer:
> 
> std::vector<Ptr<ChordNode> > GetNodes (void);
> 
> which returns a copy so the caller cannot modify the internal data structure
of
> your call and which allows you to simplify your caller because every caller
had
> to access the internal second field of the map.

Done.

http://codereview.appspot.com/180107/diff/1/16
File src/applications/chord-ipv4/chord-node.h (right):

http://codereview.appspot.com/180107/diff/1/16#newcode86
src/applications/chord-ipv4/chord-node.h:86: Ipv4Address GetIpAddress ();
On 2010/01/13 12:51:12, Mathieu Lacage wrote:
> Ipv4Address GetIpAddress () const;

Done.

http://codereview.appspot.com/180107/diff/1/16#newcode90
src/applications/chord-ipv4/chord-node.h:90: uint16_t GetPort ();
On 2010/01/13 12:51:12, Mathieu Lacage wrote:
> same as above. Same for all getters.

Done.

http://codereview.appspot.com/180107/diff/1/16#newcode114
src/applications/chord-ipv4/chord-node.h:114: bool GetRoutable ();
On 2010/01/13 12:51:12, Mathieu Lacage wrote:
> Usually, I prefer to use Is prefixes for booleans:
> bool IsRoutable () const;

Done.

http://codereview.appspot.com/180107/diff/1/18
File src/applications/chord-ipv4/chord-transaction.h (right):

http://codereview.appspot.com/180107/diff/1/18#newcode37
src/applications/chord-ipv4/chord-transaction.h:37: class ChordTransaction :
public Object
On 2010/01/13 12:51:12, Mathieu Lacage wrote:
> again, probably no reason to derive from Object. Probably you want to derive
> from SimpleRefCount<ChordTransaction>

Done.

http://codereview.appspot.com/180107/diff/1/20
File src/applications/chord-ipv4/chord-vnode.h (right):

http://codereview.appspot.com/180107/diff/1/20#newcode61
src/applications/chord-ipv4/chord-vnode.h:61: std::vector<Ptr<ChordNode> >& 
GetSuccessorList(void);
On 2010/01/13 12:51:12, Mathieu Lacage wrote:
> returns a reference to an internal private field: evil because it allows the
> caller to wreak havoc in the internal state of this class without the class
> knowing about it. This method is unused (as well as GetPredecessorList) so,
kill
> them both, please.

Done.

http://codereview.appspot.com/180107/diff/1/20#newcode218
src/applications/chord-ipv4/chord-vnode.h:218: bool FindTransaction (uint32_t
transactionId, Ptr<ChordTransaction> &chordTransaction);
On 2010/01/13 12:51:12, Mathieu Lacage wrote:
> A simpler API would be:
> Ptr<ChrodTransaction> FindTransaction (uint32_t transactionId);
> 
> and the caller would check if the returned value is == 0 or != 0

Done.

http://codereview.appspot.com/180107/diff/1/23
File src/applications/chord-ipv4/dhash-ipv4.cc (right):

http://codereview.appspot.com/180107/diff/1/23#newcode682
src/applications/chord-ipv4/dhash-ipv4.cc:682: Ptr<DHashConnection>
dHashConnection = Create<DHashConnection> (ipAddress, port, socket);
On 2010/01/13 12:51:12, Mathieu Lacage wrote:
> bwaaaaaaah. Create<DHashConnection> (...) might work but you are really really
> lucky.
> 
> DHashConnection derives from Object. As such, you must create instances of
> DHashConnection with CreateObject, not Create.
> 
> Now, it's unclear to me why DHashConnection derives from Object so, another
> option would be to keep the call to Create and make DHashConnection derive
from
> SimpleRefCount<DhashConnection> instead of Object.

Done.

http://codereview.appspot.com/180107/diff/1/23#newcode683
src/applications/chord-ipv4/dhash-ipv4.cc:683: socket->SetRecvCallback
(MakeCallback(&DHashConnection::ReadTCPBuffer, dHashConnection));
On 2010/01/13 12:51:12, Mathieu Lacage wrote:
> I see that the DHashConnection class already has a pointer to the socket so,
if
> I were you, I would move this call to Socket::SetRecvCallback (same for
> Socket::SetCloseCallbacks) inside the DHashConnection constructor and, if you
> need this DHashIpv4 class to be notified that the socket/connection has been
> closed, you can introduce a DHashConnection::SetCloseCallback method, just
like
> you did introduce DHashConnection::SetRecvCallback.

Done.

http://codereview.appspot.com/180107/diff/1/23#newcode694
src/applications/chord-ipv4/dhash-ipv4.cc:694: DHashIpv4::FindConnection
(Ptr<Socket> socket, Ptr<DHashConnection>& dHashConnection)
This can be used later on to find out which socket is used for which DHash
Connection.

On 2010/01/13 12:51:12, Mathieu Lacage wrote:
> I did not find any caller of this method so, if there is none, it makes little
> sense to use a std::map to store the connections: you could store them in a
> std::list (or std::vector)

http://codereview.appspot.com/180107/diff/1/26
File src/applications/chord-ipv4/dhash-message.h (right):

http://codereview.appspot.com/180107/diff/1/26#newcode45
src/applications/chord-ipv4/dhash-message.h:45: void SetLength (uint32_t length)
On 2010/01/13 12:51:12, Mathieu Lacage wrote:
> no code in headers unless you have clear performance reasons (clear = hard
> performance measurements to assess the impact of that specific change on a
> realistic benchmark): move to .cc file.

Done.

http://codereview.appspot.com/180107/diff/1/26#newcode59
src/applications/chord-ipv4/dhash-message.h:59: TypeId GetInstanceTypeId (void)
const;
On 2010/01/13 12:51:12, Mathieu Lacage wrote:
> we usually repeat the virtual keyword when we define a virtual method defined
in
> a base class. This helps someone who is reading this header for the first time
> to figure out the semantics of the code

Done.

http://codereview.appspot.com/180107/diff/1/26#newcode284
src/applications/chord-ipv4/dhash-message.h:284: StoreReq& GetStoreReq ()
On 2010/01/13 12:51:12, Mathieu Lacage wrote:
> don't ever return a reference or pointer to a private field. Return a copy to
> make sure that the user can't abuse it. 
> 
> In this case, I see that callers of this method use it to actually modify the
> content of this private field. In this case, you need to add API like this:
> 
> void SetStoreReq (StoreReq req);
> 
> And if you do this, you can remove SetMessageType from your API because
> SetStoreReq can explicitely also set the type of the message.
> 

Done.

http://codereview.appspot.com/180107/diff/1/27
File src/applications/chord-ipv4/dhash-object.cc (right):

http://codereview.appspot.com/180107/diff/1/27#newcode127
src/applications/chord-ipv4/dhash-object.cc:127: for (uint32_t j=0 ;
j<m_sizeOfObject; j++)
On 2010/01/13 12:51:12, Mathieu Lacage wrote:
> a more efficient version would be start.Write (m_object, m_sizeOfObject);

Done.

http://codereview.appspot.com/180107/diff/1/27#newcode153
src/applications/chord-ipv4/dhash-object.cc:153: for (j=0; j<
m_sizeOfObject;j++)
On 2010/01/13 12:51:12, Mathieu Lacage wrote:
> same here: start.Read (m_object, m_sizeOfObject);

Done.

http://codereview.appspot.com/180107/diff/1/28
File src/applications/chord-ipv4/dhash-object.h (right):

http://codereview.appspot.com/180107/diff/1/28#newcode33
src/applications/chord-ipv4/dhash-object.h:33: class DHashObject : public Object
We have changed this. Derived from SimpleRefCount<>

On 2010/01/13 12:51:12, Mathieu Lacage wrote:
> same question here: why do you derive from Object ?

http://codereview.appspot.com/180107/diff/1/30
File src/applications/chord-ipv4/dhash-transaction.h (right):

http://codereview.appspot.com/180107/diff/1/30#newcode35
src/applications/chord-ipv4/dhash-transaction.h:35: class DHashTransaction :
public Object
On 2010/01/13 12:51:12, Mathieu Lacage wrote:
> why exactly are you deriving from Object here ? If all you need is reference
> counting (i.e., the Ptr<T> stuff), you can do this instead:
> 
> class DHashTransaction : public SimpleRefCount<DHashTransaction>
> 
> 

Done.

[Gnomff, 2011-02-14 00:21:40]

Hello,

I downloaded and installed this Chord application into my ns-3.10 installation,
and I have to say it's excellent. However, I was experiencing some intermittent
crashes while running the test script. An example error message:

Scheduling Command InsertVNode...
*** glibc detected ***
/home/tim/workspace/ns-3-allinone/ns-3.10/build/debug/examples/chord-run/chord-run:
munmap_chunk(): invalid pointer: 0x0868c0c0 ***

After some tinkering I began to expect that the stack was being corrupted. I saw
that in ChordIdentifier, the m_key buffer is not null terminated. It worried me
because GetKey exposes the pointer and is used in user-defined callbacks. You
never know when a silly user is going to use a function that expects a null
terminated string. I created the function

uint8_t *
ChordIdentifier::AllocateBuffer(uint8_t numBytes)
{
	uint8_t * keybuf = (uint8_t *) malloc((numBytes * sizeof (uint8_t)) +1);
	memset(keybuf, 0, numBytes +1);
	return keybuf;
}

to replace all the calls to malloc in ChordIdentifier to make sure m_key is
always null terminated. Since my alteration I haven't seen any more crashes,
although I'm still baffled about where in the code the stack is being corrupted.