[lwip-devel] [task #6861] Pimp ip_frag.c

[Thomas Taranowski]

Follow-up Comment #25, task #6861 (project lwip):

Thanks for the careful review Jonathan...

- Ouch, sorry about the tabs.  I hate when that happens.
- Static comments noted, and mods made.
- Didn't know about the IP_HLEN vs. IPH_LEN.  I'll fix those up and retest
any reassembly code that uses those in the next couple days. 
- I would say you are correct on the else if. I converted to just the plain
old else.
- if ((ip_reass_pbufcount + clen) > IP_REASS_MAX_PBUFS) { 
  It's true that we can't assemble anything more until some pbufs have been
freed, by either completing assembly of a payload, or having the timer expire
and free a stale entry.  The problem I see, and I think you are hinting at, is
that with the check there, we don't accept any more pbufs once we hit that
check, and therefore are unable to complete any new datagrams, therefore the
oldest one MUST expire before we can continue, and hope to assemble any
further.  Timing out the oldest one is a reasonable solution, and seems better
than what we have now.  I'd like to have the code be smart, and check to see
if accepting the packet would complete a datagram first, before it just
discards it out of hand, and then, if it's unable to, go ahead and time out
the oldest one.  

- doubly linked list - I'm not convinced of anything in this life anymore :) 
The doubly linked list Simon introduced seemed like a reasonably solution, and
I didn't bother trying to refactor it out.  If we could, I think it would
simplify the code a good deal.  I think that would probably be a good goal of
another patch, along with a fix for the "@todo What to do on bogus datagram?"
block, and a method to trim datagrams to fit(which should be optional code).

- combine validate and chain - the problem with that is that, when chaining
the fragment, it's perfectly acceptable for there to be segments missing from
the chain (start!=end).  However, the validate requires that the segments be
contiguous.  Maybe something like a currently valid/contiguous marker could be
maintained in the chain_frag loop, to shortcut the validate call in most
cases.

- The previous reassembly code used a large static buffer/array, which to me
is more grievous than introducing extra code.  While working through the code,
I felt there were a number of cases where the code could be reworked into
common sections, and shortened.

I've attached a detabbed/static-i-fied patch.  I'll post the IPH_LEN, and
some additional cleanup, patch once I get some time to test.



(file #13574)
    _______________________________________________________

Additional Item Attachment:

File name: ip_frag.c.detabbed_and_static  Size:19 KB


    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/task/?6861>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/