Bug 671

+1 to merge it.

It is interresting for radvd application to process Router Solicitation (RS) in a clever way. The current behavior is reply with RAs on all interfaces. With this patch I can modify radvd to just reply on the interface the RS was received.

> Index: src/node/node.cc
> ===================================================================
> --- a/src/node/node.cc
> +++ b/src/node/node.cc
> @@ -28,6 +28,7 @@
>  #include "ns3/uinteger.h"
>  #include "ns3/log.h"
>  #include "ns3/assert.h"
> +#include "ns3/socket.h"
>
>  NS_LOG_COMPONENT_DEFINE ("Node");
>
> @@ -247,6 +248,10 @@
>                 << ") Packet UID " << packet->GetUid ());
>    bool found = false;
>
> +  SocketRecvIfTag tag;
> +  tag.SetRecvIf (device->GetIfIndex());
> +  packet->AddTag (tag);

The above is obsolete Tag usage.  Needs to be a byte or packet tag.  Suggest PacketTag.

> +
> +  virtual void SetIfIndex(uint32_t ifIndex);
>
> +void
> +Socket::SetIfIndex(uint32_t ifIndex)
> +{
> +  NS_ASSERT(0);
> +}
> +

Suggest to delete Socket::SetIfIndex () since it just causes an assert, and is not documented.

(minor nit-- please use space before parentheses in function names)

> +class SocketRecvIfTag : public Tag

This class needs doxygen.

Has this been tested for loopback interface (will loopback hit the node.cc code segment)?

Created attachment 708 [details]
Modify as newer Tag usage

I'm not aware of this bugzilla. sorry for late reply.

(In reply to comment #2)
> > Index: src/node/node.cc
> > ===================================================================
> > --- a/src/node/node.cc
> > +++ b/src/node/node.cc
> > @@ -28,6 +28,7 @@
> >  #include "ns3/uinteger.h"
> >  #include "ns3/log.h"
> >  #include "ns3/assert.h"
> > +#include "ns3/socket.h"
> >
> >  NS_LOG_COMPONENT_DEFINE ("Node");
> >
> > @@ -247,6 +248,10 @@
> >                 << ") Packet UID " << packet->GetUid ());
> >    bool found = false;
> >
> > +  SocketRecvIfTag tag;
> > +  tag.SetRecvIf (device->GetIfIndex());
> > +  packet->AddTag (tag);
> 
> The above is obsolete Tag usage.  Needs to be a byte or packet tag.  Suggest
> PacketTag.

Attached patch reflect this new Tag usage.

> > +
> > +  virtual void SetIfIndex(uint32_t ifIndex);
> >
> > +void
> > +Socket::SetIfIndex(uint32_t ifIndex)
> > +{
> > +  NS_ASSERT(0);
> > +}
> > +
> 
> Suggest to delete Socket::SetIfIndex () since it just causes an assert, and is
> not documented.
> 
> (minor nit-- please use space before parentheses in function names)

I meant this method should be implemented every child class.
SInce it involves every class modification, I removed this modification in this patch.

> > +class SocketRecvIfTag : public Tag
> 
> This class needs doxygen.

I included doxygen comments in patch.

> Has this been tested for loopback interface (will loopback hit the node.cc code
> segment)?

I have never tested with loopback, but this tag (SocketRecvIfTag) is only added in Node::ReceiveFromDevice (), while loopback only received inside their own send method.

Am I right?

Created attachment 709 [details]
new patch

This patch moves the tag setting code out of class Node into the IPv4/6 code, fixes one of the examples that needs to delete this tag, and adds a more descriptive assert message for when multiple tags of same type are added.

(In reply to comment #4)
> I'm not aware of this bugzilla. sorry for late reply.
> 
> (In reply to comment #2)
> > > Index: src/node/node.cc
> > > ===================================================================
> > > --- a/src/node/node.cc
> > > +++ b/src/node/node.cc
> > > @@ -28,6 +28,7 @@
> > >  #include "ns3/uinteger.h"
> > >  #include "ns3/log.h"
> > >  #include "ns3/assert.h"
> > > +#include "ns3/socket.h"

I think we'd be better off to keep this out of class Node, which presently does not have any socket dependencies (think of future ns-3 Nodes that are not TCP/IP-based).  So the new patch just posted adds this to the IPv4 and IPv6 code instead.

> 
> > Has this been tested for loopback interface (will loopback hit the node.cc code
> > segment)?
> 
> I have never tested with loopback, but this tag (SocketRecvIfTag) is only added
> in Node::ReceiveFromDevice (), while loopback only received inside their own
> send method.
> 
> Am I right?

The new patch handles the loopback interface correctly, I believe.

I think there are two remaining issues with this patch:

1) there should be some test code added to make sure it doesn't break in the future

2) raw sockets are not handled.  If you look at the implementation of raw sockets, the packets are not preserved across the sockets API boundary.  So, I think it will work for TCP/IP but not for raw sockets on the receive side.  Is this a problem for the IPv6 code?  Should we fix it now, or document it as a limitation of this socket tag?

