add packet-info-tag.cc for IP_PKTINFO/IPV6_PKTINFO

This changeset is created against ns-3-dev, revision 6168.

The patch aims to support new Tag class "PacketInfoTag", which carries received interface index, destination address and so on, which is defined RFC3542 for IPv6.

It will be used for quagga support in ns-3 in the future.

[Hajime Tazaki, 2010-04-02 17:14:24]

Hi, (Tom?)

Please review new changset to support IP_PKTINFO/IPV6_PKTINFO option.
It carries the packet information to the socket interface.

regards,
hajime

[Mathieu Lacage, 2010-04-07 14:53:09]

this code looks good to me. tom, josh, is this ns-3-dev material or are we too
late in the release ?

[Mathieu Lacage, 2010-04-07 14:56:24]

http://codereview.appspot.com/849047/diff/1/4
File src/internet-stack/wscript (right):

http://codereview.appspot.com/849047/diff/1/4#newcode137
src/internet-stack/wscript:137: 'ipv6-interface.h',
I have pushed this trivial change since we already export ipv4-interface.h

http://codereview.appspot.com/849047/diff/1/4#newcode143
src/internet-stack/wscript:143: 'ipv4-raw-socket-impl.h',
hajime, are you sure that you need to export these -impl.h headers ? If so, can
you point me to the code which uses them ?

[Josh Pelkey, 2010-04-07 14:57:16]

Too late for all but bug fixes.  Sorry!  This can go in very early ns-3-dev for
ns-3.9 release.

--
Josh

On 2010/04/07 14:53:09, Mathieu Lacage wrote:
> this code looks good to me. tom, josh, is this ns-3-dev material or are we too
> late in the release ?

[Mathieu Lacage, 2010-04-07 15:05:01]

On Wed, Apr 7, 2010 at 4:57 PM,  <joshpelkey@gmail.com> wrote:
> Too late for all but bug fixes.  Sorry!  This can go in very early
> ns-3-dev for ns-3.9 release.

Ok. I am going to setup a ns-3-simu-outgoing tree which contains
ns-3-dev material for merging in 3.9.
-- 
Mathieu Lacage <mathieu.lacage@gmail.com>

[Hajime Tazaki, 2010-04-07 15:38:40]

http://codereview.appspot.com/849047/diff/1/4
File src/internet-stack/wscript (right):

http://codereview.appspot.com/849047/diff/1/4#newcode143
src/internet-stack/wscript:143: 'ipv4-raw-socket-impl.h',
On 2010/04/07 14:56:24, Mathieu Lacage wrote:
> hajime, are you sure that you need to export these -impl.h headers ? If so,
can
> you point me to the code which uses them ? 


I think I'm not sure ..

In this changeset, -impl.h is used as follows:

+void
+PacketInfoTagTest::DoSendData (Ptr<Socket> socket, std::string to)
+{
+  Address realTo = InetSocketAddress (Ipv4Address (to.c_str()), 200);
+  if (socket->GetInstanceTypeId () == Ipv4RawSocketImpl::GetTypeId ())
+    {
+      socket->SendTo (Create<Packet> (123), 0, realTo);
+    }
+  else if (socket->GetInstanceTypeId () == UdpSocketImpl::GetTypeId ())
+    {
+      NS_TEST_EXPECT_MSG_EQ (socket->SendTo (Create<Packet> (123), 0, realTo),
+                             123, "XXX");
+    }
+}

In my coming patches in ns-3-simu, there is several patch which requires to
change the behavior between types of socket class. In that case, I have used
UdpSocketImpl::GetTypeId () to check the instance of socket.

Is there any better way rather than this approach?

[Mathieu Lacage, 2010-04-07 15:41:15]

