Bug 36 - Support IPv4 broadcast (255.255.255.255)
Support IPv4 broadcast (255.255.255.255)
Status: RESOLVED FIXED
Product: ns-3
Classification: Unclassified
Component: network
pre-release
PC Linux
: P1 blocker
Assigned To: ns-bugs
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2007-06-12 11:39 EDT by Gustavo J. A. M. Carneiro
Modified: 2007-08-02 10:02 EDT (History)
0 users

See Also:


Attachments
the patch (8.28 KB, patch)
2007-06-12 11:39 EDT, Gustavo J. A. M. Carneiro
Details | Diff
new patch (8.43 KB, patch)
2007-06-13 14:34 EDT, Gustavo J. A. M. Carneiro
Details | Diff
patch v3 (8.51 KB, patch)
2007-06-13 14:50 EDT, Gustavo J. A. M. Carneiro
Details | Diff
patch, v4 (9.17 KB, patch)
2007-06-14 09:15 EDT, Gustavo J. A. M. Carneiro
Details | Diff
support broadcast IP (255.255.255.255) (8.56 KB, patch)
2007-06-15 09:31 EDT, Gustavo J. A. M. Carneiro
Details | Diff
implicit network routes (1.37 KB, patch)
2007-06-15 09:32 EDT, Gustavo J. A. M. Carneiro
Details | Diff
support broadcast IP (255.255.255.255) (7.77 KB, patch)
2007-07-13 12:43 EDT, Gustavo J. A. M. Carneiro
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gustavo J. A. M. Carneiro 2007-06-12 11:39:33 EDT
The attached patch:
 - Adds support for IPv4 broadcast address (255.255.255.255)
 - Adds support for broadcast mediums in ARP
 - Adds some more debugging to the ARP module
 - Automatically adds the network of an interface to the routing table, when calling Ipv4L3Protocol::AddIpv4Interface

Now which of these is useful to merge with ns-3-dev?
Comment 1 Gustavo J. A. M. Carneiro 2007-06-12 11:39:49 EDT
Created attachment 17 [details]
the patch
Comment 2 Tom Henderson 2007-06-13 02:26:07 EDT
I'm fine with adding each of these.  Can you also add some test and/or client code that exercises the broadcast?
Comment 3 Mathieu Lacage 2007-06-13 02:47:57 EDT
I object to the last item: it really looks very much like a routing policy and I think that we should avoid adding policy to the core model. A quick look at a linux box will also show you that what you want to do is not done by the /sbin/ifconfig tool. That, to me, is a pretty strong hint that we should not do it in Ipv4L3Protocol::AddIpv4Interface.

about the patch itself:
 - please, fix the XXX in Ipv4L3Protocol::Send
 - please, do not add extra code in headers unless you have a compelling reason to. I spotted Ipv4Address::IsBroadcast
 - I object to the addition of operator & to Ipv4Address. Please, replace it with a member method unless you really plan to add support for all arithmetic operators which would make little sense.
Comment 4 Gustavo J. A. M. Carneiro 2007-06-13 08:57:31 EDT
(In reply to comment #3)
> I object to the last item: it really looks very much like a routing policy and
> I think that we should avoid adding policy to the core model. A quick look at a
> linux box will also show you that what you want to do is not done by the
> /sbin/ifconfig tool. That, to me, is a pretty strong hint that we should not do
> it in Ipv4L3Protocol::AddIpv4Interface.

Raally? That's strange, what kind of box do you have?  It was precisely the fact that ifconfig _does_ it that gave me the idea to implement it in ns-3.  For instance, if I do:

# ifconfig eth0:1 192.170.1.1 netmask 255.255.0.0

Then I get an automatic routing table entry:

gjc@spectrum:~$ route -n
Kernel IP routing table
Destination     Gateway         Genmask         Flags Metric Ref    Use Iface
[...]
192.170.0.0     0.0.0.0         255.255.0.0     U     0      0        0 eth0
[...]

I think this is a good idea because it is normally what you want to do, and if it isn't it can be undone (assuming there's an API in ns-3 to remove routing table entries).

> 
> about the patch itself:
>  - please, fix the XXX in Ipv4L3Protocol::Send

 	  // XXX report packet as dropped.

That XXX was not added by me; i'm not sure what it means or how to fix it.  Report to whom?

>  - please, do not add extra code in headers unless you have a compelling reason
> to. I spotted Ipv4Address::IsBroadcast

 OK.

