[riot-notifications] [RIOT-OS/RIOT] netif: introduce generic network interface descriptor (#11879)

Martine Lenders notifications at github.com
Fri Sep 27 15:01:41 CEST 2019


miri64 commented on this pull request.

Some comments from my side

> @@ -45,20 +48,29 @@ extern "C" {
 #define NETIF_NAMELENMAX    (8U)
 #endif
 
+/**
+ * @brief Network interface descriptor.
+ *
+ * @note All network interfaces should inherit from this structure.
+ */
+typedef struct __netif netif_t;

The usual naming scheme for a forward declaration is

```suggestion
typedef struct netif netif_t;
```

> +
+#include <string.h>
+
+#include "errno.h"
+#include "net/netif.h"
+#include "utlist.h"
+
+static netif_t *netif_list;
+
+int netif_register(netif_t *netif)
+{
+    if(netif == NULL) {
+        return -EINVAL;
+    }
+
+    LL_APPEND(netif_list, netif);

Could you try to use `clist` or `list` instead?

> @@ -1101,7 +1100,7 @@ static void test_netif_get_opt(void)
     uint8_t value[GNRC_NETIF_L2ADDR_MAXLEN];
 
     TEST_ASSERT_EQUAL_INT(sizeof(exp_ethernet),
-                          netif_get_opt((netif_t)ethernet_netif->pid,
+                          netif_get_opt((netif_t*) ethernet_netif,

Here and below

```suggestion
                          netif_get_opt((netif_t *)ethernet_netif,
```

>      /* there must be at least one interface */
-    TEST_ASSERT(NETIF_INVALID != netif);
+    TEST_ASSERT(NULL != netif);

```suggestion
    TEST_ASSERT_NOT_NULL(netif);
```

>      /* there must be at least one interface */
-    TEST_ASSERT(NETIF_INVALID != netif);
+    TEST_ASSERT(NULL != netif);

```suggestion
    TEST_ASSERT_NOT_NULL(netif);
```

> @@ -45,20 +48,29 @@ extern "C" {
 #define NETIF_NAMELENMAX    (8U)
 #endif
 
+/**
+ * @brief Network interface descriptor.
+ *
+ * @note All network interfaces should inherit from this structure.
+ */
+typedef struct __netif netif_t;
+
+struct __netif {

Needs documentation. I recommend to do it as elsewhere in the code: document this struct and document the forward declaration with

```
/**
 * @brief Forward declaration of @ref struct netif
 */
```

> @@ -45,20 +48,29 @@ extern "C" {
 #define NETIF_NAMELENMAX    (8U)
 #endif
 
+/**
+ * @brief Network interface descriptor.
+ *
+ * @note All network interfaces should inherit from this structure.
+ */
+typedef struct __netif netif_t;
+
+struct __netif {

Alternatively skip the forward declaration. It isn't really needed in the current version:

```suggestion
typedef struct netif {
    struct netif *next;
} netif_t;
```

-- 
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/11879#pullrequestreview-294302105
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20190927/52b624c3/attachment.htm>


More information about the notifications mailing list