TcpL4Protocol improvements

TcpL4Protocol improvements

[Tom Henderson, 2015-06-17 14:30:51]

https://codereview.appspot.com/244350043/diff/20001/src/internet/model/tcp-l4...
File src/internet/model/tcp-l4-protocol.h (right):

https://codereview.appspot.com/244350043/diff/20001/src/internet/model/tcp-l4...
src/internet/model/tcp-l4-protocol.h:94: 
I suggest to move the above two to private non-static members (this is only used
to support logging statements).

https://codereview.appspot.com/244350043/diff/20001/src/internet/model/tcp-l4...
src/internet/model/tcp-l4-protocol.h:101: * \brief Default deconstructor
I don't think the doxygen is needed (this is a trivial deconstructor) and
default terminology doesn't apply here (destructors can't be overloaded).

https://codereview.appspot.com/244350043/diff/20001/src/internet/model/tcp-l4...
src/internet/model/tcp-l4-protocol.h:236: Ptr<NetDevice> oif = 0) const;
I suggest to make the above 'Address' variant the only public: variant, and move
the other variants to private:  (to reduce the public API).

Also, this is an (internal) API change and perhaps warrants being mentioned in
CHANGES.html file (that TcpL4Protocol SendPacket() now requires a TcpHeader
argument).

https://codereview.appspot.com/244350043/diff/20001/src/internet/model/tcp-l4...
src/internet/model/tcp-l4-protocol.h:241: * Called after a socket has been
bound, it is inserted in an internal vector.
suggest 'of being demultiplexed'

no need to describe the internal storage, just the black-box effect that it
makes the socket operational.

https://codereview.appspot.com/244350043/diff/20001/src/internet/model/tcp-l4...
src/internet/model/tcp-l4-protocol.h:285: virtual int GetProtocolNumber (void)
const;
why virtual?

https://codereview.appspot.com/244350043/diff/20001/src/internet/model/tcp-l4...
src/internet/model/tcp-l4-protocol.h:299: * it to the ipv4 stack and adding TCP
socket factory to the node.
ipv4 and ipv6

https://codereview.appspot.com/244350043/diff/20001/src/internet/model/tcp-l4...
src/internet/model/tcp-l4-protocol.h:304: * \brief Get the tcp header of the
incoming packet and checks its checksum
and checks its checksum if needed

https://codereview.appspot.com/244350043/diff/20001/src/internet/model/tcp-l4...
src/internet/model/tcp-l4-protocol.h:307: * \param incomingTcpHeader At the end
will contain the tcp header of the packet
suggest s/At the end will contain the/Overwritten with the/

https://codereview.appspot.com/244350043/diff/20001/src/internet/model/tcp-l4...
src/internet/model/tcp-l4-protocol.h:308: * \param source Source address (who
sent the packet)
suggest: s/Source address (who sent the packet)/Source address (an underlying
Ipv4Address or Ipv6Address)/

similar comment on destination address parameter

https://codereview.appspot.com/244350043/diff/20001/src/internet/model/tcp-so...
File src/internet/model/tcp-socket-base.cc (right):

https://codereview.appspot.com/244350043/diff/20001/src/internet/model/tcp-so...
src/internet/model/tcp-socket-base.cc:334: m_tcp->AddSocket(this);
ns-3 style is to add space before opening parenthesis

[n.p, 2015-06-18 10:57:34]

https://codereview.appspot.com/244350043/diff/20001/src/internet/model/tcp-l4...
File src/internet/model/tcp-l4-protocol.h (right):

https://codereview.appspot.com/244350043/diff/20001/src/internet/model/tcp-l4...
src/internet/model/tcp-l4-protocol.h:94: 
On 2015/06/17 14:30:50, Tom Henderson wrote:
> I suggest to move the above two to private non-static members (this is only
used
> to support logging statements).

Do you think these could be reused in other code ? 

Or, maybe better, what do you think if I implement std::ostream operator << in
both TcpHeader and Ipv{4,6}Header so these functions are not needed at all?

https://codereview.appspot.com/244350043/diff/20001/src/internet/model/tcp-l4...
src/internet/model/tcp-l4-protocol.h:236: Ptr<NetDevice> oif = 0) const;
On 2015/06/17 14:30:50, Tom Henderson wrote:
> I suggest to make the above 'Address' variant the only public: variant, and
move
> the other variants to private:  (to reduce the public API).

Ok

