Yes, this implementation does not pass the transport header.  Is there a reason that you believe it is necessary?  Or do you think it needs to be more explicitly documented as such?

I think it needs to be closer to the real stack.
Came to find the problem when I was designing an application that need to read transport level header in RouteOutput.
To resolve this I had to manually attach UdpHeader in UdpSocketImpl::DoSendTo before it called RouteOutput.

Can you provide a patch to evaluate ?

Mind that I guess that if you change this, you'll have also to change the TCP behaviour, otherwise RouteOutput will have different informations to deal with between the two stacks, and this could cause issues (not totally sure, but better to be consistent).

Tommaso

Created attachment 1311 [details]
add udp header in packet before RouteOutput call

Created attachment 1312 [details]
this code verifies if the patch works with udp

Created attachment 1313 [details]
This code checks if the RouteOutput adds to the packet the tcp header before calling RouteOutput

Analysis of files that call RouteOutput

Files to be analyzed:

a) ./model/udp-socket-impl.cc: route = ipv4-> GetRoutingProtocol () -> RouteOutput (p, header, oif, errno_);
b) ./model/tcp-l4-protocol.cc: route = ipv4-> GetRoutingProtocol () -> RouteOutput (packet, header, oif, errno_);
c) ./model/socket-tcp-base.cc: route = ipv4-> GetRoutingProtocol () -> RouteOutput (<Packet> Ptr (), header, oif, errno_);
d) ./model/ipv4-raw-socket-impl.cc: route = ipv4-> GetRoutingProtocol () -> RouteOutput (p, header, oif, errno_);
e) ./model/ipv4-l3-protocol.cc: m_routingProtocol-newRoute => RouteOutput (packet, ipHeader, oif, errno_);
f) ./model/ipv4-list-routing.cc: route = (* i). second-> RouteOutput (p, header, oif, sockerr);
g) ./model/icmpv4-l4-protocol.cc: route = ipv4-> GetRoutingProtocol () -> RouteOutput (packet, header, oif, errno_);
h) ./model/ipv6-list-routing.cc: route = (* i). second-> RouteOutput (p, header, oif, sockerr);
i) ./model/ipv6-raw-socket-impl.cc: ipv6-route => GetRoutingProtocol () -> RouteOutput (p, hdr, oif, err);
j) ./model/ipv6-l3-protocol.cc: m_routingProtocol-newRoute => RouteOutput (packet, hdr, oif, err);
k) ./model/icmpv6-l4-protocol.cc: ipv6-route => GetRoutingProtocol () -> RouteOutput (packet, header, oif, err);
l) ./model/ipv6-extension.cc: = Ptr <Ipv6Route> rtentry ipv6rp-> RouteOutput (p, ipv6header, 0, err);

Analysis:

a) udp-socket-impl.cc
Modified. OK

b) tcp-l4-protocol.cc
The header is added to the packet before call RouteOutput. OK

c) tcp-socket-base.cc
It is passed an empty package. Used to set the destination address.
It is used only in Connection() to set destination address. (?)

d) ipv4-raw-socket-impl.cc
It is a Raw Socket OK

e) ipv4-l3-protocol.cc
Technically, the packet is supplied to the function from the layer 4, so the header should be filled.
Considering the above sentence as true, it would not need modification.

f) ipv4-list-routing.cc
Not necessary to change because RouteOutput is invoked by the method RouteOutput.

g)-l4-ICMPv4 protocol.cc
Technically, there is no need to change because the icmp protocol version 4 operates below the transport layer.

The letter: h, i, j, k and l, which are implementations for IPv6, follows the conclusions of the files prefixed with ipv4.

I disagree with the proposed patch.

Reason: the udp header will be added twice. Once before RouteOutput and once in the UdpL4Protocol::Send (called just a few lines after).

RouteOutput in this case is called and is used to find out which ip address have to be used (in case we do have more than one).
If we really want to have a "complete" packet with the UDP header down there, then we do need to strip the udp header right after. 
Sending a copy of the packet with its header attached to RouteOutput would probably break stuff (NixRouting ?), so I'd avoid it.

The only way to fix this is to add the header and then remove it.
OR
To move the above code down into UdpL4Protocol.
OR
To move the udp header finalization up into the socket.

Personally, I don't like too much any of the above. The first is an unnecessary code slowdown, the second is a even more erroneous, as the code does a check for a ERROR_OPNOTSUPP. As is "hey, man, you asked to send a packet to a broadcast address on a socket that can NOT send broadcast. Got it?". The third... well, it should be responsibility of UdpL4Proto to add the header !

To be honest there's a reason if this bug was not yet fixed... and it's the fact that fixing it is kinda a mess.

If we DO want to fix it, I fear that the only solution is to
1) add the udp header
2) call routOutput
3) strip the header 

The only cleaner solution is to:
1) define a new function UdpL4Protocol::SendWithUdpHeaderAlreadyFilledIn(...) <- not necessarily THIS name.
2) use that one *just* for this case.

