]> rtime.felk.cvut.cz Git - pes-rpp/rpp-lwip.git/commitdiff
3rd fix for bug #11400 (arp-queuing): More pbufs than previously thought need to...
authorgoldsimon <goldsimon>
Wed, 11 Apr 2007 18:50:45 +0000 (18:50 +0000)
committergoldsimon <goldsimon>
Wed, 11 Apr 2007 18:50:45 +0000 (18:50 +0000)
CHANGELOG
src/core/pbuf.c
src/include/lwip/pbuf.h
src/netif/etharp.c

index a4ff5967258d5dc04eea4ec1ec5e686235b12b29..fcc89da7d237683ab160c37192cbc7235f6b895f 100644 (file)
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -117,6 +117,11 @@ HISTORY
 
   ++ Bug fixes:
 
+  2007-04-11 Simon Goldschmidt
+  * etharp.c, pbuf.c, pbuf.h: 3rd fix for bug #11400 (arp-queuing): More pbufs than
+    previously thought need to be copied (everything but PBUF_ROM!). Cleaned up
+    pbuf.c: removed functions no needed any more (by etharp).
+
   2007-04-11 Kieran Mansley
   * inet.c, ip_addr.h, sockets.h, sys.h, tcp.h: Apply patch #5745: Fix
     "Constant is long" warnings with 16bit compilers.  Contributed by
index 1093a3b9cfc1cb4f01ca66f481d6ab8b41605eba..96562cb53d6eabd2d25fa57ee0045b673fe69a6b 100644 (file)
@@ -740,216 +740,6 @@ pbuf_chain(struct pbuf *h, struct pbuf *t)
   LWIP_DEBUGF(PBUF_DEBUG | LWIP_DBG_FRESH | 2, ("pbuf_chain: %p references %p\n", (void *)h, (void *)t));
 }
 
