buglwIP - A Lightweight TCP/IP stack - Bugs: bug #49139, IGMP "All Systems" vs...

 
 

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

bug #49139: IGMP "All Systems" vs MLD "All Nodes" inconsistency

Submitter:  Daniel Elstner <danielk>
Submitted:  Tue 20 Sep 2016 01:47:03 PM UTC
   
 
Category:  None Severity:  3 - Normal
Item Group:  Change Request Status:  Invalid
Privacy:  Public Assigned to:  None
Open/Closed:  Closed Planned Release:  None
lwIP version:  git head

Jump to the original submission

Sat 01 Oct 2016 07:14:07 PM UTC, comment #7: 

Proposal not implemented, but added documentation

Dirk Ziegelmeier <dziegel>
Group administrator
Fri 30 Sep 2016 06:51:20 PM UTC, comment #6: 

About the possibility of a filter that does add/remove addresses: At least for Ethernet hardware, I really can't think of such a scenario. The main problem is that the IP address does not map to a unique MAC address. So you can't just e.g. scan a table of MAC addresses to find the one to remove. One would need to maintain extra bookkeeping information to make something like that work.

I'd argue that for status_callback() it would be far more useful to have information on what exactly has changed, but it does not provide any details either.

OK, I didn't realize that IGMP could be stopped independently of the netif. Although even so, as long as the IGMP start/stop status can be queried the notification callback without parameters would be enough to deal with it.

In any case I could easily live with the API as it right now, since the one major concern I had with it (#49125) has been swiftly addressed. The handling of the all-systems/all-modes groups should probably be documented though. Also the fact that the all-systems group eats up a memp slot with IGMP.

Daniel Elstner <danielk>
Fri 30 Sep 2016 11:06:30 AM UTC, comment #5: 

I'd say it makes sense to keep the address: even if you don't need it, there's a possibility someone implements a filter that really does add/remove addresses. And as we already have the address, I see no need to not pass it on.

As to the actual subject of this bug: there's a fundamental difference in MLD and IGMP: if you have IPv6 enabled, you
always need to handle the all-nodes group. In contrast to this, IGMP is rather optional and can be enabled/disabled in products. As such, it makes sens to pass the address on to the filter because it can be removed once 'igmp_stop()' is called.

However, I do see that it could be strange for people writing IPv6 code (or netif drivers) that the all-nodes group has to be enabled without the netif being explicitly told...

Simon Goldschmidt <goldsimon>
Group administrator
Thu 29 Sep 2016 07:54:58 PM UTC, comment #4: 

Ah, OK, didn't get that.

Maybe it is useful for some people to know the exact IP and whether it was added or removed to implement a driver efficiently.

But in general, I'd also say simply rebuilding the filter list by iterating over all groups is the most simple and robust way to implement this.

Are there people reading this that can tell us how they to it? Is the info about exact IP and change type useful?

Personally, I would not use it, I would always rebuild the whole list.

Dirk Ziegelmeier <dziegel>
Group administrator
Thu 29 Sep 2016 07:29:36 PM UTC, comment #3: 

Note that I was proposing to get rid of these callbacks parameters altogether. So this would not be a problem.

Daniel Elstner <danielk>
Thu 29 Sep 2016 07:26:57 PM UTC, comment #2: 

Regarding the second part (merging the callbacks):

Problem is IGMP code only uses ip4_addr_t internally and MLD6 code uses ip6_addr_t. But a common callback needs ip_addr_t. Consequence: Creation of ip_addr_t on stack just for the callback, and the first thing the called function does is "if IP_IS_V6(ip)" checking. So this change does not implement an improvement.

Dirk Ziegelmeier <dziegel>
Group administrator
Wed 21 Sep 2016 10:27:30 AM UTC, comment #1: 

I like his idea. Comments from anybody else?

Dirk Ziegelmeier <dziegel>
Group administrator
Tue 20 Sep 2016 01:47:03 PM UTC, original submission:  

As my project now makes use of both IGMP and MLD, I discovered the following oddity:

The lwIP IGMP implementation puts the "All Systems" group in its global multicast group list, but does some special casing in the code to avoid actually sending out reports for it.

The MLD implementation, on the other hand, does not put the "All Nodes" group in its group list, and therefore does not need the special casing dance either. However, this also means that netif implementations need to explicitly set up the MAC filter to allow incoming "All Nodes" multicast traffic, in addition to handling the MAC filter callback.

Should something be done about this? Personally I lean towards making IGMP behave like MLD already does. It would avoid wasting a memory pool slot for a constant entry, and would allow the code to be simplified by getting rid of the special casing. On the other hand, making that kind of change could silently break existing code.

However, until recently the MAC filter callbacks were rather awkward to implement anyway. I have a feeling that most netif implementations probably don't bother with the filter callbacks and simply let all incoming multicast packets pass. :)

In that light, it might make sense to merge the two filter callbacks for IGMP and MLD into a single mac_filter_update() callback, without the "action" and "group" parameters. Handlers would then be required to iterate the group lists by themselves. But as I've outlined in #49125, doing so is the natural/easiest way to go about this anyway.

An API change like that would break things at compile time, thereby avoiding the problem of failing silently.

Daniel Elstner <danielk>

 

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

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by dziegel (Posted a comment)
  • -email is unavailable- added by danielk (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 2 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2016-10-01 dziegel StatusNone Invalid
        Open/ClosedOpen Closed

    Back to the top

    Powered by Savane 3.13-3230.
    Corresponding source code