On Wed, Apr 7, 2010 at 5:38 PM,  <thehajime@gmail.com> wrote:
>
> http://codereview.appspot.com/849047/diff/1/4
> File src/internet-stack/wscript (right):
>
> http://codereview.appspot.com/849047/diff/1/4#newcode143
> src/internet-stack/wscript:143: 'ipv4-raw-socket-impl.h',
> On 2010/04/07 14:56:24, Mathieu Lacage wrote:
>>
>> hajime, are you sure that you need to export these -impl.h headers ?
>
> If so, can
>>
>> you point me to the code which uses them ?
>
>
> I think I'm not sure ..
>
> In this changeset, -impl.h is used as follows:
>
> +void
> +PacketInfoTagTest::DoSendData (Ptr<Socket> socket, std::string to)
> +{
> +  Address realTo = InetSocketAddress (Ipv4Address (to.c_str()), 200);
> +  if (socket->GetInstanceTypeId () == Ipv4RawSocketImpl::GetTypeId ())
> +    {
> +      socket->SendTo (Create<Packet> (123), 0, realTo);
> +    }
> +  else if (socket->GetInstanceTypeId () == UdpSocketImpl::GetTypeId ())
> +    {
> +      NS_TEST_EXPECT_MSG_EQ (socket->SendTo (Create<Packet> (123), 0,
> realTo),
> +                             123, "XXX");
> +    }
> +}
>
> In my coming patches in ns-3-simu, there is several patch which requires
> to change the behavior between types of socket class. In that case, I
> have used UdpSocketImpl::GetTypeId () to check the instance of socket.
>
> Is there any better way rather than this approach?

Yes. DynamicCast<UdpSocket> (sock) != 0. In this case, it will not
work with Ipv4RawSocketImpl because we don't define Ipv4RawSocket in
src/node/ipv4-raw-socket.h. I would support add the latter. I am about
to merge a workaround this issue in ns-3-outgoing.

Mathieu
-- 
Mathieu Lacage <mathieu.lacage@gmail.com>

[Hajime Tazaki, 2010-04-07 15:52:29]

At Wed, 7 Apr 2010 17:41:12 +0200,
Mathieu Lacage wrote:
>
>On Wed, Apr 7, 2010 at 5:38 PM,  <thehajime@gmail.com> wrote:
>>
>> http://codereview.appspot.com/849047/diff/1/4
>> File src/internet-stack/wscript (right):
>>
>> http://codereview.appspot.com/849047/diff/1/4#newcode143
>> src/internet-stack/wscript:143: 'ipv4-raw-socket-impl.h',
>> On 2010/04/07 14:56:24, Mathieu Lacage wrote:
>>>
>>> hajime, are you sure that you need to export these -impl.h headers ?
>>
>> If so, can
>>>
>>> you point me to the code which uses them ?
>>
>>
>> I think I'm not sure ..
>>
>> In this changeset, -impl.h is used as follows:
>>
>> +void
>> +PacketInfoTagTest::DoSendData (Ptr<Socket> socket, std::string to)
>> +{
>> +  Address realTo = InetSocketAddress (Ipv4Address (to.c_str()), 200);
>> +  if (socket->GetInstanceTypeId () == Ipv4RawSocketImpl::GetTypeId ())
>> +    {
>> +      socket->SendTo (Create<Packet> (123), 0, realTo);
>> +    }
>> +  else if (socket->GetInstanceTypeId () == UdpSocketImpl::GetTypeId ())
>> +    {
>> +      NS_TEST_EXPECT_MSG_EQ (socket->SendTo (Create<Packet> (123), 0,
>> realTo),
>> +                             123, "XXX");
>> +    }
>> +}
>>
>> In my coming patches in ns-3-simu, there is several patch which requires
>> to change the behavior between types of socket class. In that case, I
>> have used UdpSocketImpl::GetTypeId () to check the instance of socket.
>>
>> Is there any better way rather than this approach?
>
>Yes. DynamicCast<UdpSocket> (sock) != 0. In this case, it will not
>work with Ipv4RawSocketImpl because we don't define Ipv4RawSocket in
>src/node/ipv4-raw-socket.h. I would support add the latter. I am about
>to merge a workaround this issue in ns-3-outgoing.

Thanks.

I'll modify the instance check with this way.

regards,
hajime

[Tom Henderson, 2010-05-05 14:01:48]

I have a few comments/questions about this patch.  

1) the implementation of PacketInfoTag seems incomplete; m_ifindex and
m_hoplimit are declared but not used.  The comment says that traffic class is
supported in RFC3542, but the member variable is not there. 

