[lwip-users] Re: [lwip] Memory leak and queued packets in etharp.c

[Martin Glunz]

Request for Comment:

Anybody to remember the queued packets in etharp.c?

OK, I've rethought the whole thing again.
Here's my conclusion:

Some of the routines at interface level can be moved
to etharp.c. These are: ifname_input(), ifname_output(),
I've named them etharp_input() and etharp_output().
The code inside these routines should be common to all kinds 
of ethernet interfaces, so the driver is simplified.

In the driver file remain the low level input and
output routines.

I've also implementd the packet queueing as a simple
array of pbuf *. This encapsulates the queueing fully
inside etharp.c, no other module needs to know about it.

Alas, this introduces a lot of changes to etharp.c and
the ethernet driver module. But as a overall result
the driver itself gets simpler, as all arp related
issues are in etharp.c. 

For the queueing the pbuf_ref_chain() is still needed.

Because I don't want to bloat this message, I've put
the sources to http://martins.wunderkis.de/lwip/
There'll be also the first (alpha) release of my SH7615
driver, as a example how to use the new etharp.c

There might be some issues with pbuf_ref_chain() and
pbuf_dechain(). The only place I've found pbuf_dechain
to be used is inside udp.c. In this case it seems
to work right.

Another issue is the queueing of packets that
are created by the user code with netbuf_ref().
This creates a chained pbuf with at least one
one element having the attribute PBUF_ROM.
There's a pending danger that the data part
of such a buffer gets overwritten by the
application before the queued packet is sent.
Should these be copied?


Just for reference (needed by my etharp.c):
> 
> /*------------------------------------------------------------------------------
> /* pbuf_ref_chain():
>  *
>  * Increments the reference count of all pbufs in a chain.
>  */
> /*------------------------------------------------------------------------------
> void
> pbuf_ref_chain(struct pbuf *p)
> {
>   while (p != NULL) {
>     p->ref++;
>     p=p->next;
>   }
> }
> 

and my freshly modified version of pbuf_dechain():
/*-----------------------------------------------------------------------------------*/
/* pbuf_dechain():
 *
 * Adjusts the ->tot_len field of the pbuf and returns the tail (if
 * any) of the pbuf chain.
 */
/*-----------------------------------------------------------------------------------*/
struct pbuf *
pbuf_dechain(struct pbuf *p)
{
  struct pbuf *q;

  if (p->ref>1) return p;       /* do nothing if pbuf is referenced more than
once */
  q = p->next;
  if (q != NULL) {
    q->tot_len = p->tot_len - p->len;
  }
  p->tot_len = p->len;
  //if (p->ref<=1) p->next = NULL;
  p->next = NULL;
  return q;
}


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