On Sun, May 21, 2017 at 11:01:34PM +0200, Dean Luga wrote: > From: dean <dlug...@gmail.com> > > This patch adds a new network of type NET_SADR_IP6, including new > structures, constants, and switch cases for the new network type. > Some existing functions are duplicates to handle the new network > type, and netlink can now handle SADR routes. > > The net_route_sadr_ip6 function is a bit of a hack. > Routing in SADR is supposed to match the destination address first, > then out of the entries with that dst, it should choose the most > specific matching src address.
Well, net_route() in integrated branch is a bit of a hack generally.
It was carried over from plain-IP branch, it needs some redesign to
better handle net types different than NET_IP.
> For the destination-only part, I use a net_addr_ip6 as the
> destination prefix. Its type is changed to NET_SADR_IP6 to satisfy
> the assertion in fib_find. The fib_find function checks the length
> of the prefix to distinguish between destination-only or full SADR
> searches.
> ---
> lib/net.c | 34 ++++++++++++++++++++++++++
> lib/net.h | 66
> +++++++++++++++++++++++++++++++++++++++++++++++---
> nest/rt-fib.c | 16 ++++++++++++
> nest/rt-table.c | 35 +++++++++++++++++++++++++-
> sysdep/linux/netlink.c | 26 ++++++++++++++++++--
> sysdep/unix/krt.c | 4 ++-
> 6 files changed, 174 insertions(+), 7 deletions(-)
>
> int
> +net_compare_sadr_ip6(const net_addr_sadr_ip6 *a, const net_addr_sadr_ip6 *b)
> +{
> + int dst_cmp = ip6_compare(a->dst_prefix, b->dst_prefix) ?:
> uint_cmp(a->dst_pxlen, b->dst_pxlen);
> + if (dst_cmp)
> + return dst_cmp;
> + else
> + return ip6_compare(a->src_prefix, b->src_prefix) ?:
> uint_cmp(a->src_pxlen, b->src_pxlen);
> +}
> +
> +int
> net_compare(const net_addr *a, const net_addr *b)
> {
> if (a->type != b->type)
> @@ -158,6 +177,8 @@ net_compare(const net_addr *a, const net_addr *b)
> return net_compare_flow6((const net_addr_flow6 *) a, (const
> net_addr_flow6 *) b);
> case NET_MPLS:
> return net_compare_mpls((const net_addr_mpls *) a, (const net_addr_mpls
> *) b);
> + case NET_SADR_IP6:
> + return net_compare_sadr_ip6((const net_addr_sadr_ip6 *) a, (const
> net_addr_sadr_ip6 *) b);
> }
> return 0;
> }
> @@ -178,6 +199,7 @@ net_hash(const net_addr *n)
> case NET_FLOW4: return NET_HASH(n, flow4);
> case NET_FLOW6: return NET_HASH(n, flow6);
> case NET_MPLS: return NET_HASH(n, mpls);
> + case NET_SADR_IP6: return NET_HASH(n, ip6);
> default: bug("invalid type");
> }
> }
This should be NET_HASH(n, sadr_ip6) even if it is the same function.
> diff --git a/lib/net.h b/lib/net.h
> index 332f4c9..22eee60 100644
> --- a/lib/net.h
> +++ b/lib/net.h
> @@ -12,7 +12,6 @@
>
> #include "lib/ip.h"
>
> -
> #define NET_IP4 1
> #define NET_IP6 2
> #define NET_VPN4 3
> @@ -22,7 +21,8 @@
> #define NET_FLOW4 7
> #define NET_FLOW6 8
> #define NET_MPLS 9
> -#define NET_MAX 10
> +#define NET_SADR_IP6 10
> +#define NET_MAX 11
I would prefer that NET_SADR_IP6 would be before NET_MPLS, as it is still
IP-family net type,
>
> #define NB_IP4 (1 << NET_IP4)
> #define NB_IP6 (1 << NET_IP6)
> @@ -33,8 +33,9 @@
> #define NB_FLOW4 (1 << NET_FLOW4)
> #define NB_FLOW6 (1 << NET_FLOW6)
> #define NB_MPLS (1 << NET_MPLS)
> +#define NB_SADR (1 << NET_SADR_IP6)
>
> -#define NB_IP (NB_IP4 | NB_IP6)
> +#define NB_IP (NB_IP4 | NB_IP6 | NB_SADR)
No, NB_IP should stay as is. It is used by protocols to specify what
network types they support and they do not get SADR support
automatically.
> #define NB_VPN (NB_VPN4 | NB_VPN6)
> #define NB_FLOW (NB_FLOW4 | NB_FLOW6)
> #define NB_DEST (NB_IP | NB_VPN | NB_MPLS)
But you should add it to NB_DEST, so destinations are allowed.
> @@ -120,6 +121,15 @@ typedef struct net_addr_mpls {
> u32 label;
> } net_addr_mpls;
>
> +typedef struct net_addr_sadr_ip6 {
> + u8 type;
> + u8 dst_pxlen;
> + u16 length;
> + ip6_addr dst_prefix;
> + u32 src_pxlen; // if u8, NET_ADDR_SADR_IP6 would leave non-zero padding
> + ip6_addr src_prefix;
> +} net_addr_sadr_ip6;
> +
> typedef union net_addr_union {
> net_addr n;
> net_addr_ip4 ip4;
> @@ -131,6 +141,7 @@ typedef union net_addr_union {
> net_addr_flow4 flow4;
> net_addr_flow6 flow6;
> net_addr_mpls mpls;
> + net_addr_sadr_ip6 sadr_ip6;
> } net_addr_union;
>
>
> @@ -169,6 +180,9 @@ extern const u16 net_max_text_length[];
> #define NET_ADDR_MPLS(label) \
> ((net_addr_mpls) { NET_MPLS, 20, sizeof(net_addr_mpls), label })
>
> static inline ip4_addr net4_prefix(const net_addr *a)
> { return ((net_addr_ip4 *) a)->prefix; }
> @@ -243,6 +263,12 @@ static inline ip4_addr net4_prefix(const net_addr *a)
> static inline ip6_addr net6_prefix(const net_addr *a)
> { return ((net_addr_ip6 *) a)->prefix; }
>
> +static inline ip6_addr net6_sadr_dst_prefix(const net_addr *a)
> +{ return ((net_addr_sadr_ip6 *) a)->dst_prefix; }
> +
> +static inline ip6_addr net6_sadr_src_prefix(const net_addr *a)
> +{ return ((net_addr_sadr_ip6 *) a)->src_prefix; }
> +
These accessors seems pointless. dst_prefix is the same as net6_prefix()
(assuming net_addr_sadr_ip6 extends net_addr_ip6 like other IPv6-based
nettypes) and src_prefix is not used outside specialized code, which would
have net typed as net_addr_sadr_ip6 and could access src_prefix directly.
> static inline ip_addr net_prefix(const net_addr *a)
> {
> switch (a->type)
> @@ -259,6 +285,9 @@ static inline ip_addr net_prefix(const net_addr *a)
> case NET_FLOW6:
> return ipa_from_ip6(net6_prefix(a));
>
> + case NET_SADR_IP6:
> + return ipa_from_ip6(net6_sadr_dst_prefix(a));
> +
You could handle that by net6_prefix() together with other IPv6-based nettypes.
> case NET_MPLS:
> default:
> return IPA_NONE;
> @@ -279,6 +308,12 @@ static inline uint net4_pxlen(const net_addr *a)
> static inline uint net6_pxlen(const net_addr *a)
> { return a->pxlen; }
>
> +static inline uint net6_sadr_dst_pxlen(const net_addr *a)
> +{ return ((net_addr_sadr_ip6 *) a)->dst_pxlen; }
> +
> +static inline uint net6_sadr_src_pxlen(const net_addr *a)
> +{ return ((net_addr_sadr_ip6 *) a)->src_pxlen; }
> +
Like net6_sadr_*_prefix
> @@ -327,6 +362,13 @@ static inline int net_equal_flow6(const net_addr_flow6
> *a, const net_addr_flow6
> static inline int net_equal_mpls(const net_addr_mpls *a, const net_addr_mpls
> *b)
> { return !memcmp(a, b, sizeof(net_addr_mpls)); }
>
> +typedef net_addr_ip6 net_addr_sadr_dst;
> +static inline int net_equal_sadr_dst(const net_addr_ip6 *a, const
> net_addr_ip6 *b)
> +{ return net_equal_ip6(a, b); }
> +
> +static inline int net_equal_sadr_ip6(const net_addr_sadr_ip6 *a, const
> net_addr_sadr_ip6 *b)
> +{ return !memcmp(a, b, sizeof(net_addr_sadr_ip6)); }
> +
>
> @@ -455,6 +502,12 @@ static inline u32 net_hash_flow6(const net_addr_flow6 *n)
> static inline u32 net_hash_mpls(const net_addr_mpls *n)
> { return n->label; }
>
> +static inline u32 net_hash_sadr_dst(const net_addr_sadr_dst* n)
> +{ return net_hash_ip6(n); }
> +
> +static inline u32 net_hash_sadr_ip6(const net_addr_sadr_ip6 *n)
> +{ return ip6_hash(n->dst_prefix) ^ ((u32) n->dst_pxlen << 26); }
> +
Perhaps unnecessary, see my comment to fib_find() below
> @@ -231,6 +232,20 @@ fib_find(struct fib *f, const net_addr *a)
> case NET_ROA6: return FIB_FIND(f, a, roa6);
> case NET_FLOW4: return FIB_FIND(f, a, flow4);
> case NET_FLOW6: return FIB_FIND(f, a, flow6);
> + case NET_SADR_IP6:
> + {
> + if (a->length != sizeof(net_addr_sadr_ip6))
> + {
> + // dst only search
> + net_addr_ip6 a0;
> + net_copy((net_addr *)&a0, a);
> + a0.length = sizeof(net_addr_sadr_ip6);
> +
> + return FIB_FIND(f, &a0, sadr_dst);
> + }
> + else
> + return FIB_FIND(f, a, sadr_ip6);
> + }
No. Generic fib_find() should implement exact-match search with matching
net_addr type. If you need/want partial match, it should be done as a
separate function. BTW, where do you need that?
Perhaps we should add some generic function partial-match IP-based search
for network types that support it (e.g., ROA, SADR). But it would need
a different interface, so caller could enumerate all valid matches.
> +static inline void *
> +net_route_sadr_ip6(rtable *t, net_addr_sadr_ip6 *n)
> +{
> + net *r;
> +
> + net_addr_ip6 dst = NET_ADDR_IP6(n->dst_prefix, n->dst_pxlen);
> + dst.type = NET_SADR_IP6;
> +
> + // search for matching dst
> + while (r = net_route_ip6(t, &dst))
> + {
> + // found a matching dst, start search for entries that match both
> prefixes
> + net_addr_sadr_ip6 full_addr =
> + NET_ADDR_SADR_IP6(dst.prefix, dst.pxlen, n->src_prefix, n->src_pxlen);
> +
> + while (r = net_find_valid(t, (net_addr *) &full_addr), (!r) &&
> (full_addr.src_pxlen > 0))
> + {
> + full_addr.src_pxlen--;
> + ip6_clrbit(&full_addr.src_prefix, full_addr.src_pxlen);
> + }
> +
> + if (r)
> + return r;
> + dst.pxlen--;
You should set dst.pxlen based on r found by net_route_ip6(),
to avoid repeated iteration over the same prefix lengths that
did not match in the inner cycle.
Also be sure that pxlen is not already zero.
> @@ -1747,6 +1779,7 @@ rt_preconfig(struct config *c)
>
> rt_new_table(cf_get_symbol("master4"), NET_IP4);
> rt_new_table(cf_get_symbol("master6"), NET_IP6);
> + rt_new_table(cf_get_symbol("master_sadr6"), NET_SADR_IP6);
> }
No, we have only IPv4 and IPv6 master table defined by default.
Other tables are defined in config file if necessary.
> @@ -1980,7 +2013,7 @@ rt_new_table(struct symbol *s, uint addr_type)
> /* Hack that allows to 'redefine' the master table */
> if ((s->class == SYM_TABLE) &&
> (s->def == new_config->def_tables[addr_type]) &&
> - ((addr_type == NET_IP4) || (addr_type == NET_IP6)))
> + ((addr_type == NET_IP4) || (addr_type == NET_IP6) || (addr_type ==
> NET_SADR_IP6)))
> return s->def;
ditto
> struct rtable_config *c = cfg_allocz(sizeof(struct rtable_config));
> diff --git a/sysdep/linux/netlink.c b/sysdep/linux/netlink.c
I will check KRT/Netlink code later.
--
Elen sila lumenn' omentielvo
Ondrej 'Santiago' Zajicek (email: santi...@crfreenet.org)
OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net)
"To err is human -- to blame it on a computer is even more so."
signature.asc
Description: Digital signature
