[Ns-developers] 802.11s code review

[Pavel Boyko]

  Mathieu, Gustavo,

  thank you very much for reviewing our code. All your comments seem 
reasonable, some of them are already fixed. The others are added to our bug 
tracker. 

  Just now we are making  major refactoring of the architecture and code 
(don't panic, most of the code remains the same), so I kindly ask you to pause 
review for 1-2 weeks. By the end of this week I will send you the module 
architecture description (can I do this on nsnam wiki?) for review. I hope it 
will answer your questions about bridging, ports, peer management, etc.

  Best regards,
  Pavel Boyko, 
  researcher at IITP RAS 

On Monday 16 March 2009 16:53:24 Mathieu Lacage wrote:
> hi,
>
> 2 weeks ago, at the ns-3 workshop
> (http://www.nsnam.org/wiki/index.php/Wns3-2009), Kirill Andreev
> presented a 802.11s implementation for ns-3
> (http://forge.wenos.ru/hgprojects/ns3dev/).
>
> There is really a _lot_ of new code here so, I have done a relatively
> high-level first review and did not look at very many details. I have
> explicitely CCed gustavo because I think he could helpfully comment on
> the code and my own comments below (more specifically, comment 3):
>
> 1) I found routing-table-test.cc which, I think, is awesome. It would
> make sense to make it a test just like src/devices/wifi/wifi-test.cc
> which is run in an automated way with "./waf check".
>
> 2) There are a bunch of coding style issues:
> http://www.nsnam.org/codingstyle.html summarizes some (but not all)
> constraints: the goal is really to try to make all of the ns-3 code look
> the same and make it impossible to figure out who wrote the code by just
> looking at its indentation and layout. A few quick examples:
>
> uint32_t
> L2RoutingNetDevice::GetIfIndex() const
>   {
>     NS_LOG_FUNCTION_NOARGS ();
>     return m_ifIndex;
>   }
>
> should be left-aligned:
>
> uint32_t
> L2RoutingNetDevice::GetIfIndex() const
> {
>   NS_LOG_FUNCTION_NOARGS ();
>   return m_ifIndex;
> }
>
> this if statement:
>
>   if (!Mac48Address::IsMatchingType (routingPort->GetAddress ()))
>     NS_FATAL_ERROR ("Device does not support eui 48 addresses: cannot be
> added to bridge.");
>
> should use braces, even for a single-line statement:
>
>   if (!Mac48Address::IsMatchingType (routingPort->GetAddress ()))
>     {
>       NS_FATAL_ERROR ("Device does not support eui 48 addresses: cannot
> be added to bridge.");
>     }
>
> This inline method declaration (WifiBeaconTimingElement):
>
>   uint16_t TIMESTAMP_TO_U16(Time x)
>   {
>     return ((uint16_t)((x.GetMicroSeconds() >> 8)&0xffff));
>   };
>
> should not be inline. i.e., it should be declared here and the
> implementation moved to the .cc file to maximize the readability of the
> header. (Of course, its name should follow the MixedCamel convention
> rather than ALL_UPPER_CASE).
>
> enum dot11sPathSelectionProtocol is declared in
> src/devices/wifi/mesh-configuration-element.h which is a bit
> un-intuitive: it would be nice if the name of the enum had at least some
> sort of relationship as to which header it is defined in. The same goes
> for DestinationAddressUnit defined in
> mesh-wifi-preq-information-element.h: maybe making it an inner class of
> the outer WifiPreqInformationElement class would help.
>
> I know some people will think I am way too anal about this but, well,
> that's fine: it's just my job to make everyone miserable :)
>
> 3) src/devices/l2-routing/l2-routing-main/l2-routing-net-device.h looks
> like a more generic version of src/devices/bridge-net-device.cc which
> delegates l2 forwarding decisions to another class. I wonder if it would
> be possible to, instead, delegate the forwarding/routing decisions to a
> subclass through a virtual method from a base class:
>
> class L2RoutingNetDevice : public NetDevice
> {
>   // check device capabilities, register receive callback,
>   // manage associated BridgeChannel, call DoAddPort to notify subclass
>   void AddPort (...);
>   uint32_t GetNPorts (void) const;
>
>   // Note: it might make sense to add EnablePort and Disable port, just
> like Hwmp::EnablePort and Hwmp::DisablePort. If so, AddPort should
> return a port index and Enable and Disable port should take that index.
> private:
>   // called by ::AddPort
>   virtual void DoAddPort (Ptr<NetDevice> dev) = 0;
>   virtual uint32_t DoGetNPorts (void) = 0;
>   // called by ::ReceiveFromDevice and ::Send
>   virtual void DoRoute (Ptr<NetDevice> from, Mac48Address src,
> Mac48Address dst, Ptr<Packet> p, uint8_t protocol) = 0;
> };
>
> class HwmpNetDevice : public L2RoutingNetDevice
> {
> private:
>   // implement all these methods.
>   virtual void DoAddPort (Ptr<NetDevice> dev);
>   virtual uint32_t DoGetNPorts (void);
>   virtual void DoRoute (Ptr<NetDevice> from, Mac48Address src,
> Mac48Address dst, Ptr<Packet> p, uint8_t protocol);
>
>   // keep track of the port list.
>   std::vector<Ptr<NetDevice> > m_ports;
> };
>
> class LearningBridgeNetDevice : L2RoutingNetDevice
> {
> private:
>   // implement all these methods.
>   virtual void DoAddPort (Ptr<NetDevice> dev);
>   virtual uint32_t DoGetNPorts (void);
>   virtual void DoRoute (Ptr<NetDevice> from, Mac48Address src,
> Mac48Address dst, Ptr<Packet> p, uint8_t protocol);
>
>   // keep track of the port list.
>   std::vector<Ptr<NetDevice> > m_ports;
> };
>
> 4) I remember one of the questions during the wns3 workshop was related
> to tags and how to remove them. I can see that, in hwmp.cc, they are
> removed with RemoveAllTags which could be problematic. I don't think we
> have a better way to handle this for now but the plan is to solve this
> problem as part of the ipv4-rework over the next 2 weeks (so, as soon as
> I am done with this review, I will work on fixing this issue to solve
> your problem here :)
>
> 5) Why are MeshWifiMac::SetPrepReceivedCallback,
> SetPerrReceivedCallback, SetPeerStatusCallback, etc. virtual ? Do you
> have subclasses of MeshWifiMac ?
>
> 6) What happens in MeshWifiMac if one of the packets to forward does not
> have an HwmpTag ? Is this tag mandatory to make MeshWifiMac work ? If
> so, maybe it should be renamed MeshWifiMacTag.
>
> 7) What is a WifiPeerManager ? What is the purpose of
> WifiPeerManager::AttachPorts ? I don't see this method being called from
> anywhere so, I wonder what it's used for.
>
> Mathieu