> Also, this is an (internal) API change and perhaps warrants being mentioned in
> CHANGES.html file (that TcpL4Protocol SendPacket() now requires a TcpHeader
> argument).

Uhm, for the API change OK, but perhaps you confused Send() and SendPacket().
SendPacket was private and accepting a TcpHeader argument (it was the only
function used somewhere in the code) while Send() takes no TcpHeader but it was
not used anywhere (and so removed).


Also, moving the two SendPacket to private would imply that I need to cast to
Address any call to SendPacket. For instance, from

m_tcp->SendPacket (p, header, m_endPoint->GetLocalAddress (),
                         m_endPoint->GetPeerAddress (),
                         m_boundnetdevice);

it becomes

m_tcp->SendPacket (p, header, Address (m_endPoint->GetLocalAddress ()),
                         Address (m_endPoint->GetPeerAddress ()),
                         m_boundnetdevice);

or I get compiler errors on SendPacket (Ipv4Address ...) being private./test.py

https://codereview.appspot.com/244350043/diff/20001/src/internet/model/tcp-l4...
src/internet/model/tcp-l4-protocol.h:285: virtual int GetProtocolNumber (void)
const;
On 2015/06/17 14:30:50, Tom Henderson wrote:
> why virtual?

It's virtual in the base class (IpL4Protocol)

https://codereview.appspot.com/244350043/diff/20001/src/internet/model/tcp-l4...
src/internet/model/tcp-l4-protocol.h:299: * it to the ipv4 stack and adding TCP
socket factory to the node.
On 2015/06/17 14:30:50, Tom Henderson wrote:
> ipv4 and ipv6

ok

https://codereview.appspot.com/244350043/diff/20001/src/internet/model/tcp-l4...
src/internet/model/tcp-l4-protocol.h:304: * \brief Get the tcp header of the
incoming packet and checks its checksum
On 2015/06/17 14:30:50, Tom Henderson wrote:
> and checks its checksum if needed

ok

https://codereview.appspot.com/244350043/diff/20001/src/internet/model/tcp-l4...
src/internet/model/tcp-l4-protocol.h:307: * \param incomingTcpHeader At the end
will contain the tcp header of the packet
On 2015/06/17 14:30:50, Tom Henderson wrote:
> suggest s/At the end will contain the/Overwritten with the/

ok

https://codereview.appspot.com/244350043/diff/20001/src/internet/model/tcp-l4...
src/internet/model/tcp-l4-protocol.h:308: * \param source Source address (who
sent the packet)
On 2015/06/17 14:30:50, Tom Henderson wrote:
> suggest: s/Source address (who sent the packet)/Source address (an underlying
> Ipv4Address or Ipv6Address)/
> 
> similar comment on destination address parameter

ok

[n.p, 2015-06-18 13:03:15]

I've posted an update of this review request. In case it should be
merged, I can provide the single patches for every commit, in order to
be easily bisected in case of problems.

Nat

