sock: Introduction of new application layer API by miri64 · Pull Request #5758 · RIOT-OS/RIOT

miri64 · 2016-08-18T11:18:30Z

This introduces a new alternative and better API to conn. It differs in the
following aspects:

a common address type for both IPv4 and IPv6 addresses is introduced
communication end-points are abstracted as end-point types sock_x_ep_t,
containing the address, its family, its port (if required for protocol) and
the interface identifier.
All functions require some kind of state. Sending of datagrams to the same
source or replying to incoming datagrams is thus simplified
TCP connection establishment was overall reworked: connected sockets and
listening sockets are now two distinct types. An accept on a listening socket
than yields a connected socket

This introduces a new alternative and better API to `conn`. It differs in the following aspects: * a common address type for both IPv4 and IPv6 addresses is introduced * communication end-points are abstracted as end-point types `sock_x_ep_t`, containing the address, its family, its port (if required for protocol) and the interface identifier. * All functions require some kind of state. Sending of datagrams to the same source or replying to incoming datagrams is thus simplified * TCP connection establishment was overall reworked: connected sockets and listening sockets are now two distinct types. An accept on a listening socket than yields a connected socket

kaspar030 · 2016-08-18T14:32:45Z

sys/include/net/sock/tcp.h

+ */
+typedef struct {
+    sock_addr_ip_t addr;    /**< IP address */
+    int family;             /**< family of sock_tcp_ep_t::addr */


Any reason why this is an int? (maybe we can carve off two bytes here)

At least on 32-bit platforms those would return anyways due to alignment. But I'm thinking of moving it to the top, that so that endpoints can be identified to a degree even after casting, if addr isn't an IP address, but e.g. a name for an ICN-based sock. And then int is the only type that would make sense, due to the alignment of the following array.

OlegHahm · 2016-08-18T22:33:34Z

To be nitpicky: the correct title of this PR should be: "sock: Introduction of a new transport layer API" since the API is usually described by the services that can be accessed through it, not by the services that are calling it.

kaspar030 · 2016-08-19T07:21:33Z

Some things:

Could we completely get rid of the ip address type? Like:

typedef struct {
    union {
        uint8_t ipv6[16];
        uint8_t ipv4[4];
    } addr;
    [..]
}

I'm afraid pre-defining the error return values to the values defined by errno.h will lead to a lot of code converting a network stack's result value to the errno ones.
Could we get rid of the #ifdef'ed includes and replace them with e.g., #include "sock_impl.h" and select the right one via include path?

kaspar030 · 2016-08-19T07:51:48Z

many of the functions should return ssize_t instead of int

OlegHahm · 2016-08-19T08:43:31Z

I'm afraid pre-defining the error return values to the values defined by errno.h will lead to a lot of code converting a network stack's result value to the errno ones.

What do you mean by converting a network stack's result value to the errno ones? What needs to be converted? Can you provide an example?

kaspar030 · 2016-08-19T09:55:02Z

What do you mean by converting a network stack's result value to the errno ones? What needs to be converted? Can you provide an example?

The API as is documents error return values like -ENOTCON, -EINVAL,
-ENOSPC, practically forcing the use of errno.h as they are usually
defined there.

Imagine a hypothetical implementation like

sock_udp_send(...) {
    [...]
    int res = _stack_udp_send(...);
    [...]
}

The network stack might return STACK_CONNECTION_NOT_SETUP defined to an
arbitrary value, instead of _ENOTCON, but the function needs to return
-ENOTCON to comply to the API.

I fear that implementations will have, at the end of every function of
the API it implements, a bunch of if/else/... or even switch statement
converting the underlying stack's return value to what this API predictates.

kaspar030 · 2016-08-19T12:19:25Z

Could we completely get rid of the ip address type?

@miri64 What do you think?

miri64 · 2016-08-19T13:26:41Z

@OlegHahm:

To be nitpicky: the correct title of this PR should be: "sock: Introduction of a new transport layer API" since the API is usually described by the services that can be accessed through it, not by the services that are calling it.

Then I can be nitpicky, too: raw IP isn't transport layer 😜 How about "network application API"?

@OlegHahm added the Waiting For Other PR label an hour ago

That's news to me... which PR are you referring to? #5511 is only important for 100% waterproof implementation of this API, but it certainly can also use stack-specific netif identifiers for now (which I did for GNRC for now, because otherwise the number of interdependent PRs will grow very large again ;-)).

@kaspar030 (sorry had to go to a meeting so I couldn't finish my replies):

Could we completely get rid of the ip address type?

In the scope of sock you mean? TBH I find it uglier to "redefine" a union in each end-point struct than just redefining the ipvx_addr_t structs.

If globally: I guess that will make a lot of people mad, that came to love the net/ipv6/addr.h header ;-).

I'm afraid pre-defining the error return values to the values defined by errno.h will lead to a lot of code converting a network stack's result value to the errno ones.

True, but worst-case this can be dealt with in a switch-case which shouldn't take as much memory. For our user-friendliness goal it is certainly more helpful to have a well-defined error behavior (because it simplifies debugging and testability "oh well I had an error, but I don't know what I did wrong exactly").

Could we get rid of the #ifdef'ed includes and replace them with e.g., #include "sock_impl.h" and select the right one via include path?

While I'm not a friend of wild include pathes, I tend to agree. Especially since this will definitely prevent some double inclusions that might happen due to faulty USEMODULE composition.

many of the functions should return ssize_t instead of int

Is this type available for all platforms. I remember some issues last time I tried to use it (way back when I implemented the first POSIX socket wrapper for DEStiny).

miri64 · 2016-08-19T13:27:42Z

Addressed comments that were straight forward ;-)