2) is this intended for both IPv4 and IPv6?  IP_PKTINFO has different fields
(ipi_spec_dest and ipi_dest).  What do you think about two separate classes,
Ipv4PacketInfoTag and Ipv6PacketInfoTag, aligned with IP_PKTINFO and
IPV6_PKTINFO, respectively?

3) the behavior is undefined for connected sockets, but you are adding to all
packets; should the adding of this tag be done in the Udp or raw socket code
instead?

4) in actual sockets, this information is off by default unless socket user sets
RCVPKTINFO on the socket.  I would suggest to either support this "off by
default" by adding a socket method SetRcvPacketInfo() such as the
BindToNetDevice() method, or else just document in the header that all Udp and
Raw sockets in ns-3 will receive this ancillary data automatically.

5) What about send direction?  Since in actual sockets, this can be set in
sendmsg, should we add such support here too?  If no interest in supporting in
the send direction, I would suggest just to add a comment in the doxygen for
this tag that support for this tag in the send direction is presently not
enabled but we would accept a patch along those lines in the future.

[Hajime Tazaki, 2010-05-24 15:31:34]

Thanks, Tom.

I have added Patchset 2 according to your comments.

On 2010/05/05 14:01:48, Tom Henderson wrote:
> I have a few comments/questions about this patch.  
> 
> 1) the implementation of PacketInfoTag seems incomplete; m_ifindex and
> m_hoplimit are declared but not used.  The comment says that traffic class is
> supported in RFC3542, but the member variable is not there. 

done.
some variables is defined but not implemented.
i have marked at the comment line.
 
> 2) is this intended for both IPv4 and IPv6?  IP_PKTINFO has different fields
> (ipi_spec_dest and ipi_dest).  What do you think about two separate classes,
> Ipv4PacketInfoTag and Ipv6PacketInfoTag, aligned with IP_PKTINFO and
> IPV6_PKTINFO, respectively?

I took your suggestion, divided two classes instead.

> 3) the behavior is undefined for connected sockets, but you are adding to all
> packets; should the adding of this tag be done in the Udp or raw socket code
> instead?

if we need to add inside Udp/Raw socket, we need to notify
ifindex to via Ipv4Endpoint::ForwardUp(), (v6 as well although i didn't yet).

now connected socket is not handled.

I've added this, but am not sure it's acceptable.


> 4) in actual sockets, this information is off by default unless socket user
sets
> RCVPKTINFO on the socket.  I would suggest to either support this "off by
> default" by adding a socket method SetRcvPacketInfo() such as the
> BindToNetDevice() method, or else just document in the header that all Udp and
> Raw sockets in ns-3 will receive this ancillary data automatically.

I've added Socket::SetRecvPktinfo ().
Then checked in Raw/Udp/Raw6 to deliver this info.

We can add recv_if information into class Packet in order to
notify received interface. But I didn't.

> 5) What about send direction?  Since in actual sockets, this can be set in
> sendmsg, should we add such support here too?  If no interest in supporting in
> the send direction, I would suggest just to add a comment in the doxygen for
> this tag that support for this tag in the send direction is presently not
> enabled but we would accept a patch along those lines in the future.

I've just commented this unimplemented func.

[Tom Henderson, 2010-06-03 05:57:08]

> > 3) the behavior is undefined for connected sockets, but you are adding to
all
> > packets; should the adding of this tag be done in the Udp or raw socket code
> > instead?
> 
> if we need to add inside Udp/Raw socket, we need to notify
> ifindex to via Ipv4Endpoint::ForwardUp(), (v6 as well although i didn't yet).
> 
> now connected socket is not handled.
> 
> I've added this, but am not sure it's acceptable.

Sorry, I couldn't understand what you meant here.  what are you not sure is
acceptable?  and is there a problem with notifying ifindex or with v6?

[Hajime Tazaki, 2010-06-03 21:35:43]

On 2010/06/03 05:57:08, Tom Henderson wrote:
> > > 3) the behavior is undefined for connected sockets, but you are adding to
> all
> > > packets; should the adding of this tag be done in the Udp or raw socket
code
> > > instead?
> > 
> > if we need to add inside Udp/Raw socket, we need to notify
> > ifindex to via Ipv4Endpoint::ForwardUp(), (v6 as well although i didn't
yet).
> > 
> > now connected socket is not handled.
> > 
> > I've added this, but am not sure it's acceptable.
> 
> Sorry, I couldn't understand what you meant here.  what are you not sure is
> acceptable?  and is there a problem with notifying ifindex or with v6?

