buglwIP - A Lightweight TCP/IP stack - Bugs: bug #19177, Faulty arp initialization in...

 
 

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

bug #19177: Faulty arp initialization in ethernetif_init skeleton

Submitter:  Thomas Taranowski <taranowski>
Submitted:  Thu 01 Mar 2007 07:30:02 AM UTC
   
 
Category:  ARP Severity:  3 - Normal
Item Group:  Faulty Behaviour Status:  Fixed
Privacy:  Public Assigned to:  fbernon
Open/Closed:  Closed Planned Release:  None
lwIP version:  None

Jump to the original submission

Mon 26 Mar 2007 04:19:45 PM UTC, comment #12: 

Ok, I commit the new LWIP_ARP option. Note for developers: I don't have disable directly code in etharp.h/.c because some modules call them, and I don't know what should be a good behavior for them :

- snmp/mib2.c for ip_ntomentry_get_value() and atentry_get_value(),

- netif.c for netif_set_ipaddr() (in fact, the actual code for "gratuitous ARP" is disable).

- dhcp.c for dhcp_check()

I close this item, but reopen it if there is something wrong for you...

Frédéric Bernon <fbernon>
Group Member
Mon 26 Mar 2007 01:52:26 PM UTC, comment #11: 

Ok, I will call it from tcpip_init. It's also because others init (ip_init, udp_init & tcp_init) were moved from tcpip_thread to tcpip_init (I thought it solve some others developers problems, and more, it help to see which init functions use timers, and which don't use them).

Good, I will do like this...



Frédéric Bernon <fbernon>
Group Member
Mon 26 Mar 2007 01:45:47 PM UTC, comment #10: 

Re LWIP_ARP: fair enough.

Re etharp_init: yes calling from tcpip_init is absolutely fine. In comment 5, you referred to the sample code in comment 1, which has  etharp_init being called from the port-specific initialisation code, not from tcpip_init.

But if you're happy with calling it from tcpip_init, that's fine with me - it's essentially equivalent to calling it from the start of tcpip_thread anyway.

Jonathan Larmour <jifl>
Group Member
Mon 26 Mar 2007 01:35:15 PM UTC, comment #9: 

Because ARP can also be used with Token Ring (and even if this protocol is not really supported now), I think that LWIP_ARP would be better. Ok to set it to 1 as default value.

About etharp_init(), I don't understand why you want to call it inside tcpip_thread? It's just a table initialisation, no timer is initialized inside this function... So, why not like in comment #1, or why not there :

void
tcpip_init(void ( initfunc)(void ), void *arg)
{
#if LWIP_ARP
  etharp_init();
#endif
  ip_init();
#if LWIP_UDP
  udp_init();
#endif
#if LWIP_TCP
  tcp_init();
#endif

  tcpip_init_done = initfunc;
  tcpip_init_done_arg = arg;
  mbox = sys_mbox_new();
  sys_thread_new(tcpip_thread, NULL, TCPIP_THREAD_PRIO);
}

I just want to understand your point of view (perhaps I miss something)...



Frédéric Bernon <fbernon>
Group Member
Mon 26 Mar 2007 01:19:21 PM UTC, comment #8: 

I think we may as well have the default as enabled (1) as it is the common case, was sort of already the default (as long as the user built the etharp module) and I guess one of the underlying netif interfaces should be enabled by default, so it can be that one. Just my opinion.

Re comment 5: I think LWIP_ARP is fine, although LWIP_ETHARP might be clearer and more in line with the filename. Your choice :-).

If we have a LWIP_ARP define, then etharp_init() can be called from tcpip_thread() before the call to tcpip_init_done(), but only #if LWIP_ARP.

Note, this would need a documentation/Changelog update (and changes to the contributed ports) because the port will now no longer call etharp_init itself. But this is an improvement - simplifying ports is good.

Jonathan Larmour <jifl>
Group Member
Mon 26 Mar 2007 01:17:39 PM UTC, comment #7: 

Yes, I think ARP should be optional code.

Kieran Mansley <kieranm>
Group Member
Mon 26 Mar 2007 01:06:58 PM UTC, comment #6: 

Is it ok for others to add LWIP_ARP in opt.h to disable ARP code if you only use PPP&SLIP (and to set it to 0 par default)?

Frédéric Bernon <fbernon>
Group Member
Fri 23 Mar 2007 08:18:33 AM UTC, comment #5: 

For arp_timer, I will prefer a LWIP_ARP set to 0 in opts.h to match to other options (their names are LWIP_UDP, LWIP_TCP, ....).