kaspar030 · 2016-08-19T13:33:04Z

Could we completely get rid of the ip address type?

In the scope of sock you mean? TBH I find it uglier to "redefine" a union in each end-point struct than just redefining the ipvx_addr_t structs.

Hm, if we remove the redundancy of the endpoint structs, it's not so
bad. And it would remove another ipv6 addr type.

I'm afraid pre-defining the error return values to the values defined by errno.h will lead to a lot of code converting a network stack's result value to the errno ones.

True, but worst-case this can be dealt with in a switch-case which shouldn't take as much memory. For our user-friendliness goal it is certainly more helpful to have a well-defined error behavior (because it simplifies debugging and testability "oh well I had an error, but I don't know what I did wrong exactly").
I had sock-specific error values in mind, which a stack would define.
Not sure if that would confuse users again... The switch-statements
would certainly enlarge code.

While I'm not a friend of wild include pathes, I tend to agree. Especially since this will definitely prevent some double inclusions that might happen due to faulty USEMODULE composition.
What's cleaner, "#include SOCK_IMPL_HEADER" or include path meddling?

many of the functions should return ssize_t instead of int

Is this type available for all platforms. I remember some issues last time I tried to use it (way back when I implemented the first POSIX socket wrapper for DEStiny).

I think it is. (quick grep shows avr and msp430 support, the rest has it
through newlib).

miri64 · 2016-08-19T13:46:07Z

Hm, if we remove the redundancy of the endpoint structs, it's not so
bad. And it would remove another ipv6 addr type.

Do you mean something like the following?

addr.h:

typedef struct {
    int family;
    union {
#ifdef SOCK_HAS_IPV6
        uint8_t ipv6[16];
#endif
        uint32_t ipv4;
    } addr;
} sock_addr_ip_t;

x.h:

typedef struct {
    sock_addr_ip_t addr;
    /* ... */
} sock_x_ep_t

Otherwise I don't know what you mean by "remov[ing] the redundancy of the endpoint structs".

I had sock-specific error values in mind, which a stack would define.
Not sure if that would confuse users again... The switch-statements
would certainly enlarge code.

And then those sock-specific errors need to be translated for the POSIX wrapper again :-/. I'm really not a fan of API-specific error values. They tend to produce exactly the problem you are describing: need for translating the errors in APIs that use the other API. True, if a stack also has its own error values we have the problem, too, but I see it way less problematic translating them one time here, than translating it for every abstraction layer.

What's cleaner, "#include SOCK_IMPL_HEADER" or include path meddling?

I dislike both equally :P But in this particular case I guess path meddling can help with the readability and maintainability of the API definition ;-).