On 18/06/15 at 10:57am, natale.patriciello@gmail.com wrote:
> 
>
https://codereview.appspot.com/244350043/diff/20001/src/internet/model/tcp-l4...
> File src/internet/model/tcp-l4-protocol.h (right):
> 
>
https://codereview.appspot.com/244350043/diff/20001/src/internet/model/tcp-l4...
> src/internet/model/tcp-l4-protocol.h:94:
> On 2015/06/17 14:30:50, Tom Henderson wrote:
> >I suggest to move the above two to private non-static members (this is
> only used
> >to support logging statements).
> 
> Do you think these could be reused in other code ?
> 
> Or, maybe better, what do you think if I implement std::ostream operator
> << in both TcpHeader and Ipv{4,6}Header so these functions are not
> needed at all?
> 
>
https://codereview.appspot.com/244350043/diff/20001/src/internet/model/tcp-l4...
> src/internet/model/tcp-l4-protocol.h:236: Ptr<NetDevice> oif = 0) const;
> On 2015/06/17 14:30:50, Tom Henderson wrote:
> >I suggest to make the above 'Address' variant the only public:
> variant, and move
> >the other variants to private:  (to reduce the public API).
> 
> Ok
> 
> >Also, this is an (internal) API change and perhaps warrants being
> mentioned in
> >CHANGES.html file (that TcpL4Protocol SendPacket() now requires a
> TcpHeader
> >argument).
> 
> Uhm, for the API change OK, but perhaps you confused Send() and
> SendPacket(). SendPacket was private and accepting a TcpHeader argument
> (it was the only function used somewhere in the code) while Send() takes
> no TcpHeader but it was not used anywhere (and so removed).
> 
> 
> Also, moving the two SendPacket to private would imply that I need to
> cast to Address any call to SendPacket. For instance, from
> 
> m_tcp->SendPacket (p, header, m_endPoint->GetLocalAddress (),
>                          m_endPoint->GetPeerAddress (),
>                          m_boundnetdevice);
> 
> it becomes
> 
> m_tcp->SendPacket (p, header, Address (m_endPoint->GetLocalAddress ()),
>                          Address (m_endPoint->GetPeerAddress ()),
>                          m_boundnetdevice);
> 
> or I get compiler errors on SendPacket (Ipv4Address ...) being
> private./test.py
> 
>
https://codereview.appspot.com/244350043/diff/20001/src/internet/model/tcp-l4...
> src/internet/model/tcp-l4-protocol.h:285: virtual int GetProtocolNumber
> (void) const;
> On 2015/06/17 14:30:50, Tom Henderson wrote:
> >why virtual?
> 
> It's virtual in the base class (IpL4Protocol)
> 
>
https://codereview.appspot.com/244350043/diff/20001/src/internet/model/tcp-l4...
> src/internet/model/tcp-l4-protocol.h:299: * it to the ipv4 stack and
> adding TCP socket factory to the node.
> On 2015/06/17 14:30:50, Tom Henderson wrote:
> >ipv4 and ipv6
> 
> ok
> 
>
https://codereview.appspot.com/244350043/diff/20001/src/internet/model/tcp-l4...
> src/internet/model/tcp-l4-protocol.h:304: * \brief Get the tcp header of
> the incoming packet and checks its checksum
> On 2015/06/17 14:30:50, Tom Henderson wrote:
> >and checks its checksum if needed
> 
> ok
> 
>
https://codereview.appspot.com/244350043/diff/20001/src/internet/model/tcp-l4...
> src/internet/model/tcp-l4-protocol.h:307: * \param incomingTcpHeader At
> the end will contain the tcp header of the packet
> On 2015/06/17 14:30:50, Tom Henderson wrote:
> >suggest s/At the end will contain the/Overwritten with the/
> 
> ok
> 
>
https://codereview.appspot.com/244350043/diff/20001/src/internet/model/tcp-l4...
> src/internet/model/tcp-l4-protocol.h:308: * \param source Source address
> (who sent the packet)
> On 2015/06/17 14:30:50, Tom Henderson wrote:
> >suggest: s/Source address (who sent the packet)/Source address (an
> underlying
> >Ipv4Address or Ipv6Address)/
> 
> >similar comment on destination address parameter
> 
> ok
> 
> https://codereview.appspot.com/244350043/

[Tom Henderson, 2015-06-23 13:48:50]

On 2015/06/18 10:57:34, n.p wrote:
>
https://codereview.appspot.com/244350043/diff/20001/src/internet/model/tcp-l4...
> File src/internet/model/tcp-l4-protocol.h (right):
> 
>
https://codereview.appspot.com/244350043/diff/20001/src/internet/model/tcp-l4...
> src/internet/model/tcp-l4-protocol.h:94: 
> On 2015/06/17 14:30:50, Tom Henderson wrote:
> > I suggest to move the above two to private non-static members (this is only
> used
> > to support logging statements).
> 
> Do you think these could be reused in other code ? 
> 
> Or, maybe better, what do you think if I implement std::ostream operator << in
> both TcpHeader and Ipv{4,6}Header so these functions are not needed at all?

Agree with operator<<

> 
>
https://codereview.appspot.com/244350043/diff/20001/src/internet/model/tcp-l4...
> src/internet/model/tcp-l4-protocol.h:236: Ptr<NetDevice> oif = 0) const;
> On 2015/06/17 14:30:50, Tom Henderson wrote:
> > I suggest to make the above 'Address' variant the only public: variant, and
> move
> > the other variants to private:  (to reduce the public API).
> 
> Ok
> 
> > Also, this is an (internal) API change and perhaps warrants being mentioned
in
> > CHANGES.html file (that TcpL4Protocol SendPacket() now requires a TcpHeader
> > argument).
> 
> Uhm, for the API change OK, but perhaps you confused Send() and SendPacket().
> SendPacket was private and accepting a TcpHeader argument (it was the only
> function used somewhere in the code) while Send() takes no TcpHeader but it
was
> not used anywhere (and so removed).

