RE: [lwip-users] ARP ETHARP_TRY_HARD

[Paul Clarke]

Hi Leon 
> 
> Hello Paul,
> 
> thanks for looking in the code, I enjoy having extra pairs of eyes 
> checking my code!
> >
> >I like the patches you have put in etharp.c but I still think that the
> >exit if !ETHARP_TRY_HARD is missing.
> >  
> Could you explain why please?
>

Although the code is correct with its TRY_HARD from a code maintenance
perspective it is a lot easier to see if there is an exit test 
for no empty slot and !flags & ETHARP_TRY_HARD.
The following code can then remove the TRY_HARD case also making 
it easier to understand. 

example

for (i = 0; i<ARP_TABLE_SIZE; i++){}
// at exit i is either an empty slot or ARP_TABLE_SIZE

if ( i== ARP_TABLE_SIZE && !flags & ETHARP_TRY_HARD)
{
        return ERR_ARG; // could not find an empty slot
}

// NOW we add this entry into our table
  /* b) choose the least destructive entry to recycle:
   * 1) empty entry
   * 2) oldest stable entry
   * 3) oldest pending entry without queued packets
   * 4) oldest pending entry with queued packets
   */

> I think the current code is correct in the find_entry(..., flags 
> = 0) case.
> 
> find_entry will:
> a) find candidate ARP entries for the given IP address
> b) select the least-cache-destructive entry
> c) see if it may alter that entry
> 
> Steps a and b are non-modifying, and are marked a and b in the source 
> code comments of etharp.c
> Step c ONLY recycles existing entries when an empty ARP entry is found:
> Step c ALWAYS fills empty ARP entries.  (I assume you do not want that 
> to happen, am I correct??)

Now that we add ARP Entries with the right TRY_HARD setting it is not
an issue. When I was testing this code it was easier to see what was 
happening if the ARP table only got filled with entries we care about
ie our connections.


> 
> See etharp.c line 308
> 
>   /* allowed to recycle a entry? */
>   if (flags & ETHARP_TRY_HARD) {
>     /* recycle (no-op for an already empty entry) */
>     arp_table[i].state = ETHARP_STATE_EMPTY;
>   }
> 
> Yes, it will fill up the cache with any broadcast traffic, BUT only 
> until the cache
> is full. Once the cache is full, it will return ERR_MEM, see line 327:
> 
>   /* no entry available */
>   } else {
>     /* return failure */
>     i = (s8_t)ERR_MEM;
>   }
> 
> Only if ETHARP_TRY_HARD is specified, it will recycle cache entries.
> 
> At least this is exactly what I think should happen.
> 
> Please let me know where my thoughts go wrong, or what you expected
> the algorithm to do?
> 
> Regards,
> 
> Leon Woestenberg.
> 
> 

Paul