Juliusz Chroboczek <jch@pps.univ-paris-diderot.fr> writes:
Please don't make this configurable, it only breaks interoperability. Section 4 of RFC 6126 is very clear about the maximum size of a Babel packet:
Noted. Snatched that from the RIP implementation; thought it might be nice to be configurable in the same way. Guess I was wrong... ;)
+ <tag>hello interval <m/num/</tag> + Interval at which periodic "hello" messages are sent on this interface, + in seconds. Default: 20 seconds for wired interfaces, 4 seconds for wireless.
Note that recent versions of babeld use 4s in all cases by default. 20s turned out to be too long for our users.
Noted. Will change it to 4.
+ These options specify the ToS/DiffServ/Traffic class/Priority of the + outgoing RIP packets.
RIP?
Only one copy/paste error from all the stuff I lifted from the RIP implementation? Not too shabby I'd say... ;)
+/* Is one number larger than another mod 65535? Since diff_mod64k is always >= + 0, just use a simple cutoff value to determine if the difference is small + enough that one is really larger. Since these comparisons are only made for + values that should not differ by more than a few numbers, this should be + safe. */ +static inline u16 ge_mod64k(u16 a, u16 b) +{ + return ((u16) a-b) < 0xfff0; +}
The comment is obsolete -- it applies to the previous version.
Well the first sentence, yeah; fixed.
+static struct babel_source * +babel_find_source(struct babel_entry *e, u64 router_id) +{ + struct babel_source *s; + WALK_LIST(s, e->sources) + if (s->router_id == router_id) + return s; + return NULL; +}
We've recently found out that in large networks, a large amount of CPU time (40%) in babeld was taken by walking the source table -- I recommend using a more efficient data structure here. (Source creation/flushing is rare, but searching for a source is very common.)
Yeah, saw that thread. Punted on doing something about it here; guess I'll see if I can figure out how the Bird hash table works... Bah! Who needs such big networks anyway?
+static void +babel_iface_timer(timer *t) +{
The triggered update logic here is not obvious to me. Perhaps a comment is suitable?
The idea is that when an update is (first) triggered, the current timestamp (in 'now') is saved, and when the update is sent, only routes which are updated at times >= that timestamp are actually transmitted. This works because the code triggering the update will simultaneously set the 'updated' time of the route(s) being modified. Now, going over this I discovered a bug where single-interface triggered updates could override the triggered time of each other. Fixed this, and will add a comment.
+static void +babel_iface_update_buffers(struct babel_iface *ifa) +{ + if (!ifa->sk) + return; + + uint rbsize = ifa->cf->rx_buffer ?: ifa->iface->mtu; + uint tbsize = ifa->cf->tx_length ?: ifa->iface->mtu;
Shouldn't you be accounting for IPv6 and UDP headers, at least for tbsize?
It already does; the max packet length is set on the line below:
+static enum parse_result +babel_read_next_hop(struct babel_pkt_tlv_header *hdr, + union babel_tlv *tlv, + struct babel_parse_state *state)
AE 2 is perfectly legal in NH TLVs. (Babeld only ever sends AE 3, but non-LL NHs are perfectly legal.) See Section 4.4.8 of RFC 6126.
Noted, and fixed.
+void +babel_send_unicast(union babel_tlv *tlv, struct babel_iface *ifa, ip_addr dest)
Note that buffering unicasts can be useful in case you're forwarding multiple requests to the same neighbour. (I babeld, we buffer at most one unicast packet per interface -- per-neighbour queueing is probably overkill.)
Noted.
+ /* A router_id may be 0, so we need a separate variable to track whether we + have seen a router_id */
Interesting observation. This suggests making 0 router-id forbidden (and all-1 too, for good measure).
Yup, would make sense :) Thanks for your comments; fixed most of them in my tree already, and will see what I can do about the rest. Cheers, -Toke