OK

> 
> 
> Also, moving the two SendPacket to private would imply that I need to cast to
> Address any call to SendPacket. For instance, from
> 
> m_tcp->SendPacket (p, header, m_endPoint->GetLocalAddress (),
>                          m_endPoint->GetPeerAddress (),
>                          m_boundnetdevice);
> 
> it becomes
> 
> m_tcp->SendPacket (p, header, Address (m_endPoint->GetLocalAddress ()),
>                          Address (m_endPoint->GetPeerAddress ()),
>                          m_boundnetdevice);
> 
> or I get compiler errors on SendPacket (Ipv4Address ...) being
private./test.py

Can we remove the other overloaded functions and just have one SendPacket()?

It is not a major point in this one instance, but it is somewhat against the
ns-3 API style to provide lots of overloaded variants of methods with different
address classes, since the Address class is polymorphic.  See, for instance, the
Socket class which is declared only with class Address but which accepts various
different address types.

> 
>
https://codereview.appspot.com/244350043/diff/20001/src/internet/model/tcp-l4...
> src/internet/model/tcp-l4-protocol.h:285: virtual int GetProtocolNumber (void)
> const;
> On 2015/06/17 14:30:50, Tom Henderson wrote:
> > why virtual?
> 
> It's virtual in the base class (IpL4Protocol)

OK

> 
>
https://codereview.appspot.com/244350043/diff/20001/src/internet/model/tcp-l4...
> src/internet/model/tcp-l4-protocol.h:299: * it to the ipv4 stack and adding
TCP
> socket factory to the node.
> On 2015/06/17 14:30:50, Tom Henderson wrote:
> > ipv4 and ipv6
> 
> ok
> 
>
https://codereview.appspot.com/244350043/diff/20001/src/internet/model/tcp-l4...
> src/internet/model/tcp-l4-protocol.h:304: * \brief Get the tcp header of the
> incoming packet and checks its checksum
> On 2015/06/17 14:30:50, Tom Henderson wrote:
> > and checks its checksum if needed
> 
> ok
> 
>
https://codereview.appspot.com/244350043/diff/20001/src/internet/model/tcp-l4...
> src/internet/model/tcp-l4-protocol.h:307: * \param incomingTcpHeader At the
end
> will contain the tcp header of the packet
> On 2015/06/17 14:30:50, Tom Henderson wrote:
> > suggest s/At the end will contain the/Overwritten with the/
> 
> ok
> 
>
https://codereview.appspot.com/244350043/diff/20001/src/internet/model/tcp-l4...
> src/internet/model/tcp-l4-protocol.h:308: * \param source Source address (who
> sent the packet)
> On 2015/06/17 14:30:50, Tom Henderson wrote:
> > suggest: s/Source address (who sent the packet)/Source address (an
underlying
> > Ipv4Address or Ipv6Address)/
> > 
> > similar comment on destination address parameter
> 
> ok

[n.p, 2015-06-26 10:35:04]

On 2015/06/23 13:48:50, Tom Henderson wrote:
> Can we remove the other overloaded functions and just have one SendPacket()?
> 
> It is not a major point in this one instance, but it is somewhat against the
> ns-3 API style to provide lots of overloaded variants of methods with
different
> address classes, since the Address class is polymorphic.  See, for instance,
the
> Socket class which is declared only with class Address but which accepts
various
> different address types.

The two functions (ipv4 and ipv6 are there because the things that they should
do are different, and switching between them with a big if statement is not a
viable solution IMHO.

Anyway, I renamed them in SendPacketV4 and SendPacketV6 and make them private;
this way, only SendPacket (Address, Address) is public and compiler do not blame
for anything. To recap:

public:
void SendPacket (Ptr<Packet> pkt, const TcpHeader &outgoing,
                   const Address &saddr, const Address &daddr,
                   Ptr<NetDevice> oif = 0) const

private:

void SendPacketV4 (Ptr<Packet> pkt, const TcpHeader &outgoing,
                   const Ipv4Address &saddr, const Ipv4Address &daddr,
                   Ptr<NetDevice> oif = 0) const


void SendPacketV6 (Ptr<Packet> pkt, const TcpHeader &outgoing,
                   const Ipv6Address &saddr, const Ipv6Address &daddr,
                   Ptr<NetDevice> oif = 0) const