etharp_init is another problem. It have to be call with other init function (like a sample is proposed in comment #1).

About idea from comment #1, where we have talk to initialize arp_timer from tcpip_init_done, to my point of view, if it's always possible, it is not a good solution to keep lwIP simple.

It's good for us that you use PPP & SLIP, because it doesn't seem that lot of active developpers can help us on this part.

Before change it with a LWIP_ARP option, I propose you to wait comments from other active developers. Agree for you?

Frédéric Bernon <fbernon>
Group Member
Fri 23 Mar 2007 07:04:50 AM UTC, comment #4: 

This patch is incorrect. PPP and SLIP users without ethernet interfaces should not have the ARP timer (and probably won't be building netif/etharp.c in the first place). That's why in the mail thread on lwip-users I was saying it would happen in the port's tcpip_init_done callback because the port knows whether there are any ethernet interfaces, and tcpip.c does not.

Either it should be left to the port, or there should be something like a new LWIP_ETH (or ETH_SUPPORT?) feature test macro in opt.h (overriden by the port's lwipopts.h), which can be used to make the ARP timer init conditional.

I think adding something like LWIP_ETH would make things simpler overall.

Separately, on the move of etharp_init to tcpip_init_done, I would think that if this change stays, it's important that this new requirement for ports is highlighted (and the example ports in contrib/ should be updated). But if we did have the ability to test for LWIP_ETH, then we could add etharp_init to tcpip_init.

Jonathan Larmour <jifl>
Group Member
Mon 05 Mar 2007 11:30:48 AM UTC, comment #3: 

With these patchs, I think that ARP initialization and use is correct, except for driver. See some solutions in forum's thread  "the ARP layer isnotprotectedagainstconcurrentaccess" (I will post a patch about that). If it's ok for others, I will integrate in CVS....



(file #12113, file #12114)

Frédéric Bernon <fbernon>
Group Member
Sat 03 Mar 2007 11:51:27 AM UTC, comment #2: 

I tend to agree with Frederic, and would remove the arp initialization from the ethernetif skeleton, in favor of sticking it in the tcpip_init_done() block.

Thomas Taranowski <taranowski>
Group Member
Thu 01 Mar 2007 08:52:30 AM UTC, comment #1: 

I think that  etharp_init() would have to be call before any net_add, because ARP Table is a global resource. Initialization sequence should be :

     #if LWIP_STATS
     stats_init      ();
     #endif /* STATS */
     #if (NO_SYS == 0)
     sys_init        ();
     #endif /* (NO_SYS == 0) */
     mem_init        ();
     memp_init       ();
     pbuf_init       ();
     etharp_init     ();
     netif_init      ();
     lwip_socket_init();

     //...
     netif_add( ...);
     //...


About the arp_timer, I proposed yesterday in the forum's thread "Timers & lwip tasks (a multithreadquestionwithLwIP)" to initialize it in the tcpip_init_done (if you use tcpip_thread).
 

Frédéric Bernon <fbernon>
Group Member
Thu 01 Mar 2007 07:30:02 AM UTC, original submission:  

The ethernetif_init skeleton executes the following code:


  ethernetif->ethaddr = (struct eth_addr *)&(netif->hwaddr[0]);

  low_level_init(netif);

  etharp_init();

  sys_timeout(ARP_TMR_INTERVAL, arp_timer, NULL);

  return ERR_OK;
}

When multiple network interfaces are instantiated, usually via the netif_add routine, the etharp_init is executed, and the arp timer start for each instantiation.  This is not desired behaviour, and results in multiple instances of the arp_timer timer, which is bad, and causes the arp_timer function to be run more frequently than expected.

I've attached a patch file that wraps the arp initialization such that it is executed at most, only one time.

Thomas Taranowski <taranowski>
Group Member

 

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

Attached Files
file #12113:  ethernetif.c.patch added by fbernon (627B - text/x-patch)
file #12114:  tcpip.c.patch added by fbernon (861B - text/x-patch)
file #12080:  ethernetif.patch added by taranowski (951B - application/octet-stream)

 

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 jifl (Posted a comment)
  • -email is unavailable- added by fbernon (Posted a comment)
  • -email is unavailable- added by taranowski (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 10 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2007-03-26 fbernon StatusNone Fixed
        Open/ClosedOpen Closed
    2007-03-23 jifl StatusFixed None
        Open/ClosedClosed Open
    2007-03-06 fbernon StatusNone Fixed
        Open/ClosedOpen Closed
    2007-03-05 fbernon Attached File- Added ethernetif.c.patch, #12113
        Attached File- Added tcpip.c.patch, #12114
    2007-03-04 fbernon Assigned toNone fbernon
    2007-03-01 taranowski Attached File- Added ethernetif.patch, #12080

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code