RE: RE : RE : [lwip-devel] igmp implementation

[Bill Florac]

Attached is current source (I hope it's ok etiquette) for your review.
 
Also see comments below:
 
Bill Florac
 

________________________________

From: address@hidden on behalf of Frédéric BERNON
Sent: Wed 7/11/2007 1:17 PM
To: lwip-devel
Subject: RE : RE : [lwip-devel] igmp implementation


Here are the changes I have made:

        - Removed igmp_init() from tcpip_thread() function and move it to 
tcpip_init(). 
          [It should work either way but it seemed a better fit.] 
        >Not so good, since - currently - the netif have to be add before the 
igmp_init
        + All igmp_init() needs is a netif. I thought the correct order was 
netif_init(), tcpip_init(), netif_add(). I call the first two before starting 
the RTOS and then call netif_add() in task. Is this not correct.
         
        - 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 
         
        - Reworked igmp.c so debug reporting was formatted like other lwip 
files.
          [I hope don't offended anyone.] 
        >No problem for me, since all informations ar kept ;)
         
        - Reworked igmp.c so all function names all begin with "igmp". 
        > I suppose it's only lookfor_group & lookup_group?
        + Yep
         
        - netif_add() now initializes igmp_mac_filter to NULL 
          [as well as the new field igmp_group_list.] 
        > To NULL??? I don't understand 
        + Pointer in netif structure is not initialized and may be random 
data...
         
        - igmp_joingroup() now checks to make sure you only add multicast 
addresses to the list. 
        >  Need to see the code to understand
        + Simply calls ip_addr_ismulticast()
         
        - 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 are not deleting 
records] 
        >  Need to see the code to understand
        + I have include my hardware specific ethernetif.c file. This may help..
         
        - Removed igmp_mac_filter() to allrouters group. 
          [I don't think this needed in V2] 
        >Wrong it's need for interoperability with some switchs/routers, IGMPv2 
is always in "draft", and field tests give some constraints. More, you could 
want to use lwIP for do your own "router"
        + If we wnat to make IwIP a router we need to add more igmp code... 
RFC2236 states that Leave Group is sent to the all-routers group as other host 
have no need for this. Implying that host don't need to listen to this address. 
That said, it's not much traffic but if we add it, should we not also add it to 
the group list?
        

        Now, in reviewing the code, I noticed that igmp_input() uses 
lookup_group() not lookfor_group(). The former will add the group to our list. 
Is this really a desired effect?  If it not in one of our groups why would we 
not toss it out? 

        >You're right, it should be lookfor...

         Some points need you send your files to understand your changes...

        Bill Florac


________________________________

        From: address@hidden on behalf of Bill Florac
        Sent: Wed 7/11/2007 4:08 AM
        To: lwip-devel
        Subject: RE: RE : [lwip-devel] igmp implementation
        
        
        Comments inserted below
         
        Bill Florac

________________________________

        From: address@hidden on behalf of Frédéric BERNON
        Sent: Wed 7/11/2007 2:58 AM
        To: lwip-devel
        Subject: RE : [lwip-devel] igmp implementation
        
        
        >>I decide that the IGMP group list should be interface specific so I 
move the group list to the netif structure.
        >Yes, it should be, Steve Reynolds code didn't, but Mace Gael did (but 
there was several problems). The current design more based on Steve code.
         
        >>I then modified igmp.c to deal on the netif structure passed.
        >Ok.
         
        >>I also change igmp_init() to igmp_start() as it only work if the 
interface is up and running.  I them added the igmp_start to the netif_set_up() 
function (removing it from tcpip.c).  
        >I'm not agree: since the "up" can be call each time te DHCP bind the 
address, you don't have to reset it at this time. I think the job is to do on 
netif_add (and it will be better for adding/removing netif at any time).
         
        OK but I still think "igmp_start" may a better name. I think we have to 
wait for interface to be up (DHCP and given it an address) to start igmp as the 
process may/should send out IGMP messages and they should have the proper IP 
address. I'm not an expert on IGMP (yet) but we need to subscribe to the local 
router that we are listening to multicast messages so it will forward them to 
us. I'm not sure if this applies to the IGMP messages but their addresses get 
added in the igmp_init() function.
         
         
        >>The fuction type igmp_mac_filter() does not need the action or group 
parameters as it rebuild the hash table based on the list in the netif 
sturcture past.  Is there any reason to keep these?
        >Yes: each join/leave group will be slower with this solution: some MAC 
have different filter feature, with not a hash table, but just a table, they 
don't have to process like this. Since it's MAC dependant, lwIP should provide 
all parameters, and let the port designer do the best solution for his hardware.
         
        OK.
         
         
        >>NETIF_FLAG_IGMP
        >Seems not necessary. You can already use NETIF_FLAG_ETHARP to know if 
