And just an idea. I've attached the patch as an example, although it
should be functional too. I mentioned that functions krt_learn_async()
and krt_learn_scan() are very similar and I thought that it may be
useful to join them into a single function. If I did not make a
mistake, all code pathes should be almost the same, with a couple of
minor exceptions:
1) old_best is set in the beginning, but was earlier only for async version
2) g = NULL after rte_free(g) in one of the branches, but it does not
affect anything and may be dropped
On Wed, Sep 21, 2022 at 6:00 PM Maria Matejka via Bird-users
<[email protected]> wrote:
>
> Hello!
>
> Thank you for finding and fixing. Will check and include.
>
> Maria
>
> On 9/21/22 17:15, Alexander Zubkov via Bird-users wrote:
> > I made a trivial patch for the case.
> >
> > On Tue, Sep 20, 2022 at 6:23 PM Alexander Zubkov <[email protected]> wrote:
> >>
> >> Hi,
> >>
> >> Bird from master branch ignores the default preference set in channel
> >> for a kernel protocol, like that:
> >>
> >> protocol kernel {
> >> learn yes;
> >> ipv4 {
> >> preference 200;
> >> import all;
> >> export none;
> >> };
> >> }
> >>
> >> Version 2.0.10 seems ok. I suppose the change was introduced with this
> >> commit:
> >>
> >> https://gitlab.nic.cz/labs/bird/-/commit/eb937358c087eaeb6f209660cc7ecfe6d6eff739
> >>
> >> It adds setting of the preference to the krt_learn_async() function.
> >> But as I understand when route is learned with netlink hook it is
> >> processed like this:
> >> nl_parse_route -> krt_got_route_async -> krt_learn_async
> >> But when it is learned from scan, it is processed like this:
> >> nl_parse_route -> krt_got_route -> krt_learn_scan
> >>
> >> And setting preference was not added to krt_learn_scan() function.
> >>
> >> I also saw that in my test - when I added a new route to the kernel
> >> while bird was running, it appeared in bird with correct preference.
> >> But some time after, the scan updated the route and it was replaced
> >> with the default preference.
diff --git a/sysdep/unix/krt.c b/sysdep/unix/krt.c
index c4a3a4a8..e61efdfe 100644
--- a/sysdep/unix/krt.c
+++ b/sysdep/unix/krt.c
@@ -318,45 +318,6 @@ krt_learn_announce_delete(struct krt_proto *p, net *n)
rte_update(&p->p, n->n.addr, NULL);
}
-/* Called when alien route is discovered during scan */
-static void
-krt_learn_scan(struct krt_proto *p, rte *e)
-{
- net *n0 = e->net;
- net *n = net_get(p->krt_table, n0->n.addr);
- rte *m, **mm;
-
- e->attrs = rta_lookup(e->attrs);
-
- for(mm=&n->routes; m = *mm; mm=&m->next)
- if (krt_same_key(m, e))
- break;
- if (m)
- {
- if (krt_uptodate(m, e))
- {
- krt_trace_in_rl(&rl_alien, p, e, "[alien] seen");
- rte_free(e);
- m->pflags |= KRT_REF_SEEN;
- }
- else
- {
- krt_trace_in(p, e, "[alien] updated");
- *mm = m->next;
- rte_free(m);
- m = NULL;
- }
- }
- else
- krt_trace_in(p, e, "[alien] created");
- if (!m)
- {
- e->next = n->routes;
- n->routes = e;
- e->pflags |= KRT_REF_SEEN;
- }
-}
-
static void
krt_learn_prune(struct krt_proto *p)
{
@@ -373,7 +334,7 @@ again:
/*
* Note that old_best may be NULL even if there was an old best route in
- * the previous step, because it might be replaced in krt_learn_scan().
+ * the previous step, because it might be replaced in krt_learn_route().
* But in that case there is a new valid best route.
*/
@@ -431,13 +392,17 @@ again:
p->reload = 0;
}
+/* sync = 0 : Called when alien route is discovered during scan */
static void
-krt_learn_async(struct krt_proto *p, rte *e, int new)
+krt_learn_route(struct krt_proto *p, rte *e, int async, int new)
{
net *n0 = e->net;
net *n = net_get(p->krt_table, n0->n.addr);
rte *g, **gg, *best, **bestp, *old_best;
+ if (async)
+ new = 1;
+
ASSERT(!e->attrs->cached);
e->attrs->pref = p->p.main_channel->preference;
@@ -453,19 +418,35 @@ krt_learn_async(struct krt_proto *p, rte *e, int new)
{
if (krt_uptodate(g, e))
{
- krt_trace_in(p, e, "[alien async] same");
+ if (async)
+ krt_trace_in(p, e, "[alien async] same");
+ else
+ krt_trace_in_rl(&rl_alien, p, e, "[alien] seen");
rte_free(e);
+ if (!async)
+ g->pflags |= KRT_REF_SEEN;
return;
}
- krt_trace_in(p, e, "[alien async] updated");
+ if (async)
+ krt_trace_in(p, e, "[alien async] updated");
+ else
+ krt_trace_in(p, e, "[alien] updated");
*gg = g->next;
rte_free(g);
+ g = NULL;
}
else
- krt_trace_in(p, e, "[alien async] created");
+ {
+ if (async)
+ krt_trace_in(p, e, "[alien async] created");
+ else
+ krt_trace_in(p, e, "[alien] created");
+ }
e->next = n->routes;
n->routes = e;
+ if (!async)
+ e->pflags |= KRT_REF_SEEN;
}
else if (!g)
{
@@ -480,6 +461,10 @@ krt_learn_async(struct krt_proto *p, rte *e, int new)
rte_free(e);
rte_free(g);
}
+
+ if (!async)
+ return;
+
best = n->routes;
bestp = &n->routes;
for(gg=&n->routes; g=*gg; gg=&g->next)
@@ -503,7 +488,7 @@ krt_learn_async(struct krt_proto *p, rte *e, int new)
if (best != old_best)
{
- DBG("krt_learn_async: distributing change\n");
+ DBG("krt_learn_route: distributing change\n");
if (best)
krt_learn_announce_update(p, best);
else
@@ -641,7 +626,7 @@ krt_got_route(struct krt_proto *p, rte *e, s8 src)
case KRT_SRC_ALIEN:
if (KRT_CF->learn)
- krt_learn_scan(p, e);
+ krt_learn_route(p, e, 0, 1);
else
{
krt_trace_in_rl(&rl_alien, p, e, "[alien] ignored");
@@ -777,7 +762,7 @@ krt_got_route_async(struct krt_proto *p, rte *e, int new, s8 src)
case KRT_SRC_ALIEN:
if (KRT_CF->learn)
{
- krt_learn_async(p, e, new);
+ krt_learn_route(p, e, 1, new);
return;
}
#endif
