Bugzilla – Bug 1826
make ns3::Ipv6Interface independent from ns3::IPv6L3protocol
Last modified: 2014-01-13 05:07:51 EST
LinuxStackHelper of DCE module uses Ipv6AddressHelper to configure IPv6 addresses of Linux kernel with the support of ns3::Ipv6Interface. current Ipv6Interface::DoSetup() and AddAddress() calls ND operation (DAD, cache initialization) which DCE doesn't use. The following patch tries to add a virtual method (GetProtocol) so that other child than IPv6L3Protocol (i.e., ns3::IPv6Linux of DCE) can handle IPv6 address list. http://code.nsnam.org/ns-3-dce/file/de7335a01743/utils/ipv6-linux-stack.patch
Hi, I'm ok with the patch, generally speaking. However, I would like to evaluate it a bit better. Can you please provide a patch against ns-3-dev ? The patch you linked is: 1) including the python bindings (harder to read), 2) incomplete :P virtual Ptr<IpL4Protocol> GetProtocol (int protocolNumber) const = 0; - There's no concrete implementation (I guess there must be one), #include "ns3/ip-l4-protocol.h" - can't find this file (or its implementation) 3) naming, scope and limitations: if Ipv6L3Protocol can return a pointer to Icmpv6L4Protocol (i.e., an L4 protocol), then it should be able to return a pointer to UdpL4Protocol and TcpL4Protocol as well, isn't it ? Anyway, I'm always open to changes leading to a better design. T.
Created attachment 1754 [details] patch for ns-3-dev (changeset:8c347165bb56) This patch is against for ns-3-dev the following changeset. changeset: 8c347165bb56
Thanks Tommaso. (In reply to Tommaso Pecorella from comment #1) > Hi, > > I'm ok with the patch, generally speaking. > > However, I would like to evaluate it a bit better. Can you please provide a > patch against ns-3-dev ? > > The patch you linked is: > 1) including the python bindings (harder to read), I attached the patch without python bindings. # but without the bindings, the build will fail if enabled. > 2) incomplete :P > virtual Ptr<IpL4Protocol> GetProtocol (int protocolNumber) const = 0; > - There's no concrete implementation (I guess there must be one), The class is an abstract class, inherited by Ipv6L3Protocol (so far). Ipv6L3Protocol has an implementation of the method, so I think there is. > #include "ns3/ip-l4-protocol.h" > - can't find this file (or its implementation) it is at src/internet/model/ip-l4-protocol.h and installed at build/include/ns3/. I believe it's completed. > 3) naming, scope and limitations: > if Ipv6L3Protocol can return a pointer to Icmpv6L4Protocol (i.e., an L4 > protocol), then it should be able to return a pointer to UdpL4Protocol and > TcpL4Protocol as well, isn't it ? Yes, it can be done without this patch.
Hi Hajime, sorry, I was tired and I didn't read all the patch carefully. Two more focused comments: 1) please make GetProtocol virtual also in Ipv6L3Protocol and Ipv4L3Protocol. It isn't strictly necessary, but it's nice. 2) in src/internet/model/ipv6.h. It shouldn't be necessary to include ns3/ip-l4-protocol.h. A simple class IpL4Protocol is enough. Once fixed these 2 comments, +1 Cheers, T.
(In reply to Tommaso Pecorella from comment #4) > Hi Hajime, > > sorry, I was tired and I didn't read all the patch carefully. never mind. thx. > Two more focused comments: > 1) please make GetProtocol virtual also in Ipv6L3Protocol and > Ipv4L3Protocol. It isn't strictly necessary, but it's nice. i will fix these. > 2) in src/internet/model/ipv6.h. > > It shouldn't be necessary to include ns3/ip-l4-protocol.h. > > A simple > class IpL4Protocol is enough. hmm, I followed the way at ipv6.h as ipv4.h does: ipv4.h, which the GetProtocol method is already a virtual method, includes ip-l4-protocol.h. I kept ipv6.h include ip-l4-protocol.h if you're fine.
(In reply to Hajime Tazaki from comment #5) > (In reply to Tommaso Pecorella from comment #4) > > 2) in src/internet/model/ipv6.h. > > > > It shouldn't be necessary to include ns3/ip-l4-protocol.h. > > > > A simple > > class IpL4Protocol is enough. > > hmm, I followed the way at ipv6.h as ipv4.h does: ipv4.h, which the > GetProtocol method is already a virtual method, includes ip-l4-protocol.h. I > kept ipv6.h include ip-l4-protocol.h if you're fine. I guess it's a matter of taste. I always try to minimize the headers inclusions in headers, to avoid circular "strong" dependencies. But again, it's a matter of personal taste I guess. If v4 does the same, then go ahead. Cheers, T.
> > > > hmm, I followed the way at ipv6.h as ipv4.h does: ipv4.h, which the > > GetProtocol method is already a virtual method, includes ip-l4-protocol.h. I > > kept ipv6.h include ip-l4-protocol.h if you're fine. > > I guess it's a matter of taste. I always try to minimize the headers > inclusions in headers, to avoid circular "strong" dependencies. > But again, it's a matter of personal taste I guess. If v4 does the same, > then go ahead. I'd like to instead vote for de-headering ipv4.h, which simplifies dependencies (also python bindings dependencies), and do the same for ipv6.h. I'll attach a suggested patch, which requires full bindings rescan if applied.
Created attachment 1755 [details] remove ipv4-l4-protocol.h from ipv4.h
I agree, +1. Btw, we should find an automatic way to cleanup header inclusions. It would be a huge patch, but it could dramatically simplify things. Cheers, T.
(In reply to Tommaso Pecorella from comment #9) > I agree, +1. > > Btw, we should find an automatic way to cleanup header inclusions. It would > be a huge patch, but it could dramatically simplify things. > > Cheers, > > T. We can incrementally improve the codebase, and it can be an introductory project. Daniel Camara already worked on this in 2012, in bug 1237, which ended up being closed for another issue, but I've revived his work in bug 1832 (which I will add to suggested projects page). I will commit the one I already did attached to the bug report, and leave further work on this to bug 1832.
Thanks Tommaso and Tom. I've fixed (de-headerd) ths files and push it. binding rescan was also done. http://code.nsnam.org/ns-3-dev/rev/9594a4df3bde