conn: Reworking the conn API

[miri64]

Motivation

After I worked a little bit with other stacks and also had some feedback from the community I would like to propose a change to the conn API, especially to the connection-less part i.e. conn_ip and conn_udp (there is no implementation of conn_tcp yet anyway to my knowledge). Some of these changes are based on already existing PRs (#3921, #4630, #4631).

The API

As an example I use conn_udp, since it is pretty similar to conn_ip (just replace uint16_t port with uint8_t proto ;-)):

#include "net/af.h"

typedef struct conn_udp conn_udp_t;
typedef void (*conn_udp_recv_cb_t)(conn_t *conn, void *data, size_t data_len,
                                   void *addr, uint16_t port);

int conn_udp_create(conn_udp_t *conn, af_t family,
                    conn_udp_recv_cb_t *recv_cb);
int conn_udp_bind(conn_udp_t *conn, void *addr, uint16_t port);
int conn_udp_getlocaladdr(conn_udp_t *conn, void *addr, uint16_t *port);
int conn_udp_recvfrom(conn_udp_t *conn, void *data, size_t data_len,
                      void *addr, uint16_t *port);
int conn_udp_sendto(conn_udp_t *conn, void *data, size_t data_len,
                    void *addr, uint16_t port);

Most of the stuff should be pretty self explanatory so I did not add doxygen for now (Just ask questions if anything is unclear).

Overview over changes

The major changes are:

