patchlwIP - A Lightweight TCP/IP stack - Patches: patch #9331, PPP: ppp_singlebuf: Use...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

patch #9331: PPP: ppp_singlebuf: Use pbuf_coalesce() instead of open-coded

Submitter:  Axel Lin <axellin>
Submitted:  Fri 05 May 2017 10:13:16 AM UTC
   
 
Category:  PPP Priority:  5 - Normal
Status:  Done Privacy:  Public
Assigned to:  gradator Open/Closed:  Closed
Planned Release:  None

Fri 05 May 2017 01:47:29 PM UTC, comment #4: 

Applied, with a follow up patch removing ppp_singlebuf.

I replaced "pbuf queue" to "pbuf chain" in the commit log for clarity.

Sylvain

Sylvain Rochet <gradator>
Group Member
Fri 05 May 2017 01:06:54 PM UTC, comment #3: 


> Maybe we should just consider that packets queues will never
> materialize, so p->len == p->tot_len and p->next == NULL are
> exactly the same.


I think the can be used, but such usages should be limited to queue/unqueue using special functions. At least the netif_loop code does so. But it does so by manually changing the pbuf struct members and does not pass the queued pbufs to any function.

So I'm OK with your proposal to see the two tests as equal. I even think this is already done in several places.

Simon Goldschmidt <goldsimon>
Group administrator
Fri 05 May 2017 12:59:44 PM UTC, comment #2: 

There is indeed a confusion between a chain of pbuf and a queue of pbuf. A chain of pbuf is one packet, and a queue is multiple packets, each of them composed with multiple pbufs. From what I observed packet queues were never implemented in lwIP and we only have single packets. Therefore p->next == NULL works as well as p->len == p->tot_len, but from the pbuf documentation, p->len == p->tot_len looks like more right because in this case we only coalesce the first packet and following packets are dropped, while p->next == NULL is false in case of a packets queues.

Maybe we should just consider that packets queues will never materialize, so p->len == p->tot_len and p->next == NULL are exactly the same.

Sylvain

Sylvain Rochet <gradator>
Group Member
Fri 05 May 2017 10:35:16 AM UTC, comment #1: 

There's a slight difference in the check ("p->next == NULL" vs. "p->len == p->tot_len"), but I guess that's a bug in pbuf_coalesce...
Other than getting a little slower, the code should work.

Simon Goldschmidt <goldsimon>
Group administrator
Fri 05 May 2017 10:13:16 AM UTC, original submission:  

pbuf_coalesce() creates a single pbuf out of a queue of pbufs, which is exactly what ppp_singlebuf() need.

I tried to test this patch but it seems my application always pass single pbuf to ppp_singlebuf(). So this is untested. Post it here for review.

Axel Lin <axellin>
Group Member

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by gradator (Posted a comment)
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by axellin (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 5 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2017-05-05 gradator CategoryNone PPP
        StatusNone Done
        Assigned toNone gradator
        Open/ClosedOpen Closed
    2017-05-05 axellin Attached File- Added 0001-PPP-ppp_singlebuf-use-pbuf_coalesce-instead-of-open-.patch, #40592

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code