[riot-notifications] [RIOT-OS/RIOT] Improving TinyDTLS package and dtls-echo example (#7615)

Alexandre Abadie notifications at github.com
Fri Apr 27 12:12:46 CEST 2018


aabadie requested changes on this pull request.

I have a few other comments and I'm also wondering if it wouldn't be better to move the dtls management at the sock level and not on top of it (as it's done here).
I'm thinking at CoAP for example.

Maybe @miri64 could give some advice here.

I'll test this PR next week.

>  
-/**
- * @brief This care about getting messages and continue with the DTLS flights.
- * This will handle all the packets arriving to the node.
- * Will determine if its from a new DTLS peer or a previous one.
+    if (code == DTLS_EVENT_CONNECTED) {
+        dtls_connected = 1;
+        DEBUG("CLIENT: DTLS Channel established!\n");
+    }
+#if ENABLE_DEBUG

Sorry about asking to change this another time but I just remembered what is the correct way of using ENABLE_DEBUG here. Here you are skipping the compilation of the next block unless ENABLE_DEBUG is explicitly set. This has the drawback of being completely skipped by the CI. The good way would be to do:

```c
/* At least a DTLS Client Hello was prepared? */
else if (ENABLE_DEBUG && (code == DTLS_EVENT_CONNECT)) {
    DEBUG("CLIENT: DTLS Channel started\n");
}
```
Then this is compiled in any case but it will be optimized by the compiler if ENABLE is 0.

>  
-    ipv6_addr_from_str(&session.addr, addr_str);
+#if ENABLE_DEBUG

same comment about ENABLE_DEBUG, you could do
```c
if (ENABLE_DEBUG) {
   ...
}
```

>  
-    /* allocate IPv6 header */
-    ip = gnrc_ipv6_hdr_build(udp, NULL,  &addr);
-    if (ip == NULL) {
-        puts("Error: unable to allocate IPv6 header");
-        gnrc_pktbuf_release(udp);
-        return -1;
-    }
-    /* add netif header, if interface was given */
-    if (iface > 0) {
-        gnrc_pktsnip_t *netif = gnrc_netif_hdr_build(NULL, 0, NULL, 0);
+    size_t i;

This could be put in the for loop directly

>  
     /*
-     * The context for the server is a little different from the client.
-     * The simplicity of GNRC do not mix transparently with
-     * the DTLS Context. At this point, the server need a fresh context
-     * however dtls_context->app must be populated with an unknown
-     * IPv6 address.
-     *
-     * The non-valid Ipv6 address ( :: ) is discarded due the chaos.
-     * For now, the first value will be the loopback.
+     * The context for the server is different from the client.
+     * TThis is because sock_udp_create() cannot work with a remote endpoint

Typo **T**This

> -        /*TODO: What happens with other clients connecting at the same time? */
+        msg_try_receive(&msg); /* Check if we got an (thread) message */
+        if (msg.type == DTLS_STOP_SERVER_MSG) {
+            active = false;
+        }
+        else { /* Normal operation */
+
+            /* NOTE: The 10 seconds time is for reaching the IPC msgs again. */
+            pckt_rcvd_size = sock_udp_recv(&udp_socket, pckt_rcvd,
+                                           sizeof(pckt_rcvd),
+                                           10 * US_PER_SEC, &remote);
+            if (pckt_rcvd_size >= 0) {
+                DEBUG("DBG-Server: Record Rcvd\n");
+                dtls_handle_read(dtls_context, pckt_rcvd, pckt_rcvd_size);
+            }
+#if ENABLE_DEBUG

The ENABLE_DEBUG could be moved to the else below:

```c
else if (ENABLE_DEBUG) {
    switch(...
```

> +                        break;
+
+                    case -ENOMEM:
+                        puts("ERROR: Memory overflow!");
+                        break;
+
+                    case -EINVAL:
+                        puts("ERROR: remote or sock is not properly initialized");
+                        break;
+
+                    case -EPROTO:
+                        puts("ERROR: source address of received packet did not equal the remote");
+                        break;
+
+                    default:
+                        printf("ERROR: unexpected code error: %i", pckt_rcvd_size);

Missing `\n` after `%i`

>  
-    } /*While */
+    /* Release resoruces (strict order) */
+    dtls_free_context(dtls_context); /* This also send a DTLS Alert record */

send**s**

>  
-    } /*While */
+    /* Release resoruces (strict order) */
+    dtls_free_context(dtls_context); /* This also send a DTLS Alert record */
+    sock_udp_close(&udp_socket);
+    msg_reply(&msg, &msg); /*Basic answer to the main thread*/

Add spaces before `Basic` and after `thread`

-- 
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/7615#pullrequestreview-115879174
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20180427/c7250a87/attachment.html>


More information about the notifications mailing list