Thanks Tom,

(In reply to comment #6)
> (In reply to comment #4)
> > I'm not aware of this bugzilla. sorry for late reply.
> > 
> > (In reply to comment #2)
> > > > Index: src/node/node.cc
> > > > ===================================================================
> > > > --- a/src/node/node.cc
> > > > +++ b/src/node/node.cc
> > > > @@ -28,6 +28,7 @@
> > > >  #include "ns3/uinteger.h"
> > > >  #include "ns3/log.h"
> > > >  #include "ns3/assert.h"
> > > > +#include "ns3/socket.h"
> 
> I think we'd be better off to keep this out of class Node, which presently does
> not have any socket dependencies (think of future ns-3 Nodes that are not
> TCP/IP-based).  So the new patch just posted adds this to the IPv4 and IPv6
> code instead.

Hmm, let's see.

> > > Has this been tested for loopback interface (will loopback hit the node.cc code
> > > segment)?
> > 
> > I have never tested with loopback, but this tag (SocketRecvIfTag) is only added
> > in Node::ReceiveFromDevice (), while loopback only received inside their own
> > send method.
> > 
> > Am I right?
> 
> The new patch handles the loopback interface correctly, I believe.
> 
> I think there are two remaining issues with this patch:
> 
> 1) there should be some test code added to make sure it doesn't break in the
> future

I agree.

> 2) raw sockets are not handled.  If you look at the implementation of raw
> sockets, the packets are not preserved across the sockets API boundary.  So, I
> think it will work for TCP/IP but not for raw sockets on the receive side.  Is
> this a problem for the IPv6 code?  Should we fix it now, or document it as a
> limitation of this socket tag?

I wish we support this tag for raw sockets too.

Actually, the aim of this tag modification is used for the
IPv6 raw socket. When a node has multiple interfaces and received
two RS message, the message should be able to distinguish
which interface he receives from.

Some POSIX/socket application (in my case, zebra/quagga)
uses recvmsg () system call with IPV6_PKTINFO. It is handled
in the branch ns-3-simu.

You can see the example of this tag usage as follows.

http://www.sfc.wide.ad.jp/~tazaki/hg/ns-3-simu_zebra_ipv6-2nd/diff/fc2c29a13125/src/process-manager/unix-datagram-socket-fd.cc


I didn't understand what you meant in the following sentence.

> If you look at the implementation of raw
>sockets, the packets are not preserved across the sockets API boundary.  

How can I see the sockets API boundary?


FYI, I took a look of other OS implementation.

In NetBSD, struct pkthdr, which is pointed from struct mbuf,
includes rcvif information. This value seems to be added at
NIC driver codes, and delivered as ipi6_ifindex in
IPV6_PKTINFO.

In Linux, struct net_device, which is pointed from struct
sk_buff, includes iif information. This value seems to be
added at sk_buff allocation, and delivered as ipi6_ifindex
in IPV6_PKTINFO.

The recv if information is handled by device driver in above case.
In ns-3, we can also handle in every NetDevice class, but I took to implement
in class Node instead.

What do you think, Tom?

> You can see the example of this tag usage as follows.
> 
> http://www.sfc.wide.ad.jp/~tazaki/hg/ns-3-simu_zebra_ipv6-2nd/diff/fc2c29a13125/src/process-manager/unix-datagram-socket-fd.cc

Yes, I agree and understand how you want to use it.

> 
> 
> I didn't understand what you meant in the following sentence.
> 
> > If you look at the implementation of raw
> >sockets, the packets are not preserved across the sockets API boundary.  
> 
> How can I see the sockets API boundary?

Actually, I think we are OK on this point.  Packets are queued in a std::list<struct Data> m_data, and the tag will be kept on the packet as it is delivered across the API using Recv().

So, the issue seems to be what we add and how/where to add it.  The place in LocalDeliver where I added it will not work for the raw sockets.  I'm not even sure that the raw socket reception code is correctly placed in the input path processing, but in any case, the current patch will not support raw sockets so clearly we should fix this since it is IPv6 RS that wants to use it, among others.

> 
> 
> FYI, I took a look of other OS implementation.
> 
> In NetBSD, struct pkthdr, which is pointed from struct mbuf,
> includes rcvif information. This value seems to be added at
> NIC driver codes, and delivered as ipi6_ifindex in
> IPV6_PKTINFO.
> 
> In Linux, struct net_device, which is pointed from struct
> sk_buff, includes iif information. This value seems to be
> added at sk_buff allocation, and delivered as ipi6_ifindex
> in IPV6_PKTINFO.
> 
> The recv if information is handled by device driver in above case.
> In ns-3, we can also handle in every NetDevice class, but I took to implement
> in class Node instead.

If we make it added in the Node, my main issue was that it added a socket dependency where we have avoided it in the past.  To avoid this dependency but add at a low layer, we could instead call it a RecvIfIndex tag and declare it in net-device.h.  I also noticed that it was causing asserts in some scripts to add this tag at a low layer since a Packet received on CSMA interfaces is received on multiple interfaces before being thrown away by non-recipients.  We should also double check that adding this tag to packets that we ultimately are throwing away (since they are not for us) will not cause a deep buffer copy of the packet.

Another question I have is whether users will come back at a later date asking for all of these RFC 2292 values:
       1.  the destination IPv6 address,
       2.  the arriving interface index, and
       3.  the arriving hop limit.
and not just the arriving interface index, and that we might want to just make this tag an IP_PKTINFO tag with multiple fields.  What do you think-- will we end up needing to fully support the ancillary data fields for ns-3-simu?  Should they be handled in one or multiple tags?

(In reply to comment #8)
> > How can I see the sockets API boundary?
> 
> Actually, I think we are OK on this point.  Packets are queued in a
> std::list<struct Data> m_data, and the tag will be kept on the packet as it is
> delivered across the API using Recv().

Okay.

> So, the issue seems to be what we add and how/where to add it.  The place in
> LocalDeliver where I added it will not work for the raw sockets.  I'm not even
> sure that the raw socket reception code is correctly placed in the input path
> processing, but in any case, the current patch will not support raw sockets so
> clearly we should fix this since it is IPv6 RS that wants to use it, among
> others.

How about adding Tag before calling LocalDeliver (e.g. Ipv6L3Protocol::Receive) ?

I guess that raw socket can inspect more information than other local delivery 
to upper layer (e.g. tcp/udp). I think the place of raw socket code is okay...

> > The recv if information is handled by device driver in above case.
> > In ns-3, we can also handle in every NetDevice class, but I took to implement
> > in class Node instead.
> 
> If we make it added in the Node, my main issue was that it added a socket
> dependency where we have avoided it in the past.  To avoid this dependency but
> add at a low layer, we could instead call it a RecvIfIndex tag and declare it
> in net-device.h.  I also noticed that it was causing asserts in some scripts to
> add this tag at a low layer since a Packet received on CSMA interfaces is
> received on multiple interfaces before being thrown away by non-recipients.  We
> should also double check that adding this tag to packets that we ultimately are
> throwing away (since they are not for us) will not cause a deep buffer copy of
> the packet.

Okay, I understand to avoid socket dependencies in class Node/NetDevice.
and agreed with the modification into IPv4/6 code, if raw socket will be supported.


> Another question I have is whether users will come back at a later date asking
> for all of these RFC 2292 values:
>        1.  the destination IPv6 address,
>        2.  the arriving interface index, and
>        3.  the arriving hop limit.

now RFC 3542 has more.
      1.  the destination IPv6 address,
      2.  the arriving interface index,
      3.  the arriving hop limit, and
      4.  the arriving traffic class value.

> and not just the arriving interface index, and that we might want to just make
> this tag an IP_PKTINFO tag with multiple fields.  What do you think-- 

Uh, this tag IP_PKTINFO with multiple fields sound good.

> will we
> end up needing to fully support the ancillary data fields for ns-3-simu? 
> Should they be handled in one or multiple tags?

I hope these value can be supported.

(In reply to comment #9)

> > and not just the arriving interface index, and that we might want to just make
> > this tag an IP_PKTINFO tag with multiple fields.  What do you think-- 
> 
> Uh, this tag IP_PKTINFO with multiple fields sound good.
> 
> > will we
> > end up needing to fully support the ancillary data fields for ns-3-simu? 
> > Should they be handled in one or multiple tags?
> 
> I hope these value can be supported.

I've requested the review for patch of this feature.

http://codereview.appspot.com/849047/show

Please find the changeset.

Note, discussion of this patch has migrated to the codereview issue above.

Reviewed at
http://codereview.appspot.com/849047/show

and merged as 

http://code.nsnam.org/ns-3-dev/rev/f380cf1aa4d8