1. addr_len parameters were removed.
2. conn_udp_create() receives a new callback parameter (an idea already proposed in conn: provide API support for asynchronous IO #4631)
3. conn_udp_bind() is introduced (no implicit bind through conn_udp_create() anymore)
4. conn_udp_sendto() needs a conn parameter now (as introduced in conn: make conn_*_sendto require a conn object #4630)

Discussion

Removal of address length parameters

• For both UDP and IP the address family given to conn_udp_create() should be enough to identify the address length

Callback parameter on connection creation

• Primary reasoning is to allow for asynchronous receiving of packets (and as such implementation of functionalities like select() or epoll() in the POSIX layer)
• A timeout for reception (as proposed in conn: extend API with timeout support #3921) can also be implemented with such a callback. The advantage would be that this would be externally to this API, keeping this API thin and independent from other modules like xtimer

No implicit bind on creation

• A bind to an address is not always needed or desired (for example to send) so forcing it on creation is undesirable

conn parameter on sendto

• simplifies server implementation (see example given in conn: make conn_*_sendto require a conn object #4630)
• most stack implementation as emb6 or lwIP require some kind connection object for sending
  • following from this you need to create a connection in sendto (making this API not thin anymore)
  • both emb6 and lwIP check if a port is already open => sending and receiving from the same port is impossible in the current conn API with those stacks
  • both stack and users are able keep track of open "connections"

[OlegHahm]

Personally, I prefer to use UDP over netapi compared to conn_udp so far. In the best/simplest case IMO we shouldn't need anything more than

conn_send(dest, data, len);
conn_register_receive_cb(source_port, buffer, maxlen);

or something similar.

[miri64]

Personally, I prefer to use UDP over netapi compared to conn_udp so far.

Any particular reason why, except conn's lacking maturaty?

In the best/simplest case IMO we shouldn't need anything more than [this …] or something similar

The problem is, that this API should (at least in my opinion) be thin, but also applicable for most (if not all) stacks we want to support. So doing this without any state seems to me pretty unrealistic to me (I'm not sure if you left the conn objects out for brevity or because you actually don't want them). Your scenario is already possible (with a little bit of reordering) with the API proposed:

conn_udp_t conn;
conn_udp_create(&conn, AF_INET6, recv_cb);
conn_udp_bind(&conn, &unspec, sport);
conn_udp_sendto(&conn, data, len, dest, dport);

For savvy developers like you using gnrc_netapi alternatively for some special extra-thin GNRC mumbo-jumbo is always an option, too of course.
Maybe to make it even easier at least usage-wise (I find the allocation of the headers always quite strenuous), a layer in-between conn and gnrc_netapi would also make sense (the other stacks provide this something similar too).

[OlegHahm]

Personally, I prefer to use UDP over netapi compared to conn_udp so far.

Any particular reason why, except conn's lacking maturaty?

I just found the API more natural. I want to send -> netapi_send. I want to receive something -> register for this or that traffic. In conn I would have to create a conn object first, specify some source information even for sending (which might or might not be NULL) etc. Just seemed overly complicated at a first glance. I've never dug deeper into this, but always found netapi more comfortable.

[kaspar030]

Here's what I came up with:

typedef struct udp_endpoint6 {
    uint16_t port;
    uint16_t iface;
    ipv6_addr_t *addr;
} udp_endpoint6_t;

typedef struct udp_endpoint4 {
    uint16_t port;
    uint16_t iface;
    ipv4_addr_t addr;
} udp_endpoint4_t;

typedef struct udp_endpoint {
    unsigned family;
    union {
#if defined(SOCK_UDP_IPV6)
        udp_endpoint6_t ipv6;
#endif
#if defined(SOCK_UDP_IPV4)
        udp_endpoint4_t ipv4;
#endif
    };
} udp_endpoint_t;

typedef struct sock_udp sock_udp_t;

int sock_udp_init(sock_udp_t *sock, const udp_endpoint_t *local, const udp_endpoint_t *remote);
ssize_t sock_udp_sendto(const udp_endpoint_t *dst, const void* data, size_t len, uint16_t src_port);
int sock_udp_set_dst(sock_udp_t *sock, const udp_endpoint_t *dst);
ssize_t sock_udp_send(sock_udp_t *sock, const void* data, size_t len);
ssize_t sock_udp_recv(sock_udp_t *sock, void* buf, size_t len, unsigned timeout, udp_endpoint_t *remote);
void sock_udp_close(sock_udp_t *sock);

(missing asynchronous stuff).

I did it another way around: I started writing examples using the API, then implemented it on top of POSIX.
One thing that came out is a fully functional DHCPv4 client, which I claim is the smallest usable one available for Linux.

The API is very minimal, sending is possible without creating an object.
The DHCP client uses a total of 5 lines of network code.

https://github.com/kaspar030/sock

[kaspar030]

For sending, no object should be needed.

And, there is no need for a seperate bind.

[miri64]

For sending, no object should be needed.

Then look into my lwip_conn_udp, please and tell me how I prevent the need for it.

And, there is no need for a seperate bind.

Why?

[OlegHahm]

Then look into my lwip_conn_udp, please and tell me how I prevent the need for it.

I think we should be clear about the goal: a) the simplest possible API or b) something that covers all corner cases? If it is b) I'm not really interested in this topic. IMO this can and should be covered by some adaptation layer, well hidden from the user at the application level.

[miri64]

If an API needs an adaption layer to implement it for something underlying it is not simple (in the sense of complexity, not usability) any more IMHO. This API should be simple in both complexity (to have our constraints in check) and usability (to make it appealing to users) terms IMHO.

[kaspar030]

For sending, no object should be needed.

Then look into my lwip_conn_udp, please and tell me how I prevent the need for it.

Why should it be needed? If the specified "src_port" is bound without sth like "O_REUSEADDR", the send fails with an error.

And, there is no need for a seperate bind.

Why?

Why is it needed?

[kaspar030]

If an API needs an adaption layer to implement it for something underlying it is not simple (in the sense of complexity, not usability)

I'm with Oleg here, we should make the API simple. The whole point here is to get rid of the useless complexity of berkley sockets, and get something that is easiy and fun to use while still being usable for more complex applications.

If a stack doesn't support the resulting API (none will), the implementation of our API for that stack_is_ an adaption layer to the network stack's native interface / semantics.

[miri64]

Why should it be needed? If the specified "src_port" is bound without sth like "O_REUSEADDR", the send fails with an error.

Because real world protocols have defined ports. If I can't do this

c = conn_create(DNS_PORT);
while (1) {
    sport = DNS_PORT;
    conn_recvfrom(&c, &dst, &dport); // dport will become DNS_PORT
    conn_sendto(data, src, sport, dst, dport);
}

The API is not ready for real-world usage. With the current conn this is not possible with lwIP and emb6 because they keep track of open ports and their connections. This means if I create a emb6-internal/lwIP-internal connection in sendto I get an error, because the port is already bound to c

And, there is no need for a seperate bind.

Why?

Why is it needed?

I explained my reasoning in OP. True, it is based on the argument, that sendto needs a state, but I did not get any valid argument against that, except that you want to send packets with as less code as possible with a particular stack.

[OlegHahm]

but I did not get any valid argument against that, except that you want to send packets with as less code as possible with a particular stack.

IMO that's the most important point of it all: provide a really light-weight, usable API without any might-be-nice-to-have-most-of-the-time-optional parameters.

[miri64]

If an API needs an adaption layer to implement it for something underlying it is not simple (in the sense of complexity, not usability)

I'm with Oleg here, we should make the API simple. The whole point here is to get rid of the useless complexity of berkley sockets, and get something that is easiy and fun to use while still being usable for more complex applications.

If a stack doesn't support the resulting API (none will), the implementation of our API for that stackis an adaption layer to the network stack's native interface / semantics.

I think we are talking about two different things here: you want to have an easy to use user API, I want an easy to port stack-to-os API. Your user API can be easily ported on top of my API.

The thing is this: we currently have two stacks that require an adaption layer (that would look very similar) and one that doesn't (sorry Kaspar not counting yours until I see any code). Which would be easily fixed by making this API a fun to port API and providing some user API on top.

[OlegHahm]

Ok, if fthe thread is about "an easy to port stack-to-os API", I'm out here. Sorry for the misunderstanding.

[miri64]

but I did not get any valid argument against that, except that you want to send packets with as less code as possible with a particular stack.
IMO that's the most important point of it all: provide a really light-weight, usable API without any might-be-nice-to-have-most-of-the-time-optional parameters.

That's exactly the point I'm talking about: conn isn't optional most of the times for sending, especially when talking about scenarios were a node is e.g. a CoAP server.

[kaspar030]

The API is not ready for real-world usage. With the current conn this is not possible with lwIP and emb6 because they keep track of open ports and their connections.

That is clearly a problem with those stacks, not of the API.

I did not get any valid argument against that, except that you want to send packets with as less code as possible with a particular stack.

No, I want to send packets with as less code as possible with any stack. The API doesn't need the bind, there's no concept where the seperate bind is needed.

[miri64]

Ok, if fthe thread is about "an easy to port stack-to-os API", I'm out here. Sorry for the misunderstanding.

Would be nice to have something that can serve both so we don't stack abstraction over abstraction.

[OlegHahm]

That will always require some compromises, I'm not willing to make here.

[miri64]

That's kind of the point of a discussion...

[miri64]

The API is not ready for real-world usage. With the current conn this is not possible with lwIP and emb6 because they keep track of open ports and their connections.

That is clearly a problem with those stacks, not of the API.

Curious then, that members of the opposition of my proposal then are requesting the same kind of behaviour for GNRC: #4387 ;-). They are not broken, their history just lies in TCP (which is why I will refer to them as TCP-based stack and stacks like GNRC as UDP-based stacks)

Seriously, I don't see any point in a lightweight abstraction API that makes it harder to port for a popular stack than just porting sockets...

I see however a possibility that is quiet convoluted, to be honest, but a compromise it is: my kind of API seems to be a good solution to port TCP-based stacks, while your solution seems to be a good solution to implement UDP-based stacks. However with a little bit of overhead they are pretty much interchangeable (you can implement my solution with your solution and vice-versa). How about we port the stack with the API that it is best suited for and provide a conversion layer for each API. Your API can then be used by users, while my API can be used to port sockets, libraries or other stuff that requires a state for the UDP session (e.g. servers).

[miri64]

Another reason for a state variable came to my mind today: connection options like Traffic Class, Flow Label and hop-limit. Or do you want to add them as parameters, too? What are the values for default values then? Why do I have to add them to every send when at least flow label and traffic class are most likely not to change?

[kaspar030]

@authmillenon I think we agree on the necessity of a state variable. Just some functions don't need it, e.g., "sock_udp_sendto()" as long as there's "sock_udp_send(state, ...)".

[miri64]

But then why does conn_udp_sendto() exist? My current analysis is:

• It makes the implementation of the API more complicated. Yes, for our stacks its easy, but we shouldn't take our stacks as the norm. When it easier to just implement sockets with them (or just take the existing socket implementation), why should I make the effort and implement it at all? Just to make the RIOT community and their weirds constraints and requirements happy? Well I guess then I just don't port my stack for RIOT.
• It makes the API itself more complicated. Given that we take options into account for the API it produces a special case: You can use this function only if you want to send with default options and only if you are sure no other connection object exists already with this source configuration.

I'm not saying it isn't nice to have a function to send UDP packets without any state, because in some cases this is really, really comfortable, but in most cases you have some kind of state - a connection - even if this is just the same UDP port you use constantly to send something. Regardless, I'm not sure a low-level API is the right place for that. And I stress again, that that is what conn should be first and foremost: A low-level API for generalized stack access (if it makes fun to program with it, that's a bonus, but I would not count it as THE top priority).

