Agreed. Proposal for better names ?

I've been calling them interface index (ifIndex) and net device index (ndIndex) in the tutorial and the difference is explained and emphasized. 

If we want to maintain alignment with Linux naming conventions, we should keep the netdevice parameter to be named ifIndex and call the Ipv4Interface parameter something else or else refactor to not require a separate Ipv4Interface index.

raising to P1 (we should resolve before the release)

Marked as P2/LATER per prior agreement.  Will revisit after 3.1 release.

Created attachment 161 [details]
change ifIndex to interfaceIndex, at IPv4 layer

attached documentation patch should mostly resolve it.  Release mgr, OK to commit?

(leaves NetDevice ifIndex as is, changes use of "ifIndex" at the Ipv4 layer to instead refer to "interfaceIndex", in most places, and clarified in a few doxygen headers about the distinction between NetDevice and Ipv4Interface indices)

reopen for consideration for 3.1 release

I'm okay with committing this, but the basic problem of two meanings still exists.

When I stare at the string "ifIndex" I read "interface index".  Now when I look at the string "interfaceIndex" I still read "interface index."

When I look at IP code, I see methods like GetIfIndexForDestination() with variables scattered around looking like "interfaceIndex."

When I look at device code, I see methods like SetIfIndex() and I see an m_ifIndex member variable.  I read "set interface index" and "interface index member variable."  When I look at Node::AddDevice() though, I find,

Node::AddDevice (Ptr<NetDevice> device)
{
  uint32_t index = m_devices.size ();
  ...
  device->SetIfIndex(index);
  ...
}

All of the methods still refer to IfIndex everywhere; and it's the method names I usually pay more attention to.

By the same logic applied recently to another bug (218) we may want to consider changing the names of the methods in Ipv4 that refer to an IfIndex to something else before we ship.

If we change these methods to be called something like BlahIpv4IndexMumble instead of BlahIfIndexMumble but leave the ifIndex found in the net devices alone we may be able to put this to rest.

This would have a relatively large scope but it is a simple rename operation and as such would be relatively low risk.

I think that a major difference with bug 218 is that this change is most likely not going to have any real impact on most scripts. Instead, it will have impact on ipv4 routing protocols, etc. which are, hopefully, more rare than user scripts.

(In reply to comment #8)
> I'm okay with committing this, but the basic problem of two meanings still
> exists.
> 
> When I stare at the string "ifIndex" I read "interface index".  Now when I look
> at the string "interfaceIndex" I still read "interface index."

Yes, this is a bit unfortunate.  In Linux, there is an ifIndex (struct net_device), but there is no integer equivalent to interfaceIndex.  Instead, interfaces (in_addrs) are named by device name strings such as "eth0" and "eth0:1".  

> 
> When I look at IP code, I see methods like GetIfIndexForDestination() with
> variables scattered around looking like "interfaceIndex."

Yes, this is what I mean that the patch "mostly" fixes this-- I did not change any method names.  I think it would be cleaner to change those method names, but I can live without it I guess (with documentation around it).

> 
> When I look at device code, I see methods like SetIfIndex() and I see an
> m_ifIndex member variable.  I read "set interface index" and "interface index
> member variable."  When I look at Node::AddDevice() though, I find,
> 
> Node::AddDevice (Ptr<NetDevice> device)
> {
>   uint32_t index = m_devices.size ();
>   ...
>   device->SetIfIndex(index);
>   ...
> }
> 
> All of the methods still refer to IfIndex everywhere; and it's the method names
> I usually pay more attention to.

good point. 

another alternative would be to use names like intIndex and devIndex at the respective levels and avoid altogether "ifIndex", including fixing method names, but that is more intrusive still.

Real world scenario; just happened to me.  Student writes code to bring down all interfaces.  I have to spend considerable time debugging his code.  Guess what was the patch to fix it:

-  for(uint32_t i = 0; i < GetNode()->GetNDevices(); i++)
-   ipv4->SetDown(i);
+  for(uint32_t i = 0; i < ipv4->GetNInterfaces (); i++)
+    {
+      ipv4->SetDown (i);

sliding to ns-3.5

fixed:  changeset d99061f1167c