RE : RE : RE : [lwip-devel] [bug #19206] the ARPlayerisnotprotectedagainstconcurrent access

[Frédéric BERNON]

>Nice patch. Although I had two input functions in my patch (one for IP, one 
>for ARP), to keep tcpip_thread from finding out what kind of frame this is, I 
>think this solution is also good. 

Yes, but footprint will increase (and because tcp_input and tcp_ethinput are 
defined, I think it is already enought (but, in final patch, I will add a #if 
ETHARP_TCPIP_INPUT, like ETHARP_TCPIP_ETHINPUT, to allow to users to disable 
tcp_input if they're sure to only want tcp_ethinput).

>Using netif->hwaddr is better than the other solution, anyway.
Yes, and other solution didn't work (wrong address casted). I hope only there 
is no aligment problem with that...

>The ethernetif.c example has redundant storage for the MAC address, which (as 
>I think) is not necessary. 
This redundant storage help me in this case, but, I'm agree with you, it's 
redundant.

>I would take the comment "Not really necessary, but..." out, though. It IS 
>necessary to protect the user from having a memory leak if
>sending all incoming frames to the tcpip_thread.
Ok, it's right

>Also, I'm not sure if we need the define ETHARP_TCPIP_ETHINPUT. Maybe to 
>reduce code
>size for PPP, but all other configurations should use this, so...
When we can reduce footprint, I think it's a good thing...

>Oh, and for some (old?) compilers, you have to put pre-processor statements at 
>the beginning of the line, or they will go unnoticed.
Uhmm, never see a such compiler, and I always thought that the indent with the 
code was only to make it more visible... But I will change that...

>Last but no least, I'm not sure if the ARP timer should be an option (again, I 
>don't know if PPP needs that one, I think not). 
If a PPP user can give us any information? I let it like that for the moment...

>OK, one more: while you're at it, can you modify one of the ports (e.g.
unixsim) to use the new functionality? I'll do the same for the windows port 
(I'm currently modifying it, anyway).
Euhhh? Why not, I can do it, but there is no someone which maintains the unix 
port? More, I can't test it, so...  Kieran?

>Other than that, I think the patch solves our problem. So please, go ahead and 
>check it in (as long as the other developers agree :)
I will commit today (if it's ok)...

====================================
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 
====================================
P Avant d'imprimer, penser à l'environnement
 


-----Message d'origine-----
De : address@hidden [mailto:address@hidden De la part de Goldschmidt Simon
Envoyé : mardi 6 mars 2007 08:15
À : lwip-devel
Objet : RE: RE : RE : [lwip-devel] [bug #19206] the 
ARPlayerisnotprotectedagainstconcurrent access


Hi,


> Patch with last fix. Always a solution to find for :
> 
> etharp_arp_input(netif, (struct eth_addr*)(netif->hwaddr),
> p); //FB Not really nice, but internal driver state in 
> "unknown" in tcpip.c 

Nice patch. Although I had two input functions in my patch (one for IP, one for 
ARP), to keep tcpip_thread from finding out what kind of frame this is, I think 
this solution is also good. Using netif->hwaddr is better than the other 
solution, anyway. The ethernetif.c example has redundant storage for the MAC 
address, which (as I think) is not necessary.

I would take the comment "Not really necessary, but..." out, though. It IS 
necessary to protect the user from having a memory leak if sending all incoming 
frames to the tcpip_thread. Also, I'm not sure if we need the define 
ETHARP_TCPIP_ETHINPUT. Maybe to reduce code size for PPP, but all other 
configurations should use this, so...

Oh, and for some (old?) compilers, you have to put pre-processor statements at 
the beginning of the line, or they will go unnoticed. Last but no least, I'm 
not sure if the ARP timer should be an option (again, I don't know if PPP needs 
that one, I think not).

OK, one more: while you're at it, can you modify one of the ports (e.g.
unixsim) to use the new functionality? I'll do the same for the windows port 
(I'm currently modifying it, anyway).

Other than that, I think the patch solves our problem. So please, go ahead and 
check it in (as long as the other developers agree :)

Simon


_______________________________________________
lwip-devel mailing list
address@hidden http://lists.nongnu.org/mailman/listinfo/lwip-devel

Frédéric BERNON.vcf
Description: Frédéric BERNON.vcf