Multi-protocol core

[Martin Mares]

Ahoj!

> commit ee3256fe1cb3e429de22e7c50a0826aaa4dc7a73
> Author: Ondrej Zajicek <santiago at crfreenet.org>
> Date:   Sat Oct 1 10:28:34 2011 +0200
> 
>     Multiprotocol core support.
>     
>     The patch from Alexander V. Chernikov.

Overall, the multiprotocol core looks good, there is a couple of minor
nit-picking comments:

| diff --git a/configure.in b/configure.in
| index 46a6ecd..9af70b7 100644
| --- a/configure.in
| +++ b/configure.in
| @@ -10,6 +10,7 @@ AC_ARG_ENABLE(debug,[  --enable-debug          enable internal debugging routine
|  AC_ARG_ENABLE(memcheck,[  --enable-memcheck       check memory allocations when debugging (default: enabled)],,enable_memcheck=yes)
|  AC_ARG_ENABLE(client,[  --enable-client         enable building of BIRD client (default: enabled)],,enable_client=yes)
|  AC_ARG_ENABLE(ipv6,[  --enable-ipv6           enable building of IPv6 version (default: disabled)],,enable_ipv6=no)
| +AC_ARG_ENABLE(mpls,[  --enable-mpls           enable building MPLS version (default: disabled)],,enable_mpls=no)
|  AC_ARG_WITH(sysconfig,[  --with-sysconfig=FILE   use specified BIRD system configuration file])
|  AC_ARG_WITH(protocols,[  --with-protocols=LIST   include specified routing protocols (default: all)],,[with_protocols="all"])
|  AC_ARG_WITH(sysinclude,[  --with-sysinclude=PATH  search for system includes on specified place])
| @@ -50,6 +51,12 @@ else
|  	all_protocols=bgp,ospf,pipe,rip,static
|  fi
|  
| +if test "$enable_mpls" = yes ; then
| +	AC_DEFINE(MPLS)
| +	AC_DEFINE(MPLS_VPN)
| +#	all_protocols="$all_protocols",ldp
| +fi
| +

Why is it called MPLS, when we call all other configuration switches CONFIG_something?

| diff --git a/lib/addrs.c b/lib/addrs.c
| new file mode 100644
| index 0000000..6553bba
| --- /dev/null
| +++ b/lib/addrs.c
| @@ -0,0 +1,293 @@
| +/*
| + *	BIRD Library -- Address Manipulation Functions
| + *
| + *	(c) 2011 Alexander V. Chernikov <melifaro at ipfw.ru>
| + *
| + *	Can be freely distributed and used under the terms of the GNU GPL.
| + */
| +
| +#include <stdio.h>
| +#include <stdlib.h>
| +
| +#include <sys/types.h>
| +#include <sys/socket.h>
| +#include <netinet/in.h>
| +#include <arpa/inet.h>

<arpa/inet.h> definitely belongs to sysdep.

| +#define PBUFS	4
| +#define PSIZE	50

This deserves a comment.

| +/**
| + * fn_print - prints a FIB node
| + * @n: pointer to fib_node structure
| + *
| + * This function prints fib node address to static buffer and
| + * returns it to the caller. Up to PBUFS(4) different buffers are
| + * available.
| + */

Recycling of a fixed number of static buffers is a rather dirty trick,
which will likely lead to hard-to-catch bugs. (And break multithreading,
if we ever use it in BIRD.)

Better let the caller supply an appropriate buffer. The number of direct
callers is going to be low anyway, since almost everybody will use bprintf().

| --- /dev/null
| +++ b/lib/addrs.h
[...]
| +typedef uint64_t u64;

Huh? It should be already defined in sysdep/config.h.

| +#ifndef IPV6
| +typedef vpn4_addr vpn_addr;
| +#else
| +typedef vpn6_addr vpn_addr;
| +#endif

Calling this beast "vpn" without further clarification is going to be
confusing -- people (even those experienced in networking) usually do not
associate "vpn" with "MPLS" implicitly. Better choose a more descriptive name.

| diff --git a/nest/route.h b/nest/route.h
| index 641b924..c3d5e2b 100644
| --- a/nest/route.h
| +++ b/nest/route.h
| @@ -36,9 +36,10 @@ struct fib_node {
|    struct fib_iterator *readers;		/* List of readers of this node */
|    byte pxlen;
|    byte flags;				/* User-defined */
| -  byte x0, x1;				/* User-defined */
| +  byte addr_type;			/* User-defined */
| +  byte addr_off;			/* address data offset */

Please use consistent capitalization (everywhere).

| +  unsigned int addr_type;		/* Type of adresses stored in fib (IPv46, VPNv46, MPLS, etc..)*/
| +  unsigned int addr_size;		/* size of address specified in entry */
| +  unsigned int addr_off;		/* value of data offset to be set in fib_node */

I did not understand anything from the latter two comments.

What is specified in an entry? An address? Or the size itself?

Offset of which data?

| @@ -116,6 +129,7 @@ void fit_put(struct fib_iterator *, struct fib_node *);
|  struct rtable_config {
|    node n;
|    char *name;
| +  int rtype;				/* Type of the table (IPv46, VPNv46, MPLS, etc..)*/

Why is it called "rtype" here and "addr_type" elsewhere?

| +/* Types of routing tables/entries */
| +#define RT_IPV4		1
| +#define RT_IPV6		2
| +#define RT_VPNV4	3
| +#define RT_VPNV6	4
| +#define RT_MPLS		5
| +
| +#define RT_MAX		6

Deserves comments.

| +/* Same tables using bit positions. Used for appropriate table linking on protocol init */
| +#define RT_IPV4_BIT	0x001
| +#define RT_IPV6_BIT	0x002
| +#define RT_VPNV4_BIT	0x004
| +#define RT_VPNV6_BIT	0x008
| +#define RT_MPLS_BIT	0x010

Better use

	#define RT_IPV4_BIT (1U << RT_IPV4)

| diff --git a/nest/rt-fib.c b/nest/rt-fib.c
| index 510aa76..bf2b562 100644
| --- a/nest/rt-fib.c
| +++ b/nest/rt-fib.c
| @@ -73,9 +73,22 @@ fib_ht_free(struct fib_node **h)
|  }
|  
|  static inline unsigned
| -fib_hash(struct fib *f, ip_addr *a)
| +fib_hash(struct fib *f, void *a)
|  {
| -  return ipa_hash(*a) >> f->hash_shift;
| +  u32 *v, x;
| +  int i;
| +
| +  if (f->hash_f)
| +    return f->hash_f(a);
| +
| +  if (f->addr_type == RT_IP)
| +    return ipa_hash(*((ip_addr *)a)) >> f->hash_shift;
| +
| +  x = 0;
| +  for (i = 0, v = (u32 *)a; i < f->addr_size / 4; i++, v++)
| +    x = x ^ *v;
| +
| +  return ((x ^ (x >> 16) ^ (x >> 8)) & 0xffff) >> f->hash_shift;
|  }

Is it guaranteed that addr_size will be always divisible by 4? If it is
a requirement, document it.

If I understand the code properly, fib_hash() is a pretty hot place, but you
do several pointless tests there -- e.g., f->addr_type is constant for the whole FIB,
but you test it during each calculation of a hash value.

Instead of that, set f->hash_f during initialization of the FIB according to
the type of addresses used.

					Martin