I think it is. (quick grep shows avr and msp430 support, the rest has it
through newlib).

Okay great, then I will use it :-).

kaspar030 · 2016-08-19T13:49:33Z

And then those sock-specific errors need to be translated for the POSIX wrapper again :-/.

No, I mean, if the API saiy "SOCK_ENOSPC", than the sock implementation
header can "#define SOCK_ENOSPC ENOSPC". A different stack could define
"#define SOCK_ENOSPC 2342". No need to define the actual value, but
certainly important to define a symbolic name in the API.

miri64 · 2016-09-02T14:36:24Z

Mhhh, then EBADF might indeed be the better choice, since that is the only error in the POSIX specs that sounds similar to that.

brummer-simon · 2016-09-02T14:45:51Z

Hmm then i think about it:

Why not use -ENOTCONN as well. I mean the existing handle is not connected anymore to the original connection ...

miri64 · 2016-09-02T15:21:04Z

Would also work, yes. Do you really need that for disconnect as well?

miri64 · 2016-09-02T15:21:47Z

What I mean is: for disconnect I imagine it is easier to just fail gracefully (i.e. in case of an error: do nothing).

kaspar030 · 2016-09-02T15:25:59Z

A passive connection (sock_tcp_t that is part of listens() queue parameter), re-opens into listening mode after closing of the previous connection. As soon as the connection is in listening mode, a new connection can connect to the sock_tcp_t struct.

I think this doesn't pose a problem. A ptr to "sock_tcp_t" is the only handle an application gets. There's no (simple enough) way to find out if the underlying connection completely disconnected and is now used for another connection. I rather propose to always require an explicit "close()" issued by the application, even if the connection is already disconnected. Only after the explicit "close()" the stack can re-use that connection object for a new connection.

Include missing header

Add more error classes

miri64 · 2016-09-02T17:05:21Z

I found some more error classes myself while porting lwIP.

brummer-simon · 2016-09-03T06:43:07Z

@kaspar - That a good Idea, then only read() and write() need a -ENOTCONN.

However the documentation should mention that a passive connection can only be reopend by issueing a disconnect()-call.

miri64 · 2016-09-03T12:08:58Z

@kaspar - That a good Idea, then only read() and write() need a -ENOTCONN.

They already have :-).

However the documentation should mention that a passive connection can only be reopend by issueing a disconnect()-call.

I'm always confused by what you mean with "passive connection". In terms of socket-speak that a listening connection? Where should I put that documentation.

brummer-simon · 2016-09-03T12:23:20Z

In socket-speak:
"Passive connection" = listen() and accept() = thats what usually servers do.
"Active connection" = open() = thats what clients do.

Hmm maybe you could a note at the disconnect()-call that says:
Reopens a connection, that was previously initialized by the listen()-call after connection termination.

miri64 · 2016-09-03T12:26:11Z

Hmm maybe you could a note at the disconnect()-call that says:
Reopens a connection that was previously initialized by the listen()-call.

That wording is a little bit confusing IMHO. How about "A sock_tcp_t instance that was initialized by a listen() call shall only be usable again after a disconnect() call."?

kaspar030 · 2016-09-03T12:28:15Z

Let's try to keep both the RFC nomenclature (active, passive) and the notion that queue members are "connected, disconnected or listening" out of the docs.