All the above solutions are ugly as hell™, but maybe the last one is the less ugly.

Cheers,

T.

I think the main difficulty in adding the UdpHeader as this patch suggests is due to the transport checksum since the IP header may change as a result of calling RouteOutput().  However, there is no access to the UdpHeader in an ns-3 Packet after it has been added unless the header is popped (i.e. removed), modified, and pushed again.

Current Linux kernel uses ip_route_output_flow(), passing in struct flowi.  If the information in struct flowi is sufficient for the expected use cases, then a solution may be to create an ns-3 equivalent of struct flowi and either
1) add it to the RouteOutput () method as an explicit argument
2) add the flowi information to the Packet as a Tag, which can be read by routing protocols that want to see the extra information

Could we first agree that the information in struct flowi is sufficient?

                struct flowi fl = { .oif = sk->sk_bound_dev_if,
                             .nl_u = { .ip4_u =
                                       { .daddr = daddr,
                                         .saddr = inet->saddr,
                                         .tos = RT_CONN_FLAGS(sk) } },
                            .proto = sk->sk_protocol,
                            .uli_u = { .ports =
                                       { .sport = inet->sport,
                                         .dport = inet->dport } } };

then, if so, agree on how to pass it across the RouteOutput() API?

Brilliant idea. /bow

I'd add it as an extra argument, purely because tags are nice but people usually do not realize that there IS a tag. On the other hand adding an extra argument means we'll have to  change all the existing routing protocols.

Hard choice, really.

Maybe a tag is easier to implement. BUT... this open a "new" point: how to document in an effective way how tags are used and what infos are there. I'll send a mail to ns-3-dev mailing list about it.

About what info we need on the tag (or the extra argument), I'd say that it should be more than enough to mimic the Linux behavior. Moreover we can always add data to a tag (or struct) without breaking stuff.

T.

Created attachment 1318 [details]
fixes - add udp header in packet before RouteOutput call

Tommaso,

Thanks for checking and identifying the problems in the "patch".
I started studying the code of ns-3 just a month ago, so I'm still trying to understand the architecture. After this discussion I learned many new things. The solution using TAG can be more flexible and interesting, mainly in the case of TCP. 

I am sending a new patch with some fixes.
I did a small refactoring of the Send() method. Now, it is not necessary remove the header. I think that for the UDP is interesting because it does not modify the structure of the simulator and solves the problem of the bug. In the TCP implementation there is calls to RouteOutput() with transport header in the packet.

I'll try to implement the solution using TAG to learn more about ns-3.

[]s

Campiolo

Created attachment 1319 [details]
fixes - add udp header in packet before RouteOutput call

Problems with diff - new file.

Thanks Jan, but I think Tom's idea is still the best one.

We don't need really to change the code to patch a flawed design, we need to improve it. Sometimes the best way to improve is to drastically change things, and that's what Tom is suggesting.

Btw, I'd pass the structure as a new argument :)

Cheers,

Tommaso

The revised patch is not addressing the issue that I raised.  I'm suggesting to add this struct to ipv4-routing-header.h:

struct FlowI
{
  Ptr<NetDevice> oif;
  Ipv4Address daddr;
  Ipv4Address saddr;
  uint8_t tos;
  uint8_t proto;
  uint16_t sport;
  uint16_t dport;
}     

and make this explicit change to the RouteOutput method:

-  virtual Ptr<Ipv4Route> RouteOutput (Ptr<Packet> p, const Ipv4Header &header, Ptr<NetDevice> oif, Socket::SocketErrno &sockerr) = 0;
+  virtual Ptr<Ipv4Route> RouteOutput (Ptr<Packet> p, const FlowI &flowi, Socket::SocketErrno &sockerr) = 0;

and update UDP and TCP accordingly.  

Note we could also pass this in as a Tag but this API change should be invisible to most users and if it breaks anyone's build, the fix is clear, so I would prefer to be explicit with this API change.

