[lwip-users] Re: [lwip] update_arp_entry() => insert_arp_entry()

[Jani Monoses]

Hi Leon
the rename sounds OK to me.I have two more suggestions related to etharp.

1)why not initialize etharp_tmp from etharp_init() so that the ethernet driver 
does not
need to.I don't see a reason for the driver to start the timer later or stop it 
and restart it 
I think it would be cleaner this way.

2)Here's a patch to use absolute ctime for ARP entries
the code looks cleaner, is smaller and not prone to wrapping the 8bit global 
ctime and yielding incorrect values in ctime - arp_table[i].ctime.
Instead of marking the time when a new entry is created and checking against 
current 
time, each entry has it's own counter inited to 0 and at a maximum value it 
expires.

what do you think?

Jani.

diff -u -r1.3 etharp.c
--- netif/etharp.c      6 Nov 2002 11:43:21 -0000       1.3
+++ netif/etharp.c      7 Nov 2002 11:24:21 -0000
@@ -110,7 +110,6 @@
 
 static const struct eth_addr ethbroadcast = {{0xff,0xff,0xff,0xff,0xff,0xff}};
 static struct etharp_entry arp_table[ARP_TABLE_SIZE];
-static u8_t ctime;
 
 /**
  * Initializes ARP module.
@@ -136,15 +135,14 @@
 {
   u8_t i;
   
-  ++ctime;
   /* remove expired entries from the ARP table */
   for(i = 0; i < ARP_TABLE_SIZE; ++i) {
     if(arp_table[i].state == ETHARP_STATE_STABLE &&       
-       ctime - arp_table[i].ctime >= ARP_MAXAGE) {
+       arp_table[i].ctime++ >= ARP_MAXAGE) {
       DEBUGF(ETHARP_DEBUG, ("etharp_timer: expired stable entry %d.\n", i));
       arp_table[i].state = ETHARP_STATE_EMPTY;
     } else if(arp_table[i].state == ETHARP_STATE_PENDING &&
-             ctime - arp_table[i].ctime >= ARP_MAXPENDING) {
+             arp_table[i].ctime++ >= ARP_MAXPENDING) {
       DEBUGF(ETHARP_DEBUG, ("etharp_timer: expired pending entry %d - 
dequeueing %p.\n", i, arp_table[i].p));
       arp_table[i].state = ETHARP_STATE_EMPTY;
       pbuf_free(arp_table[i].p);      
@@ -179,8 +177,8 @@
     j = ARP_TABLE_SIZE;
     for(i = 0; i < ARP_TABLE_SIZE; ++i) {
       if(arp_table[i].state == ETHARP_STATE_STABLE &&
-        ctime - arp_table[i].ctime > maxtime) {
-       maxtime = ctime - arp_table[i].ctime;
+        arp_table[i].ctime++ > maxtime) {
+       maxtime = arp_table[i].ctime;
        j = i;
       }
     }
@@ -210,7 +208,7 @@
        for(k = 0; k < 6; ++k) {
          arp_table[i].ethaddr.addr[k] = ethaddr->addr[k];
        }
-       arp_table[i].ctime = ctime;
+       arp_table[i].ctime = 0;
        return NULL;
       }
       if(arp_table[i].state == ETHARP_STATE_PENDING) {
@@ -219,7 +217,7 @@
        for(k = 0; k < 6; ++k) {
          arp_table[i].ethaddr.addr[k] = ethaddr->addr[k];
        }
-       arp_table[i].ctime = ctime;
+       arp_table[i].ctime = 0;
        arp_table[i].state = ETHARP_STATE_STABLE;
        p = arp_table[i].p;
        if(p != NULL) { 
@@ -250,7 +248,7 @@
   for(k = 0; k < 6; ++k) {
     arp_table[i].ethaddr.addr[k] = ethaddr->addr[k];
   }
-  arp_table[i].ctime = ctime;
+  arp_table[i].ctime = 0;
   arp_table[i].state = ETHARP_STATE_STABLE;
   arp_table[i].p = NULL;
   
@@ -482,7 +480,7 @@
     /* Now, i is the ARP table entry which we will fill with the new
        information. */
     ip_addr_set(&arp_table[i].ipaddr, ipaddr);
-    arp_table[i].ctime = ctime;
+    arp_table[i].ctime = 0;
     arp_table[i].state = ETHARP_STATE_PENDING;
 #if 1
     arp_table[i].p = q;
@@ -623,9 +621,9 @@
     j = 0;
     for(i = 0; i < ARP_TABLE_SIZE; ++i)
     {
-      if(arp_table[i].state == ETHARP_STATE_STABLE && ctime - 
arp_table[i].ctime > maxtime)
+      if(arp_table[i].state == ETHARP_STATE_STABLE && arp_table[i].ctime > 
maxtime)
       {
-        maxtime = ctime - arp_table[i].ctime;
+        maxtime = arp_table[i].ctime;
         j = i;
       }
     }
@@ -646,7 +644,7 @@
   /*    for(k = 0; k < 6; ++k) {
   arp_table[i].ethaddr.addr[k] = dest->addr[k];
   }*/
-  arp_table[i].ctime = ctime;
+  arp_table[i].ctime = 0;
   arp_table[i].state = ETHARP_STATE_PENDING;
   arp_table[i].p = NULL;
 

[This message was sent through the lwip discussion list.]