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
net: sockets: Implement sendto() and recvfrom() #1055
Conversation
fa1c014
to
3d25f3a
Compare
subsys/net/lib/sockets/sockets.c
Outdated
|
||
int zsock_inet_pton(sa_family_t family, const char *src, void *dst) | ||
{ | ||
if (net_addr_pton(family, src, dst) == 0) |
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.
add {} always, even on one liners. this is a zephyr style requirement
subsys/net/lib/sockets/sockets.c
Outdated
@@ -209,45 +210,100 @@ int zsock_accept(int sock, struct sockaddr *addr, socklen_t *addrlen) | |||
return POINTER_TO_INT(ctx); | |||
} | |||
|
|||
static struct net_pkt* get_tx_pkt(struct net_context *ctx, |
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.
this is not a getter function anymore as you are appending data as well in the process, you'll have to rename this one.
subsys/net/lib/sockets/sockets.c
Outdated
int err; | ||
struct net_pkt *send_pkt; | ||
s32_t timeout = K_FOREVER; | ||
struct net_context *ctx = INT_TO_POINTER(sock); |
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.
reverse the order of all the 5 lines above.
subsys/net/lib/sockets/sockets.c
Outdated
/* auto bind if necessary */ | ||
if (!net_context_is_bound(ctx)) { | ||
struct sockaddr src_addr; | ||
memset(&src_addr, 0, sizeof(src_addr)); |
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.
lines break are cheap and help for code readability, add empty lines after variable declaration.
subsys/net/ip/net_pkt.c
Outdated
u16_t port = 0; | ||
|
||
if (!addr || !pkt || !addrlen) | ||
return -EINVAL; |
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.
{} needed
subsys/net/ip/net_context.c
Outdated
{ | ||
NET_ASSERT(context); | ||
|
||
#if defined(CONFIG_NET_IPV6) |
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.
use IS_ENABLED with this CONFIG option, in the test right below.
Do the same for ipv4 also
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.
As a quick reply, this seems more complex than intuitively expected.
My intuition can be wrong of course, but one issue I see is trying to resolve "default local bind for UDP" issue on BSD sockets level. This isn't good approach. We should resolve as many as possible issues on net_context (i.e. native Z API level), to make sure that it, and any levels above it benefit from it.
I introduced a framework for default local binding in #817, and applied it to net_context_connect(). It was on my TODO to apply it to UDP use cases which don't require explicit connect(), but that slipped.
So, I'd recommend to use the same approach and already introduced functions from #817 to handle unbound UDP context in net_context_send(to)() and net_context_recv().
Just as send/recv operations are usually on critical path, I'd suggest calling bind_default() only on UDP paths (as TCP should be already bound in connect()).
The above would be a separate PR. And actually, I'd suggest making inet_pton() fix (good catch!) its own PR too.
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 not have anything extra, Tomasz pretty much commented everything already.
Indeed, good point. |
@tbursztyka , I will correct the coding style problem. Thanks |
3d25f3a
to
5aaee19
Compare
@askawu: Any updates here? Would you like me to give a try to resolving auto-bind issue for UDP sockets? |
subsys/net/ip/net_context.c
Outdated
@@ -2096,6 +2096,11 @@ static int sendto(struct net_pkt *pkt, | |||
return -EBADF; | |||
} | |||
|
|||
ret = bind_default(context); |
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 would definitely suggest to call this only for UDP case, there's no need to do an extra call for TCP case. There're 2 ways to do that:
- If we care about source addr to be bound for offloading case, then just add bind_default() block wrapped in
#if defined(CONFIG_NET_UDP)
after existing#if defined(CONFIG_NET_TCP)
right below. - If we can cheat on offloading case, can reuse existing
if (net_context_get_ip_proto(context) == IPPROTO_UDP)
far below.
@jukkar : What do you think?
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.
Sure, makes sense.
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.
@jukkar: Thanks for the reply, but it's "either/or" type of question (either way 1 above, or way 2) ;-). Please suggest which way to go.
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.
@GAnthony : Maybe you can suggest here something too.
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 prefer the second one.
subsys/net/ip/net_pkt.c
Outdated
if (IS_ENABLED(CONFIG_NET_IPV6) && family == AF_INET6) { | ||
struct sockaddr_in6 *addr6 = net_sin6(addr); | ||
|
||
if (*addrlen < sizeof(struct sockaddr_in6)) { |
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.
What about returning the address len in *addrlen? If you didn't intend to do that, why is it passed by pointer?
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.
Yes, the address size isn't necessary to be a pointer.
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.
Hmm, I'd say "it's useful to return the actual addrlen". Because otherwise, we probably don't even need to pass it in - struct sockaddr can accommodate both IPv4 and IPv6 addresses.
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.
It's used to prevent out of bound access. For example, the address is IPv6 but a sockaddr_in variable is passed in. I know it's rare but the caller might make a mistake. In such case, an error will be returned, so I probably don't need to return the actual addrlen.
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.
Ok, I assume you considered various usecases for this function, so sounds good. (And we can change it later if found useful.)
* | ||
* @param pkt Nework packet | ||
* @param addr Source socket address | ||
* @param addrlen The length of source socket address |
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.
Once semantics of *addrlen figure out (comment below), this docstring should describe what gets there on input, what on output.
subsys/net/lib/sockets/sockets.c
Outdated
} | ||
|
||
if (len > max_len) { | ||
len = max_len; | ||
return len; |
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.
This doesn't appear to be correct.
subsys/net/lib/sockets/sockets.c
Outdated
return len; | ||
} | ||
|
||
ssize_t zsock_sendto(int sock, const void *buf, size_t len, int flags, |
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 would do it differently - zsock_send() should just call to zsock_sendto() with NULL dest address. Given that there's issue with length returned (previous comment), you might need to it like that.
subsys/net/lib/sockets/sockets.c
Outdated
errno = EAGAIN; | ||
return -1; | ||
} | ||
|
||
err = net_context_send(send_pkt, /*cb*/NULL, timeout, NULL, NULL); | ||
SET_ERRNO(net_context_recv(ctx, zsock_received_cb, K_NO_WAIT, NULL)); |
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 is this line here? (And not in zsock_send()? zsock_send() and zsock_sendto() should behave the same, and as noted previously, the way to ensure that is make one tail-call another).
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.
My assumption is, for TCP case, zsock_connect() will be called first and callback will be registered at that time. So I only add it in zsock_sendto().
If we make zsock_send() and zsock_send() in adhoc way, any idea to prevent the registering the callback twice? Is there a proper way in net context to check if the callback has been registered?
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 see. Yep, there're some non-obvious points which only come to attention when you dig deeper into things. (Which may hint that there should be a code comment about it!)
If we make zsock_send() and zsock_send() in adhoc way, any idea to prevent the registering the callback twice? Is there a proper way in net context to check if the callback has been registered?
Well, so you already register a callback here multiple times - each call to sendto() will lead to callback being registered. If that works, then testing for UDP and setting it in send() should also work.
But overall, that's a similar problem to bind_default(). I'd say, making send() to tail-cal to sendto() is more priority than avoiding duplicate setting if recv callback. Let's try to optimize it as we can now (i.e. set it in send*() only for UDP case), and I already accept that we'll need a much later pass over the socket code to see how we can optimize things.
@@ -332,10 +382,17 @@ static inline ssize_t zsock_recv_stream(struct net_context *ctx, void *buf, size | |||
|
|||
ssize_t zsock_recv(int sock, void *buf, size_t max_len, int flags) | |||
{ | |||
return zsock_recvfrom(sock, buf, max_len, flags, NULL, NULL); |
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.
Yes, this is right. zsock_send() should be done like this.
if (net_context_get_type(ctx) == SOCK_STREAM) { | ||
/* TCP: we don't care about packet header, get rid of it asap. |
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 this conditionalizing, leading to code duplication? If we (may) need a packet header, let's keep it here, and consistently remove in zsock_recvfrom.
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.
Initially, I prefer to keep the packet header for both cases. But I found zsock_recv_stream() needs to be modified a lot because packet header isn't dropped. I will modify this to prefer consistency.
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.
Hmm, you may be right - and we can't return src addr for an arbitrary TCP recv anyway (only for recv() which starts to consume a new packet). Well, that's why it's useful to know why something was implemented this or that way, so having extra info in the commit messages or PR comments is helpful. And yeah, I'd appreciate your giving another thought to trying to avoid that code duplication - if it will lead to only more code being added, sure, it's ok to leave it like that.
@askawu: Thanks for refactors so far, and sorry for the delay with the review. Well, unfortunately it's still not there IMHO, some issues need fixing, and some things can be redone in an easier/cleaner way. Per-commit comments:
I was surprised that such a function needs to be introduced. How did we do it before? I checked echo_server and yeah, it picks src addr (to be dst addr on reply) in an adhoc way. @jukkar , how do you deal with it in http server? Would the func above be useful there, would you refactor to use it?
My usual suggestion goes here: please split unrelated patches to a standalobe PRs to allow for quicker review cycle. I also suggest to optimize TCP case (for which we know that context is already bound). There's a pending question to @jukkar how to best do that.
zsock_recvfrom() is implemented like it should be - by making zsock_recv() to dispatch to it. There's still a code duplication to optimize though. But instead of doing the same for zsock_sendto() and zsock_send(), it's done in adhoc way, which in particular leads to a bug with return length. Please redo the same way as zsock_recvfrom()/zsock_recv(). Besides being easier and cleaner that way, that's also definitely a requirement for socket offloading, I hope @GAnthony can second on that. Thanks! |
5aaee19
to
047224c
Compare
subsys/net/ip/net_pkt.c
Outdated
|
||
ctx = pkt->context; | ||
proto = net_context_get_ip_proto(ctx); | ||
family = net_context_get_family(ctx); |
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.
The pkt might not have context specified so this is error prone.
Family can be get using net_pkt_family(pkt) instead. For protocol, we could get that information from IP header, or we could store it to net_pkt header when packet is received. We have been trying to avoid increasing the size of net_pkt so first option could be done here.
So depending on family, the UDP/TCP information can be get like this
proto = NET_IPV6_HDR(pkt)->nexthdr;
or
proto = NET_IPV4_HDR(pkt)->proto;
subsys/net/ip/net_pkt.c
Outdated
return -ENOTSUP; | ||
} | ||
|
||
if (IS_ENABLED(CONFIG_NET_IPV6) && family == AF_INET6) { |
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.
If you move this block before checking UDP/TCP, then you could add the proto resolving here.
That function sounds useful to have, +1 for that. |
I must have missed this question, could you elaborate what was this question about? |
Introduce net_pkt_get_src_addr() as a helper function to get the source address and port from the packet header. Signed-off-by: Aska Wu <aska.wu@linaro.org>
This patch makes net_context_sendto() work independently without calling net_context_connect() first. It will bind default address and port if necessary. Also, since receive callback should be provided before sending data in order to receive the response, bind default address and port to prevent providing an unbound address and port to net_conn_register(). Signed-off-by: Aska Wu <aska.wu@linaro.org>
sendto() and recvfrom() are often used with datagram socket. sendto() is based on net_context_sendto() and recvfrom() is based on zsock_recv() with parsing source address from the packet header. Signed-off-by: Aska Wu <aska.wu@linaro.org>
- Add test cases of ipv4/ipv6 sendto() and recvfrom() - Set the main stack size to 2048 to enable both ipv4 and ipv6 on qemu_x86. - Use net app for network setup. Signed-off-by: Aska Wu <aska.wu@linaro.org>
047224c
to
d73c931
Compare
Update:
|
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.
LGTM
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.
LGTM, thanks for all the refactoring!
@tbursztyka : Your comments also should have been addressed, can you please have a look?
Support sendto() and recvfrom() for datagram socket.