(In reply to comment #14)
> The revised patch is not addressing the issue that I raised.  I'm suggesting to
> add this struct to ipv4-routing-header.h:

I meant to say ipv4-routing-protocol.h

Hi Tom

I agree that your solution is better.

-  virtual Ptr<Ipv4Route> RouteOutput (Ptr<Packet> p, const Ipv4Header &header,
Ptr<NetDevice> oif, Socket::SocketErrno &sockerr) = 0;
+  virtual Ptr<Ipv4Route> RouteOutput (Ptr<Packet> p, const FlowI &flowi,
Socket::SocketErrno &sockerr) = 0;


I would like suggest a hierarchical structure class to provide a solution for both ipv4 and ipv6.


class FlowI {
  uint16_t m_sport;
  uint16_t m_dport;

  uint8_t m_proto;

  uint32_t m_iifindex; // ingress iface as in Linux
  uint32_t m_oifindex;
}

class Ipv4FlowI: public FlowI {
  uint8_t m_tos;
  Ipv4Address m_saddr;
  Ipv4Address m_daddr; 
}

class Ipv6FlowI: public FlowI {
  Ipv6Address m_saddr;
  Ipv6Address m_daddr;

  uint32_t m_flowlabel; // flowlabel as in Linux
}

I do not know what is better for iifindex (iif) and iofindex (iof): uint32_t or Ptr<NetDevice> ?

what do you think?

thanks

Campiolo

Since it's an index, I'd say uint32_t. All the interfaces are referenced by an index, so...

About the hierarchical structure, I don't have anything against it, but keep in mind that the UDP-TCP-IPv6 patch will basically store addresses in TCP and UDP as simple Addresses. You might want to take a look at the incoming (hopefully) patch at http://codereview.appspot.com/5417048/

Cheers,

T.

Thanks Tommaso,

I modified the structure to:

class FlowI {
  uint16_t m_sport;
  uint16_t m_dport;

  uint8_t m_proto;

  Address m_saddr;
  Address m_daddr; 

  Ptr<NetDevice> m_iif; // ingress iface as in Linux
  Ptr<NetDevice> m_oif;
}

class Ipv4FlowI: public FlowI {
  uint8_t m_tos;
}

class Ipv6FlowI: public FlowI {
  uint32_t m_flowlabel; // flowlabel as in Linux
}

Due to the current implementations of routing protocols, I will use the Ptr <NetDevice>.  

I'll refactor the code of ns-3 with this structure.

(In reply to comment #16)
> Hi Tom
> 
> I agree that your solution is better.
> 
> -  virtual Ptr<Ipv4Route> RouteOutput (Ptr<Packet> p, const Ipv4Header &header,
> Ptr<NetDevice> oif, Socket::SocketErrno &sockerr) = 0;
> +  virtual Ptr<Ipv4Route> RouteOutput (Ptr<Packet> p, const FlowI &flowi,
> Socket::SocketErrno &sockerr) = 0;
> 
> 
> I would like suggest a hierarchical structure class to provide a solution for
> both ipv4 and ipv6.
> 

I would recommend to avoid this hierarchy; you are not really gaining anything from it, and users could fall victim to C++ slicing if they use the base class.

> 
> class FlowI {
>   uint16_t m_sport;
>   uint16_t m_dport;
> 
>   uint8_t m_proto;
> 
>   uint32_t m_iifindex; // ingress iface as in Linux
>   uint32_t m_oifindex;
> }
> 
> class Ipv4FlowI: public FlowI {
>   uint8_t m_tos;
>   Ipv4Address m_saddr;
>   Ipv4Address m_daddr; 
> }
> 
> class Ipv6FlowI: public FlowI {
>   Ipv6Address m_saddr;
>   Ipv6Address m_daddr;
> 
>   uint32_t m_flowlabel; // flowlabel as in Linux

Unless you plan to correctly populate flowlabel, I would delete it for now.  Same for m_tos (we can't yet setsockopt to set DSCP on these sockets) and m_iif (will it always be set to zero now?).  This is a struct and can be extended in the future as needed.

Will this work for now?

struct Ipv4FlowI {
  uint16_t m_sport;
  uint16_t m_dport;
  Ipv4Address m_saddr;
  Ipv4Address m_daddr;
  Ptr<NetDevice> m_oif;
  uint8_t m_proto;
}

struct Ipv6FlowI {
  uint16_t m_sport;
  uint16_t m_dport;
  Ipv6Address m_saddr;
  Ipv6Address m_daddr;
  Ptr<NetDevice> m_oif;
  uint8_t m_proto;
}

> 
> I do not know what is better for iifindex (iif) and iofindex (iof): uint32_t or
> Ptr<NetDevice> ?

Not a big deal either way, but Ptr<NetDevice> is more in keeping with the previous API, so I would go with that.

Hi

I used the suggestion proposed by Tommaso: Address.
I used the simplification proposed by Tom too, but partly, I'm still using the input interface and OO structure.
To avoid the use of class FlowI, I specified the class as "abstract".
I submitted the code using the Rietveld:
http://codereview.appspot.com/5661044/

The file "packet-socket.cc" was not modified, but the rietveld sent it.

So I await comments. thanks.

Campiolo

I disagree about the ToS. We do have a specific request on the ns-users mailing list about it, so why not.

Concerning the class proliferation, I kinda disagree with Tom. This structure is not really meant to be "exposed" to the users, beside the ones playing with routing and IP layers. Sockets should create them and use to commpunicate with the lower layers, but that's it. Sockets are IP version-agnostic, so my point was to keep it so.

Now, if we look at it carefully, in Linux we do create sockets filling up data structures that will force the socket to be v4 or v6. Of course if we mix things bad stuff will happen, but that's another point. Anyway, I wouldn't cry too much if we have 2 separate IpvxFlowI without a base class, but it could be a problem for the Socket to hold it in a "generic" way. On the other hand, is this really needed ? Dunno, I have to see the code.

I'll check it in rietveld and I'll put my comments there.

Cheers,

T.