[riot-notifications] [RIOT-OS/RIOT] net/gcoap: support DTLS (#15549)

benpicco notifications at github.com
Thu Jul 1 16:21:32 CEST 2021


@benpicco commented on this pull request.

CI still has some [style nits](https://github.com/RIOT-OS/RIOT/pull/15549/files) so I added my own 

> +        if (_sessions[i].state == SESSION_STATE_ESTABLISHED
+                && _sessions[i].sock == sock) {
+            if (session_slot == NULL
+                || session_slot->last_used_sec > _sessions[i].last_used_sec)
+            {
+                session_slot = &_sessions[i];
+            }
+        }

style nit: it's possible to reduce the level of nesting here

```suggestion
        if (_sessions[i].state != SESSION_STATE_ESTABLISHED) {
            continue;
        }
        if (_sessions[i].sock != sock) {
            continue;
        }

        if (session_slot == NULL ||
            session_slot->last_used_sec > _sessions[i].last_used_sec) {
            session_slot = &_sessions[i];
        }
```

> +void dsm_init(void)
+{
+    mutex_init(&_lock);
+    _available_slots = DTLS_PEER_MAX;
+}
+
+dsm_state_t dsm_store(sock_dtls_t *sock, sock_dtls_session_t *session,
+                      dsm_state_t new_state, bool restore)
+{
+    sock_udp_ep_t ep;
+    dsm_session_t *session_slot = NULL;
+    dsm_state_t prev_state = NO_SPACE;
+    mutex_lock(&_lock);
+
+    ssize_t res = _find_session(sock, session, &session_slot);
+    if (res != -1) {

```suggestion
    if (res >= 0) {
```

adds a bit of future-proofing should `_find_session` ever get refactored to emit more error code

or even better

```suggestion
    if (res < 0) {
        DEBUG("dsm: no space for session to store\n");
        goto out;
    }
```

and we can reduce the level of nesting

> +        if (timeout != SOCK_NO_TIMEOUT) {
+            uint32_t diff = (xtimer_now_usec() - start);
+            timeout = (diff > timeout) ? 0: timeout - diff;
+            is_timed_out = (res < 0) || (timeout == 0);
+        }

Can you add a comment about this?
Future readers won't be hunting down the PR comment :wink: 

-- 
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/15549#pullrequestreview-697297851
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20210701/9acd8447/attachment.htm>


More information about the notifications mailing list