[En-Nut-Discussion] ARP cache again

[Harald Kipp]

Hi Marek,

At 22:48 20.02.2005 +0100, you wrote:
>Hi Harald...
>
>I have slightly different solving which solving the reason, no consequence:
>
>1) If NutArpCacheQuery working with entry so no one can not removing this
>entry. This is solving with new flag ATF_WORK. If ATF_WORK is set then no
>one can not remove this entry from ARP cache. Flag ATF_WORK is erased only
>in instance of NutArpCacheQuery which produced this entry...
>
>Added to if_var.h file >>>
>#define ATF_WORK    0x08        /*!< \brief Working with entry */
>
>2) This change enforce modification in ArpCacheAging:
>static void ArpCacheAging(void){
>...
>if ((ae->ae_flags & (ATF_PERM | ATF_WORK)) == 0 &&   /* Not permanent. */
>//if ((ae->ae_flags & (ATF_PERM)) == 0 &&   /* Not permanent. */
>...
>}

This is an alternative to Dusan's proposal to re-check
the entry
         if (rc && entry) {
             entry->ae_flags |= ATF_REM;
             ArpCacheFlush(ifn);
         }
But I'm still not convinced how this should become a
problem, because ArpCacheLookup() is called each
time after NutEventWait(). Did I overlook something?




>3) But in case of running several NutAprCacheQuery instances with same
>request for IP address then exist only one entry and working(sending ARP
>packet) with this entry only one instance of NutArpCacheQuery. Exactly first
>running instance of NutAprCacheQuery with same request for IP address will
>be sending ARP packet and have control above ATF_WORK flag. Remainder
>NutArpCacheQuery instances with same request waiting to finish first
>instance of NutArpCacheQuery with same request. Such a method guarantee
>minimal strain for system with several same request to ARP cache...

In the current version the first thread creates an incomplete
ARP entry and the NETBUF. Any following thread will not create
a NETBUF, because the (incomplete) entry exists already.

The only difference with ATF_WORK is, that ArpCacheAging()
will not remove this entry. In reality this should not happen,
because ARP cache aging times are much higher than ARP response
timeouts. And even if this happens (due to misconfiguration)
it doesn't really hurt, right?




>For several different request is all indentical like original version...
>
>4) Solving problem with released netbuffer is same as solving from Dusan
>Ferbas...


Right. Again my comment, that in such a case the system will
fail anyway, but I added Dusan's proposal because it's cleaner.



>5) NutArpCacheQuery is completely rewrite.

My problems with this are:

a) Except some (non kernel) date/time routines, the whole code avoids
'goto' and I'd like to keep it this way unless there are very good
reasons for breaking this rule. IMHO, it's difficult to follow code
based on flags and 'goto'. But that's my personal view.

b) Indeed NutNetBufFree() checks for a NULL pointer. But
the documentation of this function doesn't state this. There are
several possibilities:

   b1) Change the docs and let all the rest of the code rely on this.
   b2) Let the caller check for a NULL pointer.
   b3) Let caller and NutNetBufFree() check for a NULL pointer.

Version b3 (the current) is the safest while version b2 is the fastest.
Version b1 produces the smallest code. If we change this globally, I'd
vote for b2.

c) I intentionally replaced NutHeapAllocClear() by malloc() and
memset(). I forgot the give a reason and you changed it back. :-)
The reason is, that Nut/Net should be less dependant on Nut/OS and
more portable to other kernels.

d) You introduced a new local variable flag, but it doesn't provide any
other information than what's already there.

e) In NutArpCacheUpdate() you added a variable ifn, which
is never used. This may result in a compiler warning and warnings
are handled as errors when rebuilding the system.

f) In the same routine you redefined
   int rc
to
   char rc
This may be dangerous unless it is explicitly stated that
the code requires compilation for signed characters.
In addition it may result in better code for 8-bit machines
but definitely results in bad code for CPUs with larger
data paths. This problem appears at many places, but don't
let it become worse than it already is.

g) Using \r\n instead of \n in fprintf() is at least exotic. :-)
If the stream points to a file, it produces wrong results. If the
stream is a socket, it should be used if the protocol clearly specifies
this (as most text oriented TCP protocols do).

h) Adding additional brackets by changing
   if(a & 1)
to
   if((a & 1))
may follow your personal preferences but adds additional diff
output. This makes it more difficult to extract your particular
changes. (The same applies for additional spaces and blank lines.
I put this in brackets because I'm the wrong guy to discuss this.
My discipline is very low at this.) Generally speaking, please
try to keep your modifications as limited as possible to help
others to follow them. Finally, please honor the K&R style used
in most of the rest of the code.


>My solving save strain in system(only one instance of NutArpCacheQuery with
>same request for IP address sending ARP packet) and is more protected(no
>with NutEventWait forever) and is not large, approximately 30 bytes addition
>against original version from Nut/OS(check with AVRGCC :)...

I wouldn't care an additional 30 bytes.

Dusan stated already, that the original code may get locked
in an endless wait. Please explain to me how...I'm obviously
too stupid to see it. :-)




>I thanks Dusan Ferbas for his comments...

Thanks to you for taking your time to study the code and
offer an alternate solution. Hope you don't mind my critical
views. I'm always open for discussing them.




>PS: Sorry for my terrible english :))).

Not many of us are native English speakers. We can always ask if
something doesn't make sense first.

Thanks,

Harald