RE: [lwip-devel] Problem with UDP and ARP queuing

[Mountifield, Tony]

Sorry, wrong "if".

You have (effectively):

        if (arp_table[i].p == NULL) {
            arp_table[i].p = p;
        } else {
            pbuf_queue(arp_table[i].p, p);
        }
        pbuf_ref(p);

whereas I have:

        if (arp_table[i].p == NULL) {
            pbuf_ref(p);
            arp_table[i].p = p;
        } else {
            pbuf_queue(arp_table[i].p, p);
        }

I'm not sure which of the two is correct. It depends whether queueing requires 
a refcnt increment, and if so, whether pbuf_queue() handles it.

Cheers
Tony

> -----Original Message-----
> From: David Haas [mailto:address@hidden
> Sent: 18 August 2004 15:27
> To: lwip-devel
> Subject: Re: [lwip-devel] Problem with UDP and ARP queuing
> 
> 
> Its not outside of the "if (p != NULL) block."  My indentation just 
> seems to be wrong.
> 
> The inner  "if (arp_table[i].p)" statement just decides how 
> to queue the 
> pbuf.
> The pbuf is still queued no matter which branch is taken.
> 
> David.
> 
> Mountifield, Tony wrote:
> 
> >Hi David,
> >
> >I'm not 100% conversant with how reference counts are 
> supposed to work within queues and chains. but I see that 
> your pbuf_ref() is outside the if, and therefore called also 
> after pbuf_queue(). Is this correct?
> >
> >Cheers
> >Tony
> >
> >  
> >
> >>-----Original Message-----
> >>From: David Haas [mailto:address@hidden
> >>Sent: 18 August 2004 13:29
> >>To: lwip-devel
> >>Subject: Re: [lwip-devel] Problem with UDP and ARP queuing
> >>
> >>
> >>Yes, I agree with you.
> >>
> >>In fact I had made that change a few days ago, but had not 
> >>gotton around 
> >>to creating a patch yet. My code around this area looks like this:
> >>
> >>
> >>#if ARP_QUEUEING /* queue the given q packet */
> >>      /* copy any PBUF_REF referenced payloads into PBUF_RAM */
> >>      /* (the caller of lwIP assumes the referenced payload can be
> >>       * freed after it returns from the lwIP call that 
> >>brought us here) */
> >>      p = pbuf_take(q);
> >>      /* packet could be taken over? */
> >>      if (p != NULL) {
> >>        /* queue packet */
> >>        if (arp_table[i].p)
> >>          pbuf_queue(arp_table[i].p, p);
> >>        else
> >>          arp_table[i].p = p;
> >>      /* pbufs are queued, increase the reference count */
> >>      pbuf_ref(p);
> >>        LWIP_DEBUGF(ETHARP_DEBUG | DBG_TRACE, 
> ("etharp_query: queued 
> >>packet %p on ARP entry %d\n", (void *)q, i));
> >>        result = ERR_OK;
> >>      } else {
> >>        LWIP_DEBUGF(ETHARP_DEBUG | DBG_TRACE, ("etharp_query: 
> >>could not 
> >>queue a copy of PBUF_REF packet %p (out of memory)\n", (void *)q));
> >>        /* { result == ERR_MEM } through initialization */
> >>      }
> >>#else /* ARP_QUEUEING == 0 */
> >>
> >>I've done some pretty extensive testing with TCP, but only limited 
> >>testing with UDP.
> >>
> >>David.
> >>
> >>Mountifield, Tony wrote:
> >>
> >>    
> >>
> >>>Following up again:
> >>>
> >>> 
> >>>
> >>>      
> >>>
> >>>>After further thought, an idea:
> >>>>
> >>>>   
> >>>>
> >>>>        
> >>>>
> >>>>>It may be just that udp_send() has no reason to free the 
> >>>>>header pbuf, and we can remove that bit of code, but I don't 
> >>>>>know what else may rely on it.
> >>>>>     
> >>>>>
> >>>>>          
> >>>>>
> >>>>Perhaps it is a reference-counting issue. Does/should 
> >>>>etharp_query increment a reference count in the chain/packet 
> >>>>when queuing it on an ARP table entry? And then of course 
> >>>>decrement it when dequeuing.
> >>>>   
> >>>>
> >>>>        
> >>>>
> >>>I think the following is certainly needed, but we will need 
> >>>      
> >>>
> >>to make sure there is a pbuf_free() in the right place (there 
> >>may already be) to avoid a pbuf leak:
> >>    
> >>
> >>>Index: src/netif/etharp.c
> >>>===================================================================
> >>>RCS file: /cvsroot/lwip/lwip/src/netif/etharp.c,v
> >>>retrieving revision 1.80
> >>>diff -u -r1.80 etharp.c
> >>>--- src/netif/etharp.c  17 Aug 2004 08:39:43 -0000      1.80
> >>>+++ src/netif/etharp.c  18 Aug 2004 11:19:14 -0000
> >>>@@ -755,6 +755,7 @@
> >>>        /* queue packet ... */
> >>>        if (arp_table[i].p == NULL) {
> >>>               /* ... in the empty queue */
> >>>+               pbuf_ref(p);
> >>>               arp_table[i].p = p;
> >>>        } else {
> >>>               /* ... at tail of non-empty queue */
> >>>
> >>>Cheers
> >>>Tony


***********************************************************************************
This email, its content and any attachments is PRIVATE AND
CONFIDENTIAL to TANDBERG Television. If received in error please
notify the sender and destroy the original message and attachments.

www.tandbergtv.com
***********************************************************************************