buglwIP - A Lightweight TCP/IP stack - Bugs: bug #20503, IGMP Improvement

 
 

You are not allowed to post comments on this tracker with your current authentication level.

bug #20503: IGMP Improvement

Submitter:  Bill Florac <bflorac>
Submitted:  Tue 17 Jul 2007 07:38:04 AM UTC
   
 
Category:  IPv4 Severity:  3 - Normal
Item Group:  Change Request Status:  Fixed
Privacy:  Public Assigned to:  fbernon
Open/Closed:  Closed Planned Release:  None
lwIP version:  None

Jump to the original submission

Sat 15 Sep 2007 03:22:14 PM UTC, comment #15: 

IP_MULTICAST_IF option is integrated and check in. For some documentation, you can see these links.

http://www.linux.com/base/ldp/howto/Multicast-HOWTO-6.html
http://publib.boulder.ibm.com/infocenter/iseries/v5r3/index.jsp?topic=/apis/gsocko.htm

Since it was the last feature to add, I close this task, but feel free to reopen it if I miss something.

Frédéric Bernon <fbernon>
Group Member
Sun 09 Sep 2007 08:48:54 PM UTC, comment #14: 

Ok, features of the comment #13 TODO list are check in( NETIF_FLAG_IGMP, igmp_stop, igmp reports when link "up").

It stay to implement IP_MULTICAST_IF.

Frédéric Bernon <fbernon>
Group Member
Tue 04 Sep 2007 03:35:28 PM UTC, comment #13: 

Replace mem_malloc call by memp_malloc, and use a new MEMP_NUM_IGMP_GROUP option (see opt.h to define the value). It will avoid potential fragmentation problems, use a counter to know how many times a group is used on an netif, and free it when all applications leave it. MEMP_NUM_IGMP_GROUP got 8 as default value (and init.c got a sanity check if LWIP_IGMP!=0).

TODO :

- IP_MULTICAST_IF

- a flag to define if a netif is IGMP-able or not, in the same idea of NETIF_FLAG_BROADCAST. Perhaps NETIF_FLAG_ETHARP could be enought to avoid to add a new NETIF_FLAG_IGMP flag? Thoughts?

- add a igmp_stop() call inside netif_remove ?

- resend igmp reports when link "up" ?

Finally, the idea of a "per netif" list of groups is abandoned.


Frédéric Bernon <fbernon>
Group Member
Mon 03 Sep 2007 02:54:46 PM UTC, comment #12: 