Sorry for the confusion.
I have changed the argument of Ipv4Endpoint::ForwardUp() as follows.

old)
void ForwardUp (Ptr<Packet> p, Ipv4Address saddr, Ipv4Address daddr, 
     uint16_t  sport);

new)
void ForwardUp (Ptr<Packet> p, Ipv4Address saddr, Ipv4Address daddr, 
     uint16_t sport,  Ptr<NetDevice> device);

#http://codereview.appspot.com/849047/diff/23001/24003

What I'm not sure is this modification.
Is adding Ptr<NetDevice> in the argument fine?

For the IPv6 part, I haven't modified Ipv6Endpoint class yet, since
Ipv6Rawsocket
doesn't use Ipv6Endpoint and there are no udp6/tcp6 implementation so far.

[Tom Henderson, 2010-06-20 19:06:47]

Regarding whether to change the API of Ipv4Endpoint, the main problem that I see
is that you actually will need to add more fields in the future (e.g. TTL) if
you want to implement them correctly in the socketimpl classes.  Or else just
pass a struct with all of these pktinfo fields instead of just "device"
parameter.

It may just be simpler to go back to putting this tag on all packets at the Ipv4
layer, and make a choice:
1) document in socket.h that our implementation appends pktinfo, when set by the
user, to all socket types, rather than just datagram sockets (as in Linux), or
2) append to all and strip off the tag in TCP sockets

I would probably favor 1) for simplicity.  This would also allow you to avoid
forcing a change to SocketImpl() classes to support the setting of this option
correctly.

http://codereview.appspot.com/849047/diff/23001/24001
File src/common/packet-tag-list.h (right):

http://codereview.appspot.com/849047/diff/23001/24001#newcode37
src/common/packet-tag-list.h:37: #define PACKET_TAG_MAX_SIZE 23
I think you will need Mathieu to review this change since it affects all tags.

http://codereview.appspot.com/849047/diff/23001/24005
File src/internet-stack/ipv4-packet-info-tag.h (right):

http://codereview.appspot.com/849047/diff/23001/24005#newcode36
src/internet-stack/ipv4-packet-info-tag.h:36: * See linux manpage ip(7)
Should you also mention that it provides TTL (RECVTTL)?  If you are going to
support TTL, why not also TOS?

http://codereview.appspot.com/849047/diff/23001/24005#newcode47
src/internet-stack/ipv4-packet-info-tag.h:47: // Not implemented yet
These seem to be implemented-- should you delete these comments?  Or do you mean
here that you are not using these fields yet?

http://codereview.appspot.com/849047/diff/23001/24005#newcode50
src/internet-stack/ipv4-packet-info-tag.h:50: void SetSpecDst (Ipv4Address
addr);
When you doxygenate these, please decrypt "SpecDst" for the user, so that user
knows it corresponds to local address.  Or else rename this to
GetLocalAddress/SetLocalAddress and add a comment that it corresponds to
ipi_spec_dst in real socket.

http://codereview.appspot.com/849047/diff/23001/24005#newcode80
src/internet-stack/ipv4-packet-info-tag.h:80: uint8_t m_hoplimit;
suggest to call m_ttl for IPv4.

http://codereview.appspot.com/849047/diff/23001/24006
File src/internet-stack/ipv4-raw-socket-impl.cc (right):

http://codereview.appspot.com/849047/diff/23001/24006#newcode270
src/internet-stack/ipv4-raw-socket-impl.cc:270: // Should check via getsockopt
()..
I do not understand this comment-- are you reminding yourself to implement the
Get() method in class Socket so you don't have to access the underlying member?

http://codereview.appspot.com/849047/diff/23001/24008
File src/internet-stack/ipv6-packet-info-tag.h (right):

http://codereview.appspot.com/849047/diff/23001/24008#newcode47
src/internet-stack/ipv6-packet-info-tag.h:47: // Not implemented yet
Similar comments-- these appear to be implemented methods now.

http://codereview.appspot.com/849047/diff/23001/24018
File src/node/socket.h (right):

