Ondrej Zajicek <santiago@crfreenet.org> writes:
On Tue, Jan 31, 2023 at 11:55:50AM +0100, Toke Høiland-Jørgensen via Bird-users wrote:
When creating a new babel_source object we initialise the seqno to 0. The caller will update the source object with the right metric and seqno value, for both newly created and old source objects. However if we initialise the source object seqno to 0 that may actually turn out to be a valid (higher) seqno than the one in the routing table, because of seqno wrapping. In this case the source metric will not be set properly, which breaks feasibility tracking for subsequent updates.
To fix this, add a new initial_seqno argument to babel_get_source() which is used when allocating a new object, and set that to the seqno value of the update we're sending.
Thanks, merged:
https://gitlab.nic.cz/labs/bird/-/commit/dc4c5f51f83f97100b207136ecfde8ff94e...
The find/get function pair pattern is often used in BIRD, it has some advantages in keeping invariants, but it is cumbersome when nontrivial initialization of new objects is used. Sometimes it leads to branching in caller and doing additional initialization there, in such cases it make sense to switch to find/add function pair. (just a general remark)
Noted. I think find/get works fine here, though :)
btw, the section 3.7.3 says:
"Before sending an update (prefix, plen, router-id, seqno, metric) with finite metric (i.e., not a route retraction)" and "Note that the garbage-collection timer is not reset when a retraction is sent."
But the whole section 'Update feasibility distance for redistributed routes' is done in all cases?
This has been clarified in RFC8966 as: "Note that the feasibility distance is not updated and the garbage-collection timer is not reset when a retraction (an update with infinite metric) is sent." The feasibility distance is only updated if the metric is lower, which is never true for an (infinite-metric) retraction, so that's kinda implicit anyway. We do actually update the garbage collection time regardless of the route metric in the regular update function (but not when sending an explicit retraction). I think that's OK, though?
Also, did you review BIRD Babel code whether we could switch RFC references from RFC 6126 to RFC 8966? I vaguely remember that there were such a patch.
Yeah, it should be updated to RFC8966. We implement sub-TLVs as specified in that, and the extensions (source-specific routing, MAC) are not compatible with a pure RFC6126 implementation anyway. -Toke