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

Submitted by:  Daniel Elstner <danielk>
Submitted on:  Tue 20 Sep 2016 01:47:03 PM UTC  
 
Category: NoneSeverity: 3 - Normal
Item Group: Change RequestStatus: Invalid
Privacy: PublicAssigned to: None
Open/Closed: ClosedPlanned Release: None
lwIP version: git head

(Jump to the original submission 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>
Project Member
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>
Project 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>
Project Member
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>
Project Member
Wed 21 Sep 2016 10:27:30 AM UTC, comment #1:

I like his idea. Comments from anybody else?

Dirk Ziegelmeier <dziegel>
Project Member
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>

 

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by goldsimon (Posted a comment)
  • -unavailable- added by dziegel (Posted a comment)
  • -unavailable- added by danielk (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 2 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Sat 01 Oct 2016 07:14:07 PM UTCdziegelStatusNone=>Invalid
      Open/ClosedOpen=>Closed

    Back to the top


    Powered by Savane 3.1-cleanup1