-/* For packet queueing. Note that queued packets MUST be dequeued first
- * using pbuf_dequeue() before calling other pbuf_() functions. */
-#if ARP_QUEUEING
-/**
- * Add a packet to the end of a queue.
- *
- * @param q pointer to first packet on the queue
- * @param n packet to be queued
- *
- * Both packets MUST be given, and must be different.
- */
-void
-pbuf_queue(struct pbuf *p, struct pbuf *n)
-{
-#if PBUF_DEBUG /* remember head of queue */
-  struct pbuf *q = p;
-#endif
-  /* programmer stupidity checks */
-  LWIP_ASSERT("p == NULL in pbuf_queue: this indicates a programmer error\n", p != NULL);
-  LWIP_ASSERT("n == NULL in pbuf_queue: this indicates a programmer error\n", n != NULL);
-  LWIP_ASSERT("p == n in pbuf_queue: this indicates a programmer error\n", p != n);
-  if ((p == NULL) || (n == NULL) || (p == n)){
-    LWIP_DEBUGF(PBUF_DEBUG | LWIP_DBG_HALT | 3, ("pbuf_queue: programmer argument error\n"));
-    return;
-  }
-
-  /* iterate through all packets on queue */
-  while (p->next != NULL) {
-/* be very picky about pbuf chain correctness */
-#if PBUF_DEBUG
-    /* iterate through all pbufs in packet */
-    while (p->tot_len != p->len) {
-      /* make sure invariant condition holds */
-      LWIP_ASSERT("p->len < p->tot_len", p->len < p->tot_len);
-      /* make sure each packet is complete */
-      LWIP_ASSERT("p->next != NULL", p->next != NULL);
-      p = p->next;
-      /* { p->tot_len == p->len => p is last pbuf of a packet } */
-    }
-    /* { p is last pbuf of a packet } */
-    /* proceed to next packet on queue */
-#endif
-    /* proceed to next pbuf */
-    if (p->next != NULL) p = p->next;
-  }
-  /* { p->tot_len == p->len and p->next == NULL } ==>
-   * { p is last pbuf of last packet on queue } */
-  /* chain last pbuf of queue with n */
-  p->next = n;
-  /* n is now referenced to by the (packet p in the) queue */
-  pbuf_ref(n);
-#if PBUF_DEBUG
-  LWIP_DEBUGF(PBUF_DEBUG | LWIP_DBG_FRESH | 2,
-    ("pbuf_queue: newly queued packet %p sits after packet %p in queue %p\n",
-    (void *)n, (void *)p, (void *)q));
-#endif
-}
-
-/**
- * Remove a packet from the head of a queue.
- *
- * The caller MUST reference the remainder of the queue (as returned). The
- * caller MUST NOT call pbuf_ref() as it implicitly takes over the reference
- * from p.
- * 
- * @param p pointer to first packet on the queue which will be dequeued.
- * @return first packet on the remaining queue (NULL if no further packets).
- *
- */
-struct pbuf *
-pbuf_dequeue(struct pbuf *p)
-{
-  struct pbuf *q;
-  LWIP_ASSERT("p != NULL", p != NULL);
-
-  /* iterate through all pbufs in packet p */
-  while (p->tot_len != p->len) {
-    /* make sure invariant condition holds */
-    LWIP_ASSERT("p->len < p->tot_len", p->len < p->tot_len);
-    /* make sure each packet is complete */
-    LWIP_ASSERT("p->next != NULL", p->next != NULL);
-    p = p->next;
-  }
-  /* { p->tot_len == p->len } => p is the last pbuf of the first packet */
-  /* remember next packet on queue in q */
-  q = p->next;
-  /* dequeue packet p from queue */
-  p->next = NULL;
-  /* any next packet on queue? */
-  if (q != NULL) {
-    /* although q is no longer referenced by p, it MUST be referenced by
-     * the caller, who is maintaining this packet queue. So, we do not call
-     * pbuf_free(q) here, resulting in an implicit pbuf_ref(q) for the caller. */
-    LWIP_DEBUGF(PBUF_DEBUG | LWIP_DBG_FRESH | 2, ("pbuf_dequeue: first remaining packet on queue is %p\n", (void *)q));
-  } else {
-    LWIP_DEBUGF(PBUF_DEBUG | LWIP_DBG_FRESH | 2, ("pbuf_dequeue: no further packets on queue\n"));
-  }
-  return q;
-}
-#endif
-
-/**
- *
- * Create PBUF_POOL (or PBUF_RAM) copies of PBUF_REF pbufs.
- *
- * Used to queue packets on behalf of the lwIP stack, such as
- * ARP based queueing.
- *
- * Go through a pbuf chain and replace any PBUF_REF buffers
- * with PBUF_POOL (or PBUF_RAM) pbufs, each taking a copy of
- * the referenced data.
- *
- * @note You MUST explicitly use p = pbuf_take(p);
- * The pbuf you give as argument, may have been replaced
- * by a (differently located) copy through pbuf_take()!
- *
- * @note Any replaced pbufs will be freed through pbuf_free().
- * This may deallocate them if they become no longer referenced.
- *
- * @param p Head of pbuf chain to process
- *
- * @return Pointer to head of pbuf chain
- */
-struct pbuf *
-pbuf_take(struct pbuf *p)
-{
-  struct pbuf *q , *prev, *head;
-  LWIP_ASSERT("pbuf_take: p != NULL\n", p != NULL);
-  LWIP_DEBUGF(PBUF_DEBUG | LWIP_DBG_TRACE | 3, ("pbuf_take(%p)\n", (void*)p));
-
-  prev = NULL;
-  head = p;
-  /* iterate through pbuf chain */
-  do
-  {
-    /* pbuf is of type PBUF_REF? */
-    if (p->flags == PBUF_FLAG_REF) {
-      LWIP_DEBUGF(PBUF_DEBUG | LWIP_DBG_TRACE, ("pbuf_take: encountered PBUF_REF %p\n", (void *)p));
-      /* allocate a pbuf (w/ payload) fully in RAM */
-      /* PBUF_POOL buffers are faster if we can use them */
-      if (p->len <= PBUF_POOL_BUFSIZE) {
-        q = pbuf_alloc(PBUF_RAW, p->len, PBUF_POOL);
-        if (q == NULL) {
-          LWIP_DEBUGF(PBUF_DEBUG | LWIP_DBG_TRACE | 2, ("pbuf_take: Could not allocate PBUF_POOL\n"));
-        }
-      } else {
-        /* no replacement pbuf yet */
-        q = NULL;
-        LWIP_DEBUGF(PBUF_DEBUG | LWIP_DBG_TRACE | 2, ("pbuf_take: PBUF_POOL too small to replace PBUF_REF\n"));
-      }
-      /* no (large enough) PBUF_POOL was available? retry with PBUF_RAM */
-      if (q == NULL) {
-        q = pbuf_alloc(PBUF_RAW, p->len, PBUF_RAM);
-        if (q == NULL) {
-          LWIP_DEBUGF(PBUF_DEBUG | LWIP_DBG_TRACE | 2, ("pbuf_take: Could not allocate PBUF_RAM\n"));
-        }
-      }
-      /* replacement pbuf could be allocated? */
-      if (q != NULL)
-      {
-        /* copy p to q */
-        /* copy successor */
-        q->next = p->next;
-        /* remove linkage from original pbuf */
-        p->next = NULL;
-        /* remove linkage to original pbuf */
-        if (prev != NULL) {
-          /* prev->next == p at this point */
-          LWIP_ASSERT("prev->next == p", prev->next == p);
-          /* break chain and insert new pbuf instead */
-          prev->next = q;
-        /* prev == NULL, so we replaced the head pbuf of the chain */
-        } else {
-          head = q;
-        }
-        /* copy pbuf payload */
-        memcpy(q->payload, p->payload, p->len);
-        q->tot_len = p->tot_len;
-        q->len = p->len;
-        /* in case p was the first pbuf, it is no longer refered to by
-         * our caller, as the caller MUST do p = pbuf_take(p);
-         * in case p was not the first pbuf, it is no longer refered to
-         * by prev. we can safely free the pbuf here.
-         * (note that we have set p->next to NULL already so that
-         * we will not free the rest of the chain by accident.)
-         */
-        pbuf_free(p);
-        /* do not copy ref, since someone else might be using the old buffer */
-        LWIP_DEBUGF(PBUF_DEBUG, ("pbuf_take: replaced PBUF_REF %p with %p\n", (void *)p, (void *)q));
-        p = q;
-      } else {
-        /* deallocate chain */
-        pbuf_free(head);
-        LWIP_DEBUGF(PBUF_DEBUG | 2, ("pbuf_take: failed to allocate replacement pbuf for %p\n", (void *)p));
-        return NULL;
-      }
-    /* p->flags != PBUF_FLAG_REF */
-    } else {
-      LWIP_DEBUGF(PBUF_DEBUG | LWIP_DBG_TRACE | 1, ("pbuf_take: skipping pbuf not of type PBUF_REF\n"));
-    }
-    /* remember this pbuf */
-    prev = p;
-    /* proceed to next pbuf in original chain */
-    p = p->next;
-  } while (p);
-  LWIP_DEBUGF(PBUF_DEBUG | LWIP_DBG_TRACE | 1, ("pbuf_take: end of chain reached.\n"));
-
-  return head;
-}
-
 /**
  * Dechains the first pbuf from its succeeding pbufs in the chain.
  *
@@ -988,3 +778,56 @@ pbuf_dechain(struct pbuf *p)
   LWIP_ASSERT("p->tot_len == p->len", p->tot_len == p->len);
   return (tail_gone > 0? NULL: q);
 }
+
+#if ARP_QUEUEING
+/**
+ *
+ * Create PBUF_RAM copies of pbufs.
+ *
+ * Used to queue packets on behalf of the lwIP stack, such as
+ * ARP based queueing.
+ *
+ * @note You MUST explicitly use p = pbuf_take(p);
+ *
+ * @note Only one packet is copied, no packet queue!
+ *
+ * @param p Head of pbuf chain to process
+ *
+ * @return Pointer to head of pbuf chain
+ */
+struct pbuf *
+pbuf_copy(struct pbuf *p)
+{
+  u16_t copied=0;
+  struct pbuf *p_copy;
+  LWIP_ASSERT("pbuf_copy: p != NULL\n", p != NULL);
+  LWIP_DEBUGF(PBUF_DEBUG | LWIP_DBG_TRACE | 3, ("pbuf_copy(%p)\n", (void*)p));
+
+  /* allocate one pbuf to hold the complete packet */
+  p_copy = pbuf_alloc(PBUF_RAW, p->tot_len, PBUF_RAM);
+  if(p_copy == NULL) {
+    /* out of memory */
+    return NULL;
+  }
+
+  /* iterate through pbuf chain */
+  do
+  {
+    /* copy one part of the original chain */
+    memcpy((char*)p_copy->payload + copied, p->payload, p->len);
+    copied += p->len;
+    LWIP_DEBUGF(PBUF_DEBUG, ("pbuf_copy: copied pbuf %p\n", (void *)p));
+
+    if(p->len == p->tot_len) {
+      /* don't copy more than one packet! */
+      LWIP_ASSERT("pbuf_copy() does not allow packet queues!\n",
+                  p->next == NULL);
+    }
+    /* proceed to next pbuf in original chain */
+    p = p->next;
+  } while (p);
+  LWIP_DEBUGF(PBUF_DEBUG | LWIP_DBG_TRACE | 1, ("pbuf_copy: end of chain reached.\n"));
+
+  return p_copy;
+}
+#endif /* ARP_QUEUEING */
index 546aa30357cefb90564f38c547081c6402f4ff81..bd7bb8ed3ffb0a74ade2820ea9aa60dd739f63c2 100644 (file)
@@ -105,9 +105,9 @@ u8_t pbuf_free(struct pbuf *p);
 u8_t pbuf_clen(struct pbuf *p);  
 void pbuf_cat(struct pbuf *h, struct pbuf *t);
 void pbuf_chain(struct pbuf *h, struct pbuf *t);
