net/unicoap: Unified and Modular CoAP stack: Messaging and Minimal Server (pt 2)

examples/networking/coap/unicoap_server/README.md

@@ -19,7 +19,7 @@ To try this example on your host, run:
 BOARD=native make flash term
 ```
 This will compile and run the application.
-The application will print a link-layer address.
+The application will print a network-layer address.
 
 In a second terminal session, you can run
 

examples/networking/coap/unicoap_server/app.config

@@ -1,3 +1,3 @@
-CONFIG_UNICOAP_DEBUG_LOGGING=y
-CONFIG_UNICOAP_ASSIST=y
+CONFIG_UNICOAP_DEBUG_LOGGING=n
+CONFIG_UNICOAP_ASSIST=n
 CONFIG_UNICOAP_CREATE_THREAD=y

sys/include/net/unicoap/application.h

@@ -263,6 +263,8 @@
      * @brief Resource path
      *
      * Must be a null-terminated string with slash-separated path components
+     *
+     * @warning This path must not have any trailing slash separators, apart from the root path `/`
      */
     const char* path;
 
@@ -353,7 +355,7 @@
  * </sensors/light>;rt="light-lux"
  * ```
  *
  * @see [Constrained RESTful Environments (CoRE) Link Format](https://datatracker.ietf.org/doc/html/rfc6690)
  *
  * @param[in,out] buffer The buffer that will contain the built string in CoRE Link Format
  * @param capacity The capacity @p buffer in bytes
@@ -479,7 +481,7 @@
     /**
      * @brief Allowed protocols for the resources grouped together by this resource
      *
      * Example: Set to `UNICOAP_PROTOCOL_FLAG(UNICOAP_PROTO_UDP) | UNICOAP_PROTOCOL_FLAG(UNICOAP_PROTO_DTLS)`
      * to allow only CoAP over UDP and DTLS. Alternatively you can use
      * `UNICOAP_PROTOCOLS(UNICOAP_PROTO_UDP, UNICOAP_PROTO_DTLS)`.
      *
@@ -516,8 +518,10 @@
  * @pre @p listener is a valid pointer to a single listener
  *
  * @param[in] listener  Listener containing the resources.
+ * @returns Negative integer on error, zero on success.
+ * @retval `-ENOENT` if the given listener is not registered with `unicoap`.
  */
-void unicoap_listener_deregister(unicoap_listener_t* listener);
+int unicoap_listener_deregister(unicoap_listener_t* listener);
 
 /**
  * @brief Determines whether the complete Uri-Path matches the resources path.
@@ -533,7 +537,7 @@
  *
  * @returns A boolean value indicating whether the specified path matches the resource definition.
  */
 bool unicoap_resource_match_path_string(const unicoap_resource_t* resource, const char* path, size_t length);
 
 /**
  * @brief Determines whether the complete Uri-Path matches the resources path.
@@ -566,7 +570,7 @@
 #define UNICOAP_METHOD_FLAG(method)        (1 << (method))
 
 /**
  * @brief Macro that builds a bit field where the i-th bit indicates the i-th request method (`0.0i`)
  * @see @ref unicoap_resource_t.flags
  *
  * Use this macro to create a bitfield of allowed methods for a given
@@ -596,7 +600,7 @@
  * @param methods Methods bit field, each set bit's position corresponds to a method allowed
  * @param method Method to be checked
  */
 static inline bool unicoap_resource_match_method(unicoap_method_set_t methods, unicoap_method_t method)
 {
     return (methods & UNICOAP_METHOD_FLAG(method)) != 0;
 }

sys/net/application_layer/unicoap/options.c

@@ -863,28 +863,28 @@ ssize_t unicoap_options_get_next_query_by_name(unicoap_options_iterator_t* itera
                                                unicoap_option_number_t number, const char* name,
                                                const char** value)
 {
-    char* _name = NULL;
-    const char* component = NULL;
+    const uint8_t* _name = NULL;
+    const uint8_t* component = NULL;
     ssize_t res = -1;
     while ((res =unicoap_options_get_next_by_number(iterator, number,
                                                     (const uint8_t**)&component)) >= 0) {
         assert(component);
-        _name = (char*)component;
+        _name = component;
 
         while (res > 0 && *component != '=') {
             component += 1;
             res -= 1;
         }
 
-        if (strncmp(name, _name, (uintptr_t)component - (uintptr_t)_name) != 0) {
+        if (strncmp(name, (char*)_name, (uintptr_t)component - (uintptr_t)_name) != 0) {
             continue;
         }
 
         if (res > 0) {
             assert(*component == '=');
             component += 1;
             res -= 1;
-            *value = component;
+            *value = (const char*)component;
         }
         else {
             *value = NULL;

sys/net/application_layer/unicoap/server.c

@@ -25,29 +25,70 @@
 #include "debug.h"
 #include "private.h"
 
+
+/** @brief Returns new length of path excluding trailing slashes */
+static inline size_t _trim_trailing_slashes(const char* path, size_t length) {
+    if (length > 0) {
+        size_t i = length - 1;
+        while (i > 0) {
+            if (path[i] == '/') {
+                i -= 1;
+            } else {
+                break;
+            }
+        }
+        return i + 1;
+    } else {
+        return length;
+    }
+}
+
 bool unicoap_resource_match_path_string(const unicoap_resource_t* resource,
-                                        const char* lhs_path, size_t lhs_length)
+                                        const char* lhs_path, size_t _lhs_length)
 {
     assert(resource);
     assert(lhs_path);
 
     /* We are comparing the left-hand side (path from request) to the right-hand side (resource). */
 
     size_t rhs_length = strlen(resource->path);
+    assert(rhs_length > 0);
+    size_t lhs_length = _trim_trailing_slashes(lhs_path, _lhs_length);
 
     if (resource->flags & UNICOAP_RESOURCE_FLAG_MATCH_SUBTREE) {
         /* The actual path (LHS) length may be longer. If it is shorter, bail out. */
         if (lhs_length < rhs_length) {
             return false;
         }
+
+        /* The actual path (LHS) is now either as long or longer. If it is longer, then we
+         * expect a slash to indicate a subtree. Either the RHS path already ends with a slash
+         * or the LHS has a slash that succeeds the last RHS character.
+         *
+         * Examples:
+         *
+         * LHS: /a   -> RHS ends in slash, every path that is longer than RHS is a subpath
+         * RHS: /
+         *
+         * LHS: /a/a -> RHS ends in slash, every path that is longer than RHS is a subpath
+         * RHS: /a/
+         *
+         * LHS: /a/a -> RHS does not end in slash, so we need to require one to indicate subpath
+         * RHS: /a
+         */
+        if (lhs_length > rhs_length && !(lhs_path[rhs_length] == '/' || resource->path[rhs_length - 1] == '/'))  {
+            return false;
+        }
+
+        return strncmp(lhs_path, resource->path, rhs_length) == 0; /* RHS is null-terminated */
     } else {
         /* The actual path (LHS) length must match. If it is unequal, bail out. */
         if (lhs_length != rhs_length) {
             return false;
         }
-    }
 
-    return strncmp(lhs_path, resource->path, lhs_length) == 0; /* rhs is null-terminated */
+        return strncmp(lhs_path, resource->path, lhs_length) == 0; /* RHS is null-terminated */
+    }
 }
 
 bool unicoap_resource_match_path_options(const unicoap_resource_t* resource,
@@ -63,38 +104,64 @@ bool unicoap_resource_match_path_options(const unicoap_resource_t* resource,
 
     char* cursor = (char*)resource->path;
     const char* end = resource->path + strlen(resource->path);
+
+    /* Skip multiple successive slashes.
+     * https://pubs.opengroup.org/onlinepubs/009695399/basedefs/xbd_chap03.html#tag_03_266
+     */
     while (*cursor == '/') {
         cursor += 1;
     }
     char* start = cursor;
+    const char* component = NULL;
+
+    /* You might think, why not check cursor < end. To remove the need for end - 1 and bounds
+     * checks, we use cursor <= end and treat that case as if the NULL-terminator were a slash.
+     * The slash/NULL-terminator signals to the loop that it is supposed to compare the
+     * sequence of characters it has read since the last slash to the current Uri-Path option. */
     while (cursor <= end) {
+        /* Found the end of a path component.
+         * That may either be a slash or the end of the string. */
         if ((*cursor == '/') || ((cursor != start) && (cursor == end))) {
-            const char* component;
 
             int res = -1;
             if ((res = unicoap_options_get_next_uri_path_component(&iterator, &component)) < 0) {
                 break;
             }
+            /* Cursor points to the element with the index one greater than the last character. */
             size_t size = (uintptr_t)cursor - (uintptr_t)start;
+
+            /* Compare Uri-Path component with string path component. */
             if (((size_t)res != size) || (strncmp(start, component, size) != 0)) {
                 return false;
             }
 
+            /* Skip multiple successive slashes. */
             while (*cursor == '/') {
                 cursor += 1;
             }
             start = cursor;
         }
         else {
+            /* Skip over path component characters. */
             cursor += 1;
         }
     }
 
+    if (cursor != end + 1) {
+        /* If the resource's path is longer than the actual path, the paths definitely don't match.
+         * The end + 1 comparison is justified as we treat the NULL-terminator like a trailing
+         * slash. See above. */
+        return false;
+    }
+
     if (resource->flags & UNICOAP_RESOURCE_FLAG_MATCH_SUBTREE) {
+        /* There may be more Uri-Path options. But this is fine as subpaths are allowed. */
         return true;
     }
     else {
-        return cursor == (end + 1);
+        /* Make sure we read all options, i.e., the actual path is not longer than the resource's.
+         */
+        return unicoap_options_get_next_uri_path_component(&iterator, &component) == -1;
     }
 }
 
@@ -184,7 +251,6 @@ int unicoap_server_process_request(unicoap_packet_t* packet, const unicoap_resou
     assert(packet);
     assert(packet->remote);
     int res = 0;
-    unicoap_message_t* message = packet->message;
 
     unicoap_request_context_t context = {
         .resource = resource, ._packet = packet
@@ -197,15 +263,15 @@ int unicoap_server_process_request(unicoap_packet_t* packet, const unicoap_resou
     };
 
     SERVER_DEBUG("invoking handler\n");
-    res = resource->handler(message, &aux, &context, resource->handler_arg);
+    res = resource->handler(packet->message, &aux, &context, resource->handler_arg);
 
     if (res > 0) {
         SERVER_DEBUG("sending response " UNICOAP_CODE_CLASS_DETAIL_FORMAT
                      " from return value\n",
                      unicoap_code_class((uint8_t)res), unicoap_code_detail((uint8_t)res));
 
         if (IS_ACTIVE(CONFIG_UNICOAP_PREVENT_OPTIONAL_RESPONSES)) {
-            if (unicoap_response_is_optional(message->options, (unicoap_status_t)res)) {
+            if (unicoap_response_is_optional(packet->message->options, (unicoap_status_t)res)) {
                 SERVER_DEBUG("response " UNICOAP_CODE_CLASS_DETAIL_FORMAT
                              " is optional, not responding\n",
                              unicoap_code_class((uint8_t)res),
@@ -214,7 +280,7 @@ int unicoap_server_process_request(unicoap_packet_t* packet, const unicoap_resou
             }
         }
 
-        unicoap_response_init_empty(message, (unicoap_status_t)res);
+        unicoap_response_init_empty(packet->message, (unicoap_status_t)res);
         return unicoap_server_send_response_body(packet, resource);
     }
     else if (context._packet) {
@@ -229,8 +295,8 @@ int unicoap_server_process_request(unicoap_packet_t* packet, const unicoap_resou
                                FIXIT("set USEMODULE += unicoap_deferred_response and"
                                      "call unicoap_defer_response")
                                FIXIT("ignore request by returning UNICOAP_IGNORING_REQUEST"));
-                unicoap_response_init_string(message, UNICOAP_STATUS_INTERNAL_SERVER_ERROR,
-                                             "application");
+                unicoap_response_init_string(packet->message,
+                                             UNICOAP_STATUS_INTERNAL_SERVER_ERROR, "application");
                 goto error;
             }
         }

sys/net/application_layer/unicoap/state.c

@@ -204,7 +204,6 @@ int unicoap_deinit(void)
     }
 
     _deinit_drivers(&_queue);
-    /* TODO: Thread: Figure out how to die properly, in an orderly fashion. */
     _unicoap_pid = KERNEL_PID_UNDEF;
     memset(&_state, 0, sizeof(_state));
     memset(&_queue, 0, sizeof(_queue));
@@ -245,18 +244,19 @@ void unicoap_listener_register(unicoap_listener_t* listener)
     }
 }
 
-void unicoap_listener_deregister(unicoap_listener_t* listener)
+int unicoap_listener_deregister(unicoap_listener_t* listener)
 {
     assert(listener);
 
     unicoap_listener_t* l = _state.listeners;
     while (l) {
         if (l->next == listener) {
             l->next = listener->next;
-            break;
+            return 0;
         }
         l = l->next;
     }
+    return -ENOENT;
 }
 
 /* MARK: - Intersection with messaging */
@@ -333,23 +333,21 @@ unicoap_preprocessing_result_t unicoap_exchange_preprocess(unicoap_packet_t* pac
             unicoap_options_set_size1(&response_options, CONFIG_UNICOAP_BLOCK_SIZE);
             unicoap_response_init_with_options(message, UNICOAP_STATUS_REQUEST_ENTITY_TOO_LARGE,
                                                NULL, 0, &response_options);
-            goto server_prewarming_error;
+            unicoap_messaging_send(packet, UNICOAP_MESSAGING_FLAGS_DEFAULT);
+            return UNICOAP_PREPROCESSING_ERROR_REQUEST;
         }
 
         const unicoap_resource_t* resource = NULL;
         const unicoap_listener_t* listener = NULL;
         if ((res = unicoap_resource_find(packet, &resource, &listener)) != 0) {
             unicoap_response_init_empty(message, res < 0 ? UNICOAP_STATUS_INTERNAL_SERVER_ERROR :
                                                            (unicoap_status_t)res);
-            goto server_prewarming_error;
+            unicoap_messaging_send(packet, UNICOAP_MESSAGING_FLAGS_DEFAULT);
+            return UNICOAP_PREPROCESSING_ERROR_REQUEST;
         }
         arg->resource = resource;
         *flags = _messaging_flags_resource(resource->flags);
         return UNICOAP_PREPROCESSING_SUCCESS_REQUEST;
-
-server_prewarming_error:
-        unicoap_messaging_send(packet, UNICOAP_MESSAGING_FLAGS_DEFAULT);
-        return UNICOAP_PREPROCESSING_ERROR_REQUEST;
     }
 
     case UNICOAP_CODE_CLASS_RESPONSE_SUCCESS: