<!DOCTYPE html>
<html>
<head>
<meta charset="utf-8" />
<meta name="generator" content="pandoc" />
<meta name="viewport" content="width=device-width, initial-scale=1.0, user-scalable=yes" />
<style>
html {
line-height: 1.2;
font-family: serif;
font-size: 0.9em;
color: black;
background-color: white;
}
body {
margin: 0;
margin-right: auto;
max-width: 36em;
padding: 1em;
hyphens: auto;
overflow-wrap: break-word;
text-rendering: optimizeLegibility;
font-kerning: normal;
}
@media print {
body {
background-color: transparent;
color: black;
font-size: 11pt;
}
p, h2, h3 {
orphans: 3;
widows: 3;
}
h2, h3, h4 {
page-break-after: avoid;
}
}
p {
margin: 1em 0;
}
a {
color: black;
}
a:visited {
color: black;
}
img {
max-width: 100%;
}
h1, h2, h3, h4, h5, h6 {
margin-top: 1.4em;
}
h5, h6 {
font-size: 1em;
font-style: italic;
}
h6 {
font-weight: normal;
}
ol, ul {
padding-left: 1.7em;
margin-top: 1em;
}
li > ol, li > ul {
margin-top: 0;
}
blockquote {
margin: 0.5em;
padding-left: 0.5em;
border-left: 2px solid #e6e6e6;
color: #444;
}
code {
font-family: 'Lucida Console', monospace;
font-size: 95%;
margin: 0;
}
pre {
margin: 1em 0;
overflow: auto;
max-width: unset;
width: fit-content;
}
pre code {
padding: 0;
overflow: visible;
overflow-wrap: normal;
max-width: unset;
white-space: pre-wrap;
}
pre code span {
white-space: pre;
}
.sourceCode {
background-color: transparent;
overflow: visible;
}
code.diff span.kw,
code.diff span.dt {
font-weight: bold;
}
code.diff span.va {
background-color: rgba(192, 255, 192, 64);
color: rgb(0, 64, 0);
}
code.diff span.st {
background-color: rgba(255, 192, 192, 64);
color: rgb(64, 0, 0);
}
pre.diff {
background-color: rgb(240, 240, 240);
padding: 0.4em;
border: 1pt solid grey;
}
hr {
background-color: black;
border: none;
height: 1px;
margin: 1em 0;
}
table {
margin: 1em 0;
border-collapse: collapse;
width: 100%;
overflow-x: auto;
display: block;
font-variant-numeric: lining-nums tabular-nums;
}
table caption {
margin-bottom: 0.75em;
}
tbody {
margin-top: 0.5em;
border-top: 1px solid black;
border-bottom: 1px solid black;
}
th {
border-top: 1px solid black;
padding: 0.25em 0.5em 0.25em 0.5em;
}
td {
padding: 0.125em 0.5em 0.25em 0.5em;
}
header {
margin-bottom: 4em;
text-align: center;
}
code{white-space: pre-wrap;}
span.smallcaps{font-variant: small-caps;}
span.underline{text-decoration: underline;}
div.column{display: inline-block; vertical-align: top; width: 50%;}
div.hanging-indent{margin-left: 1.5em; text-indent: -1.5em;}
ul.task-list{list-style: none;}
q { quotes: "„" "”" "»" "«"; }
.display.math{display: block; text-align: center; margin: 0.5rem auto;}
</style>
</head>
<body>
<p>Hello Sébastien,</p>
<p>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:</p>
<ul>
<li>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.</li>
<li>please check and prepare merging into BIRD 3</li>
<li>just by the stats, the docs update looks a little bit sparse but we
may change our mind after reading your patches</li>
</ul>
<p>Having all these three will massively help not only accepting your
patches, but also maintaining the feature long-term.</p>
<p>For more information, please check our Contribution policy: <a
href="https://gitlab.nic.cz/labs/bird/-/blob/master/CONTRIBUTING.md"
class="uri">https://gitlab.nic.cz/labs/bird/-/blob/master/CONTRIBUTING.md</a></p>
<p>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.</p>
<p>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.</p>
<blockquote>
<p>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.</p>
</blockquote>
<p>Our requirements above (mostly with the test setup) apply for that as
well.</p>
<blockquote>
<ul>
<li>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.</li>
</ul>
</blockquote>
<p>This needs a specific update for BIRD 3, as the BGP attributes are
specified a little bit differently there.</p>
<blockquote>
<p>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 <em>next; byte flags; byte weight; + byte sid6s_orig;
/</em> Number of SRv6 SIDs before hostentry was applied <em>/ + byte
sid6s; /</em> Number of all SRv6 SIDs <em>/ + ip6_addr
sid6[SRV6_MAX_SID_STACK]; byte labels_orig; /</em> Number of labels
before hostentry was applied <em>/ byte labels; /</em> Number of all
labels */ u32 label[0];</p>
</blockquote>
<p>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.</p>
<blockquote>
<p>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 <em>c) </em>/</p>
<p>void -rta_apply_hostentry(rta <em>a, struct hostentry </em>he,
mpls_label_stack <em>mls) +rta_apply_hostentry(rta </em>a, struct
hostentry <em>he, mpls_label_stack </em>mls, srv6_sid_stack *sid6)</p>
</blockquote>
<p>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.</p>
<blockquote>
<p>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</p>
<p>+#include <linux/seg6_iptunnel.h> + #ifndef MSG_TRUNC /* Hack:
Several versions of glibc miss this one :( */ #define MSG_TRUNC 0x20
#endif</p>
</blockquote>
<p>This <em>may</em> need an autoconf guard but I have no idea how old
this feature actually is.</p>
<blockquote>
<p>[…] + /* SRH header (8 bytes) <em>/ + put_u8(pos, 0); /</em> nexthdr:
kernel overwrites this <em>/ + pos += 1; + put_u8(pos, (srh_len / 8) -
1); /</em> hdrlen in 8-byte units <em>/ + pos += 1; + put_u8(pos, 4);
/</em> type: SRv6 <em>/ + pos += 1; + put_u8(pos, sid_count - 1); /</em>
segments_left <em>/ + pos += 1; + put_u8(pos, sid_count - 1); /</em>
last_entry/first_segment <em>/ + pos += 1; + put_u8(pos, 0); /</em>
flags <em>/ + pos += 1; + put_u16(pos, 0); /</em> tag */ + pos += 2;
[…]</p>
</blockquote>
<p>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?</p>
<blockquote>
<ul>
<li>ip6_addr __sid = get_ip6(sb + 1);</li>
</ul>
</blockquote>
<p>No double-underscore locals allowed. Looks like, and may collide
with, compiler internals.</p>
<blockquote>
<ul>
<li><pre><code> if (trans_off % 32 + trans_len <= 32) {</code></pre></li>
<li><pre><code> __sid.addr[trans_off / 32] &= ~(((1 << trans_len) - 1) << (32 - trans_len - trans_off % 32));</code></pre></li>
<li><pre><code> __sid.addr[trans_off / 32] |= raw_label << (32 - trans_off % 32 - trans_len);</code></pre></li>
<li><pre><code> } else {</code></pre></li>
<li><pre><code> __sid.addr[trans_off / 32] &= ~(((1 << trans_len) - 1) >> (trans_off % 32 + trans_len - 32));</code></pre></li>
<li><pre><code> __sid.addr[trans_off / 32] |= raw_label >> (trans_off % 32 + trans_len - 32);</code></pre></li>
<li></li>
<li><pre><code> __sid.addr[trans_off / 32 + 1] &= ~(((1 << trans_len) - 1) << (32 - trans_off % 32 - trans_len + 32));</code></pre></li>
<li><pre><code> __sid.addr[trans_off / 32 + 1] |= raw_label << (32 - trans_off % 32 - trans_len + 32);</code></pre></li>
<li><pre><code> }</code></pre></li>
</ul>
</blockquote>
<p>This needs not only a comment but probably a massive comment, and
possibly a function. All the production builds are with <code>-O2</code>
and <code>-flto</code> anyway, and it’s not a shame to <em>name</em>
parts of the expression, so that even a junior dev would understand what
this code is doing.</p>
<p>Thank you for your understanding.<br />
Maria</p>
<p>–<br />
Maria Matejka (she/her) | BIRD Team Leader | CZ.NIC, z.s.p.o.</p>
</body>
</html>