Hello Maria, Thank you for this initial feedback, it is appreciated. I have already read the contribution policy, and to be honest based on what I read I did not expect a comment about tests ("bird-tools.git [...] This repository is quite messy and you may need some help with it. We're planning to move the Netlab suite into the main git repository; after we do that, we'll require every contribution to add tests"). I haven't looked into BIRD 3 yet and I don't know much about it, just that is has threads :) Both of those (adding tests and supporting BIRD 3) are probably a lot of work and unfortunately I can't promise I will be able to work on both... I addressed some of your comments, please see attached patches (only netlink and bgp have updates): - Use struct for SRH in netlink (from Linux kernel header). - Renamed variables with double-underscore prefix. - Moved SID transposition in its own function and added comments and intermediate variables. As for the other comments:
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.
Maybe I missed something or misunderstood what you mean, but I think EA are per routes, not per nexthop, so that would not work.
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.
This patch set originates in 2019, I implemented SRv6 L3VPN support to BIRD because we needed it for our networks (and we still run it). At the time BIRD did not support the l3vpn protocol, so as you can imagine there were a number of kludges. Also BIRD 3 did not exists yet. End of last year I started to rebase and clean the implementation to remove the kludges and integrate with the l3vpn protocol. It was a great opportunity to share the patch set with the BIRD community, so I asked for feedback on the initial nest and static patches on the mailing list but I got no comment then (https://bird.network.cz/pipermail/bird-users/2025-September/018385.html and https://bird.network.cz/pipermail/bird-users/2025-October/018446.html).
This _may_ need an autoconf guard but I have no idea how old this feature actually is.
SEG6 support was added into Linux 4.10 (February 19 2017). Thank you for your hard work on the BIRD project. -- Sébastien ________________________________________ De : Maria Matejka <maria.matejka@nic.cz> Envoyé : jeudi 12 février 2026 18:32 À : Sébastien PARISOT Cc : BIRD Users Objet : Re: [PATCH] SRv6 support for BIRD 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.