Some changes are check in: Initialize igmp_mac_filter to NULL in netif_add (this field should be set in the netif's "init" function). Use the "imr_interface" field (for socket layer) and/or the "interface" field (for netconn layer), for join/leave operations. The igmp_join/leavegroup first parameter change from a netif to an ipaddr. This field could be a netif's ipaddr, or "any" (same meaning than ip_addr_isany).


Frédéric Bernon <fbernon>
Group Member
Wed 29 Aug 2007 09:14:16 PM UTC, comment #11: 

Ok, the igmp initialization changes are check in (see CHANGELOG).

Frédéric Bernon <fbernon>
Group Member
Wed 29 Aug 2007 10:17:55 AM UTC, comment #10: 

A part of changes request by Bill are "postponed" to post-1.3.0. I think to :

- IP_MULTICAST_IF (comment #6, but it's easy to integrate)
- igmp group list per netif (more things to check)

But there is some little changes that I really to do to have a good igmp integration for 1.3.0 (some igmp_init should be done each time we "add" a netif, and use memp of allocations). This is something I will do this week.


Frédéric Bernon <fbernon>
Group Member
Wed 29 Aug 2007 09:12:29 AM UTC, comment #9: 

Is this done now?

Kieran Mansley <kieranm>
Group Member
Thu 09 Aug 2007 06:40:34 PM UTC, comment #8: 

Since the patch doesn't change anything for users with LWIP_IGMP=0, then I have check in.

Another tip on this improvement, IGMP statistics are not handle like the others. I will centralize them in "stats" module...

Frédéric Bernon <fbernon>
Group Member
Thu 09 Aug 2007 05:48:16 PM UTC, comment #7: 

Finally, since if LWIP_IGMP=0, the packet is dropped in ip_input, the code have to be conditionned by the same:

Index: src/core/udp.c
===================================================================
RCS file: /sources/lwip/lwip/src/core/udp.c,v
retrieving revision 1.93
diff -w -b -u -0 -p -r1.93 udp.c
--- src/core/udp.c 3 Jul 2007 20:28:36 -0000 1.93
+++ src/core/udp.c 9 Aug 2007 17:42:52 -0000
@@ -156,0 +157,3 @@ udp_input(struct pbuf p, struct netif
+#if LWIP_IGMP
+         ip_addr_ismulticast(&(iphdr->dest)) ||
+#endif /* LWIP_IGMP */

No Objects?

Frédéric Bernon <fbernon>
Group Member
Thu 09 Aug 2007 05:12:37 PM UTC, comment #6: 

Ok, the minor part of the patch is included. Bill, if you can check and confirm this part is good for you?

There is also this patch which is need to be able to receive a multicast datagram if we are not binded to "any". No objects? Note I don't propose it with LWIP_IGMP, since you can decide to receive a multicast packet on the non-igmp network, without wanting the igmp module.

Index: src/core/udp.c
===================================================================
RCS file: /sources/lwip/lwip/src/core/udp.c,v
retrieving revision 1.93
diff -w -b -u -0 -p -r1.93 udp.c
--- src/core/udp.c 3 Jul 2007 20:28:36 -0000 1.93
+++ src/core/udp.c 9 Aug 2007 17:00:17 -0000
@@ -156,0 +157 @@ udp_input(struct pbuf p, struct netif
+         ip_addr_ismulticast(&(iphdr->dest)) ||


Last, IP_MULTICAST_IF option should be implemented (see http://docs.hp.com/en/B2355-90136/ch05s05.html for a sample). It's declared in sockets.h for source code compatibility, and return an error "not implemented" at run time. The big lines of the changes are: if LWIP_IGMP=1, we should add a field "default_xcast_if" in "struct udp_pcb", and patch ip_route() in ip.c to use it if it's defined != NULL (and, of course, implement get/set sockopt).





Frédéric Bernon <fbernon>
Group Member
Tue 17 Jul 2007 09:51:22 AM UTC, comment #5: 


>Does that also mean we have to send GroupLeaving to both addresses?


No, most of time, the problem is on the ip_dst_addr of igmp "query" message. This is something I see on the field with few manufacturers. Interoperability is not "great" with IGMP. Some others problems are :

- behavior of IGMP switch/router when a group membership is "empty" on the network (no one join the group). So flood the traffic (it's default in RFC), some blocks the traffic (it's more better practice, most of modern switchs/routers do that, some propose the option).

- behavior for the IGMP Querier election is not always weel supported (most of time, due to different timings.

- behavior for the IGMP Leave message

- behavior for the interoperability with IGMPv2 on IGMPv3-capable switchs/routers.


>OK, I'm fine with no options. Perhaps I can leave it to someone else with more experience with memp change it to memp?


I will do..

Frédéric Bernon <fbernon>
Group Member
Tue 17 Jul 2007 09:41:52 AM UTC, comment #4: 


>>Removed igmp_mac_filter() to allrouters group. [I don't think this needed in V2]


>Not ok, since, like we have talk on the mailing list, some switchs/routers doesn't use 224.0.0.1, but 224.0.0.2. I suppose it's due to a problem in their RFC interpretation, but on the field, we have to accept both to receive query messages. My experience with industrial switchs/routers using IGMP is this is a "component" not always very well implemented, where the field sometimes a better "rule" than the RFC (always in draft for memory). Since let receive 224.0.0.2 packets is not a big problem, I think we have to let receive


If we are going to keep 224.0.0.2 then we need to add a group2 = igmp_lookup_group(netif, &allrouters) as it is missing. Otherwise, it will be tossed out. Does that also mean we have to send GroupLeaving to both addresses?


>>>Use LWIP_IGMP_FREE_GROUPS to enable this feature


>If there is a problem with memory (and there is one in IGMP), I'm in flavor to fix it without any options. Of course, it depend of the way to do it. I would like to use memp rather than mem to allocate igmp_group structs.


OK, I'm fine with no options. Perhaps I can leave it to someone else with more experience with memp change it to memp?


Bill





Bill Florac <bflorac>
Tue 17 Jul 2007 09:26:40 AM UTC, comment #3: 

Additional comments:

bold*Removed igmp_init() from tcpip_thread() function and force users to call after netif_add() *bold
[Perhaps there is a better solution but netif_add() needs to be called to initialize the netif values before igmp can use them. This is not guaranteed in tcpip_thread.]



bold*Reworked igmp.c to allow for different group list per interface. *bold
[This required adding a igmp_group_list to the netif structure (netif.h). This is the suggested implementation according the RFC. This allows separate groups to be joined on each interface. Normally would join only on one interface and let the network manage getting it to other subnets.]


bold*Reworked igmp.c so the debug reporting was formatted like other lwip files.*bold
[I hope don't offended anyone.]


bold*netif_add() now initializes igmp_mac_filter to NULL.*bold
[The igmp code checks to see if this assigned before calling. A random value would cause problems if it was used before we got to point it to the correct function]


bold*igmp_joingroup() now checks to make sure you only add multicast addresses to the list.*bold
[Just to make sure we don't create invalid groups and send out invalid igmp reports.]


bold*Added group_state member DEAD_MEMBER to separate groups that are being initialized to those that are stale.*bold
[This allow correct building of hash key as we may don't delete records. The initial code did not delete group records after leaving a group.  The routine to set the hash value for the MAC rebuilds by reading the entire list. I needed a way to determine the ones that should be removed from the hash.]


bold*Removed igmp_mac_filter() to allrouters group.*bold
[According to RFC-2236 we intentionally don't want our host to subscribe to this. If, at sometime in the future, we add router support, we can adjust as needed.]


bold*In igmp_input(), changed igmp_lookup_group() to igmp_lookfor_group.*bold
[This is a BUG! If it's not for us, we don't want it to make a group for us. List need to be managed by the application. This BUG also exists in ip.c]


bold*igmp_leavegroup() can now delete memory used by the group. Use LWIP_IGMP_FREE_GROUPS to enable this feature.*bold
[The application I'm working with may add and remove groups often. Over time, I cold run out of memory....]

Bill

(file #13372)

Bill Florac <bflorac>
Tue 17 Jul 2007 09:09:38 AM UTC, comment #2: 

Some of the changes can be done for 1.3.0, since there are bugs fix, or coding styles. I will check in them in a first step. For the others, they need more tests. I hope to find time for them in the end of the week, or next week... I take this bug (I can test it, of course, if there is no objection)...

>NOTES: This only implements "host" function


ok

>Interface should be up before joining groups


ok. About one of the last change for link up/down, I think we also have to resend reports when the link change to "up"

>Removed igmp_init() from tcpip_thread() function and force users to call after netif_add() [Perhas there is a better solution]


I will look your change, since some init are need to be done in "the good order" (but which one ? :) )

>Reworked igmp.c to allow for different group list per interface. [This required adding a igmp_group_list to the netif structure (netif.h).]


ok (but need to study that)

>Reworked igmp.c so debug reporting was formatted like other lwip files. [I hope don't offended anyone.]


ok (will be check in for 1.3.0)

>Reworked igmp.c so all function names all begin with "igmp".


ok (will be check in for 1.3.0)

>netif_add() now initializes igmp_mac_filter to NULL [as well as the new field igmp_group_list.]


ok (just have to see if it's done at the "good" time (before low_level_init). The netif change is not attached. The last you post on the mailing list is good?

>igmp_joingroup() now checks to make sure you only add multicast addresses to the list.


ok (will be check in for 1.3.0) since it was a bug

>Added group_state member DEAD_MEMBER to separate groups that are being initialized to those that are stale. [This allow correct building of hash key as we may not deleting records]


ok (but need to study that)

>Removed igmp_mac_filter() to allrouters group. [I don't think this needed in V2]


Not ok, since, like we have talk on the mailing list, some switchs/routers doesn't use 224.0.0.1, but 224.0.0.2. I suppose it's due to a problem in their RFC interpretation, but on the field, we have to accept both to receive query messages. My experience with industrial switchs/routers using IGMP is this is a "component" not always very well implemented, where the field sometimes a better "rule" than the RFC (always in draft for memory). Since let receive 224.0.0.2 packets is not a big problem, I think we have to let receive


>In igmp_input(), changed igmp_lookup_group() to igmp_lookfor_group. [If it's not for us, we don't want it!]


ok, this is a bug for me (to fix in 1.3.0)

>igmp_leavegroup() can now delete memory used by the group.

Use LWIP_IGMP_FREE_GROUPS to enable this feature

If there is a problem with memory (and there is one in IGMP), I'm in flavor to fix it without any options. Of course, it depend of the way to do it. I would like to use memp rather than mem to allocate igmp_group structs.

Frédéric Bernon <fbernon>
Group Member
Tue 17 Jul 2007 07:58:48 AM UTC, comment #1: 

Here is my list of changes. Most of these were listed in previous emails.
 
- NOTES: This only implements "host" function
         Interface should be up before joining groups
- Removed igmp_init() from tcpip_thread() function and force users to call
  after netif_add() [Perhas there is a better solution]
- Reworked igmp.c to allow for different group list per interface.
  [This required adding a igmp_group_list to the netif structure (netif.h).]
- Reworked igmp.c so debug reporting was formatted like other lwip files.
  [I hope don't offended anyone.]
- Reworked igmp.c so all function names all begin with "igmp".
- netif_add() now initializes igmp_mac_filter to NULL
  [as well as the new field igmp_group_list.]
- igmp_joingroup() now checks to make sure you only add multicast addresses
  to the list.
- Added group_state member DEAD_MEMBER to separate groups that are being
  initialized to those that are stale.  [This allow correct building of hash
  key as we may not deleting records]
- Removed igmp_mac_filter() to allrouters group.
  [I don't think this needed in V2]
- In igmp_input(), changed igmp_lookup_group() to igmp_lookfor_group.
  [If it's not for us, we don't want it!]
- igmp_leavegroup() can now delete memory used by the group.
  Use LWIP_IGMP_FREE_GROUPS to enable this feature

Frédéric Bernon <fbernon>
Group Member
Tue 17 Jul 2007 07:38:04 AM UTC, original submission:  

Attached is a improved IGMP implmention.  Changes are listed in the igmp.c file. Fixes some bug, add some additionally function.

Bill Florac

Bill Florac <bflorac>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attached Files
file #13372:  netif.zip added by bflorac (979B - application/x-zip-compressed - Missing modified netif patches)
file #13370:  ip.c.patch added by bflorac (559B - text/plain - Additonal bug fix in ip.c)
file #13366:  igmp.c.patch added by bflorac (18KiB - text/plain - patch and source files)
file #13367:  igmp.c added by bflorac (23KiB - application/octet-stream - patch and source files)
file #13368:  igmp.h.patch added by bflorac (1KiB - text/plain - patch and source files)
file #13369:  igmp.h added by bflorac (5KiB - application/octet-stream - patch and source files)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by kieranm (Posted a comment)
  • -email is unavailable- added by fbernon (Posted a comment)
  • -email is unavailable- added by bflorac (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 12 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2007-09-15 fbernon StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2007-09-09 fbernon Planned Release 1.3.0
    2007-09-04 fbernon StatusNone Ready For Test
    2007-07-26 thedoctor Carbon-Copy- Added thedoctor
    2007-07-17 bflorac Attached File- Added netif.zip, #13372
    2007-07-17 fbernon Assigned toNone fbernon
    2007-07-17 bflorac Attached File- Added ip.c.patch, #13370
    2007-07-17 bflorac Attached File- Added igmp.c.patch, #13366
        Attached File- Added igmp.c, #13367
        Attached File- Added igmp.h.patch, #13368
        Attached File- Added igmp.h, #13369

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code