[PATCH v2 2/2] Add the Babel protocol.

[Juliusz Chroboczek]

Nice.

> +		tx length <number>;

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:

   It MUST NOT send packets larger than the attached interface's MTU
   (adjusted for lower-layer headers) or 512 octets, whichever is larger,
   but not exceeding 2^16 - 1 adjusted for lower-layer headers.

> +		rx buffer <number>;

In the same section, it says

   Every Babel speaker MUST be able to receive packets that are as large
   as any attached interface's MTU (adjusted for lower-layer headers) or
   512 octets, whichever is larger.

> +      <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.

> +      These options specify the ToS/DiffServ/Traffic class/Priority of the
> +      outgoing RIP packets.

RIP?

> +/* 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.

> +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.)

> +static void
> +babel_iface_timer(timer *t)
> +{

The triggered update logic here is not obvious to me.  Perhaps a comment
is suitable?

> +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?

> +static enum parse_result
> +babel_read_ihu(struct babel_pkt_tlv_header *hdr,
> +               union babel_tlv *tlv,
> +               struct babel_parse_state *state)

[...]

> +  // We handle link-local IPs. In every other case, the addr field will be 0 but
> +  // validation will succeed. The handler takes care of these cases.

Hmm.  Well, ok.

> +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.

> +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.)

> +  /* 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).

> /* We use multithreading */
> #undef USE_PTHREADS

Wrong comment?

-- Juliusz