the interface is Ethernet/ARP. If the idea is to define that such or such 
interface has IGMP capability, so yes. But in this case, you should better 
implement the 3 IGMP level described in RFC.Be careful, netif's "flags" field 
is a u8_t.
         
        OK. I does not due anything yet. I was just emulating other stacks I 
have seen.
         
        
        
        
         
        >But you don't talk about the "big" problem (not so big, but): the 
ip_input patch: even with a "per netif" group list, the current ip_input code 
only select one netif to do the processing. If two netif "join" the same group, 
what do you propose to handle that? I think it could be more intrusive (in the 
source code and in execution time). This point if the most important to my 
point of view. A better IGMP implementation will also support SO_REUSEADDR/PORT 
(see https://savannah.nongnu.org/task/?6995 
<https://savannah.nongnu.org/task/?6995> ), since two sockets/services could 
need to receive the same information. Last, a socket level filter will be the 
last step (because, when a socket do a "leavegroup, there can be already 
packets in its recvmbox from this group, and they should be dropped, and not 
given to the application).
         
        Well, was looking for a place to pass down the stack the group list. 
Since the call to igmp_mac_filter() aleady had the netif structure it seems to 
make since. I need this during the igmp_leavegroup so I keep the hash bit if to 
address have the same key. This avoids having to create an separate table 
(consuming more RAM) for up to 64 reference counters in the igmp_mac_filter() 
function.
         
        Interesting question, of which I don't have an answer regarding the two 
sockets/services issue... Should the fact that there may be multiple interface 
be transparent to the application or would the application have to "register" 
it's desire with each interface?
         
        >There is also some tips to change in igmp.c : it should better use 
memp than mem, the remove of "group" struct is not handled, etc...
         
        >Since I done the integration, I have planned to improve all that in 
some time. But I think that "close" the release is more important, and I 
thought do that after. Problem, we don't know when will ended the release. So 
feel free to continue if you can't wait. Perhaps we could open a task "Improve 
IGMP implementation"? I can easily test it since one of my lwIP products is 
using lot of multicast (sending/receiving...).
         
         
         
         
         
        
        
        ====================================
        Frédéric BERNON 
        HYMATOM SA 
        Chef de projet informatique 
        Microsoft Certified Professional 
        Tél. : +33 (0)4-67-87-61-10 
        Fax. : +33 (0)4-67-70-85-44 
        Email : address@hidden 
        Web Site : http://www.hymatom.fr <http://www.hymatom.fr/>  
        ====================================
        P Avant d'imprimer, penser à l'environnement
         

                -----Message d'origine-----
                De : address@hidden [mailto:address@hidden De la part de Bill 
Florac
                Envoyé : mercredi 11 juillet 2007 06:37
                À : lwip-devel
                Objet : RE: [lwip-devel] igmp implementation
                
                
                Follow-up,
                 
                I decide that the IGMP group list should be interface specific 
so I move the group list to the netif structure. I then modified igmp.c to deal 
on the netif structure passed.  I also change igmp_init() to igmp_start() as it 
only work if the interface is up and running.  I them added the igmp_start to 
the netif_set_up() function (removing it from tcpip.c).  The fuction type 
igmp_mac_filter() does not need the action or group parameters as it rebuild 
the hash table based on the list in the netif sturcture past.  Is there any 
reason to keep these?
                 
                While at it, I added a NETIF_FLAG_IGMP flag to the netif 
flags....
                 
                All may seem confusing but if this seem like the right 
direction, I'll submit the changes once I have done some testing.
                 
                Bill

________________________________

                From: address@hidden on behalf of Bill Florac
                Sent: Tue 7/10/2007 2:08 PM
                To: address@hidden
                Subject: [lwip-devel] igmp implementation
                
                

                I'm attempting to implement igmp off the latest build.  I see 
that I need to implement at igmp_mac_filter function.  My plan is to implement 
this in the ethernetif.c module.  It would be assigned to the netif structure 
in the low_level_init() function.  The function would then make the correct 
calls to the device driver (Atmel AT91 EMAC) to set or clear the correct hash 
key.
                
                As we leave a group we need to make sure that know another 
joined group address resolves to the same hash key. If so, we don't want to 
clear the key. There are a number of ways to resolve this.  We could keep a 
some sort of reference count or we can test the entire group list as we leave a 
group. If we do a test, either the igmp_mac_filter() would need to have the 
list of groups or the calling function igmp_leavegroup() would have to have 
access to the hashkey() function (making it HW dependent).
                
                Any thoughts how to best/better handle this?
                
                Bill
                
                
                _______________________________________________
                lwip-devel mailing list
                address@hidden
                http://lists.nongnu.org/mailman/listinfo/lwip-devel

<<winmail.dat>>