[kaspar030]

It makes the implementation of the API more complicated.

That is not true, as if there's `udp_send(state, ...)andudp_sendto(...)`` (without state), the latter can be implemented as two-liner.

It makes the API itself more complicated.

That depends on the definition of "complicated" with the context of APIs.

Given that we take options into account for the API it produces a special case: You can use this function only if you want to send with default options

Well, I remember having to set the hop limit of packets once when hand-crafting multicast. Never used traffic class or flow labels other than for hacky stuff. So I must say, I'm very perfectly fine with a UDP send function that takes buffer, target IP/port and source port and does what I want. I would also argue that this send function is enough to write 99.9% of all UDP applications.

For all the others, there's the function that binds to state.

and only if you are sure no other connection object exists already with this source configuration.

Why? Not allowing a send when an application requests it is a policy decision of a network stack. It is perfectly valid to return EACCES if that source is already bound.

When it easier to just implement sockets with them (or just take the existing socket implementation), why should I make the effort and implement it at all? Just to make the RIOT community and their weirds constraints and requirements happy?

Well, my goal is to end up with a socket API that is easy to use, powerful enough for everything and at the same time efficient, so it becomes a pleasure to write network applications. The sheer quantity of high quality code written against that API will make every network stack developer want to have that API for his/her stack.

