[Ns-developers] 802.11s code review

[Gustavo Carneiro]

2009/3/16 Mathieu Lacage <mathieu.lacage at sophia.inria.fr>

> 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;
> };


OK, here's my first superficial comment.

First of all, I was not present at the NS conference, so I am really out of
context, looking at this for the first time.

I really do not understand why you need an inner learning bridge inside a
.11s mesh point netdevice.  I do not understand what the concept of "port"
inside a netdevice means.  Last time I saw a .11s draft (a couple of years
ago), I saw nothing of this sort; all I saw was a tree formed by mesh portal
announcements, and routing was either "send to the mesh portal" or "discover
route using AODV-like or OLSR-like protocol", nothing at all like learning
bridge was used for intra-mesh forwarding.  So without understanding this
"port" concept, I really cannot comment on this.  Perhaps the 802.11s
standard has evolved, but I wouldn't know because I am not following...

One major comment I would have is, stop calling classes L2This and L2That.
Layer 2 is too broad a term to be used in this context.  Maybe Ieee802Xxx,
or MeshXxx.

Another major comment on style is that the "design patterns" guys usually
say it is bad to design too much inheritance-oriented.  Here I see
LearningBridgeNetDevice --> L2RoutingNetDevice --> NetDevice.  The design
patterns gurus often say you should use aggregation instead of inheritance
when possible.  So ask yourselves, could the same problem be solved via
aggregation instead of inheritance without noticeably complicating things?


>
> 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
>
>


-- 
Gustavo J. A. M. Carneiro
INESC Porto, Telecommunications and Multimedia Unit
"The universe is always one step beyond logic." -- Frank Herbert