ARMmbed / sal-stack-lwip

LwIP package for mbed
Other
4 stars 14 forks source link

Fixed issue with SAL TCP receive queue #34

Closed antevir closed 6 years ago

antevir commented 8 years ago

The receive queue for TCP in SAL wrapper called tcp_recved() with wrong length.

Change-Id: Ia1ae4b4af080bd8104011cd9b1e22354883644a3

ciarmcom commented 8 years ago

Automatic CI verification build not done, please verify manually.

antevir commented 8 years ago

I will try to explain why the new rx queue is needed. If we start with looking on the current solution in recv_copy_free():

                if (copied < p->len) {
                    /* advance the payload pointer by the number of bytes copied */
                    p->payload = (char *)p->payload + copied;
                    /* reduce the length by the number of bytes copied */
                    p->len -= copied;
                    /* break out of the loop */
                    copied = 0;
                } else {
                    struct pbuf *q;
                    uint16_t freelen = p->tot_len;
                    q = p->next;
                    /* decrement the number of bytes copied by the length of the buffer */
                    copied -= p->len;
                    /* Free the current pbuf */
                    /* NOTE: This operation is interrupt safe, but not thread safe. */
                    if (q != NULL) {
                        pbuf_ref(q);
                    }
                    socket->rxBufChain = q;
                    pbuf_free(p);
                    /* Update the TCP window */
                    tcp_recved(socket->impl, freelen);
                    p = q;
                }

tcp_recved() is called with p->tot_len each time a pbuf is freed. Since the data is chained with pbuf_cat() the tot_len for the first pbuf will be the total length for the complete chain of pbufs.

First idea for a fix would be to change so tcp_recved() is called with p->len (which normally is the lenght of the single pbuf entry). However, due to the code in the first if-statement the len field may have been modified and doesn't reflect how much data the pbuf allocated in the first place.

Instead of adjusting the payload pointer and len field when not reading out a complete pbuf we have choosen to instead use an offset variable that can be used as a parameter to pbuf_copy_partial()

bremoran commented 8 years ago

I'd rather not copy the pbufs extra times. I think it would make more sense to increase the TCP window by the number of bytes copied, wouldn't it?

Is this issue fixed by https://github.com/ARMmbed/sal-stack-lwip/pull/48?