-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
sys/net/sock: add sock_udp_sendv() API #17485
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do like the idea very much. One comment inline.
eebbe76
to
a59f52b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why an array? There's reasons why gnrc and netdev use iolists instead of arrays. They're much more stack friendly, especially for layered protocols.
And, iolists are used at the lowest level. I don't think gnrc can do it, because it needs pktsnips, but theoretically an application could provide an iolist, and the network stack just has to prepend the udp header and pass it on. Maybe this can be added to gnrc's machinery, with a special "iolist" pktsnip type.
An array would need to be reallocated, potentially multiple times.
We are on the sock layer here so we can't use network stack native list representations anyway.
I agree that lists provide additional flexibility in case someone wants to add multiple layers on top of |
Think of an application writing some content using protocol X. This is a potential call stack using iolists:
Using arrays, each layer would probably get an array as parameter. to add data, that array either needs to be in reverse order and large enough (who knows how large?), or, be recreated on each layer, but again, how big? variable length?. IMO, lists save a lot here. |
How about this? I'm not entirely happy with having to reverse the list depending on which network stack is used, but then again this is neatly hidden from the user and the list based interface does indeed provide more flexibility to the application. |
I still don't understand why the iolist has to be reversed. just to be clear, iolists are in-order, meaning, first entry (list head) contains bytes 0-x, second contains x+1 . ..., and so on, right? That's how it is passed from the application, and that's how the good old netdev expects (or expected) it. In the PR, you're reversing that for lwip and openwsn, but not gnrc? Even though lwip writes in-order using netcon_write_partly() and openwsn also copies in-order? does gnrc_pktbuf_add prepend? It seems to me something is turned around. if gnrc_pktbuf_add prepends, we need something that appends. There's gnrc_pkt_append, that sounds like it could be used (but might need allocation of a pktsnip, even though the data could be just pointed to? but, are we clear on the supposed order of the iolists? (You really don't want to re-use iolist.h?) |
Sometimes one does not see the forest due to all the trees 😄 I'll split up the commits and force push shortly. |
3a9d951
to
eb8e7a2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not familiar with the netconn/netbuf API, but it seems the TCP send can be simplified.
Fine, I still maintain that the |
you mean because of the member names, or because of the const? |
From my perspective you can squash this! |
both really, e.g. |
d0e7a39
to
bada975
Compare
bada975
to
784009c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK.
Code looks fine. Did some basic UDP testing, all good.
Thank you for the review! |
Contribution description
Both GNRC and LWIP support the notion of packet snips/slices: A packet does not have to be in continuous memory.
This is useful when e.g. sending a blockwise CoAP message where the CoAP header can be a small buffer and the CoAP block payload can be just a pointer into a larger buffer (possibly on ROM).
However, the sock API completely throws this out of the window by only allowing a single payload buffer in
sock_udp_send()
.This means we need another buffer to copy the payload and the header into, artificially doubling memory requirements and introducing an unnecessary copy step.
Fix this by introducing a new
sock_udp_sendv()
function. This allows to specify an array of payload chunks instead of a single buffer.sock_udp_send()
is now simple a wrapper around the new function.Implemented for
Testing procedure
Issues/PRs references