Hello Sébastien,

thank you for contributing to BIRD. While we deeply appreciate your work, we’ll need you to do a little bit more to get into our review queue:

please add also proper test setups to both demonstrate that the thing indeed works, but also to demonstrate how the thing is expected to be used.
please check and prepare merging into BIRD 3
just by the stats, the docs update looks a little bit sparse but we may change our mind after reading your patches

Having all these three will massively help not only accepting your patches, but also maintaining the feature long-term.

For more information, please check our Contribution policy: https://gitlab.nic.cz/labs/bird/-/blob/master/CONTRIBUTING.md

Some of these things may look hypocritical because e.g. we don’t have autotests for some already existing features and parts of BIRD, but we consider it a good practice to have tests, and we don’t want our technological debt to increase.

Also, please see following short notes on some things which caught my eye on a fast skim. No note means simply that it didn’t look suspicious on first glance. I’m pointing out things which are almost certainly obvious problems.

This patch (for master branch / 2.18) adds SRv6 L3VPN support to BIRD. It applies on top of the Multiple Labels capability (RFC 8277) patch I sent earlier.

Our requirements above (mostly with the test setup) apply for that as well.

Patch 0001 is preparatory work that adds a new internal attribute BA_RAW_MPLS_LABEL_STACK (0xfd) that stores the full 24-bit wire values from MPLS label fields in BGP NLRI, alongside the existing BA_MPLS_LABEL_STACK (0xfe) which stores decoded 20-bit label values.

This needs a specific update for BIRD 3, as the BGP attributes are specified a little bit differently there.

net_addr_mpls n = NET_ADDR_MPLS(fec->label); diff –git a/nest/route.h b/nest/route.h index 5a9e7fa..3addc17 100644 — a/nest/route.h +++ b/nest/route.h @@ -434,6 +434,9 @@ struct nexthop { struct nexthop next; byte flags; byte weight; + byte sid6s_orig; / Number of SRv6 SIDs before hostentry was applied / + byte sid6s; / Number of all SRv6 SIDs / + ip6_addr sid6[SRV6_MAX_SID_STACK]; byte labels_orig; / Number of labels before hostentry was applied / byte labels; / Number of all labels */ u32 label[0];

This is unacceptable at all, it adds an awful lot of bytes into a memory-constrained data structure, and you run into massive merging problems with BIRD 3. You need an EA for this, for sure.

diff –git a/nest/rt-table.c b/nest/rt-table.c index ed364d3..cdcebd5 100644 — a/nest/rt-table.c +++ b/nest/rt-table.c @@ -2391,7 +2391,7 @@ rt_postconfig(struct config c) /

void -rta_apply_hostentry(rta a, struct hostentry he, mpls_label_stack mls) +rta_apply_hostentry(rta a, struct hostentry he, mpls_label_stack mls, srv6_sid_stack *sid6)

All the changes in the hostentry logic are going to make a massive merge conflict not only with BIRD 3 but also with the (in-progress) igp filter feature expected for BIRD 3.3, and this is exactly why we ask contributors to check first with the core team before doing something big.

diff –git a/sysdep/linux/netlink-sys.h b/sysdep/linux/netlink-sys.h index 4c99307..463205f 100644 — a/sysdep/linux/netlink-sys.h +++ b/sysdep/linux/netlink-sys.h @@ -18,6 +18,8 @@ #include <linux/lwtunnel.h> #endif

+#include <linux/seg6_iptunnel.h> + #ifndef MSG_TRUNC /* Hack: Several versions of glibc miss this one :( */ #define MSG_TRUNC 0x20 #endif

This may need an autoconf guard but I have no idea how old this feature actually is.

[…] + /* SRH header (8 bytes) / + put_u8(pos, 0); / nexthdr: kernel overwrites this / + pos += 1; + put_u8(pos, (srh_len / 8) - 1); / hdrlen in 8-byte units / + pos += 1; + put_u8(pos, 4); / type: SRv6 / + pos += 1; + put_u8(pos, sid_count - 1); / segments_left / + pos += 1; + put_u8(pos, sid_count - 1); / last_entry/first_segment / + pos += 1; + put_u8(pos, 0); / flags / + pos += 1; + put_u16(pos, 0); / tag */ + pos += 2; […]

I suspect that this would be much easier to read and check if you used a structure and assigned to it appropriately. Or, if that is impossible, what about the ADVANCE macro from BGP?

ip6_addr __sid = get_ip6(sb + 1);

No double-underscore locals allowed. Looks like, and may collide with, compiler internals.

 if (trans_off % 32 + trans_len <= 32) {

   __sid.addr[trans_off / 32] &= ~(((1 << trans_len) - 1) << (32 - trans_len - trans_off % 32));

   __sid.addr[trans_off / 32] |= raw_label << (32 - trans_off % 32 - trans_len);

```
 } else {
```

   __sid.addr[trans_off / 32] &= ~(((1 << trans_len) - 1) >> (trans_off % 32 + trans_len - 32));

   __sid.addr[trans_off / 32] |= raw_label >> (trans_off % 32 + trans_len - 32);

   __sid.addr[trans_off / 32 + 1] &= ~(((1 << trans_len) - 1) << (32 - trans_off % 32 - trans_len + 32));

   __sid.addr[trans_off / 32 + 1] |= raw_label << (32 - trans_off % 32 - trans_len + 32);

```
 }
```

This needs not only a comment but probably a massive comment, and possibly a function. All the production builds are with -O2 and -flto anyway, and it’s not a shame to name parts of the expression, so that even a junior dev would understand what this code is doing.

Thank you for your understanding.
Maria

–
Maria Matejka (she/her) | BIRD Team Leader | CZ.NIC, z.s.p.o.