The former is confusing (I assume as only actual implementors seem to know the meaning.

The latter is IMHO mixing up things, as it is weird to have a memory area "listening", and it also seems implementation dependent.

-----Original Message-----
From: Simon Brummer [email protected]
To: RIOT-OS/RIOT [email protected]
Cc: Kaspar Schleiser [email protected], Mention [email protected]
Sent: Sa., 03 Sep. 2016 14:22
Subject: Re: [RIOT-OS/RIOT] sock: Introduction of new application layer API (#5758)

In socket-speak:
"Passive connection" = listen() and accept() = thats what usually servers do.
"Active connection" = open() = thats what clients do.

Hmm maybe you could a note at the disconnect()-call that says:
Reopens a connection that was previously initialized by the listen()-call.

You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#5758 (comment)

kaspar030 · 2016-09-03T12:29:35Z

yes, or maybe clarify that it will not be reused (by accept) for new connections until closed.

-----Original Message-----
From: Martine Lenders [email protected]
To: RIOT-OS/RIOT [email protected]
Cc: Kaspar Schleiser [email protected], Mention [email protected]
Sent: Sa., 03 Sep. 2016 14:25
Subject: Re: [RIOT-OS/RIOT] sock: Introduction of new application layer API (#5758)

Hmm maybe you could a note at the disconnect()-call that says:
Reopens a connection that was previously initialized by the listen()-call.

That wording is a little bit confusing IMHO. How about "A sock_tcp_t instance that was initialized by a listen() call shall only be usable again after a disconnect() call."?

You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#5758 (comment)

brummer-simon · 2016-09-03T15:04:51Z

How about?

A sock_tcp_t instance that was initialized by a listen() call can only be reused by an accept() call after a disconnect() call."?

kaspar030

I'm not happy with the documentation, but let's merge this and (maybe) improve later.

kaspar030 · 2016-09-27T20:47:15Z

sys/include/net/sock.h

+/**
+ * @defgroup    net_sock    Sock API
+ * @ingroup     net
+ * @brief       Provides a minimal common API for applications to connect to the


I don't like that tagline.

The API is supposed to be used to write network applications. Would you mind to rephrase?

If there is so much doc change needed, why did you merge?!?

kaspar030 · 2016-09-27T20:49:00Z

sys/include/net/sock.h

+ * ~~~~~~~~~~~~~~~~~~~~
+ *
+ * This module provides a minimal set of functions to establish connections or
+ * send and receives datagrams using different types of communication. Together,


why minimal set? (pls drop if you can't explain)

receives -> receive

"differnet types of communication"? please rephrase

kaspar030 · 2016-09-27T20:50:28Z

sys/include/net/sock.h

+ *
+ * This module provides a minimal set of functions to establish connections or
+ * send and receives datagrams using different types of communication. Together,
+ * they serve as a common API that connects application- and network stack code.


why common?

again, I find it misleading to describe this API as "connecting application with network stack code".

kaspar030 · 2016-09-27T20:50:48Z

sys/include/net/sock.h

+ * * @ref sock_tcp_t (net/sock/tcp.h): TCP sock
+ * * @ref sock_udp_t (net/sock/udp.h): UDP sock
+ *
+ * Each network stack must implement at least one sock type.


I think you can drop this.

kaspar030 · 2016-09-27T20:51:07Z

sys/include/net/sock.h

+ *
+ * Each network stack must implement at least one sock type.
+ *
+ * Note that there might be no relation between the different sock types.


what does that mean?

kaspar030 · 2016-09-27T21:03:13Z

sys/include/net/sock/ip.h

+ * @param[in] data      Pointer where the received data should be stored.
+ *                      May be `NULL` if `len == 0`.
+ * @param[in] len       Maximum space available at @p data.
+ * @param[in] proto     Protocol to use in the packet send, in case


send -> sent or "to be sent"

kaspar030 · 2016-09-27T21:03:43Z

sys/include/net/sock/ip.h

+ * @param[in] proto     Protocol to use in the packet send, in case
+ *                      `sock == NULL`. If `sock != NULL` this parameter will be
+ *                      ignored.
+ * @param[in] remote    Remote end point for the send data.


"to be sent" as above

kaspar030 · 2016-09-27T21:04:05Z

sys/include/net/sock/ip.h

+ *                      sock_ip_ep_t::family may be AF_UNSPEC, if local
+ *                      end point of @p sock provides this information.
+ *
+ * @note    Function blocks until packet is handed to the stack.


kaspar030 · 2016-09-27T21:04:14Z

sys/include/net/sock/ip.h

+ *
+ * @note    Function blocks until packet is handed to the stack.
+ *
+ * @return  The number of bytes send on success.


send -> sent

If anyone is going to fix this: there are various more occurrences of this typo all over the place.

kaspar030 · 2016-09-27T21:04:53Z

sys/include/net/sock/tcp.h

+typedef struct _sock_tl_ep sock_tcp_ep_t;   /**< An end point for a TCP sock object */
+
+/**
+ * @brief   Implementation-specific type of a TCP sock object


pls drop implementation specific

cgundogan · 2016-09-27T21:12:20Z

@kaspar030 accidently hit the merge button? 🍻

kaspar030 · 2016-09-27T21:26:23Z

No, I realized that the API has been thoroughly discussed and my only complaints are either doxygen typos or require a longer (out-of-scope discussion) of the documentation style as a whole.

cgundogan · 2016-09-27T21:33:46Z

I am refering to the commit list. 18 commits marked as fixup (:

kaspar030 · 2016-09-27T21:36:13Z

Why does this happen to me? :)

What do we do?

cgundogan · 2016-09-27T21:41:49Z

What do we do?

simple solution. just bring some beer for the next hack'n'ack (:

kaspar030 · 2016-09-27T21:53:10Z

simple solution. just bring some beer for the next hack'n'ack (:

Will do. Sorry @everyone for messing up our beautiful commit history.

miri64 added Area: network Area: Networking Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. labels Aug 18, 2016

miri64 added this to the Release 2016.10 milestone Aug 18, 2016

miri64 assigned kaspar030 Aug 18, 2016

kaspar030 reviewed Aug 18, 2016
View reviewed changes

OlegHahm added the State: waiting for other PR State: The PR requires another PR to be merged first label Aug 19, 2016

fixup! sock: Introduction of new application layer API

b71c2f3

fixup! sock: Introduction of new application layer API

44af876

miri64 added 2 commits September 2, 2016 18:59

fixup! sock: Introduction of new application layer API

d17ef91

Include missing header

fixup! sock: Introduction of new application layer API

09703e1

Add more error classes

miri64 mentioned this pull request Sep 2, 2016

pkg: provide sock support for lwIP #5802

Closed

5 tasks

kaspar030 approved these changes Sep 27, 2016

View reviewed changes

kaspar030 merged commit 526917b into RIOT-OS:master Sep 27, 2016

miri64 deleted the sock/api/initial branch September 28, 2016 05:13

miri64 mentioned this pull request Sep 28, 2016

sock: doc fixes #5884

Merged

This was referenced Oct 17, 2016

conn: Wishlist for Redo #4474

Closed

RFC: Future of gnrc_networking #3638

Closed

conn: Reworking the conn API #5091

Closed

kaspar030 mentioned this pull request Dec 11, 2016

gnrc_sock_udp: add sendto #6196

Closed

Conversation

miri64 commented Aug 18, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OlegHahm commented Aug 18, 2016

Uh oh!

kaspar030 commented Aug 19, 2016

Uh oh!

kaspar030 commented Aug 19, 2016

Uh oh!

OlegHahm commented Aug 19, 2016

Uh oh!

kaspar030 commented Aug 19, 2016

Uh oh!

kaspar030 commented Aug 19, 2016

Uh oh!

miri64 commented Aug 19, 2016

Uh oh!

miri64 commented Aug 19, 2016

Uh oh!

kaspar030 commented Aug 19, 2016

Uh oh!

miri64 commented Aug 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kaspar030 commented Aug 19, 2016

Uh oh!

miri64 commented Sep 2, 2016

Uh oh!

brummer-simon commented Sep 2, 2016

Uh oh!

miri64 commented Sep 2, 2016

Uh oh!

miri64 commented Sep 2, 2016

Uh oh!

kaspar030 commented Sep 2, 2016

Uh oh!

miri64 commented Sep 2, 2016

Uh oh!

brummer-simon commented Sep 3, 2016

Uh oh!

miri64 commented Sep 3, 2016

Uh oh!

brummer-simon commented Sep 3, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

miri64 commented Sep 3, 2016

Uh oh!

kaspar030 commented Sep 3, 2016

Uh oh!

kaspar030 commented Sep 3, 2016

Uh oh!

brummer-simon commented Sep 3, 2016

Uh oh!

kaspar030 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miri64 commented Aug 19, 2016 •

edited

Loading

brummer-simon commented Sep 3, 2016 •

edited

Loading

cgundogan commented Sep 27, 2016 •

edited

Loading