What I really don't want is another network API that sucks to use. Why should I invest effort in developing that (apart from the fact that we already have three of them)?

[miri64]

It makes the implementation of the API more complicated.

That is not true, as if there's udp_send(state, ...) and udp_sendto(...)` (without state), the latter can be implemented as two-liner.

These are no two-liners [1] [2] [3]. And in the case of lwIP and emb6 they are not even considering the fact yet, that sending from a source port that is already bound by a conn isn't possible.

and only if you are sure no other connection object exists already with this source configuration.

Why? Not allowing a send when an application requests it is a policy decision of a network stack. It is perfectly valid to return EACCES if that source is already bound.

Now I at least know you are not even reading a word I'm writing.... That this is a problem the basis of my whole argument! Because it makes server implementations plain impossible (or at least very hard to do) this way! When I listen on a connection we need to do this on a port. This port is usually expected to be the source port of a reply. But when the conn object with the reply is already listening one either has to disconnect from the conn first before sending with "your" sendto (please don't call "my" sendto send... its confusing because it implies that a connect() before-hand for me which is not what I'm talking about...) or the API needs to store all allocated connection objects and use an already allocated connection for sending somehow, if one exist. This is way more complicated!

Yes, with GNRC and your nano-stack "your" way is possible and actually easy to implement because we (currently) don't care if there is already a port open on the same address by different applications, but not with the other 99.99999% of stacks that are out there, because they were written with TCP in mind first (were having multiple applications using the same source port/listening port is a very stupid thing to do) and thus even their UDP implementations are in a bad mood as soon as you try to allocate a port multiple times.

When it easier to just implement sockets with them (or just take the existing socket implementation), why should I make the effort and implement it at all? Just to make the RIOT community and their weirds constraints and requirements happy?

Well, my goal is to end up with a socket API that is easy to use, powerful enough for everything and at the same time efficient, so it becomes a pleasure to write network applications. The sheer quantity of high quality code written against that API will make every network stack developer want to have that API for his/her stack.

What I really don't want is another network API that sucks to use. Why should I invest effort in developing that (apart from the fact that we already have three of them)?

You are throwing huge blocks into the way of an API developer for those stacks, because they need to implement a whole adaption layer to make this API usable, which is supposed to be a thin and common adaption layer for OS/application<>stack communtication. Compare that to 2 lines of code more (that you need for at least 50% of applications anyway, even with your proposal) + a few parameters more (wait… in my solution they are actually less.... [edit: miscounted]) in the application code [4]. I think that is a very fair trade-off and still time-efficient and fun to program, but you don't seem even one bit interested in trying to find a compromise here.

[1] https://github.com/RIOT-OS/RIOT/blob/master/sys/net/gnrc/conn/udp/gnrc_conn_udp.c#L92-L135
[2] https://github.com/RIOT-OS/RIOT/pull/3551/files#diff-5f36276932c1cd3ccbfe9c6ac97507c5R64
[3] https://github.com/RIOT-OS/RIOT/pull/4713/files#diff-fb9a5166d6a9efcd8278e2efcc187146R160
[4]

conn_t conn;
conn_udp_endpoint_t dst = { .addr = { .i6 = addr }, .port = port }; /* iface should be set via option */
conn_udp_create(&conn, AF_INET6, NULL);
conn_udp_sendto(&conn, data, data_len, &dst);

/* vs. */
udp_endpoint6_t dst = { .port = port, .iface = ANY, .addr = addr };
sock_udp_sendto(&dst, data, data_len, (uint16_t)random_getuint32());

