-
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
conn: Simplify and overhaul conn API #5533
Conversation
b974bc9
to
2629544
Compare
* @return pointer to an IPv6 address configured on an interface with the best | ||
* match to @p dst | ||
*/ | ||
ipv6_addr_t *conn_find_best_source(const ipv6_addr_t *dst); |
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 stated in the description: the new implicit binding capabilities of the API make this function unnecessary.
2629544
to
ba6852c
Compare
I also provided a small simple application for |
conn_ep_udp_t client_address; | ||
int res; | ||
|
||
msg_receive(&msg); /* wait for message */ |
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.
For none-asynchronous handling of incoming messages one could just start from the code from 88 here and remove the surrounding switch-case.
ba6852c
to
55d9f57
Compare
IMHO that can be a little more straight forward. pls compare this. |
Yes, but it isn't asynchronous. As stated in #5533 (comment) this is also possible with this API. Here's your example (and pretty much the synchronous version without shell of my example) with my API (and RIOT-fied). #include <errno.h>
#include <stdio.h>
#include <string.h>
#include "net/af.h"
#include "net/conn/udp.h"
int main(void)
{
char buf[128];
conn_udp_t conn;
conn_ep_udp_t local = { .family=AF_INET6, .port=60001 };
conn_ep_udp_t remote = { 0 };
int res = conn_udp_create(&conn, &local, NULL);
if (res < 0) {
return -1;
}
while(1) {
res = conn_udp_recv(&conn, buf, sizeof(buf), &remote);
if (res < 0) {
errno = -res; perror("recv");
return 0;
}
else {
buf[res] = '\0';
printf("res=%zu text=\"%s\"\n", res, buf);
conn_udp_send(&conn, buf, res, &remote);
}
}
return 0;
} Bahm, one line less ;-P (okay, I cheated with the |
try the other side (the echo client).
Yep, and I like it a lot better. Feels like you took my implementation, "s/sock_/conn_/", then added the callback. ;) Let's find the optimum. I'll concentrate on UDP for now.
Why do you think that? Some thoughts:
e.g., remove it from the
Not sure the last two can be expressed with this PR's API.
That means for all but server uses, there'd be a NULL argument, which seems clumsy / redundant. I'm not entirely happy with any of those options.
What do you think?
|
Did not actually compile this one, but this should do it (given the server's address is "abcd::1"): #include <errno.h>
#include <stdio.h>
#include <string.h>
#include "net/af.h"
#include "net/conn/udp.h"
int main(void)
{
char buf[] = "Hello!";
conn_udp_t conn;
conn_ep_udp_t remote = { .family=AF_INET6, port=60001 };
ipv6_addr_from_str(&remote.addr.ipv6, "abcd::1");
int res = conn_udp_create(&conn, NULL, NULL);
if (res < 0) {
return -1;
}
res = conn_udp_send(&conn, buf, sizeof(buf), &remote);
if (res < 0) {
errno = -res; perror("send");
return -1;
}
res = conn_udp_recv(&conn, buf, sizeof(buf), &remote);
if (res < 0) {
errno = -res; perror("recv");
return 0;
}
else {
buf[res] = '\0';
printf("res=%zu text=\"%s\"\n", res, buf);
}
return 0;
}
Because due to send always coming with some state, we don't have to maintain a list of all states to successfully send on stacks that come with their own port management.
Yupp, just realized that when trying to port this interface to GNRC: even with #5526 I need to allocate three pointers for callback support (one for the GNRC-internal callback, one for the context, and one for the user-owned
Point 3 is simple (again no guarantee for compilability ;-)): #include <errno.h>
#include <stdio.h>
#include <string.h>
#include "net/af.h"
#include "net/conn/udp.h"
int main(void)
{
char buf[] = "Hello!";
conn_udp_t conn;
conn_ep_udp_t remote = { .family=AF_INET6, port=60001 };
ipv6_addr_from_str(&remote.addr.ipv6, "abcd::1");
res = conn_udp_send(NULL, buf, sizeof(buf), &remote);
if (res < 0) {
errno = -res; perror("send");
return -1;
}
return 0;
} For 2. I see that it might be a nice-to-have feature, but in the end UDP just isn't working this way. I always found it baffling that you can call #include <errno.h>
#include <stdio.h>
#include <string.h>
#include "net/af.h"
#include "net/conn/udp.h"
int main(void)
{
char buf[] = "Hello!";
conn_udp_t conn;
conn_ep_udp_t local = { .family=AF_INET6, port=60001 };
conn_ep_udp_t remote = { .family=AF_INET6, port=60001 };
ipv6_addr_from_str(&remote.addr.ipv6, "abcd::1");
int res = conn_udp_create(&conn, &local, NULL);
if (res < 0) {
return -1;
}
res = conn_udp_send(&conn, buf, sizeof(buf), &remote);
if (res < 0) {
errno = -res; perror("send");
return -1;
}
return 0;
} I'm not sure adding
Only if
Sure, timeouts are needed, but I don't want to pull-in #include <errno.h>
#include <stdio.h>
#include <string.h>
#include "net/af.h"
#include "net/conn/udp.h"
#include "xtimer.h"
#define RECV (0x9999)
void callback(conn_udp_t *conn)
{
msg_t msg = { .type = RECV, .content = { .ptr = conn } };
msg_send(&msg. handler_pid);
}
void *handler(void *)
{
char buf[128];
msg_t msg;
conn_udp_t conn;
conn_ep_udp_t local = { .family=AF_INET6, port=60001 };
conn_ep_udp_t remote;
int res = conn_udp_create(&conn, &local, callback);
if (res < 0) {
return -1;
}
if (xtimer_msg_receive_timeout(&msg, 1000) < 0) {
puts("timeout");
return -1;
}
if (msg.type == RECV) {
res = conn_udp_recv(msg.content.ptr, buf, sizeof(buf), &remote);
if (res < 0) {
errno = -res; perror("send");
return -1;
}
}
return 0;
} The code part from the |
(edited away some errors in the last example) |
Made callbacks optional, not by removing the parameter though. It is still set by parameter, but a macro ( Also updated some doc. |
I don't buy that these stacks are that inflexible. Any of those that
I see, this PR just doesn't support this. Well, posix sockets do, and if
That alone clearly saves copying the remote address & port for every packet sent.
Let's not rate simplicity, cleanliness and comprehensiblity yet, too
Some way of setting a connection object's remote is needed, see above.
Personally I'd consider not setting a timeout a bug, but that aside:
There are perfectly valid, simple programs that need timeouts, but for The example (using xtimer_msg_receive_timeout) has so many conceptual Also, please consider that the API might be useful on other OSs, without |
How many are there?
2a and 2b are identical. |
|
Need to research this, but you might be right, at least for lwIP.
You might have a point with this (as well with the
Let me think about this, too ;-) |
e708560
to
3669d98
Compare
Okay, thought about it, and changed it. Now I'm thinking about also adding a Here are the examples for the three use-cases now \1. server handling multiple clients int main(void)
{
char buf[128];
conn_udp_t conn;
conn_ep_udp_t local = { .family=AF_INET6, .port=60001 };
conn_ep_udp_t remote = { 0 };
int res = conn_udp_create(&conn, &local, NULL, NULL);
if (res < 0) {
return -1;
}
while(1) {
res = conn_udp_recvfrom(&conn, buf, sizeof(buf), &remote, 0);
if (res < 0) {
errno = -res; perror("recvfrom");
return 0;
}
else {
buf[res] = '\0';
printf("res=%zu text=\"%s\"\n", res, buf);
conn_udp_sendto(&conn, buf, res, &remote);
}
}
return 0;
} \2. p2p connection #include <errno.h>
#include <stdio.h>
#include <string.h>
#include "net/af.h"
#include "net/conn/udp.h"
int main(void)
{
char buf[] = "Hello!";
conn_udp_t conn;
conn_ep_udp_t local = { .family=AF_INET6, port=60001 };
conn_ep_udp_t remote = { .family=AF_INET6, port=60001 };
ipv6_addr_from_str(&remote.addr.ipv6, "abcd::1");
int res = conn_udp_create(&conn, &local, &remote, NULL);
if (res < 0) {
return -1;
}
res = conn_udp_send(&conn, buf, sizeof(buf));
if (res < 0) {
errno = -res; perror("send");
return -1;
}
return 0;
} \3. client with arbitrary local endpoint #include <errno.h>
#include <stdio.h>
#include <string.h>
#include "net/af.h"
#include "net/conn/udp.h"
int main(void)
{
char buf[] = "Hello!";
conn_udp_t conn;
conn_ep_udp_t remote = { .family=AF_INET6, port=60001 };
ipv6_addr_from_str(&remote.addr.ipv6, "abcd::1");
res = conn_udp_send(NULL, buf, sizeof(buf), &remote);
if (res < 0) {
errno = -res; perror("send");
return -1;
}
return 0;
} |
Okay, thought about it, and changed it. Now |_create()| of all
connectivity types also receives a |dst| argument and
|_recv()|/|_recvfrom()| (the latter was introduced together with
|_sendto()| to differentiate from the functions without end point
required) got an (optional) timeout parameter.
I'm thinking about also adding a |_set_local()| and |_set_remote()|
function.
Nice. Now with UDP this PR are basically equivalent to [1] which I
proposed months ago...
Still good to converge. ;)
And still, IMHO the callback parameter must go, as we (currently) cannot
execute the callback in the same thread's context.
Are you very tied to the name conn? For IP it doesn't make sense at all,
for UDP just so-so, only with TCP it fits.
[1] https://github.com/kaspar030/sock/blob/master/sock_udp.h
|
Says the one criticizing me for sticking to close to sockets ;-P. I like |
I don't see that as a blocker... And I want the callback support! |
Is there ever a way to execute callbacks in a running thread's context? That sounds really messy. I want callbacks to finally bring the much requested |
Asynchronously? Probably not. That's even supermessy on Posix.
How about a simple event loop?
I want asynchronous conn, too! ;) Let's try to make it somewhat nice. |
This is a lot to read through and think about for one night being absent from github ;-). More detail reply will come in, but for now a small factual correction: @kaspar030 wrote
Neither does By now, I'm also thinking that some kind of external event loop might be the better solution. |
Okay let me summarize the current discussion (and give my rebuttal where I didn't already):
|
Does this overall reflect the opinions stated above? Regarding F2F meeting agreed. When do you have time for that in the next days? |
@miri64
In my opinion it doesn't matter, I personally will still have to look it up in the manual every time I use it regardless if it's in-out or out-in. |
Oh one thing I forgot:
|
note: edited the post (in case someone follows discussion via mail) ;-) |
Some example for this kind of include-loop: Let's say I want to define |
7f575f3
to
84a2100
Compare
I made the parameter change, and rebased and squashed (the latter because I think the commit history does not much contribute to the discussion). |
This change overhauls the `conn` API in the following manner: * The source address on creation may now be `NULL`, implying an implicit bind when sending. This makes the `conn_find_best_source()` function unnecessary. * a common address type for both IPv4 and IPv6 addresses was introduced * instead of having addresses, address length and ports/protocols given separately a new data type called "end points" for every connectivity type is introduced `conn_ep_x_t`, consisting of the address, its family, an (optional) interface identifier (see RIOT-OS#5111) and its port/protocol * send for connection-less communication was simplified by not requiring source information anymore (a stack is encouraged to find a best match itself). It and the interface one is supposed to send over can however be supplied by (optionally) providing an already created `conn` object * TCP connection establishment was simplified: listen/connect functions were dropped, instead a user can either give the remote end point on creation (implicit connect) or omit it (implicit listen)
84a2100
to
b808249
Compare
Updated doc based on some lessons learned from trying to implement it (which is really fun on both sides of the API now! :D). Will provide the backend, when some dependencies got merged in. Otherwise this PR will be even more hard to read for people coming in ;-) |
Sorry for the late reply. Sometime next week would be fine. |
How about Wednesday? I will write a mail to devel to get some other devs involved, too. |
Perfect! |
I just finished my examplary GNRC port for |
Without having read the whole discussion, is this still a Point ? " TCP connection establishment was simplified: listen/connect functions were dropped, instead a user can either give the remote end point on creation (implicit connect) or omit it (implicit listen)" Is it still up to debate? This would remove the possiblity to allow only specific connections that came from a specified IP-Address. Oh and I would like to seen the listen call prototype changed to: int conn_tcp_listen(conn_tcp_t *conn, conn_tcp_t queue[], int queue_len); |
@brummer-simon are you available tomorrow? If yes, let's discuss this at the meeting. |
I don't understand that comment. Do you mean that listen and connect would be called on the same socket (with POSIX sockets)? |
Nope but in posix, if you call listen with a specified address in the connection defining socket, the passive connection structures will only accept connection requests from peers that connect that specific address and port combination. If you call listen with an unspecified address, only the port number is important. If you remove listen() and use only conn_create, you lose this option because you get rid of the connection specifing structure thats needed for the listen call. |
Oh and something else:
Your |
Ah... Okay then lets discuss this tomorrow, how to handle this. |
Yes i am sure. Every queue element handles a full tcp connection. The data structure have no shared subset (except destination port number maybe). In simpelst scenario where only a single TCP connection is handled at a time, the queue has a size of one element. About my availablity tomorrow: I'll try to join the mumble session, but I can't promise that i'll be there. |
Closed in favor for #5533. |
This change (as discussed in #5091) overhauls the
conn
API in the following manner:conn
objects now also one can add a callback for asynchronousreception on creation
NULL
, implying an implicit bindwhen sending. This makes the
conn_find_best_source()
function unnecessary.separately a new data type called "end points" for every connectivity type is
introduced
conn_ep_x_t
, consisting of the address, its family, an (optional)interface identifier (see
pkg/oonf_api: use new repo url #5111netif: initial import of a common network interface API #5511) and its port/protocolinformation anymore (a stack is encouraged to find a best match itself).
It and the interface one is supposed to send over can however be supplied by
(optionally) providing an already created
conn
objectdropped, instead a user can either give the remote end point on creation
(implicit connect) or omit it (implicit listen)
For actual porting this API change needs
#5111#5511 for interface. #5526 would be helpful for efficient porting to GNRC, but it isn't required (one can easily do the same with a dispatching thread).Some doc changes from #5509 are also included in this PR.