http://codereview.appspot.com/849047/diff/23001/24018#newcode558
src/node/socket.h:558: void SetRecvPktInfo (bool flag);
Doxygen doesn't mention that it applies to only Datagram and Raw sockets.  There
is no enforcement; if set on a TCP socket, will not cause an error.  One way to
handle this would be to make it pure virtual and force the sockets to implement
this correctly.

[Hajime Tazaki, 2010-06-21 06:23:40]

Thanks, Tom, Mathieu.

I've updated according to your comments.
Mathieu, I have one inquiry for the modification of  some constant.
 #define PACKET_TAG_MAX_SIZE 23

Will send email you directly.

http://codereview.appspot.com/849047/diff/23001/24005
File src/internet-stack/ipv4-packet-info-tag.h (right):

http://codereview.appspot.com/849047/diff/23001/24005#newcode36
src/internet-stack/ipv4-packet-info-tag.h:36: * See linux manpage ip(7)
On 2010/06/20 19:06:47, Tom Henderson wrote:
> Should you also mention that it provides TTL (RECVTTL)?  If you are going to
> support TTL, why not also TOS?

I've added IP_RECVTTL, IP_RECVTOS as examples of the socket option.
But not used in current code..

http://codereview.appspot.com/849047/diff/23001/24005#newcode47
src/internet-stack/ipv4-packet-info-tag.h:47: // Not implemented yet
On 2010/06/20 19:06:47, Tom Henderson wrote:
> These seem to be implemented-- should you delete these comments?  Or do you
mean
> here that you are not using these fields yet?  

latter. These are implemented but not used as a socket operation in the current
code (never set local address).

I've modified the comments.

http://codereview.appspot.com/849047/diff/23001/24005#newcode50
src/internet-stack/ipv4-packet-info-tag.h:50: void SetSpecDst (Ipv4Address
addr);
On 2010/06/20 19:06:47, Tom Henderson wrote:
> When you doxygenate these, please decrypt "SpecDst" for the user, so that user
> knows it corresponds to local address.  Or else rename this to
> GetLocalAddress/SetLocalAddress and add a comment that it corresponds to
> ipi_spec_dst in real socket.

I changed the name to Get/SetLocalAddress.

http://codereview.appspot.com/849047/diff/23001/24005#newcode80
src/internet-stack/ipv4-packet-info-tag.h:80: uint8_t m_hoplimit;
On 2010/06/20 19:06:47, Tom Henderson wrote:
> suggest to call m_ttl for IPv4.

Hmm, changed.

http://codereview.appspot.com/849047/diff/23001/24006
File src/internet-stack/ipv4-raw-socket-impl.cc (right):

http://codereview.appspot.com/849047/diff/23001/24006#newcode270
src/internet-stack/ipv4-raw-socket-impl.cc:270: // Should check via getsockopt
()..
On 2010/06/20 19:06:47, Tom Henderson wrote:
> I do not understand this comment-- are you reminding yourself to implement the
> Get() method in class Socket so you don't have to access the underlying
member?

I'm sorry to confuse you.
I imagined that it would be nice if we support SetSockOpt/GetSockOpt in ns-3
socket near future. Then we can use GetSockOpt () in here to check this.

http://codereview.appspot.com/849047/diff/23001/24008
File src/internet-stack/ipv6-packet-info-tag.h (right):

http://codereview.appspot.com/849047/diff/23001/24008#newcode47
src/internet-stack/ipv6-packet-info-tag.h:47: // Not implemented yet
On 2010/06/20 19:06:47, Tom Henderson wrote:
> Similar comments-- these appear to be implemented methods now.

Modified as well.

http://codereview.appspot.com/849047/diff/23001/24018
File src/node/socket.h (right):

http://codereview.appspot.com/849047/diff/23001/24018#newcode558
src/node/socket.h:558: void SetRecvPktInfo (bool flag);
On 2010/06/20 19:06:47, Tom Henderson wrote:
> Doxygen doesn't mention that it applies to only Datagram and Raw sockets. 
There
> is no enforcement; if set on a TCP socket, will not cause an error.  One way
to
> handle this would be to make it pure virtual and force the sockets to
implement
> this correctly.  

I've modified the Doxgen comments.