[miri64]

I think I found a suitable compromise!

Let's revisit: A user generally has two use-cases for sending out datagrams: either they expect a reply or not. If they expect don't expect a reply, the content of source address and port is basically unimportant to them so it can as well be picked be picked by the stack (at random or by some internal measure). If they expect a reply they have to create a connectivity object anyway to wait for said reply.

So my proposed compromise is: go with the prototype for conn_udp_sendto() as I proposed

int conn_udp_sendto(conn_udp_t *conn, void *data, size_t data_len,
                    conn_udp_endpoint_t *dst);

but let conn be allowed to be NULL or unbound. This way sporadic sending is still possible, as you @kaspar030 and @OlegHahm were asking for.

[miri64]

At least lwIP and emb6 should be able to handle such cases and GNRC anyways.

[OlegHahm]

Seems to be a step into the right direction.

[kaspar030]

@miri64 I still think the conn proposal is tackling the issue from the wrong direction.

Did you try writing hypothetical simple and more complex applications against the API (without it being finished or implemented)? That way you get a grip on what you need and what is cumbersome.

It seems to me that you a) try to keep it similar to sockets and b) try to make it easily implementable on a variety of network stacks.

[miri64]

That might be, because I want it to be (in exactly that order of importance):

1. Be as thin as possible wrapper around a variety of stacks,
2. Be simple and intuitive to use,
3. Capable of adding an equally thin application library on top (if need be),
4. Capable of implementing sockets with it.

This is the order we should go IMHO, because otherwise we will invent the wheel over and over again, every time a new stack gets integrated. The API as I propose is now very similar to your approach (now that I have this solution we might not even need bind(), as NULL is now an unbound conn), so I really don't see why you think I try emulate sockets.

[miri64]

Here is how the API would look like in total btw with conn_udp as an example:

typedef struct conn_udp conn_udp_t;

typedef union {
    ipv6_addr_t ipv6;
    ipv4_addr_t ipv4;
} conn_addr_t;

typedef struct {
    conn_addr_t addr;
    int family;
    uint16_t port;
} conn_udp_ep_t; // ep == end point

typedef void (*conn_udp_recv_cb_t)(conn_udp_t *conn, void *data, size_t data_len,
                                   conn_udp_ep_t *src);

// recv_cb may be NULL
int conn_udp_create(conn_udp_t *conn, // edit: removed superfluous `family` parameter
                    const conn_udp_ep_t *ep,
                    conn_udp_recv_cb_t *recv_cb);
int conn_udp_get_local(conn_udp_t *conn, const conn_udp_ep_t *ep);
int conn_udp_recv(conn_udp_t *conn, void *data, size_t data_len,
                  conn_udp_ep_t *src);
// conn may be NULL
int conn_udp_send(conn_udp_t *conn, void *data, size_t data_len,
                  conn_udp_ep_t *dst);

Given the two use-cases I described (which are basically a simple and a more complex implementation as you asked for) I would implement them as such with this API:

Periodic beacon (simple):

conn_udp_ep_t all = { { ALL_NODES }, AF_INET6, 1337 };
char data[] = "wuff";

while (1) {
    conn_udp_send(NULL, data, sizeof(data), &all);
    xtimer_sleep(TIME);
}

DNS server (more complex):

void recv_cb(conn_udp_t *conn, void *data, size_t data_len,
                  conn_udp_ep_t *src) {
    // do DNS stuff
    // send reply
    conn_udp_send(conn, reply, reply_len, src);
}

conn_udp_ep_t addr = { { ANY }, AF_INET6, 53 };
conn_udp_t conn;
uint8_t buf[BUFSIZE];

conn_create(&conn, &addr, recv_cb);

Hit me with some challenges, because this wasn't cumbersome or tedious at all ;-).

[miri64]

Two additional operations to manipulate state in the future (TTL, interface to send over, etc) I also would add are

conn_udp_set(conn_udp_t *conn, conn_opt_t opt, void *val, size_t len);
conn_udp_get(conn_udp_t *conn, conn_opt_t opt, void *val, size_t maxlen);

conn_opt_t might be interchangable with netopt_t I'm not sure....

[miri64]

As soon as we have a common interface API conn_udp_ep_t might even get a interface field, so sending over a specific interface wouldn't even be that complicated anymore.

[miri64]

Please have a look at 928593f for some actual header files.

[miri64]

c670e29 includes interfaces as defined in #5511 similar as you did it in your proposal.

[miri64]

I proposed the API as a PR in #5533.

[miri64]

Sock (the result of that discussion) was merged in #5758.