[riot-notifications] [RIOT-OS/RIOT] gnrc_ipv6_ext_frag: Initial import of IPv6 fragmentation (#11623)

benpicco notifications at github.com
Mon Sep 16 23:50:42 CEST 2019


benpicco commented on this pull request.

At a first glance this looks good too - I would enjoy more documentation though.
I don't know much about IPv6 fragmentation, so maybe by the time I understand it, it's well documented :wink: 

>  static void _send_by_netif_hdr(gnrc_pktsnip_t *pkt)
 {
     assert(pkt->type == GNRC_NETTYPE_NETIF);
     gnrc_netif_t *netif = gnrc_netif_hdr_get_netif(pkt->data);
 
     _send_to_iface(netif, pkt);
 }
+#else   /* MODULE_GNRC_IPV6_EXT_FRAG */
+#define _fragment_pkt_if_needed(pkt, netif, from_me)    (false)

If this were 
```C
static bool _fragment_pkt_if_needed(gnrc_pktsnip_t *pkt,
                                    gnrc_netif_t *netif,
                                    bool from_me)
{
    (void) pkt;
    (void) netif;
    (void) from_me;
    return false;
}
```

you could save yourself that stray `(void)prep_hdr;` further down in the code.

> @@ -45,6 +45,15 @@ extern "C" {
  * @ingroup     config
  * @{
  */
+/**
+ * @brief   IPv6 fragmentation send buffer size

Again, please extend the terse documentation by specifying if this is the number of possible fragmented datagrams being sent or the maximal number of fragments.

> @@ -34,7 +35,17 @@ extern "C" {
 /**
  * @brief   Message type to time reassembly buffer garbage collection
  */
-#define GNRC_IPV6_EXT_FRAG_RBUF_GC  (0xfe00U)
+#define GNRC_IPV6_EXT_FRAG_RBUF_GC      (0xfe00U)
+
+/**
+ * @brief   Message type to send further fragments of a IPv6 packet

So this is for all but the first fragment?

(It's actually sent for all but the last fragment)

> @@ -34,7 +35,17 @@ extern "C" {
 /**
  * @brief   Message type to time reassembly buffer garbage collection
  */
-#define GNRC_IPV6_EXT_FRAG_RBUF_GC  (0xfe00U)
+#define GNRC_IPV6_EXT_FRAG_RBUF_GC      (0xfe00U)
+
+/**
+ * @brief   Message type to send further fragments of a IPv6 packet
+ */
+#define GNRC_IPV6_EXT_FRAG_SEND         (0xfe01U)
+
+/**
+ * @brief   Message type to send a fragment via the IPv6 layer

So this is any fragment?
When reading just this, I don't know how this differs from `GNRC_IPV6_EXT_FRAG_SEND` other than that it would also include the first fragment.

But I suspect it means something entirely different.

> +        switch (ptr->type) {
+            case GNRC_NETTYPE_IPV6: {
+                ipv6_hdr_t *hdr = ptr->data;
+                last_per_frag = ptr;
+                nh = hdr->nh;
+                break;
+            }
+            case GNRC_NETTYPE_IPV6_EXT: {
+                ipv6_ext_t *hdr = ptr->data;
+                switch (nh) {
+                    /* "[...] that is, all headers up to and including the
+                     * Routing header if present, else the Hop-by-Hop Options
+                     * header if present, [...]"
+                     * (IPv6 header comes before Hop-by-Hop Options comes before
+                     * Routing header, so an override to keep the quoted
+                     * priorities is ensured) */

*No wonder you need fragmentation with so many headers…*

> +    snd_buf->per_frag = NULL;
+    snd_buf->pkt = NULL;
+}
+
+static void _snd_buf_free(gnrc_ipv6_ext_frag_send_t *snd_buf)
+{
+    if (snd_buf->per_frag) {
+        gnrc_pktbuf_release(snd_buf->per_frag);
+    }
+    if (snd_buf->pkt) {
+        gnrc_pktbuf_release(snd_buf->pkt);
+    }
+    _snd_buf_del(snd_buf);
+}
+
+static gnrc_pktsnip_t *_determine_last_per_frag(gnrc_pktsnip_t *ptr)

Just a small description of what this function does would be nice.

> +            snd_buf->pkt->next = snd_buf->pkt->next->next;
+            ptr->next = NULL;
+        }
+        last->next = ptr;
+        last = ptr;
+        remaining -= ptr->size;
+        snd_buf->offset += ptr->size;
+    }
+    assert(len != NULL);
+    /* adapt IPv6 header length field */
+    *len = byteorder_htons(gnrc_pkt_len(to_send->next->next));
+    msg.type = GNRC_IPV6_EXT_FRAG_SEND_FRAG;
+    msg.content.ptr = to_send;
+    msg_try_send(&msg, gnrc_ipv6_pid);
+    if (last_fragment) {
+        _snd_buf_del(snd_buf);

Not `_snd_buf_free()` ?

> +            snd_buf->pkt = ptr->next;
+            ptr->next = NULL;
+        }
+        else {
+            ptr = gnrc_pktbuf_mark(snd_buf->pkt, remaining,
+                                   GNRC_NETTYPE_UNDEF);
+            if (ptr == NULL) {
+                DEBUG("ipv6_ext_frag: packet buffer full, canceling fragmentation\n");
+                gnrc_pktbuf_release(to_send);
+                _snd_buf_free(snd_buf);
+                return;
+            }
+            assert(snd_buf->pkt->next == ptr);  /* we just created it with mark */
+            snd_buf->pkt->next = snd_buf->pkt->next->next;
+            ptr->next = NULL;
+        }

You can move `ptr->next = NULL;` here, it's set in both cases.

-- 
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/11623#pullrequestreview-288879830
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20190916/70a9d036/attachment.htm>


More information about the notifications mailing list