[Hajime Tazaki, 2010-06-28 14:10:44]

On 2010/06/21 06:23:40, Hajime Tazaki wrote:
> Thanks, Tom, Mathieu.
> 
> I've updated according to your comments.
> Mathieu, I have one inquiry for the modification of  some constant.
>  #define PACKET_TAG_MAX_SIZE 23
> 
> Will send email you directly.

According to private discussion, I've changed as follows;
1) delete m_nexthop from the tag (not used for now)
2) shrink m_ifindex to a uint8_t or uint16_t
3) modify the Doxygen comment as follows:

+ * This tag in the send direction is presently not enabled but we
+ * would accept a patch along those lines in the future.   To include
+ * the nexthop in the send direction would increase the size of the
+ * tag beyond 20 bytes, so in that case, we recommend that an
+ * additional tag be used to carry the IPv6 next hop address.

Patchset 4 includes these modifications.

Now maximum size of packet tag is reverted to original value:
#define PACKET_TAG_MAX_SIZE 20

[Tom Henderson, 2010-07-07 14:19:54]

It seems to me that the internal API change will be unavoidable to correctly
implement this.  What about the following? 

class Ipv4L4Protocol:

  virtual enum RxStatus Receive(Ptr<Packet> p,
-                                Ipv4Address const &source,
-                                Ipv4Address const &destination,
+                                const Ipv4Header& header,
                               Ptr<Ipv4Interface> incomingInterface) = 0;

and for class Ipv4EndPoint:

- void ForwardUp (Ptr<Packet> p, Ipv4Address saddr, Ipv4Address daddr, uint16_t
sport);
+ void ForwardUp (Ptr<Packet> p, const Ipv4Header& header, uint16_t sport,
Ptr<Ipv4Interface> incomingInterface);

- void SetRxCallback (Callback<void,Ptr<Packet>, Ipv4Address, Ipv4Address,
uint16_t> callback);
+ void SetRxCallback (Callback<void,Ptr<Packet>, Ipv4Header, uint16_t,
Ptr<Ipv4Interface> > callback);


(I changed the above from Ptr NetDevice to Ipv4Interface since it seems like it
just gets converted back into an Ipv4Interface later in your code-- I don't know
if it causes other issues).

Also, this is minor comment, but what about putting the tag implementation in
the src/node directory (since it seems like the rest of the socket tags are
defined there, and a future user will have to pull in src/node and
src/internet-stack to use that socket feature)?

[Hajime Tazaki, 2010-07-08 15:36:27]

Patch set 5 include the following modification as Tom suggested.

On 2010/07/07 14:19:54, Tom Henderson wrote:
> class Ipv4L4Protocol:
> 
>   virtual enum RxStatus Receive(Ptr<Packet> p,
> -                                Ipv4Address const &source,
> -                                Ipv4Address const &destination,
> +                                const Ipv4Header& header,
>                                Ptr<Ipv4Interface> incomingInterface) = 0;
> 
> and for class Ipv4EndPoint:
> 
> - void ForwardUp (Ptr<Packet> p, Ipv4Address saddr, Ipv4Address daddr,
uint16_t
> sport);
> + void ForwardUp (Ptr<Packet> p, const Ipv4Header& header, uint16_t sport,
> Ptr<Ipv4Interface> incomingInterface);
> 
> - void SetRxCallback (Callback<void,Ptr<Packet>, Ipv4Address, Ipv4Address,
> uint16_t> callback);
> + void SetRxCallback (Callback<void,Ptr<Packet>, Ipv4Header, uint16_t,
> Ptr<Ipv4Interface> > callback);
> 
> 
> (I changed the above from Ptr NetDevice to Ipv4Interface since it seems like
it
> just gets converted back into an Ipv4Interface later in your code-- I don't
know
> if it causes other issues).

I've changed these from NetDevice to Ipv4Interface.


> Also, this is minor comment, but what about putting the tag implementation in
> the src/node directory (since it seems like the rest of the socket tags are
> defined there, and a future user will have to pull in src/node and
> src/internet-stack to use that socket feature)?

I agree. Moved to src/node directory.

[Hajime Tazaki, 2010-07-13 04:30:34]

Merged as changeset 6442.