-struct pbuf *pbuf_take(struct pbuf *f);
 struct pbuf *pbuf_dechain(struct pbuf *p);
-void pbuf_queue(struct pbuf *p, struct pbuf *n);
-struct pbuf * pbuf_dequeue(struct pbuf *p);
+#if ARP_QUEUEING
+struct pbuf *pbuf_copy(struct pbuf *f);
+#endif
 
 #endif /* __LWIP_PBUF_H__ */
index 73b0d82bc39704f28d115ab488e4365eb6f1aac3..73d679d215c11cf2ada1645b46e8f7f6cb57af6e 100644 (file)
@@ -756,6 +756,8 @@ etharp_output(struct netif *netif, struct ip_addr *ipaddr, struct pbuf *q)
  * @param q If non-NULL, a pbuf that must be delivered to the IP address.
  * q is not freed by this function.
  *
+ * @note q must only be ONE packet, not a packet queue!
+ *
  * @return
  * - ERR_BUF Could not make room for Ethernet header.
  * - ERR_MEM Hardware address unknown, and no more ARP entries available
@@ -828,21 +830,38 @@ err_t etharp_query(struct netif *netif, struct ip_addr *ipaddr, struct pbuf *q)
     } else if (arp_table[i].state == ETHARP_STATE_PENDING) {
 #if ARP_QUEUEING /* queue the given q packet */
       struct pbuf *p;
-      /* 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) */
-      /* @todo: if q->type == PBUF_REF, do we have a memory leak (since we call pbuf_ref(p) later)? */
-      p = pbuf_take(q);
+      int copy_needed = 0;
+      /* IF q includes a PBUF_REF, PBUF_POOL or PBUF_RAM, we have no choice but
+       * to copy the whole queue into a new PBUF_RAM (see bug #11400) 
+       * PBUF_ROMs can be left as they are, since ROM must not get changed. */
+      p = q;
+      while(p) {
+        if(p->len == p->tot_len) {
+          LWIP_ASSERT("no packet queues allowed!", p->next == 0);
+        }
+        if(p->flags != PBUF_FLAG_ROM) {
+          copy_needed = 1;
+          break;
+        }
+        p = p->next;
+      }
+      if(copy_needed) {
+        /* copy the whole packet into new pbufs */
+        p = pbuf_copy(q);
+      } else {
+        /* referencing the old pbuf is enough */
+        p = q;
+        pbuf_ref(p);
+     }
       /* packet could be taken over? */
       if (p != NULL) {
         /* queue packet ... */
-        struct etharp_q_entry *newEntry;
+        struct etharp_q_entry *new_entry;
         /* allocate a new arp queue entry */
-        newEntry = memp_malloc(MEMP_ARP_QUEUE);
-        LWIP_ASSERT("newEntry != NULL", newEntry != NULL);
-        newEntry->next = 0;
-        newEntry->p = p;
-        pbuf_ref(p);
+        new_entry = memp_malloc(MEMP_ARP_QUEUE);
+        LWIP_ASSERT("newEntry != NULL", new_entry != NULL);
+        new_entry->next = 0;
+        new_entry->p = p;
         if(arp_table[i].q != NULL) {
           /* queue was already existent, append the new entry to the end */
           struct etharp_q_entry *r;
@@ -850,10 +869,10 @@ err_t etharp_query(struct netif *netif, struct ip_addr *ipaddr, struct pbuf *q)
           while(r->next != NULL) {
             r = r->next;
           }
-          r->next = newEntry;
+          r->next = new_entry;
         } else {
           /* queue did not exist, first item in queue */
-          arp_table[i].q = newEntry;
+          arp_table[i].q = new_entry;
         }
         LWIP_DEBUGF(ETHARP_DEBUG | LWIP_DBG_TRACE, ("etharp_query: queued packet %p on ARP entry %"S16_F"\n", (void *)q, (s16_t)i));
         result = ERR_OK;