>  - I object to the addition of operator & to Ipv4Address. Please, replace it
> with a member method unless you really plan to add support for all arithmetic
> operators which would make little sense.

Personally I find it intuitive to write an expression like "address & mask", to combine an address and mask.  But it's debatable, a method name is OK too I guess.

Comment 5 Mathieu Lacage 2007-06-13 09:11:16 EDT
(In reply to comment #4)
> (In reply to comment #3)
> > I object to the last item: it really looks very much like a routing policy and
> > I think that we should avoid adding policy to the core model. A quick look at a
> > linux box will also show you that what you want to do is not done by the
> > /sbin/ifconfig tool. That, to me, is a pretty strong hint that we should not do
> > it in Ipv4L3Protocol::AddIpv4Interface.
> 
> Raally? That's strange, what kind of box do you have?  It was precisely the
> fact that ifconfig _does_ it that gave me the idea to implement it in ns-3. 
> For instance, if I do:
> 
> # ifconfig eth0:1 192.170.1.1 netmask 255.255.0.0
> 
> Then I get an automatic routing table entry:
> 
> gjc@spectrum:~$ route -n
> Kernel IP routing table
> Destination     Gateway         Genmask         Flags Metric Ref    Use Iface
> [...]
> 192.170.0.0     0.0.0.0         255.255.0.0     U     0      0        0 eth0
> [...]
> 
> I think this is a good idea because it is normally what you want to do, and if
> it isn't it can be undone (assuming there's an API in ns-3 to remove routing
> table entries).

Maybe the place to put this policy would be the "ifconfig" code tom wants to get someday. I really am not too comfortable about adding this to this function whose sole purpose (for now) is to create an entry in the ipv4 interface list.

> > 
> > about the patch itself:
> >  - please, fix the XXX in Ipv4L3Protocol::Send
> 
>           // XXX report packet as dropped.
> 
> That XXX was not added by me; i'm not sure what it means or how to fix it. 
> Report to whom?

I misread the patch then: I thought you had added it yourself. I suspect that this is a comment by me then. It might be worth to report bugs for each XXX we spot in the source to keep track of them.


thanks
Comment 6 Gustavo J. A. M. Carneiro 2007-06-13 12:12:06 EDT
(In reply to comment #2)
> Can you also add some test and/or client code that exercises the broadcast?

I can only add a limited broadcast example (or unit test, whichever you prefer), as ns-3 has no shared medium network interface yet.  I could also make a sample broadcast network interface, if there is none available already.  Any preference/suggestions?
Comment 7 Tom Henderson 2007-06-13 14:27:36 EDT
(In reply to comment #6)
> (In reply to comment #2)
> > Can you also add some test and/or client code that exercises the broadcast?
> 
> I can only add a limited broadcast example (or unit test, whichever you
> prefer), as ns-3 has no shared medium network interface yet.  I could also make
> a sample broadcast network interface, if there is none available already.  Any
> preference/suggestions?
> 

I don't have a strong preference, other than just guessing that if you are patching the simulator for this capability, you must be trying to use it somehow.  We don't yet have a policy for trying to get code coverage on unit tests, validation, and example programs, but Mathieu is contributing the bulk of what we have so far in his unit tests. 

Emmanuelle is working on Ethernet interface and maybe we can wait for that for a sample/example program.

Comment 8 Gustavo J. A. M. Carneiro 2007-06-13 14:34:37 EDT
Created attachment 18 [details]
new patch
Comment 9 Gustavo J. A. M. Carneiro 2007-06-13 14:50:57 EDT
Created attachment 19 [details]
patch v3

Sorry for the spam.  I think I found one XXX comment where mathieu was right :P It was due to code copy-pasted from a very old ns3 version, and so didn't have proper tracing in the broadcast send path.
Comment 10 Tom Henderson 2007-06-13 15:36:08 EDT
(In reply to comment #9)
> Created an attachment (id=19) [edit]
> patch v3
> 
> Sorry for the spam.  I think I found one XXX comment where mathieu was right :P
> It was due to code copy-pasted from a very old ns3 version, and so didn't have
> proper tracing in the broadcast send path.
> 

Gustavo, I may prefer to split out the broadcast example into another one, or maybe we just wait for an Ethernet example for where it makes more sense.  So maybe hold off on simple-p2p.cc part.

Regarding the other parts of the patch:

- I agree with Mathieu that AddIpv4Interface is not the right place to add the network route, and I confess to missing that detail when I first reviewed it.  However, it does seem to me that SetUp() and SetAddress() are logical places to insert this route.   I see that the SetUp() piece of the patch has been now removed.

I guess it depends on what people think the semantics are of SetUp().  What does it mean to be up?  It means that the interface is usable if it has an address, which means that it has to have a proper route associated with it (that is one way to think about it).  When you do a SetUp() in practice (ifconfig up), it adds this network route.  So I don't see why we should have different semantics.

So I would like to suggest that SetUp() has the following logic:
if (has IP address) {
  AddNetworkRoute()
]

SetDown() {
  if (is up && has IP address) {
    DelNetworkRoute()
  }
}

SetAddress() {
  if (is up) {
     AddNetworkRoute();
  }
}


DelAddress() { // this function doesn't yet exist
  if (is up) {
    DelNetworkRoute();  
  }
}
Comment 11 Gustavo J. A. M. Carneiro 2007-06-14 09:15:06 EDT
Created attachment 20 [details]
patch, v4

Changes: adds default network route in Ipv4L3Protocol::SetUp if interface address and mask have been set.

Can I commit at least some of these changes?  I realize I am mixing at least a couple of different things in one patch, but I'll be sure to commit to mercurial separately.
Comment 12 Mathieu Lacage 2007-06-14 12:58:56 EDT
in ArpIpv4Interface::SendTo, you do not need to call QI yourself: it was called already at the top of the method

Comment 13 Tom Henderson 2007-06-15 00:27:04 EDT
+
+  // If interface address and network mask have been set, add a
+  // default route to the network of the interface (like e.g. ifconfig
+  // does on a Linux box)
+  if ((interface->GetAddress ()) != (Ipv4Address ())
+      and (interface->GetNetworkMask ()) != (Ipv4Mask ()))
+    {
+      AddNetworkRouteTo (interface->GetAddress ().CombineMask (interface->GetNetworkMask ()),
+                         interface->GetNetworkMask (), i);
+    }
 }

A few nits:
- please add similar logic to SetDown() if we are putting the above in SetUp()  (although it is not as straightforward how to find the route index to remove in that method)
- it is not a "default" route; just a network route
- s/and/&&
Comment 14 Mathieu Lacage 2007-06-15 04:23:31 EDT
> - please add similar logic to SetDown() if we are putting the above in SetUp() 
> (although it is not as straightforward how to find the route index to remove in
> that method)

It is trivial to store the route index in the Ipv4L3Protocol class as a member variable.
Comment 15 Gustavo J. A. M. Carneiro 2007-06-15 09:09:56 EDT
(In reply to comment #13)
> - please add similar logic to SetDown() if we are putting the above in SetUp() 
> (although it is not as straightforward how to find the route index to remove in that method)

Actually, what Linux ifconfig does, and I think it makes sense, is to remove all routes that use an interface when that interface is going down.

> - it is not a "default" route; just a network route
> - s/and/&&

OK, but IMHO 'and' is much more readable than '&&'.
Comment 16 Gustavo J. A. M. Carneiro 2007-06-15 09:31:29 EDT
Created attachment 21 [details]
support broadcast IP (255.255.255.255)
Comment 17 Gustavo J. A. M. Carneiro 2007-06-15 09:32:38 EDT
Created attachment 22 [details]
implicit network routes

I have split the previous patch in two..
Comment 18 Gustavo J. A. M. Carneiro 2007-07-13 12:25:14 EDT
I _really_ need the "support broadcast IP" patch to go in soon.  Otherwise the OLSR code I'm working on won't work:

Olsr node 10.1.1.1: SendQueuedMessages
OLSR node 10.1.1.1 sending a OLSR packet
no route to host. drop.

This happens because OLSR sends UDP datagrams to the broadcast IP address but NS-3 currently is not supporting that.
Comment 19 Gustavo J. A. M. Carneiro 2007-07-13 12:43:32 EDT
Created attachment 27 [details]
support broadcast IP (255.255.255.255)

(same patch updated to ns-3-dev tip)
Comment 20 Gustavo J. A. M. Carneiro 2007-07-13 12:47:35 EDT
Comment on attachment 22 [details]
implicit network routes

Moved to bug 49
Comment 21 Tom Henderson 2007-07-13 17:33:04 EDT
I support pushing this in now.
Comment 22 Gustavo J. A. M. Carneiro 2007-08-02 10:02:09 EDT
merged