[riot-notifications] [RIOT-OS/RIOT] sys: new sock submodule for DTLS (#11909)

Martine Lenders notifications at github.com
Wed Jul 31 17:30:23 CEST 2019


miri64 requested changes on this pull request.

Only reviewed the API and its documentation so far. Not the more general documentation on top.

> +#include <stdlib.h>
+#include <sys/types.h>
+
+#include "net/sock.h"
+#include "net/sock/udp.h"
+#include "net/credman.h"
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+/**
+ * @brief   Type for a DTLS sock object
+ *
+ * @note    API implementors: `struct sock_dtls` needs to be defined by
+ *          implementation-specific `sock_dtls_types.h`.

```suggestion
 *          an implementation-specific `sock_dtls_types.h`.
```

or

```suggestion
 *          the implementation-specific `sock_dtls_types.h`.
```

> +
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+/**
+ * @brief   Type for a DTLS sock object
+ *
+ * @note    API implementors: `struct sock_dtls` needs to be defined by
+ *          implementation-specific `sock_dtls_types.h`.
+ */
+typedef struct sock_dtls sock_dtls_t;
+
+/**
+ * @brief Information about the established session with
+ *        the remote endpoint. Used when sending and

Typically a `@brief` is only one sentence. More information can be added to the `@details` section (which is implicitly created for non-specified paragraphs):

```suggestion
 *        the remote endpoint.
 *
 * This session object is used when sending and receiving data to the endpoint.
```

However, this information is somewhat obvious (as you don't do much else with a `sock`) so maybe the last sentence can be dropped completely.

> + * @brief   Type for a DTLS sock object
+ *
+ * @note    API implementors: `struct sock_dtls` needs to be defined by
+ *          implementation-specific `sock_dtls_types.h`.
+ */
+typedef struct sock_dtls sock_dtls_t;
+
+/**
+ * @brief Information about the established session with
+ *        the remote endpoint. Used when sending and
+ *        receiving data to the endpoint
+ */
+typedef struct sock_dtls_session sock_dtls_session_t;
+
+/**
+ * @brief Must be called once before any other use

Exactly once or at least once? Who typically calls it? Why do I need to call it?

> + * @brief Information about the established session with
+ *        the remote endpoint. Used when sending and
+ *        receiving data to the endpoint
+ */
+typedef struct sock_dtls_session sock_dtls_session_t;
+
+/**
+ * @brief Must be called once before any other use
+ */
+void sock_dtls_init(void);
+
+/**
+ * @brief Creates a new DTLS sock object
+ *
+ * @param[out] sock     The resulting DTLS sock object
+ * @param[in] udp_sock  Existing UDP sock to be used

```suggestion
 * @param[in] udp_sock  Existing UDP sock to be used underneath
```

?

> + *        the remote endpoint. Used when sending and
+ *        receiving data to the endpoint
+ */
+typedef struct sock_dtls_session sock_dtls_session_t;
+
+/**
+ * @brief Must be called once before any other use
+ */
+void sock_dtls_init(void);
+
+/**
+ * @brief Creates a new DTLS sock object
+ *
+ * @param[out] sock     The resulting DTLS sock object
+ * @param[in] udp_sock  Existing UDP sock to be used
+ * @param[in] tag       Credential tag of the sock. Used to get the right

What does "right" mean in this context?

> +void sock_dtls_init(void);
+
+/**
+ * @brief Creates a new DTLS sock object
+ *
+ * @param[out] sock     The resulting DTLS sock object
+ * @param[in] udp_sock  Existing UDP sock to be used
+ * @param[in] tag       Credential tag of the sock. Used to get the right
+ *                      credential from pool.
+ * @param[in] method    Defines the method for the client or server to use.
+ *
+ * @return  0 on success.
+ * @return  value < 0 on error
+ */
+int sock_dtls_create(sock_dtls_t *sock, sock_udp_t *udp_sock,
+                     credman_tag_t tag,unsigned method);

```suggestion
                     credman_tag_t tag, unsigned method);
```

> +/**
+ * @brief Must be called once before any other use
+ */
+void sock_dtls_init(void);
+
+/**
+ * @brief Creates a new DTLS sock object
+ *
+ * @param[out] sock     The resulting DTLS sock object
+ * @param[in] udp_sock  Existing UDP sock to be used
+ * @param[in] tag       Credential tag of the sock. Used to get the right
+ *                      credential from pool.
+ * @param[in] method    Defines the method for the client or server to use.
+ *
+ * @return  0 on success.
+ * @return  value < 0 on error

What values? Please specify, it might be important for the user to know.

> + */
+typedef struct sock_dtls_session sock_dtls_session_t;
+
+/**
+ * @brief Must be called once before any other use
+ */
+void sock_dtls_init(void);
+
+/**
+ * @brief Creates a new DTLS sock object
+ *
+ * @param[out] sock     The resulting DTLS sock object
+ * @param[in] udp_sock  Existing UDP sock to be used
+ * @param[in] tag       Credential tag of the sock. Used to get the right
+ *                      credential from pool.
+ * @param[in] method    Defines the method for the client or server to use.

What kind of method are you referring to here?

> + * @param[in] tag       Credential tag of the sock. Used to get the right
+ *                      credential from pool.
+ * @param[in] method    Defines the method for the client or server to use.
+ *
+ * @return  0 on success.
+ * @return  value < 0 on error
+ */
+int sock_dtls_create(sock_dtls_t *sock, sock_udp_t *udp_sock,
+                     credman_tag_t tag,unsigned method);
+
+/**
+ * @brief Initialises the server to listen for incoming connections
+ *
+ * @param[in] sock      DTLS sock to listen to
+ */
+void sock_dtls_init_server(sock_dtls_t *sock);

In accordance with `gnrc_tcp`: maybe name it `gnrc_dtls_listen()`? Not sure, if we already talked about this :sweat_smile:...

> + * @brief Initialises the server to listen for incoming connections
+ *
+ * @param[in] sock      DTLS sock to listen to
+ */
+void sock_dtls_init_server(sock_dtls_t *sock);
+
+/**
+ * @brief Establish DTLS session to a server. Execute the handshake step in
+ *        DTLS.
+ *
+ * @param[in]  sock     DLTS sock to use
+ * @param[in]  ep       Endpoint to establish session with
+ * @param[out] remote   The established session, cannot be NULL
+ *
+ * @return  0 on success
+ * @return  value < 0 on other errors

Are there more than the ones defined below? If yes, define them instead of being vague.

> + *
+ * @return  0 on success.
+ * @return  value < 0 on error
+ */
+int sock_dtls_create(sock_dtls_t *sock, sock_udp_t *udp_sock,
+                     credman_tag_t tag,unsigned method);
+
+/**
+ * @brief Initialises the server to listen for incoming connections
+ *
+ * @param[in] sock      DTLS sock to listen to
+ */
+void sock_dtls_init_server(sock_dtls_t *sock);
+
+/**
+ * @brief Establish DTLS session to a server. Execute the handshake step in

One sentence per `@brief`

> + * @return  -EINVAL, if sock_udp_ep_t::addr of @p remote->ep is an
+ *          invalid address.
+ * @return  -EINVAL, if sock_udp_ep_t::port of @p remote->ep is 0.
+ * @return  -ENOMEM, if no memory was available to send @p data.
+ */
+ssize_t sock_dtls_send(sock_dtls_t *sock, sock_dtls_session_t *remote,
+                       const void *data, size_t len);
+
+/**
+ * @brief Destroys DTLS sock created by `sock_dtls_create`
+ *
+ * @pre `(sock != NULL)`
+ *
+ * @param sock          DTLS sock to destroy
+ */
+void sock_dtls_destroy(sock_dtls_t *sock);

```suggestion
void sock_dtls_close(sock_dtls_t *sock);
```

to be in line with the other `sock` APIs?

> + * @return  -ENOBUFS, if buffer space is not large enough to store received
+ *          credentials.
+ * @return  -ETIMEDOUT, if timed out when trying to establish session.
+ */
+int sock_dtls_establish_session(sock_dtls_t *sock, sock_udp_ep_t *ep,
+                                sock_dtls_session_t *remote);
+
+/**
+ * @brief Close an existing DTLS session
+ *
+ * @pre `(sock != NULL) && (ep != NULL)`
+ *
+ * @param[in] sock      @ref sock_dtls_t, which the session is established on
+ * @param[in] remote    Remote session to close
+ */
+void sock_dtls_close_session(sock_dtls_t *sock, sock_dtls_session_t *remote);

Not to be confused with `sock_dtls_close()` (in case we rename `sock_dtls_destroy()`) how about naming this then `sock_dtls_term_session` or `sock_dtls_terminate_session()` to avoid confusion.

> + * @brief Close an existing DTLS session
+ *
+ * @pre `(sock != NULL) && (ep != NULL)`
+ *
+ * @param[in] sock      @ref sock_dtls_t, which the session is established on
+ * @param[in] remote    Remote session to close
+ */
+void sock_dtls_close_session(sock_dtls_t *sock, sock_dtls_session_t *remote);
+
+/**
+ * @brief Decrypts and reads a message from a remote peer.
+ *
+ * @param[in] sock      DTLS sock to use.
+ * @param[out] remote   Remote DTLS session of the received data.
+ *                      Cannot be NULL.
+ * @param[out] data     Buffer where the data should be stored.

What kind of data?

> + * @pre `(sock != NULL) && (ep != NULL)`
+ *
+ * @param[in] sock      @ref sock_dtls_t, which the session is established on
+ * @param[in] remote    Remote session to close
+ */
+void sock_dtls_close_session(sock_dtls_t *sock, sock_dtls_session_t *remote);
+
+/**
+ * @brief Decrypts and reads a message from a remote peer.
+ *
+ * @param[in] sock      DTLS sock to use.
+ * @param[out] remote   Remote DTLS session of the received data.
+ *                      Cannot be NULL.
+ * @param[out] data     Buffer where the data should be stored.
+ * @param[in] maxlen    Maximum memory available at @p data.
+ * @param[in] timeout   Timeout for receive in microseconds.

```suggestion
 * @param[in] timeout   Receive timeout in microseconds.
```

> +
+/**
+ * @brief Decrypts and reads a message from a remote peer.
+ *
+ * @param[in] sock      DTLS sock to use.
+ * @param[out] remote   Remote DTLS session of the received data.
+ *                      Cannot be NULL.
+ * @param[out] data     Buffer where the data should be stored.
+ * @param[in] maxlen    Maximum memory available at @p data.
+ * @param[in] timeout   Timeout for receive in microseconds.
+ *                      If 0 and no data is available, the function returns
+ *                      immediately.
+ *                      May be SOCK_NO_TIMEOUT to wait until data
+ *                      is available.
+ *
+ * @note Function may block if data not available and @p timeout != 0

```suggestion
 * @note Function may block if data is not available and @p timeout != 0
```

> + * @return  -ENOMEM, if no memory was available to receive @p data.
+ * @return  -ETIMEDOUT, if @p timeout expired.
+ */
+ssize_t sock_dtls_recv(sock_dtls_t *sock, sock_dtls_session_t *remote,
+                       void *data, size_t maxlen, uint32_t timeout);
+
+/**
+ * @brief Encrypts and sends a message to a remote peer
+ *
+ * @param[in] sock      DTLS sock to use
+ * @param[in] remote    DTLS session to use. A new session will be established
+ *                      if no session exist between client and server.
+ * @param[in] data      Pointer where the data to be send are stored
+ * @param[in] len       Length of @p data to be send
+ *
+ * @note Function may block

In which cases does it block?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/RIOT-OS/RIOT/pull/11909#pullrequestreview-269084443
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20190731/a3ce1850/attachment-0001.htm>


More information about the notifications mailing list